ums: capture learnings from serocalculator#392 - #727
Conversation
Four learnings from a 4-round ARDI session that merged. `fully-clean.md` gains two entries: - Version parity belongs in the declare-ready sweep, not only after a merge. `sync-with-main.md` already covers the post-merge comparison; the gap is the no-merge case, where `main` advances on its own and lands on the branch's exact version. There is no conflict and no failing check yet, so the mergeability re-check the file already prescribes passes while `version-check` is about to go red. - A sixth review-job failure mode, running opposite to the five already catalogued: the review is genuine and complete, but the workflow posts the reviewer's own `gh pr comment ... <<'EOF'` invocation instead of the body. Reads as a broken run and can misfire a verdict-detecting guard into a needless retry. `memories/r-quarto.md` gains two: - altdoc keeps reference topics in two independent hand-maintained lists (`altdoc/reference.qmd` and `quarto_website.yml`'s Reference section). Updating one renders cleanly while leaving topics unreachable from navigation, and no check catches it. Includes the mechanical cross-check and the 3-vs-2 occurrence count for verifying against rendered output. - Two silent blind spots in the `lint-changed-files` shape: an unpaginated `gh::gh()` that reads only the first 30 changed files, and `lint_package()` never scanning repository-root scripts. Both absent from `lint-changed-lines`. `memories/github.md` extends the GitHub Pages 403 bullet: the deployed preview is readable through `get_file_contents` against the `gh-pages` branch, which reaches the real rendered artifact rather than the local re-render the existing advice falls back to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two findings from the review round. `mcp__github__list_commits` appeared only in the new `memories/github.md` bullet, with no entry in the mapping registry to anchor it. The tool name is right --- it was used in the session the bullet documents, with `sha: gh-pages`, and returned the deploy history the note describes --- but "used once and written down" isn't discoverable later, which is the registry's whole job. Adds a `LIST_COMMITS` operation to `tool-mappings.yml` (the source; `tool-mappings.md` and the 172 embedded wrapper tables are regenerated by `sync-codex-skill-wrappers.py`, which `validate` requires to stay in sync), and points the bullet at it. Also drops `TWO` from the new `r-quarto.md` heading. The file capitalizes common words nowhere else; its one all-caps heading is `WORDLIST`, a real filename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
|
Claude finished @d-morrison's task in 1m 12s —— View job Review
Verdict: Ready for mergeThis is a confirming review of the prior verdict (round 1, 2026-07-25T18:29:07Z: "Ready for merge with two minor items"). Both findings are addressed in Finding 1 addressed ( Finding 2 addressed ( No new issues introduced by the fix commit. |
ARD summary — round pushed as
|
| # | Finding | Disposition |
|---|---|---|
| 1 | mcp__github__list_commits unanchored — no registry entry, single reference in the corpus |
Address — added LIST_COMMITS to tool-mappings.yml |
| 2 | TWO all-caps heading out of register with the file |
Address — reworded |
On finding 1, the verification question resolves cleanly: the tool name is correct, and not because the MCP server source says so — this session called mcp__github__list_commits with sha: gh-pages and got back the real Deploy preview for PR 392 🛫 history, which is exactly the use the bullet documents. I still took the registry option rather than the verification-note option, because "verified once in a session that has since ended" isn't discoverable by anyone later, and being discoverable is what the registry is for.
tool-mappings.yml is the source of truth; tool-mappings.md plus the operations table embedded in all 172 Codex wrappers are generated from it by scripts/sync-codex-skill-wrappers.py, and validate fails if they drift. So the diff is large by construction. I checked it's purely mechanical: exactly one added line per wrapper, byte-identical across all 172, nothing else changed.
The underlying point is worth keeping: a tool name appearing exactly once in the corpus has nothing to check it against, so a correct one and a hallucinated one look identical to a later reader. Registering it is what makes the difference legible.
Local checks re-run and clean: check-memory-file-size.py, check-links.py, validate-skills.py, plus diff-scoped scans for banned punctuation (0) and multi-sentence lines (0).
Generated by Claude Code
Ready for merge —
|
| ✅ Passed |
|---|
validate · new-line-breaks / check-new-line-breaks · review / claude-review · review / require-review · gather-context |
(claude / claude jobs show skipped, as expected for a non-mention run.)
Review: Ready for merge, unqualified — verdict posted and its check run completed, with require-review green. Both round-1 findings confirmed addressed; no new issues. Both inline threads replied to and resolved.
Mergeability, re-verified just now: main ahead by 0, zero conflicts, local tip matches the pushed tip. Not an R package, so the version-check parity trap this PR documents doesn't apply here.
One correction for the record, since the round-2 review gave a different figure than I did: it says "The 100 codex-skills wrapper tables". The real count is 172 — sync-codex-skill-wrappers.py reported Generated 172 Codex skill wrappers, and git diff --stat showed 172 files changed, 172 insertions(+). It doesn't affect either finding or the verdict, so I didn't spend a round on it, but the number in this thread should be right.
Not merging — that's your call. Done here; PR is free.
Generated by Claude Code
Post-merge UMS from a 4-round ARDI session on
UCD-SERG/serocalculator#392, which merged today.Four learnings, filed where each already had a home rather than as new fragments.
shared/workflow/fully-clean.mdVersion parity belongs in the declare-ready sweep, not only after a merge.
sync-with-mainalready covers comparingDESCRIPTIONversions after mergingmainin. The gap is the case with no merge at all:mainadvances on its own after the last review round and lands on the branch's exact version, soversion-checkgoes from green to red with nothing to point at. No conflict, no failing check yet, no warning — the mergeability re-check this file already prescribes passes clean while the PR is about to fail. Caught on #392 minutes after aReady for mergeverdict on an otherwise all-green head.A sixth review-job failure mode, running opposite to the five already catalogued there. Those are all a review that ran and produced something misleading; this is a review that is genuine and complete, but whose workflow posts the reviewer's own
gh pr comment ... <<'EOF'invocation instead of the body. Nothing is lost, but it reads as a broken run — so a human discounts a review that actually passed — and a verdict-detecting guard script is left matching against a shell command, which can misfire into a needless retry and a second full review's cost. Filed upstream asd-morrison/gha#312.memories/r-quarto.mdaltdoc keeps reference topics in two independent hand-maintained lists —
altdoc/reference.qmdand theReferencesection ofaltdoc/quarto_website.yml. Updating one and not the other renders perfectly cleanly, leaving topics listed on the index page but unreachable from navigation. Nothing catches it: notdocs-check, not the docs build, not lint. The pkgdown → altdoc migration makes it especially easy to hit, since the old_pkgdown.ymlheld a single list, so porting entries off it naturally updates one of two successors. Includes the mechanical cross-check and the 3-vs-2 occurrence count for verifying against rendered output. Tracked for CI inUCD-SERG/serocalculator#610.Two silent blind spots in the
lint-changed-filesshape, both absent fromlint-changed-lines: an unpaginatedgh::gh()that reads only the first 30 changed files (everything past the 30th lands in the exclusion list), andlintr::lint_package()never scanning repository-root scripts likeapp.R. On #392 the first silently skipped 8 of 38 files and missed two realline_length_linterhits in the PR's own new test file; the second let anundesirable_function_linterhit in rootapp.Rpass a locallint_package()run. Filed asUCD-SERG/serocalculator#608.memories/github.mdExtends the GitHub Pages 403 bullet with a better fallback. The existing note says to treat a policy-blocked preview as unavailable and re-render locally. But
rossjrw/pr-preview-actioncommits each build togh-pagesunderpr-preview/pr-<N>/, soget_file_contentswithref: refs/heads/gh-pagesreturns the exact bytes the blocked URL would have served — the real rendered artifact, which a local re-render only approximates, with no Quarto toolchain needed. Includes the practical caveats: large pages spill to a file (grep, don't read whole), diff byte counts to confirm a genuinely new build, and check the branch's commit log to see which build is actually deployed.Checks
check-memory-file-size.pyandcheck-links.pyboth pass. Added lines were also checked for multi-sentence lines and for banned non-ASCII punctuation — five em-dashes I'd introduced are fixed.🤖 Generated with Claude Code
https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
Generated by Claude Code