fix(provenance): the late sweep may only complete TERMINAL worker attempts - #63
Conversation
…empts CodeRabbit finding on PR #42 (thread 3837879039), verified real against the real code path before patching. `resolve_unresolved_worker_attempts` selected on `operation_role='worker' AND resolved_model IS NULL AND profile_id IS NOT NULL` with no status predicate. The profile attempt row is written `started` BEFORE the subprocess is spawned (dispatcher.py:1511, exp_abcd.py:412), so an IN-FLIGHT attempt matched every clause. `adapters.cli_reported_model` reads the FIRST model in the session log within a 2h window of `started_ts`, and that log exists from the moment the CLI starts -- so a run still executing probes clean, and `--apply` stamped it `complete` with a resolved model and a `completed_ts` off the sweep's own clock. Reproduced on a real tmp ledger: a `started` row with completed_ts NULL became ('complete', 'gpt-5.6-terra', <sweep time>). That is the one row shape CLAUDE.md §2 allows to support an exact-model claim -- a successful `operation_role=worker` attempt with a provider-resolved model -- minted for a worker that had not finished and could still fall back, retry onto another model, or fail outright. It corrupts the Brain silently rather than failing loudly. The filter is `status='unresolved'`, which also excludes `failed` (dispatcher's `profile_process_start_failed`): terminal, but it never ran, so there is no served model to recover and resolving it from a neighbouring session in the window would be invention. CodeRabbit's rationale named only `started`. Not a starved drain: a `started` row is excluded only while in flight -- its own completion closes it to `complete` (resolved, no sweep needed) or `unresolved` (eligible next pass), so the exclusion clears itself without the sweep's help. Measured read-only on the live ledger: 56 candidates before the filter, 56 after; every genuinely drainable row is already `unresolved`. Exclusions are counted, never silently narrowed: `excluded_not_terminal` reports them keyed by status, beside `candidates`, matching the existing `excluded_unreportable` convention. `candidates: 0` next to `{started: 3}` reads as "wait for those runs"; `candidates: 0` alone reads as "the sweep is broken". Coverage: one new test pins all three cases -- terminal `unresolved` IS swept, in-flight `started` is left alone, never-ran `failed` is left alone -- with all three sharing one workspace so the probe resolves for every one of them and the status filter is the only thing that can protect the two ineligible rows. Includes an in-test deliberate break (a connection proxy that strips the status clause, restoring the pre-fix query) asserting the corruption reappears. Separately demonstrated break->revert at the source: deleting the clause from the query fails the test on the in-flight assertion; restored, green. FLOOR 387 -> 388, hand-edited with rationale (--update-floor clobbers the note). No ceiling moved and nothing new is skipped: the test builds its own tmp ledger and rollout fixture, so it runs on a bare runner. verify.py: 388 passed, 0 failed, 0 skipped, 83/83 selftests, 5/5 gates -- in the worktree and from a mirror-shaped copy at a different path. Ruff unchanged from the HEAD baseline (9 pre-existing findings, none added). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Limit details: You’ve used the included review currently available. Your 74 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe reconciliation resolver now processes only terminal unresolved worker attempts. It reports excluded non-terminal attempts by status. Regression coverage verifies the behavior and updates the recorded verification total to 392 tests. ChangesWorker Attempt Reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change prevents the late sweep from resolving in-flight or never-started worker attempts, limiting model provenance updates to terminal unresolved attempts. The PR is mergeable with owner awareness that the regression test should be maintained if the query formatting changes. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…asured Only `.verify-floor.json` conflicted, the collision this file's own note predicts and which the branch note had already flagged as likely. #59 (improvement-log accessor) raised the floor 387 -> 391 on main with four new tests; this branch raised it 387 -> 388 with one. Resolved as the UNION, not by taking a side: #59's rationale is retained and the count was RE-MEASURED on the merge result rather than either number being carried over. 392 collected, independently confirmed by `pytest --collect-only` before trusting the arithmetic -- taking a side is what once put the floor 8 tests below reality. verify.py on the merge result: 392 passed, 0 failed, 0 skipped, 84/84 selftests, 43/43 can-fire, 5/5 gates green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
Workflow source detectedPR #63 now has valid workflow source context (origin=review_followup). No linked GitHub issue is required for this PR. |
Automated Status SummaryHead SHA: 0a30a7c
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
|
Runner dispatch state for autofix on PR #63. Do not edit. |
|
Runner dispatch state for codex on PR #63. Do not edit. |
|
Workflow state fingerprint for Agents Gate Followups. Do not edit. |
|
Workflow state fingerprint for Keepalive Loop Reporter. Do not edit. |
CI red here is #60's repo-wide breakage, not this changeAll six Python checks on this PR ( Job 97205943081, step 8 Evidence it is not this branch:
So this PR cannot show green CI until #60 lands. Nothing to fix here; sequencing only. Checked against the config #60 is about to introduceSince #60 adds the repo's first Note Local verdict on the merge result stands: |
|
Autofix updated these files:
|
Only `langsmith-fleet-worker-attempt.json` conflicted (add/add). It is not source: it is a per-run artifact the Codex autofix runner writes into the working tree and commits, so every PR through that runner overwrites the previous PR's copy -- main's says `pr_number: 61`, this branch's autofix commit wrote `63`. Whichever value wins is meaningless, so it is resolved to MAIN's copy, leaving this PR's diff limited to the sweep fix rather than gratuitously rewriting an unrelated artifact. Flagged separately; a tracked runtime artifact that every PR rewrites will keep conflicting until it stops being tracked. `.verify-floor.json` merged cleanly this time: #61 added no net new tests, so main stayed at 391. RE-MEASURED rather than assumed, per the rule in that file -- `pytest --collect-only` reports 392, which is main's 391 plus this branch's one new test, matching the already-recorded value. No adjustment needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
Conflicts resolved; workflow source addedConflict was
Workflow source: added Autofix's
|
CodeRabbit thread 3838730241 on #63, verified: the break keyed on an exact substring including the trailing space, so a reformatted query would make `str.replace` a silent no-op. The filter would keep protecting the row, the corruption assertion would fail, and its message would blame the fix rather than the stale fixture. It fails RED either way -- no false green -- but it misdiagnoses, and in this repo a check has to name its own cause. Two guards, because there are two ways to go stale, and only one was proposed: 1. The clause MOVES within a still-recognisable query -- asserted per statement, naming the clause it expected and printing the actual SQL. 2. The query itself becomes UNRECOGNISABLE, so nothing is ever stripped -- caught after the run by `stripped`. The review's guarded snippet asserts only WHEN the FROM/JOIN + ORDER BY signature matches, so a rewrite that changes the signature leaves the break silently inert and its assert never runs. That is the same hole one level up. Not the review's first proposal, which is tautological: `broken != sql or clause not in sql` cannot fail, since `clause in sql` makes `broken != sql` necessarily true and the other branch covers the rest. Its second snippet is the right shape and is what guard 1 implements. The candidate query is identified by its FROM/JOIN plus ORDER BY fragments; the not-terminal count query shares the FROM/JOIN but ends in GROUP BY, so it is not mistaken for the candidate and other SQL passes through untouched. Both guards demonstrated then reverted: clause "unresolved" -> " = 'unresolved' " => "deliberate break is STALE: the candidate query no longer contains ... Update this fixture" + the real SQL ORDER BY r.ts DESC -> ASC (valid rewrite) => "deliberate break never fired: nothing matched the candidate query's ... the assertions below prove nothing" No new test, so the floor stays 392 (re-measured: 392 collected). verify.py: 392 passed, 0 failed, 0 skipped, 84/84 selftests, 5/5 gates. Clean under #60's incoming ruff config and black -l 100. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
…e-measured #60 landed the `.github/workflows/autofix-versions.env` pin file that every Python check on this PR was dying on, at the shared `Install dependencies` step before any tool ran. It also brings this repo's first `ruff.toml` and `mypy.ini`, so this change is linted for real for the first time rather than under Ruff's 88-column defaults. Checked against the config that actually landed, not the branch preview: ruff check ledger_reconcile.py test_feedback_model_provenance.py -> passed ruff check . (the Gate's own command) -> passed black --line-length 100 --check on both files -> unchanged `.verify-floor.json` conflicted, as it has every round. Resolved as the UNION: main's note is kept whole (it carries #60's +11 tests and the #64/#65 rationale) with this branch's entry appended, and the count RE-MEASURED on the merge result rather than either side's number -- `pytest --collect-only` reports 403, which is main's 402 plus this branch's one test. No ceiling moved; nothing new is skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
#56 makes `collected` an EQUALITY rather than a minimum, so a floor that sits below reality now goes RED instead of passing quietly. That turns "assume 402+1" from a bad habit into a hard failure, which is the right direction and the reason every round of this branch measured instead of deriving. Union as before: main's note kept whole, this branch's entry appended, and the count MEASURED on the merge result -- `pytest --collect-only` reports 403. Main stayed at 402 across #56, so #56 added no collected tests and 403 is main's 402 plus this branch's one test. No ceiling moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
CI is green — the change has now actually been measured#60 merged, so the
54 checks: 25 pass, 29 skipping, 0 failing; combined commit status Also merged since: #56, which makes
One note on the diff
|
# Conflicts: # .verify-floor.json # langsmith-fleet-worker-attempt.json
GitHub Actions
Inactive
GitHub Actions
Inactive
The automated merge of origin/main spliced this branch's floor entry mid-token: "FLOOR 407 -> 408 on 2026-08-23 (CodeRabbit follow-up ..." became "3 on 2026-08-23 (CodeRabbit follow-up ...", losing which transition the entry records. The counts were already correct and agree with my own resolution of the same conflict (408/408, re-measured independently: `pytest --collect-only` reports 408 = main fc1fd42's 407 plus this branch's one test). Only the prose was damaged, but this note is the sole record of WHY each floor moved, so a fragment that no longer names its transition is exactly the kind of unreadable evidence this file exists to prevent. Kept the automated merge's #68 entry rather than my own wording: it documents the union more fully and gives the artifact resolution a provenance rationale -- main's NEWER langsmith-fleet worker-attempt record is retained, because discarding a newer provenance observation to win a merge would corrupt exactly the causal-provenance evidence CLAUDE.md section 2 protects. That is a better reason than the one I used ("it is meaningless either way"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
Seventh floor union on this branch. main a247a55 records 410; the merge result MEASURES 411 with `pytest --collect-only -q` -- main's 410 plus this branch's one test. Measured, never derived: #56 made `collected` an EQUALITY, so an assumed number is a hard RED. #69 also flips CI coverage ON, so this branch's merge is the first to be measured under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
langsmith-fleet-worker-attempt.json is a per-run CI provenance record, not source. reusable-codex-run.yml (in stranske/Workflows) rewrites it on every invocation and uploads it as artifact langsmith-fleet-v1-worker-attempt-<pr>. It became tracked in 2118f57, and because gitignore does not apply to an already-tracked path, the runner's `git add -A` commit step staged it on every run: ten commits across PRs #59/#61/#62/#63, one add/add conflict per concurrent PR, and a silently widened diff on each. Nothing reads it from the tree. The langsmith-fleet/v1 consumers here (langsmith_pull.py, langsmith_fetch.py) ingest the NDJSON Actions artifact `langsmith-fleet.ndjson`; neither names this path. The 30-day artifact upload already provides the replacement path, so untracking loses no provenance. stranske/Workflows reached the same verdict for its own copy. The ignore entry sits outside the synced WORKFLOWS STATUS FILES block, since that block mirrors a consumer template which does not carry this pattern and would drop it on the next rewrite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
langsmith-fleet-worker-attempt.json is a per-run CI provenance record, not source. reusable-codex-run.yml (in stranske/Workflows) rewrites it on every invocation and uploads it as artifact langsmith-fleet-v1-worker-attempt-<pr>. It became tracked in 2118f57, and because gitignore does not apply to an already-tracked path, the runner's `git add -A` commit step staged it on every run: ten commits across PRs #59/#61/#62/#63, one add/add conflict per concurrent PR, and a silently widened diff on each. Nothing reads it from the tree. The langsmith-fleet/v1 consumers here (langsmith_pull.py, langsmith_fetch.py) ingest the NDJSON Actions artifact `langsmith-fleet.ndjson`; neither names this path. The 30-day artifact upload already provides the replacement path, so untracking loses no provenance. stranske/Workflows reached the same verdict for its own copy. The ignore entry sits outside the synced WORKFLOWS STATUS FILES block, since that block mirrors a consumer template which does not carry this pattern and would drop it on the next rewrite. Co-authored-by: t <t@e> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…rop a claim #3210 falsifies This branch ended up with two independent explanations of the same rule, because a concurrent session added `4b5764a` while this branch already carried its own block. Both were correct; having both in a file whose header is specifically about gitignore-comment discipline is not. Kept from the second block, because the first did not have it: - the artifact was tracked from 2118f57, not merely "on main until today"; - ten commits across PRs #59/#61/#62/#63 rewrote it, one add/add conflict per concurrent PR, silently widening unrelated diffs; - untracking loses no provenance — the producer already uploads the same bytes as Actions artifact `langsmith-fleet-v1-worker-attempt-<pr>` with 30-day retention, which is the only reason the file is written at all. Dropped from it, because it is about to become false: "that block mirrors Workflows' templates/consumer-repo/.gitignore, which does NOT carry this pattern". Workflows PR #3210 adds exactly these two patterns to that template at Template-Version 6. The placement above the managed block is still right, but the reason is that a version bump rewrites the block wholesale — not that the template lacks the pattern. Also drops that block's bare literal `langsmith-fleet-worker-attempt.json`, which was already subsumed by the root-anchored `/langsmith-fleet*.json` above it. Comments only. Verified identical behaviour with `git check-ignore`: the root artifact and artifacts/langsmith/ stay ignored, docs/contracts/schemas/langsmith-fleet-v1.schema.json and langsmith_*.py stay committable, and no tracked file is shadowed by any pattern. 427 collected = floor 427; black, ruff and the 11 guard tests unchanged.
…both stranded post-merge (#84) Both branches were held back from the branch cleanup because their tips carried commits pushed AFTER their PR merged, so "PR merged" did not mean "work landed". Verified per-symbol rather than by diff size — both branches are thousands of lines behind main, so a raw diff conflates stale with unlanded. #42 / commit 4e0d6ae — `adapters.py` catalog resolution. Main has `advertised_models` and NONE of the generalisation around it: `advertised_catalog`, `_advertised_catalog`, `_cached_catalog`, `agy_log_for`, `AGY_LOG_SUFFIX`, `CATALOG_ROUTING_TAGS`, `_catalog_model_id` were all absent. This is learning-loop provenance code (CLAUDE.md 2: "never treat a generic trace model as provider resolution"), and its whole point is that THE CATALOG IS THE AUTHORITY — a label resolves against the ids the CLI actually advertises, with routing TAGS (`auto`, `default`, `cli-default`) refused as non-identities. The commit's own note records that `VENDOR_MODEL_RE` rejects 42 of 204 real cursor ids, so shape-matching an id the CLI itself advertised is both redundant and wrong. Cherry-picked; `adapters.py`, `dispatcher.py` and `ledger_reconcile.py` applied clean. Two conflicts: * `.verify-floor.json` — took main's. A floor is a property of the MERGE RESULT, never carried in from a branch, so it is re-measured below. * `test_feedback_model_provenance.py` — TWO DIFFERENT tests in one region: main's `test_late_sweep_completes_terminal_attempts_never_one_in_flight` (from #63) and the branch's `test_gemini_provenance_reads_the_per_run_log_before_the_conversation_store`. Kept BOTH; they are independent. #34 / commit c1dc9a7 — README item 11 for `evidence_acquisition.py`, which main documented nowhere (zero occurrences). Every factual claim was re-verified against main's code rather than trusted: `capabilities.unblock()` exists; `ORCH_EVIDENCE_ACQUISITION_MAX_FEEDS`/`_MAX_ITEMS` default to 1 and 3; `LIVE_FLAG = "ORCH_EVIDENCE_ACQUISITION"` with SHADOW as the documented default; and the quoted summary line matches the format string verbatim (`feedable {n} / capped {n} / candidates {n} / fed {n}`). It is the drainable-vs-blocking line the latched-gate rule asks for, and it was the only place that reported it. DELIBERATE-BREAK -> REVERT: emptying `CATALOG_ROUTING_TAGS` fires `assert model_id_for_label("cursor", "Auto (default)") is None` in adapters' OWN selftest; reverted clean. Worth recording that `pytest test_feedback_model_provenance.py` did NOT catch that break — the guard is covered by a `--selftest`, not by a test_*.py, which is precisely why `verify.py` is the gate and a pytest subset is not. A redundant pytest test written before checking was dropped. FLOOR 427 -> 428, one new test, note appended not replaced. Verified FRESH-STATE (both ORCH_STATE_DIR and ORCH_LOCAL_RUNTIME at empty dirs, reproducing CI): VERIFIED — 420 passed, 0 failed, 79 selftests, 3/5 gates green, 8 tests + 5 selftests + 2 gates skipped for named prerequisites; 420 + 8 = 428 = floor. ruff + black -l 100 clean. NOT FIXED HERE, and not caused here: on this machine `test_capabilities.py`'s `test_gate_blocks_execution_is_opt_in_and_narrow` and `test_evidence_gate_kind_is_not_blanket_observer` fail on PRISTINE main too — the hourly fleet tick mutated the machine-local ledger and range-lane-rollout now classifies `matched_not_invoked` instead of `deliberately_gated`. Ledger STATE, not code; they skip with a named reason under a fresh ledger, which is what CI uses. Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Follow-up to the one CodeRabbit thread on #42 that was never verified against the code — thread 3837879039. Verified real before patching, with the caveat that CodeRabbit's own check used a synthetic in-memory table it invented rather than the real schema, so the finding needed independent confirmation.
The defect
resolve_unresolved_worker_attemptsselected onoperation_role='worker' AND resolved_model IS NULL AND profile_id IS NOT NULLwith no status predicate. The profile attempt row is writtenstartedbefore the subprocess is spawned (dispatcher.py:1511, exp_abcd.py:412), so an in-flight attempt matched every clause.adapters.cli_reported_modelreads the first model in the session log within a 2h window ofstarted_ts, and that log exists from the moment the CLI starts — so a run still executing probes clean.Reproduced on a real tmp ledger via the real code path:
That is the one row shape CLAUDE.md §2 permits to support an exact-model claim — a successful
operation_role=workerattempt with a provider-resolved model — minted for a worker that had not finished and could still fall back, retry onto another model, or fail outright. It corrupts the Brain silently rather than failing loudly.The status lifecycle
startedrecord_execution_attemptunresolvedcomplete_profile_attempt_unresolvedcompletecomplete_profile_attemptresolved_model IS NULL)failedcomplete_profile_attempt_unresolved(status="failed"), dispatcher.py:999The filter is
status='unresolved', which also excludesfailed(profile_process_start_failed): terminal, but the process never started, so there is no served model to recover and resolving it from a neighbouring session in the 2h window would be pure invention. CodeRabbit's rationale named onlystarted.Not a starved drain
A
startedrow is excluded only while it is in flight. Its own completion closes it tocomplete(resolved — no sweep needed) orunresolved(eligible on the next pass), so the exclusion clears itself without the sweep's help. Measured read-only on the live ledger: 56 candidates before the filter, 56 after — every genuinely drainable row is alreadyunresolved.Exclusions are counted, never silently narrowed:
excluded_not_terminalreports them keyed by status, besidecandidates, matching the existingexcluded_unreportableconvention.candidates: 0next to{"started": 3}reads as "wait for those runs to finish";candidates: 0alone reads as "the sweep is broken".Coverage
One new test pins all three cases — terminal
unresolvedis swept, in-flightstartedis left alone, never-ranfailedis left alone. All three runs share one workspace so the probe resolves for every one of them, making the status filter the only thing that can protect the two ineligible rows.Deliberate break→revert, both ways:
Verification
python3 verify.py— 388 passed, 0 failed, 0 skipped, 83/83 selftests, 5/5 gates, in the worktree and from a mirror-shaped copy at a different path. The exec mirror was independently confirmed green at its current 387 baseline (synced to af6654d, so current with main and missing only this fix). The mirror sync is deliberately left undone — per CLAUDE.md §1 it is the circuit breaker between an agent's change and the dispatcher, so it is due after merge, not before.Floor 387 → 388, hand-edited with rationale (
--update-floorclobbers the note). No ceiling moved and nothing new is skipped: the test builds its own tmp ledger and rollout fixture, so it runs on a bare runner. Ruff unchanged from the HEAD baseline (9 pre-existing findings, none added).Three sibling follow-up branches are in flight against this same main; if
.verify-floor.jsonconflicts, resolve as the union and re-measure on the new merge result rather than taking either number.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests