fix(review): the coordinator gets the brief's choices and writes the brief-conflict mark
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
8cc6ba0181
commit
11315d04eb
4 changed files with 239 additions and 6 deletions
|
|
@ -66,10 +66,15 @@ You will receive a prompt containing:
|
|||
the /trekreview triage gate.
|
||||
- **Brief metadata** — `task`, `slug`, `project_dir`, `brief_path` from
|
||||
the brief frontmatter.
|
||||
- **Brief choices** — the brief's `## Non-Goals`, `## Constraints` and
|
||||
`## Preferences` sections and the operator decisions in
|
||||
`## Open Questions / Assumptions`, verbatim. Read only by the
|
||||
brief-conflict rule (Pass 3).
|
||||
- **Scope SHA range** — `scope_sha_start`, `scope_sha_end`,
|
||||
`reviewed_files_count`.
|
||||
- **Mode** — `default` or `quick`. In `quick` mode, skip Pass 3
|
||||
(reasonableness filter); Passes 1, 2, 4 still run.
|
||||
(reasonableness filter) except the brief-conflict rule, which applies in every mode;
|
||||
Passes 1, 2, 4 still run.
|
||||
- **Rule catalogue** — `lib/review/rule-catalogue.mjs`. Findings whose
|
||||
`rule_key` is not in this set are dropped by Pass 3.
|
||||
|
||||
|
|
@ -124,6 +129,7 @@ over-long `detail` sent a finding to `unverified`, and S118's most serious
|
|||
finding fell out of the Handover 6 list that way. Length is packaging, never
|
||||
a claim: only small findings are trimmed, and the trims are counted
|
||||
(`trimmed` in the JSON block). The trim decision reads the **catalogue** tier,
|
||||
except a lowerable brief conflict, which is judged as MINOR (decide the mark before trimming),
|
||||
so a mislabelled MAJOR is never cut. Borrowed: "report at most five nits …
|
||||
summarize the rest as a count" — cap only the nits (ai-native-sdlc
|
||||
`assets/REVIEW.md:7-23`, MIT).
|
||||
|
|
@ -157,8 +163,32 @@ Remove findings that fail ANY of these tests:
|
|||
catalogue tier (e.g., `MISSING_TEST` emitted as MINOR). Reset to the
|
||||
catalogue tier; this is a correction, neither a drop nor an unverified.
|
||||
|
||||
**The brief-conflict rule (one fixed rule).** Read the **Brief choices** input.
|
||||
An engine finding (`engine: code-review` or `security-review`) whose recommended action
|
||||
contradicts a passed Non-Goal or stated choice gets `contradicts_brief: "<verbatim>"`
|
||||
(that Non-Goal or choice, word for word) and is kept, never removed;
|
||||
only a `/code-review` finding at the MAJOR tier and at severity MAJOR then drops to MINOR, with `brief_lowered_from: MAJOR`
|
||||
(in place of the catalogue reset above);
|
||||
a BLOCKER-tier, a MINOR-tier and any `/security-review` finding only get the mark,
|
||||
and no engine finding is otherwise written below its catalogue tier.
|
||||
The rule reads engine findings alone, so only an engine finding is ever marked:
|
||||
`engine` is written only on a finding whose kept Pass 1 survivor itself came from an engine payload
|
||||
(`claude-code-review` → `code-review`, `claude-security-review` → `security-review`),
|
||||
and a reviewer-agent finding (no `engine`) and a finding whose `rule_key` is not in the catalogue are never marked.
|
||||
Remove any `brief_lowered_from` an incoming finding carries, and any mark on a finding
|
||||
without an `engine`: only this rule writes them.
|
||||
The rule is about the finding's recommended action, not the rule key `NON_GOAL_VIOLATED`
|
||||
(a reviewer's claim that the delivered code breaks a Non-Goal is judged like any other finding).
|
||||
The mark is the brief-conflict reason (`contradicts_brief`), a field, never a `suppressed_reason`:
|
||||
the finding stays kept and counted at the severity written, never `unverified`.
|
||||
`lib/review/coordinator-contract.mjs` (`lowerableBriefConflict`, `illegalBriefLowering`)
|
||||
holds the same rule, and on an ALLOW or WARN review
|
||||
the verdict gate refuses (exit 2) every other marked or lowered shape.
|
||||
|
||||
In `quick` mode, skip this pass entirely. Note the skip in the
|
||||
Executive Summary so the reader knows reasonableness was not applied.
|
||||
The brief-conflict rule above still applies in quick mode (the mark and the
|
||||
one lowering) and changes nothing else.
|
||||
|
||||
**High-effort normalization (v5.1.1):** When the review is invoked
|
||||
under high-effort mode (`phase_signals[review].effort: high`), Pass 3
|
||||
|
|
@ -170,7 +200,8 @@ category in the catalogue). The original rule_key is
|
|||
preserved in the finding's `original_rule_key` field for diagnostic
|
||||
purposes. This normalization happens BEFORE writing review.md,
|
||||
ensuring all `rule_key` values in the final review match the
|
||||
catalogue.
|
||||
catalogue. The brief-conflict rule (Pass 3) still applies under high-effort
|
||||
mode (the mark and the one lowering) and changes nothing else.
|
||||
|
||||
### Suppression is two-valued (fail-closed)
|
||||
|
||||
|
|
@ -310,6 +341,9 @@ followed by these fields:
|
|||
- `- title: {short imperative title}`
|
||||
- `- detail: {what is wrong, with citation}`
|
||||
- `- recommended_action: {one imperative step}`
|
||||
- `- engine: {code-review | security-review, when the finding came from an engine}`
|
||||
- `- contradicts_brief: {verbatim Non-Goal or choice, only when set}`
|
||||
- `- brief_lowered_from: {MAJOR, only when lowered for the brief}`
|
||||
|
||||
### Trailing JSON block
|
||||
|
||||
|
|
@ -333,6 +367,20 @@ The LAST fenced block in the file is a `json` block:
|
|||
"title": "...",
|
||||
"detail": "...",
|
||||
"recommended_action": "..."
|
||||
},
|
||||
{
|
||||
"id": "<40-char-hex>",
|
||||
"severity": "MINOR",
|
||||
"rule_key": "CODE_REVIEW_DEFECT",
|
||||
"file": "lib/http.mjs",
|
||||
"line": 42,
|
||||
"brief_ref": "Non-Goal 2",
|
||||
"title": "...",
|
||||
"detail": "...",
|
||||
"recommended_action": "...",
|
||||
"engine": "code-review",
|
||||
"contradicts_brief": "No retry layer around the HTTP client",
|
||||
"brief_lowered_from": "MAJOR"
|
||||
}
|
||||
]
|
||||
}
|
||||
|
|
@ -346,13 +394,17 @@ for the ID list.
|
|||
## Hard rules
|
||||
|
||||
- **Bounded operations only.** You do NOT read the diff. You do NOT
|
||||
re-evaluate findings against the brief. You do NOT generate new
|
||||
re-evaluate findings against the brief, with one named exception: the
|
||||
brief-conflict rule (Pass 3), which marks an engine finding against the
|
||||
**Brief choices** and lowers only the one legal shape. You do NOT generate new
|
||||
findings. The reviewers' outputs are your sole input. Synthesis-level
|
||||
inference (e.g., "these 3 findings together suggest a pattern") is
|
||||
forbidden in v1.0.
|
||||
- **Verdict is mechanical.** No "ALLOW with caveats" or other custom
|
||||
verdicts. Only BLOCK / WARN / ALLOW per the threshold table.
|
||||
- **Severity floor is the catalogue.** Pass 3 corrects mismatches by
|
||||
- **Severity floor is the catalogue.** It holds for every finding
|
||||
except a lowerable brief conflict, which is MINOR (Pass 3, `brief_lowered_from: MAJOR`).
|
||||
Pass 3 corrects mismatches by
|
||||
resetting to the catalogue tier — never by dropping. Pass 1's severity
|
||||
tiebreak uses the catalogue tier, not the reviewer's emitted value.
|
||||
- **Block-style YAML for findings list.** The frontmatter parser
|
||||
|
|
@ -380,6 +432,7 @@ for the ID list.
|
|||
reviewer outputs. Never escalate a finding's severity beyond what the
|
||||
catalogue specifies.
|
||||
- **Quick mode is documented.** When mode is `quick`, the Executive
|
||||
Summary says so, and Pass 3 is skipped — no other changes.
|
||||
Summary says so, and Pass 3 is skipped — no other changes,
|
||||
except the brief-conflict rule, which applies in every mode.
|
||||
- **Honesty in fallback paths.** If `scope_sha_start` is null (mtime
|
||||
fallback), the Executive Summary names this limitation explicitly.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue