Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion capabilities/reviewer-assist/capability.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
capability: reviewer-assist
summary: >-
The private Reviewer Brief: what changed, stated focus, manual-review
focus, and open questions, composed from the finalized result only.
focus, and open questions, composed from the finalized result only;
plus the optional machine-readable review result, a projection of the
same finalized result returned to the caller.
loads: on-activation
activation:
- always, once findings, severity, coverage, and the verdict are final
- the caller explicitly requests a machine-readable (structured/JSON) result
adapters: [github]
files:
- skills/github-pr-review/policies/reviewer-brief.md
- skills/github-pr-review/policies/structured-output.md
- skills/github-pr-review/templates/reviewer-brief.md
requires: [review-kernel]
never:
Expand Down
6 changes: 4 additions & 2 deletions docs/review-result/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ 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.
`github-pr-review` can emit a review result on explicit request ([#70](https://github.com/amirbena/code-review-skill/issues/70),
[`structured-output.md`](../../skills/github-pr-review/policies/structured-output.md));
`local-code-review` emission is [#69](https://github.com/amirbena/code-review-skill/issues/69).

## Document map

Expand Down
14 changes: 9 additions & 5 deletions docs/review-result/review-result-model.md
Original file line number Diff line number Diff line change
Expand Up @@ -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").
`github-pr-review` emits one on explicit request; local emission is pending — see section 7.

## 1. Files

Expand Down Expand Up @@ -123,8 +123,12 @@ schema failing.
| 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
`github-pr-review` emits the result on explicit request, returned to the
caller only, with PR-specific field population owned by its
[`structured-output.md`](../../skills/github-pr-review/policies/structured-output.md)
([#70](https://github.com/amirbena/code-review-skill/issues/70)). Until
[#69](https://github.com/amirbena/code-review-skill/issues/69) lands,
`local-code-review` still produces only 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.
1 change: 1 addition & 0 deletions scripts/packaging/generate_package_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ class Entry(NamedTuple):
Entry("skills/github-pr-review/policies/finding-placement.md", "finding-placement-derivation"),
Entry("skills/github-pr-review/policies/review-output.md", "publication-github"),
Entry("skills/github-pr-review/policies/reviewer-brief.md", "reviewer-assist"),
Entry("skills/github-pr-review/policies/structured-output.md", "reviewer-assist"),
Entry("skills/github-pr-review/policies/review-status-enforcement.md", "publication-github"),
Entry("skills/github-pr-review/runbooks/passive-pr-review.md", None),
Entry("skills/github-pr-review/runbooks/active-pr-review.md", None),
Expand Down
1 change: 1 addition & 0 deletions scripts/packaging/package-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
{ "source": "skills/github-pr-review/policies/finding-placement.md", "destination": "policies/finding-placement.md" },
{ "source": "skills/github-pr-review/policies/review-output.md", "destination": "policies/review-output.md" },
{ "source": "skills/github-pr-review/policies/reviewer-brief.md", "destination": "policies/reviewer-brief.md" },
{ "source": "skills/github-pr-review/policies/structured-output.md", "destination": "policies/structured-output.md" },
{ "source": "skills/github-pr-review/policies/review-status-enforcement.md", "destination": "policies/review-status-enforcement.md" },
{ "source": "skills/github-pr-review/runbooks/passive-pr-review.md", "destination": "runbooks/passive-pr-review.md" },
{ "source": "skills/github-pr-review/runbooks/active-pr-review.md", "destination": "runbooks/active-pr-review.md" },
Expand Down
5 changes: 5 additions & 0 deletions skills/github-pr-review/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ Every result also carries a private, caller-only
your stated focus, and 2-4 areas worth a manual look — that is
structurally excluded from GitHub publication.

On explicit request it can also return a machine-readable review result
(same schema as local review; reviewed head SHA and decision populated) —
returned to you only, never published, and not the commit status. See
[`structured-output.md`](policies/structured-output.md).

## Boundaries worth knowing

| Rule | Short version |
Expand Down
5 changes: 5 additions & 0 deletions skills/github-pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,11 @@ If unavailable, do not fake success — fall back to passive review.
composed once findings/severity/coverage/verdict are finalized and
structurally excluded from anything published to GitHub — see
[`policies/reviewer-brief.md`](policies/reviewer-brief.md).
- **On explicit request**, additionally a machine-readable review result
in the same schema as `local-code-review` (PR head SHA and decision
populated), returned to the caller only — never published, and not the
commit status — see
[`policies/structured-output.md`](policies/structured-output.md).
- **Passive:** a human-readable report using the shared shape
([`review-summary.md`](../../shared/templates/review-summary.md)),
returned to the caller, not published.
Expand Down
1 change: 1 addition & 0 deletions skills/github-pr-review/metadata/skill.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ capabilities:
repository_access_mode: api-only|optional-repository-backed|required-repository-backed # required checkout failure is REVIEW INCOMPLETE; see ../policies/repository-checkout.md
parallel_review: optional # split review across workers per dimension when the runtime exposes a reliable capability; execution optimisation only; see ../../../shared/policies/parallel-review.md and ../policies/parallel-review.md
review_action_authorization: active-mode-request-is-its-own-authorization # single canonical publication mode (PASSIVE|SEMI|ACTIVE); an explicit ACTIVE request is itself sufficient authorization to publish, subject to self-review/reviewer-independence/GitHub-permission/HEAD; ambiguity fails closed; see ../policies/review-action-authorization.md
structured_review_output: optional # explicit-request only; one schema-valid machine-readable review result (same schema as local review; reviewed head SHA, base/merge-base, derived decision populated) returned to the caller in every mode; never published to GitHub and never the commit status; changes no finding/severity/verdict; see ../policies/structured-output.md
machine_readable_review_status: optional # one stable aggregated exact-HEAD GitHub status/check for the reviewed SHA, separate from native APPROVE/REQUEST_CHANGES; a blocking status is blocking-only enforcement (allowed for a self-review); a success status needs the same ACTIVE publication mode + reviewer independence as APPROVE and is never published by a self-review; a new HEAD inherits no green; see ../policies/review-status-enforcement.md
required_check_setup: explicit-opt-in-only # adding the status context to base-branch required checks is a separate explicitly-requested action, minimal and preserving (never touches approval-count / dismiss_stale_reviews_on_push / require_last_push_approval / bypass actors / unrelated rules); never performed during an ordinary review; see ../policies/review-status-enforcement.md

Expand Down
5 changes: 5 additions & 0 deletions skills/github-pr-review/policies/github-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ reviewer-brief.md private caller-facing Reviewer Brief; never
published; composed only from the finalized
analysis result above
↓
structured-output.md optional machine-readable review result (same
schema as local review); PR head SHA + decision
populated; caller-only, never published; not the
commit status below
↓
review-status-enforcement.md optional exact-HEAD machine-readable status;
blocking vs. positive authority; enforcement
detection; explicit opt-in required-check setup
Expand Down
126 changes: 126 additions & 0 deletions skills/github-pr-review/policies/structured-output.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
# Policy — Structured Output

Canonical semantics for the optional machine-readable review result
`github-pr-review` can return to its caller. Canonical index:
[`github-review.md`](github-review.md), which places this file after
[`reviewer-brief.md`](reviewer-brief.md): the document is a projection of
the same finalized analysis result and is composed only once findings,
severity, coverage, and the verdict are final.

The document is the same review-result document `local-code-review` emits:
identical field names, enums, and meaning. The shape is stated in
"Document shape" below so it is usable from the packaged Skill alone (this
repository's `docs/review-result/` holds the JSON Schema it mirrors; it is
not packaged). Beyond that shape, this policy states only what is specific
to a pull request — which values populate the PR-shaped fields — and
defines no finding, severity, or decision rule of its own.

## What it is not

- **Not a GitHub status or check.** A pass/fail commit status for the
reviewed SHA is a different mechanism, owned by
[`review-status-enforcement.md`](review-status-enforcement.md). Emitting
this document never publishes, alters, or substitutes for that status.
- **Not a review body or inline comment.** It is returned to the caller
only and is never an input to the publication payload built by
[`review-output.md`](review-output.md), "Batched review construction and
submission" — the same structural boundary as the Reviewer Brief.
- **Not a new analysis phase.** Composing it changes no finding, severity,
coverage signal, or verdict, and it runs in every publication mode
(PASSIVE, SEMI, ACTIVE), whether or not anything is published.

## Activation

Off by default. Emitted only when the caller explicitly asks for a
machine-readable, structured, or JSON review result (for example "also
return the review as JSON"). It is additive: the human-readable report,
the Reviewer Brief, and any publication are unchanged.

## Document shape

One JSON object. Keys are `snake_case`; required keys are always present;
an optional finding field with no value is omitted, never `null`.

```json
{
"schema_version": "1.0.0",
"skill": "github-pr-review",
"reviewed_state": {
"repository": "owner/name", "base_branch": "main",
"base_sha": "<sha|null>", "merge_base_sha": "<sha|null>",
"reviewed_head_sha": "<sha|null>", "reviewer_identity": "<string|null>",
"completeness": "full | delta-re-review", "prior_reviewed_sha": "<sha|null>"
},
"coverage": "complete | incomplete",
"decision": { "derived": "clean | blocking",
"outcome": "clean | blocking | incomplete" },
"counts": { "p0": 0, "p1": 0, "p2": 0 },
"summary": "<the review's What changed prose>",
"findings": [ { "id": "F1", "severity": "P0 | P1 | P2", "title": "",
"location": "", "fix_location_resolved": true, "evidence": "",
"impact": "", "fix": "",
"runtime_validation": "reasoned | runtime-confirmed | attempted-inconclusive",
"confidence": "confirmed | credible | runtime-validation-unavailable | external-contract-unvalidated | insufficient-context",
"identity": { "stable_id": "fid_v1_<hex>", "matching_eligible": true } } ]
}
```

Optional finding fields, each from [`finding.md`](../../../shared/templates/finding.md)
and omitted when empty: `evidence_location`, `affected_locations`,
`follow_up`, `details`, `contextual_evidence`, `capability`, `defect_kind`.
`fix_location_resolved` is `false` exactly when the finding carries the
"evidence location; fix/action location unresolved" annotation. `counts`
equals the tally of `findings` by severity; no other key is permitted.

## Field population

Shared fields (`schema_version`, `findings`, `counts`, `coverage`,
`summary`, and each finding's fields) are populated exactly as
`local-code-review` populates them, from the finalized findings and the
shared [`finding.md`](../../../shared/templates/finding.md) contract. The
PR-specific values are:

| Field | Value for a PR review |
| --- | --- |
| `skill` | `github-pr-review` |
| `reviewed_state.repository` | the PR's base repository as `owner/name` |
| `reviewed_state.base_branch` | the PR's base branch name (the effective review base for a stacked PR) |
| `reviewed_state.base_sha` | the base SHA at review time |
| `reviewed_state.merge_base_sha` | the merge base of base and head at review time |
| `reviewed_state.reviewed_head_sha` | the exact PR head SHA the review analyzed and revalidated ([`review-output.md`](review-output.md), "HEAD revalidation") — never the current tip if it moved afterward |
| `reviewed_state.reviewer_identity` | the authenticated review identity, or `null` when it could not be established |
| `reviewed_state.completeness` | `full`, or `delta-re-review` when [`reviewer-delta-review.md`](reviewer-delta-review.md) applied |
| `reviewed_state.prior_reviewed_sha` | the same-reviewer prior reviewed SHA a delta review superseded; `null` at a chain root |
| `decision.derived` | mechanically derived from the finding set: `blocking` when any unresolved P0/P1 remains, otherwise `clean` |
| `decision.outcome` | `derived`, except `incomplete` when coverage is incomplete |

If the head cannot be established the review is incomplete: emit
`coverage: incomplete`, `decision.outcome: incomplete`, and
`reviewed_head_sha: null` — never a graded outcome.

### Decision consistency

`decision.derived` and `decision.outcome` are computed from the same final
verdict the human output states, never separately:

| Machine value | Human rendering |
| --- | --- |
| `clean` | `Approve` (permitted or not — see below) |
| `blocking` | `Request Changes` |
| `incomplete` (outcome only) | `REVIEW INCOMPLETE` |

The document records the derived decision, not the GitHub event that was
submitted. A self-review, a PASSIVE run, or an unauthorized `APPROVE`
still yields `clean` when the findings are clean; the event actually
posted is reported by the human output, not encoded here. `clean` never
means "no findings" — P2 findings still appear in `findings`.

## Delivery

One JSON document in a fenced `json` block, in a distinct part of the
returned result appended after the caller-facing report, never merged into
GitHub-shaped content. The final human-facing summary remains the last
review-owned publication of an active run
([`review-output.md`](review-output.md), "Submission ordering"); this
document is returned to the caller, not published, so it does not change
that ordering.
5 changes: 5 additions & 0 deletions skills/github-pr-review/runbooks/active-pr-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,11 @@ stop
body, per
[`../policies/reviewer-brief.md`](../policies/reviewer-brief.md),
"Composition with invocation modes" — never its fields or boundaries.
**When the caller explicitly requested a machine-readable result**,
compose it here too, from the same finalized result, per
[`../policies/structured-output.md`](../policies/structured-output.md);
like the brief it joins the returned result only and is never passed
into steps 15-16.
14. **Apply the review-action authorization gate** per
[`../policies/review-action-authorization.md`](../policies/review-action-authorization.md)
and [`../policies/review-output.md`](../policies/review-output.md),
Expand Down
4 changes: 4 additions & 0 deletions skills/github-pr-review/runbooks/passive-pr-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ finally: remove the temporary checkout (success, any failure, interruption)
field, synthesis, and mode-composition rule in that policy (clean
review, delta re-review, stacked PR, partitioned large PR alike) so
passive and active results carry identical brief semantics.
**When the caller explicitly requested a machine-readable result**,
also compose it here, from the same finalized result, per
[`../policies/structured-output.md`](../policies/structured-output.md);
it is returned to the caller only.
8e. **Check verdict consistency** per
[`../../../shared/policies/verdict-consistency.md`](../../../shared/policies/verdict-consistency.md)
before returning the report composed above: confirm the decision
Expand Down
65 changes: 65 additions & 0 deletions tests/policy/review/findings/test_structured_output_docs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
#!/usr/bin/env python3
"""Documentation-contract checks for github-pr-review structured output (Issue #70).

Pins that the packaged policy populates every PR-shaped schema field, uses
the schema's own enum values, and stays off the publication path.

Run with:
python3 -m unittest tests.policy.review.findings.test_structured_output_docs
"""

from __future__ import annotations

import json
import unittest

from tests.support.paths import REPO_ROOT

POLICY = REPO_ROOT / "skills" / "github-pr-review" / "policies" / "structured-output.md"
OUTPUT_POLICY = REPO_ROOT / "skills" / "github-pr-review" / "policies" / "review-output.md"
SCHEMA = REPO_ROOT / "docs" / "review-result" / "review-result.schema.json"
EXAMPLE = REPO_ROOT / "docs" / "review-result" / "examples" / "review-result.example.json"


class StructuredOutputPolicyTests(unittest.TestCase):
def setUp(self) -> None:
self.text = POLICY.read_text(encoding="utf-8")
self.schema = json.loads(SCHEMA.read_text(encoding="utf-8"))

def test_every_reviewed_state_field_is_populated(self) -> None:
for key in self.schema["properties"]["reviewed_state"]["required"]:
self.assertIn(f"`reviewed_state.{key}`", self.text)

def test_decision_fields_and_skill_value_are_populated(self) -> None:
for token in ("`decision.derived`", "`decision.outcome`", "`skill`"):
self.assertIn(token, self.text)
self.assertIn("github-pr-review", self.schema["properties"]["skill"]["enum"])

def test_decision_values_match_schema_enums(self) -> None:
decision = self.schema["properties"]["decision"]["properties"]
for value in decision["outcome"]["enum"]:
self.assertIn(f"`{value}`", self.text)

def test_not_a_commit_status_and_not_published(self) -> None:
self.assertIn("review-status-enforcement.md", self.text)
self.assertIn("never an input to the publication payload", self.text)
self.assertNotIn("structured-output", OUTPUT_POLICY.read_text(encoding="utf-8"))

def test_inline_shape_matches_schema_and_example(self) -> None:
finding = self.schema["definitions"]["finding"]
for key in finding["required"] + list(self.schema["required"]):
self.assertIn(f'"{key}"', self.text)
for key in finding["properties"]:
self.assertIn(f"`{key}`" if key not in finding["required"] else f'"{key}"', self.text)
example = json.loads(EXAMPLE.read_text(encoding="utf-8"))
self.assertEqual(set(example["reviewed_state"]), set(self.schema["properties"]["reviewed_state"]["required"]))
self.assertEqual(example["skill"], "github-pr-review")

def test_runbooks_compose_structured_output(self) -> None:
for name in ("active-pr-review.md", "passive-pr-review.md"):
runbook = (POLICY.parents[1] / "runbooks" / name).read_text(encoding="utf-8")
self.assertIn("policies/structured-output.md", runbook)


if __name__ == "__main__":
unittest.main()
Loading