diff --git a/.changeset/wise-rams-travel.md b/.changeset/wise-rams-travel.md new file mode 100644 index 000000000..d74c6ae9c --- /dev/null +++ b/.changeset/wise-rams-travel.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3757 +--- +**A plan that declares a file removal can now be merged by cleanup-wave** — add a `files_deleted:` list to a plan's frontmatter and the post-wave deletions guard authorizes exactly those paths, so a refactor that folds one file into another stops needing a manual merge outside the tool. Anything the plan did not declare still blocks that entry, and only that entry. Plans and manifests without the field behave exactly as before. (#3003) diff --git a/agents/gsd-plan-checker.md b/agents/gsd-plan-checker.md index 7a4e25387..1be809339 100644 --- a/agents/gsd-plan-checker.md +++ b/agents/gsd-plan-checker.md @@ -214,7 +214,7 @@ issue: **Question:** Do two same-wave plans depend on each other through shared mutable state or execution order without declaring it? Dimension 3 checks *declared* edges and the wave guard -checks `files_modified` overlap; neither sees an undeclared edge, which under parallel +checks `files_modified`/`files_deleted` overlap (#3003); neither sees an undeclared edge, which under parallel execution becomes an intermittent failure nobody can attribute. **Scope: PLAN pairs, not tasks.** Tasks inside one plan run sequentially and cannot race. @@ -229,7 +229,7 @@ Execution; strong-but-local coupling inside one plan is fine): produces. **Do NOT flag:** both sides only READ it, or it is immutable; the pair already overlaps in -`files_modified` (report that once, on the file axis); the plans sit in a different wave, which +`files_modified` or `files_deleted` (report that once, on the file axis); the plans sit in a different wave, which already orders them; two tasks inside one plan; a vague same-subsystem claim naming no resource; incompatible *transformations* of one entity — that is Dimension 9. diff --git a/agents/gsd-planner.md b/agents/gsd-planner.md index 84f630b15..538f05366 100644 --- a/agents/gsd-planner.md +++ b/agents/gsd-planner.md @@ -748,12 +748,12 @@ for each plan in plan_order: # Implicit dependency: files_modified overlap forces a later wave. for each plan B in plan_order: for each earlier plan A where A != B: - if any file in B.files_modified is also in A.files_modified: + if any file in (B.files_modified + B.files_deleted) is also in (A.files_modified + A.files_deleted): B.wave = max(B.wave, A.wave + 1) waves[B.id] = B.wave ``` -**Rule:** Same-wave plans must have zero `files_modified` overlap. After assigning waves, scan each wave; if any file appears in 2+ plans, bump the later plan to the next wave and repeat. +**Rule:** Same-wave plans must have zero `files_modified`/`files_deleted` overlap. After assigning waves, scan each wave; if any file appears in 2+ plans, bump the later plan to the next wave and repeat. diff --git a/capabilities/claude-orchestration/fragments/execute-wave-pre.md b/capabilities/claude-orchestration/fragments/execute-wave-pre.md index d85f57b89..e14499375 100644 --- a/capabilities/claude-orchestration/fragments/execute-wave-pre.md +++ b/capabilities/claude-orchestration/fragments/execute-wave-pre.md @@ -206,9 +206,16 @@ unchanged: gsd_run query worktree.record-agent --manifest "$WAVE_WORKTREE_MANIFEST" \ --agent-id "" --path "" \ --branch "" --base "" \ - --files "" + --files "" \ + --deletions "" ``` + `--deletions` (#3003) carries the plan's declared `files_deleted` so a plan that scoped a file + removal merges through `cleanup-wave` instead of being blocked. Unlike `--files` it is not + advisory: omitting it leaves the deletions guard blocking on any deletion at all, so this + dispatch path must pass it or plans declaring a removal fail to merge here while succeeding on + the inline path. + The verb's write-strict validation applies as inline: on a non-zero exit or any missing field, stop and ask for recovery — do not append an under-populated entry. diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index e4d6e03b5..7df08b4ca 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -1045,6 +1045,27 @@ node gsd-tools.cjs worktree record-agent \ `--files` is optional (#2596). When supplied it records the plan's declared `files_modified` — the same whitespace-separated `PLAN_FILES` list the per-plan worktree gate already builds — as an extra `files_modified` array on the entry, and `cleanup-wave` then reports any path the branch committed outside it. A blank or omitted `--files` writes no field at all, leaving the 4-field on-disk shape untouched, and the scope check is simply skipped for that entry: an unrecorded scope means *unknown*, never *declares nothing*. Values are compared against a diff, never opened as paths and never passed to a shell. +`--deletions` is optional (#3003). When supplied it records the plan's declared `files_deleted` — built by the per-plan worktree gate exactly like `PLAN_FILES`, from the plan's own frontmatter — as a `declared_deletions` array on the entry. It is the opt-in the deletions guard reads (see below). A blank or omitted `--deletions` writes no field, leaving the on-disk shape untouched and the guard's original unconditional block in force. Like `--files`, values are compared against a diff and never opened or passed to a shell. + +**Intentional deletions (gate, #3003)** + +`cleanup-wave` blocks the merge of any executor branch whose diff deletes a file — a net against a mass-deletion accident. A plan whose scope legitimately includes removing a file declares those paths in its `files_deleted` frontmatter, which reaches the entry as `declared_deletions`; the guard then blocks only the deletions **not** in that list. + +| Branch deletes | Entry declares | Result | +|---|---|---| +| nothing | — | merges | +| `tests/a.ts` | *(no field)* | **blocked** — unchanged pre-#3003 behavior | +| `tests/a.ts` | `["tests/a.ts"]` | merges | +| `tests/a.ts`, `src/b.ts` | `["tests/a.ts"]` | **blocked**, and the block detail names only `src/b.ts` | +| `tests/a.ts` | `["tests"]` | **blocked** — a directory does not authorize its children | +| `tests/a.ts` | `["*.ts"]` | **blocked** — globs are literal paths here, matching nothing | + +Matching is **exact after normalization**: git's C-quoting is decoded, backslashes become forward slashes, and a leading `./` and any trailing `/` are stripped — on both sides. The decode matters more than it looks: with `core.quotepath` at its git default, a path like `tests/é.ts` is reported as the literal `"tests/\303\251.ts"`, which would never compare equal to the plainly-declared path, so a correctly declared deletion of any non-ASCII path would block forever with nothing pointing at the encoding. It is deliberately neither a prefix nor a glob match — either would let one declaration authorize a whole set of deletions, which is the accident the guard exists to catch. A declared path that was not in fact deleted is inert. A blocked entry still isolates: the rest of the wave proceeds (#2852). If the deletion check itself fails the entry blocks on `deletion_check_failed` and is never filtered — a broken check is not an authorization. + +A declared deletion is also treated as in-scope by the advisory below, so authorizing a removal does not then warn that the removed path was out of the declared scope. That is done by **subtracting** declared deletions from the advisory's findings, not by adding them to the declared scope it matches against — the advisory reads its scope list with prefix-and-glob semantics, so adding them would quietly give `declared_deletions` a second, wider matching rule than the table above, and `["*.md"]` would go from inert to silencing the advisory entirely. One field, one matching rule, on every surface. Subtraction also means the advisory's activation is unchanged: it still runs only when `files_modified` is recorded, so a plan that declares deletions alone stays as silent as it was before #3003. + +One limit worth knowing, shared with `--files` and failing closed: a declared path containing a **space** cannot be expressed, because the flag value is whitespace-separated — such a path splits into fragments, matches nothing, and the entry blocks. Flag values are also read positionally and never re-inspected for shape, so a malformed `--deletions --files src/a.ts` records the literal `--files` as the declaration; that is harmless (it is a path git never reports as deleted, so it authorizes nothing) and `--files` still resolves to `src/a.ts` on its own lookup. + **Scope conformance at merge (advisory, #2596)** When a manifest entry carries a declared `files_modified`, `cleanup-wave` compares the branch's actual committed diff (`HEAD...`) against it and appends one entry to the result's `warnings` array for every path outside the declared scope, with `code: "scope_out_of_declared"` and the offending `path`. If the diff itself cannot be computed the entry gets a single `code: "scope_check_unavailable"` warning instead, so an unknown result is never mistaken for a clean one. Warnings are also aggregated on the top-level `warnings` array, each tagged with its `branch`. diff --git a/docs/how-to/interpret-scope-conformance-warnings.md b/docs/how-to/interpret-scope-conformance-warnings.md index 0decc9c00..cdaf05089 100644 --- a/docs/how-to/interpret-scope-conformance-warnings.md +++ b/docs/how-to/interpret-scope-conformance-warnings.md @@ -83,7 +83,7 @@ Absence of `scope_out_of_declared` / `scope_check_unavailable` warnings is not p ## Known limits - **Glob matching is literal-prefix only.** `src/**/*.ts` matches anything under `src/`, including `src/a/b.json` — the check does not parse glob syntax past the literal prefix. -- **Renames are not detected specially.** A rename appears in the diff as an add plus a delete. In practice this rarely reaches the scope-conformance check at all: the pre-existing deletions guard blocks any entry whose diff contains a deletion before the scope-conformance check runs. +- **Renames are not detected specially, and mostly are not gated at all.** Git's rename detection runs by default, so a *pure* rename (content unchanged, similarity 100%) is reported as a single `R` entry and appears in **no** `--diff-filter=D` output. The deletions guard therefore never fires on it — before or after #3003 — and the rename lands here at the advisory, where the new path is compared against the declared scope like any other. Only a rename that also edits the file enough to fall below git's similarity threshold decomposes into a separate add and delete; that delete is a real deletion, and the guard blocks the entry unless the old path is declared in `files_deleted`. Declaring it is the fix in that case. The old path needs no `files_modified` entry either way: a declared deletion is subtracted from this check's findings, so it never shows up as out-of-scope. - **`/gsd-quick` worktrees have no plan-declared scope.** They are never checked, for the same reason as the "no `--files` recorded" case above. --- diff --git a/docs/reference/plan-md.md b/docs/reference/plan-md.md index 747b3a17a..862be6c75 100644 --- a/docs/reference/plan-md.md +++ b/docs/reference/plan-md.md @@ -35,6 +35,8 @@ files_modified: - src/components/PostFeed.tsx - src/components/PostCard.tsx - src/app/feed/page.tsx +files_deleted: + - src/components/LegacyFeed.tsx autonomous: true requirements: ["FEED-01", "FEED-03"] user_setup: [] @@ -69,6 +71,7 @@ must_haves: | `wave` | Yes | integer | Execution wave. Plans in wave 1 run in parallel (no dependencies). Plans in wave 2+ wait for all plans in the previous wave to complete. Pre-computed at plan time by `gsd-planner`. | | `depends_on` | Yes | array of plan IDs | Plans this plan must wait for. Empty array = wave 1. Example: `["03-01"]` means this plan runs after Plan 01 in Phase 3. | | `files_modified` | Yes | array of paths | Every file this plan creates or modifies. Used by the plan-checker to detect same-wave file conflicts and by execute-phase for merge tracking. | +| `files_deleted` | No | array of paths | Every file this plan deliberately **removes**. The post-wave cleanup gauntlet blocks the merge of any executor branch whose diff deletes a file — a net against a mass-deletion accident — and this field is the opt-in that names the exceptions. Matching is exact per path after separator normalization: a declared path merges, an undeclared one still blocks that plan's entry (and only that entry). There are no globs and no directory prefixes, so a declaration can never authorize more than it literally lists. Omit the field and the guard's original unconditional block stays in force, which is why absence is always the safe default (#3003). Counts toward same-wave conflict detection alongside `files_modified`: a plan deleting a file another plan in the same wave is editing is the sharpest conflict there is — one branch removes what the other is writing — so the two plans are pushed into different waves regardless of which side holds the deletion. | | `autonomous` | Yes | boolean | `true` when all tasks are type `auto`. `false` when the plan contains any `checkpoint:*` task that requires human interaction. | | `requirements` | Yes | array of IDs | Requirement IDs from ROADMAP.md that this plan addresses. Every phase requirement ID must appear in at least one plan's `requirements` field. Empty arrays are a BLOCKER. | | `user_setup` | No | array of objects | External-service setup steps that Claude cannot automate (account creation, secret retrieval, dashboard configuration). When present, execute-phase generates a `USER-SETUP.md` checklist for the developer. | diff --git a/gsd-core/bin/lib/capability-registry.cjs b/gsd-core/bin/lib/capability-registry.cjs index 5b9dbf7a6..f8e49cead 100644 --- a/gsd-core/bin/lib/capability-registry.cjs +++ b/gsd-core/bin/lib/capability-registry.cjs @@ -705,7 +705,7 @@ const capabilities = { "into": "executor", "fragment": { "path": "fragments/execute-wave-pre.md", - "inline": "# Claude orchestration — Workflow execution backend (BETA)\n\n> Injected at `execute:wave:pre` `into: executor` only when\n> `claude_orchestration.enabled` is true. Default-off; `onError: skip`.\n\n## When this contribution is active\n\nThe Claude orchestration capability is **default-off and BETA**. It activates only\nwhen ALL of the following hold:\n\n1. `claude_orchestration.enabled` is `true` in `.planning/config.json`, AND\n2. the active runtime is **Claude Code** (the Workflow tool is Claude / Agent\n SDK-specific), AND\n3. `claude_orchestration.execution_backend` resolves to `workflow` — either\n explicitly, or via `auto` — **and** the Agent SDK version is\n `>= claude_orchestration.min_agent_sdk_version` (default `0.3.149`). The SDK\n floor applies in both `auto` and `workflow` modes (fail-closed: a pre-release\n or older SDK never activates the preview backend).\n\nDetection is fail-closed: any miss degrades to **inline, manual, one-agent-per-\nmessage dispatch** — exactly today's behaviour. On a non-Claude runtime this\ncontribution is a no-op.\n\n## Why `execute:wave:pre` (not `execute:wave:post`)\n\nThis is a **dispatch-backend selector** — it decides HOW a wave's executor agents\nare spawned. That decision has to be made BEFORE the wave's `Agent()` calls in\n`execute-phase.md` step 3, not after the wave has already finished (#2285). The\ncapability previously registered at `execute:wave:post`, which fires only after\nworktree merge/post-merge tests/tracking updates — by then the wave was already\ndispatched inline, so the contribution was structurally unable to change how\ndispatch happened. This fragment is injected at the point that actually precedes\ndispatch.\n\n## What the orchestrator does when the Workflow backend is active\n\nBefore spawning executor agents for the current wave (execute-phase.md step 3),\nresolve the dispatch backend through the single composed CLI seam:\n\n```bash\ngsd-tools claude-orchestration resolve-wave-dispatch \\\n --waves \"$WAVE_MANIFEST_PATH\" --run-id \"$PHASE_RUN_ID\" \\\n --runtime \"$RUNTIME\" \\\n --phase-dir \"$PHASE_DIR\" --raw\n```\n\n`--agent-sdk-version` is no longer passed here (#2590). The router resolves the\ninstalled Agent SDK version itself; see **Agent SDK version** below. The former\n`${AGENT_SDK_VERSION:+--agent-sdk-version \"$AGENT_SDK_VERSION\"}` line was also\n**shell-dependent**: zsh does not word-split unquoted parameter expansions, so it\ncollapsed to a SINGLE argv element there, `argValue()` never matched, and the run\nfailed into `agent_sdk_version_unknown` — indistinguishable from genuinely\nunknown. Pass `--agent-sdk-version ` explicitly only to pin a version.\n\nThis composes `detectWorkflowBackend` (the gate ladder above) with\n`emitWorkflowScript` (the wave→plan mapping below) in ONE call — the pure\nfunction backing it is `resolveWaveDispatch` in\n`gsd-core/bin/lib/claude-orchestration.cjs`. Response shape:\n`{ backend: 'inline'|'workflow', reason, script?, summary? }`.\n\n### Manifest construction (`$WAVE_MANIFEST_PATH`, `$PHASE_RUN_ID`, `$PHASE_DIR`)\n\nThese are NOT pre-existing execute-phase.md variables — the orchestrator builds\nthem at this step, from data it already has in-context from `discover_and_group_plans`\n(the `PLAN_INDEX` JSON) and step 2.5 (the per-plan `USE_WORKTREES_FOR_PLAN` decision):\n\n1. **`$PHASE_DIR`** — reuse `{phase_dir}` from the `INIT` bundle (already loaded\n in the `initialize` step). No new value needed.\n\n2. **`$PHASE_RUN_ID`** — a stable identifier for THIS phase-execution attempt, so\n `resumeFromRunId` can resume an interrupted run without re-dispatching plans\n the Workflow tool already completed. Construct it deterministically —\n `execute-{phase_number}-{phase_slug}` — from `INIT`'s `phase_number`/`phase_slug`\n (both are already validated identifiers used elsewhere in this workflow, so\n they satisfy `emitWorkflowScript`'s `isScriptableIdentifier` check). Do NOT\n mint a new random id per wave — the SAME `$PHASE_RUN_ID` is reused for every\n wave in the phase so the Workflow tool can correctly track cross-wave resume\n state.\n\n3. **`$WAVE_MANIFEST_PATH`** — a fresh temp file for THIS wave's manifest (one\n wave = one `waves` array with a single entry, matching the wave-by-wave\n dispatch loop; do not batch multiple waves into one manifest — waves are\n dispatched in wave order, not all at once):\n\n ```bash\n WAVE_MANIFEST_PATH=$(mktemp \"${TMPDIR:-/tmp}/gsd-wave-dispatch-XXXXXX\") && mv \"$WAVE_MANIFEST_PATH\" \"$WAVE_MANIFEST_PATH.json\" && WAVE_MANIFEST_PATH=\"$WAVE_MANIFEST_PATH.json\"\n ```\n\n Then **use the Write tool** (not a bash/jq pipeline — the orchestrator already\n has every field parsed in-context) to write the manifest JSON to\n `$WAVE_MANIFEST_PATH`:\n\n ```json\n {\n \"waves\": [\n {\n \"id\": \"wave-{N}\",\n \"plans\": [\n {\n \"id\": \"{plan_id}\",\n \"brief\": \"{the SAME ... prompt block step 3 builds for this plan's inline Agent() call}\",\n \"files_modified\": [\"{from PLAN_INDEX.plans[].files_modified for this plan}\"],\n \"use_worktree\": {true unless step 2.5 set USE_WORKTREES_FOR_PLAN=false for this plan}\n }\n ]\n }\n ]\n }\n ```\n\n - **`id`** — the plan id from `PLAN_INDEX`, e.g. `\"01-01\"`.\n - **`brief`** — MUST carry the same task content as step 3's inline `Agent()`\n prompt (the ``/``/``/\n `` block, with `{plan_number}`/`{phase_number}`/\n `{phase_name}` substituted) — a short summary here would NOT reproduce\n step 3's behavior and would violate the \"identical artifacts\" contract.\n - **`files_modified`** — copy verbatim from the plan's `PLAN_INDEX` entry.\n - **`use_worktree`** — `true` for every plan UNLESS step 2.5's per-plan\n worktree gate (`execute-phase/steps/per-plan-worktree-gate.md`) set\n `USE_WORKTREES_FOR_PLAN=false` for that plan (submodule-touching plan, or\n project-level `USE_WORKTREES=false`) — in which case pass `false` here so\n `emitWorkflowScript` omits `isolation: \"worktree\"` for that plan (#2772 /\n #2285 finding 1). **Never** hardcode `true` — that would force worktree\n isolation on a plan the inline path explicitly keeps out of worktrees.\n\n4. **`$AGENT_SDK_VERSION`** — no longer built here; the router resolves it.\n\n**Agent SDK version:** the orchestrator has no *bash-computable* way to\nintrospect the live Agent SDK version — but the router runs in Node, so it\nresolves the version itself (#2590), in this order:\n\n1. an explicit `--agent-sdk-version ` (pin a version),\n2. `GSD_AGENT_SDK_VERSION`,\n3. the **installed** `@anthropic-ai/claude-agent-sdk` package version, read from\n its `package.json` on disk by walking `node_modules` up the tree. (Read\n directly rather than via `require.resolve`: the SDK's `exports` map does not\n expose `./package.json`, so `require.resolve` throws\n `ERR_PACKAGE_PATH_NOT_EXPORTED`.)\n\nPreviously nothing computed this at all, so gate 5 returned\n`agent_sdk_version_unknown` on **every** automated run and the Workflow backend\ncould never activate — while `gsd-tools capability state` still reported the\ncapability `active: true`. Fail-closed is preserved: when no version can be\nresolved, gate 5 still declines to `inline`. What changed is that a resolvable\nversion is now actually found, so a genuinely-too-old SDK reports\n`agent_sdk_version_below_floor` — the truthful reason — instead of `unknown`.\n\n**If `backend == \"workflow\"`:** run the emitted `script` via the Workflow tool\nfor THIS wave instead of the per-message `Agent()` loop in step 3. The script\ncomposes the SAME `gsd-executor` agent type the inline path uses, with\nworktree isolation applied PER PLAN from the manifest's `use_worktree` field\n(see `emitWorkflowScript`):\n\n- **waves → one or more sequential `parallel()` barriers** — each wave is a\n barrier group; when plans within a wave share `files_modified`, they are split\n into separate sequential stages within that wave's barrier.\n- **plans → `agent(brief, { agentType: 'gsd-executor', isolation: 'worktree' })`**\n when `use_worktree` is not `false`, or `agent(brief, { agentType: 'gsd-executor' })`\n (no isolation) when it is — so the produced `SUMMARY.md` and commits are\n identical to inline dispatch, INCLUDING the inline path's submodule safety\n gate (#2772 / #2285 finding 1).\n- **`files_modified` overlap → separate sequential stages** — the same overlap\n rule execute-phase already applies inline (step 1 of the wave loop).\n- **`resumeFromRunId`** — **pass `summary.resumeRunId` as the Workflow tool's\n `resumeFromRunId` INPUT when you invoke the tool.** It is a tool parameter,\n not a script function; the script deliberately does not call it (#2590 — doing\n so threw \"resumeFromRunId is not defined\" and rejected the entire script).\n Omitting it from the tool invocation silently regresses phase-resume to a\n no-op: an interrupted phase re-runs completed plans.\n\n### After the run: manifest bridge into the merge chain (#3302)\n\nThe single Workflow tool call replaces step 3's per-plan `Agent()` loop — which also\nmeans step 3's manifest bookkeeping (creation + per-agent recording) does NOT happen on\nthis path. The orchestrator MUST bridge the run's per-agent results into the SAME\nmanifest-scoped merge chain inline dispatch uses, before steps 4–5.8, which then run\nunchanged:\n\n1. **Create the manifest BEFORE invoking the tool** (this is step 3's creation block,\n which this path skips). When ANY plan in the wave has `use_worktree` not `false`:\n\n ```bash\n if [ -z \"${WAVE_WORKTREE_MANIFEST:-}\" ]; then\n M=$(mktemp \"${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX\") && mv \"$M\" \"$M.json\" && WAVE_WORKTREE_MANIFEST=\"$M.json\" || exit 1 # XXXXXX must be path-final on BSD/macOS (#1520)\n # Persist the dispatch-time orchestrator worktree root so wave-cleanup pins back\n # to the orchestrator's OWN worktree (#630), exactly as inline dispatch does.\n ORCH_ROOT=$(git rev-parse --show-toplevel)\n ORCH_ROOT=\"$ORCH_ROOT\" MANIFEST=\"$WAVE_WORKTREE_MANIFEST\" node -e 'const fs=require(\"fs\");fs.writeFileSync(process.env.MANIFEST,JSON.stringify({orchestrator_root:process.env.ORCH_ROOT||null,worktrees:[]})+\"\\n\")'\n export WAVE_WORKTREE_MANIFEST\n fi\n ```\n\n2. **Invoke the Workflow tool with the emitted script and\n `resumeFromRunId: summary.resumeRunId`.** The script top-level `return`s one entry\n per dispatched plan: `{ plan, expects_worktree, metadata }`. `metadata` is that\n plan's executor `` JSON (`{agent_id, worktree_path, branch,\n expected_base}` — captured by the executor itself per\n `agents/gsd-executor.md`), or `null` when the agent's result carried none\n (interrupted agent, resumed-from-cache plan, or a non-worktree plan).\n\n3. **Record every worktree plan** exactly as inline dispatch does at step 3's\n \"After each `Agent()` returns\" — one `worktree.record-agent` per returned entry\n with `expects_worktree: true` and complete metadata:\n\n ```bash\n gsd_run query worktree.record-agent --manifest \"$WAVE_WORKTREE_MANIFEST\" \\\n --agent-id \"\" --path \"\" \\\n --branch \"\" --base \"\" \\\n --files \"\"\n ```\n\n The verb's write-strict validation applies as inline: on a non-zero exit or any\n missing field, stop and ask for recovery — do not append an under-populated entry.\n\n4. **HALT on uncapturable metadata — never a silently-empty manifest (#3302).**\n After recording, the manifest must hold one entry per `expects_worktree: true`\n outcome (`summary.worktreePlans` from `resolve-wave-dispatch` is the expected\n count). Any shortfall — a `null` `metadata`, a missing/empty field, or a count\n mismatch — means commits are stranded on their `worktree-wf_*` branches and\n `worktree.cleanup-wave` would merge nothing while the phase looks green. STOP the\n phase with the failing plan id and the recovery hint below; do NOT run\n `worktree.cleanup-wave` and do NOT proceed to step 4.\n\n **Recovery hint:** the unmerged `worktree-wf_*` branch still holds the work. Recover\n the missing metadata from the run's per-agent result journal (`journal.jsonl` — one\n `{\"type\":\"result\",…}` line per agent — in the Workflow run's transcript dir), re-run\n `worktree.record-agent` by hand, then re-run cleanup. If the journal cannot be\n recovered either, merge the branch manually after review — never discard it.\n\n5. **Resume (`resumeFromRunId`).** Cached/resumed agents do not re-emit their final\n messages, so a previously-completed plan can return with `metadata: null`. Recover\n that plan's metadata from the ORIGINAL run's journal (same hint as above). If it\n cannot be recovered, fail loudly per rule 4 — a resumed run must never report\n success over silently-dropped agent work.\n\n6. **Non-worktree plans** (`expects_worktree: false` — `use_worktree: false` in the\n manifest): they ran without isolation; their commits are already on the main working\n tree. No record-agent entry, no manifest write.\n\nWith the manifest populated, steps 4–5.8 (wait/completion bookkeeping, step 5.5's\nmanifest-scoped `worktree.cleanup-wave`, post-merge gate, tracking update) run\nUNCHANGED — the Workflow backend replaces HOW agents are spawned and returns their\nmetadata; the merge chain itself is the inline path's own, now with real input.\n\n**If `backend == \"inline\"`** (any gate miss, or `resolve-wave-dispatch` itself\nunavailable/erroring): proceed to step 3's standard per-message `Agent()`\ndispatch — the default, byte-identical-to-today path. `onError: skip` on this\ncontribution means a `resolve-wave-dispatch` command failure is treated exactly\nlike an `inline` result, never as a fatal wave error.\n\n## Fallback contract\n\nDetection is fail-closed end-to-end: capability disabled, non-Claude runtime,\n`execution_backend:\"inline\"`, missing/incapable host descriptor, unknown or\nbelow-floor Agent SDK version, or an `emitWorkflowScript` failure on a malformed\nwave manifest — ANY of these degrades to `backend:\"inline\"` and execute-phase's\nstandard inline dispatch (step 3) runs unmodified. The Workflow backend never\npartially activates; the executor MUST NOT assume parallelism, a shared budget,\nor resume-from-run-id semantics when `backend == \"inline\"`.\n" + "inline": "# Claude orchestration — Workflow execution backend (BETA)\n\n> Injected at `execute:wave:pre` `into: executor` only when\n> `claude_orchestration.enabled` is true. Default-off; `onError: skip`.\n\n## When this contribution is active\n\nThe Claude orchestration capability is **default-off and BETA**. It activates only\nwhen ALL of the following hold:\n\n1. `claude_orchestration.enabled` is `true` in `.planning/config.json`, AND\n2. the active runtime is **Claude Code** (the Workflow tool is Claude / Agent\n SDK-specific), AND\n3. `claude_orchestration.execution_backend` resolves to `workflow` — either\n explicitly, or via `auto` — **and** the Agent SDK version is\n `>= claude_orchestration.min_agent_sdk_version` (default `0.3.149`). The SDK\n floor applies in both `auto` and `workflow` modes (fail-closed: a pre-release\n or older SDK never activates the preview backend).\n\nDetection is fail-closed: any miss degrades to **inline, manual, one-agent-per-\nmessage dispatch** — exactly today's behaviour. On a non-Claude runtime this\ncontribution is a no-op.\n\n## Why `execute:wave:pre` (not `execute:wave:post`)\n\nThis is a **dispatch-backend selector** — it decides HOW a wave's executor agents\nare spawned. That decision has to be made BEFORE the wave's `Agent()` calls in\n`execute-phase.md` step 3, not after the wave has already finished (#2285). The\ncapability previously registered at `execute:wave:post`, which fires only after\nworktree merge/post-merge tests/tracking updates — by then the wave was already\ndispatched inline, so the contribution was structurally unable to change how\ndispatch happened. This fragment is injected at the point that actually precedes\ndispatch.\n\n## What the orchestrator does when the Workflow backend is active\n\nBefore spawning executor agents for the current wave (execute-phase.md step 3),\nresolve the dispatch backend through the single composed CLI seam:\n\n```bash\ngsd-tools claude-orchestration resolve-wave-dispatch \\\n --waves \"$WAVE_MANIFEST_PATH\" --run-id \"$PHASE_RUN_ID\" \\\n --runtime \"$RUNTIME\" \\\n --phase-dir \"$PHASE_DIR\" --raw\n```\n\n`--agent-sdk-version` is no longer passed here (#2590). The router resolves the\ninstalled Agent SDK version itself; see **Agent SDK version** below. The former\n`${AGENT_SDK_VERSION:+--agent-sdk-version \"$AGENT_SDK_VERSION\"}` line was also\n**shell-dependent**: zsh does not word-split unquoted parameter expansions, so it\ncollapsed to a SINGLE argv element there, `argValue()` never matched, and the run\nfailed into `agent_sdk_version_unknown` — indistinguishable from genuinely\nunknown. Pass `--agent-sdk-version ` explicitly only to pin a version.\n\nThis composes `detectWorkflowBackend` (the gate ladder above) with\n`emitWorkflowScript` (the wave→plan mapping below) in ONE call — the pure\nfunction backing it is `resolveWaveDispatch` in\n`gsd-core/bin/lib/claude-orchestration.cjs`. Response shape:\n`{ backend: 'inline'|'workflow', reason, script?, summary? }`.\n\n### Manifest construction (`$WAVE_MANIFEST_PATH`, `$PHASE_RUN_ID`, `$PHASE_DIR`)\n\nThese are NOT pre-existing execute-phase.md variables — the orchestrator builds\nthem at this step, from data it already has in-context from `discover_and_group_plans`\n(the `PLAN_INDEX` JSON) and step 2.5 (the per-plan `USE_WORKTREES_FOR_PLAN` decision):\n\n1. **`$PHASE_DIR`** — reuse `{phase_dir}` from the `INIT` bundle (already loaded\n in the `initialize` step). No new value needed.\n\n2. **`$PHASE_RUN_ID`** — a stable identifier for THIS phase-execution attempt, so\n `resumeFromRunId` can resume an interrupted run without re-dispatching plans\n the Workflow tool already completed. Construct it deterministically —\n `execute-{phase_number}-{phase_slug}` — from `INIT`'s `phase_number`/`phase_slug`\n (both are already validated identifiers used elsewhere in this workflow, so\n they satisfy `emitWorkflowScript`'s `isScriptableIdentifier` check). Do NOT\n mint a new random id per wave — the SAME `$PHASE_RUN_ID` is reused for every\n wave in the phase so the Workflow tool can correctly track cross-wave resume\n state.\n\n3. **`$WAVE_MANIFEST_PATH`** — a fresh temp file for THIS wave's manifest (one\n wave = one `waves` array with a single entry, matching the wave-by-wave\n dispatch loop; do not batch multiple waves into one manifest — waves are\n dispatched in wave order, not all at once):\n\n ```bash\n WAVE_MANIFEST_PATH=$(mktemp \"${TMPDIR:-/tmp}/gsd-wave-dispatch-XXXXXX\") && mv \"$WAVE_MANIFEST_PATH\" \"$WAVE_MANIFEST_PATH.json\" && WAVE_MANIFEST_PATH=\"$WAVE_MANIFEST_PATH.json\"\n ```\n\n Then **use the Write tool** (not a bash/jq pipeline — the orchestrator already\n has every field parsed in-context) to write the manifest JSON to\n `$WAVE_MANIFEST_PATH`:\n\n ```json\n {\n \"waves\": [\n {\n \"id\": \"wave-{N}\",\n \"plans\": [\n {\n \"id\": \"{plan_id}\",\n \"brief\": \"{the SAME ... prompt block step 3 builds for this plan's inline Agent() call}\",\n \"files_modified\": [\"{from PLAN_INDEX.plans[].files_modified for this plan}\"],\n \"use_worktree\": {true unless step 2.5 set USE_WORKTREES_FOR_PLAN=false for this plan}\n }\n ]\n }\n ]\n }\n ```\n\n - **`id`** — the plan id from `PLAN_INDEX`, e.g. `\"01-01\"`.\n - **`brief`** — MUST carry the same task content as step 3's inline `Agent()`\n prompt (the ``/``/``/\n `` block, with `{plan_number}`/`{phase_number}`/\n `{phase_name}` substituted) — a short summary here would NOT reproduce\n step 3's behavior and would violate the \"identical artifacts\" contract.\n - **`files_modified`** — copy verbatim from the plan's `PLAN_INDEX` entry.\n - **`use_worktree`** — `true` for every plan UNLESS step 2.5's per-plan\n worktree gate (`execute-phase/steps/per-plan-worktree-gate.md`) set\n `USE_WORKTREES_FOR_PLAN=false` for that plan (submodule-touching plan, or\n project-level `USE_WORKTREES=false`) — in which case pass `false` here so\n `emitWorkflowScript` omits `isolation: \"worktree\"` for that plan (#2772 /\n #2285 finding 1). **Never** hardcode `true` — that would force worktree\n isolation on a plan the inline path explicitly keeps out of worktrees.\n\n4. **`$AGENT_SDK_VERSION`** — no longer built here; the router resolves it.\n\n**Agent SDK version:** the orchestrator has no *bash-computable* way to\nintrospect the live Agent SDK version — but the router runs in Node, so it\nresolves the version itself (#2590), in this order:\n\n1. an explicit `--agent-sdk-version ` (pin a version),\n2. `GSD_AGENT_SDK_VERSION`,\n3. the **installed** `@anthropic-ai/claude-agent-sdk` package version, read from\n its `package.json` on disk by walking `node_modules` up the tree. (Read\n directly rather than via `require.resolve`: the SDK's `exports` map does not\n expose `./package.json`, so `require.resolve` throws\n `ERR_PACKAGE_PATH_NOT_EXPORTED`.)\n\nPreviously nothing computed this at all, so gate 5 returned\n`agent_sdk_version_unknown` on **every** automated run and the Workflow backend\ncould never activate — while `gsd-tools capability state` still reported the\ncapability `active: true`. Fail-closed is preserved: when no version can be\nresolved, gate 5 still declines to `inline`. What changed is that a resolvable\nversion is now actually found, so a genuinely-too-old SDK reports\n`agent_sdk_version_below_floor` — the truthful reason — instead of `unknown`.\n\n**If `backend == \"workflow\"`:** run the emitted `script` via the Workflow tool\nfor THIS wave instead of the per-message `Agent()` loop in step 3. The script\ncomposes the SAME `gsd-executor` agent type the inline path uses, with\nworktree isolation applied PER PLAN from the manifest's `use_worktree` field\n(see `emitWorkflowScript`):\n\n- **waves → one or more sequential `parallel()` barriers** — each wave is a\n barrier group; when plans within a wave share `files_modified`, they are split\n into separate sequential stages within that wave's barrier.\n- **plans → `agent(brief, { agentType: 'gsd-executor', isolation: 'worktree' })`**\n when `use_worktree` is not `false`, or `agent(brief, { agentType: 'gsd-executor' })`\n (no isolation) when it is — so the produced `SUMMARY.md` and commits are\n identical to inline dispatch, INCLUDING the inline path's submodule safety\n gate (#2772 / #2285 finding 1).\n- **`files_modified` overlap → separate sequential stages** — the same overlap\n rule execute-phase already applies inline (step 1 of the wave loop).\n- **`resumeFromRunId`** — **pass `summary.resumeRunId` as the Workflow tool's\n `resumeFromRunId` INPUT when you invoke the tool.** It is a tool parameter,\n not a script function; the script deliberately does not call it (#2590 — doing\n so threw \"resumeFromRunId is not defined\" and rejected the entire script).\n Omitting it from the tool invocation silently regresses phase-resume to a\n no-op: an interrupted phase re-runs completed plans.\n\n### After the run: manifest bridge into the merge chain (#3302)\n\nThe single Workflow tool call replaces step 3's per-plan `Agent()` loop — which also\nmeans step 3's manifest bookkeeping (creation + per-agent recording) does NOT happen on\nthis path. The orchestrator MUST bridge the run's per-agent results into the SAME\nmanifest-scoped merge chain inline dispatch uses, before steps 4–5.8, which then run\nunchanged:\n\n1. **Create the manifest BEFORE invoking the tool** (this is step 3's creation block,\n which this path skips). When ANY plan in the wave has `use_worktree` not `false`:\n\n ```bash\n if [ -z \"${WAVE_WORKTREE_MANIFEST:-}\" ]; then\n M=$(mktemp \"${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX\") && mv \"$M\" \"$M.json\" && WAVE_WORKTREE_MANIFEST=\"$M.json\" || exit 1 # XXXXXX must be path-final on BSD/macOS (#1520)\n # Persist the dispatch-time orchestrator worktree root so wave-cleanup pins back\n # to the orchestrator's OWN worktree (#630), exactly as inline dispatch does.\n ORCH_ROOT=$(git rev-parse --show-toplevel)\n ORCH_ROOT=\"$ORCH_ROOT\" MANIFEST=\"$WAVE_WORKTREE_MANIFEST\" node -e 'const fs=require(\"fs\");fs.writeFileSync(process.env.MANIFEST,JSON.stringify({orchestrator_root:process.env.ORCH_ROOT||null,worktrees:[]})+\"\\n\")'\n export WAVE_WORKTREE_MANIFEST\n fi\n ```\n\n2. **Invoke the Workflow tool with the emitted script and\n `resumeFromRunId: summary.resumeRunId`.** The script top-level `return`s one entry\n per dispatched plan: `{ plan, expects_worktree, metadata }`. `metadata` is that\n plan's executor `` JSON (`{agent_id, worktree_path, branch,\n expected_base}` — captured by the executor itself per\n `agents/gsd-executor.md`), or `null` when the agent's result carried none\n (interrupted agent, resumed-from-cache plan, or a non-worktree plan).\n\n3. **Record every worktree plan** exactly as inline dispatch does at step 3's\n \"After each `Agent()` returns\" — one `worktree.record-agent` per returned entry\n with `expects_worktree: true` and complete metadata:\n\n ```bash\n gsd_run query worktree.record-agent --manifest \"$WAVE_WORKTREE_MANIFEST\" \\\n --agent-id \"\" --path \"\" \\\n --branch \"\" --base \"\" \\\n --files \"\" \\\n --deletions \"\"\n ```\n\n `--deletions` (#3003) carries the plan's declared `files_deleted` so a plan that scoped a file\n removal merges through `cleanup-wave` instead of being blocked. Unlike `--files` it is not\n advisory: omitting it leaves the deletions guard blocking on any deletion at all, so this\n dispatch path must pass it or plans declaring a removal fail to merge here while succeeding on\n the inline path.\n\n The verb's write-strict validation applies as inline: on a non-zero exit or any\n missing field, stop and ask for recovery — do not append an under-populated entry.\n\n4. **HALT on uncapturable metadata — never a silently-empty manifest (#3302).**\n After recording, the manifest must hold one entry per `expects_worktree: true`\n outcome (`summary.worktreePlans` from `resolve-wave-dispatch` is the expected\n count). Any shortfall — a `null` `metadata`, a missing/empty field, or a count\n mismatch — means commits are stranded on their `worktree-wf_*` branches and\n `worktree.cleanup-wave` would merge nothing while the phase looks green. STOP the\n phase with the failing plan id and the recovery hint below; do NOT run\n `worktree.cleanup-wave` and do NOT proceed to step 4.\n\n **Recovery hint:** the unmerged `worktree-wf_*` branch still holds the work. Recover\n the missing metadata from the run's per-agent result journal (`journal.jsonl` — one\n `{\"type\":\"result\",…}` line per agent — in the Workflow run's transcript dir), re-run\n `worktree.record-agent` by hand, then re-run cleanup. If the journal cannot be\n recovered either, merge the branch manually after review — never discard it.\n\n5. **Resume (`resumeFromRunId`).** Cached/resumed agents do not re-emit their final\n messages, so a previously-completed plan can return with `metadata: null`. Recover\n that plan's metadata from the ORIGINAL run's journal (same hint as above). If it\n cannot be recovered, fail loudly per rule 4 — a resumed run must never report\n success over silently-dropped agent work.\n\n6. **Non-worktree plans** (`expects_worktree: false` — `use_worktree: false` in the\n manifest): they ran without isolation; their commits are already on the main working\n tree. No record-agent entry, no manifest write.\n\nWith the manifest populated, steps 4–5.8 (wait/completion bookkeeping, step 5.5's\nmanifest-scoped `worktree.cleanup-wave`, post-merge gate, tracking update) run\nUNCHANGED — the Workflow backend replaces HOW agents are spawned and returns their\nmetadata; the merge chain itself is the inline path's own, now with real input.\n\n**If `backend == \"inline\"`** (any gate miss, or `resolve-wave-dispatch` itself\nunavailable/erroring): proceed to step 3's standard per-message `Agent()`\ndispatch — the default, byte-identical-to-today path. `onError: skip` on this\ncontribution means a `resolve-wave-dispatch` command failure is treated exactly\nlike an `inline` result, never as a fatal wave error.\n\n## Fallback contract\n\nDetection is fail-closed end-to-end: capability disabled, non-Claude runtime,\n`execution_backend:\"inline\"`, missing/incapable host descriptor, unknown or\nbelow-floor Agent SDK version, or an `emitWorkflowScript` failure on a malformed\nwave manifest — ANY of these degrades to `backend:\"inline\"` and execute-phase's\nstandard inline dispatch (step 3) runs unmodified. The Workflow backend never\npartially activates; the executor MUST NOT assume parallelism, a shared budget,\nor resume-from-run-id semantics when `backend == \"inline\"`.\n" }, "produces": [], "consumes": [ @@ -4369,7 +4369,7 @@ const byLoopPoint = { "into": "executor", "fragment": { "path": "fragments/execute-wave-pre.md", - "inline": "# Claude orchestration — Workflow execution backend (BETA)\n\n> Injected at `execute:wave:pre` `into: executor` only when\n> `claude_orchestration.enabled` is true. Default-off; `onError: skip`.\n\n## When this contribution is active\n\nThe Claude orchestration capability is **default-off and BETA**. It activates only\nwhen ALL of the following hold:\n\n1. `claude_orchestration.enabled` is `true` in `.planning/config.json`, AND\n2. the active runtime is **Claude Code** (the Workflow tool is Claude / Agent\n SDK-specific), AND\n3. `claude_orchestration.execution_backend` resolves to `workflow` — either\n explicitly, or via `auto` — **and** the Agent SDK version is\n `>= claude_orchestration.min_agent_sdk_version` (default `0.3.149`). The SDK\n floor applies in both `auto` and `workflow` modes (fail-closed: a pre-release\n or older SDK never activates the preview backend).\n\nDetection is fail-closed: any miss degrades to **inline, manual, one-agent-per-\nmessage dispatch** — exactly today's behaviour. On a non-Claude runtime this\ncontribution is a no-op.\n\n## Why `execute:wave:pre` (not `execute:wave:post`)\n\nThis is a **dispatch-backend selector** — it decides HOW a wave's executor agents\nare spawned. That decision has to be made BEFORE the wave's `Agent()` calls in\n`execute-phase.md` step 3, not after the wave has already finished (#2285). The\ncapability previously registered at `execute:wave:post`, which fires only after\nworktree merge/post-merge tests/tracking updates — by then the wave was already\ndispatched inline, so the contribution was structurally unable to change how\ndispatch happened. This fragment is injected at the point that actually precedes\ndispatch.\n\n## What the orchestrator does when the Workflow backend is active\n\nBefore spawning executor agents for the current wave (execute-phase.md step 3),\nresolve the dispatch backend through the single composed CLI seam:\n\n```bash\ngsd-tools claude-orchestration resolve-wave-dispatch \\\n --waves \"$WAVE_MANIFEST_PATH\" --run-id \"$PHASE_RUN_ID\" \\\n --runtime \"$RUNTIME\" \\\n --phase-dir \"$PHASE_DIR\" --raw\n```\n\n`--agent-sdk-version` is no longer passed here (#2590). The router resolves the\ninstalled Agent SDK version itself; see **Agent SDK version** below. The former\n`${AGENT_SDK_VERSION:+--agent-sdk-version \"$AGENT_SDK_VERSION\"}` line was also\n**shell-dependent**: zsh does not word-split unquoted parameter expansions, so it\ncollapsed to a SINGLE argv element there, `argValue()` never matched, and the run\nfailed into `agent_sdk_version_unknown` — indistinguishable from genuinely\nunknown. Pass `--agent-sdk-version ` explicitly only to pin a version.\n\nThis composes `detectWorkflowBackend` (the gate ladder above) with\n`emitWorkflowScript` (the wave→plan mapping below) in ONE call — the pure\nfunction backing it is `resolveWaveDispatch` in\n`gsd-core/bin/lib/claude-orchestration.cjs`. Response shape:\n`{ backend: 'inline'|'workflow', reason, script?, summary? }`.\n\n### Manifest construction (`$WAVE_MANIFEST_PATH`, `$PHASE_RUN_ID`, `$PHASE_DIR`)\n\nThese are NOT pre-existing execute-phase.md variables — the orchestrator builds\nthem at this step, from data it already has in-context from `discover_and_group_plans`\n(the `PLAN_INDEX` JSON) and step 2.5 (the per-plan `USE_WORKTREES_FOR_PLAN` decision):\n\n1. **`$PHASE_DIR`** — reuse `{phase_dir}` from the `INIT` bundle (already loaded\n in the `initialize` step). No new value needed.\n\n2. **`$PHASE_RUN_ID`** — a stable identifier for THIS phase-execution attempt, so\n `resumeFromRunId` can resume an interrupted run without re-dispatching plans\n the Workflow tool already completed. Construct it deterministically —\n `execute-{phase_number}-{phase_slug}` — from `INIT`'s `phase_number`/`phase_slug`\n (both are already validated identifiers used elsewhere in this workflow, so\n they satisfy `emitWorkflowScript`'s `isScriptableIdentifier` check). Do NOT\n mint a new random id per wave — the SAME `$PHASE_RUN_ID` is reused for every\n wave in the phase so the Workflow tool can correctly track cross-wave resume\n state.\n\n3. **`$WAVE_MANIFEST_PATH`** — a fresh temp file for THIS wave's manifest (one\n wave = one `waves` array with a single entry, matching the wave-by-wave\n dispatch loop; do not batch multiple waves into one manifest — waves are\n dispatched in wave order, not all at once):\n\n ```bash\n WAVE_MANIFEST_PATH=$(mktemp \"${TMPDIR:-/tmp}/gsd-wave-dispatch-XXXXXX\") && mv \"$WAVE_MANIFEST_PATH\" \"$WAVE_MANIFEST_PATH.json\" && WAVE_MANIFEST_PATH=\"$WAVE_MANIFEST_PATH.json\"\n ```\n\n Then **use the Write tool** (not a bash/jq pipeline — the orchestrator already\n has every field parsed in-context) to write the manifest JSON to\n `$WAVE_MANIFEST_PATH`:\n\n ```json\n {\n \"waves\": [\n {\n \"id\": \"wave-{N}\",\n \"plans\": [\n {\n \"id\": \"{plan_id}\",\n \"brief\": \"{the SAME ... prompt block step 3 builds for this plan's inline Agent() call}\",\n \"files_modified\": [\"{from PLAN_INDEX.plans[].files_modified for this plan}\"],\n \"use_worktree\": {true unless step 2.5 set USE_WORKTREES_FOR_PLAN=false for this plan}\n }\n ]\n }\n ]\n }\n ```\n\n - **`id`** — the plan id from `PLAN_INDEX`, e.g. `\"01-01\"`.\n - **`brief`** — MUST carry the same task content as step 3's inline `Agent()`\n prompt (the ``/``/``/\n `` block, with `{plan_number}`/`{phase_number}`/\n `{phase_name}` substituted) — a short summary here would NOT reproduce\n step 3's behavior and would violate the \"identical artifacts\" contract.\n - **`files_modified`** — copy verbatim from the plan's `PLAN_INDEX` entry.\n - **`use_worktree`** — `true` for every plan UNLESS step 2.5's per-plan\n worktree gate (`execute-phase/steps/per-plan-worktree-gate.md`) set\n `USE_WORKTREES_FOR_PLAN=false` for that plan (submodule-touching plan, or\n project-level `USE_WORKTREES=false`) — in which case pass `false` here so\n `emitWorkflowScript` omits `isolation: \"worktree\"` for that plan (#2772 /\n #2285 finding 1). **Never** hardcode `true` — that would force worktree\n isolation on a plan the inline path explicitly keeps out of worktrees.\n\n4. **`$AGENT_SDK_VERSION`** — no longer built here; the router resolves it.\n\n**Agent SDK version:** the orchestrator has no *bash-computable* way to\nintrospect the live Agent SDK version — but the router runs in Node, so it\nresolves the version itself (#2590), in this order:\n\n1. an explicit `--agent-sdk-version ` (pin a version),\n2. `GSD_AGENT_SDK_VERSION`,\n3. the **installed** `@anthropic-ai/claude-agent-sdk` package version, read from\n its `package.json` on disk by walking `node_modules` up the tree. (Read\n directly rather than via `require.resolve`: the SDK's `exports` map does not\n expose `./package.json`, so `require.resolve` throws\n `ERR_PACKAGE_PATH_NOT_EXPORTED`.)\n\nPreviously nothing computed this at all, so gate 5 returned\n`agent_sdk_version_unknown` on **every** automated run and the Workflow backend\ncould never activate — while `gsd-tools capability state` still reported the\ncapability `active: true`. Fail-closed is preserved: when no version can be\nresolved, gate 5 still declines to `inline`. What changed is that a resolvable\nversion is now actually found, so a genuinely-too-old SDK reports\n`agent_sdk_version_below_floor` — the truthful reason — instead of `unknown`.\n\n**If `backend == \"workflow\"`:** run the emitted `script` via the Workflow tool\nfor THIS wave instead of the per-message `Agent()` loop in step 3. The script\ncomposes the SAME `gsd-executor` agent type the inline path uses, with\nworktree isolation applied PER PLAN from the manifest's `use_worktree` field\n(see `emitWorkflowScript`):\n\n- **waves → one or more sequential `parallel()` barriers** — each wave is a\n barrier group; when plans within a wave share `files_modified`, they are split\n into separate sequential stages within that wave's barrier.\n- **plans → `agent(brief, { agentType: 'gsd-executor', isolation: 'worktree' })`**\n when `use_worktree` is not `false`, or `agent(brief, { agentType: 'gsd-executor' })`\n (no isolation) when it is — so the produced `SUMMARY.md` and commits are\n identical to inline dispatch, INCLUDING the inline path's submodule safety\n gate (#2772 / #2285 finding 1).\n- **`files_modified` overlap → separate sequential stages** — the same overlap\n rule execute-phase already applies inline (step 1 of the wave loop).\n- **`resumeFromRunId`** — **pass `summary.resumeRunId` as the Workflow tool's\n `resumeFromRunId` INPUT when you invoke the tool.** It is a tool parameter,\n not a script function; the script deliberately does not call it (#2590 — doing\n so threw \"resumeFromRunId is not defined\" and rejected the entire script).\n Omitting it from the tool invocation silently regresses phase-resume to a\n no-op: an interrupted phase re-runs completed plans.\n\n### After the run: manifest bridge into the merge chain (#3302)\n\nThe single Workflow tool call replaces step 3's per-plan `Agent()` loop — which also\nmeans step 3's manifest bookkeeping (creation + per-agent recording) does NOT happen on\nthis path. The orchestrator MUST bridge the run's per-agent results into the SAME\nmanifest-scoped merge chain inline dispatch uses, before steps 4–5.8, which then run\nunchanged:\n\n1. **Create the manifest BEFORE invoking the tool** (this is step 3's creation block,\n which this path skips). When ANY plan in the wave has `use_worktree` not `false`:\n\n ```bash\n if [ -z \"${WAVE_WORKTREE_MANIFEST:-}\" ]; then\n M=$(mktemp \"${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX\") && mv \"$M\" \"$M.json\" && WAVE_WORKTREE_MANIFEST=\"$M.json\" || exit 1 # XXXXXX must be path-final on BSD/macOS (#1520)\n # Persist the dispatch-time orchestrator worktree root so wave-cleanup pins back\n # to the orchestrator's OWN worktree (#630), exactly as inline dispatch does.\n ORCH_ROOT=$(git rev-parse --show-toplevel)\n ORCH_ROOT=\"$ORCH_ROOT\" MANIFEST=\"$WAVE_WORKTREE_MANIFEST\" node -e 'const fs=require(\"fs\");fs.writeFileSync(process.env.MANIFEST,JSON.stringify({orchestrator_root:process.env.ORCH_ROOT||null,worktrees:[]})+\"\\n\")'\n export WAVE_WORKTREE_MANIFEST\n fi\n ```\n\n2. **Invoke the Workflow tool with the emitted script and\n `resumeFromRunId: summary.resumeRunId`.** The script top-level `return`s one entry\n per dispatched plan: `{ plan, expects_worktree, metadata }`. `metadata` is that\n plan's executor `` JSON (`{agent_id, worktree_path, branch,\n expected_base}` — captured by the executor itself per\n `agents/gsd-executor.md`), or `null` when the agent's result carried none\n (interrupted agent, resumed-from-cache plan, or a non-worktree plan).\n\n3. **Record every worktree plan** exactly as inline dispatch does at step 3's\n \"After each `Agent()` returns\" — one `worktree.record-agent` per returned entry\n with `expects_worktree: true` and complete metadata:\n\n ```bash\n gsd_run query worktree.record-agent --manifest \"$WAVE_WORKTREE_MANIFEST\" \\\n --agent-id \"\" --path \"\" \\\n --branch \"\" --base \"\" \\\n --files \"\"\n ```\n\n The verb's write-strict validation applies as inline: on a non-zero exit or any\n missing field, stop and ask for recovery — do not append an under-populated entry.\n\n4. **HALT on uncapturable metadata — never a silently-empty manifest (#3302).**\n After recording, the manifest must hold one entry per `expects_worktree: true`\n outcome (`summary.worktreePlans` from `resolve-wave-dispatch` is the expected\n count). Any shortfall — a `null` `metadata`, a missing/empty field, or a count\n mismatch — means commits are stranded on their `worktree-wf_*` branches and\n `worktree.cleanup-wave` would merge nothing while the phase looks green. STOP the\n phase with the failing plan id and the recovery hint below; do NOT run\n `worktree.cleanup-wave` and do NOT proceed to step 4.\n\n **Recovery hint:** the unmerged `worktree-wf_*` branch still holds the work. Recover\n the missing metadata from the run's per-agent result journal (`journal.jsonl` — one\n `{\"type\":\"result\",…}` line per agent — in the Workflow run's transcript dir), re-run\n `worktree.record-agent` by hand, then re-run cleanup. If the journal cannot be\n recovered either, merge the branch manually after review — never discard it.\n\n5. **Resume (`resumeFromRunId`).** Cached/resumed agents do not re-emit their final\n messages, so a previously-completed plan can return with `metadata: null`. Recover\n that plan's metadata from the ORIGINAL run's journal (same hint as above). If it\n cannot be recovered, fail loudly per rule 4 — a resumed run must never report\n success over silently-dropped agent work.\n\n6. **Non-worktree plans** (`expects_worktree: false` — `use_worktree: false` in the\n manifest): they ran without isolation; their commits are already on the main working\n tree. No record-agent entry, no manifest write.\n\nWith the manifest populated, steps 4–5.8 (wait/completion bookkeeping, step 5.5's\nmanifest-scoped `worktree.cleanup-wave`, post-merge gate, tracking update) run\nUNCHANGED — the Workflow backend replaces HOW agents are spawned and returns their\nmetadata; the merge chain itself is the inline path's own, now with real input.\n\n**If `backend == \"inline\"`** (any gate miss, or `resolve-wave-dispatch` itself\nunavailable/erroring): proceed to step 3's standard per-message `Agent()`\ndispatch — the default, byte-identical-to-today path. `onError: skip` on this\ncontribution means a `resolve-wave-dispatch` command failure is treated exactly\nlike an `inline` result, never as a fatal wave error.\n\n## Fallback contract\n\nDetection is fail-closed end-to-end: capability disabled, non-Claude runtime,\n`execution_backend:\"inline\"`, missing/incapable host descriptor, unknown or\nbelow-floor Agent SDK version, or an `emitWorkflowScript` failure on a malformed\nwave manifest — ANY of these degrades to `backend:\"inline\"` and execute-phase's\nstandard inline dispatch (step 3) runs unmodified. The Workflow backend never\npartially activates; the executor MUST NOT assume parallelism, a shared budget,\nor resume-from-run-id semantics when `backend == \"inline\"`.\n" + "inline": "# Claude orchestration — Workflow execution backend (BETA)\n\n> Injected at `execute:wave:pre` `into: executor` only when\n> `claude_orchestration.enabled` is true. Default-off; `onError: skip`.\n\n## When this contribution is active\n\nThe Claude orchestration capability is **default-off and BETA**. It activates only\nwhen ALL of the following hold:\n\n1. `claude_orchestration.enabled` is `true` in `.planning/config.json`, AND\n2. the active runtime is **Claude Code** (the Workflow tool is Claude / Agent\n SDK-specific), AND\n3. `claude_orchestration.execution_backend` resolves to `workflow` — either\n explicitly, or via `auto` — **and** the Agent SDK version is\n `>= claude_orchestration.min_agent_sdk_version` (default `0.3.149`). The SDK\n floor applies in both `auto` and `workflow` modes (fail-closed: a pre-release\n or older SDK never activates the preview backend).\n\nDetection is fail-closed: any miss degrades to **inline, manual, one-agent-per-\nmessage dispatch** — exactly today's behaviour. On a non-Claude runtime this\ncontribution is a no-op.\n\n## Why `execute:wave:pre` (not `execute:wave:post`)\n\nThis is a **dispatch-backend selector** — it decides HOW a wave's executor agents\nare spawned. That decision has to be made BEFORE the wave's `Agent()` calls in\n`execute-phase.md` step 3, not after the wave has already finished (#2285). The\ncapability previously registered at `execute:wave:post`, which fires only after\nworktree merge/post-merge tests/tracking updates — by then the wave was already\ndispatched inline, so the contribution was structurally unable to change how\ndispatch happened. This fragment is injected at the point that actually precedes\ndispatch.\n\n## What the orchestrator does when the Workflow backend is active\n\nBefore spawning executor agents for the current wave (execute-phase.md step 3),\nresolve the dispatch backend through the single composed CLI seam:\n\n```bash\ngsd-tools claude-orchestration resolve-wave-dispatch \\\n --waves \"$WAVE_MANIFEST_PATH\" --run-id \"$PHASE_RUN_ID\" \\\n --runtime \"$RUNTIME\" \\\n --phase-dir \"$PHASE_DIR\" --raw\n```\n\n`--agent-sdk-version` is no longer passed here (#2590). The router resolves the\ninstalled Agent SDK version itself; see **Agent SDK version** below. The former\n`${AGENT_SDK_VERSION:+--agent-sdk-version \"$AGENT_SDK_VERSION\"}` line was also\n**shell-dependent**: zsh does not word-split unquoted parameter expansions, so it\ncollapsed to a SINGLE argv element there, `argValue()` never matched, and the run\nfailed into `agent_sdk_version_unknown` — indistinguishable from genuinely\nunknown. Pass `--agent-sdk-version ` explicitly only to pin a version.\n\nThis composes `detectWorkflowBackend` (the gate ladder above) with\n`emitWorkflowScript` (the wave→plan mapping below) in ONE call — the pure\nfunction backing it is `resolveWaveDispatch` in\n`gsd-core/bin/lib/claude-orchestration.cjs`. Response shape:\n`{ backend: 'inline'|'workflow', reason, script?, summary? }`.\n\n### Manifest construction (`$WAVE_MANIFEST_PATH`, `$PHASE_RUN_ID`, `$PHASE_DIR`)\n\nThese are NOT pre-existing execute-phase.md variables — the orchestrator builds\nthem at this step, from data it already has in-context from `discover_and_group_plans`\n(the `PLAN_INDEX` JSON) and step 2.5 (the per-plan `USE_WORKTREES_FOR_PLAN` decision):\n\n1. **`$PHASE_DIR`** — reuse `{phase_dir}` from the `INIT` bundle (already loaded\n in the `initialize` step). No new value needed.\n\n2. **`$PHASE_RUN_ID`** — a stable identifier for THIS phase-execution attempt, so\n `resumeFromRunId` can resume an interrupted run without re-dispatching plans\n the Workflow tool already completed. Construct it deterministically —\n `execute-{phase_number}-{phase_slug}` — from `INIT`'s `phase_number`/`phase_slug`\n (both are already validated identifiers used elsewhere in this workflow, so\n they satisfy `emitWorkflowScript`'s `isScriptableIdentifier` check). Do NOT\n mint a new random id per wave — the SAME `$PHASE_RUN_ID` is reused for every\n wave in the phase so the Workflow tool can correctly track cross-wave resume\n state.\n\n3. **`$WAVE_MANIFEST_PATH`** — a fresh temp file for THIS wave's manifest (one\n wave = one `waves` array with a single entry, matching the wave-by-wave\n dispatch loop; do not batch multiple waves into one manifest — waves are\n dispatched in wave order, not all at once):\n\n ```bash\n WAVE_MANIFEST_PATH=$(mktemp \"${TMPDIR:-/tmp}/gsd-wave-dispatch-XXXXXX\") && mv \"$WAVE_MANIFEST_PATH\" \"$WAVE_MANIFEST_PATH.json\" && WAVE_MANIFEST_PATH=\"$WAVE_MANIFEST_PATH.json\"\n ```\n\n Then **use the Write tool** (not a bash/jq pipeline — the orchestrator already\n has every field parsed in-context) to write the manifest JSON to\n `$WAVE_MANIFEST_PATH`:\n\n ```json\n {\n \"waves\": [\n {\n \"id\": \"wave-{N}\",\n \"plans\": [\n {\n \"id\": \"{plan_id}\",\n \"brief\": \"{the SAME ... prompt block step 3 builds for this plan's inline Agent() call}\",\n \"files_modified\": [\"{from PLAN_INDEX.plans[].files_modified for this plan}\"],\n \"use_worktree\": {true unless step 2.5 set USE_WORKTREES_FOR_PLAN=false for this plan}\n }\n ]\n }\n ]\n }\n ```\n\n - **`id`** — the plan id from `PLAN_INDEX`, e.g. `\"01-01\"`.\n - **`brief`** — MUST carry the same task content as step 3's inline `Agent()`\n prompt (the ``/``/``/\n `` block, with `{plan_number}`/`{phase_number}`/\n `{phase_name}` substituted) — a short summary here would NOT reproduce\n step 3's behavior and would violate the \"identical artifacts\" contract.\n - **`files_modified`** — copy verbatim from the plan's `PLAN_INDEX` entry.\n - **`use_worktree`** — `true` for every plan UNLESS step 2.5's per-plan\n worktree gate (`execute-phase/steps/per-plan-worktree-gate.md`) set\n `USE_WORKTREES_FOR_PLAN=false` for that plan (submodule-touching plan, or\n project-level `USE_WORKTREES=false`) — in which case pass `false` here so\n `emitWorkflowScript` omits `isolation: \"worktree\"` for that plan (#2772 /\n #2285 finding 1). **Never** hardcode `true` — that would force worktree\n isolation on a plan the inline path explicitly keeps out of worktrees.\n\n4. **`$AGENT_SDK_VERSION`** — no longer built here; the router resolves it.\n\n**Agent SDK version:** the orchestrator has no *bash-computable* way to\nintrospect the live Agent SDK version — but the router runs in Node, so it\nresolves the version itself (#2590), in this order:\n\n1. an explicit `--agent-sdk-version ` (pin a version),\n2. `GSD_AGENT_SDK_VERSION`,\n3. the **installed** `@anthropic-ai/claude-agent-sdk` package version, read from\n its `package.json` on disk by walking `node_modules` up the tree. (Read\n directly rather than via `require.resolve`: the SDK's `exports` map does not\n expose `./package.json`, so `require.resolve` throws\n `ERR_PACKAGE_PATH_NOT_EXPORTED`.)\n\nPreviously nothing computed this at all, so gate 5 returned\n`agent_sdk_version_unknown` on **every** automated run and the Workflow backend\ncould never activate — while `gsd-tools capability state` still reported the\ncapability `active: true`. Fail-closed is preserved: when no version can be\nresolved, gate 5 still declines to `inline`. What changed is that a resolvable\nversion is now actually found, so a genuinely-too-old SDK reports\n`agent_sdk_version_below_floor` — the truthful reason — instead of `unknown`.\n\n**If `backend == \"workflow\"`:** run the emitted `script` via the Workflow tool\nfor THIS wave instead of the per-message `Agent()` loop in step 3. The script\ncomposes the SAME `gsd-executor` agent type the inline path uses, with\nworktree isolation applied PER PLAN from the manifest's `use_worktree` field\n(see `emitWorkflowScript`):\n\n- **waves → one or more sequential `parallel()` barriers** — each wave is a\n barrier group; when plans within a wave share `files_modified`, they are split\n into separate sequential stages within that wave's barrier.\n- **plans → `agent(brief, { agentType: 'gsd-executor', isolation: 'worktree' })`**\n when `use_worktree` is not `false`, or `agent(brief, { agentType: 'gsd-executor' })`\n (no isolation) when it is — so the produced `SUMMARY.md` and commits are\n identical to inline dispatch, INCLUDING the inline path's submodule safety\n gate (#2772 / #2285 finding 1).\n- **`files_modified` overlap → separate sequential stages** — the same overlap\n rule execute-phase already applies inline (step 1 of the wave loop).\n- **`resumeFromRunId`** — **pass `summary.resumeRunId` as the Workflow tool's\n `resumeFromRunId` INPUT when you invoke the tool.** It is a tool parameter,\n not a script function; the script deliberately does not call it (#2590 — doing\n so threw \"resumeFromRunId is not defined\" and rejected the entire script).\n Omitting it from the tool invocation silently regresses phase-resume to a\n no-op: an interrupted phase re-runs completed plans.\n\n### After the run: manifest bridge into the merge chain (#3302)\n\nThe single Workflow tool call replaces step 3's per-plan `Agent()` loop — which also\nmeans step 3's manifest bookkeeping (creation + per-agent recording) does NOT happen on\nthis path. The orchestrator MUST bridge the run's per-agent results into the SAME\nmanifest-scoped merge chain inline dispatch uses, before steps 4–5.8, which then run\nunchanged:\n\n1. **Create the manifest BEFORE invoking the tool** (this is step 3's creation block,\n which this path skips). When ANY plan in the wave has `use_worktree` not `false`:\n\n ```bash\n if [ -z \"${WAVE_WORKTREE_MANIFEST:-}\" ]; then\n M=$(mktemp \"${TMPDIR:-/tmp}/gsd-worktree-wave-XXXXXX\") && mv \"$M\" \"$M.json\" && WAVE_WORKTREE_MANIFEST=\"$M.json\" || exit 1 # XXXXXX must be path-final on BSD/macOS (#1520)\n # Persist the dispatch-time orchestrator worktree root so wave-cleanup pins back\n # to the orchestrator's OWN worktree (#630), exactly as inline dispatch does.\n ORCH_ROOT=$(git rev-parse --show-toplevel)\n ORCH_ROOT=\"$ORCH_ROOT\" MANIFEST=\"$WAVE_WORKTREE_MANIFEST\" node -e 'const fs=require(\"fs\");fs.writeFileSync(process.env.MANIFEST,JSON.stringify({orchestrator_root:process.env.ORCH_ROOT||null,worktrees:[]})+\"\\n\")'\n export WAVE_WORKTREE_MANIFEST\n fi\n ```\n\n2. **Invoke the Workflow tool with the emitted script and\n `resumeFromRunId: summary.resumeRunId`.** The script top-level `return`s one entry\n per dispatched plan: `{ plan, expects_worktree, metadata }`. `metadata` is that\n plan's executor `` JSON (`{agent_id, worktree_path, branch,\n expected_base}` — captured by the executor itself per\n `agents/gsd-executor.md`), or `null` when the agent's result carried none\n (interrupted agent, resumed-from-cache plan, or a non-worktree plan).\n\n3. **Record every worktree plan** exactly as inline dispatch does at step 3's\n \"After each `Agent()` returns\" — one `worktree.record-agent` per returned entry\n with `expects_worktree: true` and complete metadata:\n\n ```bash\n gsd_run query worktree.record-agent --manifest \"$WAVE_WORKTREE_MANIFEST\" \\\n --agent-id \"\" --path \"\" \\\n --branch \"\" --base \"\" \\\n --files \"\" \\\n --deletions \"\"\n ```\n\n `--deletions` (#3003) carries the plan's declared `files_deleted` so a plan that scoped a file\n removal merges through `cleanup-wave` instead of being blocked. Unlike `--files` it is not\n advisory: omitting it leaves the deletions guard blocking on any deletion at all, so this\n dispatch path must pass it or plans declaring a removal fail to merge here while succeeding on\n the inline path.\n\n The verb's write-strict validation applies as inline: on a non-zero exit or any\n missing field, stop and ask for recovery — do not append an under-populated entry.\n\n4. **HALT on uncapturable metadata — never a silently-empty manifest (#3302).**\n After recording, the manifest must hold one entry per `expects_worktree: true`\n outcome (`summary.worktreePlans` from `resolve-wave-dispatch` is the expected\n count). Any shortfall — a `null` `metadata`, a missing/empty field, or a count\n mismatch — means commits are stranded on their `worktree-wf_*` branches and\n `worktree.cleanup-wave` would merge nothing while the phase looks green. STOP the\n phase with the failing plan id and the recovery hint below; do NOT run\n `worktree.cleanup-wave` and do NOT proceed to step 4.\n\n **Recovery hint:** the unmerged `worktree-wf_*` branch still holds the work. Recover\n the missing metadata from the run's per-agent result journal (`journal.jsonl` — one\n `{\"type\":\"result\",…}` line per agent — in the Workflow run's transcript dir), re-run\n `worktree.record-agent` by hand, then re-run cleanup. If the journal cannot be\n recovered either, merge the branch manually after review — never discard it.\n\n5. **Resume (`resumeFromRunId`).** Cached/resumed agents do not re-emit their final\n messages, so a previously-completed plan can return with `metadata: null`. Recover\n that plan's metadata from the ORIGINAL run's journal (same hint as above). If it\n cannot be recovered, fail loudly per rule 4 — a resumed run must never report\n success over silently-dropped agent work.\n\n6. **Non-worktree plans** (`expects_worktree: false` — `use_worktree: false` in the\n manifest): they ran without isolation; their commits are already on the main working\n tree. No record-agent entry, no manifest write.\n\nWith the manifest populated, steps 4–5.8 (wait/completion bookkeeping, step 5.5's\nmanifest-scoped `worktree.cleanup-wave`, post-merge gate, tracking update) run\nUNCHANGED — the Workflow backend replaces HOW agents are spawned and returns their\nmetadata; the merge chain itself is the inline path's own, now with real input.\n\n**If `backend == \"inline\"`** (any gate miss, or `resolve-wave-dispatch` itself\nunavailable/erroring): proceed to step 3's standard per-message `Agent()`\ndispatch — the default, byte-identical-to-today path. `onError: skip` on this\ncontribution means a `resolve-wave-dispatch` command failure is treated exactly\nlike an `inline` result, never as a fatal wave error.\n\n## Fallback contract\n\nDetection is fail-closed end-to-end: capability disabled, non-Claude runtime,\n`execution_backend:\"inline\"`, missing/incapable host descriptor, unknown or\nbelow-floor Agent SDK version, or an `emitWorkflowScript` failure on a malformed\nwave manifest — ANY of these degrades to `backend:\"inline\"` and execute-phase's\nstandard inline dispatch (step 3) runs unmodified. The Workflow backend never\npartially activates; the executor MUST NOT assume parallelism, a shared budget,\nor resume-from-run-id semantics when `backend == \"inline\"`.\n" }, "produces": [], "consumes": [ diff --git a/gsd-core/templates/phase-prompt.md b/gsd-core/templates/phase-prompt.md index 064729fb5..2d1dad3ba 100644 --- a/gsd-core/templates/phase-prompt.md +++ b/gsd-core/templates/phase-prompt.md @@ -19,6 +19,9 @@ type: execute wave: N # Execution wave (1, 2, 3...). Pre-computed at plan time. depends_on: [] # Plan IDs this plan requires (e.g., ["01-01"]). files_modified: [] # Files this plan modifies. +files_deleted: [] # OPTIONAL. Files this plan REMOVES. Declaring a path here is what + # lets worktree cleanup-wave merge the branch that deletes it; an + # undeclared deletion still blocks. Exact paths, not globs or dirs. autonomous: true # false if plan has checkpoints requiring user interaction requirements: [] # REQUIRED — Requirement IDs from ROADMAP this plan addresses. MUST NOT be empty. user_setup: [] # Human-required setup Claude cannot automate (see below) diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 0ffe69c48..29e1e2b3c 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -778,7 +778,7 @@ increases monotonically across waves. `{status}` is `complete` (success), ) ``` - After each `Agent()` returns, parse executor-returned worktree metadata (``) before harness metadata, then record the `{agent_id, worktree_path, branch, expected_base}` entry with `gsd_run query worktree.record-agent --manifest "$WAVE_WORKTREE_MANIFEST" --agent-id … --path … --branch … --base … --files "$PLAN_FILES"`. The verb validates every field at write time using the `cleanup-wave` reader's own rules (write-strict `--agent-id`), failing loudly with a recovery hint rather than appending an under-populated entry the reader would later drop silently. On a non-zero exit or any missing field: stop and ask for recovery instead of scanning worktrees. + After each `Agent()` returns, parse executor-returned worktree metadata (``) before harness metadata, then record the `{agent_id, worktree_path, branch, expected_base}` entry with `gsd_run query worktree.record-agent --manifest "$WAVE_WORKTREE_MANIFEST" --agent-id … --path … --branch … --base … --files "$PLAN_FILES" --deletions "$PLAN_DELETIONS"`. The verb validates every field at write time using the `cleanup-wave` reader's own rules (write-strict `--agent-id`), failing loudly with a recovery hint rather than appending an under-populated entry the reader would later drop silently. On a non-zero exit or any missing field: stop and ask for recovery instead of scanning worktrees. > **Worktree recovery policy (#48 + #1292):** See `execute-phase/steps/worktree-recovery-policy.md` — FAIL-CLOSED rule for base/HEAD-namespace mismatches AND isolated-run fail-safe recovery. diff --git a/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md b/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md index fcf5522a2..6143c1c58 100644 --- a/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md +++ b/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md @@ -276,7 +276,8 @@ CREATE_JSON=$(gsd_run query worktree.create \ --branch "$WT_BRANCH" \ --base "$EXPECTED_BASE" \ --root "$ORCH_ROOT" \ - --files "$PLAN_FILES" 2>&1) || { + --files "$PLAN_FILES" \ + --deletions "$PLAN_DELETIONS" 2>&1) || { echo "FATAL: worktree create failed for plan {plan_number}: $CREATE_JSON" >&2 exit 1 } @@ -304,6 +305,8 @@ fi `--files` carries the plan's declared `files_modified` (the same `PLAN_FILES` the per-plan worktree gate extracts) so this backend routes through the SAME advisory scope-conformance check the Claude worktree path uses at merge (#2596) — one validation, both backends. It is advisory and never blocks; omitting it just skips the check. +`--deletions` carries the plan's declared `files_deleted` (`PLAN_DELETIONS`, extracted alongside `PLAN_FILES`) so this backend also routes through the deletions guard's opt-in (#3003). Unlike `--files` this one is **not** advisory: omitting it leaves the guard blocking on any deletion at all, which would make a plan that declares a removal merge on the harness path and fail here. Every dispatch surface that records a worktree must pass it. + `worktree create` records the entry in `$WAVE_WORKTREE_MANIFEST` itself, so **do not** call `worktree.record-agent` for these plans — that verb is the harness-path counterpart, used because the harness creates the worktree behind GSD's back. Double-recording is deduped by path+branch, but the create verb is the single writer here. Spawn `EXEC_JSON`'s `command` + `args` as a background process with its working directory set to `EXEC_JSON.cwd`. The `cwd` is returned for **every** host, including those whose descriptor has no cwd flag (`cwdFlag: null`) and therefore bind through the process's own working directory — always set it, never assume the flag did the job. Wait for all spawned executors in the wave before merging. diff --git a/gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md b/gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md index 2a999fb66..791af29a8 100644 --- a/gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md +++ b/gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md @@ -10,6 +10,10 @@ Run this for **each plan in the current wave** before its `Agent()` dispatch. Th # plan_json is the JSON object for this plan from PLAN_INDEX.plans[] # files_modified is an array of strings (repo-relative paths or globs) PLAN_FILES=$(jq -r '.files_modified // [] | join(" ")' <<<"$plan_json") +# #3003: files_deleted is the paths the plan declared it will REMOVE. Separate from +# files_modified on purpose — it authorizes the cleanup-wave deletions guard, and a +# deletion authorization must never be inferred from a general scope declaration. +PLAN_DELETIONS=$(jq -r '.files_deleted // [] | join(" ")' <<<"$plan_json") plan_id=$(jq -r '.id' <<<"$plan_json") ``` @@ -18,11 +22,23 @@ Then run the per-plan gate: ```bash USE_WORKTREES_FOR_PLAN="$USE_WORKTREES" +# #3003: this gate asks "does the plan touch a submodule at all", and REMOVING a file +# inside one is as much a touch as modifying it. Both declared channels feed the +# intersection: before files_deleted existed a deleted path had to appear in +# files_modified to be planned at all, so the gate saw it. Now that plan-md.md tells +# authors a deleted path needs no files_modified entry, reading files_modified alone +# would let a deletion-only submodule plan keep worktree isolation on — exactly the +# case #2772 disabled it for. Note this is the OPPOSITE posture from the cleanup-wave +# deletions guard: there the two channels are kept apart because a deletion +# AUTHORIZATION must never be inferred; here they are merged because a safety fallback +# must never MISS a touch. +PLAN_SCOPE_PATHS=$(printf '%s %s' "$PLAN_FILES" "$PLAN_DELETIONS" | tr -s ' ' | sed 's/^ //; s/ $//') + if [ -n "$SUBMODULE_PATHS" ] && [ "$USE_WORKTREES_FOR_PLAN" != "false" ]; then - if [ -z "$PLAN_FILES" ]; then + if [ -z "$PLAN_SCOPE_PATHS" ]; then # Fallback: planned paths are unknown/unparseable — fall back to the safe # behavior (disable worktree isolation for this plan) and log why. - echo "[worktree] Plan ${plan_id}: files_modified missing/unparseable — disabling worktree isolation as a safety fallback (submodule project)" + echo "[worktree] Plan ${plan_id}: files_modified and files_deleted both missing/unparseable — disabling worktree isolation as a safety fallback (submodule project)" USE_WORKTREES_FOR_PLAN=false else # Compute intersection with glob-safe normalization. Both sides are @@ -37,7 +53,7 @@ if [ -n "$SUBMODULE_PATHS" ] && [ "$USE_WORKTREES_FOR_PLAN" != "false" ]; then sm="${sm_raw#./}" sm="${sm%/}" [ -z "$sm" ] && continue - for pf_raw in $PLAN_FILES; do + for pf_raw in $PLAN_SCOPE_PATHS; do # Normalize planned path the same way pf="${pf_raw#./}" pf="${pf%/}" @@ -113,3 +129,5 @@ fi ``` **`PLAN_FILES` is reused after dispatch (#2596):** pass it as `--files "$PLAN_FILES"` on the step-3 `worktree.record-agent` call (and on `worktree.create` in the orchestrator-worktree path) so the post-wave cleanup gauntlet can compare each plan branch's actual committed diff against the scope the plan declared, and report any path outside it. That check is advisory — it warns, it never blocks the merge — and omitting the flag simply skips it for that plan. + +**`PLAN_DELETIONS` is reused the same way (#3003):** pass it as `--deletions "$PLAN_DELETIONS"` on the same `worktree.record-agent` / `worktree.create` calls. Unlike `--files`, this one is **not** advisory — it is what lets the post-wave deletions guard merge a branch whose plan declared a file removal. Matching is exact per path: a declared path merges, anything else still blocks that entry (and only that entry). Omitting the flag keeps the guard's original behavior of blocking on any deletion at all, so a plan that declares nothing loses nothing. diff --git a/src/phase.cts b/src/phase.cts index 080c28265..c92989b34 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -593,6 +593,7 @@ interface RawPlan { autonomous: boolean; objective: string | null; filesModified: string[]; + filesDeleted: string[]; taskCount: number; hasSummary: boolean; /** #2830: true iff this plan's own SUMMARY declares `status: halted` (a designed stop). */ @@ -826,6 +827,7 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { autonomous: planDoc.autonomous, objective: planDoc.objective, filesModified: planDoc.filesModified, + filesDeleted: planDoc.filesDeleted, agentHint: planDoc.agentHint, taskCount: planDoc.taskCount, hasSummary, @@ -929,6 +931,7 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { autonomous: rawPlan.autonomous, objective: rawPlan.objective, files_modified: rawPlan.filesModified, + files_deleted: rawPlan.filesDeleted, agent_hint: rawPlan.agentHint, task_count: rawPlan.taskCount, has_summary: rawPlan.hasSummary, diff --git a/src/plan-document.cts b/src/plan-document.cts index 0f5db9416..9383ff4a9 100644 --- a/src/plan-document.cts +++ b/src/plan-document.cts @@ -79,6 +79,14 @@ interface PlanDocument { agentHint: string | null; /** Frontmatter `files_modified` / `files-modified`, normalised to an array. */ filesModified: string[]; + /** + * #3003: frontmatter `files_deleted` / `files-deleted`, normalised to an array. + * Paths the plan declares it will REMOVE, so `worktree cleanup-wave`'s deletions + * guard can authorize exactly those and keep blocking anything undeclared. + * OPTIONAL — a plan that omits it declares nothing and keeps the guard's original + * unconditional block. + */ + filesDeleted: string[]; tasks: PlanTask[]; /** * Legacy count. Invariant: `taskCount === tasks.length`, always. Exposed as @@ -289,6 +297,13 @@ function parsePlanDocument(content: string, planPath = ''): PlanDocument { filesModified = Array.isArray(fmFiles) ? fmFiles.map(String) : [String(fmFiles)]; } + let filesDeleted: string[] = []; + const fmDeleted = fm['files_deleted'] || fm['files-deleted']; + if (fmDeleted) { + // eslint-disable-next-line @typescript-eslint/no-base-to-string -- FrontmatterValue scalar-to-string + filesDeleted = Array.isArray(fmDeleted) ? fmDeleted.map(String) : [String(fmDeleted)]; + } + let agentHint: string | null = null; const fmAgentHint = fm['agent_hint']; if (fmAgentHint !== undefined) { @@ -304,6 +319,7 @@ function parsePlanDocument(content: string, planPath = ''): PlanDocument { autonomous, agentHint, filesModified, + filesDeleted, tasks, taskCount: tasks.length, }; diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index dc2687bbb..60b75eeb9 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -486,6 +486,18 @@ interface CleanupManifestEntry { allowed_bases?: string[]; /** #2596: the plan's declared `files_modified`, when the recorder supplied it. Absent = unknown, never "declares nothing". */ files_modified?: string[]; + /** + * #3003: paths the PLAN declared it would delete. The deletions guard blocks only + * deletions NOT in this list. Absent = declares nothing, which keeps the guard's + * original unconditional block — never "authorizes everything". + * + * A path LIST, not a boolean, deliberately: a boolean would disarm the guard for the + * whole entry, so an unexpected deletion riding along with a declared one would pass + * unnoticed. Matching is EXACT after `normalizeScopePath`, never a prefix and never a + * glob — a directory prefix authorizes a mass deletion, which is the precise accident + * this guard exists to catch. + */ + declared_deletions?: string[]; } function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | null { @@ -510,6 +522,12 @@ function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | n // would make an unrecorded plan look 100% out of scope. const filesModified = (Array.isArray(e.files_modified) ? e.files_modified : []) .filter((f): f is string => typeof f === 'string' && f.trim().length > 0); + // #3003: same liberal-in-what-we-accept rule as `files_modified` above — non-array, + // or non-string / empty elements, are dropped rather than coerced, and an EMPTY + // result omits the field entirely so "declares nothing" stays indistinguishable + // from "not recorded" (the guard's own absence-check already treats both as unknown). + const declaredDeletions = (Array.isArray(e.declared_deletions) ? e.declared_deletions : []) + .filter((f): f is string => typeof f === 'string' && f.trim().length > 0); const normalized: CleanupManifestEntry = { agent_id: typeof e.agent_id === 'string' ? e.agent_id : null, worktree_path: worktreePath, @@ -518,6 +536,7 @@ function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | n allowed_bases: allowedBases, }; if (filesModified.length > 0) normalized.files_modified = filesModified; + if (declaredDeletions.length > 0) normalized.declared_deletions = declaredDeletions; return normalized; } @@ -634,16 +653,78 @@ function repoRootStillMidMerge(execGit: ExecGitFn, repoRoot: string): boolean { const SUMMARY_ARTIFACT_DIR = '.planning'; const SUMMARY_ARTIFACT_SUFFIX = 'SUMMARY.md'; +/** + * Decode a git-quoted path. With `core.quotepath` left at its default (`true`) + * git wraps any path containing non-ASCII or special bytes in double quotes + * and C-escapes it — e.g. `tests/é.ts` is emitted as the literal + * `"tests/\303\251.ts"`. Both sides of the deletion/scope comparison + * (a declared path and a git-reported path) must agree on the same decoded + * shape, and decoding it here — rather than passing `-c core.quotepath=false` + * on the `execGit` call — keeps the git argv, and therefore every existing + * test fixture that asserts on exact argv, unchanged. It also works + * regardless of the user's own `core.quotepath` config. + * + * A value that is not wrapped in a leading and trailing `"` is returned + * completely untouched — this is the overwhelmingly common (plain ASCII) + * case and must not be altered in any way. + * + * Escapes decode to BYTES, collected into a Buffer and decoded as UTF-8 only + * at the end: `\303\251` is two bytes that together form one character (é), + * so decoding them one at a time would produce mojibake. Malformed input + * (a trailing lone backslash, or an octal escape with fewer than three + * digits) never throws — it degrades to treating the character literally, so + * a single bad path can never take down the whole cleanup wave. + */ +function decodeGitQuotedPath(raw: string): string { + if (raw.length < 2 || !raw.startsWith('"') || !raw.endsWith('"')) return raw; + const body = raw.slice(1, -1); + const bytes: number[] = []; + for (let i = 0; i < body.length; i++) { + const ch = body[i]; + if (ch !== '\\' || i === body.length - 1) { + // UTF-8 bytes, not the code unit: a DECLARED path may be quoted while + // still holding a literal `é`, and pushing 0xE9 alone is invalid UTF-8. + // codePointAt keeps a surrogate pair together. + const codePoint = body.codePointAt(i); + const char = codePoint === undefined ? ch : String.fromCodePoint(codePoint); + bytes.push(...Buffer.from(char, 'utf8')); + i += char.length - 1; + continue; + } + const rest = body.slice(i + 1); + const octalMatch = /^([0-3][0-7][0-7])/.exec(rest); + if (octalMatch) { + bytes.push(parseInt(octalMatch[1], 8)); + i += 3; + continue; + } + const next = body[i + 1]; + const CONTROL_ESCAPES: Record = { + a: 0x07, b: 0x08, f: 0x0c, n: 0x0a, r: 0x0d, t: 0x09, v: 0x0b, + '\\': 0x5c, '"': 0x22, + }; + if (Object.prototype.hasOwnProperty.call(CONTROL_ESCAPES, next)) { + bytes.push(CONTROL_ESCAPES[next]); + } else { + bytes.push(next.charCodeAt(0)); + } + i += 1; + } + return Buffer.from(bytes).toString('utf8'); +} + /** * Normalize one path for scope comparison. Applied to BOTH sides so a declared - * path and a git-reported path meet in the same shape: backslashes become - * slashes unconditionally (a backslash path is not a Windows-only input), - * a leading `./` and any trailing `/` are stripped. This is the single - * normalizer shared by the SUMMARY-artifact predicate and the scope advisory, - * so the two can never disagree about what `./a\b/` means. + * path and a git-reported path meet in the same shape: any git C-quoting is + * decoded first (order matters — the backslash-to-slash conversion below + * would destroy the escape sequences if it ran first), then backslashes + * become slashes unconditionally (a backslash path is not a Windows-only + * input), and a leading `./` and any trailing `/` are stripped. This is the + * single normalizer shared by the SUMMARY-artifact predicate and the scope + * advisory, so the two can never disagree about what `./a\b/` means. */ function normalizeScopePath(raw: string): string { - return String(raw || '') + return decodeGitQuotedPath(String(raw || '').trim()) .replace(/\\/g, '/') .trim() .replace(/^\.\//, '') @@ -869,6 +950,42 @@ function planWaveScopeConformance( return warnings; } +/** + * #3003: split git's reported deletions into declared and undeclared. + * + * EXACT match after `normalizeScopePath` — the same normalizer both other path + * comparisons in this file use, so a declared path and a git-reported path meet in the + * same shape (`a\b` and `./a/b/` both become `a/b`). + * + * Deliberately NOT `declaredScopePrefix`. That helper returns `null` for a glob-leading + * pattern meaning "matches everything", which is right for the ADVISORY it serves — a + * false alarm there costs more than a miss — and exactly wrong for a GATE, where it would + * let `["*.ts"]` disarm the guard. A glob here is simply a literal path that matches + * nothing. Prefix matching is likewise refused: `["tests"]` must not authorize deleting + * everything under tests/. + * + * Pure — no git, no IO. Returns the undeclared residue in the order git reported it. + */ +function partitionDeclaredDeletions(deletedPaths: unknown, declared: unknown): string[] { + const allowed = new Set( + (Array.isArray(declared) ? declared : []) + .filter((p): p is string => typeof p === 'string') + .map((p) => normalizeScopePath(p)) + .filter((p) => p.length > 0), + ); + const undeclared: string[] = []; + const seen = new Set(); + for (const raw of Array.isArray(deletedPaths) ? deletedPaths : []) { + if (typeof raw !== 'string') continue; + const normalized = normalizeScopePath(raw); + if (!normalized || seen.has(normalized)) continue; + seen.add(normalized); + if (allowed.has(normalized)) continue; + undeclared.push(normalized); + } + return undeclared; +} + interface WaveCleanupEntryResult extends CleanupManifestEntry { status: string; reason: string | null; @@ -950,13 +1067,20 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work continue; // #2852: isolate } if (deletions.stdout) { - // Unconditional: any deletion in this entry's branch blocks THIS entry. Whether - // that guard should have an opt-in for intentional deletions is a deferred - // product decision (issue #2852's own triage scoped it out — tracked in #3003); - // this fix only isolates the block to this one entry (#2852) instead of aborting - // the rest of the wave, same as every other block reason below. - blockEntry(result, 'branch_contains_deletions', deletions.stdout); - continue; // #2852: isolate + // #3003: a deletion the PLAN declared is authorized; anything else still blocks. + // Only a SUCCESSFUL check reaches here — a failed one blocked above on its own + // reason, so a broken check can never be filtered into a pass. + // + // The block detail carries ONLY the undeclared residue. Listing declared paths + // there would misdirect the operator toward paths that were fine. + const undeclaredDeletions = partitionDeclaredDeletions( + deletions.stdout.split('\n'), + entry.declared_deletions, + ); + if (undeclaredDeletions.length > 0) { + blockEntry(result, 'branch_contains_deletions', undeclaredDeletions.join('\n')); + continue; // #2852: isolate + } } // #2596: advisory scope conformance — does the branch's ACTUAL committed @@ -968,13 +1092,34 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work // ADVISORY ONLY. Unlike the deletions check above, a finding here does NOT // call blockEntry and does NOT touch `ok` — the merge proceeds. Promotion // to a hard gate is a separate, disclosed change. - if (Array.isArray(entry.files_modified) && entry.files_modified.length > 0) { + // #3003: a declared deletion is in scope by construction — `git diff --name-only` + // includes deleted paths, so without accounting for the declaration, authorizing a + // deletion would immediately warn that the same path is out of declared scope. + // + // It is SUBTRACTED from the findings rather than UNIONED into the declared scope, + // and the difference is not cosmetic. `planWaveScopeConformance` reads its scope + // list with prefix-and-glob semantics, so unioning would silently hand + // `declared_deletions` a second, WIDER matching rule than the gate gives it: + // `["*.md"]` (inert at the gate) would yield a null prefix meaning "matches + // everything" and mute the advisory entirely, and `["src"]` would mute all of + // `src/`. One field with two matching rules is a trap. Subtracting keeps the + // field exact-match-only on every surface it touches. + // + // Gating stays on `files_modified` alone for the same reason: a plan that declares + // only deletions has still declared no modification scope, so the advisory stays + // exactly as silent as it was before this change instead of warning on every + // modified path. + const declaredFiles = Array.isArray(entry.files_modified) ? entry.files_modified : []; + if (declaredFiles.length > 0) { const scopeDiff = execGit(['diff', '--name-only', `HEAD...${entry.branch}`], { cwd: plan.repoRoot }); const scopeWarnings: WaveCleanupWarning[] = !gitResultOk(scopeDiff) // A broken advisory must never become a gate: record that conformance // is unknown rather than blocking (or, worse, silently passing). ? [{ code: WAVE_CLEANUP_WARNING.SCOPE_CHECK_UNAVAILABLE, branch: entry.branch, path: null }] - : planWaveScopeConformance((scopeDiff.stdout || '').split('\n'), entry.files_modified, entry.branch); + : planWaveScopeConformance((scopeDiff.stdout || '').split('\n'), declaredFiles, entry.branch) + // A path-less warning (SCOPE_CHECK_UNAVAILABLE) is never subtracted. + .filter((w) => w.path === null + || partitionDeclaredDeletions([w.path], entry.declared_deletions).length > 0); result.warnings.push(...scopeWarnings); allWarnings.push(...scopeWarnings); } @@ -1117,6 +1262,7 @@ interface RecordAgentFields { branch: string; base: string; files?: string; + deletions?: string; } /** @@ -1184,12 +1330,16 @@ function planWorktreeRecordAgent(manifestRaw: string, fields: RecordAgentFields) // 2. Shared validation: run the candidate through the reader's normalizer. // If it returns null the reader would drop this entry on read — reject now. const declaredScope = parseDeclaredScopeFlag(fields.files); + // #3003: reuse parseDeclaredScopeFlag — a blank/absent --deletions leaves the + // entry shape untouched, mirroring the --files rule directly above. + const declaredDeletions = parseDeclaredScopeFlag(fields.deletions); const candidate = { agent_id: agentId, worktree_path: worktreePath, branch, expected_base: base, ...(declaredScope.length > 0 ? { files_modified: declaredScope } : {}), + ...(declaredDeletions.length > 0 ? { declared_deletions: declaredDeletions } : {}), }; const entry = normalizeCleanupManifestEntry(candidate); if (!entry) { @@ -1283,6 +1433,11 @@ function planWorktreeRecordAgent(manifestRaw: string, fields: RecordAgentFields) if (entry.files_modified && entry.files_modified.length > 0) { recorded.files_modified = entry.files_modified; } + // #3003: same conservative rule as --files above — a blank --deletions + // leaves the entry shape untouched. + if (entry.declared_deletions && entry.declared_deletions.length > 0) { + recorded.declared_deletions = entry.declared_deletions; + } worktrees.push(recorded); return { @@ -1311,7 +1466,7 @@ interface RecordAgentCmdResult { /** * CLI command: append a validated per-agent entry to a wave cleanup manifest. * - * Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""] + * Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""] [--deletions ""] * * Fails loudly (non-zero exit + recovery hint on stderr) when a field is * missing/garbled or the manifest is absent/malformed, rather than appending an @@ -1320,14 +1475,15 @@ interface RecordAgentCmdResult { function cmdWorktreeRecordAgent(cwd: string, args: string[] = [], deps: RecordAgentCmdDeps = {}): RecordAgentCmdResult { const flag = (name: string): string => { const i = args.indexOf(name); - return i >= 0 && i + 1 < args.length ? args[i + 1] : ''; + if (i < 0 || i + 1 >= args.length) return ''; + return args[i + 1]; }; const write = deps.write || ((s: string) => process.stdout.write(s)); const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); const manifestPath = flag('--manifest'); if (!manifestPath) { - writeErr('Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""]\n'); + writeErr('Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""] [--deletions ""]\n'); process.exitCode = 2; return { ok: false, reason: 'usage', entry: null }; } @@ -1351,6 +1507,7 @@ function cmdWorktreeRecordAgent(cwd: string, args: string[] = [], deps: RecordAg branch: flag('--branch'), base: flag('--base'), files: flag('--files'), + deletions: flag('--deletions'), }); if (!plan.ok || plan.manifest === null) { @@ -1381,6 +1538,7 @@ interface WorktreeCreateFields { branch: string; base: string; files?: string; + deletions?: string; } interface WorktreeCreatePlan { @@ -1419,12 +1577,16 @@ function planWorktreeCreate(fields: WorktreeCreateFields): WorktreeCreatePlan { } const declaredScope = parseDeclaredScopeFlag(fields.files); + // #3003: reuse parseDeclaredScopeFlag — a blank/absent --deletions leaves the + // entry shape untouched, mirroring the --files rule directly above. + const declaredDeletions = parseDeclaredScopeFlag(fields.deletions); const candidate = { agent_id: agentId, worktree_path: worktreePath, branch, expected_base: base, ...(declaredScope.length > 0 ? { files_modified: declaredScope } : {}), + ...(declaredDeletions.length > 0 ? { declared_deletions: declaredDeletions } : {}), }; const entry = normalizeCleanupManifestEntry(candidate); if (!entry) { @@ -1581,7 +1743,7 @@ interface WorktreeCreateCmdResult { * validated manifest entry so the worktree is immediately manageable by * `worktree cleanup-wave` / `worktree reap-orphans`. * - * Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""] + * Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""] [--deletions ""] * * #2584 FIX 1 — ORDERING CONTRACT: every manifest read/parse/shape-validate/ * plan step runs BEFORE the git side effect (step 5). The ONLY manifest @@ -1595,14 +1757,15 @@ interface WorktreeCreateCmdResult { function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCmdDeps & WorktreeDeps = {}): WorktreeCreateCmdResult { const flag = (name: string): string => { const i = args.indexOf(name); - return i >= 0 && i + 1 < args.length ? args[i + 1] : ''; + if (i < 0 || i + 1 >= args.length) return ''; + return args[i + 1]; }; const write = deps.write || ((s: string) => process.stdout.write(s)); const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); const manifestPath = flag('--manifest'); if (!manifestPath) { - writeErr('Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""]\n'); + writeErr('Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""] [--deletions ""]\n'); process.exitCode = 2; return { ok: false, reason: 'usage' }; } @@ -1667,6 +1830,7 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm branch: flag('--branch'), base: flag('--base'), files: flag('--files'), + deletions: flag('--deletions'), }); if (!plan.ok || !plan.entry) { @@ -1741,6 +1905,11 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm if (Array.isArray(plan.entry.files_modified) && plan.entry.files_modified.length > 0) { recorded.files_modified = plan.entry.files_modified; } + // #3003: same conservative rule as --files above — a blank --deletions + // leaves the entry shape untouched. + if (Array.isArray(plan.entry.declared_deletions) && plan.entry.declared_deletions.length > 0) { + recorded.declared_deletions = plan.entry.declared_deletions; + } const dedupeKey = `${recorded.worktree_path}\0${recorded.branch}`; const alreadyPresent = worktrees.some((existing) => { const normalized = normalizeCleanupManifestEntry(existing); diff --git a/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json b/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json index 4f46a6a83..71bfa2dc1 100644 --- a/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json +++ b/tests/emitted-drift-acks/1954-plan-checker-undeclared-coupling.json @@ -1,6 +1,6 @@ { "version": 1, "paths": { - "gsd-plan-checker.md": "#1954: Dimension 3 gains sub-dimension 3b (undeclared / temporal coupling), plus one success-criteria line. Growth is the new sub-dimension only — no existing text was rewritten. The addition is deliberately dense rather than extracted: ADR-1610 Decision 4 names eager `@`-import relocation as proxy-gaming (it shrinks the measured file while leaving loaded context unchanged or larger), legitimate extraction is Read-at-step lazy, and this agent has no lazy-read seam; issue #1954's approved scope is also explicitly 'No new files'. After the change the file sits at 48810 bytes against the LARGE tier hard cap of 49152 (tests/agent-size-budget.test.cjs), i.e. 342 bytes of headroom. That is deliberate and disclosed: the cap is not crossed and is not raised, but the next contributor who needs room in this agent must do a lazy extraction rather than add prose. Content justification: Dimension 3 proves declared dependency edges resolve and are acyclic, and execute-phase's intra-wave guard proves same-wave plans do not overlap in files_modified — neither axis sees an undeclared edge, so two same-wave plans coupled through a shared config key, table, migration, env var, singleton or cache (or through one plan's produced state) pass plan-check and fail intermittently under parallel execution. 3b is advisory only (WARNING, never blocker, per the issue's rejected-alternatives list) and reuses the existing dependency_correctness finding key so no consumer sees a new dimension. — #2401 append (merged into this fragment because two ack sources may never name the same path): the Verify Command Path Resolvability dimension is now a short stub pointing at the extracted gsd-core/references/verify-command-path-resolvability.md (the full dimension body — including the {VERIFY_PATHS} probe-consumption rules and severity/reason table — moved out to stay under the LARGE cap). The file is now 49064 bytes, 88 bytes of headroom under the same 49152 LARGE cap." + "gsd-plan-checker.md": "#1954: Dimension 3 gains sub-dimension 3b (undeclared / temporal coupling), plus one success-criteria line. Growth is the new sub-dimension only — no existing text was rewritten. The addition is deliberately dense rather than extracted: ADR-1610 Decision 4 names eager `@`-import relocation as proxy-gaming (it shrinks the measured file while leaving loaded context unchanged or larger), legitimate extraction is Read-at-step lazy, and this agent has no lazy-read seam; issue #1954's approved scope is also explicitly 'No new files'. After the change the file sits at 48810 bytes against the LARGE tier hard cap of 49152 (tests/agent-size-budget.test.cjs), i.e. 342 bytes of headroom. That is deliberate and disclosed: the cap is not crossed and is not raised, but the next contributor who needs room in this agent must do a lazy extraction rather than add prose. Content justification: Dimension 3 proves declared dependency edges resolve and are acyclic, and execute-phase's intra-wave guard proves same-wave plans do not overlap in files_modified — neither axis sees an undeclared edge, so two same-wave plans coupled through a shared config key, table, migration, env var, singleton or cache (or through one plan's produced state) pass plan-check and fail intermittently under parallel execution. 3b is advisory only (WARNING, never blocker, per the issue's rejected-alternatives list) and reuses the existing dependency_correctness finding key so no consumer sees a new dimension. — #2401 append (merged into this fragment because two ack sources may never name the same path): the Verify Command Path Resolvability dimension is now a short stub pointing at the extracted gsd-core/references/verify-command-path-resolvability.md (the full dimension body — including the {VERIFY_PATHS} probe-consumption rules and severity/reason table — moved out to stay under the LARGE cap). The file is now 49064 bytes, 88 bytes of headroom under the same 49152 LARGE cap. — #3003 append (merged into this fragment because two ack sources may never name the same path): Dimension 3b's description of what the wave guard already covers now names both declared-scope channels (`files_modified`/`files_deleted`), so a delete-vs-modify pair between same-wave plans is attributed once on the file axis rather than re-reported as undeclared coupling. +43 bytes, landing at 49107 with 45 bytes of headroom under the same cap — the 88-byte warning above still stands and is now tighter." } } diff --git a/tests/emitted-drift-acks/2775-planner-package-legitimacy-gate.json b/tests/emitted-drift-acks/2775-planner-package-legitimacy-gate.json index cd1f8f816..cfdd7d7a7 100644 --- a/tests/emitted-drift-acks/2775-planner-package-legitimacy-gate.json +++ b/tests/emitted-drift-acks/2775-planner-package-legitimacy-gate.json @@ -2,7 +2,7 @@ "version": 1, "paths": { "gsd-planner.md": { - "reason": "#2775: the STRIDE supply-chain row for npm/pip/cargo installs was rewritten from 'slopcheck + blocking human checkpoint for [ASSUMED]/[SUS]' to 'package-legitimacy gate + blocking human checkpoint for [ASSUMED]/[SUS]', matching ADR-0656 (registry-API verdicts are the gate; slopcheck is an optional escalate-only adapter no shipped configuration wires). The +14 bytes is the corrected mitigation description agents read at plan time, not incidental prose growth. #3565 appends the fenced `## Return Markers` section (~1.2 KB) enumerating the six exact stall-watch dispatch markers plan-phase.md matches on — the new registry lint requires every declared marker to be emitted in-fence by its producer, and the planner previously documented none of them anywhere. #2401 append (merged into this fragment because two ack sources may never name the same path): the 'Inherit the command that already worked' paragraph is now a short pointer to the extracted gsd-core/references/planner-verify-command-grounding.md (prior_verify_commands inheritance, npm --prefix grounding rules, and the guessing prohibition all moved to the reference). The file is now 49274 bytes, well under the XL tier's 57344-byte cap, and 49077 chars (LF-normalized) under the separate 49152-char cap asserted by tests/planner-decomposition.test.cjs, tests/precondition-element.test.cjs, tests/reversibility-tagging.test.cjs, and tests/security.test.cjs." + "reason": "#2775: the STRIDE supply-chain row for npm/pip/cargo installs was rewritten from 'slopcheck + blocking human checkpoint for [ASSUMED]/[SUS]' to 'package-legitimacy gate + blocking human checkpoint for [ASSUMED]/[SUS]', matching ADR-0656 (registry-API verdicts are the gate; slopcheck is an optional escalate-only adapter no shipped configuration wires). The +14 bytes is the corrected mitigation description agents read at plan time, not incidental prose growth. #3565 appends the fenced `## Return Markers` section (~1.2 KB) enumerating the six exact stall-watch dispatch markers plan-phase.md matches on — the new registry lint requires every declared marker to be emitted in-fence by its producer, and the planner previously documented none of them anywhere. #2401 append (merged into this fragment because two ack sources may never name the same path): the 'Inherit the command that already worked' paragraph is now a short pointer to the extracted gsd-core/references/planner-verify-command-grounding.md (prior_verify_commands inheritance, npm --prefix grounding rules, and the guessing prohibition all moved to the reference). The file is now 49274 bytes, well under the XL tier's 57344-byte cap, and 49077 chars (LF-normalized) under the separate 49152-char cap asserted by tests/planner-decomposition.test.cjs, tests/precondition-element.test.cjs, tests/reversibility-tagging.test.cjs, and tests/security.test.cjs. #3003 append (merged into this fragment because two ack sources may never name the same path): the implicit-dependency wave rule now computes same-wave overlap over files_modified PLUS files_deleted, so a plan deleting a file another same-wave plan edits is pushed to a later wave instead of racing it -- one branch removing what the other is writing is the sharpest conflict there is, and reading files_modified alone scored that pair conflict-free. Deliberately terse (+56 LF chars, landing at 49133 against the 49152-char cap named above, 19 chars of headroom). The `# Implicit dependency: files_modified overlap forces a later wave.` comment is left VERBATIM: tests/parallel-dependent-plans.test.cjs matches that exact unbackticked substring, so rewording it to name the new channel reds the suite -- the files_deleted addition rides in the pseudocode and the Rule sentence instead: the rationale is carried in docs/reference/plan-md.md, which has no cap. The next contributor who needs room in this agent must extract, not add prose." } } } diff --git a/tests/emitted-drift-acks/2856-live-dom-uat.json b/tests/emitted-drift-acks/2856-live-dom-uat.json deleted file mode 100644 index 42bac3c05..000000000 --- a/tests/emitted-drift-acks/2856-live-dom-uat.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "version": 1, - "paths": { - "execute-phase.md": { - "reason": "#2856 \u2014 step 5.75 now dispatches every `kind == \"step\"` at execute:wave:post. That point previously dispatched only contribution and gate, so any registered step was declared and silently never run; the capability validator refuses to generate a registry containing such a step. Also adds the validate-ref.command-before-shell warning matching the sibling gate-dispatch line. Supersedes the spent #3370 fragment entry (merged into next, so its ripple is already absorbed at the base and it can no longer clear anything), which also named execute-phase.md and would otherwise double-ack the same path \u2014 the same supersede that entry itself performed on the spent #3324 fragment. #3659 corrects the auto-degrade paragraph's restore-advice to the orchestrator/harness split (+~300B): worktree.baseRef:\"head\" restores parallel execution only where GSD itself creates worktrees; harness-isolated runtimes do not read the setting (#48, upstream claude-code#44965). Deliberate growth." - } - } -} diff --git a/tests/emitted-drift-acks/3003-declared-deletions.json b/tests/emitted-drift-acks/3003-declared-deletions.json new file mode 100644 index 000000000..23b0e66e6 --- /dev/null +++ b/tests/emitted-drift-acks/3003-declared-deletions.json @@ -0,0 +1,7 @@ +{ + "$comment": "Growth ack (#2914 fragment). Reason: #3003 threads a plan-declared deletion list from plan frontmatter to the cleanup-wave deletions guard. execute-phase.md gains --deletions \"$PLAN_DELETIONS\" on the record-agent call; 92326 -> 92356 LF bytes (+30). Supersedes the spent #2856 fragment entry (tests/emitted-drift-acks/2856-live-dom-uat.json, merged into next so its ripple is already absorbed at the base and it can no longer clear anything), which also named execute-phase.md and would otherwise double-ack the same path — the same supersede that entry itself performed on the spent #3370 fragment, which had performed it on #3324. Only this path needs an entry: currentSizes() (tests/helpers/emitted-runtime.cjs:916-929) reads gsd-core/workflows/ and agents/ with a NON-recursive readdirSync that skips directories, so the two other grown shipped files are outside the growth ratchet — gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md (+885) sits in a subdirectory and gsd-core/templates/phase-prompt.md (+285) is under templates/. Their emitted-hash ripples are attributable to this diff and need no acknowledgment.", + "version": 1, + "paths": { + "execute-phase.md": "gsd-core/workflows/execute-phase.md +30B: pass --deletions \"$PLAN_DELETIONS\" to worktree.record-agent so a plan-declared deletion reaches the cleanup-wave guard (#3003)" + } +} diff --git a/tests/planning-inspect.unit.test.cjs b/tests/planning-inspect.unit.test.cjs index 817008201..5c47f5d85 100644 --- a/tests/planning-inspect.unit.test.cjs +++ b/tests/planning-inspect.unit.test.cjs @@ -273,6 +273,28 @@ describe('plan-document — frontmatter scheduling metadata', () => { }); }); +describe('plan-document — frontmatter filesDeleted', () => { + test('no frontmatter at all defaults filesDeleted to []', () => { + const parsed = parsePlanDocument('plain body, no frontmatter'); + assert.deepStrictEqual(parsed.filesDeleted, []); + }); + + test('scalar files_deleted (underscore key) is wrapped into a one-element array', () => { + const parsed = parsePlanDocument(frontmatterDoc(['files_deleted: src/gone.ts'], ['body'])); + assert.deepStrictEqual(parsed.filesDeleted, ['src/gone.ts']); + }); + + test('array files-deleted (hyphen key) is mapped element-wise', () => { + const parsed = parsePlanDocument(frontmatterDoc(['files-deleted: [a.ts, b.ts]'], ['body'])); + assert.deepStrictEqual(parsed.filesDeleted, ['a.ts', 'b.ts']); + }); + + test('empty files_deleted list yields []', () => { + const parsed = parsePlanDocument(frontmatterDoc(['files_deleted: []'], ['body'])); + assert.deepStrictEqual(parsed.filesDeleted, []); + }); +}); + describe('plan-document — planIdFromFile / TASK_KIND', () => { test('strips the -PLAN.md suffix from a root-form plan file', () => { assert.strictEqual(planIdFromFile('1-01-PLAN.md'), '1-01'); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index b07886eba..49ecff62e 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -7131,3 +7131,351 @@ describe('#2596 --files on the record-agent and create verbs', () => { } }); }); + + +// ══ #3003 — declared deletions for the cleanup-wave guard ═══════════════════════════════ +// +// The deletions guard blocked ANY deletion in an executor branch, unconditionally. A plan +// whose stated scope includes removing a file (folding a test into a sibling suite) could +// not be merged by the tool meant to merge it, forcing a manual --no-ff outside the tool — +// strictly less safe than what the guard protects against. +// +// #3003's pinned decision: an optional `declared_deletions` PATH LIST on the manifest entry. +// A path list, not a boolean, precisely so an unexpected deletion riding along with a +// declared one still blocks. These tests exist mostly to hold that line — the rows that +// matter are the OVER-AUTHORIZATION set, because every way of loosening the matcher +// (prefix, glob, startsWith) silently rebuilds the boolean opt-in that was rejected. +// +// Matching is EXACT after normalization. No globs, no prefixes. That is deliberate +// Greenspun-avoidance: `declaredScopePrefix` already exists for the ADVISORY and returns +// null ("matches everything") for a glob-leading pattern — correct there, because a false +// alarm costs more than a miss for an advisory. For a GATE that same rule would let +// `["*.ts"]` disarm the guard completely. +// +// See https://github.com/open-gsd/gsd-core/issues/3003 + +describe('#3003 — declared deletions: authorization is exact set membership', () => { + const REPO = '/repo/main'; + const WT = '/repo/.claude/worktrees/agent-a1'; + const BR = 'worktree-agent-a1'; + + /** A wave-cleanup git double whose deletion list and per-key overrides are injectable. */ + function makeDeletionGit({ deletions = '', deletionExit = 0, changed = null } = {}) { + return (args) => { + const key = args.join(' '); + const ok = (stdout = '') => ({ exitCode: 0, stdout, stderr: '', signal: null, error: null, timedOut: false }); + if (key === `-C ${WT} rev-parse --abbrev-ref HEAD`) return ok(BR); + if (key === `merge-base HEAD ${BR}`) return ok('abc123'); + if (key === `diff --diff-filter=D --name-only HEAD...${BR}`) { + return deletionExit === 0 + ? ok(deletions) + : { exitCode: deletionExit, stdout: '', stderr: 'fatal: bad revision', signal: null, error: null, timedOut: false }; + } + if (key === `diff --name-only HEAD...${BR}`) return ok(changed === null ? deletions : changed); + if (key === `-C ${WT} status --porcelain --untracked-files=all`) return ok(''); + return ok(); + }; + } + + function runWave(entry, gitOpts) { + return executeWorktreeWaveCleanupPlan( + { + ok: true, + repoRoot: REPO, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ agent_id: 'a1', worktree_path: WT, branch: BR, expected_base: 'abc123', ...entry }], + }, + { execGit: makeDeletionGit(gitOpts) }, + ); + } + + const firstEntry = (result) => result.entries[0]; + const blockedOnDeletions = (result) => firstEntry(result).reason === 'branch_contains_deletions'; + + // ── backward compatibility: these must pass BEFORE the change too ────────────────────── + + test('no deletions proceeds', () => { + const result = runWave({}, { deletions: '' }); + assert.notEqual(firstEntry(result).reason, 'branch_contains_deletions'); + }); + + test('absent declaration keeps the unconditional block', () => { + const result = runWave({}, { deletions: 'tests/a.test.ts\n' }); + assert.ok(blockedOnDeletions(result), 'an entry with no declaration must block exactly as before'); + }); + + // ── the feature ──────────────────────────────────────────────────────────────────────── + + test('a fully declared deletion merges', () => { + const result = runWave( + { declared_deletions: ['tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + }); + + test('an undeclared deletion still blocks, and names only the residue', () => { + const result = runWave( + { declared_deletions: ['tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\nsrc/billing.ts\n' }, + ); + assert.ok(blockedOnDeletions(result)); + const detail = firstEntry(result).stderr; + assert.match(detail, /src\/billing\.ts/, 'the undeclared path must be named'); + assert.doesNotMatch(detail, /tests\/a\.test\.ts/, + 'a declared path must NOT appear in the block detail — it would misdirect the operator'); + }); + + test('an over-declaration is inert', () => { + const result = runWave( + { declared_deletions: ['tests/a.test.ts', 'never/deleted.ts'] }, + { deletions: 'tests/a.test.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result)); + }); + + test('an empty declaration is not an authorization', () => { + const result = runWave({ declared_deletions: [] }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(blockedOnDeletions(result)); + }); + + test('a broken deletion check is never an authorization', () => { + const result = runWave( + { declared_deletions: ['tests/a.test.ts'] }, + { deletions: '', deletionExit: 128 }, + ); + assert.equal(firstEntry(result).reason, 'deletion_check_failed', + 'a failed check must block on its own reason, never be filtered into a pass'); + }); + + // ── the over-authorization set: each of these BLOCKS, and each would PASS under a + // prefix / glob / startsWith matcher. This is the line the design exists to hold. ──── + + test('a directory declaration does not authorize its children', () => { + const result = runWave({ declared_deletions: ['tests'] }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(blockedOnDeletions(result), + 'prefix matching would authorize a mass deletion — the exact accident the guard catches'); + }); + + test('a glob declaration authorizes nothing', () => { + const result = runWave({ declared_deletions: ['*.ts'] }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(blockedOnDeletions(result), + 'a glob-leading declaration must not disarm the guard (declaredScopePrefix returns null here)'); + }); + + test('a declaration is not a string prefix of another path', () => { + const result = runWave({ declared_deletions: ['tests/a.ts'] }, { deletions: 'tests/ab.ts\n' }); + assert.ok(blockedOnDeletions(result), 'startsWith would leak tests/a.ts -> tests/ab.ts'); + }); + + // ── normalization: both sides meet in the same shape ─────────────────────────────────── + + test('a backslash declaration normalizes on any OS', () => { + const result = runWave({ declared_deletions: ['tests\\a.test.ts'] }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(!blockedOnDeletions(result)); + }); + + test('leading ./ and trailing slash normalize', () => { + const result = runWave({ declared_deletions: ['./tests/a.test.ts'] }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(!blockedOnDeletions(result)); + }); + + test('a duplicated declaration is inert', () => { + const result = runWave( + { declared_deletions: ['tests/a.test.ts', 'tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result)); + }); + + test('non-string and blank declarations are dropped', () => { + const result = runWave( + { declared_deletions: [null, 0, '', ' ', [], 'tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), 'junk elements drop; the one real path still authorizes'); + }); + + test('a non-array declaration is treated as absent', () => { + for (const bogus of ['tests/a.test.ts', {}, 0, true]) { + const result = runWave({ declared_deletions: bogus }, { deletions: 'tests/a.test.ts\n' }); + assert.ok(blockedOnDeletions(result), `non-array ${JSON.stringify(bogus)} must not authorize`); + } + }); + + // ── the advisory interaction the design nearly missed ────────────────────────────────── + + test('a declared deletion is in scope for the advisory', () => { + // `git diff --name-only` includes deleted paths, and the #2596 advisory compares that + // against files_modified ALONE. Without subtracting the declaration out, authorizing a + // deletion produces a SCOPE_OUT_OF_DECLARED warning for the very path just authorized. + const result = runWave( + { files_modified: ['src/keep.ts'], declared_deletions: ['tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\n', changed: 'src/keep.ts\ntests/a.test.ts\n' }, + ); + // The merge assertion is load-bearing: without it a revert blocks the entry, warnings + // come back empty, and the path-absence assertion below passes for the wrong reason. + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + const paths = firstEntry(result).warnings.map((w) => w.path); + assert.ok(!paths.includes('tests/a.test.ts'), + 'a declared deletion must not be reported out-of-scope'); + }); + + test('the advisory does not activate on declarations alone', () => { + // Before this fix, gating unioned files_modified + declared_deletions into the scope + // list, so a plan that declared ONLY a deletion (no files_modified) still produced a + // non-empty scope list, and every modified path warned as out-of-declared-scope on a + // plan that had declared no modification scope at all. Gating on files_modified alone + // keeps the advisory as silent as it was pre-#2596 when nothing was declared modified. + const result = runWave( + { declared_deletions: ['src/gone.ts'] }, + { deletions: 'src/gone.ts\n', changed: 'src/gone.ts\nsrc/other.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + assert.deepEqual(firstEntry(result).warnings, [], 'no files_modified means no advisory scope at all'); + }); + + test('a glob in declared_deletions does not mute the advisory', () => { + // `declaredScopePrefix` returns null for a glob-leading pattern, meaning "matches + // everything" — correct for the advisory's OWN matcher, but under the old UNION this + // silenced the advisory entirely for a modified path that has nothing to do with the + // glob. Exact-match subtraction gives declared_deletions one rule on every surface. + const result = runWave( + { files_modified: ['src/kept.ts'], declared_deletions: ['*.md'] }, + { deletions: '', changed: 'src/kept.ts\nsrc/stray.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + const paths = firstEntry(result).warnings.map((w) => w.path); + assert.ok(paths.includes('src/stray.ts'), 'a glob declaration must not disarm the advisory'); + }); + + test('a bare directory in declared_deletions does not mute the advisory for its children', () => { + // Same trap as the glob case: a directory-shaped declared_deletions entry authorizes + // nothing at the gate (exact match only), but under the old UNION it would have widened + // the advisory's own prefix matching to cover everything under that directory. + const result = runWave( + { files_modified: ['src/kept.ts'], declared_deletions: ['src'] }, + { deletions: '', changed: 'src/kept.ts\nsrc/stray.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + const paths = firstEntry(result).warnings.map((w) => w.path); + assert.ok(paths.includes('src/stray.ts'), 'a bare directory declaration must not mute the advisory for its children'); + }); + + test('the advisory still fires for a genuinely out-of-scope path', () => { + const result = runWave( + { files_modified: ['src/keep.ts'], declared_deletions: ['tests/a.test.ts'] }, + { deletions: 'tests/a.test.ts\n', changed: 'src/keep.ts\ntests/a.test.ts\nsrc/rogue.ts\n' }, + ); + const paths = firstEntry(result).warnings.map((w) => w.path); + assert.ok(paths.includes('src/rogue.ts'), 'the advisory must not be blunted by this change'); + }); + + // ── git C-quoting decode: both sides meet in the same shape ──────────────────────────── + + test('a declared non-ASCII deletion merges even though git C-quotes the path', () => { + // With core.quotepath at its git default, a non-ASCII deleted path comes back from + // `git diff --diff-filter=D --name-only` wrapped in double quotes and C-escaped: + // `tests/é.ts` is reported as the literal string built here with String.raw so the + // runtime value actually contains backslash-3-0-3 / backslash-2-5-1 sequences, not a + // JS-interpreted escape. Confirmed via `raw.length === 19` and `raw.includes('\\303')`. + // Without decodeGitQuotedPath this quoted form can never equal the plainly-declared + // path below, so the entry would block forever. + const quoted = String.raw`"tests/\303\251.ts"`; + const result = runWave( + { declared_deletions: ['tests/é.ts'] }, + { deletions: `${quoted}\n` }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + }); + + test('a declaration written in git-quoted form also matches a plainly reported path', () => { + // The reverse direction: normalizeScopePath runs on BOTH sides, so a declaration + // authored in the quoted-and-escaped form must still match a plain git report. This + // pins the symmetry so a future one-sided decode (only on the git side) is caught. + const quoted = String.raw`"tests/\303\251.ts"`; + const result = runWave( + { declared_deletions: [quoted] }, + { deletions: 'tests/é.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + }); + + test('an undeclared non-ASCII deletion still blocks, and the residue names the decoded path', () => { + // The path must not be declared, so the guard blocks — and the operator-facing detail + // must show the DECODED path (the one they can actually act on), not the raw escaped + // quoted form git emitted. + const quoted = String.raw`"tests/\303\251.ts"`; + const result = runWave( + { declared_deletions: ['src/keep.ts'] }, + { deletions: `${quoted}\n` }, + ); + assert.ok(blockedOnDeletions(result)); + const detail = firstEntry(result).stderr; + assert.match(detail, /tests\/é\.ts/, 'the block detail must name the decoded path, not the raw escaped form'); + assert.doesNotMatch(detail, /\\303\\251/, 'the raw C-escaped bytes must not leak into the operator-facing detail'); + }); + + test('a path merely containing a quote is not decoded', () => { + // `tests/a"b.ts` is not wrapped in a leading-and-trailing quote pair, so the + // startsWith('"') && endsWith('"') guard must leave it completely untouched — declaring + // that exact literal string must still merge. + const result = runWave( + { declared_deletions: ['tests/a"b.ts'] }, + { deletions: 'tests/a"b.ts\n' }, + ); + assert.ok(!blockedOnDeletions(result), `expected merge, got ${firstEntry(result).reason}`); + }); + + // NOTE: "a fully declared deletion merges" (above) already covers a plain ASCII declared + // deletion merging — no additional plain-ASCII regression test added here to avoid + // duplicating it. + + // ── #2852 regression: a block isolates, it does not abort the wave ───────────────────── + + test('a blocked entry does not abort the rest of the wave', () => { + const second = { agent_id: 'a2', worktree_path: '/repo/.claude/worktrees/agent-a2', branch: 'worktree-agent-a2', expected_base: 'abc123' }; + const result = executeWorktreeWaveCleanupPlan( + { + ok: true, + repoRoot: REPO, + action: 'cleanup_wave', + discovery: 'manifest', + entries: [ + { agent_id: 'a1', worktree_path: WT, branch: BR, expected_base: 'abc123', declared_deletions: ['tests/a.test.ts'] }, + second, + ], + }, + { execGit: makeDeletionGit({ deletions: 'tests/a.test.ts\nsrc/billing.ts\n' }) }, + ); + assert.equal(result.entries[0].reason, 'branch_contains_deletions'); + assert.equal(result.entries.length, 2, 'the second entry must still have been processed'); + assert.deepEqual(result.pending, [], 'nothing may be left pending — that would be an aborted wave'); + }); + + // ── property: authorization is exactly set membership ────────────────────────────────── + + test('property: a deletion merges iff its normalized path is in the declared set', () => { + const PATHS = ['a.ts', 'src/b.ts', 'tests/c.test.ts', 'deep/nested/d.ts', 'e.md']; + fc.assert( + fc.property( + fc.subarray(PATHS, { minLength: 1 }), + fc.subarray(PATHS, { minLength: 1 }), + (deleted, declared) => { + const result = runWave( + { declared_deletions: declared }, + { deletions: `${deleted.join('\n')}\n` }, + ); + const everyDeletionDeclared = deleted.every((p) => declared.includes(p)); + assert.equal( + !blockedOnDeletions(result), + everyDeletionDeclared, + `deleted=${JSON.stringify(deleted)} declared=${JSON.stringify(declared)}`, + ); + }, + ), + { seed: 3003, numRuns: 200 }, + ); + }); +});