docs: require testing against template repos before declaring ready to merge - #408
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…erence self-verify exception
|
Addressed findings from review of
|
|
@claude review |
|
👀 Picked up by workflow run #31026094798. Setup runs first; Claude itself responds after that. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Antigravity Agent Report (Code-Review)
Here is a comprehensive code review of PR #408.
Executive Summary
PR #408 introduces a standing guidance section to CLAUDE.md requiring developers and AI assistants to test changes to actions and reusable workflows against representative lab template repositories before declaring a PR clean or ready to merge.
Overall, this documentation addition is accurate, well-reasoned, and correctly points to existing downstream template repositories. Below are actionable suggestions for semantic formatting, link completeness, and streamlining prose.
Detailed Findings
1. Formatting & Readability: Apply Semantic Line Breaks
Location: CLAUDE.md:L1504-L1515
Finding:
Lines 1504, 1506, 1508, 1513, and 1515 are long, unbroken lines (up to 367 characters). In accordance with Rule 7 of the repository's guidelines (Suggest semantic line breaks in prose), text in CLAUDE.md should break at sentence/clause boundaries (~60–80 characters). This keeps git diffs tightly scoped to individual sentences when guidelines evolve in future PRs.
Suggested Revision:
## Test changes against a template repo before declaring ready to merge
Before declaring a PR ready to merge (or clean / ready for merge verdict)
for PRs in this repo (`gha`) that change GitHub Actions or their component
actions/workflows, test the changes using one of the lab's template repos
(`rpt`, `qwt`, `qbt`, `qmt`, etc.).
Running unit tests or `_selftest.yml` in `gha` alone is not sufficient when
modifying an action or workflow component because `_selftest.yml` exercises
local composites or throwaway fixtures rather than full downstream project
structures (such as R package vignettes or Quarto site builds) that pin `@v2`
reusable workflows. To ensure full compatibility and prevent downstream breakage:
- Test the updated action/workflow against a representative template repository
or test bed (e.g., [`d-morrison/test.hac`](https://github.com/d-morrison/test.hac)
or a template repo branch pointing `uses:` to the PR's branch/SHA):
- **`rpt`** ([`Morrison-Lab/rpt`](https://github.com/Morrison-Lab/rpt)): R package template
- **`qwt`** ([`d-morrison/qwt`](https://github.com/d-morrison/qwt)): Quarto website template
- **`qbt`** ([`d-morrison/qbt`](https://github.com/d-morrison/qbt)): Quarto book template
- **`qmt`** ([`d-morrison/qmt`](https://github.com/d-morrison/qmt)): Quarto manuscript template
- For PRs fixing reusable workflows (`claude-code-review.yml` or `claude.yml`)
that cannot self-verify on the unmerged `@v2` floating tag, follow the
manual/offline verification path documented in
[A PR fixing claude-code-review.yml (or claude.yml) itself can't self-verify before merge](#a-pr-fixing-claude-code-reviewyml-or-claudeyml-itself-cant-self-verify-before-merge).
Verify that the action or workflow succeeds and behaves as expected in the
target template context before declaring the PR clean or ready to merge.2. Completeness & Links: Fully Qualify and Link test.hac
Location: CLAUDE.md:L1508
Finding:
In the bullet point on line 1508, test.hac is referenced in backticks as an example test bed, but unlike rpt, qwt, qbt, and qmt, it is unlinked and lacks its owner prefix (d-morrison/test.hac).
To match the repository owner qualification and hyperlinking standard used for all template repositories in the list immediately following it, consider updating test.hac to a full Markdown link (e.g., d-morrison/test.hac).
3. Redundancy: Consolidate Opening and Closing Sentences
Location: CLAUDE.md:L1504, CLAUDE.md:L1515
Finding:
Line 1504 begins with:
"Before declaring a PR ready to merge (or clean / ready for merge verdict) for PRs in this repo (
gha) that change GitHub Actions or their component actions/workflows..."
Line 1515 concludes the section with:
"Verify that the action or workflow succeeds and behaves as expected in the target template context before declaring the PR clean or ready to merge."
Per Rule 10 (Question redundant content), line 1515 largely restates the condition introduced in line 1504. Breaking up lines semantically as shown in Finding 1 helps make the flow more natural and eliminates redundant phrasing.
|
Working on this (ARDIA sweep) — paws off until I'm done. |
…t redundancy Address antigravity review findings on #408: - Apply semantic line breaks to the new section (Rule 7). - Remove the 'test.hac' reference: no test.hac repo exists under Morrison-Lab/d-morrison/UCD-SERG/ucdavis, so linking it would cite a nonexistent repo. Fold the 'point uses: at the PR branch/SHA' guidance into the paragraph instead. - Cut the redundant closing 'Verify that...' sentence, folding the succeed-check into the setup paragraph (Rule 10).
ARD summary (round: antigravity findings)Addressed all three inline findings in
Also note the prior |
There was a problem hiding this comment.
🤖 Antigravity Agent Report (Code-Review)
An expert code review of Pull Request #408 (docs: require testing against template repos before declaring ready to merge) has been performed. Below are the actionable findings and recommendations from a comprehensive single-pass analysis against the repository's guidelines and standards in CLAUDE.md.
Key Findings & Recommendations
1. Repository Owner Inconsistency for rpt Template
Location: CLAUDE.md:L1516
In the template repos list, rpt is linked as Morrison-Lab/rpt:
- **`rpt`** ([`Morrison-Lab/rpt`](https://github.com/Morrison-Lab/rpt)) -- R package templateHowever, the other three template repositories in lines 1517–1519 use the d-morrison owner (d-morrison/qwt, d-morrison/qbt, d-morrison/qmt). Furthermore, elsewhere in CLAUDE.md (e.g. line 419, line 1488) and in changelog.d/retarget-doc-urls-after-org-move.fixed.md:L21 ("Repositories that did not move -- qwt, rme, rpt -- are untouched."), rpt is documented as remaining under d-morrison/rpt.
CLAUDE.md specifically warns under its owner retargeting rules:
"Read it as applying to any repeated string being retargeted -- an owner, a URL, a tag, a renamed input -- and grep for that string rather than for
@v1."
Suggested Fix:
-- **`rpt`** ([`Morrison-Lab/rpt`](https://github.com/Morrison-Lab/rpt)) -- R package template
+- **`rpt`** ([`d-morrison/rpt`](https://github.com/d-morrison/rpt)) -- R package template2. Technical Clarification: Testing Reusable Workflows Calling @v2 Composite Actions
Location: CLAUDE.md:L1512-L1513
The instructions state:
Instead, point a template repo's
uses:at the PR's branch or SHA and confirm the workflow succeeds there:
When testing a PR that modifies a reusable workflow directly (e.g., .github/workflows/check-links.yml), pointing the template repo's workflow uses: at @<pr-branch> correctly executes the PR's reusable workflow.
However, if the PR modifies a nested composite action (e.g., .github/actions/check-secrets/action.yml) that a reusable workflow invokes via an explicit tag (e.g., uses: Morrison-Lab/gha/.github/actions/check-secrets@v2), pointing the template's top-level reusable workflow call to @<pr-branch> will still fetch the composite action from the un-slid @v2 tag on main, rather than the composite code on the PR branch.
To make these instructions bulletproof for contributors working on composite actions, add a brief note pointing out that when testing changes to internal composite actions, the composite step reference inside the test workflow must also temporarily point to @<pr-branch> (or be tested via direct step invocation) during template validation.
3. Formatting: Semantic Line Breaks in Added Prose
Location: CLAUDE.md:L1503-L1525
In accordance with Guideline 7 ("Suggest semantic line breaks in prose") from CLAUDE.md, long paragraphs should break at clause and sentence boundaries (roughly 60–80 characters) rather than wrapping arbitrarily across lines. This keeps git diffs tightly scoped when individual sentences are updated in future PRs.
Suggested Revision:
-Before declaring a PR ready to merge -- or reporting a clean / ready-for-merge
-verdict -- for a change in this repo (`gha`) that touches a GitHub Action or a
-component action/workflow, test that change against one of the lab's template
-repos.
-
-Running the unit tests or `_selftest.yml` in `gha` alone is not sufficient for
-such a change, because `_selftest.yml` exercises local composites and throwaway
-fixtures rather than the full downstream project structures -- an R package's
-vignettes, a Quarto site build -- that pin the `@v2` reusable workflows.
-Instead, point a template repo's `uses:` at the PR's branch or SHA and confirm
-the workflow succeeds there:
+Before declaring a PR ready to merge --
+or reporting a clean / ready-for-merge verdict --
+for a change in this repo (`gha`) that touches a GitHub Action or a component action/workflow,
+test that change against one of the lab's template repos.
+
+Running the unit tests or `_selftest.yml` in `gha` alone is not sufficient for such a change,
+because `_selftest.yml` exercises local composites and throwaway fixtures
+rather than the full downstream project structures --
+an R package's vignettes, a Quarto site build --
+that pin the `@v2` reusable workflows.
+Instead, point a template repo's `uses:` at the PR's branch or SHA
+and confirm the workflow succeeds there:
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…eview workflows Address #408 round-2 review: - Add a caveat that repointing a template's top-level uses: does NOT exercise a change to a nested composite action (its @v2 refs resolve at job-prep time from the released tag), so a composite-only test passes vacuously; instruct repointing the nested refs too. - Replace the review-workflow exception's incorrect '@v2 self-verify' rationale with the real, empirically-confirmed obstacle: testing a review-workflow change runs claude-code-action against a modified caller, which fails its workflow-validation and reddens the check with no verdict (proven 2026-08-05 via a throwaway dispatched review). - rpt link stays Morrison-Lab/rpt (verified: d-morrison/rpt 301-redirects there); the 'rpt untouched' changelog line is stale, tracked in #421.
ARD summary (round 2)
Round-2 verified empirically via throwaway test PR #420 (closed) — which is itself an application of this very rule, and is what surfaced the corrected obstacle in finding 5. |
There was a problem hiding this comment.
🤖 Antigravity Agent Report (Code-Review)
Here is a comprehensive code review of PR #408.
Review Summary
This PR adds clear, actionable documentation to CLAUDE.md establishing a standing requirement to test GitHub Actions changes against one of the lab's template repos before declaring a PR ready to merge. The technical explanations of job-preparation composite resolution and OIDC App-token validation behavior for review workflows are accurate and well-reasoned.
A few findings regarding repo owner consistency, link wrapping, and semantic line break formatting are detailed below.
Findings
1. Inconsistent owner namespace across template repos
Location: CLAUDE.md:L1516-L1519
Issue:
The bulleted list mixes the Morrison-Lab org prefix for rpt with the d-morrison owner prefix for qwt, qbt, and qmt:
- **`rpt`** ([`Morrison-Lab/rpt`](https://github.com/Morrison-Lab/rpt)) -- R package template
- **`qwt`** ([`d-morrison/qwt`](https://github.com/d-morrison/qwt)) -- Quarto website template
- **`qbt`** ([`d-morrison/qbt`](https://github.com/d-morrison/qbt)) -- Quarto book template
- **`qmt`** ([`d-morrison/qmt`](https://github.com/d-morrison/qmt)) -- Quarto manuscript templateRecommendation:
Per the repo guidelines on owner retargeting ("Read it as applying to any repeated string being retargeted -- an owner, a URL, a tag, a renamed input..."), maintain namespace consistency across sibling lab template repos in the same list. Update the Quarto template references to Morrison-Lab if they have been retargeted alongside rpt:
- **`rpt`** ([`Morrison-Lab/rpt`](https://github.com/Morrison-Lab/rpt)) -- R package template
- **`qwt`** ([`Morrison-Lab/qwt`](https://github.com/Morrison-Lab/qwt)) -- Quarto website template
- **`qbt`** ([`Morrison-Lab/qbt`](https://github.com/Morrison-Lab/qbt)) -- Quarto book template
- **`qmt`** ([`Morrison-Lab/qmt`](https://github.com/Morrison-Lab/qmt)) -- Quarto manuscript template2. Awkward line wrap and length in markdown link syntax
Location: CLAUDE.md:L1546-L1548
Issue:
The cross-reference link to the self-verify section wraps mid-clause across three lines, stranding a single word (merge) at the start of L1548 right before the opening parenthesis of the anchor target ](#a-pr-.... Consequently, L1548 reaches 93 characters:
Fall back to the manual/offline path in [A PR fixing claude-code-review.yml (or
claude.yml) itself can't self-verify before
merge](#a-pr-fixing-claude-code-reviewyml-or-claudeyml-itself-cant-self-verify-before-merge),Recommendation:
Keep the full link anchor and text intact on a semantic clause boundary:
Fall back to the manual/offline path in
[A PR fixing claude-code-review.yml (or claude.yml) itself can't self-verify before merge](#a-pr-fixing-claude-code-reviewyml-or-claudeyml-itself-cant-self-verify-before-merge),
or give the action a `github_token` override that skips the OIDC exchange.3. Semantic line break formatting in prose
Location: CLAUDE.md:L1502-L1549
Issue:
Paragraphs in the newly added section break continuously mid-clause across 75–81 character bounds (e.g., L1502–L1505, L1521–L1532, L1536–L1545).
Recommendation:
Per Item 7 of the review guidelines (Semantic Line Breaks), break lines at natural sentence or clause boundaries (~60–80 chars) so future diffs remain cleanly isolated to single changed clauses.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address #408 round-3 antigravity finding 2 (link text wrapped mid-anchor, stranding 'merge'); keep the full link intact on one line (URL-inflation exception).
ARD summary (round 3 — antigravity nits)Primary
Substantively clean (claude Ready-for-merge; only style nits remain, both dispositioned). |
🤖 Antigravity Agent Report (Code-Review)Code Review SummaryPR #408: This pull request adds an important standing requirement to Overall, this is a well-crafted, thorough, and highly valuable addition to repo guidance. It documents critical edge cases—such as nested Below are two minor actionable findings to refine technical accuracy and formatting. Detailed Findings & Recommendations1. Precision regarding composite action directory locationsLocation: Context: Repointing the template's top-level `uses:` exercises a change to a reusable
workflow's own YAML, but not a change to a **composite action**
(`.github/actions/<x>/`, where most of this repo's capabilities live).
A reusable workflow pins its internal composite calls to a literal `@v2`
(e.g. `claude-code-review.yml` has ~10 such `uses: .../actions/<x>@v2` sites),
and those resolve at job-preparation time from the released tag regardless of
the ref the parent workflow file was fetched from (see [Re-running *failed jobs*
cannot verify a tag slide](#re-running-failed-jobs-cannot-verify-a-tag-slide)).Finding:
Because reusable workflows pin both root capability composites ( Suggested Revision: -Repointing the template's top-level `uses:` exercises a change to a reusable
-workflow's own YAML, but not a change to a **composite action**
-(`.github/actions/<x>/`, where most of this repo's capabilities live).
-A reusable workflow pins its internal composite calls to a literal `@v2`
-(e.g. `claude-code-review.yml` has ~10 such `uses: .../actions/<x>@v2` sites),
+Repointing the template's top-level `uses:` exercises a change to a reusable
+workflow's own YAML, but not a change to a **composite action**
+(either root capability actions like `check-links/` or internal helpers under
+`.github/actions/`).
+A reusable workflow pins its internal composite calls to a literal `@v2`
+(e.g. `claude-code-review.yml` has ~10 such `uses: .../actions/<x>@v2` sites and
+reusable wrappers call root composites like `uses: .../check-links@v2`),
and those resolve at job-preparation time from the released tag regardless of
the ref the parent workflow file was fetched from (see [Re-running *failed jobs*
cannot verify a tag slide](#re-running-failed-jobs-cannot-verify-a-tag-slide)).2. Semantic line length suggestionLocation: Finding: Suggested Revision: -(Confirmed empirically 2026-08-05 via a throwaway dispatched review; the failure
-surfaces as a fast `no execution output`, not a literal `401`.)
+(Confirmed empirically 2026-08-05 via a throwaway dispatched review;
+the failure surfaces as a fast `no execution output`, not a literal `401`.)Verification Summary
|
|
Claude finished review — View run No cleanup needed — the earlier redirect and fetch attempts were both blocked before creating anything. I have everything needed to finalize the review. Review: #408Summary: Pure documentation change (1 file, This is a confirming review. The most recent full review round already reached Ready for merge (2026-08-05T23:56:19Z, run 31057673981), after all findings from two earlier rounds were addressed:
Since that verdict, the only change is commit Independent re-verification performed this round (not just trusting prior rounds or the commit message):
No new findings. No AI-authorship tells, no unresolved ambiguity, no unsupported claims found in the current diff. VerdictReady for merge — the prior round's Ready-for-merge verdict (comment) still stands; the one change since then is a cosmetic link-wrap fix, and my independent re-verification of every factual/technical claim in the diff (repo-owner redirects, composite-ref count, anchor-link resolution, the OIDC-validation mechanism, and both empirical PR/issue citations) found nothing inaccurate or unaddressed. |
|
✅ Driven to clean via ARDIA: Ready for merge at 27c6ef1, all checks green, 0 open threads (3 review rounds; substantive composite-test + review-workflow-obstacle findings addressed, owner-consistency + line-break nits rebutted/declined with verification). Unclaiming — ready for your merge. |
…gment changelog.d/retarget-doc-urls-after-org-move.fixed.md claimed rpt was among the repositories that 'did not move', but d-morrison/rpt now 301-redirects to Morrison-Lab/rpt -- and that stale claim misled an automated review on #408 into flagging a correct Morrison-Lab/rpt link as wrong. Drop rpt from the 'did not move' list (leaving qwt, rme) and note that it has since moved, mirroring the fragment's existing 'that stopped holding once ai-config renamed the marketplace' pattern. The 'Ported from d-morrison/rpt' provenance notes elsewhere (CLAUDE.md, the website reference page, code comments, the add-altdoc changelog fragment) are left as-is: they are accurate historical statements (the code WAS ported from d-morrison/rpt), the redirect keeps them navigable, and this matches the fragment's own 'historical entries keep the name the repository had when they were written' convention. Closes #421 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Adds a standing requirement to
CLAUDE.mdstating that before declaring a PR ready to merge (or clean) for PRs inghathat touch GitHub Actions or their component actions/workflows, the changes must be tested against one of the lab's template repos (rpt,qwt,qbt,qmt, etc.).Verification
CLAUDE.mdformatting withmarkdownlint.