fix(storage): upgrade a caller-owned deferred transaction before assertion preservation reads#3101
Conversation
…rtion preservation reads Problem: #3051 fixed upsert_assertion's TOCTOU race for the common case by wrapping the preserve/read/write decision in BEGIN IMMEDIATE when the connection is idle. But _immediate_user_write_transaction's fallback for an already-open transaction (`if conn.in_transaction: yield; return`) never upgrades that transaction to hold the write lock. A caller-owned deferred (plain BEGIN) transaction has not yet reserved the SQLite writer slot, so the terminal-status preservation read taken inside it can still race a concurrent operator judgment landing between that read and this connection's eventual write -- reintroducing the exact silent-revert bug polylogue-41ow tracks, for any caller that reuses a connection across a transaction it opened itself rather than a fresh connection per call. Audited every production caller of upsert_assertion/judge_assertion_candidate (annotations/write.py, security/lifecycle.py, security/secret_scan.py, scenarios/corpus.py, storage/repair.py, storage/raw_reconciler.py, and user_write.py's own batch helpers): none currently reach these functions with a caller-owned deferred transaction open, so this exact gap is not yet exploitable in production -- but it is latent, one refactor away, the same character as the vwia writer-twin bug fixed earlier this sweep. Solution: close it defensively. When the connection already has an open transaction, open a SAVEPOINT and force a zero-row `UPDATE assertions SET updated_at_ms = updated_at_ms WHERE 0` before yielding -- this forces SQLite to acquire the RESERVED lock immediately, before any preserve/read decision runs, without committing or rolling back state the caller owns. If the caller's transaction already holds the write lock (the common in-repo case: a batch of upsert_mark/upsert_blackboard_note/upsert_saved_view/upsert_assertion calls sharing one BEGIN IMMEDIATE opened by the first call), the forced write is a no-op. Also added the canonical busy_timeout PRAGMA to the fresh-transaction branch. Deliberately did not port the auto-commit-on-fresh-transaction behavior from an available external delivery patch (feature/assertions/judgment-transaction): tracing scenarios/corpus.py's intentional multi-call batch (five upserts sharing one transaction, one final caller commit) confirmed that change would silently split one atomic batch into five separate commits. Verification: - New regression: test_cross_connection_replay_inside_caller_owned_deferred_transaction_cannot_resurrect_operator_accept (tests/unit/storage/test_archive_tiers_assertions.py) -- two real SQLite connections; the detector opens a plain BEGIN (not IMMEDIATE) before calling upsert_assertion, then a concurrent operator judgment attempts to land between the detector's SELECT and its write. - Anti-vacuity: reverted the source fix via git stash, keeping the new test -- it fails (operator resurrects the candidate within 0.15s); passes after restoring the fix. - devtools test tests/unit/storage/test_archive_tiers_assertions.py tests/unit/storage/test_archive_tiers_assertion_write_through.py tests/unit/storage/test_comparative_judgment_assertions.py -> 61 passed - devtools test across every other upsert_assertion/judge_assertion_candidate write-path suite (annotations, security, cli judge/note/candidates, mcp judgment tools, storage user_audit/user_overlay/blackboard/public_claims, scenarios/corpus demo seeding) -> 128 passed, no batching regression. - devtools verify --quick -> exit 0. Ref polylogue-41ow Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…and (#3138) ## Summary Ports the remaining additive components of the parked ann-04 delivery (`feature/assertions/judgment-transaction`) that were not already independently absorbed by prior lane-g work — bounded evidence previews shared by CLI+API, a non-destructive judgment-queue health surface, and actor-scoped terminal-capture idempotency — and consolidates candidate assertion review from the `mark candidates` subcommand group into the existing root `judge` command. ## Problem polylogue-2o3d tracked ~1700 lines of a stale (base `536a53efa`, ~20 commits behind at rebase time) delivery branch: bounded evidence previews for CLI/MCP, queue health in judge/status, mark-candidates dedup, actor-scoped capture idempotency, and an operator canary route. A prior lane-g investigation (2026-07-18) determined most of the delivery's mechanism-adjacent diff was either obsolete (MCP-surface hunks, retired by the six-tool cutover) or superseded (the writer-slot TOCTOU fix, separately landed as polylogue-41ow / PR #3101) — leaving this scoped, real feature surface to reconcile and land as one coherent PR. **Note on the branch's own commit message** (`443f236f8`): it describes the delivery's "core fix" as the SAVEPOINT+zero-row-write transaction mechanism. That mechanism is **not** in this PR — the rebase auto-merged it alongside master's already-independently-implemented `_immediate_user_write_transaction` (41ow), producing two redundant transaction layers. This PR's first fixup commit removes the delivery's copy entirely; `user_write.py` is now byte-identical to master except for one genuinely new test (`test_upsert_assertion_owned_transaction_rolls_ back_on_write_failure`) that had no prior coverage. The actual content of this PR is the additive feature surface below. ## Solution - **`judge` command consolidation** (`cli/commands/judge.py`, `cli/query_verbs.py`): the `mark candidates list/review/accept/reject/ defer/supersede` subcommand group is removed from `query_verbs.py`'s `mark` verb (239 lines, mostly deletions — confirmed relocation, not drop, via line-by-line diff against both old and new call surfaces) and its functionality is folded into the existing root `judge` command, which gains `--review`, `--status`, `--defer`, `--supersede`, `--target-ref`, `--candidate-status`, `--until`, `--limit`, `--actor-ref`, `--replacement-kind`/`--body`, plus an interactive accept/reject/defer/edit-and-accept/skip prompt flow. `polylogue judge --help` verified live. - **Bounded evidence previews** (`api/archive.py`, `surfaces/payloads.py`): `list_assertion_candidate_reviews` now resolves up to 5 evidence refs per candidate via the existing `resolve_ref` facade, producing `AssertionEvidencePreviewPayload` rows (state: resolved/missing/unsupported/pending/error) with per-ref failure isolation — one resolver exception degrades only that preview, not the whole review (`test_one_evidence_resolution_failure_degrades_only_that_ preview`). - **Queue health** (`api/archive.py`, `daemon/status.py`, `cli/commands/status.py`): `assertion_candidate_queue_health()` projects pending/status/kind/source counts, retention, and producer/scheduler telemetry from `user.db` + `ops.db` into a `healthy-empty` / `empty-unverified` / `pending` / `stale-pending` / `producer-stalled` / `unavailable` state, surfaced via `judge --status`, `daemon status` JSON, and `polylogue status`. - **Actor-scoped capture idempotency** (`api/archive.py`, `cli/commands/note.py`): `--idempotency-key` on `polylogue note` derives a deterministic assertion id from `author_ref + key`; a replay with identical content returns the original row, a replay with different content raises a conflict — protected by a `BEGIN IMMEDIATE` reservation around the lookup-then-write so two racing replays can't both observe absence. - **Rebase fixup** (separate commit): dropped the superseded transaction mechanism and its redundant test (see note above); fixed 5 mypy errors and one degrade-loudly finding the rebase surfaced; fixed a doc string (`--list|--accept|...`) that the doc-commands verifier corrupted via markdown pipe-escaping. ## Verification - `devtools verify --quick` — green (lint + mypy --strict + render + doc-commands + degrade-loudly, all clean) - Full delivery-affected sweep (api/cli/product/storage/daemon status, 794 tests) — 793 passed, 1 pre-existing failure (`test_should_use_plain_contract[False-1-True-True]`) confirmed to reproduce identically on clean `origin/master` via an isolated scratch worktree — unrelated, not touched by this PR - `polylogue judge --help` — live CLI wiring confirmed working Ref polylogue-2o3d Ref polylogue-41ow --------- Co-authored-by: GPT-Pro external agent <noreply@openai.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…lution deliveries Ref polylogue-41ow, polylogue-fd2s, polylogue-9gh1, polylogue-2o3d Problem: two files staged at /realm/tmp/gpt-pro-intake-0719/ (judgment-*.md/.diff, config-*.md/.diff) were flagged as fresh wave-2 deliveries needing triage, targeting snapshot 536a53e (#3046). Master has since moved ~180 commits including 33 merges. Solution: verified pairing first (judgment-* files cross-reference polylogue-41ow/37t.12/mrxt and match mission ann-04; config-* files cross-reference polylogue-fd2s/9gh1 and match mission misc-01). SHA-256 of every file was then found byte-identical to the already-recorded results/ann-04/r01 and results/misc-01/r01 packages -- these are not new deliveries, they are exact re-submissions of packages already triaged 2026-07-18. Re-verified git apply --check against both the stated snapshot (passes cleanly for both) and current origin/master (fails on every production file for both -- confirms drift). Cross-referenced current source against each package's claims: - ann-04 (judgment-transaction): bd polylogue-41ow (closed, PR #3101+#3110) independently fixed the writer-slot TOCTOU race; bd polylogue-2o3d (closed, PR #3138) independently ported the delivery's entire additive scope (evidence previews, queue health, judge consolidation, capture idempotency) after its own line-by-line audit. Supersession is now total, not partial as r01 judged it before those PRs landed. Recorded as r02 (state=superseded). - misc-01 (config-resolution-closure): PR #3079 (merged 2026-07-18, explicitly sourced from an earlier revision of this same packet family) already shipped ResolvedRuntimeConfig/single-resolver/archive_root-fix. Live-checked polylogue/config.py:1514 -- deep-merge for nested health tables (polylogue-cxlk) is already landed. Genuine residual found: uu8r-scope env-bypass migrations (daemon/backup.py, pipeline/services/archive_ingest.py, and ~5-6 more files this delivery's own bypass-caller table names) are still unmigrated on current master, but must be re-derived against the already-landed config API rather than applying this patch's hunks (no shared ancestor). Recorded as r02 (state=superseded, with residual noted for polylogue-uu8r). Verification: sha256sum on all 8 delivered files cross-checked against existing r01 records (identical); git apply --check run against both 536a53e (clean) and origin/master e963e87 (fails, 20-73 files per patch); bd show on polylogue-41ow/fd2s/9gh1/cxlk/uu8r/2o3d to confirm current close/open state and what each merged PR actually covers; grep -rl ResolvedRuntimeConfig and read of _merge_toml/_deep_merge_table in polylogue/config.py to confirm the architecture is live, not just claimed. Triage notes appended to polylogue-41ow and polylogue-fd2s via bd update --append-notes. No patch was applied to master; no bead was closed or reopened by this change. Co-Authored-By: Claude <noreply@anthropic.com>
… deliveries Ref polylogue-41ow, polylogue-fd2s, polylogue-9gh1, polylogue-2o3d Problem: two files staged at /realm/tmp/gpt-pro-intake-0719/ (judgment-*.md/.diff, config-*.md/.diff) were flagged as fresh wave-2 deliveries needing triage, targeting snapshot 536a53e (#3046). Master has since moved ~180 commits including 33 merges. Solution: verified pairing first (judgment-* files cross-reference polylogue-41ow/37t.12/mrxt and match mission ann-04; config-* files cross-reference polylogue-fd2s/9gh1 and match mission misc-01). SHA-256 of every file was then found byte-identical to the already-recorded results/ann-04/r01 and results/misc-01/r01 packages -- these are not new deliveries, they are exact re-submissions of packages already triaged 2026-07-18. Per results/README.md custody policy ("Duplicated browser downloads with identical SHA-256 values are deliberately not copied twice", precedent: lin-01/r01), this revision does NOT mint a second package-revision identity (r02) or duplicate the raw/extracted artifact bytes. Instead the 2026-07-20 reassessment is recorded as a dated `reassessments` entry inside each existing r01 receipt.json, alongside an updated top-level `state` where the evidence changed: - ann-04/r01: state needs_rebase_review -> superseded. bd polylogue-41ow (closed, PR #3101+#3110) independently fixed the writer-slot TOCTOU race; bd polylogue-2o3d (closed, PR #3138) independently ported the delivery's entire additive scope (evidence previews, queue health, judge consolidation, capture idempotency) after its own line-by-line audit found the MCP-surface hunks obsolete (six-tool cutover) and the delivery's transaction mechanism redundant with #3101. Supersession is now total, not partial as originally judged before those PRs landed. - misc-01/r01: state stays superseded, rationale now explicit. PR #3079 (merged 2026-07-18, sourced from an earlier revision of this same packet family) already shipped ResolvedRuntimeConfig/single-resolver/ archive_root-fix/deep-merge. There is no viable rebase target for this patch against the current config API -- #3079 shipped a differently shaped ResolvedRuntimeConfig with no common ancestor, so any further work must be re-derived, not revived from this diff. The genuine residual (uu8r-scope env-bypass migrations, e.g. daemon/backup.py, pipeline/services/archive_ingest.py, still unmigrated on master) is tracked durably as polylogue-uu8r directly, not as a pending state on this package. Re-verified git apply --check against both the stated snapshot (passes cleanly for both) and current origin/master, checked twice: once at the original triage base (e963e87) and again after rebasing onto the latest tip (1c0f4ec, three commits later -- #3175 CLI onboarding convergence plus two beads-filing commits, none touching the affected files). Both patches fail identically both times. Verification: sha256sum on all 8 delivered files cross-checked against existing r01 records (identical); git apply --check as above; bd show on polylogue-41ow/fd2s/9gh1/cxlk/uu8r/2o3d to confirm current close/open state and what each merged PR actually covers; grep -rl ResolvedRuntimeConfig and read of _merge_toml/_deep_merge_table in polylogue/config.py to confirm the architecture is live, not just claimed; devtools verify --quick 16/16 green (pre-push hook). Triage notes appended to polylogue-41ow and polylogue-fd2s via bd update --append-notes, including a correction note for the earlier (superseded) r02 framing. No patch was applied to master; no bead was closed or reopened by this change. Co-Authored-By: Claude <noreply@anthropic.com>
…lution deliveries (#3177) ## Summary Re-triages two ChatGPT-Pro handoff packages staged at `/realm/tmp/gpt-pro-intake-0719/` (`judgment-*.md/.diff`, `config-*.md/.diff`), targeting snapshot `536a53efac0cbe4a2473ad379e4db49ef3fce74d` (#3046). Both are byte-identical duplicate downloads of packages already on file as `results/ann-04/r01` and `results/misc-01/r01`. Per the custody policy (`results/README.md`: "Duplicated browser downloads with identical SHA-256 values are deliberately not copied twice", precedent: `lin-01/r01`), this PR does **not** mint a second package-revision identity or duplicate raw/extracted artifact bytes — it records the 2026-07-20 reassessment as a dated `reassessments` entry inside each existing `r01` receipt, with the top-level `state` updated where the evidence changed. No patch is applied to master; no bead is closed or reopened. ## Problem Both files were flagged as fresh wave-2 deliveries needing triage. Master has moved ~180 commits (33+ merges) past the stated snapshot. ## Solution **Pairing verification (step 0):** `judgment-*` files cross-reference polylogue-41ow/37t.12/mrxt and match mission `ann-04-judgment-transaction`; `config-*` files cross-reference polylogue-fd2s/9gh1 and match mission `misc-01-config-closure`. The mapping was correct as staged. **Commission identification (step 1):** both missions already have `r01` result entries on disk. SHA-256 of every one of the 8 delivered files is byte-identical to the corresponding `results/ann-04/r01` and `results/misc-01/r01` artifacts (`diff -q` reports no differences) — these are re-submissions of packages already triaged 2026-07-18, not new deliveries. **Triage (step 2), recorded as `reassessments` entries in the existing `r01` receipts:** - `git apply --check` passes cleanly for both patches against the stated snapshot (integrity confirmed) and fails for both against current `origin/master`, re-checked three times as master advanced during triage (`e963e87f5` → `1c0f4eccd` → `610ce8d9`, none of the intervening commits touch the affected core files). - **ann-04 (judgment-transaction): `r01` state `needs_rebase_review` → `superseded`.** bd `polylogue-41ow` (closed, PR #3101+#3110) independently fixed the writer-slot TOCTOU race; bd `polylogue-2o3d` (closed, PR #3138) independently ported the delivery's entire non-overlapping additive scope (evidence previews, queue health, judge-command consolidation, capture idempotency) after its own line-by-line audit found the MCP-surface hunks obsolete (six-tool cutover) and the delivery's transaction mechanism redundant with #3101. Supersession is now total — every acceptance-matrix row this delivery claims already has an independent, merged, adjudicated master implementation. - **misc-01 (config-resolution-closure): `r01` state stays `superseded`, rationale now explicit.** PR #3079 (merged 2026-07-18, explicitly sourced from an earlier revision of this same GPT-Pro packet family) already shipped `ResolvedRuntimeConfig`, the single resolver, the `archive_root` TOML fix, and deep-merge for nested health tables (live-checked `polylogue/config.py:1514` `_merge_toml`/`_deep_merge_table`). `needs_rebase_review` does not apply here: there is no viable rebase target for this patch against the current config API — #3079 shipped a differently-shaped `ResolvedRuntimeConfig` with no common ancestor, so any further work must be **re-derived against the landed API, not revived** from this diff. The genuine residual (bd `polylogue-uu8r`'s env-bypass migration scope — spot-checked `polylogue/daemon/backup.py:695` and `polylogue/pipeline/services/archive_ingest.py:48`, both still direct `os.environ.get()` reads on master) is tracked durably as `polylogue-uu8r` directly, not as a pending state on this package. Also noted, out of scope for this PR: `polylogue-fd2s`/`polylogue-9gh1` still read `status=open` in `bd` despite #3079's landing — likely bookkeeping lag, flagged in the bead notes for a separate housekeeping pass. A stray `clipboard-current.txt` in the intake directory is unrelated leftover clipboard text for the already-integrated `mandate-01` delivery (same SHA as `results/mandate-01/r01`, PR #3088, merged) — not part of either package, not touched by this PR. ## Verification - `sha256sum` on all 8 delivered files, cross-checked against existing `r01` records: identical. - `git apply --check` against `536a53efac0cbe4a2473ad379e4db49ef3fce74d`: clean for both patches (detached worktree). - `git apply --check` against `origin/master`, checked at `e963e87f5`, `1c0f4eccd`, and `610ce8d9` as master advanced during triage: fails identically every time (20 production files for judgment, 21 for config; some judgment files/tests no longer exist post six-tool MCP cutover). - `bd show polylogue-41ow/fd2s/9gh1/cxlk/uu8r/2o3d --json` to confirm current close/open state and what each merged PR actually covers. - `grep -rl ResolvedRuntimeConfig polylogue/` plus a read of `polylogue/config.py:1514` (`_merge_toml`/`_deep_merge_table`) to confirm the architecture is live on master, not just claimed by a PR title. - `devtools verify --quick`: 16/16 green (pre-push hook, this branch, each push). - Triage notes appended to `polylogue-41ow` and `polylogue-fd2s` (plus a correction note on both retracting the earlier, policy-violating `r02` framing) via `bd update --append-notes` + `bd export`. Ref polylogue-41ow, polylogue-fd2s, polylogue-9gh1, polylogue-2o3d Co-authored-by: Claude <noreply@anthropic.com>
Summary
Closes a latent residual in the assertion write-lock TOCTOU fix (#3051): a caller-owned deferred transaction was never upgraded to hold the SQLite write lock before the terminal-status preservation read.
Problem
#3051fixedupsert_assertion's TOCTOU race for the common case by wrapping the preserve/read/write decision inBEGIN IMMEDIATEwhen the connection is idle (_immediate_user_write_transaction). But its fallback for an already-open transaction —if conn.in_transaction: yield; return— never upgrades that transaction to hold the write lock. A caller-owned deferred (plainBEGIN) transaction has not yet reserved the SQLite writer slot, so the terminal-status preservation read taken inside it can still race a concurrent operator judgment landing between that read and this connection's eventual write — reintroducing the exact silent-revert bugpolylogue-41owtracks, for any caller that reuses a connection across a transaction it opened itself rather than a fresh connection per call.I audited every production caller of
upsert_assertion/judge_assertion_candidate(annotations/write.py,security/lifecycle.py,security/secret_scan.py,scenarios/corpus.py,storage/repair.py,storage/raw_reconciler.py, anduser_write.py's own batch helpers): none currently reach these functions with a caller-owned deferred transaction open, so this exact gap is not yet exploitable in production — but it is latent, one refactor away, the same character as thevwiawriter-twin bug fixed earlier in this hardening sweep (#3099).Solution
Closed it defensively. When the connection already has an open transaction, open a
SAVEPOINTand force a zero-rowUPDATE assertions SET updated_at_ms = updated_at_ms WHERE 0before yielding — this forces SQLite to acquire theRESERVEDlock immediately, before any preserve/read decision runs, without committing or rolling back state the caller owns. If the caller's transaction already holds the write lock (the common in-repo case: a batch ofupsert_mark/upsert_blackboard_note/upsert_saved_view/upsert_assertioncalls sharing oneBEGIN IMMEDIATEopened by the first call in the batch), the forced write is a no-op. Also added the canonicalbusy_timeoutPRAGMA to the fresh-transaction branch.Deliberately did not port the auto-commit-on-fresh-transaction behavior from an available external delivery patch (
feature/assertions/judgment-transaction): tracingscenarios/corpus.py's intentional multi-call batch (five upserts sharing one transaction, one final caller commit) confirmed that change would silently split one atomic batch into five separate commits. Also did not port that delivery's non-overlapping additive scope (evidence previews, queue health, capture idempotency, canary script) — that's feature delivery, tracked separately aspolylogue-2o3d.Verification
test_cross_connection_replay_inside_caller_owned_deferred_transaction_cannot_resurrect_operator_accept(tests/unit/storage/test_archive_tiers_assertions.py) — two real SQLite connections; the detector opens a plainBEGIN(notIMMEDIATE) before callingupsert_assertion, then a concurrent operator judgment attempts to land between the detector's SELECT and its write.git stash, keeping the new test — it fails (operator resurrects the candidate within 0.15s); passes after restoring the fix.devtools test tests/unit/storage/test_archive_tiers_assertions.py tests/unit/storage/test_archive_tiers_assertion_write_through.py tests/unit/storage/test_comparative_judgment_assertions.py→ 61 passeddevtools testacross every otherupsert_assertion/judge_assertion_candidatewrite-path suite (annotations, security, cli judge/note/candidates, mcp judgment tools, storage user_audit/user_overlay/blackboard/public_claims, scenarios/corpus demo seeding) → 128 passed, no batching regression.devtools verify --quick→ exit 0.Ref polylogue-41ow