fix(review): extend the suppressions list with common LLM false positives

[frosimanuel]

Why (in your own words)

The suppressions list in review/checklist.md ("DO NOT flag these") is good but short. This adds eight more entries in the same voice, chosen by the same criterion as the existing ones (would a senior engineer on this team actually change it in review?): error handling the framework owns, validation on an internal function whose callers validate, long exhaustive switches, null deref after a type guard, "N+1" on a fixed-cardinality loop, deliberate fire-and-forget, hardcoded values in test fixtures, Math.random() for jitter. These are the non-findings we dismiss by hand most often when running /review on our own repos. Plus one sentence at the top of the section: zero findings is a valid, expected outcome; do not withhold approval to look rigorous.

The behaviour this targets is concrete and shows up live (below): on a clean 19-line diff, the current skill promoted a pre-existing design contract to a CRITICAL 10/10 finding and rewrote the PR's public API, test and description to "fix" it. With this change, same model, same diff, same prompt, it reports the same observation as "one thing to know, not a finding" and touches nothing.

What I tried and dropped: a "green is not verified" block in SKILL.md.tmpl (typecheck is a separate gate, skipped is never PASS, exit codes through pipes, numbers need pwd/branch). On a fixture with a piped typecheck, a test.skip and a real TS2322, the unpatched skill already measured the pipe (raw exit 2 vs piped 0), re-enabled the skip and corrected the PR description, so the block showed no delta in findings. It also pushed the always-loaded skeleton to 62539 bytes, over the parity gate's 61500. This PR touches only checklist.md; the skeleton is unchanged at 61314 bytes.

Live evidence

A fixture repo, a tiny TokenBucket package with bun test and a bun run typecheck CI step. The skill directory (review/) was copied into .claude/skills/review/ from upstream main ("before") and from this branch ("after"), and each copy was run with GSTACK_SESSION_KIND=spawned claude -p "/review this branch (...) against main. The PR description is in PR.md ..." --setting-sources project, same prompt, same fixture, same model. One disclosure: the skill reads its checklist from ~/.claude/skills/gstack/review/checklist.md, so in both copies that one path was rewritten to point at the copied checklist; nothing else differs from upstream main / this branch respectively.

A clean branch

Branch feature/peek, 19 lines: a peek(): number accessor that returns the current token count without consuming, one test, PR.md with an honest green claim (typecheck exit 0, 4 pass, 0 skip). Nothing wrong with the diff for a reviewer who accepts the module's existing contract (refill is caller-driven; take() reads the same lazily-refilled counter).

Before (upstream main): "3 issues (1 critical, 2 informational) — all fixed." It rated the lazily-refilled count a CRITICAL 10/10 ("peek() returned a stale count, defeating the PR's own purpose"), then, without being asked, changed the public signature to peek(nowMs = Date.now()), added a private tokensAt() helper shared with refill(), rewrote the test, and edited PR.md. It closed with STATUS: DONE_WITH_CONCERNS and three pre-existing main bugs (take(NaN), take(-1000), backwards clock).

After (this branch): "Pre-Landing Review: No issues found." It made the same observation and classified it differently:

One thing to know, not a finding: peek() reads the lazily-refilled counter, so an idle bucket that has actually recovered still reports its depleted count until someone calls refill(nowMs). That's the pre-existing contract — take() at src/rate-limit.ts:19 behaves identically ... Nothing to change here. ... Both adversarial passes landed on this same observation and both classified it as no-fix.

Zero files touched, STATUS: DONE. Same model, same diff, same prompt. The difference is the skill telling it that a clean diff is allowed to come back clean, and that pre-existing contracts are not findings against the PR.

Caveat: the auto-applied fix in the "before" run happened because GSTACK_SESSION_KIND=spawned makes the skill take the recommended option instead of asking; interactively it would have been an ASK. The finding itself (critical, 10/10) is the point, not the auto-apply.

Repeated, because one run each proves nothing. Five runs in total on this fixture:

run	skill	result
1	upstream main	3 issues, 1 CRITICAL 10/10; changed peek() signature, test and PR.md; DONE_WITH_CONCERNS
2	upstream main	"FIXED (1 critical)", confidence 10/10, same signature change; DONE_WITH_CONCERNS
3	this change, draft form*	"No issues found"; 0 files touched; DONE
4	this change, draft form*	1 informational at confidence 5/10 ("verify this is actually an issue"), "Skip — no code changed"
5	this change, as committed	"No issues found"; 0 files touched; DONE

* Runs 3 and 4 used an earlier draft of this branch that also carried the "zero findings" sentence in SKILL.md.tmpl and a "green is not verified" block there (dropped, see Why). Run 5 is the exact content of this PR: the sentence and the eight entries in checklist.md, SKILL.md identical to upstream. The quotes above ("One thing to know, not a finding...") are from run 3; run 5 says the same thing in its own words: "correct-by-design here: take() reads this.tokens identically ... neither charged against this PR".

Same observation every time. Upstream promoted it to a critical finding and rewrote the PR 2 out of 2 times; with the checklist change it stayed a note 3 out of 3 times.

Full claude -p transcripts for all runs are attached as a comment on this PR.

Scope

• Changed: review/checklist.md only: one sentence at the top of "Suppressions — DO NOT flag these" and eight entries appended to it. bun run gen:skill-docs produces no diff.
• Verified live by: claude -p runs of upstream main vs this branch on a clean 19-line fixture branch; see Live evidence. Tier 1 bun run test on this branch (see note below).
• Did NOT test: Tier 2 E2E (bun run test:e2e).

Tier 1 note. bun run test on this branch on my machine (macOS, no Aside): 2 files fail, test/codex-hardening.test.ts (bash-native watchdog timing) and test/aside-render.test.ts (needs a browse binary). Both fail identically on upstream main (0530392) on the same machine, and neither touches review/. test/parity-suite.test.ts and the skill budget tests pass (this PR keeps review/SKILL.md byte-identical to upstream). A per-branch comparison against main is in a comment below.

Liveness proof (required)

Checklist

• Liveness screenshot attached: GSTACK PR typed live into a real surface (not edited onto the image)
• This is not a generated-file-only diff (checklist.md is hand-written; no generated file changes)
• No ETHOS.md edits, and no changes to voice / founder perspective / YC references
• New public command / external service / host adapter has an accepted issue linked (or N/A) — N/A
• Linked issue or reproduction: see Live evidence

🤖 Generated with Claude Code

https://claude.ai/code/session_01EYQXGocbEnQvhMgpPFhVZu

[trunk-io]

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[frosimanuel]

Full claude -p transcripts (assistant text + tool calls, extracted from the session .jsonl): https://gist.github.com/frosimanuel/ce9c08b86d1e2c7571ac2c8cf2c33445

Files 01–05 are the five clean-diff runs in the table in Live evidence. 06/07 are the bad-branch fixture used for the "green is not verified" block that was dropped from this PR (no delta in findings), included so the decision to drop it is checkable.

[frosimanuel]

Tier 1 comparison against main (promised in the PR body)

Short version: I could not get a fully green Tier 1 on this machine on any ref, including upstream main at 0530392. Every failure on the PR branches is a timing/watchdog test in a file the PR does not touch, and the failure set shifts from run to run. The parity/skeleton-budget tests (test/parity-suite.test.ts) passed on every branch and never appeared in a failing list. Please treat CI as the authoritative gate; this comment exists so the numbers are on the record rather than hand-waved.

Method. bun run scripts/test-free-shards.ts --wall-timeout 1800, one branch at a time, nothing else of mine running. Each branch is a single commit on top of upstream main 0530392. Same macOS box for all runs.

Ref	Commit	Failing tests / files	Failing files
upstream main	0530392	10 / 9 (+1 unhandled error between tests)	aside-render, busy-daemon-iron-rule, daemon, diff-scope, e2e/diagram-gate, eval-cli-family, hook-scripts ×3, relink, server-no-import-side-effects
pr/plan-reviews-design-doc-is-data (#2818)	0016f67	3 / 3	e2e/combined-gate, gbrain-local-status, gstack-memory-helpers
pr/plan-ceo-review-phasing-check (#2819), run 1	793d010	60 / 23	see note below
pr/plan-ceo-review-phasing-check (#2819), run 2	793d010	6 / 6	e2e/diagram-gate, e2e/landscape-gate, gbrain-local-status, gstack-memory-helpers, pair-agent-e2e, telemetry
pr/review-false-positives (#2820)	332f360	3 / 2	aside-render ×2, codex-hardening

What the PRs touch. #2818: plan-ceo-review/SKILL.md{,.tmpl}, plan-eng-review/SKILL.md{,.tmpl}. #2819: plan-ceo-review/SKILL.md{,.tmpl}. #2820: review/checklist.md only. None of the failing files above reads those artifacts.

About the 60-failure run on #2819. That run coincided with the machine swapping hard (swap 8.1 GB used of 9.2 GB, ~16 MB of free pages). Individual tests reported 400+ second durations and spawnSync /bin/sh ETIMEDOUT, across 23 unrelated files (brain-sync, relink, hook-scripts, gstack-skill-start, slug-parity…). The immediate re-run of the same commit, same command, dropped to 6 timing-flavoured failures. I am reporting the bad run rather than hiding it, but I don't think it says anything about the two-line change in the branch.

Why the sets don't overlap. aside-render needs a browse binary that this box doesn't have; codex-hardening is a watchdog test; the rest are cooldown/timeout/e2e-render gates that flip under memory pressure. The same kind of test fails on main here.

What I did verify deterministically on each branch: bun run gen:skill-docs is a no-op after the commit (generated SKILL.md matches the template), and the skeleton byte budget holds (56489 / 78735 on #2818 vs 56500 / 79000 caps; #2820 leaves review/SKILL.md byte-identical to main at 61314).

If a maintainer wants a specific failing file re-run in isolation on this branch, say which and I'll attach the log.