fix(scripts): re-derive the canary's stale comment claims and re-pin two loose markers - #2930
Merged
Merged
Conversation
…roduce Four claims in the canary's comment prose did not reproduce against the repository they describe. None of them is executable, so nothing failed -- which is the problem, because they read as measured and the next person to touch the calibration would have built on them. Each is rewritten to a claim that stays true rather than renumbered to a figure that will rot again. The "1527-commit history" total reproduced at no named endpoint -- it was 1546 at 738791c and 1549 shortly after -- so it is stated as "every first-parent commit of main" instead. The result the pin actually rests on, `Revert "` 0 and `revert:` 1, is unchanged and is what the sentence now leads with. "The replay exited 0 for 31h28m" credited the replay with a silence it was not present for. 31h28m is the CONTENT-ABSENCE window; this canary merged part-way through it, at 7b47d22, 6h13m before the restore. The passage now separates the two windows and says what was actually true over the tail: the row reproduced the removal exactly as recorded while the content was still gone. The row was never wrong about what it asserted -- it asserted the other question. The repo-wide-grep counterfactual mixed tenses. Its present-tense half holds; its past-tense half did not, because CHANGELOG.md got its copy of the marker from 534eac1, the restore commit itself, so during the absence a repo-wide grep for the evals marker still matched nothing. The passage now says which tense it is speaking in and why. "A four-row corpus" and "all five markers" are both counts that go stale as rows are added, and neither count was load-bearing. They are now "per marker row" and "every marker". Also records the one-occurrence obligation on marker literals: `grep -qF` answers "is this string anywhere in this file", so a marker appearing at both a definition and a use site is satisfied by a re-land that restored only the use -- a partial re-land reported as ok, the exact failure the assertion exists to catch. Two shipped markers bind that loosely today; #2915 tracks re-pinning them. Closes #2917 Co-authored-by: Claude <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 56s —— View job Security review
No security issues found. Skip-gate applies: every changed line in No injection surfaces, authorization/access-control logic, secrets/token handling, or workflow permissions are present in this diff, so there is nothing in scope for this lane. |
|
Last security-reviewed head: |
…once `--verify-restoration` tests a marker with `grep -qF`, which answers "is this string anywhere in this file". Two shipped markers were bare identifiers that occur twice in the file they bind to -- once at the definition and once at a use site: _FIND_SIDE_EFFECT_PRIMARIES destructive_guard.py:1010 and :1047 MERGED_PR_GRAPHQL_ALIAS_PAGE audit-fleet.sh:286 and :2257 A re-land that restored only the use site would therefore satisfy the grep and the row would report ok -- a PARTIAL re-land reported as restored, which is precisely the failure this assertion exists to catch. Each now records its definition line instead: `_FIND_SIDE_EFFECT_PRIMARIES = frozenset(` and `MERGED_PR_GRAPHQL_ALIAS_PAGE=100`. Both occur exactly once in their bound file, and a restore that reinstated only the use site cannot contain either. To be precise about what was and was not broken: both bare forms still discriminated the recorded incidents, because those reverts removed the definition and the use together. Measured at the recorded revs, each bare marker is absent at the reverting commit and present at its parent. The weakness was never in what they assert about the past -- it was that the one future shape the assertion exists to catch could slip past them. All five markers now occur exactly once in their bound file, and each new literal is present at the parent of its reverting commit and absent at the reverting commit itself. Closes #2915 Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton
changed the title
![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: 3b10dd1a93
ℹ️ 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".
Re-pinning `_FIND_SIDE_EFFECT_PRIMARIES` and `MERGED_PR_GRAPHQL_ALIAS_PAGE` from bare identifiers to their definition lines closed only one direction. A definition line occurs exactly once and a use-site-only re-land cannot contain it, so the use-only false green is gone -- but the inverse opened in its place: a re-land that restores the definition and never reconnects the use satisfies the row while the guard stays disconnected. So each constant is now TWO marker rows, its definition and its only use, and both must hold. A definition-only re-land passes the definition row and fails the use row; a use-only re-land does the reverse. Measured for both added literals, and re-measured independently for all seven marker rows now in the corpus: each occurs exactly once in the file it binds to on the working tree and at origin/main, zero times in that path at its recorded incident sha, and at least once at that sha's first parent. Also corrects the note above the 9239f15 row. It credited #2640 with the destructive_guard.py restore; #2640 (6f0a311) changes no file under plugins/disk-hygiene, both markers are absent at the only two intervening commits that touched the file (#2671, #2676), and both first appear at de26c27 (#2706). The #2640 credit on the cc58cbc row is correct and is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwdkpWf6bptu3AqTMoeg2H
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 55s —— View job Security review
No security issues found. Skip-gate does not fully apply this time: the previously reviewed head (
No injection surfaces, authorization/access-control logic, secrets/token handling, or workflow permissions are present in this diff. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job, or workflow_dispatch this workflow with the PR number, to retry the review. A new push re-triggers this lane only if the caller's |
kyle-sexton
removed
the
do-not-merge
This was referenced Aug 17, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
…ts corpus (#2986) #2917 item 2 named the same unhedged `31h28m` claim in two files. #2930 corrected `scripts/check-silent-revert.sh` and left `scripts/silent-revert-incidents.txt` byte-identical, so since #2930 the two files have actively contradicted each other on the one question the whole restoration assertion turns on. This finishes item 2 and clears the two smaller prose defects the same review pass found. ## 1. The surviving claim in the corpus file `scripts/silent-revert-incidents.txt` said: > It says nothing about whether the content that commit deleted is on main today, > and that gap has already cost this repo 31h28m: the content #2828 is about was > missing from main while the f603880 row below printed `ok ... fires as > recorded` and the replay exited 0. The defect is that "that gap has already cost this repo 31h28m" charges the whole absence to a mechanism that existed for only part of it, and it does so in the same breath as "the replay exited 0" — so the sentence reads as thirty-one hours of a running, silent replay. It was not. The canary did not exist for most of that window. Rewritten to separate the two windows in the same terms the script now uses, and to keep the point the passage was carrying — that the replay's silence was never a statement about content presence: > The content #2828 is about was absent from main for 31h28m: f603880 removed it > and 534eac1 (#2829) put it back. > > 31h28m is the CONTENT-ABSENCE window, not a window this replay ran through in > silence -- the canary did not exist for most of it. It merged part-way in, at > 7b47d22 (#2808), 6h13m before the restore, so the replay covered only the tail > of the absence. Over that tail the f603880 row below printed > `ok ... fires as recorded` and the replay exited 0 while the content was still > gone: the row was never wrong about what it asserted, it asserted the other > question. Two re-lands (#2714, #2803) each missed it, and a hand audit caught it, > not this file. Both figures are closed historical intervals between two fixed commits, so neither can rot — that is why they are stated rather than replaced with a hedge. Neither is a running total. ### What the two figures measure Both are committer-date deltas, re-derived here rather than carried over: | interval | endpoints | seconds | figure | | --- | --- | --- | --- | | content absence | `f603880da` (1786752447) → `534eac138` (1786865745) | 113298 | 31h28m18s → **31h28m** | | replay presence | `7b47d2253` (1786843322) → `534eac138` (1786865745) | 22423 | 6h13m43s → **6h13m** | `7b47d2253` (#2808, the commit that merged this canary) is an ancestor of `534eac138` and its timestamp falls inside the absence interval, so the canary genuinely merged part-way through. The replay therefore covered 22423 of 113298 seconds — under a fifth — and the prose says "only the tail" rather than quoting that fraction, matching the script. ## 2. The broken antecedent in `check-silent-revert.sh` In the `The restoration assertion (#2855)` comment block, one paragraph establishes that marker text is matched with `grep -F` and therefore "may contain any character -- no separator is reserved inside it". The next sentence a reader needs is "The ONE reserved position is a LEADING `[`". #2930 inserted its ~30-line uniqueness block ("a marker must occur exactly ONCE in the file it binds to", plus the definition/use two-row design and the corpus-review obligation) between them, and did so without even a blank comment line before "The ONE reserved position", so that sentence ran straight out of a paragraph about counting occurrences and its antecedent sat about thirty lines uphill. Fixed by relocating the uniqueness block below the disposition paragraphs, which restores the original adjacency: whitespace/any-character → `[` is the one reserved position → the disposition needs a non-empty reason → the disposition ends at the first `]`. The uniqueness block now closes the section. Two connective repairs were needed because the block no longer follows the sentence it used to lean on, and both preserve what is asserted: - `For the same reason a marker must occur exactly ONCE` → `Literal matching is also why a marker must occur exactly ONCE`. "The same reason" was the looseness of literal substring matching, which the block's own next sentence (``grep -qF` answers "is this string anywhere in this file"`) immediately restates, so naming it costs nothing and removes the anaphor. - `Nothing enforces this:` → `Nothing enforces the once-rule:`, since "this" now has no adjacent referent. Nothing else in the block changed — the diff is a pure relocation apart from those two clauses. ## 3. The distributing parenthetical Both files claimed that both `f603880da` markers occur in the same three sibling sites: > Both markers on the f603880 row also occur elsewhere in the same plugin on > current main -- the engine script, its test file, CHANGELOG.md That reads as three sites applying to both markers. Re-measured from scratch on `origin/main` with `git grep -F` per literal, no pathspec: **README marker** (bound to `plugins/disk-hygiene/README.md`) — 5 hits: - `plugins/disk-hygiene/README.md:6` (the bound file) - `plugins/disk-hygiene/skills/clean/scripts/hygiene.py:2807` - `plugins/disk-hygiene/skills/clean/scripts/hygiene.py:3601` - `plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py:566` - `scripts/silent-revert-incidents.txt:107` (the marker row itself) No CHANGELOG hit. **evals marker** (bound to `plugins/disk-hygiene/skills/clean/evals/evals.json`) — 3 hits: - `plugins/disk-hygiene/skills/clean/evals/evals.json:138` (the bound file) - `plugins/disk-hygiene/CHANGELOG.md:139` - `scripts/silent-revert-incidents.txt:108` (the marker row itself) No engine-script or test hit. So the sites do not distribute: the engine script and its test belong to the README marker only, CHANGELOG.md to the evals marker only. The load-bearing claim survives untouched — each literal does occur outside the file it binds to, which is why a repo-wide grep would report the incident restored on a tree where it is not — and the prose now says that in the per-marker form. Neither rewrite quotes either literal. Doing so would have added an occurrence of the very string whose occurrence set the sentence enumerates; the prose refers to them by role ("the README marker", "the evals marker"), which is what the script already did. Nor does either rewrite assert exhaustiveness, since both literals also appear on the corpus page itself — a fact that file already states separately. ### The provenance sentence had to move with it The script's old follow-on read: > Stated in the present tense on purpose: the sibling copies are a property of main > as it stands now, not of the tree during the #2828 window. Measurement contradicts that for the README half, so it could not be left standing next to a corrected parenthetical: - The evals half holds. At `f603880da` the evals literal occurred nowhere in the tree, and a `git log -S` walk over the whole `f603880da..534eac1` range returns exactly one commit — `534eac138` itself. So `plugins/disk-hygiene/CHANGELOG.md` acquired its copy at the restore, and the interval is measured rather than interpolated between endpoints: a repo-wide grep for that literal matched nothing at any point during the absence. - The README half does not. `git log -S` over `f603880da..534eac1` returns `94ae28728` (#2803, one of the two re-lands the passage already names as having missed the README) and then the restore. `94ae28728` put the string into `hygiene.py` and `test_hygiene.py` and never into `README.md`, and it landed 2026-08-15T20:37:17-04:00, which is 44m45s before `7b47d2253` merged this canary. Grepping at `7b47d2253` confirms it: three hits, all in the engine script and its test, none in `README.md`. So for the entire tail of the absence that the replay actually covered, a repo-wide grep for the README marker matched on a tree whose bound file did not have the content. The passage now says that, which turns the path-scoping argument from a hypothetical into the answer this very incident would have produced. ## Scope note The parenthetical defect is called out in #2981 against `check-silent-revert.sh`, but `silent-revert-incidents.txt` carried its own shorter copy of the same claim. Fixing only one would have re-created exactly the two-files-disagree condition this issue exists to close, so both were corrected — the corpus version kept shorter, to that file's register. ### A third site carries the same defect, and is deliberately NOT touched here Verification swept `31h28m` across the whole tree rather than only the two files in scope, and turned up a site neither #2917 nor #2981 names — `.github/workflows/silent-revert-canary.yml:154–157`, the workflow that actually runs the replay: > Both are needed, and the gap between them has already cost 31h28m: the content > #2828 is about was missing from main while the replay printed `ok` and exited 0 Same unhedged construction, charging all 31h28m to a period in which "the replay printed `ok`". It is pre-existing — `git diff origin/main` on that path is empty — and it is outside the file scope agreed for this change, so it is left alone here rather than folded in unannounced. It should be filed and fixed, and the correction is already derived above: the content was absent for 31h28m, of which this replay was present only for the final 6h13m, and over that tail it printed `ok` and exited 0 while the content was still gone. Recording it here so the remaining inconsistency is a known, measured follow-up rather than something a later reader rediscovers as evidence that this fix was incomplete. ## Verification CI is the verification of record; the shell suites were deliberately not run locally (they behave badly on Windows). Everything asserted above was measured with `git log`, `git rev-parse`, `git merge-base`, `git log -S`, and `git grep -F`, at explicit revs. A fresh-context verifier subagent then re-derived every figure independently, with the reasoning above withheld: both committer-date windows and their arithmetic, the ancestry and interval placement of `7b47d2253` and `94ae28728`, the complete per-literal occurrence sets on `origin/main` and on the branch tree (confirming the edits added no new occurrence of either literal), the adjacency repair in part 2 and that the relocated block's claims are unchanged, that every `31h28m` mention in the two edited files is now hedged and window-separated, and that the corpus file contains no stray non-comment line that would make the parser exit 2. It returned PASS on all sixteen claims, and its repo-wide sweep is what surfaced the workflow site noted above. The edits also add no line over 80 columns to either file — the over-80 counts are identical to `origin/main`'s (61 in the script, 13 in the corpus, all pre-existing). ## Related Closes #2981 - #2917 — the parent issue, whose item 2 this completes - #2930 — corrected the script half and left the corpus half behind - #2855 — added the restoration markers and the comment block part 2 repairs - #2847 — the standard requiring a figure that does not rot Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 21, 2026
…nt (#3066) ## Summary `.github/workflows/silent-revert-canary.yml` was the third site of the claim #2917 set out to remove. Its comment charged the whole `31h28m` content-absence window to the replay's silence — so the workflow contradicted the two `scripts/` files the job it documents actually invokes. This brings the last of the three sites into agreement. ## Fix Comment text only, in one hunk. No `run:`, `uses:`, `with:`, or any other executable YAML was touched (`git diff --numstat` = `6 3` on that one file; every `+`/`-` line begins with ` #`). Before: > incidents deleted is on the tree TODAY. Both are needed, and the gap > between them has already cost 31h28m: the content #2828 is about was > missing from main while the replay printed `ok` and exited 0, two > re-lands each missed it, and a hand audit found it, not this lane. After: > incidents deleted is on the tree TODAY. Both are needed, and #2828 > is the gap made real: its content sat off main for 31h28m, from > f603880 to the 534eac1 (#2829) restore. This lane covered only > the tail -- it merged at 7b47d22 (#2808), 6h13m out from the > restore -- and across that tail the replay still printed `ok` and > exited 0 with the content gone: it answered the other question. Two > re-lands each missed it, and a hand audit found it, not this lane. The load-bearing point is preserved rather than deleted: the replay's silence still did not mean the content was present. What changes is which window that silence is charged to. "Both are needed" is kept deliberately — the `ORDER IS LOAD-BEARING` paragraph immediately below reads off it. The wording is the workflow's own rather than a lift from either sibling; in particular it keeps this file's "not this lane" voice instead of the scripts' "not this canary" / "not this file". ## Verification Both figures re-derived here from committer dates (`git show -s --format=%ct`), not carried over from the issue: | window | from | to | seconds | rendered | | --- | --- | --- | --- | --- | | content absence | `f603880da` `1786752447` | `534eac138` `1786865745` | `113298` | 31h28m | | replay presence | `7b47d2253` `1786843322` | `534eac138` `1786865745` | `22423` | 6h13m | `113298s` = 31h28m18s and `22423s` = 6h13m43s; both are floored to the minute, matching the convention the two sibling files already use. `7b47d2253` is an ancestor of `main` (`git merge-base --is-ancestor` exit 0) and its committer epoch falls strictly inside the absence interval, which is what makes "only the tail" true. Repo-wide sweep for further sites: swept all tracked files for `31h28m`, `31h`, `6h13m`, `113298`, `22423`, the three incident shas, and numberless restatements (`exited 0 for`, ``printed `ok` ``, `while the replay`, `absence window`). **No fourth site exists.** The only near-hit outside the three known files is `plugins/claude-ops/skills/lanes/scripts/restart-consumer.test.sh:572` — "`lock-held` and exited 0 forever" about a Task Scheduler consumer — an unrelated sentence that merely shares the words. No follow-up issue needed. The change is comment-only, so no test asserts it; CI is the verification of record for the surrounding workflow, and the shell suites were not run locally (they run badly on this machine's platform). ## The other two sites Both siblings are already on `main` and untouched here — #2930 corrected `scripts/check-silent-revert.sh`, and #2986 (merged as `0e9ed8eb0`) corrected `scripts/silent-revert-incidents.txt`. This branch is merged up to a `main` that carries both, so all three passages can be read side by side in one tree. The workflow was the last holdout. ## Related - #2917 — the parent issue that named this claim - #2930 — corrected `scripts/check-silent-revert.sh` - #2986 — corrected `scripts/silent-revert-incidents.txt` - #2847 — the standard this follows: state the claim that stays true rather than renumbering a figure that rots Closes #2988 Co-authored-by: Claude Opus 5 <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
1 participant
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.
What
Two related fixes to the silent-revert canary, both about claims that do not hold: four comment claims that no longer reproduce, and two
markerliterals that bind more loosely than the corpus prose implies.Part 1 — four comment claims re-derived (#2917)
None is executable, so nothing failed — which is the problem: they read as measured, so the next person to touch the calibration would have built on them. Each is rewritten to a claim that stays true, not renumbered to a figure that rots again. That follows the standard set in #2847, which removed a "fires roughly once a month" claim rather than renumbering it, because the corpus measures a burst and no honest per-month rate exists.
738791c45, 1549 shortly after. Now "every first-parent commit of main". The result the pin rests on (Revert "0,revert:1) is unchanged and now leads the sentence.7b47d2253, 6h13m before the restore. The passage now separates the two windows and states what was true over the tail: thef603880darow reproduced the removal exactly as recorded while the content was still gone. The row was never wrong about what it asserted — it asserted the other question.CHANGELOG.mdgot its copy of the marker from534eac138— the restore commit itself — so during the absence a repo-wide grep for the evals marker still matched nothing.Part 2 — two loose markers split into definition and use rows (#2915)
--verify-restorationtests a marker withgrep -qF, which answers "is this string anywhere in this file". Two markers were bare identifiers occurring twice in their bound file:_FIND_SIDE_EFFECT_PRIMARIESdestructive_guard.py:1010:1047MERGED_PR_GRAPHQL_ALIAS_PAGEaudit-fleet.sh:286:2257A re-land restoring only the use site would satisfy the grep and the row would report
ok— a partial re-land reported as restored, exactly the failure the assertion exists to catch (#2828 was a partial re-land nobody noticed).The first pass narrowed each marker to its definition line. Review pointed out that this closes only one direction: a re-land restoring the definition while omitting the use would still report
ok, leaving the guard defined but disconnected. Narrowing cannot fix that — a single row can only bind one of the two halves.So each constant is now recorded as two rows, a definition row and a use row:
A definition-only re-land passes the definition row and fails the use row; a use-only re-land does the reverse. Each literal is the whole expression rather than a bare identifier, and none depends on leading whitespace —
parse_incidents_filesplits withread -r kind sha path rest, so leading and trailing whitespace on the literal is stripped and interior whitespace preserved.Being precise about what was broken: both bare forms did still discriminate the recorded incidents, because those reverts removed definition and use together. Measured at the recorded revs, each bare marker is absent at the reverting commit and present at its parent. The weakness was never in what they assert about the past — it was that the future shapes the assertion exists to catch could slip past them.
Measurements behind this PR
All by
grep -oF … | wc -lagainst the bound file,git show <rev>:<path>, andgit show <rev>^:<path>. Every marker row now in the corpus, not only the changed ones:origin/mainSafe tidiness is the primary objectivedisk-hygiene/README.mdempty-directories-remain-first-class-tidiness-findingsclean/evals/evals.json_FIND_SIDE_EFFECT_PRIMARIES = frozenset(destructive_guard.pyname.casefold() for name in _FIND_SIDE_EFFECT_PRIMARIESdestructive_guard.py--apply-plan may be supplied only onceaudit-fleet.shMERGED_PR_GRAPHQL_ALIAS_PAGE=100audit-fleet.shgql_page_end=$((gql_page_start + MERGED_PR_GRAPHQL_ALIAS_PAGE))audit-fleet.shEvery literal occurs exactly once in the file it binds to, is present at the parent of its reverting commit, and is absent at the reverting commit — the discrimination a marker must have. Both new literals were additionally swept commit-by-commit and are present at every first-parent commit that has touched their file since the restore.
One attribution corrected
The corpus prose above the
9239f1541row credited #2640 with restoringdestructive_guard.py. Measured: #2640 (6f0a31109) touches four files, all underplugins/repo-fleet-hygiene, none underplugins/disk-hygiene. Both markers are absent at the two intervening first-parent commits that touched the file (224b04070#2671,1a612904b#2676) and first appear atde26c27b4(#2706). The note now credits #2706 and records the correction in the file's own established shape. The #2640 credits on thecc58cbc53row are correct —6f0a31109is theaudit-fleet.shrestore — and were left alone.Verification note
The comment changes touch no executable behavior. The marker rows are data the replay reads, so
--verify-restorationis the check that matters and it runs in CI on this PR. The suite was not run locally against this tree — CI is the verification of record.Closes #2917
Closes #2915
Related
attribute_fileswallows blame stderr, so counts are lower boundscleanrows carry no attribution field, so nothing in CI asserts their figures