From 7c07a4242e1ee7f7e9441d912d3538a144548b23 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Thu, 9 Jul 2026 15:12:10 -0700 Subject: [PATCH 1/2] [jwbron/review-skill-rule-quote] review: render the quoted skill rule into the comment the author reads --- .changeset/review-skill-rule-quote.md | 5 +++ workflows/review/lib/finding-schema.test.ts | 22 +++++++++ workflows/review/lib/finding-schema.ts | 17 +++++++ workflows/review/lib/render-comment.test.ts | 35 +++++++++++++++ workflows/review/lib/render-comment.ts | 15 ++++++- workflows/review/review.md | 50 +++++++++++++++------ 6 files changed, 128 insertions(+), 16 deletions(-) create mode 100644 .changeset/review-skill-rule-quote.md 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/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 => { * * **