fix(agents): reviewers drop a finding only with a Declined to Judge line, research-scout gets Tavily, no dead override reference
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
ab6b32c930
commit
4312882451
6 changed files with 199 additions and 18 deletions
|
|
@ -171,8 +171,9 @@ Severity is fixed by `rule_key`. Do NOT override the catalogue:
|
||||||
| `COVERAGE_SILENT_SKIP` | MAJOR |
|
| `COVERAGE_SILENT_SKIP` | MAJOR |
|
||||||
|
|
||||||
If a finding feels less severe than its catalogue tier, do NOT downgrade
|
If a finding feels less severe than its catalogue tier, do NOT downgrade
|
||||||
it. Either drop the finding (it was wrong) or emit it at the
|
it. Emit it at the catalogue's severity. Drop the finding only when you
|
||||||
catalogue's severity.
|
can say why it is wrong, and then write it as one line under
|
||||||
|
`### Declined to Judge` with that reason.
|
||||||
|
|
||||||
## Output format
|
## Output format
|
||||||
|
|
||||||
|
|
@ -254,8 +255,9 @@ parsers find it by reading the last `json` code fence.
|
||||||
`(file, line, rule_key)` with `lib/review/fill-finding-ids.mjs`).
|
`(file, line, rule_key)` with `lib/review/fill-finding-ids.mjs`).
|
||||||
- `line` is an integer; use `0` when the finding is file-scoped without
|
- `line` is an integer; use `0` when the finding is file-scoped without
|
||||||
a specific line (e.g., MISSING_TEST for an entire file).
|
a specific line (e.g., MISSING_TEST for an entire file).
|
||||||
- `rule_key` MUST be in the catalogue. Reviewers that emit unknown rule
|
- `rule_key` MUST be in the catalogue. A finding with a rule key outside
|
||||||
keys are dropped by the coordinator's reasonableness filter.
|
it is not dropped: it is routed to `unverified` by the coordinator's
|
||||||
|
reasonableness filter (Pass 3).
|
||||||
|
|
||||||
## Rules
|
## Rules
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -93,8 +93,9 @@ Cite both the changed file:line AND the regressed file:line.
|
||||||
|
|
||||||
When the brief explicitly asks for tests of a specific behavior and they
|
When the brief explicitly asks for tests of a specific behavior and they
|
||||||
are missing, escalate to `MISSING_TEST` MAJOR. When tests are
|
are missing, escalate to `MISSING_TEST` MAJOR. When tests are
|
||||||
nice-to-have, downgrade is forbidden — emit at the catalogue tier or
|
nice-to-have, downgrade is forbidden — emit at the catalogue tier. Drop
|
||||||
drop the finding.
|
the finding only when you can say why it is wrong, and then write it as
|
||||||
|
one line under `### Declined to Judge` with that reason.
|
||||||
|
|
||||||
### 5. Placeholder code — `PLACEHOLDER_IN_CODE` (MAJOR)
|
### 5. Placeholder code — `PLACEHOLDER_IN_CODE` (MAJOR)
|
||||||
|
|
||||||
|
|
@ -110,9 +111,8 @@ literals or example fenced blocks):
|
||||||
- `// stub`
|
- `// stub`
|
||||||
- `throw new Error('not implemented')`
|
- `throw new Error('not implemented')`
|
||||||
|
|
||||||
Cite the exact line. The MANDATORY OVERRIDE rule above forbids saying
|
Cite the exact line. Never accept "not implemented" placeholders as fine
|
||||||
"not implemented" placeholders are fine "for now" — they are MAJOR
|
"for now" — they are MAJOR findings until removed.
|
||||||
findings until removed.
|
|
||||||
|
|
||||||
### 6. Security surface — `SECURITY_INJECTION` (BLOCKER)
|
### 6. Security surface — `SECURITY_INJECTION` (BLOCKER)
|
||||||
|
|
||||||
|
|
@ -154,8 +154,10 @@ Severity is fixed by `rule_key`. Do NOT override the catalogue:
|
||||||
| `UNDECLARED_DEPENDENCY` | MAJOR |
|
| `UNDECLARED_DEPENDENCY` | MAJOR |
|
||||||
| `COVERAGE_SILENT_SKIP` | MAJOR |
|
| `COVERAGE_SILENT_SKIP` | MAJOR |
|
||||||
|
|
||||||
If a finding feels off-tier, either drop it (it was wrong) or emit it
|
If a finding feels off-tier, emit it at the catalogue's severity. Drop
|
||||||
at the catalogue's severity. Do not invent severity overrides.
|
it only when you can say why it is wrong, and then write it as one line
|
||||||
|
under `### Declined to Judge` with that reason. Do not invent severity
|
||||||
|
overrides.
|
||||||
|
|
||||||
## Output format
|
## Output format
|
||||||
|
|
||||||
|
|
@ -239,8 +241,9 @@ parsers find it by reading the last `json` code fence.
|
||||||
the canonical SHA1).
|
the canonical SHA1).
|
||||||
- `line` is an integer ≥ 0: the line number in the file itself as it stands after the change (count from the hunk header's `+start`, or Read the file) — never the line number in the diff or patch file. A line past the end of the file fails the schema check and you are asked again;
|
- `line` is an integer ≥ 0: the line number in the file itself as it stands after the change (count from the hunk header's `+start`, or Read the file) — never the line number in the diff or patch file. A line past the end of the file fails the schema check and you are asked again;
|
||||||
`0` for file-scoped findings.
|
`0` for file-scoped findings.
|
||||||
- `rule_key` MUST be in the catalogue. Reviewers that emit unknown rule
|
- `rule_key` MUST be in the catalogue. A finding with a rule key outside
|
||||||
keys are dropped by the coordinator's reasonableness filter.
|
it is not dropped: it is routed to `unverified` by the coordinator's
|
||||||
|
reasonableness filter (Pass 3).
|
||||||
|
|
||||||
## Rules
|
## Rules
|
||||||
|
|
||||||
|
|
@ -255,8 +258,10 @@ parsers find it by reading the last `json` code fence.
|
||||||
injection sink. Never flag a regression without naming both files.
|
injection sink. Never flag a regression without naming both files.
|
||||||
Speculative findings are dropped by the coordinator.
|
Speculative findings are dropped by the coordinator.
|
||||||
- **No silent severity downgrades.** The catalogue tier is the floor.
|
- **No silent severity downgrades.** The catalogue tier is the floor.
|
||||||
If a finding feels less serious than its catalogue severity, either
|
If a finding feels less serious than its catalogue severity, emit it
|
||||||
drop it or emit it as the catalogue says.
|
as the catalogue says. Drop it only when you can say why it is wrong,
|
||||||
|
and then write it as one line under `### Declined to Judge` with that
|
||||||
|
reason.
|
||||||
- **Token budget honesty.** When summary-only is in effect for a file,
|
- **Token budget honesty.** When summary-only is in effect for a file,
|
||||||
state explicitly "summary-only — structural pass" so the coordinator
|
state explicitly "summary-only — structural pass" so the coordinator
|
||||||
knows the depth limit.
|
knows the depth limit.
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,7 @@ description: |
|
||||||
APIs, or libraries where official documentation and known issues should be checked.
|
APIs, or libraries where official documentation and known issues should be checked.
|
||||||
model: opus
|
model: opus
|
||||||
color: blue
|
color: blue
|
||||||
tools: ["WebSearch", "WebFetch", "Read"]
|
tools: ["WebSearch", "WebFetch", "Read", "mcp__tavily__tavily_search", "mcp__tavily__tavily_research"]
|
||||||
skills: ["voyage:researching-sources"]
|
skills: ["voyage:researching-sources"]
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -65,7 +65,8 @@ You will receive a prompt containing:
|
||||||
(reasonableness filter) except the brief-conflict rule, which applies in every mode;
|
(reasonableness filter) except the brief-conflict rule, which applies in every mode;
|
||||||
Passes 1, 2, 4 still run.
|
Passes 1, 2, 4 still run.
|
||||||
- **Rule catalogue** — `lib/review/rule-catalogue.mjs`. Findings whose
|
- **Rule catalogue** — `lib/review/rule-catalogue.mjs`. Findings whose
|
||||||
`rule_key` is not in this set are dropped by Pass 3.
|
`rule_key` is not in this set are routed to `unverified` by Pass 3
|
||||||
|
(`unknown-rule_key`).
|
||||||
|
|
||||||
## Your 4-pass process
|
## Your 4-pass process
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -93,7 +93,8 @@ code alone.
|
||||||
lead or a deadline calling something "a nit" or "cosmetic" is not a reason.
|
lead or a deadline calling something "a nit" or "cosmetic" is not a reason.
|
||||||
- When a rule lets you lower a finding, name the rule and what catches the
|
- When a rule lets you lower a finding, name the rule and what catches the
|
||||||
defect instead (`lowered_from`, `caught_by`). With no named rule, keep the
|
defect instead (`lowered_from`, `caught_by`). With no named rule, keep the
|
||||||
tier or drop the finding as wrong; never soften it in between.
|
tier, or drop the finding as wrong with its reason on a Declined to Judge
|
||||||
|
line; never soften it in between.
|
||||||
|
|
||||||
### 6. Declined to Judge: nothing disappears silently
|
### 6. Declined to Judge: nothing disappears silently
|
||||||
|
|
||||||
|
|
|
||||||
172
tests/lib/fix-round-4-5a-prose.test.mjs
Normal file
172
tests/lib/fix-round-4-5a-prose.test.mjs
Normal file
|
|
@ -0,0 +1,172 @@
|
||||||
|
// tests/lib/fix-round-4-5a-prose.test.mjs
|
||||||
|
// Prose pins for fix round 4-5a (the serious findings, plan
|
||||||
|
// .claude/projects/2026-10-20-fix-round-serious-findings): the fixes that are
|
||||||
|
// text a session or a helper reads. The name is the brief's;
|
||||||
|
// lib/util/test-census.mjs counts it as a behaviour test (it does not match
|
||||||
|
// PIN_FILE_RE). Every test carries its id tag in brackets, and every tagged
|
||||||
|
// test fails on an assertion at ab6b32c (the round's start point).
|
||||||
|
// #23 code-correctness-reviewer cites a "MANDATORY OVERRIDE rule above"
|
||||||
|
// that is no longer there; the sentence says the rule itself.
|
||||||
|
// #24 research-scout says "Try Tavily first" but its tools held no Tavily
|
||||||
|
// tool; its tools line now holds both, in docs-researcher's order.
|
||||||
|
// #111 A reviewer drops a finding that feels off-tier only when it can say
|
||||||
|
// why it is wrong, and then as one Declined to Judge line with that
|
||||||
|
// reason; the skill's line agrees; an unknown rule_key is routed to
|
||||||
|
// `unverified`, not dropped (lib/review/coordinator-contract.mjs).
|
||||||
|
// Fix the SOURCE a pin guards, not the pin.
|
||||||
|
|
||||||
|
import { test } from 'node:test';
|
||||||
|
import { strict as assert } from 'node:assert';
|
||||||
|
import { existsSync, readFileSync, readdirSync } from 'node:fs';
|
||||||
|
import { dirname, join } from 'node:path';
|
||||||
|
import { fileURLToPath } from 'node:url';
|
||||||
|
import { spawnSync } from 'node:child_process';
|
||||||
|
import { findSection } from '../../lib/parsers/plan-schema.mjs';
|
||||||
|
import { readFrontmatterFields } from '../../lib/skills/skill-frontmatter.mjs';
|
||||||
|
|
||||||
|
const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..');
|
||||||
|
|
||||||
|
/** A repo file's text; the file must exist. */
|
||||||
|
function read(rel) {
|
||||||
|
assert.ok(existsSync(join(ROOT, rel)), `missing file ${rel}`);
|
||||||
|
return readFileSync(join(ROOT, rel), 'utf8');
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Whitespace (line breaks too) collapsed to one space. */
|
||||||
|
const flat = (s) => s.replace(/\s+/g, ' ').trim();
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The text of a heading's section, up to the next heading of the same or a
|
||||||
|
* higher level outside a fence. The heading must exist.
|
||||||
|
*/
|
||||||
|
function section(text, heading) {
|
||||||
|
const level = heading.match(/^#+/)[0].length;
|
||||||
|
const s = findSection(text, heading.replace(/^#+ /, ''), level);
|
||||||
|
assert.ok(s, `missing ${heading}`);
|
||||||
|
const lines = s.text.split('\n');
|
||||||
|
let fenced = false;
|
||||||
|
for (let i = 1; i < lines.length; i++) {
|
||||||
|
if (/^\s{0,3}(`{3,}|~{3,})/.test(lines[i])) fenced = !fenced;
|
||||||
|
const m = !fenced && lines[i].match(/^(#+) /);
|
||||||
|
if (m && m[1].length < level) return lines.slice(0, i).join('\n');
|
||||||
|
}
|
||||||
|
return s.text;
|
||||||
|
}
|
||||||
|
|
||||||
|
/** From the line holding `marker` to the first line where `stop` is true, flattened. */
|
||||||
|
function partAt(rel, marker, stop) {
|
||||||
|
const lines = read(rel).split('\n');
|
||||||
|
const start = lines.findIndex((l) => l.includes(marker));
|
||||||
|
assert.ok(start !== -1, `${rel} must hold "${marker}"`);
|
||||||
|
let end = lines.length;
|
||||||
|
for (let i = start + 1; i < lines.length; i++) if (stop(lines[i])) { end = i; break; }
|
||||||
|
return flat(lines.slice(start, end).join(' '));
|
||||||
|
}
|
||||||
|
|
||||||
|
/** The single line in `text` that runs `lib/<mod>` (and holds `verb`, when given). */
|
||||||
|
function line(text, mod, verb = '') {
|
||||||
|
const esc = mod.replace(/[.*+?^${}()|[\]\\/]/g, '\\$&');
|
||||||
|
const re = new RegExp(`^\\s*node \\$\\{CLAUDE_PLUGIN_ROOT\\}/lib/${esc}\\b`);
|
||||||
|
const hits = text.split('\n').filter((l) => re.test(l) && l.includes(verb));
|
||||||
|
assert.equal(hits.length, 1, `expected exactly one \`${mod}${verb ? ` ${verb}` : ''}\` line; found ${hits.length}`);
|
||||||
|
return hits[0].trim();
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Run a command line from the prose with the plugin root filled in. */
|
||||||
|
function bash(cmd, cwd) {
|
||||||
|
return spawnSync('bash', ['-c', cmd.split('${CLAUDE_PLUGIN_ROOT}').join(ROOT)], { cwd, encoding: 'utf8' });
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- #23 ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
test('[#23] no agent file refers to a MANDATORY OVERRIDE rule', () => {
|
||||||
|
const hits = readdirSync(join(ROOT, 'agents'))
|
||||||
|
.filter((f) => f.endsWith('.md'))
|
||||||
|
.filter((f) => read(`agents/${f}`).includes('MANDATORY OVERRIDE'));
|
||||||
|
assert.deepEqual(hits, [], 'an agent still points at a MANDATORY OVERRIDE rule that is not there');
|
||||||
|
const d5 = flat(section(read('agents/code-correctness-reviewer.md'), '### 5. Placeholder code — `PLACEHOLDER_IN_CODE` (MAJOR)'));
|
||||||
|
assert.match(d5, /Never accept "not implemented" placeholders as fine "for now"/);
|
||||||
|
});
|
||||||
|
|
||||||
|
// --- #24 ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
test('[#24] research-scout holds both Tavily tools, in docs-researcher\'s order, and still tries Tavily first', () => {
|
||||||
|
const text = read('agents/research-scout.md');
|
||||||
|
const fm = readFrontmatterFields(text);
|
||||||
|
assert.ok(fm.ok, fm.error);
|
||||||
|
assert.deepEqual(fm.fields.tools.value,
|
||||||
|
['WebSearch', 'WebFetch', 'Read', 'mcp__tavily__tavily_search', 'mcp__tavily__tavily_research']);
|
||||||
|
assert.match(text, /\*\*Try Tavily first\*\*/);
|
||||||
|
});
|
||||||
|
|
||||||
|
// --- #111 --------------------------------------------------------------------------
|
||||||
|
|
||||||
|
/** No bare "drop it" choice; a drop needs a reason, written as one Declined to Judge line. */
|
||||||
|
function dropsOnlyWithTrace(part, where) {
|
||||||
|
assert.doesNotMatch(part, /either drop (it|the finding)|or drop (it|the finding)\s*\.|drop (it|the finding) \(it was wrong\)/i,
|
||||||
|
`${where}: an off-tier finding may still be dropped with no trace`);
|
||||||
|
assert.match(part, /drop[^.]{0,60}only when you can say why it is wrong[^.]{0,120}Declined to Judge[^.]{0,60}with that reason/i,
|
||||||
|
`${where}: a drop must carry its reason on a Declined to Judge line`);
|
||||||
|
assert.match(part, /catalogue('s)? (tier|severity)/i, `${where}: the catalogue tier stays the default`);
|
||||||
|
}
|
||||||
|
|
||||||
|
const toBlank = (l) => l.trim() === '';
|
||||||
|
|
||||||
|
test('[#111] code-correctness-reviewer dimension 4: a nice-to-have test gap is dropped only with a Declined to Judge line', () => {
|
||||||
|
dropsOnlyWithTrace(partAt('agents/code-correctness-reviewer.md', 'When the brief explicitly asks for tests', toBlank),
|
||||||
|
'code-correctness-reviewer dimension 4');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] code-correctness-reviewer severity rules: an off-tier finding is dropped only with a Declined to Judge line', () => {
|
||||||
|
dropsOnlyWithTrace(partAt('agents/code-correctness-reviewer.md', 'If a finding feels off-tier', toBlank),
|
||||||
|
'code-correctness-reviewer ## Severity rules');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] code-correctness-reviewer rules: "No silent severity downgrades" drops only with a Declined to Judge line', () => {
|
||||||
|
dropsOnlyWithTrace(partAt('agents/code-correctness-reviewer.md', '**No silent severity downgrades.**', (l) => /^- /.test(l) || toBlank(l)),
|
||||||
|
'code-correctness-reviewer ## Rules');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] brief-conformance-reviewer severity rules: a finding is dropped only with a Declined to Judge line', () => {
|
||||||
|
dropsOnlyWithTrace(partAt('agents/brief-conformance-reviewer.md', 'If a finding feels less severe than its catalogue tier', toBlank),
|
||||||
|
'brief-conformance-reviewer ## Severity rules');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] reviewing-adversarially: keep the tier, or drop as wrong with its reason on a Declined to Judge line', () => {
|
||||||
|
const s5 = flat(section(read('skills/reviewing-adversarially/SKILL.md'), '### 5. No downgrade without a reason'));
|
||||||
|
assert.match(s5, /keep the tier, or drop the finding as wrong with its reason on a Declined to Judge line; never soften it in between/);
|
||||||
|
assert.doesNotMatch(s5, /keep the tier or drop the finding as wrong;/);
|
||||||
|
});
|
||||||
|
|
||||||
|
/** The stale "unknown rule keys are dropped" line now says they are routed to `unverified`. */
|
||||||
|
function routedNotDropped(part, where) {
|
||||||
|
assert.doesNotMatch(part, /dropped by (the coordinator's reasonableness filter|Pass 3)/i, `${where}: still says unknown rule keys are dropped`);
|
||||||
|
assert.match(part, /routed to `unverified`/, `${where}: must say an unknown rule key is routed to \`unverified\``);
|
||||||
|
}
|
||||||
|
|
||||||
|
const toBullet = (l) => /^- /.test(l) || toBlank(l) || /^#/.test(l);
|
||||||
|
|
||||||
|
test('[#111] code-correctness-reviewer: an unknown rule_key is routed to `unverified`, not dropped', () => {
|
||||||
|
routedNotDropped(partAt('agents/code-correctness-reviewer.md', '`rule_key` MUST be in the catalogue', toBullet),
|
||||||
|
'code-correctness-reviewer ## JSON output rules');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] brief-conformance-reviewer: an unknown rule_key is routed to `unverified`, not dropped', () => {
|
||||||
|
routedNotDropped(partAt('agents/brief-conformance-reviewer.md', '`rule_key` MUST be in the catalogue', toBullet),
|
||||||
|
'brief-conformance-reviewer ## JSON output rules');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('[#111] review-coordinator input: a rule_key outside the catalogue is routed to `unverified` by Pass 3', () => {
|
||||||
|
routedNotDropped(partAt('agents/review-coordinator.md', '**Rule catalogue** — `lib/review/rule-catalogue.mjs`', toBullet),
|
||||||
|
'review-coordinator ## Input');
|
||||||
|
});
|
||||||
|
|
||||||
|
// Steps 2-24 use these helpers; referenced here so the file states its interface.
|
||||||
|
test('helpers: line and bash are the shared line-runner pair', () => {
|
||||||
|
assert.equal(typeof line, 'function');
|
||||||
|
assert.equal(typeof bash, 'function');
|
||||||
|
assert.equal(line('x\n node ${CLAUDE_PLUGIN_ROOT}/lib/util/a.mjs go\n', 'util/a.mjs', 'go'), 'node ${CLAUDE_PLUGIN_ROOT}/lib/util/a.mjs go');
|
||||||
|
const r = bash('printf %s "${CLAUDE_PLUGIN_ROOT}"', ROOT);
|
||||||
|
assert.equal(r.status, 0);
|
||||||
|
assert.equal(r.stdout, ROOT);
|
||||||
|
});
|
||||||
Loading…
Add table
Add a link
Reference in a new issue