* 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...<branch> 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 <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/curious-tunas-greet.md
Normal file
5
.changeset/curious-tunas-greet.md
Normal file
@@ -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)
|
||||
@@ -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 `<normalized source>\0<reason>` — **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 <branch> <path> <base>`; 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 <branch> <path> <base>`; 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`.
|
||||
|
||||
@@ -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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir>
|
||||
--manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir> \
|
||||
[--files "<space-separated declared paths>"]
|
||||
```
|
||||
|
||||
**`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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha>
|
||||
--manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> \
|
||||
[--files "<space-separated declared paths>"]
|
||||
```
|
||||
|
||||
**`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-<id>` and legacy `worktree-agent-<id>`), 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-<id>` and legacy `worktree-agent-<id>`), 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...<branch>`) 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.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
96
docs/how-to/interpret-scope-conformance-warnings.md
Normal file
96
docs/how-to/interpret-scope-conformance-warnings.md
Normal file
@@ -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...<branch>`) 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)
|
||||
@@ -777,7 +777,7 @@ increases monotonically across waves. `{status}` is `complete` (success),
|
||||
)
|
||||
```
|
||||
|
||||
After each `Agent()` returns, parse executor-returned worktree metadata (`<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 (`<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.
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 <worktreePath>/.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<string | null> = [];
|
||||
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<string>();
|
||||
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 <quick_id>-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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha>
|
||||
* Usage: worktree record-agent --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> [--files "<space-separated paths>"]
|
||||
*
|
||||
* 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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha>\n');
|
||||
writeErr('Usage: worktree record-agent --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> [--files "<space-separated paths>"]\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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir>
|
||||
* Usage: worktree create --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir> [--files "<space-separated paths>"]
|
||||
*
|
||||
* #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 <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir>\n');
|
||||
writeErr('Usage: worktree create --manifest <path> --agent-id <id> --path <worktree> --branch <branch> --base <sha> --root <dir> [--files "<space-separated paths>"]\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,
|
||||
|
||||
@@ -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 <worktree>/.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)}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user