diff --git a/ai-agents/AGENTS.d/git-worktrees.md b/ai-agents/AGENTS.d/git-worktrees.md index 1a297bc2..528df37e 100644 --- a/ai-agents/AGENTS.d/git-worktrees.md +++ b/ai-agents/AGENTS.d/git-worktrees.md @@ -2,7 +2,9 @@ Applies when the repo uses the **bare-worktree layout**: the parent directory holds `.bare/` plus one directory per branch (e.g. `E:\Personal Projects\dotfiles\{.bare, main}`). **Check before creating a branch** — look for a sibling `.bare/`, or run `git worktree list`. In a normal clone, branch as usual and ignore the rest of this section. -- **A new branch means a new worktree. Never `git checkout -b` inside the `main` worktree.** From inside an existing worktree (e.g. `main`), not the bare-repo parent dir itself: `git worktree add ../ -b ` (flatten any `/` in the branch name for the directory), then change the working directory to that worktree and do the work there. `git worktree add`'s path is resolved against the shell's cwd, not the repo root — running it from the bare-repo parent dir (the one holding `.bare` + `main`) instead of from `main` lands the new worktree one level too high, as a sibling of the bare-repo dir rather than inside it. -- **Why:** the point of the layout is that every worktree is a stable, always-available checkout of its branch. `main` must stay on `main` so it remains browsable and buildable while feature work happens elsewhere — and so anything pointing at that path (tooling, another session, a live config) keeps seeing main. +- **A new branch means a new worktree. Never `git checkout -b` inside the default-branch worktree (e.g. `main`).** From inside an existing worktree (e.g. `main`), not the bare-repo parent dir itself: `git worktree add ../ -b ` (flatten any `/` in the branch name for the directory), then change the working directory to that worktree and do the work there. `git worktree add`'s path is resolved against the shell's cwd, not the repo root — running it from the bare-repo parent dir (the one holding `.bare` + `main`) instead of from `main` lands the new worktree one level too high, as a sibling of the bare-repo dir rather than inside it. +- **Why:** the point of the layout is that every worktree is a stable, always-available checkout of its branch. The default-branch worktree must stay on the default branch so it remains browsable and buildable while feature work happens elsewhere — and so anything pointing at that path (tooling, another session, a live config) keeps seeing it. +- **Don't hardcode the default branch name.** It's `main` in this repo but not necessarily elsewhere — detect it with `git symbolic-ref --short refs/remotes/origin/HEAD` (strip the `origin/` prefix) rather than assuming `main`/`master`. If it's unset, stop and ask which branch is the default rather than guessing. +- **Enforced automatically by `/implement`.** [`implement/SKILL.md`](../skills/implement/SKILL.md)'s first step checks whether the current branch is the default branch and, if so, branches off before doing anything else — in both layouts (`git worktree add` here, `git checkout -b` in a normal clone). This page stays the mechanics/rationale reference; the skill is what actually fires it. - **After the PR is merged:** change back to the main worktree → `git pull` → `git worktree remove ` (drops the registration *and* the directory) → **then** delete the branch. That order is forced: git refuses to delete a branch while a worktree still holds it, so the worktree always goes first. - **Squash-merged PRs need `-D`, not `-d`.** GitHub's squash-merge rewrites the branch's commits, so `git branch -d` refuses with "not fully merged" even though every change landed. Verify nothing is lost with `git diff main --stat` (empty ⇒ safe), then force-delete. `git branch -D` is denied by a Claude Code deny hook, so ask me to run that one. diff --git a/ai-agents/skills/dispatch-implement/SKILL.md b/ai-agents/skills/dispatch-implement/SKILL.md index 1deccc5f..57676df0 100644 --- a/ai-agents/skills/dispatch-implement/SKILL.md +++ b/ai-agents/skills/dispatch-implement/SKILL.md @@ -56,21 +56,30 @@ checks, review, commit. (see Override below). 5. **Dispatch**, one subagent per ticket, each prompted to run `/implement` on that ticket and nothing else. Parallel units go out in a single message so they actually run concurrently; - sequential units wait for the previous unit's result before the next dispatch. -6. **Integrate parallel children** (sequential children skip this — they already committed to the - current branch, in turn). An isolated child commits to its own branch in its own worktree, so its - work is not on the invoking branch until you bring it over. Before dispatch, capture each child's - pre-dispatch `HEAD` — that commit is the range base for that child's cherry-pick. Once every - parallel child has returned, for each in ticket order cherry-pick `base..tip` (its own branch tip) - onto the invoking branch — the full range, since `/implement` does not promise exactly one commit — - then run the project's fast checks once over the integrated result. The full suite is not re-run - post-integration: each child already ran it in isolation before its own commit, per `/implement`'s - own contract; this is an accepted tradeoff for a thin wrapper, not an oversight. On a cherry-pick - conflict, run `git cherry-pick --abort` to leave the invoking branch clean, then stop and hand back - the child branch and worktree names; do not resolve it silently, since a conflict here means the - non-overlapping judgment in step 3 was wrong. Once integration succeeds, remove the child worktrees - and delete the child branches — their commits are now fully represented on the invoking branch via - the cherry-picks, so there is no reason to keep them. + sequential units wait for the previous unit's result before the next dispatch. **Any run — + parallel or sequential — starting on the default branch:** branch off once, here, before + dispatching the first child — same detection as `/implement`'s own first step + (`implement/SKILL.md`, `AGENTS.d/git-worktrees.md`), named `/-` when the + invocation shares a spec/epic, else `/-` off the first ticket listed, in + invocation order. Step 6 cherry-picks each parallel child's range onto the invoking branch, so + the invoking branch must not be the default branch; see + `docs/adr/implement-enforces-worktree-per-ticket-in-its-own-first-step.md` for the full + rationale. +6. **Integrate parallel children** (sequential children skip this — they commit straight to the + invoking branch in turn, which step 5 guarantees is never the default branch). An isolated child commits to + its own branch in its own worktree, so its work is not on the invoking branch until you bring it + over. Before dispatch, capture each child's pre-dispatch `HEAD` — that commit is the range base + for that child's cherry-pick. Once every parallel child has returned, for each in ticket order + cherry-pick `base..tip` (its own branch tip) onto the invoking branch — the full range, since + `/implement` does not promise exactly one commit — then run the project's fast checks once over + the integrated result. The full suite is not re-run post-integration: each child already ran it + in isolation before its own commit, per `/implement`'s own contract; this is an accepted + tradeoff for a thin wrapper, not an oversight. On a cherry-pick conflict, run + `git cherry-pick --abort` to leave the invoking branch clean, then stop and hand back the child + branch and worktree names; do not resolve it silently, since a conflict here means the + non-overlapping judgment in step 3 was wrong. Once integration succeeds, remove the child + worktrees and delete the child branches — their commits are now fully represented on the invoking + branch via the cherry-picks, so there is no reason to keep them. 7. **Report** per ticket: model used, parallel or sequential and why, the child's outcome, and for a parallel run the integration result (integrated cleanly, or which branches are left for the user). diff --git a/ai-agents/skills/implement/SKILL.md b/ai-agents/skills/implement/SKILL.md index dedf63ad..48351d53 100644 --- a/ai-agents/skills/implement/SKILL.md +++ b/ai-agents/skills/implement/SKILL.md @@ -6,6 +6,39 @@ disable-model-invocation: true Implement the work described by the user in the spec or tickets. +**Before anything else, make sure this isn't landing on the default branch.** Determine the +default branch (`git symbolic-ref --short refs/remotes/origin/HEAD`, stripped of the `origin/` +prefix — don't assume `main`/`master`). If that's unset (e.g. a fresh or `--single-branch` +clone with no `origin/HEAD`), stop and ask which branch is the default rather than guessing or +running an untested network fallback. If the current branch is the default branch, branch off +first, automatically, without asking: + +- **Bare-worktree layout** (sibling `.bare/`, or `git worktree list` shows more than one entry — + see [git-worktrees.md](../../AGENTS.d/git-worktrees.md)): from the current worktree, + `git worktree add ../ -b ` (flatten any `/` in `` for + ``), then move into that new worktree and continue all further work there. + **Claude Code:** a plain shell `cd` to a sibling worktree directory does not reliably persist + across tool calls in the sandboxed Bash tool (verified: it silently resets to the original + directory) — use `EnterWorktree` with `path` set to the new worktree's directory instead. From + the main session's own launch directory, its first-entry check accepts any path already + registered in `git worktree list`, so the bare-worktree layout's sibling directory qualifies + even though it isn't under `.claude/worktrees/`. From a pinned subagent (working directory + pinned at launch — subagent isolation or an explicit cwd, e.g. a `dispatch-implement` child) + that finds itself on the default branch, `EnterWorktree`'s contract requires the target to be + a worktree already under `.claude/worktrees/` of the same repo, so the bare-worktree sibling + directory does not qualify there: either stop and hand back rather than guess, or use + `EnterWorktree name:` instead of `git worktree add` + `path` — it creates the new worktree + under `.claude/worktrees/` and is accepted from a pinned agent. **Codex CLI / Pi:** unverified + whether their shell tools persist a `cd` across calls the same way — check before relying on + it, and use their own worktree/session primitive if one exists rather than assuming a bare + `cd` holds. If persistence can't be confirmed and no such primitive exists, stop and ask the + user to switch directories manually rather than proceeding on an unverified `cd`. +- **Normal clone**: `git checkout -b ` — no directory change involved, so no `cd`/session + caveat applies here. + +Name `` as `/-` (conventional-commit ``, e.g. `feat`/`fix`; +slug from the ticket/spec title) — drop the `-` segment when there's no ticket. + Use `/tdd` where possible, at pre-agreed seams. Run the project's fast checks (typecheck / lint / analyzer) regularly and single test files as you go; run the full test suite once at the end. diff --git a/docs/adr/implement-enforces-worktree-per-ticket-in-its-own-first-step.md b/docs/adr/implement-enforces-worktree-per-ticket-in-its-own-first-step.md new file mode 100644 index 00000000..6d90cf28 --- /dev/null +++ b/docs/adr/implement-enforces-worktree-per-ticket-in-its-own-first-step.md @@ -0,0 +1,109 @@ +# `/implement` enforces worktree-per-ticket in its own first step + +## Status + +Accepted. Governs how `ai-agents/skills/implement/SKILL.md` prevents ticket work from landing +on the default branch. + +## Context + +`AGENTS.d/git-worktrees.md` documents the bare-worktree layout's core rule — a new branch +means a new worktree, never `git checkout -b` inside the default-branch worktree — but the rule +was advisory only: nothing stopped `/implement` from running directly in the `main` worktree. + +A prior attempt enforced this with a `PreToolUse` hook plus a marker file +(`.active-ticket.json`) written by the implement skill to tell the hook "a ticket is in play." +It was ported three times across Claude Code, Codex CLI, and Pi with diverging capability: +Claude Code's hooks support an ask/confirm decision, Codex and Pi only support hard allow/deny. +No research turned up precedent for a marker/state-file mechanism used this way — every real +example of branch-per-task enforcement in AI coding agents and worktree tooling is either an +orchestration step that just creates the worktree up front (Codex Branch Workspaces, +OpenHands' worktree-per-task convention, Claude Code's/Cursor's opt-in worktree session modes), +or a hook intercepting git commands directly (with well-documented friction: absolute-path +requirements, deprecated schemas, false positives on legitimate branch-then-commit sequences). Git itself has no per-worktree hook — a hook applies repo-wide across every worktree, so a hook +scoped to "block commits when a ticket is active" still needs external state to know a ticket +is active, which is exactly what the marker file was for. + +The actual mistake in scope is narrower than "any commit on main": it's `/implement` itself +running while the current worktree/clone sits on the default branch. `dispatch-implement` +already isolates parallel children into their own worktree (`isolation: "worktree"`); its +sequential children and any standalone `/implement` invocation inherit whatever branch the +invoking session is already on, which is the actual gap. + +## Decision + +Move the check into `/implement`'s own first step, as plain skill-instruction text — no hook, +no marker file, no new state: + +1. Detect the default branch (`git symbolic-ref --short refs/remotes/origin/HEAD`), since it + varies per repo — verified in this repo, resolves to `origin/main`. If it's unset, stop and + ask rather than run an untested network fallback + (`git remote set-head`/`git remote show origin` were considered and dropped for this reason). +2. If the current branch is the default branch, branch off automatically before doing anything + else — `git worktree add` in the bare-worktree layout, `git checkout -b` in a normal clone — + then continue the rest of `/implement` from there. On Claude Code, moving into the new + worktree needs `EnterWorktree` with `path`; a plain shell `cd` resets on the next tool call — + verified live that a `cd` to a sibling worktree directory in the Bash tool silently resets to + the original directory on the next call, so the switch would not actually hold without it. + Codex CLI's and Pi's shell + persistence for this case is unverified and flagged as such in `implement/SKILL.md` rather + than assumed. +3. Name the branch `/-` (conventional-commit type, ticket number when one + exists, slug from the ticket/spec title). + +This covers a standalone `/implement` call directly. For `dispatch-implement`, it exposed a gap +in that skill's own assumptions, for both dispatch modes: sequential children were assumed to +always commit straight to the invoking branch (true only as long as `/implement` never moved the +workspace itself), and parallel children's step-6 cherry-picks target *the invoking branch*, +which would be the default branch if the parent session started there. `dispatch-implement`'s +step 5 is updated alongside this ADR: whenever a dispatch run — parallel or sequential — would +start on the default branch, the *parent* branches off once, before dispatching the first child, +using the same detection and naming `/implement` uses. Every sequential child's own `/implement` +call then finds itself already off the default branch and commits straight to it in turn, and +every parallel child's integration cherry-picks land on that same feature branch instead of the +default branch — preserving `dispatch-implement`'s original assumptions for both modes rather +than replacing them with a wider integration path. The alternative (let each child branch off +independently and integrate the results one at a time) was rejected: it would spawn one +throwaway worktree per ticket and still land cherry-picks back onto the default branch, which is +the exact outcome this whole mechanism exists to prevent. + +This resolves the prior design's capability split for free: the skill follows this as prompt +text, so it behaves identically across runtimes — "detect and just do it" works the same way on +Claude Code, Codex CLI, and Pi, with no ask/confirm-vs-hard-block distinction to reconcile since +no hook is involved. + +This nudges against `docs/adr/implement-stays-minimal-dispatch-in-wrapper-skill.md`, which kept +dispatch logic (ticket parsing, parallel/sequential judgment, per-runtime dispatch mechanics, +model selection) out of `/implement` and in the `dispatch-implement` wrapper instead. The +worktree check isn't dispatch logic — it doesn't parse tickets, judge parallelism, or pick a +dispatch primitive — it's a workspace precondition, the same category as the fast-checks step +`/implement` already runs before committing. It stays in scope for that ADR's boundary. + +## Rejected alternatives + +### Advisory-only (status quo) + +Already in place via `AGENTS.d/git-worktrees.md` and clearly insufficient — that's the problem +being solved. + +### `PreToolUse` hook + marker file, per runtime + +The prior attempt. Rejected: three ports for one rule, diverging ask/deny behavior across +runtimes, a marker file that's new state with its own lifecycle (write, read, staleness, who +clears it), and no precedent anywhere for this shape solving this problem. + +### Hard-blocking hook without a marker file + +Would need to distinguish "this commit is ticket work" from any other commit on the default +branch (e.g. a docs fix, a version bump) without state to say a ticket is in play — the same +problem the marker file existed to solve, or a hook broad enough to block *every* commit on the +default branch regardless of intent, which is a different and unwanted rule (the grilling +session that produced this ADR confirmed the mistake in scope is `/implement` on the default +branch specifically, not commits on the default branch in general). + +### Confirm before creating the worktree, instead of auto-creating + +Considered and rejected in favor of auto-create-then-proceed, matching Codex Branch Workspaces +and OpenHands' pattern: the first step of ticket work just creates the workspace, no +interruption. Creating a worktree/branch is cheap and reversible (`git worktree remove`, +`git branch -D`), so it doesn't need a confirm gate the way a destructive action would.