Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jwbron wants to merge 6 commits into
base: jwbron/review-dispatch-tax
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7c07a42
[jwbron/review-skill-rule-quote] review: render the quoted skill rule…
jwbron Jul 9, 2026
b54f94e
[tmp/skill-rule-quote] Merge branch 'jwbron/review-dispatch-tax' into…
jwbron Jul 10, 2026
ddbdd46
[jwbron/review-skill-rule-quote] Merge branch 'jwbron/review-dispatch…
jwbron Jul 10, 2026
514d9ea
Merge branch 'jwbron/review-dispatch-tax' into jwbron/review-skill-ru…
jwbron Jul 10, 2026
7eb5501
review: live-enabled corpus format and ten live cases (#233)
jwbron Jul 10, 2026
ccfdd5a
[landtmp] Merge remote-tracking branch 'origin/main' into landtmp
jwbron Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/review-skill-rule-quote.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
14 changes: 13 additions & 1 deletion workflows/review/eval/corpus/live.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down
22 changes: 22 additions & 0 deletions workflows/review/lib/finding-schema.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 17 additions & 0 deletions workflows/review/lib/finding-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"])
Expand Down
35 changes: 35 additions & 0 deletions workflows/review/lib/render-comment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)", () => {
Expand Down
15 changes: 13 additions & 2 deletions workflows/review/lib/render-comment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "```");
}
Expand Down
50 changes: 36 additions & 14 deletions workflows/review/review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 →
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down Expand Up @@ -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"}]
Expand Down
Loading