Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions packages/cli/src/commands/review/agent-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2471,6 +2471,31 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
expect(p).not.toMatch(/If you find no issues, say/i);
});

it('welds the fix-witness format into the launched finder briefs', () => {
// The fix-witness mandate is pinned in SKILL.md by SKILL.test.ts, but
// this half is the one that actually reaches the agents: the
// FINDING_FORMAT embedded in every finder brief. Deleting the Fix
// witness line — or the exemption clause below it — shipped green once,
// because no test read a BUILT brief; launched finders would stop being
// asked for the criterion and Step 7's posting rule would go inert on
// every agent-built round. Pin both halves through the brief.
const brief = buildRoleBrief(PLAN, '1a');
expect(brief).toContain(
'**Fix witness:** <the test that must go RED if that fix is removed',
);
expect(brief).toContain('**This field never gates reporting**');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the next round, not declined: a two-line assertion pinning the N/A exemption tail. Critical-only mode is active with the window over its test budget (423/400 net test lines), so this round's test budget went to the regression tests witnessing the Critical fixes; the prompt line the pin protects is untouched this round and the finding does not go stale.

中文说明

延后到下一轮,并非拒绝:这是钉住 N/A 豁免尾句的两行断言。当前处于仅处理 Critical 的模式,且窗口测试预算已超(净测试行 423/400),本轮的测试预算用于见证 Critical 修复的回归测试;该断言保护的提示词文本本轮未被触碰,发现不会过时。

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in df6d20e, taking your suggestion as written — the exemption tail is now pinned beside the prefix in the built brief. Mutation-probed: deleting the clause from FINDING_FORMAT reds agent-prompt.test.ts.

// The exemption TAIL, pinned beside the prefix. The prefix assertion
// above stops before it, so deleting or rewording `or "N/A" ...` shipped
// green — and the two copies of the finding format (SKILL.md, pinned by
// SKILL.test.ts, and this embedded one) could drift on exactly that
// clause. Finders would then read a brief that mandates Fix witness with
// no way out, and rounds would start demanding tests for fixes that add
// no guard at all — a rename, a comment, a docs line.
expect(brief).toContain(
'or "N/A" when the fix adds no guard, branch or behaviour a test can pin',
);
});

it('injects generic repository context into reviewers and a narrow verification boundary into Agent 7', () => {
const contextPlan = {
...PR_PLAN,
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/src/commands/review/agent-prompt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ const FINDING_FORMAT = `Format each finding using this structure:
- **Issue:** <one-line statement of the defect>
- **Failure scenario:** <the concrete trigger and the concrete wrong outcome: what input, state, timing, or config makes this code misbehave, and what incorrect output / crash / leak / exposure results>
- **Suggested fix:** <concrete code suggestion when possible, or "N/A">
- **Fix witness:** <the test that must go RED if that fix is removed — the test file and the behaviour it pins — or "N/A" when the fix adds no guard, branch or behaviour a test can pin>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The review-agent prompt's new Fix witness finding-format line (and the guard-owes-a-test paragraph) is pinned by no test, while the parallel SKILL.md clauses are pinned — so the two copies of the finding format can drift with the suite green. SKILL.test.ts pins only SKILL.md text; agent-prompt.test.ts has no snapshot or toContain over FINDING_FORMAT (verified by grep — no matches). A future edit that removes or rewrites the agent-prompt line passes every gate; review agents then stop writing fixWitness, the artifact field is empty, Step 7's comment rule has nothing to quote, and posted comments silently lose the acceptance criterion — the exact unwitnessed-guard hole this PR exists to close.

Suggested change
- **Fix witness:** <the test that must go RED if that fix is removed — the test file and the behaviour it pins — or "N/A" when the fix adds no guard, branch or behaviour a test can pin>
Add a pin in agent-prompt.test.ts mirroring the SKILL one — assert the agent prompt body contains "**Fix witness:**" and the "owes a test" clause.
中文说明

review-agent 提示词中新增的 Fix witness finding 格式行(及「guard 欠一个测试」段落)没有任何测试钉住,而平行的 SKILL.md 条款都被钉住了——于是两份 finding 格式可以在套件全绿的情况下漂移。SKILL.test.ts 只钉 SKILL.md 文本;agent-prompt.test.ts 对 FINDING_FORMAT 没有快照或 toContain(grep 证实无匹配)。未来任何删除或改写 agent-prompt 行的编辑都能通过所有 gate;review agent 随之停止写 fixWitness,artifact 字段为空,Step 7 评论规则无内容可引用,已发布评论悄悄丢失验收标准——这正是本 PR 要关闭的「未见证 guard」漏洞。

— deepseek-v4-flash via Qwen Code /review (v0.21.11)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the next round — this round's batch was bounded (~8 findings) and spent on the three Criticals first. Same work as rc:3824051695: pin the agent-prompt Fix witness line and the owes-a-test clause in agent-prompt.test.ts, mirroring the SKILL pins. Next round.

中文说明

延迟到下一轮——本轮批次有上限(约 8 条 finding),优先处理了三条 Critical。与 rc:3824051695 同一工作:在 agent-prompt.test.ts 中钉住 agent 提示词的 Fix witness 行与「欠一个测试」条款,镜像 SKILL 的钉子。下一轮处理。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the next round — this round's batch was bounded (~8 findings) and spent on the three Criticals first. Same work as rc:3824047036: pin the Fix witness format line and the never-gates-reporting clause in agent-prompt.test.ts. Next round.

中文说明

延迟到下一轮——本轮批次有上限(约 8 条 finding),优先处理了三条 Critical。与 rc:3824047036 同一工作:在 agent-prompt.test.ts 中钉住 Fix witness 格式行与「永不阻断上报」条款。下一轮处理。

- **Severity:** Critical | Suggestion | Nice to have
- **Confidence:** high | low

Expand All @@ -229,7 +230,9 @@ const FINDING_FORMAT = `Format each finding using this structure:
- A line too long to quote whole — a multi-KB single-line Markdown paragraph — may be quoted as a distinctive verbatim **fragment** of at least 12 characters (measured after whitespace collapse); it resolves to the line containing it.
- Fill in **File** and the line number anyway. The path selects the file and the line breaks a tie when the snippet genuinely repeats. Neither is trusted as the answer.

**The failure scenario is the finding's evidence, and it gates reporting.** For a quality finding, state the concrete cost instead of a crash — what is duplicated, wasted, or made harder to change — or quote the rule it violates. A **Suggestion** or **Nice to have** whose failure scenario you cannot fill in concretely **is not a finding: do not report it.** A suspected **Critical** whose trigger you cannot pin down IS still reported, at \`Confidence: low\`, with the scenario naming the mechanism and what remains uncertain — a later verification stage rules on it. "This looks risky", with no nameable trigger and no nameable cost, is how a hallucinated finding reaches a pull request.`;
**The failure scenario is the finding's evidence, and it gates reporting.** For a quality finding, state the concrete cost instead of a crash — what is duplicated, wasted, or made harder to change — or quote the rule it violates. A **Suggestion** or **Nice to have** whose failure scenario you cannot fill in concretely **is not a finding: do not report it.** A suspected **Critical** whose trigger you cannot pin down IS still reported, at \`Confidence: low\`, with the scenario naming the mechanism and what remains uncertain — a later verification stage rules on it. "This looks risky", with no nameable trigger and no nameable cost, is how a hallucinated finding reaches a pull request.

**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you.`;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The Fix-witness mandate and the N/A exemption contradict each other inside the same new paragraph: the mandate says a fix that "adds or changes a guard, a branch, or a behaviour" must fill Fix witness, while the exemption list immediately grants N/A to "a fix whose whole content is deleting code" — and deleting a guard/branch is both a code deletion and a behaviour change, so a guard-removal fix satisfies both clauses with no rule for which wins. A review agent whose Suggested fix is a behavior-changing deletion (the "remove the over-broad null-check", "delete the swallowing catch" shape) reads the deletion exemption and writes N/A; the removal posts with no acceptance criterion, nothing pins the guard's absence, a later change re-adds it (or the removal was wrong), no test goes red, and the hole returns as next round's finding. The witness for a deletion is pinnable (a test asserting the behaviour the guard suppressed fails if the guard is re-added). SKILL.md:773's "adds no guard" wording does not have this contradiction.

Suggested change
**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you.`;
Carve guard/branch removals out of the deletion exemption — e.g. "a fix whose whole content is deleting code that changes no guard, branch or behaviour" — or align the mandate to SKILL.md's "adds" and disclose that removals are N/A.
中文说明

Fix-witness 强制条款与 N/A 豁免在同一个新段落内互相矛盾:强制条款说「新增或改变 guard、分支或行为」的修复必须填 Fix witness,而豁免清单紧接着把 N/A 授予「全部内容只是删除代码的修复」——而删除 guard/分支既是代码删除又是行为改变,因此一个「移除 guard」的修复同时满足两条,却没有规则裁决谁优先。建议修复为「改变行为的删除」(如「去掉过宽的 null 检查」「删除吞异常的空 catch」)的 review agent 会读到删除豁免而写 N/A;该删除以无验收标准发布,guard 的缺失无人钉住,之后某次改动重新加回(或删除本身就是错的)时不会有测试变红,漏洞作为下一轮 finding 回归。删除的见证是可钉的(断言被 guard 抑制行为的测试在 guard 被加回时会失败)。SKILL.md:773 的「adds no guard」措辞没有这个矛盾。

— deepseek-v4-flash via Qwen Code /review (v0.21.11)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to the next round — this round's batch was bounded (~8 findings) and spent on the three Criticals first. The deletion-exemption contradiction (a guard removal is both a deletion and a behaviour change) needs the carve-out wording; next round, together with the matching agent-prompt paragraph.

中文说明

延迟到下一轮——本轮批次有上限(约 8 条 finding),优先处理了三条 Critical。删除豁免的矛盾(移除 guard 既是删除又是行为改变)需要豁免排除措辞;下一轮与 agent 提示词对应段落一并处理。


/**
* What not to report.
Expand Down
Loading
Loading