| name | fix-issue |
| 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, a reused persistent env (no per-issue env creation), diagnostic-first investigation, authoring-mistake-vs-engine-bug triage (never dodge an engine bug by doctoring the reproducer), invariant-test-first implementation, runtime validators, evidence-cited fix verification, /simplify pass, full-repo lint, visual review via render preview, narrow-the-fix iteration on regressions, cost discipline (targeted tests, CI render-diff over local gallery rebuilds, skip-ci on WIP), scope discipline that fixes adjacent fallout in-session via delegated subagents instead of deferring it to a child issue, standalone issue-body hygiene, additive-only PR hygiene (no force-push, no narrative comments), clean execution of an authorised admin-merge (preserve history, no CI re-run), origin verification after every push, and PR creation. Supports an optional autonomous / net-negative mode (activated by "no deferrals", "work overnight", "net-negative issues", "drive it to conclusion", or "push a complete fix and address all the fallout") that suspends the multi-session-deferral carve-out and drives every surfaced fallout issue to a landed, reviewable PR the same session, while still merging only what the user authorises per-PR. Trigger on phrases like "fix issue |
Fix Issue
Structured workflow for fixing nf-metro GitHub issues in an isolated worktree.
Emphasises diagnostic-first investigation, invariant tests before code, and
additive-only PR hygiene so a fix never silently regresses the gallery.
Communication: keep status updates terse and lead any explanation of a
mechanism or a render with one plain-English sentence before the code or
coordinates. Prefer a narrow table to a wide one. When asked to "explain
simply" or for "less words", cut - don't re-expand.
Conventions (substitute if your setup differs):
- Local nf-metro checkout:
~/projects/nf-metro
- Issues + PRs target the canonical upstream
seqeralabs/nf-metro. If
you're working from a fork, resolve the owner with
gh repo view --json owner -q .owner.login.
- micromamba:
/opt/homebrew/bin/micromamba (macOS Apple Silicon codesign
workaround). On other platforms, just micromamba if it's on PATH.
Scope discipline: fix the fallout, don't defer it
This skill has historically erred toward "that's a separate issue" the moment
a fix surfaces a second problem - a related engine bug, a coverage gap, a
stale test, a lint violation in a file already open, even one in a
completely different subsystem. That default is flipped: resolving
fallout within this same session is the default; filing an issue and leaving
it for a future session is the exception. A queue of maybe-someday child
issues is administrative burden on the user, not a scope win - re-triaging
each one later costs more than fixing it now while the context is already
loaded.
- When diagnosis, implementation,
/simplify, lint, or CI turns up an
adjacent, fixable problem anywhere in the repo - including in a different
subsystem than the one you're already editing - fix it in this session
rather than filing it and moving on. Being in a different subsystem,
needing its own worktree, or requiring you to load unfamiliar code is
not, by itself, a reason to defer.
- Use the
Agent tool to protect your own context budget while doing this,
not as a reason to skip it. Always pass an explicit model param on the
Agent call - never omit it and let the child silently inherit the
session's model. Omitting model is not "no decision was made", it's
"opus got picked by default without anyone deciding that." Default to
model: "sonnet"; set model: "opus" only when you decide before
spawning that the triage or fix genuinely needs harder judgment, and say
so in one line when you do. If you notice after the fact that a child ran
on opus with no model param set, that's a mistake to correct (restart it
on sonnet), not a choice to defend - don't rationalize keeping it because
the task turned out to suit opus in hindsight; that judgment call has to
happen at spawn time, explicitly, or not at all. Run agents in the
background (or a separate worktree, per the global worktree rule, if the
fallout touches files you're not actively editing) and fold results back
in before the session ends. Independent pieces of fallout can run as
concurrent subagents instead of serially eating your own context - that's
what makes taking on a different subsystem's fix tractable in the same
session.
- "Resolved in this session" does not mean "crammed into one PR." If folding
the fallout into the primary PR would make it harder to review, ship it as
its own sibling PR instead. The bar is that the fallout's PR gets built,
pushed, and left in a reviewable state before the session ends - not that a
future session has to rediscover it from a filed issue.
- Reserve an actual filed issue + deferral for cases that are genuinely
disproportionate: a fix that would take multiple sessions to complete in
its own right, or something that needs a decision only the user can make.
Size and duration are the test, not which subsystem the fallout lives
in. When you do defer, say so explicitly and why - deferral should be a
stated judgment call, never the silent default.
- This is not licence for scope creep into features the user didn't ask
about - it's about not walking away from problems the current work
surfaced.
This governs every step below: the "second finding" in Step 3, coverage gaps
in the Step 7 gate-coverage ratchet, and anything /simplify or review turns
up in Step 6 all default to fix-now-via-subagent (in this PR or a sibling
one) over file-and-defer.
Optional mode: autonomous / net-negative
Activate this mode when the user signals it - "no deferrals", "no giving up",
"drive it to conclusion", "net-negative issues", "work overnight", "I'll leave
you to it", or an explicit "push a complete fix and address all the fallout".
It does not relax the diagnostic or verification rigour of the steps below; it
changes two defaults.
1. The deferral exception is suspended. The "genuinely disproportionate /
multi-session" carve-out above does not apply in this mode. A coupled or hard
defect, and any incident issue the work surfaces (a second engine bug, a
seam/reversal soundness gap, a hygiene refactor /simplify flags), is driven
to a landed, reviewable PR this session - not filed and left. Delegate the
heavy work to agents (one own worktree each, explicit model, run in the
background so independent fallout runs concurrently), and give each a hard
acceptance bar rather than an open question: "clean render AND byte-identical
corpus verified via real render_svg() md5 hashes; iterate until both hold;
do not report not-bounded." The CI render-diff is the vetting gate that makes a
corpus-risky change to shared code safe to ship autonomously - a byte-identical
corpus plus a green render-diff is the proof, so you are free to touch a shared
handler as long as that holds. When a first agent reports "blocked / not
bounded", re-brief it with the diagnosis it produced and push through; a
thorough diagnosis is the start of the fix, never a substitute for it.
2. Net-negative open-issue count. The session must END with fewer open
issues than it started. Never open an issue and stop: opening one only to close
it via a same-session PR is fine (net zero on that issue); leaving it OPEN at
session end is the failure mode this mode exists to prevent. If you file a
child issue at all, you owe its resolution - an agent, a sibling PR - the same
session. Track the arithmetic explicitly at the end: issues closed vs. issues
opened-and-still-open; the second number must be 0, or the total must go down.
Bounded to the fix's fallout, not a frontier. This mode resolves what the
current work surfaces - a coupled defect, an incident bug, a /simplify
refactor of code you just touched - with a proportionate handful of agents. It
is not licence to keep pulling threads into a full-engine overhaul or a cascade
of dozens of child sessions. If a surfaced item is itself a large program - a
subsystem rewrite, a sweeping cross-cutting refactor, anything that would
genuinely need many agents or several sessions in its own right - that is the
one case that still stops: flag it, size it, and get a user decision rather
than silently recursing into it. The bar is "finish the thing I was asked to
fix, plus the debris it kicked up", not "fix everything adjacent". Deferral
returns here, and only here - narrowed to genuinely large programs, never to
"this is hard" or "it is a different subsystem".
Still in force in this mode - do not over-read "autonomous":
- Never merge without explicit per-PR authorisation from the user. Autonomy is
about resolving and pushing complete work, not self-authorising merges.
"Drive to conclusion" means "get every PR green and reviewable"; merge only
what the user okays, per-PR (Step 8 / Step 12). Pre-authorisation to work
overnight is not pre-authorisation to merge.
- Every step's rigour below still applies to every PR, including the fallout
ones: diagnostic-first, invariant-test-first,
/simplify, evidence-cited
verification, additive-only pushes, origin check, standalone issue bodies.
- Verify each delegated agent's result yourself - re-run the byte-identical
corpus diff with real renders, confirm the test fails without the fix - and
eyeball the actual render; do not merge a claim.
- The one acceptable open-at-end issue is one that genuinely needs a decision
only the user can make - and then you say so explicitly and get agreement,
you do not default to it.
Step 1: Understand the Issue
gh issue view <N> --repo seqeralabs/nf-metro
Summarize the problem and proposed approach. Wait for user confirmation before
proceeding - unless the user has pre-authorised autonomous work (the optional
mode above; e.g. "I'll leave you to work overnight, drive it to conclusion"),
in which case proceed without blocking and report as you go.
Issue hygiene
Every issue is run through this skill fresh in a later session, so the
issue body must be standalone and self-contained. When you learn
something during the fix that a future session would need (the real cause, a
repro, a constraint), fold it into the issue body - do not scatter it
across comments, and do not leave superseded-approach detail that would
mislead a fresh reader. Keep the body concise. If the fix uncovers a
genuinely separable defect, default to fixing it in this session (see
"Scope discipline" above - delegate to a subagent if it would crowd your
diagnostic context, even for a different subsystem) rather than filing a
child issue and walking away. File a standalone child issue only when the
defect is a multi-session undertaking in its own right, and say so
explicitly.
Step 2: Worktree + Environment Setup
cd ~/projects/nf-metro
git fetch origin main
git worktree add /tmp/nf-metro-fix-<N> -b fix/<N>-<slug> origin/main
All subsequent work happens inside /tmp/nf-metro-fix-<N>.
Environment: reuse one persistent env, don't create one per issue
nf-metro is pure Python; the deps (cairo, drawsvg, networkx, pillow,
cairosvg, pytest, ruff, mypy, types-networkx) change rarely. Creating a
fresh micromamba env per issue re-solves and re-downloads all of that
every session for no benefit. Keep one long-lived deps env and point it
at the worktree's code per-command:
ulimit -n 1000000 && export CONDA_OVERRIDE_OSX=15.0 && /opt/homebrew/bin/micromamba create -n nf-metro-dev python=3.11 cairo -y
source ~/.local/bin/mm-activate nf-metro-dev
pip install "drawsvg" "networkx" "pillow" "cairosvg" "pytest" "pytest-xdist" "ruff" "mypy" "types-networkx" "click"
Then run the worktree's code by prepending its src/ to PYTHONPATH on
each command - do not pip install -e the worktree into this env:
source ~/.local/bin/mm-activate nf-metro-dev
cd /tmp/nf-metro-fix-<N>
export PYTHONPATH=/tmp/nf-metro-fix-<N>/src
python -m nf_metro render <file.mmd> -o /tmp/out.svg
python -m pytest -k <selector>
Why per-command PYTHONPATH, not editable install: an editable install
binds one env's site-packages to exactly one worktree path, so it collides
the moment you run two worktrees in parallel. PYTHONPATH is set per command
and shadows whatever is installed, so any number of parallel worktree
sessions share the single nf-metro-dev env with zero cross-talk. (If you
genuinely want an isolated editable install for one worktree, dedicate a
separate env to it - never editable-install a shared env against a
worktree.)
Commit hooks need the tools on PATH in the same Bash call: the repo
uses prek (config prek.toml, not pre-commit), whose mypy hook is
language: system and so needs mypy on PATH. Shell state does not
persist between Bash calls, so run the commit as one call with the env
activated: source ~/.local/bin/mm-activate nf-metro-dev && cd <worktree> && PRE_COMMIT_ALLOW_NO_CONFIG=1 git commit ....
Step 3: Diagnostic Before Fix
Do not propose fixes from hypotheses. Reproduce the symptom in numbers
before writing any code:
- Render the affected example(s) on the current
main (the before-state).
- Inspect the rendered SVG: read the actual coordinates / element
attributes that are wrong. Print them, log them, eyeball them.
- Restate the bug as "element X has property P=, expected
P=" - a concrete numeric or structural claim. If you can't state
the bug this way, you don't understand it yet; keep digging.
Only after the symptom is pinned down to specific numbers should you reason
about which layout pass / function produced them.
Check your premise against current origin/main first
Diagnose against latest remote, not a stale tree. git fetch origin main and
confirm the bug still reproduces on latest before reasoning about a cause -
a sibling PR may already have fixed it or changed the very code you're reading.
If the user says something is already addressed, re-fetch and look again before
disagreeing; "I'm looking at outdated code" is a recurring wrong turn. If a
related PR merges mid-session, git merge origin/main into the worktree and
re-diagnose before continuing.
Classify: authoring mistake or engine bug?
Before touching anything, decide which of two things you're looking at:
- (a) An mmd authoring mistake - the
.mmd misdescribes the pipeline
(wrong line on a station, a missing edge, a bad directive). The fix is to
edit the input. probe_layout.py labels many of these ("authoring
mistakes vs engine bugs"); nf-metro explain shows the rule each inferred
decision followed.
- (b) An engine bug on correct mmd - the input faithfully describes the
pipeline and the engine lays it out badly. The fix goes in
src/
(layout / routing / parser). The reproducing .mmd stays untouched.
State which one it is, in numbers, before writing code.
Once it's an engine bug, the reproducer is frozen evidence
Never "fix" an engine bug by editing the input to dodge the bad layout.
Do not simplify the reproducing .mmd's labels, drop stations, reorder
lines, or add directives to make the ugly render go away. That changes the
question instead of answering it, and ships a false fix. The map is correct;
the engine must handle it.
This applies to fixtures you author, too: when building a new regression
fixture, don't file it down to sidestep a second bad render you notice while
constructing it (e.g. shortening a multi-line label so a label-interaction
bug won't show). A second bad render is a second finding - per "Scope
discipline" above, default to fixing it in this session (spin up a subagent
to diagnose/fix it in parallel rather than letting it derail your primary
diagnosis, even if it lands in an unrelated part of the engine) rather than
just noting it and moving on. Only file it standalone and defer if it is
genuinely a multi-session undertaking on its own. A fixture that has
been quietly simplified to look clean no longer locks the bug it was meant to
lock. (Real example: a "ORF quant" -> "ORFquant" relabel that hid a
multi-line-label interaction rather than reporting it.)
The only legitimate input edits during an engine fix are: authoring a
faithful new reproducer, or correcting a genuine (a)-class authoring
mistake you've identified as the actual cause.
This rule holds for the whole fix, not just the freeze moment
The pull toward touching the .mmd instead of src/ doesn't only show up
when you first classify the bug - it resurfaces later, when the code fix
turns out to be harder than expected: trimming a label, dropping a station,
reordering lines, splitting a section, or adding a directive so the
reproducer renders cleanly while the engine still mishandles the general
case. That is the same workaround wearing the disguise of "polish" or
"narrowing." Step 9's narrowing means gating the code path on a real
structural precondition (a topology predicate, a config flag) - never
gating it by rewording the input so the bad path isn't exercised anymore.
If you catch yourself thinking "what if I just changed the mmd instead"
partway through implementation, treat that as the signal to stop and go
fix the engine, not as a shortcut worth taking.
Diagnostic tooling
The repo bundles two scripts that do exactly this render-and-read-the-numbers
work, usable for any layout issue regardless of how it was reported:
python .claude/skills/nf-metro-stress-render/scripts/probe_layout.py <file.mmd> --json
python .claude/skills/nf-metro-stress-render/scripts/inspect_layout.py <file.mmd>
Plus nf-metro explain <file.mmd> (the rule behind each inferred layout
decision) and nf-metro info --json (the structural model). These are
conveniences, not requirements - any way you pin the bug to numbers is fine.
If the issue happens to have been filed by the nf-metro-stress-render skill,
it carries a correct-by-construction repro .mmd in a <details> fold in the
issue body - start from that rather than re-deriving one. Most issues won't have
this; in that case build the reproducer yourself as usual.
Step 4: Write the Invariant Test FIRST
First, check for an existing regression lock
Most issues arrive bare and you write the failing test yourself (skip to the
numbered steps below). But some - notably those filed by the
nf-metro-stress-render skill - arrive with their regression infra already
in place: a fixture in examples/topologies/, a GALLERY_ENTRIES row in
scripts/build_gallery.py, and a strict=True xfail test referencing the issue
number. Grep before you write anything:
grep -rn "#<N>" tests/ scripts/build_gallery.py examples/topologies/
- If a strict-xfail lock exists, that is your failing test - don't write a
duplicate, and don't re-add the fixture or gallery entry. Confirm it xfails on
the current tree (it documents the live defect).
- Completing the fix flips that strict-xfail to XPASS, which reds CI - that
is the signal the bug is actually fixed. Finish by removing the
xfail
marker so the now-passing assertion becomes a permanent positive guard.
(Deleting the whole test loses the guard; leaving the marker keeps CI red.)
- If no lock exists (the common case), proceed with the steps below.
xfail is a lock on a known bug, not an escape hatch
The strict=True xfail pattern above exists for issues that arrive with
regression infra already wired in - it locks a bug that's already filed and
tracked. It is not a tool for this session to reach for when the real fix
turns out to be harder than expected. Do not mark a new test xfail
(strict or otherwise) so you can move on and leave the actual bug for a
future session to untangle - that's the same deferral "Scope discipline"
above already rejects, just spelled with a pytest decorator instead of a
filed child issue. If a test you write in the steps below won't pass, that
means the fix isn't done yet; keep going. It does not mean the test should
be muted.
The only acceptable use of a new xfail is the genuine multi-session case
"Scope discipline" already covers: the underlying fix is disproportionate to
this issue, you've said so explicitly, and you're filing a follow-up issue
for it - the xfail marker is a byproduct of that stated deferral, not a
substitute for making it. Treat "I'll xfail this for now" as the same red
flag as "I'll simplify the mmd for now": both quietly convert a bug the
engine has into a fixture that dodges it instead of one that gets fixed.
Before any production code change:
- Write a test that encodes the invariant the bug violates (e.g. "no two
stations share a grid cell", "trunk centre is symmetric about the fan
midpoint"). Place it under
tests/, ideally extending the layout
invariants suite.
- Parametrise the test over multiple fixtures, not a single
.mmd.
The existing test_layout_invariants.py historically over-relies on
da_pipeline.mmd; new invariants should be exercised against several
gallery fixtures so they generalise.
- Run the test and verify it fails on
main. If it passes, the test
doesn't actually encode the bug - rewrite it.
- Now write the fix.
- Re-run the test and verify it passes.
This guarantees the test is meaningful (it caught the bug) and the fix is
meaningful (the test now passes because of the fix, not coincidence).
Step 5: Add a Runtime Validator
Where the invariant is about layout properties that could regress silently
(overlap, off-grid placement, asymmetry, etc.), also add a _guard_*
function and wire it into compute_layout's validate block.
Validators must fail loudly - raise with a clear, contextual error
message. Silent warnings or print()s are not acceptable; they get
ignored. The runtime check protects future changes; the unit test pins the
current behaviour.
Step 6: /simplify Pass
After the fix and tests are passing, invoke the simplify Skill on the
changed code. Apply its suggestions and commit as a separate commit:
refactor: tighten <area> after fix for #<N>
Keeping fix: and refactor: commits separate makes the fix itself easy
to review and easy to revert in isolation if regressions surface.
Re-running it later: /simplify is expensive, so don't re-run it after
every follow-up commit. Only re-run it on the final aggregate diff if later
steps (narrowing a regression, lint/mypy fixes) added a substantial chunk
of new production code the first pass never saw. A couple of small,
already-clean follow-up edits don't warrant a second pass.
Step 7: Lint and Tests
The repo's prek hooks (config prek.toml) run on every git commit: ruff
check/format on src/ and tests/, mypy, trailing whitespace, yaml. If a
commit fails on a hook, fix the issue and re-commit. Never skip hooks with
--no-verify.
To run the checks without committing (needs prek, which lives on the
nf-core env, plus a stub-complete mypy):
micromamba run -n nf-core prek run --all-files
Then run the targeted tests for the area you changed - not the full
suite by default:
cd /tmp/nf-metro-fix-<N> && PYTHONPATH=src python -m pytest tests/test_layout_invariants.py -k "<fixture-or-invariant>" -q --no-header
CI already runs the complete suite across three Python versions x three
shards on every push (the test matrix in .github/workflows/ci.yml), so a
routine full local run mostly duplicates work CI is about to do anyway - if
something's broken, the push reds just as loudly, without paying for a local
pass first. Reserve a full local run for when it earns its cost:
- The change is risky - it touches shared/widely-used code (
engine.py
orchestration, a helper called from many phases, the parser's core model,
a shared dispatch table) where a narrow selector genuinely can't see the
blast radius. Here a full local run is worth it precisely because it's
faster than CI: half a minute locally beats waiting on the full
three-version x three-shard matrix to come back red on something a local
run would have caught immediately.
- You're about to ask for an admin-merge and want that confidence before
bypassing the review gate, not after.
- A targeted run passed but you have a concrete reason to suspect a wider
regression it wouldn't reproduce (e.g. you touched a shared constant or a
cross-cutting guard used well outside the issue's area).
For an ordinary fix scoped to one function, one routing handler, or one
layout phase, targeted tests (plus the gate-coverage ratchet below, if the
change is in layout/routing/) is enough - let CI's matrix be the
full-suite gate rather than re-running it locally "just to be safe."
Cost discipline (applies throughout)
Layout iteration is where sessions burn tokens and compute. Keep it tight:
- Reuse the persistent env (Step 2). Do not
micromamba create per
issue - it re-solves the whole dependency set every session for nothing.
- Full suite vs targeted. Default to the narrowest selection
(
python -m pytest tests/test_layout_invariants.py -k "<fixture-or-invariant>",
then --lf to re-run only what just failed), with -q --no-header -x to
keep the output tiny. Don't run the full suite locally as a routine
pre-push step - see the reserved-cases list above; when none of them
apply, skip straight to pushing and let CI's matrix be the full-suite
check. addopts bakes in -n auto so a full run is cheap in
wall-time (~half a minute), but the cost that matters here is
repetition - each run's summary re-entering context - so don't pay it
when CI is about to run the same suite for free. The routing/TB ratchets
are 3.11-only, so if you do run locally, keep the env on 3.11 or they
skip and only red in CI.
- Read coordinates, don't rasterize, for non-visual questions.
inspect_layout.py / probe_layout.py print the geometry as cheap text;
a render -> cairosvg PNG -> open -> image-into-context cycle is far heavier
and only earns its cost for a genuine visual check. "Is station X on the
trunk?" is a coordinate read, not a screenshot.
- Poll CI once, in the background. A single background watch
(
until gh pr checks <N> ...; done) pulls you back when checks resolve;
re-running gh pr checks by hand each turn just dumps status into context
repeatedly.
- Lean on the CI render-diff for regression review; don't rebuild the
gallery locally in a loop. The CI preview (Step 8) is the authoritative
whole-corpus diff. A local
build_gallery / render-diff sweep repeated
many times just duplicates it. Local rendering is for a single file's
quick sanity check.
- Read the big layout files in wide slices and stay oriented.
Re-fetching
engine.py / fan_bundles.py / ordering.py /
routing/* twenty times over a session is the single largest cache-read
cost. Read the region once, generously, and keep it in working context.
- Default
[skip ci] on work-in-progress pushes (WIP snapshots, refactor
passes). Let CI run on the final pre-review push - which this repo needs
anyway, because the render-diff is the visual review. (A commit that
fixes a known CI failure must re-run CI: no [skip ci] on those.)
If your change touched layout/routing/: the gate-coverage ratchet
Adding, removing, or rewriting an if/while in a layout/routing/
module - or adding a topology fixture that closes a gap - can red one of
the three ratchet tests in tests/test_routing_gate_coverage.py. These
are not flaky; each names a specific reconciliation you owe in this
same PR. Do not silence them by hand-editing the baseline or the
generated matrix doc, and do not delete a triage entry just to make a
test pass.
test_no_new_un_exercised_routing_gate_arm - your change added a gate
with an un-exercised arm. Either author a fixture that hits both arms,
or - if the arm is genuinely unreachable - confirm that and regenerate
the baseline to acknowledge it.test_gate_coverage_baseline_in_sync - your change closed a gap or
removed a gate the baseline still lists. Regenerate the baseline.test_triage_sidecar_references_open_gaps - you edited a gate's
condition text or removed it, so its entry in
tests/data/routing_gate_triage.json now names a non-gap. Prune (or
re-key) that entry.
Regenerate with the coverage script (needs the [dev] extra and the
pinned interpreter):
python scripts/routing_gate_coverage.py --write
Gotcha: the arc model is CPython-version-specific, so these tests
skip off the pinned BASELINE_PYTHON (3.11). If your fix env is a
different Python you will not see the failure locally - it surfaces only
in CI. When in doubt, regenerate under 3.11. The full methodology (the
four verdicts, why these tests exist, the phantom-arc trap) is in
docs/dev/routing_gate_triage.md;
for a dedicated triage campaign use the nf-metro-gate-triage skill.
If you added a topology fixture: regenerate the guard-trace golden
Every fixture under examples/topologies/ carries a committed guard-trace
golden at tests/data/guard_golden/examples/topologies/<stem>.json (the
ordered list of which guard fired at which stage). A new fixture has no
golden yet, so tests/test_guard_registry_golden.py reds with
"<stem>.mmd absent from the golden baseline". This is a full-corpus
gate: the targeted tests you run for the fix's own area never touch it, so
it only surfaces when you run that test (or the full suite) or on the push.
Run one of those before pushing rather than eating a red CI cycle.
Regenerate:
NF_METRO_REGEN_GUARD_GOLDEN=1 python tests/test_guard_registry_golden.py
Then check the blast radius with git status: only your new fixture's
.json should appear. A handful of guards sit on a float threshold and fire
differently across architectures, so if the regen also rewrites other
fixtures' goldens you are on arm64 committing an x86_64-divergent trace -
revert those and keep only the new file. CI checks the golden on x86_64, so
when a fixture's trace is genuinely arch-sensitive, regenerate on a Linux
x86_64 box (per the cross-architecture rule).
A new topology fixture therefore owes three committed artifacts, not
one: the .mmd, its GALLERY_ENTRIES/gallery.yaml row (Step 8, so the
render-diff sees it), and this guard-trace golden.
Step 8: Visual Review via Render Preview
Primary method: CI render preview (authoritative)
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:
https://seqeralabs.github.io/nf-metro/_pr/<PR_NUMBER>/
Render-preview verdict gating
The sticky comment ends in a verdict line. Gate the next step on it:
- "No visual changes detected" -> a clean result, but not a
licence to merge. Report the verdict and wait for the user to say
merge. There is no standing auto-merge authorisation.
- "Ready for review" (or any wording indicating visual deltas exist)
-> STOP. Surface the deltas to the user with one short line per
affected gallery example describing what changed (e.g.
da_pipeline.mmd: trunk shifted 12px right).
In all cases, merging is the user's call, made per-PR:
- Never merge until the user explicitly asks for this PR to be merged.
- Never use
gh pr merge --admin (or any other bypass of the
repo's branch-protection / review-required policy) on your own
initiative. If a normal merge is blocked because the repo requires
review, that block is the policy working - stop and tell the user it
needs their review or an explicit instruction to admin-merge. Do not
cite "prior PRs were admin-merged" as authorisation; past instances
are history, not standing consent.
When the user does authorise a merge
Once the user says "merge" / "admin merge" for this PR, that word is the
authorisation - execute it, don't re-litigate. The recurring mistakes (this
is the single most-corrected behaviour across sessions) are all forms of
doing too much:
- Merge with a merge commit, never squash:
gh pr merge <N> --admin --merge --delete-branch. Preserve the branch's commit history;
--squash collapses it and is the wrong default here. (--admin bypasses
only the review gate, not CI - it's fine once CI is green or the unverified
delta is CI-irrelevant.) Omit --delete-branch if a child PR is based on
this branch - deleting it auto-closes the child; retarget children first
per Step 12.
- Don't update the branch first. If GitHub says "head branch is not up
to date", do not
git merge origin/main into it, do not push a commit, do
not "refresh" it - all of these fire a full CI re-run the user is
explicitly trying to avoid. The diff was already CI-validated; a trivial
base-behind is CI-irrelevant, which is exactly what --admin is for.
- Don't re-run or wait on fresh CI, and cancel in-flight runs first if
any (
gh run cancel). "Merge" means merge now, then clean up (Step 12) -
not "start another test cycle".
This is the pinin4fjords:eco-merge philosophy; that skill encapsulates the
"bypass the up-to-date requirement when the unverified delta is
CI-irrelevant" check if you want it. The self-initiation guardrail above
still holds: you never reach for admin-merge on your own; you execute it
cleanly when told.
State the evidence for every "it's fixed" claim
Never assert a fix works without naming what proved it. Every "resolved" /
"this is fixed" / "renders correctly" claim must cite the specific render
and the concrete numbers it was checked against - the file, and the
coordinate or element that moved from the observed value to the target value
you wrote down in Step 3. "I believe it's resolved" with no named render is
not a verdict; it invites the reply "which render did you re-assess on?".
Two traps this closes:
- "Didn't abort" / "the one invariant passes" is not "renders
correctly". Removing an abort can merely expose a poor layout the abort
was masking. After any layout/routing fix, look at the full render (crop
the region and read it) and run
probe_layout + inspect_layout for the
whole-layout picture (crossings, port alignment, column gaps), not only the
invariant you targeted.
- A clean render-diff verdict only covers the gallery corpus. It says
nothing about a NEW fixture that isn't in the gallery yet. Put new
regression fixtures in
scripts/build_gallery.py (GALLERY_ENTRIES), not
only examples/topologies/, so CI's render-diff makes them visible to a
human. A topologies-only or tests-only fixture is invisible in the PR
preview.
Do not present a prototype as an improvement before the user has agreed it
is one. If you rendered it and it still has problems, say so and keep
working; don't defend a weak fix.
Optional: quick local render of a single file
For a fast sanity check of one specific .mmd file before pushing:
source ~/.local/bin/mm-activate nf-metro-dev
export PYTHONPATH=/tmp/nf-metro-fix-<N>/src
cd /tmp/nf-metro-fix-<N> && python -m nf_metro render <file.mmd> -o /tmp/<name>.svg
python -c "import cairosvg; cairosvg.svg2png(url='/tmp/<name>.svg', write_to='/tmp/<name>.png', scale=2)"
open /tmp/<name>.png
Useful for quick iteration but does not replace the full CI gallery
review.
Optional: local before/after comparison
For a before/after sweep before pushing, use the /render-topologies
skill.
Step 9: Narrow Over-Applying Fixes
If the render preview shows the fix changed more than the targeted
example unexpectedly, do not ship it as-is. For each affected example,
classify the visual delta as one of:
- I (improvement) - keep
- N (neutral) - keep
- D (detrimental) - must be narrowed
The bar is "no meaningful visual regression", not pixel-identity. A
subtle spacing or coordinate shift that comes with a cleaner, more elegant
implementation is fine (classify it N or I); do not contort the code to
preserve a byte-identical render. Only a genuine degradation is a D.
For each detrimental delta, find the precondition that distinguishes
the target case (where the fix helps) from the regressing case (where it
hurts). Gate the fix on that precondition (e.g. a topology predicate, a
config flag, a layout property test) so it only fires when applicable.
Re-render and re-verify the verdict before merging.
A fix that ships with even one unaddressed D-delta is not finished.
Step 10: Commit, Push, Verify Origin
Open the PR:
cd /tmp/nf-metro-fix-<N>
gh pr create --repo seqeralabs/nf-metro --base main --title "<title>" --body "$(cat <<'EOF'
## Summary
<bullets describing the aggregate diff against main, no narrative>
Fixes #<N>
## Test plan
- [ ] pytest passes (including new invariant test)
- [ ] ruff check + ruff format clean on whole repo
- [ ] Runtime validator added (if applicable)
- [ ] Visual review of [render preview](https://seqeralabs.github.io/nf-metro/_pr/<PR_NUMBER>/)
- [ ] Render-preview verdict: <No visual changes | deltas classified I/N>
Generated with Claude Code
EOF
)"
After every git push, verify origin HEAD matches local:
gh pr view <PR_NUMBER> --json headRefOid -q .headRefOid
git rev-parse HEAD
The two must match. Past agents have lost commits to silent push
failures; do not skip this check.
Additive only - no force-push, ever
The local pre-push hook blocks force-pushes for a reason. To undo
anything, use git revert <hash> and push the revert as a new commit.
Never rewrite shared history (no --force, no --force-with-lease, no
interactive rebase on a pushed branch). This applies even when "it would
be cleaner" - cleanliness is not worth the risk of an agent silently
dropping work.
An ordinary additive (fast-forward) push is not blocked by that hook -
only rewrites are. Don't mistake an unrelated push failure for a force-push
block, and don't ask the user to run a plain push you can run yourself.
Narrative belongs in the PR description, not in comments
Do not post explanatory comments on the PR walking through what changed,
what was tried, or what was reverted. Edit the PR description instead:
gh pr edit <PR_NUMBER> --body-file /tmp/pr-body.md
The description should be a standalone summary of the current state of
the diff against main - not a chronology of how the PR got there.
If narrative comments already exist (yours or a prior agent's), sweep
them via the GraphQL deleteIssueComment mutation. Keep the CI
sticky render-preview comment.
Step 11: Drive End-to-End
A fix-issue session is not done when /simplify returns control to the
parent, or when the local tests pass. It is done when:
- The fix lands in
src/, not in a doctored reproducer (Step 3), and the
"it's fixed" claim cites the render + numbers that prove it (Step 8).
- Commits are pushed.
- Origin HEAD verified against local.
- CI is green on the final commit.
- Render-preview verdict is captured and gated on per Step 8.
- PR description is standalone (per Step 10).
Do not hand back to the user partway through this list saying "the
simplify pass is done" or "tests pass locally". Carry the work all the
way to a reviewable PR.
Step 12: Post-Merge Cleanup
Once the PR merges, do cleanup operations in this order to avoid
GitHub auto-closing dependent PRs:
- Retarget any child PRs based on this branch over to
main (or
the next-up base) first, via gh pr edit <child> --base main.
GitHub auto-closes PRs whose base branch is deleted; closed PRs whose
base ref no longer exists cannot be reopened without restoring the
deleted branch.
- Delete the remote branch:
git push origin --delete fix/<N>-<slug>
(or via the GitHub UI's auto-delete on merge).
- Remove the local worktree:
git worktree remove /tmp/nf-metro-fix-<N>.
- Delete the local branch:
git branch -D fix/<N>-<slug>.
Leave the shared nf-metro-dev env in place - it is reused across issues
(Step 2), so there is nothing per-issue to remove.
Offer this cleanup to the user; only run it after they agree.
For shepherding a whole stacked chain of PRs back into main (rather
than a single issue fix), see pr-chain-vet.
