From a5706bd39d74e5a6382545252cb92fd5f6e05a9c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 9 Aug 2026 16:12:57 -0400 Subject: [PATCH] enhance(#2596): validate a wave branch's committed diff stays in its declared scope (#3264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2596): failing-first suite for worktree-wave scope conformance Binds the advisory diff-vs-declared-scope check to behavior before it exists: the pure coverage predicate, the SUMMARY-artifact exemption and its parity with the rescue walker, the gauntlet integration (never flips ok, degrades on a git failure, survives a later block), the manifest normalizer's files_modified handling, and the --files negative-input matrix on record-agent/create. Refs #2596 * enhance(#2596): warn when a wave branch commits outside its declared scope The worktree-wave merge gauntlet validated branch, base, deletions, SUMMARY rescue and a clean worktree, but never compared a plan branch's actual committed diff against the files_modified the plan declared — so an executor that committed outside its brief merged into shared phase state silently. Adds an advisory scope-conformance check: when the manifest entry carries a declared scope, the gauntlet diffs HEAD... and appends one structured warning per path outside it. It never flips ok and never blocks the merge; promotion to a hard gate is a separate, disclosed change. With no declared scope no git subprocess is spent at all. Refs #2596 * docs(#2596): document the advisory worktree-wave scope-conformance check Records the optional --files flag on worktree record-agent/create, the advisory warnings channel cleanup-wave now emits, and its two deliberate noise limits (SUMMARY-artifact exemption, literal-prefix glob matching). Wires execute-phase to pass the plan's already-parsed PLAN_FILES. Refs #2596 * fix(#2596): close review findings on the scope-conformance advisory - share one path normalizer between the SUMMARY-artifact predicate and the scope comparison so the exemption and the check cannot drift - wire --files into the orchestrator-worktree dispatch, which created a worktree but never declared its scope, so the advisory silently did not apply on that backend; ADR-1239 requires both adapters share one check - correct the now-false blockquote claiming the check does not exist yet - add the fast-check property tests the repo requires for parser logic - add the record-agent/create parity test that Generative Fix Divergence requires for two surfaces implementing one rule Refs #2596 * fix(#2596): keep execute-phase.md under the frozen pre-phase-6 byte ceiling The one-sentence note added with the --files flag pushed execute-phase.md to 93708 bytes, past the ADR-857 PRE_PHASE6 cap of 93600 — the tightest of the three workflow size gates, and a hard cap an acknowledgment cannot clear. It failed three tests plus the differential attribution check. Condense the note to a one-line pointer (93543, 57 B of headroom); the full explanation already lives in docs/CLI-TOOLS.md and the dispatch step. The flag itself stays in the command, because the orchestrator reads this workflow at runtime and cannot pick it up from docs/. Acknowledge the remaining 143 B of growth by appending to the existing execute-phase.md fragment rather than adding a second one — the ack lint rejects two sources naming the same path. Refs #2596 * fix(#2596): make the execute-phase.md edit net-negative, not merely under the cap The size gate on this file is two assertions, not one: bytes < 93600 AND bytes <= 93400. The base is exactly 93400, so the file is at its budget and any growth trips the margin assertion — the previous fix cleared the ceiling but not that. Move the --files explanation to per-plan-worktree-gate.md, which already owns PLAN_FILES and carries no cap, and reclaim the rest from two clauses in the sentence being edited: the cleanup-wave rules phrasing, and a 'non-zero exit' the very next sentence already states. execute-phase.md ends at 93392, eight bytes below base. The flag itself stays in the command — the orchestrator reads this workflow at runtime and cannot pick it up from docs/. With no growth left, the acknowledgment is unnecessary and its byte delta was no longer true, so the shared ack fragment is restored byte-identical to base. Refs #2596 * docs(#2596): add the how-to for interpreting scope-conformance warnings The docs for this change were entirely Reference — the flag and the warning codes — with the task-oriented quadrant empty. Adds the page that answers the question an operator actually has when the advisory fires: what the two codes mean, that nothing is blocked so there is no failure to hunt for, how to tell whether the executor over-reached or the plan under-declared, and the three ways the check legitimately stays silent so an absence of warnings is not mistaken for proof of conformance. Refs #2596 * chore(#2596): backfill changeset pr number to 3264 --------- Co-authored-by: sim --- .changeset/curious-tunas-greet.md | 5 + CONTEXT.md | 2 +- docs/CLI-TOOLS.md | 20 +- docs/README.md | 1 + .../interpret-scope-conformance-warnings.md | 96 +++ gsd-core/workflows/execute-phase.md | 2 +- .../steps/executor-isolation-dispatch.md | 7 +- .../steps/per-plan-worktree-gate.md | 2 + src/worktree-safety.cts | 203 +++++- tests/worktree-safety.test.cjs | 631 ++++++++++++++++++ 10 files changed, 952 insertions(+), 17 deletions(-) create mode 100644 .changeset/curious-tunas-greet.md create mode 100644 docs/how-to/interpret-scope-conformance-warnings.md diff --git a/.changeset/curious-tunas-greet.md b/.changeset/curious-tunas-greet.md new file mode 100644 index 000000000..39433470a --- /dev/null +++ b/.changeset/curious-tunas-greet.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3264 +--- +**Worktree-wave merges now warn when a plan branch committed outside its declared scope** — the `execute-phase` cleanup gauntlet compares each branch's actual committed diff against the `files_modified` the plan declared and reports every path outside it. Advisory only: the merge still proceeds and the exit status is unchanged. (#2596) diff --git a/CONTEXT.md b/CONTEXT.md index 6e127b5da..693fb2beb 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -136,7 +136,7 @@ Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `\0` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `DEFECT.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module. ### Worktree Safety Policy Module -CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult`, `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — declared and testable but UNCONSUMED (no scheduler calls it yet; Phase 3 wires it). Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. +CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult` (per-entry gauntlet: branch → base → deletions → **advisory scope conformance (#2596)** → SUMMARY-rescue → clean-worktree → merge → remove; the scope check compares the branch's committed diff against the entry's declared `files_modified` and appends `WAVE_CLEANUP_WARNING`-coded entries to a `warnings` channel WITHOUT touching `ok` — an advisory, not a gate, and skipped entirely with no git call when no scope was declared), `planWaveScopeConformance(changedPaths, declaredFiles, branch) → WaveCleanupWarning[]` (pure; literal-prefix path coverage deliberately mirroring the submodule-intersection gate's glob-prefix rule rather than introducing a second matcher; over-accepts by design because a false alarm costs an advisory more than a miss), `isSummaryArtifactRelPath(relPath) → boolean` (the single definition of "executor-written SUMMARY artifact", shared with `defaultFindSummaryFiles` so the rescue walker and the scope exemption cannot drift), `WAVE_CLEANUP_WARNING` (frozen advisory-code enum: `scope_out_of_declared`, `scope_check_unavailable`), `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: requires `--root` — confinement is mandatory, not opt-in; omitting it fails closed with `reason:'root_required'` before any git side effect, rather than silently creating an unconfined worktree (#3050); plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — declared and testable but UNCONSUMED (no scheduler calls it yet; Phase 3 wires it). `worktree record-agent` / `worktree create` accept an optional `--files` recording the plan's declared scope, consumed by the advisory scope-conformance check above; a blank or omitted value leaves the 4-field on-disk entry shape unchanged. Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps) → {root, reason}` (a projection over `resolveWorktreeContext`; returns the `reason` alongside `root` — a `git_timed_out` reason means `root` is a best-effort cwd fallback, not a confirmed resolution, and callers must surface that risk rather than trust it silently, #3050) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. ### Worktree Lifecycle Module Workflow contract seam covering agent worktree lifecycle orchestration rules. The `worktree_branch_check` block lives in one canonical fragment (`gsd-core/references/worktree-branch-check.md`) that `execute-phase.md`, `quick.md`, `diagnose-issues.md`, and `execute-plan.md` embed at dispatch. Key invariants: `worktree_branch_check` is **verify-only and fail-closed** — the orchestrator owns worktree lifecycle and base recovery, so the sub-agent holds no state-correction primitives; HEAD attachment verified via `git symbolic-ref`; positive allow-list `^worktree-agent-*` enforced; `git update-ref` on protected refs is prohibited; on base mismatch the sub-agent halts with `exit 42` and surfaces to the orchestrator (#48); the orchestrator runs a cwd-drift guard at `execute_waves` entry that resolves the worktree root and refuses drift into an agent worktree (#48); #1856: that refusal now also reports what the agent worktree holds — commits ahead of the resolved base and uncommitted files, both with true counts plus a truncation notice — and the commit/switch/merge-or-cherry-pick sequence to integrate them, because `re-run from the orchestrator worktree` alone silently meant abandoning work that lives only on the agent branch. The refusal condition and exit code are unchanged, and every added command is diagnostic and `|| true`-guarded so a failure degrades to the plain refusal; cleanup is manifest-scoped (`WAVE_WORKTREE_MANIFEST`) not global-discovery-based; worktree spawning is sequential (one `run_in_background` at a time to avoid `config.lock` contention). Test anchor: `tests/worktree.test.cjs`. diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index f71eabc1f..84a9d7d9a 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -842,10 +842,11 @@ node gsd-tools.cjs worktree set-baseref # Returns JSON: { ok, reason, entry, manifest_path } (exit 0), or # { ok:false, reason, hint } with a non-zero exit on a rejected/failed create. node gsd-tools.cjs worktree create \ - --manifest --agent-id --path --branch --base --root + --manifest --agent-id --path --branch --base --root \ + [--files ""] ``` -**`worktree create`** validates and records the manifest entry BEFORE running any git command, then runs `git worktree add` for the validated `{path, branch, base}`, and only on success finalizes the manifest write — a rejected entry or a failed `git worktree add` never leaves a partially-recorded manifest or an unmanifested worktree on disk. `--root` is **mandatory** (#3050): the fail-closed root-confinement check resolves `--path` and `--root` and rejects (`reason:"path_outside_root"`) unless `--path` resolves strictly inside `--root` — this closes a prior gap where an unconfined `--path` (no `--root` check at all) could point a spawned executor's worktree anywhere on the filesystem. Omitting `--root` fails closed with `reason:"root_required"` rather than silently skipping confinement. All other flags share `worktree record-agent`'s validation rules above (`--branch` namespace, non-empty/non-whitespace `--path`/`--branch`/`--base`, `--agent-id` required). +**`worktree create`** validates and records the manifest entry BEFORE running any git command, then runs `git worktree add` for the validated `{path, branch, base}`, and only on success finalizes the manifest write — a rejected entry or a failed `git worktree add` never leaves a partially-recorded manifest or an unmanifested worktree on disk. `--root` is **mandatory** (#3050): the fail-closed root-confinement check resolves `--path` and `--root` and rejects (`reason:"path_outside_root"`) unless `--path` resolves strictly inside `--root` — this closes a prior gap where an unconfined `--path` (no `--root` check at all) could point a spawned executor's worktree anywhere on the filesystem. Omitting `--root` fails closed with `reason:"root_required"` rather than silently skipping confinement. All other flags share `worktree record-agent`'s validation rules above (`--branch` namespace, non-empty/non-whitespace `--path`/`--branch`/`--base`, `--agent-id` required). It also accepts the same optional `--files` as `record-agent` (#2596). ### Wave-manifest recording @@ -856,10 +857,21 @@ The execute-phase orchestrator records each spawned executor's worktree identity # Returns JSON: { ok, reason, entry, manifest_path } (exit 0), or # { ok:false, reason, hint } with a non-zero exit on a rejected entry. node gsd-tools.cjs worktree record-agent \ - --manifest --agent-id --path --branch --base + --manifest --agent-id --path --branch --base \ + [--files ""] ``` -**`worktree record-agent`** appends one `{agent_id, worktree_path, branch, expected_base}` entry to an already-initialized manifest, validating every field **at write time using the same rules the `cleanup-wave` reader enforces** — `--branch` must match the disposable `^(worktree-)?agent-[A-Za-z0-9._/-]+$` namespace (accepts both `agent-` and legacy `worktree-agent-`), and `--path`/`--branch`/`--base` must be non-empty. `--agent-id` is required (write-strict), even though the reader treats it as optional. A missing or garbled field — or a duplicate `(worktree_path, branch)` the reader would dedup away — fails loudly with a recovery hint and a non-zero exit **without** writing, instead of appending an under-populated or silently-dropped entry. Whitespace-only `--path`/`--base` are rejected (values are trimmed). The on-disk manifest shape is unchanged (the reader re-derives `allowed_bases`); the orchestrator still initializes the empty `{orchestrator_root, worktrees: []}` shell inline before any agent is recorded. +**`worktree record-agent`** appends one `{agent_id, worktree_path, branch, expected_base}` entry to an already-initialized manifest, validating every field **at write time using the same rules the `cleanup-wave` reader enforces** — `--branch` must match the disposable `^(worktree-)?agent-[A-Za-z0-9._/-]+$` namespace (accepts both `agent-` and legacy `worktree-agent-`), and `--path`/`--branch`/`--base` must be non-empty. `--agent-id` is required (write-strict), even though the reader treats it as optional. A missing or garbled field — or a duplicate `(worktree_path, branch)` the reader would dedup away — fails loudly with a recovery hint and a non-zero exit **without** writing, instead of appending an under-populated or silently-dropped entry. Whitespace-only `--path`/`--base` are rejected (values are trimmed). The on-disk manifest shape is unchanged unless `--files` is supplied (see below); the reader still re-derives `allowed_bases`, and the orchestrator still initializes the empty `{orchestrator_root, worktrees: []}` shell inline before any agent is recorded. + +`--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. + +**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`. + +This is advisory: it does not change `ok`, `reason`, the per-entry `status`, or the exit code, and the merge proceeds either way. Promotion to a hard gate would be a separate, disclosed change. + +Two deliberate limits keep it from crying wolf. `.planning/**/*SUMMARY.md` paths are always exempt — the executor writes a SUMMARY by orchestration contract and no plan declares it. Glob patterns are matched by their literal prefix only, so `src/**/*.ts` covers everything under `src/`, and a pattern with no literal prefix (`*.md`) suppresses warnings for that entry rather than reporting every file. --- diff --git a/docs/README.md b/docs/README.md index 38e008f83..5aa89fe0f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -33,6 +33,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Work in parallel with workstreams](how-to/work-in-parallel-with-workstreams.md) — run independent lines of work simultaneously using workstreams - [Isolate work with workspaces](how-to/isolate-work-with-workspaces.md) — use workspaces to sandbox experimental or risky changes - [Debug a failed execution](how-to/debug-a-failed-execution.md) — diagnose and recover from broken or incomplete phase execution +- [Interpret scope-conformance warnings](how-to/interpret-scope-conformance-warnings.md) — read the advisory the worktree-wave merge emits when a plan branch commits outside its declared scope - [Spike and sketch](how-to/spike-and-sketch.md) — use `/gsd-spike` and `/gsd-sketch` for exploratory work before committing to a plan - [Design a UI phase](how-to/design-a-ui-phase.md) — use the UI phase loop for frontend and visual work - [Develop a Capability for GSD 1.5+](how-to/develop-a-capability.md) — add feature Capabilities, hook fragments, and registry entries diff --git a/docs/how-to/interpret-scope-conformance-warnings.md b/docs/how-to/interpret-scope-conformance-warnings.md new file mode 100644 index 000000000..0decc9c00 --- /dev/null +++ b/docs/how-to/interpret-scope-conformance-warnings.md @@ -0,0 +1,96 @@ +# How to interpret scope-conformance warnings + +**Goal:** Understand the advisory `warnings` that the `worktree.cleanup-wave` gauntlet emits when a plan branch commits changes outside the scope it declared, and decide what — if anything — to do about one. + +**Prerequisites:** GSD Core is installed and you have run `/gsd-execute-phase` with worktree isolation enabled (`workflow.use_worktrees: true`, the default), so that plan branches were merged back by a `cleanup-wave`. + +--- + +## What you will see + +When `/gsd-execute-phase` runs plans in isolated worktrees, each plan branch is merged back into the phase branch by the `worktree.cleanup-wave` gauntlet. If the wave manifest recorded the plan's declared scope (`files_modified`, passed as `--files` to `worktree record-agent` / `worktree create`), the gauntlet compares the branch's actual committed diff (`HEAD...`) against that declared scope and reports any committed path that falls outside it. + +A realistic `cleanup-wave` result with one such warning: + +```json +{ + "ok": true, + "reason": "wave-cleanup-complete", + "warnings": [ + { + "code": "scope_out_of_declared", + "branch": "plan-04-add-rate-limiter", + "path": "src/util/rate-limiter-cache.cts" + } + ], + "entries": [ + { + "branch": "plan-04-add-rate-limiter", + "status": "merged_removed", + "warnings": [ + { + "code": "scope_out_of_declared", + "path": "src/util/rate-limiter-cache.cts" + } + ] + } + ] +} +``` + +`ok: true` and `status: "merged_removed"` are unchanged by the warning — the merge happened. The same warning object appears twice: once nested under the offending entry, and once aggregated into the top-level `warnings` array so you can scan for conformance issues across the whole wave without walking every entry. + +--- + +## Why this happens + +`/gsd-execute-phase` gives each plan a declared scope up front (`files_modified` in the phase plan index), so that parallel plan branches can be reasoned about and merged with some confidence about what each one touched. The scope-conformance check is a post-hoc, best-effort verification of that promise: after a branch merges, the gauntlet diffs what was actually committed against what was declared, and flags any mismatch as a warning. This is issue #2596. + +--- + +## The two warning codes + +- **`scope_out_of_declared`** — emitted once per committed path that falls outside the plan's declared scope. A branch with three unexpected paths produces three of these warnings. +- **`scope_check_unavailable`** — emitted once per entry when the scope diff itself could not be computed (a `git` failure or a timeout), rather than once per path. This means conformance is **unknown** for that entry, not clean — the check deliberately distinguishes "could not verify" from "verified and clean" so an unknown result is never silently read as a pass. + +--- + +## It does not block + +Scope conformance is advisory by design. A `scope_out_of_declared` or `scope_check_unavailable` warning never changes `ok`, `reason`, the per-entry `status`, or the process exit code — the merge proceeds exactly as it would with zero warnings. There is no failure to fix here. Promoting scope conformance to a hard gate (one that blocks the merge) would be a separate, explicitly disclosed change — it is not what this check does today. + +--- + +## What to do about one + +1. Compare the reported `path` against the plan's declared `files_modified` in the phase plan index. +2. Decide which side is wrong: either the executor committed something outside its brief (over-reach), or the plan's `files_modified` under-declared what the work actually needed to touch. +3. If the plan under-declared its scope, widen `files_modified` in the plan so future waves report accurately. +4. If the executor over-reached, review that path's changes specifically — it is already merged into the phase branch, so treat the review as catching it before it travels further (into a PR, a release, or a later phase). + +--- + +## When nothing is reported + +Absence of `scope_out_of_declared` / `scope_check_unavailable` warnings is not proof that a branch stayed in scope. The check legitimately stays silent in three cases: + +- **No `--files` was recorded for the plan.** Scope is unknown, so no comparison runs at all — not even a `git` call is made for that entry. +- **Every out-of-scope path is a `.planning/**/*SUMMARY.md` artifact.** These are always exempt: the executor writes them by orchestration contract, and no plan declares them as part of its scope, so flagging them would be noise rather than signal. +- **A declared pattern had no literal prefix** (for example, `*.md`). Matching is prefix-based (see below), so a pattern with no literal prefix cannot usefully bound anything — the check suppresses warnings for that entry by design, so an advisory check never cries wolf on a pattern it cannot meaningfully evaluate. + +--- + +## 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. +- **`/gsd-quick` worktrees have no plan-declared scope.** They are never checked, for the same reason as the "no `--files` recorded" case above. + +--- + +## Related + +- [CLI Tools reference — worktree commands](../CLI-TOOLS.md#worktree-commands) — the `worktree record-agent` / `worktree create` `--files` reference +- [Execute a phase](execute-a-phase.md) +- [Debug a failed execution](debug-a-failed-execution.md) +- [docs index](../README.md) diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index cfe342083..f92e1117e 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -777,7 +777,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 …`. The verb validates every field at write time using the same rules the `cleanup-wave` reader enforces (write-strict `--agent-id`), failing loudly with a non-zero exit and 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"`. 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 e3344f888..51b1656db 100644 --- a/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md +++ b/gsd-core/workflows/execute-phase/steps/executor-isolation-dispatch.md @@ -154,7 +154,8 @@ CREATE_JSON=$(gsd_run query worktree.create \ --path "$WT_PATH" \ --branch "$WT_BRANCH" \ --base "$EXPECTED_BASE" \ - --root "$ORCH_ROOT" 2>&1) || { + --root "$ORCH_ROOT" \ + --files "$PLAN_FILES" 2>&1) || { echo "FATAL: worktree create failed for plan {plan_number}: $CREATE_JSON" >&2 exit 1 } @@ -180,6 +181,8 @@ if [ "$EXEC_OK" != "true" ]; then 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. + `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. @@ -188,5 +191,5 @@ The executor never touches `STATE.md`/`ROADMAP.md`, and that guard needs no new Merge-back, validation, and cleanup are the **existing** gauntlet, unchanged: the serialized `worktree.cleanup-wave` merge loop that stops the wave and retains the worktree on conflict, and manifest-only cleanup (never glob-inferred). Because the manifest shape is identical, the orchestrator path reuses it verbatim. -> **Declared-scope conformance (#2596):** ADR-1239 specifies that *both* isolation adapters route their merge through a check that each plan branch's committed diff stayed inside its declared `files_modified` scope. That check does not exist yet for either adapter (it is tracked as #2596). When it lands it must be wired into this path **and** the harness path together. +> **Declared-scope conformance (#2596):** ADR-1239 specifies that *both* isolation adapters route their merge through a check that each plan branch's committed diff stayed inside its declared `files_modified` scope. That check now exists, advisory-first, and is wired into **both** paths: this one passes `--files "$PLAN_FILES"` to `worktree create` above, the harness path passes it to `worktree record-agent`, and `cleanup-wave` runs the one comparison for both. A path outside the declared scope is reported in the result's `warnings` array; it does not block the merge. Promotion to a hard gate is a separate, disclosed change. 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 898060617..2a999fb66 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 @@ -111,3 +111,5 @@ else gsd_run query dispatch-isolation --raw --phase "${PHASE_NUMBER:-}" --plan "$plan_id" >/dev/null 2>&1 || true 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. diff --git a/src/worktree-safety.cts b/src/worktree-safety.cts index 741ed6734..dc2687bbb 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -484,6 +484,8 @@ interface CleanupManifestEntry { branch: string; expected_base: string; allowed_bases?: string[]; + /** #2596: the plan's declared `files_modified`, when the recorder supplied it. Absent = unknown, never "declares nothing". */ + files_modified?: string[]; } function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | null { @@ -500,13 +502,23 @@ function normalizeCleanupManifestEntry(entry: unknown): CleanupManifestEntry | n const allowedBases = Array.from(new Set( [expectedBase, ...rawAllowedBases.filter((base): base is string => typeof base === 'string' && base.length > 0)] )); - return { + // #2596: liberal in what we accept — non-array, or non-string / empty + // elements, are dropped rather than coerced. An EMPTY result omits the field + // entirely, so "declared nothing" is indistinguishable from "not recorded": + // that ambiguity is already resolved as *unknown* by the per-plan submodule + // gate's own `[ -z "$PLAN_FILES" ]` rule, and inventing a second rule here + // 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); + const normalized: CleanupManifestEntry = { agent_id: typeof e.agent_id === 'string' ? e.agent_id : null, worktree_path: worktreePath, branch, expected_base: expectedBase, allowed_bases: allowedBases, }; + if (filesModified.length > 0) normalized.files_modified = filesModified; + return normalized; } interface NormalizedManifestResult { @@ -613,6 +625,42 @@ function repoRootStillMidMerge(execGit: ExecGitFn, repoRoot: string): boolean { return true; // any other exit code (e.g. a fatal git error) — fail closed } +// #2596: the single definition of "this file is an executor-written SUMMARY +// artifact". Shared by `defaultFindSummaryFiles` (which walks for them to +// rescue) and the scope advisory below (which must never flag them) — a plan's +// declared `files_modified` never lists a SUMMARY, because the executor writes +// it by orchestration contract, so a second copy of this rule would make the +// advisory fire on essentially every wave. +const SUMMARY_ARTIFACT_DIR = '.planning'; +const SUMMARY_ARTIFACT_SUFFIX = 'SUMMARY.md'; + +/** + * 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. + */ +function normalizeScopePath(raw: string): string { + return String(raw || '') + .replace(/\\/g, '/') + .trim() + .replace(/^\.\//, '') + .replace(/\/+$/, ''); +} + +/** + * True when a worktree-relative path is a SUMMARY artifact. Input may use + * either separator; normalization to POSIX is unconditional (backslash paths + * reach Linux too). + */ +function isSummaryArtifactRelPath(relPath: string): boolean { + const normalized = normalizeScopePath(relPath); + return normalized.startsWith(`${SUMMARY_ARTIFACT_DIR}/`) + && normalized.endsWith(SUMMARY_ARTIFACT_SUFFIX); +} + /** * Walk /.planning/ recursively and collect absolute paths of * all files whose names match *SUMMARY.md. Returns [] when the directory @@ -622,7 +670,7 @@ function repoRootStillMidMerge(execGit: ExecGitFn, repoRoot: string): boolean { * find "$WT/.planning" -name "*SUMMARY.md" */ function defaultFindSummaryFiles(worktreePath: string): string[] { - const planningDir = path.join(worktreePath, '.planning'); + const planningDir = path.join(worktreePath, SUMMARY_ARTIFACT_DIR); const results: string[] = []; function walk(dir: string): void { let entries: fs.Dirent[]; @@ -631,7 +679,7 @@ function defaultFindSummaryFiles(worktreePath: string): string[] { const full = path.join(dir, entry.name); if (entry.isDirectory()) { walk(full); - } else if (entry.isFile() && entry.name.endsWith('SUMMARY.md')) { + } else if (entry.isFile() && entry.name.endsWith(SUMMARY_ARTIFACT_SUFFIX)) { results.push(full); } } @@ -742,10 +790,90 @@ function rescueSummaryArtifacts( return { rescuedRelPaths, failures }; } +/** + * #2596: advisory codes emitted by the wave-cleanup gauntlet. Frozen and + * exported so tests assert on a code rather than on rendered prose (this repo + * forbids raw-text matching on test output). + */ +const WAVE_CLEANUP_WARNING = Object.freeze({ + /** A committed path fell outside the plan's declared `files_modified`. */ + SCOPE_OUT_OF_DECLARED: 'scope_out_of_declared', + /** The scope diff could not be computed, so conformance is unknown. */ + SCOPE_CHECK_UNAVAILABLE: 'scope_check_unavailable', +}); + +interface WaveCleanupWarning { + code: string; + branch: string; + /** The offending path; null when the check itself could not run. */ + path: string | null; +} + +/** + * The literal directory prefix a declared path covers, or `null` when the + * pattern begins with a glob metacharacter and therefore has no usable prefix. + * + * Deliberately literal-prefix only — NOT a glob engine. A hand-rolled + * `**`/`*`/`?` matcher inside a worktree-lifecycle module is an informal, + * undocumented pattern language living where no language belongs, and this + * repo forbids external deps in core. The submodule-intersection gate + * (`workflows/execute-phase/steps/per-plan-worktree-gate.md`) already ships + * exactly this glob-prefix rule; reusing it beats inventing a second one. + * + * `null` (no literal prefix, e.g. `*.md`) means "matches everything": for an + * ADVISORY, a false alarm costs more than a miss, so the ambiguous case + * suppresses rather than shouts. + */ +function declaredScopePrefix(declared: string): string | null { + const globAt = declared.search(/[*?[]/); + if (globAt < 0) return declared; + const literal = declared.slice(0, globAt).replace(/\/+$/, ''); + return literal.length > 0 ? literal : null; +} + +/** + * #2596: compare a branch's actual committed paths against the plan's declared + * scope. Pure — no git, no IO. Returns one warning per out-of-scope path, in + * the order the paths were given. Returns [] when nothing usable was declared: + * absence of data is not evidence of over-reach. + */ +function planWaveScopeConformance( + changedPaths: unknown, + declaredFiles: unknown, + branch: string, +): WaveCleanupWarning[] { + if (!Array.isArray(declaredFiles)) return []; + const prefixes: Array = []; + for (const declared of declaredFiles) { + if (typeof declared !== 'string') continue; + const normalized = normalizeScopePath(declared); + if (!normalized) continue; + prefixes.push(declaredScopePrefix(normalized)); + } + if (prefixes.length === 0) return []; + + const warnings: WaveCleanupWarning[] = []; + const seen = new Set(); + for (const raw of Array.isArray(changedPaths) ? changedPaths : []) { + if (typeof raw !== 'string') continue; + const changed = normalizeScopePath(raw); + if (!changed || seen.has(changed)) continue; + seen.add(changed); + if (isSummaryArtifactRelPath(changed)) continue; + const covered = prefixes.some((prefix) => ( + prefix === null || changed === prefix || changed.startsWith(`${prefix}/`) + )); + if (covered) continue; + warnings.push({ code: WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, branch, path: changed }); + } + return warnings; +} + interface WaveCleanupEntryResult extends CleanupManifestEntry { status: string; reason: string | null; stderr: string; + warnings: WaveCleanupWarning[]; } interface WaveCleanupResult { @@ -754,6 +882,7 @@ interface WaveCleanupResult { reason: string; entries: WaveCleanupEntryResult[]; pending: CleanupManifestEntry[]; + warnings: WaveCleanupWarning[]; } function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: WorktreeDeps = {}): WaveCleanupResult { @@ -766,11 +895,13 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work reason: plan ? (plan.reason || 'missing_entries') : 'missing_plan', entries: [], pending: entries, + warnings: [], }; } const results: WaveCleanupEntryResult[] = []; const pending: CleanupManifestEntry[] = []; + const allWarnings: WaveCleanupWarning[] = []; let ok = true; // #2852: every per-entry failure site marks the SAME shape — status='blocked', @@ -793,6 +924,7 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work status: 'pending', reason: null, stderr: '', + warnings: [], }; const branchCheck = execGit(['-C', entry.worktree_path, 'rev-parse', '--abbrev-ref', 'HEAD'], { cwd: plan.repoRoot }); @@ -827,6 +959,26 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work continue; // #2852: isolate } + // #2596: advisory scope conformance — does the branch's ACTUAL committed + // diff stay inside the scope the plan declared? Gated on a declared scope + // being present: with nothing declared there is nothing to compare, so no + // git subprocess is spent at all (and every pre-#2596 fixture, none of + // which declares one, issues exactly the git calls it always did). + // + // 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) { + 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); + result.warnings.push(...scopeWarnings); + allWarnings.push(...scopeWarnings); + } + // Safety net: rescue uncommitted SUMMARY.md artifacts before the dirty check. // The executor leaves -SUMMARY.md uncommitted by contract — the // orchestrator commits it. Mirrors quick.md shell fallback (#2296, #2070, #2838, #3804). @@ -915,6 +1067,7 @@ function executeWorktreeWaveCleanupPlan(plan: WaveCleanupPlan | null, deps: Work reason: ok ? 'ok' : 'cleanup_blocked', entries: results, pending, + warnings: allWarnings, }; } @@ -963,6 +1116,17 @@ interface RecordAgentFields { worktreePath: string; branch: string; base: string; + files?: string; +} + +/** + * #2596: split a `--files` value into declared paths. Whitespace-separated, + * matching the `PLAN_FILES` shape the per-plan worktree gate already builds + * with `jq -r '.files_modified // [] | join(" ")'`. Values are DATA compared + * against a diff — never opened, never passed to a shell. + */ +function parseDeclaredScopeFlag(raw?: string): string[] { + return String(raw || '').split(/\s+/).filter((token) => token.length > 0); } interface RecordAgentPlan { @@ -990,8 +1154,9 @@ interface RecordAgentPlan { * rejected loudly — the reader dedups on that key, so a re-record would be * silently dropped (the failure mode this verb exists to eliminate). The * on-disk shape stays the existing 4-field entry (`agent_id`, `worktree_path`, - * `branch`, `expected_base`) — no schema change; the reader re-derives - * `allowed_bases`. + * `branch`, `expected_base`) unless `--files` declares a scope, in which case + * an optional `files_modified` is appended (#2596); the reader still + * re-derives `allowed_bases`. */ function planWorktreeRecordAgent(manifestRaw: string, fields: RecordAgentFields): RecordAgentPlan { // 1. Write-strict required-field check (loud, with which flag is missing). @@ -1018,11 +1183,13 @@ 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); const candidate = { agent_id: agentId, worktree_path: worktreePath, branch, expected_base: base, + ...(declaredScope.length > 0 ? { files_modified: declaredScope } : {}), }; const entry = normalizeCleanupManifestEntry(candidate); if (!entry) { @@ -1111,6 +1278,11 @@ function planWorktreeRecordAgent(manifestRaw: string, fields: RecordAgentFields) branch: entry.branch, expected_base: entry.expected_base, }; + // #2596: only written when a scope was actually declared — conservative in + // what we send, so a blank --files leaves the 4-field shape untouched. + if (entry.files_modified && entry.files_modified.length > 0) { + recorded.files_modified = entry.files_modified; + } worktrees.push(recorded); return { @@ -1139,7 +1311,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 + * Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""] * * 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 @@ -1155,7 +1327,7 @@ function cmdWorktreeRecordAgent(cwd: string, args: string[] = [], deps: RecordAg const manifestPath = flag('--manifest'); if (!manifestPath) { - writeErr('Usage: worktree record-agent --manifest --agent-id --path --branch --base \n'); + writeErr('Usage: worktree record-agent --manifest --agent-id --path --branch --base [--files ""]\n'); process.exitCode = 2; return { ok: false, reason: 'usage', entry: null }; } @@ -1178,6 +1350,7 @@ function cmdWorktreeRecordAgent(cwd: string, args: string[] = [], deps: RecordAg worktreePath: flag('--path'), branch: flag('--branch'), base: flag('--base'), + files: flag('--files'), }); if (!plan.ok || plan.manifest === null) { @@ -1207,6 +1380,7 @@ interface WorktreeCreateFields { worktreePath: string; branch: string; base: string; + files?: string; } interface WorktreeCreatePlan { @@ -1244,11 +1418,13 @@ function planWorktreeCreate(fields: WorktreeCreateFields): WorktreeCreatePlan { }; } + const declaredScope = parseDeclaredScopeFlag(fields.files); const candidate = { agent_id: agentId, worktree_path: worktreePath, branch, expected_base: base, + ...(declaredScope.length > 0 ? { files_modified: declaredScope } : {}), }; const entry = normalizeCleanupManifestEntry(candidate); if (!entry) { @@ -1405,7 +1581,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 + * Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""] * * #2584 FIX 1 — ORDERING CONTRACT: every manifest read/parse/shape-validate/ * plan step runs BEFORE the git side effect (step 5). The ONLY manifest @@ -1426,7 +1602,7 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm const manifestPath = flag('--manifest'); if (!manifestPath) { - writeErr('Usage: worktree create --manifest --agent-id --path --branch --base --root \n'); + writeErr('Usage: worktree create --manifest --agent-id --path --branch --base --root [--files ""]\n'); process.exitCode = 2; return { ok: false, reason: 'usage' }; } @@ -1490,6 +1666,7 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm worktreePath: flag('--path'), branch: flag('--branch'), base: flag('--base'), + files: flag('--files'), }); if (!plan.ok || !plan.entry) { @@ -1559,6 +1736,11 @@ function cmdWorktreeCreate(cwd: string, args: string[] = [], deps: RecordAgentCm branch: plan.entry.branch, expected_base: plan.entry.expected_base, }; + // #2596: only written when a scope was actually declared — conservative in + // what we send, so a blank --files leaves the 4-field shape untouched. + if (Array.isArray(plan.entry.files_modified) && plan.entry.files_modified.length > 0) { + recorded.files_modified = plan.entry.files_modified; + } const dedupeKey = `${recorded.worktree_path}\0${recorded.branch}`; const alreadyPresent = worktrees.some((existing) => { const normalized = normalizeCleanupManifestEntry(existing); @@ -1963,6 +2145,9 @@ export = { normalizeCleanupManifest, planWorktreeWaveCleanup, executeWorktreeWaveCleanupPlan, + WAVE_CLEANUP_WARNING, + planWaveScopeConformance, + isSummaryArtifactRelPath, cmdWorktreeCleanupWave, planWorktreeRecordAgent, cmdWorktreeRecordAgent, diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index 01df5f94b..6f7f417b1 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -6466,3 +6466,634 @@ test('bug-3542: stash pushed in main checkout is visible inside a linked worktre }); }); } + +// ─── #2596: advisory diff-vs-declared-scope conformance at worktree-wave merge ─ +// +// The wave-cleanup gauntlet validated branch, base, deletions, SUMMARY rescue and +// a clean worktree — but never compared the branch's ACTUAL committed diff against +// the scope the plan declared in `files_modified`. An executor that committed +// outside its brief merged silently. This is the advisory-first check: it warns, +// it does not block (promotion to a gate is a separate, disclosed change). + +const { + WAVE_CLEANUP_WARNING, + planWaveScopeConformance, + isSummaryArtifactRelPath, + normalizeCleanupManifest, +} = require(WORKTREE_SAFETY_PATH); + +describe('#2596 scope conformance — planWaveScopeConformance (pure)', () => { + const BR = 'worktree-agent-a1'; + const codesOf = (warnings) => warnings.map((w) => w.code); + const pathsOf = (warnings) => warnings.map((w) => w.path); + + test('exact path match is in scope', () => { + assert.deepEqual(planWaveScopeConformance(['src/a.ts'], ['src/a.ts'], BR), []); + }); + + test('declared directory covers a file beneath it', () => { + assert.deepEqual(planWaveScopeConformance(['src/foo/bar.ts'], ['src/foo'], BR), []); + }); + + test('prefix sibling is out of scope (the boundary is /, not startsWith)', () => { + const warnings = planWaveScopeConformance(['src/foobar.ts'], ['src/foo'], BR); + assert.deepEqual(codesOf(warnings), [WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED]); + assert.deepEqual(pathsOf(warnings), ['src/foobar.ts']); + assert.equal(warnings[0].branch, BR); + }); + + test('trailing slash on a declared directory is normalized', () => { + assert.deepEqual(planWaveScopeConformance(['src/foo/bar.ts'], ['src/foo/'], BR), []); + }); + + test('leading ./ is normalized on both sides', () => { + assert.deepEqual(planWaveScopeConformance(['./src/a.ts'], ['./src/a.ts'], BR), []); + }); + + test('backslash separators normalize unconditionally (not only on win32)', () => { + assert.deepEqual(planWaveScopeConformance(['src\\a.ts'], ['src/a.ts'], BR), []); + assert.deepEqual(planWaveScopeConformance(['src/a.ts'], ['src\\a.ts'], BR), []); + }); + + test('glob literal prefix covers paths beneath it', () => { + assert.deepEqual(planWaveScopeConformance(['src/a/b.ts'], ['src/**/*.ts'], BR), []); + }); + + test('glob matching is literal-prefix only — the documented over-accept', () => { + // `src/**/*.ts` also covers `src/a/b.json`. Deliberate: for an advisory a false + // alarm costs more than a miss, and this mirrors the submodule-intersection + // gate's own glob-prefix handling rather than inventing a second matcher. + assert.deepEqual(planWaveScopeConformance(['src/a/b.json'], ['src/**/*.ts'], BR), []); + }); + + test('a pattern with no literal prefix suppresses warnings rather than crying wolf', () => { + assert.deepEqual(planWaveScopeConformance(['src/a.ts'], ['*.md'], BR), []); + }); + + test('an undeclared sibling file is out of scope', () => { + const warnings = planWaveScopeConformance(['src/b.ts'], ['src/a.ts'], BR); + assert.deepEqual(pathsOf(warnings), ['src/b.ts']); + }); + + test('an all-blank declared list is unknown, not empty — no warnings', () => { + assert.deepEqual(planWaveScopeConformance(['src/b.ts'], ['', ' ', './', '/'], BR), []); + }); + + test('an absent, empty or non-array declared list yields no warnings', () => { + assert.deepEqual(planWaveScopeConformance(['src/b.ts'], undefined, BR), []); + assert.deepEqual(planWaveScopeConformance(['src/b.ts'], [], BR), []); + assert.deepEqual(planWaveScopeConformance(['src/b.ts'], 'src/b.ts', BR), []); + }); + + test('a committed SUMMARY artifact is never a scope violation', () => { + assert.deepEqual( + planWaveScopeConformance(['.planning/q1-SUMMARY.md'], ['src/a.ts'], BR), + [], + ); + }); + + test('every out-of-scope path gets its own warning, in diff order', () => { + const warnings = planWaveScopeConformance( + ['src/a.ts', 'z/second.ts', 'a/first.ts'], + ['src/a.ts'], + BR, + ); + assert.deepEqual(pathsOf(warnings), ['z/second.ts', 'a/first.ts']); + assert.deepEqual(codesOf(warnings), [ + WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, + WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, + ]); + }); + + // CLAUDE.md → TEST RULES: a parser needs at least one property test. + // Both are deterministic — seed pinned, run count bounded — per the repo's + // "property tests must be reproducible" rule. The two segment alphabets are + // disjoint so the negative property can never accidentally build a path that + // IS covered, and neither alphabet contains `.planning`, so the SUMMARY + // exemption cannot mask a result. + test('property: any path beneath a declared literal directory is always in scope', () => { + fc.assert( + fc.property( + fc.array(fc.constantFrom('src', 'lib', 'core', 'deep'), { minLength: 1, maxLength: 4 }), + fc.array(fc.constantFrom('a', 'b', 'c', 'd.ts'), { minLength: 1, maxLength: 3 }), + (declaredSegments, tailSegments) => { + const declared = declaredSegments.join('/'); + const changed = `${declared}/${tailSegments.join('/')}`; + return planWaveScopeConformance([changed], [declared], BR).length === 0; + }, + ), + { numRuns: 250, seed: 2596 }, + ); + }); + + test('property: a path sharing no prefix with any declared path always warns exactly once', () => { + fc.assert( + fc.property( + fc.array(fc.constantFrom('src', 'lib', 'core'), { minLength: 1, maxLength: 3 }), + fc.array(fc.constantFrom('zeta', 'yankee', 'xray.ts'), { minLength: 1, maxLength: 3 }), + (declaredSegments, changedSegments) => { + const declared = declaredSegments.join('/'); + const changed = changedSegments.join('/'); + const warnings = planWaveScopeConformance([changed], [declared], BR); + return warnings.length === 1 + && warnings[0].path === changed + && warnings[0].code === WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED; + }, + ), + { numRuns: 250, seed: 2596 }, + ); + }); +}); + +describe('#2596 SUMMARY-artifact predicate and its parity with the walker', () => { + const nodeFs = require('node:fs'); + + test('a .planning SUMMARY is recognized', () => { + assert.equal(isSummaryArtifactRelPath('.planning/q1-SUMMARY.md'), true); + }); + + test('a nested .planning SUMMARY is recognized', () => { + assert.equal(isSummaryArtifactRelPath('.planning/phases/3/x-SUMMARY.md'), true); + }); + + test('SUMMARY.md outside .planning is not exempt', () => { + assert.equal(isSummaryArtifactRelPath('docs/SUMMARY.md'), false); + }); + + test('the SUMMARY suffix must terminate the name', () => { + assert.equal(isSummaryArtifactRelPath('.planning/SUMMARY.md.bak'), false); + }); + + test('the .planning boundary is a whole path segment', () => { + assert.equal(isSummaryArtifactRelPath('.planningx/q-SUMMARY.md'), false); + }); + + test('parity: the SUMMARY walker and the scope-exemption predicate agree', (t) => { + // The rescue walker roots at /.planning and accepts *SUMMARY.md. + // The scope advisory must exempt exactly that set, or it would warn on the + // very artifacts the rescue path exists to carry. Drive the PRODUCTION + // walker (no findSummaryFiles override) over a real temp tree and + // cross-check every collected path against the predicate. + const tmp = createTempDir('wt-2596-summary-parity'); + t.after(() => cleanup(tmp)); + + const planning = path.join(tmp, '.planning', 'phases', '3'); + nodeFs.mkdirSync(planning, { recursive: true }); + nodeFs.writeFileSync(path.join(planning, 'p3-SUMMARY.md'), 'x'); + nodeFs.writeFileSync(path.join(tmp, '.planning', 'SUMMARY.md'), 'x'); + // Decoys the walker must skip and the predicate must reject. + nodeFs.writeFileSync(path.join(planning, 'notes.md'), 'x'); + nodeFs.mkdirSync(path.join(tmp, 'docs'), { recursive: true }); + nodeFs.writeFileSync(path.join(tmp, 'docs', 'SUMMARY.md'), 'x'); + + const copiedFrom = []; + executeWorktreeWaveCleanupPlan( + { + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ + agent_id: 'a1', + worktree_path: tmp, + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }, + { + // No findSummaryFiles override: this exercises the production walker. + execGit: (args) => { + const key = args.join(' '); + if (key === `-C ${tmp} rev-parse --abbrev-ref HEAD`) { + return { exitCode: 0, stdout: 'worktree-agent-a1', stderr: '', timedOut: false }; + } + if (key === 'merge-base HEAD worktree-agent-a1') { + return { exitCode: 0, stdout: 'abc123', stderr: '', timedOut: false }; + } + // cat-file -e → non-zero, so every found SUMMARY is rescued (copied). + if (args.includes('cat-file')) { + return { exitCode: 128, stdout: '', stderr: '', timedOut: false }; + } + return { exitCode: 0, stdout: '', stderr: '', timedOut: false }; + }, + existsSync: () => false, + readFileSync: () => 'x', + mkdirSync: () => {}, + copyFileSync: (src) => { copiedFrom.push(src); }, + }, + ); + + assert.equal(copiedFrom.length, 2, + 'the walker must find exactly the two .planning SUMMARY artifacts, not the docs/ decoy'); + for (const abs of copiedFrom) { + const rel = abs.slice(tmp.length).replace(/^[/\\]/, '').replace(/\\/g, '/'); + assert.equal( + isSummaryArtifactRelPath(rel), true, + `a path the SUMMARY walker collects must be exempt from the scope advisory: ${rel}`, + ); + } + // Negative direction: paths the walker skips must also fail the predicate. + assert.equal(isSummaryArtifactRelPath('docs/SUMMARY.md'), false); + assert.equal(isSummaryArtifactRelPath('.planning/phases/3/notes.md'), false); + }); +}); + +describe('#2596 scope conformance — executeWorktreeWaveCleanupPlan integration', () => { + const WT = '/repo/.claude/worktrees/agent-a1'; + const BR = 'worktree-agent-a1'; + const SCOPE_DIFF = `diff --name-only HEAD...${BR}`; + const noSummaries = { findSummaryFiles: () => [] }; + + function makeGit(overrides = {}) { + const calls = []; + const git = (args) => { + const key = args.join(' '); + calls.push(key); + if (Object.prototype.hasOwnProperty.call(overrides, key)) return overrides[key]; + if (key === `-C ${WT} rev-parse --abbrev-ref HEAD`) { + return { exitCode: 0, stdout: BR, stderr: '', timedOut: false }; + } + if (key === `merge-base HEAD ${BR}`) { + return { exitCode: 0, stdout: 'abc123', stderr: '', timedOut: false }; + } + return { exitCode: 0, stdout: '', stderr: '', timedOut: false }; + }; + return { git, calls }; + } + + const planWith = (entry) => ({ + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [{ agent_id: 'a1', worktree_path: WT, branch: BR, expected_base: 'abc123', ...entry }], + }); + + test('no declared scope issues no scope git call at all', () => { + const { git, calls } = makeGit(); + const result = executeWorktreeWaveCleanupPlan(planWith({}), { execGit: git, ...noSummaries }); + assert.equal(result.ok, true); + assert.deepEqual(result.warnings, []); + assert.deepEqual(result.entries[0].warnings, []); + assert.equal( + calls.includes(SCOPE_DIFF), false, + 'an entry with no declared files_modified must not spend a git subprocess', + ); + }); + + test('an in-scope branch produces no advisory', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'src/a.ts\nsrc/b.ts\n', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts', 'src/b.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.equal(result.ok, true); + assert.deepEqual(result.entries[0].warnings, []); + assert.deepEqual(result.warnings, []); + }); + + test('an out-of-scope path warns without blocking the merge', () => { + const { git, calls } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'src/a.ts\nsecrets/creds.json\n', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.equal(result.ok, true, 'the advisory must NOT flip ok — it is not a gate'); + assert.equal(result.reason, 'ok'); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.deepEqual(result.entries[0].warnings, [ + { code: WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, branch: BR, path: 'secrets/creds.json' }, + ]); + assert.deepEqual(result.warnings, result.entries[0].warnings); + // Negative proof: the merge/remove/delete sequence still ran. + assert.ok(calls.some((c) => c.startsWith(`merge ${BR}`)), 'merge must still run'); + assert.ok(calls.includes(`worktree remove ${WT} --force`), 'remove must still run'); + assert.ok(calls.includes(`branch -D ${BR}`), 'branch delete must still run'); + }); + + test('every out-of-scope path gets its own warning, in diff order', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'z/two.ts\nsrc/a.ts\na/one.ts\n', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.deepEqual(result.entries[0].warnings.map((w) => w.path), ['z/two.ts', 'a/one.ts']); + }); + + test('a committed SUMMARY artifact in the diff is not a scope violation', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'src/a.ts\n.planning/q1-SUMMARY.md\n', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.deepEqual(result.entries[0].warnings, []); + }); + + test('a failed scope diff degrades to an advisory, never a block', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 128, stdout: '', stderr: 'fatal: bad revision', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.equal(result.ok, true, 'a broken advisory check must never become a gate'); + assert.equal(result.entries[0].status, 'merged_removed'); + assert.deepEqual(result.entries[0].warnings, [ + { code: WAVE_CLEANUP_WARNING.SCOPE_CHECK_UNAVAILABLE, branch: BR, path: null }, + ]); + }); + + test('a timed-out scope diff degrades to an advisory', () => { + const execGit = makeFaultyGit({ + faults: [{ + kind: 'timeout', + when: (args) => args[0] === 'diff' && !args.includes('--diff-filter=D'), + }], + passthrough: makeGit().git, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit, ...noSummaries }, + ); + assert.equal(result.ok, true); + assert.deepEqual(result.entries[0].warnings.map((w) => w.code), [ + WAVE_CLEANUP_WARNING.SCOPE_CHECK_UNAVAILABLE, + ]); + }); + + test('CRLF diff output yields clean paths and no phantom warning', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'src/a.ts\r\nsecrets/x.json\r\n\r\n', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.deepEqual(result.entries[0].warnings.map((w) => w.path), ['secrets/x.json']); + }); + + test('scope warnings survive a later block', () => { + const { git } = makeGit({ + [SCOPE_DIFF]: { exitCode: 0, stdout: 'secrets/x.json\n', stderr: '', timedOut: false }, + [`-C ${WT} status --porcelain --untracked-files=all`]: { exitCode: 0, stdout: '?? scratch.txt', stderr: '', timedOut: false }, + }); + const result = executeWorktreeWaveCleanupPlan( + planWith({ files_modified: ['src/a.ts'] }), + { execGit: git, ...noSummaries }, + ); + assert.equal(result.ok, false, 'the dirty-worktree block still blocks'); + assert.equal(result.entries[0].status, 'blocked'); + assert.equal(result.entries[0].reason, 'worktree_dirty'); + assert.deepEqual( + result.entries[0].warnings.map((w) => w.path), ['secrets/x.json'], + 'an advisory recorded before the block is still true and must survive it', + ); + }); + + test('warnings are attributed per entry and aggregated at the top level', () => { + const WT2 = '/repo/.claude/worktrees/agent-a2'; + const BR2 = 'worktree-agent-a2'; + const execGit = (args) => { + const key = args.join(' '); + if (key === `-C ${WT} rev-parse --abbrev-ref HEAD`) return { exitCode: 0, stdout: BR, stderr: '', timedOut: false }; + if (key === `-C ${WT2} rev-parse --abbrev-ref HEAD`) return { exitCode: 0, stdout: BR2, stderr: '', timedOut: false }; + if (key.startsWith('merge-base HEAD ')) return { exitCode: 0, stdout: 'abc123', stderr: '', timedOut: false }; + if (key === `diff --name-only HEAD...${BR}`) return { exitCode: 0, stdout: 'src/a.ts\n', stderr: '', timedOut: false }; + if (key === `diff --name-only HEAD...${BR2}`) return { exitCode: 0, stdout: 'src/rogue.ts\n', stderr: '', timedOut: false }; + return { exitCode: 0, stdout: '', stderr: '', timedOut: false }; + }; + const result = executeWorktreeWaveCleanupPlan({ + ok: true, + repoRoot: '/repo/main', + action: 'cleanup_wave', + discovery: 'manifest', + entries: [ + { agent_id: 'a1', worktree_path: WT, branch: BR, expected_base: 'abc123', files_modified: ['src/a.ts'] }, + { agent_id: 'a2', worktree_path: WT2, branch: BR2, expected_base: 'abc123', files_modified: ['src/b.ts'] }, + ], + }, { execGit, ...noSummaries }); + + assert.equal(result.ok, true); + assert.deepEqual(result.entries[0].warnings, []); + assert.deepEqual(result.entries[1].warnings, [ + { code: WAVE_CLEANUP_WARNING.SCOPE_OUT_OF_DECLARED, branch: BR2, path: 'src/rogue.ts' }, + ]); + assert.deepEqual( + result.warnings, result.entries[1].warnings, + 'the top-level aggregate carries every entry warning, tagged with its branch', + ); + }); + + test('WAVE_CLEANUP_WARNING is a frozen, locked code set', () => { + assert.deepEqual( + Object.keys(WAVE_CLEANUP_WARNING).sort(), + ['SCOPE_CHECK_UNAVAILABLE', 'SCOPE_OUT_OF_DECLARED'], + ); + assert.equal(Object.isFrozen(WAVE_CLEANUP_WARNING), true); + }); +}); + +describe('#2596 files_modified on the cleanup manifest', () => { + const base = { + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }; + const only = (manifest) => normalizeCleanupManifest(manifest).entries[0]; + const has = (obj, key) => Object.prototype.hasOwnProperty.call(obj, key); + + test('a files_modified array is carried through', () => { + assert.deepEqual( + only({ worktrees: [{ ...base, files_modified: ['src/a.ts'] }] }).files_modified, + ['src/a.ts'], + ); + }); + + test('a scalar files_modified is dropped, not coerced', () => { + assert.equal(has(only({ worktrees: [{ ...base, files_modified: 'src/a.ts' }] }), 'files_modified'), false); + }); + + test('non-string files_modified elements are dropped', () => { + assert.deepEqual( + only({ worktrees: [{ ...base, files_modified: [1, null, {}, 'src/a.ts'] }] }).files_modified, + ['src/a.ts'], + ); + }); + + test('an empty files_modified is treated as unknown (field omitted)', () => { + assert.equal(has(only({ worktrees: [{ ...base, files_modified: [] }] }), 'files_modified'), false); + }); + + test('an absent files_modified leaves the entry shape unchanged', () => { + assert.deepEqual( + Object.keys(only({ worktrees: [base] })).sort(), + ['agent_id', 'allowed_bases', 'branch', 'expected_base', 'worktree_path'], + ); + }); +}); + +describe('#2596 --files on the record-agent and create verbs', () => { + // process.exitCode is global; restore it so a failure-path exit code does not + // leak into the runner's own exit status. + function withExitCode2596(fn) { + const saved = process.exitCode; + try { return fn(); } finally { process.exitCode = saved; } + } + + const baseArgs = [ + '--manifest', 'manifest.json', + '--agent-id', 'a1', + '--path', '/repo/.claude/worktrees/agent-a1', + '--branch', 'worktree-agent-a1', + '--base', 'abc123', + ]; + const has = (obj, key) => Object.prototype.hasOwnProperty.call(obj, key); + + function recordAgent(extraArgs = []) { + let written = null; + const result = withExitCode2596(() => cmdWorktreeRecordAgent('/repo/main', [...baseArgs, ...extraArgs], { + readFile: () => '{"orchestrator_root":"/repo/main","worktrees":[]}', + writeFile: (_p, c) => { written = c; }, + write: () => {}, + writeErr: () => {}, + })); + return { result, entry: written ? JSON.parse(written).worktrees[0] : null }; + } + + test('--files records the declared scope', () => { + const { result, entry } = recordAgent(['--files', 'src/a.ts src/b.ts']); + assert.equal(result.ok, true); + assert.deepEqual(entry.files_modified, ['src/a.ts', 'src/b.ts']); + }); + + test('omitting --files leaves the manifest shape unchanged', () => { + const { result, entry } = recordAgent(); + assert.equal(result.ok, true); + assert.deepEqual( + Object.keys(entry).sort(), + ['agent_id', 'branch', 'expected_base', 'worktree_path'], + ); + }); + + test('an empty --files writes no field', () => { + const { result, entry } = recordAgent(['--files', '']); + assert.equal(result.ok, true); + assert.equal(has(entry, 'files_modified'), false); + }); + + test('a whitespace-only --files writes no field', () => { + const { result, entry } = recordAgent(['--files', ' \t ']); + assert.equal(result.ok, true); + assert.equal(has(entry, 'files_modified'), false); + }); + + test('--files splits on any whitespace run', () => { + const { entry } = recordAgent(['--files', 'a.ts b.ts\tc.ts\nd.ts']); + assert.deepEqual(entry.files_modified, ['a.ts', 'b.ts', 'c.ts', 'd.ts']); + }); + + test('a trailing --files with no value is treated as absent', () => { + const { result, entry } = recordAgent(['--files']); + assert.equal(result.ok, true, 'a valueless flag must not crash the verb'); + assert.equal(has(entry, 'files_modified'), false); + }); + + test('a flag-shaped --files value is not re-parsed as a flag', () => { + const { entry } = recordAgent(['--files', '--branch']); + assert.deepEqual(entry.files_modified, ['--branch']); + assert.equal(entry.branch, 'worktree-agent-a1', 'the real --branch value must be untouched'); + }); + + test('shell metacharacters in --files are inert data', () => { + const hostile = 'a.ts; rm -rf / && $(whoami) `id` "q" \'p\''; + const { entry } = recordAgent(['--files', hostile]); + assert.deepEqual(entry.files_modified, [ + 'a.ts;', 'rm', '-rf', '/', '&&', '$(whoami)', '`id`', '"q"', "'p'", + ]); + }); + + test('a traversal-shaped --files value is inert data, never dereferenced', () => { + const readPaths = []; + const result = withExitCode2596(() => cmdWorktreeRecordAgent('/repo/main', [...baseArgs, '--files', '../../etc/passwd'], { + readFile: (p) => { readPaths.push(p); return '{"orchestrator_root":"/repo/main","worktrees":[]}'; }, + writeFile: () => {}, + write: () => {}, + writeErr: () => {}, + })); + assert.equal(result.ok, true); + assert.deepEqual( + readPaths, [path.resolve('/repo/main', 'manifest.json')], + 'the only path opened is the manifest — a declared-scope value is compared, never read', + ); + }); + + test('a duplicate --files takes the first occurrence, like every other flag', () => { + const { entry } = recordAgent(['--files', 'a.ts', '--files', 'b.ts']); + assert.deepEqual(entry.files_modified, ['a.ts']); + }); + + test('worktree create records the declared scope too', () => { + let written = null; + const result = withExitCode2596(() => cmdWorktreeCreate('/repo/main', [ + ...baseArgs, '--root', '/repo', '--files', 'src/a.ts', + ], { + readFile: () => '{"orchestrator_root":"/repo/main","worktrees":[]}', + writeFile: (_p, c) => { written = c; }, + write: () => {}, + writeErr: () => {}, + execGit: () => ({ exitCode: 0, stdout: '', stderr: '', timedOut: false }), + })); + assert.equal(result.ok, true); + assert.deepEqual(JSON.parse(written).worktrees[0].files_modified, ['src/a.ts']); + }); + + // #2596 parity (CLAUDE.md → Generative Fix Divergence): `record-agent` and + // `create` each decide independently whether to write `files_modified`. + // Assert the two surfaces agree on every input class — including the blank + // cases where BOTH must omit the field — so a change to one that is not + // mirrored in the other fails here instead of shipping a backend-dependent + // advisory. + test('parity: record-agent and create agree on files_modified for every --files input', () => { + const createEntry = (extraArgs) => { + let written = null; + withExitCode2596(() => cmdWorktreeCreate('/repo/main', [...baseArgs, '--root', '/repo', ...extraArgs], { + readFile: () => '{"orchestrator_root":"/repo/main","worktrees":[]}', + writeFile: (_p, c) => { written = c; }, + write: () => {}, + writeErr: () => {}, + execGit: () => ({ exitCode: 0, stdout: '', stderr: '', timedOut: false }), + })); + return written ? JSON.parse(written).worktrees[0] : null; + }; + + for (const extraArgs of [ + ['--files', 'src/a.ts src/b.ts'], + ['--files', 'src/a.ts'], + ['--files', ''], + ['--files', ' \t '], + ['--files'], + [], + ]) { + const fromRecord = recordAgent(extraArgs).entry; + const fromCreate = createEntry(extraArgs); + assert.equal( + has(fromRecord, 'files_modified'), has(fromCreate, 'files_modified'), + `record-agent and create disagree on WHETHER to write files_modified for ${JSON.stringify(extraArgs)}`, + ); + assert.deepEqual( + fromRecord.files_modified, fromCreate.files_modified, + `record-agent and create disagree on the files_modified VALUE for ${JSON.stringify(extraArgs)}`, + ); + } + }); +});