From 8d604c15dea32753cfa33e9d0a0b25290302c402 Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 13 Jul 2026 03:48:39 +0900 Subject: [PATCH] =?UTF-8?q?feat(reviewing-prs):=20fix-forward=20finding=20?= =?UTF-8?q?routing=20=E2=80=94=20native=20severity=20as=20the=20blocker=20?= =?UTF-8?q?bit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ROUTE flips the burden of proof (agent-harness direction): the engine's critical/high (P1 class) is the blocker bit → FIX NOW; every finding below it defaults to the new LOG bin (was TOO SMALL's argued deferral) and lands on the standing tech-debt issue. Promoting a non-blocker to an in-loop fix is now the exception that needs a stated reason. RE-REVIEW gains a statelessness clause: re-review rounds re-flag logged findings; match by file+substance, never fix/re-log/re-count them, and exit on 'no NEW blocker', not a clean report. SELF-MERGE wording aligned; SKILL.md tech-debt sink updated; spec gets a 2026-07-13 Revision Note. Motivation: SD-5 dogfood routed 7/7 findings FIX NOW across 3 engine rounds — the old default spends momentum on non-blockers. --- .../specs/2026-07-08-pr-review-loop-design.md | 13 +++++++ skills/reviewing-prs/SKILL.md | 5 +-- .../references/review-worker-protocol.md | 35 +++++++++++++------ 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/docs/doperpowers/specs/2026-07-08-pr-review-loop-design.md b/docs/doperpowers/specs/2026-07-08-pr-review-loop-design.md index 7e0f8a14c3..0a648a2fb7 100644 --- a/docs/doperpowers/specs/2026-07-08-pr-review-loop-design.md +++ b/docs/doperpowers/specs/2026-07-08-pr-review-loop-design.md @@ -502,3 +502,16 @@ Pending — written at finish. recap could be read as licensing a merge in observation mode → now gated on "auto-merge is on", pinned by a regression assert) and two LOW (off-state prose wording; full-clone requirement for the base read, now documented). +- 2026-07-13 (fix-forward routing): flipped the finding-routing default, + adopting agent-harness's burden-of-proof direction after SD-5 showed the + old default in action (7/7 findings routed FIX NOW, 3 engine rounds). + The engine's native severity is now the blocker bit: critical/high (P1 + class) → FIX NOW; everything below → new LOG bin (renamed from + TOO SMALL), deferred to the standing tech-debt issue by default — + promoting a non-blocker to an in-loop fix is the argued exception, the + reverse of the old triple-condition deferral. RE-REVIEW gains a + statelessness clause: re-flagged already-logged findings are matched by + file+substance, not re-fixed/re-logged/re-counted; exit = no NEW + blocker, not a clean report. SELF-MERGE wording aligned ("non-blocker + findings, each explicitly routed"). Protocol + SKILL.md tech-debt sink + updated; spec body §Review Worker Protocol left as the historical draft. diff --git a/skills/reviewing-prs/SKILL.md b/skills/reviewing-prs/SKILL.md index 49e79e7491..33080d360b 100644 --- a/skills/reviewing-prs/SKILL.md +++ b/skills/reviewing-prs/SKILL.md @@ -129,8 +129,9 @@ PR-review-event trigger arrives with runner registration. ## Tech-debt sink -Small valid-but-non-blocking findings go to ONE standing GitHub issue per -repo (label `tech-debt`) as structured comments — never to a tracked file: +Non-blocking findings — everything below the engine's critical/high class +— go by DEFAULT to ONE standing GitHub issue per repo (label `tech-debt`) +as structured comments — never to a tracked file: parallel workers on branches editing one file is a merge-conflict factory, and the edit would land inside the very PR under review. Register the standing issue as a `deferred` P3 ticket so board-lint stays green. Promote diff --git a/skills/reviewing-prs/references/review-worker-protocol.md b/skills/reviewing-prs/references/review-worker-protocol.md index 0659ae7af5..a3b63d361a 100644 --- a/skills/reviewing-prs/references/review-worker-protocol.md +++ b/skills/reviewing-prs/references/review-worker-protocol.md @@ -47,30 +47,43 @@ EVALUATE every finding against codebase reality before acting: for actual usage before accepting the scope. - Fix one finding at a time; test each before the next. -ROUTE each finding to exactly one bin: -- FIX NOW — valid and within this PR's scope: fix, test, commit, push - (git push origin HEAD:{{HEAD_REF}} — you are on a detached HEAD). +ROUTE each finding to exactly one bin. The engine's native severity IS +the blocker bit — trust it, don't re-derive it. Blocker = the engine's +critical/high (P1) class: demonstrable bug, correctness/security issue, +broken behavior, or a test that verifies nothing. Everything below that +defaults to LOG, not to a fix — momentum outranks polish: +- FIX NOW — a verified blocker within this PR's scope: fix, test, commit, + push (git push origin HEAD:{{HEAD_REF}} — you are on a detached HEAD). + Promoting a non-blocker to FIX NOW is the exception, never the default: + it takes a stated reason in the review trail (e.g. the engine + under-rated a real correctness issue). - TOO BIG — valid but new scope (a design fork, a new subsystem, or more than about half the original PR's size): register a ticket — {{BOARD_SCRIPTS}}/board-register.sh "" <bug|enhancement> <P0..P3> --spawned-by {{ISSUE_NUMBER}} — then flesh out its pre-spec body (gh issue edit <new> --body-file -). NEVER fix it in this PR. -- TOO SMALL — valid, non-blocking, and fixing it costs momentum or an - unwarranted re-review round: append a structured comment to the standing - tech-debt issue (gh issue comment {{TECH_DEBT_ISSUE}}) — finding, - file:line, severity, why deferred. +- LOG — valid non-blocker (the DEFAULT for every finding below + critical/high): append a structured comment to the standing tech-debt + issue (gh issue comment {{TECH_DEBT_ISSUE}}) — finding, file:line, + severity, why deferred — and move on. - INVALID — does not hold against the code: rebuttal comment on the PR citing the refuting code. RE-REVIEW (max 3 engine rounds total) when ANY: a critical/high finding led to a fix; cumulative fixes exceed ~50 changed lines or 3 files; any fix changed behavior (not comments/docs/renames). Skip when fixes were trivial -or none. At the cap with unresolved critical/high findings: do NOT grant -confidence — set ticket #{{ISSUE_NUMBER}} to needs-human with an impasse -summary and end your turn. +or none. The engine is stateless: a re-review round WILL re-flag findings +you already logged. Match re-flagged findings against your tech-debt +comments by file and substance (line numbers shift after fixes); a match +is already routed — do not fix it, do not log it twice, do not count it +toward the re-review triggers above. The exit condition is no NEW blocker, +not a clean report. At the cap with unresolved critical/high findings: do +NOT grant confidence — set ticket #{{ISSUE_NUMBER}} to needs-human with an +impasse summary and end your turn. ESCALATE when review is complete. The SELF-MERGE tier requires ALL of: -- final verdict approve (or only low findings, each explicitly routed); +- final verdict approve (or only non-blocker findings, each explicitly + routed); - post-fix diff ≤ ~150 changed lines AND ≤ 5 files; - the PR base ({{BASE_REF}}) is NOT the repo default branch ({{DEFAULT_BRANCH}}); base-is-default: {{BASE_IS_DEFAULT}}. Self-merge lands