fix(3584): address codex-review findings on PR #3606
Codex review surfaced five issues in the initial slash-formatter PR — three MED
and two LOW. All five are addressed:
1. (MED) formatter corrupted argument tails under codex runtime. Splitting on
the first whitespace and lowercasing only the command token preserves
path-like arguments (`Map-Codebase --paths C:\\Users\\Me\\Project`) and
case-sensitive flag values. (`runtime-slash.cjs`)
2. (MED) profile-output `cmdGenerateDevPreferences` emitted a hardcoded
`command_name: '/gsd-dev-preferences'`. Replaced with
`formatGsdSlash('dev-preferences', resolveRuntime(cwd))` so the structured
result honors codex/skills runtime distinction. (`profile-output.cjs`)
3. (MED) `drift.detectDrift` → `buildMessage` called `resolveRuntime(null)`,
ignoring a project's `.planning/config.json` `runtime` setting when
`GSD_RUNTIME` env var was absent. Threaded `projectDir` through
`detectDrift({projectDir})` → `buildMessage(..., projectDir)` →
`resolveRuntime(projectDir)`. `verify.cmdVerifyCodebaseDrift` now passes
`cwd` into the detect call. (`drift.cjs`, `verify.cjs`)
4. (LOW) `gsd2-import.buildPreview` had the same env-vs-config issue. Threaded
`cwd` through `buildPreview(..., projectDir)` for parity. (`gsd2-import.cjs`)
5. (LOW) The codex emitter test only asserted absence of `/gsd:` — a regression
to `/gsd-` (skills) form in codex output would have passed undetected.
Added positive assertions that every gsd-referencing fix string under
`GSD_RUNTIME=codex` contains `$gsd-` and contains neither `/gsd-` nor
`/gsd:`. Also added formatter unit tests pinning the argument-tail
preservation contract for both hyphen and codex runtimes.
Full suite: 9365/9365 pass (+2 new). Lint clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -190,7 +190,7 @@ function detectDrift(input) {
|
||||
if (action === 'auto-remap') {
|
||||
spawnMapper = true;
|
||||
}
|
||||
message = buildMessage(elements, affectedPaths, action);
|
||||
message = buildMessage(elements, affectedPaths, action, input.projectDir);
|
||||
}
|
||||
|
||||
return {
|
||||
@@ -228,7 +228,7 @@ function skipped(reason) {
|
||||
};
|
||||
}
|
||||
|
||||
function buildMessage(elements, affectedPaths, action) {
|
||||
function buildMessage(elements, affectedPaths, action, projectDir) {
|
||||
const byCat = {};
|
||||
for (const e of elements) {
|
||||
(byCat[e.category] ||= []).push(e.path);
|
||||
@@ -254,7 +254,7 @@ function buildMessage(elements, affectedPaths, action) {
|
||||
lines.push(`Auto-remap scheduled for paths: ${affectedPaths.join(', ')}`);
|
||||
} else {
|
||||
const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs');
|
||||
const mapCmd = formatGsdSlash('map-codebase', resolveRuntime(null));
|
||||
const mapCmd = formatGsdSlash('map-codebase', resolveRuntime(projectDir));
|
||||
lines.push(
|
||||
`Run ${mapCmd} --paths ${affectedPaths.join(',')} to refresh planning context.`,
|
||||
);
|
||||
|
||||
@@ -402,7 +402,7 @@ function buildPlanningArtifacts(gsd2Data) {
|
||||
/**
|
||||
* Format a dry-run preview string for display before writing.
|
||||
*/
|
||||
function buildPreview(gsd2Data, artifacts) {
|
||||
function buildPreview(gsd2Data, artifacts, projectDir) {
|
||||
const lines = ['Preview — files that will be created in .planning/:'];
|
||||
|
||||
for (const rel of artifacts.keys()) {
|
||||
@@ -422,7 +422,7 @@ function buildPreview(gsd2Data, artifacts) {
|
||||
lines.push('Cannot migrate automatically:');
|
||||
lines.push(' - GSD-2 cost/token ledger (no v1 equivalent)');
|
||||
const { formatGsdSlash, resolveRuntime } = require('./runtime-slash.cjs');
|
||||
lines.push(` - GSD-2 database state (rebuilt from files on first ${formatGsdSlash('health', resolveRuntime(null))})`);
|
||||
lines.push(` - GSD-2 database state (rebuilt from files on first ${formatGsdSlash('health', resolveRuntime(projectDir))})`);
|
||||
lines.push(' - VS Code extension state');
|
||||
|
||||
return lines.join('\n');
|
||||
@@ -472,7 +472,7 @@ function cmdFromGsd2(args, cwd, raw) {
|
||||
|
||||
const gsd2Data = parseGsd2(gsdDir);
|
||||
const artifacts = buildPlanningArtifacts(gsd2Data);
|
||||
const preview = buildPreview(gsd2Data, artifacts);
|
||||
const preview = buildPreview(gsd2Data, artifacts, cwd);
|
||||
|
||||
if (dryRun) {
|
||||
return output({ success: true, dryRun: true, preview }, raw);
|
||||
|
||||
@@ -829,7 +829,7 @@ function cmdGenerateDevPreferences(cwd, options, raw) {
|
||||
|
||||
const result = {
|
||||
command_path: outputPath,
|
||||
command_name: '/gsd-dev-preferences',
|
||||
command_name: formatGsdSlash('dev-preferences', resolveRuntime(cwd)),
|
||||
dimensions_included: dimensionsIncluded,
|
||||
source: analysis.data_source || 'session_analysis',
|
||||
};
|
||||
|
||||
@@ -30,14 +30,23 @@ function formatGsdSlash(commandName, runtime) {
|
||||
const bare = stripped === commandName ? commandName : stripped;
|
||||
if (bare === '') return commandName;
|
||||
|
||||
// Split on the first whitespace so only the command token is rewritten —
|
||||
// anything after the first space is caller-supplied arguments (phase
|
||||
// numbers, --flags, --paths C:\\Users\\Me, etc.) that must round-trip
|
||||
// untouched. Codex lowercases only the command token; preserving the
|
||||
// argument tail prevents path/flag corruption on case-sensitive systems.
|
||||
const wsMatch = bare.match(/^(\S+)(\s[\s\S]*)?$/);
|
||||
const token = wsMatch ? wsMatch[1] : bare;
|
||||
const tail = wsMatch && wsMatch[2] ? wsMatch[2] : '';
|
||||
|
||||
const rt = String(runtime || 'claude').toLowerCase();
|
||||
if (rt === 'codex') {
|
||||
// Codex skills are invoked as $gsd-<cmd> (shell-var syntax). Lowercased
|
||||
// because shell-var identifiers in the surrounding prose are conventionally
|
||||
// Codex skills are invoked as $gsd-<cmd> (shell-var syntax). The command
|
||||
// token is lowercased because shell-var identifiers are conventionally
|
||||
// lowercase; matches the convertCodexSlash() projection in bin/install.js.
|
||||
return `$gsd-${bare.toLowerCase()}`;
|
||||
return `$gsd-${token.toLowerCase()}${tail}`;
|
||||
}
|
||||
return `/gsd-${bare}`;
|
||||
return `/gsd-${token}${tail}`;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -1358,6 +1358,7 @@ function cmdVerifyCodebaseDrift(cwd, raw) {
|
||||
structureMd,
|
||||
threshold,
|
||||
action,
|
||||
projectDir: cwd,
|
||||
});
|
||||
|
||||
emit({
|
||||
|
||||
@@ -260,7 +260,7 @@ describe('bug-3584: validate context recommendation uses hyphen form', () => {
|
||||
});
|
||||
|
||||
describe('bug-3584: validate health uses formatter for codex runtime too', () => {
|
||||
test('validate health under codex emits $gsd-<cmd> in fix strings', (t) => {
|
||||
test('validate health under codex emits $gsd-<cmd> in fix strings (positive assertion)', (t) => {
|
||||
const tmpDir = createTempDir();
|
||||
t.after(() => cleanup(tmpDir));
|
||||
|
||||
@@ -281,20 +281,30 @@ describe('bug-3584: validate health uses formatter for codex runtime too', () =>
|
||||
.concat(payload.warnings || [])
|
||||
.concat(payload.info || []);
|
||||
|
||||
const fixesWithSlash = allIssues
|
||||
// Collect fixes that mention any gsd slash-command form so we can lock
|
||||
// both the absence of legacy forms AND the presence of the codex shape.
|
||||
const fixesWithGsdRef = allIssues
|
||||
.map((i) => i.fix)
|
||||
.filter((f) => typeof f === 'string' && /gsd-/.test(f));
|
||||
.filter((f) => typeof f === 'string' && /(?:\$|\/)gsd[-:]/.test(f));
|
||||
|
||||
// For codex, hyphen-with-slash form `/gsd-<cmd>` should be replaced by
|
||||
// shell-var `$gsd-<cmd>`. Skip the assertion if no relevant fix was
|
||||
// produced (test only asserts the contract when something is emitted).
|
||||
for (const fix of fixesWithSlash) {
|
||||
assert.ok(
|
||||
fixesWithGsdRef.length > 0,
|
||||
'validate health on a bare tmpdir must produce at least one fix hint referencing a gsd command',
|
||||
);
|
||||
|
||||
for (const fix of fixesWithGsdRef) {
|
||||
assert.ok(
|
||||
fix.includes('$gsd-'),
|
||||
`codex validate health fix must use shell-var $gsd- form, got ${JSON.stringify(fix)}`,
|
||||
);
|
||||
assert.ok(
|
||||
!fix.includes('/gsd:'),
|
||||
`codex validate health fix must not contain /gsd: colon form, got ${JSON.stringify(fix)}`,
|
||||
);
|
||||
// Codex emits $gsd- — any /gsd- in the fix means the formatter wasn't applied.
|
||||
// (We don't have a hard count guarantee — a fix can also reference a plain url etc.)
|
||||
assert.ok(
|
||||
!fix.includes('/gsd-'),
|
||||
`codex validate health fix must not contain /gsd- (skills) form, got ${JSON.stringify(fix)}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -148,6 +148,29 @@ describe('formatGsdSlash — runtime-aware slash command formatter', () => {
|
||||
'/gsd-execute-phase 03',
|
||||
);
|
||||
});
|
||||
|
||||
test('codex form lowercases only the command token, not the argument tail', () => {
|
||||
// Regression for codex review finding: a previous implementation
|
||||
// lowercased the full input including arguments, which would corrupt
|
||||
// Windows paths and case-sensitive flag values passed as args.
|
||||
assert.strictEqual(
|
||||
formatGsdSlash('Map-Codebase --paths C:\\Users\\Me\\Project', 'codex'),
|
||||
'$gsd-map-codebase --paths C:\\Users\\Me\\Project',
|
||||
);
|
||||
assert.strictEqual(
|
||||
formatGsdSlash('execute-phase 03 --Name FooBar', 'codex'),
|
||||
'$gsd-execute-phase 03 --Name FooBar',
|
||||
);
|
||||
});
|
||||
|
||||
test('hyphen form preserves token case (it does not get lowercased)', () => {
|
||||
// Symmetry with codex: only codex lowercases the token. Hyphen-form
|
||||
// runtimes preserve whatever case the caller supplied for the token.
|
||||
assert.strictEqual(
|
||||
formatGsdSlash('Plan-Phase 03', 'claude'),
|
||||
'/gsd-Plan-Phase 03',
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user