From be04ce0970d79a4652442ff41cbd7ba178bc283b Mon Sep 17 00:00:00 2001 From: amirbena Date: Thu, 24 Sep 2026 16:25:12 +0300 Subject: [PATCH] Add opt-in structured review result to local review (#69) local-code-review can now append one schema-versioned machine-readable JSON result after its unchanged human report when the invocation sets structured_review_result (default off). A packaged policy owns the serialization and finding-identity minting; a drift test pins it to the #67 schema and the identity reference model. Co-Authored-By: Claude Sonnet 5 --- .../structured-output/capability.yaml | 16 ++ docs/ARCHITECTURE.md | 9 +- docs/features/README.md | 1 + docs/features/structured-review-result.md | 55 +++++ docs/review-result/README.md | 5 +- docs/review-result/review-result-model.md | 12 +- .../packaging/generate_package_manifest.py | 1 + scripts/packaging/package-manifest.json | 1 + shared/policies/README.md | 1 + shared/policies/invocation-options.md | 31 ++- shared/policies/structured-output.md | 201 ++++++++++++++++++ skills/github-pr-review/metadata/skill.yaml | 1 + skills/local-code-review/README.md | 7 +- skills/local-code-review/SKILL.md | 6 + skills/local-code-review/metadata/skill.yaml | 2 + .../runbooks/local-review.md | 10 +- .../templates/local-review-report.md | 7 + ..._github_pr_review_output_tightening_223.py | 10 +- .../test_structured_review_result_docs.py | 115 ++++++++++ tests/reference/review/invocation_options.py | 20 +- tests/unit/review/test_invocation_options.py | 62 ++++++ 21 files changed, 551 insertions(+), 22 deletions(-) create mode 100644 capabilities/structured-output/capability.yaml create mode 100644 docs/features/structured-review-result.md create mode 100644 shared/policies/structured-output.md create mode 100644 tests/policy/review/test_structured_review_result_docs.py diff --git a/capabilities/structured-output/capability.yaml b/capabilities/structured-output/capability.yaml new file mode 100644 index 00000000..de4bdeb8 --- /dev/null +++ b/capabilities/structured-output/capability.yaml @@ -0,0 +1,16 @@ +capability: structured-output +summary: >- + Opt-in machine-readable rendering of an already-finalized review: one + schema-versioned JSON result appended after the unchanged human report. +loads: on-activation +activation: + - the invocation resolves structured_review_result to true +adapters: [local, github] +files: + - shared/policies/structured-output.md +requires: [review-kernel] +never: + - changing a finding, severity, coverage, or the mechanical Decision + - replacing or reordering the human-readable report + - inventing an identity value that was not computed +benchmark: tests/reference/review/review_result.py diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index baf84c7b..374b9468 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -466,8 +466,9 @@ and no packaged Skill resource depends on them. recorded in the model ([`review-result/README.md`](review-result/README.md) → [`review-result/review-result-model.md`](review-result/review-result-model.md), - #67, with a test-only validator). No packaged Skill resource is changed; - versioning (#68) and runtime emission (#69/#70) are deferred — see + #67, with a test-only validator). Versioning is #68; + `local-code-review` emits the result opt-in (#69, packaged + `structured-output.md`); GitHub emission (#70) is deferred — see "Future work" below. - **Candidate-finding validation model** — the `observation → candidate claim → validated finding → severity` reasoning contract: the @@ -657,8 +658,8 @@ or runbook implements them today: renders it in human output. The #67 schema ([`review-result/README.md`](review-result/README.md)) now carries `confidence`, `severity`, `location`, and the rest as a formal contract - for machine consumers; its versioning policy (#68), Skill wiring and - runtime emission (#69/#70), and consumers (#71) are still unbuilt. + for machine consumers; its versioning policy (#68) and local emission + (#69) are built; GitHub emission (#70) and consumers (#71) are still unbuilt. - **Repository-intelligence retrieval and a packaged relationship-influence field** — the repository-intelligence model (#129, "Repository-development instrumentation" above) is a design record and a test-only reference diff --git a/docs/features/README.md b/docs/features/README.md index 50798f8a..8a3b9fcd 100644 --- a/docs/features/README.md +++ b/docs/features/README.md @@ -32,6 +32,7 @@ each Skill's own README | [GitHub publication & review authorization](github-review-publication.md) | one canonical publication mode — `PASSIVE` / `SEMI` / `ACTIVE` — self-review, and the optional machine-readable status/check | `github-pr-review` | default is non-mutating `PASSIVE`; an explicit `ACTIVE` request is itself sufficient authorization to publish, subject to reviewer independence + GitHub permission | [`review-action-authorization.md`](../../skills/github-pr-review/policies/review-action-authorization.md), [`review-output.md`](../../skills/github-pr-review/policies/review-output.md), [`review-status-enforcement.md`](../../skills/github-pr-review/policies/review-status-enforcement.md) | | [Coding-agent fix prompt](fix-prompt.md) | appending a ready-to-run implementation prompt to qualifying findings | `local-code-review` | explicitly requested (`include_fix_prompt`, default off); output only | [`remediation-guidance.md`](../../shared/policies/remediation-guidance.md) | | [Reviewer Brief](reviewer-brief.md) | a private, caller-facing handoff — what changed, user-provided focus, manual review focus, open questions — appended to every result and structurally excluded from GitHub publication | `github-pr-review` | always on; presentation only over already-finalized analysis | [`reviewer-brief.md`](../../skills/github-pr-review/policies/reviewer-brief.md) | +| [Structured review result](structured-review-result.md) | appending one schema-versioned machine-readable JSON result (findings, coverage, decision, reviewed SHA) after the unchanged human report | `local-code-review` | explicitly requested (`structured_review_result`, default off); output only | [`structured-output.md`](../../shared/policies/structured-output.md), [`invocation-options.md`](../../shared/policies/invocation-options.md) | | [Severity descriptions](severity-description.md) | expanding the compact `P0` / `P1` / `P2` finding-headline code to include its short canonical parenthetical (`Critical` / `Blocking` / `Non-Blocking`) | `github-pr-review` | explicitly requested (`include_severity_description`, default off — compact); presentation only | [`invocation-options.md`](../../shared/policies/invocation-options.md), [`review-output.md`](../../skills/github-pr-review/policies/review-output.md) | ## Not a feature guide diff --git a/docs/features/structured-review-result.md b/docs/features/structured-review-result.md new file mode 100644 index 00000000..3c0b21fe --- /dev/null +++ b/docs/features/structured-review-result.md @@ -0,0 +1,55 @@ +# Structured review result + +## What it does + +When enabled, `local-code-review` appends one **machine-readable JSON +result** after its normal human report — the same findings, coverage, and +`REVIEW CLEAN` / `CHANGES REQUIRED` / `REVIEW INCOMPLETE` decision, in the +schema-versioned shape defined for review results. The document carries +`schema_version` and the reviewed head SHA. It is normalized internally to +the `structured_review_result` option (default `false`). + +Without it, the report is exactly the default human report. + +## When it is useful + +- A script, dashboard, or another agent needs to consume the review + without parsing prose. + +## Which Skill(s) + +`local-code-review` only. `github-pr-review` does not emit it yet +(tracked separately). + +## Default, conditional, or requested + +**Explicitly requested; default off.** Set from the current invocation +only: `structured_review_result=true`, the bare option name, *"include a +structured review result"*, or the fixed phrases *"machine-readable review +result"* / *"review result as JSON"*. Vague wording such as *"give me +JSON"* does not enable it. + +## How to invoke it + +```text +/local-code-review +structured_review_result=true +``` + +## Limitations & safety boundaries + +- **Output only.** Findings, severities, coverage, and the mechanical + decision are identical on and off; the human report is unchanged and + still comes first. +- `reviewed_head_sha` is `null` when the reviewed target includes + uncommitted changes, because a commit SHA cannot identify them. +- It is not emitted for an ungraded outcome (for example an unresolved + Jira reference) or when the report itself was withheld. +- Nothing is written to a file or published; it is returned inside the + one report. + +## Canonical semantics + +[`shared/policies/structured-output.md`](../../shared/policies/structured-output.md) +· [`shared/policies/invocation-options.md`](../../shared/policies/invocation-options.md) +· schema and versioning: [`../review-result/README.md`](../review-result/README.md). diff --git a/docs/review-result/README.md b/docs/review-result/README.md index 8bb0fce5..73e60c38 100644 --- a/docs/review-result/README.md +++ b/docs/review-result/README.md @@ -7,8 +7,9 @@ Schema and example for the machine-readable form of one review's output. Like [`../review-telemetry/README.md`](../review-telemetry/README.md) and [`../finding-confidence/README.md`](../finding-confidence/README.md), these are repository-development docs: **not** packaged into either Skill -archive, and no packaged Skill resource depends on them. Nothing emits a -review result yet. +archive, and no packaged Skill resource depends on them. `local-code-review` emits a +result on request ([#69](https://github.com/amirbena/code-review-skill/issues/69)); +`github-pr-review` does not yet. ## Document map diff --git a/docs/review-result/review-result-model.md b/docs/review-result/review-result-model.md index 9a0bcfee..bfd816ab 100644 --- a/docs/review-result/review-result-model.md +++ b/docs/review-result/review-result-model.md @@ -14,8 +14,8 @@ this record is the bug. Not packaged: no packaged Skill resource depends on this record, the schema, or the example (see [`../../AGENTS.md`](../../AGENTS.md), "Packaged -Skills are independent of repository-level instructions"). Nothing emits a -review result yet — see section 7. +Skills are independent of repository-level instructions"). Emission is +narrower than the schema — see section 7. ## 1. Files @@ -119,12 +119,12 @@ schema failing. | Concern | Owner | | --- | --- | | Versioning policy and compatibility rules for `schema_version` | [`schema-versioning.md`](schema-versioning.md) ([#68](https://github.com/amirbena/code-review-skill/issues/68)) | -| Skill wiring and runtime emission of a result | [#69](https://github.com/amirbena/code-review-skill/issues/69), [#70](https://github.com/amirbena/code-review-skill/issues/70) | +| Local Skill emission (opt-in `structured_review_result`; packaged restatement in [`structured-output.md`](../../shared/policies/structured-output.md), pinned to this schema by a drift test) | [#69](https://github.com/amirbena/code-review-skill/issues/69) | +| GitHub Skill emission | [#70](https://github.com/amirbena/code-review-skill/issues/70) | | Consumers of the result | [#71](https://github.com/amirbena/code-review-skill/issues/71) | | Parent capability | [#44](https://github.com/amirbena/code-review-skill/issues/44) | -Until emission lands, this record, the schema, and the example are -contract-only: the packaged Skills still produce their existing Markdown -output, and [`finding.md`](../../shared/templates/finding.md)'s note that a +Only `local-code-review` emits a result, and only on request; `github-pr-review` still +produces its existing Markdown output, and [`finding.md`](../../shared/templates/finding.md)'s note that a machine-readable renderer would be "another projection of the same fields" is what this schema is the first instance of. diff --git a/scripts/packaging/generate_package_manifest.py b/scripts/packaging/generate_package_manifest.py index 98b3e104..12343ce1 100644 --- a/scripts/packaging/generate_package_manifest.py +++ b/scripts/packaging/generate_package_manifest.py @@ -100,6 +100,7 @@ class Entry(NamedTuple): Entry("shared/policies/invocation-options.md", None), Entry("shared/policies/remediation-guidance.md", "remediation"), Entry("shared/policies/remediation-scope-boundary.md", "remediation"), + Entry("shared/policies/structured-output.md", "structured-output"), Entry("shared/policies/specialist-depth.md", "specialist-depth"), Entry("shared/policies/security-deepening.md", "specialist-depth"), Entry("shared/policies/distributed-systems-deepening.md", "specialist-depth"), diff --git a/scripts/packaging/package-manifest.json b/scripts/packaging/package-manifest.json index 085766a6..a6339865 100644 --- a/scripts/packaging/package-manifest.json +++ b/scripts/packaging/package-manifest.json @@ -32,6 +32,7 @@ { "source": "shared/policies/invocation-options.md", "destination": "shared/policies/invocation-options.md" }, { "source": "shared/policies/remediation-guidance.md", "destination": "shared/policies/remediation-guidance.md" }, { "source": "shared/policies/remediation-scope-boundary.md", "destination": "shared/policies/remediation-scope-boundary.md" }, + { "source": "shared/policies/structured-output.md", "destination": "shared/policies/structured-output.md" }, { "source": "shared/policies/specialist-depth.md", "destination": "shared/policies/specialist-depth.md" }, { "source": "shared/policies/security-deepening.md", "destination": "shared/policies/security-deepening.md" }, { "source": "shared/policies/distributed-systems-deepening.md", "destination": "shared/policies/distributed-systems-deepening.md" }, diff --git a/shared/policies/README.md b/shared/policies/README.md index 4df09ff5..cd97f2e4 100644 --- a/shared/policies/README.md +++ b/shared/policies/README.md @@ -33,6 +33,7 @@ packaged Skill is installed; they do not depend on this repository. | [`failure-retry-recovery.md`](failure-retry-recovery.md) | The signal-triggered failure-state, retry-safety, and recovery pass, and the applicability-gated observability hierarchy that decides whether a missing detection/diagnosis signal is itself a finding. | | [`architectural-placement.md`](architectural-placement.md) | Whether changed code is correctly placed within the surrounding execution lifecycle: the semantic-risk trigger vocabulary, bounded ring-by-ring context expansion, stop conditions, and the evidence requirement for a placement finding. Also owns the bounded, analogue-based trigger for an undocumented structural/organizational responsibility-placement pattern. | | [`api-contract-compatibility.md`](api-contract-compatibility.md) | The API/contract compatibility depth owner: recognizing a changed repository contract, classifying its change shape as compatible / breaking / context-dependent, and the fail-closed rule for an unresolvable consumer surface. | +| [`structured-output.md`](structured-output.md) | The opt-in machine-readable review result: activation, placement after the unchanged human report, the serialization of the finalized review, and finding-identity minting. Currently consumed by `local-code-review` only. | | [`severity.md`](severity.md) | The single P0/P1/P2 severity each finding receives and the mechanical severity → decision derivation. | | [`verdict-consistency.md`](verdict-consistency.md) | The read-only, pre-render/pre-publish check that a rendered or submitted decision signal agrees with `severity.md`'s already-finalized mechanical derivation; withhold-and-report on a detected mismatch, never self-correct or warn-and-continue. | | [`evidence.md`](evidence.md) | Every finding must rest on concrete repository evidence; what counts as evidence. | diff --git a/shared/policies/invocation-options.md b/shared/policies/invocation-options.md index 9b47cda0..e2edf275 100644 --- a/shared/policies/invocation-options.md +++ b/shared/policies/invocation-options.md @@ -62,6 +62,13 @@ language interpretation. anchor (`github-pr-review`'s `finding-placement.md` is unchanged and remains authoritative for placement), or publication ordering — only the wording of an inline finding. +- `structured_review_result` — local-only, default `false`; when `true`, + `local-code-review` appends one schema-versioned machine-readable JSON + result after its unchanged human report, per + [`structured-output.md`](structured-output.md). + `github-pr-review` normalizes it for parity but has no structured result + surface, so it has no effect there. Output-only: it never changes scope, + findings, severity, coverage, or the mechanical Decision. - `include_severity_description` — default `false` for both Skills; controls whether `github-pr-review`'s reader-visible severity-legend parenthetical (`P0 (Critical)` / `P1 (Blocking)` / `P2 (Non-Blocking)`) @@ -122,7 +129,8 @@ underscores treated as equivalent inside the option name: The finite vocabulary is the five canonical option concepts: `fix prompt`, `fix guidance`, `finding details`, `human review output`, and `severity -description`. Ordinary +description`. The local-only `structured review result` concept is recognized +by its own fixed phrase set below. Ordinary mentions, questions about an option, quoted examples, and vague requests such as “make it helpful”, “be detailed”, or “make it nicer” are ambiguous and do not set a flag. Do not use sentiment, urgency, severity, prior turns, or a @@ -218,6 +226,27 @@ the severity-legend parenthetical renders; it never changes whether severity is shown, whether the headline is emphasized, or any semantics owned by `severity.md`. +### `structured_review_result` phrasings + +Alongside the canonical `structured_review_result=true|false` assignment and +the bare option name (`structured_review_result`, `structured review +result`, `structured-review-result`), it recognizes a small, fixed set of +explicit phrasings (case-insensitively, whitespace-flexible): + +- affirmative: `machine-readable review result`, `review result as json`; +- negative: `no machine-readable review result`, `human report only`. + +Text naming `structured review result` (any of the spellings above) belongs +to this option alone: it is never also read as the `structured review` +negative phrase of `human_review_output`, so requesting both a senior-style +review and a structured result sets both. + +This phrase set is exhaustive. Anything outside it — "give me json", "make +it parseable", a question about the option — is ambiguous and does not set +the flag; the default `false` then applies. When both an affirmative and a +negative phrasing appear, the values conflict and the option falls through +to the default. + Resolve each option independently with this precedence: ```text diff --git a/shared/policies/structured-output.md b/shared/policies/structured-output.md new file mode 100644 index 00000000..404b5405 --- /dev/null +++ b/shared/policies/structured-output.md @@ -0,0 +1,201 @@ +# Policy — Structured Review Result (opt-in) + +An opt-in, machine-readable rendering of the review that has **already** +been finalized. It is one more projection of the same findings, coverage, +and mechanical Decision the human report renders — never a second review, +never a source of a finding, severity, or decision of its own. Currently +consumed by `local-code-review` only. + +## Activation + +The option `structured_review_result` (boolean, default `false`) is +normalized per [`invocation-options.md`](invocation-options.md). When it +resolves `false`, this policy is not loaded and the report is exactly the +default report. When it resolves `true`, the Skill renders the human report +**unchanged** and then appends the result below. The option is +output-only: scope, inspection, evidence, findings, severity, +deduplication, coverage, and the Decision are identical on and off. + +## Placement and form + +After the report's trailing Review Metadata / Review scope contract, +append exactly one section: + +```text +### Structured Review Result +``` + +followed by exactly one fenced `json` block holding one JSON object — no +comments, no trailing commas, no second document. The block is generated +from the finalized review, after the verdict-consistency check; if that +check withheld the report, or the review is ungraded +(`JIRA CONTEXT UNRESOLVED`), emit no block and state one line +`Structured review result not emitted: `. Never publish, stream, +or write it to a file: it is returned inside the same single report. + +## Document shape + +Unknown keys are forbidden at every object. Required keys are always +present; an optional finding field with no value is **omitted**, never +`null`. + +| Key | Value | +| --- | --- | +| `schema_version` | the string `"1.0.0"` | +| `skill` | `"local-code-review"` | +| `reviewed_state` | object, below | +| `coverage` | `"complete"` or `"incomplete"` — the report's Coverage | +| `decision` | `{ "derived", "outcome" }`, below | +| `counts` | `{ "p0", "p1", "p2" }` — integer tally of `findings` by severity | +| `summary` | the report's "What changed" prose, non-empty | +| `findings` | every finding in report order, **including P2 on a clean review**; `[]` when none | + +`reviewed_state` — all keys required: + +| Key | Value | +| --- | --- | +| `repository` | `owner/name` from the upstream remote URL when one exists, otherwise the repository directory name | +| `base_branch` | the review base branch (Review Metadata) | +| `base_sha` | full lowercase hex SHA of the review base, or `null` | +| `merge_base_sha` | full hex SHA of the merge base of base and HEAD, or `null` | +| `reviewed_head_sha` | Local HEAD's full hex SHA **only when the whole reviewed target is committed** (staged, unstaged, and untracked categories all empty); otherwise `null`, because a SHA does not identify uncommitted content and must not claim it does | +| `reviewer_identity` | `null` unless the runtime established a reviewer identity | +| `completeness` | `"full"` | +| `prior_reviewed_sha` | `null` — a stateless review never reads prior state | + +`decision.derived` is `"blocking"` when any P0/P1 is present, else +`"clean"` ([`severity.md`](severity.md), "Decision derivation +(mechanical)"). `decision.outcome` equals `derived`, except +`"incomplete"` when coverage is `incomplete` +([`review-stopping-criteria.md`](review-stopping-criteria.md), "Labeling"). +Rendered labels map `clean` → `REVIEW CLEAN`, `blocking` → `CHANGES +REQUIRED`, `incomplete` → `REVIEW INCOMPLETE`; a serialized `clean` never +means "no findings". + +## Finding object + +Serialized from the finalized finding in +[`../templates/finding.md`](../templates/finding.md), one key per field, +`snake_case`: + +| Key | Required | Value | +| --- | --- | --- | +| `id` | yes | the review-local `F` label | +| `severity` | yes | `"P0"`, `"P1"`, or `"P2"` | +| `title`, `evidence`, `impact`, `fix` | yes | the finding's text, non-empty | +| `location` | yes | the canonical location **without** the trailing source-category or unresolved-fix annotation | +| `fix_location_resolved` | yes | `false` exactly when the unresolved-fix annotation applies, else `true` | +| `runtime_validation` | yes | `"reasoned"` (default), `"runtime-confirmed"`, or `"attempted-inconclusive"` | +| `confidence` | yes | `"credible"` (default), `"confirmed"`, `"runtime-validation-unavailable"`, `"external-contract-unvalidated"`, or `"insufficient-context"` | +| `identity` | yes | `{ "stable_id", "matching_eligible" }`, below | +| `evidence_location`, `follow_up`, `details`, `capability` | no | only when populated (`details` regardless of the detail-presentation option) | +| `affected_locations` | no | array of `{ "location", "note" }`, at least two, only on a consolidated finding | +| `contextual_evidence` | no | array of non-empty strings, only when provenance exists | +| `defect_kind` | no | lowercase kebab-case slug, only when a narrow class applies | + +The implementation prompt and the source-category annotation are +rendering-only and are never serialized. `confidence` must not contradict +`runtime_validation` (a `runtime-confirmed` finding is `confirmed`). + +## Finding identity + +`identity.stable_id` is `fid_v1_` followed by the first 32 lowercase hex +characters of the SHA-256 of a canonical serialization of the finding's +descriptor. Compute the digest with a shell hashing command +(`shasum -a 256` or equivalent); never write a value you did not compute. +The construction is a pure function of the inputs below — never of +severity, the `F` ordinal, the HEAD SHA, discovery order, or other +findings. + +**Tokenizer.** Over a source fragment: outside string literals delete +`/* … */` and `#`-to-end-of-line comments (`//` is **not** a comment); +delete ASCII control characters; collapse whitespace runs. Emit, in order: +identifiers (`[A-Za-z_][A-Za-z0-9_]*`), numbers (`\d+(\.\d+)?`), quoted +string literals verbatim, maximal runs of operator characters +(`- + * / % = ! < > & | ^ ~ .`), and single brackets. Never emit `,` or +`;`. Preserve order, multiplicity, and case. + +**Descriptor fields**, in this order. `ABSENT` = the facet legitimately +does not apply; `UNCLASSIFIABLE` = present but not reducible without +guessing. Never guess a value. + +| Field | Construction | +| --- | --- | +| `repository` | `reviewed_state.repository` | +| `location_intent` | `line`, `symbol`, `file`, `cross_file`, or `repository` by the canonical `location`'s precision; none fits → `UNCLASSIFIABLE` | +| `path` | repository-relative, `/` separators, no leading `./` or `/`, no `.`/`..` segment; `ABSENT` for `cross_file`/`repository` or no usable path | +| `symbol` | the enclosing qualified named definition (chain joined with `.`) read from the reviewed source; `ABSENT` when it cannot be named | +| `construct` | `statement`, `declaration`, `call`, `expression`, `config_key`, `section`, or `block`; none fits → `UNCLASSIFIABLE` | +| `anchor_tokens` | tokenizer over the smallest source/config fragment that demonstrates the defect, at the reviewed revision; empty list when there is none | +| `mechanism_key` | tokenizer over the source fragment naming the unsafe operation or violated invariant; no fragment → `UNCLASSIFIABLE` | +| `cause_key` / `behavior_key` | the finding's concise cause → faulty-behavior claim (no impact/fix/severity), case-folded and whitespace-collapsed, split at its first connective among ` so `, ` causing `, ` resulting in `, ` leads to `, ` which causes `, ` therefore `, ` -> `, ` → `; tokenizer over the left / right clause after trimming whitespace and trailing sentence punctuation. No connective or an empty side → `UNCLASSIFIABLE` | + +**Serialization.** A string `s` → `len(s)` `\x1f` `s`; a token list `t` → +`len(t)` `\x1f` the tokens joined by `\x1f`; `ABSENT` → `\x00A`; +`UNCLASSIFIABLE` → `\x00U`. Emit each field as `\x1d`, +join the fields with `\x1e`, and prefix the whole with `v1\x1e`. + +**`matching_eligible`** is `false` when any holds, else `true`: repository +unresolvable; `location_intent` is `UNCLASSIFIABLE`; `anchor_tokens` is +empty **and** `mechanism_key` is `UNCLASSIFIABLE`; none of `symbol`, +`mechanism_key`, `cause_key`, `behavior_key` is classified; or the +reviewed source needed for the anchor or mechanism could not be read. A +non-eligible finding still gets its deterministic `stable_id`. When in +doubt, `false`. + +## Example + +One blocking review of an uncommitted target (so `reviewed_head_sha` is +`null`), with one finding: + +```json +{ + "schema_version": "1.0.0", + "skill": "local-code-review", + "reviewed_state": { + "repository": "acme/payments", + "base_branch": "main", + "base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071", + "merge_base_sha": "3f9c1e2a7b4d5c60918273645a1b2c3d4e5f6071", + "reviewed_head_sha": null, + "reviewer_identity": null, + "completeness": "full", + "prior_reviewed_sha": null + }, + "coverage": "complete", + "decision": { "derived": "blocking", "outcome": "blocking" }, + "counts": { "p0": 0, "p1": 1, "p2": 0 }, + "summary": "Adds bounded retries around the charge call.", + "findings": [ + { + "id": "F1", + "severity": "P1", + "title": "Retry loop reports a timed-out charge as successful", + "location": "src/payments/retry.py:88", + "fix_location_resolved": true, + "evidence": "`charge_with_retry` falls through to `return ChargeResult.ok()` after every attempt times out.", + "impact": "A charge that never completed is recorded as paid.", + "fix": "Return a failure result when every attempt timed out.", + "runtime_validation": "reasoned", + "confidence": "credible", + "identity": { + "stable_id": "fid_v1_2c7a0d6b2c80a885a1752c05f15d70c0", + "matching_eligible": true + } + } + ] +} +``` + +The `stable_id` above is illustrative; a real value is always computed as +described in "Finding identity". + +## Non-goals and ownership + +- No new review semantics: every value is owned by the policy or template + named in this file; this file owns only the serialization and its + opt-in activation. +- No GitHub publication of the result (`github-pr-review` does not consume + this policy), no persistence, and no cross-review lifecycle state. +- No change to the default report: with the option off, nothing here + applies. diff --git a/skills/github-pr-review/metadata/skill.yaml b/skills/github-pr-review/metadata/skill.yaml index a83e3297..23f71a60 100644 --- a/skills/github-pr-review/metadata/skill.yaml +++ b/skills/github-pr-review/metadata/skill.yaml @@ -79,6 +79,7 @@ shared: - ../../../shared/policies/invocation-options.md - ../../../shared/policies/remediation-guidance.md - ../../../shared/policies/remediation-scope-boundary.md + - ../../../shared/policies/structured-output.md - ../../../shared/policies/specialist-depth.md - ../../../shared/policies/security-deepening.md - ../../../shared/policies/distributed-systems-deepening.md diff --git a/skills/local-code-review/README.md b/skills/local-code-review/README.md index 69e397f1..01009dd0 100644 --- a/skills/local-code-review/README.md +++ b/skills/local-code-review/README.md @@ -51,7 +51,9 @@ without widening the local delta; [`include_fix_prompt`](../../docs/features/fix-prompt.md) adds a coding-agent-ready prompt to qualifying findings; [`human_review_output`](../../docs/features/human-review-output.md) renders -the summary in a concise senior-engineer voice. +the summary in a concise senior-engineer voice; +[`structured_review_result`](../../docs/features/structured-review-result.md) +appends a schema-versioned machine-readable result. [Runtime validation evidence](../../docs/features/runtime-validation.md) applies automatically when the repository declares a suitable command and the runtime provides a verified isolation boundary. The @@ -109,7 +111,8 @@ These are summaries. The binding text lives in [runtime validation](../../docs/features/runtime-validation.md), [parallel review](../../docs/features/parallel-review.md), [human-style output](../../docs/features/human-review-output.md), - [fix prompt](../../docs/features/fix-prompt.md) + [fix prompt](../../docs/features/fix-prompt.md), + [structured result](../../docs/features/structured-review-result.md) - [`runbooks/local-review.md`](runbooks/local-review.md) — the full numbered procedure - [`policies/`](policies/repository-state.md) — the rules this Skill owns diff --git a/skills/local-code-review/SKILL.md b/skills/local-code-review/SKILL.md index 1fe6eb60..1ec7f1d0 100644 --- a/skills/local-code-review/SKILL.md +++ b/skills/local-code-review/SKILL.md @@ -142,6 +142,12 @@ current invocation only per Output-only: it never changes the Review Target, inspection, evidence, finding identity, severity, deduplication, PR-context reconciliation, or the mechanical Decision. +- `structured_review_result` (boolean, default `false`) — output-only + opt-in: when `true`, one schema-versioned machine-readable JSON result + is appended after the unchanged human report, per + [`structured-output.md`](../../shared/policies/structured-output.md); + loaded only then. Never changes findings, severity, coverage, or the + mechanical Decision. - `human_inline_findings` (derived default — `explicit_value ?? human_review_output`) — a `github-pr-review` inline-comment concept, recognized here only for direct/mediated normalization parity; it has diff --git a/skills/local-code-review/metadata/skill.yaml b/skills/local-code-review/metadata/skill.yaml index be392404..31a6854e 100644 --- a/skills/local-code-review/metadata/skill.yaml +++ b/skills/local-code-review/metadata/skill.yaml @@ -21,6 +21,7 @@ capabilities: local_repository_access: required human_review_output: optional # presentation-only; natural-language opt-in ("make the review shorter and more human" / "like a senior engineer"), no CLI flag; renders only the human-facing body in a concise senior-engineer voice, trailing metadata unchanged; never changes findings/severity/dedup/reconciliation or the mechanical Decision; see ../../../shared/policies/invocation-options.md human_inline_findings: optional # presentation-only; derived default `explicit_value ?? human_review_output`; a github-pr-review inline-comment concept, normalized here only for direct/mediated parity — local-code-review publishes no inline review comments, so it has no effect on local output; see ../../../shared/policies/invocation-options.md + structured_review_result: optional # output-only; opt-in via `structured_review_result=true` or a fixed phrase set; appends one schema-versioned machine-readable JSON result after the unchanged human report, generated from the finalized review; never changes findings/severity/coverage or the mechanical Decision; see ../../../shared/policies/structured-output.md external_review_context: optional # free-form requirements / pasted Jira / GitHub Issue / HLD / ADR / plan; see ../../../shared/policies/review-context.md and ../policies/review-context.md requirement_coverage: conditional # activates only for authoritative requirements / acceptance criteria; emits evidence-backed per-requirement status plus task-relative completeness; see ../../../shared/policies/requirement-coverage.md jira_context_resolution: optional # only when a Jira reference is supplied; resolved via an available Jira MCP/connector, read-only; unresolved -> JIRA CONTEXT UNRESOLVED (no inference fallback); see ../../../shared/policies/review-context.md "Jira context resolution" @@ -65,6 +66,7 @@ shared: - ../../../shared/policies/invocation-options.md - ../../../shared/policies/remediation-guidance.md - ../../../shared/policies/remediation-scope-boundary.md + - ../../../shared/policies/structured-output.md - ../../../shared/policies/specialist-depth.md - ../../../shared/policies/security-deepening.md - ../../../shared/policies/distributed-systems-deepening.md diff --git a/skills/local-code-review/runbooks/local-review.md b/skills/local-code-review/runbooks/local-review.md index 043341bf..dcf0cf3c 100644 --- a/skills/local-code-review/runbooks/local-review.md +++ b/skills/local-code-review/runbooks/local-review.md @@ -471,7 +471,15 @@ which a value must be resolved before it is used, or what is reported. (review base, per-category inclusion/exclusion, initial-review-vs- re-review, and, per that template's own "Relevance-aware metadata rendering," the staged fingerprint and whether previously reviewed - state changed) — and return it. **Stop.** + state changed) — and return it, together with the step 13b section when + that step applies. **Stop.** +13b. **If, and only if, the current invocation normalized + `structured_review_result` to `true`:** load + [`structured-output.md`](../../../shared/policies/structured-output.md) + and append its single "Structured Review Result" section after the + rendered report, generated from the already-finalized findings, + coverage, and Decision. The human report above is unchanged. Skip this + step entirely otherwise. ## Constraints diff --git a/skills/local-code-review/templates/local-review-report.md b/skills/local-code-review/templates/local-review-report.md index 1abf7c6a..605fa1af 100644 --- a/skills/local-code-review/templates/local-review-report.md +++ b/skills/local-code-review/templates/local-review-report.md @@ -353,6 +353,13 @@ serve are owned by the linked policies and are not restated here. the one field that **does** change the Decision: `incomplete` overrides the mechanical clean/blocking derivation and renders `REVIEW INCOMPLETE` instead, per the "Decision" rule above. +- **Structured Review Result (opt-in).** Only when the invocation + selects `structured_review_result` (default `false`), one + `### Structured Review Result` section holding a single fenced `json` + block follows the trailing "Review scope contract", per + [`../../../shared/policies/structured-output.md`](../../../shared/policies/structured-output.md). + It is generated from the finalized review; with the option off, the + report has no such section and is unchanged. - **No loop/orchestration metadata.** This report never tracks review iteration count, a configured maximum, or whether another iteration is allowed — that belongs to the orchestrator (see diff --git a/tests/policy/review/presentation/test_github_pr_review_output_tightening_223.py b/tests/policy/review/presentation/test_github_pr_review_output_tightening_223.py index 33311c63..d7f3d037 100644 --- a/tests/policy/review/presentation/test_github_pr_review_output_tightening_223.py +++ b/tests/policy/review/presentation/test_github_pr_review_output_tightening_223.py @@ -115,11 +115,13 @@ # comparison) — also deliberate and unrelated to #223. The other files in # this map are untouched by # #89/#237/#211/#121/#175/#258/#82/#303/#369/#377/#406/#449/#134/#74 and keep -# their original #223-era hashes. +# their original #223-era hashes. The local report template, runbook, and +# SKILL.md were re-captured after Issue #69 (the opt-in structured review +# result option) — deliberate and unrelated to #223. LOCAL_BASELINE_HASHES = { - LOCAL_REPORT: "1abf7c6a28f6d6b73a845aa8c8f9e06e7d062a82", - LOCAL_RUNBOOK: "043341bfe83aa24d83775699abfbfe70da732c2f", - LOCAL_SKILL: "9f638065b9d0f7285b3887127065cb82739a13c3", + LOCAL_REPORT: "605fa1af75319e224140caa15c80f8e2448622a3", + LOCAL_RUNBOOK: "dcf0cf3c3283f7b4acc8a2b26f8c4951360d78e7", + LOCAL_SKILL: "f46c252519f80d00a4f4cb9b2f06f510b14b96df", LOCAL_POLICY_DIR / "invocation-approval.md": "3fad248e86f655af57a06a99624a226d56238e0d", LOCAL_POLICY_DIR / "pr-context.md": "5698bb668ec44b7cf588b26b037bda7220811809", LOCAL_POLICY_DIR / "repository-state.md": "6792d7e3f7ae1ad0214fff2e85db5e28ac6fb108", diff --git a/tests/policy/review/test_structured_review_result_docs.py b/tests/policy/review/test_structured_review_result_docs.py new file mode 100644 index 00000000..f766287a --- /dev/null +++ b/tests/policy/review/test_structured_review_result_docs.py @@ -0,0 +1,115 @@ +#!/usr/bin/env python3 +"""Drift guards for the opt-in structured review result (Issue #69). + +The packaged policy restates the review-result schema's shape and the +finding-identity minting recipe so a portable Skill can emit them without +the repository's `docs/`. These tests keep that restatement pinned to the +schema and to the test-only identity reference model. +""" + +from __future__ import annotations + +import json +import re +import unittest + +from tests.reference.review import finding_identity +from tests.reference.review.review_result import validate_review_result +from tests.support.paths import REPO_ROOT + +POLICY = REPO_ROOT / "shared/policies/structured-output.md" +SCHEMA = REPO_ROOT / "docs/review-result/review-result.schema.json" +SKILL = REPO_ROOT / "skills/local-code-review/SKILL.md" +RUNBOOK = REPO_ROOT / "skills/local-code-review/runbooks/local-review.md" +TEMPLATE = REPO_ROOT / "skills/local-code-review/templates/local-review-report.md" +OPTIONS = REPO_ROOT / "shared/policies/invocation-options.md" +LOCAL_YAML = REPO_ROOT / "skills/local-code-review/metadata/skill.yaml" + + +def _keys_in_policy(text: str, keys: list[str]) -> list[str]: + return [k for k in keys if f"`{k}`" not in text] + + +class StructuredReviewResultPolicyTests(unittest.TestCase): + @classmethod + def setUpClass(cls) -> None: + cls.policy = POLICY.read_text(encoding="utf-8") + cls.schema = json.loads(SCHEMA.read_text(encoding="utf-8")) + + def test_every_schema_key_is_named(self) -> None: + props = self.schema["properties"] + finding = self.schema["definitions"]["finding"]["properties"] + for label, keys in ( + ("top-level", list(props)), + ("reviewed_state", list(props["reviewed_state"]["properties"])), + ("finding", list(finding)), + ("identity", list(finding["identity"]["properties"])), + ): + with self.subTest(label=label): + self.assertEqual(_keys_in_policy(self.policy, keys), []) + + def test_schema_version_matches_current_schema(self) -> None: + self.assertIn(f'`"{self.schema["properties"]["schema_version"]["const"]}"`', self.policy) + + def test_enum_values_are_named(self) -> None: + finding = self.schema["definitions"]["finding"]["properties"] + enums = [ + self.schema["properties"]["skill"]["enum"][:1], + self.schema["properties"]["coverage"]["enum"], + self.schema["properties"]["decision"]["properties"]["outcome"]["enum"], + finding["severity"]["enum"], + finding["runtime_validation"]["enum"], + finding["confidence"]["enum"], + ] + for values in enums: + for value in values: + with self.subTest(value=value): + self.assertIn(f'"{value}"', self.policy) + + def test_required_keys_are_documented_as_required(self) -> None: + finding = self.schema["definitions"]["finding"] + optional = set(finding["properties"]) - set(finding["required"]) + section = self.policy.split("## Finding object", 1)[1].split("## Finding identity", 1)[0] + for key in finding["required"]: + with self.subTest(key=key): + self.assertRegex(section, rf"`{key}`[^\n]*\| yes \|") + for key in optional: + with self.subTest(key=key): + self.assertRegex(section, rf"`{key}`[^\n]*\| no \|") + + def test_identity_recipe_matches_reference_model(self) -> None: + text = self.policy + for connective in finding_identity.CAUSE_BEHAVIOR_CONNECTIVES: + self.assertIn(f"`{connective}`", text) + for value in finding_identity.LOCATION_INTENTS + finding_identity.CONSTRUCT_KINDS: + self.assertIn(f"`{value}`", text) + rows = re.findall(r"^\| `([a-z_]+)`(?: / `([a-z_]+)`)? \|", text.split("**Descriptor fields**", 1)[1], re.M) + documented = [name for row in rows for name in row if name] + self.assertEqual(documented, list(finding_identity.DISCRIMINATING_FIELDS)) + self.assertIn(f"`{finding_identity.IDENTITY_SCHEME}\\x1e`", text) + self.assertIn("fid_v1_", text) + self.assertIn("first 32 lowercase hex", text) + for field in finding_identity.STRONG_SEMANTIC_FIELDS: + self.assertIn(f"`{field}`", text.split("**`matching_eligible`**", 1)[1]) + + def test_policy_example_is_a_valid_review_result(self) -> None: + blocks = re.findall(r"```json\n(.*?)\n```", self.policy, re.S) + self.assertEqual(len(blocks), 1) + self.assertEqual(validate_review_result(json.loads(blocks[0])), ()) + + def test_policy_is_portable_and_defaults_off(self) -> None: + self.assertNotIn("](../../skills/", self.policy) + self.assertNotIn("AGENTS.md", self.policy) + self.assertIn("default `false`", self.policy) + self.assertIn("unchanged", self.policy) + + def test_skill_wiring_is_opt_in_and_registered(self) -> None: + for path in (SKILL, RUNBOOK, TEMPLATE, LOCAL_YAML, OPTIONS): + with self.subTest(path=path.name): + self.assertIn("structured", path.read_text(encoding="utf-8").lower()) + self.assertIn("structured_review_result", SKILL.read_text(encoding="utf-8")) + self.assertIn("If, and only if", RUNBOOK.read_text(encoding="utf-8").split("13b.", 1)[1]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/reference/review/invocation_options.py b/tests/reference/review/invocation_options.py index ea64f7e6..5d330e87 100644 --- a/tests/reference/review/invocation_options.py +++ b/tests/reference/review/invocation_options.py @@ -17,6 +17,7 @@ "human_review_output": "human review output", "human_inline_findings": "human inline findings", "include_severity_description": "severity description", + "structured_review_result": "structured review result", } # Options with no fixed Skill default: after every other option is resolved, @@ -53,6 +54,10 @@ "show severity descriptions", "show blocking/non-blocking labels", ), + "structured_review_result": ( + "machine-readable review result", + "review result as json", + ), } OPTION_EXTRA_NEGATIVE: dict[str, tuple[str, ...]] = { "human_review_output": ( @@ -74,9 +79,19 @@ "don't include severity descriptions", "show only p0/p1/p2", ), + "structured_review_result": ( + "no machine-readable review result", + "human report only", + ), } +# The `structured review result` name is consumed by its own option so its +# `structured review` prefix is never read as human_review_output's negative +# phrase (invocation-options.md, "`structured_review_result` phrasings"). +_STRUCTURED_NAME = re.compile(r"(? str: """A whitespace-flexible, word-bounded matcher for a fixed phrase.""" return r"(? dict[str, bool]: result = dict(defaults) explicit: set[str] = set() for option in OPTION_CONCEPTS: - canonical = _canonical_values(text, option) + option_text = text if option == "structured_review_result" else _STRUCTURED_NAME.sub(" ", text) + canonical = _canonical_values(option_text, option) if False in canonical: result[option] = False explicit.add(option) @@ -140,7 +156,7 @@ def normalize(text: str, *, defaults: Mapping[str, bool]) -> dict[str, bool]: result[option] = True explicit.add(option) else: - natural = _natural_values(text, option) + natural = _natural_values(option_text, option) if len(natural) == 1: result[option] = natural.pop() explicit.add(option) diff --git a/tests/unit/review/test_invocation_options.py b/tests/unit/review/test_invocation_options.py index af2edf78..f6c7f51c 100644 --- a/tests/unit/review/test_invocation_options.py +++ b/tests/unit/review/test_invocation_options.py @@ -17,6 +17,7 @@ "human_inline_findings": False, # local-code-review has no severity legend; normalized for parity only "include_severity_description": False, + "structured_review_result": False, } GITHUB_DEFAULTS = { "include_fix_prompt": False, @@ -25,6 +26,7 @@ "human_review_output": False, "human_inline_findings": False, "include_severity_description": False, + "structured_review_result": False, } @@ -438,5 +440,65 @@ def test_local_defaults_normalize_for_parity_only(self) -> None: ) +class StructuredReviewResultOptionTests(unittest.TestCase): + def test_defaults_false(self) -> None: + self.assertFalse( + normalize("review this", defaults=LOCAL_DEFAULTS)["structured_review_result"] + ) + + def test_canonical_and_natural_forms_enable_it(self) -> None: + for text in ( + "structured_review_result=true", + "structured review result", + "include a structured review result", + "give me a machine-readable review result", + "emit the review result as JSON", + ): + with self.subTest(text=text): + self.assertTrue( + normalize(text, defaults=LOCAL_DEFAULTS)["structured_review_result"] + ) + + def test_negative_and_ambiguous_forms_stay_off(self) -> None: + for text in ( + "structured_review_result=false", + "no machine-readable review result", + "human report only", + "give me json", + "make it parseable", + "What does structured_review_result do?", + ): + with self.subTest(text=text): + self.assertFalse( + normalize(text, defaults=LOCAL_DEFAULTS)["structured_review_result"] + ) + + def test_conflict_falls_through_to_default(self) -> None: + text = "machine-readable review result but human report only" + self.assertFalse( + normalize(text, defaults=LOCAL_DEFAULTS)["structured_review_result"] + ) + + def test_name_is_not_read_as_human_output_negative_phrase(self) -> None: + result = normalize( + "review like a senior engineer and include a structured review result", + defaults=LOCAL_DEFAULTS, + ) + self.assertTrue(result["human_review_output"]) + self.assertTrue(result["structured_review_result"]) + self.assertFalse( + normalize("structured review", defaults={**LOCAL_DEFAULTS, "human_review_output": True})[ + "human_review_output" + ] + ) + + def test_it_does_not_change_any_other_option(self) -> None: + off = normalize("review this", defaults=LOCAL_DEFAULTS) + on = normalize("review this, structured_review_result=true", defaults=LOCAL_DEFAULTS) + self.assertTrue(on.pop("structured_review_result")) + off.pop("structured_review_result") + self.assertEqual(on, off) + + if __name__ == "__main__": unittest.main()