review: post-merge fold fixes from #365, #370, and #371 - #378
Conversation
The consequential one is sequencing: the dispatcher-death prose said 'report the run incomplete, then continue at Step 9', and reporting incomplete can end the turn, so the cache compensation the death path exists to trigger could be skipped with it. Step 9 now runs first, and the guidance-repost claim is scoped to the next full-depth run. The ban test folds whitespace before matching (a wrapped 'gh aw update' counted as no mention), gains a positive control so a broadened exclusion fails rather than passing vacuously, and checks existence before reading so the stale-allowlist message renders; git grep -l was considered and declined in a comment, since line-based matching is exactly what misses the wrapped form. foldToken's three length thresholds are documented and the empty-subject-tokens early return is pinned.
🦋 Changeset detectedLatest commit: 266849c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
suggestion (non-blocking): The whitespace fold this PR adds is not pinned by any test, so it can regress silently.
Full finding
The whitespace fold this PR adds is not pinned by any test, so it can regress silently. Introduced by this change. mentionsTool is the PR's headline hardening — it exists so a prose-wrapped gh aw\nupdate cannot slip the substring check — but nothing in the file fails if the fold is removed.
Cheap check I ran: reverted mentionsTool to a plain text.includes("gh aw update") and re-ran the sweep over the current tree (git ls-files with the same three exclusions, then read each file). All five ALLOWED files still match and offenders is still empty, so all three tests stay green without the fold. That is because every allowlisted file happens to carry the phrase on one line today; the new positive control only proves the sweep reaches those files, not that it folds whitespace on the way.
So the guarantee is load-bearing on repo content that could change at any doc rewrap. One direct assertion on the helper pins it:
it("counts a line-wrapped mention (the whitespace fold)", () => {
expect(mentionsTool("never recommend gh aw\nupdate here")).toBe(true);
expect(mentionsTool("gh aw compile; update the lock")).toBe(false);
});The negative half matters too — it is what keeps the fold from being widened into a match on any gh aw plus a later update.
review details
found by correctness-reviewerFull finding
Gating the incomplete report on the cache CLI alone still leaves Step 9's out/ artifact upload exposed to the same turn-ending hazard. Pre-existing hazard; this change narrows it rather than amplifying it, so this is a note, not a blocker — but the residual is worth a word since the sentence is being rewritten anyway.
Step 9 has two emissions, not one. First the cache-record CLI, then (I read Step 9 to confirm) "Finally, if you wrote any sub-agent outputs to /tmp/gh-aw/review/out/ this run (Step 3), upload that directory as a run-scoped artifact with the upload-artifact safe output." The new wording gates the incomplete report on the CLI only, so an orchestrator that follows it literally may report incomplete between the two — and by the hunk's own premise ("reporting incomplete can end the turn") that drops the out/ upload.
That matters on precisely this path. The motivating incident named two lines down, run 32418662895, was killed during claim validation, i.e. late enough that out/ held most of the sub-agent results. On a death with no plan and no posted review, that directory is the only evidence of what the run actually saw, and the death notice itself carries none of it.
The old ordering put the report ahead of all of Step 9, so both the cache write and the upload were at risk; this change rescues the cache write. Extending the gate to the whole step rescues the artifact too.
review details
found by correctness-reviewerreview details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| * The three length thresholds all protect stem recognizability: a token of | ||
| * 3 or fewer chars is returned whole (too short to carry a strippable | ||
| * suffix; "les" is a word, not "le"+s), a suffix strip must leave a stem of | ||
| * at least 3 chars ("goes" loses "es" only because "go"+es fails the floor |
There was a problem hiding this comment.
suggestion (non-blocking): The middle threshold's example says "goes" loses "es" but the code (and the same sentence) keeps "goe". The PR's stated #365 item is documenting foldToken's three length thresholds; tracing the loop on "goes" ("es" fails 4-2>=3, no break, then "s" passes 4-1>=3) shows it loses only the "s", which contradicts the clause's own "loses 'es'" wording. The other two threshold explanations check out against the code.
| * at least 3 chars ("goes" loses "es" only because "go"+es fails the floor | |
| * at least 3 chars ("goes" keeps its "e": "go"+es fails the floor, so it | |
| * falls through to the "s" strip and folds to "goe"), and the trailing-e |
Lower-confidence observations (1)
workflows/review/gh-aw-update-ban.test.ts:88nitpick (non-blocking): Test title leads with a category prefix instead of the file's verb-phrase form. (conventions)
review details
found by completeness | also flagged by first-principles: The threshold comment misdescribes the code it documents.; documentation: The added foldToken threshold docstring's "goes" example contradicts what the code does.| * matches within a line, so the wrapped form would slip past it. | ||
| */ | ||
| const mentionsTool = (text: string): boolean => | ||
| text.replace(/\s+/g, " ").includes("gh aw update"); |
There was a problem hiding this comment.
suggestion (non-blocking): The whitespace fold — this PR's headline test fix — is not pinned by any assertion. I checked all five ALLOWED files with and without the fold applied: each contains the literal gh aw update on one line, so the positive control and the stale-allowlist detector both pass with the fold removed, and grep -rn mentionsTool shows no other caller. A one-line direct assertion on mentionsTool would pin the behavior independently of incidental repo content.
| text.replace(/\s+/g, " ").includes("gh aw update"); | |
| it("counts a line-wrapped mention (whitespace fold)", () => { | |
| expect(mentionsTool("...recommend gh aw\nupdate to repin...")).toBe( | |
| true, | |
| ); | |
| expect(mentionsTool("gh aw update")).toBe(true); | |
| expect(mentionsTool("gh awful updates")).toBe(false); | |
| }); |
review details
found by test-adequacy| drops `risksPatternsKey` so the next run reposts the guidance comment | ||
| your death notice collapsed, and leaves the prior fingerprints | ||
| and noting that the next push triggers a fresh full review; then skip | ||
| Steps 4-8, continue at Step 9, and report the run incomplete only AFTER |
There was a problem hiding this comment.
question (non-blocking): The reordered death path defers the incomplete report only past Step 9's cache CLI, not past Step 9's upload-artifact step. Pre-existing hazard — the old wording put the report ahead of all of Step 9, so this change narrows it rather than introducing it — but the residual is worth a word since the sentence is being rewritten anyway.
Step 9 has two emissions: cache-record.ts, then "Finally, if you wrote any sub-agent outputs to /tmp/gh-aw/review/out/ this run (Step 3), upload that directory as a run-scoped artifact". lib/dispatch.ts writes each agent's result to out/<agent>.json as it goes, so on the run 32418662895 shape (killed at the Bash ceiling mid-claim-validation) that directory is the only evidence of what the run saw — and by the hunk's own premise ("reporting incomplete can end the turn") an orchestrator that reports right after the cache CLI drops it. Extending the gate to all of Step 9 rescues the artifact too.
review details
found by skill-auditor (out-of-lane) | also flagged by holistic| // Without this, a broadened exclusion (or a broken read) would make | ||
| // the offenders sweep pass vacuously. The same sweep, WITHOUT the | ||
| // allowlist filter, must find every allowlisted file. | ||
| const found = sweptFiles().filter((file) => |
There was a problem hiding this comment.
suggestion (non-blocking): The positive control's only new claim is sweep membership, which needs no file reads. The third test (lines 100-113) already reads each ALLOWED file and asserts mentionsTool is true, so the read half of the positive control is redundant; the only claim unique to it is that a broadened exclusion in sweptFiles() has not dropped an allowlisted path. Asserting membership in the swept path list alone states that claim exactly and costs one git ls-files.
| const found = sweptFiles().filter((file) => | |
| const swept = new Set(sweptFiles()); for (const file of ALLOWED) { expect(swept, `sweep no longer reaches ${file}`).toContain(file); } |
review details
found by first-principles
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
2 participants
The final approvals on #365, #370, and #371 landed after their last pushes, so their folds (12 observations) had no branch left to fix; this carries the ones worth taking.
Not carried: #370's four standing threads (plan-CLI death, dead-process thought, green-run remedy, report_incomplete's surface) are PRA-49's scope, and #365's HOLD-list length question is what the bodyStats instrumentation that PR staged exists to answer.
1956 tests green locally.
KORE-2498