fix(pr-review): accept phase blocked by wrong layer names; finalize now publishes artifacts#81
Merged
Merged
Conversation
…ow publishes artifacts Two bugs in `canon pr-review`: 1. **accept always failed**: `validate_reviewer_output()` checked layer coverage against `CANONICAL_LAYERS` from `onion.rs` (the old 5-layer model: diff, whole_file, related_context, logical_stress, tests). The pr-review mode uses 7 layers (early-signal, application-source, high-risk-surfaces, related-context, logical-stress, tests, coverage-accounting), so `diff` and `whole_file` were always reported as missing. The filesystem-based check in `mode_pr_review_accept.rs` already validates all 7 layers correctly, so the redundant JSON-level check was removed. 2. **finalize did not publish artifacts**: `run_pr_review_finalize()` wrote outputs only to `.canon/runs/<RUN_ID>/pr-review/`. Added `publish_artifacts()` that copies the 8 packet artifacts to `.canon/artifacts/<RUN_ID>/pr-review/` on finalize, fulfilling the skill contract. Also bumps workspace version 0.72.5 → 0.72.6. Coverage: validate.rs 100%, mode_pr_review_finalize.rs 88.9% (+64 covered lines). Tests: 935 passed, 0 failed (+3 new).
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two bugs that made
canon pr-reviewunusable: theacceptphase waspermanently blocked by a layer-name mismatch, and
finalizenever publishedartifacts to the expected
.canon/artifacts/directory.Root Causes
1.
acceptblocked — wrong layer names in validationvalidate_reviewer_output()invalidate.rscalledcheck_layer_coverage()which validated
layer_statesagainstCANONICAL_LAYERS— the old 5-layermodel (
diff,whole_file,related_context,logical_stress,tests).The pr-review mode uses 7 layers (
early-signal,application-source,high-risk-surfaces,related-context,logical-stress,tests,coverage-accounting). Sincediffandwhole_filedon't exist in the newsystem,
acceptalways producedcanonical-review-output.jsonwithvalid: falseand the errors:The filesystem-based check in
mode_pr_review_accept.rsalready validates all7 layers correctly — the JSON-level check was redundant and actively harmful.
Fix: Removed
check_layer_coverage()fromvalidate_reviewer_output().The existing filesystem check in
mode_pr_review_accept.rs::check_layer_coverage()remains the authoritative layer coverage validator.
2.
finalizedidn't publish to.canon/artifacts/run_pr_review_finalize()wrote all review artifacts (summary, report,findings, manifest, etc.) to
.canon/runs/<RUN_ID>/pr-review/only.The skill contract states artifacts should be in
.canon/artifacts/<RUN_ID>/pr-review/, but nothing copied them there.Fix: Added
publish_artifacts()called at the end offinalize, whichcopies the 8 packet artifacts (
01-review-summary.md,06-review-report.md,review-findings.json,missing-tests.md,manifest.toml,canonical-review-output.json,packet-metadata.json,coverage-accounting.md) from the run directory to the artifacts directory.Changes
crates/canon-engine/src/review/validate.rscheck_layer_coverage(); renamed test; added schema version testcrates/canon-engine/src/orchestrator/service/mode_pr_review_finalize.rspublish_artifacts(); added 2 new tests; extended integration testCargo.toml+ 38 other filesCoverage
validate.rsmode_pr_review_finalize.rsVerification
cargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo fmt— cleancargo test --lib -p canon-engine— 935 passed, 0 failed