fix(3739): gap-checker now detects padded-prefix CONTEXT.md (#3764)
* test(3739): add RED tests for padded-prefix CONTEXT.md gap-checker miss Covers bare and padded (01-CONTEXT.md, 02.1-CONTEXT.md) forms, an uncovered-decision counter-test, and unit tests for the upcoming findContextMdIn() helper. All 6 new tests fail before the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): extract findContextMdIn() helper; fix gap-checker bare lookup gap-checker.cjs:136 used a bare path.join(absPhaseDir, 'CONTEXT.md') that silently returned '' for any phase using the padded-prefix convention (01-CONTEXT.md, 02.1-CONTEXT.md, etc.). Extract findContextMdIn(absDir) to planning-workspace.cjs — the module already imported by gap-checker, init, roadmap, and core — and wire gap-checker.cjs to call it instead of the bare lookup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(3739): replace inline dual-form predicate with findContextMdIn() at all 4 remaining sites The dual-form predicate `f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'` existed verbatim at 5 sites across init.cjs (×3), roadmap.cjs, and core.cjs — Rule of Three mandates extraction at ≥3 sites. All 4 remaining call sites now delegate to findContextMdIn() from planning-workspace.cjs. No behaviour change; all existing tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(3739): update changeset to reference PR #3764 * fix(3739): findContextMdIn prefers bare CONTEXT.md over padded form (deterministic precedence) `.find()` with `f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'` returned the first match in `readdirSync` order — undefined on most filesystems. When both `CONTEXT.md` and `01-CONTEXT.md` exist the winner was arbitrary. The old gap-checker.cjs:136 code always used bare `CONTEXT.md` first (existsSync on the bare path). Restore that invariant: check `files.includes('CONTEXT.md')` before falling through to the padded scan. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(3739): dual-file precedence test — bare CONTEXT.md wins over padded form Adds two test cases for the scenario where both CONTEXT.md and 01-CONTEXT.md exist in the same phase directory: 1. Helper level: findContextMdIn() must return 'CONTEXT.md' (not the padded filename) when both files are present on disk. 2. Integration level: gap-analysis must resolve decisions from the bare form only; D-PADDED (from 01-CONTEXT.md) must not appear when the bare form shadows it. Without these tests a future change to findContextMdIn could silently regress the precedence guarantee. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): bug-2798 tests skip cleanly when sdk/dist is absent (was hard-fail) The 3 tests in bug-2798-context-window-config-key.test.cjs invoke the built SDK CLI (sdk/dist/cli.js) and require sdk/dist/query/config-schema.js. When dist is absent, they threw hard errors rather than observable skips. Apply the same `if (!existsSync(...)) { t.skip(...); return; }` guard used in bug-2767-gsd-sdk-commit-files-flag.test.cjs (c2812313). Tests 1 & 2 guard on sdk/dist/cli.js; test 3 guards on sdk/dist/query/config-schema.js. When dist is present all 3 run; when absent all 3 emit actionable skip lines. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(3739): eliminate double readdirSync in findContextMdIn callers findContextMdIn now accepts either a directory path or an already-read files array, allowing callers that already hold a directory listing (core.cjs:getPhaseFileStats, roadmap.cjs:countPhasePlansAndSummaries, gap-checker.cjs:runGapAnalysis) to skip redundant readdirSync calls. Test coverage added for the array-argument overload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: redesign flaky concurrent add-blocker test with deterministic barrier Previous design relied on OS scheduler to interleave two subprocess writes, producing a flake under CI load. Redesigned using a file-barrier (Option A) that forces both subprocesses to reach a ready-gate before either proceeds, guaranteeing true concurrent lock contention and eliminating timing dependency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/eager-wolves-tumble.md
Normal file
5
.changeset/eager-wolves-tumble.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 3764
|
||||
---
|
||||
**`gap-analysis` now respects padded-prefix CONTEXT.md** — `gap-checker.cjs:136` silently missed `NN-CONTEXT.md` decisions on phases using the padded-prefix convention; now matches the dual-form pattern used elsewhere.
|
||||
@@ -23,6 +23,7 @@ const {
|
||||
withPlanningLock,
|
||||
getActiveWorkstream,
|
||||
setActiveWorkstream,
|
||||
findContextMdIn,
|
||||
} = require('./planning-workspace.cjs');
|
||||
const { findProjectRoot } = require('./project-root.generated.cjs');
|
||||
|
||||
@@ -1810,7 +1811,7 @@ function getPhaseFileStats(phaseDir) {
|
||||
plans: filterPlanFiles(files),
|
||||
summaries: filterSummaryFiles(files),
|
||||
hasResearch: files.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'),
|
||||
hasContext: files.some(f => f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'),
|
||||
hasContext: findContextMdIn(files) !== null,
|
||||
hasVerification: files.some(f => f.endsWith('-VERIFICATION.md') || f === 'VERIFICATION.md'),
|
||||
hasReviews: files.some(f => f.endsWith('-REVIEWS.md') || f === 'REVIEWS.md'),
|
||||
};
|
||||
|
||||
@@ -17,7 +17,7 @@
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { escapeRegex, output, error } = require('./core.cjs');
|
||||
const { planningPaths, planningDir } = require('./planning-workspace.cjs');
|
||||
const { planningPaths, planningDir, findContextMdIn } = require('./planning-workspace.cjs');
|
||||
const { parseDecisions } = require('./decisions.cjs');
|
||||
|
||||
/**
|
||||
@@ -133,16 +133,24 @@ function runGapAnalysis(cwd, phaseDir) {
|
||||
const reqMd = fs.existsSync(reqPath) ? fs.readFileSync(reqPath, 'utf-8') : '';
|
||||
const reqItems = parseRequirements(reqMd).map(r => ({ ...r, source: 'REQUIREMENTS.md' }));
|
||||
|
||||
const ctxPath = path.join(absPhaseDir, 'CONTEXT.md');
|
||||
const ctxMd = fs.existsSync(ctxPath) ? fs.readFileSync(ctxPath, 'utf-8') : '';
|
||||
// Read the phase directory once; reuse the listing for both context detection
|
||||
// and plan-file enumeration (avoids redundant readdirSync calls).
|
||||
let phaseDirFiles = [];
|
||||
try {
|
||||
if (fs.existsSync(absPhaseDir)) phaseDirFiles = fs.readdirSync(absPhaseDir);
|
||||
} catch { /* unreadable */ }
|
||||
|
||||
const ctxFile = findContextMdIn(phaseDirFiles);
|
||||
const ctxPath = ctxFile ? path.join(absPhaseDir, ctxFile) : null;
|
||||
const ctxMd = ctxPath ? fs.readFileSync(ctxPath, 'utf-8') : '';
|
||||
const dItems = parseDecisions(ctxMd).map(d => ({ ...d, source: 'CONTEXT.md' }));
|
||||
|
||||
const items = [...reqItems, ...dItems];
|
||||
|
||||
let planText = '';
|
||||
try {
|
||||
if (fs.existsSync(absPhaseDir)) {
|
||||
const files = fs.readdirSync(absPhaseDir).filter(f => /-PLAN\.md$/.test(f));
|
||||
if (phaseDirFiles.length > 0) {
|
||||
const files = phaseDirFiles.filter(f => /-PLAN\.md$/.test(f));
|
||||
planText = files.map(f => {
|
||||
try { return fs.readFileSync(path.join(absPhaseDir, f), 'utf-8'); }
|
||||
catch { return ''; }
|
||||
|
||||
@@ -6,7 +6,7 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { execGit, platformWriteSync, platformReadSync } = require('./shell-command-projection.cjs');
|
||||
const { loadConfig, resolveModelInternal, findPhaseInternal, getRoadmapPhaseInternal, pathExistsInternal, gitWorktreeInfoInternal, generateSlugInternal, getMilestoneInfo, getMilestonePhaseFilter, stripShippedMilestones, extractCurrentMilestone, normalizePhaseName, toPosixPath, output, error, checkAgentsInstalled, phaseTokenMatches } = require('./core.cjs');
|
||||
const { planningPaths, planningDir, planningRoot } = require('./planning-workspace.cjs');
|
||||
const { planningPaths, planningDir, planningRoot, findContextMdIn } = require('./planning-workspace.cjs');
|
||||
const { maskIfSecret } = require('./secrets.cjs');
|
||||
const scanPhasePlans = require('./plan-scan.cjs');
|
||||
const { stateExtractField } = require('./state-document.cjs');
|
||||
@@ -407,7 +407,7 @@ function cmdInitPlanPhase(cwd, phase, raw, options = {}) {
|
||||
const phaseDirFull = path.join(cwd, phaseInfo.directory);
|
||||
try {
|
||||
const files = fs.readdirSync(phaseDirFull);
|
||||
const contextFile = files.find(f => f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md');
|
||||
const contextFile = findContextMdIn(phaseDirFull);
|
||||
if (contextFile) {
|
||||
result.context_path = toPosixPath(path.join(phaseInfo.directory, contextFile));
|
||||
}
|
||||
@@ -885,7 +885,7 @@ function cmdInitPhaseOp(cwd, phase, raw) {
|
||||
const phaseDirFull = path.join(cwd, phaseInfo.directory);
|
||||
try {
|
||||
const files = fs.readdirSync(phaseDirFull);
|
||||
const contextFile = files.find(f => f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md');
|
||||
const contextFile = findContextMdIn(phaseDirFull);
|
||||
if (contextFile) {
|
||||
result.context_path = toPosixPath(path.join(phaseInfo.directory, contextFile));
|
||||
}
|
||||
@@ -1198,7 +1198,7 @@ function cmdInitManager(cwd, raw) {
|
||||
const phaseFiles = fs.readdirSync(fullDir);
|
||||
planCount = listPhasePlanFiles(fullDir).length;
|
||||
summaryCount = listPhaseSummaryFiles(fullDir).length;
|
||||
hasContext = phaseFiles.some(f => f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md');
|
||||
hasContext = findContextMdIn(fullDir) !== null;
|
||||
hasResearch = phaseFiles.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md');
|
||||
|
||||
if (summaryCount >= planCount && planCount > 0) diskStatus = 'complete';
|
||||
|
||||
@@ -371,6 +371,35 @@ function setActiveWorkstream(cwd, name) {
|
||||
createPlanningWorkspace(cwd).activeWorkstream.set(name);
|
||||
}
|
||||
|
||||
/**
|
||||
* Locate the CONTEXT.md file in a phase directory, handling both the bare
|
||||
* form (`CONTEXT.md`) and the padded-prefix convention (`NN-CONTEXT.md`,
|
||||
* `NN.N-CONTEXT.md`, etc.) used by gsd-discuss-phase output.
|
||||
*
|
||||
* Returns the filename (not the full path) of the first match, or null if
|
||||
* no CONTEXT.md exists in the directory.
|
||||
*
|
||||
* Canonical dual-form predicate extracted here to eliminate the 5-site
|
||||
* duplication that previously existed across init.cjs, roadmap.cjs,
|
||||
* core.cjs, gap-checker.cjs (#3739).
|
||||
*
|
||||
* @param {string|string[]} absDirOrFiles - Absolute path to the phase directory,
|
||||
* OR an already-read files array (avoids a redundant readdirSync at call sites
|
||||
* that already hold a directory listing).
|
||||
* @returns {string|null}
|
||||
*/
|
||||
function findContextMdIn(absDirOrFiles) {
|
||||
try {
|
||||
const files = Array.isArray(absDirOrFiles)
|
||||
? absDirOrFiles
|
||||
: fs.readdirSync(absDirOrFiles);
|
||||
if (files.includes('CONTEXT.md')) return 'CONTEXT.md';
|
||||
return files.find(f => f.endsWith('-CONTEXT.md')) ?? null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
createPlanningWorkspace,
|
||||
createSharedPointerAdapter,
|
||||
@@ -382,4 +411,5 @@ module.exports = {
|
||||
withPlanningLock,
|
||||
getActiveWorkstream,
|
||||
setActiveWorkstream,
|
||||
findContextMdIn,
|
||||
};
|
||||
|
||||
@@ -6,7 +6,7 @@ const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { escapeRegex, normalizePhaseName, phaseMarkdownRegexSource, phaseMarkdownRegexSourceExact, output, error, findPhaseInternal, stripShippedMilestones, extractCurrentMilestone, replaceInCurrentMilestone, phaseTokenMatches } = require('./core.cjs');
|
||||
const { platformWriteSync } = require('./shell-command-projection.cjs');
|
||||
const { planningPaths, withPlanningLock } = require('./planning-workspace.cjs');
|
||||
const { planningPaths, withPlanningLock, findContextMdIn } = require('./planning-workspace.cjs');
|
||||
const scanPhasePlans = require('./plan-scan.cjs');
|
||||
|
||||
/**
|
||||
@@ -41,13 +41,13 @@ function coerceTruthToString(t) {
|
||||
function countPhasePlansAndSummaries(phaseDir) {
|
||||
const { planCount, summaryCount } = scanPhasePlans(phaseDir);
|
||||
// hasContext and hasResearch are not plan-scan concerns — read the directory
|
||||
// once for the non-plan metadata that cmdRoadmapAnalyze needs.
|
||||
// once and share the listing for all non-plan metadata that cmdRoadmapAnalyze needs.
|
||||
let phaseFiles = [];
|
||||
try { phaseFiles = fs.readdirSync(phaseDir); } catch { /* empty */ }
|
||||
return {
|
||||
planCount,
|
||||
summaryCount,
|
||||
hasContext: phaseFiles.some(f => f.endsWith('-CONTEXT.md') || f === 'CONTEXT.md'),
|
||||
hasContext: findContextMdIn(phaseFiles) !== null,
|
||||
hasResearch: phaseFiles.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -55,7 +55,11 @@ describe('bug-2798: context_window is a valid config key', () => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
test('config-set context_window succeeds (not rejected as unknown key)', () => {
|
||||
test('config-set context_window succeeds (not rejected as unknown key)', (t) => {
|
||||
if (!fs.existsSync(SDK_CLI)) {
|
||||
t.skip('sdk/dist/cli.js not built — run `cd sdk && npm run build` to enable this integration test');
|
||||
return;
|
||||
}
|
||||
const result = runConfigSet('context_window', 1000000, tmpDir);
|
||||
|
||||
assert.strictEqual(result.exitCode, 0, 'should exit 0 (key is valid)');
|
||||
@@ -64,7 +68,11 @@ describe('bug-2798: context_window is a valid config key', () => {
|
||||
assert.strictEqual(result.json?.key, 'context_window');
|
||||
});
|
||||
|
||||
test('context_window value is written to config.json', () => {
|
||||
test('context_window value is written to config.json', (t) => {
|
||||
if (!fs.existsSync(SDK_CLI)) {
|
||||
t.skip('sdk/dist/cli.js not built — run `cd sdk && npm run build` to enable this integration test');
|
||||
return;
|
||||
}
|
||||
runConfigSet('context_window', 500000, tmpDir);
|
||||
|
||||
const config = JSON.parse(
|
||||
@@ -73,7 +81,11 @@ describe('bug-2798: context_window is a valid config key', () => {
|
||||
assert.strictEqual(config.context_window, 500000, 'context_window should be persisted');
|
||||
});
|
||||
|
||||
test('config-schema CJS and SDK allowlists both include context_window', () => {
|
||||
test('config-schema CJS and SDK allowlists both include context_window', (t) => {
|
||||
if (!fs.existsSync(path.join(REPO_ROOT, 'sdk', 'dist', 'query', 'config-schema.js'))) {
|
||||
t.skip('sdk/dist/query/config-schema.js not built — run `cd sdk && npm run build` to enable this integration test');
|
||||
return;
|
||||
}
|
||||
const cjsSchema = require(path.join(REPO_ROOT, 'get-shit-done', 'bin', 'lib', 'config-schema.cjs'));
|
||||
const sdkSchema = require(path.join(REPO_ROOT, 'sdk', 'dist', 'query', 'config-schema.js'));
|
||||
|
||||
|
||||
222
tests/bug-3739-gap-checker-padded-prefix-context.test.cjs
Normal file
222
tests/bug-3739-gap-checker-padded-prefix-context.test.cjs
Normal file
@@ -0,0 +1,222 @@
|
||||
/**
|
||||
* Bug #3739: gap-analysis silently skips CONTEXT.md decisions when the file
|
||||
* uses the padded-prefix convention (e.g. 01-CONTEXT.md, 02.1-CONTEXT.md).
|
||||
*
|
||||
* Verifies:
|
||||
* 1. Padded-prefix CONTEXT.md (NN-CONTEXT.md) decisions ARE included in the
|
||||
* gap report — was silently skipped before the fix.
|
||||
* 2. Decisions from padded-prefix CONTEXT.md ARE checked for coverage.
|
||||
* 3. Bare CONTEXT.md still works — no regression on the existing path.
|
||||
* 4. A padded-prefix decision that is NOT covered in the plan is surfaced
|
||||
* as "Not covered" (not silently dropped from the report).
|
||||
* 5. planning-workspace.cjs findContextMdIn() helper returns the right
|
||||
* filename for both bare and padded forms (unit test for the extractor).
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test, beforeEach, afterEach } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
|
||||
describe('bug #3739 — gap-analysis padded-prefix CONTEXT.md', () => {
|
||||
let tmpDir;
|
||||
let phaseDir;
|
||||
|
||||
function writeRequirements(ids) {
|
||||
const lines = ids.map((id, i) => `- [ ] **${id}** Requirement ${i + 1}`);
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'REQUIREMENTS.md'),
|
||||
`# Requirements\n\n${lines.join('\n')}\n`
|
||||
);
|
||||
}
|
||||
|
||||
function writeContextAs(filename, decisions) {
|
||||
const dLines = decisions.map(d => `- **${d.id}:** ${d.text}`).join('\n');
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, filename),
|
||||
`# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n${dLines}\n</decisions>\n`
|
||||
);
|
||||
}
|
||||
|
||||
function writePlan(name, body) {
|
||||
fs.writeFileSync(path.join(phaseDir, `${name}-PLAN.md`), body);
|
||||
}
|
||||
|
||||
function ensureConfig() {
|
||||
const r = runGsdTools('config-ensure-section', tmpDir);
|
||||
assert.ok(r.success, `config-ensure-section failed: ${r.error}`);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
phaseDir = path.join(tmpDir, '.planning', 'phases', '01-test');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
ensureConfig();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// ── Test 1: padded-prefix decisions appear in the gap report ─────────────
|
||||
|
||||
test('decisions from padded-prefix CONTEXT.md (01-CONTEXT.md) appear in gap report', () => {
|
||||
writeContextAs('01-CONTEXT.md', [
|
||||
{ id: 'D-01', text: 'Use library X' },
|
||||
{ id: 'D-02', text: 'Fail loud on unknown input' },
|
||||
]);
|
||||
writePlan('01', '# Plan\n\nImplements D-01 and D-02.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
const d01 = out.rows.find(x => x.item === 'D-01');
|
||||
const d02 = out.rows.find(x => x.item === 'D-02');
|
||||
|
||||
assert.ok(d01, 'D-01 row must appear in gap report when CONTEXT.md uses padded-prefix 01-CONTEXT.md');
|
||||
assert.ok(d02, 'D-02 row must appear in gap report when CONTEXT.md uses padded-prefix 01-CONTEXT.md');
|
||||
assert.strictEqual(d01.source, 'CONTEXT.md', 'source label must be CONTEXT.md');
|
||||
assert.strictEqual(d01.status, 'Covered', 'D-01 is mentioned in plan — must be Covered');
|
||||
assert.strictEqual(d02.status, 'Covered', 'D-02 is mentioned in plan — must be Covered');
|
||||
});
|
||||
|
||||
// ── Test 2: uncovered padded-prefix decision surfaces as Not covered ──────
|
||||
|
||||
test('uncovered decision from padded-prefix CONTEXT.md surfaces as Not covered', () => {
|
||||
writeContextAs('01-CONTEXT.md', [
|
||||
{ id: 'D-01', text: 'Use library X' },
|
||||
]);
|
||||
writePlan('01', '# Plan\n\nUnrelated work, no mention of any D-NN.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
const d01 = out.rows.find(x => x.item === 'D-01');
|
||||
assert.ok(d01, 'D-01 row must appear even when not covered');
|
||||
assert.strictEqual(d01.status, 'Not covered',
|
||||
'D-01 must be Not covered (not silently absent) when plan omits it');
|
||||
});
|
||||
|
||||
// ── Test 3 (counter-test): bare CONTEXT.md still works — no regression ───
|
||||
|
||||
test('bare CONTEXT.md still works (regression guard)', () => {
|
||||
writeContextAs('CONTEXT.md', [
|
||||
{ id: 'D-05', text: 'Bare form decision' },
|
||||
]);
|
||||
writePlan('01', '# Plan\n\nImplements D-05.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
const d05 = out.rows.find(x => x.item === 'D-05');
|
||||
assert.ok(d05, 'D-05 must appear when CONTEXT.md uses bare filename');
|
||||
assert.strictEqual(d05.status, 'Covered', 'D-05 must be Covered');
|
||||
});
|
||||
|
||||
// ── Test 4: deeper padded prefix (02.1-CONTEXT.md) ───────────────────────
|
||||
|
||||
test('multi-segment padded prefix (02.1-CONTEXT.md) decisions appear in gap report', () => {
|
||||
writeContextAs('02.1-CONTEXT.md', [
|
||||
{ id: 'D-03', text: 'Use postgres' },
|
||||
]);
|
||||
writePlan('01', '# Plan\n\nImplements D-03.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
const d03 = out.rows.find(x => x.item === 'D-03');
|
||||
assert.ok(d03, 'D-03 must appear from 02.1-CONTEXT.md');
|
||||
assert.strictEqual(d03.status, 'Covered');
|
||||
});
|
||||
|
||||
// ── Test 5: findContextMdIn helper unit test ─────────────────────────────
|
||||
|
||||
test('findContextMdIn helper returns padded filename when present', () => {
|
||||
const { findContextMdIn } = require('../get-shit-done/bin/lib/planning-workspace.cjs');
|
||||
// Write 01-CONTEXT.md into the phase dir (already created in beforeEach)
|
||||
fs.writeFileSync(path.join(phaseDir, '01-CONTEXT.md'), '# context\n');
|
||||
|
||||
const found = findContextMdIn(phaseDir);
|
||||
assert.strictEqual(found, '01-CONTEXT.md',
|
||||
'findContextMdIn must return the padded-prefix filename');
|
||||
});
|
||||
|
||||
test('findContextMdIn helper returns bare filename when only bare form exists', () => {
|
||||
const { findContextMdIn } = require('../get-shit-done/bin/lib/planning-workspace.cjs');
|
||||
fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# context\n');
|
||||
|
||||
const found = findContextMdIn(phaseDir);
|
||||
assert.strictEqual(found, 'CONTEXT.md',
|
||||
'findContextMdIn must return CONTEXT.md for bare form');
|
||||
});
|
||||
|
||||
test('findContextMdIn helper returns null when no CONTEXT.md exists', () => {
|
||||
const { findContextMdIn } = require('../get-shit-done/bin/lib/planning-workspace.cjs');
|
||||
// phaseDir exists but is empty (no CONTEXT.md)
|
||||
const found = findContextMdIn(phaseDir);
|
||||
assert.strictEqual(found, null,
|
||||
'findContextMdIn must return null when no CONTEXT.md exists');
|
||||
});
|
||||
|
||||
// ── Test 5b: findContextMdIn accepts pre-read files array (avoids double readdirSync) ──
|
||||
|
||||
test('findContextMdIn accepts an already-read files array (avoids double readdirSync)', () => {
|
||||
const { findContextMdIn } = require('../get-shit-done/bin/lib/planning-workspace.cjs');
|
||||
// Passing an array directly should behave identically to passing a directory path.
|
||||
assert.strictEqual(findContextMdIn(['CONTEXT.md', 'other.md']), 'CONTEXT.md',
|
||||
'bare form found in array');
|
||||
assert.strictEqual(findContextMdIn(['01-CONTEXT.md', 'other.md']), '01-CONTEXT.md',
|
||||
'padded form found in array');
|
||||
assert.strictEqual(findContextMdIn(['unrelated.md']), null,
|
||||
'returns null when no CONTEXT.md in array');
|
||||
// Bare wins over padded when both are present
|
||||
assert.strictEqual(findContextMdIn(['01-CONTEXT.md', 'CONTEXT.md']), 'CONTEXT.md',
|
||||
'bare form preferred over padded form when both in array');
|
||||
});
|
||||
|
||||
// ── Test 6: dual-file precedence — bare CONTEXT.md wins over padded form ──
|
||||
|
||||
test('findContextMdIn prefers bare CONTEXT.md over padded form (helper level)', () => {
|
||||
const { findContextMdIn } = require('../get-shit-done/bin/lib/planning-workspace.cjs');
|
||||
// Write BOTH forms into the phase directory
|
||||
fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# bare context\n');
|
||||
fs.writeFileSync(path.join(phaseDir, '01-CONTEXT.md'), '# padded context\n');
|
||||
|
||||
const found = findContextMdIn(phaseDir);
|
||||
assert.strictEqual(found, 'CONTEXT.md',
|
||||
'findContextMdIn must return bare CONTEXT.md when both forms exist — matches pre-refactor gap-checker behavior');
|
||||
});
|
||||
|
||||
test('gap-analysis uses bare CONTEXT.md decisions when both forms exist (integration level)', () => {
|
||||
// Bare form has D-BARE; padded form has D-PADDED.
|
||||
// If the integration path resolves bare correctly, only D-BARE appears in the report.
|
||||
const bareContent =
|
||||
'# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n- **D-BARE:** From bare form\n</decisions>\n';
|
||||
const paddedContent =
|
||||
'# Phase Context\n\n<decisions>\n## Implementation Decisions\n\n- **D-PADDED:** From padded form\n</decisions>\n';
|
||||
fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), bareContent);
|
||||
fs.writeFileSync(path.join(phaseDir, '01-CONTEXT.md'), paddedContent);
|
||||
|
||||
writePlan('01', '# Plan\n\nImplements D-BARE.\n');
|
||||
|
||||
const r = runGsdTools(['gap-analysis', '--phase-dir', phaseDir], tmpDir);
|
||||
assert.ok(r.success, `gap-analysis failed: ${r.error}`);
|
||||
const out = JSON.parse(r.output);
|
||||
|
||||
const dBare = out.rows.find(x => x.item === 'D-BARE');
|
||||
const dPadded = out.rows.find(x => x.item === 'D-PADDED');
|
||||
|
||||
assert.ok(dBare, 'D-BARE (from bare CONTEXT.md) must appear in gap report');
|
||||
assert.ok(!dPadded, 'D-PADDED (from 01-CONTEXT.md) must NOT appear — bare form takes precedence');
|
||||
assert.strictEqual(dBare.status, 'Covered', 'D-BARE must be Covered');
|
||||
});
|
||||
});
|
||||
@@ -20,7 +20,7 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const os = require('os');
|
||||
const { execFileSync, execSync } = require('child_process');
|
||||
const { execFileSync, execSync, spawn } = require('child_process');
|
||||
const { promisify } = require('util');
|
||||
const { exec } = require('child_process');
|
||||
|
||||
@@ -233,6 +233,26 @@ describe('#1925 TOCTOU: state commands use readModifyWriteStateMd', () => {
|
||||
});
|
||||
|
||||
test('state add-blocker: both concurrent calls append different blockers', async () => {
|
||||
// Deterministic concurrency via file-barrier synchronization (Option A).
|
||||
//
|
||||
// Problem with the prior design: Promise.all([execAsync(A), execAsync(B)])
|
||||
// offers no guarantee that both subprocesses are alive simultaneously. On a
|
||||
// loaded CI runner one subprocess can fully complete (acquire lock → transform
|
||||
// → release lock → exit) before the other's Node runtime has even started.
|
||||
// When that happens the second subprocess never contends on the lock — the
|
||||
// test trivially passes — but the test also fails to exercise what it claims
|
||||
// to test. On Docker overlay-fs under load the opposite pathology occurs:
|
||||
// both subprocesses race O_EXCL creation, and depending on scheduler timing
|
||||
// one can observe stale fs state, causing a lost update that fails the
|
||||
// assertion. Either way, the outcome is non-deterministic.
|
||||
//
|
||||
// Redesign: a barrier file forces both subprocesses to reach their "ready"
|
||||
// gate before either is allowed to proceed. The barrier is removed only
|
||||
// after BOTH have signalled readiness, guaranteeing true overlap in the
|
||||
// critical section. No sleep-based synchronization; the barrier loop uses
|
||||
// Atomics.wait (same primitive as acquireStateLock) so it yields the CPU
|
||||
// instead of spinning.
|
||||
|
||||
writeStateMd(tmpDir, [
|
||||
'# Project State',
|
||||
'',
|
||||
@@ -242,14 +262,96 @@ describe('#1925 TOCTOU: state commands use readModifyWriteStateMd', () => {
|
||||
'None.',
|
||||
].join('\n') + '\n');
|
||||
|
||||
const nodeBin = process.execPath;
|
||||
const cmdA = `"${nodeBin}" "${TOOLS_PATH}" state add-blocker --text "Need API credentials" --cwd "${tmpDir}"`;
|
||||
const cmdB = `"${nodeBin}" "${TOOLS_PATH}" state add-blocker --text "Waiting for design review" --cwd "${tmpDir}"`;
|
||||
// ── Barrier infrastructure ────────────────────────────────────────────────
|
||||
// barrierPath: exists while subprocesses must hold. Removed by the test
|
||||
// orchestrator once both subprocesses have signalled readiness.
|
||||
// ready-{id}: each subprocess creates this file to signal it is at the gate.
|
||||
const barrierPath = path.join(tmpDir, '.barrier');
|
||||
const readyA = path.join(tmpDir, '.ready-a');
|
||||
const readyB = path.join(tmpDir, '.ready-b');
|
||||
fs.writeFileSync(barrierPath, '1'); // erect the barrier
|
||||
if (fs.existsSync(readyA)) fs.unlinkSync(readyA);
|
||||
if (fs.existsSync(readyB)) fs.unlinkSync(readyB);
|
||||
|
||||
await Promise.all([
|
||||
execAsync(cmdA, { encoding: 'utf-8' }).catch(() => {}),
|
||||
execAsync(cmdB, { encoding: 'utf-8' }).catch(() => {}),
|
||||
]);
|
||||
// ── Wrapper script written to tmpDir ─────────────────────────────────────
|
||||
// Each subprocess runs this wrapper, which:
|
||||
// 1. Writes its ready-signal so the orchestrator knows it is alive.
|
||||
// 2. Spins (Atomics.wait, 10 ms steps) until the barrier is removed.
|
||||
// 3. Immediately calls gsd-tools to exercise the real lock contention.
|
||||
//
|
||||
// TOOLS_PATH and the caller-supplied args are injected via env vars to avoid
|
||||
// shell-quoting complexity when the tmpDir path contains spaces.
|
||||
const wrapperPath = path.join(tmpDir, '.barrier-wrapper.cjs');
|
||||
fs.writeFileSync(wrapperPath, [
|
||||
"'use strict';",
|
||||
'const fs = require("fs");',
|
||||
'const path = require("path");',
|
||||
'const { execFileSync } = require("child_process");',
|
||||
'const { TOOLS_PATH, BARRIER_FILE, READY_FILE, BLOCKER_TEXT, CWD_PATH } = process.env;',
|
||||
'',
|
||||
'// Signal readiness to the orchestrator.',
|
||||
'fs.writeFileSync(READY_FILE, String(process.pid));',
|
||||
'',
|
||||
'// Wait at the barrier (yield via Atomics.wait so we do not spin the CPU).',
|
||||
'// Budget: 10 s — if the orchestrator never releases us, something is broken.',
|
||||
'const sab = new SharedArrayBuffer(4);',
|
||||
'const sai = new Int32Array(sab);',
|
||||
'const deadline = Date.now() + 10000;',
|
||||
'while (fs.existsSync(BARRIER_FILE)) {',
|
||||
' if (Date.now() > deadline) { process.stderr.write("barrier timeout\\n"); process.exit(1); }',
|
||||
' Atomics.wait(sai, 0, 0, 10); // sleep 10 ms, then re-check',
|
||||
'}',
|
||||
'',
|
||||
'// Barrier is down — execute the actual gsd-tools command.',
|
||||
'execFileSync(process.execPath, [TOOLS_PATH, "state", "add-blocker", "--text", BLOCKER_TEXT, "--cwd", CWD_PATH], {',
|
||||
' stdio: "pipe",',
|
||||
'});',
|
||||
].join('\n'));
|
||||
|
||||
const nodeBin = process.execPath;
|
||||
|
||||
// ── Spawn both subprocesses ───────────────────────────────────────────────
|
||||
// Both start immediately; both block at the barrier until the orchestrator
|
||||
// confirms both are ready, then both proceed to contend on the STATE.md lock.
|
||||
function spawnWrapper(blockerId, readyFile) {
|
||||
return new Promise((resolve, reject) => {
|
||||
const child = spawn(nodeBin, [wrapperPath], {
|
||||
env: {
|
||||
...process.env,
|
||||
TOOLS_PATH,
|
||||
BARRIER_FILE: barrierPath,
|
||||
READY_FILE: readyFile,
|
||||
BLOCKER_TEXT: blockerId,
|
||||
CWD_PATH: tmpDir,
|
||||
},
|
||||
stdio: 'pipe',
|
||||
});
|
||||
let stderr = '';
|
||||
child.stderr.on('data', (d) => { stderr += d.toString(); });
|
||||
child.on('close', (code) => {
|
||||
if (code !== 0) reject(new Error(`wrapper exited ${code}: ${stderr}`));
|
||||
else resolve();
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
const promiseA = spawnWrapper('Need API credentials', readyA);
|
||||
const promiseB = spawnWrapper('Waiting for design review', readyB);
|
||||
|
||||
// ── Orchestrate: wait for both ready-signals, then drop the barrier ───────
|
||||
// Poll with Atomics.wait (10 ms steps). Budget: 10 s.
|
||||
const sab2 = new SharedArrayBuffer(4);
|
||||
const sai2 = new Int32Array(sab2);
|
||||
const deadline2 = Date.now() + 10000;
|
||||
while (!fs.existsSync(readyA) || !fs.existsSync(readyB)) {
|
||||
if (Date.now() > deadline2) throw new Error('Timed out waiting for both subprocesses to reach barrier');
|
||||
Atomics.wait(sai2, 0, 0, 10);
|
||||
}
|
||||
// Both subprocesses are at the gate — drop the barrier simultaneously.
|
||||
fs.unlinkSync(barrierPath);
|
||||
|
||||
// ── Collect results ───────────────────────────────────────────────────────
|
||||
await Promise.all([promiseA, promiseB]);
|
||||
|
||||
const content = readStateMd(tmpDir);
|
||||
assert.ok(
|
||||
|
||||
Reference in New Issue
Block a user