Files
msd-core/tests/workflow-size-budget.test.cjs
Tom Boucher ca2644a71a fix(worktree): unlock-retry on locked cleanup + startup orphan sweep (#3707) (#3719)
* fix(worktree): unlock-retry on locked cleanup + startup orphan sweep (#3707)

Two root causes fixed:

1. **In-session cleanup blocked**: `executeWorktreeWaveCleanupPlan` now attempts
   `git worktree unlock <path>` then retries `git worktree remove --force` when the
   initial single-force remove fails on a locked worktree. Previously every cleanup
   after a successful merge was silently blocked.

2. **Cross-session orphan accumulation**: new `reapOrphanWorktrees` helper sweeps
   `.git/worktrees/*/locked` at startup. It reaps entries where the pid is dead,
   the branch tip is an ancestor of the default branch (ancestry guard prevents data
   loss on squash-merge repos), and the lock mtime is older than 5 minutes (race
   guard). Wired into `quick.md` and `execute-phase.md` startup blocks guarded by
   `USE_WORKTREES != false`.

SDK: adds `worktree.reap-orphans` query command (routes through gsd-tools.cjs).
Tests: 11 real-fs tests covering unlock-retry, dead-pid reap, live-pid skip,
unmerged skip, fresh-mtime skip, idempotent double-call, and structural wiring.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore(changeset): add Fixed fragment for PR #3707 (worktree orphan cleanup)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(worktree): fix test portability on Windows + macOS for bug-3707 reap tests

- worktreeMeta helper: replace /\/\.git$/ with /[/\\]\.git$/ so the
  gitdir path suffix is stripped on both Windows (backslash) and Unix.
- worktreeMeta helper: normalize CRLF→LF before splitting porcelain
  blocks, fixing block parsing when git emits CRLF on Windows.
- reapOrphanWorktrees: replace single 'main' rev-parse with a
  [defaultBranch, 'main', 'master'] candidate loop so test fixtures
  without a remote origin (where branch may be 'master') don't bail
  early. Intentionally excludes 'HEAD' to prevent false reaping when
  HEAD is detached or on a feature branch (Codex adversarial finding).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(worktree): CI green — macOS symlink path, Windows test helper, pid portability, EPERM liveness

Four fixes to get macOS + Windows CI from red to green:

1. **macOS symlink mismatch** (worktree-safety.cjs): `reapOrphanWorktrees` now
   builds a canonical→listed path map from `git worktree list --porcelain` using
   `fs.realpathSync.native`. Uses the listed path (as git knows it) for
   `git worktree unlock/remove`, not the gitdir-derived path.  Fixes the
   `/var/folders` vs `/private/var/folders` discrepancy on GitHub macOS runners
   where `git worktree unlock <realpath>` was silently failing because git's
   list stored the unresolved symlink path.

2. **Windows path separator in test helper** (test file): `worktreeMeta`
   `.replace(/\/\.git$/, '')` → `.replace(/[/\\]\.git$/, '')`. On Windows,
   git writes backslash separators in the gitdir file; the Unix-only regex was
   causing `Cannot find .git/worktrees/<name>` for all Suite 2 tests.

3. **Non-portable PID in tests** (test file): All `'999999'` dead-PID literals
   replaced with `deadPid()` helper that spawns a real short-lived child, captures
   its PID, and returns it after exit. Eliminates flakiness on Linux systems where
   `pid_max` can reach 4194304, making 999999 a live PID.

4. **EPERM fail-closed in isPidAlive** (worktree-safety.cjs): `catch { return false }`
   → checks `err.code === 'EPERM'` and returns `true` (alive). On Windows and
   cross-user scenarios, `process.kill(pid, 0)` throws EPERM for live but
   inaccessible processes; treating that as dead would reap a live worktree.

Adversarial review via codex confirmed:
- Squash-merge repos: fail-closed (CONCERN, not BUG — by design, not data-loss)
- canonicalToListed map: SAFE (fail-closed on realpathSync error)
- Concurrent reapers: SAFE (both prune; second gets skipped: remove_failed)
- Startup blocking: CONCERN (no global cap, 10s/call × N worktrees) — tracked,
  not fixed here (requires separate perf work)
- gsd-sdk missing: SAFE (quick.md checks and fails fast with guidance)

All 27 local tests + Docker (holodeck) green.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(worktree): address codex adversarial findings — fail-closed default branch + CRLF map

Two fixes from codex adversarial review of PR 3718:

1. **Default branch resolution (data-loss risk)**: `reapOrphanWorktrees` now
   uses `refs/remotes/origin/<branch>` exclusively when a remote is configured.
   If `origin/HEAD` is absent but a remote exists, we bail out (fail-closed)
   rather than falling back to a local `main`/`master` that may not be the
   real integration branch.  The `main`/`master` fallback is only used when
   there is provably no remote (local-only test fixtures).

2. **CRLF normalization in canonical-path mapper**: The `worktree list
   --porcelain` output was split on '\n\n' without normalizing CRLF first.
   On Windows, git emits CRLF, which caused block-splitting to fail and
   left the canonicalToListed map only partially populated, weakening the
   symlink/path-mismatch fix introduced earlier.

3. **Windows 8.3 short-path fix (test helper)**: Both `beforeEach` blocks
   now call `resolvedTmpDir()` which pre-resolves `os.tmpdir()` via
   `fs.realpathSync.native` so temp paths avoid RUNNER~1-style short names
   that git stores in long form, causing worktreeMeta path comparisons to
   fail on Windows CI.

All 11 real-fs + 16 unit tests green locally.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(worktree): adversarial findings + macOS CI path-mismatch fix

## Root cause (macOS CI fail)
`reapOrphanWorktrees` stored `worktreePath` (gitdir-derived, real path via
git's symlink resolution, e.g. `/private/var/folders/…`) in results, while
the test's `wtDir` used the unresolved symlink form (`/var/folders/…`).  After
reaping, `canonicalPath(wtDir)` can no longer call `realpathSync.native`
(directory gone), so it falls back to `path.resolve` — which returns the
symlink form — causing the `result.find()` comparison to miss.

## Fixes applied

### Source — worktree-safety.cjs
1. **Finding 1 (fail-closed PID check)**: Non-parseable lock content (e.g.
   `"Locked by claude-code agent-xxx"`) is now treated as ALIVE with reason
   `lock_owner_unknown`, not as dead.  Previously it fell through as dead.
2. **Finding 1b (EPERM safe)**: `isPidAlive` call wrapped in try/catch; any
   thrown error (EPERM = process exists but cross-user on Windows) → ALIVE.
3. **Finding 2 (startup warning)**: `cmdWorktreeReapOrphans` now writes a
   one-line stderr warning when ≥1 entry is skipped or when reaper throws,
   while keeping exit-zero so workflows don't break.
4. **Finding 3 (default-branch discovery)**: Local-only fallback now tries
   `init.defaultBranch` config and HEAD symref before `main`/`master`, so
   repos configured with `trunk`, `dev`, etc. get correct orphan detection.
5. **macOS path fix**: Result entry for reaped worktrees now uses `gitKnownPath`
   (from `git worktree list`) instead of `worktreePath` (from gitdir file),
   ensuring the caller always sees the path git uses for the worktree.

### Test — bug-3707-locked-worktree-cleanup.test.cjs
6. **macOS CI fix**: Pre-compute `wtDirCanonical = canonicalPath(wtDir)` before
   calling `reapOrphanWorktrees` so the comparison works after removal.
7. **Gap 1**: New test — Claude Code lock format (`"Locked by claude-code …"`)
   must not be reaped; asserts `status=skipped, reason=lock_owner_unknown`.
8. **Gap 2**: New test — `isPidAlive` throwing EPERM → must not reap.
9. **Gap 3**: New test — repo with `init.defaultBranch=trunk`; merged worktree
   must be reaped (verifies trunk is discovered as the integration branch).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(test): raise waitForStoppedAt timeout 2 s → 5 s for Windows/Node22 CI load

Subprocess write latency exceeds 2 s on loaded windows-latest/Node22 runners
(test duration was 6181 ms); 5 s gives sufficient headroom without changing
any production behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-20 15:22:12 -04:00

327 lines
15 KiB
JavaScript
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
// allow-test-rule: pending-migration-to-typed-ir [#2974]
// Tracked in #2974 for migration to typed-IR assertions per CONTRIBUTING.md
// "Prohibited: Raw Text Matching on Test Outputs". Do not copy this pattern.
/**
* Workflow size budget.
*
* Workflow definitions in `get-shit-done/workflows/*.md` are loaded verbatim
* into Claude's context every time the corresponding `/gsd:*` command is
* invoked. Unbounded growth is paid on every invocation across every session.
*
* Tiered the same way as agent budgets (#2361):
* - XL : top-level orchestrators (e.g., execute-phase, autonomous)
* - LARGE : multi-step planners
* - DEFAULT : focused single-purpose workflows (target tier)
*
* Raising a budget is a deliberate choice — adjust the constant, write a
* rationale in the PR, and confirm the bloat is not duplicated content
* that belongs in `get-shit-done/references/` or a per-mode subdirectory
* (see `workflows/discuss-phase/modes/` for the progressive-disclosure
* pattern introduced by #2551).
*
* See:
* - https://github.com/gsd-build/get-shit-done/issues/2551 (this test)
* - https://github.com/gsd-build/get-shit-done/issues/2361 (agent budget)
*/
const { test, describe } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('fs');
const path = require('path');
const WORKFLOWS_DIR = path.join(__dirname, '..', 'get-shit-done', 'workflows');
// Bumped from 1700 → 1800 in #3181 to absorb MVP-mode verb-call additions
// in execute-phase.md (1727 → ) and plan-phase.md (1714 → ) from #3178.
// Follow-up #3182 (TBD): extract MVP-mode bodies to `<workflow>/modes/mvp.md`
// per the discuss-phase/modes/ precedent and revert this back to 1700.
// Bumped from 1800 → 1810 in #3707 to absorb the startup orphan-sweep
// block added to execute-phase.md (+2 lines: one comment + one bash command).
const XL_BUDGET = 1810;
const LARGE_BUDGET = 1500;
const DEFAULT_BUDGET = 1000;
// Top-level orchestrators that own end-to-end multi-phase rubrics.
// Grandfathered at current sizes — see PR #2551 for #2551 progressive-disclosure
// pattern that future shrinks should follow.
const XL_WORKFLOWS = new Set([
'execute-phase', // 1727 (post-MVP-verb-integration; was 1622)
'plan-phase', // 1714 (post-MVP-verb-integration; was 1493)
'new-project', // 1391
]);
// Multi-step planners and bigger feature workflows. Grandfathered.
const LARGE_WORKFLOWS = new Set([
'docs-update', // 1155
'autonomous', // 789
'complete-milestone', // 847
'verify-work', // 740
'transition', // 693
'discuss-phase-assumptions', // 670
'progress', // 619
'new-milestone', // 611
'update', // 587
'quick', // 971
'code-review', // 515
]);
const ALL_WORKFLOWS = fs.readdirSync(WORKFLOWS_DIR)
.filter(f => f.endsWith('.md'))
.map(f => f.replace('.md', ''));
function budgetFor(workflow) {
if (XL_WORKFLOWS.has(workflow)) return { tier: 'XL', limit: XL_BUDGET };
if (LARGE_WORKFLOWS.has(workflow)) return { tier: 'LARGE', limit: LARGE_BUDGET };
return { tier: 'DEFAULT', limit: DEFAULT_BUDGET };
}
function lineCount(filePath) {
const content = fs.readFileSync(filePath, 'utf-8');
if (content.length === 0) return 0;
const trailingNewline = content.endsWith('\n') ? 1 : 0;
return content.split('\n').length - trailingNewline;
}
describe('SIZE: workflow line-count budget', () => {
for (const workflow of ALL_WORKFLOWS) {
const { tier, limit } = budgetFor(workflow);
test(`${workflow} (${tier}) stays under ${limit} lines`, () => {
const filePath = path.join(WORKFLOWS_DIR, workflow + '.md');
const lines = lineCount(filePath);
assert.ok(
lines <= limit,
`${workflow}.md has ${lines} lines — exceeds ${tier} budget of ${limit}. ` +
`Extract per-mode bodies to a workflows/${workflow}/modes/ subdirectory, ` +
`templates to workflows/${workflow}/templates/, or shared references ` +
`to get-shit-done/references/. See workflows/discuss-phase/ for the pattern.`
);
});
}
});
describe('SIZE: discuss-phase progressive disclosure (issue #2551)', () => {
// Issue #2551 explicitly targets discuss-phase.md at <500 lines, separate from
// the per-tier grandfathered budgets above. This is the headline metric of the
// refactor — every other workflow above 500 is grandfathered at its current
// size and may shrink later by following the same pattern.
const DISCUSS_PHASE_TARGET = 500;
test(`discuss-phase.md is under ${DISCUSS_PHASE_TARGET} lines (issue #2551 target)`, () => {
const filePath = path.join(WORKFLOWS_DIR, 'discuss-phase.md');
const lines = lineCount(filePath);
assert.ok(
lines < DISCUSS_PHASE_TARGET,
`discuss-phase.md has ${lines} lines — must be under ${DISCUSS_PHASE_TARGET} per #2551. ` +
`Per-mode logic belongs in workflows/discuss-phase/modes/<mode>.md, ` +
`templates in workflows/discuss-phase/templates/.`
);
});
const SUBDIR = path.join(WORKFLOWS_DIR, 'discuss-phase');
test('mode files exist for every documented mode', () => {
const expected = ['power', 'all', 'auto', 'chain', 'text', 'batch', 'analyze', 'default', 'advisor'];
for (const mode of expected) {
const p = path.join(SUBDIR, 'modes', `${mode}.md`);
assert.ok(
fs.existsSync(p),
`Expected mode file ${path.relative(WORKFLOWS_DIR, p)} — missing. ` +
`Each --flag in commands/gsd/discuss-phase.md must have a matching mode file.`
);
}
});
test('every mode file is a real, non-empty workflow doc', () => {
const modesDir = path.join(SUBDIR, 'modes');
if (!fs.existsSync(modesDir)) {
assert.fail(`workflows/discuss-phase/modes/ directory does not exist`);
}
for (const file of fs.readdirSync(modesDir)) {
if (!file.endsWith('.md')) continue;
const p = path.join(modesDir, file);
const content = fs.readFileSync(p, 'utf-8');
assert.ok(content.trim().length > 100,
`${file} is empty or near-empty (${content.length} chars) — extraction must preserve behavior, not stub it out`);
}
});
test('templates extracted to discuss-phase/templates/', () => {
const expected = ['context.md', 'discussion-log.md', 'checkpoint.json'];
for (const t of expected) {
const p = path.join(SUBDIR, 'templates', t);
assert.ok(fs.existsSync(p),
`Expected template ${path.relative(WORKFLOWS_DIR, p)} — missing.`);
}
});
test('parent discuss-phase.md dispatches to mode files (power)', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
assert.ok(
/discuss-phase\/modes\/power\.md/.test(parent) ||
/discuss-phase-power\.md/.test(parent),
`Parent discuss-phase.md must reference workflows/discuss-phase/modes/power.md ` +
`(or the legacy discuss-phase-power.md alias) somewhere in its dispatch logic.`
);
});
test('parent dispatches to all extracted modes (auto, chain, all, advisor)', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
for (const mode of ['auto', 'chain', 'all', 'advisor']) {
assert.ok(
new RegExp(`discuss-phase/modes/${mode}\\.md`).test(parent),
`Parent discuss-phase.md must reference workflows/discuss-phase/modes/${mode}.md`
);
}
});
test('parent reads CONTEXT.md template at the write step (not at top)', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
// The template reference must appear inside or near the write_context step,
// not in the top-level <required_reading> block (which would defeat lazy load).
const requiredReadingMatch = parent.match(/<required_reading>([\s\S]*?)<\/required_reading>/);
if (requiredReadingMatch) {
assert.ok(
!/discuss-phase\/templates\/context\.md/.test(requiredReadingMatch[1]),
`CONTEXT.md template must NOT be in <required_reading> — that defeats lazy loading. ` +
`Read it inside the write_context step, just before writing the file.`
);
}
assert.ok(
/discuss-phase\/templates\/context\.md/.test(parent),
`Parent must reference workflows/discuss-phase/templates/context.md somewhere ` +
`(inside write_context step) so the template loads only when CONTEXT.md is being written.`
);
});
test('advisor block is gated behind USER-PROFILE.md existence check', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
// The guard MUST be a file-existence check (test -f or equivalent), not an
// unconditional Read of the advisor mode file.
assert.ok(
/USER-PROFILE\.md/.test(parent),
'Parent must reference USER-PROFILE.md to detect advisor mode'
);
assert.ok(
/test\s+-[ef]\s+["'$].*USER-PROFILE/.test(parent) ||
/\[\[\s+-[ef]\s+["'$].*USER-PROFILE/.test(parent) ||
/\[\s+-[ef]\s+["'$].*USER-PROFILE/.test(parent),
'Advisor mode detection must use a file-existence guard (test -f / [ -f ]) ' +
'so the advisor mode file is only Read when USER-PROFILE.md exists.'
);
// Confirm advisor.md Read is conditional on ADVISOR_MODE
const advisorReadGuarded =
/ADVISOR_MODE[\s\S]{0,200}?modes\/advisor\.md/.test(parent) ||
/modes\/advisor\.md[\s\S]{0,200}?ADVISOR_MODE/.test(parent) ||
/if[\s\S]{0,200}?ADVISOR_MODE[\s\S]{0,400}?advisor\.md/.test(parent);
assert.ok(
advisorReadGuarded,
'Read of modes/advisor.md must be guarded by ADVISOR_MODE (which derives from USER-PROFILE.md existence). ' +
'Skip the Read entirely when no profile is present.'
);
});
test('auto mode file documents skipping interactive questions (regression)', () => {
const auto = fs.readFileSync(path.join(SUBDIR, 'modes', 'auto.md'), 'utf-8');
assert.ok(
/skip[\s\S]{0,80}interactive|without\s+(?:using\s+)?AskUserQuestion|recommended\s+(?:option|default)/i.test(auto),
`auto.md must preserve the documented behavior: skip interactive questions ` +
`and pick the recommended option without using AskUserQuestion.`
);
});
test('auto mode preserves the single-pass cap (regression for inline rule)', () => {
const auto = fs.readFileSync(path.join(SUBDIR, 'modes', 'auto.md'), 'utf-8');
assert.ok(
/single\s+pass|max_discuss_passes|MAX_PASSES|pass\s+cap/i.test(auto),
`auto.md must preserve the auto-mode pass cap rule from the original workflow. ` +
`Without it, the workflow can self-feed and consume unbounded resources.`
);
});
test('all mode file documents auto-selecting all gray areas (regression)', () => {
const allMode = fs.readFileSync(path.join(SUBDIR, 'modes', 'all.md'), 'utf-8');
assert.ok(
/auto-select(?:ed)?\s+ALL|select\s+ALL|all\s+gray\s+areas/i.test(allMode),
`all.md must preserve the documented behavior: auto-select ALL gray areas ` +
`without asking the user.`
);
});
test('chain mode documents auto-advance to plan-phase (regression)', () => {
const chain = fs.readFileSync(path.join(SUBDIR, 'modes', 'chain.md'), 'utf-8');
assert.ok(
/plan-phase/.test(chain) && /(auto-advance|auto\s+plan)/i.test(chain),
`chain.md must preserve the documented auto-advance to plan-phase behavior.`
);
});
test('text mode documents replacing AskUserQuestion (regression)', () => {
const textMode = fs.readFileSync(path.join(SUBDIR, 'modes', 'text.md'), 'utf-8');
assert.ok(
/AskUserQuestion/.test(textMode) && /(numbered\s+list|plain[-\s]text)/i.test(textMode),
`text.md must preserve the rule: replace AskUserQuestion with plain-text numbered lists.`
);
});
test('batch mode documents 2-5 question grouping (regression)', () => {
const batch = fs.readFileSync(path.join(SUBDIR, 'modes', 'batch.md'), 'utf-8');
assert.ok(
/2[-\s–]5|2\s+to\s+5|--batch=N|--batch\s+N/.test(batch),
`batch.md must preserve the 2-5 questions-per-batch rule.`
);
});
test('analyze mode documents trade-off table presentation (regression)', () => {
const analyze = fs.readFileSync(path.join(SUBDIR, 'modes', 'analyze.md'), 'utf-8');
assert.ok(
/trade[-\s]off|tradeoff|pros[\s\S]{0,30}cons/i.test(analyze),
`analyze.md must preserve the trade-off analysis presentation rule.`
);
});
test('CONTEXT.md template preserves all required sections', () => {
const tpl = fs.readFileSync(path.join(SUBDIR, 'templates', 'context.md'), 'utf-8');
for (const section of ['<domain>', '<decisions>', '<canonical_refs>', '<code_context>', '<specifics>', '<deferred>']) {
assert.ok(tpl.includes(section),
`CONTEXT.md template missing required section ${section} — extraction dropped content.`);
}
// spec_lock is conditional but the template still has to include it as a documented option
assert.ok(/spec_lock/i.test(tpl),
`CONTEXT.md template must document the conditional <spec_lock> section for SPEC.md integration.`);
});
test('checkpoint template is valid JSON', () => {
const raw = fs.readFileSync(path.join(SUBDIR, 'templates', 'checkpoint.json'), 'utf-8');
assert.doesNotThrow(() => JSON.parse(raw),
`checkpoint.json template must parse as valid JSON — downstream code reads it.`);
const parsed = JSON.parse(raw);
for (const key of ['phase', 'phase_name', 'timestamp', 'areas_completed', 'areas_remaining', 'decisions']) {
assert.ok(key in parsed,
`checkpoint.json template missing required field "${key}" — schema regression vs original workflow.`);
}
});
test('parent does not leak per-mode bodies inline (would defeat extraction)', () => {
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
// Heuristic: the parent should not contain the full DISCUSSION-LOG.md template body
// (extracted to templates/discussion-log.md) — that's the heaviest single block.
// Look for unique strings that ONLY appear in the original inline template.
const inlineDiscussionLogSignal = /\| Option \| Description \| Selected \|/g;
const occurrences = (parent.match(inlineDiscussionLogSignal) || []).length;
assert.ok(occurrences === 0,
`Parent discuss-phase.md still contains the inline DISCUSSION-LOG.md table — ` +
`that block must move to workflows/discuss-phase/templates/discussion-log.md.`);
});
test('negative: invalid mode flag combinations document a clear error path', () => {
// Sanity check: the parent file should explicitly handle the mode dispatch
// rather than silently doing nothing on an unknown flag pattern.
const parent = fs.readFileSync(path.join(WORKFLOWS_DIR, 'discuss-phase.md'), 'utf-8');
assert.ok(
/ARGUMENTS|--auto|--chain|--all|--power/.test(parent),
'Parent must dispatch on $ARGUMENTS — losing the flag-parsing block would silently ' +
'fall back to default mode and obscure user errors.'
);
});
});