diff --git a/.changeset/sturdy-jays-roam.md b/.changeset/sturdy-jays-roam.md new file mode 100644 index 000000000..3a1a20bbd --- /dev/null +++ b/.changeset/sturdy-jays-roam.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 1448 +--- +Added a validated `gsd-tools worktree record-agent` writer verb that appends a per-agent entry to the wave cleanup manifest, validating every field at write time with the same rules the `cleanup-wave` reader enforces (write-strict `--agent-id`) and failing loudly with a recovery hint instead of silently appending an under-populated entry. The execute-phase orchestrator now records each spawned worktree through this verb. (#1448) diff --git a/CONTEXT.md b/CONTEXT.md index aaded44ec..226ea9a41 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -101,7 +101,7 @@ Cross-seam principle (ADR-1411, epic #1411): context resolution — config loadi Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P3, #1416). Config-interpreting read verbs expose `Resolution { value, configured, reason, warnings }` (`src/resolution.cts`); agent-skills is the first adopter, where `value = { block, skills_count }` and `source`/`degraded` remain config-provenance extras outside the envelope. Other read verbs expose at least `warnings[]` (e.g. capability-state `{ runtimeConfigDir, capabilities, warnings? }`) without `configured`/`reason`, which are meaningful only for config-interpreting verbs. Mutation verbs expose `warnings[]` (advisory) PLUS `errors[]` (operation-not-applied), e.g. capability-writer `{ capabilities, warnings, errors }`. The shared seam across all shapes is `warnings: string[]`; a single generic `Resolution` across read+write verbs was rejected by the deletion test (`configured`/`reason` are meaningless for capability verbs; `errors[]` cannot fold into `warnings[]`) — ADR-1411 P3 amendment. Recurrence prevention is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`), not by a shared envelope. A CI guard (`scripts/lint-resolution-provenance.cjs`, wired into `lint:ci`) enforces that every registered config-interpreting read verb keeps a `configured_empty`/`not_configured` contract test; the registry in that script is the registration point for future verbs (ADR-1411 P4 / #1417). ### 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`. 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)` (a projection over `resolveWorktreeContext`) 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`, `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). 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)` (a projection over `resolveWorktreeContext`) 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); 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`. @@ -424,7 +424,7 @@ A legal deferred state of an Execute step (`external_job_waiting`): the executor `WORKTREE.SEAM.current=Worktree Safety Policy Module` `WORKTREE.SEAM.files=[gsd-core/bin/lib/worktree-safety.cjs]` -`WORKTREE.SEAM.interface=[resolveWorktreeContext, parseWorktreePorcelain, planWorktreePrune, executeWorktreePrunePlan]` +`WORKTREE.SEAM.interface=[resolveWorktreeContext, parseWorktreePorcelain, planWorktreePrune, executeWorktreePrunePlan, planWorktreeRecordAgent, cmdWorktreeRecordAgent]` `WORKTREE.SEAM.default-prune-policy=metadata_prune_only (non-destructive)` `WORKTREE.SEAM.decision-1=retain non-destructive default; destructive path only as explicit future opt-in scaffold` diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 9ed4d6ea6..1e09f6314 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -546,6 +546,20 @@ node gsd-tools.cjs worktree set-baseref **`worktree set-baseref`** applies a no-clobber write of `worktree.baseRef:"head"` to `.claude/settings.local.json`. If the file already contains an explicit `baseRef` value other than `"head"`, the existing value is preserved and `skipped:"explicit-other"` is returned. Malformed JSON causes an error rather than a silent overwrite. Both fresh installs and upgrades of GSD Core run this automatically when `workflow.use_worktrees` is enabled (the default); the command is also available for manual use — for example, to apply the setting when worktrees were toggled on after installation, or to re-apply it after a settings change. +### Wave-manifest recording + +The execute-phase orchestrator records each spawned executor's worktree identity into a wave cleanup manifest so the matching `cleanup-wave` reader can later merge and remove exactly those worktrees. + +```bash +# Append a validated per-agent entry to the wave cleanup manifest. +# 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 +``` + +**`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, 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. + --- ## Graphify diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index fe460ac67..6475d4a58 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -2128,6 +2128,8 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand const worktreeSafety = require('./lib/worktree-safety.cjs'); if (subcommand === 'cleanup-wave') { worktreeSafety.cmdWorktreeCleanupWave(cwd, args.slice(2)); + } else if (subcommand === 'record-agent') { + worktreeSafety.cmdWorktreeRecordAgent(cwd, args.slice(2)); } else if (subcommand === 'reap-orphans') { worktreeSafety.cmdWorktreeReapOrphans(cwd); } else if (subcommand === 'base-check') { @@ -2135,7 +2137,7 @@ async function runCommand(command, args, cwd, raw, defaultValue, originalCommand } else if (subcommand === 'set-baseref') { require('./lib/worktree-base-ref.cjs').cmdWorktreeSetBaseRef(cwd, args.slice(2)); } else { - error('Unknown worktree subcommand. Available: cleanup-wave, reap-orphans, base-check, set-baseref', ERROR_REASON.SDK_UNKNOWN_COMMAND); + error('Unknown worktree subcommand. Available: cleanup-wave, record-agent, reap-orphans, base-check, set-baseref', ERROR_REASON.SDK_UNKNOWN_COMMAND); } break; } diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index 6db8d6a84..99416e5de 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -687,7 +687,7 @@ increases monotonically across waves. `{status}` is `complete` (success), ) ``` - After each `Agent()` returns, parse executor-returned worktree metadata (``) before harness metadata, then atomically append `{agent_id, worktree_path, branch, expected_base}` to `WAVE_WORKTREE_MANIFEST`. Missing: 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 …`. 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. > **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/src/worktree-safety.cts b/src/worktree-safety.cts index 892166789..5212d8ceb 100644 --- a/src/worktree-safety.cts +++ b/src/worktree-safety.cts @@ -868,6 +868,241 @@ function cmdWorktreeCleanupWave(cwd: string, args: string[] = []): void { } } +interface RecordAgentFields { + agentId: string; + worktreePath: string; + branch: string; + base: string; +} + +interface RecordAgentPlan { + ok: boolean; + reason: string; + hint?: string; + entry: CleanupManifestEntry | null; + /** Serialized manifest to write back (with trailing newline); null when ok === false. */ + manifest: string | null; +} + +/** + * Pure planner for the per-agent wave-manifest append. + * + * Validates the candidate entry at write time using the SAME rules the + * cleanup-wave reader enforces (via `normalizeCleanupManifestEntry`), so an + * entry that `record-agent` accepts is guaranteed to survive + * `normalizeCleanupManifest` on read — a field that would be silently dropped + * at cleanup time fails loudly here instead. + * + * `agent_id` is treated write-strict (required) even though the reader is + * lenient (nullable): the whole point of this verb is to catch an + * under-populated entry at write time, and an entry whose author cannot be + * identified defeats that. A duplicate `(worktree_path, branch)` is also + * 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`. + */ +function planWorktreeRecordAgent(manifestRaw: string, fields: RecordAgentFields): RecordAgentPlan { + // 1. Write-strict required-field check (loud, with which flag is missing). + // Trim first so a whitespace-only value (" ") is rejected here rather + // than deferred to a guaranteed `git worktree remove` failure at cleanup. + const agentId = (fields.agentId || '').trim(); + const worktreePath = (fields.worktreePath || '').trim(); + const branch = (fields.branch || '').trim(); + const base = (fields.base || '').trim(); + const missing: string[] = []; + if (!agentId) missing.push('--agent-id'); + if (!worktreePath) missing.push('--path'); + if (!branch) missing.push('--branch'); + if (!base) missing.push('--base'); + if (missing.length > 0) { + return { + ok: false, + reason: 'missing_field', + hint: `record-agent requires ${missing.join(', ')}. Re-run with all of --agent-id, --path, --branch, --base set to non-empty (non-whitespace) values.`, + entry: null, + manifest: null, + }; + } + + // 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 candidate = { + agent_id: agentId, + worktree_path: worktreePath, + branch, + expected_base: base, + }; + const entry = normalizeCleanupManifestEntry(candidate); + if (!entry) { + return { + ok: false, + reason: 'invalid_entry', + hint: `Entry failed cleanup-manifest validation: --path/--branch/--base must be non-empty and --branch must match ^worktree-agent-[A-Za-z0-9._/-]+$ (got branch="${branch}"). Fix the field and re-run.`, + entry: null, + manifest: null, + }; + } + + // 3. Parse the existing manifest. The init shell ({orchestrator_root, worktrees: []}) + // is written inline by the orchestrator before any agent spawns; a missing or + // malformed manifest is a loud failure here, not a silent under-populated write. + let parsed: unknown; + try { + parsed = JSON.parse(manifestRaw); + } catch { + return { + ok: false, + reason: 'invalid_manifest_json', + hint: 'Manifest is not valid JSON. The orchestrator must initialize it as {"orchestrator_root": "...", "worktrees": []} before recording agents.', + entry: null, + manifest: null, + }; + } + + // Accept the canonical {worktrees: []} shell or a bare top-level array (both + // are read by normalizeCleanupManifest); preserve any other top-level keys. + let worktrees: unknown[]; + let writeBack: unknown; + if (Array.isArray(parsed)) { + worktrees = parsed; + writeBack = worktrees; + } else if (parsed && typeof parsed === 'object') { + const container = parsed as Record; + if (container.worktrees === undefined) container.worktrees = []; + if (!Array.isArray(container.worktrees)) { + return { + ok: false, + reason: 'manifest_shape_invalid', + hint: 'Manifest "worktrees" must be an array. Re-initialize as {"orchestrator_root": "...", "worktrees": []}.', + entry: null, + manifest: null, + }; + } + worktrees = container.worktrees; + writeBack = container; + } else { + return { + ok: false, + reason: 'manifest_shape_invalid', + hint: 'Manifest must be a JSON object {"worktrees": []} or a top-level array.', + entry: null, + manifest: null, + }; + } + + // 4. Reject a duplicate (worktree_path, branch). The reader dedups on this + // exact key, but only over entries that NORMALIZE successfully — so an + // existing malformed same-key entry (which the reader would drop) must NOT + // block recording a valid one. Run each existing entry through the reader's + // own normalizer and compare only the entries the reader would keep; this + // matches its dedup behavior exactly. A real duplicate signals an upstream + // double-spawn — surface it loudly instead of silently dropping it. + const dupKey = `${entry.worktree_path}\0${entry.branch}`; + const isDuplicate = worktrees.some((existing) => { + const normalized = normalizeCleanupManifestEntry(existing); + return normalized !== null && `${normalized.worktree_path}\0${normalized.branch}` === dupKey; + }); + if (isDuplicate) { + return { + ok: false, + reason: 'duplicate_entry', + hint: `The manifest already records worktree_path="${entry.worktree_path}" branch="${entry.branch}". The cleanup reader dedups on (worktree_path, branch), so re-recording would be silently dropped — this usually signals an upstream double-spawn. Investigate rather than re-record.`, + entry: null, + manifest: null, + }; + } + + // 5. Append the minimal 4-field entry, matching the existing on-disk format. + const recorded: CleanupManifestEntry = { + agent_id: entry.agent_id, + worktree_path: entry.worktree_path, + branch: entry.branch, + expected_base: entry.expected_base, + }; + worktrees.push(recorded); + + return { + ok: true, + reason: 'ok', + entry: recorded, + manifest: `${JSON.stringify(writeBack, null, 2)}\n`, + }; +} + +interface RecordAgentCmdDeps { + readFile?: (p: string) => string; + writeFile?: (p: string, content: string) => void; + write?: (s: string) => void; + writeErr?: (s: string) => void; +} + +interface RecordAgentCmdResult { + ok: boolean; + reason: string; + hint?: string; + entry: CleanupManifestEntry | null; + manifest_path?: string; +} + +/** + * CLI command: append a validated per-agent entry to a wave cleanup manifest. + * + * Usage: worktree record-agent --manifest --agent-id --path --branch --base + * + * 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 + * under-populated entry that the cleanup reader would silently drop. + */ +function cmdWorktreeRecordAgent(cwd: string, args: string[] = [], deps: RecordAgentCmdDeps = {}): RecordAgentCmdResult { + const flag = (name: string): string => { + const i = args.indexOf(name); + return i >= 0 && i + 1 < args.length ? args[i + 1] : ''; + }; + const write = deps.write || ((s: string) => process.stdout.write(s)); + const writeErr = deps.writeErr || ((s: string) => process.stderr.write(s)); + + const manifestPath = flag('--manifest'); + if (!manifestPath) { + writeErr('Usage: worktree record-agent --manifest --agent-id --path --branch --base \n'); + process.exitCode = 2; + return { ok: false, reason: 'usage', entry: null }; + } + + const resolved = path.resolve(cwd, manifestPath); + const readFile = deps.readFile || ((p: string) => fs.readFileSync(p, 'utf8')); + let manifestRaw: string; + try { + manifestRaw = readFile(resolved); + } catch (err) { + const hint = `Manifest not found or unreadable at ${manifestPath}. The orchestrator must initialize it ({"orchestrator_root": "...", "worktrees": []}) before recording agents.`; + writeErr(`[gsd] worktree.record-agent: manifest_read_failed — ${hint}\n`); + write(`${JSON.stringify({ ok: false, reason: 'manifest_read_failed', hint, error: (err as Error).message }, null, 2)}\n`); + process.exitCode = 1; + return { ok: false, reason: 'manifest_read_failed', hint, entry: null }; + } + + const plan = planWorktreeRecordAgent(manifestRaw, { + agentId: flag('--agent-id'), + worktreePath: flag('--path'), + branch: flag('--branch'), + base: flag('--base'), + }); + + if (!plan.ok || plan.manifest === null) { + writeErr(`[gsd] worktree.record-agent: ${plan.reason} — ${plan.hint || ''}\n`); + write(`${JSON.stringify({ ok: false, reason: plan.reason, hint: plan.hint }, null, 2)}\n`); + process.exitCode = 1; + return { ok: false, reason: plan.reason, hint: plan.hint, entry: null }; + } + + const writeFile = deps.writeFile || ((p: string, content: string) => fs.writeFileSync(p, content, 'utf8')); + writeFile(resolved, plan.manifest); + write(`${JSON.stringify({ ok: true, reason: 'ok', entry: plan.entry, manifest_path: resolved }, null, 2)}\n`); + return { ok: true, reason: 'ok', entry: plan.entry, manifest_path: resolved }; +} + /** * Reap orphaned linked worktrees whose lock owner process is dead, whose * branch tip is fully merged into the default branch, and whose lock file @@ -1167,6 +1402,8 @@ export = { planWorktreeWaveCleanup, executeWorktreeWaveCleanupPlan, cmdWorktreeCleanupWave, + planWorktreeRecordAgent, + cmdWorktreeRecordAgent, reapOrphanWorktrees, cmdWorktreeReapOrphans, resolveWorktreeRoot, diff --git a/tests/phase6-capstone-conformance.test.cjs b/tests/phase6-capstone-conformance.test.cjs index 377e663d4..4f3f580d1 100644 --- a/tests/phase6-capstone-conformance.test.cjs +++ b/tests/phase6-capstone-conformance.test.cjs @@ -193,8 +193,15 @@ describe('ADR-857 Phase 6 capstone conformance (#1139)', () => { // extract to capabilities. Frozen pre-phase-6 sizes (LF bytes); the files must // drop strictly below these. This also defeats double-run gaming — declaring a // hook while leaving the inline block keeps the file from shrinking -> red. + // + // #1298: the execute-phase.md ceiling was raised from 93166 to accommodate + // wiring the mandatory `worktree record-agent` writer verb into the per-agent + // wave-manifest append. That verb is privileged host machinery (ADR-857 + // Decision #1) — NOT the optional-feature inline logic this budget ratchets + // toward capabilities — so its footprint legitimately raises the host-loop + // ceiling rather than signalling an un-extracted optional feature. const { lfByteCount } = require('../scripts/workflow-size.cjs'); - const PRE_PHASE6 = { 'plan-phase.md': 94519, 'execute-phase.md': 93166 }; + const PRE_PHASE6 = { 'plan-phase.md': 94519, 'execute-phase.md': 93600 }; const notShrunk = []; for (const [file, frozen] of Object.entries(PRE_PHASE6)) { const now = lfByteCount(path.join(ROOT, 'gsd-core', 'workflows', file)); diff --git a/tests/workflow-size-baseline.json b/tests/workflow-size-baseline.json index 756e2c9f3..8cf4b894c 100644 --- a/tests/workflow-size-baseline.json +++ b/tests/workflow-size-baseline.json @@ -24,7 +24,7 @@ "docs-update.md": 55662, "edit-phase.md": 12883, "eval-review.md": 9923, - "execute-phase.md": 93024, + "execute-phase.md": 93426, "execute-plan.md": 31365, "explore.md": 10497, "extract-learnings.md": 12849, diff --git a/tests/worktree-cleanup.test.cjs b/tests/worktree-cleanup.test.cjs index 19f45cb53..467540e41 100644 --- a/tests/worktree-cleanup.test.cjs +++ b/tests/worktree-cleanup.test.cjs @@ -677,7 +677,9 @@ describe('bug #3384: worktree cleanup workflow contracts', () => { const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf8'); assert.match(content, /WAVE_WORKTREE_MANIFEST/); assert.match(content, /worktree\.cleanup-wave/); - assert.match(content, /atomically append `\{agent_id, worktree_path, branch, expected_base\}`/); + // #1298: the per-agent manifest write now goes through the validated + // `worktree record-agent` writer verb (was a prose "atomically append"). + assert.match(content, /record the `\{agent_id, worktree_path, branch, expected_base\}` entry with `gsd_run query worktree\.record-agent/); assert.match(content, /try\{if\(!p\)throw new Error\("WAVE_WORKTREE_MANIFEST is unset"\)/); assert.match(content, /WT_PATHS_FILE=.*gsd-worktree-paths-/); assert.doesNotMatch(content, /done < <\(node -e 'const fs=require\("fs"\);const p=process\.env\.WAVE_WORKTREE_MANIFEST/); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index 3eb7044a4..c5020e4bd 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -18,6 +18,7 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const path = require('node:path'); +const fc = require('fast-check'); const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); const WORKTREE_SAFETY_PATH = path.join( @@ -37,6 +38,8 @@ const { snapshotWorktreeInventory, planWorktreeWaveCleanup, executeWorktreeWaveCleanupPlan, + planWorktreeRecordAgent, + cmdWorktreeRecordAgent, } = require(WORKTREE_SAFETY_PATH); const isWindows = process.platform === 'win32'; @@ -562,6 +565,382 @@ describe('planWorktreeWaveCleanup', () => { }); }); +// ─── planWorktreeRecordAgent (#1298 writer verb) ────────────────────────────── +// These tests pin the verb's reason for existing: a per-agent entry that +// record-agent ACCEPTS must survive the cleanup-wave reader, and one it REJECTS +// is exactly what the reader would have dropped silently. If write- and +// read-side validation ever diverge, the round-trip tests below fail. + +describe('planWorktreeRecordAgent', () => { + const VALID = { + agentId: 'a1', + worktreePath: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + base: 'abc123', + }; + + test('appends a validated entry that the cleanup-wave reader accepts (write/read parity)', () => { + const plan = planWorktreeRecordAgent('{"orchestrator_root":"/repo/main","worktrees":[]}', VALID); + assert.equal(plan.ok, true); + assert.deepEqual(plan.entry, { + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }); + // The serialized manifest must round-trip through the reader the cleanup + // path uses — proving write and read validate identically. + const written = JSON.parse(plan.manifest); + assert.equal(written.orchestrator_root, '/repo/main'); // preserved, no schema change + const readBack = planWorktreeWaveCleanup('/repo/main', written); + assert.equal(readBack.ok, true); + assert.equal(readBack.entries.length, 1); + assert.equal(readBack.entries[0].agent_id, 'a1'); + }); + + test('preserves existing entries and other top-level keys when appending', () => { + const existing = JSON.stringify({ + orchestrator_root: '/repo/main', + worktrees: [{ + agent_id: 'a0', + worktree_path: '/repo/.claude/worktrees/agent-a0', + branch: 'worktree-agent-a0', + expected_base: 'aaa000', + }], + }); + const plan = planWorktreeRecordAgent(existing, VALID); + assert.equal(plan.ok, true); + const written = JSON.parse(plan.manifest); + assert.equal(written.orchestrator_root, '/repo/main'); + assert.equal(written.worktrees.length, 2); + assert.deepEqual(written.worktrees.map((w) => w.agent_id), ['a0', 'a1']); + }); + + test('accepts a bare top-level array manifest', () => { + const plan = planWorktreeRecordAgent('[]', VALID); + assert.equal(plan.ok, true); + const written = JSON.parse(plan.manifest); + assert.ok(Array.isArray(written)); + assert.equal(written.length, 1); + assert.equal(written[0].branch, 'worktree-agent-a1'); + }); + + // Write-strict agent_id: the reader treats agent_id as nullable, but the + // writer requires it — an entry whose author cannot be identified defeats the + // verb's purpose. This is the deliberate write-strict-vs-read-lenient decision. + test('fails loudly when --agent-id is empty (write-strict, unlike the lenient reader)', () => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', { ...VALID, agentId: '' }); + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'missing_field'); + assert.match(plan.hint, /--agent-id/); + assert.equal(plan.manifest, null); + }); + + test('reports every missing field, not just the first', () => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', { + agentId: '', worktreePath: '', branch: '', base: '', + }); + assert.equal(plan.reason, 'missing_field'); + for (const flag of ['--agent-id', '--path', '--branch', '--base']) { + assert.match(plan.hint, new RegExp(flag.replace(/[-]/g, '\\$&'))); + } + }); + + // Branch-regex consistency caveat: a branch outside the disposable namespace + // is what the reader drops silently — record-agent must reject it at write time. + test('rejects a branch outside the worktree-agent-* namespace (the entry the reader would drop)', () => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', { ...VALID, branch: 'feature/user-work' }); + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'invalid_entry'); + assert.match(plan.hint, /worktree-agent-/); + assert.equal(plan.manifest, null); + // Confirm the rejected entry is genuinely one the reader drops. + const readBack = planWorktreeWaveCleanup('/repo/main', { + worktrees: [{ agent_id: 'a1', worktree_path: VALID.worktreePath, branch: 'feature/user-work', expected_base: 'abc123' }], + }); + assert.equal(readBack.ok, false); + assert.equal(readBack.reason, 'empty_manifest'); + }); + + test('fails loudly on malformed manifest JSON instead of clobbering it', () => { + const plan = planWorktreeRecordAgent('{not valid json', VALID); + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'invalid_manifest_json'); + assert.equal(plan.manifest, null); + }); + + test('rejects a manifest whose worktrees field is not an array', () => { + const plan = planWorktreeRecordAgent('{"worktrees":{}}', VALID); + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'manifest_shape_invalid'); + assert.equal(plan.manifest, null); + }); + + // The reader dedups on (worktree_path, branch); a re-record would be silently + // dropped at cleanup — exactly the failure mode the verb exists to eliminate — + // so the writer must reject it loudly rather than swallow it. + test('rejects a duplicate (worktree_path, branch) loudly instead of writing a droppable entry', () => { + const existing = JSON.stringify({ + worktrees: [{ + agent_id: 'a1', + worktree_path: '/repo/.claude/worktrees/agent-a1', + branch: 'worktree-agent-a1', + expected_base: 'abc123', + }], + }); + // Same path+branch, different agent_id/base — still a duplicate by the reader's key. + const plan = planWorktreeRecordAgent(existing, { ...VALID, agentId: 'a1-retry', base: 'deadbee' }); + assert.equal(plan.ok, false); + assert.equal(plan.reason, 'duplicate_entry'); + assert.match(plan.hint, /worktree-agent-a1/); + assert.equal(plan.manifest, null); + }); + + test('detects a duplicate stored under the legacy `path` field too', () => { + const existing = JSON.stringify({ + worktrees: [{ path: '/repo/.claude/worktrees/agent-a1', branch: 'worktree-agent-a1', expected_base: 'abc123' }], + }); + const plan = planWorktreeRecordAgent(existing, VALID); + assert.equal(plan.reason, 'duplicate_entry'); + }); + + // Reader-alignment: the cleanup reader dedups only over entries that normalize + // successfully, so a malformed same-key entry it would DROP must not block a + // valid recording — otherwise the writer is stricter than the reader and + // blocks legitimate recovery. + test('a malformed same-key existing entry does not block recording a valid one', () => { + const existing = JSON.stringify({ + // Same path+branch as VALID but no expected_base — the reader drops this. + worktrees: [{ worktree_path: '/repo/.claude/worktrees/agent-a1', branch: 'worktree-agent-a1' }], + }); + const plan = planWorktreeRecordAgent(existing, VALID); + assert.equal(plan.ok, true); + const readBack = planWorktreeWaveCleanup('/repo/main', JSON.parse(plan.manifest)); + assert.equal(readBack.ok, true); + assert.equal(readBack.entries.length, 1); // reader keeps only the valid one + assert.equal(readBack.entries[0].expected_base, 'abc123'); + }); + + test('rejects whitespace-only --path/--base (values are trimmed)', () => { + const wsPath = planWorktreeRecordAgent('{"worktrees":[]}', { ...VALID, worktreePath: ' ' }); + assert.equal(wsPath.reason, 'missing_field'); + assert.match(wsPath.hint, /--path/); + const wsBase = planWorktreeRecordAgent('{"worktrees":[]}', { ...VALID, base: ' \t ' }); + assert.equal(wsBase.reason, 'missing_field'); + assert.match(wsBase.hint, /--base/); + }); + + test('trims incidental surrounding whitespace on accepted values', () => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', { + agentId: ' a1 ', worktreePath: ' /repo/wt-a1 ', branch: ' worktree-agent-a1 ', base: ' abc123 ', + }); + assert.equal(plan.ok, true); + assert.deepEqual(plan.entry, { + agent_id: 'a1', worktree_path: '/repo/wt-a1', branch: 'worktree-agent-a1', expected_base: 'abc123', + }); + }); +}); + +// ─── planWorktreeRecordAgent — property-based write/read parity (#1298) ──────── +// The verb's reason for existing is the write→read parity invariant, so it must +// carry a fast-check property test (RULESET.TESTS.property-based-testing): an +// entry the writer ACCEPTS must survive the cleanup reader unchanged, and an +// entry with an invalid branch must be REJECTED symmetrically. + +describe('planWorktreeRecordAgent — fast-check parity invariant (#1298)', () => { + const seg = fc.stringMatching(/^[A-Za-z0-9._/-]+$/); // include '/' — the namespace allows it + const agentBranch = seg.map((s) => `worktree-agent-${s}`); + const nonEmpty = fc.stringMatching(/^\S[\S ]*$/); // no leading whitespace, not blank + + test('any writer-accepted entry round-trips through the cleanup reader unchanged', () => { + fc.assert(fc.property( + fc.record({ agentId: nonEmpty, worktreePath: nonEmpty, branch: agentBranch, base: nonEmpty }), + (fields) => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', fields); + if (!plan.ok) return; // rejection is fine; this property is about accepted entries + const readBack = planWorktreeWaveCleanup('/repo/main', JSON.parse(plan.manifest)); + assert.equal(readBack.ok, true); + assert.equal(readBack.entries.length, 1); + const e = readBack.entries[0]; + assert.equal(e.worktree_path, fields.worktreePath.trim()); + assert.equal(e.branch, fields.branch.trim()); + assert.equal(e.expected_base, fields.base.trim()); + assert.equal(e.agent_id, fields.agentId.trim()); + }, + )); + }); + + test('an entry with a branch outside the worktree-agent-* namespace is always rejected', () => { + fc.assert(fc.property( + fc.record({ + agentId: nonEmpty, + worktreePath: nonEmpty, + // Any branch that does NOT match the disposable namespace. + branch: fc.string({ minLength: 1 }).filter((b) => !/^worktree-agent-[A-Za-z0-9._/-]+$/.test(b.trim())), + base: nonEmpty, + }), + (fields) => { + const plan = planWorktreeRecordAgent('{"worktrees":[]}', fields); + assert.equal(plan.ok, false); + assert.equal(plan.manifest, null); + }, + )); + }); +}); + +// ─── cmdWorktreeRecordAgent (#1298 CLI wrapper) ─────────────────────────────── + +describe('cmdWorktreeRecordAgent', () => { + // process.exitCode is global; each failure-path test resets it so a failing + // exit code does not leak into the test runner's own exit status. + function withExitCode(fn) { + const saved = process.exitCode; + try { return fn(); } finally { process.exitCode = saved; } + } + + const okArgs = [ + '--manifest', 'manifest.json', + '--agent-id', 'a1', + '--path', '/repo/.claude/worktrees/agent-a1', + '--branch', 'worktree-agent-a1', + '--base', 'abc123', + ]; + + test('writes the manifest and reports ok on the happy path', () => { + let writtenPath = null; + let writtenContent = null; + const out = []; + const result = cmdWorktreeRecordAgent('/repo/main', okArgs, { + readFile: () => '{"orchestrator_root":"/repo/main","worktrees":[]}', + writeFile: (p, c) => { writtenPath = p; writtenContent = c; }, + write: (s) => out.push(s), + writeErr: () => {}, + }); + assert.equal(result.ok, true); + assert.equal(writtenPath, path.resolve('/repo/main', 'manifest.json')); + const written = JSON.parse(writtenContent); + assert.equal(written.worktrees.length, 1); + assert.equal(written.worktrees[0].agent_id, 'a1'); + assert.match(out.join(''), /"ok": true/); + }); + + test('exits 2 with usage when --manifest is missing', () => { + withExitCode(() => { + const errs = []; + const result = cmdWorktreeRecordAgent('/repo/main', ['--agent-id', 'a1'], { + writeErr: (s) => errs.push(s), + write: () => {}, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, 'usage'); + assert.equal(process.exitCode, 2); + assert.match(errs.join(''), /Usage: worktree record-agent/); + }); + }); + + test('exits 1 loudly when the manifest cannot be read', () => { + withExitCode(() => { + const errs = []; + const result = cmdWorktreeRecordAgent('/repo/main', okArgs, { + readFile: () => { throw new Error('ENOENT'); }, + writeErr: (s) => errs.push(s), + write: () => {}, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, 'manifest_read_failed'); + assert.equal(process.exitCode, 1); + assert.match(errs.join(''), /manifest_read_failed/); + }); + }); + + test('does not write the manifest when the entry is invalid', () => { + withExitCode(() => { + let wrote = false; + const errs = []; + const result = cmdWorktreeRecordAgent('/repo/main', + ['--manifest', 'm.json', '--agent-id', 'a1', '--path', '/p', '--branch', 'feature/x', '--base', 'abc123'], { + readFile: () => '{"worktrees":[]}', + writeFile: () => { wrote = true; }, + writeErr: (s) => errs.push(s), + write: () => {}, + }); + assert.equal(result.ok, false); + assert.equal(result.reason, 'invalid_entry'); + assert.equal(wrote, false); // must NOT append an under-populated entry + assert.equal(process.exitCode, 1); + assert.match(errs.join(''), /worktree-agent-/); + }); + }); +}); + +// ─── record-agent: real CLI dispatch + workflow wiring (#1298 integration) ──── +// The unit tests above inject IO; these pin the live `gsd-tools.cjs query +// worktree.record-agent` dispatch and the execute-phase.md call site, so a +// future typo in the dotted command or the workflow wiring fails loudly. + +describe('worktree record-agent — real CLI dispatch (#1298)', () => { + const fs = require('node:fs'); + const { execFileSync } = require('node:child_process'); + const GSD_TOOLS = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); + + test('the dotted `query worktree.record-agent` path writes an entry the cleanup reader accepts', () => { + const dir = createTempDir(); + try { + const manifest = path.join(dir, 'wave-manifest.json'); + fs.writeFileSync(manifest, `${JSON.stringify({ orchestrator_root: dir, worktrees: [] })}\n`); + const out = execFileSync(process.execPath, [ + GSD_TOOLS, 'query', 'worktree.record-agent', + '--manifest', manifest, + '--agent-id', 'a1', + '--path', path.join(dir, 'wt-a1'), + '--branch', 'worktree-agent-a1', + '--base', 'abc123', + ], { encoding: 'utf8' }); + assert.match(out, /"ok": true/); + const written = JSON.parse(fs.readFileSync(manifest, 'utf8')); + assert.equal(written.worktrees.length, 1); + assert.equal(written.worktrees[0].agent_id, 'a1'); + // What the live CLI wrote must read back through the cleanup reader. + const readBack = planWorktreeWaveCleanup(dir, written); + assert.equal(readBack.ok, true); + assert.equal(readBack.entries[0].branch, 'worktree-agent-a1'); + } finally { + cleanup(dir); + } + }); + + test('a missing field fails loudly via the real CLI (non-zero exit, manifest untouched)', () => { + const dir = createTempDir(); + try { + const manifest = path.join(dir, 'wave-manifest.json'); + fs.writeFileSync(manifest, `${JSON.stringify({ worktrees: [] })}\n`); + let threw = false; + try { + execFileSync(process.execPath, [ + GSD_TOOLS, 'query', 'worktree.record-agent', + '--manifest', manifest, + '--path', path.join(dir, 'wt'), '--branch', 'worktree-agent-x', '--base', 'abc123', + ], { encoding: 'utf8', stdio: 'pipe' }); + } catch (err) { + threw = true; + assert.equal(err.status, 1); + assert.match(String(err.stderr), /record-agent: missing_field/); + } + assert.ok(threw, 'CLI must exit non-zero when --agent-id is missing'); + assert.deepEqual(JSON.parse(fs.readFileSync(manifest, 'utf8')).worktrees, []); + } finally { + cleanup(dir); + } + }); + + test('the execute-phase.md per-agent append calls the record-agent verb', () => { + const wf = fs.readFileSync( + path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'), 'utf8', + ); + assert.match(wf, /worktree\.record-agent/, 'execute-phase.md must wire the record-agent verb'); + }); +}); + // ─── executeWorktreeWaveCleanupPlan ─────────────────────────────────────────── describe('executeWorktreeWaveCleanupPlan', () => {