diff --git a/.changeset/review-skill-rule-quote.md b/.changeset/review-skill-rule-quote.md new file mode 100644 index 00000000..02d33dd2 --- /dev/null +++ b/.changeset/review-skill-rule-quote.md @@ -0,0 +1,5 @@ +--- +"review": minor +--- + +Skill findings show the author the actual rule. Quote-the-rule already requires the exact rule text in a lens skill finding's `evidence_trace`, but evidence traces never reach the PR: the author reads only `model_authored_prose`, so they see a paraphrase of a rule they cannot check. The finding schema gains an optional `rule_quote` field (the exact rule text, verbatim from the skill file; validated non-empty when present, and optional, so `FINDING_SCHEMA_VERSION` stays 2), each lens's lens-owned-skills discipline says to fill it, and `renderComment` plus the orchestrator's normalization step surface it into the posted comment as a `> **Rule:** …` blockquote between the prose and any suggestion block. Only the wrapping is code-owned; the quote is skill-file text copied verbatim. diff --git a/workflows/review/eval/corpus/live.ts b/workflows/review/eval/corpus/live.ts index 9ca12eaf..890e4a9e 100644 --- a/workflows/review/eval/corpus/live.ts +++ b/workflows/review/eval/corpus/live.ts @@ -72,7 +72,19 @@ export type LiveDefectSpec = { */ export type CaseLive = { prContext: LivePrContext; - /** Case-dir-relative path to the post-change tree (default `tree`). */ + /** + * Case-dir-relative path to the post-change tree (default `tree`). + * + * Trees are deliberately minimal: validation requires only the changed + * files to exist. The staged copy of this tree is the sub-agent's whole + * world (its cwd, with no network), so the agent WILL often try to read + * an imported module or caller that is not there; that read returns an + * ordinary not-found tool error the agent tolerates and works around, + * not a run failure. The cost of a missing file is realism, not a crash: + * include the context files whose absence would change what a reviewer + * concludes (e.g. the decorator module a sibling handler imports), and + * nothing more. + */ tree: string; /** Labeled defects a live run must catch. */ mustCatchSpecs?: LiveDefectSpec[]; diff --git a/workflows/review/lib/finding-schema.test.ts b/workflows/review/lib/finding-schema.test.ts index d0f771f8..33d978c2 100644 --- a/workflows/review/lib/finding-schema.test.ts +++ b/workflows/review/lib/finding-schema.test.ts @@ -162,6 +162,28 @@ describe("validateFinding — well-formed findings", () => { expect(result.ok).toBe(true); }); + it("accepts the optional rule_quote when present and non-empty", () => { + const result = validateFinding( + makeValidFinding({ + rule_quote: + "Always wrap errors with errors.Wrap before returning them.", + }), + ); + expect(result.ok).toBe(true); + }); + + it("rejects an empty or non-string rule_quote", () => { + for (const bad of ["", 42]) { + const result = validateFinding(makeValidFinding({rule_quote: bad})); + expect(result.ok).toBe(false); + if (!result.ok) { + expect( + result.errors.some((e) => e.startsWith("rule_quote:")), + ).toBe(true); + } + } + }); + it("accepts every KNOWN_LENSES value", () => { for (const lens of KNOWN_LENSES) { expect(validateFinding(makeValidFinding({lens})).ok).toBe(true); diff --git a/workflows/review/lib/finding-schema.ts b/workflows/review/lib/finding-schema.ts index 112ea005..d038ce85 100644 --- a/workflows/review/lib/finding-schema.ts +++ b/workflows/review/lib/finding-schema.ts @@ -156,6 +156,16 @@ export type Finding = { failure_scenario: string; /** Optional unified-diff patch the author suggests (rendered as a suggestion). */ suggested_patch?: string; + /** + * For a skill (best-practice) finding: the exact rule text, quoted + * verbatim from the skill file, that the finding asserts is violated. + * Quote-the-rule already requires the quote in `evidence_trace`, but + * authors never see evidence traces — this field is what the renderer + * surfaces into the comment (as a `> **Rule:** …` blockquote), so the + * author reads the actual rule, not a paraphrase. Optional: only skill + * findings carry it. + */ + rule_quote?: string; /** * Optional pre-merge obligation text. Drives the conditional-approval * (APPROVE-with-obligations) rendering. @@ -352,6 +362,13 @@ export const validateFinding = (input: unknown): ValidationResult => { errors.push("suggested_patch: must be a non-empty string when present"); } + if ( + input["rule_quote"] !== undefined && + !isNonEmptyString(input["rule_quote"]) + ) { + errors.push("rule_quote: must be a non-empty string when present"); + } + if ( input["pre_merge_obligation"] !== undefined && !isNonEmptyString(input["pre_merge_obligation"]) diff --git a/workflows/review/lib/render-comment.test.ts b/workflows/review/lib/render-comment.test.ts index 72d571f9..1fd9c6a8 100644 --- a/workflows/review/lib/render-comment.test.ts +++ b/workflows/review/lib/render-comment.test.ts @@ -139,6 +139,41 @@ describe("renderComment — templated Conventional Comment", () => { const rendered = renderComment(makeFinding({suggested_patch: patch})); expect(rendered).toContain(patch); }); + + it("surfaces a skill finding's rule_quote as a Rule blockquote", () => { + const rendered = renderComment( + makeFinding({ + severity: "advisory", + lens: "conventions", + rule_quote: + "Always wrap errors with errors.Wrap before returning them.", + }), + ); + expect(rendered).toMatchInlineSnapshot(` + "**suggestion (non-blocking, best-practice):** User input flows unsanitized into a shell command. + + > **Rule:** Always wrap errors with errors.Wrap before returning them." + `); + }); + + it("orders prose, rule blockquote, then suggestion block", () => { + const rendered = renderComment( + makeFinding({ + rule_quote: "The exact rule text.", + suggested_patch: "-a\n+b", + }), + ); + const proseAt = rendered.indexOf("User input flows"); + const ruleAt = rendered.indexOf("> **Rule:** The exact rule text."); + const patchAt = rendered.indexOf("```suggestion"); + expect(proseAt).toBeGreaterThan(-1); + expect(ruleAt).toBeGreaterThan(proseAt); + expect(patchAt).toBeGreaterThan(ruleAt); + }); + + it("emits no Rule blockquote when rule_quote is absent", () => { + expect(renderComment(makeFinding())).not.toContain("> **Rule:**"); + }); }); describe("renderReviewBody — one non-empty line per verdict (+ notes)", () => { diff --git a/workflows/review/lib/render-comment.ts b/workflows/review/lib/render-comment.ts index 94f53808..02a365e9 100644 --- a/workflows/review/lib/render-comment.ts +++ b/workflows/review/lib/render-comment.ts @@ -117,19 +117,30 @@ export const labelForFinding = (finding: Finding): ConventionalLabel => { * * **