docs(pr-reviews): [#2219] unify PR review processing - #2232
josecelano merged 30 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2232 +/- ##
===========================================
- Coverage 85.49% 85.39% -0.11%
===========================================
Files 353 353
Lines 31315 31315
Branches 31315 31315
===========================================
- Hits 26773 26740 -33
- Misses 4168 4193 +25
- Partials 374 382 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Unifies PR review processing into a single workflow + audit format (covering Copilot and human reviews), adds deterministic “review-finding” references, and updates tooling/docs to match the new canonical locations and contracts.
Changes:
- Introduces unified PR review audit template +
docs/pr-reviews/archive docs; retires legacy templates/READMEs. - Adds
process-pr-reviewskill and updates prompts/agents/helpers + contract test assertions for the unified workflow. - Adds a nightly rustfmt parity step to
pre-commit.shand expands templates/specs to require toolchain-qualified validation evidence.
Reviewed changes
Copilot reviewed 110 out of 110 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/templates/README.md | Updates template index to point at the new unified PR review template. |
| docs/templates/PR-REVIEW-TEMPLATE.md | New unified audit template (fields, schema, findings table, references). |
| docs/templates/PR-REVIEW-FEEDBACK-TEMPLATE.md | Deletes legacy PR-review-feedback template in favor of unified audit template. |
| docs/templates/ISSUE.md | Adds guidance to record toolchain/runtime for validation commands. |
| docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md | Deletes legacy Copilot suggestions template in favor of unified audit template. |
| docs/skills/semantic-skill-link-convention.md | Adds review-finding: semantic marker + citation guidance. |
| docs/pr-reviews/README.md | Adds canonical README for unified review audit archive. |
| docs/pr-review-feedback/pr-2207-review-feedback.md | Updates skill-links/artifacts to unified workflow + adds migration note. |
| docs/pr-review-feedback/pr-2178-review-feedback.md | Updates skill-links/artifacts + fixes Copilot audit link + adds migration note. |
| docs/pr-review-feedback/pr-2177-review-feedback.md | Updates skill-links/artifacts + adds migration note. |
| docs/pr-review-feedback/pr-2174-review-feedback.md | Updates legacy Copilot audit reference + adds migration note. |
| docs/pr-review-feedback/README.md | Deletes legacy PR-review-feedback archive README. |
| docs/issues/open/2219-2003-unify-pr-review-processing/manual-verification-evidence.md | Adds manual verification evidence for the unified workflow and formatter parity. |
| docs/issues/open/2219-2003-unify-pr-review-processing/implementation-retrospective.md | Adds implementation retrospective for #2219. |
| docs/issues/open/2219-2003-unify-pr-review-processing/agent-review-reports.md | Adds append-only independent review reports for #2219. |
| docs/issues/open/2219-2003-unify-pr-review-processing/ISSUE.md | Expands contract/plan/evidence to reflect unified workflow, schema, and references. |
| docs/index.md | Replaces legacy Copilot/PR-feedback archive sections with unified PR review archive. |
| docs/copilot-pr-reviews/pr-2229-copilot-suggestions.md | Repoints skill-link metadata to unified workflow. |
| docs/copilot-pr-reviews/pr-2228-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2227-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2225-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2224-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2223-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2220-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2217-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2214-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2211-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2210-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2208-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2207-copilot-suggestions.md | Repoints skill-link marker + artifacts + adds migration note. |
| docs/copilot-pr-reviews/pr-2206-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2205-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2202-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2198-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2197-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2189-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2188-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2186-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2184-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2182-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2178-copilot-suggestions.md | Repoints template/artifacts + adds migration note. |
| docs/copilot-pr-reviews/pr-2177-copilot-suggestions.md | Repoints skill-link marker + adds migration note. |
| docs/copilot-pr-reviews/pr-2176-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2174-copilot-suggestions.md | Repoints skill-link marker + adds migration note. |
| docs/copilot-pr-reviews/pr-2173-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2171-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2170-copilot-suggestions.md | Repoints skill-link metadata to unified workflow. |
| docs/copilot-pr-reviews/pr-2168-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2166-copilot-suggestions.md | Repoints skill-link metadata to unified workflow. |
| docs/copilot-pr-reviews/pr-2164-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2163-copilot-suggestions.md | Repoints skill-link metadata to unified workflow. |
| docs/copilot-pr-reviews/pr-2161-copilot-suggestions.md | Repoints skill-link metadata to unified workflow. |
| docs/copilot-pr-reviews/pr-2153-copilot-suggestions.md | Repoints skill-link metadata + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2152-copilot-suggestions.md | Repoints skill-link marker to unified workflow. |
| docs/copilot-pr-reviews/pr-2147-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2144-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2141-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2139-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2137-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2135-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2133-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2131-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2128-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2126-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2124-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2123-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2119-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2118-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2110-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2108-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2102-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2099-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2098-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2097-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2094-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2093-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2090-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2087-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2085-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2084-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2061-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2037-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2032-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2027-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2025-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-2024-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2021-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2020-copilot-suggestions.md | Updates referenced paths + notes to unified workflow naming. |
| docs/copilot-pr-reviews/pr-2017-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2013-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-2008-copilot-suggestions.md | Updates referenced paths in rows + skill-link marker to unified workflow. |
| docs/copilot-pr-reviews/pr-2007-copilot-suggestions.md | Updates referenced paths in rows to unified archive paths. |
| docs/copilot-pr-reviews/pr-1993-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-1991-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/pr-1967-copilot-suggestions.md | Repoints skill-link marker + artifacts to unified workflow. |
| docs/copilot-pr-reviews/README.md | Deletes legacy Copilot audit archive README. |
| docs/copilot-pr-reviews/EXAMPLE-COMPLETED.md | Deletes legacy Copilot audit example file. |
| docs/agents/orchestration.md | Updates orchestration diagrams/ownership to route Copilot handling through unified audit. |
| contrib/dev-tools/git/hooks/pre-commit.sh | Adds nightly rustfmt parity step to pre-commit gate. |
| contrib/dev-tools/checks/tests/test-agent-review-report-contract.sh | Extends contract test to enforce unified workflow + YAML/frontmatter invariants. |
| .github/skills/dev/testing/write-unit-test/SKILL.md | Adds toolchain-qualified evidence guidance for test/verification planning. |
| .github/skills/dev/pr-reviews/resolve-review-threads/SKILL.md | Updates helper skill to reference unified workflow as parent. |
| .github/skills/dev/pr-reviews/process-pr-review/SKILL.md | Adds new unified PR review processing skill. |
| .github/skills/dev/pr-reviews/process-pr-review-feedback/SKILL.md | Converts legacy skill into a one-release compatibility redirect. |
| .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md | Converts legacy skill into a one-release compatibility redirect. |
| .github/skills/dev/pr-reviews/fetch-review-threads/SKILL.md | Updates helper skill to reference unified workflow as parent. |
| .github/skills/dev/planning/create-issue/SKILL.md | Requires toolchain-qualified evidence for recorded command results. |
| .github/prompts/process-copilot-suggestions.prompt.md | Updates prompt to delegate to unified workflow and audit location. |
| .github/agents/copilot-suggestions-handler.agent.md | Updates agent entry point to delegate to unified workflow and unified audit file. |
| .github/PULL_REQUEST_TEMPLATE/review-findings.md | Adds advisory “review findings” guidance template for reviewers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at 54ae5faaf39d29158489b1285426c4e3436a0665. Recomputed from the bytes at this head.
Base a8cd44fc11137b71dd67d53b8b113ccd96fbf9bf; git merge-tree --write-tree returns tree 673af02edd80e63c604ed271971b5b2ddccf0064 with exit 0, so the merge is clean (claim scoped to that base commit). 111 files, +1709/-1065, 17 commits.
This is a well-evidenced change and most of it holds up under recomputation: the migration preserves every review record, the three Copilot threads were right and were genuinely fixed rather than merely resolved, and the manual-verification evidence reproduces exactly. Three defects block merge, all of the kind that would land a wrong process rather than a wrong document.
Verification performed
| Check | Result |
|---|---|
pre-commit.sh --format=json at head (TORRUST_GIT_HOOKS_LOG_DIR=.tmp) |
pass, exit_code=0, 8/8 steps, 69.61 s |
V1 false-green reproduction at defe8466 |
reproduced exactly — cargo +nightly fmt --all -- --check exits 1 naming packages/udp-server/src/handlers/mod.rs:268, .../server/request_buffer.rs:141, .../statistics/event/handler/error.rs:116 |
| V3 / V4 evidence snippets | reproduced byte-identically (rc=0 each) |
V2 evidence (PR #2174 review 5155990517; threads PRRT_kwDOGp2yqc6gtUXI, PRRT_kwDOGp2yqc6gtUXL) |
verified — the review exists, both threads are isResolved and isOutdated, and each carries a source comment plus an author reply, as the evidence states |
| Rename purity (82 renames) | header/link rewrites only, except the 9 files in Blocker 1 |
| Commit hygiene | 17 Conventional Commit subjects; no Co-Authored-By or AI-attribution trailer; no banned token in the diff |
The nightly-formatter step added to the hook is properly justified: V1 demonstrates a real historical false green, and I reproduced it independently.
Blockers
1. 21 rewritten path references name a file that has never existed
The migration rewrote the directory of self-referential paths inside 9 historical audit records but not the basename, producing references to files that exist nowhere in the tree.
-| 3 | `PRRT_kwDOGp2yqc6gYMxO` | `docs/copilot-pr-reviews/pr-2176-copilot-suggestions.md` | ...
+| 3 | `PRRT_kwDOGp2yqc6gYMxO` | `docs/pr-reviews/pr-2176-copilot-suggestions.md` | ...
The migrated file is docs/pr-reviews/pr-2176-review.md. The only copilot-suggestions files at this head are the four -copilot-suggestions-legacy.md records, so docs/pr-reviews/pr-<N>-copilot-suggestions.md is a path that existed neither before nor after the migration.
21 occurrences across 9 files:
$ git grep -c -E 'docs/pr-reviews/pr-[0-9]+-copilot-suggestions\.md' 54ae5faa -- docs
docs/pr-reviews/pr-2007-review.md:1
docs/pr-reviews/pr-2008-review.md:4
docs/pr-reviews/pr-2013-review.md:3
docs/pr-reviews/pr-2017-review.md:2
docs/pr-reviews/pr-2020-review.md:2
docs/pr-reviews/pr-2021-review.md:1
docs/pr-reviews/pr-2024-review.md:2
docs/pr-reviews/pr-2027-review.md:4
docs/pr-reviews/pr-2176-review.md:2The same rewrite also restates history: action: added ``<!-- skill-link: process-copilot-suggestions -->`` became action: added ``<!-- skill-link: process-pr-review -->`` , which describes a marker that did not exist when that action was taken. That conflicts with this PR's own rule in process-pr-review/SKILL.md step 4 — "Do not backfill or reinterpret historical audit records."
Either restore the original docs/copilot-pr-reviews/... values as historical source metadata (my preference — the Path column records what the thread was filed against at the time), or repoint them at the real pr-<N>-review.md files. The narrative cells should be restored verbatim either way. Note the -legacy collision cases were handled correctly, so this is a gap in the plain-rename path only.
2. docs/AGENTS.md still routes agents to both deleted directories
docs/AGENTS.md is not in this PR's 111 files, and at this head it still documents the directory map as:
| `copilot-pr-reviews/` | Copilot PR review records and suggestion threads |
| `pr-review-feedback/` | PR review feedback audit records |
Both directories are deleted by this PR, and the replacement pr-reviews/ is absent from the map. This is the canonical agent-facing docs index, so after merge it points every agent at two paths that no longer exist. docs/index.md and docs/templates/README.md were both updated correctly — docs/AGENTS.md was missed.
This is also the exact failure mode the retrospective predicts: "Markdown link checking does not validate YAML frontmatter artifact paths… the migration therefore required a structural review". These are code-span paths, not links, so nothing in the gate catches them; the structural review missed this file.
3. The batch-resolve reply guard is dropped, while the batch tool stays documented
At base, the safeguard that no thread is bulk-resolved without a reply was documented in two live places, both deleted here:
.github/agents/copilot-suggestions-handler.agent.md:119— "has a reply. Runcheck-thread-reply-status.shfirst to confirm."process-copilot-suggestions/SKILL.md:185— "first confirm every thread already has a user reply… This script exits with code 1 if any thread lacks a reply. Only proceed with the batch resolver once it exits 0."
At this head, git grep check-thread-reply-status matches only the script's own usage text — no skill, agent, or prompt references it. Meanwhile resolve-review-threads/SKILL.md still documents the "Batch Pattern" with resolve-all-unresolved-threads.sh and no reply precondition, and the unified skill never mentions the batch path at all.
The net effect on the merged process: the bulk-resolve tool remains reachable and documented, but the only documented precondition protecting Rule 1 — "Always reply before resolving", which the unified skill otherwise preserves faithfully in step 7 — is gone. Please carry the guard into process-pr-review/SKILL.md or into the resolve-review-threads batch section.
Major
4. The new contract test takes an undeclared PyYAML dependency and misreports its absence
require_yaml_related_artifact shells out to python3 with import yaml. On the project's build server the head test fails at the head tree:
$ bash contrib/dev-tools/checks/tests/test-agent-review-report-contract.sh # at 54ae5faa
ModuleNotFoundError: No module named 'yaml'
Expected .../process-pr-review/SKILL.md YAML frontmatter to contain related artifact: docs/issues/open/2219-2003-unify-pr-review-processing/ISSUE.md
rc=1Two separate problems:
- Undeclared dependency. The base version of this test used only
sed/grep/awk.import yamlappears nowhere else in the repository and there is no requirements file, so this PR introduces a third-party Python requirement into a previously dependency-free bash test.ISSUE.md:453justifies it as "the already-installed PyYAML dependency", which does not hold on the build server. - Misleading failure mode. Because the helper tests only
python3's exit status, a missing interpreter module is reported as a content violation. The message accuses the skill of missing a semantic link that is in fact present — I confirmed all fiverelated-artifactsentries are there, so the assertion itself is sound and would pass with PyYAML available.
Mitigating: this test is not wired into the pre-commit gate or any workflow in .github/workflows/, so nothing turns red in CI. That also means the breakage is silent for anyone running it by hand. A dependency check that fails with a clear message, or a sed-based parse consistent with the sibling helpers, would fix both halves.
Suggestions (non-blocking)
.github/PULL_REQUEST_TEMPLATE/review-findings.mdis reviewer guidance in an author-facing directory. This PR creates that directory (it did not exist at base, and there is no defaultpull_request_template.md). Its contents are instructions for reviewers, but everything in that directory is offered to PR authors at creation time — an audience that can never act on it.docs/skills/ordocs/templates/would match the audience; the skill's advisory link would just repoint.related-artifactsquietly widens its value type. The convention table still types it<repo-relative-path>, while the new prose allowsreview-finding:pr-<N>-<id>values — anddocs/pr-reviews/pr-2232-review.mdalready uses three. Worth updating that table row so the type and the practice agree.
Nits (non-blocking)
docs/templates/README.md: the consolidatedPR-REVIEW-TEMPLATE.mdrow sits betweenGITHUB-WORKFLOW-ACTIONS-UPDATE-PR.mdandEPIC.md, breaking the table's alphabetical order. The misplacement is inherited from the row it replaced, but the properly-placedPR-REVIEW-FEEDBACK-TEMPLATE.mdrow was the one removed.ISSUE.mdfrontmatter carrieslast-updated-utc: 2026-09-15T17:50:00Zwhile its own progress log records entries at 19:05 and 19:10 UTC.
On the two open frictions
- F48 (no shape for re-reviewing a re-pushed head): partially addressed, author side only. The re-raise vocabulary is new and useful — step 3: "A later item requesting the same current-tree change is a re-raise: keep its source row and record
RE_RAISE_OF:<FindingId>", and the findings template's "Use the original<FindingId>when re-raising an earlier finding, and state that it is a re-raise in the body." What is still missing is the reviewer-side shape:review-pr/SKILL.mdis unchanged by this PR, so nothing says how to scope round N+1 against a re-pushed head. - F49 (no N/A convention in the review checklist): not addressed. The checklist lives in
review-pr/SKILL.md, which this PR does not touch (git diffover that path is empty).
Worth flagging explicitly: this PR changes the reviewer's own contract (the advisory finding format and severity vocabulary) without touching review-pr/SKILL.md, so the reviewer-side skill and the author-side skill now describe review findings in different terms.
Copilot threads
All three were correct on the merits and are genuinely fixed in this tree, not merely resolved:
- PR-template frontmatter — correct;
review-findings.mdnow begins<!-- cspell:disable -->with no YAML header. Fixed by441f193c. - Formatter step ordering — correct; the hook's
STEPSarray now runs nightly formatting as step 5 andlinter allas step 6, which my gate run confirms in its JSON output. Fixed by2d555384. - Flow-style YAML sequence — correct;
related-artifactsis now a block list with no trailing comma. Fixed by84e50416.
docs/pr-reviews/pr-2232-review.md is accurate. Every identifier in it checks out against the GitHub state: source review PRR_kwDOGp2yqc8AAAABNsfloA, source comments r4018776876 / r4018776945 / r4018776990 on the paths it names, and reply URLs r4019117053 / r4019117216 / r4019117462. The three cited resolution commit subjects all exist in the branch.
CI
At this head, 15 of 17 check runs are green as of 19:35 UTC — including Unit on both toolchains, all six build matrix jobs, Docs Lint, Documented Clippy Allows, Layer Boundary Bans, and both Codecov reports. Docker E2E is skipped and Test (Docker) (release) is still running. Nothing is red. CI does not bear on the four findings above: none of them is reachable by a gate, which is the substance of Finding 4.
|
@da2ce7 Thank you for the recomputed evidence — every finding checked out against the current tree. All are processed through the unified audit (docs/pr-reviews/pr-2232-review.md) as F4-F12 from review
Notes on the blockers:
Validation at the new head: focused contract test, ShellCheck, |
da2ce7
left a comment
da2ce7
left a comment
There was a problem hiding this comment.
Reviewed at a9886e3520f047814116e4c2e44c40174b83905f. Recomputed from the bytes.
The head moved twice during this round, so here is exactly what was measured where. Every finding below was recomputed at 0ed2c2c2 — already one commit past the c8170f8f your disposition comment describes. While that recomputation ran you pushed a9886e35 (docs(pr-reviews): document first-use lessons and follow-up), which is documentation only: +37/−2 across implementation-retrospective.md and pr-2232-review.md. I verified that delta line by line rather than re-running the round, because it touches none of the files the findings rest on — the nine restored audit records, docs/AGENTS.md, resolve-review-threads/SKILL.md and the contract test are all untouched by it. So every result below holds at this head. The one new item it introduces is N6.
Base a8cd44fc11137b71dd67d53b8b113ccd96fbf9bf is the merge base (git merge-base returns the base itself), and git merge-tree --write-tree returns tree 74119ba0ab1f1d9e02d1ae4a31194550c7ca00a4 with exit 0, so the merge is clean at that base. 112 files, +1851/-1047, 24 commits.
All three Blockers and the Major are genuinely gone from the tree. I tested each claim against the bytes rather than reading the table: the F4 claim is exactly true under both readings, the restored require_yaml_related_artifact still fails on four independent mutations that delete or corrupt what it asserts, and the full eight-step gate passes at this head. Two Minor and three Nit items remain, all of them fallout from the two smallest repairs, none blocking.
Round-1 findings, verified at this head
| r1 finding | Your row | Verified | What established it |
|---|---|---|---|
| Blocker 1 — 21 rewritten paths to a never-existing file | F4 FIXED | FIXED | Nine per-file diffs against the pre-migration original: +3/-3, three one-for-one hunks each; git grep -E returns nothing; Path and action: cells restored verbatim |
Blocker 2 — docs/AGENTS.md routes to deleted directories |
F5 FIXED | FIXED | git diff on docs/AGENTS.md: both rows removed, pr-reviews/ row added, rest of file unchanged |
| Blocker 3 — batch-resolve reply guard dropped | F6 FIXED | FIXED | Guard restored at resolve-review-threads/SKILL.md:10,65,70,107, stated as a requirement, reachable from process-pr-review step 7 |
| Major 4 — undeclared PyYAML dependency | F7 FIXED | FIXED | Run A passes on a host with no PyYAML; four end-to-end mutations of the asserted frontmatter all rejected |
| Suggestion — reviewer guidance in an author-facing directory | F8 FIXED | FIXED (with fallout, N1/N2 below) | .github/PULL_REQUEST_TEMPLATE/ absent at head; docs/templates/REVIEW-FINDINGS.md present and linked from the skill |
Suggestion — related-artifacts value type |
F9 FIXED | FIXED | Convention table row widened; new review-finding marker row and section; the three review-finding: values already in use now type-check |
| Nit — templates README ordering | F10 FIXED | FIXED | This PR's own rows are now correctly placed (N4 below is about the recorded verification wording, not the fix) |
Nit — stale last-updated-utc |
F11 FIXED | FIXED | last-updated-utc: 2026-09-15T20:13:00Z equals the newest processing-log entry |
| Reviewer-side vocabulary divergence (with F48/F49) | F12 FOLLOW_UP | carried forward — deferral accepted on the condition below | git diff over review-pr/SKILL.md across the PR is empty, as the audit states |
Blocker 1 — exactly as claimed, under both readings
Pairing established by rename detection rather than basename, git diff -M --find-renames: all nine detected sources are the expected docs/copilot-pr-reviews/pr-<N>-copilot-suggestions.md, similarity R096-R098. Each restored file diffed against that original:
file add del pairs hunks
pr-2007 3 3 3/3 3
pr-2008 3 3 3/3 3
pr-2013 3 3 3/3 3
pr-2017 3 3 3/3 3
pr-2020 3 3 3/3 3
pr-2021 3 3 3/3 3
pr-2024 3 3 3/3 3
pr-2027 3 3 3/3 3
pr-2176 3 3 3/3 3
pairs counts hunks whose old and new side are both exactly one line. Three of three in every file, so the claim holds under the strict line-pair reading and under the raw +3/-3 reading alike. The three changed lines are the frontmatter skill name, the frontmatter skill path, and the document's own marker comment — nothing else:
@@ -4 +4 @@ semantic-links:
- - process-copilot-suggestions
+ - process-pr-review
@@ -6 +6 @@ semantic-links:
- - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md
+ - .github/skills/dev/pr-reviews/process-pr-review/SKILL.md
@@ -10 +10 @@ semantic-links:
-<!-- skill-link: process-copilot-suggestions -->
+<!-- skill-link: process-pr-review -->
Because the diff is only those three lines, every Path cell and every narrative cell is byte-identical to the pre-migration record. Spot-checked directly on the restatement r1 named:
$ git grep -n -E 'action: added .*skill-link' 0ed2c2c2 -- docs/pr-reviews
0ed2c2c2:docs/pr-reviews/pr-2008-review.md:61:| 9 | PRRT_kwDOGp2yqc6SfBQ1 | `docs/copilot-pr-reviews/pr-2008-copilot-suggestions.md` | ... | action: added `<!-- skill-link: process-copilot-suggestions -->` for the governing review workflow. | DONE | RESOLVED |Both halves restored: the Path cell again records what the thread was filed against, and the completed action again names the marker that existed when the action was taken.
$ git grep -E 'docs/pr-reviews/pr-[0-9]+-copilot-suggestions\.md' 0ed2c2c2
(no output)Blocker 3 — the guard is a requirement, and the batch path reaches it
resolve-review-threads/SKILL.md now opens its Batch Pattern with the precondition, ahead of the batch resolver invocation, and closes with a matching checklist item:
Before any batch resolution, confirm every targeted thread already has a reply. Run the
`fetch-review-threads` helper script `check-thread-reply-status.sh` first; it exits with code 1
when any thread lacks a reply. Only proceed with the batch resolver once it exits 0. This
preserves the workflow rule that every resolvable thread is replied to before it is resolved.
That is a requirement, not a suggestion, and it matches the force of the base wording. On reachability: process-pr-review step 7 routes resolution through resolve-review-threads, so an agent following the unified skill arrives at the Batch Pattern section with the guard in it before the resolver snippet. Both referenced scripts exist at head. The concern r1 raised — a documented bulk tool whose only documented precondition had vanished — is closed.
Major 4 — the awk parse still fails when it should
Run A now passes on a host with no PyYAML, which is the repair landing rather than an environment change (python3 -c 'import yaml' still reports ModuleNotFoundError on that host, recorded in the same run as the matrix below).
The part that matters is whether the check still catches its own violation. I ran the full head script against four mutated copies of the tree, each built outside both worktrees with the real docs/ symlinked in, so only the asserted frontmatter differs:
MUTANT M0_control rc=0 want=0 OK wall=0.35 s
output: All agent review report contract tests passed.
MUTANT M1_delete_entry rc=1 want=1 OK wall=0.19 s
output: Expected .../process-pr-review/SKILL.md YAML frontmatter to contain related artifact: docs/issues/open/2219-2003-unify-pr-review-processing/ISSUE.md
MUTANT M2_near_miss rc=1 want=1 OK wall=0.19 s
MUTANT M3_flow_style rc=1 want=1 OK wall=0.22 s
MUTANT M4_unclosed_frontmatter rc=1 want=1 OK wall=0.21 s
M1 deletes the asserted entry; M2 appends .bak to it; M3 re-expresses the same five values in flow style; M4 removes the frontmatter's closing ---. All four are rejected. Unit-level cases on the extracted helper agree: a longer path containing the expected value as a substring does not match (value == artifact is an exact compare), a value under a sibling list key does not match, and a value after the closing --- does not match.
One observation for the record, not a defect in this test: read in isolation, the helper scans past the frontmatter when the frontmatter is never closed, so a body-level related-artifacts list can satisfy it. That is unreachable here because require_yaml_frontmatter runs first on the same file under set -e — which is exactly what M4 demonstrates. It only matters if the helper is reused without that guard.
Contract-test matrix, same host and same environment as round 1
ENV python3: Python 3.12.3
ENV import yaml: ModuleNotFoundError: No module named 'yaml'
--- RUN A: head script @ head tree ---
All agent review report contract tests passed.
STEP run_a_head_script_head_tree rc=0 wall=0.39 s
--- RUN B: head script @ base tree ---
Expected .../.github/agents/copilot-suggestions-handler.agent.md to contain: docs/pr-reviews/pr-<PR_NUMBER>-review.md
STEP run_b_head_script_base_tree rc=1 wall=0.11 s
--- RUN C: base script @ base tree (control) ---
All agent review report contract tests passed.
STEP run_c_base_script_base_tree rc=0 wall=0.10 s
Round 1 measured A rc=1 (PyYAML absent), B rc=1, C rc=0 on this same host with PyYAML still absent. A has moved to rc=0 because the dependency was genuinely dropped, and B is still non-zero, so the check has not stopped discriminating.
Gate at this head
gate_head=0ed2c2c264d6ff5467b3f913b8a18ef53138ec09
STEP pre_commit_gate rc=0 wall=39.06 s
Eight of eight steps pass, one run, nightly toolchain (rustc 1.100.0-nightly).
New in this round — non-blocking
N1 (Minor) — the relocated template is not on the canonical-template contract. a897f83c moved the file into docs/templates/ but touched only four paths, so two obligations that attach to that directory were missed. The repository's own create-markdown-template skill states them:
7. For a canonical repository template in `docs/templates/`, add the template to the catalog in
`docs/index.md` ...
and its Validate list requires that "a canonical repository template appears in docs/templates/, docs/templates/README.md, and docs/index.md". REVIEW-FINDINGS.md appears in the first two only; the docs/index.md template catalog at lines 147-158 lists thirteen templates and not this one. Separately, step 3 requires frontmatter, and REVIEW-FINDINGS.md is the only one of the fourteen files in docs/templates/ without it. The reason it had none was sound at its old path — GitHub rendered it verbatim in a PR body, which is Copilot finding F1 — but that reason left with the move, and CARGO-DEPENDENCY-UPDATE-PR.md is the in-repo precedent for a docs/templates/ file bound for a GitHub surface that still carries frontmatter. No gate catches either half.
N2 (Minor) — the issue spec still places the reviewer guidance under .github/. Three references survive in docs/issues/open/2219-2003-unify-pr-review-processing/ISSUE.md:
$ git grep -n -F 'PULL_REQUEST_TEMPLATE/review-findings.md' 0ed2c2c2 -- docs/issues/open/2219-2003-unify-pr-review-processing/ISSUE.md
...ISSUE.md:321:`.github/PULL_REQUEST_TEMPLATE/review-findings.md` and links the advisory finding guidance in the
...ISSUE.md:359:| T5 | DONE | Publish requested reviewer finding format | Created `.github/PULL_REQUEST_TEMPLATE/review-findings.md` and ...
...ISSUE.md:560:| AC5 | DONE | `.github/PULL_REQUEST_TEMPLATE/review-findings.md`; `manual-verification-evidence.md` section V3 |Line 359 is a completed-task record and is fine as history. The other two are not: line 321 is the spec's present-tense statement of the delivered design, and line 560 is the AC5 evidence cell, which cites a path absent from the tree. AC5's own wording — "documented, linked from the PR template, and explicitly advisory" — no longer holds either, since there is no PR template and the link now comes from the skill. The spec's closing checklist asserts that acceptance criteria "reflect actual behavior", so this is the spec disagreeing with itself. I weighed this against r1's Blocker 1, which is the same class of defect; it stays Minor because it misstates one completed verification in one open spec rather than routing agents or rewriting nine permanent audit records.
N3 (Nit) — the F4 verification command recorded in the audit does not discriminate. pr-2232-review.md records the check as git grep -c 'docs/pr-reviews/pr-[0-9]+-copilot-suggestions\.md'. Without -E that is a basic regular expression in which + is literal, so it returns nothing at the broken head too:
$ git grep -c 'docs/pr-reviews/pr-[0-9]+-copilot-suggestions\.md' 54ae5faa
(no output)
$ git grep -c -E 'docs/pr-reviews/pr-[0-9]+-copilot-suggestions\.md' 54ae5faa
54ae5faa:docs/pr-reviews/pr-2007-review.md:1
54ae5faa:docs/pr-reviews/pr-2008-review.md:4
...The claim is true — I proved it with the -E form and with the nine per-file diffs — but the command as written would have passed before the fix, so it is not evidence. Adding -E restores it.
N4 (Nit) — the F10 verification overstates. The audit records "table rows read in alphabetical order". GITHUB-WORKFLOW-ACTIONS-UPDATE-PR.md still precedes EPIC.md. That inversion is pre-existing at the base and this PR did not touch that row, so the nit r1 raised is genuinely fixed; only the recorded claim is broader than the tree.
N5 (Nit) — two docs/AGENTS.md rows lost their column alignment. Every row of that directory map is 199 characters at the base; at this head the refactor-plans/ and pr-reviews/ rows are 200. The refactor-plans/ row is touched by the diff with no semantic change at all.
The F12 deferral
The deferral is reasonable, on one condition. The divergence is real and I re-confirmed it: this PR changes the advisory finding format and the severity vocabulary while git diff over review-pr/SKILL.md is empty, so after merge a reviewer following the reviewer-side skill and an author following process-pr-review describe the same finding in different terms. That is tolerable for one release because the new format is explicitly advisory — REVIEW-FINDINGS.md states that a review is never rejected for omitting it and that authors process free-form feedback with the same rigor — so the author side degrades gracefully rather than breaking. The condition is that the follow-up be a real tracked task rather than a line in this PR's audit: at this head the only record of it is the F12 row and the processing log, and the audit is about this pull request, not about reviewer-side work that outlives it. F48 and F49 have now been carried across two rounds on that basis.
N6 (Nit) — the newest processing-log entry is dated after the commit that wrote it
a9886e35 appends an entry stamped 2026-09-15 20:40 UTC. Two measurements put it early: the commit introducing it is authored 2026-09-15T20:28:55Z, eleven minutes before, and the action it records — "Re-requested Cameron's review on the remediated head" — happened at 2026-09-15T20:22:42Z per the pull request timeline, seventeen minutes before. Every other entry in that log checks out, including 19:36 UTC - Cameron (da2ce7) submitted changes-requested review, which matches review 5214932721 to the second. Non-blocking, and the same family as the last-updated-utc nit from round 1: this file ships as the permanent record of the round, so its timestamps are the part readers will rely on.
CI
At a9886e35, 15 of 17 check runs are green as of 20:46 UTC — both Unit jobs, all six build matrix jobs, Docs Lint, Documented Clippy Allows, Layer Boundary Bans, Generate Coverage Report, copilot-setup-steps, and both Codecov reports. Docker E2E is skipped and Test (Docker) (release) is still running. Nothing is red. The eight-step gate above is the substantive evidence; as in round 1, none of the findings here is reachable by a gate, which was the point of Major 4.
Audit record
docs/pr-reviews/pr-2232-review.md is accurate on identity. The cited review node PRR_kwDOGp2yqc8AAAABNtWO8Q is the node ID of review 5214932721, submitted CHANGES_REQUESTED by da2ce7 at 19:36:02Z; that review carries no inline comments, as the processing log states; the consolidated response URL #issuecomment-5687518457 is the disposition comment; and PRR_kwDOGp2yqc8AAAABNsfloA on F1-F3 is the Copilot review. The F4-F12 rows match what round 1 actually raised, including severities. N3 and N4 above are the two recorded verification claims that do not reproduce as written.
Hygiene is clean: 24 commits, all Conventional Commit subjects, one author and committer throughout, no attribution trailer, and the token that chat transports mangle appears nowhere in the diff.
|
ACK a9886e3 — three Blockers and the Major verified gone from the bytes, gate green, two Minor and four Nit items non-blocking. |
|
Follow-up tracking: the first-use improvement candidates from this PR's review rounds are now tracked in #2233 (reviewer-side skill alignment incl. F12/F48/F49, code-span path guardrail, retirement obligation inventory, rename-purity verification, tiered model-routing design). The lessons are recorded in the #2219 implementation retrospective, and the audit's F12 entry now cites #2233 as its resolution reference. |
|
@da2ce7 Thank you for the approval and the recomputed verification of every round-1 disposition. The six non-blocking items from your approval review
On the F12 condition: agreed — the deferral is now backed by tracked issue #2233 (created before your approval), which also carries the other first-use improvement candidates. The T5 historical row in the spec was left untouched, matching your reading that completed-task records are history. Also noted for #2233's design input: your observation that the awk helper relies on Validation at this head: focused contract test, full linter suite, |
|
ACK 712abbd |
Introduces one canonical, auditable process for all pull-request review findings and resolvable threads.
The change consolidates legacy skills and audit records under docs/pr-reviews, defines reviewer finding and portable review-finding references, adds normalized analysis fields, preserves immutable review history, and protects the documentation contract with a deterministic structural test.
Files and packages touched: PR-review skills, agents, prompts, audit templates and archive, issue evidence, documentation, the pre-commit hook, and the agent-review-report contract test.
Validation: focused contract test, ShellCheck, documentation linting, linter all, git diff --check, independent completion review, and the mandatory pre-commit gate.
Closes #2219