From 65f49af383a0914b2945a84d45bff34ad18ad5af Mon Sep 17 00:00:00 2001 From: Lex Christopherson Date: Wed, 25 Mar 2026 22:38:14 -0600 Subject: [PATCH 01/12] refactor: deduplicate session-lock compromise handler and state assignment The lock acquisition had a primary path and a retry path with identical 28-line onCompromised callbacks and 6-line state assignment blocks (68 lines of copy-paste). Extract into createLockCompromisedHandler() and assignLockState() helpers so bug fixes only need to be applied once. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/session-lock.ts | 118 +++++++++---------- 1 file changed, 54 insertions(+), 64 deletions(-) diff --git a/src/resources/extensions/gsd/session-lock.ts b/src/resources/extensions/gsd/session-lock.ts index 7c0a0d6ce..f0f3d2562 100644 --- a/src/resources/extensions/gsd/session-lock.ts +++ b/src/resources/extensions/gsd/session-lock.ts @@ -167,6 +167,56 @@ function ensureExitHandler(_gsdDir: string): void { }); } +// ─── Lock Acquisition Helpers ─────────────────────────────────────────────── + +/** + * Create the onCompromised callback for proper-lockfile. + * + * proper-lockfile fires onCompromised when it detects mtime drift (system sleep, + * event loop stall, etc.). The default handler throws inside setTimeout — an + * uncaught exception that crashes or corrupts process state. + * + * False-positive suppression (#1362): If we're still within the stale window + * (30 min since acquisition), the mtime mismatch is from an event loop stall + * during a long LLM call — not a real takeover. Log and continue. + * + * PID ownership check (#1578): Past the stale window, check if the lock file + * still contains our PID before declaring compromise. Retry reads tolerate + * transient filesystem hiccups (NFS/CIFS latency, APFS snapshots, etc.) (#2324). + */ +function createLockCompromisedHandler(lockFilePath: string): () => void { + return () => { + const elapsed = Date.now() - _lockAcquiredAt; + if (elapsed < 1_800_000) { + process.stderr.write( + `[gsd] Lock heartbeat caught up after ${Math.round(elapsed / 1000)}s — long LLM call, no action needed.\n`, + ); + return; + } + const existing = readExistingLockDataWithRetry(lockFilePath); + if (existing && existing.pid === process.pid) { + process.stderr.write( + `[gsd] Lock heartbeat mismatch after ${Math.round(elapsed / 1000)}s — lock file still owned by PID ${process.pid}, treating as false positive.\n`, + ); + return; + } + _lockCompromised = true; + _releaseFunction = null; + }; +} + +/** + * Assign module-level lock state after a successful lock acquisition. + */ +function assignLockState(basePath: string, release: () => void, lockFilePath: string): void { + _releaseFunction = release; + _lockedPath = basePath; + _lockPid = process.pid; + _lockCompromised = false; + _lockAcquiredAt = Date.now(); + _snapshotLockPath = lockFilePath; +} + // ─── Public API ───────────────────────────────────────────────────────────── /** @@ -226,43 +276,10 @@ export function acquireSessionLock(basePath: string): SessionLockResult { realpath: false, stale: 1_800_000, // 30 minutes — safe for laptop sleep / long event loop stalls update: 10_000, // Update lock mtime every 10s to prove liveness - onCompromised: () => { - // proper-lockfile detected mtime drift (system sleep, event loop stall, etc.). - // Default handler throws inside setTimeout — an uncaught exception that crashes - // or corrupts process state. - // - // False-positive suppression (#1362): If we're still within the stale window - // (30 min since acquisition), the mtime mismatch is from an event loop stall - // during a long LLM call — not a real takeover. Log and continue. - const elapsed = Date.now() - _lockAcquiredAt; - if (elapsed < 1_800_000) { - process.stderr.write( - `[gsd] Lock heartbeat caught up after ${Math.round(elapsed / 1000)}s — long LLM call, no action needed.\n`, - ); - return; // Suppress false positive - } - // Past the stale window — check if the lock file still belongs to us before - // declaring compromise (#1578). Retry reads to tolerate transient filesystem - // hiccups (NFS/CIFS latency, APFS snapshots, etc.) (#2324). - const existing = readExistingLockDataWithRetry(lp); - if (existing && existing.pid === process.pid) { - process.stderr.write( - `[gsd] Lock heartbeat mismatch after ${Math.round(elapsed / 1000)}s — lock file still owned by PID ${process.pid}, treating as false positive.\n`, - ); - return; // Our PID still owns the lock file — no real takeover - } - // Lock file is gone or owned by another PID after retries — real compromise - _lockCompromised = true; - _releaseFunction = null; - }, + onCompromised: createLockCompromisedHandler(lp), }); - _releaseFunction = release; - _lockedPath = basePath; - _lockPid = process.pid; - _lockCompromised = false; - _lockAcquiredAt = Date.now(); - _snapshotLockPath = lp; // Snapshot the resolved path for consistent access (#1363) + assignLockState(basePath, release, lp); // Safety net: clean up lock dir on process exit if _releaseFunction // wasn't called (e.g., normal exit after clean completion) (#1245). @@ -290,36 +307,9 @@ export function acquireSessionLock(basePath: string): SessionLockResult { realpath: false, stale: 1_800_000, // 30 minutes — match primary lock settings update: 10_000, - onCompromised: () => { - // Same false-positive suppression as the primary lock (#1512). - // Without this, the retry path fires _lockCompromised unconditionally - // on benign mtime drift (laptop sleep, heavy LLM event loop stalls). - const elapsed = Date.now() - _lockAcquiredAt; - if (elapsed < 1_800_000) { - process.stderr.write( - `[gsd] Lock heartbeat caught up after ${Math.round(elapsed / 1000)}s — long LLM call, no action needed.\n`, - ); - return; - } - // Check PID ownership before declaring compromise (#1578). - // Retry reads to tolerate transient filesystem hiccups (#2324). - const existing = readExistingLockDataWithRetry(lp); - if (existing && existing.pid === process.pid) { - process.stderr.write( - `[gsd] Lock heartbeat mismatch after ${Math.round(elapsed / 1000)}s — lock file still owned by PID ${process.pid}, treating as false positive.\n`, - ); - return; - } - _lockCompromised = true; - _releaseFunction = null; - }, + onCompromised: createLockCompromisedHandler(lp), }); - _releaseFunction = release; - _lockedPath = basePath; - _lockPid = process.pid; - _lockCompromised = false; - _lockAcquiredAt = Date.now(); - _snapshotLockPath = lp; // Snapshot for retry path too (#1363) + assignLockState(basePath, release, lp); // Safety net — uses centralized handler to avoid double-registration ensureExitHandler(gsdDir); From ed23fe56ab7412a6b426af4e51bafce3155d0347 Mon Sep 17 00:00:00 2001 From: Lex Christopherson Date: Wed, 25 Mar 2026 22:39:52 -0600 Subject: [PATCH 02/12] refactor: consolidate branch name patterns into single module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SLICE_BRANCH_RE, QUICK_BRANCH_RE, and WORKFLOW_BRANCH_RE were scattered across worktree.ts and git-service.ts. Extract all three into branch-patterns.ts as the single source of truth. Both original modules re-export for backward compatibility — no consumer changes needed. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/branch-patterns.ts | 16 ++++++++++++++++ src/resources/extensions/gsd/git-service.ts | 15 +++------------ src/resources/extensions/gsd/worktree.ts | 5 +++-- 3 files changed, 22 insertions(+), 14 deletions(-) create mode 100644 src/resources/extensions/gsd/branch-patterns.ts diff --git a/src/resources/extensions/gsd/branch-patterns.ts b/src/resources/extensions/gsd/branch-patterns.ts new file mode 100644 index 000000000..70d8a40ab --- /dev/null +++ b/src/resources/extensions/gsd/branch-patterns.ts @@ -0,0 +1,16 @@ +/** + * GSD branch naming patterns — single source of truth. + * + * gsd/// → SLICE_BRANCH_RE + * gsd/quick/- → QUICK_BRANCH_RE + * gsd//<...> → WORKFLOW_BRANCH_RE (non-milestone gsd/ branches) + */ + +/** Matches gsd/ slice branches: gsd/[worktree/]M001[-hash]/S01 */ +export const SLICE_BRANCH_RE = /^gsd\/(?:([a-zA-Z0-9_-]+)\/)?(M\d+(?:-[a-z0-9]{6})?)\/(S\d+)$/; + +/** Matches gsd/quick/ task branches */ +export const QUICK_BRANCH_RE = /^gsd\/quick\//; + +/** Matches gsd/ workflow branches (non-milestone, e.g. gsd/workflow-name/...) */ +export const WORKFLOW_BRANCH_RE = /^gsd\/(?!M\d)[\w-]+\//; diff --git a/src/resources/extensions/gsd/git-service.ts b/src/resources/extensions/gsd/git-service.ts index 69851c418..1d84ae3d6 100644 --- a/src/resources/extensions/gsd/git-service.ts +++ b/src/resources/extensions/gsd/git-service.ts @@ -18,8 +18,8 @@ import { loadEffectiveGSDPreferences } from "./preferences.js"; import { detectWorktreeName, - SLICE_BRANCH_RE, } from "./worktree.js"; +import { SLICE_BRANCH_RE, QUICK_BRANCH_RE, WORKFLOW_BRANCH_RE } from "./branch-patterns.js"; import { nativeGetCurrentBranch, nativeDetectMainBranch, @@ -243,17 +243,8 @@ export function readIntegrationBranch(basePath: string, milestoneId: string): st * * The file is committed immediately so the metadata is persisted in git. */ -/** Regex matching GSD quick-task branches: gsd/quick/- */ -export const QUICK_BRANCH_RE = /^gsd\/quick\//; - -/** - * Matches all GSD workflow-template branches: gsd//. - * - * Template IDs are lowercase alphanumeric with hyphens (e.g. hotfix, bugfix, - * small-feature, dep-upgrade). The negative lookahead excludes milestone - * branches (gsd/M001/... or gsd/M001-abc123/...) which use SLICE_BRANCH_RE. - */ -export const WORKFLOW_BRANCH_RE = /^gsd\/(?!M\d)[\w-]+\//; +/** Re-export for backward compatibility — canonical definitions in branch-patterns.ts */ +export { QUICK_BRANCH_RE, WORKFLOW_BRANCH_RE } from "./branch-patterns.js"; export function writeIntegrationBranch( basePath: string, diff --git a/src/resources/extensions/gsd/worktree.ts b/src/resources/extensions/gsd/worktree.ts index 84d3dd6d2..0f84166ae 100644 --- a/src/resources/extensions/gsd/worktree.ts +++ b/src/resources/extensions/gsd/worktree.ts @@ -235,8 +235,9 @@ export function getSliceBranchName(milestoneId: string, sliceId: string, worktre return `gsd/${milestoneId}/${sliceId}`; } -/** Regex that matches both plain and worktree-namespaced slice branches. */ -export const SLICE_BRANCH_RE = /^gsd\/(?:([a-zA-Z0-9_-]+)\/)?(M\d+(?:-[a-z0-9]{6})?)\/(S\d+)$/; +/** Re-export for backward compatibility — canonical definition in branch-patterns.ts */ +export { SLICE_BRANCH_RE } from "./branch-patterns.js"; +import { SLICE_BRANCH_RE } from "./branch-patterns.js"; /** * Parse a slice branch name into its components. From 5bf36151d6fb2f589106e716903588cc591fab9f Mon Sep 17 00:00:00 2001 From: Lex Christopherson Date: Wed, 25 Mar 2026 22:40:14 -0600 Subject: [PATCH 03/12] chore: remove dead worktree code and unused methods Delete unused resource-version.ts (functions duplicated in auto-worktree-sync.ts with zero imports). Remove GitServiceImpl.git() private method with no call sites. Clean up orphaned section headers and dangling JSDoc in git-service.ts. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/resources/extensions/gsd/git-service.ts | 12 --- .../extensions/gsd/resource-version.ts | 101 ------------------ 2 files changed, 113 deletions(-) delete mode 100644 src/resources/extensions/gsd/resource-version.ts diff --git a/src/resources/extensions/gsd/git-service.ts b/src/resources/extensions/gsd/git-service.ts index 69851c418..2ad819d41 100644 --- a/src/resources/extensions/gsd/git-service.ts +++ b/src/resources/extensions/gsd/git-service.ts @@ -454,11 +454,6 @@ export class GitServiceImpl { this._milestoneId = milestoneId; } - /** Convenience wrapper: run git in this repo's basePath. */ - private git(args: string[], options: { allowFailure?: boolean; input?: string } = {}): string { - return runGit(this.basePath, args, options); - } - /** * Smart staging: `git add -A` excluding GSD runtime paths via pathspec. * Falls back to plain `git add -A` if the exclusion pathspec fails. @@ -617,11 +612,6 @@ export class GitServiceImpl { return nativeGetCurrentBranch(this.basePath); } - /** True if currently on a GSD slice branch. */ - // ─── Branch Lifecycle ────────────────────────────────────────────────── - - // ─── S05 Features ───────────────────────────────────────────────────── - /** * Create a snapshot ref for the given label (typically a slice branch name). * Gated on prefs.snapshots === true. Ref path: refs/gsd/snapshots/