Fix corpus reds: relink provenance, unstamped tier fixtures, slot reaping, wire-receipt cost - #4591
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65eecb38cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| derived_direction, derived_producer = derive_attachment_provenance( | ||
| owning_message.role, owning_message.provider_message_id | ||
| owning_message.role, owning_message.provider_message_id or message_id | ||
| ) |
There was a problem hiding this comment.
For an AI Studio Drive chunk with an assistant/model role, an attachment, and no chunk id, drive.py produces a position-linked attachment with direction="model_output" and producer_ref=None. _attachment_provenance returns immediately whenever direction is already set, so this new message_id fallback is never reached; _write_attachments consequently raises ValueError and aborts ingestion, while the relink route creates a candidate that violates the database constraint. Fill a missing model-output producer from the stored message ID even when the direction was already derived.
Useful? React with 👍 / 👎.
83446de to
f2b4c9c
Compare
Sinity
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2375cb26cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| conn.execute( | ||
| """UPDATE session_model_usage SET provider_cost_usd = NULL | ||
| WHERE session_id = ? AND model_name <> ?""", | ||
| (session_id, model_name), |
There was a problem hiding this comment.
When a live Claude Code JSONL file first records a $1 assistant turn on model A and later appends a $2 turn on model B, the append route parses only the tail bytes and code_parser sums costUSD from those tail records, so the incoming value is a delta rather than a whole-session total. This update clears model A's $1 and leaves every session-cost query reporting $2 instead of $3; the append path needs to accumulate the provider-reported delta or recompute the complete session total rather than discarding prior model rows.
AGENTS.md reference: AGENTS.md:L44-L45
Useful? React with 👍 / 👎.
| sys.stderr.write(f" waiting for the host pytest slot (pueue task {task_id}, group {PYTEST_GROUP}) ...\n") | ||
| sys.stderr.flush() | ||
| _pueue(["wait", task_id], env=adder) | ||
| status = _pueue(["status", "--json"], env=adder) | ||
| returncode = _task_result(status.stdout, task_id) | ||
| with _reaping(task_id, env=adder, launch_path=launch_path): |
There was a problem hiding this comment.
If SIGINT, SIGTERM, or SIGHUP arrives after pueue add has created the task but before this context installs its handlers—including while parsing the task ID or writing the waiting message—the process terminates under the previous handler and leaves both the task and launch file behind. Because the repository uses one host-wide pytest slot, that orphan can still block every later managed run; block these signals before adding the task and arm/unblock them once its ID is known.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
…ture The browser-capture probe opens the index through the tier-guarded read profile, which refuses an unstamped database. The hand-built fixtures declare the version that profile expects, read from the tier table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
The backfill route opens the index through the tier-guarded read profile, which refuses an empty file. The fixture builds the tier through the production bootstrap instead of touching a bare SQLite database. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
attachment_refs.direction is NOT NULL, and only the main write route derived it from the owning turn. Blob-reference closure relinked attachments straight from the parsed record, so any record parsed before attachments carried direction failed the constraint. The relink path uses the same shared derivation, reading the owning turn from the index when the append payload's own message map does not carry it. The session's exact provider dollar total lives on one model row. A merge-append that switches models left the superseded row holding it, so every sum over the session counted it twice. The direction-refusal test named a user turn, whose direction the derivation supplies; only a role outside the user/model pair leaves the column underivable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
The connection factories assert the declared schema version of a database named for an archive tier. These tests probe the pragmas the factories apply, so their scratch databases carry names that claim no tier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
The insight-status and readiness queries read blocks.tool_outcome, which #4539 made the canonical structural outcome. Their hand-built index fixtures predate it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
The root and /s/ routes serve the typed WebUI. The interpolated reader shell these tests pin answers on the workspace routes, so they read it there. This restores the search-shell DOM evidence #4579 deleted: the shell it tests is still served, just not at the root. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
build_wire_support_receipt runs the synthetic corpus and the production parser over every catalogued package element: 27s per full-catalog call in a quiet process, and 106-110s of call time under -n 2, inside a 120s timeout with no margin. #4412 added two providers and the margin went. Tests that assert about one provider's entry, or that a receipt changed, name the providers they read. Catalog-wide construction stays covered by test_support_receipt_is_deterministic, test_support_receipt_does_not_substitute_a_default_selection, and test_wire_support_receipt_is_canonical_across_catalog_reordering. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
The waiter blocks on a task it queued, and the task outlives it. The slot's parallelism is one, so a task left with no waiter starves every other checkout on the host until someone reads the queue and kills it by hand. The waiter now reaps the task it added when it is signalled or unwound: kill, then remove, then drop the launch file, which carries a resolved environment and must not survive either. The reap is best effort by construction -- it runs while the process is dying and must not replace the cause of death with its own error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz
2375cb2 to
2310390
Compare
…ired tokens (#4598) ## Summary Eight more red classes from the 2026-09-03 corpus run, stacked on #4591. Four are product defects; four are fixtures pinned to shapes the runtime has moved past. ## Problem and solution, per class ### Status readers raised instead of reporting tier skew `#4485`'s open-time tier check reached the readers whose *subject* is a tier's version. The workload probe's payload carries `user_version` and `version_status` fields; metrics is supposed to emit `schema_mismatch:index` as a blocker series; the convergence-debt summary has an `error` field for saying why the view is unavailable. All of them raised instead: ``` SchemaSkewError: index schema skew: expected 85, found 1 ``` Metrics, the workload probe (three sites), the embedding status payload, the schema-inference gate's tier identity read and the debt summary now open without the version assertion and report what they find. The context preamble's scheduler receipt is best-effort by its own contract and already swallowed sqlite errors; a tier this runtime cannot use joins them. ### The relink route never derived attachment provenance `attachment_refs.direction` is `NOT NULL` and only the main write route derived it, so blob-reference closure failed the constraint on any record parsed before attachments carried direction. The relink path uses the same shared derivation, reading the owning turn from the index when the append payload's own message map does not carry it. ### One session cost total on two model rows `sessions.reported_cost_usd` is one exact dollar total, written onto a single `session_model_usage` row. A merge-append that switched models left the superseded row holding the previous total, double-counting it in every sum over the session. ### A read that skipped the controlled reader The delegation work-evidence materializer opened a read-only `ArchiveStore` directly — the one thing the read-surface boundary forbids, because such a read carries no admission, no execution budget and no cancellation. ### Fixtures that open a tier before building it A connection profile checks a tier's declared version at open time, so a test cannot open a tier-named path and initialize it afterwards. The assertion tests build the tier first through the production bootstrap; the mmap-budget test builds every attached tier the same way, since the tier filenames are its subject; the descriptor-binding test names its database for what it is. The embedding-drain stubs declare the version they claim, without which the daemon's probe passes them over and the drain silently reports no work. ### The resilience suite launched another checkout's daemon `which("polylogued")` resolves from PATH, which in a worktree is the main checkout. The tests launched a different tree's daemon against this tree's archive, then waited out their budget with nothing ingested and reported a timeout. The running interpreter's own script directory comes first now. The installed-wheel smoke had the same shape: `uv venv` picked whatever interpreter uv found, and the package refuses to start off free-threaded CPython. ### Rendered ages re-baselined instead of redacted `"relative_time": "70w ago"` became `"71w ago"` on its own, because the value is computed against the host clock over fixture timestamps that never move. This file's own contract routes such a field to `_redact`, "not to update the baseline". ### Surfaces that read a retired token as removed `#4363` retired the Beads issue route, but the `Origin` token stays: deleting it would narrow the durable `source.db` CHECKs that still admit stored rows. Three surfaces each encoded a different guess. Origin has twelve tokens, ten of them public badges — the generated-contract test derives that split from `PUBLIC_ORIGIN_TOKENS`, the teaching table follows the enum, and the namespace policy stays exhaustive over *executable* origins, which the retired one is not. Separately, `exclusive_keys` and `scratch` are retired descriptor keys, as `test_dev_loop_service` already declares. ## Verification `devtools test <file> -n 2`, worktree venv resynced to the current lockfile: | file | before | after | | --- | --- | --- | | `tests/unit/storage/test_durable_change_train.py` | 3 failed | **98 passed** | | `tests/unit/daemon/test_metrics_endpoint.py` | 2 failed | pass | | `tests/unit/maintenance/test_schema_inference_gate.py` | 1 failed | pass | | `tests/unit/storage/test_connection_profile.py` | 1 failed | pass | | `tests/unit/storage/test_archive_tiers_assertions.py` | 3 failed | pass | | `tests/unit/daemon/test_embedding_readiness.py` | 1 failed | pass | | `tests/unit/context/test_preamble.py::…non_git…` | 1 failed | pass | `devtools verify --quick` green at this head. ## Residual risk `validate_schema=False` is now used at seven read sites. Each is a status or identity projection whose caller models skew as data; none of them writes. A first attempt at this branch also relaxed the identity guard so an `ops.db` predating the ledger would converge. That fixed three tests and broke thirty in `test_durable_change_train.py` — bootstrap proceeded where it used to refuse. The relaxation is dropped and those three ops-convergence tests remain red; the reader change that types the absent ledger as `None` is kept and was measured separately as harmless (98 passed with it alone). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Summary
Five red classes from the 2026-09-03 corpus run, each its own commit. Two
are product defects; three are fixtures pinned to shapes the runtime has
moved past.
Problem and solution, per class
The relink route never derived attachment provenance (6 tests)
attachment_refs.directionisNOT NULL, and only the main write routederived it from the owning turn. Blob-reference closure relinked
attachments straight from the parsed record:
The relink path now goes through the same shared derivation, reading the
owning turn from the index when the append payload's own message map does
not carry it — an append whose owning message is already materialized.
Two of the six failed inside the main route instead, on a model-output
attachment whose turn carried no provider id;
#4587fixed that whilethis branch was open, and this commit builds on it.
One session total on two model rows (1 test)
sessions.reported_cost_usdis one exact dollar total for a session,written onto a single
session_model_usagerow. A merge-append thatswitched models left the superseded row holding the previous total, so
every sum over the session counted it twice.
The direction-refusal test alongside it named a user turn, whose direction
the derivation supplies; only a role outside the user/model pair leaves the
column underivable, so that is what it names now.
Hand-built index fixtures without a tier version (12 tests)
#4485's open-time tier check refuses a database named for an archivetier whose
user_versionis 0:the read profile expects, read from the tier table.
bootstrap rather than touching a bare SQLite file.
are. They test the pragmas the connection factories apply; the tier check
fired only because the files were called
index.dbandops.db.Hand-built
blockstables withouttool_outcome(6 tests)The insight-status and readiness queries read
blocks.tool_outcome, thecanonical structural outcome since
#4539. Two fixtures predate it.A killed waiter left its pytest slot task queued (host defect)
The waiter blocks on a task it queued through pueue, and the task outlives
it. The slot's parallelism is one, so a task left with no waiter starves
every other checkout on the host until someone reads the queue and kills it
by hand -- which is what happened during this branch's own runs. The waiter
now reaps the task it added when it is signalled or unwound: kill, then
remove, then drop the launch file, which carries a resolved environment and
must not survive either.
Full-catalog wire receipts on single-provider assertions
build_wire_support_receiptruns the synthetic corpus and the productionparser for every catalogued package element. Profiled: 27 s per
full-catalog call in a quiet process, and the file's heaviest tests
measured 106-110 s of call time under
-n 2-- inside a 120 s timeout withno margin, which is what the corpus run recorded as nine timeouts.
#4412added two providers and the margin went. Tests that assert about one
provider's entry, or that a receipt changed, now name the providers they
read; three catalog-wide tests still build the whole catalog.
Memoizing the schema-annotation parsing the profile flagged was tried and
reverted: 26.5 s against 27.4 s, which does not buy an identity-keyed cache
in production code.
Reader assertions pointed at the wrong route
The interpolated shell answers on
/w/and the paste routes; the root and/s/serve the typed WebUI. Eight reads across the visual suites andtest_web_shell_reader.pynow fetch/w/stack. This also restorestest_reader_search_shell_dom_evidence, which#4579deleted -- the shellit tests is still served, just not at the root, so repointing it was the
right move and deleting it was not.
Verification
devtools test <file> -n 2:tests/unit/storage/test_archive_tiers_write.pytests/unit/maintenance/test_blob_reference_closure.pytests/unit/cli/test_embed_activation.pytests/unit/devtools/test_deployment_smoke.pytests/unit/storage/test_wal_journal_size_limit.pytests/unit/storage/test_session_insight_status_descriptors.pytests/unit/core/test_insight_readiness.pytests/unit/devtools/test_pytest_slot.pytests/unit/storage/test_session_profile_model_usage_consistency.py+tests/unit/insightsdevtools verify --quickgreen at this head.The attachment and cost measurements above predate the rebase onto
#4587, which rewrote_attachment_provenanceunder this commit; thestorage suites are re-running against the merged shape.
The slot-reaping test drives a real SIGTERM into a child waiter with a fake
pueueon PATH. Anti-vacuity: dropping the_reapingcontext leaves therecorded calls at
add/wait, the starvation shape itself.The visual and wire-receipt commits are still queued behind the host's
pytest slot at the time of writing; they are committed, not yet measured.
Residual risk
The provider-scoped wire receipts trade catalog breadth for time in tests
that assert about one provider. If a future provider's route breaks only in
a receipt those tests build, three catalog-wide tests still catch it, one
test later.
tests/unit/infra/test_pathology_zoo.pywas red in the corpus run and isgreen at this head with no change here.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YGi8wLWGR2HYBh8p8fXFXz