fix(3785): case-insensitive depends_on resolution in phase resolver (#88)
* fix(3785): case-insensitive depends_on resolution in phase resolver planMap, canonicalToId, and shortFormToId in phasePlanIndex used strict Map.has() with no case normalization. A depends_on ref in mixed/lowercase against an uppercase-suffix plan ID (e.g. '20-01-auth' → '20-01-Auth') dropped the DAG edge, assigning the dependent plan to wave 1 instead of wave 2. Fix: normalize all keys and lookup values to lowercase so the three-tier resolution is case-insensitive. Adds regression test (#3785). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(changeset): add PR 3798 changelog fragment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3785): detect case-fold collisions; apply lowercase-both to CJS path - Add collision guard in both sdk/src/query/phase.ts (phasePlanIndex) and get-shit-done/bin/lib/phase.cjs (cmdPhasePlanIndex): when two plan IDs in the same phase are identical after toLowerCase(), throw/error immediately with a clear message naming both files instead of silently overwriting one in planMap and misrouting depends_on edges. - Apply the same lowercase-both normalization (#3785) to cmdPhasePlanIndex in phase.cjs, which was missing from the original PR — planMap and canonicalToId keys are now lowercased on write; dep strings are lowercased before lookup. - Add regression tests to tests/phase.test.cjs: case-insensitive resolution test (runs on all platforms) and collision-detection test (skipped on macOS/Windows where FS is case-insensitive). - Update changeset to describe both the SDK and CJS fixes. Identified via Codex adversarial review of PR #3798. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3785): address review — KNOWN GAP comment for CJS shortFormToId, canonical-casing tests, depends_on output normalization - F1: Reword changeset for accuracy (plannerID drift trigger; two-tier CJS gap honest). Add KNOWN GAP comment in phase.cjs before Kahn's loop noting CJS lacks shortFormToId (tracked as follow-up parity gap, out of scope for #3785). - F2: Add strict planA.id === '20-01-Auth' assertions in both SDK (vitest) and CJS (node --test) tests — a future regression that silently lowercases stored IDs would now fail the test. - F3: Normalize depends_on output to canonical plan IDs in both SDK phase.ts and CJS phase.cjs. User-typed '20-01-auth' in depends_on resolves to '20-01-Auth' in output via planMap lookup. Add planB.depends_on === ['20-01-Auth'] assertions in both test suites. - F5: Add seenLower guard-scope comment (full-ID collisions only; shared-prefix collisions handled by first-write-wins from sorted planFiles). - F6: Add ASCII-safe toLowerCase comment at first call site in both SDK and CJS. - F7: Add intentional-separation comment on seenLower vs planMap in both SDK and CJS. Reviewers: gsd-code-reviewer (MN-01/NT-1) + sonnet adversarial. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3785): cover case-insensitive depends_on resolution branches Add 4 focused test cases exercising branches introduced by #3785: - All-uppercase depends_on ref resolving to lowercase plan ID via planMap - External cross-phase dep preserved as-is in Pass 3 output (planMap miss) - Mixed-case short canonical prefix resolving via canonicalToId - Plans with undefined/empty depends_on emit empty array correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -690,4 +690,95 @@ describe('phasePlanIndex', () => {
|
||||
// A clear, dedicated warning naming the unresolved reference must surface.
|
||||
expect(warnings.some(w => /unresolved/i.test(w) && w.includes('does-not-exist'))).toBe(true);
|
||||
});
|
||||
|
||||
it('#3785: depends_on resolution is case-insensitive — mixed-case plan ID resolves correctly', async () => {
|
||||
// Regression: planMap / canonicalToId / shortFormToId used strict Map.has() with no
|
||||
// case normalization. A depends_on ref in lowercase against an uppercase-suffix plan
|
||||
// ID dropped the DAG edge, causing the dependent plan to land in wave 1 instead of
|
||||
// wave 2 (wrong ordering).
|
||||
const phase20 = join(tmpDir, '.planning', 'phases', '20-case-insensitive');
|
||||
await mkdir(phase20, { recursive: true });
|
||||
// Plan A: filename uses uppercase suffix (e.g. user named it "Phase-2-FOO")
|
||||
await writeFile(join(phase20, '20-01-Auth-PLAN.md'), [
|
||||
'---',
|
||||
'phase: 20',
|
||||
'plan: 01',
|
||||
'wave: 1',
|
||||
'autonomous: true',
|
||||
'depends_on: []',
|
||||
'---',
|
||||
'<objective>Plan A — uppercase suffix in filename.</objective>',
|
||||
].join('\n'));
|
||||
// Plan B: depends_on references Plan A using a different case than the plan ID
|
||||
await writeFile(join(phase20, '20-02-PLAN.md'), [
|
||||
'---',
|
||||
'phase: 20',
|
||||
'plan: 02',
|
||||
'wave: 2',
|
||||
'autonomous: true',
|
||||
'depends_on:',
|
||||
' - 20-01-auth',
|
||||
'---',
|
||||
'<objective>Plan B — lowercase dep ref to mixed-case plan A.</objective>',
|
||||
].join('\n'));
|
||||
|
||||
const result = await phasePlanIndex(['20'], tmpDir);
|
||||
const data = result.data as Record<string, unknown>;
|
||||
const plans = data.plans as Array<Record<string, unknown>>;
|
||||
const waves = data.waves as Record<string, string[]>;
|
||||
const warnings = (data.warnings as string[] | undefined) ?? [];
|
||||
|
||||
const planA = plans.find(p => (p.id as string).startsWith('20-01'));
|
||||
const planB = plans.find(p => p.id === '20-02');
|
||||
expect(planA).toBeDefined();
|
||||
expect(planB).toBeDefined();
|
||||
|
||||
// Lock canonical casing: plan ID must be preserved as-is from the filename, not lowercased.
|
||||
expect(planA!.id).toBe('20-01-Auth');
|
||||
|
||||
// The DAG edge must resolve: B must be in a later wave than A.
|
||||
expect(planA!.wave).toBeLessThan(planB!.wave as number);
|
||||
// Structurally: A in wave 1, B in wave 2.
|
||||
expect(waves['1']).toBeDefined();
|
||||
expect(waves['2']).toBeDefined();
|
||||
expect((waves['1'] as string[]).some(id => (id as string).startsWith('20-01'))).toBe(true);
|
||||
expect(waves['2']).toContain('20-02');
|
||||
// depends_on output must use canonical plan ID, not the user-typed casing.
|
||||
expect(planB!.depends_on).toEqual(['20-01-Auth']);
|
||||
// No unresolved-dep warning should be emitted.
|
||||
expect(warnings.some(w => /unresolved/i.test(w))).toBe(false);
|
||||
});
|
||||
|
||||
// This test can only run on Linux where the filesystem is case-sensitive.
|
||||
// On macOS/Windows (case-insensitive FS), writing both files silently collapses
|
||||
// them to one file, so the collision scenario cannot be triggered via disk.
|
||||
const itLinuxOnly = process.platform === 'linux' ? it : it.skip;
|
||||
itLinuxOnly('#3785 adversarial: case-insensitive collision — two plan IDs that differ only by case throw a clear error', async () => {
|
||||
// Regression guard: if two plan files produce IDs that are identical when
|
||||
// lowercased, planMap would silently overwrite one entry and route depends_on
|
||||
// edges to whichever plan survived. This must fail fast instead.
|
||||
const phase21 = join(tmpDir, '.planning', 'phases', '21-collision');
|
||||
await mkdir(phase21, { recursive: true });
|
||||
// These two filenames produce IDs '21-01-auth' and '21-01-Auth' — same when lowercased.
|
||||
await writeFile(join(phase21, '21-01-auth-PLAN.md'), [
|
||||
'---',
|
||||
'phase: 21',
|
||||
'plan: 01',
|
||||
'autonomous: true',
|
||||
'depends_on: []',
|
||||
'---',
|
||||
'<objective>Plan lowercase.</objective>',
|
||||
].join('\n'));
|
||||
await writeFile(join(phase21, '21-01-Auth-PLAN.md'), [
|
||||
'---',
|
||||
'phase: 21',
|
||||
'plan: 01',
|
||||
'autonomous: true',
|
||||
'depends_on: []',
|
||||
'---',
|
||||
'<objective>Plan uppercase.</objective>',
|
||||
].join('\n'));
|
||||
|
||||
await expect(phasePlanIndex(['21'], tmpDir)).rejects.toThrow(/collision/i);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -563,24 +563,55 @@ export const phasePlanIndex: QueryHandler = async (args, projectDir, workstream)
|
||||
|
||||
// ── Pass 2: topological level assignment via depends_on DAG ──────────────
|
||||
|
||||
// Guard: detect case-insensitive key collisions before building dependency
|
||||
// maps. Two plan IDs that differ only by case would silently overwrite each
|
||||
// other in planMap, routing depends_on edges to whichever plan survived last.
|
||||
// This is a configuration error — fail fast with the conflicting IDs so the
|
||||
// author can rename one file. (#3785 follow-up from adversarial review)
|
||||
//
|
||||
// This guard catches case-fold collisions on full plan IDs.
|
||||
// Shared-numeric-prefix collisions (e.g. '20-01-Auth' and '20-01' both
|
||||
// producing canonical '20-01') are resolved by first-write-wins ordering
|
||||
// from sorted planFiles — not explicitly guarded here.
|
||||
// seenLower is intentionally separate from planMap — it exists only to detect
|
||||
// collisions before planMap is built, so the error fires before any Map
|
||||
// entry silently overwrites another.
|
||||
const seenLower = new Map<string, string>(); // lowercase key → original id
|
||||
for (const p of rawPlans) {
|
||||
// ASCII plan IDs only — toLowerCase() is correct and locale-safe here.
|
||||
const lower = p.id.toLowerCase();
|
||||
const existing = seenLower.get(lower);
|
||||
if (existing !== undefined) {
|
||||
throw new GSDError(
|
||||
`depends_on index collision in phase ${normalized}: plan IDs '${existing}' and '${p.id}' are identical when case-folded. Rename one file to avoid ambiguous dependency resolution.`,
|
||||
ErrorClassification.Execution,
|
||||
);
|
||||
}
|
||||
seenLower.set(lower, p.id);
|
||||
}
|
||||
|
||||
// Build a map from plan ID → RawPlan for fast lookup.
|
||||
// Deps that reference plans outside this phase are silently ignored (treated
|
||||
// as already-satisfied external deps — the plan becomes a source node).
|
||||
const planMap = new Map<string, RawPlan>(rawPlans.map(p => [p.id, p]));
|
||||
// Keys are lowercased so that depends_on refs with different casing still
|
||||
// resolve to the correct plan (#3785: case-insensitive identifier resolution).
|
||||
const planMap = new Map<string, RawPlan>(rawPlans.map(p => [p.id.toLowerCase(), p]));
|
||||
// Secondary index: canonical prefix → full plan ID, so depends_on: ['03-01'] resolves
|
||||
// to '03-01-auth-hardening-PLAN.md'-derived ID '03-01-auth-hardening' (k015).
|
||||
const canonicalToId = new Map<string, string>(rawPlans.map(p => [extractCanonicalPlanId(p.id), p.id]));
|
||||
// Keyed lowercase for the same case-insensitive reason (#3785).
|
||||
const canonicalToId = new Map<string, string>(rawPlans.map(p => [extractCanonicalPlanId(p.id).toLowerCase(), p.id]));
|
||||
// Tertiary index: same-phase short-form ('01') → full plan ID, derived from each plan's
|
||||
// canonical '<phase>-<plan>' by splitting on the LAST '-'. The phase segment may
|
||||
// contain dots (e.g. '99.9') or letters (e.g. '02A'); only the trailing '-NN' is the
|
||||
// short form. Same-phase plans share a phase prefix so '01' is unambiguous within a
|
||||
// single phase-plan-index call. (#3488)
|
||||
// Keyed lowercase for the same case-insensitive reason (#3785).
|
||||
const shortFormToId = new Map<string, string>();
|
||||
for (const p of rawPlans) {
|
||||
const canonical = extractCanonicalPlanId(p.id);
|
||||
const lastDash = canonical.lastIndexOf('-');
|
||||
if (lastDash > 0 && lastDash < canonical.length - 1) {
|
||||
const shortForm = canonical.slice(lastDash + 1);
|
||||
const shortForm = canonical.slice(lastDash + 1).toLowerCase();
|
||||
// First write wins — preserve deterministic ordering from sorted planFiles.
|
||||
if (!shortFormToId.has(shortForm)) {
|
||||
shortFormToId.set(shortForm, p.id);
|
||||
@@ -602,13 +633,16 @@ export const phasePlanIndex: QueryHandler = async (args, projectDir, workstream)
|
||||
// and same-phase short-form ('01') forms. The short-form lookup (#3488)
|
||||
// is keyed off the plan-id suffix so it works for integer ('99'), letter
|
||||
// ('02A'), and decimal ('99.9') phase IDs alike.
|
||||
// All lookups are lowercased so mixed-case depends_on refs resolve
|
||||
// correctly regardless of the case used in the plan filename (#3785).
|
||||
const depLower = dep.toLowerCase();
|
||||
let resolvedDep: string | undefined;
|
||||
if (planMap.has(dep)) {
|
||||
resolvedDep = dep;
|
||||
} else if (canonicalToId.has(dep)) {
|
||||
resolvedDep = canonicalToId.get(dep);
|
||||
} else if (shortFormToId.has(dep)) {
|
||||
resolvedDep = shortFormToId.get(dep);
|
||||
if (planMap.has(depLower)) {
|
||||
resolvedDep = planMap.get(depLower)!.id;
|
||||
} else if (canonicalToId.has(depLower)) {
|
||||
resolvedDep = canonicalToId.get(depLower);
|
||||
} else if (shortFormToId.has(depLower)) {
|
||||
resolvedDep = shortFormToId.get(depLower);
|
||||
}
|
||||
if (!resolvedDep) {
|
||||
// Looks like an in-phase short-form / canonical reference that didn't resolve.
|
||||
@@ -703,7 +737,13 @@ export const phasePlanIndex: QueryHandler = async (args, projectDir, workstream)
|
||||
const plan: Record<string, unknown> = {
|
||||
id: raw.id,
|
||||
wave: effectiveWave,
|
||||
depends_on: raw.dependsOn,
|
||||
// Resolve each user-typed dep to its canonical plan ID (preserving on-disk casing)
|
||||
// so the output never reflects the user's case typo. Unresolved deps (external
|
||||
// phase refs) are kept as-is since planMap only contains plans in this phase.
|
||||
depends_on: raw.dependsOn.map(dep => {
|
||||
const lower = String(dep).toLowerCase();
|
||||
return planMap.get(lower)?.id ?? dep;
|
||||
}),
|
||||
autonomous: raw.autonomous,
|
||||
objective: raw.objective,
|
||||
files_modified: raw.filesModified,
|
||||
|
||||
Reference in New Issue
Block a user