Skip to content

Commit 425021c

Browse files
pinin4fjordspinin4fjords
committed
Merge remote-tracking branch 'origin/main' into pr239-rebase
2 parents 602608c + 55d492b commit 425021c

40 files changed

Lines changed: 10820 additions & 270 deletions

‎.claude/skills/README.md‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
# `.claude/skills`
2+
3+
Claude Code skills for working on and with nf-metro. Skills auto-trigger
4+
from the descriptions in each `SKILL.md` frontmatter; for the conventions
5+
each one assumes (local paths, env names, render-preview URL), see the
6+
preamble at the top of the relevant `SKILL.md`.
7+
8+
## Cohort map
9+
10+
The six skills fall into three conceptual groups.
11+
12+
### Pipeline-side
13+
14+
For someone integrating nf-metro into their own Nextflow pipeline repo:
15+
16+
| Skill | When to use |
17+
|---|---|
18+
| [`pipeline-metro-diagram`](pipeline-metro-diagram/SKILL.md) | Author the `.mmd` content for a pipeline's metro map: lines, stations, sections, off-track inputs, the render-inspect-edit iteration loop. |
19+
| [`pipeline-metro-setup`](pipeline-metro-setup/SKILL.md) | Wire the rendered map into the pipeline repo: file layout, render commands, README image swap, CHANGELOG, install-line pinning (released version vs named-branch on a fork). |
20+
21+
### nf-metro-side authoring
22+
23+
For someone changing nf-metro itself:
24+
25+
| Skill | When to use |
26+
|---|---|
27+
| [`fix-issue`](fix-issue/SKILL.md) | General end-to-end workflow for a GitHub issue: worktree, environment, implement, test, push, PR. The "skeleton" most other nf-metro authoring tasks build on. |
28+
| [`nf-metro-layout-fix`](nf-metro-layout-fix/SKILL.md) | Drive code-level fixes to nf-metro layout when a real pipeline render exposes a bug. Savepoint pattern, invariant-test-first-then-fix-then-runtime-validator loop, conditional gating, the "improvement ratchet". |
29+
| [`pr-chain-vet`](pr-chain-vet/SKILL.md) | Per-PR vetting on a stacked PR chain: gallery diff vs `main`, classify every changed example, `/simplify` pass, sweep narrative comments, get CI green, post-merge cleanup in the right order. |
30+
31+
### Visual verification
32+
33+
Opt-in only (`disable-model-invocation: true` — the user must invoke
34+
explicitly):
35+
36+
| Skill | When to use |
37+
|---|---|
38+
| [`render-topologies`](render-topologies/SKILL.md) | Local pixel-diff of all gallery renders between the current branch and `origin/main`. Only needed for pre-push confidence; the CI render preview on the PR is the authoritative review. |
39+
40+
## How the skills relate
41+
42+
- `pipeline-metro-diagram`'s "is it mmd or nf-metro?" triage in Step 5
43+
hands off to `nf-metro-layout-fix` when the diagnosis is engine-side.
44+
- `nf-metro-layout-fix` Step 4 hands off to `pr-chain-vet` for the
45+
per-PR vetting workflow that ships the resulting chain back to `main`.
46+
- `pipeline-metro-setup` Stage 2 Case B (named-branch pin on a fork) is
47+
the bridge developers use *while* `nf-metro-layout-fix` +
48+
`pr-chain-vet` work is in flight.
49+
- `fix-issue` Step 4 references `render-topologies` for the optional
50+
pre-push local diff.
51+
52+
## Conventions
53+
54+
Most skills assume:
55+
56+
- Local nf-metro checkout at `~/projects/nf-metro`
57+
- Upstream slug `pinin4fjords/nf-metro` (issues + PR targets)
58+
- CI render preview at `pinin4fjords.github.io/nf-metro/_pr/<N>/`
59+
60+
If your setup differs, substitute in the commands; the conventions are
61+
called out at the top of each `SKILL.md` so the substitution points are
62+
visible.
63+
64+
## Step / Stage nomenclature
65+
66+
Most skills use `## Step N: ...` for numbered procedural sections.
67+
`pipeline-metro-setup` uses `## Stage N: ...` instead — its four stages
68+
are deliberately coarser-grained (a pipeline-integration journey, not a
69+
debugging checklist) and the word choice signals that.

‎.claude/skills/fix-issue/SKILL.md‎

Lines changed: 196 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,35 @@
11
---
22
name: fix-issue
3-
description: End-to-end workflow for fixing GitHub issues on the nf-metro repo. Use when the user references a GitHub issue (by number, URL, or description) and wants it fixed. Handles worktree setup, environment creation, implementation, testing, visual review, and PR creation. Trigger on phrases like "fix issue #N", "address #N", "work on issue N", or any request to fix a bug or implement a feature that references an issue.
3+
description: End-to-end workflow for fixing GitHub issues on the nf-metro repo with diagnostic rigor. Use when the user references a GitHub issue (by number, URL, or description) and wants it fixed. Handles worktree setup, environment creation, diagnostic-first investigation, invariant-test-first implementation, runtime validators, /simplify pass, full-repo lint, visual review via render preview, narrow-the-fix iteration on regressions, additive-only PR hygiene (no force-push, no narrative comments), origin verification after every push, and PR creation. Trigger on phrases like "fix issue #N", "address #N", "work on issue N", or any request to fix a bug or implement a feature that references an issue. For shepherding a chain of already-existing PRs back to main, see `pr-chain-vet` instead.
44
---
55

66
# Fix Issue
77

88
Structured workflow for fixing nf-metro GitHub issues in an isolated worktree.
9+
Emphasises diagnostic-first investigation, invariant tests before code, and
10+
additive-only PR hygiene so a fix never silently regresses the gallery.
911

10-
## Phase 1: Understand the Issue
12+
**Conventions** (substitute if your setup differs):
13+
- Local nf-metro checkout: `~/projects/nf-metro`
14+
- Issues + PRs target the canonical upstream `pinin4fjords/nf-metro`. If
15+
you're working from a fork, resolve the owner with
16+
`gh repo view --json owner -q .owner.login`.
17+
- micromamba: `/opt/homebrew/bin/micromamba` (macOS Apple Silicon codesign
18+
workaround). On other platforms, just `micromamba` if it's on PATH.
19+
20+
## Step 1: Understand the Issue
1121

1222
```bash
1323
gh issue view <N> --repo pinin4fjords/nf-metro
1424
```
1525

1626
Summarize the problem and proposed approach. Wait for user confirmation before proceeding.
1727

18-
## Phase 2: Worktree + Environment Setup
28+
## Step 2: Worktree + Environment Setup
1929

2030
```bash
2131
# Worktree
22-
cd /Users/jonathan.manning/projects/nf-metro
32+
cd ~/projects/nf-metro
2333
git fetch origin main
2434
git worktree add /tmp/nf-metro-fix-<N> -b fix/<N>-<slug> origin/main
2535

@@ -31,31 +41,109 @@ pip install -e "/tmp/nf-metro-fix-<N>[docs]"
3141

3242
All subsequent work happens inside `/tmp/nf-metro-fix-<N>`.
3343

34-
## Phase 3: Implement the Fix
44+
## Step 3: Diagnostic Before Fix
45+
46+
**Do not propose fixes from hypotheses.** Reproduce the symptom in numbers
47+
before writing any code:
48+
49+
1. Render the affected example(s) on the current `main` (the before-state).
50+
2. Inspect the rendered SVG: read the actual coordinates / element
51+
attributes that are wrong. Print them, log them, eyeball them.
52+
3. Restate the bug as "element X has property P=<observed>, expected
53+
P=<target>" - a concrete numeric or structural claim. If you can't state
54+
the bug this way, you don't understand it yet; keep digging.
55+
56+
Only after the symptom is pinned down to specific numbers should you reason
57+
about which layout pass / function produced them.
58+
59+
## Step 4: Write the Invariant Test FIRST
60+
61+
Before any production code change:
62+
63+
1. Write a test that encodes the invariant the bug violates (e.g. "no two
64+
stations share a grid cell", "trunk centre is symmetric about the fan
65+
midpoint"). Place it under `tests/`, ideally extending the layout
66+
invariants suite.
67+
2. **Parametrise the test over multiple fixtures**, not a single `.mmd`.
68+
The existing `test_layout_invariants.py` historically over-relies on
69+
`da_pipeline.mmd`; new invariants should be exercised against several
70+
gallery fixtures so they generalise.
71+
3. Run the test and **verify it fails on `main`**. If it passes, the test
72+
doesn't actually encode the bug - rewrite it.
73+
4. Now write the fix.
74+
5. Re-run the test and verify it passes.
75+
76+
This guarantees the test is meaningful (it caught the bug) and the fix is
77+
meaningful (the test now passes because of the fix, not coincidence).
78+
79+
## Step 5: Add a Runtime Validator
80+
81+
Where the invariant is about layout properties that could regress silently
82+
(overlap, off-grid placement, asymmetry, etc.), also add a `_guard_*`
83+
function and wire it into `compute_layout`'s validate block.
84+
85+
Validators must **fail loudly** - raise with a clear, contextual error
86+
message. Silent warnings or `print()`s are not acceptable; they get
87+
ignored. The runtime check protects future changes; the unit test pins the
88+
current behaviour.
89+
90+
## Step 6: /simplify Pass
3591

36-
The shell cwd resets after each Bash call. Always chain `cd` into the worktree:
92+
After the fix and tests are passing, invoke the `simplify` Skill on the
93+
changed code. Apply its suggestions and commit as a **separate** commit:
94+
95+
```
96+
refactor: tighten <area> after fix for #<N>
97+
```
98+
99+
Keeping `fix:` and `refactor:` commits separate makes the fix itself easy
100+
to review and easy to revert in isolation if regressions surface.
101+
102+
## Step 7: Whole-Repo Lint
103+
104+
CI lint scans the entire repository, not just `src/` and `tests/`.
105+
Pre-existing mis-formats in scripts, docs config, etc. will trip CI on
106+
your PR even though they predate your change.
37107

38108
```bash
39-
source ~/.local/bin/mm-activate nf-metro-fix-<N> && cd /tmp/nf-metro-fix-<N> && ruff format src/ tests/ && ruff check src/ tests/ && pytest
109+
source ~/.local/bin/mm-activate nf-metro-fix-<N> && cd /tmp/nf-metro-fix-<N> && ruff format . && ruff check .
40110
```
41111

42-
Fix any failures before proceeding.
112+
Run from the repo root, no path restriction. Fix or commit any deltas
113+
that appear (a separate `style: ruff format whole repo` commit is fine).
114+
Then run the test suite:
43115

44-
## Phase 4: Visual Review
116+
```bash
117+
pytest
118+
```
119+
120+
## Step 8: Visual Review via Render Preview
45121

46-
### Primary method: CI render preview (recommended)
122+
### Primary method: CI render preview (authoritative)
47123

48-
Push the branch and create a PR. The CI workflow (`.github/workflows/pr-renders.yml`) automatically renders all gallery examples on both the PR branch and base, generates a before/after visual diff page, and posts a sticky comment on the PR with the preview link:
124+
Push the branch and create a PR. The CI workflow
125+
(`.github/workflows/pr-renders.yml`) automatically renders all gallery
126+
examples on both the PR branch and base, generates a before/after visual
127+
diff page, and posts a sticky comment on the PR with the preview link:
49128

50129
```
51130
https://pinin4fjords.github.io/nf-metro/_pr/<PR_NUMBER>/
52131
```
53132

54-
This is the authoritative visual review. After creating the PR in Phase 5, point the user to the render preview link for review.
133+
### Render-preview verdict gating
134+
135+
The sticky comment ends in a verdict line. Gate the next step on it:
136+
137+
- **"No visual changes detected"** -> eligible for auto-merge under the
138+
user's standing authorisation for no-change PRs. Proceed.
139+
- **"Ready for review"** (or any wording indicating visual deltas exist)
140+
-> **STOP**. Surface the deltas to the user with one short line per
141+
affected gallery example describing what changed (e.g.
142+
`da_pipeline.mmd: trunk shifted 12px right`). Do not auto-merge.
55143

56144
### Optional: quick local render of a single file
57145

58-
For a fast sanity check of one specific `.mmd` file before pushing, render it locally:
146+
For a fast sanity check of one specific `.mmd` file before pushing:
59147

60148
```bash
61149
source ~/.local/bin/mm-activate nf-metro-fix-<N>
@@ -64,42 +152,123 @@ python -c "import cairosvg; cairosvg.svg2png(url='/tmp/<name>.svg', write_to='/t
64152
open /tmp/<name>.png
65153
```
66154

67-
This is useful for quick iteration but does not replace the full CI gallery review.
155+
Useful for quick iteration but does not replace the full CI gallery
156+
review.
68157

69158
### Optional: local before/after comparison
70159

71-
If you need a before/after comparison before pushing (e.g. risky change, user wants early feedback), use the `/render-topologies` skill.
160+
For a before/after sweep before pushing, use the `/render-topologies`
161+
skill.
72162

73-
## Phase 5: Commit and PR
163+
## Step 9: Narrow Over-Applying Fixes
74164

75-
Once tests pass:
165+
If the render preview shows the fix changed **more than the targeted
166+
example** unexpectedly, do not ship it as-is. For each affected example,
167+
classify the visual delta as one of:
168+
169+
- **I** (improvement) - keep
170+
- **N** (neutral) - keep
171+
- **D** (detrimental) - must be narrowed
172+
173+
For each detrimental delta, find the **precondition** that distinguishes
174+
the target case (where the fix helps) from the regressing case (where it
175+
hurts). Gate the fix on that precondition (e.g. a topology predicate, a
176+
config flag, a layout property test) so it only fires when applicable.
177+
Re-render and re-verify the verdict before merging.
178+
179+
A fix that ships with even one unaddressed D-delta is not finished.
180+
181+
## Step 10: Commit, Push, Verify Origin
182+
183+
Open the PR:
76184

77185
```bash
78186
cd /tmp/nf-metro-fix-<N>
79187
gh pr create --repo pinin4fjords/nf-metro --base main --title "<title>" --body "$(cat <<'EOF'
80188
## Summary
81-
<bullets>
189+
<bullets describing the aggregate diff against main, no narrative>
82190
83191
Fixes #<N>
84192
85193
## Test plan
86-
- [ ] pytest passes
87-
- [ ] ruff check clean
194+
- [ ] pytest passes (including new invariant test)
195+
- [ ] ruff check + ruff format clean on whole repo
196+
- [ ] Runtime validator added (if applicable)
88197
- [ ] Visual review of [render preview](https://pinin4fjords.github.io/nf-metro/_pr/<PR_NUMBER>/)
198+
- [ ] Render-preview verdict: <No visual changes | deltas classified I/N>
89199
90-
🤖 Generated with [Claude Code](https://claude.com/claude-code)
200+
Generated with Claude Code
91201
EOF
92202
)"
93203
```
94204

95-
After CI posts the render preview link, ask the user to review it.
205+
After every `git push`, **verify origin HEAD matches local**:
206+
207+
```bash
208+
gh pr view <PR_NUMBER> --json headRefOid -q .headRefOid
209+
git rev-parse HEAD
210+
```
211+
212+
The two must match. Past agents have lost commits to silent push
213+
failures; do not skip this check.
214+
215+
### Additive only - no force-push, ever
216+
217+
The local pre-push hook blocks force-pushes for a reason. To undo
218+
anything, use `git revert <hash>` and push the revert as a new commit.
219+
Never rewrite shared history (no `--force`, no `--force-with-lease`, no
220+
interactive rebase on a pushed branch). This applies even when "it would
221+
be cleaner" - cleanliness is not worth the risk of an agent silently
222+
dropping work.
96223

97-
## Phase 6: Cleanup
224+
### Narrative belongs in the PR description, not in comments
98225

99-
Offer to clean up (only if user agrees):
226+
Do not post explanatory comments on the PR walking through what changed,
227+
what was tried, or what was reverted. Edit the PR description instead:
100228

101229
```bash
102-
cd /Users/jonathan.manning/projects/nf-metro
103-
git worktree remove /tmp/nf-metro-fix-<N>
104-
/opt/homebrew/bin/micromamba env remove -n nf-metro-fix-<N> -y
230+
gh pr edit <PR_NUMBER> --body-file /tmp/pr-body.md
105231
```
232+
233+
The description should be a standalone summary of the current state of
234+
the diff against main - not a chronology of how the PR got there.
235+
236+
If narrative comments already exist (yours or a prior agent's), sweep
237+
them via the GraphQL `deleteIssueComment` mutation. **Keep** the CI
238+
sticky render-preview comment.
239+
240+
## Step 11: Drive End-to-End
241+
242+
A fix-issue session is not done when `/simplify` returns control to the
243+
parent, or when the local tests pass. It is done when:
244+
245+
1. Commits are pushed.
246+
2. Origin HEAD verified against local.
247+
3. CI is green on the final commit.
248+
4. Render-preview verdict is captured and gated on per Step 8.
249+
5. PR description is standalone (per Step 10).
250+
251+
Do not hand back to the user partway through this list saying "the
252+
simplify pass is done" or "tests pass locally". Carry the work all the
253+
way to a reviewable PR.
254+
255+
## Step 12: Post-Merge Cleanup
256+
257+
Once the PR merges, do cleanup operations **in this order** to avoid
258+
GitHub auto-closing dependent PRs:
259+
260+
1. **Retarget any child PRs** based on this branch over to `main` (or
261+
the next-up base) **first**, via `gh pr edit <child> --base main`.
262+
GitHub auto-closes PRs whose base branch is deleted; closed PRs whose
263+
base ref no longer exists cannot be reopened without restoring the
264+
deleted branch.
265+
2. Delete the **remote** branch: `git push origin --delete fix/<N>-<slug>`
266+
(or via the GitHub UI's auto-delete on merge).
267+
3. Remove the local worktree: `git worktree remove /tmp/nf-metro-fix-<N>`.
268+
4. Delete the local branch: `git branch -D fix/<N>-<slug>`.
269+
5. Remove the conda env: `/opt/homebrew/bin/micromamba env remove -n nf-metro-fix-<N> -y`.
270+
271+
Offer this cleanup to the user; only run it after they agree.
272+
273+
For shepherding a whole stacked chain of PRs back into `main` (rather
274+
than a single issue fix), see `pr-chain-vet`.

0 commit comments

Comments
 (0)