review: render the quoted skill rule into the comment the author reads

.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.

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);

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"])

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)", () => {

workflows/review/lib/render-comment.ts

@@ -117,19 +117,30 @@ export const labelForFinding = (finding: Finding): ConventionalLabel => {
  *
  *     **<label>:** <model_authored_prose>
  *
+ *     > **Rule:** <rule_quote>
+ *
  *     ```suggestion
  *     <suggested_patch>
  *     ```
  *
  * The label and the `**…:**` wrapping are code-owned; the prose after it is the
  * model's `model_authored_prose` copied verbatim (it already carries the subject
- * and any discussion). The suggestion block is appended only when the finding
- * carries a `suggested_patch`, again copied verbatim. No other text is emitted.
+ * and any discussion). For a skill finding carrying a `rule_quote`, the exact
+ * rule text is surfaced as a blockquote — quote-the-rule already puts it in
+ * `evidence_trace`, but authors never see evidence traces, so the comment is
+ * where the actual rule must appear (the quote itself is skill-file text copied
+ * verbatim; only the `> **Rule:**` wrapping is code-owned). The suggestion
+ * block is appended only when the finding carries a `suggested_patch`, again
+ * copied verbatim. No other text is emitted.
  */
 export const renderComment = (finding: Finding): string => {
     const label = labelForFinding(finding);
     const lines: string[] = [`**${label}:** ${finding.model_authored_prose}`];
 
+    if (finding.rule_quote !== undefined) {
+        lines.push("", `> **Rule:** ${finding.rule_quote}`);
+    }
+
     if (finding.suggested_patch !== undefined) {
         lines.push("", "```suggestion", finding.suggested_patch, "```");
     }

workflows/review/review.md

@@ -607,7 +607,12 @@ the model: `blocking` → `issue (blocking)`, `advisory` → `suggestion (non-bl
 lens is a correctness/risk lens, so it renders as a plain label, not a `, best-practice`
 variant). Take the candidate's `path`/`line` from the finding's `anchor` (a `line` anchor →
 `path`+`line`; a `pr` anchor → a top-level review comment with no line), its comment
-text from `model_authored_prose` (with `suggested_patch` as the fix block), and its
+text from `model_authored_prose` (with `suggested_patch` as the fix block; for a skill
+finding carrying `rule_quote`, append the quoted rule to the candidate's `discussion`
+as a `> **Rule:** <rule_quote>` blockquote between the prose and the fix block,
+matching the shared lib's `renderComment` — the quote is skill-file text copied
+verbatim, and it is what lets the author read the actual rule instead of a
+paraphrase), and its
 `failure_scenario` verbatim (it rides into `claims.json` for the validator). After this
 normalization a lens finding is a candidate in the **same** shape as every other
 reviewer's, so it flows through the identical scope-filter → `claims.json` → verdict →
@@ -1439,7 +1444,11 @@ skill-level default or a per-rule `must`/`never`/`blocking` vs `should`/`advisor
 annotation) sets the finding's `severity`; when the skill declares none, judge by
 impact. Flag a skill violation only when you can quote **both** the exact rule
 text from the skill file **and** the exact violating line; put both quotes in
-`evidence_trace`, with no spirit-of-the-doc inference.
+`evidence_trace`, with no spirit-of-the-doc inference. Also copy the exact rule
+text, verbatim, into the finding's `rule_quote` field: evidence traces never reach
+the author, and `rule_quote` is what gets rendered into the comment they read (as
+a `> **Rule:** …` blockquote), so the author sees the actual rule, not a
+paraphrase.
 
 ## Out-of-lane handoff
 
@@ -1465,7 +1474,9 @@ entry; `failure_scenario` names the concrete failing scenario (specific
 inputs/state, then the wrong outcome) — it is the specific claim the
 claim-validator attacks, so make it checkable; `producing_hunt` names the hunt
 that produced the finding; `model_authored_prose` carries the entire human-read
-comment. Omit `suggested_patch`/`pre_merge_obligation` unless they apply.
+comment. Omit `suggested_patch`/`pre_merge_obligation` unless they apply; a skill
+finding also carries `rule_quote` (§Lens-owned skills), which the orchestrator
+renders into the posted comment.
 
 Run **every** incident-derived hunt in your definition, even when the diff looks
 clean, and record each hunt's state in `hunts[]` as exactly one of: `found` (the
@@ -2413,7 +2424,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "producing_hunt": "authz-on-new-endpoint",
     "model_authored_prose": "the one- or two-sentence comment the author will read",
     "suggested_patch": "optional replacement/patch text",
-    "pre_merge_obligation": "optional: a condition that must hold before merge"
+    "pre_merge_obligation": "optional: a condition that must hold before merge",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "authz-on-new-endpoint", "state": "ran|not-applicable|found"}]
@@ -2474,7 +2486,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "unmoderated-model-output",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "unmoderated-model-output", "state": "ran|not-applicable|found"}]
@@ -2533,7 +2546,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "bulk-send-without-audience-filter",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "bulk-send-without-audience-filter", "state": "ran|not-applicable|found"}]
@@ -2593,7 +2607,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "cache-key-missing-identifier",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "cache-key-missing-identifier", "state": "ran|not-applicable|found"}]
@@ -2654,7 +2669,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "non-nullable-column-without-default",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "non-nullable-column-without-default", "state": "ran|not-applicable|found"}]
@@ -2714,7 +2730,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "unawaited-async",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "unawaited-async", "state": "ran|not-applicable|found"}]
@@ -2774,7 +2791,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "breaking-field-removal-or-retype",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "breaking-field-removal-or-retype", "state": "ran|not-applicable|found"}]
@@ -2838,7 +2856,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "serialized-shape-change",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "serialized-shape-change", "state": "ran|not-applicable|found"}]
@@ -2900,7 +2919,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "flag-default-unsafe",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "flag-default-unsafe", "state": "ran|not-applicable|found"}]
@@ -2960,7 +2980,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "float-money",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "float-money", "state": "ran|not-applicable|found"}]
@@ -3023,7 +3044,8 @@ Conventional-Comment `label` is emitted (the orchestrator computes it from
     "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce",
     "producing_hunt": "hardcoded-user-facing-string",
     "model_authored_prose": "the comment the author will read",
-    "suggested_patch": "optional", "pre_merge_obligation": "optional"
+    "suggested_patch": "optional", "pre_merge_obligation": "optional",
+    "rule_quote": "optional: for a skill finding, the exact rule text, verbatim"
   }],
   "out_of_lane_observations": [{"path": "...", "line": 0, "observation": "one sentence: the concern, stated concretely", "failure_scenario": "one sentence: the concrete inputs/state and the wrong outcome they produce", "suggested_lane": "correctness"}],
   "hunts": [{"hunt": "hardcoded-user-facing-string", "state": "ran|not-applicable|found"}]