From 77b16993f8acbb4e4c1b8a237eb53124c781f379 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 19:52:17 -0400 Subject: [PATCH] fix(3584): address coderabbit findings on PR #3606 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit surfaced one outstanding inline finding plus an outside-diff finding and a finer point on two already-remediated sites; all addressed: 1. (inline, Minor) runtime-slash.cjs:31 — a degenerate input like `/gsd:`, `gsd:`, or `gsd-` normalizes to empty and the previous fallback returned the original colon-form input, reintroducing the deprecated shape the module exists to suppress. Now returns `''` (empty string) so callers see "no command" instead of an unroutable string. Also catches whitespace-only inputs the same way. New unit tests pin the contract. 2. (inline, Major) drift.cjs library purity — the earlier remediation passed `projectDir` into `detectDrift` and re-resolved runtime inside the library. CodeRabbit (correctly) flagged this as breaking the module's pure-library contract. detectDrift now accepts `input.runtime` directly; verify.cmdVerifyCodebaseDrift resolves the runtime once and passes the literal name in. drift.cjs no longer reads env or config at all. 3. (duplicate inline, Minor) gsd2-import.cjs:475 — when `gsd2-import --path ` targets a project that isn't the process cwd, the preview command was formatted for the wrong runtime. buildPreview now receives the resolved `projectDir` (the same one used to find the .gsd/ root) instead of the raw `cwd`. 4. (outside-diff, Minor) tests/copilot-install.test.cjs:1-5 — the `allow-test-rule: integration-test-input` rationale block specifically named verify.cjs as the fixture, but #3584 changed that test to use a synthetic input. Comment now describes the real shape of the file's readFileSync usage (commands/, agents/, install.js source inputs to the installer/converter functions under test) and notes the synthetic substitution for the bin/lib path. Full suite: 9366/9366 pass (+1 new test). Lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- get-shit-done/bin/lib/drift.cjs | 15 ++++++++--- get-shit-done/bin/lib/gsd2-import.cjs | 4 ++- get-shit-done/bin/lib/runtime-slash.cjs | 6 ++++- get-shit-done/bin/lib/verify.cjs | 4 ++- .../bug-3584-runtime-slash-formatter.test.cjs | 25 +++++++++++++------ tests/copilot-install.test.cjs | 12 ++++++--- 6 files changed, 47 insertions(+), 19 deletions(-) diff --git a/get-shit-done/bin/lib/drift.cjs b/get-shit-done/bin/lib/drift.cjs index 0723eb4aa..ef6213ba2 100644 --- a/get-shit-done/bin/lib/drift.cjs +++ b/get-shit-done/bin/lib/drift.cjs @@ -116,6 +116,9 @@ function isPathMapped(file, structureMd) { * @param {string|null|undefined} input.structureMd - contents of STRUCTURE.md * @param {number} [input.threshold=3] - min number of drift elements that triggers action * @param {'warn'|'auto-remap'} [input.action='warn'] + * @param {string} [input.runtime='claude'] - runtime name (claude, codex, ...) used + * to format the slash-command in the remediation message. Caller resolves and + * passes this in to keep drift.cjs a pure library with no env/config reads. * @returns {object} result */ function detectDrift(input) { @@ -190,7 +193,7 @@ function detectDrift(input) { if (action === 'auto-remap') { spawnMapper = true; } - message = buildMessage(elements, affectedPaths, action, input.projectDir); + message = buildMessage(elements, affectedPaths, action, input.runtime); } return { @@ -228,7 +231,7 @@ function skipped(reason) { }; } -function buildMessage(elements, affectedPaths, action, projectDir) { +function buildMessage(elements, affectedPaths, action, runtime) { const byCat = {}; for (const e of elements) { (byCat[e.category] ||= []).push(e.path); @@ -253,8 +256,12 @@ function buildMessage(elements, affectedPaths, action, projectDir) { if (action === 'auto-remap') { lines.push(`Auto-remap scheduled for paths: ${affectedPaths.join(', ')}`); } else { - const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs'); - const mapCmd = formatGsdSlash('map-codebase', resolveRuntime(projectDir)); + // drift.cjs is a pure library — it must never read env/config. The + // caller (verify.cmdVerifyCodebaseDrift) resolves the runtime once and + // passes it in via input.runtime so emitted commands match the project + // the caller is targeting, not the current process directory. + const { formatGsdSlash } = require('./runtime-slash.cjs'); + const mapCmd = formatGsdSlash('map-codebase', runtime || 'claude'); lines.push( `Run ${mapCmd} --paths ${affectedPaths.join(',')} to refresh planning context.`, ); diff --git a/get-shit-done/bin/lib/gsd2-import.cjs b/get-shit-done/bin/lib/gsd2-import.cjs index cb7bcd5cc..f00220dfe 100644 --- a/get-shit-done/bin/lib/gsd2-import.cjs +++ b/get-shit-done/bin/lib/gsd2-import.cjs @@ -472,7 +472,9 @@ function cmdFromGsd2(args, cwd, raw) { const gsd2Data = parseGsd2(gsdDir); const artifacts = buildPlanningArtifacts(gsd2Data); - const preview = buildPreview(gsd2Data, artifacts, cwd); + // Use projectDir (resolved from --path) — not the process cwd — so the + // preview command targets the project actually being imported (#3584). + const preview = buildPreview(gsd2Data, artifacts, projectDir); if (dryRun) { return output({ success: true, dryRun: true, preview }, raw); diff --git a/get-shit-done/bin/lib/runtime-slash.cjs b/get-shit-done/bin/lib/runtime-slash.cjs index d1ba1ecd1..30cda98f6 100644 --- a/get-shit-done/bin/lib/runtime-slash.cjs +++ b/get-shit-done/bin/lib/runtime-slash.cjs @@ -28,7 +28,11 @@ function formatGsdSlash(commandName, runtime) { const stripped = commandName.replace(/^[/$]?gsd[-:]/i, ''); // If the regex matched nothing (no prefix), the input is already a bare name. const bare = stripped === commandName ? commandName : stripped; - if (bare === '') return commandName; + // Defensive: a degenerate input like `/gsd:`, `gsd-`, or whitespace-only + // normalizes to empty. Returning the original colon-form would re-emit the + // deprecated shape that this module exists to suppress (#3584). Return an + // empty string so callers see "no command" rather than the broken input. + if (bare === '' || bare.trim() === '') return ''; // Split on the first whitespace so only the command token is rewritten — // anything after the first space is caller-supplied arguments (phase diff --git a/get-shit-done/bin/lib/verify.cjs b/get-shit-done/bin/lib/verify.cjs index ced96f4bd..d50884aff 100644 --- a/get-shit-done/bin/lib/verify.cjs +++ b/get-shit-done/bin/lib/verify.cjs @@ -1358,7 +1358,9 @@ function cmdVerifyCodebaseDrift(cwd, raw) { structureMd, threshold, action, - projectDir: cwd, + // #3584: keep drift.cjs a pure library — resolve the runtime here and + // pass the literal name in so drift never touches env/config itself. + runtime: resolveRuntime(cwd), }); emit({ diff --git a/tests/bug-3584-runtime-slash-formatter.test.cjs b/tests/bug-3584-runtime-slash-formatter.test.cjs index 92c6e7076..e917e2fa2 100644 --- a/tests/bug-3584-runtime-slash-formatter.test.cjs +++ b/tests/bug-3584-runtime-slash-formatter.test.cjs @@ -125,14 +125,23 @@ describe('formatGsdSlash — runtime-aware slash command formatter', () => { assert.strictEqual(formatGsdSlash('', 'claude'), ''); }); - test('whitespace-only string is treated as empty after prefix strip', () => { - // Whitespace is preserved as-is in the output rather than mangled, - // so callers can detect "no real command" instead of receiving "/gsd- ". - const result = formatGsdSlash(' ', 'claude'); - assert.ok( - result === ' ' || result === '/gsd- ', - `whitespace input should round-trip or be predictably wrapped, got ${JSON.stringify(result)}`, - ); + test('whitespace-only string returns empty string (no spurious /gsd- emission)', () => { + assert.strictEqual(formatGsdSlash(' ', 'claude'), ''); + assert.strictEqual(formatGsdSlash('\t\n', 'codex'), ''); + }); + + test('degenerate prefix-only input returns empty (does NOT re-emit colon form)', () => { + // Regression guard for the CodeRabbit finding on the original PR: + // a previous fallback returned `commandName` unchanged when the bare + // tail was empty, which re-introduced the deprecated `/gsd:` shape for + // inputs like `/gsd:`, `gsd:`, or `gsd-`. The formatter must never + // emit the colon form — return empty so callers detect "no command" + // instead of receiving an unroutable string. + assert.strictEqual(formatGsdSlash('/gsd:', 'claude'), ''); + assert.strictEqual(formatGsdSlash('gsd:', 'claude'), ''); + assert.strictEqual(formatGsdSlash('gsd-', 'claude'), ''); + assert.strictEqual(formatGsdSlash('/gsd-', 'codex'), ''); + assert.strictEqual(formatGsdSlash('$gsd-', 'codex'), ''); }); test('commands with arguments preserve the argument tail', () => { diff --git a/tests/copilot-install.test.cjs b/tests/copilot-install.test.cjs index 8bc73cc08..40abb3eb0 100644 --- a/tests/copilot-install.test.cjs +++ b/tests/copilot-install.test.cjs @@ -1,8 +1,12 @@ // allow-test-rule: integration-test-input -// Reads verify.cjs as real test fixture input to the convertClaudeToCopilotContent() -// function under test. The file is not inspected for string presence; it is the -// input whose *transformation* is being asserted. This is the correct level of testing -// for format-conversion functions where a real source file is the canonical test case. +// Reads shipped source files (commands/gsd/*.md, agents/*.md, bin/install.js) as +// real test fixture input for installer/converter functions like +// convertClaudeToCopilotContent() and the install.js plumbing. Those files are +// not inspected for string presence; they are inputs whose *transformation* or +// installation behavior is being asserted. The converter-purity test on +// bin/lib/*.cjs uses a synthetic input string instead (per #3584: +// runtime-slash.cjs eliminated literal /gsd: refs from runtime CJS, so reading +// verify.cjs is no longer a meaningful fixture for testing the converter). /** * GSD Tools Tests - Copilot Install Plumbing