* test(#4282): prove PATTERNS.md is unrecognized by the artifact registry Regression test only, no fix yet: CANONICAL_EXACT in src/artifacts.cts was never updated when workflows/graduation.md started writing .planning/ PATTERNS.md, same omission class as the already-fixed #3224 (WINDOWS.md). Expected RED on this commit (src/artifacts.cts is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4282): register PATTERNS.md as a canonical .planning/ artifact CANONICAL_EXACT in src/artifacts.cts was never updated when workflows/graduation.md started writing .planning/PATTERNS.md for the `patterns` graduation-target category -- same omission class as the already-fixed #3224 (WINDOWS.md). validate.health's W019 falsely flagged it as unrecognized on every repo that has run the graduation scan. Also backfilled 5 other pre-existing stale rows in gsd-core/templates/README.md's artifact table (WINDOWS.md, STATE-ARCHIVE.md, milestone.lock, state.json, skill-manifest.json) that were already in the source registry but missing from the docs table -- found while fixing this exact drift class, cheap to close alongside it. RED proven on f3dd791fb8cda18196803e7144ce20e506d6490b (test-only commit, gsd-test outcome:failed, exactly the new PATTERNS.md test failing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): fix stale function name in skill-manifest.json comment Review finding: both the source comment and the new docs row said "routeSkillManifest" -- no such symbol exists (verified via Memtrace); the actual function is cmdSkillManifest (src/init.cts). Copied verbatim from a pre-existing comment, not introduced by this PR, but cheap to fix alongside. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): add changeset fragment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: isolate lint-vendored-deps-manifest.test.cjs's fixRow tests from the real vendor file Genuine, pre-existing defect found and fixed per this repo's no-defer policy (discovered while investigating a real CI failure during this PR's own merge attempt, user-directed investigation -- not deferred to a separate issue since it was actively blocking work and root-caused with concrete evidence, not speculation). Root cause: fixRow(row) (scripts/lint-vendored-deps.cjs) unconditionally does fs.copyFileSync(upstreamCjs, vendoredCjs) as its first line. All three tests in the #4573 describe block called fixRow(row) with the REAL js-yaml row, so all three wrote to the real, shared gsd-core/bin/lib/vendor/ js-yaml.cjs -- a file other test files' require() calls can read at any moment, since node --test runs files concurrently in this repo. fs.copyFileSync's write is not atomic against a concurrent reader on every filesystem; a concurrent require() elsewhere caught the file mid-overwrite and read a truncated file, crashing an entirely unrelated test (m9-statelock-write-error-orphan.test.cjs) with a SyntaxError. Confirmed via two real CI log fetches, not assumed: the exact same shard grouping (same 308 files) ran clean ~90 minutes earlier during PR #4615's own final merge CI, with the identical #3660 reap-fix code already present -- ruling out a deterministic connection to that change and confirming a genuine, non-deterministic timing race in this pre-existing test design. Fix: all three tests now redirect row.vendoredCjs to a private os.tmpdir() path via a cloned row object before calling fixRow, so the real vendored file is never touched. upstreamCjs stays pointed at the real node_modules copy (read-only, safe to share). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: also isolate fixRow's package.json pin-rewrite from the real file Review finding (major) on the previous race-condition fix: fixRow's pin -rewrite path still hardcoded path.join(ROOT, 'package.json'), so the third #4573 test still wrote the real, shared package.json -- read at module top-level by dozens of other test files, the same concurrent-file race class already fixed for the vendored .cjs copy. Adds an optional pkgRoot parameter (defaults to the real ROOT) threaded through readPinState/checkRow/fixRow -- fully backward-compatible, every existing call site (the CLI --fix path, any other caller) is unaffected since the default is unchanged. The pin-rewrite test now builds an isolated temp root (its own package.json + node_modules/js-yaml/package.json) and passes it explicitly, so the real package.json is never touched either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use helpers.cleanup instead of raw fs.rmSync in test cleanup CI caught it: local/no-raw-rmsync-in-tests flagged the three t.after temp-dir cleanup calls added for the fixRow isolation fix. helpers.cleanup() carries the Windows-EBUSY retry budget (maxRetries/retryDelay) that raw fs.rmSync lacks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/brave-geese-sing.md
Normal file
5
.changeset/brave-geese-sing.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4618
|
||||
---
|
||||
**`validate.health` no longer flags `.planning/PATTERNS.md` as an unrecognized file.** The graduation workflow (`/gsd-extract-learnings`) writes this file on gsd-core's own instruction, but the artifact registry was never updated to recognize it -- every repo that had run the graduation scan sat permanently at `status: degraded`. (#4282)
|
||||
@@ -23,6 +23,12 @@ These files live directly at `.planning/` — not inside phase subdirectories.
|
||||
| `config.json` | `config.json` | `/gsd:new-project`, `/gsd:health --repair` | Project-specific GSD configuration |
|
||||
| `CLAUDE.md` | *(inline)* | `/gsd-profile` | Auto-assembled Claude Code context file |
|
||||
| `RETROSPECTIVE.md` | *(inline)* | `/gsd:complete-milestone` | Living milestone retrospective updated at each milestone close |
|
||||
| `WINDOWS.md` | *(none)* | broken-windows ledger (`src/broken-windows.cts`) | Tracked known-broken items pending resolution (#3224) |
|
||||
| `STATE-ARCHIVE.md` | *(none)* | `state.cts`'s `cmdStatePrune` | Pruned historical STATE.md entries |
|
||||
| `milestone.lock` | *(none)* | `src/milestone-lock.cts` | Persistent milestone (phase + session) claim, unlike the transient `STATE.md.lock`/`WAITING.json` (#3311) |
|
||||
| `state.json` | *(none)* | `src/state-contract.cts` | Machine-readable state contract published at step boundaries (#3227) |
|
||||
| `skill-manifest.json` | *(none)* | `init.cts`'s `cmdSkillManifest --write` | Project-scoped skill manifest (#3964) |
|
||||
| `PATTERNS.md` | *(inline)* | `/gsd:extract-learnings` (graduation, `workflows/graduation.md`, `patterns` target) | Graduated cross-phase patterns -- distinct from the per-phase `NN-PATTERNS.md` below (#4282) |
|
||||
|
||||
### Version-stamped artifacts (pattern: `vX.Y-*.md`)
|
||||
|
||||
|
||||
@@ -233,13 +233,17 @@ function checkHandAuthoredTwin(row) {
|
||||
* installed version. Shared by checkRow (compares) and fixRow (rewrites) so
|
||||
* the two can never silently diverge on how a pin is read.
|
||||
* @param {VendoredPackage} row
|
||||
* @param {string} [pkgRoot] Override for testing -- defaults to the real repo ROOT. Lets a
|
||||
* test point fixRow's pin-rewrite at an isolated temp package.json instead of writing the
|
||||
* real, shared one, which other concurrently-running node --test files read at module
|
||||
* top-level (the same race class already fixed for the vendored .cjs copy).
|
||||
* @returns {{pinnedSpec: string | undefined, installedVersion: string | undefined}}
|
||||
*/
|
||||
function readPinState(row) {
|
||||
const pkgPath = path.join(ROOT, 'package.json');
|
||||
function readPinState(row, pkgRoot = ROOT) {
|
||||
const pkgPath = path.join(pkgRoot, 'package.json');
|
||||
const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8'));
|
||||
const pinnedSpec = pkg.devDependencies && pkg.devDependencies[row.name];
|
||||
const installedPkgPath = path.join(ROOT, 'node_modules', row.name, 'package.json');
|
||||
const installedPkgPath = path.join(pkgRoot, 'node_modules', row.name, 'package.json');
|
||||
let installedVersion;
|
||||
if (fs.existsSync(installedPkgPath)) {
|
||||
installedVersion = JSON.parse(fs.readFileSync(installedPkgPath, 'utf8')).version;
|
||||
@@ -250,9 +254,13 @@ function readPinState(row) {
|
||||
/**
|
||||
* Run all applicable freshness checks for one vendored package row.
|
||||
* @param {VendoredPackage} row
|
||||
* @param {string} [pkgRoot] Override for testing -- defaults to the real repo ROOT. Lets a
|
||||
* test point fixRow's pin-rewrite at an isolated temp package.json instead of writing the
|
||||
* real, shared one, which other concurrently-running node --test files read at module
|
||||
* top-level (the same race class already fixed for the vendored .cjs copy).
|
||||
* @returns {string[]} findings (empty when the row is fresh)
|
||||
*/
|
||||
function checkRow(row) {
|
||||
function checkRow(row, pkgRoot = ROOT) {
|
||||
const findings = [];
|
||||
|
||||
const cjsDrift = compareFiles(row.vendoredCjs, row.upstreamCjs);
|
||||
@@ -271,7 +279,7 @@ function checkRow(row) {
|
||||
findings.push(...checkHandAuthoredTwin(row));
|
||||
}
|
||||
|
||||
const { pinnedSpec, installedVersion } = readPinState(row);
|
||||
const { pinnedSpec, installedVersion } = readPinState(row, pkgRoot);
|
||||
if (!pinnedSpec) {
|
||||
findings.push(`package.json devDependencies.${row.name} is missing`);
|
||||
} else if (installedVersion === undefined) {
|
||||
@@ -303,9 +311,13 @@ function checkRow(row) {
|
||||
* after this runs, that is by design: the caller must not treat it as
|
||||
* fixed.
|
||||
* @param {VendoredPackage} row
|
||||
* @param {string} [pkgRoot] Override for testing -- defaults to the real repo ROOT. Lets a
|
||||
* test point fixRow's pin-rewrite at an isolated temp package.json instead of writing the
|
||||
* real, shared one, which other concurrently-running node --test files read at module
|
||||
* top-level (the same race class already fixed for the vendored .cjs copy).
|
||||
* @returns {string[]} findings remaining after the fix (empty when fully resolved)
|
||||
*/
|
||||
function fixRow(row) {
|
||||
function fixRow(row, pkgRoot = ROOT) {
|
||||
fs.copyFileSync(resolvePath(row.upstreamCjs), resolvePath(row.vendoredCjs));
|
||||
|
||||
if (row.twinKind === 'upstream-verbatim' && row.upstreamDts) {
|
||||
@@ -313,18 +325,18 @@ function fixRow(row) {
|
||||
if (row.srcTwin) fs.copyFileSync(resolvePath(row.upstreamDts), resolvePath(row.srcTwin));
|
||||
}
|
||||
|
||||
const { pinnedSpec, installedVersion } = readPinState(row);
|
||||
const { pinnedSpec, installedVersion } = readPinState(row, pkgRoot);
|
||||
if (pinnedSpec && installedVersion !== undefined) {
|
||||
const newPin = `${pinOperatorPrefix(pinnedSpec)}${installedVersion}`;
|
||||
if (newPin !== pinnedSpec) {
|
||||
const pkgPath = path.join(ROOT, 'package.json');
|
||||
const pkgPath = path.join(pkgRoot, 'package.json');
|
||||
const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8'));
|
||||
pkg.devDependencies[row.name] = newPin;
|
||||
fs.writeFileSync(pkgPath, `${JSON.stringify(pkg, null, 2)}\n`);
|
||||
}
|
||||
}
|
||||
|
||||
return checkRow(row);
|
||||
return checkRow(row, pkgRoot);
|
||||
}
|
||||
|
||||
function main() {
|
||||
|
||||
@@ -28,7 +28,8 @@ export const CANONICAL_EXACT: ReadonlySet<string> = new Set([
|
||||
'STATE-ARCHIVE.md', // state.cts's cmdStatePrune writes this at the .planning/ root
|
||||
'milestone.lock', // #3311: milestone (phase + session) claim (src/milestone-lock.cts); persistent, unlike the transient STATE.md.lock/WAITING.json
|
||||
'state.json', // #3227: machine-readable state contract published at step boundaries (src/state-contract.cts)
|
||||
'skill-manifest.json', // init.cts routeSkillManifest --write (project-scoped planning root, #3964)
|
||||
'skill-manifest.json', // init.cts cmdSkillManifest --write (project-scoped planning root, #3964)
|
||||
'PATTERNS.md', // #4282: graduated cross-phase patterns (workflows/graduation.md, `patterns` target) -- distinct from the per-phase NN-PATTERNS.md (templates/README.md)
|
||||
]);
|
||||
|
||||
// Pattern-match canonical file names (regex tests on the basename)
|
||||
|
||||
@@ -76,6 +76,17 @@ describe('isCanonicalPlanningFile', () => {
|
||||
assert.strictEqual(isCanonicalPlanningFile('WINDOWS.md'), true);
|
||||
});
|
||||
|
||||
test('#4282: PATTERNS.md (graduated cross-phase patterns) is a canonical .planning/ artifact', () => {
|
||||
// workflows/graduation.md's own graduation-target table instructs the agent to
|
||||
// append to .planning/PATTERNS.md for the `patterns` category. Before #4282 it
|
||||
// was absent from the registry, so validate health flagged it W019
|
||||
// "Unrecognized" with advice to archive/delete a file gsd-core itself produces.
|
||||
// Distinct from the per-phase NN-PATTERNS.md (templates/README.md), which lives
|
||||
// under a phase subdirectory and was never subject to W019 in the first place.
|
||||
assert.ok(CANONICAL_EXACT.has('PATTERNS.md'), 'PATTERNS.md must be in CANONICAL_EXACT');
|
||||
assert.strictEqual(isCanonicalPlanningFile('PATTERNS.md'), true);
|
||||
});
|
||||
|
||||
test('returns false for unrecognized file', () => {
|
||||
assert.strictEqual(isCanonicalPlanningFile('random-file.md'), false);
|
||||
});
|
||||
|
||||
@@ -22,6 +22,7 @@ const assert = require('node:assert/strict');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
|
||||
const {
|
||||
VENDORED,
|
||||
@@ -287,30 +288,46 @@ describe('#4573: pinOperatorPrefix / fixRow — mechanical --fix for Dependabot-
|
||||
});
|
||||
|
||||
test('fixRow resolves mechanical .cjs drift: mutated vendored js-yaml.cjs is byte-restored to match node_modules', (t) => {
|
||||
// Operates on an ISOLATED TEMP COPY, never the real gsd-core/bin/lib/vendor/js-yaml.cjs.
|
||||
// node --test runs files concurrently; the real file is `require()`-able by other test
|
||||
// files at any moment, and fs.copyFileSync's write is not atomic against a concurrent
|
||||
// reader on every filesystem. A prior version of this test wrote directly to the real
|
||||
// file and a concurrent require() elsewhere caught it mid-overwrite, reading a truncated
|
||||
// file and crashing an unrelated test with a SyntaxError -- confirmed via real CI logs,
|
||||
// not a hypothetical. upstreamCjs stays pointed at the real node_modules copy (read-only,
|
||||
// nothing writes to node_modules during tests, so sharing it is safe); only the
|
||||
// destination is redirected to a private temp path.
|
||||
const row = jsYamlRow();
|
||||
const vendoredAbs = path.join(REPO_ROOT, row.vendoredCjs);
|
||||
const upstreamAbs = path.join(REPO_ROOT, row.upstreamCjs);
|
||||
const original = fs.readFileSync(vendoredAbs, 'utf8');
|
||||
fs.writeFileSync(vendoredAbs, `${original}\n// mutated for test\n`);
|
||||
t.after(() => {
|
||||
// Safety net: fixRow copies FROM upstream, so the vendored file should
|
||||
// already be back in its original clean state — but re-copy from
|
||||
// upstream regardless in case an assertion above threw before fixRow
|
||||
// completed, so this test never leaves the tree dirty.
|
||||
fs.copyFileSync(upstreamAbs, vendoredAbs);
|
||||
});
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lint-vendored-deps-fixrow-'));
|
||||
const tempVendoredAbs = path.join(tmpDir, 'js-yaml.cjs');
|
||||
t.after(() => cleanup(tmpDir));
|
||||
|
||||
const findings = fixRow(row);
|
||||
const original = fs.readFileSync(upstreamAbs, 'utf8');
|
||||
fs.writeFileSync(tempVendoredAbs, `${original}\n// mutated for test\n`);
|
||||
const tempRow = { ...row, vendoredCjs: tempVendoredAbs };
|
||||
|
||||
const findings = fixRow(tempRow);
|
||||
assert.deepEqual(findings, [], `expected fixRow to leave zero findings, got: ${JSON.stringify(findings)}`);
|
||||
assert.ok(
|
||||
fs.readFileSync(vendoredAbs).equals(fs.readFileSync(upstreamAbs)),
|
||||
fs.readFileSync(tempVendoredAbs).equals(fs.readFileSync(upstreamAbs)),
|
||||
'expected the vendored .cjs to byte-equal node_modules/js-yaml/dist/js-yaml.js after fixRow',
|
||||
);
|
||||
});
|
||||
|
||||
test('fixRow does NOT mask a genuine hand-authored-twin incompatibility: a fake declared export still surfaces after --fix', (t) => {
|
||||
// fixRow's first line unconditionally copies onto row.vendoredCjs regardless of what this
|
||||
// test is exercising -- redirect it to a private temp path too, same reasoning as the
|
||||
// preceding test (avoid ANY write to the real, shared, concurrently-`require()`-able
|
||||
// gsd-core/bin/lib/vendor/js-yaml.cjs). srcTwin (a .d.cts type-only file, never require()'d
|
||||
// at runtime) is mutated in place as before -- this test's actual subject.
|
||||
const row = jsYamlRow();
|
||||
const srcTwinAbs = path.join(REPO_ROOT, row.srcTwin);
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lint-vendored-deps-fixrow-'));
|
||||
const tempVendoredAbs = path.join(tmpDir, 'js-yaml.cjs');
|
||||
t.after(() => cleanup(tmpDir));
|
||||
const tempRow = { ...row, vendoredCjs: tempVendoredAbs };
|
||||
|
||||
const original = fs.readFileSync(srcTwinAbs, 'utf8');
|
||||
fs.writeFileSync(
|
||||
srcTwinAbs,
|
||||
@@ -320,7 +337,7 @@ describe('#4573: pinOperatorPrefix / fixRow — mechanical --fix for Dependabot-
|
||||
fs.writeFileSync(srcTwinAbs, original);
|
||||
});
|
||||
|
||||
const findings = fixRow(row);
|
||||
const findings = fixRow(tempRow);
|
||||
assert.ok(
|
||||
findings.some((f) => f.includes('thisFixRowExportDoesNotExistAtRuntime')),
|
||||
`expected the hand-authored-twin finding to survive fixRow, got: ${JSON.stringify(findings)}`,
|
||||
@@ -328,25 +345,39 @@ describe('#4573: pinOperatorPrefix / fixRow — mechanical --fix for Dependabot-
|
||||
});
|
||||
|
||||
test('fixRow preserves the pin\'s original range-operator style when rewriting package.json', (t) => {
|
||||
// Isolates BOTH real-file writes fixRow makes: the vendoredCjs copy (redirected to a temp
|
||||
// path, same reasoning as the two tests above) AND the package.json pin rewrite this test
|
||||
// specifically exercises (redirected via fixRow's pkgRoot parameter to an isolated temp
|
||||
// root containing its own package.json + node_modules/js-yaml/package.json). Neither the
|
||||
// real vendored .cjs nor the real package.json is touched -- both are readable at module
|
||||
// top-level by other concurrently-running node --test files, the same race class already
|
||||
// fixed for the vendored .cjs.
|
||||
const row = jsYamlRow();
|
||||
const pkgPath = path.join(REPO_ROOT, 'package.json');
|
||||
const installedPkgPath = path.join(REPO_ROOT, 'node_modules', 'js-yaml', 'package.json');
|
||||
const installedVersion = JSON.parse(fs.readFileSync(installedPkgPath, 'utf8')).version;
|
||||
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lint-vendored-deps-fixrow-'));
|
||||
t.after(() => cleanup(tmpDir));
|
||||
const tempVendoredAbs = path.join(tmpDir, 'js-yaml.cjs');
|
||||
const tempRow = { ...row, vendoredCjs: tempVendoredAbs };
|
||||
|
||||
const originalContent = fs.readFileSync(pkgPath, 'utf8');
|
||||
t.after(() => {
|
||||
fs.writeFileSync(pkgPath, originalContent);
|
||||
});
|
||||
const realInstalledPkgPath = path.join(REPO_ROOT, 'node_modules', 'js-yaml', 'package.json');
|
||||
const installedVersion = JSON.parse(fs.readFileSync(realInstalledPkgPath, 'utf8')).version;
|
||||
|
||||
const pkg = JSON.parse(originalContent);
|
||||
const tempPkgRoot = path.join(tmpDir, 'pkgroot');
|
||||
const tempNodeModulesJsYamlDir = path.join(tempPkgRoot, 'node_modules', 'js-yaml');
|
||||
fs.mkdirSync(tempNodeModulesJsYamlDir, { recursive: true });
|
||||
const stalePin = installedVersion === '4.0.0' ? '~4.0.1' : '~4.0.0';
|
||||
pkg.devDependencies['js-yaml'] = stalePin;
|
||||
fs.writeFileSync(pkgPath, `${JSON.stringify(pkg, null, 2)}\n`);
|
||||
fs.writeFileSync(
|
||||
path.join(tempPkgRoot, 'package.json'),
|
||||
`${JSON.stringify({ devDependencies: { 'js-yaml': stalePin } }, null, 2)}\n`,
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(tempNodeModulesJsYamlDir, 'package.json'),
|
||||
`${JSON.stringify({ name: 'js-yaml', version: installedVersion }, null, 2)}\n`,
|
||||
);
|
||||
|
||||
const findings = fixRow(row);
|
||||
const findings = fixRow(tempRow, tempPkgRoot);
|
||||
assert.deepEqual(findings, [], `expected fixRow to leave zero findings, got: ${JSON.stringify(findings)}`);
|
||||
|
||||
const after = JSON.parse(fs.readFileSync(pkgPath, 'utf8'));
|
||||
const after = JSON.parse(fs.readFileSync(path.join(tempPkgRoot, 'package.json'), 'utf8'));
|
||||
const pinnedAfter = after.devDependencies['js-yaml'];
|
||||
assert.equal(
|
||||
pinnedAfter,
|
||||
|
||||
Reference in New Issue
Block a user