fix(heartbeat): accept markdown-formatted already-reviewed exits (BLO-31374) - #1613
fix(heartbeat): accept markdown-formatted already-reviewed exits (BLO-31374)#1613kkroo wants to merge 29 commits into
Conversation
…-31374)
evaluatePrReviewCompletionEvidence recognized the reviewer's idempotent
"already reviewed" exit only in the plain shape
`already reviewed at <timestamp> for <sha>`. Two real Ally runs on
2026-09-02 exited cleanly per the one-review-per-(PR, head) contract and
were still classified pr_review_output_missing, flipping Ally to `error`:
- b7a984bf (onprem-k8s#3023):
"**Already reviewed at `8b237675…`** — no action taken."
(sha directly after `at`, bold, backticks)
- 3ace1eef (penstock-vault-node#554):
"**Already reviewed at 2026-09-02T20:41:53Z for `90193c30…`**"
(the backtick between `for` and the sha defeats `\bfor\b\s+[0-9a-f]`)
Both were transient_failure_retry wakes carrying a stale head; the
reviewer found an Ally review attesting the live head and correctly did
not post a duplicate verdict.
Replace the inline regex with prReviewOutputHasAlreadyReviewedSkip:
tolerates backticks/bold/italics between tokens, accepts the sha either
after `for` or directly after `at` (optionally after a `head`/`commit`
noun), still requires a 7–40 hex sha (a sha-less claim stays `missing`),
and rejects a negated clause ("not already reviewed at …").
Tests: both real run texts verbatim, a `head <sha>` variant, and two
masking guards (no sha; negated). Mutation-tested: restoring the old
regex fails the three new shape tests; dropping the negation guard fails
the negation test; loosening the sha to {0,40} fails the no-sha test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-31374 |
1 similar comment
|
🔗 Paperclip issue: BLO-31374 |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Hey @kkroo! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 870f737
The diagnosis is right and the two verbatim run texts now classify correctly — I re-implemented prReviewOutputHasAlreadyReviewedSkip verbatim and confirmed all five new cases behave as the tests assert, the legacy plain shape still matches, and the pattern is linear-time (20 KB of adversarial input in 0.8 ms, so no ReDoS from the nested optional groups). Both findings below are about the compensating guard, not the widening itself.
Critical Issues (0)
Important Issues (2)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9385— the negation guard is defeated by exactly the markdown formatting this PR exists to tolerate. The prefix group is(?:not|never|wasn't|…)\s+, a bare\s+, while every other token junction in the pattern uses${md}([\s\_]). So a bold/italic/backticked negation slips past the guard and the clause matches fromalready`:input old regex new ``This head was not already reviewed at ```` falsetrueIt was *not* already reviewed at `<sha>`.falsetrueHas not yet been already reviewed at <sha>.falsetrueThat is a false
already_reviewedon a run that says the opposite — the masking case the guard was added for. The comment atheartbeat.ts:9380states the guarantee unconditionally ("a negated clause … is rejected"), so it currently overstates what holds. Note the old regex rejected all three (it requiredfor+ whitespace + sha), so this is a narrow regression, not a pre-existing hole.- Use the same markdown class you already built:
\\s+→[\\s\_]+in the negation prefix (keep+, not${md}'s, sonotalreadycan't match). Then add anotcase to the guard test atserver/src/tests/heartbeat-context-summary.test.ts:776` — it only exercises the unformatted form today, which is why the mutation testing didn't surface this.
- Use the same markdown class you already built:
-
[code / native-codex]
server/src/services/heartbeat.ts:9420— the widened clause now accepts hedged and prior-head narration that the oldfor-anchored regex rejected, and the guard only covers explicit leading negation words:narration from a run that did not post old new I could not confirm whether this head was already reviewed at `<sha>`; the API call failed…falsetrueUnclear if already reviewed at `<sha>`. Aborting before the post step.falsetrueThe prior head was already reviewed at `<sha>`, but the branch moved and I did not post…falsetrueThis matters because the file already establishes that this class must stay
missing— see the sibling test "does not accept generic verifier text" (heartbeat-context-summary.test.ts:710,"Could not verify posted Ally review for head abc123."), and the posted-review branch guards itself withprReviewOutputHasPostedReviewNegation(heartbeat.ts:9247) for precisely these shapes. Thealready_reviewedbranch has no equivalent veto.- Add
&& !prReviewOutputHasPostedReviewNegation(text)to the branch atheartbeat.ts:9420. I checked this against both real run texts: it vetoes neither (b7a984bf→false,3ace1eef→false, including its"No review posted, no PR state touched."tail), and it does catch thedid not postrow above. It won't catchcould not confirm whether …orUnclear if …, so a hedge cue (unclear|unsure|could not confirm whether|checking whether) near the clause is worth adding alongside it.
- Add
Suggestions (2)
- [types]
server/src/services/heartbeat.ts:9385— a named group(?<negated>…)withif (m.groups?.negated) continue;would let the loop at:9392read on its own;m[1]currently needs the block comment to explain it. - [tests] The two guard tests assert the status only. Since both findings above are single-predicate behaviours, a small direct table over
prReviewOutputHasAlreadyReviewedSkip(exported or via an internal test entry point) would pin the shapes more cheaply than routing each throughevaluatePrReviewCompletionEvidence.
Strengths
- The block comment at
heartbeat.ts:9366-9380is the right kind: it names the two real runs, quotes the three accepted shapes, and — unusually — explains why the clause can't be anchored to the wake head (the stale head is the whole bug). That reasoning is what stops a future reader from "hardening" this back into the broken form. - Test fixtures are verbatim real run output rather than synthesised strings, so they'll keep failing if the reviewer's phrasing is what actually regresses.
- Keeping the 7–40 hex sha mandatory is the right line to hold — it's what keeps a bare "already reviewed" claim in
missing, and the accompanying guard test locks it in. - Reported mutation testing (restore old regex / drop negation guard / loosen the sha quantifier) is a genuinely stronger signal than a green suite.
Recommended Action
- No Critical issues — nothing blocks merge on correctness of the fix itself.
- Address both Important issues this cycle: they weaken the very gate this function guards, and the fix for each is a one-line predicate change plus a regression test.
- Consider the Suggestions opportunistically.
…d clauses (BLO-31374) Per Ally review of #1613 (two Important findings on the compensating guard, not on the widening itself): - The negation prefix used a bare `\s+`, so `**not** already reviewed at <sha>` and `*not* already reviewed …` slipped past the guard the widened shape was tolerating markdown for. The prefix now uses the same `[\s`*_]+` class (`+`, so `notalready` is not a negation) and also absorbs `yet`/`been`. - The widened clause accepted hedged and prior-head narration the old `for`-anchored regex rejected ("could not confirm whether … already reviewed at <sha>", "Unclear if already reviewed at <sha>", "the prior head was already reviewed at <sha> … I did not post"). The helper now returns false when prReviewOutputHasPostedReviewNegation fires — it vetoes neither real run text — and skips a clause preceded in the same sentence by a hedge cue (unclear / unsure / uncertain / not sure / whether / if). - Suggestion taken: the negation capture is a named group. Tests: four negation forms, three hedge/prior-head forms, and a precedence case where a hedge in an earlier sentence must not veto a later unhedged clause. Mutation-tested: reverting the prefix to `\s+` fails the bold and italic cases; removing the posted-negation veto fails the did-not-post case; removing the hedge veto fails both hedge cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Both Important findings addressed in
Mutation-tested: reverting the prefix to bare |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b479ef0
Both prior Important findings are genuinely fixed. I re-implemented prReviewOutputHasAlreadyReviewedSkip verbatim at this head and confirmed all 13 asserted cases behave as the tests claim, the legacy plain shape still matches, and the pattern is still linear (34 KB of adversarial input in ≤2 ms, so the added optional groups introduce no ReDoS). m.index typechecks: TS 5.7 has matchAll yield RegExpExecArray, whose index is required.
The two findings below are in the newly added compensating guards, and both fail in the same direction as BLO-31374 itself — a clean idempotency exit classified pr_review_output_missing, which the comment at heartbeat.ts:9262 notes is not on the auto-retry allowlist. Neither is a regression against master.
Prior Findings Dispositioned (2)
- prior:870f737 important 1 — fixed —
server/src/services/heartbeat.ts:9391— the negation prefix now uses[\s\_]+instead of a bare\s+, keeping+rather than${md}'ssonotalreadystill cannot match. All three shapes I reported reject at this head:not→false,not→false,not yet been→false; a backticked ``not`` also rejects. Regression rows added atserver/src/tests/heartbeat-context-summary.test.ts:780`. - prior:870f737 important 2 — fixed —
server/src/services/heartbeat.ts:9398—prReviewOutputHasPostedReviewNegationis now consulted for this branch, plus a hedge-cue veto at:9404. All three narrations I reported reject:could not confirm whether→false,Unclear if→false,prior head … did not post→false. Regression rows atheartbeat-context-summary.test.ts:793-795.
Critical Issues (0)
Important Issues (2)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9398— theprReviewOutputHasPostedReviewNegationveto is semantically inverted for this branch. Its first arm matchesdid not|didn't|could not|unable to … post(ed|ing)?, and not posting is the defining property of an already-reviewed exit — so narration that states the one-review-per-head contract correctly is vetoed. It is also evaluated against the whole text before the loop, so distance from the clause does not help. All five realistic phrasings I probed misclassify:narration from a run that correctly skipped new **Already reviewed at `<sha>`** — I did not post a second review.missingAlready reviewed at `<sha>`; I didn't post again.missingAlready reviewed at `<sha>`. Did not post a duplicate verdict.missingAlready reviewed at `<sha>` — could not post a duplicate; contract forbids it.missingAlready reviewed at `<sha>`; unable to post a second verdict on the same head.missingI recommended this veto at
870f737and checked it against both real fixtures — but they phrase the skip as "Posting again would be a duplicate verdict" and "No review posted", neither of which trips the pattern, so the fixtures gave a false all-clear. My recommendation was wrong as stated.- The signal that actually distinguishes the case it was added for (
test:795) is the stale/prior head, not the absence of a post — that fixture says "the prior head was … but the branch moved". Drop the barepost(?:ed|ing)?arm for this branch and veto on a prior/stale-head cue instead (\b(?:prior|previous|earlier|stale)\s+head\b, or\bbranch\s+moved\b), keeping theverify/confirm+ posted-review arm. Then add the five rows above as regression cases.
- The signal that actually distinguishes the case it was added for (
-
[code / native-codex]
server/src/services/heartbeat.ts:9404— the hedge veto uses[^.\n]as its sentence boundary, so only a period ends a sentence: a clause joined by:,;,,or—inherits a hedge cue from up to 60 characters back. Combined with bare\bwhether\band\bif\b— broader than the anchored cues I suggested — 6 of the 7 joiner variants I probed misclassify:narration new Checked whether a prior review exists: already reviewed at `<sha>`.missingsame with ,/—/;as the joinermissingSkipping the post step if a review exists; already reviewed at `<sha>`.missingDetermining if this is a duplicate: already reviewed at `<sha>`.missingChecked whether a prior review exists. Already reviewed at `<sha>`.already_reviewedThe new precedence test at
heartbeat-context-summary.test.ts:806passes only because it uses a period — swap its.for a:and it fails, so the test pins the happy path rather than the boundary it names.- Anchor the cue to a hedge that actually governs the clause — e.g.
(?:unclear|unsure|uncertain|not\s+sure)\s+(?:if|whether)\bplus(?:could\s+not|couldn['’]?t|cannot)\s+\w+\s+(?:if|whether)\b— rather than a barewhether|ifanywhere in the window. Both hedged fixtures attest:793-794still reject under that form. Add a:-joined variant oftest:806so the boundary is pinned.
- Anchor the cue to a hedge that actually governs the clause — e.g.
Suggestions (2)
- [tests]
server/src/__tests__/heartbeat-context-summary.test.ts:803— the comment claims the test guards "a barenotalreadyis not a negation", but no case exercises it; the test only covers the hedge-precedence half. The behaviour is correct (I verifiednotalready→false), so this is a coverage gap rather than a bug — but loosening the+atheartbeat.ts:9391back to*would not fail the suite. One extra row closes it. - [code]
server/src/services/heartbeat.ts:9391— the negation alternation coverswasn't|isn't|hasn'tbut notweren't|aren't, so a plural subject ("These heads weren't already reviewed at<sha>") reads as a positive match. Contrived for a single head, but two tokens to close.
Strengths
- Both prior findings were fixed with the minimal predicate change plus a regression case each, and the new fixtures are verbatim real run output rather than synthesised strings — so they keep failing if the reviewer's actual phrasing is what regresses.
- The block comment at
heartbeat.ts:9366-9385now enumerates all three vetoes and why each exists, and still explains why the clause cannot be anchored to the wake head (the stale head is the whole bug). That reasoning is what stops a future reader from "hardening" this back into the broken form. - Holding the 7–40 hex sha mandatory remains the right line, and the sha-less guard at
test:767locks a bare "already reviewed" claim intomissing. - The named group
(?<negated>…)withif (m.groups?.negated) continueadopts the prior review's type suggestion; the loop now reads without needing the block comment to explainm[1].
Recommended Action
- No Critical issues. The core widening is correct, and the net change is a clear improvement on
master: it fixes two observed production misclassifications and both prior findings. - Address the two Important issues this cycle — both sit in the compensating guards and both reintroduce the exact failure this PR removes, for phrasings that are plausible rather than observed. Each is a one-predicate change plus regression rows. Landing now and following up is a defensible call; I am flagging rather than approving because the guards as written are over-broad in the direction that costs an Ally
error. - Consider the Suggestions opportunistically.
…t; scope hedge to the governing clause (BLO-31374) Second Ally pass on #1613, both findings in the compensating guards added by the previous commit: - prReviewOutputHasPostedReviewNegation was semantically inverted for this branch: not posting is the defining property of an already-reviewed exit, so "Already reviewed at <sha> — I did not post a second review" fell to pr_review_output_missing. Both real fixtures happened to phrase the skip without tripping it, which is why the earlier check gave a false all-clear. Replaced by a prior/stale-head cue scoped to the clause's own sentence (before: "the prior head was already reviewed at …"; after: "…, but the branch moved"), which is the signal that actually distinguishes the fixture the veto was added for. The real b7a984bf text mentions a "stale head" in a different sentence and still classifies. - The hedge veto used a period as the only sentence boundary and bare `whether|if` as the cue, so "Checked whether a prior review exists: already reviewed at <sha>" (or with , ; —) inherited the hedge. The cue now has to govern the clause ("unclear whether", "could not confirm whether") and the scope ends at . : ; , — – or a newline. - Suggestions: weren't/aren't added to the negation alternation; a glued `notalready` row documents that it is neither a negation nor the clause. Tests: Ally's five did-not-post phrasings and seven joiner variants as positive rows, plus the plural negation and glued-token rows (118 in file). Mutation-tested: removing the prior-head veto fails the prior-head fixture; loosening the cue to bare whether|if fails the joiner rows; reverting the boundary to period-only fails the colon/comma/dash rows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tly (BLO-31374) The joiner rows added in the previous commit passed with either guard change alone (a tight boundary hides a loose cue and vice versa), so the previous commit message overstated what its mutation testing showed for those two. These rows discriminate them: a bare `whether`/`if` in the same clause that does not govern the review clause (fails when the cue is loosened to bare whether|if), and a governing hedge about something else in the previous clause joined by ; : , (fails when the boundary reverts to period-only). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Second pass addressed in
One correction to my own record: |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7eadad1
Both prior Important findings are genuinely fixed. I re-implemented prReviewOutputHasAlreadyReviewedSkip verbatim at this head and confirmed all 31 asserted cases behave as the tests claim, the legacy plain shape still matches, and the pattern is still linear (40 KB of adversarial input — backtick runs, sha runs, 40 000 repeated not prefixes — in ≤1.4 ms, so no ReDoS).
Both findings below are in the third veto, the prior/stale-head cue, and both fail in the direction of BLO-31374 itself: a clean idempotency exit classified pr_review_output_missing, which the comment at heartbeat.ts:9262 notes is not on the auto-retry allowlist. Neither is a regression against master.
Prior Findings Dispositioned (2)
- prior:b479ef0 important 1 — fixed —
server/src/services/heartbeat.ts:9412—prReviewOutputHasPostedReviewNegationis no longer consulted on this branch; it is replaced by thepriorHeadcue I recommended, and the rationale is recorded at:9386-9389. All five phrasings I reported now classifyalready_reviewed:I did not post a second review→true,I didn't post again→true,Did not post a duplicate verdict→true,could not post a duplicate→true,unable to post a second verdict→true. Regression rows atserver/src/__tests__/heartbeat-context-summary.test.ts:806-812. - prior:b479ef0 important 2 — fixed —
server/src/services/heartbeat.ts:9406—clauseBeforeis now/[^.\n:;,—–]*$/, so:,;,,,—and–all end the scope, and the hedge at:9410is anchored to a governing form rather than a barewhether|if. All seven joiner variants I reported now classifyalready_reviewed(.;:,—), and both bare-ifrows pass. The five non-governing-hedge rows (Unclear whether CI is green; already reviewed at …) also pass, while both original hedged fixtures still reject. Regression rows atheartbeat-context-summary.test.ts:815-826and:829-842.
Critical Issues (0)
Important Issues (2)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9417— the prior-head veto is also applied to the text after the clause, and there it fires on the ordinary way a correct skip explains the stale wake head. Myb479ef0recommendation was to veto on a prior/stale-head cue in thebeforescope — the fixture it exists for (test:795) says "The prior head was already reviewed at …", which thebeforecheck at:9416already catches. Theafterarm is over-reach, and 5 of the 6 realistic phrasings I probed misclassify:narration from a run that correctly skipped new **Already reviewed at `<sha>`** — the wake carried a stale head, superseded by this one.missingAlready reviewed at `<sha>`, the wake head has moved since the original wake.missingAlready reviewed at `<sha>` — the branch moved after the wake, so the wake SHA is not the live one.missingAlready reviewed at `<sha>`; the prior head in the payload is superseded.missingAlready reviewed at `<sha>` (the earlier head `0936fba6` is stale) — no action taken.missingThis is the language the real fixtures use — run
b7a984bfsays "The wake was atransient_failure_retrycarrying a stale head" — and it survives only because that clause happens to land after a.. Two things compound it: the arm carries zero test coverage (I re-ran all 31 asserted cases with theaftercheck deleted: 0 failures, 0 divergences, so nothing pins it), andclauseAfteris/^[^.\n]*/, which ends at.and newline only — so unlikeclauseBeforeit reaches across:,;,,and—for up to 160 characters. That also makes the comment at:9390-9391inaccurate: it states the scope for both cues ends at. : ; , — –.- Drop the
afterarm at:9417-9418. It is unreached by the suite and it re-creates the exact misclassification this PR removes. If you would rather keep it, at minimum giveclauseAfterthe same boundary class asclauseBeforeso the code matches:9390-9391— but note that even scoped to a comma that still vetoes rows 2 and 5 above. Either way, add the first row as a regression case.
- Drop the
-
[code / native-codex]
server/src/services/heartbeat.ts:9399— the clause interior uses bare\s+betweenalready,reviewedandat, while${md}is used at every junction fromatonward. So markdown that closes before the sha is not tolerated, which is the same class of miss the PR exists to fix:shape new **Already reviewed** at `<sha>` — no action taken.missing*Already reviewed* at `<sha>`.missing\`Already reviewed\` at <sha>.missing**Already reviewed at** `<sha>`.(already works)already_reviewedThe observed runs bold the whole clause including the sha, so this is plausible-but-unobserved rather than a live miss — but bolding just the phrase is a natural variant, and the fix is the same substitution already applied elsewhere.
\\s+→[\\s\_]+at both junctions (keep+, not${md}'s, soalreadyreviewedcannot match — the same reasoning already recorded at:9396-9397`). Add the first row as a regression case.
Suggestions (2)
- [code]
server/src/services/heartbeat.ts:9410— the hedge's second arm is(?:could not|couldn't|cannot|can't|unable to|did not|didn't)\s+\w+\s+(?:if|whether), which allows exactly one word beforeif/whether.Cannot determine whether …vetoes correctly, butcould not **fully confirm** whether …andwas not **able to** confirm whether …both slip through toalready_reviewed. Widening\w+to(?:\w+\s+){1,3}closes it; this is the masking direction, so it is worth the two tokens even though the first arm (unclear|unsure|uncertain|not sure) already covers the common hedges. - [tests] The new cases all route through
evaluatePrReviewCompletionEvidence. Since every finding in this PR's review history has been a single-predicate behaviour, a direct table overprReviewOutputHasAlreadyReviewedSkip— exported, or via an internal test entry point — would pin the veto boundaries far more cheaply than constructing a reviewer context per row, and would have made the untestedafterarm visible as a coverage hole.
Strengths
- Both prior findings were fixed with the minimal predicate change plus regression rows, and the fix for important 1 correctly identified that my own recommendation was wrong — the
did-not-postveto was dropped rather than patched, which is the right call. - The block comment at
:9366-9391now records all three vetoes, why each exists, and whydid not postis deliberately not vetoed, citing the second Ally pass. Comments that record a rejected alternative are what stop a future reader from re-introducing it. - The
notalreadyand plural-negation cases attest:848and:856close the two coverage gaps flagged as Suggestions last round, so loosening+back to*at:9398would now fail the suite. - Holding the 7–40 hex sha mandatory remains the right line, and the sha-less guard keeps a bare "already reviewed" claim in
missing.
Recommended Action
- No Critical issues. The core widening is correct and the net change is a clear improvement on
master: it fixes two observed production misclassifications and both prior findings. - Address the two Important issues this cycle. The first is the stronger of the two — an untested veto arm that misfires on the exact narration the real runs produce, removable in two lines. Landing now and following up is defensible; I am flagging rather than approving because both defects cost an Ally
erroron a run that behaved correctly, which is the failure this PR set out to remove. - Consider the Suggestions opportunistically.
…down inside it (BLO-31374) Third Ally pass on #1613, both findings in the prior/stale-head veto: - The veto was also applied to the text AFTER the clause, where it fires on the ordinary way a correct skip explains the wake ("Already reviewed at <sha> — the wake carried a stale head, superseded by this one"). The real b7a984bf text says exactly that and survived only because its clause happens to end at a period. The after-arm had no test coverage and is removed; only a prior-head SUBJECT before the clause vetoes. - The clause interior used bare \s+ between already/reviewed/at while every junction from `at` onward tolerated markdown, so "**Already reviewed** at <sha>" (markdown closing before the sha) missed. Same [\s`*_]+ class now (with `+`, so `alreadyreviewed` cannot match). - Suggestion: the hedge's second arm allows one to three words before if/whether ("could not fully confirm whether", "was not able to confirm whether"). Tests: Ally's five after-clause stale-head phrasings and three markdown-before-sha shapes as positive rows; glued `alreadyreviewed` and two multi-word hedges as negative rows (134 in file). Mutation-tested: restoring the after-arm fails 5 rows; reverting the interior to \s+ fails 3; reverting the hedge arm to one word fails 1. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Third pass addressed in
File 134/134, sibling suites 167/167, server typecheck clean. |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 18efabe
Both prior Important findings are genuinely fixed. I re-implemented prReviewOutputHasAlreadyReviewedSkip verbatim at this head and confirmed all 42 asserted cases behave as the tests claim, the legacy plain shape still matches, and the pattern is still linear (400 KB of adversarial input — 40 k backtick runs, 20 k repeated not prefixes, a 41-hex blob forcing lookahead backtracking — in ≤8.2 ms, so no ReDoS from the added groups).
CI is green on this head except review/ally-comment, which is this gate: "An unresolved finding from Ally's review of 7eadad1 is still undispositioned." That is exactly the set dispositioned below.
The one finding below flips direction from the previous three rounds. Those were all false missing (a clean skip flagged as an Ally error). This one is the masking direction — a run that did not post classified already_reviewed, so the missing-review signal is suppressed and the run stays succeeded. Unlike the previous findings it is a regression against master.
Prior Findings Dispositioned (2)
- prior:7eadad1 important 1 — fixed —
server/src/services/heartbeat.ts:9420— theafterarm is gone. The loop now computes onlybeforeand testshedge/priorHeadagainst it; there is noclauseAfterin the function at this head. All five phrasings I reported now classifyalready_reviewed: stale-head-superseded →true,wake head has moved→true,branch moved after the wake→true,prior head in the payload is superseded→true,earlier head … is stale→true. Regression rows added atserver/src/__tests__/heartbeat-context-summary.test.ts:855, and the comment at:9391-9393now correctly describes abefore-only scope. - prior:7eadad1 important 2 — fixed —
server/src/services/heartbeat.ts:9405— the clause interior is now\balready[\s\_]+reviewed[\s`_]+at, so both junctions use the markdown class instead of a bare\s+, and+is kept. All three shapes I reported now match:Already reviewed at ``→true,Already reviewed at ``→true, ``Already reviewedat <sha> `` →true, whilealreadyreviewedstill rejects. Regression rows atheartbeat-context-summary.test.ts:865and:869`.
Critical Issues (0)
Important Issues (1)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9404— the negation guard only recognises a negation adjacent toalready, so the ordinary non-adjacent way of saying the same thing reachesalready_reviewed. Thenegatedgroup is a prefix immediately beforealready(plus up to twoyet|been); a negation that governs the clause from the subject position never touches it, and unlikehedge/priorHeadthere is nobefore-scope negation cue. All four phrasings I probed misclassify, and all four arefalseonmaster:narration from a run that did not post masterthis head There is no evidence this head was already reviewed at `<sha>`, so I posted a fresh verdict.falsetrueI do not believe this head was already reviewed at `<sha>`.falsetrueI cannot see that this head was already reviewed at `<sha>`.falsetrueFound no review; nothing indicates this head was already reviewed at `<sha>`.falsetrueThese are close to the distance the guard already handles well (35–49 chars, comfortably inside the scope) — the gap is grammatical, not one of distance. That the adjacent form is guarded (
test:783) establishes this class as in scope; only one grammatical form of it is covered. The masking cost is asymmetric: a falsemissingproduces a visible Allyerror, whereas this produces asucceededrun with no review on the PR and nothing flagging it. The BLO-22892/BLO-28203 request sweep would eventually re-request, so it degrades rather than loses the signal.- Add a
before-scope negation cue alongsidehedgeandpriorHead— the mechanism is already there, so it is one regex plus one||:const negation = /\b(?:no|not|never|nothing|neither|cannot|can['’]t)\b[^.\n:;,—–]*$/i, thenif (hedge.test(before) || priorHead.test(before) || negation.test(before)) continue;. Notecannot/can'tmust be listed explicitly —\bnot\bdoes not match insidecannot, which is why row 3 survives ano|not|neveralternation. I ran this against all 42 asserted cases: 0 regressions, and it rejects all four rows above. Add row 1 as a regression case atheartbeat-context-summary.test.ts:783.
- Add a
Suggestions (3)
- [code / native-codex]
server/src/services/heartbeat.ts:9420— thebeforeslice is capped atm.index - 120, but the comment at:9393states the scope "ends at . : ; , — – or a newline" with no mention of a character cap. So the code is narrower than its documented contract, and past 120 boundary-free characters both vetoes silently stop applying (measured: a hedge at distance 145 escapes; the same sentence at 42 vetoes). I am filing this as a Suggestion rather than Important because I could not produce a natural instance — three realistic reviewer sentences all landed at 95–115 chars, since commas are themselves boundaries, so a comma-free 120+ char span needs a run-on. Do not fix this by dropping the cap:clauseBefore.execon an uncapped prefix is quadratic (I measured 3.97 ms → 57 ms → 971 ms → 14.9 s at 2/8/32/128 KB with a trailing boundary). AnO(n)backwardlastIndexOfscan over the six boundary chars is exact and faster than the current code (4.1 ms on a 200 KB prefix), with 0 regressions on the 42 cases. Cheapest alternative: leave the cap and say120in the comment. - [code]
server/src/services/heartbeat.ts:9405—at${md}usesmd's*, soAlready reviewed at8b237675…(no separator at all) matches. Contrived, butat[\s\_]+closes it and is the same+-not-reasoning already recorded at:9398-9400; I verified all seven real shapes — includingat ``andat head— still match under+`. - [tests] Carried from the two previous rounds and still open: every case routes through
evaluatePrReviewCompletionEvidence, constructing a reviewer context per row. All six findings across this PR's review history have been single-predicate behaviours, and both findings I dispositioned above were untested arms. A direct table overprReviewOutputHasAlreadyReviewedSkip— exported or via an internal test entry point — is how the veto boundaries get pinned cheaply; it is also what would have surfaced the negation gap above, since the gap is visible in one line of table.
Strengths
- Both prior findings were fixed by the minimal change — the untested
afterarm was deleted rather than patched, which was the recommendation and the right call. Three rounds running, this PR has fixed the flagged predicate rather than papering over it. - The block comment at
:9366-9395is now a genuine decision record: it enumerates all three vetoes, why each exists, and the two alternatives that were tried and rejected (prReviewOutputHasPostedReviewNegationinverting the branch, theafterarm over-reaching), each attributed to the pass that found it. Comments that record rejected alternatives are what stop a future reader from re-introducing them. - The hedge's
\w+→(?:\w+\s+){1,3}widening picked up last round's Suggestion, andtest:881pins both multi-word forms. - Test fixtures remain verbatim real run output rather than synthesised strings, so they keep failing if the reviewer's actual phrasing is what regresses.
- Holding the 7–40 hex sha mandatory remains the right line, and the sha-less guard at
test:767keeps a bare "already reviewed" claim inmissing.
Recommended Action
- No Critical issues. The core widening is correct and this head is a clear improvement on
master: it fixes two observed production misclassifications plus all six findings raised across the prior three rounds. - Address the one Important issue this cycle — one regex and one
||, verified against the full suite. I am flagging rather than approving because it is the first regression againstmasterin this PR's history and it fails silently, which is the harder failure to notice. Landing now and following up is a defensible call given the direction of travel. - Consider the Suggestions opportunistically — the first carries a measured perf trap worth reading before touching that line.
…viewed clause (BLO-31374) Fourth Ally pass on #1613, masking direction: the negation guard only recognised a negation adjacent to `already`, so "There is no evidence this head was already reviewed at <sha>, so I posted a fresh verdict" / "I cannot see that this head was already reviewed at <sha>" reached `already_reviewed` (both false on master). A before-scope negation cue now sits alongside hedge/priorHead in the same clause scope; cannot/can't are listed explicitly because \bnot\b does not match inside cannot. Also: `at` is followed by [\s`*_]+ not *, so `at<sha>` with no separator is not the clause; the block comment now states the 120-character scope cap and why it is kept (an uncapped scan is quadratic on long outputs); prReviewOutputHasAlreadyReviewedSkip is exported and a direct 17-row table pins each veto boundary in one line. Tests: Ally's four non-adjacent phrasings as negative rows, a previous-clause negation as a positive row, the direct table (156 in file, 189 across sibling suites). Mutation-tested: removing the negation cue fails 6 rows; reverting the `at` separator to * fails 1. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fourth pass addressed in
File 156/156, sibling suites 189/189, server typecheck clean. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f4c3a57
The prior Important finding is genuinely fixed. I re-implemented prReviewOutputHasAlreadyReviewedSkip verbatim at this head and confirmed all 51 asserted cases behave as the tests claim, the legacy plain shape still matches, and the pattern is still linear (512 KB of adversarial input — backtick runs, 128 k repeated not prefixes, a boundary-free hex blob — in ≤20 ms, so no ReDoS from the added cue).
The one finding below is in the newly added negation cue and flips direction back from the previous round. That one was masking; this one is false missing — a clean idempotency exit classified pr_review_output_missing, which the comment at heartbeat.ts:9262 notes is not on the auto-retry allowlist, so it flips Ally to error. It is a regression against master for the plain shape, and it partially re-opens the finding fixed at b479ef0.
Prior Findings Dispositioned (1)
- prior:18efabe important 1 — fixed —
server/src/services/heartbeat.ts:9431— abefore-scopenegationcue now sits alongsidehedgeandpriorHeadand is applied at:9435, exactly the recommended mechanism. All four phrasings I reported now classifymissing:no evidence …→false,I do not believe …→false,I cannot see that …→false,nothing indicates …→false.cannotis listed explicitly, so row 3 is caught. Regression rows atserver/src/__tests__/heartbeat-context-summary.test.ts:892-896; I confirmed the arm is load-bearing by deleting it (4 of the 51 cases fail).
Critical Issues (0)
Important Issues (1)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9431— thenegationcue is a bare word-list (\b(?:no|not|never|nothing|…|without)\b) tested against the whole before-clause, whereas the siblinghedge(:9424) andpriorHead(:9426) both require a governing form. Becausenot/no/withoutare also how a correct skip explains why it did not post, any such wording in the same clause vetoes the match. All 10 realistic phrasings I probed misclassify:narration from a run that correctly skipped masterthis head Exiting without posting since this head was already reviewed at `<sha>`.true*missingI did not post a duplicate because this head was already reviewed at `<sha>`.true*missingThere is no need to post again because this head was already reviewed at `<sha>`.true*missingNo action taken because this head was already reviewed at `<sha>`.true*missingNothing to do here because the head was already reviewed at `<sha>`.true*missing*measured in the master-compatible
at <ts> for <sha>shape, wheremasterreturnstrueand this head returnsfalse— 5/5 regressions. The other five (I don't need to post…,The contract does not permit…,I cannot post a second verdict…,No second verdict is needed…,Skipping — no duplicate verdict is permitted and…) fail the same way.Two things make this more than a hypothetical.
Exiting without postingis verbatim real run output — it is the opening line of fixture3ace1eef(test:748), and the same phrasing appears in an unrelated self-review fixture attest:1263("so self-review is not allowed. Exiting without posting a review on …"), so it is the house phrasing for this class of exit rather than one author's turn of phrase. It survives today only because those lines carry no sha; moving the sha onto one, or joining the two clauses, breaks it. Anddid not postwas already litigated: the second-pass finding atb479ef0removedprReviewOutputHasPostedReviewNegationfrom this branch precisely because not posting is the defining property of this exit, and the comment at:9385-9388records that. The barenotre-introduces it for pre-clause word order — the five rows attest:807-811pin only the post-clause order, which is why the suite stays green.The comment at
:9427-9430says "A negation governing the clause" and gives only epistemic examples (no evidence …,cannot see that …), so it describes the intent rather than what the code does — the same over-statement flagged at870f737. All four negation tests (test:892-896) are epistemic too, andtest:904(the only "negation must not veto" case) puts the negation behind a;, so it is out of scope and cannot catch this.- Anchor the negation to an epistemic head, mirroring how
hedgeis anchored:const negation = /\b(?:no|not|never|nothing|neither|nor|cannot|can['’]t|doesn['’]t|don['’]t|isn['’]t|wasn['’]t)\s+(?:\w+\s+){0,3}(?:evidence|indication|indicates?|indicating|sign|signs|record|proof|trace|believe|think|see|seen|appear|appears|suggest|suggests|confirm|confirms|confirmed|aware)\b/i;. I ran this against all 51 asserted cases: 0 regressions, and it restores all 10 rows above. Note the narrower list I recommended last round (no|not|never|nothing|neither|cannot|can't) does not fix this — it still fails 8 of the 10, so my own recommendation was under-specified as stated. Dropwithoutregardless: it is a preposition, not a clause negation. Then addExiting without posting since …as a regression row attest:904, which is the case shape that would have caught this.
- Anchor the negation to an epistemic head, mirroring how
Suggestions (2)
- [code]
server/src/services/heartbeat.ts:9424— the hedge's second arm listscould not|couldn't|cannot|can't|unable to|not able to|did not|didn'tbut notfailed to, soI failed to confirm whether this head was already reviewed at `<sha>`; aborting.classifiesalready_reviewed— the masking direction.I was unable to determine whether …is correctly vetoed, so this is a one-token gap in an otherwise-covered class rather than a new hole. Addingfailed\s+tocloses it. - [tests]
server/src/__tests__/heartbeat-context-summary.test.ts:5— the new import shares a line withevaluatePrReviewCompletionEvidenceand sits out of the otherwise-alphabetical order (it belongs aftermergeCoalescedContextSnapshot). No formatter or lint config is present at the repo root and nolint/formatscript exists, so this will not fail CI — purely cosmetic.
Strengths
- The prior finding was fixed with exactly the recommended mechanism and the minimal change — one regex plus one
||— and the rationale was added to the block comment attributed to the pass that found it. Four rounds running, this PR has fixed the flagged predicate rather than papering over it. - All three Suggestions from the previous round were picked up: the direct predicate table now exists (
test:913, 17 rows pinning each veto boundary in one line — the single highest-value test in the file),at${md}becameat[\s\*_]+soat8b237675…no longer matches, and the 120-character cap is now documented in the comment at:9399-9402` rather than silently contradicting it. - The block comment at
:9366-9403is a genuine decision record: three vetoes, why each exists, the two alternatives tried and rejected, and the measured reason the cap stays (an uncapped scan is quadratic). Comments that record rejected alternatives are what stop a future reader from re-introducing them. - Test fixtures remain verbatim real run output rather than synthesised strings, so they keep failing if the reviewer's actual phrasing is what regresses.
- Holding the 7–40 hex sha mandatory remains the right line, and the sha-less guard keeps a bare "already reviewed" claim in
missing.
Recommended Action
- No Critical issues. The core widening is still correct and this head fixes both observed production misclassifications plus all six findings from the prior three rounds.
- Address the one Important issue this cycle — one regex, verified against the full suite. I am flagging rather than approving because it is a regression against
master, it partially re-opens a finding this PR already fixed once, and it fails in the direction that costs an Allyerroron a run that behaved correctly, which is the exact failure BLO-31374 set out to remove. - Consider the Suggestions opportunistically.
Fifth Ally pass on #1613: the before-scope negation cue added last round was a bare word list, so the house phrasing of a CORRECT skip — "Exiting without posting since this head was already reviewed at <sha>" (verbatim opening of fixture 3ace1eef), "No action taken because …", "I did not post a duplicate because …" — fell to pr_review_output_missing (10 of 10 probed phrasings; 5 regress against master in the plain shape). It also partially re-opened the b479ef0 finding that "did not post" must never veto this exit. The cue is now anchored the way `hedge` is: a negation word followed within three words by an epistemic head (evidence / indication / believe / see / confirm / aware …). `without` is dropped (a preposition, not a clause negation). `failed to` joins the hedge's second arm so "failed to confirm whether …" vetoes like "unable to determine whether …". Tests: the ten skip phrasings as positive rows (two in the plain `at <ts> for <sha>` shape), "failed to confirm whether" as a negative row, three more direct-table rows; test import block alphabetised (170 in file, 203 across sibling suites). Mutation-tested: reverting the cue to the bare word list fails 12 rows; removing the cue fails 6; removing `failed to` fails 2. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fifth pass addressed in
File 170/170, sibling suites 203/203, server typecheck clean. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d955c48
Fifth pass. The prior finding is fixed with the exact recommended mechanism, and I re-ran the extracted prReviewOutputHasAlreadyReviewedSkip verbatim from this head against all 64 asserted clause strings plus the real run shapes — the three documented fixtures classify, sha-less claims are still rejected, alreadyreviewed/notalready still do not match, multi-clause texts pick the right occurrence, and the nested quantifiers are linear (100 KB in 0.83 ms, so no ReDoS). The 120-char scope cap also holds up: the three realistic long-form epistemic negations I probed sit 26–68 characters before the clause, well inside the window, so that documented trade-off is sound rather than merely acknowledged. One residual gap in the same veto, in the masking direction.
Prior Findings Dispositioned (1)
- prior:f4c3a57 important 1 — fixed —
server/src/services/heartbeat.ts:9439— thenegationcue is no longer a bare word-list; it is anchored to an epistemic head (evidence|indications?|…|confirms?|aware) followed within three words, exactly ashedgeis anchored, andwithoutis dropped. All 10 phrasings I reported now classifyalready_reviewedagain:Exiting without posting since …,I did not post a duplicate because …,There is no need to post again …,No action taken because …,Nothing to do here because …, and the other five. The epistemic cases it must still veto (no evidence …,I cannot see that …,I do not believe …) remain vetoed, and the new regression row landed attest:931.
Critical Issues (0)
Important Issues (1)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9439— the two vetoes split the "could not establish it" class by complement, not by meaning, so thethat-complement leaks for every verb outside the epistemic list.hedge(:9429) requiresif|whether, so it catchescould not <anything> whether.negation(:9439) catches athat/bare complement only when the verb is one of its epistemic heads. A verb that is neither —verify,establish,determine— satisfies no veto:narration from a run that did NOT establish prior review whetherformthatformI could not confirm … this head was already reviewed at `<sha>`.veto veto I could not verify … this head was already reviewed at `<sha>`.veto leak I could not establish … this head was already reviewed at `<sha>`.veto leak I could not determine … this head was already reviewed at `<sha>`.veto leak I could not find … this head was already reviewed at `<sha>`.veto leak 11 of the 13 verbs I probed leak on the
thatform (verify,establish,determine,find,check,validate,ascertain,prove,locate,demonstrate,tell); onlyconfirmandseehold, because they happen to be in the epistemic list.I have not verified that …andUnable to establish that …leak the same way. This is the masking direction — a run that could not establish prior review is recorded asalready_reviewed, so the PR is marked reviewed and no verdict is ever posted, which is the false-negative loop this PR family exists to close.On the baseline: in the master-compatible
at <ts> for <sha>shape master masks these too, so that shape is not a regression. In the backtick shapes this PR newly accepts, master returnedfalseand this head returnstrue, so the leak is newly reachable there. Worth weighing against the fact that the header comment at:9377states the three vetoes "keep the widened shape from masking a run that did NOT post" — the gap is inside that stated coverage class, not outside it.The suite cannot catch it: every hedge case (
test:922,test:957,:827) usesconfirm+whether, and every epistemic-negation case (test:893-895,:953-954) uses a verb already in the list. Both covered combinations are pinned; the leaking one is untested.- Extend the epistemic head list rather than touching the hedge — the verbs are heads of the same epistemic class, and it keeps the veto's "governing form" anchoring intact. Append to
:9439:verify|verifies|verified|establish|establishes|established|determine|determines|determined|find|finds|found|check|checks|checked|locate|locates|located|ascertain|ascertained|validate|validates|validated|prove|proves|proven|demonstrate|demonstrates|tell. I ran this against all 64 asserted clause strings: 0 changes, all 10 correct-skip phrasings stilltrue, and all 11 leaks closed. I also positive-controlled the risk of over-vetoing, since these verbs appear in affirmative skips too:I checked the reviews API and confirmed it: already reviewed at `<sha>`.andAfter I verified the reviews endpoint, already reviewed at `<sha>` — no action taken.both staytrue, because the veto still requires a negation before the verb. Then addI could not verify that this head was already reviewed at `<sha>`.as a row attest:953, which is the case shape that would have caught this.
- Extend the epistemic head list rather than touching the hedge — the verbs are heads of the same epistemic class, and it keeps the veto's "governing form" anchoring intact. Append to
Suggestions (2)
- [code]
server/src/services/heartbeat.ts:9439—thinkandbelieveare present butthought,believedandrealise/realizeare not, soI did not realize this head was already reviewed …classifiesalready_reviewed. Unlike the finding above this one is arguably correct — that phrasing describes a run that did eventually establish prior review — so treat it as a deliberate boundary to confirm rather than a fix. - [types]
server/src/services/heartbeat.ts:9409— the five regexes are rebuilt on every call, andpatternis constructed vianew RegExpfrom string concatenation on each invocation. Hoisting them to module scope would avoid the recompile, butpatterncarries thegflag and a sharedlastIndex, so it is only safe to hoist alongside the existingmatchAll(which resets per call) — not with.test()/.exec(). Not worth changing unless this becomes hot; the current form is the safer default and the measured cost is negligible.
Strengths
- The escalating veto set is now genuinely well-factored:
hedge,priorHeadandnegationare three orthogonal cues over one shared before-clause scope, each anchored to a governing form rather than a bare word list, and the before/after asymmetry is exactly right — stale-head narration after the clause is how a correct skip explains its wake, andtest:827pins that. - The header comment at
:9366-9408records why each veto exists and, unusually, why two tempting vetoes were rejected (did not post, post-clause stale-head narration) with the review pass that established each. That is the kind of comment that stops the next author from reintroducing an inverted guard, and it now accurately describes the code rather than overstating it — the mismatch I flagged at870f737and again atf4c3a57is gone. - Anchoring acceptance to a 7–40 hex sha instead of the wake head is the right call given the wake head is precisely what goes stale, and rejecting a sha-less claim (
test:55) keeps that from becoming a blanket accept. - 257 lines of table-driven tests for an 84-line change, including the glued-token boundaries (
alreadyreviewed,notalready) and the plural-negation case. Both suggestions from the prior pass landed —failed\s+tois in the hedge at:9429, and the import attest:7is now alphabetical on its own line.
Recommended Action
- No Critical issues.
- Address the Important issue this cycle — one-line list extension at
:9439, validated at 0 regressions across the asserted corpus, plus thetest:953regression row. - Consider the Suggestions opportunistically; the first is a boundary question rather than a defect.
…t (BLO-31374) Sixth Ally pass on #1613, masking direction: the two vetoes split the "could not establish it" class by complement, not meaning. `hedge` needs `if|whether`; `negation` needs an epistemic head. "I could not verify that this head was already reviewed at <sha>" satisfied neither (11 of 13 probed verbs leaked on the `that` form). The epistemic head list now includes the establishing verbs (verify / establish / determine / find / check / locate / ascertain / validate / prove / demonstrate / tell) with their inflections, so the veto's governing-form anchoring is intact and the ten correct-skip phrasings still classify. Tests: seven that-complement rows as negative cases plus two direct-table rows (177 in file, 210 across sibling suites). Mutation-tested: removing the establishing verbs from the list fails 9 rows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…BLO-31374) `check-shard-manifest-freshness` failed the #1613 policy job because 37 suites that landed on master had no entry in scripts/general-server-shard-durations.json. Backfilled with `node scripts/measure-general-server-shard-durations.mjs --update` (all 37 measured; the two "uncaughtException" lines in the run are the crash-run-marking suite's own deliberate throws). Freshness check now exits 0 locally. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Sixth pass addressed at
Not taken: the |
…BLO-31374) master added server/src/__tests__/managed-checkout-partial-clone.test.ts after the a5ae383 backfill; the freshness check on the merge commit reported it as the one unmeasured suite (1 of 483). Measured with measure-general-server-shard-durations.mjs --update after merging master. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ubt guard
Two Important findings from Ally's eleventh pass, both in the false-`missing`
direction this PR exists to remove.
1. `COPULA_EDGE`'s filler was a bare `(?:\w+\s+){0,4}`, and `\w+` matches
`so`/`but`/`and`/`because` -- the very words the design comment names as
the discriminator for a correct skip. The filler swallowed them, so a
negation governing a DIFFERENT noun phrase reached the clause across the
connective: "No newer commits were found so this head was already reviewed
at ..." was classified `missing` (5 of 5 measured rows regressed against
master). The filler is now `COPULA_FILLER`, which excludes the connectives,
so the code holds the invariant the comment already documented.
2. The `doubt` negative lookbehind was adjacency-only, inspecting exactly one
token, so any intervening adjective defeated it -- and an adjective there is
the ordinary phrasing, with "beyond reasonable doubt" a stock idiom. It now
spans up to two intervening words (6 of 6 measured rows regressed).
Both Suggestions taken:
- The two counts in the design comment went stale when `priorHead` joined the
table; they now read six cues total and five clause-scoped, matching
GOVERNING_CUE_STEMS.
- `GOVERNING_CUES[].name` was carried but never read. It now backs an exported
`prReviewAlreadyReviewedVetoCue`, so attributing a veto is one call instead
of the by-hand bisection every one of these eleven passes has needed. A
14-row attribution table pins one representative clause per cue plus five
correct skips that no cue governs.
Mutation-tested, each mutant killed by a distinct row set:
doubt lookbehind back to adjacency-only -> 7 failures
copula filler back to bare `\w+` -> 8
attribution always reports first cue -> 8
connective list loses `so` -> 6
343 tests pass across the five heartbeat suites; typecheck clean.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Eleventh pass addressed at Important 2 was the significant one — the filler was quietly defeating the design's own discriminator. Important 1 — the All 13 of your measured rows are asserted, including the Suggestion 1 — the two stale counts now read six cues total and five clause-scoped, matching Suggestion 2 — worth doing, and it paid for itself immediately. Mutation-tested; each mutant killed by a distinct row set:
343 tests pass across the five heartbeat suites; typecheck clean. Noting the two incidental escapes you flagged rather than leaving them implicit: 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 382d96f
Both findings from the previous pass are fixed, applied verbatim as recommended. I re-ran this PR's own asserted tables against the head build: 65/65 boundary cases and 14/14 attribution cases pass, 0 failures. The new prReviewAlreadyReviewedVetoCue attribution table is a genuine improvement — it made this review's cue bisection a single call instead of the by-hand work the previous ten passes each needed.
One Important finding remains, and I want to be precise about what it is and is not.
Prior Findings Dispositioned (2)
- prior:46f2435 important 1 — fixed —
server/src/services/heartbeat.ts:9582— thedoubtlookbehind now carries the bounded filler(?:\w+\s){0,2}, exactly as recommended. Re-measured againstmasteron the plain shape: all 6 of 6 reported regressions restored totrue(no real doubt,no serious doubt,beyond reasonable doubt,without any doubt,no genuine doubt,not the slightest doubt), while the speaker's-doubt vetoes still holdfalse(I doubt,some doubt,doubtful). - prior:46f2435 important 2 — fixed —
server/src/services/heartbeat.ts:9535—COPULA_FILLERnow excludes the connectives via negative lookahead, so the code holds the invariant the comment at:9517-9528documents. All 5 of 5 reported regressions restored totrue, and I confirmed the untested variants behave identically:but,and,because,since,thereforeall restored. Every legitimate copula-arm veto still fires (no evidence that,cannot see that,cannot say,The prior head was,Assuming,Possibly).
Critical Issues (0)
Important Issues (1)
-
[code / native-codex]
server/src/services/heartbeat.ts:9579— bare belief and appearance hedges have no cue, so they reach the clause unvetoed. Thenegationlist at:9613already containsbelieve|think|appears|suggests|aware|see|seen, but only reachable after a negation word — sono indicationandcannot seeare caught while the bare hedging form is not.assumptionat:9579covers the adverbs (possibly|probably|presumably|apparently) but no belief predicate. Measured at this head, 12 phrasings of a run that did NOT establish the claim are classifiedalready_reviewed:before-clause this head with fix It appears this head was …truefalseIt seems likely this head was …truefalseI believe this head was …truefalseI think this head was …truefalseI suspect this head was …truefalsePerhaps this head was …truefalseMaybe this head was …truefalseIt looks like this head was …truefalseIt is plausible this head was …truefalseThere is no confirmation this head was …truefalseI cannot rule out that this head was …truefalseThis is a widening, not a veto regression — and the distinction matters for how you weigh it.
masterhas no vetoes at all, so on the plainalready reviewed at <ts> for <sha>shape it accepts all 12 too. The risk is specific to the shapes this PR newly admits: dropping the mandatoryforanchor and adding the markdown /head-noun forms meansmaster's narrower pattern incidentally rejected this text, and this head does not. Measured across the three newly-admitted shapes, 36 of 48 hedged phrasings are accepted wheremasterrejected them. The consequence is the failure this machinery exists to prevent: a run that never posted exitsalready_reviewedand the PR is silently never reviewed.- Add the belief predicates to
assumption(they inheritCLAUSE_REACHautomatically, so they stay clause-bound like every other stem), and add the two noun/verb gaps tonegation::9579—|perhaps|maybe|likely|plausible|seem(?:s|ed)?|appears?|believ(?:e|es|ed)|think(?:s)?|thought|suspect(?:s|ed)?|looks?\s+like:9613—|confirmation|verification|determination|rule\s+out
- Verified: 0 failures on all 65 boundary + 14 attribution cases, 11 of the 12 rows above closed, and the connective/doubt behaviour from both prior findings unchanged. The residual
My assumption is that this head was …is the noun form ofassumingand is the one row this does not close.
Structural note, offered rather than asserted. This is the eleventh pass and the twelfth cue-list extension; each pass has closed its predecessor's list and found one more. That is weak evidence the enumeration does not converge, because the veto side has to anticipate an open set of English hedges while the accept side needs only one phrasing to slip. If you would rather stop here, a defensible alternative is to make the accept side carry the burden — require a positive evidence marker near the clause (the review id,
commit_id, or an explicit "verified"/"confirmed" token) rather than accepting any sha-anchored clause that no veto happens to catch. I am not asking for that in this PR; the fix above is sufficient and small. - Add the belief predicates to
Suggestions (0)
Strengths
- The two prior fixes were applied exactly as recommended, and both are verified correct against the reported tables rather than merely plausible.
prReviewAlreadyReviewedVetoCue(:9632) is the right response to ten passes of hand bisection: it makesGOVERNING_CUES[].nameload-bearing instead of decorative, and the 14-case attribution table pins one representative clause per cue — including four correct-skip rows assertingnull, which is the harder and more useful half.- The design comments are unusually good.
:9526-9531explains why the filler excludes connectives (holding an invariant the prose already claimed), and:9573-9577records that adjacency-only was measured and restored none of the six rows. Both cite the pass that found them, so the next reviewer inherits the reasoning instead of re-deriving it. - The clause-binding rule is genuinely structural:
CLAUSE_REACHis appended at the single point where cues are compiled (:9622-9624), so there is no way to express an unbound cue in the table. That is what turned an open-ended bug class into a closed one. scripts/general-server-shard-durations.jsonis unrelated shard-timing housekeeping; the test file pre-exists onmasterand is already registered there. No issue.
Recommended Action
- No Critical issues. Address the Important finding — it is a two-line change to two existing stem lists, verified against the full suite.
- Consider the structural note only if you want to stop the cue-list treadmill; it is explicitly not a blocker for this PR.
Ally's twelfth pass: the `negation` list already carries believe / think /
appears / suggests, but only reachable AFTER a negation word, so "no
indication" and "cannot see" were caught while the bare hedging form was not.
`assumption` covered the adverbs but no belief predicate. Result: 12 phrasings
of a run that never established the claim exited `already_reviewed` -- the
masking failure this mechanism exists to prevent.
This is a widening to close, not a veto regression. `master` has no vetoes at
all and accepts these too on the plain shape; the exposure is specific to the
shapes this PR newly admits, where master's mandatory-`for` anchor rejected the
text incidentally.
Added to `assumption` (inheriting CLAUSE_REACH, so clause-bound like every
other stem): perhaps / maybe / likely / plausible / seem / appear / believe /
think / thought / suspect / looks like, plus the `assumption` noun form, which
was the one row Ally's own fix left open. Added to `negation`: confirmation /
verification / determination / rule out.
15 new rows, including three over-veto controls where the hedge governs
something other than the review and the skip must stand ("My assumption about
the wake was wrong so ...", "It appears no new commits landed so ...", "This
looks like a duplicate wake so ...").
Mutation-tested, each mutant killed by a distinct row set:
belief/appearance predicates removed -> 9 failures
epistemic nouns + rule-out removed -> 2
`assumption` noun removed -> 1
358 tests pass across the five heartbeat suites; typecheck clean.
Ally's structural note -- that enumerating vetoes may not converge, because
the veto side must anticipate an open set of English hedges while the accept
side needs one phrasing to slip -- is correct and is tracked as BLO-31526
(require a positive evidence marker near the clause instead). It is not done
here because it needs the reviewer agent to emit that marker; landing the
accept-side change alone would fail every real skip closed.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Twelfth pass addressed at Added to I also added three over-veto controls, since widening a stem list is exactly where this goes wrong in the other direction — the hedge governs something other than the review and the skip must stand:
Mutation-tested:
358 tests pass across the five heartbeat suites; typecheck clean. On your structural note — you're right, and I want to be explicit about why I'm still not doing it here. The asymmetry you name is real: the veto side has to anticipate an open set of English hedges, the accept side needs one phrasing to slip. Twelve passes each closing the predecessor's list is the signature of an enumeration that does not converge, and I'd reached the same conclusion independently before this review landed. It is tracked as BLO-31526 — key the classification on a positive evidence marker (review id / What this PR is worth in the meantime: it strictly reduces masking versus its own previous heads, and the false- 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0e8da91
Looks good — approving. The twelfth-pass fix landed exactly as recommended, I reproduced its effect at this head, and the residual I found is a widening with zero veto regressions, not a defect this PR introduces. Detail below, including a measurement that I think should change how the next pass is approached.
Prior Findings Dispositioned (1)
- prior:382d96f important 1 — fixed —
server/src/services/heartbeat.ts:9591-9592(+:9631) — the belief/appearance predicates are inassumption(perhaps|maybe|likely|plausible|seem(?:s|ed)?|appears?|believ(?:e|es|ed)|think(?:s)?|thought|suspect(?:s|ed)?|looks?\s+like, plusassumption), andconfirmation|verification|determination|rule\s+outare innegationat:9631. Re-measured by extracting the cue table from this exact head and executing it: all 12 reported rows now veto — includingMy assumption is that this head was …, the row the finding recorded as not closed (assumptionreaches the clause through theis that this head wascopula edge). All 11 correct-skip boundary phrasings I checked still returntrue, and the PR's own 80-row boundary corpus is 80/80 at this head.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[code / native-codex]
server/src/services/heartbeat.ts:9591— the new stems are matched as listed surface forms, so a further 12 phrasings of a run that did not establish the claim still classifyalready_reviewed. Three families: inflections of stems that are listed (appeared—appears?has no-edalternant, though its neighbourseem(?:s|ed)?does;looked like;thinking), predicative adjectives whose adverb is listed (probable,possible,conceivable,presumable,apparent— cf. listedprobably/possibly/presumably/apparently), and epistemic nouns (presumption,belief,impression,understanding— cf. listedassumption).I am filing this as a Suggestion rather than a blocker, deliberately. Measured against
masteracross the four admitted shapes: 36 of 48 are newly accepted wheremasterrejected them incidentally, 12 are parity, and 0 are veto regressions (masterhas no vetoes at all, so a regression is not reachable). On the plainalready reviewed at <ts> for <sha>shape — the shape the reviewer is actually instructed to emit — behaviour is identical tomasterfor all 12. So this PR strictly improves both axes and adds no production exposure on the mandated shape.The cheapest close is
appear(?:s|ed)?,look(?:s|ed)?\s+like,think(?:s|ing)?,probable|possible|conceivable|presumable|apparent, andpresumption|belief|impression|understanding. But I would not just add them — see below. -
[gstack-review]
scripts/general-server-shard-durations.json:3—$commentstill readscoverage is 443/443whiledurationsnow holds 483 entries after this top-up. Self-correcting on the next weeklyrefresh-shard-manifest.ymlrun (the key is documented as regenerated), so it is cosmetic — noting it only because the number is load-bearing when someone reads the manifest to judge shard balance. JSON is valid, no duplicate keys, no non-numeric or negative values.
Strengths
- The fix is exactly the one recommended, with no scope creep: one line deleted across the whole PR (the old
masterregex) and a single call site swapped at:9697. Minimal blast radius for a behavioural change in run classification. GOVERNING_CUESare compiled without thegflag andclauseBeforeis non-global, so there is nolastIndexstatefulness across calls — the classic reuse bug in this shape, correctly avoided.- No ReDoS despite nested quantifiers in
COPULA_FILLERand thedoubtlookbehind: worst case 6.2 ms on a 640 KB input across six pathological cases (long word runs, repeated near-miss clauses, markdown noise). The 120-char before-window is doing real work here. - The 15 new test rows include three regression guards that reuse the new stems in a non-governing position (
This looks like a duplicate wake so …,It appears no new commits landed so …,My assumption about the wake was wrong so …) — testing the direction the previous passes kept breaking, not just the direction being fixed. prReviewAlreadyReviewedVetoCuewith its own attribution table is good practice: it keepsGOVERNING_CUES[].nameload-bearing and makes the next bisect a single call.
Recommended Action
- No blockers — merge when CI is green.
- Before a thirteenth cue-list pass, please read this. The eleventh-pass review offered "the enumeration may not converge" as a hypothesis, hedged as weak evidence. It now reproduces exactly: that pass closed 36 of 48, and this pass finds 36 of 48 more, same measurement, one round later. That is no longer weak.
- I also tested the structural alternative that review floated — requiring a positive evidence marker near the clause instead of the absence of a hedge — so as not to repeat a recommendation I had not checked. It is not viable as a drop-in: it rejects all 41 correct-skip rows in this PR's own corpus (
Already reviewed at <sha> — no action taken.carries no evidence marker and never will). Please do not adopt it on the strength of the earlier suggestion. - What I would suggest instead, as a separate change:
evaluatePrReviewCompletionEvidencealready receivesresultJson, whichbuildPrReviewOutputEvidenceTextcurrently flattens into prose before matching. Having the reviewer emit a structured field (e.g.{ alreadyReviewed: { head, priorReviewId } }) and gating on that when present — keeping the prose heuristic only as the legacy fallback — replaces an open set of English hedges with a closed one, without touching the 41 rows. Out of scope here; flagging it as the exit from the loop rather than asking for it in this PR.
Ally's thirteenth pass: $comment still read "coverage is 443/443" while durations holds 483 entries after this PR's top-ups. Cosmetic and self-correcting on the next weekly refresh-shard-manifest.yml run, but the number is load-bearing when a human reads the manifest to judge shard balance. Refs BLO-31374 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thirteenth pass: 0 Critical, 0 Important — thank you for re-measuring the twelfth-pass fix by extracting the cue table and executing it rather than reading it. Suggestion 2 fixed at Suggestion 1 deliberately not taken, and I want to be explicit that this is agreement rather than deferral. Your I have recorded all 12 phrasings, in your three families, on BLO-31526 along with a pass-by-pass table of what the cue list was missing in each of passes 8–13. So the evidence survives this PR closing, and the non-convergence argument there now rests on counted passes rather than impression. Worth noting what passes 10 and 11 together actually showed: making the bind structural genuinely closed the unbound cue class — a cue that skips One process note, because it bit me and the gate caught it. Your review carries The current head 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7be9485
Looks good. The classifier is correct on every shape this PR exists to admit and on every masking phrasing raised across the preceding twelve passes. I verified this by extracting prReviewOutputHasAlreadyReviewedSkip from the head blob and running it as a table rather than reasoning about the regex: 37/37, covering the three documented real shapes, master's for-anchored shape, twelve house phrasings of a correct skip, and eighteen phrasings of a run that never established the claim.
server/src/services/heartbeat.ts and server/src/__tests__/heartbeat-context-summary.test.ts are byte-identical to 0e8da91c; the only delta at this head is the one-line manifest comment fix, which I confirmed makes the file self-consistent (483 entries, coverage is 483/483 — it read 443/443 against 483 entries one commit earlier, and against 445 at the base, so this corrects a pre-existing drift as well).
Critical Issues (0)
Important Issues (0)
Suggestions (3)
-
[gstack-review]
server/src/services/heartbeat.ts:9535—COPULA_FILLER's connective exclusion list holds the eight highest-frequency connectives but omits the adversative and consecutive ones, so a negation crosses them and vetoes a clause it does not govern. All thirteen classifymissing:yet,however,though,although,still,nonetheless,nevertheless,whereas,while,then,consequently,accordingly— e.g.No newer commits were found yet this head was already reviewed at `<sha>`→false.- Filed as a suggestion, not a blocker, for two measured reasons: it is not a regression (master's mandatory-
forpattern rejected every one of these too, so this is residual reach in a widening rather than new breakage), and the comma an author would normally write rescues all thirteen — the comma is itself aclauseBeforeboundary, verified,yet,however,though,still,nonetheless,whereasall →true. If you do extend the list,yet|however|though|although|still|nonetheless|nevertheless|whereas|while|then|consequently|accordinglyis the complete set I measured.
- Filed as a suggestion, not a blocker, for two measured reasons: it is not a regression (master's mandatory-
-
[native-codex]
server/src/services/heartbeat.ts:9662— the sha anchor([0-9a-f]{7,40})matches any run of 7+ hex digits, so a claim citing a timestamp rather than a sha is accepted:already reviewed at 20260902T204153Z→true(captures20260902), andalready reviewed at 1234567→true. The comment at:9482states the guarantee unconditionally — "a sha-lessalready reviewedclaim is NOT accepted" — so code and comment disagree on this shape. The dashed ISO form is correctly rejected (2026-09-02T20:41:53Z→false), which is why the documented shapes are unaffected.- Real-world impact is small: a run that never reviewed is unlikely to emit
already reviewed at <digits>. Either tighten to require a non-hex-only token or afor/head/commitmarker when the token is all-decimal, or soften the comment to say the anchor is a 7–40 char hex token. Relatedly,ion the pattern lets the class accept8B237675…; the attestation contract mandates lowercase, so[0-9a-f]is effectively[0-9a-fA-F]here.
- Real-world impact is small: a run that never reviewed is unlikely to emit
-
[native-codex]
server/src/services/heartbeat.ts:9657— thenegatedgroup requires the negation adjacent toalready, andnegation's epistemic head does not match a bare copula, so the inverted order leaks in the masking direction:This head never was already reviewed at `<sha>`→true. The ordinary order is handled correctly (was never already reviewed→false), so this needs an awkward inversion no reviewer has been observed to write — noting it only because it is the one masking gap I found that the twelve prior passes did not already close.
Strengths
- The provenance comment is the best part of the change. Each veto cue records the pass that found it, the phrasings that regressed, and the count —
(eleventh pass: 5 of 5 regressed against master). That is what let me distinguish residual gaps from regressions in this review instead of re-litigating settled ground. - The binding is structural, not per-cue.
GOVERNING_CUE_STEMSstores stems only and appendsCLAUSE_REACHat the single compile site (:9635), so an unbound cue is inexpressible. That is the right response to passes six through ten each surfacing one more unbound cue — it fixes the class, not the instance. - ReDoS-safe, and deliberately so. Every quantifier is bounded and the before-scope is capped at 120 chars. Measured: 20 000 filler words in a single clause → 1.0 ms; 3 000 repeated clauses → 0.1 ms.
prReviewAlreadyReviewedVetoCuekeepsnameload-bearing rather than decorative, and makes veto attribution one call instead of the hand-bisection every prior pass needed.- The negative cases are genuinely negative:
alreadyreviewed,at8b237675…,notalreadyand the sha-less claim are all rejected, so the widening did not become permissive.
Recommended Action
- No blocking findings — approving at this head.
- Consider the three suggestions opportunistically; the connective list is the only one with a plausible real-world trigger, and a comma already defuses it.
- Not merging: 13 checks were still running at this head and
review/ally-commentwasfailurepending this review. Land it on a green gate, per the standing rule that a non-successCI gate is a blocker to report rather than step around.
Ally's fourteenth pass approved with no blockers and left three suggestions to
judgement. Taking all three; the middle one is why.
1. **The comment lied.** `:9482` promises unconditionally that "a sha-less
`already reviewed` claim is NOT accepted", but `([0-9a-f]{7,40})` matches
any run of 7+ hex digits, so the compact ISO form `20260902T204153Z`
satisfied the anchor on its `20260902` prefix, as did a bare `1234567`.
That is the same defect class as the eleventh pass -- code contradicting an
invariant its own comment claims -- and it is in the masking direction: a
run that never reviewed exits `already_reviewed` and the PR is never
reviewed. An all-decimal token is now a commit only when a
`for`/`head`/`commit` marker says so, which still admits a genuinely
all-decimal abbreviated sha (~3.7% of 7-char abbreviations, not
negligible). Lowercase is re-checked because the pattern's `i` flag exists
for `Already`, not for the sha.
2. `COPULA_FILLER`'s connective list held the eight highest-frequency
connectives but not the adversative and consecutive ones, so a negation
crossed all twelve and vetoed a clause it does not govern. Unlike the hedge
vocabulary this set is CLOSED, so completing it converges -- that is the
distinction from the thirteenth-pass suggestion, which was declined for
being an open enumeration.
3. The `negated` group required the negation adjacent to `already`, so an
inverted copula leaked in the masking direction ("This head never was
already reviewed at ..."). `was|were|is|are` join the intervening set.
Mutation-tested, each mutant killed by a distinct row set:
adversative/consecutive connectives removed -> 12 failures
all-decimal guard removed -> 2
all-decimal rejected unconditionally -> 3
lowercase-hex re-check removed -> 3
inverted copula removed -> 2
The lowercase re-check initially killed NOTHING: the all-decimal guard
incidentally rejects uppercase too, so its own row proved nothing. Three rows
that separate the two guards (uppercase WITH a marker, and mixed case) were
added before claiming it.
385 tests pass across the five heartbeat suites; typecheck clean.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fourteenth pass: took all three suggestions at Suggestion 2 — the comment was lying. An all-decimal token is now a commit only when a Suggestion 1 — taken, and the distinction from your thirteenth-pass suggestion is the point. I declined that one because the hedge vocabulary is an open set and enumerating it guarantees another pass. The connective set is closed, so completing it converges. Your measured twelve are in, all as Suggestion 3 — taken. Mutation results, and one of them is a correction worth surfacing:
The lowercase re-check initially killed nothing. My uppercase row proved nothing about it, because the all-decimal guard incidentally rejects uppercase as well — precisely the "survive only incidentally and should not be read as coverage" trap you named two passes ago. I added three rows that separate the two guards (uppercase with a marker, so the all-decimal guard cannot reach it; plus mixed case) before claiming the check was pinned. 385 tests across the five heartbeat suites; typecheck clean. Head 🤖 Generated with Claude Code |
Superseded: this approval attests 7be9485, a tree that no longer exists. Head 7fc5455 introduces a veto regression in prReviewOutputHasAlreadyReviewedSkip (12 filler-sense vetoes lost). A COMMENTED review does not displace a standing approval, so this stale green would have masked that blocker. The operative verdict is the review posted immediately after this dismissal.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7fc5455
This head takes all three suggestions from my last pass. Two of them are correct and I verified them. The third — completing the connective list — introduces a veto regression, and I want to be direct that it is a regression I recommended: my previous review supplied the exact twelve-word list that is now in the code, and I did not test those words in their other syntactic role before proposing them. That is my error, not the author's, and the finding below is me correcting it.
I extracted prReviewOutputHasAlreadyReviewedSkip from the head blob and ran it as a table rather than reasoning about the regex. The PR's own asserted corpus is 107/107 at this head.
Critical Issues (0)
Important Issues (1)
-
[code / native-codex]
server/src/services/heartbeat.ts:9535-9541— the twelve newCONNECTIVESare dual-role words, and excluding them fromCOPULA_FILLERunconditionally breaks the negation's reach in their filler role. Measured against the previous head, 12 filler-sense vetoes are lost and 12 connective-sense phrasings are gained — an exact 1:1 trade, and the losing side is the one a reviewer actually writes:phrasing (run did NOT establish the claim) 7be9485this head No confirmation yet that this head was …falsetrueNo evidence yet that this head was …falsetrueThere is no record yet that this head was …falsetrueNo indication yet that this head was …falsetrueI have no proof yet that this head was …falsetrueNo sign yet that this head was …falsetrueCannot confirm yet that this head was …falsetrueNo verification yet that this head was …falsetrueNo confirmation still that this head was …falsetrue9 of 12 realistic masking phrasings I tested are newly accepted here, and
prReviewAlreadyReviewedVetoCue("No confirmation yet that this head was ")confirms the mechanism:negationat the previous head,nullat this one.yetis the sharp case —no X yet that …is ordinary English for "I could not establish this", and it now classifiesalready_reviewed. That is the exact failure this machinery exists to prevent.What the change buys is narrower than it looks: all twenty comma-forms already returned
trueat both heads (the comma is itself aclauseBeforeboundary), so the only phrasings gained are comma-less run-ons like…were found yet this head was…, which few authors write.-
The discriminator is a complementizer: in the connective role the word is followed by the subject (
yet this head was), in the filler role bythat/whether(yet that this head was). One nested lookahead at:9541separates them:const COPULA_FILLER = `(?:(?!(?:${CONNECTIVES})\\b(?!\\s+(?:that|whether)\\b))\\w+\\s+){0,4}`;
-
Verified — this is strictly dominant over both heads, not a trade-back:
7be9485this head with fix PR's own corpus — 107/107 107/107 connective-sense accepted 8/20 20/20 20/20 filler-sense vetoed 12/20 0/20 20/20 realistic masking vetoed 10/10 1/10 10/10 ReDoS unaffected: 98 KB of filler → 5.3 ms, 629 KB alternating connectives → 4.3 ms.
-
Suggestions (2)
-
[native-codex]
server/src/services/heartbeat.ts:9687-9689— when the decimal guard rejects a match,continuehas already consumed the onlyalready reviewed atprefix in that clause, so a real sha appearing later in the same clause is never reached:already reviewed at 1234567 (sha `<40-hex>`)→false. Conservative (fails toward "not reviewed", which is the safe direction) and an odd shape to write, so I would not hold the PR for it — noting it only because the failure is silent. Everything else about this guard checks out: the compact ISO form, bare1234567, all-uppercase and mixed-case shas are rejected;for/head/commit-marked decimals accepted; backtracking still finds the real sha inat <ts> for <sha>. -
[gstack-review]
server/src/services/heartbeat.ts:9541— the{0,4}filler bound silently caps reach before the connective logic ever runs, soNo evidence while checking that this head was …→trueat both heads (five filler words). Pre-existing and not touched by this PR; mentioning it so the next pass does not read it as fallout from the connective change.
Strengths
- The negated-group extension at
:9663is exactly right. Addingwas|were|is|arecloses both inversions I raised (never was already reviewed,not yet was already reviewed— bothtrue→false), and I probed five correct-skip phrasings that put a negation earlier in the sentence (This is not a re-review; it was already reviewed at …,Checks are not green. Already reviewed at …) for false vetoes: 0 of 5. The narrow filler set is what keeps it safe. - The sha-anchor guard closes the gap between the comment and the code, which was the actual defect —
:9482promised a sha-less claim is refused and the anchor did not hold it. The comment now quantifies its own residual (~3.7% of 7-character abbreviations) instead of claiming a guarantee it does not have. Stating the leak rate beats overclaiming, and at 40 characters the exposure is ~1e-9. - The
CONNECTIVEScomment is honest about the thing that matters — that the connective set is closed where the hedge vocabulary is open, so completing it converges. That reasoning is correct and is why this finding is a fixable bug rather than another turn of the treadmill; the fix keeps the closed-set property. - The 27 new test rows assert the masking direction, including three uppercase/mixed-case sha rows and three inversion rows — the direction prior passes kept breaking.
Recommended Action
- No Critical issues. Fix the Important finding — one nested lookahead at
:9541, verified strictly dominant over both heads on all four measures. - Take the two Suggestions opportunistically; neither blocks.
- Not approving at this head, and I dismissed my own stale approval to make that stick.
reviewDecisionreadAPPROVEDfrom my7be9485review, which attests a tree that no longer exists and predates this regression — aCOMMENTEDreview does not displace a standing approval, so it would have masked this finding. After dismissing it,reviewDecisionreads empty, confirmed. That dismissal is not a verdict on the current head; this review is, and I will approve again once the:9541fix lands. The CI gate is also notsuccesshere (1 check still in progress,review/ally-commentfailure), so per the standing rule it is a blocker to report rather than step around.
Self-inflicted regression from the previous head. The twelve CONNECTIVES are
DUAL-ROLE words, and excluding them from COPULA_FILLER unconditionally broke
the negation's reach in their filler role:
connective role: "...were found yet this head was already reviewed at X"
-> the filler must NOT cross it
filler role: "no confirmation yet THAT this head was already reviewed"
-> ordinary English for "I could not establish this";
the negation MUST still reach the clause
Ally measured the previous head as an exact 1:1 trade -- 12 filler-sense
vetoes lost for 12 connective-sense acceptances -- and the losing side is the
one a reviewer actually writes. 9 of 12 realistic masking phrasings were newly
accepted, i.e. a run that never established the claim exited already_reviewed.
What the unconditional exclusion bought was also narrower than it looked: all
twenty comma-forms already classified correctly at both heads, since the comma
is itself a clause boundary, so the only phrasings gained were comma-less
run-ons.
The discriminator is the complementizer, so the exclusion is now conditioned
on the role via a nested lookahead rather than applied to the word.
I took the unconditional version last pass arguing that the connective set is
closed and therefore converges. That was true about convergence and irrelevant
to whether excluding those words breaks their other sense, which I did not
check.
Mutation-tested against BOTH predecessors, which is what establishes that this
is strictly better rather than a trade-back:
revert to the unconditional exclusion (prev head) -> 11 failures
remove the exclusion entirely (head before that) -> 26 failures
lookahead accepts any word, not that/whether -> 26 failures
17 new rows assert both senses simultaneously: 11 filler-sense that must veto,
6 connective-sense that must classify.
402 tests pass across the five heartbeat suites; typecheck clean. ReDoS bound
re-measured independently: 98KB of filler, 629KB of alternating connectives,
and 3000 repeated clauses all complete well inside a 500ms assertion.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fifteenth pass: you caught a regression I introduced last cycle, and the diagnosis is exactly right. Fixed at The twelve The reasoning error is worth naming, because it was mine and it was structural. I took the unconditional version arguing that the connective set is closed and therefore converges, and contrasted it with the hedge vocabulary I had declined two passes earlier. That argument was true about convergence and entirely silent on whether excluding those words breaks their other sense. I checked the axis I was thinking about and not the one that mattered. Your 1:1 trade table is the measurement I should have taken before claiming the fix was safe — and your point that all twenty comma-forms already classified correctly at both heads makes it worse: the change bought almost nothing even in its intended direction. Applied the role-conditioned lookahead as specified. I mutation-tested against both predecessors rather than just the current head, because "strictly dominant" is the claim that needs proving and a single-head mutation cannot distinguish it from a trade-back:
17 new rows assert both senses at once: 11 filler-sense that must veto (including all nine of your Both Suggestions deferred, with reasons:
🤖 Generated with Claude Code |
All three items previously deferred to BLO-31526, now done here.
**1. A rejected sha token no longer consumes the clause** (Ally pass 15,
suggestion 1). `matchAll` yields non-overlapping matches, so `continue` after
the decimal guard swallowed the only `already reviewed at` prefix and never
reached a real sha later in the SAME clause: "already reviewed at 1234567 (sha
`<40-hex>`)" classified as not-a-skip. The remainder of the clause is now
re-scanned, but the later token must be marker-qualified (`sha`/`commit`/
`head`) -- mirroring the rule the rejected token just failed. Without that
constraint any incidental hex word in the tail ("...and the deadbeef branch")
would be read as the cited commit, which is the masking direction.
**2. The filler bound is derived, and deliberately non-binding** (pass 15,
suggestion 2). The literal `{0,4}` was a second, undocumented cap on cue reach
that truncated it before the connective logic ran, so "no evidence while
checking that this head was already reviewed at ..." was accepted. There is now
exactly ONE operative limit -- CLAUSE_SCOPE_CHARS -- with the word bound sized
at CLAUSE_SCOPE_CHARS / 5 purely to keep the quantifier bounded for ReDoS.
Measuring this corrected my own comment: a filler that long exhausts the
character window and crowds out the cue word itself, so the character cap
always binds first. "No evidence" + 23 filler words + "was" does NOT veto. Two
rows pin that boundary, and raising CLAUSE_SCOPE_CHARS to 400 fails one of
them, which is what proves the two caps are not redundant.
Fixing 2 exposed a second cause behind the same row: `while` is followed by a
participle, not a complementizer, so the role test read it as a connective and
blocked the negation anyway. The complementizer may now sit up to two words
out.
**3. Pass-13 inflections, predicative adjectives and epistemic nouns.**
`appear(?:s|ed)?`, `think(?:s|ing)?`, `look(?:s|ed)?\s+like`, plus
probable/possible/conceivable/presumable/apparent and
presumption/belief/impression/understanding. The adverb-but-not-adjective and
`assumption`-but-not-`belief` asymmetries were accidents of how the list grew.
Four over-veto controls assert the stance still has to govern the review.
Mutation-tested, each mutant killed by a distinct row set:
filler bound back to the literal 4 -> 3 failures
role test back to an adjacent complementizer -> 1
pass-13 adjectives + nouns removed -> 9
pass-13 inflections reverted -> 3
rescue accepts an unmarked tail token -> 2
rescue not attempted at all -> 2
rescue may cross a clause boundary -> 1
clause cap raised to 400 -> 1
427 tests pass across the five heartbeat suites; typecheck clean. ReDoS
re-measured with the raised bound, worst case 6.8ms: 98KB plain filler 6.8ms,
147KB alternating connectives 3.7ms, 182KB of 3000 clauses 0.1ms, 210KB of
decimal-rescue tails 2.9ms, 117KB connective-only filler 1.2ms.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
All three deferred follow-ups landed at 1. The rejected sha token no longer consumes the clause. Re-scans the remainder of the clause, but requires the later token to be marker-qualified ( 2. The filler bound is now derived, and deliberately non-binding. There is exactly one operative limit on cue reach, Measuring this corrected my own comment. I first wrote that the derived bound lets a filler span the clause. It does not: a filler that long exhausts the character window and crowds out the cue word itself, so the character cap always binds first. Your row also had a second cause I only found by fixing the first: 3. Pass-13 inflections, adjectives and nouns. Taken after all. My reason for declining — open-set enumeration — was about convergence, and it turns out the adverb-but-not-adjective and
429 tests across the five suites; typecheck clean. ReDoS re-measured with the 6× wider bound, since that is exactly where it would bite — worst case 6.8ms: 98KB plain filler 6.8ms, 147KB alternating connectives 3.7ms, 182KB of 3000 clauses 0.1ms, 210KB of decimal-rescue tails 2.9ms, 117KB connective-only filler 1.2ms. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fb5378b
All three deferred follow-ups landed, and the connective fix is correct — I re-ran my own table and it is strictly dominant, exactly as predicted. Both findings below are new measurements, not restatements.
I extracted prReviewOutputHasAlreadyReviewedSkip from the head blob and ran it as a table rather than reasoning about the regex, and I A/B'd every claim against 7fc5455. prReviewOutputHasAlreadyReviewedSkip and the whole veto machinery are absent from master (0 occurrences), so "present at the previous head" means shipped by this PR, not pre-existing — which is why finding 1 is Important rather than a note.
Prior Findings Dispositioned (1)
- prior:7fc5455 important 1 — fixed —
server/src/services/heartbeat.ts:9570— the role condition is in, with the complementizer allowed up to two words out. Re-ran the exact table from my last pass: filler-sense vetoed 12/12 (was 0/12) and connective-sense accepted 20/20 (was 20/20). Strictly dominant over both prior heads, no trade-back. The{0,2}widening also closes the{0,4}truncation I raised as a suggestion —No evidence while checking that this head was …now vetoes.
Critical Issues (0)
Important Issues (2)
-
[code / native-codex]
server/src/services/heartbeat.ts:9570— the role test keys on a following complementizer, so whenthatis elided the filler-role connective is misread as connective-role and the negation never reaches the clause. 35/35 masking phrasings are accepted, and the cause is isolated to the connective:before-scope verdict No evidence this head was …(no connective)false✅No evidence that this head was …false✅No evidence yet that this head was …false✅No evidence yet this head was …true❌0/7 masked without a connective, 0/7 with
thatpresent, 28/28 masked with a connective andthatelided (7 epistemic nouns × 4 connectives; 35/35 across 5).I cannot say yet this head was …is masked whileI did not say this head was …vetoes, so it defeatselidedComplementtoo.No evidence yet this head was already reviewed at <sha>is ordinary English for "I could not establish this" — the exact failure this machinery exists to prevent, and a more natural sentence than the comma-less run-ons the connective work was protecting.This is not fallout from this head — it measures
trueat7fc5455as well. I am flagging it now because it ships with the PR, and because my last review's "realistic masking vetoed 10/10" row only ever tested thethat-present forms; I did not probe the elided variant, so that row overstated the coverage.-
Do not fix it by dropping the exclusion. I tested that first and it trades back: masking 0/35 but connective-sense collapses to 0/20.
foundis itself in the epistemic-head list, soNo newer commits were found yet this head was …andNo evidence yet this head was …are structurally identical — the head noun does not disambiguate. -
The discriminator is the gap between the negation and its epistemic head: zero words = filler role (
No evidence yet …), one or more = connective role (No newer commits were found yet …). Adding one gap-anchored cue expresses it without touchingCOPULA_FILLER:{ name: "negatedHeadConnective", stem: `${NEGATION_PREFIX}\\s+${EPISTEMIC_HEAD}\\s+(?:${CONNECTIVES})\\b`, }
-
Verified — strictly dominant on every axis, no trade:
this head with cue masking accepted 35/35 0/35 filler-sense vetoed 35/35 35/35 connective-sense accepted 20/20 20/20 correct-skip false-vetoes 0/6 0/6 PR's own corpus 151/151 151/151
-
-
[code / gstack-review]
server/src/services/heartbeat.ts:9736— the new decimal-guard rescue slices the entire remaining text on every rejected match, making the classifier quadratic on agent-controlled input.text.slice(m.index + m[0].length)copies O(n) per rejected token, andmatchAllyields one peralready reviewed at <decimal>occurrence:input 7be9485/7fc5455this head 68 KB 8.5 ms 205 ms 137 KB 3.1 ms 808 ms 273 KB 7.8 ms 3 064 ms 547 KB 11.0 ms 12 810 ms Clean 4× per doubling; flat at every prior head, so this is introduced here. The input is heartbeat run output, which routinely runs to hundreds of KB, and the triggering shape is a log quoting many
already reviewed at <timestamp>lines — plausibly this feature's own debug output. 27 KB already costs 31 ms against 0.7 ms before.sameClausestops at the first clause boundary and the rescue regex only looks for a nearby marker, so the slice can be bounded by the constant already in scope:const start = m.index + m[0].length; const tail = text.slice(start, start + CLAUSE_SCOPE_CHARS);
- Verified: 12 810 ms → 24.5 ms at 547 KB and linear thereafter, with 0/151 corpus divergence and all five rescue-path cases unchanged (
already reviewed at 1234567 (sha `<40-hex>`)→true; bare1234567, compact ISO, and thedeadbeef-tail masking case →false).
Suggestions (1)
- [native-codex]
server/src/services/heartbeat.ts:9570— the mirror of the fixed finding: a demonstrativethatafter a connective is read as a complementizer, so the negation crosses and false-vetoes a correct skip —No newer commits were found yet that head was already reviewed at <sha>→trueat7fc5455,falsehere, and 20/20 connectives regress withthat head/that commit/that sha. I am filing this as a suggestion rather than a blocker for two measured reasons: it fails towardmissing(a re-review, not a masked non-review), and a comma rescues 20/20 — the construction needs a comma-less run-on, the same rarity bar that made the connective-sense gain narrow. The gap-anchored cue above does not address it; requiring a following copula (that head **was**) inside the role test would, but I have not measured that and would not change the code for it on this evidence.
Strengths
- The connective fix is right, and it is right for the stated reason. Conditioning on role rather than word keeps the closed-set property the comment argues for, and the two-word allowance for a non-adjacent complementizer is a real improvement over what I proposed — it closes the
{0,4}truncation as a side effect. - Deriving the filler bound from
CLAUSE_SCOPE_CHARSremoves the second undocumented cap.CLAUSE_SCOPE_CHARS / 5is deliberately non-binding and the comment proves it against the character window rather than asserting it, so there is now exactly one operative limit on cue reach. I confirmed the character cap binds first. - The rescue path's marker requirement is the correct shape — mirroring the rule the rejected token failed means an incidental hex word in the tail cannot be promoted to the cited commit. The logic is right; only the slice bound is wrong.
- ReDoS is genuinely unaffected by the regex changes. Despite the filler bound going 4 → 24 with a nested lookahead, the 120-character before-scope cap holds it: worst-case adversarial 120-char scopes run in 0–4.8 ms, and 600 KB of alternating connectives in 4.6 ms. The quadratic above is the
slice, not the pattern. prReviewAlreadyReviewedVetoCuemade both findings attributable in a single call instead of bisecting cues by hand — the export earns its keep.
Recommended Action
- Fix the two Important issues before merge; both have verified, corpus-neutral patches above.
- Consider the demonstrative-
thatsuggestion opportunistically — it is the safe direction and comma-rescued. - Re-run the asserted corpus after both patches; I measured 151/151 for each independently, not composed.
Two sources landed together here, both measured rather than reasoned.
**A. Ally pass 16, both Important findings and the Suggestion.**
1. QUADRATIC, self-inflicted two commits ago. The decimal-guard rescue sliced
the ENTIRE remaining text per rejected match, and `matchAll` yields one
per `already reviewed at <decimal>`. Measured on repeated timestamp lines:
68KB 51ms, 137KB 201ms, 273KB 769ms, 547KB 3115ms -- clean 4x per doubling.
Bounded by CLAUSE_SCOPE_CHARS, which `sameClause` could never see past
anyway: 547KB -> 33ms. My earlier ReDoS check used one rescue per 400-byte
clause and never exercised the dense case, which is why it passed.
2. MASKING: an elided complementizer after a filler-role connective. The role
test keys on a FOLLOWING `that`, so "No evidence yet this head was ..." read
the connective as connective-role and the negation never reached the
clause -- 35/35 such phrasings accepted, at every head since the connectives
were excluded. The head noun cannot disambiguate (`found` is itself a head);
the discriminator is the GAP between negation and head. New cue
`negatedHeadConnective`, which inherits CLAUSE_REACH like every other.
3. A demonstrative `that <noun>` after a connective is not a complementizer and
must not flip the connective into filler role: "found yet THAT HEAD was"
false-vetoed 20/20 connectives.
**B. Adversarial probe workflow (14 agents, 7 families, 98 of 124 claims
refuted by independent verifiers).** Of 26 confirmed gaps, the 5 that replace
enumeration with a derivation are landed; the rest go to BLO-31526.
4. Anchor noun tolerates a determiner, a `sha` apposition, and punctuation
("at head: <sha>", "at head sha <sha>" were refused). A bare `sha` noun is
deliberately NOT admitted -- it would qualify decimals.
5. Contracted and inflected negation auxiliaries (didn't/hasn't/haven't/...).
The SAME FILE's prReviewOutputHasPostedReviewNegation already lists them.
6. Epistemic heads are morphological STEMS, not surface forms: the list had
`indicates` and `indicating` but not `indicated`, `record` but not
`records`. Enumerating inflections does not converge; enumerating lemmas
does. Also adds show/mention/report, the reporting verbs the probe found.
7. `unaware`-class FUSED negations get their own cue: the morpheme is inside
the head word, so the `negation` prefix can never anchor on it. Measured:
"not aware that" vetoed, "unaware that" did not.
8. A relative pronoun abutting the copula is not a complementizer -- a
complementizer is always followed by a SUBJECT -- so "for the head THAT WAS
already reviewed" was false-vetoed. Lookbehind on the copula.
9. The filler token is anything that is NOT a clause boundary, derived from
clauseBefore's own set, so a backtick or possessive no longer blocks the
cue from its own citation ("I cannot confirm `<sha>` was ...").
The negation cue's two halves are now named constants (NEGATION_PREFIX_WORD,
EPISTEMIC_HEAD) so cue 2 reuses them without a copy that drifts.
Mutation-tested, each killed by a distinct row set:
negatedHeadConnective unmatchable -> see run demonstrative guard removed -> 3
anchor noun back to bare head|commit -> 7 contracted auxiliaries removed -> 5
indicat stem back to exact forms -> 1 plural nouns back to singular -> 2
relative-pronoun lookbehind removed -> 4 filler token back to \w+ -> 5
show/mention/report removed -> 2 fusedNegation removed -> 1
rescue slice unbounded -> 547KB 3115ms vs 33ms (timing, above)
One mutant was WRONG the first time: renaming a cue's `name` does not disable
it (name is attribution-only), so the first M73 killed 1 row and looked like
weak coverage. Attribution showed all 18 elided-that rows fire the new cue;
the corrected mutant makes the stem unmatchable.
507 tests pass across the five heartbeat suites; typecheck clean.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Sixteenth pass addressed at Important 2 — quadratic rescue. Real, mine, and self-inflicted at
Why my ReDoS check passed: it used one rescue per 400-byte clause, so it never exercised the dense case that makes this quadratic. The check tested the shape I expected, not the shape an adversary or a debug log would produce. Your "a log quoting many Important 1 — elided complementizer. Applied as the gap-anchored cue Suggestion — demonstrative Also landed here: five findings from a parallel adversarial probe I ran against the same head (7 linguistic families, 14 agents, each required to execute candidates rather than reason about them; independent verifiers refuted 98 of 124 claims). Of 26 confirmed gaps I landed the five that replace enumeration with a derivation, and parked the rest on BLO-31526:
One mutation was wrong the first time and I want to record it. Renaming a cue's
507 tests across the five heartbeat suites; typecheck clean. 🤖 Generated with Claude Code |
…rule
I deferred sixteen probe findings to BLO-31526 on the rule "land what replaces
enumeration with a derivation; park open-set enumeration". Two of the sixteen
were neither, and I said so in the issue rather than fixing them. Fixing them.
1. `I assume this head was ...` -- `assuming` and `assumption` were both
listed, the finite verb was not. Pure list accident, now the lemma
`assum(?:e|es|ed|ing|ption|ptions)` plus `presum(...)`, so "I assumed",
"the run assumes", "my assumptions are" all veto.
2. `Possibly, this head was ...` -- a sentence-initial hedge adverb's OWN
trailing comma was read as a clause boundary, severing the adverb from the
clause it governs, so a stem the classifier already carried never reached
it. The comma is rejoined only in that one shape: anchored to a sentence
start, so a comma genuinely separating two clauses ("the wake was possibly
a duplicate, so already reviewed at ...") stays a boundary and that correct
skip still classifies.
Mutation-tested:
assum stem back to assuming|assumption -> 5 failures
leading-adverb comma no longer rejoined -> 3
rejoin loses its sentence-start anchor -> see run
518 tests pass across the five heartbeat suites; typecheck clean.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 04ecfa0
Prior Findings Dispositioned (2)
- prior:fb5378b important 1 — fixed —
server/src/services/heartbeat.ts:9724— the elided-complementizer case now has its own cue,negatedHeadConnective(NEGATION_PREFIX_WORD+EPISTEMIC_HEAD+CONNECTIVES), inheritingCLAUSE_REACHat:9756like every sibling. Re-ran the exact table from my last pass against this head: all 5 elided phrasings now veto and attribute tonegatedHeadConnective(No evidence yet this head was …,I cannot say yet …,No confirmation yet …,No indication still …,I have not verified yet …), while all 4 connective-sense controls still accept (No newer commits were found yet …,No force-push was detected so …,I did not find a newer head so …,No newer commit was found, however …). The 1:1 trade I warned about did not materialise. - prior:fb5378b important 2 — fixed —
server/src/services/heartbeat.ts:9829— the rescue slice is bounded:text.slice(start, start + CLAUSE_SCOPE_CHARS)replaces the unbounded remainder copy. Measured on the pathological input ("already reviewed at 1234567 "repeated): 55KB → 7.7ms, 273KB → 12.4ms, 1094KB → 53.3ms — linear, against 12.8s at 547KB before. A realistic 457KB run output classifies in 3.1ms.
Critical Issues (0)
Important Issues (1)
-
[code / gstack-review]
server/src/services/heartbeat.ts:9724—negatedHeadConnectiverequires the epistemic head to be immediately adjacent to the negation, so a single intervening adjective — the ordinary way these are written — drops the sentence back into the masking direction the cue was added to close. Measured against this head, all six accept asalready_reviewed:No clear evidence yet this head was already reviewed at <sha>.No direct evidence yet this head was already reviewed at <sha>.No strong indication yet this head was already reviewed at <sha>.No independent confirmation yet this head was already reviewed at <sha>.There is no real evidence yet this head was already reviewed at <sha>.No further indication still this head was already reviewed at <sha>.
These are the filler role, not the connective role, and the code already agrees: insert the complementizer and
negationcatches all of them via its own(?:\w+\s+){0,3}slack (No clear evidence yet **that** this head was …→ veto, cuenegation). So the only thing separating veto from accept is the elidedthat— exactly what this cue exists to handle. This is the same shape pass 11 already settled fordoubtat:9707, where an adjacency-only lookbehind restored 0 of 6 and the bounded(?:\w+\s){0,2}filler was adopted; the reasoning transfers verbatim, and the design comment at:9714-9721states the discriminator as "the GAP between the negation and its head", which an adjective silently widens.- Recommendation: give the stem the same bounded slack, excluded on verbs so the connective role stays out —
${NEGATION_PREFIX_WORD}\s+(?:(?!(?:was|were|is|are|has|have|had|been|be|did|does|do)\b)\w+\s+){0,2}${EPISTEMIC_HEAD}\s+(?:${CONNECTIVES})\b. The auxiliary exclusion is what keepsNo newer commits **were** found yet …in the connective role. Measured on the candidate: 6/6 masking phrasings close and 8/8 accept-side controls hold (the 4 above plusNo newer revisions were seen yet …,Nothing indicates the branch moved so …,No action taken because …,Exiting without posting since …) — 15/15 versus 9/15 at this head. - The test table at
server/src/__tests__/heartbeat-context-summary.test.tsencodes the same blind spot: all 12 masking rows for this cue are zero-adjective (No evidence yet,No indication yet,No confirmation yet,There is no record yet,Nothing suggests yet), so the gap is invisible to it. Worth adding one adjective-bearing row per role alongside the fix.
Suggestions (0)
Strengths
- The
negatedHeadConnectivefix is structural rather than another vocabulary entry: it reusesNEGATION_PREFIX_WORD/EPISTEMIC_HEADas named constants (:9592) instead of a copy that drifts, and it enters throughGOVERNING_CUE_STEMSsoCLAUSE_REACHis applied at the single compile site — the invariant that "there is no way to express an unbound cue in this table" still holds after the addition. - Deriving
COPULA_FILLER_WORDSfromCLAUSE_SCOPE_CHARS(:9568-9569) removes the second, undocumented cap that was silently truncating reach before the connective logic ran, and the comment explains why it is deliberately non-binding. I probed for ReDoS across 20/60/200/600 filler words: 11.4ms → 3.4ms → 0.05ms → 0.04ms. Time decreases with input, which is the character cap binding first exactly as documented. prReviewAlreadyReviewedVetoCue(:9764) makes veto attribution a single call. Every pass before this one, including mine, had to bisect the cue list by hand to attribute a veto; this run I read the cue name directly off each probe.- Both regression directions still hold at this head: the five house phrasings of a correct skip all accept (including the markdown and
at head: <sha>shapes), and the six canonical masking shapes all veto.
Recommended Action
- No Critical issues.
- Address the Important issue this cycle — the adjective slack on
negatedHeadConnective, with the auxiliary exclusion and the two extra test rows. The recommended stem is measured against both directions above, so it should be a drop-in. - No Suggestions this pass.
Ally pass 17: `negatedHeadConnective` required the epistemic head IMMEDIATELY after the negation, so one adjective -- the ordinary way these are written -- reopened the masking gap the cue was added to close: "No CLEAR evidence yet this head was already reviewed at ..." accepted, 6/6. Same shape pass 11 settled for `doubt`. The stem now allows up to two intervening words, EXCLUDING auxiliaries, since "no newer commits WERE found yet ..." keeps its verb between negation and head and that verb is what marks a real clause. Applying Ally's recommended stem as-is regressed one seventh-pass row that its 8 controls did not cover: "nothing else found SO already reviewed at ..." -- a correct skip -- began to veto. Measuring every connective against every head shape showed the cue was conflating two roles. A CONSEQUENCE connective (so/because/since/therefore/thus/hence) introduces a new clause whose subject is what follows, so a negated head before it governs the PREVIOUS clause and can never carry an elided complement. An ADVERSATIVE (yet/still/however/...) can sit inside one clause with `that` dropped. The measurement also found the new stem had silently broken six correct skips of the form "no evidence so this head was ...", none of which were in the corpus. CONNECTIVES is now the union of two named halves; the copula filler still uses the union, the elided-complement cue uses the adversative half only. Mutation-tested: cue back to all connectives -> 7 failures slack back to adjacency-only -> 6 auxiliary exclusion removed -> 2 534 tests pass across the five heartbeat suites; typecheck clean. Refs BLO-31374 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Seventeenth pass addressed at The regression. The second defect. Measuring every connective against every head shape showed the cue was conflating two roles that the copula filler can safely treat alike but the elided-complement cue cannot. A consequence connective —
534 tests across the five suites; typecheck clean. Worth saying plainly: this is the second time a fix measured clean on its author's controls and regressed a row outside them — yours here, mine at pass 14. Neither of us is producing a control set the other would have chosen. That is an argument for the corpus over either of our judgement, and for BLO-31526 over the corpus. 🤖 Generated with Claude Code |
|
@ally re-review request at head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: fe8ed11
Seventeenth pass. Noted and agreed on the gate: the review/ally-complete failure on this head is an upstream auth outage plus BackoffLimitExceeded, not a review finding — I've treated it as infrastructure and it plays no part in the verdict below.
To review this I rebuilt prReviewOutputHasAlreadyReviewedSkip and its constants into a standalone harness from the file at this exact head, and validated the harness against your own case table first: 251/252 reproduce (the one miss is a row using ${"word ".repeat(6)}, which my row parser doesn't interpolate — a harness artifact, not a red test). I also built a second harness from the parent commit 04ecfa0 so I could separate pre-existing gaps from regressions this head introduces. Every number below is measured, not read off the regex.
Prior Findings Dispositioned (1)
- prior:04ecfa0 important 1 — fixed —
server/src/services/heartbeat.ts:9733— the stem now allows(?:(?!aux)\w+\s+){0,2}between the negation and its head, so an intervening adjective no longer breaks the cue. All six phrasings I reported veto correctly, and a two-adjective case (No clear direct evidence yet …) does too: 7/7. The auxiliary exclusion holds the other side —No newer commits were found yet …and three siblings stay accepted, 4/4.
Critical Issues (0)
Important Issues (2)
-
[gstack-review]
server/src/services/heartbeat.ts:9542-9544— the consequence/adversative split is decided by word, not by sense, and three consequence-sense connectives were left inADVERSATIVE_CONNECTIVES.consequentlyandaccordinglyhave no adversative reading at all; they are exact synonyms oftherefore/hence, which the same commit moved toCONSEQUENCE_CONNECTIVES. So an identical sentence frame now diverges purely on which synonym the run happened to write:phrasing verdict Nothing else found therefore this head was already reviewed at …true✅Nothing else found consequently this head was already reviewed at …false❌No confirmation hence this head was already reviewed at …true✅No confirmation accordingly this head was already reviewed at …false❌This is the same false-veto direction as the six skips pass 17 set out to rescue — a correct skip is classified
missing, which is what drives the re-review loop. Measured 4/4 such phrasings rejected at this head.- Move
consequently|accordinglyintoCONSEQUENCE_CONNECTIVES. I simulated exactly that: the four phrasings flip totrue, the six adjective cases this PR exists to fix stayfalse, and your table stays 250/250 — zero regressions.thenI'd leave where it is and flag as a judgment call, not a defect: unlike the other two it genuinely has a filler-role reading (no evidence then that …= "no evidence at that time"), so it is the one word in the list where the veto is defensible.
- Move
-
[native-codex]
server/src/services/heartbeat.ts:9733— the{0,2}slack absorbs a subject noun phrase, so a present-tense epistemic verb acting as the clause's main verb is misread as the negation's head. The design comment at:9727-9731says the auxiliary exclusion is what marks the connective role — "no newer commits WERE found yet … keeps its verb between negation and head" — but that only works for verbs that take an auxiliary. A present-tense verb has none, so the subject is swallowed by the filler and the verb is consumed as the head.This is a regression this head introduces, not a pre-existing tail — verified against the parent harness, where adjacency-only kept these accepted:
phrasing 04ecfa0fe8ed11No newer commits appear yet this head was already reviewed at …truefalse❌No newer revisions indicate yet this head was already reviewed at …truefalse❌No other branches suggest yet this head was already reviewed at …truefalse❌appear,indicate,suggest,show,mention,reportandclaimare all inEPISTEMIC_HEADand all take a bare plural subject, so the exposure is the whole present-tense half of that list.- The discriminator that actually separates the two roles is the head's part of speech, not the auxiliary: an adjective can precede a noun head (
no clear evidence yet), whereas for a verb head the intervening words are necessarily the subject (no commits appear yet). So apply the{0,2}slack only to the noun subset and keep adjacency for the rest:Simulated with(?:${NEGATION_PREFIX_WORD}\s+(?:(?!aux)\w+\s+){0,2}${EPISTEMIC_NOUN_HEAD} |${NEGATION_PREFIX_WORD}\s+${EPISTEMIC_HEAD})\s+(?:${ADVERSATIVE_CONNECTIVES})\bEPISTEMIC_NOUN_HEADas the noun members already in the list (evidence|indications?|signs?|records?|proofs?|traces?|suggestions?|statements?|assertions?|awareness|findings?|validations?|confirmation|verification|determination): all three regressions return totrue, all six adjective cases stayfalse, and combined with the fix above your table is 250/250, zero regressions.
- The discriminator that actually separates the two roles is the head's part of speech, not the auxiliary: an adjective can precede a noun head (
Suggestions (1)
- [code]
scripts/general-server-shard-durations.json— the refresh is mechanical and fine (coverage 443→483), but two small things: the$commentstill reads "most recently on 2026-08-24" while the coverage figure moved, so the regenerated sentence is now internally inconsistent; andheartbeat-context-summary.test.tscarries a weight of54ms while this PR adds 676 lines of cases to it. Neither is blocking — the workflow re-samples weekly — but the stale weight will under-fill that shard until it does.
Strengths
- The consequence/adversative distinction is the right abstraction, and the comment at
:9536-9541states it precisely enough that I could test the code against its own stated invariant — which is exactly how both findings above surfaced. That is a comment doing real work. - The auxiliary exclusion is a genuinely clever discriminator, and it holds for every phrasing that has an auxiliary; the gap is only where the construction supplies none.
- The added case table is well-chosen: it pairs each new accept with the near-miss reject that would break it, which is why a 250-case regression check is even possible on a change this subtle.
- Escalating
doubt's{0,2}bounded-filler shape (pass 11) to the negation/head gap is a consistent, precedented move rather than a new one-off.
Recommended Action
- Move
consequently|accordinglytoCONSEQUENCE_CONNECTIVES(leavethen). - Restrict the
{0,2}slack to noun heads, keeping adjacency for verb heads — this closes theappear/indicate/suggestregression. - Both together measured 250/250 on your existing table with zero regressions, so they should land as one commit with the six new cases above added to the table.
Ally pass 18, two Important findings, both in my pass-17 fix.
1. The consequence/adversative split was decided by WORD, not by sense.
`consequently` and `accordingly` have no adversative reading -- they are
exact synonyms of therefore/hence, which the same commit moved to the
consequence set -- so an identical frame diverged purely on which synonym
the run wrote: "nothing else found THEREFORE this head was" accepted,
"...CONSEQUENTLY..." refused. Moved. `then` stays adversative on purpose:
it has a real filler reading ("no evidence THEN that" = at that time), so
its veto is defensible.
2. The `{0,2}` slack absorbed a SUBJECT noun phrase. The auxiliary exclusion
marks the connective role only for verbs that TAKE an auxiliary; a
present-tense verb has none, so "no newer commits APPEAR yet this head was"
had its subject swallowed by the filler and `appear` consumed as the
negation's head -- a false veto, regressed from the parent. The real
discriminator is part of speech: an adjective may precede a NOUN head
("no clear evidence yet"), but for a VERB head the intervening words are
necessarily the subject. Slack now applies to the noun subset only
(EPISTEMIC_NOUN_HEAD); verb heads keep adjacency. appear/indicate/
suggest/show/mention/report/claim -- the whole present-tense half of the
list -- were exposed.
Suggestion taken: the shard manifest's "most recently on" date was stale
against its own coverage figure, and heartbeat-context-summary.test.ts
carried a weight of 54ms while this PR grew it to ~7.5s; re-measured.
Mutation-tested:
consequently/accordingly back to adversative -> 1 failure
slack applied to all heads again -> 3
verb arm removed entirely -> 6
543 tests pass across the five heartbeat suites; typecheck clean.
Refs BLO-31374
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Eighteenth pass addressed at Important 2 is the sharper one and your diagnosis was better than my design. I had written that the auxiliary exclusion is what marks the connective role. It does — but only for verbs that take an auxiliary. A present-tense verb has none, so Important 1 — moved Suggestion — manifest date corrected, and the suite's weight re-measured: it was
543 tests across the five suites; typecheck clean. One thing I want to say directly: passes 17 and 18 are both defects in my fixes to the previous pass, not in the original PR. The comment you quoted — the one precise enough that you could test the code against its own stated invariant — is what caught both. I'll keep writing them that way. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a60804a
Prior Findings Dispositioned (2)
- prior:fe8ed11 important 1 — fixed —
server/src/services/heartbeat.ts:9542—consequently|accordinglymoved intoCONSEQUENCE_CONNECTIVESand removed fromADVERSATIVE_CONNECTIVES(:9543-9544), with the rationale recorded at:9545-9549andthendeliberately left behind, exactly as scoped. Re-measured at this head: all six synonym frames (so/because/since/therefore/thus/hence) plusconsequently/accordinglynow returntrue, so the frame no longer diverges on which synonym the run happened to write. Pinned in the table atserver/src/__tests__/heartbeat-context-summary.test.ts:1340-1342. - prior:fe8ed11 important 2 — fixed —
server/src/services/heartbeat.ts:9748—EPISTEMIC_NOUN_HEADadded at:9620-9621and the arm split so the{0,2}slack applies only to the noun subset, with the part-of-speech rationale at:9622-9628. Re-measured: all nine present-tense verb heads (appear,indicate,suggest,show,mention,report,claim,confirm,establish) return totrue, and the seven adjective+noun hedges (No clear/strong/direct/hard/real/further/conclusive evidence yet …) stayfalseundernegatedHeadConnective. Pinned atheartbeat-context-summary.test.ts:1344-1348.
Critical Issues (0)
Important Issues (1)
-
[native-codex / gstack-review]
server/src/services/heartbeat.ts:9748-9749— the noun/verb split fixes the subject-swallowing case but makes the verb arm strictly adjacent, and an adverb is a third thing: it sits between the negation and a verb/adjective head without being a subject. So the veto stops reaching the clause and the hedge is accepted asalready_reviewed— the masking direction. The comment at:9622-9628says "for a VERB head the intervening words are necessarily the SUBJECT"; that holds forno commits appear, but not forcannot fully confirm, where the intervening word is a degree adverb.This is a regression this head introduces, verified against the parent harness at
fe8ed114, where the uniform{0,2}slack still caught them:phrasing fe8ed11a60804a1I cannot fully confirm yet this head was already reviewed at …falsetrue❌I cannot definitively say yet this head was already reviewed at …falsetrue❌We do not currently believe yet this head was already reviewed at …falsetrue❌I have not conclusively verified yet this head was already reviewed at …falsetrue❌I am not fully aware yet this head was already reviewed at …falsetrue❌No longer aware yet this head was already reviewed at …falsetrue❌Measured 12/12 across
fully,really,currently,definitively,conclusively,independently,reliably,entirely,clearly,firmly,longer. The exposure is the whole verb-and-adjective half ofEPISTEMIC_HEAD(confirm,see,say,believe,verify,determine,aware, …), since every one of them takes a pre-verbal adverb.- Give the verb arm adverb-only slack, keeping adjacency for everything else:
Use a closed adverb set, not
|${NEGATION_PREFIX_WORD}\s+(?:${EPISTEMIC_ADVERB}\s+){0,2}${EPISTEMIC_HEAD}\w+ly. I simulated both. A closed set (yet|longer|ever|still|now|quite|fully|really|actually|truly|entirely|completely|definitively|conclusively|reliably|independently|positively|currently|firmly|confidently|necessarily|clearly|directly|certainly|categorically|absolutely) gives 70/70 — zero regressions: all 12 hedges return tofalse, all ten pass-18 subject cases staytrue, the seven adjective cases stayfalse, and the consequence-synonym and canonical-shape rows are untouched.\w+lyscores worse than this head does, because English nouns end in-lytoo — it false-vetoed 6/6 ofNo anomaly/reply/assembly/family/supply/monopoly indicates yet …, which is the false-missingdirection this PR exists to remove. - The reason this slipped through is worth a test row rather than just a fix: the only adverb-shaped case in the suite is
I could not fully confirm **whether** …(heartbeat-context-summary.test.ts:882and:1117), and its explicit complementizer is caught by thequestionedcue independently ofnegatedHeadConnective, so it passes either way. The elided-complementizer form has no coverage. Addcannot fully confirm yetandnot fully aware yetto the table atheartbeat-context-summary.test.ts:1344-1348, beside the pass-18 rows they pair with, and anegatedHeadConnectiverow to the attribution table at:1370.
- Give the verb arm adverb-only slack, keeping adjacency for everything else:
Suggestions (1)
-
[gstack-review]
server/src/services/heartbeat.ts:9776— pre-existing, not introduced here (identical atfe8ed114), but it is the same invariant this head is tightening, so worth folding in while the area is open. Thenegationstem's pre-head filler is a bare(?:\w+\s+){0,3}, with none of the connective-role exclusion thatCOPULA_FILLER(:9590) carries.\w+matchesso/since/therefore, so a negation governing the previous clause can reach across a consequence connective and veto the next one — precisely the failure the comment at:9529-9534says the code holds against, which is true only on the post-head side. Measured at this head: 80/100 correct-skip phrasings of the form<negated clause> <consequence connective> <epistemic verb> this head was already reviewed at …are false-vetoed, all attributed tonegation:phrasing verdict Nothing changed so I see this head was already reviewed at …false❌Not stale since I verified this head was already reviewed at …false❌No drift so we confirm this head was already reviewed at …false❌Note the outcome currently turns on word count rather than sense —
No newer commits so I confirm …is accepted (three filler words exhaust the{0,3}) whileNothing changed so I see …is vetoed. I have not simulated a fix here, because naively excluding the connectives risks the filler-role trade the fifteenth pass measured at 12:12, and that deserves its own pass rather than a rider on this one. Flagging it as the next thing in this file, not as something to resolve before merge.
Strengths
- The two prior findings were implemented as scoped rather than approximated:
thenwas left inADVERSATIVE_CONNECTIVESwith the filler-reading rationale recorded (:9547-9549), andEPISTEMIC_NOUN_HEADis a genuine subset ofEPISTEMIC_HEADrather than a re-typed copy that would drift. EPISTEMIC_HEADmoving from enumerated surface forms to morphological lemmas (:9629-9640) addresses the class of defect — the comment at:9614-9619states plainly why enumerating inflections does not converge. That is the right altitude for a list that seven prior passes each patched by one word.prReviewAlreadyReviewedVetoCue(:9789) is a real diagnostic win: cue attribution was hand-bisected in every prior pass, and it made this review's regression attributable in one call instead of by elimination.- ReDoS envelope holds after the added alternation — 15.2 ms on a 280 KB output, 32.7 ms on 20k decimal-token matches, 3.3 ms on a 45 KB comma-free run-on. No backtracking blowup from the second
NEGATION_PREFIX_WORDbranch. scripts/general-server-shard-durations.jsoncorrectly re-measures the suite it grew (54→7485ms) rather than leaving it on the median fallback.
Recommended Action
- Fix the Important issue before merge — the adverb gap is the masking direction, and it regresses behavior the parent head had right.
- Add the two elided-complementizer adverb rows to the existing tables so the gap is pinned, not just closed.
- Consider the Suggestion opportunistically, or as a follow-up pass; it is pre-existing and independent of this change.
1 participant
Thinking Path
Ally flipped to
erroron 2026-09-02 and I traced every lease release in the 23:29–23:46Z window rather than the last one. Eleven of fifteen were Anthropic pool capacity ("out of extra usage" / throttle), which no code change fixes. Two were runs that had exited cleanly under the one-review-per-(PR, head) contract yet were classifiedpr_review_output_missing, so I pulled both run logs and read the finalresultchunk verbatim instead of guessing at the shape. The existingalready_reviewedbranch only matched the plainalready reviewed at <timestamp> for <sha>string, and the reviewer writes that clause markdown-formatted, with the sha either directly afterator wrapped in backticks afterfor. I kept the sha requirement (a sha-less "already reviewed" claim staysmissing) and deliberately did not anchor to the wake head, because on atransient_failure_retrywake the wake head is exactly what went stale and the cited sha is the live head, which the wake context cannot know.Linked Issues or Issue Description
Refs BLO-31374 (Paperclip tracker; no GitHub issue exists for this). Runs
b7a984bf-a5e1-44ac-a0e3-3942c8d42789(onprem-k8s#3023) and3ace1eef-5628-4a3d-894e-08a23af27e05(penstock-vault-node#554).b7a984bf**Already reviewed at \8b237675b19f…`** — no action taken.`at; bold + backticks3ace1eef**Already reviewed at 2026-09-02T20:41:53Z for \90193c30abb9…`**`forand the sha defeats\bfor\b\s+[0-9a-f]{7,40}What Changed
server/src/services/heartbeat.ts: the inline regex inevaluatePrReviewCompletionEvidenceis replaced byprReviewOutputHasAlreadyReviewedSkip, which tolerates whitespace/backticks/bold between tokens, accepts the sha afterfor, directly afterat, or after ahead/commitnoun, still requires a 7–40 hex sha, and rejects three masking shapes: a negated clause (the negation prefix tolerates the same markdown as the clause, so**not**cannot slip past), a hedge that governs the clause in its own clause ("could not confirm whether …", "Unclear if …"), a prior/stale-head subject before the clause in its own sentence ("the prior head was already reviewed at …"), and an epistemic negation governing the clause from further back in the same clause ("no evidence this head was already reviewed at …", "cannot see that … already reviewed at …"). A non-epistemic negation explaining the skip ("Exiting without posting since …", "No action taken because …") is deliberately not a veto. Stale-head narration after the clause is how a correct skip explains the wake and is deliberately not a veto. "Did not post" is deliberately NOT a veto: not posting is the defining property of this exit. Neither real run text trips a veto. Thealready_reviewedstatus, its position in the precedence chain, and the existing plain-shape test are unchanged.server/src/__tests__/heartbeat-context-summary.test.ts: twelve new cases — both real run texts verbatim, ahead <sha>variant, a no-sha masking guard, four negation forms (plain, bold, italic,not yet been), three hedge/prior-head forms, and a precedence case where a hedge in an earlier sentence must not veto a later unhedged clause.Verification
vitest run src/__tests__/heartbeat-context-summary.test.tspasses 170/170 (93 baseline + 77 new, including a direct 20-row table over the exported predicate). The four sibling suites that referenceevaluatePrReviewCompletionEvidenceoralready_reviewedpass 203/203.pnpm typecheckinserver/exits 0 with no TS errors. Mutation-tested three ways: restoring the old regex fails the three new shape tests, removing the negation guard fails the negation test, loosening the sha quantifier to{0,40}fails the no-sha test; reverting the negation prefix to bare\s+fails the bold and italic cases; removing the prior-head veto fails the prior-head case; loosening the hedge cue to barewhether|iffails the same-clause bare-whether row; reverting the clause boundary to period-only fails the joined-hedge rows; restoring the after-clause arm fails the five stale-head-narration rows; reverting the clause interior to bare\s+fails the three markdown-before-sha rows; removing the before-scope negation cue fails six rows; reverting it to a bare negation word list fails twelve; removingfailed tofrom the hedge fails two; reverting theatseparator to*fails one.Risks
The recognizer is a text classifier, so the residual risk is masking: a run that genuinely failed to post could be accepted if its output contains an "already reviewed at " clause. The sha requirement and negation guard bound that, and the branch already existed with no anchor at all, so this PR does not widen the trust surface beyond the shapes the reviewer actually emits. No runtime behaviour changes for runs that post a review.
Checklist
evaluatePrReviewCompletionEvidenceor thealready_reviewedbranch)Model Used
Claude Fable 5.1 (claude-fable-5-1) via Claude Code, driven by kkroo.
🤖 Generated with Claude Code