Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The validation plan contains blocking inconsistencies that prevent the proposed workflow from being followed as written.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Drafts EPIC #2278’s order-5 author self-audit gate for the PR-review workflow. This PR changes planning documentation only.
Changes:
- Defines gate requirements, implementation tasks, acceptance criteria, and verification scenarios.
- Records the draft in the EPIC while leaving order 5 as
TODO.
| File | Description |
|---|---|
| docs/issues/open/2278-2003-strengthen-pr-review-author-self-audit/EPIC.md | Updates the timestamp and records the draft. |
| docs/issues/drafts/2278-author-self-audit-gate/ISSUE.md | Specifies the gate, verification plan, and maintainer decisions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… record Related to torrust#2278
… record Related to torrust#2278
dc6e647 to
5d667d8
Compare
|
Rebased onto |
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
Review round 2. Changes requested.
The spec content and the round 1 audit check out at 5d667d817:
- The cited lines resolve at the branch base.
- Both
git grepclaims in the audit record match the stated lines. validate-audit-record.py --pr-number 2431 --base 29afe946cexits 0.linter markdown,cspell, andlycheeall exit 0.
The problem is that develop has moved under the line-number citations (F3-F5).
Merge plan. I'm holding this PR until #2434 merges (#2494 has merged). Both PRs change the EPIC's frontmatter stamp and Progress Log. You can address F3-F5 now. If F4 switches to references that don't move, the rebase after #2434 should only need to resolve the EPIC frontmatter and Progress Log conflicts.
[Nit][F6] PR body: "Files Touched" omits docs/pr-reviews/pr-2431-review/PR-REVIEW.md, and "Validation" says citations resolve at develop eb96d2957, but the branch was rebased onto 29afe946c. Please refresh both after the rebase.
Open questions. On the six requested decisions: I'm fine with the draft's proposed answers for questions 2 to 5. Those are no gate bullet in the copied ## Completion Rules, one pre-posting pass covering several replies posted together, F75 wording staying with order 8, and F55 kept here in its own commit. I'll confirm question 1 once F5 restates the overlap with #2362.
… record Related to torrust#2278
5d667d8 to
a987d74
Compare
…torrust#2431 Review 5469277375 left three inline findings and one review-body finding. The audit record gains their rows and detail blocks: F3 and F4 resolved by the stable-references commit, F5 by the overlap restatement, and F6, the description's Files Touched and Validation, by the replacement description, cited through the PR conversation response as the skill prescribes for a fix outside the tree. The Processing Log records the review, the rebase, the three commits from their author dates, and this record.
|
F6 (review 5469277375): FIXED in the PR description, which I replaced. Files Touched now lists |
|
Round 2 re-check at
Decision 1: whichever of order 5 and #2362 has its implementation ready first lands first. The other rebases and takes the next skill version. With no passage rewritten by both, nothing else depends on the order. Decisions 1–5 are now settled. Remaining before approval: #2434 has merged, and the branch now conflicts with
I'll re-check the resolution after the push and approve. |
…ication Draft the EPIC torrust#2278 order 5 specification: a manual author self-audit gate in process-pr-review before each audit commit, reply, thread resolution and re-review request, covering matrix lines 35 and 39-47, F65 and the manual part of F55; record the draft in the EPIC log. This is the spec-first step: the GitHub issue is created, and the draft moved to docs/issues/open/, only after the maintainer reviews it. Related to torrust#2278
…-audit gate Related to torrust#2278
… record Related to torrust#2278
…red R2 and R4 into the order-5 change Review 5467975631 approved PR torrust#2494 and deferred two non-blocking findings to the next torrust#2278 change touching these files, because a push would have dismissed the approval. R2: the References line still said the PR torrust#2484 retrospective reaches develop when PR torrust#2484 merges, which has happened, so the clause goes. R4: the PR torrust#2484 sweep row's Disposition read "Defer (tool)", while its rationale defers only the audit-body check and assigns the tree-wide checks to other owners; in the matrix's vocabulary those are Out of scope, so the cell now says so. A Progress Log entry records the carry.
…C by stable references The order-5 draft cited the process-pr-review skill, the review template, the validator, the torrust#2278 matrix and the EPIC by line number at its old base. Since then develop added a skill section and two sentences, rewrote a template paragraph and shifted the matrix rows, so several citations named other text; the next merge into the EPIC would move the rest. Each citation now names a section heading or a short quote, matrix rows are named by F-ID or by source PR and quote, and the skill's current version, 1.5, is stated where the draft says order 5 bumps it. The two earlier Progress Log entries named rows by line number at their base and now name them the same way.
…ix assigns to order 5 PR torrust#2494 added the PR torrust#2484 retrospective's rows to the torrust#2278 matrix and assigned two of them to order 5: a manual sweep before each push to a reviewed branch, and anchoring each Current-tree verification to a heading or quoted phrase. The order-5 draft predates them and named neither, so the order would have closed without delivering two adopted items. The draft now carries both in its Background, Scope table, tasks and acceptance criteria, citing the matrix rows by source PR and quote, and its Progress Log records that they entered with this round.
…ent develop The draft's Dependencies bullet gave the planned rewrites of torrust#2362 as line ranges from the old base and said this order rewrites none of them. On develop a later skill change inserted two lines inside one of those ranges and rewrote a template paragraph inside another, so both the ranges and the conclusion needed re-deriving before the maintainer decides which order lands first. The bullet now names by quote each passage that torrust#2362 rewrites, states that no passage is rewritten by both orders, names the three sections both edit and the shared version bump, and notes that the fix-outside-the-tree rule already on develop is the text that the F5 and F6 wording of torrust#2362 starts from.
…torrust#2431 Review 5469277375 left three inline findings and one review-body finding. The audit record gains their rows and detail blocks: F3 and F4 resolved by the stable-references commit, F5 by the overlap restatement, and F6, the description's Files Touched and Validation, by the replacement description, cited through the PR conversation response as the skill prescribes for a fix outside the tree. The Processing Log records the review, the rebase, the three commits from their author dates, and this record.
7b7348d to
2840951
Compare
|
Rebased onto |
Related to #2278
Summary
This spec-only PR drafts EPIC #2278 order 5, "Add the author self-audit gate to
process-pr-review". It contains planning documentation only: no skill, template, validator or workflow change.docs/issues/drafts/2278-author-self-audit-gate/ISSUE.md. The order is docs only. It takes its content from the EPIC's retrospective-improvement matrix: the rows routed to the self-audit gate (run the validator when it can run and do its checks by hand otherwise, re-derive everything after a second re-raise and after context compaction, corrections re-derived from named Git or GitHub commands, command and result before narrative, no self-referential counts, one fix per commit,git show --staton cited commits, log stamps from Git and GitHub timestamps), the F65 counterpart to "update progressively", the manual part of F55, and the two PR docs(issues): [#2482] add the sub-EPIC specification for #2482 #2484 items the matrix assigns to order 5 (a manual sweep before each push to a reviewed branch, and anchoring eachCurrent-tree verificationto a heading or quoted phrase). Each gate pass is recorded as one append-only Processing Log entry naming the gated event and the commands run; no helper is a prerequisite and no per-finding field is added. Four commit points: the skill's gate section, template guidance, two checklist items, and the evidence with the EPIC row.develop686a42f45; order 5 bumps it from there, or from the version Reconcile the residual audit-contract rules from the PR #2313 post-merge review #2362 leaves.TODOstatus and placeholder path until the issue exists.Requested Maintainer Decisions
The maintainer agreed with the draft's proposed answers to questions 2 to 5 in review 5469277375; question 1 waits on the restated #2362 overlap below.
develop686a42f45, Reconcile the residual audit-contract rules from the PR #2313 post-merge review #2362 rewrites, in the skill, Workflow step 7's superseded-thread sentence, step 8 ("Consolidate review bodies"), the Detail-Entry Fields sentence onFIXEDresolution references, and the Completion Checklist item on consolidated responses; in the template, the Status Values bullet on outdated threads, theResolution referenceplaceholder and the guidance paragraph after it, and the Completion Rules bullets on outdated threads, consolidated responses and citing a fix. This order rewrites none of those passages, so no sentence is changed by both. Both edit three of the same sections (Workflow step 7, the Completion Checklist, and the template guidance beside Finding Details), where this order inserts next to Reconcile the residual audit-contract rules from the PR #2313 post-merge review #2362's passages, and both bump the skill version. The rule for a fix outside the tree, already ondevelop, is the text Reconcile the residual audit-contract rules from the PR #2313 post-merge review #2362's F5 and F6 wording starts from. Which lands first? The second rebases over the first's text in those three sections and takes the next skill version.## Completion Rulessection carries no gate bullet, because every new record would copy it and order 8 would byte-diff it. Agreed as proposed.RE_RAISE_OFdoes not say whose finding ID it takes) keeps its wording with order 8, where the register places it. Agreed as proposed.docs/issues/open/with its number, and row 5 links to it.Files Touched
docs/issues/drafts/2278-author-self-audit-gate/ISSUE.md(new)docs/issues/open/2278-2003-strengthen-pr-review-author-self-audit/EPIC.md(one Progress Log line for the draft; with the carry commit, the References line, a second Progress Log line and the frontmatter stamp)docs/issues/open/2278-2003-strengthen-pr-review-author-self-audit/retrospective-improvement-matrix.md(the carry commit: one Disposition cell and the frontmatter stamp)docs/pr-reviews/pr-2431-review/PR-REVIEW.md(new: the review audit record, rounds 1 and 2)Rebase, carried items and round 2
PR #2494 merged into
developat686a42f45and also changed the #2278 EPIC, so this branch was rebased onto it; every other file merged cleanly. The EPIC conflict is resolved by keeping both sides: PR #2494's changes, including its two 2026-10-08 Progress Log entries, and this branch's 2026-10-03 entry for the draft, placed before them so the log stays chronological.docs(issues): [#2278] carry the PR #2494 review's deferred R2 and R4 into the order-5 changeapplies the two non-blocking findings that review 5467975631 deferred to this change: the References line drops the clause on when the PR #2484 retrospective reachesdevelop, and the PR #2484 sweep row's Disposition reads "Adopt (manual sweep); Defer (audit-body tool check); Out of scope (tree-wide tool checks)". Review 5469277375 is answered bydocs(issues): [#2278] cite the skill, template, matrix and EPIC by stable references(F3, F4),docs(issues): [#2278] restate the #2362 overlap on current develop(F5) and this description (F6).docs(issues): [#2278] cover the PR #2484 items the matrix assigns to order 5adds the two items the matrix has assigned to this order since PR #2494, anddocs(pr-reviews): [#2278] record the maintainer's round 2 on PR #2431records the round. After PR #2434 merged, the branch was rebased again, ontodevelopf960c78d7; its ten commits keep their content, and the #2278 EPIC conflict is resolved as the maintainer asked.Merge order
PR #2434 merged into
developatf960c78d7, and this branch is rebased onto it. The rebase touched only the #2278 EPIC's frontmatter stamp and the position of one Progress Log entry, resolved as the maintainer asked: both sides' entries are kept, this branch's 2026-10-03 17:09 entry sits beforedevelop's 2026-10-04 19:18 entry, andlast-updated-utcstays "2026-10-09 11:16", the stamp of the merged log's latest entry.Validation
At this head:
In a Linux container with the workspace nightly toolchain, at the rebased head
284095128, basedevelopf960c78d7(the merge base): pre-commit profile gate exit 0 (78 s);linter markdown,linter cspell,linter lycheeexit 0;frontmatter-validator --all19 errors / 4 warnings at the head and 19 / 4 atdevelopre-measured;validate-audit-record.py --pr-number 2431against the review comments captured after the round-2 replies were posted: 6 rows, 12 log entries, 0 failures;git range-diffagainst7b7348dbfshows 8 commits equal and 2 differing only in the resolved EPIC hunks.