From caec78ed3871d7e30ee44d67333dbbf403b3c2fb Mon Sep 17 00:00:00 2001 From: LakshmanTurlapati Date: Thu, 2 Apr 2026 21:36:36 -0500 Subject: [PATCH] fix: harden workstream session routing fallback --- get-shit-done/bin/lib/core.cjs | 68 ++++++-- get-shit-done/references/workstream-flag.md | 35 ++++ tests/workstream.test.cjs | 177 ++++++++++++++++++++ 3 files changed, 264 insertions(+), 16 deletions(-) diff --git a/get-shit-done/bin/lib/core.cjs b/get-shit-done/bin/lib/core.cjs index e80204abe..f3254efdf 100644 --- a/get-shit-done/bin/lib/core.cjs +++ b/get-shit-done/bin/lib/core.cjs @@ -22,10 +22,11 @@ const WORKSTREAM_SESSION_ENV_KEYS = [ 'WT_SESSION', 'TMUX_PANE', 'ZELLIJ_SESSION_NAME', - 'TTY', - 'SSH_TTY', ]; +let cachedControllingTtyToken = null; +let didProbeControllingTtyToken = false; + // ─── Path helpers ──────────────────────────────────────────────────────────── /** Normalize a relative path to always use forward slashes (cross-platform). */ @@ -637,10 +638,14 @@ function sanitizeWorkstreamSessionToken(value) { return token ? token.slice(0, 160) : null; } -function getControllingTtyToken() { - for (const envKey of ['TTY', 'SSH_TTY']) { - const token = sanitizeWorkstreamSessionToken(process.env[envKey]); - if (token) return `tty-${token.replace(/^dev_/, '')}`; +function probeControllingTtyToken() { + if (didProbeControllingTtyToken) return cachedControllingTtyToken; + didProbeControllingTtyToken = true; + + // `tty` reads stdin. When stdin is already non-interactive, spawning it only + // adds avoidable failures on the routing hot path and cannot reveal a stable token. + if (!(process.stdin && process.stdin.isTTY)) { + return cachedControllingTtyToken; } try { @@ -650,13 +655,31 @@ function getControllingTtyToken() { }).trim(); if (ttyPath && ttyPath !== 'not a tty') { const token = sanitizeWorkstreamSessionToken(ttyPath.replace(/^\/dev\//, '')); - if (token) return `tty-${token}`; + if (token) cachedControllingTtyToken = `tty-${token}`; } } catch {} - return null; + return cachedControllingTtyToken; } +function getControllingTtyToken() { + for (const envKey of ['TTY', 'SSH_TTY']) { + const token = sanitizeWorkstreamSessionToken(process.env[envKey]); + if (token) return `tty-${token.replace(/^dev_/, '')}`; + } + + return probeControllingTtyToken(); +} + +/** + * Resolve a deterministic session key for workstream-local routing. + * + * Order: + * 1. Explicit runtime/session env vars (`GSD_SESSION_KEY`, `CODEX_THREAD_ID`, etc.) + * 2. Terminal identity exposed via `TTY` or `SSH_TTY` + * 3. One best-effort `tty` probe when stdin is interactive + * 4. `null`, which tells callers to use the legacy shared pointer fallback + */ function getWorkstreamSessionKey() { for (const envKey of WORKSTREAM_SESSION_ENV_KEYS) { const raw = process.env[envKey]; @@ -685,16 +708,32 @@ function getSessionScopedWorkstreamFile(cwd) { }; } -function readActiveWorkstreamPointer(filePath, cwd) { +function clearActiveWorkstreamPointer(filePath, cleanupDirPath) { + try { fs.unlinkSync(filePath); } catch {} + + // Session-scoped pointers for a repo share one tmp directory. Only remove it + // when it is empty so clearing or self-healing one session never deletes siblings. + if (cleanupDirPath) { + try { fs.rmdirSync(cleanupDirPath); } catch {} + } +} + +/** + * Pointer files are self-healing: invalid names or deleted-workstream pointers + * are removed on read so the session falls back to `null` instead of carrying + * silent stale state forward. Session-scoped callers may also prune an empty + * per-project tmp directory; shared `.planning/active-workstream` callers do not. + */ +function readActiveWorkstreamPointer(filePath, cwd, cleanupDirPath = null) { try { const name = fs.readFileSync(filePath, 'utf-8').trim(); if (!name || !/^[a-zA-Z0-9_-]+$/.test(name)) { - try { fs.unlinkSync(filePath); } catch {} + clearActiveWorkstreamPointer(filePath, cleanupDirPath); return null; } const wsDir = path.join(planningRoot(cwd), 'workstreams', name); if (!fs.existsSync(wsDir)) { - try { fs.unlinkSync(filePath); } catch {} + clearActiveWorkstreamPointer(filePath, cleanupDirPath); return null; } return name; @@ -716,7 +755,7 @@ function readActiveWorkstreamPointer(filePath, cwd) { function getActiveWorkstream(cwd) { const sessionScoped = getSessionScopedWorkstreamFile(cwd); if (sessionScoped) { - return readActiveWorkstreamPointer(sessionScoped.filePath, cwd); + return readActiveWorkstreamPointer(sessionScoped.filePath, cwd, sessionScoped.dirPath); } const sharedFilePath = path.join(planningRoot(cwd), 'active-workstream'); @@ -737,10 +776,7 @@ function setActiveWorkstream(cwd, name) { : path.join(planningRoot(cwd), 'active-workstream'); if (!name) { - try { fs.unlinkSync(filePath); } catch {} - if (sessionScoped) { - try { fs.rmdirSync(sessionScoped.dirPath); } catch {} - } + clearActiveWorkstreamPointer(filePath, sessionScoped ? sessionScoped.dirPath : null); return; } if (!/^[a-zA-Z0-9_-]+$/.test(name)) { diff --git a/get-shit-done/references/workstream-flag.md b/get-shit-done/references/workstream-flag.md index 0b24d5ca5..10d908b2b 100644 --- a/get-shit-done/references/workstream-flag.md +++ b/get-shit-done/references/workstream-flag.md @@ -24,6 +24,41 @@ GSD now prefers a session-scoped pointer keyed by runtime/session identity or the controlling TTY). This keeps concurrent sessions isolated while preserving legacy compatibility for runtimes that do not expose a stable session key. +## Session Identity Resolution + +When GSD resolves the session-scoped pointer in step 3 above, it uses this order: + +1. Explicit runtime/session env vars such as `GSD_SESSION_KEY`, `CODEX_THREAD_ID`, + `CLAUDE_SESSION_ID`, `CLAUDE_CODE_SSE_PORT`, `OPENCODE_SESSION_ID`, + `GEMINI_SESSION_ID`, `CURSOR_SESSION_ID`, `WINDSURF_SESSION_ID`, + `TERM_SESSION_ID`, `WT_SESSION`, `TMUX_PANE`, and `ZELLIJ_SESSION_NAME` +2. `TTY` or `SSH_TTY` if the shell/runtime already exposes the terminal path +3. A single best-effort `tty` probe, but only when stdin is interactive + +If none of those produce a stable identity, GSD does not keep probing. It falls +back directly to the legacy shared `.planning/active-workstream` file. + +This matters in headless or stripped environments: when stdin is already +non-interactive, GSD intentionally skips shelling out to `tty` because that path +cannot discover a stable session identity and only adds avoidable failures on the +routing hot path. + +## Pointer Lifecycle + +Session-scoped pointers are intentionally lightweight and best-effort: + +- Clearing a workstream for one session removes only that session's pointer file +- If that was the last pointer for the repo, GSD also removes the now-empty + per-project temp directory +- If sibling session pointers still exist, the temp directory is left in place +- When a pointer refers to a workstream directory that no longer exists, GSD + treats it as stale state: it removes that pointer file and resolves to `null` + until the session explicitly sets a new active workstream again + +GSD does not currently run a background garbage collector for historical temp +directories. Cleanup is opportunistic at the pointer being cleared or self-healed, +and broader temp hygiene is left to OS temp cleanup or future maintenance work. + ## Routing Propagation All workflow routing commands include `${GSD_WS}` which: diff --git a/tests/workstream.test.cjs b/tests/workstream.test.cjs index 442cfaf4f..c02f0736d 100644 --- a/tests/workstream.test.cjs +++ b/tests/workstream.test.cjs @@ -4,7 +4,9 @@ const { describe, test, before, after, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); +const crypto = require('crypto'); const fs = require('fs'); +const os = require('os'); const path = require('path'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); @@ -19,6 +21,53 @@ function createProjectWithState(tmpDir, roadmap, state) { } } +function createFailingTtyEnv(tmpDir) { + const binDir = path.join(tmpDir, 'fake-bin'); + const markerFile = path.join(tmpDir, 'tty-invoked.log'); + const inheritedPath = process.env.PATH || process.env.Path || ''; + + fs.mkdirSync(binDir, { recursive: true }); + fs.writeFileSync( + path.join(binDir, 'tty'), + '#!/bin/sh\nif [ -n "$GSD_TTY_MARKER" ]; then printf "tty\\n" >> "$GSD_TTY_MARKER"; fi\nexit 99\n', + 'utf-8' + ); + fs.chmodSync(path.join(binDir, 'tty'), 0o755); + fs.writeFileSync( + path.join(binDir, 'tty.cmd'), + '@echo off\r\nif not "%GSD_TTY_MARKER%"=="" echo tty>>"%GSD_TTY_MARKER%"\r\nexit /b 99\r\n', + 'utf-8' + ); + + return { + markerFile, + env: { + PATH: `${binDir}${path.delimiter}${inheritedPath}`, + GSD_TTY_MARKER: markerFile, + }, + }; +} + +function getSessionPointerDir(tmpDir) { + const planningPath = fs.realpathSync.native(path.join(tmpDir, '.planning')); + const projectId = crypto + .createHash('sha1') + .update(planningPath) + .digest('hex') + .slice(0, 16); + return path.join(os.tmpdir(), 'gsd-workstream-sessions', projectId); +} + +function sanitizeSessionToken(value) { + const token = String(value).trim().replace(/[^a-zA-Z0-9._-]+/g, '_').replace(/^_+|_+$/g, ''); + return token ? token.slice(0, 160) : null; +} + +function getSessionPointerFileName(envKey, value) { + const token = sanitizeSessionToken(value); + return `${envKey.toLowerCase().replace(/[^a-z0-9]+/g, '-')}-${token}`; +} + // ─── planningDir / planningPaths env-var awareness ────────────────────────── describe('planningDir workstream awareness via env var', () => { @@ -139,6 +188,134 @@ describe('session-scoped active workstream routing', () => { }); }); +describe('session resolution hardening', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + + for (const [ws, status] of [['alpha', 'Alpha active'], ['beta', 'Beta active']]) { + const wsDir = path.join(tmpDir, '.planning', 'workstreams', ws); + fs.mkdirSync(path.join(wsDir, 'phases'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), `# State\n**Status:** ${status}\n`); + } + }); + + afterEach(() => cleanup(tmpDir)); + + test('headless runs skip tty probing and use the shared active-workstream fallback', () => { + const { markerFile, env } = createFailingTtyEnv(tmpDir); + const set = runGsdTools(['workstream', 'set', 'alpha', '--raw'], tmpDir, env); + const get = runGsdTools(['workstream', 'get', '--raw'], tmpDir, env); + + assert.ok(set.success, `headless set failed: ${set.error}`); + assert.ok(get.success, `headless get failed: ${get.error}`); + assert.ok(!fs.existsSync(markerFile), 'headless fallback should not invoke the tty subprocess'); + assert.strictEqual(get.output, 'alpha'); + assert.strictEqual( + fs.readFileSync(path.join(tmpDir, '.planning', 'active-workstream'), 'utf-8').trim(), + 'alpha' + ); + assert.ok(!fs.existsSync(getSessionPointerDir(tmpDir)), 'headless fallback should not create session tmp pointers'); + }); + + test('explicit runtime session ids outrank tty-derived identities', () => { + const set = runGsdTools(['workstream', 'set', 'alpha', '--raw'], tmpDir, { + GSD_SESSION_KEY: 'shared-session', + TTY: '/dev/pts/42', + }); + const get = runGsdTools(['workstream', 'get', '--raw'], tmpDir, { + GSD_SESSION_KEY: 'shared-session', + TTY: '/dev/pts/99', + }); + + assert.ok(set.success, `session-key set failed: ${set.error}`); + assert.ok(get.success, `session-key get failed: ${get.error}`); + assert.strictEqual(get.output, 'alpha'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', 'active-workstream'))); + }); + + test('TTY environment variables provide a session-scoped pointer without spawning tty', () => { + const { markerFile, env } = createFailingTtyEnv(tmpDir); + const ttyEnv = { ...env, TTY: '/dev/pts/42' }; + const set = runGsdTools(['workstream', 'set', 'beta', '--raw'], tmpDir, ttyEnv); + const get = runGsdTools(['workstream', 'get', '--raw'], tmpDir, ttyEnv); + + assert.ok(set.success, `TTY set failed: ${set.error}`); + assert.ok(get.success, `TTY get failed: ${get.error}`); + assert.ok(!fs.existsSync(markerFile), 'TTY env should be used directly without invoking the tty subprocess'); + assert.strictEqual(get.output, 'beta'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', 'active-workstream'))); + }); +}); + +describe('pointer lifecycle hardening', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + + for (const [ws, status] of [['alpha', 'Alpha active'], ['beta', 'Beta active']]) { + const wsDir = path.join(tmpDir, '.planning', 'workstreams', ws); + fs.mkdirSync(path.join(wsDir, 'phases'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), `# State\n**Status:** ${status}\n`); + } + }); + + afterEach(() => cleanup(tmpDir)); + + test('clearing one session pointer leaves sibling session pointers intact', () => { + const sessionDir = getSessionPointerDir(tmpDir); + const alphaFile = getSessionPointerFileName('GSD_SESSION_KEY', 'session-alpha'); + const betaFile = getSessionPointerFileName('GSD_SESSION_KEY', 'session-beta'); + + runGsdTools(['workstream', 'set', 'alpha', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + runGsdTools(['workstream', 'set', 'beta', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-beta' }); + + const clearAlpha = runGsdTools(['workstream', 'set', '--clear', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + const beta = runGsdTools(['workstream', 'get', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-beta' }); + + assert.ok(clearAlpha.success, `clear alpha failed: ${clearAlpha.error}`); + assert.ok(beta.success, `beta get failed: ${beta.error}`); + assert.strictEqual(beta.output, 'beta'); + assert.ok(fs.existsSync(sessionDir), 'session tmp directory should remain while a sibling pointer exists'); + assert.deepStrictEqual(fs.readdirSync(sessionDir).sort(), [betaFile]); + assert.ok(!fs.existsSync(path.join(sessionDir, alphaFile))); + }); + + test('stale pointers self-clean without deleting sibling session pointers', () => { + const sessionDir = getSessionPointerDir(tmpDir); + const betaFile = getSessionPointerFileName('GSD_SESSION_KEY', 'session-beta'); + + runGsdTools(['workstream', 'set', 'alpha', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + runGsdTools(['workstream', 'set', 'beta', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-beta' }); + fs.rmSync(path.join(tmpDir, '.planning', 'workstreams', 'alpha'), { recursive: true, force: true }); + + const alpha = runGsdTools(['workstream', 'get'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + const beta = runGsdTools(['workstream', 'get', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-beta' }); + + assert.ok(alpha.success, `stale alpha get failed: ${alpha.error}`); + assert.ok(beta.success, `beta get after stale cleanup failed: ${beta.error}`); + assert.strictEqual(JSON.parse(alpha.output).active, null); + assert.strictEqual(beta.output, 'beta'); + assert.ok(fs.existsSync(sessionDir), 'sibling pointer should keep the session tmp directory alive'); + assert.deepStrictEqual(fs.readdirSync(sessionDir).sort(), [betaFile]); + }); + + test('clearing the last session pointer removes the empty session tmp directory', () => { + const sessionDir = getSessionPointerDir(tmpDir); + const set = runGsdTools(['workstream', 'set', 'alpha', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + + assert.ok(set.success, `set alpha failed: ${set.error}`); + assert.ok(fs.existsSync(sessionDir), 'session tmp directory should exist after storing a session-scoped pointer'); + + const clear = runGsdTools(['workstream', 'set', '--clear', '--raw'], tmpDir, { GSD_SESSION_KEY: 'session-alpha' }); + + assert.ok(clear.success, `clear alpha failed: ${clear.error}`); + assert.ok(!fs.existsSync(sessionDir), 'last-pointer cleanup should remove the empty session tmp directory'); + }); +}); + // ─── Workstream CRUD ──────────────────────────────────────────────────────── describe('workstream create', () => {