Skip to content

docs(pr-reviews): close the PR #2527 post-merge findings after PR #2530 merged - #2533

Merged
josecelano merged 1 commit into
torrust:developfrom
josecelano:chore/2527-close-post-merge-review
Oct 14, 2026
Merged

josecelano merged 1 commit into
torrust:developfrom
josecelano:chore/2527-close-post-merge-review

Conversation

@josecelano

Copy link
Copy Markdown
Member

Closes the loop on the post-merge review findings on #2527, now that the follow-up #2530 has merged into develop (as b24550171).

Related to #2527 and #2530. Maintainer approval for the post-merge follow-up: #2527 (comment)

Changes

docs/pr-reviews/pr-2527-review/PR-REVIEW.md only:

  • F1–F4 move from FOLLOW_UP/OPEN to FIXED/RESOLVED.
  • Each row's resolution reference is its fix commit subject on develop:
    • F1: docs(issues): [#2517] record that the doctests landed with each refactor commit
    • F2: docs(issues): [#2517] point the Risks overlap note at #2521
    • F3: docs(issues): [#2517] say the stats-before reading was compared but not stored
    • F4: chore(issues): archive closed issue #2517 spec to docs/issues/closed
  • Current-tree verification re-checked on develop at b24550171.
  • Each Reply URL cites the new FIXED reply on its refactor: [#2517] make the statistics repositories' metrics lock private #2527 thread. The earlier FOLLOW_UP replies remain on the threads and are named in the Processing Log.
  • A Processing Log entry records the chore(issues): archive closed issue #2517 spec #2530 merge.

Validation

  • validate-audit-record.py --pr-number 2527 --base 988c3bc76: 4 rows, 0 failures. The base is the develop commit that chore(issues): archive closed issue #2517 spec #2530 was based on, so the fix commits fall inside the range the validator checks.
  • Pre-commit and pre-push checks passed.

The four #2527 threads are resolved after this PR is opened.

… PR torrust#2530 merged

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 14, 2026 16:33
@josecelano
josecelano requested a review from da2ce7 October 14, 2026 16:33
@josecelano josecelano self-assigned this Oct 14, 2026
@josecelano

Copy link
Copy Markdown
Member Author

ACK 0d377b0

@josecelano
josecelano merged commit e2e21f2 into torrust:develop Oct 14, 2026
1 check passed
@josecelano
josecelano deleted the chore/2527-close-post-merge-review branch October 14, 2026 16:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The audit updates match the merged commits, current documentation, and resolved GitHub threads.

0 open findings

What changed in this PR

Updates PR #2527’s audit after follow-up PR #2530 merged, closing all four post-merge findings.

Changes:

  • Marks F1–F4 as FIXED/RESOLVED.
  • Records verified fix commits, reply URLs, and merge details.
  • Appends the final processing-log entry.
File Description
docs/​pr-reviews/​pr-2527-review/​PR-REVIEW.md Finalizes the four post-merge findings and their verification evidence.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@da2ce7 da2ce7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 0d377b033ef1dfb206585b87ed0686cfdec036b9 (post-merge). Recomputed from the bytes at the merged head, against the merge base b24550171; the PR merged at 16:36:48Z as e2e21f20c.

Label: POST_MERGE_FINDINGS — this review was requested at 16:33:53Z, and the PR merged at 16:36:48Z, before this round ran. The two findings below stand at the merged head and are left for a follow-up. Neither is blocking, and no ACK follows a merged PR.

The record. Rows F1-F4 keep their source comments' severities (Minor, Nit, Nit, Nit), their source URLs and review id 5520161573, and move to FIXED/RESOLVED. That is what step 6 of "Reviews Submitted After Merge" prescribes (process-pr-review SKILL.md:141-143: "After the follow-up merges, update the original audit with the durable merge reference, change completed dispositions to FIXED, reply if needed, then resolve addressed threads").

Each Resolution reference is a subject that occurs once in history, on a commit that does what its finding asked:

  • fa48afeb3 points the Risks bullet at #2521.
  • b97093fe9 says the stats-before reading was compared but not stored.
  • c8cae256b adds the correction at the evidence's commit list, plus a 16:00 Progress Log entry.
  • The archive commit moves the evidence stamp from 14:15 to 15:06 as it edits the file, and later commits keep it current.

Each Reply URL is the FIXED reply (16:29:40-45Z) on its own thread, and the four FOLLOW_UP replies the log names exist. The Current-tree sentences hold at b24550171 and at 48c4d3013 (the issue folder is identical in both): evidence :4, :20-23 and :147-149; ISSUE.md :131-133, :236, :305 and :373-374. The approval URL (15:56:51Z) is in Ownership and precedes the first fix commit (15:58 UTC). The log is chronological, and its 16:30 entry follows the 16:28:07Z merge and the replies. All four threads read resolved=true in the capture.

Validator. I ran it offline from the head worktree with the captured comments. --base 184446a2b and the PR body's base (the merge of PR 2528, from which PR 2530 branched) both give 4 rows, 4 log entries and 0 failures. The merge base b24550171 and develop fail all four rows: the fix commits are their ancestors, so <base>..HEAD holds only this commit, or nothing. 184446a2b is the base to use, because its range holds every commit that fixed a row, including F2's Out of Scope half (F6).

Hygiene. One commit, author = committer (16:31:11Z). The subject is Conventional with the docs(pr-reviews) scope. It has no [#…] tag, which fits the other audit commits and the title rule at AGENTS.md:394 (the subject equals the PR title), since it closes no issue. No body beyond one trailer line, git show --check is clean, and the banned token is absent.

Review bot. Its overview (16:46:05Z, after the merge) posts no numbered findings.

Findings. F5 and F6 (Nit), inline.

Checked, no finding. The PR body's claims match the diff and the capture, including its validator result. The categories and ORIGINAL relationships are unchanged. The 16:01 entry reports replies posted at 16:02:32-39Z; the commit carrying it is later, so this is not raised.

Checks

On the loop's compute hub at the merged head, merge base b24550171 (the first parent of the merge commit e2e21f20c; receipts server-gates-krkavec-297 and -300): pre-commit profile gate exit 0 (80 s); linter markdown, linter cspell, linter lychee exit 0; frontmatter-validator --all: 19 errors / 4 warnings at this head and 19 / 4 at b24550171 re-measured, none in the file this PR edits; validate-audit-record.py --pr-number 2527 --base 184446a2b exit 0 against this head's pr-2527-review record with the captured #2527 threads (4 rows, 4 log entries, 0 failures). GitHub checks at this head at posting time: "Docs Lint: completed/success". At posting, develop is 48c4d3013.

- Current-tree verification: on `develop` at `b24550171`, Out of Scope (:131-133) and the Risks
bullet (:373-374) of the archived `ISSUE.md` both name #2521, and the only "not committed yet"
left is the historical 09:06 UTC Progress Log entry (:236).
- Resolution reference: docs(issues): [#2517] point the Risks overlap note at #2521

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit][F6] F2's Resolution reference cites only half of its fix

F2 was fixed in two commits, and the Solution above names both, but this field names only the Risks commit. The Out of Scope half, docs(issues): [#2517] point the SI-30 traits out-of-scope note at #2521 (8901bc508, on develop through the PR 2528 merge), belongs here as well, ;-separated, a form the validator accepts. A scratch copy of this record that lists both subjects validates against --base 184446a2b (4 rows, 0 failures). The PR body's base rejects it, because 8901bc508 is an ancestor of that base, so the validation base should move to 184446a2b too. Non-blocking.

Re-verified F1-F4 on `develop` and replied `FIXED` on each thread. Each row's Reply URL now
cites that reply; the earlier `FOLLOW_UP` replies (r4273199747, r4273200081, r4273200471,
r4273200938) remain on the threads. Moved F1-F4 to `FIXED`/`RESOLVED`, citing their fix commit
subjects; the threads are resolved after this audit is pushed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nit][F5] The record never logs the final thread refresh

The last Processing Log entry ends with a promise: "the threads are resolved after this audit is pushed". Step 6 of "Reviews Submitted After Merge" (process-pr-review SKILL.md:143) ends with "Refresh GraphQL and record zero unresolved actionable threads", and this record's own Completion Rules (:175) say the same. The four threads on the merged PR now read resolved=true, so the follow-up is one appended entry that records that refresh and its time. Non-blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants