/code-review max findings: the ledger promise, and four stale records - #290
Merged
Conversation
…ecords Five findings held on re-measurement; two the sweep raised did not and are not acted on (an "editable install" claim — the venv runs a plain stale 2.3.0 from site-packages, so the drift guard is correct; and an "impossible arithmetic" claim about C-46's figures — those were scoped to PR #280, which adds no tests, and reproduce exactly CI-shaped). 1. HIGH — the torn-run message truncates at five objects and pointed the reader at "the run log" for the rest. The only writer of a file_id was `logger.info`, and nothing in this package sets a level: pipeline-core removed its own `setLevel` so the application owns it. A launcher at WARNING would have written no id anywhere, making the pointer a promise to an empty file — and leaving the operator diffing the bucket by hand, which is the state C-105 exists to remove. The complete ledger is now emitted at ERROR, one greppable `TORN-LEDGER` line per object, which is also ADR-008's log-and-raise. Tested and mutation-proven. 2. The CIC described the SUPERSEDED findability check — "refuses a falsy answer" — which is the question C-94 says "the previous delivery already answers yes to". It now documents the run-scoping and both refusals, including the two-id one an operator would actually meet. 3. C-84 enumerated "the two honest ways to make it pass" while the test offers three, and the omitted one — ACKNOWLEDGED_UNTIL — is the only remedy available without console access. The register is where someone goes when the gate reddens; leaving it out reproduced the merge-queue-hostage outcome the acknowledgement exists to prevent. 4. C-103's Location cited unfao.py:137-151 as "the two branches" and claimed the lines were re-read. They are at 169 and 174; 137-151 is now the body of the C-94 preflight added later in the same branch. This is the second time in four days, so the fix is structural: cite by symbol. 5. Register header still said 2026-08-17 with seven later-dated records below it, and C-105 claimed the manager is at 435/450 when `wc -l` says 434. Registered as C-109, because the fourth is a class rather than a typo. Measured: eleven of 28 open entries cited a `.py:line`; spot-checking six found THREE already stale, all drifted by a later change in the same week. C-103, C-105, C-106 and C-30 converted to `path::symbol`; eight entries still cite lines where the target genuinely is one. The entry records why the obvious guard does not work, so it is not proposed again cheaply. Every symbol citation verified to resolve to a real `def`. Suite: 468 passed, 3 skipped, 37 xfailed; the 26 failures are C-104 venv drift. ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five findings held on re-measurement. Two did not, and are deliberately not acted on:
.dist-info." The venv runs a plain stale 2.3.0 from site-packages —datafactory_contractabsent,MetadataSearchIncompleteabsent.test_locked_environmentis reporting correctly.426 → 430 passedis arithmetically impossible for a PR adding 36 tests." Those figures are scoped to PR C-30/C-46: the exclusion-manifest tripwire now runs in CI — the 2026-08-03 revert was the right observation with the wrong cause #280, which adds no test files. Reproduced CI-shaped against a tracked-files-only worktree: 426 → 430 exactly.The one that mattered
The torn-run message truncates at five objects and pointed at "the run log" for the rest — but the only writer of a
file_idwaslogger.info. Nothing in this package sets a level; pipeline-core removed its ownsetLevelprecisely so the application owns it. A launcher running at WARNING would have written no id anywhere, making the message a promise to an empty file — and leaving the operator diffing the bucket by hand, which is the exact state C-105 exists to remove.The complete ledger now goes out at ERROR, one greppable
TORN-LEDGERline per object. That is also ADR-008's log-and-raise. Mutation-proven: dropping it fails the new test.Four stale records
ACKNOWLEDGED_UNTIL) is the only remedy without console access — the register is where someone goes when the gate reddensunfao.py:137-151, "line numbers re-read"Last Updated 2026-08-17; C-105 "435/450"wc -lsays 434Registered as C-109, because #4 is a class
Measured: eleven of 28 open entries cited a
.py:line. Spot-checking six found three already stale — C-105's (written four days earlier) landing onstaging.mkdir, C-106's on a section comment, C-30's onreturn summary. All drifted because a later change in the same week moved them.C-103 is the sharp case: stale twice in four days, the second time carrying an explicit "re-read" claim that was false when it merged. A citation that decays faster than the review cycle is worse than a vaguer one, because it is confidently wrong.
C-103, C-105, C-106 and C-30 converted to
path::symbol; eight entries still cite lines where the target genuinely is one. The entry records why the obvious guard fails (checking the file is long enough catches nothing — every stale citation points at a real line) so it is not proposed again cheaply.Verification
All five symbol citations verified to resolve to a real
def. Suite 468 passed, 3 skipped, 37 xfailed; the 26 failures are C-104 venv drift, green in CI.ruffclean.One correction of my own, caught on re-reading: C-109 first said "nine of 37", from an inconsistent denominator. Re-measured with a single query — eleven of 28 before, eight of 29 after.