Plan Implementation

Plan the Implementation

You have an issue number (from find-work, or one you filed with write-issue) and you're about to implement a change for gastownhall/gascity. This skill front-loads the analysis the maintainer's adoption review will run — so you address the concerns before writing code, when they're cheap.

The output is a written plan. No code is written until the plan is done and you confirm it.

main below means the upstream main you're targeting — origin/main if origin is gastownhall/gascity, else upstream/main.

Phase 1 — Issue analysis

Read the issue; extract what's broken, where it lives, why it matters.

gh issue view <number> --repo gastownhall/gascity --json title,body,labels,comments
gh issue list --repo gastownhall/gascity --search "<keywords>" --state all --limit 10
gh pr list   --repo gastownhall/gascity --search "<keywords>" --state all --limit 10

Competing-PR gate (BLOCKING)

Before anything else, check whether an open PR already targets this issue, or whether it's already fixed:

gh pr list   --repo gastownhall/gascity --state open --search "<issue number>" --json number,title,author,createdAt
gh issue view <number> --repo gastownhall/gascity --json state

If a competing PR exists or the issue is closed, STOP. Pick a different issue or, if you still think yours adds value, say so explicitly in the plan and decide consciously. Don't silently start work someone else is already doing — it wastes effort and creates merge conflicts.

Architectural-refactor gate (BLOCKING)

The competing-PR check catches issue-level duplicates; it misses area-level ones. Before scoping a fix, check whether the file/package you'll touch sits inside an active architectural refactor. If it does, a narrow fix gets superseded and your time is wasted.

# Accepted / Implementing design docs:
grep -lE "^\| Status \|.*\b(Accepted|Implementing|Implemented)\b" engdocs/design/*.md

# Open maintainer PRs consolidating your target area (substitute a keyword
# from the subsystem you're touching; skim hits for any that unify/refactor/
# supersede it):
gh pr list --repo gastownhall/gascity --state open --search "<your-subsystem-keyword> in:title" --json number,title

# Recently-merged PRs that used "Supersedes" (the area is being consolidated):
gh pr list --repo gastownhall/gascity --state merged --search "supersedes in:body" --limit 5 --json number,title

If your area has an Accepted/Implementing design doc OR an open consolidation PR touching it, STOP and choose one of:

• Point-fix — a single-line change the refactor can absorb. Ask in the issue whether that's wanted.
• Wait for the refactor to land, then rebase.
• Pivot to an issue outside the refactor area.

Read the relevant design doc before writing any code. Its Status field and ## Phase N headings tell you which parts are live and which are queued; a fix landing in a queued phase won't survive rebase. (Gas City precedent: a single broad refactor superseded eight narrow session-model PRs; another superseded two env-projection PRs.)

Phase 2 — Blast radius

Map the impact surface with the map-blast-radius skill: enumerate the functions you'll touch, their callers and execution contexts, the config-field sync chain, domain-boundary crossings, and concurrency. Carry its HIGH/MED findings into the plan's Risks and Blast radius sections.

Phase 3 — Convention alignment

Verify the plan follows these patterns before writing code:

Architecture patterns

• do()/cmd() split** — cmdFoo() (wiring) + doFoo() (pure logic, injected deps)
• Provider interfaces — new implementations pass the conformance suite in *test/conformance.go
• Nil-guard tracker — optional subsystems use if tracker != nil; nil means disabled
• Config override chain — city.toml → [agent_defaults] → [[agent]] → AgentPatch → AgentOverride

Test strategy (decide BEFORE writing code)

1. Which tier?
   • "Does the store handle corrupt X?" → unit test
   • "Does gc foo print the right output?" → testscript (.txtar)
   • "Does this work with real tmux?" → integration test (//go:build integration)
   • "Does the provider shim handle this?" → acceptance test (test/acceptance/)
   • "Are components called in the right order?" → coordination test
2. Fakes, not mocks — runtime.NewFake(), beads.MemStore, fsys.NewFake(). No gomock. New fakes next to the interface with var _ Interface = (*Fake)(nil).
3. Error injection — per-path (f.Errors["/path"] = err) or modal (f.Broken = true).
4. Testscript env vars — only GC_SESSION, GC_BEADS, GC_DOLT. More than 2 → unit test.
5. Regression depth — bug fixes test through the full write path; assertions discriminate the exact bug path (B14).

Branch setup

git fetch origin
git checkout -b <prefix>/<issue-number>-<short-desc> origin/main
# prefix: fix/ feat/ refactor/ docs/

Branch from the upstream main you're targeting; don't git checkout main first.

Phase 3.5 — Design-capture decision

Gas City's strongest contributions land architectural work with a design artifact attached — maintainers author the engdocs/design/ canon, and a PR that implements against (or proposes) a design doc clears review in one pass instead of costing a "what's the intent here?" round-trip. This is the single highest-leverage thing you can do to make an architectural PR land cleanly.

Does this change need a design doc?

Write (or update) an engdocs/design/<name>.md when any of these is true:

• It introduces or changes a subsystem boundary or cross-cutting mechanism — read-path/routing, store topology, supervisor or session lifecycle, a provider/worker interface, the config override chain, an event/wire format, the endpoint model.
• It adds a new package or a new public contract/schema other components or packs consume.
• It changes behavior other components depend on — events.jsonl shape, a CLI contract a pack reads, the bd+Dolt contract.
• The work is cohesive feature-scale, not a point fix.

Skip the doc (a code-only PR is correct) when the change is a single-function bug fix, test-only, docs-only, or a behavior-preserving mechanical refactor. Respect KISS/YAGNI — never write a design doc for a one-liner.

Two capture mechanisms — know which is which

• engdocs/design/*.md — forward-looking design proposal. This is the one a contributor authors.
• release-gates/*.md — per-change acceptance contract tied to a builder-fleet deploy. You won't author these, but if your area already has one, cite it the same way you'd cite a design doc.

If the area ALREADY has a design doc or release-gate

You found it in the Phase 1 architectural-refactor gate. Don't start a competing doc — implement against it and cite it by path in the PR body and in the commit that lands the core change (implements engdocs/design/<name>.md).

If it needs a NEW doc, draft the stub now — before code

Author it in the branch so it's reviewed with the diff, not bolted on after. Confirm the live shape against any existing file in engdocs/design/ first, then:

cat > engdocs/design/<short-kebab-name>.md <<'MD'
---
title: "<Title Case Name>"
---

| Field | Value |
|---|---|
| Status | Proposed |
| Date | <YYYY-MM-DD> |
| Author(s) | <your handle> |
| Issue | #<number> |
| Supersedes | N/A |

## Summary

<2-4 sentences: what this changes and the one-line reason it's worth doing.>

## Problem

<What's broken or missing today, for a future maintainer with no context.>

## Design

<The approach. Name the subsystem boundaries it touches and the contract it
establishes — the "why this shape" the review otherwise reverse-engineers.>

## Alternatives considered

<At least one rejected option and why. Pre-empts "did you consider X?".>
MD

Register it — add one row to the Current Design Set table in engdocs/design/index.md:

| `<short-kebab-name>` | Proposed | <one-line note> |

Open at Status: Proposed; let review move it to Accepted and the landing PR move it to Implemented.

Phase 4 — Plan output

Produce this and confirm it before writing code:

Issue: #<number> — <title>

Root cause: <one sentence>

Files to change:
  <file>:<function> — <what changes>

Blast radius: <summary from the map-blast-radius skill>

Design capture:
  Change class: <point-fix | test/docs-only | refactor | architectural>
  [ ] Trigger fired? (subsystem boundary / new package / new contract / feature-scale)
  [ ] Existing engdocs/design doc to cite, OR new stub drafted (Proposed)
  [ ] Registered in engdocs/design/index.md
  (point-fix / test-only / docs-only / refactor → "N/A, code-only PR" + one-line why)

Convention triggers (the B-rules this change must satisfy):
  [ ] Config field sync (B11, B15)
  [ ] Store write error propagation / retain-and-retry (B12)
  [ ] Timeout isolation (B13)
  [ ] do*()/cmd*() split (B19)
  [ ] Test doubles + env hermeticity (B20)
  [ ] Map key separation (B18)
  [ ] Startup vs reload (B16)
  [ ] Goroutine lifecycle (B17)
  [ ] Config snapshot safety (B21)
  [ ] Dead code audit (B22)
  [ ] Fix scope completeness + parallel siblings (B23)
  [ ] Verify-before-delete (B24)
  [ ] Constant grep radius (B25)
  [ ] Golden snapshot / doc drift (B26)
  [ ] Env projection layer (B29)
  [ ] Package-level race-safety (B30)
  [ ] Hard-fail examples audit (B31)
  [ ] Test save/restore of pkg state (B33)

Test plan:
  Unit: <what to test, which fakes>
  Testscript: <which .txtar to add/modify>
  Integration: <if needed>

Risks:
  <what could go wrong, what the maintainer will scrutinize>

Confirm the plan before writing any code.

Phase 5 — What the maintainer will check

Gas City adoption reviews run a multi-model automated pass and consistently flag these. Address them proactively — each maps to a B-rule in review:

1. Silent error handling — _ = store.Write() or a bool return that doesn't fire false on write error (B12)
2. Shared code paths — timeouts/semaphores bleeding into unintended subsystems (B13)
3. Nil/empty distinction — config fields where "not set" vs "explicitly empty" matters (B11)
4. Infrastructure-agent contamination — loops applying work config to control_dispatcher (B15)
5. Goroutine lifecycle — fire-and-forget goroutines in CLI paths; defer-block done channels on ALL return paths (B17)
6. Startup vs reload safety — one-time operations wired into reload paths (B16)
7. Test specificity — assertions that could pass for the wrong reason (B14)
8. Raw string literals — a constant defined but not used everywhere; grep the ENTIRE codebase (B25)
9. Dead code introduced by the PR — helpers added "for completeness", never called (B22)
10. Incomplete state coverage — a fix handling one state but not the others the bug also hits (B23)
11. Verify-before-delete — pruning without confirming the successor state exists; fail-open (B24)
12. Missing regression branches — multiple code paths (early-exit/timeout/error/success), only the happy path tested (B14)
13. Parallel code-path siblings (the #1 finding) — fixing one call site while identical siblings stay broken (B23)
14. Env-var hermeticity — os.Setenv instead of t.Setenv() in tests (B20)
15. Golden-snapshot / doc drift — changed output without updating tutorial golden files / .txtar fixtures (B26)
16. Package-level mutable-state races — var Foo bool written by reload, read by request paths; must be atomic.* (B30)
17. Hard-fail conversions — turning silent degradation into a hard error without auditing examples/ + adding a release note (B31)
18. Test cleanup that hardcodes state instead of restoring it (B33)
19. Env projection inlined into command handlers instead of cmd/gc/bd_env.go (B29)

After the code is written, run fine-tune (which ends by running review) to verify against the full list before you push.