fix(review): a past-the-end line makes a finding unverified, never refuted, and the Declined to Judge copy-through is pinned
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
761f406faa
commit
7175783ce5
4 changed files with 87 additions and 6 deletions
|
|
@ -114,7 +114,7 @@ is load-bearing** — see *Suppression is two-valued* below:
|
|||
| Filter | Test | Fails if | Removed as |
|
||||
|--------|------|----------|------------|
|
||||
| Succinctness | `title.length ≤ 100` and `detail.length ≤ 800` chars | Title is a paragraph or detail is a wall of text | **never removed.** MINOR/SUGGESTION (catalogue tier): the text is trimmed (ending `[…trimmed]`), the finding is kept and counted. BLOCKER/MAJOR: kept whole, every character. |
|
||||
| Accuracy | `file` resolves under the repo root AND `line` is plausible (≥ 0; ≤ file line count when known) | Path traversal escape, negative line, or impossibly large line number | **dropped** (`accuracy:refuted`) |
|
||||
| Accuracy | `file` resolves under the repo root AND `line` ≥ 0 | Path traversal escape or negative line | **dropped** (`accuracy:refuted`). A line past the end of the file (when its line count is known) is not refuted: the citation is wrong, the claim is unchecked → `unverified` (`accuracy:line-out-of-range`) |
|
||||
| Actionability | `recommended_action` is non-empty AND begins with an imperative verb | Empty action, "consider …" hedges, or restating the title | `unverified` (`actionability:empty`) |
|
||||
|
||||
A finding that carries `engine` (`code-review` / `security-review`) came
|
||||
|
|
@ -126,8 +126,10 @@ voyage-lekerepo trial, 2026-10-06.)
|
|||
|
||||
Succinctness and Actionability read the finding's *packaging*; neither
|
||||
examines the claim, so neither can establish the finding is unreal. Accuracy
|
||||
does: a citation that escapes the repo root refutes the finding as a claim
|
||||
about this codebase.
|
||||
does: a citation that escapes the repo root, or a negative line, refutes the
|
||||
finding as a claim about this codebase. A line past the end of the file does
|
||||
not: the citation is wrong, the claim is unchecked. A line number alone never
|
||||
refutes a finding, so it goes to `unverified` (`accuracy:line-out-of-range`).
|
||||
|
||||
**No finding is struck for length** (ordre 5). Before review_version 1.1 an
|
||||
over-long `detail` sent a finding to `unverified`, and S118's most serious
|
||||
|
|
|
|||
|
|
@ -61,7 +61,8 @@ export const JUDGE_DETAIL_MAX = 800;
|
|||
* (agents/review-coordinator.md Pass 3 — "Speculative 'code might break
|
||||
* somewhere' findings have no anchor").
|
||||
*
|
||||
* `accuracy:refuted` (Pass 2 Accuracy — a citation escaping the repo root) and
|
||||
* `accuracy:refuted` (Pass 2 Accuracy — a citation escaping the repo root, or
|
||||
* a negative line; a line past the end of the file does not refute) and
|
||||
* `file-existence:refuted` (Pass 3 — absent from both working tree and diff)
|
||||
* are emitted by the LLM coordinator, whose fs/judgement branches this module
|
||||
* excludes. They are declared here anyway: the vocabulary is owned in one
|
||||
|
|
@ -79,14 +80,17 @@ export const REFUTING_REASONS = Object.freeze(new Set([
|
|||
* finding, so `succinctness:*` is no longer emitted (a stray one still fails
|
||||
* closed through `classifySuppression`). `file-existence:indeterminate`
|
||||
* is emitted by the LLM coordinator's Pass 3 (which runs the fs Glob this
|
||||
* module deliberately excludes); the vocabulary is owned here so prose and lib
|
||||
* cannot drift.
|
||||
* module deliberately excludes), and `accuracy:line-out-of-range` by its
|
||||
* Pass 2 Accuracy (a line past the end of the file: the citation is wrong, the
|
||||
* claim is unchecked, so a line number alone never refutes); the vocabulary is
|
||||
* owned here so prose and lib cannot drift.
|
||||
*/
|
||||
export const UNVERIFIED_REASONS = Object.freeze([
|
||||
'actionability:empty',
|
||||
'engine:out-of-scope',
|
||||
'unknown-rule_key',
|
||||
'file-existence:indeterminate',
|
||||
'accuracy:line-out-of-range',
|
||||
]);
|
||||
// The brief-conflict mark (`contradicts_brief`) is in neither set on purpose:
|
||||
// it is a field on a KEPT finding, never a `suppressed_reason`. As a reason it
|
||||
|
|
|
|||
|
|
@ -360,6 +360,22 @@ test('classifySuppression — the refuting reasons the LLM coordinator emits are
|
|||
'unresolvable must never collapse into refuted');
|
||||
});
|
||||
|
||||
test('classifySuppression — a line past the end of the file is unverified, never refuted', () => {
|
||||
// Fail-closed already: an undeclared reason is unverified. Holds before and
|
||||
// after the reason is declared (agents/review-coordinator.md Pass 2 Accuracy).
|
||||
assert.equal(classifySuppression('accuracy:line-out-of-range'), 'unverified');
|
||||
});
|
||||
|
||||
test('[CL] accuracy:line-out-of-range is a declared unverified reason, not a refuting one', () => {
|
||||
// A line number alone never refutes a finding: the citation is wrong, the
|
||||
// claim is unchecked. Declaring it here makes the vocabulary test below hold
|
||||
// the word in agents/review-coordinator.md.
|
||||
assert.ok(UNVERIFIED_REASONS.includes('accuracy:line-out-of-range'),
|
||||
'accuracy:line-out-of-range is not in UNVERIFIED_REASONS');
|
||||
assert.equal(REFUTING_REASONS.has('accuracy:line-out-of-range'), false,
|
||||
'accuracy:line-out-of-range must never refute');
|
||||
});
|
||||
|
||||
test('suppression vocabulary — the two sets are disjoint and every reason is documented in the prose', () => {
|
||||
const refuting = [...REFUTING_REASONS];
|
||||
const overlap = refuting.filter((r) => UNVERIFIED_REASONS.includes(r));
|
||||
|
|
|
|||
|
|
@ -1323,3 +1323,62 @@ test('[F9] sibling: templates/trekreview-template.md', () => {
|
|||
assert.ok(cov.includes('| lib/foo.mjs | deep-review | default for changed code |'), 'the template has no deep-review default row');
|
||||
assert.ok(cov.includes('| docs/guide.md | summary-only | docs/** is summary-only |'), 'the template has no docs summary-only row');
|
||||
});
|
||||
|
||||
// --- CL: a line number alone never refutes a finding ------------------------
|
||||
// Pass 2's Accuracy row dropped a finding as refuted (`accuracy:refuted`) for an
|
||||
// "impossibly large line number", so a finding whose claim was never checked
|
||||
// weighed nothing and could not hold ALLOW back. Only a citation that escapes
|
||||
// the repo root or a negative line refutes; a line past the end of the file
|
||||
// makes the finding `unverified` (`accuracy:line-out-of-range`): the citation
|
||||
// is wrong, the claim is unchecked. The lib side is pinned in
|
||||
// tests/lib/coordinator-contract.test.mjs.
|
||||
//
|
||||
// --- DJ: the Declined to Judge copy-through is held -------------------------
|
||||
// A reviewer's own `### Declined to Judge` lines reach review.md only through
|
||||
// three sentences: /trekreview Phase 6 passes them to the coordinator, the
|
||||
// coordinator's Input names them, and body section 9 copies them. Each is
|
||||
// pinned by its own flattened regex. These pass at ab6b32c by design (no text
|
||||
// changed); the round's red script proves them red by deleting the sentences.
|
||||
|
||||
const COORD = 'agents/review-coordinator.md';
|
||||
|
||||
/** The one Pass 2 table row whose first cell is `Accuracy`. */
|
||||
function accuracyRow() {
|
||||
const p2 = section(read(COORD), '### Pass 2 — HubSpot Judge filters (3 criteria)');
|
||||
const hits = p2.split('\n').filter((l) => /^\| Accuracy \|/.test(l));
|
||||
assert.equal(hits.length, 1, `expected exactly one Accuracy row in Pass 2; found ${hits.length}`);
|
||||
return hits[0];
|
||||
}
|
||||
|
||||
test('[CL] the Accuracy row refutes only a root escape or a negative line; a line past the end is unverified (accuracy:line-out-of-range)', () => {
|
||||
const row = accuracyRow();
|
||||
assert.match(row, /dropped|accuracy:refuted/, 'control: the Accuracy row still drops something as refuted');
|
||||
for (const phrase of [/impossibly large line number/, /≤ file line count/]) {
|
||||
assert.doesNotMatch(row, phrase, `the Accuracy row still pairs ${phrase} with dropped / accuracy:refuted`);
|
||||
}
|
||||
assert.match(row, /Path traversal escape or negative line \| \*\*dropped\*\* \(`accuracy:refuted`\)/,
|
||||
'the Accuracy row does not drop exactly a root escape or a negative line as refuted');
|
||||
assert.match(row, /A line past the end of the file[^|]*the citation is wrong, the claim is unchecked[^|]*`unverified` \(`accuracy:line-out-of-range`\)/,
|
||||
'the Accuracy row does not send a line past the end of the file to unverified (accuracy:line-out-of-range)');
|
||||
const why = flat(section(read(COORD), '### Pass 2 — HubSpot Judge filters (3 criteria)'));
|
||||
assert.match(why, /Accuracy does: a citation that escapes the repo root, or a negative line, refutes the finding as a claim about this codebase\. A line past the end of the file does not: the citation is wrong, the claim is unchecked\./,
|
||||
'the paragraph after the table still lets a line number alone refute');
|
||||
});
|
||||
|
||||
test('[DJ] /trekreview Phase 6 passes each reviewer\'s Declined to Judge lines verbatim for the coordinator to copy', () => {
|
||||
const p6 = flat(section(read(REVIEW), '## Phase 6 — Coordinator dedup + verdict'));
|
||||
assert.match(p6, /pass each reviewer's `### Declined to Judge` lines verbatim from its saved `\{project_dir\}\/\.review-out-\{reviewer\}\.md`: the coordinator copies them into `## Declined to Judge`, so nothing a reviewer set aside disappears\./,
|
||||
'Phase 6 no longer hands the reviewers\' Declined to Judge lines to the coordinator');
|
||||
});
|
||||
|
||||
test('[DJ] review-coordinator Input: the reviewers\' Declined to Judge lines are copied into body section 9, not ruled on', () => {
|
||||
const input = flat(section(read(COORD), '## Input'));
|
||||
assert.match(input, /\*\*Reviewers' Declined to Judge lines\*\* — each reviewer's own `### Declined to Judge` list, verbatim\. You copy them into `## Declined to Judge` \(body section 9\); you do not rule on them\./,
|
||||
'the coordinator\'s Input no longer names the reviewers\' Declined to Judge lines to copy');
|
||||
});
|
||||
|
||||
test('[DJ] review-coordinator body section 9 copies every reviewer Declined to Judge line as {reviewer}: {line}, outside the JSON array and the verdict', () => {
|
||||
const body = flat(section(read(COORD), '### Body sections (in order)'));
|
||||
assert.match(body, /then every line a reviewer wrote under its own `### Declined to Judge`, copied as `\{reviewer\}: \{line\}` \(they are not findings: they stay out of the JSON `declined_to_judge` array and do not change the verdict\)/,
|
||||
'body section 9 no longer copies the reviewers\' Declined to Judge lines');
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue