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
4 changes: 4 additions & 0 deletions docs/review-result/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ archive, and no packaged Skill resource depends on them.
[`../../shared/templates/finding.md`](../../shared/templates/finding.md),
[`../../shared/policies/severity.md`](../../shared/policies/severity.md),
[`../findings/README.md`](../findings/README.md).
- Cross-Skill contract tests and sample outputs
([#71](https://github.com/amirbena/code-review-skill/issues/71)):
[`../../tests/reference/review/structured_output_contract.py`](../../tests/reference/review/structured_output_contract.py),
described in [`review-result-model.md`](review-result-model.md) section 8.
- Test-only version-rule reference:
[`../../tests/reference/review/review_result_version.py`](../../tests/reference/review/review_result_version.py).
- The architecture map: [`../ARCHITECTURE.md`](../ARCHITECTURE.md).
21 changes: 20 additions & 1 deletion docs/review-result/review-result-model.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ schema failing.
| Versioning policy and compatibility rules for `schema_version` | [`schema-versioning.md`](schema-versioning.md) ([#68](https://github.com/amirbena/code-review-skill/issues/68)) |
| 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) |
| Cross-Skill contract tests (section 8) | [#71](https://github.com/amirbena/code-review-skill/issues/71) |
| Consumers of the result | None in this repository; consumer handling is the guidance in [`schema-versioning.md`](schema-versioning.md) section 3 |
| Parent capability | [#44](https://github.com/amirbena/code-review-skill/issues/44) |

`github-pr-review` emits the result on explicit request, returned to the
Expand All @@ -135,3 +136,21 @@ and identity minting owned by the shared
[`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.

## 8. Cross-Skill contract tests

[#71](https://github.com/amirbena/code-review-skill/issues/71) checks
sample outputs of both Skills against this record. The samples are
[`../../tests/unit/review/findings/structured_output_samples/`](../../tests/unit/review/findings/structured_output_samples/);
the checks are
[`../../tests/reference/review/structured_output_contract.py`](../../tests/reference/review/structured_output_contract.py).

| Check | Rule |
| --- | --- |
| Schema and version | Each result passes the section 5 validator. A missing or malformed `schema_version`, or a different `MAJOR`, fails closed ([`schema-versioning.md`](schema-versioning.md) section 3). A version newer than the published schema is also rejected. This is a producer rule: producers emit the version they were written for (section 4). A consumer would accept a newer `MINOR`. |
| Surface population | `skill` names the producer. `reviewed_state.reviewed_head_sha` equals the known workspace head, or the PR head. It is `null` only for an uncommitted local target or an incomplete PR review. |
| Shared-field parity | `skill` and `reviewed_state` are the only surface-specific fields. For the same review, every other field is identical across the two Skills. |
| Report ↔ result agreement | The rendered decision label maps to `decision.outcome` through the section 4 table. The rendered counts, coverage, reviewed head, finding set (severity and title), finding ids, and affected locations equal the result's. The comparator reads both surfaces and never re-derives the decision. |

The benchmark does not read the result. It invokes `local-code-review` with
the option off and scores only the Markdown report.
2 changes: 1 addition & 1 deletion tests/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ none of the four top-level kinds (`unit`, `policy`, `reference`,
| `unit/benchmark/` | Unit coverage for the `runtime_platform/benchmark/reference/` models: `test_benchmark_corpus.py` validates the `docs/benchmark/corpus/` fixtures (#51) through the `benchmark_fixture.py` reference validator; `test_benchmark_runner.py` drives the reference runner (#52) over the corpus and asserts per-case isolation, cleanup, and byte-for-byte preservation of a deliberately dirty source repository; `test_benchmark_report.py` drives the reference report (#53) and asserts a seeded regression is classified distinctly from an improvement and that output is byte-identical for identical inputs; `test_benchmark_match.py` drives the reference matcher (#54) and asserts every `match-criteria.md` §8 worked example classifies as documented; `test_benchmark_metrics.py` drives the reference metric (#55) and asserts every `missed-and-incorrect-findings.md` §8 worked example counts as documented; `test_benchmark_severity.py` drives the reference metric (#56) and asserts every `severity-accuracy.md` §7 worked example classifies as documented; `test_benchmark_dupes.py` drives the reference metric (#57) and asserts every `duplicate-noise.md` §7 worked example clusters as documented; `test_benchmark_citation.py` drives the reference citation-existence check (#349) and asserts every `citation-fidelity.md` §7 worked example is classified as documented. |
| `unit/governance/` | Repository automation and hygiene: issue-claim reconciliation, issue-label sync, PR-description length, and the repository-wide Markdown-link validator. |
| `unit/release/` | Coverage for `scripts/release/release_worthiness.py`, split along its path-classification, CHANGELOG coverage, Unreleased roll, main()/`$GITHUB_OUTPUT` contract, pure SemVer/ref helpers, and preflight/verify responsibilities (`_shared.py` holds the fake Git/GitHub runner and CHANGELOG fixtures they share), plus `test_changelog_generation.py` and `test_release_intent.py`. |
| `unit/review/` | Unit coverage for the `reference/review/` models. Sub-packages give the specific policy/capability each module protects a predictable home (#293): `findings/` (finding confidence/contract/identity, the machine-readable review result schema #67, plus `test_finding_identity_regression.py` — the data-driven finding-identity regression corpus, #61), `stateful_review/` (delta re-review, review-status enforcement, and `test_rereview_regression_fixtures.py` — the data-driven stateful delta re-review regression corpus of paired before/after review histories, #66), `specialist_depth/` (risk-based review depth corpus and scenarios, #90), `root_cause/` (root-cause consolidation, semantic-implication and null-absence corpora, `test_architectural_placement_fixtures.py` — paired local-only vs. bounded context-expansion outcomes for architecturally misplaced behavior, #153 — `test_candidate_finding_validation.py` — the observation → candidate claim → validated finding → severity reasoning-gate corpus, #382 — and `test_candidate_finding_validation_corpus.py` — the `docs/benchmark/corpus/candidate-finding-validation/` precision sub-corpus pinning that contract's false-escalation and non-suppression outcomes, plus a PR-#390-derived real-world scenario, #383), `repository_intelligence/` (repository expansion, instructions, and the repository-intelligence model plus its corpus), and `stacked_pr/` (stacked-PR topology and end-to-end checkout). Everything else — decision semantics, invocation options, PR/Jira context, parallel review, and other modules with no second clustering member — stays directly under `unit/review/`. |
| `unit/review/` | Unit coverage for the `reference/review/` models. Sub-packages give the specific policy/capability each module protects a predictable home (#293): `findings/` (finding confidence/contract/identity, the machine-readable review result schema #67, `test_structured_output_contract.py` — cross-Skill contract tests over the paired sample outputs in `structured_output_samples/`, #71, plus `test_finding_identity_regression.py` — the data-driven finding-identity regression corpus, #61), `stateful_review/` (delta re-review, review-status enforcement, and `test_rereview_regression_fixtures.py` — the data-driven stateful delta re-review regression corpus of paired before/after review histories, #66), `specialist_depth/` (risk-based review depth corpus and scenarios, #90), `root_cause/` (root-cause consolidation, semantic-implication and null-absence corpora, `test_architectural_placement_fixtures.py` — paired local-only vs. bounded context-expansion outcomes for architecturally misplaced behavior, #153 — `test_candidate_finding_validation.py` — the observation → candidate claim → validated finding → severity reasoning-gate corpus, #382 — and `test_candidate_finding_validation_corpus.py` — the `docs/benchmark/corpus/candidate-finding-validation/` precision sub-corpus pinning that contract's false-escalation and non-suppression outcomes, plus a PR-#390-derived real-world scenario, #383), `repository_intelligence/` (repository expansion, instructions, and the repository-intelligence model plus its corpus), and `stacked_pr/` (stacked-PR topology and end-to-end checkout). Everything else — decision semantics, invocation options, PR/Jira context, parallel review, and other modules with no second clustering member — stays directly under `unit/review/`. |
| `integration/github/` | Coverage that shells out to real Git for the GitHub PR checkout lifecycle. |
| `integration/packaging/` | End-to-end packaging-boundary guard: manifest/path-safety, script parity, hidden-runtime-dependency + disclaimer prose, and the built local/GitHub archive contents (`_shared.py` holds the shared paths, manifest helpers, and the reference-module list). `test_distribution_consumer_install.py` (#511) checks that the published distribution tree is discoverable and installable through the `skills` CLI with self-contained copies; its `npx` tests run only with `DISTRIBUTION_INSTALL_CHECK=1`. |
| `integration/release/` | Coverage that builds the packaged archives to exercise the release-worthiness PR boundary. |
Expand Down
1 change: 1 addition & 0 deletions tests/integration/packaging/_shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def _reference_module_path(name: str) -> Path:
"finding_confidence.py",
"review_telemetry.py",
"review_result.py",
"structured_output_contract.py",
"invocation_options.py",
"finding_identity.py",
"runtime_validation.py",
Expand Down
22 changes: 22 additions & 0 deletions tests/policy/review/findings/test_review_result_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,28 @@ def test_model_defers_versioning_and_emission(self) -> None:
self.assertRegex(text, rf"issues/{issue[1:]}\)")


class ContractTestRecordTests(unittest.TestCase):
def _boundaries(self) -> str:
return MODEL.read_text(encoding="utf-8").split("## 7. Boundaries", 1)[1].split("## 8.", 1)[0]

def test_consumers_are_not_attributed_to_the_contract_test_issue(self) -> None:
row = next(line for line in self._boundaries().splitlines() if line.startswith("| Consumers of the result"))
self.assertNotIn("issues/71", row)

def test_contract_tests_are_recorded_and_linked(self) -> None:
self.assertIn("| Cross-Skill contract tests (section 8) | [#71]", self._boundaries())
section = MODEL.read_text(encoding="utf-8").split("## 8. Cross-Skill contract tests", 1)[1]
for target in (
"../../tests/reference/review/structured_output_contract.py",
"../../tests/unit/review/findings/structured_output_samples/",
):
with self.subTest(target=target):
self.assertIn(f"]({target})", section)

def test_contract_module_is_registered_as_test_only(self) -> None:
self.assertIn("structured_output_contract.py", _shared.REFERENCE_TEST_MODULES)


class WiringAndPackagingTests(unittest.TestCase):
def test_architecture_map_links_the_record(self) -> None:
self.assertIn("review-result/README.md", ARCHITECTURE.read_text(encoding="utf-8"))
Expand Down
Loading
Loading