fix(3584): address coderabbit findings on PR #3606
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 <dir>` 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) <noreply@anthropic.com>
This commit is contained in:
@@ -116,6 +116,9 @@ function isPathMapped(file, structureMd) {
|
|||||||
* @param {string|null|undefined} input.structureMd - contents of STRUCTURE.md
|
* @param {string|null|undefined} input.structureMd - contents of STRUCTURE.md
|
||||||
* @param {number} [input.threshold=3] - min number of drift elements that triggers action
|
* @param {number} [input.threshold=3] - min number of drift elements that triggers action
|
||||||
* @param {'warn'|'auto-remap'} [input.action='warn']
|
* @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
|
* @returns {object} result
|
||||||
*/
|
*/
|
||||||
function detectDrift(input) {
|
function detectDrift(input) {
|
||||||
@@ -190,7 +193,7 @@ function detectDrift(input) {
|
|||||||
if (action === 'auto-remap') {
|
if (action === 'auto-remap') {
|
||||||
spawnMapper = true;
|
spawnMapper = true;
|
||||||
}
|
}
|
||||||
message = buildMessage(elements, affectedPaths, action, input.projectDir);
|
message = buildMessage(elements, affectedPaths, action, input.runtime);
|
||||||
}
|
}
|
||||||
|
|
||||||
return {
|
return {
|
||||||
@@ -228,7 +231,7 @@ function skipped(reason) {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
function buildMessage(elements, affectedPaths, action, projectDir) {
|
function buildMessage(elements, affectedPaths, action, runtime) {
|
||||||
const byCat = {};
|
const byCat = {};
|
||||||
for (const e of elements) {
|
for (const e of elements) {
|
||||||
(byCat[e.category] ||= []).push(e.path);
|
(byCat[e.category] ||= []).push(e.path);
|
||||||
@@ -253,8 +256,12 @@ function buildMessage(elements, affectedPaths, action, projectDir) {
|
|||||||
if (action === 'auto-remap') {
|
if (action === 'auto-remap') {
|
||||||
lines.push(`Auto-remap scheduled for paths: ${affectedPaths.join(', ')}`);
|
lines.push(`Auto-remap scheduled for paths: ${affectedPaths.join(', ')}`);
|
||||||
} else {
|
} else {
|
||||||
const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs');
|
// drift.cjs is a pure library — it must never read env/config. The
|
||||||
const mapCmd = formatGsdSlash('map-codebase', resolveRuntime(projectDir));
|
// 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(
|
lines.push(
|
||||||
`Run ${mapCmd} --paths ${affectedPaths.join(',')} to refresh planning context.`,
|
`Run ${mapCmd} --paths ${affectedPaths.join(',')} to refresh planning context.`,
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -472,7 +472,9 @@ function cmdFromGsd2(args, cwd, raw) {
|
|||||||
|
|
||||||
const gsd2Data = parseGsd2(gsdDir);
|
const gsd2Data = parseGsd2(gsdDir);
|
||||||
const artifacts = buildPlanningArtifacts(gsd2Data);
|
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) {
|
if (dryRun) {
|
||||||
return output({ success: true, dryRun: true, preview }, raw);
|
return output({ success: true, dryRun: true, preview }, raw);
|
||||||
|
|||||||
@@ -28,7 +28,11 @@ function formatGsdSlash(commandName, runtime) {
|
|||||||
const stripped = commandName.replace(/^[/$]?gsd[-:]/i, '');
|
const stripped = commandName.replace(/^[/$]?gsd[-:]/i, '');
|
||||||
// If the regex matched nothing (no prefix), the input is already a bare name.
|
// If the regex matched nothing (no prefix), the input is already a bare name.
|
||||||
const bare = stripped === commandName ? commandName : stripped;
|
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 —
|
// Split on the first whitespace so only the command token is rewritten —
|
||||||
// anything after the first space is caller-supplied arguments (phase
|
// anything after the first space is caller-supplied arguments (phase
|
||||||
|
|||||||
@@ -1358,7 +1358,9 @@ function cmdVerifyCodebaseDrift(cwd, raw) {
|
|||||||
structureMd,
|
structureMd,
|
||||||
threshold,
|
threshold,
|
||||||
action,
|
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({
|
emit({
|
||||||
|
|||||||
@@ -125,14 +125,23 @@ describe('formatGsdSlash — runtime-aware slash command formatter', () => {
|
|||||||
assert.strictEqual(formatGsdSlash('', 'claude'), '');
|
assert.strictEqual(formatGsdSlash('', 'claude'), '');
|
||||||
});
|
});
|
||||||
|
|
||||||
test('whitespace-only string is treated as empty after prefix strip', () => {
|
test('whitespace-only string returns empty string (no spurious /gsd- emission)', () => {
|
||||||
// Whitespace is preserved as-is in the output rather than mangled,
|
assert.strictEqual(formatGsdSlash(' ', 'claude'), '');
|
||||||
// so callers can detect "no real command" instead of receiving "/gsd- ".
|
assert.strictEqual(formatGsdSlash('\t\n', 'codex'), '');
|
||||||
const result = formatGsdSlash(' ', 'claude');
|
});
|
||||||
assert.ok(
|
|
||||||
result === ' ' || result === '/gsd- ',
|
test('degenerate prefix-only input returns empty (does NOT re-emit colon form)', () => {
|
||||||
`whitespace input should round-trip or be predictably wrapped, got ${JSON.stringify(result)}`,
|
// 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', () => {
|
test('commands with arguments preserve the argument tail', () => {
|
||||||
|
|||||||
@@ -1,8 +1,12 @@
|
|||||||
// allow-test-rule: integration-test-input
|
// allow-test-rule: integration-test-input
|
||||||
// Reads verify.cjs as real test fixture input to the convertClaudeToCopilotContent()
|
// Reads shipped source files (commands/gsd/*.md, agents/*.md, bin/install.js) as
|
||||||
// function under test. The file is not inspected for string presence; it is the
|
// real test fixture input for installer/converter functions like
|
||||||
// input whose *transformation* is being asserted. This is the correct level of testing
|
// convertClaudeToCopilotContent() and the install.js plumbing. Those files are
|
||||||
// for format-conversion functions where a real source file is the canonical test case.
|
// 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
|
* GSD Tools Tests - Copilot Install Plumbing
|
||||||
|
|||||||
Reference in New Issue
Block a user