From 1d2b35e86136a5576ca93512993385db332cc249 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:46:46 -0300 Subject: [PATCH 1/5] test: cut the unit suite's critical path from 6.4 s to 1.7 s ci-background-task.test.mjs held the whole suite's wall time: its two blocking tests ran a 2.5 s and a 3 s busy loop one after the other inside a single worker, so --parallel=4 could not help (#89). - The blockers now sleep. spawnSync already freezes the parent's event loop, so a busy loop adds nothing but CPU starvation of the measured child (the flake in 692c0617). - The windows shrink with their margins kept: a 300 ms child behind a 1000 ms block (tolerance 250 ms), and a 1500 ms block with a 750 ms threshold for the 8 MiB writer, which took at most 190 ms under 4-core load. A startTask mutant that relays the child's pipes and times the await still fails both tests. - The two tests move to their own registered files so the parallel runner can place them on different workers. - The vacuity files start every gate at module load and await each result, instead of running 7 and 4 gates one after another. Measured on 4 cores: 6.43 s -> 1.72 s, 72 of 72 tests passing. Refs #89 --- scripts/config/test-contracts.json | 38 +++++ scripts/lib/ci-background-task.mjs | 4 +- .../tests/unit/agent-score-vacuity.test.mjs | 95 +++++++----- ...-background-task-blocked-lifetime.test.mjs | 70 +++++++++ ...background-task-pipe-backpressure.test.mjs | 73 +++++++++ .../tests/unit/ci-background-task.fixture.mjs | 36 +++++ .../tests/unit/ci-background-task.test.mjs | 145 +----------------- .../tests/unit/skill-score-vacuity.test.mjs | 62 +++++--- 8 files changed, 319 insertions(+), 204 deletions(-) create mode 100644 scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs create mode 100644 scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs create mode 100644 scripts/tests/unit/ci-background-task.fixture.mjs diff --git a/scripts/config/test-contracts.json b/scripts/config/test-contracts.json index 601d2b6f..e89f09e3 100644 --- a/scripts/config/test-contracts.json +++ b/scripts/config/test-contracts.json @@ -183,6 +183,44 @@ "selection": { "kind": "node-test-file", "selector": "scripts/tests/unit/ci-background-task.test.mjs", + "expected_min": 4 + }, + "platforms": ["linux", "macos"], + "toolchains": ["node-24"], + "release_role": "gate", + "replaces": [], + "skips": [] + }, + { + "id": "unit-ci-background-task-blocked-lifetime", + "version": 1, + "title": "Ci Background Task blocked lifetime unit contracts", + "owner": { + "suite": "scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs", + "layer": "unit" + }, + "selection": { + "kind": "node-test-file", + "selector": "scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs", + "expected_min": 1 + }, + "platforms": ["linux", "macos"], + "toolchains": ["node-24"], + "release_role": "gate", + "replaces": [], + "skips": [] + }, + { + "id": "unit-ci-background-task-pipe-backpressure", + "version": 1, + "title": "Ci Background Task pipe backpressure unit contracts", + "owner": { + "suite": "scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs", + "layer": "unit" + }, + "selection": { + "kind": "node-test-file", + "selector": "scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs", "expected_min": 1 }, "platforms": ["linux", "macos"], diff --git a/scripts/lib/ci-background-task.mjs b/scripts/lib/ci-background-task.mjs index b5852dec..000266c1 100644 --- a/scripts/lib/ci-background-task.mjs +++ b/scripts/lib/ci-background-task.mjs @@ -13,8 +13,8 @@ export const RUN_ORIGIN_NS = process.hrtime.bigint(); // runs, which is the same defect with a smaller error bar. The launcher below is a separate OS // process, so the kernel schedules it independently of this loop. It measures its own child with one // monotonic clock and reports the lifetime; the parent only has to place that interval on its own -// timeline. Verify: run a 1 s child behind a 2.5 s blocking `spawnSync` and read the recorded -// duration (`bun test ./scripts/tests/unit/ci-background-task.test.mjs`). +// timeline. Verify: run a 300 ms child behind a 1 s blocking `spawnSync` and read the recorded +// duration (`bun test ./scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs`). const LAUNCHER_SOURCE = [ "import { spawn } from 'node:child_process';", "import fs from 'node:fs';", diff --git a/scripts/tests/unit/agent-score-vacuity.test.mjs b/scripts/tests/unit/agent-score-vacuity.test.mjs index a8e248ad..e9687a3a 100644 --- a/scripts/tests/unit/agent-score-vacuity.test.mjs +++ b/scripts/tests/unit/agent-score-vacuity.test.mjs @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -94,37 +94,60 @@ delete baseEnv.CARGO_TARGET_DIR; delete baseEnv.GITHUB_ACTIONS; delete baseEnv.NODE_OPTIONS; +// Each gate is a full `scripts/ci.mjs` run that spends seconds waiting on its children, so every +// mode starts when the module loads and the runs overlap; each test awaits only its own mode. const runCi = (mode) => { const needsFixture = mode === 'empty-directory' || mode.startsWith('missing-'); - const result = spawnSync( - process.execPath, - [...(needsFixture ? [`--import=${fixtureHook}`] : []), 'scripts/ci.mjs', '--plugin', TARGET], - { - cwd: REPO, - encoding: 'utf8', - timeout: 180_000, - env: { - ...baseEnv, - PATH: `${shimDir}${path.delimiter}${process.env.PATH ?? ''}`, - DOCKS_AGENT_SCORE_MODE: mode, - ...(needsFixture - ? { - DOCKS_MISSING_ROOT: - mode === 'missing-skills-root' ? SKILLS_ROOT : mode === 'missing-agents-root' ? AGENTS_ROOT : '', - } - : {}), + return new Promise((resolve) => { + const child = spawn( + process.execPath, + [...(needsFixture ? [`--import=${fixtureHook}`] : []), 'scripts/ci.mjs', '--plugin', TARGET], + { + cwd: REPO, + timeout: 180_000, + env: { + ...baseEnv, + PATH: `${shimDir}${path.delimiter}${process.env.PATH ?? ''}`, + DOCKS_AGENT_SCORE_MODE: mode, + ...(needsFixture + ? { + DOCKS_MISSING_ROOT: + mode === 'missing-skills-root' ? SKILLS_ROOT : mode === 'missing-agents-root' ? AGENTS_ROOT : '', + } + : {}), + }, }, - }, - ); - return { status: result.status, output: `${result.stdout ?? ''}${result.stderr ?? ''}` }; + ); + let stdout = ''; + let stderr = ''; + child.stdout.setEncoding('utf8').on('data', (chunk) => { + stdout += chunk; + }); + child.stderr.setEncoding('utf8').on('data', (chunk) => { + stderr += chunk; + }); + // A spawn failure resolves with a null status, as `spawnSync` reported it, and the error in the + // output, so the waiting test fails instead of hanging; a `close` after `error` is then a no-op. + child.on('error', (error) => resolve({ status: null, output: `${stdout}${stderr}${error.stack ?? error}` })); + child.on('close', (status) => resolve({ status, output: `${stdout}${stderr}` })); + }); }; +const gates = new Map( + ['full', 'missing-skills-root', 'missing-agents-root', 'empty-directory', 'crash', 'mismatch', 'non-finite'].map( + (mode) => [mode, runCi(mode)], + ), +); + const rx = (literal) => new RegExp(literal.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); -test.after(() => fs.rmSync(shimDir, { recursive: true, force: true })); +test.after(async () => { + await Promise.all(gates.values()); + fs.rmSync(shimDir, { recursive: true, force: true }); +}); -test('a scorer covering every agent on disk keeps the gate green', () => { - const { status, output } = runCi('full'); +test('a scorer covering every agent on disk keeps the gate green', async () => { + const { status, output } = await gates.get('full'); assert.equal(status, 0, output); assert.match( output, @@ -134,35 +157,35 @@ test('a scorer covering every agent on disk keeps the gate green', () => { ); assert.match(output, rx(`${TARGET} agents per-file all ≥ 10`)); }); -test('a missing declared skills root fails explicitly', () => { - const { status, output } = runCi('missing-skills-root'); +test('a missing declared skills root fails explicitly', async () => { + const { status, output } = await gates.get('missing-skills-root'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} declared skills root missing: ${SKILLS_ROOT}`)); }); -test('a missing declared agents root fails explicitly', () => { - const { status, output } = runCi('missing-agents-root'); +test('a missing declared agents root fails explicitly', async () => { + const { status, output } = await gates.get('missing-agents-root'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} declared agents root missing: ${AGENTS_ROOT}`)); }); -test('an empty declared agents directory fails instead of passing vacuously', () => { - const { status, output } = runCi('empty-directory'); +test('an empty declared agents directory fails instead of passing vacuously', async () => { + const { status, output } = await gates.get('empty-directory'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} declared agents root is empty — no agent score set to gate: ${AGENTS_ROOT}`)); assert.doesNotMatch(output, rx(`${TARGET} agents per-file all ≥ 10`)); }); -test('an agent scorer that exits non-zero fails the gate naming the command', () => { - const { status, output } = runCi('crash'); +test('an agent scorer that exits non-zero fails the gate naming the command', async () => { + const { status, output } = await gates.get('crash'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} agent scorer exited 3 — no score set to gate`)); assert.match(output, rx(`bun scripts/agents/score.mjs --per-file ${AGENTS_ROOT}`)); assert.doesNotMatch(output, rx(`${TARGET} agents per-file all ≥ 10`)); }); -test('a same-size score set with a wrong row name fails corroboration', () => { - const { status, output } = runCi('mismatch'); +test('a same-size score set with a wrong row name fails corroboration', async () => { + const { status, output } = await gates.get('mismatch'); assert.notEqual(status, 0); assert.match( output, @@ -175,8 +198,8 @@ test('a same-size score set with a wrong row name fails corroboration', () => { assert.doesNotMatch(output, rx(`${TARGET} agents per-file all ≥ 10`)); }); -test('a non-finite agent score fails before floor arithmetic', () => { - const { status, output } = runCi('non-finite'); +test('a non-finite agent score fails before floor arithmetic', async () => { + const { status, output } = await gates.get('non-finite'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} agent scorer produced non-finite score(s): ${EXPECTED[0]} NaN`)); assert.doesNotMatch(output, rx(`${TARGET} agents per-file all ≥ 10`)); diff --git a/scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs b/scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs new file mode 100644 index 00000000..516c0837 --- /dev/null +++ b/scripts/tests/unit/ci-background-task-blocked-lifetime.test.mjs @@ -0,0 +1,70 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { startTask } from '../../lib/ci-background-task.mjs'; +import { blockEventLoop, fixture } from './ci-background-task.fixture.mjs'; + +// An await-shaped duration spans the whole block, so it lands at least MIN_AWAITED_MS - CHILD_MS +// (650 ms) away from the child's own report: well outside the 250 ms tolerance, which still absorbs +// the launcher's startup and the parent's placement of the interval. +const CHILD_MS = 300; +const BLOCK_MS = 1000; +// Timer granularity may shave a little off the nominal block, never more. +const MIN_AWAITED_MS = 950; +const SELF_REPORT_TOLERANCE_MS = 250; + +// The child reports its own lifetime, and the recorded duration must match that report. A wall-clock +// constant would be wrong here: under load this host has been observed running a nominal one-second +// child for 2.5 s, which is a true measurement, not a defect. Matching the child's self-report still +// fails hard against await-lifetime accounting, because that reports the blocking window instead. +test('background duration follows the child lifetime while the main thread is blocked', async () => { + const f = fixture(); + try { + const tasks = []; + const commands = []; + const reportPath = path.join(f.root, 'child-lifetime.json'); + const childSource = [ + "import fs from 'node:fs';", + 'const bootNs = process.hrtime.bigint();', + 'setTimeout(() => {', + ' const selfMs = Number(process.hrtime.bigint() - bootNs) / 1e6;', + ' fs.writeFileSync(process.argv[1], JSON.stringify({ self_ms: selfMs }));', + ' process.exit(0);', + `}, ${CHILD_MS});`, + ].join('\n'); + const wallStartedAt = process.hrtime.bigint(); + const pending = startTask( + 'timed child', + process.execPath, + ['--input-type=module', '--eval', childSource, '--', reportPath], + { + tasks, + commands, + artifactRoot: f.artifactRoot, + errorStream: f.errorStream, + }, + ); + + const blocker = blockEventLoop(BLOCK_MS); + assert.equal(blocker.status, 0, blocker.stderr); + assert.equal(await pending, true); + const awaitedWallMs = Number((process.hrtime.bigint() - wallStartedAt) / 1_000_000n); + const childSelfMs = JSON.parse(fs.readFileSync(reportPath, 'utf8')).self_ms; + + assert.equal(tasks.length, 1); + assert.ok(awaitedWallMs >= MIN_AWAITED_MS, `blocking window collapsed: awaited ${awaitedWallMs} ms`); + assert.ok( + Math.abs(tasks[0].duration_ms - childSelfMs) <= SELF_REPORT_TOLERANCE_MS, + `recorded ${tasks[0].duration_ms} ms but the child measured ${Math.round(childSelfMs)} ms`, + ); + assert.ok( + tasks[0].duration_ms <= childSelfMs + SELF_REPORT_TOLERANCE_MS, + `recorded ${tasks[0].duration_ms} ms is await-shaped against a ${Math.round(childSelfMs)} ms child`, + ); + assert.ok(tasks[0].duration_ms <= awaitedWallMs, `recorded ${tasks[0].duration_ms} ms exceeds the awaited window`); + assert.equal(commands[0].duration_ms, tasks[0].duration_ms); + } finally { + f.cleanup(); + } +}); diff --git a/scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs b/scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs new file mode 100644 index 00000000..76413477 --- /dev/null +++ b/scripts/tests/unit/ci-background-task-pipe-backpressure.test.mjs @@ -0,0 +1,73 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { startTask } from '../../lib/ci-background-task.mjs'; +import { blockEventLoop, fixture } from './ci-background-task.fixture.mjs'; + +// The 8 MiB writer took at most 190 ms under 4-core load, so an unthrottled run clears the threshold +// about fourfold, while a throttled one is held for the whole block, twice the threshold. +const BLOCK_MS = 1500; +const THROTTLED_THRESHOLD_MS = 750; +const MAX_ATTEMPTS = 3; + +// A chatty child must not be throttled by the orchestrator's blocked event loop. When the parent +// relayed the child's output, one pipe buffer of backpressure suspended the child until the join +// point, and the recorded lifetime became the blocking window. The full gate reproduced exactly +// that: `bun run check:js` runs in about one second and was recorded at 356 s. +// +// Best-of-N on purpose, and the reason is a property of what is being measured. "The parent does +// not FORCE serialization" is disproved by a single clean observation, whereas one slow +// observation proves nothing: on a loaded host the kernel can legitimately starve the writer and +// the recorded lifetime approaches the block by scheduling alone. That is what a single sample kept +// reporting while the blocker still busy-looped — 3013 ms against a 3000 ms block — with the same +// bytes passing on the next run. A real regression reintroduces backpressure deterministically and +// is throttled on EVERY attempt, so this still fails closed on the defect it exists to catch while +// no longer failing on the machine's load. +test('a child that outwrites one pipe buffer is not throttled by the blocked parent', async () => { + const chatty = [ + 'let written = 0;', + 'const tick = () => {', + ' while (written < 8 * 1024) {', + ' written += 1;', + " if (!process.stdout.write('x'.repeat(1024))) {", + " process.stdout.once('drain', tick);", + ' return;', + ' }', + ' }', + ' process.exit(0);', + '};', + 'tick();', + ].join('\n'); + + const attempts = []; + for ( + let attempt = 0; + attempt < MAX_ATTEMPTS && (attempts.length === 0 || attempts.at(-1) >= THROTTLED_THRESHOLD_MS); + attempt += 1 + ) { + const f = fixture(); + try { + const tasks = []; + const commands = []; + const pending = startTask('chatty child', process.execPath, ['--input-type=module', '--eval', chatty], { + tasks, + commands, + artifactRoot: f.artifactRoot, + errorStream: f.errorStream, + }); + + const blocker = blockEventLoop(BLOCK_MS); + assert.equal(blocker.status, 0, blocker.stderr); + assert.equal(await pending, true); + assert.equal(commands[0].status, 'passed'); + attempts.push(tasks[0].duration_ms); + } finally { + f.cleanup(); + } + } + + assert.ok( + attempts.some((duration) => duration < THROTTLED_THRESHOLD_MS), + `an 8 MiB writer behind a ${BLOCK_MS} ms block was throttled on every attempt (${attempts.join(' ms, ')} ms), ` + + 'so the parent serializes it rather than the host merely being busy', + ); +}); diff --git a/scripts/tests/unit/ci-background-task.fixture.mjs b/scripts/tests/unit/ci-background-task.fixture.mjs new file mode 100644 index 00000000..3552d296 --- /dev/null +++ b/scripts/tests/unit/ci-background-task.fixture.mjs @@ -0,0 +1,36 @@ +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { Writable } from 'node:stream'; + +// Shared by the ci-background-task unit files, which are split so Bun's parallel runner can place +// the two timing-sensitive cases on separate workers. +export function fixture() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ci-background-task-test-')); + const artifactRoot = path.join(root, 'artifacts'); + fs.mkdirSync(artifactRoot); + let errors = ''; + const errorStream = new Writable({ + write(chunk, _encoding, callback) { + errors += chunk.toString(); + callback(); + }, + }); + return { + root, + artifactRoot, + errorStream, + errors: () => errors, + cleanup: () => fs.rmSync(root, { recursive: true, force: true }), + }; +} + +// Holds this process's event loop for `milliseconds`, the way `ci.mjs` does between join points. +// `spawnSync` itself blocks the event loop, so the blocker needs no CPU, and a busy loop starves the +// measured child (the flake in commit 692c0617). +export function blockEventLoop(milliseconds) { + return spawnSync(process.execPath, ['--input-type=module', '--eval', `setTimeout(() => {}, ${milliseconds})`], { + encoding: 'utf8', + }); +} diff --git a/scripts/tests/unit/ci-background-task.test.mjs b/scripts/tests/unit/ci-background-task.test.mjs index 506cc473..43d09335 100644 --- a/scripts/tests/unit/ci-background-task.test.mjs +++ b/scripts/tests/unit/ci-background-task.test.mjs @@ -1,31 +1,10 @@ import assert from 'node:assert/strict'; import { spawnSync } from 'node:child_process'; import fs from 'node:fs'; -import os from 'node:os'; import path from 'node:path'; -import { Writable } from 'node:stream'; import test from 'node:test'; import { beginCommand, startTask, summarizeCommands } from '../../lib/ci-background-task.mjs'; - -function fixture() { - const root = fs.mkdtempSync(path.join(os.tmpdir(), 'ci-background-task-test-')); - const artifactRoot = path.join(root, 'artifacts'); - fs.mkdirSync(artifactRoot); - let errors = ''; - const errorStream = new Writable({ - write(chunk, _encoding, callback) { - errors += chunk.toString(); - callback(); - }, - }); - return { - root, - artifactRoot, - errorStream, - errors: () => errors, - cleanup: () => fs.rmSync(root, { recursive: true, force: true }), - }; -} +import { fixture } from './ci-background-task.fixture.mjs'; function nodeDelay(milliseconds, exitCode = 0) { return [ @@ -34,128 +13,6 @@ function nodeDelay(milliseconds, exitCode = 0) { ]; } -// The child reports its own lifetime, and the recorded duration must match that report. A wall-clock -// constant would be wrong here: under load this host has been observed running a nominal one-second -// child for 2.5 s, which is a true measurement, not a defect. Matching the child's self-report still -// fails hard against await-lifetime accounting, because that reports the blocking window instead. -test('background duration follows the child lifetime while the main thread is blocked', async () => { - const f = fixture(); - try { - const tasks = []; - const commands = []; - const reportPath = path.join(f.root, 'child-lifetime.json'); - const childSource = [ - "import fs from 'node:fs';", - 'const bootNs = process.hrtime.bigint();', - 'setTimeout(() => {', - ' const selfMs = Number(process.hrtime.bigint() - bootNs) / 1e6;', - ' fs.writeFileSync(process.argv[1], JSON.stringify({ self_ms: selfMs }));', - ' process.exit(0);', - '}, 1000);', - ].join('\n'); - const wallStartedAt = process.hrtime.bigint(); - const pending = startTask( - 'timed child', - process.execPath, - ['--input-type=module', '--eval', childSource, '--', reportPath], - { - tasks, - commands, - artifactRoot: f.artifactRoot, - errorStream: f.errorStream, - }, - ); - - const blocker = spawnSync( - process.execPath, - ['--input-type=module', '--eval', 'const until = Date.now() + 2500; while (Date.now() < until) {}'], - { encoding: 'utf8' }, - ); - assert.equal(blocker.status, 0, blocker.stderr); - assert.equal(await pending, true); - const awaitedWallMs = Number((process.hrtime.bigint() - wallStartedAt) / 1_000_000n); - const childSelfMs = JSON.parse(fs.readFileSync(reportPath, 'utf8')).self_ms; - - assert.equal(tasks.length, 1); - assert.ok(awaitedWallMs >= 2400, `blocking window collapsed: awaited ${awaitedWallMs} ms`); - assert.ok( - Math.abs(tasks[0].duration_ms - childSelfMs) <= 250, - `recorded ${tasks[0].duration_ms} ms but the child measured ${Math.round(childSelfMs)} ms`, - ); - assert.ok( - tasks[0].duration_ms <= childSelfMs + 250, - `recorded ${tasks[0].duration_ms} ms is await-shaped against a ${Math.round(childSelfMs)} ms child`, - ); - assert.ok(tasks[0].duration_ms <= awaitedWallMs, `recorded ${tasks[0].duration_ms} ms exceeds the awaited window`); - assert.equal(commands[0].duration_ms, tasks[0].duration_ms); - } finally { - f.cleanup(); - } -}); - -// A chatty child must not be throttled by the orchestrator's blocked event loop. When the parent -// relayed the child's output, one pipe buffer of backpressure suspended the child until the join -// point, and the recorded lifetime became the blocking window. The full gate reproduced exactly -// that: `bun run check:js` runs in about one second and was recorded at 356 s. -// -// Best-of-N on purpose, and the reason is a property of what is being measured. "The parent does -// not FORCE serialization" is disproved by a single clean observation, whereas one slow -// observation proves nothing: the blocker pins a core for its whole window, so on a loaded host -// the kernel can legitimately starve the writer and the recorded lifetime approaches the block by -// scheduling alone. That is what a single sample kept reporting here — 3013 ms against a 3000 ms -// block — with the same bytes passing on the next run. A real regression reintroduces backpressure -// deterministically and is throttled on EVERY attempt, so this still fails closed on the defect it -// exists to catch while no longer failing on the machine's load. The threshold is unchanged. -test('a child that outwrites one pipe buffer is not throttled by the blocked parent', async () => { - const chatty = [ - 'let written = 0;', - 'const tick = () => {', - ' while (written < 8 * 1024) {', - ' written += 1;', - " if (!process.stdout.write('x'.repeat(1024))) {", - " process.stdout.once('drain', tick);", - ' return;', - ' }', - ' }', - ' process.exit(0);', - '};', - 'tick();', - ].join('\n'); - - const attempts = []; - for (let attempt = 0; attempt < 3 && (attempts.length === 0 || attempts.at(-1) >= 1500); attempt += 1) { - const f = fixture(); - try { - const tasks = []; - const commands = []; - const pending = startTask('chatty child', process.execPath, ['--input-type=module', '--eval', chatty], { - tasks, - commands, - artifactRoot: f.artifactRoot, - errorStream: f.errorStream, - }); - - const blocker = spawnSync( - process.execPath, - ['--input-type=module', '--eval', 'const until = Date.now() + 3000; while (Date.now() < until) {}'], - { encoding: 'utf8' }, - ); - assert.equal(blocker.status, 0, blocker.stderr); - assert.equal(await pending, true); - assert.equal(commands[0].status, 'passed'); - attempts.push(tasks[0].duration_ms); - } finally { - f.cleanup(); - } - } - - assert.ok( - attempts.some((duration) => duration < 1500), - `an 8 MiB writer behind a 3 s block was throttled on every attempt (${attempts.join(' ms, ')} ms), ` + - 'so the parent serializes it rather than the host merely being busy', - ); -}); - test('summarizeCommands accounts for concurrent background commands', async () => { const f = fixture(); try { diff --git a/scripts/tests/unit/skill-score-vacuity.test.mjs b/scripts/tests/unit/skill-score-vacuity.test.mjs index bb2ae799..904187dd 100644 --- a/scripts/tests/unit/skill-score-vacuity.test.mjs +++ b/scripts/tests/unit/skill-score-vacuity.test.mjs @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -77,27 +77,45 @@ const baseEnv = { ...process.env }; delete baseEnv.CARGO_TARGET_DIR; delete baseEnv.GITHUB_ACTIONS; -const runCi = (mode) => { - const result = spawnSync(process.execPath, ['scripts/ci.mjs', '--plugin', TARGET], { - cwd: REPO, - encoding: 'utf8', - timeout: 180_000, - env: { - ...baseEnv, - PATH: `${shimDir}${path.delimiter}${process.env.PATH ?? ''}`, - DOCKS_SCORE_MODE: mode, - }, +// Each gate is a full `scripts/ci.mjs` run that spends seconds waiting on its children, so every +// mode starts when the module loads and the runs overlap; each test awaits only its own mode. +const runCi = (mode) => + new Promise((resolve) => { + const child = spawn(process.execPath, ['scripts/ci.mjs', '--plugin', TARGET], { + cwd: REPO, + timeout: 180_000, + env: { + ...baseEnv, + PATH: `${shimDir}${path.delimiter}${process.env.PATH ?? ''}`, + DOCKS_SCORE_MODE: mode, + }, + }); + let stdout = ''; + let stderr = ''; + child.stdout.setEncoding('utf8').on('data', (chunk) => { + stdout += chunk; + }); + child.stderr.setEncoding('utf8').on('data', (chunk) => { + stderr += chunk; + }); + // A spawn failure resolves with a null status, as `spawnSync` reported it, and the error in the + // output, so the waiting test fails instead of hanging; a `close` after `error` is then a no-op. + child.on('error', (error) => resolve({ status: null, output: `${stdout}${stderr}${error.stack ?? error}` })); + child.on('close', (status) => resolve({ status, output: `${stdout}${stderr}` })); }); - return { status: result.status, output: `${result.stdout ?? ''}${result.stderr ?? ''}` }; -}; + +const gates = new Map(['full', 'empty', 'subset', 'crash'].map((mode) => [mode, runCi(mode)])); const rx = (literal) => new RegExp(literal.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); -test.after(() => fs.rmSync(shimDir, { recursive: true, force: true })); +test.after(async () => { + await Promise.all(gates.values()); + fs.rmSync(shimDir, { recursive: true, force: true }); +}); // The control case. Without it, a gate broken outright would satisfy every case below. -test('a scorer covering every skill on disk keeps the gate green', () => { - const { status, output } = runCi('full'); +test('a scorer covering every skill on disk keeps the gate green', async () => { + const { status, output } = await gates.get('full'); assert.equal(status, 0, output); assert.match( output, @@ -108,8 +126,8 @@ test('a scorer covering every skill on disk keeps the gate green', () => { assert.match(output, rx(`${TARGET} skills per-file all clear`)); }); -test('an empty score set fails the gate instead of reporting all clear', () => { - const { status, output } = runCi('empty'); +test('an empty score set fails the gate instead of reporting all clear', async () => { + const { status, output } = await gates.get('empty'); assert.notEqual(status, 0); assert.match( output, @@ -122,8 +140,8 @@ test('an empty score set fails the gate instead of reporting all clear', () => { assert.doesNotMatch(output, rx(`${TARGET} skills per-file all clear`)); }); -test('a short score set fails the gate and names the unscored skill', () => { - const { status, output } = runCi('subset'); +test('a short score set fails the gate and names the unscored skill', async () => { + const { status, output } = await gates.get('subset'); assert.notEqual(status, 0); assert.match( output, @@ -135,8 +153,8 @@ test('a short score set fails the gate and names the unscored skill', () => { assert.doesNotMatch(output, rx(`${TARGET} skills per-file all clear`)); }); -test('a scorer that exits non-zero fails the gate naming the command', () => { - const { status, output } = runCi('crash'); +test('a scorer that exits non-zero fails the gate naming the command', async () => { + const { status, output } = await gates.get('crash'); assert.notEqual(status, 0); assert.match(output, rx(`${TARGET} skill scorer exited 3 — no score set to gate`)); assert.match( From 5efaf4ef90b64f829e79a9775a8476028a346bc0 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Thu, 22 Oct 2026 23:48:34 -0300 Subject: [PATCH 2/5] test: run the plan-CLI cases in parallel through node --test scripts/tests/plan-cli.mjs ran about 32 cases in one process against one shared fake-gh state and one scratch repository, so its 198 plan.mjs starts ran strictly in series: 12.4 s, about 95% of each plugin gate (#89). Each case is now a node:test test with its own context (scratch repo, fake-gh state seeded with the plan labels, environment), built by scripts/tests/plan-cli/harness.mjs. The 44 cases sit in 9 files balanced by measured cost, and Node's runner gives each file its own process. The frozen-steps case and the status-transitions case that reused its plan are one test. scripts/tests/plan-cli.mjs stays the entry, so the gate invocation, the targeting call counts, the test-contract registry, and the validator-table row do not change; it runs the case files with --test-concurrency set to the available parallelism. plan.mjs still runs on Node through process.execPath. A new case pins the contract's list order (open plans first, then closed, each ascending) over a mixed state. The shared state used to exercise that path only by accident; plan.mjs line coverage stays at 996 of 1006 lines. Measured on 4 cores: 12.4 s serial -> 3.9 s. Refs #89 --- package.json | 2 +- scripts/AGENTS.md | 7 +- scripts/tests/author-tooling.mjs | 6 +- scripts/tests/plan-cli.mjs | 1072 +---------------- .../tests/plan-cli/archive-closer.test.mjs | 97 ++ .../plan-cli/archive-retire-list.test.mjs | 175 +++ .../tests/plan-cli/fenced-examples.test.mjs | 67 ++ scripts/tests/plan-cli/harness.mjs | 224 ++++ .../tests/plan-cli/indented-and-mode.test.mjs | 94 ++ scripts/tests/plan-cli/records.test.mjs | 178 +++ scripts/tests/plan-cli/reviews.test.mjs | 146 +++ scripts/tests/plan-cli/status.test.mjs | 94 ++ scripts/tests/plan-cli/steps-table.test.mjs | 82 ++ scripts/tests/plan-cli/write-gates.test.mjs | 103 ++ 14 files changed, 1291 insertions(+), 1056 deletions(-) create mode 100644 scripts/tests/plan-cli/archive-closer.test.mjs create mode 100644 scripts/tests/plan-cli/archive-retire-list.test.mjs create mode 100644 scripts/tests/plan-cli/fenced-examples.test.mjs create mode 100644 scripts/tests/plan-cli/harness.mjs create mode 100644 scripts/tests/plan-cli/indented-and-mode.test.mjs create mode 100644 scripts/tests/plan-cli/records.test.mjs create mode 100644 scripts/tests/plan-cli/reviews.test.mjs create mode 100644 scripts/tests/plan-cli/status.test.mjs create mode 100644 scripts/tests/plan-cli/steps-table.test.mjs create mode 100644 scripts/tests/plan-cli/write-gates.test.mjs diff --git a/package.json b/package.json index 45d5b74f..a03831ee 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "test:unit": "bun test --parallel=4 --timeout=180000 ./scripts/tests/unit/*.test.mjs", "test:observability": "bun scripts/tests/ci-observability.mjs", "test:contracts": "bun scripts/tests/test-contracts.mjs", - "test:coverage": "node --test --experimental-test-coverage --test-coverage-include='plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs' --test-coverage-exclude='**/scripts/tests/**' --test-coverage-exclude='**/plugins/**/test/**' scripts/tests/plan-cli.mjs" + "test:coverage": "node --test --experimental-test-coverage --test-coverage-include='plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs' --test-coverage-exclude='**/scripts/tests/**' --test-coverage-exclude='**/plugins/**/test/**' scripts/tests/plan-cli/*.test.mjs" }, "trustedDependencies": [], "devDependencies": { diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 41577c88..0375883a 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -24,7 +24,8 @@ owned by `.github/AGENTS.md` (PR topology). The changed-path → shard mapping i The root `AGENTS.md` (Plans) decides when a change needs a canonical plan. -The live plan author suite runs the helper smoke in `scripts/tests/plan-cli.mjs` +The live plan author suite is the set of plan-CLI case files in +`scripts/tests/plan-cli/`. The `scripts/tests/plan-cli.mjs` entry runs them against the shipped `plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs`. The canonical issue-body contract lives in @@ -62,10 +63,10 @@ Plugin behavior stays registry-driven: extend descriptor capabilities rather tha | `config/read-floor.mjs` | reads per-file floors from `scoring.json` | — | | `tests/skill-trigger-collision.mjs` | cross-skill trigger-overlap audit — fails on a ≥5-token unrouted pair (`--report` prints the matrix) | pass/fail | | `tests/idempotency.mjs` | content-hash determinism + every stored hash in sync | pass/fail | -| `tests/plan-cli.mjs` | smoke-tests the shipped plan helper (`plan.mjs`) | pass/fail | +| `tests/plan-cli.mjs` | runs the plan-CLI case files in `tests/plan-cli/` in parallel through Node's test runner against the shipped plan helper (`plan.mjs`) | pass/fail | | `tests/ci-observability.mjs` | validates command timing records, wall-time reconstruction, and CI host metadata | pass/fail | | `tests/test-contracts.mjs` | validates the closed test-contract registry and its discovered, registered, selected, and executed sets | pass/fail | -| `tests/author-tooling.mjs` | author-tool contracts: Biome rejects a syntax defect, combined skill validation, tree/guard operational failures, skills-guard spawn failure; this table lists every `scripts/**/*.mjs` outside `lib/`, `tests/unit/`, and the non-validator entry points `release.mjs`/`ci-target.mjs`/`capture-tdd-red.mjs`, and every first-column `.mjs` path resolves from `scripts/` or the repo root | pass/fail | +| `tests/author-tooling.mjs` | author-tool contracts: Biome rejects a syntax defect, combined skill validation, tree/guard operational failures, skills-guard spawn failure; this table lists every `scripts/**/*.mjs` outside `lib/`, `tests/unit/`, `tests/plan-cli/`, and the non-validator entry points `release.mjs`/`ci-target.mjs`/`capture-tdd-red.mjs`, and every first-column `.mjs` path resolves from `scripts/` or the repo root | pass/fail | | `tests/ci-plugin-targeting.mjs` | CI targeting and release contracts: shard selection, `ci.yml` trigger block, release module and dry-run safety (`--unit` in the gate) | pass/fail | | shellcheck (target-selected) | `-S warning` over selected plugins' `hooks/*.sh`, via `shellHooks(p)`; a full invocation selects every plugin | pass/warn | diff --git a/scripts/tests/author-tooling.mjs b/scripts/tests/author-tooling.mjs index fa226bf1..7d084e3c 100755 --- a/scripts/tests/author-tooling.mjs +++ b/scripts/tests/author-tooling.mjs @@ -183,7 +183,8 @@ function testTreeGuardOperationalFailures() { // scripts/AGENTS.md validator table: every validator script has a row, every row resolves. // lib/ holds shared modules (a lib validator may still have a row); tests/unit/ runs as one -// suite; the allowlist names entry points that are not validators. +// suite, and the tests/plan-cli/ case files run as one suite through the tests/plan-cli.mjs row; +// the allowlist names entry points that are not validators. const NON_VALIDATOR_SCRIPTS = new Set(['release.mjs', 'ci-target.mjs', 'capture-tdd-red.mjs']); function validatorTableErrors(root) { @@ -199,7 +200,8 @@ function validatorTableErrors(root) { const full = path.join(dir, e.name); const rel = path.relative(scriptsDir, full).split(path.sep).join('/'); if (e.isDirectory()) { - if (e.name !== 'node_modules' && rel !== 'lib' && rel !== 'tests/unit') walkScripts(full); + if (e.name !== 'node_modules' && rel !== 'lib' && rel !== 'tests/unit' && rel !== 'tests/plan-cli') + walkScripts(full); } else if (e.isFile() && e.name.endsWith('.mjs') && !NON_VALIDATOR_SCRIPTS.has(rel)) onDisk.push(rel); } })(scriptsDir); diff --git a/scripts/tests/plan-cli.mjs b/scripts/tests/plan-cli.mjs index 9bccb6a5..b3aeb9d4 100755 --- a/scripts/tests/plan-cli.mjs +++ b/scripts/tests/plan-cli.mjs @@ -1,1057 +1,29 @@ #!/usr/bin/env node -import assert from 'node:assert/strict'; +// This entry keeps one stable name for the gate, the CI targeting contracts, and the test-contract +// registry. The plan-CLI cases live in plan-cli/*.test.mjs; Node's test runner gives each case file +// its own process, so the files run in parallel against the shipped plan.mjs. import { spawnSync } from 'node:child_process'; -import { createHash } from 'node:crypto'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { - normalizePlan, - parseReviewComment, -} from '../../plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs'; -const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); -const PLAN_CLI = path.join(ROOT, 'plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs'); -const GH_STUB_DIR = path.join(ROOT, 'scripts/tests/fixtures/gh-stub'); -const stateBase = process.env.XDG_STATE_HOME || path.join(process.env.HOME || os.homedir(), '.local', 'state'); -const testScratchRoot = path.join(stateBase, 'docks'); -fs.mkdirSync(testScratchRoot, { recursive: true, mode: 0o700 }); -fs.chmodSync(testScratchRoot, 0o700); -const scratch = fs.realpathSync(fs.mkdtempSync(path.join(testScratchRoot, 'plan-cli-'))); -const stateHome = fs.realpathSync(fs.mkdtempSync(path.join(testScratchRoot, 'plan-cli-state-'))); -const statePath = path.join(scratch, 'gh-state.json'); -const initialized = spawnSync('git', ['init', '--quiet'], { cwd: scratch, encoding: 'utf8' }); -assert.equal(initialized.status, 0, initialized.stderr); -const committed = spawnSync( - 'git', - [ - '-c', - 'user.name=Plan CLI test', - '-c', - 'user.email=plan-cli@example.invalid', - 'commit', - '--quiet', - '--allow-empty', - '-m', - 'fixture', - ], - { cwd: scratch, encoding: 'utf8' }, -); -assert.equal(committed.status, 0, committed.stderr); -fs.writeFileSync( - statePath, - `${JSON.stringify( - { - repo: { nameWithOwner: 'DocksDocks/fixture', visibility: 'PRIVATE', defaultBranchRef: { name: 'main' } }, - repositoryDefaults: { 'DocksDocks/fixture': 'main' }, - viewerLogin: 'plan-agent', - labels: {}, - issues: [], - prs: [], - nextIssue: 1, - nextComment: 1, - clock: 0, - calls: [], - }, - null, - 2, - )}\n`, -); -const childEnv = { - ...process.env, - PATH: `${GH_STUB_DIR}${path.delimiter}${process.env.PATH ?? ''}`, - GH_STUB_STATE: statePath, - XDG_STATE_HOME: stateHome, -}; - -function run(...args) { - return spawnSync(process.execPath, [PLAN_CLI, ...args], { - cwd: scratch, - encoding: 'utf8', - env: childEnv, - }); -} - -function expectSuccess(result, label) { - assert.equal(result.status, 0, `${label}: ${result.stderr}`); -} - -function refuse(result, message) { - assert.equal(result.status, 1, message); - assert.equal(result.stderr, `${message}\n`); -} - -function loadState() { - return JSON.parse(fs.readFileSync(statePath, 'utf8')); -} - -function updateState(update) { - const state = loadState(); - update(state); - fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`); -} - -function issue(number) { - return loadState().issues.find((entry) => entry.number === number); -} - -function updateIssue(number, update) { - updateState((state) => update(state.issues.find((entry) => entry.number === number))); -} - -function addIssueComment(number, body, author = 'plan-agent') { - updateState((state) => { - const entry = state.issues.find((candidate) => candidate.number === number); - const id = state.nextComment++; - entry.comments ??= []; - entry.comments.push({ - id, - body, - author, - createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), - }); - }); -} - -function body(status = 'planned') { - return [ - '', - '', - '## Goal', - '', - 'Fix the parser.', - '', - 'Mode: plan-and-implement', - '', - '## Research', - '', - 'The parser loses mixed-case records.', - '', - '## Steps', - '', - '| # | Id | Task | Files | Depends | Effect | Status | Done when |', - '|---:|---|---|---|---|---|---|---|', - `| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | ${status} | Records round-trip |`, - '', - '## Acceptance', - '', - '| ID | Command | Expected |', - '|---|---|---|', - '| A1 | node smoke.mjs | Exit 0 |', - '', - '## Do not touch', - '', - 'Release tooling.', - '', - '## Open questions', - '', - 'None.', - '', - '## Verification Results', - '', - 'Not run.', - '', - ].join('\n'); -} - -function createPlan(name, text = body()) { - const result = run('new', '--title', name, '--goal', 'Preserve plan records'); - expectSuccess(result, `new ${name}`); - const match = /^plan created: #(\d+) (https:\/\/\S+)$/m.exec(result.stdout); - assert.ok(match, 'new prints the issue identity'); - const number = Number(match[1]); - updateIssue(number, (entry) => { - entry.body = text; - }); - return number; -} - -function exported(number) { - const result = run('export', String(number)); - expectSuccess(result, 'export'); - const file = result.stdout.trim(); - assert.ok(path.isAbsolute(file), 'export returns an absolute file path'); - return file; -} - -function edit(number, transform = (text) => text) { - const file = exported(number); - fs.writeFileSync(file, transform(fs.readFileSync(file, 'utf8'))); - return run('edit', String(number), '--file', file); -} - -function closedPlan(name, { status = 'done', base = 'main', prBody } = {}) { - const number = createPlan(name, body(status)); - const closer = { - __typename: 'PullRequest', - number: 100 + number, - url: `https://github.com/DocksDocks/fixture/pull/${100 + number}`, - state: 'MERGED', - mergedAt: '2026-08-21T00:00:00Z', - baseRefName: base, - body: prBody === undefined ? `Closes #${number}` : prBody?.replaceAll('ISSUE', String(number)), - repository: { nameWithOwner: 'DocksDocks/fixture' }, - }; - updateIssue(number, (entry) => { - entry.state = 'CLOSED'; - entry.stateReason = 'COMPLETED'; - entry.labels = ['plan', 'plan:ongoing']; - entry.timelineItems = [{ closer }]; - }); - addIssueComment(number, '### Code review (round 2)\n\ncode-review: pass'); - return { number, closer }; -} - -try { - // Existing label metadata is not an input for new and must survive unchanged. - const existingLabels = { - plan: { color: '123456', description: 'Keep this color' }, - 'plan:drafting': { color: 'abcdef', description: 'Keep this description' }, - }; - updateState((state) => { - state.labels = structuredClone(existingLabels); - }); - createPlan('labels already present'); - assert.deepEqual( - Object.fromEntries(Object.entries(loadState().labels).filter(([name]) => name in existingLabels)), - existingLabels, - 'new creates missing labels without changing existing metadata', - ); - assert.deepEqual( - Object.keys(loadState().labels).sort(), - ['plan', 'plan:blocked', 'plan:drafting', 'plan:ongoing', 'plan:planned'], - 'new creates each missing plan label', - ); - const allLabels = loadState().labels; - createPlan('all labels already present'); - assert.deepEqual(loadState().labels, allLabels, 'repeating new leaves every existing label unchanged'); - - // Normalization must reach the remote body, not just the local export. - const irregular = body('Done') - .replace('## Goal', '## goal') - .replace('Mode: plan-and-implement\n', '') - .replace('fix_parser', 'Fix-Parser') - .replace('| local |', '| Local |') - .replace('Fix the parser.', 'Fix the parser — preserve records.'); - const normalized = normalizePlan(irregular); - assert.match(normalized.body, /Mode: plan-only/); - assert.match(normalized.body, /## Goal\n/); - assert.match(normalized.body, /\| fix_parser \|/); - assert.match(normalized.body, /\| `?local`? \| `?done`? \|/); - assert.ok(!normalized.body.includes('—')); - assert.match(normalized.body, /Fix the parser - preserve records\./); - const normalization = createPlan('normalization', irregular); - const normalizedEdit = edit(normalization); - expectSuccess(normalizedEdit, 'normalize edit'); - assert.equal(issue(normalization).body, normalized.body); - assert.match(normalizedEdit.stdout, /^advice: .*mode/im); - const normalizedFile = exported(normalization); - assert.equal( - fs.readFileSync(`${normalizedFile}.origin`, 'utf8'), - `${createHash('sha256').update(normalized.body).digest('hex')}\n`, - ); - - // Machine path advice names each distinct path into a home, a mount, a named temporary or runtime directory, - // a Windows drive, or a network share. URL routes, portable system paths, and slash text get none (#71). - const machinePathAdvice = (text) => - normalizePlan(body().replace('The parser loses mixed-case records.', text)).advice.filter((message) => - message.startsWith('body contains an absolute machine path'), - ); - const named = (...paths) => paths.map((found) => `body contains an absolute machine path: ${found}`); - for (const [text, expected] of [ - ['Clone into /home//projects/x.', named('/home//projects/x')], - ['Open `/Users//src`.', named('/Users//src')], - ['Read /root/.ssh/config and /mnt/data/x', named('/root/.ssh/config', '/mnt/data/x')], - [ - 'Mounts /media//usb, /run/media//usb, /Volumes/Data/x', - named('/media//usb', '/run/media//usb', '/Volumes/Data/x'), - ], - ['Scratch `/tmp/`, /var/tmp/x, /private/tmp/x', named('/tmp/', '/var/tmp/x', '/private/tmp/x')], - [ - 'Temp /var/folders/ab/cd/T/x and /private/var/folders/ab/T/x', - named('/var/folders/ab/cd/T/x', '/private/var/folders/ab/T/x'), - ], - ['XDG_RUNTIME_DIR=/run/user/1000/bus', named('/run/user/1000/bus')], - [ - 'Windows C:\\Users\\\\x, D:/work/x, \\\\server\\share\\x', - named('C:\\Users\\\\x', 'D:/work/x', '\\\\server\\share\\x'), - ], - ['See [log](/home//log.txt) and file:///home//y', named('/home//log.txt', '/home//y')], - ['Twice /home/a/x, /home/a/x then /tmp/y;', named('/home/a/x', '/tmp/y')], - ['Routes /login/, `/wp-json//v1/items/123`, /search/transfer/user', []], - ['TMPDIR=/tmp node t.mjs, /usr/bin/php8.5, #!/usr/bin/env node, x > /dev/null', []], - ["Pick `A`/`B`, awk '/^## /{f=0}', __DIR__ . '/vendor/autoload.php'", []], - ['URLs https://example.com/home/a and ssh://host/home/a', []], - ['Portable ~/.local/state/docks/x, $HOME/x, bare /tmp, /tmp/ and /home/', []], - ["Regex `'\\\\d+\\\\.'`", []], - ]) - assert.deepEqual(machinePathAdvice(text), expected, text); - - // Ids that differ only by case or dash collapse after normalization; the write refuses the collision. - const collision = createPlan('id-collision'); - const collisionEdit = edit(collision, (text) => - text.replace( - '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', - '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |\n| 2 | Fix-Parser | Second | src/other.mjs | - | local | planned | Also round-trips |', - ), - ); - refuse(collisionEdit, 'duplicate step id after normalization: fix_parser'); - assert.equal(issue(collision).body, body(), 'a refused collision writes nothing'); - - // Old records stay readable, but the next body write uses v4. - const legacy = createPlan( - 'legacy', - body() - .replace('plan-contract: v4', 'plan-contract: v3') - .replace('## Verification Results', '## Review\n\nOld body review.\n\n## Verification Results'), - ); - const legacyShow = run('show', String(legacy), '--body'); - expectSuccess(legacyShow, 'show v3'); - assert.equal(legacyShow.stdout.trim(), issue(legacy).body.trim()); - assert.match(legacyShow.stderr, /reviews: plan=none code=none/); - expectSuccess(edit(legacy), 'edit v3'); - assert.match(issue(legacy).body, /^/); - assert.doesNotMatch(issue(legacy).body, /^## Review$/m); - - // Ownership is checked before a mutating command can change the record. - const foreign = createPlan('foreign owner'); - updateIssue(foreign, (entry) => { - entry.assignees = ['other-agent']; - }); - refuse(run('status', String(foreign), 'ongoing'), `plan #${foreign} is owned by other-agent`); - assert.deepEqual(issue(foreign).labels, ['plan', 'plan:drafting']); - const pagedOwner = createPlan('owner beyond first page'); - updateIssue(pagedOwner, (entry) => { - entry.assignees = [...Array(100).fill('plan-agent'), 'other-agent']; - }); - refuse(run('status', String(pagedOwner), 'ongoing'), `plan #${pagedOwner} is owned by other-agent`); - assert.deepEqual(issue(pagedOwner).labels, ['plan', 'plan:drafting'], 'foreign owner on page two prevents writes'); - - // A second read catches a writer that races the edit after provenance passes. - const raced = createPlan('raced body'); - const raceFile = exported(raced); - fs.writeFileSync(raceFile, issue(raced).body.replace('Fix parser', 'Repair parser')); - const remoteBody = issue(raced).body.replace('Fix parser', 'Remote task'); - updateState((state) => { - state.remoteChange = { issue: raced, viewsBeforeChange: 1, body: remoteBody }; - }); - refuse(run('edit', String(raced), '--file', raceFile), 'plan issue changed remotely; re-read and retry'); - assert.equal(issue(raced).body, remoteBody); - - // GraphQL may report HTTP-successful partial data; neither that nor missing data authorizes a write. - const graphqlFailure = createPlan('graphql failure'); - updateState((state) => { - state.graphqlError = [{ message: 'partial snapshot' }]; - }); - const partial = run('status', String(graphqlFailure), 'ongoing'); - assert.equal(partial.status, 1, 'GraphQL errors fail closed even with usable data'); - assert.match(partial.stderr, /graphql/i); - assert.deepEqual(issue(graphqlFailure).labels, ['plan', 'plan:drafting']); - updateState((state) => { - state.graphqlMissingData = true; - }); - const missingData = run('status', String(graphqlFailure), 'ongoing'); - assert.equal(missingData.status, 1, 'missing GraphQL data fails closed'); - assert.match(missingData.stderr, /graphql/i); - assert.equal(issue(graphqlFailure).body, body(), 'neither partial nor missing GraphQL data writes a body'); - - // An arbitrary file cannot bypass export provenance. - const provenance = createPlan('provenance'); - const missingFile = path.join(scratch, 'missing.md'); - fs.writeFileSync(missingFile, issue(provenance).body); - refuse( - run('edit', String(provenance), '--file', missingFile), - `missing export provenance: ${missingFile}.origin does not exist; run \`plan.mjs export ${provenance}\` and re-apply the edit`, - ); - const staleFile = exported(provenance); - updateIssue(provenance, (entry) => { - entry.body = entry.body.replace('Fix parser', 'Remote task'); - }); - refuse( - run('edit', String(provenance), '--file', staleFile), - `stale export: ${staleFile} was exported from a superseded body; run \`plan.mjs export ${provenance}\` and re-apply the edit`, - ); - assert.match(issue(provenance).body, /Remote task/); - const unreadableFile = exported(provenance); - fs.writeFileSync(`${unreadableFile}.origin`, 'not a digest\n'); - refuse( - run('edit', String(provenance), '--file', unreadableFile), - `unreadable export provenance: ${unreadableFile}.origin`, - ); - - // The event history keeps step state frozen even if phase labels are reset. - const frozen = createPlan('frozen steps'); - updateIssue(frozen, (entry) => { - entry.events = [{ event: 'labeled', label: { name: 'plan:ongoing' } }]; - }); - for (const [name, before, after] of [ - ['Status', '| planned |', '| done |'], - ['Id', '| fix_parser |', '| renamed |'], - ['Depends', '| - |', '| 2 |'], - ]) { - const result = edit(frozen, (text) => text.replace(before, after)); - assert.equal(result.status, 1, `${name} must stay frozen`); - assert.match(result.stderr, /^step state is frozen once work starts: .+\n$/); - assert.equal(issue(frozen).body, body(), `${name} refusal must preserve the remote body`); - } - expectSuccess( - edit(frozen, (text) => text.replace('Fix parser', 'Repair parser')), - 'task text edit', - ); - assert.match(issue(frozen).body, /Repair parser/); - const pagedHistory = createPlan('ongoing history beyond first page'); - updateIssue(pagedHistory, (entry) => { - entry.events = [ - ...Array.from({ length: 100 }, () => ({ event: 'labeled', label: { name: 'unrelated' } })), - { event: 'labeled', label: { name: 'plan:ongoing' } }, - ]; - }); - refuse( - edit(pagedHistory, (text) => text.replace('| planned |', '| done |')), - 'step state is frozen once work starts: step fix_parser status changed', - ); - assert.equal(issue(pagedHistory).body, body(), 'a late history event freezes the original step state'); - - // Labels cannot erase started work, but drafting can enter implementation. - const transitions = createPlan('status transitions'); - expectSuccess(run('status', String(transitions), 'ongoing'), 'drafting to ongoing'); - assert.ok(issue(transitions).labels.includes('plan:ongoing')); - refuse(run('status', String(transitions), 'planned'), 'illegal plan status transition: ongoing -> planned'); - refuse(run('status', String(frozen), 'planned'), 'illegal plan status transition: drafting -> planned'); - - // Review prose is flexible. Severity still prevents an unsafe code pass. - const reviewCases = [ - ['## Plan review (round 2)\n\nplan-review: pass', { kind: 'plan', verdict: 'pass' }], - [ - '### Plan review — round 2\n\nplan review: changes\nParser — records are lost.', - { kind: 'plan', verdict: 'repair' }, - ], - [ - '# Code review\n\ncode-review: approved\nHIGH: The parser loses data.', - { kind: 'code', verdict: 'fixes-required' }, - ], - [ - '### Code review round 1 - 2026-09-09\nCode-review: pass\n- [low] src/api.mjs:3 - the high-level API name is unclear; critical path is fine - rename', - { kind: 'code', verdict: 'pass' }, - ], - [ - '### Code review round 1 - 2026-09-09\nCode-review: pass\n- [HIGH] src/api.mjs:3 - data loss - fix', - { kind: 'code', verdict: 'fixes-required' }, - ], - ['**### Code review round 1**\n\n**Code-review: pass**', { kind: 'code', verdict: 'pass' }], - ['### Code review round 2\n\n**Code-review:** `pass`.', { kind: 'code', verdict: 'pass' }], - ['## **Plan review** round 1\n\n- Plan-review: _repair_', { kind: 'plan', verdict: 'repair' }], - [ - '### Code review round 3\nCode-review: pass\n- **[HIGH]** src/example.mjs:1 - data loss - fix', - { kind: 'code', verdict: 'fixes-required' }, - ], - [ - '### Code review round 3\nCode-review: pass\n1. Bug [critical] src/example.mjs:1 - data loss - fix', - { kind: 'code', verdict: 'fixes-required' }, - ], - ['### Code review round 3\nCode-review: pass\n> Severity: High', { kind: 'code', verdict: 'fixes-required' }], - ]; - for (const [text, expected] of reviewCases) assert.deepEqual(parseReviewComment(text), expected); - const reviews = createPlan('trusted reviews'); - addIssueComment(reviews, '## Plan review (round 2)\n\nplan-review: pass'); - addIssueComment(reviews, '## Plan review\n\nplan-review: blocked', 'foreign-agent'); - addIssueComment(reviews, '## Code review\n\ncode-review: pass\nHIGH: Data loss.'); - let shown = run('show', String(reviews)); - expectSuccess(shown, 'trusted review show'); - assert.match(shown.stdout, /^reviews: plan=pass code=fixes-required$/m); - addIssueComment(reviews, '## Plan review\n\nplan-review: repair'); - updateIssue(reviews, (entry) => { - entry.comments.at(-1).createdAt = '2020-01-01T00:00:00Z'; - }); - shown = run('show', String(reviews)); - expectSuccess(shown, 'timestamp review show'); - assert.match(shown.stdout, /^reviews: plan=pass code=fixes-required$/m); - const pagedReviews = createPlan('reviews beyond first page'); - addIssueComment(pagedReviews, '## Code review\n\ncode-review: pass'); - updateState((state) => { - const entry = state.issues.find((candidate) => candidate.number === pagedReviews); - for (let index = 0; index < 99; index += 1) { - const id = state.nextComment++; - entry.comments.push({ - id, - body: 'Routine discussion.', - author: 'foreign-agent', - createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), - }); - } - }); - addIssueComment(pagedReviews, '## Code review\n\ncode-review: repair', 'plan-agent'); - addIssueComment(pagedReviews, '## Code review\n\ncode-review: pass', 'foreign-agent'); - const pagedShow = run('show', String(pagedReviews)); - expectSuccess(pagedShow, 'show all review pages'); - assert.match(pagedShow.stdout, /^reviews: plan=none code=fixes-required$/m, 'late trusted review wins'); - updateState((state) => { - state.graphqlError = { on: 'page', errors: [{ message: 'comment page unavailable' }] }; - }); - const brokenReviewPage = run('show', String(pagedReviews)); - assert.equal(brokenReviewPage.status, 1, 'a failed review page cannot produce a verdict'); - assert.match(brokenReviewPage.stderr, /graphql/i); - assert.equal(brokenReviewPage.stdout, ''); - - const boldReview = createPlan('bold review record'); - addIssueComment(boldReview, '**### Code review round 1**\n\n**Code-review: pass**'); - const boldShow = run('show', String(boldReview)); - expectSuccess(boldShow, 'show bold review record'); - assert.match(boldShow.stdout, /^reviews: plan=none code=pass$/m); - - // Phase labels compare case-insensitively and are removed by their stored spelling. - const mixedLabels = createPlan('mixed-case labels'); - updateIssue(mixedLabels, (entry) => { - entry.labels = ['plan', 'Plan:Drafting']; - }); - assert.match(run('show', String(mixedLabels)).stdout, /· drafting ·/, 'mixed-case phase label is read'); - expectSuccess(run('status', String(mixedLabels), 'planned'), 'transition from a mixed-case label'); - assert.deepEqual(issue(mixedLabels).labels, ['plan', 'plan:planned'], 'the mixed-case label is removed'); - updateIssue(mixedLabels, (entry) => { - entry.events = [{ event: 'labeled', label: { name: 'Plan:Ongoing' } }]; - }); - refuse( - edit(mixedLabels, (text) => text.replace('| planned |', '| done |')), - 'step state is frozen once work starts: step fix_parser status changed', - ); - refuse(run('status', String(mixedLabels), 'drafting'), 'illegal plan status transition: planned -> drafting'); - assert.deepEqual(issue(mixedLabels).labels, ['plan', 'plan:planned'], 'history keeps the plan out of drafting'); - const pagedLabels = createPlan('phase label beyond first page'); - const unrelatedLabels = Array.from({ length: 100 }, (_, index) => `topic:${index}`); - updateIssue(pagedLabels, (entry) => { - entry.labels = ['plan', ...unrelatedLabels, 'Plan:Drafting']; - }); - const pagedLabelShow = run('show', String(pagedLabels)); - expectSuccess(pagedLabelShow, 'show label on second page'); - assert.match(pagedLabelShow.stdout, /· drafting ·/); - expectSuccess(run('status', String(pagedLabels), 'planned'), 'transition label on second page'); - assert.deepEqual( - issue(pagedLabels).labels, - ['plan', ...unrelatedLabels, 'plan:planned'], - 'phase repair preserves other labels and removes the stored spelling', - ); - - // Archive needs terminal work, a trusted pass, and the actual merged closer. - const landed = closedPlan('merged closer'); - const archived = run('archive', String(landed.number)); - expectSuccess(archived, 'archive merged closer'); - assert.equal(archived.stdout.trim(), `plan #${landed.number} finished (closed by ${landed.closer.url})`); - assert.deepEqual(issue(landed.number).labels, ['plan']); - // The latest actual closure wins over older closures and a later non-closure event. - const latestClosure = closedPlan('latest closure'); - updateIssue(latestClosure.number, (entry) => { - entry.timelineItems = [ - { __typename: 'ClosedEvent', closer: { ...latestClosure.closer, baseRefName: 'release' } }, - { __typename: 'ClosedEvent', closer: latestClosure.closer }, - { __typename: 'LabeledEvent', label: { name: 'plan:ongoing' } }, - ]; - }); - expectSuccess(run('archive', String(latestClosure.number)), 'archive latest closure'); - - const qualified = closedPlan('qualified closing keyword', { - prBody: 'fIxEs: dOcKsDoCkS/FiXtUrE#ISSUE', - }); - expectSuccess(run('archive', String(qualified.number)), 'archive qualified mixed-case keyword'); - - const tabSeparated = closedPlan('tab-separated closing keyword', { prBody: 'Resolves\t#ISSUE' }); - expectSuccess(run('archive', String(tabSeparated.number)), 'archive tab-separated keyword'); - - for (const [name, prBody] of [ - ['no closing keyword', 'Refs #ISSUE'], - ['number prefix', 'Closes #ISSUE0'], - ['number suffix', 'Closes #ISSUEa'], - ['keyword prefix', 'notCloses #ISSUE'], - ['issue URL', 'Closes https://github.com/DocksDocks/fixture/issues/ISSUE'], - ['different repository', 'Fixes: Another/repo#ISSUE'], - ['keyword and reference in separate paragraphs', 'Closes:\n\n#ISSUE is still open'], - ['keyword and reference on separate lines', 'Closes\n#ISSUE'], - ['null PR description', null], - ]) { - const unproven = closedPlan(name, { prBody }); - refuse( - run('archive', String(unproven.number)), - 'archive requires a closing pull request merged into DocksDocks/fixture:main', - ); - assert.deepEqual(issue(unproven.number).labels, ['plan', 'plan:ongoing']); - } - - const commitClosed = closedPlan('commit closer beyond first association page'); - const commitOid = 'a'.repeat(40); - updateState((state) => { - const entry = state.issues.find((candidate) => candidate.number === commitClosed.number); - entry.timelineItems = [ - { __typename: 'ClosedEvent', closer: commitClosed.closer }, - { __typename: 'ClosedEvent', closer: { __typename: 'Commit', oid: commitOid } }, - ]; - state.commits = [ - { - oid: commitOid, - associatedPullRequests: [ - ...Array.from({ length: 100 }, (_, index) => ({ - number: 10000 + index, - state: 'MERGED', - mergedAt: '2026-08-21T00:00:00Z', - baseRefName: 'release', - })), - { ...commitClosed.closer, body: null }, - ], - }, - ]; - }); - const commitArchive = run('archive', String(commitClosed.number)); - expectSuccess(commitArchive, 'archive merged PR associated with commit on second page'); - assert.equal( - commitArchive.stdout.trim(), - `plan #${commitClosed.number} finished (closed by ${commitClosed.closer.url})`, - ); - assert.deepEqual(issue(commitClosed.number).labels, ['plan']); - - const wrongBase = closedPlan('wrong base', { base: 'release' }); - refuse( - run('archive', String(wrongBase.number)), - 'archive requires a closing pull request merged into DocksDocks/fixture:main', - ); - const unfinished = closedPlan('unfinished step', { status: 'in-flight' }); - refuse(run('archive', String(unfinished.number)), 'archive refused: non-terminal step fix_parser'); - // A broken Steps table parses to zero rows; that never counts as complete work. - const tableless = closedPlan('broken steps table'); - updateIssue(tableless.number, (entry) => { - entry.body = entry.body.replace('| # | Id | Task | Files | Depends | Effect | Status | Done when |\n', ''); - }); - refuse(run('archive', String(tableless.number)), 'archive refused: no Steps rows parsed'); - - // A merged closer does not replace review approval. - const unapproved = closedPlan('review repair'); - addIssueComment(unapproved.number, '## Code review\n\ncode-review: repair\nFix data loss.'); - refuse(run('archive', String(unapproved.number)), 'archive requires Code-review: pass'); - assert.ok(issue(unapproved.number).labels.includes('plan:ongoing')); - - const largeComments = closedPlan('large comment pages'); - updateState((state) => { - const entry = state.issues.find((candidate) => candidate.number === largeComments.number); - for (let i = 0; i < 20; i += 1) { - const id = state.nextComment++; - entry.comments.push({ - id, - body: `Evidence round ${i}\n${'x'.repeat(60000)}`, - author: 'plan-agent', - createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), - }); - } - }); - const largeShow = run('show', String(largeComments.number)); - expectSuccess(largeShow, 'show large comment pages'); - assert.match(largeShow.stdout, /^reviews: plan=none code=pass$/m); - const largeArchive = run('archive', String(largeComments.number)); - expectSuccess(largeArchive, 'archive large comment pages'); - assert.equal( - largeArchive.stdout.trim(), - `plan #${largeComments.number} finished (closed by ${largeComments.closer.url})`, - ); - - const partialMerge = closedPlan('closed by a partial merge', { status: 'in-flight' }); - const partialShow = run('show', String(partialMerge.number)); - expectSuccess(partialShow, 'show early closure'); - assert.ok( - partialShow.stdout - .split(/\r?\n/) - .includes( - `advice: plan is closed as completed while step fix_parser is in-flight; closed by ${partialMerge.closer.url}; reopen the issue if work remains`, - ), - ); - refuse( - run('step', String(partialMerge.number), 'fix_parser', 'in-flight'), - `plan #${partialMerge.number} is finished; reopen it before moving step fix_parser to in-flight`, - ); - updateIssue(partialMerge.number, (entry) => { - entry.state = 'OPEN'; - entry.stateReason = 'REOPENED'; - }); - expectSuccess(run('step', String(partialMerge.number), 'fix_parser', 'done'), 'finish step after reopen'); - - // Retire repairs labels before closing; list retains its filtered open/closed view. - const listed = createPlan('listed drafting plan'); - const unlisted = createPlan('issue without plan label'); - updateIssue(unlisted, (entry) => { - entry.labels = ['other']; - }); - const retiring = createPlan('retired plan'); - updateIssue(retiring, (entry) => { - entry.assignees = []; - }); - updateState((state) => { - state.labelRemovalErrorOnce = 'label removal unavailable'; - }); - refuse( - run('retire', String(retiring), '--reason', 'No longer needed'), - 'gh issue edit failed: label removal unavailable', - ); - assert.equal(issue(retiring).state, 'OPEN', 'a failed label repair cannot close the issue'); - assert.deepEqual(issue(retiring).labels, ['plan', 'plan:drafting']); - const retired = run('retire', String(retiring), '--reason', 'No longer needed'); - expectSuccess(retired, 'retire an unassigned plan'); - assert.equal(retired.stdout.trim(), `plan #${retiring} retired`); - assert.equal(issue(retiring).state, 'CLOSED'); - assert.equal(issue(retiring).stateReason, 'NOT_PLANNED'); - assert.deepEqual(issue(retiring).labels, ['plan']); - assert.deepEqual(issue(retiring).assignees, ['plan-agent'], 'retire claims the plan in the label edit'); - const draftingList = run('list', '--status', 'drafting'); - expectSuccess(draftingList, 'list drafting plans'); - assert.match(draftingList.stdout, new RegExp(`^drafting\\t#${listed}\\tlisted drafting plan$`, 'm')); - assert.doesNotMatch(draftingList.stdout, /issue without plan label/); - assert.doesNotMatch(draftingList.stdout, new RegExp(`\\t#${retiring}\\t`)); - const retiredList = run('list', '--status', 'retired'); - expectSuccess(retiredList, 'list retired plans'); - assert.equal(retiredList.stdout.trim(), `retired\t#${retiring}\tretired plan`); - - // A terminal row cannot be reopened by the step command. - const terminal = createPlan('terminal step', body('done')); - expectSuccess(run('status', String(terminal), 'ongoing'), 'start terminal plan'); - refuse(run('step', String(terminal), 'fix_parser', 'in-flight'), 'illegal step status transition: done -> in-flight'); - assert.match(issue(terminal).body, /\| `?done`? \|/); - - // A step update preserves every sibling row, including one whose cell holds an escaped pipe. - const piped = createPlan('escaped pipe sibling'); - const pipedRow = '| 2 | grep_pipe | Grep `a \\| b` | src/grep.mjs | - | local | planned | Pipe survives |'; - expectSuccess( - edit(piped, (text) => text.replace('| Records round-trip |\n', `| Records round-trip |\n${pipedRow}\n`)), - 'add piped row', - ); - assert.ok(issue(piped).body.includes(pipedRow), 'edit preserves the escaped pipe cell'); - expectSuccess(run('status', String(piped), 'ongoing'), 'start piped plan'); - expectSuccess(run('step', String(piped), 'fix_parser', 'in-flight'), 'update sibling of piped row'); - assert.ok(issue(piped).body.includes(pipedRow), 'step preserves the escaped pipe row'); - assert.match(issue(piped).body, /\| fix_parser \| .* \| in-flight \|/); - const backslashRow = - '| 3 | back_slash | Path `C:\\\\` | src\\\\win.mjs | - | local | planned | Ends with a literal backslash |'; - expectSuccess( - edit(piped, (text) => text.replace(`${pipedRow}\n`, `${pipedRow}\n${backslashRow}\n`)), - 'add a row whose cell ends in an even backslash run', - ); - assert.ok(issue(piped).body.includes(backslashRow), 'an even backslash run before a pipe is a delimiter'); - const oddCell = String.raw`Keep a \\\| b`; - assert.equal(oddCell, 'Keep a \\\\\\| b', 'the raw fixture holds exactly three backslashes before the pipe'); - const oddRow = `| 4 | odd_run | ${oddCell} | src/odd.mjs | - | local | planned | Three backslashes escape the pipe |`; - expectSuccess( - edit(piped, (text) => text.replace(`${backslashRow}\n`, `${backslashRow}\n${oddRow}\n`)), - 'add a row with an odd backslash run before a pipe', - ); - assert.ok( - issue(piped).body.includes(`| ${oddCell} |`), - 'an odd backslash run before a pipe keeps the pipe in the cell', - ); - expectSuccess(run('step', String(piped), 'fix_parser', 'done'), 'step rewrite beside the odd-run row'); - assert.ok(issue(piped).body.includes(oddRow), 'the odd-run row survives a step rewrite as one cell'); - - // A row that does not parse to eight cells blocks every write instead of vanishing. - const malformed = createPlan('malformed row'); - refuse( - edit(malformed, (text) => - text.replace( - '| Records round-trip |\n', - '| Records round-trip |\n| 2 | broken | Unescaped a | b | src | - | local | planned | Lost |\n', - ), - ), - 'Steps row 2 has 9 cells; expected 8. Escape a literal pipe as \\| so the row is preserved.', - ); - updateIssue(malformed, (entry) => { - entry.body = entry.body.replace( - '| Records round-trip |\n', - '| Records round-trip |\n| 2 | broken | Unescaped a | b | src | - | local | planned | Lost |\n', - ); - }); - expectSuccess(run('status', String(malformed), 'ongoing'), 'start malformed plan'); - refuse( - run('step', String(malformed), 'fix_parser', 'in-flight'), - 'Steps row 2 has 9 cells; expected 8. Escape a literal pipe as \\| so the row is preserved.', - ); - assert.match(issue(malformed).body, /\| broken \|/, 'the malformed row is untouched'); - - // A missing separator line is inserted; the first data row is never mistaken for it. - const unseparated = createPlan('missing separator'); - expectSuccess( - edit(unseparated, (text) => - text.replace('|---:|---|---|---|---|---|---|---|\n', '').replace('|---|---|---|\n', ''), - ), - 'edit without separators', - ); - const unseparatedBody = issue(unseparated).body; - assert.ok(unseparatedBody.includes('| 1 | fix_parser | Fix parser |'), 'first step row survives'); - assert.ok(unseparatedBody.includes('| A1 | node smoke.mjs | Exit 0 |'), 'first acceptance row survives'); - assert.equal(unseparatedBody, body(), 'separators are restored'); - - // A fenced example table is not the live table, and a duplicate section heading blocks the write. - const fenced = createPlan('fenced example'); - const example = [ - '```markdown', - '| # | Id | Task | Files | Depends | Effect | Status | Done when |', - '|---:|---|---|---|---|---|---|---|', - '| 1 | example | Example | x | - | local | done | Never |', - '```', - '', - ].join('\n'); - expectSuccess( - edit(fenced, (text) => text.replace('## Steps\n\n', `## Steps\n\n${example}`)), - 'edit with fenced example', - ); - assert.ok( - issue(fenced).body.includes('| 1 | example | Example | x | - | local | done | Never |'), - 'example untouched', - ); - expectSuccess(run('status', String(fenced), 'ongoing'), 'start fenced plan'); - expectSuccess(run('step', String(fenced), 'fix_parser', 'in-flight'), 'step targets the live table'); - assert.match(issue(fenced).body, /\| fix_parser \| .* \| in-flight \|/); - assert.ok( - issue(fenced).body.includes('| 1 | example | Example | x | - | local | done | Never |'), - 'example still untouched', - ); - const doubled = createPlan('duplicate heading'); - refuse( - edit(doubled, (text) => text.replace('## Acceptance', '## steps\n\nNothing.\n\n## Acceptance')), - 'duplicate section heading: Steps', - ); - assert.equal(issue(doubled).body, body(), 'a duplicate heading writes nothing'); - const fencedMode = createPlan('fenced mode example'); - const modeExample = '```markdown\nMode: plan-and-implement\n```\n\n'; - expectSuccess( - edit(fencedMode, (text) => - text - .replace('Mode: plan-and-implement\n', 'Mode: plan-only\n') - .replace('Fix the parser.\n\n', `Fix the parser.\n\n${modeExample}`), - ), - 'edit with fenced mode example', - ); - const fencedModeBody = issue(fencedMode).body; - assert.ok(fencedModeBody.includes(modeExample.trimEnd()), 'fenced Mode example is preserved'); - assert.match(fencedModeBody, /\nMode: plan-only\n/, 'the unfenced Mode decision is the live mode'); - assert.equal(fencedModeBody.match(/^Mode:/gm).length, 2, 'one example, one live Mode line'); - - // A fenced block that ends a section survives normalization with either fence style. - for (const fence of ['```', '~~~']) { - const trailing = createPlan(`trailing fence ${fence}`); - const evidence = `${fence}text\nnode smoke.mjs | tail -1\n${fence}`; - expectSuccess( - edit(trailing, (text) => - text.replace('The parser loses mixed-case records.\n', `The parser loses mixed-case records.\n\n${evidence}\n`), - ), - `edit with trailing ${fence} fence`, - ); - assert.ok(issue(trailing).body.includes(evidence), `trailing ${fence} block is preserved`); - } - - // An indented code example never becomes the live Mode. - const indentedMode = createPlan('indented mode example'); - expectSuccess( - edit(indentedMode, (text) => - text - .replace('Mode: plan-and-implement\n', 'Mode: plan-only\n') - .replace('Fix the parser.\n\n', 'Fix the parser.\n\n Mode: plan-and-implement\n\n'), - ), - 'edit with indented mode example', - ); - assert.ok(issue(indentedMode).body.includes(' Mode: plan-and-implement\n'), 'indented example is preserved'); - assert.match(issue(indentedMode).body, /\nMode: plan-only\n/, 'the plan-only decision is the live mode'); - - for (const [label, transform] of [ - [ - 'leading', - (text) => - text.replace( - '## Goal\n\nFix the parser.\n\n', - '## Goal\n\n Mode: plan-and-implement\n\nFix the parser.\n\n', - ), - ], - ['trailing', (text) => text.replace('Mode: plan-only\n\n', 'Mode: plan-only\n\n Mode: plan-and-implement\n\n')], - ]) { - const placed = createPlan(`${label} indented mode example`); - expectSuccess( - edit(placed, (text) => transform(text.replace('Mode: plan-and-implement\n', 'Mode: plan-only\n'))), - `edit with ${label} indented example`, - ); - expectSuccess(edit(placed), `${label} second normalization`); - assert.ok(issue(placed).body.includes(' Mode: plan-and-implement\n'), `${label} indented example is preserved`); - assert.match(issue(placed).body, /\nMode: plan-only\n/, `${label}: the plan-only decision stays live`); - assert.equal(issue(placed).body.match(/^Mode:/gm).length, 1, `${label}: exactly one live Mode line`); - } - - // An indented example table before the live table is neither selected nor rewritten. - const indentedTable = createPlan('indented example table'); - const indentedExample = [ - ' | # | Id | Task | Files | Depends | Effect | Status | Done when |', - ' |---:|---|---|---|---|---|---|---|', - ' | 1 | example | Example | x | - | local | done | Never |', - ].join('\n'); - expectSuccess( - edit(indentedTable, (text) => text.replace('## Steps\n\n', `## Steps\n\n${indentedExample}\n\n`)), - 'edit with indented example table', - ); - assert.ok(issue(indentedTable).body.includes(indentedExample), 'indented example bytes are preserved'); - expectSuccess(run('status', String(indentedTable), 'ongoing'), 'start indented-table plan'); - expectSuccess(run('step', String(indentedTable), 'fix_parser', 'in-flight'), 'step targets the live table'); - assert.match(issue(indentedTable).body, /\| fix_parser \| .* \| in-flight \|/); - assert.ok(issue(indentedTable).body.includes(indentedExample), 'indented example still untouched'); - - // An explicit --mode outranks a Mode line embedded in --goal; a later live Mode line wins. - const explicit = run( - 'new', - '--title', - 'explicit mode', - '--goal', - 'Ship it.\nMode: plan-and-implement\n\n## Context\nDetails.\n\n```text\n## fenced heading\n```', - '--mode', - 'plan-only', - ); - expectSuccess(explicit, 'new with explicit mode'); - assert.doesNotMatch(explicit.stdout, /Mode defaulted/, 'advice honors the explicit --mode'); - const explicitNumber = Number(/^plan created: #(\d+)/m.exec(explicit.stdout)[1]); - assert.match(issue(explicitNumber).body, /\nMode: plan-only\n/, 'explicit --mode is the live mode'); - assert.equal(issue(explicitNumber).body.match(/^Mode:/gm).length, 1, 'the embedded Mode line is removed'); - assert.ok( - issue(explicitNumber).body.includes('### Context\nDetails.\n\n```text\n## fenced heading\n```\n\nMode: plan-only'), - 'an unfenced H2 in --goal is demoted to H3, a fenced one is kept, and both stay inside Goal', - ); - refuse( - edit(explicitNumber, (text) => text.replace('## Acceptance', '## Context\n\nDetails.\n\n## Acceptance')), - 'unknown section heading: Context; use only Goal, Research, Steps, Acceptance, Do not touch, Open questions, Verification Results or write it below an existing section as ### or plain text', - ); - const embedded = run('new', '--title', 'embedded mode', '--goal', 'Ship it.\nMode: plan-and-implement'); - expectSuccess(embedded, 'new with embedded mode only'); - assert.match( - issue(Number(/^plan created: #(\d+)/m.exec(embedded.stdout)[1])).body, - /\nMode: plan-and-implement\n/, - 'goal-authored Mode is kept without --mode', - ); - - // Dependency normalization is idempotent even when a step id is a number. - const numericId = createPlan('numeric step id'); - const numericRows = [ - '| 1 | prepare | Prepare | src/a.mjs | - | local | planned | Ready |', - '| 2 | 1 | Numeric | src/b.mjs | - | local | planned | Ready |', - '| 3 | finish | Finish | src/c.mjs | prepare | local | planned | Ready |', - ].join('\n'); - expectSuccess( - edit(numericId, (text) => - text.replace( - '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', - numericRows, - ), - ), - 'edit with numeric id', - ); - const once = issue(numericId).body; - assert.match(once, /\| 3 \| finish \| Finish \| src\/c\.mjs \| 1 \|/, 'prepare resolves to display 1'); - expectSuccess(edit(numericId), 'second normalization'); - assert.equal(issue(numericId).body, once, 'a second normalization changes nothing'); - - // A drafted plan enters planned or ongoing only after research replaces the placeholder (#78). - const unresearched = createPlan( - 'unresearched plan', - body().replace('The parser loses mixed-case records.', '_Not researched yet._'), - ); - refuse(run('status', String(unresearched), 'planned'), 'status planned requires a filled Research section'); - refuse(run('status', String(unresearched), 'ongoing'), 'status ongoing requires a filled Research section'); - assert.deepEqual(issue(unresearched).labels, ['plan', 'plan:drafting'], 'a refused status keeps the phase label'); - expectSuccess( - run('status', String(unresearched), 'blocked', '--reason', 'Waiting for research'), - 'an unresearched plan can still block', - ); - const emptyResearch = createPlan('empty research', body().replace('The parser loses mixed-case records.', '')); - refuse(run('status', String(emptyResearch), 'planned'), 'status planned requires a filled Research section'); - const quotedPlaceholder = createPlan( - 'research quotes the placeholder', - body().replace( - 'The parser loses mixed-case records.', - 'A new plan holds `_Not researched yet._` until research is published.', - ), - ); - const quotedStatus = run('status', String(quotedPlaceholder), 'planned'); - expectSuccess(quotedStatus, 'research prose may quote the placeholder'); - assert.doesNotMatch(quotedStatus.stdout, /Research is not filled/, 'quoted placeholder is not unfilled research'); - - // A failed remote edit keeps the export valid, so the same file can be sent again (#78). - const retried = createPlan('retried edit'); - const retryFile = exported(retried); - fs.writeFileSync(retryFile, fs.readFileSync(retryFile, 'utf8').replace('Not run.', 'Smoke passed.')); - const transientError = 'GraphQL: Something went wrong while executing your query'; - updateState((state) => { - state.issueEditError = transientError; - }); - const failedEdit = run('edit', String(retried), '--file', retryFile); - refuse(failedEdit, `gh issue edit failed: ${transientError}`); - assert.equal(failedEdit.stdout, '', 'a failed edit prints no result'); - updateState((state) => { - delete state.issueEditError; - }); - expectSuccess(run('edit', String(retried), '--file', retryFile), 'retry with the same export'); - assert.match(issue(retried).body, /Smoke passed\./, 'the retry writes the edited body'); - - // Steps that edit the same path need a Depends chain, or parallel work collides (#78). - const sharedFiles = createPlan('shared files'); - const sharedRows = [ - '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', - '| 2 | parse_dates | Parse dates | `src/parser.mjs`, src/dates.mjs | - | local | planned | Dates parse |', - '| 3 | format_dates | Format dates | src/dates.mjs | parse_dates | local | planned | Dates format |', - '| 4 | print_dates | Print dates | src/dates.mjs | format_dates | local | planned | Dates print |', - ].join('\n'); - const sharedEdit = edit(sharedFiles, (text) => - text.replace( - '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', - sharedRows, - ), - ); - expectSuccess(sharedEdit, 'edit with shared files'); - assert.deepEqual( - sharedEdit.stdout.split('\n').filter((line) => line.includes(' share ')), - ['advice: steps fix_parser and parse_dates share src/parser.mjs with no Depends link'], - 'only steps without a Depends chain get shared-path advice', - ); - - // A plan-review pass covers the body that the reviewer read (#78). - const reviewed = createPlan('reviewed plan'); - addIssueComment(reviewed, '### Plan review round 1\n\nPlan-review: pass\n\nNone.'); - const resultsEdit = edit(reviewed, (text) => text.replace('Not run.', 'Smoke passed.')); - expectSuccess(resultsEdit, 'edit verification results after a plan pass'); - assert.doesNotMatch(resultsEdit.stdout, /plan review passed before this edit/, 'evidence edits need no new round'); - const researchEdit = edit(reviewed, (text) => - text.replace( - 'The parser loses mixed-case records.', - 'The parser loses mixed-case records.\n\nDecision: keep the old date format.', - ), - ); - expectSuccess(researchEdit, 'edit research after a plan pass'); - assert.match( - researchEdit.stdout, - /^advice: plan review passed before this edit; changed sections: Research; run a new plan-review round before code that depends on them$/m, - ); - - updateState((state) => { - for (let i = 0; i < 500; i += 1) { - const number = state.nextIssue++; - state.issues.push({ - number, - title: `bulk plan ${i}`, - body: '', - state: 'OPEN', - stateReason: null, - labels: ['plan', 'plan:drafting'], - assignees: ['plan-agent'], - url: `https://github.com/DocksDocks/fixture/issues/${number}`, - createdAt: '2026-08-20T20:00:00Z', - updatedAt: '2026-08-20T20:00:00Z', - comments: [], - events: [], - }); - } - }); - const limitedList = run('list'); - expectSuccess(limitedList, 'list limit advice'); - assert.match(limitedList.stdout, /^advice: list stopped at 500 plans; more plans may exist$/m); - - console.log( - 'plan-cli smoke PASSED: labels, snapshot paging, normalization, ownership, race, provenance, step freeze, transitions, review trust, archive proof, list, retire, large output, Markdown review records, early closure, list limit, machine path advice, research gate, edit retry, shared files advice, re-review advice', - ); -} finally { - fs.rmSync(scratch, { recursive: true, force: true }); - fs.rmSync(stateHome, { recursive: true, force: true }); -} +const CASE_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), 'plan-cli'); +const files = fs.existsSync(CASE_DIR) + ? fs + .readdirSync(CASE_DIR) + .filter((name) => name.endsWith('.test.mjs')) + .sort() + .map((name) => path.join(CASE_DIR, name)) + : []; +if (files.length === 0) { + console.error(`plan-cli: no case files (*.test.mjs) found in ${CASE_DIR}`); + process.exit(1); +} + +const result = spawnSync(process.execPath, ['--test', `--test-concurrency=${os.availableParallelism()}`, ...files], { + stdio: 'inherit', +}); +if (result.error) console.error(`plan-cli: node --test did not start: ${result.error.message}`); +else if (result.signal) console.error(`plan-cli: node --test was killed by ${result.signal}`); +process.exit(result.status ?? 1); diff --git a/scripts/tests/plan-cli/archive-closer.test.mjs b/scripts/tests/plan-cli/archive-closer.test.mjs new file mode 100644 index 00000000..6123b11d --- /dev/null +++ b/scripts/tests/plan-cli/archive-closer.test.mjs @@ -0,0 +1,97 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('archive finishes a plan with terminal work, a trusted pass, and the merged closer', (t) => { + const { run, issue, closedPlan } = planCliContext(t); + // Archive needs terminal work, a trusted pass, and the actual merged closer. + const landed = closedPlan('merged closer'); + const archived = run('archive', String(landed.number)); + expectSuccess(archived, 'archive merged closer'); + assert.equal(archived.stdout.trim(), `plan #${landed.number} finished (closed by ${landed.closer.url})`); + assert.deepEqual(issue(landed.number).labels, ['plan']); +}); + +test('archive uses the latest actual closure over older closures and a later non-closure event', (t) => { + const { run, updateIssue, closedPlan } = planCliContext(t); + // The latest actual closure wins over older closures and a later non-closure event. + const latestClosure = closedPlan('latest closure'); + updateIssue(latestClosure.number, (entry) => { + entry.timelineItems = [ + { __typename: 'ClosedEvent', closer: { ...latestClosure.closer, baseRefName: 'release' } }, + { __typename: 'ClosedEvent', closer: latestClosure.closer }, + { __typename: 'LabeledEvent', label: { name: 'plan:ongoing' } }, + ]; + }); + expectSuccess(run('archive', String(latestClosure.number)), 'archive latest closure'); +}); + +test('archive accepts a repository-qualified closing keyword in mixed case', (t) => { + const { run, closedPlan } = planCliContext(t); + const qualified = closedPlan('qualified closing keyword', { + prBody: 'fIxEs: dOcKsDoCkS/FiXtUrE#ISSUE', + }); + expectSuccess(run('archive', String(qualified.number)), 'archive qualified mixed-case keyword'); +}); + +test('archive accepts a closing keyword separated from the reference by a tab', (t) => { + const { run, closedPlan } = planCliContext(t); + const tabSeparated = closedPlan('tab-separated closing keyword', { prBody: 'Resolves\t#ISSUE' }); + expectSuccess(run('archive', String(tabSeparated.number)), 'archive tab-separated keyword'); +}); + +test('archive refuses a merged pull request whose description does not prove it closes the plan', (t) => { + const { run, issue, closedPlan } = planCliContext(t); + for (const [name, prBody] of [ + ['no closing keyword', 'Refs #ISSUE'], + ['number prefix', 'Closes #ISSUE0'], + ['number suffix', 'Closes #ISSUEa'], + ['keyword prefix', 'notCloses #ISSUE'], + ['issue URL', 'Closes https://github.com/DocksDocks/fixture/issues/ISSUE'], + ['different repository', 'Fixes: Another/repo#ISSUE'], + ['keyword and reference in separate paragraphs', 'Closes:\n\n#ISSUE is still open'], + ['keyword and reference on separate lines', 'Closes\n#ISSUE'], + ['null PR description', null], + ]) { + const unproven = closedPlan(name, { prBody }); + refuse( + run('archive', String(unproven.number)), + 'archive requires a closing pull request merged into DocksDocks/fixture:main', + ); + assert.deepEqual(issue(unproven.number).labels, ['plan', 'plan:ongoing']); + } +}); + +test('archive finds the merged pull request for a commit closer beyond the first association page', (t) => { + const { run, updateState, issue, closedPlan } = planCliContext(t); + const commitClosed = closedPlan('commit closer beyond first association page'); + const commitOid = 'a'.repeat(40); + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === commitClosed.number); + entry.timelineItems = [ + { __typename: 'ClosedEvent', closer: commitClosed.closer }, + { __typename: 'ClosedEvent', closer: { __typename: 'Commit', oid: commitOid } }, + ]; + state.commits = [ + { + oid: commitOid, + associatedPullRequests: [ + ...Array.from({ length: 100 }, (_, index) => ({ + number: 10000 + index, + state: 'MERGED', + mergedAt: '2026-08-21T00:00:00Z', + baseRefName: 'release', + })), + { ...commitClosed.closer, body: null }, + ], + }, + ]; + }); + const commitArchive = run('archive', String(commitClosed.number)); + expectSuccess(commitArchive, 'archive merged PR associated with commit on second page'); + assert.equal( + commitArchive.stdout.trim(), + `plan #${commitClosed.number} finished (closed by ${commitClosed.closer.url})`, + ); + assert.deepEqual(issue(commitClosed.number).labels, ['plan']); +}); diff --git a/scripts/tests/plan-cli/archive-retire-list.test.mjs b/scripts/tests/plan-cli/archive-retire-list.test.mjs new file mode 100644 index 00000000..8fdd27e3 --- /dev/null +++ b/scripts/tests/plan-cli/archive-retire-list.test.mjs @@ -0,0 +1,175 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('archive refuses a closer merged into a branch other than the default branch', (t) => { + const { run, closedPlan } = planCliContext(t); + const wrongBase = closedPlan('wrong base', { base: 'release' }); + refuse( + run('archive', String(wrongBase.number)), + 'archive requires a closing pull request merged into DocksDocks/fixture:main', + ); +}); + +test('archive refuses a plan with a non-terminal step', (t) => { + const { run, closedPlan } = planCliContext(t); + const unfinished = closedPlan('unfinished step', { status: 'in-flight' }); + refuse(run('archive', String(unfinished.number)), 'archive refused: non-terminal step fix_parser'); +}); + +test('archive refuses a broken Steps table that parses to zero rows', (t) => { + const { run, updateIssue, closedPlan } = planCliContext(t); + // A broken Steps table parses to zero rows; that never counts as complete work. + const tableless = closedPlan('broken steps table'); + updateIssue(tableless.number, (entry) => { + entry.body = entry.body.replace('| # | Id | Task | Files | Depends | Effect | Status | Done when |\n', ''); + }); + refuse(run('archive', String(tableless.number)), 'archive refused: no Steps rows parsed'); +}); + +test('archive refuses a merged closer when the latest code review asks for repair', (t) => { + const { run, issue, addIssueComment, closedPlan } = planCliContext(t); + // A merged closer does not replace review approval. + const unapproved = closedPlan('review repair'); + addIssueComment(unapproved.number, '## Code review\n\ncode-review: repair\nFix data loss.'); + refuse(run('archive', String(unapproved.number)), 'archive requires Code-review: pass'); + assert.ok(issue(unapproved.number).labels.includes('plan:ongoing')); +}); + +test('show and archive read every page of large comments', (t) => { + const { run, updateState, closedPlan } = planCliContext(t); + const largeComments = closedPlan('large comment pages'); + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === largeComments.number); + for (let i = 0; i < 20; i += 1) { + const id = state.nextComment++; + entry.comments.push({ + id, + body: `Evidence round ${i}\n${'x'.repeat(60000)}`, + author: 'plan-agent', + createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), + }); + } + }); + const largeShow = run('show', String(largeComments.number)); + expectSuccess(largeShow, 'show large comment pages'); + assert.match(largeShow.stdout, /^reviews: plan=none code=pass$/m); + const largeArchive = run('archive', String(largeComments.number)); + expectSuccess(largeArchive, 'archive large comment pages'); + assert.equal( + largeArchive.stdout.trim(), + `plan #${largeComments.number} finished (closed by ${largeComments.closer.url})`, + ); +}); + +test('show advises on a plan closed with unfinished work, and step needs a reopen before it writes', (t) => { + const { run, updateIssue, closedPlan } = planCliContext(t); + const partialMerge = closedPlan('closed by a partial merge', { status: 'in-flight' }); + const partialShow = run('show', String(partialMerge.number)); + expectSuccess(partialShow, 'show early closure'); + assert.ok( + partialShow.stdout + .split(/\r?\n/) + .includes( + `advice: plan is closed as completed while step fix_parser is in-flight; closed by ${partialMerge.closer.url}; reopen the issue if work remains`, + ), + ); + refuse( + run('step', String(partialMerge.number), 'fix_parser', 'in-flight'), + `plan #${partialMerge.number} is finished; reopen it before moving step fix_parser to in-flight`, + ); + updateIssue(partialMerge.number, (entry) => { + entry.state = 'OPEN'; + entry.stateReason = 'REOPENED'; + }); + expectSuccess(run('step', String(partialMerge.number), 'fix_parser', 'done'), 'finish step after reopen'); +}); + +test('retire repairs labels before it closes the issue, and list filters open and closed plans', (t) => { + const { run, updateState, issue, updateIssue, createPlan } = planCliContext(t); + // Retire repairs labels before closing; list retains its filtered open/closed view. + const listed = createPlan('listed drafting plan'); + const unlisted = createPlan('issue without plan label'); + updateIssue(unlisted, (entry) => { + entry.labels = ['other']; + }); + const retiring = createPlan('retired plan'); + updateIssue(retiring, (entry) => { + entry.assignees = []; + }); + updateState((state) => { + state.labelRemovalErrorOnce = 'label removal unavailable'; + }); + refuse( + run('retire', String(retiring), '--reason', 'No longer needed'), + 'gh issue edit failed: label removal unavailable', + ); + assert.equal(issue(retiring).state, 'OPEN', 'a failed label repair cannot close the issue'); + assert.deepEqual(issue(retiring).labels, ['plan', 'plan:drafting']); + const retired = run('retire', String(retiring), '--reason', 'No longer needed'); + expectSuccess(retired, 'retire an unassigned plan'); + assert.equal(retired.stdout.trim(), `plan #${retiring} retired`); + assert.equal(issue(retiring).state, 'CLOSED'); + assert.equal(issue(retiring).stateReason, 'NOT_PLANNED'); + assert.deepEqual(issue(retiring).labels, ['plan']); + assert.deepEqual(issue(retiring).assignees, ['plan-agent'], 'retire claims the plan in the label edit'); + const draftingList = run('list', '--status', 'drafting'); + expectSuccess(draftingList, 'list drafting plans'); + assert.match(draftingList.stdout, new RegExp(`^drafting\\t#${listed}\\tlisted drafting plan$`, 'm')); + assert.doesNotMatch(draftingList.stdout, /issue without plan label/); + assert.doesNotMatch(draftingList.stdout, new RegExp(`\\t#${retiring}\\t`)); + const retiredList = run('list', '--status', 'retired'); + expectSuccess(retiredList, 'list retired plans'); + assert.equal(retiredList.stdout.trim(), `retired\t#${retiring}\tretired plan`); +}); + +test('list advises that more plans may exist when it stops at 500 plans', (t) => { + const { run, updateState } = planCliContext(t); + updateState((state) => { + for (let i = 0; i < 500; i += 1) { + const number = state.nextIssue++; + state.issues.push({ + number, + title: `bulk plan ${i}`, + body: '', + state: 'OPEN', + stateReason: null, + labels: ['plan', 'plan:drafting'], + assignees: ['plan-agent'], + url: `https://github.com/DocksDocks/fixture/issues/${number}`, + createdAt: '2026-08-20T20:00:00Z', + updatedAt: '2026-08-20T20:00:00Z', + comments: [], + events: [], + }); + } + }); + const limitedList = run('list'); + expectSuccess(limitedList, 'list limit advice'); + assert.match(limitedList.stdout, /^advice: list stopped at 500 plans; more plans may exist$/m); +}); + +test('list prints open plans first, then closed plans, each in ascending number order', (t) => { + const { run, updateIssue, createPlan, closedPlan } = planCliContext(t); + // Interleave creation so number order differs from the open/closed grouping. + const finished = closedPlan('finished plan'); + const drafting = createPlan('drafting plan'); + const retired = createPlan('retired plan'); + updateIssue(retired, (entry) => { + entry.state = 'CLOSED'; + entry.stateReason = 'NOT_PLANNED'; + entry.labels = ['plan']; + }); + const ongoing = createPlan('ongoing plan'); + updateIssue(ongoing, (entry) => { + entry.labels = ['plan', 'plan:ongoing']; + }); + const listed = run('list'); + expectSuccess(listed, 'list mixed open and closed plans'); + assert.deepEqual(listed.stdout.trim().split('\n'), [ + `drafting\t#${drafting}\tdrafting plan`, + `ongoing\t#${ongoing}\tongoing plan`, + `finished\t#${finished.number}\tfinished plan`, + `retired\t#${retired}\tretired plan`, + ]); +}); diff --git a/scripts/tests/plan-cli/fenced-examples.test.mjs b/scripts/tests/plan-cli/fenced-examples.test.mjs new file mode 100644 index 00000000..c164941f --- /dev/null +++ b/scripts/tests/plan-cli/fenced-examples.test.mjs @@ -0,0 +1,67 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('a fenced example table or Mode line is never live, and a duplicate section heading blocks the write', (t) => { + const { run, issue, createPlan, edit } = planCliContext(t); + // A fenced example table is not the live table, and a duplicate section heading blocks the write. + const fenced = createPlan('fenced example'); + const example = [ + '```markdown', + '| # | Id | Task | Files | Depends | Effect | Status | Done when |', + '|---:|---|---|---|---|---|---|---|', + '| 1 | example | Example | x | - | local | done | Never |', + '```', + '', + ].join('\n'); + expectSuccess( + edit(fenced, (text) => text.replace('## Steps\n\n', `## Steps\n\n${example}`)), + 'edit with fenced example', + ); + assert.ok( + issue(fenced).body.includes('| 1 | example | Example | x | - | local | done | Never |'), + 'example untouched', + ); + expectSuccess(run('status', String(fenced), 'ongoing'), 'start fenced plan'); + expectSuccess(run('step', String(fenced), 'fix_parser', 'in-flight'), 'step targets the live table'); + assert.match(issue(fenced).body, /\| fix_parser \| .* \| in-flight \|/); + assert.ok( + issue(fenced).body.includes('| 1 | example | Example | x | - | local | done | Never |'), + 'example still untouched', + ); + const doubled = createPlan('duplicate heading'); + refuse( + edit(doubled, (text) => text.replace('## Acceptance', '## steps\n\nNothing.\n\n## Acceptance')), + 'duplicate section heading: Steps', + ); + assert.equal(issue(doubled).body, body(), 'a duplicate heading writes nothing'); + const fencedMode = createPlan('fenced mode example'); + const modeExample = '```markdown\nMode: plan-and-implement\n```\n\n'; + expectSuccess( + edit(fencedMode, (text) => + text + .replace('Mode: plan-and-implement\n', 'Mode: plan-only\n') + .replace('Fix the parser.\n\n', `Fix the parser.\n\n${modeExample}`), + ), + 'edit with fenced mode example', + ); + const fencedModeBody = issue(fencedMode).body; + assert.ok(fencedModeBody.includes(modeExample.trimEnd()), 'fenced Mode example is preserved'); + assert.match(fencedModeBody, /\nMode: plan-only\n/, 'the unfenced Mode decision is the live mode'); + assert.equal(fencedModeBody.match(/^Mode:/gm).length, 2, 'one example, one live Mode line'); +}); + +test('a fenced block that ends a section survives normalization with either fence style', (t) => { + const { issue, createPlan, edit } = planCliContext(t); + for (const fence of ['```', '~~~']) { + const trailing = createPlan(`trailing fence ${fence}`); + const evidence = `${fence}text\nnode smoke.mjs | tail -1\n${fence}`; + expectSuccess( + edit(trailing, (text) => + text.replace('The parser loses mixed-case records.\n', `The parser loses mixed-case records.\n\n${evidence}\n`), + ), + `edit with trailing ${fence} fence`, + ); + assert.ok(issue(trailing).body.includes(evidence), `trailing ${fence} block is preserved`); + } +}); diff --git a/scripts/tests/plan-cli/harness.mjs b/scripts/tests/plan-cli/harness.mjs new file mode 100644 index 00000000..282ac7d8 --- /dev/null +++ b/scripts/tests/plan-cli/harness.mjs @@ -0,0 +1,224 @@ +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../..'); +const PLAN_CLI = path.join(ROOT, 'plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs'); +const GH_STUB_DIR = path.join(ROOT, 'scripts/tests/fixtures/gh-stub'); +// The state a repository reaches after its first `new`: every plan label exists as the fake gh +// `label create` stores it. A case that depends on label metadata replaces this map itself. +const PLAN_LABELS = ['plan', 'plan:drafting', 'plan:planned', 'plan:ongoing', 'plan:blocked']; + +export function expectSuccess(result, label) { + assert.equal(result.status, 0, `${label}: ${result.stderr}`); +} + +export function refuse(result, message) { + assert.equal(result.status, 1, message); + assert.equal(result.stderr, `${message}\n`); +} + +export function body(status = 'planned') { + return [ + '', + '', + '## Goal', + '', + 'Fix the parser.', + '', + 'Mode: plan-and-implement', + '', + '## Research', + '', + 'The parser loses mixed-case records.', + '', + '## Steps', + '', + '| # | Id | Task | Files | Depends | Effect | Status | Done when |', + '|---:|---|---|---|---|---|---|---|', + `| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | ${status} | Records round-trip |`, + '', + '## Acceptance', + '', + '| ID | Command | Expected |', + '|---|---|---|', + '| A1 | node smoke.mjs | Exit 0 |', + '', + '## Do not touch', + '', + 'Release tooling.', + '', + '## Open questions', + '', + 'None.', + '', + '## Verification Results', + '', + 'Not run.', + '', + ].join('\n'); +} + +// Each test gets its own scratch repository, fake gh state, and XDG_STATE_HOME, so tests share no +// issue numbers, labels, or exports and the runner can execute case files in parallel. +export function planCliContext(t) { + const stateBase = process.env.XDG_STATE_HOME || path.join(process.env.HOME || os.homedir(), '.local', 'state'); + const testScratchRoot = path.join(stateBase, 'docks'); + fs.mkdirSync(testScratchRoot, { recursive: true, mode: 0o700 }); + fs.chmodSync(testScratchRoot, 0o700); + const scratch = fs.realpathSync(fs.mkdtempSync(path.join(testScratchRoot, 'plan-cli-'))); + const stateHome = fs.realpathSync(fs.mkdtempSync(path.join(testScratchRoot, 'plan-cli-state-'))); + t.after(() => { + fs.rmSync(scratch, { recursive: true, force: true }); + fs.rmSync(stateHome, { recursive: true, force: true }); + }); + const statePath = path.join(scratch, 'gh-state.json'); + const initialized = spawnSync('git', ['init', '--quiet'], { cwd: scratch, encoding: 'utf8' }); + assert.equal(initialized.status, 0, initialized.stderr); + const committed = spawnSync( + 'git', + [ + '-c', + 'user.name=Plan CLI test', + '-c', + 'user.email=plan-cli@example.invalid', + 'commit', + '--quiet', + '--allow-empty', + '-m', + 'fixture', + ], + { cwd: scratch, encoding: 'utf8' }, + ); + assert.equal(committed.status, 0, committed.stderr); + fs.writeFileSync( + statePath, + `${JSON.stringify( + { + repo: { nameWithOwner: 'DocksDocks/fixture', visibility: 'PRIVATE', defaultBranchRef: { name: 'main' } }, + repositoryDefaults: { 'DocksDocks/fixture': 'main' }, + viewerLogin: 'plan-agent', + labels: Object.fromEntries(PLAN_LABELS.map((name) => [name, { color: 'ededed' }])), + issues: [], + prs: [], + nextIssue: 1, + nextComment: 1, + clock: 0, + calls: [], + }, + null, + 2, + )}\n`, + ); + const childEnv = { + ...process.env, + PATH: `${GH_STUB_DIR}${path.delimiter}${process.env.PATH ?? ''}`, + GH_STUB_STATE: statePath, + XDG_STATE_HOME: stateHome, + }; + + function run(...args) { + return spawnSync(process.execPath, [PLAN_CLI, ...args], { + cwd: scratch, + encoding: 'utf8', + env: childEnv, + }); + } + + function loadState() { + return JSON.parse(fs.readFileSync(statePath, 'utf8')); + } + + function updateState(update) { + const state = loadState(); + update(state); + fs.writeFileSync(statePath, `${JSON.stringify(state, null, 2)}\n`); + } + + function issue(number) { + return loadState().issues.find((entry) => entry.number === number); + } + + function updateIssue(number, update) { + updateState((state) => update(state.issues.find((entry) => entry.number === number))); + } + + function addIssueComment(number, body, author = 'plan-agent') { + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === number); + const id = state.nextComment++; + entry.comments ??= []; + entry.comments.push({ + id, + body, + author, + createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), + }); + }); + } + + function createPlan(name, text = body()) { + const result = run('new', '--title', name, '--goal', 'Preserve plan records'); + expectSuccess(result, `new ${name}`); + const match = /^plan created: #(\d+) (https:\/\/\S+)$/m.exec(result.stdout); + assert.ok(match, 'new prints the issue identity'); + const number = Number(match[1]); + updateIssue(number, (entry) => { + entry.body = text; + }); + return number; + } + + function exported(number) { + const result = run('export', String(number)); + expectSuccess(result, 'export'); + const file = result.stdout.trim(); + assert.ok(path.isAbsolute(file), 'export returns an absolute file path'); + return file; + } + + function edit(number, transform = (text) => text) { + const file = exported(number); + fs.writeFileSync(file, transform(fs.readFileSync(file, 'utf8'))); + return run('edit', String(number), '--file', file); + } + + function closedPlan(name, { status = 'done', base = 'main', prBody } = {}) { + const number = createPlan(name, body(status)); + const closer = { + __typename: 'PullRequest', + number: 100 + number, + url: `https://github.com/DocksDocks/fixture/pull/${100 + number}`, + state: 'MERGED', + mergedAt: '2026-08-21T00:00:00Z', + baseRefName: base, + body: prBody === undefined ? `Closes #${number}` : prBody?.replaceAll('ISSUE', String(number)), + repository: { nameWithOwner: 'DocksDocks/fixture' }, + }; + updateIssue(number, (entry) => { + entry.state = 'CLOSED'; + entry.stateReason = 'COMPLETED'; + entry.labels = ['plan', 'plan:ongoing']; + entry.timelineItems = [{ closer }]; + }); + addIssueComment(number, '### Code review (round 2)\n\ncode-review: pass'); + return { number, closer }; + } + + return { + scratch, + run, + loadState, + updateState, + issue, + updateIssue, + addIssueComment, + createPlan, + exported, + edit, + closedPlan, + }; +} diff --git a/scripts/tests/plan-cli/indented-and-mode.test.mjs b/scripts/tests/plan-cli/indented-and-mode.test.mjs new file mode 100644 index 00000000..fa562f44 --- /dev/null +++ b/scripts/tests/plan-cli/indented-and-mode.test.mjs @@ -0,0 +1,94 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('an indented Mode example never becomes the live Mode, wherever it sits in Goal', (t) => { + const { issue, createPlan, edit } = planCliContext(t); + // An indented code example never becomes the live Mode. + const indentedMode = createPlan('indented mode example'); + expectSuccess( + edit(indentedMode, (text) => + text + .replace('Mode: plan-and-implement\n', 'Mode: plan-only\n') + .replace('Fix the parser.\n\n', 'Fix the parser.\n\n Mode: plan-and-implement\n\n'), + ), + 'edit with indented mode example', + ); + assert.ok(issue(indentedMode).body.includes(' Mode: plan-and-implement\n'), 'indented example is preserved'); + assert.match(issue(indentedMode).body, /\nMode: plan-only\n/, 'the plan-only decision is the live mode'); + + for (const [label, transform] of [ + [ + 'leading', + (text) => + text.replace( + '## Goal\n\nFix the parser.\n\n', + '## Goal\n\n Mode: plan-and-implement\n\nFix the parser.\n\n', + ), + ], + ['trailing', (text) => text.replace('Mode: plan-only\n\n', 'Mode: plan-only\n\n Mode: plan-and-implement\n\n')], + ]) { + const placed = createPlan(`${label} indented mode example`); + expectSuccess( + edit(placed, (text) => transform(text.replace('Mode: plan-and-implement\n', 'Mode: plan-only\n'))), + `edit with ${label} indented example`, + ); + expectSuccess(edit(placed), `${label} second normalization`); + assert.ok(issue(placed).body.includes(' Mode: plan-and-implement\n'), `${label} indented example is preserved`); + assert.match(issue(placed).body, /\nMode: plan-only\n/, `${label}: the plan-only decision stays live`); + assert.equal(issue(placed).body.match(/^Mode:/gm).length, 1, `${label}: exactly one live Mode line`); + } +}); + +test('an indented example table before the live table is neither selected nor rewritten', (t) => { + const { run, issue, createPlan, edit } = planCliContext(t); + const indentedTable = createPlan('indented example table'); + const indentedExample = [ + ' | # | Id | Task | Files | Depends | Effect | Status | Done when |', + ' |---:|---|---|---|---|---|---|---|', + ' | 1 | example | Example | x | - | local | done | Never |', + ].join('\n'); + expectSuccess( + edit(indentedTable, (text) => text.replace('## Steps\n\n', `## Steps\n\n${indentedExample}\n\n`)), + 'edit with indented example table', + ); + assert.ok(issue(indentedTable).body.includes(indentedExample), 'indented example bytes are preserved'); + expectSuccess(run('status', String(indentedTable), 'ongoing'), 'start indented-table plan'); + expectSuccess(run('step', String(indentedTable), 'fix_parser', 'in-flight'), 'step targets the live table'); + assert.match(issue(indentedTable).body, /\| fix_parser \| .* \| in-flight \|/); + assert.ok(issue(indentedTable).body.includes(indentedExample), 'indented example still untouched'); +}); + +test('an explicit --mode outranks a Mode line in --goal, and a goal Mode line stays live without --mode', (t) => { + const { run, issue, edit } = planCliContext(t); + // An explicit --mode outranks a Mode line embedded in --goal; a later live Mode line wins. + const explicit = run( + 'new', + '--title', + 'explicit mode', + '--goal', + 'Ship it.\nMode: plan-and-implement\n\n## Context\nDetails.\n\n```text\n## fenced heading\n```', + '--mode', + 'plan-only', + ); + expectSuccess(explicit, 'new with explicit mode'); + assert.doesNotMatch(explicit.stdout, /Mode defaulted/, 'advice honors the explicit --mode'); + const explicitNumber = Number(/^plan created: #(\d+)/m.exec(explicit.stdout)[1]); + assert.match(issue(explicitNumber).body, /\nMode: plan-only\n/, 'explicit --mode is the live mode'); + assert.equal(issue(explicitNumber).body.match(/^Mode:/gm).length, 1, 'the embedded Mode line is removed'); + assert.ok( + issue(explicitNumber).body.includes('### Context\nDetails.\n\n```text\n## fenced heading\n```\n\nMode: plan-only'), + 'an unfenced H2 in --goal is demoted to H3, a fenced one is kept, and both stay inside Goal', + ); + refuse( + edit(explicitNumber, (text) => text.replace('## Acceptance', '## Context\n\nDetails.\n\n## Acceptance')), + 'unknown section heading: Context; use only Goal, Research, Steps, Acceptance, Do not touch, Open questions, Verification Results or write it below an existing section as ### or plain text', + ); + const embedded = run('new', '--title', 'embedded mode', '--goal', 'Ship it.\nMode: plan-and-implement'); + expectSuccess(embedded, 'new with embedded mode only'); + assert.match( + issue(Number(/^plan created: #(\d+)/m.exec(embedded.stdout)[1])).body, + /\nMode: plan-and-implement\n/, + 'goal-authored Mode is kept without --mode', + ); +}); diff --git a/scripts/tests/plan-cli/records.test.mjs b/scripts/tests/plan-cli/records.test.mjs new file mode 100644 index 00000000..92a4ea6e --- /dev/null +++ b/scripts/tests/plan-cli/records.test.mjs @@ -0,0 +1,178 @@ +import assert from 'node:assert/strict'; +import { createHash } from 'node:crypto'; +import fs from 'node:fs'; +import test from 'node:test'; +import { normalizePlan } from '../../../plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('new creates each missing plan label and keeps existing label metadata unchanged', (t) => { + const { loadState, updateState, createPlan } = planCliContext(t); + // Existing label metadata is not an input for new and must survive unchanged. + const existingLabels = { + plan: { color: '123456', description: 'Keep this color' }, + 'plan:drafting': { color: 'abcdef', description: 'Keep this description' }, + }; + updateState((state) => { + state.labels = structuredClone(existingLabels); + }); + createPlan('labels already present'); + assert.deepEqual( + Object.fromEntries(Object.entries(loadState().labels).filter(([name]) => name in existingLabels)), + existingLabels, + 'new creates missing labels without changing existing metadata', + ); + assert.deepEqual( + Object.keys(loadState().labels).sort(), + ['plan', 'plan:blocked', 'plan:drafting', 'plan:ongoing', 'plan:planned'], + 'new creates each missing plan label', + ); + const allLabels = loadState().labels; + createPlan('all labels already present'); + assert.deepEqual(loadState().labels, allLabels, 'repeating new leaves every existing label unchanged'); +}); + +test('edit writes the normalized body to the remote issue and to the export provenance', (t) => { + const { issue, createPlan, exported, edit } = planCliContext(t); + // Normalization must reach the remote body, not just the local export. + const irregular = body('Done') + .replace('## Goal', '## goal') + .replace('Mode: plan-and-implement\n', '') + .replace('fix_parser', 'Fix-Parser') + .replace('| local |', '| Local |') + .replace('Fix the parser.', 'Fix the parser — preserve records.'); + const normalized = normalizePlan(irregular); + assert.match(normalized.body, /Mode: plan-only/); + assert.match(normalized.body, /## Goal\n/); + assert.match(normalized.body, /\| fix_parser \|/); + assert.match(normalized.body, /\| `?local`? \| `?done`? \|/); + assert.ok(!normalized.body.includes('—')); + assert.match(normalized.body, /Fix the parser - preserve records\./); + const normalization = createPlan('normalization', irregular); + const normalizedEdit = edit(normalization); + expectSuccess(normalizedEdit, 'normalize edit'); + assert.equal(issue(normalization).body, normalized.body); + assert.match(normalizedEdit.stdout, /^advice: .*mode/im); + const normalizedFile = exported(normalization); + assert.equal( + fs.readFileSync(`${normalizedFile}.origin`, 'utf8'), + `${createHash('sha256').update(normalized.body).digest('hex')}\n`, + ); +}); + +test('machine path advice names each distinct machine path and skips routes, portable paths, and slash text', () => { + // Machine path advice names each distinct path into a home, a mount, a named temporary or runtime directory, + // a Windows drive, or a network share. URL routes, portable system paths, and slash text get none (#71). + const machinePathAdvice = (text) => + normalizePlan(body().replace('The parser loses mixed-case records.', text)).advice.filter((message) => + message.startsWith('body contains an absolute machine path'), + ); + const named = (...paths) => paths.map((found) => `body contains an absolute machine path: ${found}`); + for (const [text, expected] of [ + ['Clone into /home//projects/x.', named('/home//projects/x')], + ['Open `/Users//src`.', named('/Users//src')], + ['Read /root/.ssh/config and /mnt/data/x', named('/root/.ssh/config', '/mnt/data/x')], + [ + 'Mounts /media//usb, /run/media//usb, /Volumes/Data/x', + named('/media//usb', '/run/media//usb', '/Volumes/Data/x'), + ], + ['Scratch `/tmp/`, /var/tmp/x, /private/tmp/x', named('/tmp/', '/var/tmp/x', '/private/tmp/x')], + [ + 'Temp /var/folders/ab/cd/T/x and /private/var/folders/ab/T/x', + named('/var/folders/ab/cd/T/x', '/private/var/folders/ab/T/x'), + ], + ['XDG_RUNTIME_DIR=/run/user/1000/bus', named('/run/user/1000/bus')], + [ + 'Windows C:\\Users\\\\x, D:/work/x, \\\\server\\share\\x', + named('C:\\Users\\\\x', 'D:/work/x', '\\\\server\\share\\x'), + ], + ['See [log](/home//log.txt) and file:///home//y', named('/home//log.txt', '/home//y')], + ['Twice /home/a/x, /home/a/x then /tmp/y;', named('/home/a/x', '/tmp/y')], + ['Routes /login/, `/wp-json//v1/items/123`, /search/transfer/user', []], + ['TMPDIR=/tmp node t.mjs, /usr/bin/php8.5, #!/usr/bin/env node, x > /dev/null', []], + ["Pick `A`/`B`, awk '/^## /{f=0}', __DIR__ . '/vendor/autoload.php'", []], + ['URLs https://example.com/home/a and ssh://host/home/a', []], + ['Portable ~/.local/state/docks/x, $HOME/x, bare /tmp, /tmp/ and /home/', []], + ["Regex `'\\\\d+\\\\.'`", []], + ]) + assert.deepEqual(machinePathAdvice(text), expected, text); +}); + +test('edit refuses step ids that collide after normalization and writes nothing', (t) => { + const { issue, createPlan, edit } = planCliContext(t); + // Ids that differ only by case or dash collapse after normalization; the write refuses the collision. + const collision = createPlan('id-collision'); + const collisionEdit = edit(collision, (text) => + text.replace( + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |\n| 2 | Fix-Parser | Second | src/other.mjs | - | local | planned | Also round-trips |', + ), + ); + refuse(collisionEdit, 'duplicate step id after normalization: fix_parser'); + assert.equal(issue(collision).body, body(), 'a refused collision writes nothing'); +}); + +test('show reads a v3 record and the next edit writes it as v4', (t) => { + const { run, issue, createPlan, edit } = planCliContext(t); + // Old records stay readable, but the next body write uses v4. + const legacy = createPlan( + 'legacy', + body() + .replace('plan-contract: v4', 'plan-contract: v3') + .replace('## Verification Results', '## Review\n\nOld body review.\n\n## Verification Results'), + ); + const legacyShow = run('show', String(legacy), '--body'); + expectSuccess(legacyShow, 'show v3'); + assert.equal(legacyShow.stdout.trim(), issue(legacy).body.trim()); + assert.match(legacyShow.stderr, /reviews: plan=none code=none/); + expectSuccess(edit(legacy), 'edit v3'); + assert.match(issue(legacy).body, /^/); + assert.doesNotMatch(issue(legacy).body, /^## Review$/m); +}); + +test('dependency normalization resolves a numeric step id and a second pass changes nothing', (t) => { + const { issue, createPlan, edit } = planCliContext(t); + // Dependency normalization is idempotent even when a step id is a number. + const numericId = createPlan('numeric step id'); + const numericRows = [ + '| 1 | prepare | Prepare | src/a.mjs | - | local | planned | Ready |', + '| 2 | 1 | Numeric | src/b.mjs | - | local | planned | Ready |', + '| 3 | finish | Finish | src/c.mjs | prepare | local | planned | Ready |', + ].join('\n'); + expectSuccess( + edit(numericId, (text) => + text.replace( + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + numericRows, + ), + ), + 'edit with numeric id', + ); + const once = issue(numericId).body; + assert.match(once, /\| 3 \| finish \| Finish \| src\/c\.mjs \| 1 \|/, 'prepare resolves to display 1'); + expectSuccess(edit(numericId), 'second normalization'); + assert.equal(issue(numericId).body, once, 'a second normalization changes nothing'); +}); + +test('edit advises on steps that share a path only when no Depends chain links them (#78)', (t) => { + const { createPlan, edit } = planCliContext(t); + // Steps that edit the same path need a Depends chain, or parallel work collides (#78). + const sharedFiles = createPlan('shared files'); + const sharedRows = [ + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + '| 2 | parse_dates | Parse dates | `src/parser.mjs`, src/dates.mjs | - | local | planned | Dates parse |', + '| 3 | format_dates | Format dates | src/dates.mjs | parse_dates | local | planned | Dates format |', + '| 4 | print_dates | Print dates | src/dates.mjs | format_dates | local | planned | Dates print |', + ].join('\n'); + const sharedEdit = edit(sharedFiles, (text) => + text.replace( + '| 1 | fix_parser | Fix parser | src/parser.mjs | - | local | planned | Records round-trip |', + sharedRows, + ), + ); + expectSuccess(sharedEdit, 'edit with shared files'); + assert.deepEqual( + sharedEdit.stdout.split('\n').filter((line) => line.includes(' share ')), + ['advice: steps fix_parser and parse_dates share src/parser.mjs with no Depends link'], + 'only steps without a Depends chain get shared-path advice', + ); +}); diff --git a/scripts/tests/plan-cli/reviews.test.mjs b/scripts/tests/plan-cli/reviews.test.mjs new file mode 100644 index 00000000..d822770a --- /dev/null +++ b/scripts/tests/plan-cli/reviews.test.mjs @@ -0,0 +1,146 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { parseReviewComment } from '../../../plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('review parsing accepts flexible prose, and a severe finding turns a code pass into fixes-required', () => { + // Review prose is flexible. Severity still prevents an unsafe code pass. + const reviewCases = [ + ['## Plan review (round 2)\n\nplan-review: pass', { kind: 'plan', verdict: 'pass' }], + [ + '### Plan review — round 2\n\nplan review: changes\nParser — records are lost.', + { kind: 'plan', verdict: 'repair' }, + ], + [ + '# Code review\n\ncode-review: approved\nHIGH: The parser loses data.', + { kind: 'code', verdict: 'fixes-required' }, + ], + [ + '### Code review round 1 - 2026-09-09\nCode-review: pass\n- [low] src/api.mjs:3 - the high-level API name is unclear; critical path is fine - rename', + { kind: 'code', verdict: 'pass' }, + ], + [ + '### Code review round 1 - 2026-09-09\nCode-review: pass\n- [HIGH] src/api.mjs:3 - data loss - fix', + { kind: 'code', verdict: 'fixes-required' }, + ], + ['**### Code review round 1**\n\n**Code-review: pass**', { kind: 'code', verdict: 'pass' }], + ['### Code review round 2\n\n**Code-review:** `pass`.', { kind: 'code', verdict: 'pass' }], + ['## **Plan review** round 1\n\n- Plan-review: _repair_', { kind: 'plan', verdict: 'repair' }], + [ + '### Code review round 3\nCode-review: pass\n- **[HIGH]** src/example.mjs:1 - data loss - fix', + { kind: 'code', verdict: 'fixes-required' }, + ], + [ + '### Code review round 3\nCode-review: pass\n1. Bug [critical] src/example.mjs:1 - data loss - fix', + { kind: 'code', verdict: 'fixes-required' }, + ], + ['### Code review round 3\nCode-review: pass\n> Severity: High', { kind: 'code', verdict: 'fixes-required' }], + ]; + for (const [text, expected] of reviewCases) assert.deepEqual(parseReviewComment(text), expected); +}); + +test('show reports only trusted reviews, and an older timestamp cannot override a newer verdict', (t) => { + const { run, updateIssue, addIssueComment, createPlan } = planCliContext(t); + const reviews = createPlan('trusted reviews'); + addIssueComment(reviews, '## Plan review (round 2)\n\nplan-review: pass'); + addIssueComment(reviews, '## Plan review\n\nplan-review: blocked', 'foreign-agent'); + addIssueComment(reviews, '## Code review\n\ncode-review: pass\nHIGH: Data loss.'); + let shown = run('show', String(reviews)); + expectSuccess(shown, 'trusted review show'); + assert.match(shown.stdout, /^reviews: plan=pass code=fixes-required$/m); + addIssueComment(reviews, '## Plan review\n\nplan-review: repair'); + updateIssue(reviews, (entry) => { + entry.comments.at(-1).createdAt = '2020-01-01T00:00:00Z'; + }); + shown = run('show', String(reviews)); + expectSuccess(shown, 'timestamp review show'); + assert.match(shown.stdout, /^reviews: plan=pass code=fixes-required$/m); +}); + +test('show reads reviews beyond the first comment page and fails closed when a comment page fails', (t) => { + const { run, updateState, addIssueComment, createPlan } = planCliContext(t); + const pagedReviews = createPlan('reviews beyond first page'); + addIssueComment(pagedReviews, '## Code review\n\ncode-review: pass'); + updateState((state) => { + const entry = state.issues.find((candidate) => candidate.number === pagedReviews); + for (let index = 0; index < 99; index += 1) { + const id = state.nextComment++; + entry.comments.push({ + id, + body: 'Routine discussion.', + author: 'foreign-agent', + createdAt: new Date(Date.parse('2026-08-20T20:00:00Z') + id * 1000).toISOString(), + }); + } + }); + addIssueComment(pagedReviews, '## Code review\n\ncode-review: repair', 'plan-agent'); + addIssueComment(pagedReviews, '## Code review\n\ncode-review: pass', 'foreign-agent'); + const pagedShow = run('show', String(pagedReviews)); + expectSuccess(pagedShow, 'show all review pages'); + assert.match(pagedShow.stdout, /^reviews: plan=none code=fixes-required$/m, 'late trusted review wins'); + updateState((state) => { + state.graphqlError = { on: 'page', errors: [{ message: 'comment page unavailable' }] }; + }); + const brokenReviewPage = run('show', String(pagedReviews)); + assert.equal(brokenReviewPage.status, 1, 'a failed review page cannot produce a verdict'); + assert.match(brokenReviewPage.stderr, /graphql/i); + assert.equal(brokenReviewPage.stdout, ''); +}); + +test('show reads a review record written in bold Markdown', (t) => { + const { run, addIssueComment, createPlan } = planCliContext(t); + const boldReview = createPlan('bold review record'); + addIssueComment(boldReview, '**### Code review round 1**\n\n**Code-review: pass**'); + const boldShow = run('show', String(boldReview)); + expectSuccess(boldShow, 'show bold review record'); + assert.match(boldShow.stdout, /^reviews: plan=none code=pass$/m); +}); + +test('status planned or ongoing requires research that replaces the placeholder (#78)', (t) => { + const { run, issue, createPlan } = planCliContext(t); + // A drafted plan enters planned or ongoing only after research replaces the placeholder (#78). + const unresearched = createPlan( + 'unresearched plan', + body().replace('The parser loses mixed-case records.', '_Not researched yet._'), + ); + refuse(run('status', String(unresearched), 'planned'), 'status planned requires a filled Research section'); + refuse(run('status', String(unresearched), 'ongoing'), 'status ongoing requires a filled Research section'); + assert.deepEqual(issue(unresearched).labels, ['plan', 'plan:drafting'], 'a refused status keeps the phase label'); + expectSuccess( + run('status', String(unresearched), 'blocked', '--reason', 'Waiting for research'), + 'an unresearched plan can still block', + ); + const emptyResearch = createPlan('empty research', body().replace('The parser loses mixed-case records.', '')); + refuse(run('status', String(emptyResearch), 'planned'), 'status planned requires a filled Research section'); + const quotedPlaceholder = createPlan( + 'research quotes the placeholder', + body().replace( + 'The parser loses mixed-case records.', + 'A new plan holds `_Not researched yet._` until research is published.', + ), + ); + const quotedStatus = run('status', String(quotedPlaceholder), 'planned'); + expectSuccess(quotedStatus, 'research prose may quote the placeholder'); + assert.doesNotMatch(quotedStatus.stdout, /Research is not filled/, 'quoted placeholder is not unfilled research'); +}); + +test('a plan-review pass covers evidence edits, and a Research edit after it advises a new round (#78)', (t) => { + const { addIssueComment, createPlan, edit } = planCliContext(t); + // A plan-review pass covers the body that the reviewer read (#78). + const reviewed = createPlan('reviewed plan'); + addIssueComment(reviewed, '### Plan review round 1\n\nPlan-review: pass\n\nNone.'); + const resultsEdit = edit(reviewed, (text) => text.replace('Not run.', 'Smoke passed.')); + expectSuccess(resultsEdit, 'edit verification results after a plan pass'); + assert.doesNotMatch(resultsEdit.stdout, /plan review passed before this edit/, 'evidence edits need no new round'); + const researchEdit = edit(reviewed, (text) => + text.replace( + 'The parser loses mixed-case records.', + 'The parser loses mixed-case records.\n\nDecision: keep the old date format.', + ), + ); + expectSuccess(researchEdit, 'edit research after a plan pass'); + assert.match( + researchEdit.stdout, + /^advice: plan review passed before this edit; changed sections: Research; run a new plan-review round before code that depends on them$/m, + ); +}); diff --git a/scripts/tests/plan-cli/status.test.mjs b/scripts/tests/plan-cli/status.test.mjs new file mode 100644 index 00000000..20549bbf --- /dev/null +++ b/scripts/tests/plan-cli/status.test.mjs @@ -0,0 +1,94 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('history freezes step state once work starts, and status refuses transitions that erase started work', (t) => { + const { run, issue, updateIssue, createPlan, edit } = planCliContext(t); + // The event history keeps step state frozen even if phase labels are reset. + const frozen = createPlan('frozen steps'); + updateIssue(frozen, (entry) => { + entry.events = [{ event: 'labeled', label: { name: 'plan:ongoing' } }]; + }); + for (const [name, before, after] of [ + ['Status', '| planned |', '| done |'], + ['Id', '| fix_parser |', '| renamed |'], + ['Depends', '| - |', '| 2 |'], + ]) { + const result = edit(frozen, (text) => text.replace(before, after)); + assert.equal(result.status, 1, `${name} must stay frozen`); + assert.match(result.stderr, /^step state is frozen once work starts: .+\n$/); + assert.equal(issue(frozen).body, body(), `${name} refusal must preserve the remote body`); + } + expectSuccess( + edit(frozen, (text) => text.replace('Fix parser', 'Repair parser')), + 'task text edit', + ); + assert.match(issue(frozen).body, /Repair parser/); + const pagedHistory = createPlan('ongoing history beyond first page'); + updateIssue(pagedHistory, (entry) => { + entry.events = [ + ...Array.from({ length: 100 }, () => ({ event: 'labeled', label: { name: 'unrelated' } })), + { event: 'labeled', label: { name: 'plan:ongoing' } }, + ]; + }); + refuse( + edit(pagedHistory, (text) => text.replace('| planned |', '| done |')), + 'step state is frozen once work starts: step fix_parser status changed', + ); + assert.equal(issue(pagedHistory).body, body(), 'a late history event freezes the original step state'); + + // Labels cannot erase started work, but drafting can enter implementation. + const transitions = createPlan('status transitions'); + expectSuccess(run('status', String(transitions), 'ongoing'), 'drafting to ongoing'); + assert.ok(issue(transitions).labels.includes('plan:ongoing')); + refuse(run('status', String(transitions), 'planned'), 'illegal plan status transition: ongoing -> planned'); + refuse(run('status', String(frozen), 'planned'), 'illegal plan status transition: drafting -> planned'); +}); + +test('phase labels compare case-insensitively and a transition removes the stored spelling', (t) => { + const { run, issue, updateIssue, createPlan, edit } = planCliContext(t); + // Phase labels compare case-insensitively and are removed by their stored spelling. + const mixedLabels = createPlan('mixed-case labels'); + updateIssue(mixedLabels, (entry) => { + entry.labels = ['plan', 'Plan:Drafting']; + }); + assert.match(run('show', String(mixedLabels)).stdout, /· drafting ·/, 'mixed-case phase label is read'); + expectSuccess(run('status', String(mixedLabels), 'planned'), 'transition from a mixed-case label'); + assert.deepEqual(issue(mixedLabels).labels, ['plan', 'plan:planned'], 'the mixed-case label is removed'); + updateIssue(mixedLabels, (entry) => { + entry.events = [{ event: 'labeled', label: { name: 'Plan:Ongoing' } }]; + }); + refuse( + edit(mixedLabels, (text) => text.replace('| planned |', '| done |')), + 'step state is frozen once work starts: step fix_parser status changed', + ); + refuse(run('status', String(mixedLabels), 'drafting'), 'illegal plan status transition: planned -> drafting'); + assert.deepEqual(issue(mixedLabels).labels, ['plan', 'plan:planned'], 'history keeps the plan out of drafting'); +}); + +test('a phase label beyond the first label page is read and replaced, and other labels stay', (t) => { + const { run, issue, updateIssue, createPlan } = planCliContext(t); + const pagedLabels = createPlan('phase label beyond first page'); + const unrelatedLabels = Array.from({ length: 100 }, (_, index) => `topic:${index}`); + updateIssue(pagedLabels, (entry) => { + entry.labels = ['plan', ...unrelatedLabels, 'Plan:Drafting']; + }); + const pagedLabelShow = run('show', String(pagedLabels)); + expectSuccess(pagedLabelShow, 'show label on second page'); + assert.match(pagedLabelShow.stdout, /· drafting ·/); + expectSuccess(run('status', String(pagedLabels), 'planned'), 'transition label on second page'); + assert.deepEqual( + issue(pagedLabels).labels, + ['plan', ...unrelatedLabels, 'plan:planned'], + 'phase repair preserves other labels and removes the stored spelling', + ); +}); + +test('step refuses to reopen a terminal row', (t) => { + const { run, issue, createPlan } = planCliContext(t); + // A terminal row cannot be reopened by the step command. + const terminal = createPlan('terminal step', body('done')); + expectSuccess(run('status', String(terminal), 'ongoing'), 'start terminal plan'); + refuse(run('step', String(terminal), 'fix_parser', 'in-flight'), 'illegal step status transition: done -> in-flight'); + assert.match(issue(terminal).body, /\| `?done`? \|/); +}); diff --git a/scripts/tests/plan-cli/steps-table.test.mjs b/scripts/tests/plan-cli/steps-table.test.mjs new file mode 100644 index 00000000..d9647825 --- /dev/null +++ b/scripts/tests/plan-cli/steps-table.test.mjs @@ -0,0 +1,82 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('edit and step preserve sibling rows whose cells hold escaped pipes or backslash runs', (t) => { + const { run, issue, createPlan, edit } = planCliContext(t); + // A step update preserves every sibling row, including one whose cell holds an escaped pipe. + const piped = createPlan('escaped pipe sibling'); + const pipedRow = '| 2 | grep_pipe | Grep `a \\| b` | src/grep.mjs | - | local | planned | Pipe survives |'; + expectSuccess( + edit(piped, (text) => text.replace('| Records round-trip |\n', `| Records round-trip |\n${pipedRow}\n`)), + 'add piped row', + ); + assert.ok(issue(piped).body.includes(pipedRow), 'edit preserves the escaped pipe cell'); + expectSuccess(run('status', String(piped), 'ongoing'), 'start piped plan'); + expectSuccess(run('step', String(piped), 'fix_parser', 'in-flight'), 'update sibling of piped row'); + assert.ok(issue(piped).body.includes(pipedRow), 'step preserves the escaped pipe row'); + assert.match(issue(piped).body, /\| fix_parser \| .* \| in-flight \|/); + const backslashRow = + '| 3 | back_slash | Path `C:\\\\` | src\\\\win.mjs | - | local | planned | Ends with a literal backslash |'; + expectSuccess( + edit(piped, (text) => text.replace(`${pipedRow}\n`, `${pipedRow}\n${backslashRow}\n`)), + 'add a row whose cell ends in an even backslash run', + ); + assert.ok(issue(piped).body.includes(backslashRow), 'an even backslash run before a pipe is a delimiter'); + const oddCell = String.raw`Keep a \\\| b`; + assert.equal(oddCell, 'Keep a \\\\\\| b', 'the raw fixture holds exactly three backslashes before the pipe'); + const oddRow = `| 4 | odd_run | ${oddCell} | src/odd.mjs | - | local | planned | Three backslashes escape the pipe |`; + expectSuccess( + edit(piped, (text) => text.replace(`${backslashRow}\n`, `${backslashRow}\n${oddRow}\n`)), + 'add a row with an odd backslash run before a pipe', + ); + assert.ok( + issue(piped).body.includes(`| ${oddCell} |`), + 'an odd backslash run before a pipe keeps the pipe in the cell', + ); + expectSuccess(run('step', String(piped), 'fix_parser', 'done'), 'step rewrite beside the odd-run row'); + assert.ok(issue(piped).body.includes(oddRow), 'the odd-run row survives a step rewrite as one cell'); +}); + +test('a Steps row that does not parse to eight cells blocks edit and step instead of vanishing', (t) => { + const { run, issue, updateIssue, createPlan, edit } = planCliContext(t); + // A row that does not parse to eight cells blocks every write instead of vanishing. + const malformed = createPlan('malformed row'); + refuse( + edit(malformed, (text) => + text.replace( + '| Records round-trip |\n', + '| Records round-trip |\n| 2 | broken | Unescaped a | b | src | - | local | planned | Lost |\n', + ), + ), + 'Steps row 2 has 9 cells; expected 8. Escape a literal pipe as \\| so the row is preserved.', + ); + updateIssue(malformed, (entry) => { + entry.body = entry.body.replace( + '| Records round-trip |\n', + '| Records round-trip |\n| 2 | broken | Unescaped a | b | src | - | local | planned | Lost |\n', + ); + }); + expectSuccess(run('status', String(malformed), 'ongoing'), 'start malformed plan'); + refuse( + run('step', String(malformed), 'fix_parser', 'in-flight'), + 'Steps row 2 has 9 cells; expected 8. Escape a literal pipe as \\| so the row is preserved.', + ); + assert.match(issue(malformed).body, /\| broken \|/, 'the malformed row is untouched'); +}); + +test('edit restores missing table separators without taking the first data row as one', (t) => { + const { issue, createPlan, edit } = planCliContext(t); + // A missing separator line is inserted; the first data row is never mistaken for it. + const unseparated = createPlan('missing separator'); + expectSuccess( + edit(unseparated, (text) => + text.replace('|---:|---|---|---|---|---|---|---|\n', '').replace('|---|---|---|\n', ''), + ), + 'edit without separators', + ); + const unseparatedBody = issue(unseparated).body; + assert.ok(unseparatedBody.includes('| 1 | fix_parser | Fix parser |'), 'first step row survives'); + assert.ok(unseparatedBody.includes('| A1 | node smoke.mjs | Exit 0 |'), 'first acceptance row survives'); + assert.equal(unseparatedBody, body(), 'separators are restored'); +}); diff --git a/scripts/tests/plan-cli/write-gates.test.mjs b/scripts/tests/plan-cli/write-gates.test.mjs new file mode 100644 index 00000000..8f0863cc --- /dev/null +++ b/scripts/tests/plan-cli/write-gates.test.mjs @@ -0,0 +1,103 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import test from 'node:test'; +import { body, expectSuccess, planCliContext, refuse } from './harness.mjs'; + +test('a mutating command refuses a plan owned by another agent, also when the owner is on a later page', (t) => { + const { run, issue, updateIssue, createPlan } = planCliContext(t); + // Ownership is checked before a mutating command can change the record. + const foreign = createPlan('foreign owner'); + updateIssue(foreign, (entry) => { + entry.assignees = ['other-agent']; + }); + refuse(run('status', String(foreign), 'ongoing'), `plan #${foreign} is owned by other-agent`); + assert.deepEqual(issue(foreign).labels, ['plan', 'plan:drafting']); + const pagedOwner = createPlan('owner beyond first page'); + updateIssue(pagedOwner, (entry) => { + entry.assignees = [...Array(100).fill('plan-agent'), 'other-agent']; + }); + refuse(run('status', String(pagedOwner), 'ongoing'), `plan #${pagedOwner} is owned by other-agent`); + assert.deepEqual(issue(pagedOwner).labels, ['plan', 'plan:drafting'], 'foreign owner on page two prevents writes'); +}); + +test('edit refuses a body that changed remotely after the provenance check passed', (t) => { + const { run, updateState, issue, createPlan, exported } = planCliContext(t); + // A second read catches a writer that races the edit after provenance passes. + const raced = createPlan('raced body'); + const raceFile = exported(raced); + fs.writeFileSync(raceFile, issue(raced).body.replace('Fix parser', 'Repair parser')); + const remoteBody = issue(raced).body.replace('Fix parser', 'Remote task'); + updateState((state) => { + state.remoteChange = { issue: raced, viewsBeforeChange: 1, body: remoteBody }; + }); + refuse(run('edit', String(raced), '--file', raceFile), 'plan issue changed remotely; re-read and retry'); + assert.equal(issue(raced).body, remoteBody); +}); + +test('partial or missing GraphQL data fails closed and writes nothing', (t) => { + const { run, updateState, issue, createPlan } = planCliContext(t); + // GraphQL may report HTTP-successful partial data; neither that nor missing data authorizes a write. + const graphqlFailure = createPlan('graphql failure'); + updateState((state) => { + state.graphqlError = [{ message: 'partial snapshot' }]; + }); + const partial = run('status', String(graphqlFailure), 'ongoing'); + assert.equal(partial.status, 1, 'GraphQL errors fail closed even with usable data'); + assert.match(partial.stderr, /graphql/i); + assert.deepEqual(issue(graphqlFailure).labels, ['plan', 'plan:drafting']); + updateState((state) => { + state.graphqlMissingData = true; + }); + const missingData = run('status', String(graphqlFailure), 'ongoing'); + assert.equal(missingData.status, 1, 'missing GraphQL data fails closed'); + assert.match(missingData.stderr, /graphql/i); + assert.equal(issue(graphqlFailure).body, body(), 'neither partial nor missing GraphQL data writes a body'); +}); + +test('edit refuses a file whose export provenance is missing, stale, or unreadable', (t) => { + const { scratch, run, issue, updateIssue, createPlan, exported } = planCliContext(t); + // An arbitrary file cannot bypass export provenance. + const provenance = createPlan('provenance'); + const missingFile = path.join(scratch, 'missing.md'); + fs.writeFileSync(missingFile, issue(provenance).body); + refuse( + run('edit', String(provenance), '--file', missingFile), + `missing export provenance: ${missingFile}.origin does not exist; run \`plan.mjs export ${provenance}\` and re-apply the edit`, + ); + const staleFile = exported(provenance); + updateIssue(provenance, (entry) => { + entry.body = entry.body.replace('Fix parser', 'Remote task'); + }); + refuse( + run('edit', String(provenance), '--file', staleFile), + `stale export: ${staleFile} was exported from a superseded body; run \`plan.mjs export ${provenance}\` and re-apply the edit`, + ); + assert.match(issue(provenance).body, /Remote task/); + const unreadableFile = exported(provenance); + fs.writeFileSync(`${unreadableFile}.origin`, 'not a digest\n'); + refuse( + run('edit', String(provenance), '--file', unreadableFile), + `unreadable export provenance: ${unreadableFile}.origin`, + ); +}); + +test('a failed remote edit keeps the export valid, so a retry with the same file succeeds (#78)', (t) => { + const { run, updateState, issue, createPlan, exported } = planCliContext(t); + // A failed remote edit keeps the export valid, so the same file can be sent again (#78). + const retried = createPlan('retried edit'); + const retryFile = exported(retried); + fs.writeFileSync(retryFile, fs.readFileSync(retryFile, 'utf8').replace('Not run.', 'Smoke passed.')); + const transientError = 'GraphQL: Something went wrong while executing your query'; + updateState((state) => { + state.issueEditError = transientError; + }); + const failedEdit = run('edit', String(retried), '--file', retryFile); + refuse(failedEdit, `gh issue edit failed: ${transientError}`); + assert.equal(failedEdit.stdout, '', 'a failed edit prints no result'); + updateState((state) => { + delete state.issueEditError; + }); + expectSuccess(run('edit', String(retried), '--file', retryFile), 'retry with the same export'); + assert.match(issue(retried).body, /Smoke passed\./, 'the retry writes the edited body'); +}); From 267482e53b5a280be23ee7ab8c61a994dca81c71 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:17:04 -0300 Subject: [PATCH 3/5] perf(plan-lifecycle): cut a gh round trip from every plan write and list Every plan.mjs write ran four gh processes: the snapshot, a pre-write `gh issue view`, `gh issue edit`, and a read-back `gh issue view`. `list` ran a repository lookup and then `gh issue list` (#89). - A write now sends one `gh api graphql` request. Its first mutation, updateIssue, carries the body and any claim, so the body and the claim succeed or fail together. Label repair follows as discrete add and remove mutations: a full labelIds list would drop a label added concurrently, which gh avoids for the same reason. The last mutation returns the stored state, which replaces the read-back. - The pre-write read stays: the contract requires it and a case tests the race. Retire still closes only after its label write checks out. - The body travels as `-F body=@`. gh fills `{owner}`, `{repo}`, and `{branch}` in any other -F value, so a quoted token would be stored rewritten; an @ file is sent byte for byte. - `list` reads the repository's plan issues in one paged query. - A status write on a repository that lacks a phase label now creates it, as `new` does, instead of failing inside `gh issue edit`. On GitHub: a status write takes about 2.8 s instead of 3.2 s, and `list` about 0.6 s instead of 1.3 s. Refusal strings and the contract are unchanged. The fake gh now mirrors gh's -F handling (an @ file is verbatim, other values are converted and placeholder-filled), so the plan-CLI suite catches a body sent the wrong way. Refs #89 --- .../skills/productivity/plan-manager/SKILL.md | 2 +- .../plan-manager/scripts/plan.mjs | 254 ++++++++----- scripts/tests/fixtures/gh-stub/gh | 334 +++++++++++++----- .../plan-cli/archive-retire-list.test.mjs | 2 +- scripts/tests/plan-cli/records.test.mjs | 21 ++ scripts/tests/plan-cli/status.test.mjs | 12 + scripts/tests/plan-cli/write-gates.test.mjs | 2 +- 7 files changed, 458 insertions(+), 169 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md index 7dd18ae1..86ec04cc 100644 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/SKILL.md @@ -4,7 +4,7 @@ description: "Use when a goal may need the six-phase plan flow: decide, draft, r user-invocable: true metadata: pattern: tool-wrapper - updated: "2026-10-21" + updated: "2026-10-23" content_hash: "37cfc75fba999189e56e877a1cb3ad02be4e26c5e3f289bd83e1d3e3f08a427b" --- diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index dd2c7e12..905e28ff 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -14,12 +14,18 @@ const STEPS_SEPARATOR = '|---:|---|---|---|---|---|---|---|'; const ACCEPTANCE_HEADER = '| ID | Command | Expected |'; const ACCEPTANCE_SEPARATOR = '|---|---|---|'; const PLAN_LABELS = ['plan', 'plan:drafting', 'plan:planned', 'plan:ongoing', 'plan:blocked']; -const ISSUE_FIELDS = 'number,title,body,state,stateReason,labels,assignees,url,createdAt,updatedAt'; const ACTING_LOGIN_ERROR = 'cannot resolve the acting GitHub login (the GraphQL viewer.login lookup returned nothing)'; const PAGE_INFO = 'pageInfo{hasNextPage endCursor}'; const PULL_REQUEST_FIELDS = 'number url state mergedAt baseRefName repository{nameWithOwner}'; const PULL_REQUEST_CONNECTION = `nodes{${PULL_REQUEST_FIELDS}} ${PAGE_INFO}`; -const REPOSITORY_LABELS = PLAN_LABELS.map((name) => `${name.split(':').at(-1)}:label(name:"${name}"){name}`).join(' '); +const REPOSITORY_LABELS = PLAN_LABELS.map((name) => `${name.split(':').at(-1)}:label(name:"${name}"){name id}`); +// The pre-write read returns the body to compare and the node ids that the write mutation needs. +const PRE_WRITE_FIELDS = 'id,body,updatedAt,labels,assignees'; +// The last field of a write mutation returns the stored state that the post-write checks read. +const STORED_ISSUE_FIELDS = `body labels(first:100){nodes{name} ${PAGE_INFO}} assignees(first:100){nodes{login}}`; +// `list` reads the newest plans first, as `gh issue list` did, so the 500-plan cap keeps the same plans. +const PLAN_LIST_ARGS = ',labels:["plan"],states:[OPEN,CLOSED],orderBy:{field:CREATED_AT,direction:DESC}'; +const PLAN_LIST_FIELDS = 'number title state stateReason labels(first:100){nodes{name}}'; const GH_MAX_BUFFER_MIB = 256; const PLAN_LIST_LIMIT = 500; // A machine path points into a user or root home, a mount, a named temporary or runtime directory, @@ -61,16 +67,20 @@ function runGh(argv) { } return result.stdout; } -function graphql(query, variables = {}) { - const argv = ['api', 'graphql', '-f', `query=${query}`, '-F', 'owner={owner}', '-F', 'name={repo}']; - for (const [key, value] of Object.entries(variables)) argv.push('-F', `${key}=${value}`); - const response = parseJson(runGh(argv), 'gh api graphql'); +// Run `gh api graphql` with the given fields and return the response data. +function graphqlData(fields) { + const response = parseJson(runGh(['api', 'graphql', ...fields]), 'gh api graphql'); if (!response || (response.errors !== undefined && (!Array.isArray(response.errors) || response.errors.length))) { fail(`gh api graphql returned errors: ${response?.errors?.[0]?.message ?? 'malformed errors'}`); } if (!response.data || typeof response.data !== 'object') fail('gh api graphql returned missing data'); return response.data; } +function graphql(query, variables = {}) { + const fields = ['-f', `query=${query}`, '-F', 'owner={owner}', '-F', 'name={repo}']; + for (const [key, value] of Object.entries(variables)) fields.push('-F', `${key}=${value}`); + return graphqlData(fields); +} function parseJson(output, command) { try { return JSON.parse(output); @@ -86,7 +96,7 @@ function issueConnectionSelection(field, nodeFields, extraArgs = '', after = fal return `${field}(first:100${after ? ',after:$after' : ''}${extraArgs}){nodes{${nodeFields}} ${PAGE_INFO}}`; } -function connectionNodes(initial, name, nextPage) { +function connectionNodes(initial, name, nextPage, limit = Infinity) { const nodes = []; const seen = new Set(); let connection = initial; @@ -102,7 +112,7 @@ function connectionNodes(initial, name, nextPage) { ) fail(`gh api graphql returned malformed ${name}`); nodes.push(...connection.nodes); - if (!connection.pageInfo.hasNextPage) return nodes; + if (!connection.pageInfo.hasNextPage || nodes.length >= limit) return nodes; const cursor = connection.pageInfo.endCursor; if (typeof cursor !== 'string' || !cursor || seen.has(cursor)) fail(`gh api graphql returned malformed ${name} cursor`); @@ -177,7 +187,7 @@ function readSnapshot( ? `latestClosure:timelineItems(last:1,itemTypes:[CLOSED_EVENT]){nodes{${CLOSED_EVENT_FIELDS}}}` : '', ].join(' '); - const query = `query($owner:String!,$name:String!${number === undefined ? '' : ',$number:Int!'}){${acting ? 'viewer{login}' : ''} repository(owner:$owner,name:$name){nameWithOwner visibility ${archive ? 'defaultBranchRef{name}' : ''} ${labelLookup ? REPOSITORY_LABELS : ''} ${number === undefined ? '' : `issue(number:$number){${issueFields}}`}}}`; + const query = `query($owner:String!,$name:String!${number === undefined ? '' : ',$number:Int!'}){${acting ? 'viewer{login id}' : ''} repository(owner:$owner,name:$name){nameWithOwner visibility ${archive ? 'defaultBranchRef{name}' : ''} ${labelLookup ? REPOSITORY_LABELS.join(' ') : ''} ${number === undefined ? '' : `issue(number:$number){${issueFields}}`}}}`; const data = graphql(query, number === undefined ? {} : { number }); const repo = data.repository; if ( @@ -189,19 +199,25 @@ function readSnapshot( fail('gh api graphql returned malformed repository'); repository = repo; const login = data.viewer?.login; + const viewerId = data.viewer?.id; if (acting && (typeof login !== 'string' || !login.trim())) fail(ACTING_LOGIN_ERROR); + if (acting && (typeof viewerId !== 'string' || !viewerId)) fail('gh api graphql returned malformed viewer'); + // Each plan label name maps to its node id, or to null when the repository lacks the label. + let labelIds; if (labelLookup) { - const missingLabels = []; + labelIds = new Map(); for (const name of PLAN_LABELS) { const label = repo[name.split(':').at(-1)]; - if (label === undefined) fail('gh api graphql returned malformed repository labels'); - if (label === null) missingLabels.push(name); - else if (typeof label.name !== 'string' || token(label.name) !== name) + if ( + label === undefined || + (label !== null && + (typeof label.name !== 'string' || token(label.name) !== name || typeof label.id !== 'string' || !label.id)) + ) fail('gh api graphql returned malformed repository labels'); + labelIds.set(name, label?.id ?? null); } - return { missingLabels }; } - if (number === undefined) return {}; + if (number === undefined) return { labelIds }; const issue = repo.issue; if ( !issue || @@ -262,16 +278,13 @@ function readSnapshot( } issue.closer = closer; } - return { issue, login }; + return { issue, login, viewerId, labelIds }; } function parseIssueNumber(value) { const match = /^#?([1-9]\d*)$/.exec(String(value ?? '')); if (!match) fail(`invalid plan issue: ${value ?? '(missing)'}`); return Number(match[1]); } -function issueView(number, fields = ISSUE_FIELDS, repo = repository.nameWithOwner) { - return parseJson(runGh(['issue', 'view', String(number), '--json', fields, '--repo', repo]), 'gh issue view'); -} function workStarted(issue, recordStatus) { if (recordStatus !== 'drafting' && recordStatus !== 'planned') return true; return issue.history.some((event) => token(event.label.name) === 'plan:ongoing'); @@ -297,44 +310,105 @@ function withBodyFile(body, callback) { fs.rmSync(temporary, { force: true }); } } -function editIssueBodyIfUnchanged(issue, after, labels = {}) { - const current = issueView(issue.number, 'body,updatedAt'); +// The pre-write read: the issue as GitHub stores it now, with the node ids that the write mutation needs. +function preWriteIssue(number) { + const current = parseJson( + runGh(['issue', 'view', String(number), '--json', PRE_WRITE_FIELDS, '--repo', repository.nameWithOwner]), + 'gh issue view', + ); + const hasId = (node) => typeof node?.id === 'string' && node.id !== ''; + if ( + !hasId(current) || + typeof current.body !== 'string' || + !Array.isArray(current.labels) || + !Array.isArray(current.assignees) || + !current.labels.every((label) => hasId(label) && typeof label.name === 'string') || + !current.assignees.every(hasId) + ) + fail('gh issue view returned malformed plan issue'); + return current; +} +// A status write can target a phase label that the repository lacks: create it as `new` does, then read its id. +function createdLabelId(name) { + runGh(['label', 'create', name, '--repo', repository.nameWithOwner]); + const query = `query($owner:String!,$name:String!){repository(owner:$owner,name:$name){label(name:${JSON.stringify(name)}){name id}}}`; + const label = graphql(query).repository?.label; + if (typeof label?.name !== 'string' || token(label.name) !== name || typeof label.id !== 'string' || !label.id) + fail('gh api graphql returned malformed repository labels'); + return label.id; +} +// A write is the pre-write read plus one GraphQL request (#89). GitHub runs its mutation fields in order: +// updateIssue first, so the body and the claim of an unassigned plan land in one write; then discrete label +// additions and removals, so a label that someone else adds meanwhile survives. The last field returns the +// stored state, which the post-write checks read. +function writeIssueIfUnchanged(issue, { body, add, remove = [], labelIds } = {}) { + const claims = issue.claimantId !== undefined; + if (body === undefined && !add && remove.length === 0 && !claims) return; + const current = preWriteIssue(issue.number); if (current.body !== issue.body) fail('plan issue changed remotely; re-read and retry'); - withBodyFile(after, (bodyFile) => { - const argv = ['issue', 'edit', String(issue.number), '--body-file', bodyFile]; - if (Object.hasOwn(issue, 'claimLogin')) { - if (!issue.claimLogin) fail(ACTING_LOGIN_ERROR); - argv.push('--add-assignee', '@me'); + const idLists = []; + const fields = []; + if (body !== undefined || claims) { + const input = ['id:$issue']; + if (body !== undefined) input.push('body:$body'); + if (claims) { + idLists.push(['assignees', [...new Set([...current.assignees.map((owner) => owner.id), issue.claimantId])]]); + input.push('assigneeIds:$assignees'); } - if (labels.add) argv.push('--add-label', labels.add); - for (const label of labels.remove ?? []) argv.push('--remove-label', label); - argv.push('--repo', repository.nameWithOwner); - runGh(argv); - }); - const stored = issueView(issue.number, 'body,updatedAt'); - if (stored.body !== after) fail('plan issue body differs after edit'); -} -function editIssueLabelsIfBodyUnchanged(issue, labels) { - const remove = labels.remove ?? []; - const claims = Object.hasOwn(issue, 'claimLogin'); - if (remove.length === 0 && !labels.add && !claims) return; - const current = issueView(issue.number, 'body,updatedAt'); - if (current.body !== issue.body) fail('plan issue changed remotely; re-read and retry'); - const argv = ['issue', 'edit', String(issue.number)]; - if (claims) { - if (!issue.claimLogin) fail(ACTING_LOGIN_ERROR); - argv.push('--add-assignee', '@me'); + fields.push({ mutation: 'updateIssue', input: input.join(','), payload: 'issue' }); } - if (labels.add) argv.push('--add-label', labels.add); - for (const label of remove) argv.push('--remove-label', label); - argv.push('--repo', repository.nameWithOwner); - runGh(argv); - if (remove.length > 0 || labels.add) { - const stored = labelNames(issueView(issue.number, 'labels').labels); - if ((labels.add && !stored.includes(labels.add)) || remove.some((label) => stored.includes(token(label)))) { - fail('plan issue labels differ after edit'); - } + if (add) { + idLists.push(['add', [labelIds.get(add) ?? createdLabelId(add)]]); + fields.push({ mutation: 'addLabelsToLabelable', input: 'labelableId:$issue,labelIds:$add', payload: 'labelable' }); + } + const removed = new Set(remove.map(token)); + const removeIds = current.labels.filter((label) => removed.has(token(label.name))).map((label) => label.id); + if (removeIds.length) { + idLists.push(['remove', removeIds]); + fields.push({ + mutation: 'removeLabelsFromLabelable', + input: 'labelableId:$issue,labelIds:$remove', + payload: 'labelable', + }); } + // Nothing left to change: the pre-write read already shows every label to remove as absent. + if (!fields.length) return; + const declarations = [ + '$issue:ID!', + ...(body === undefined ? [] : ['$body:String!']), + ...idLists.map(([name]) => `$${name}:[ID!]!`), + ]; + const selections = fields.map(({ mutation, input, payload }, index) => { + const stored = + payload === 'issue' ? `issue{${STORED_ISSUE_FIELDS}}` : `labelable{... on Issue{${STORED_ISSUE_FIELDS}}}`; + return `${mutation}(input:{${input}}){${index === fields.length - 1 ? stored : 'clientMutationId'}}`; + }); + const send = (bodyFile) => + graphqlData([ + '-f', + `query=mutation(${declarations.join(',')}){${selections.join(' ')}}`, + '-f', + `issue=${current.id}`, + // gh passes an @file field byte for byte; any other -F value gets type conversion and placeholder filling. + ...(bodyFile === undefined ? [] : ['-F', `body=@${bodyFile}`]), + ...idLists.flatMap(([name, ids]) => ids.flatMap((id) => ['-f', `${name}[]=${id}`])), + ]); + const data = body === undefined ? send() : withBodyFile(body, send); + const last = fields.at(-1); + const stored = data[last.mutation]?.[last.payload]; + if ( + typeof stored?.body !== 'string' || + !Array.isArray(stored.labels?.nodes) || + !Array.isArray(stored.assignees?.nodes) + ) + fail('gh api graphql returned malformed stored plan issue'); + if (body !== undefined && stored.body !== body) fail('plan issue body differs after edit'); + if (!add && remove.length === 0) return; + const storedLabels = issueNodes(issue.number, stored.labels, 'labels', 'name'); + if (storedLabels.some((label) => typeof label.name !== 'string')) fail('gh api graphql returned malformed labels'); + const labels = labelNames(storedLabels); + if ((add && !labels.includes(add)) || remove.some((label) => labels.includes(token(label)))) + fail('plan issue labels differ after edit'); } function parseOptions(args, allowed, repeatable = new Set()) { const options = {}; @@ -523,7 +597,13 @@ function replaceTable(sections, name, header, separator, rows) { // Steps that edit the same path need a Depends order, or parallel work collides (#78). function sharedFilesAdvice(rows) { const paths = rows.map( - (row) => new Set(row[3].split(',').map((part) => unquoteCode(part.trim())).filter((part) => part && part !== '-')), + (row) => + new Set( + row[3] + .split(',') + .map((part) => unquoteCode(part.trim())) + .filter((part) => part && part !== '-'), + ), ); const dependencies = new Map( rows.map((row) => [row[0], row[4] === '-' ? [] : row[4].split(',').map((part) => part.trim())]), @@ -628,7 +708,8 @@ function researchFilled(plan) { } function printAdvice(text, status, extra = [], options = {}) { const result = normalizePlan(text, options); - if (status !== 'drafting' && !researchFilled(parsePlan(text))) result.advice.push('Research is not filled after drafting.'); + if (status !== 'drafting' && !researchFilled(parsePlan(text))) + result.advice.push('Research is not filled after drafting.'); for (const message of [...result.advice, ...extra]) console.log(`advice: ${message}`); } // A plan-review pass covers the body that the reviewer read (#78). @@ -690,23 +771,28 @@ function reviewVerdicts(issue) { return verdicts; } function readPlanIssue(value, forWrite = false, connections = {}) { - const { issue, login } = readSnapshot(parseIssueNumber(value), { acting: forWrite, ...connections }); + const { issue, login, viewerId, labelIds } = readSnapshot(parseIssueNumber(value), { + acting: forWrite, + ...connections, + }); if (!/^\s*/i.test(issue.body)) fail(`unreadable plan contract: #${issue.number}`); if (forWrite) { const owners = issue.assignees.map((owner) => owner.login); const foreign = owners.find((owner) => owner !== login); if (foreign) fail(`plan #${issue.number} is owned by ${foreign}`); - if (!owners.length) issue.claimLogin = login; + // The write claims an unassigned plan for the acting viewer. + if (!owners.length) issue.claimantId = viewerId; } - return { issue, parsed: parsePlan(issue.body), status: statusFromIssue(issue) }; + return { issue, parsed: parsePlan(issue.body), status: statusFromIssue(issue), labelIds }; } function createPlan(args) { const options = parseOptions(args, new Set(['--title', '--goal', '--mode', '--label']), new Set(['--label'])); if (!options['--title']?.trim() || !options['--goal']?.trim()) fail('new requires --title and --goal'); const extras = options['--label'] ?? []; for (const label of extras) if (/^plan(?::|$)/i.test(label)) fail(`reserved label namespace: ${label}`); - const { missingLabels } = readSnapshot(undefined, { acting: true, labelLookup: true }); - for (const label of missingLabels) runGh(['label', 'create', label, '--repo', repository.nameWithOwner]); + const { labelIds } = readSnapshot(undefined, { acting: true, labelLookup: true }); + for (const [label, id] of labelIds) + if (id === null) runGh(['label', 'create', label, '--repo', repository.nameWithOwner]); const goalText = blankFencedRegions(options['--goal']) .split('\n') .map((masked, index) => { @@ -817,7 +903,7 @@ function editPlan(args) { }); } fs.writeFileSync(file, body); - editIssueBodyIfUnchanged(issue, body); + writeIssueIfUnchanged(issue, { body }); // Refresh provenance only after the remote write, so a failed write can be retried with the same file (#78). fs.writeFileSync(originFile(file), `${bodyDigest(body)}\n`, { mode: 0o600 }); fs.chmodSync(originFile(file), 0o600); @@ -840,7 +926,7 @@ function setPlanStatus(args) { if (!PLAN_STATUSES.has(target)) fail(`unknown plan status: ${target}`); if (target === 'blocked' && (!options['--reason']?.trim() || /[\r\n]/.test(options['--reason']))) fail('blocked status requires --reason as single-line text'); - const { issue, parsed, status } = readPlanIssue(value, true, { history: true }); + const { issue, parsed, status, labelIds } = readPlanIssue(value, true, { history: true, labelLookup: true }); if (String(issue.state).toUpperCase() === 'CLOSED') fail(`plan #${issue.number} is closed; status applies to open plans`); if (workStarted(issue, status) && !['ongoing', 'blocked'].includes(target)) @@ -853,9 +939,11 @@ function setPlanStatus(args) { `${target === 'blocked' ? `Blocked: ${options['--reason']}\n\n` : ''}${content}`.trim(), ); const source = render(parsed.sections); - editIssueBodyIfUnchanged(issue, normalizePlan(source).body, { + writeIssueIfUnchanged(issue, { + body: normalizePlan(source).body, add: `plan:${target}`, remove: labelsToRemove(issue, target), + labelIds, }); console.log(`plan #${issue.number} status: ${status} -> ${target}`); printAdvice(issue.body, target); @@ -891,30 +979,29 @@ function setStepStatus(args) { parsed.steps.map((step) => Object.values(step)), ); const source = render(parsed.sections); - editIssueBodyIfUnchanged(issue, normalizePlan(source).body); + writeIssueIfUnchanged(issue, { body: normalizePlan(source).body }); console.log(`plan #${issue.number} step ${row.id}: ${current} -> ${target}`); printAdvice(source, status, advice); } function listPlans(args) { const selected = parseOptions(args, new Set(['--status']))['--status']; - const issues = parseJson( - runGh([ - 'issue', - 'list', - '--label', - 'plan', - '--state', - 'all', - '--limit', - String(PLAN_LIST_LIMIT), - '--json', - ISSUE_FIELDS.replace('body,', ''), - '--repo', - repository.nameWithOwner, - ]), - 'gh issue list', - ); - if (!Array.isArray(issues)) fail('gh issue list returned malformed issues'); + // One GraphQL query per 100 plans; like every snapshot, it names the repository through gh's {owner}/{repo}. + const query = `query($owner:String!,$name:String!,$after:String){repository(owner:$owner,name:$name){${issueConnectionSelection('issues', PLAN_LIST_FIELDS, PLAN_LIST_ARGS, true)}}}`; + const page = (variables) => graphql(query, variables).repository?.issues; + const nodes = connectionNodes(page(), 'plan issues', (after) => page({ after }), PLAN_LIST_LIMIT); + if ( + nodes.some( + (issue) => + !Number.isInteger(issue.number) || + typeof issue.title !== 'string' || + typeof issue.state !== 'string' || + (issue.stateReason !== null && typeof issue.stateReason !== 'string') || + !Array.isArray(issue.labels?.nodes) || + issue.labels.nodes.some((label) => typeof label?.name !== 'string'), + ) + ) + fail('gh api graphql returned malformed plan issues'); + const issues = nodes.slice(0, PLAN_LIST_LIMIT).map((issue) => ({ ...issue, labels: issue.labels.nodes })); issues.sort( (a, b) => Number(a.state.toUpperCase() === 'CLOSED') - Number(b.state.toUpperCase() === 'CLOSED') || a.number - b.number, @@ -959,7 +1046,7 @@ function archivePlan(args, retired = false) { if (!closer) fail(`archive requires a closing pull request merged into ${repository.nameWithOwner}:${defaultBranch}`); } - editIssueLabelsIfBodyUnchanged(issue, { remove: labelsToRemove(issue) }); + writeIssueIfUnchanged(issue, { remove: labelsToRemove(issue) }); if (retired) runGh([ 'issue', @@ -993,7 +1080,6 @@ function main([command, ...args]) { if (!Object.hasOwn(commands, command)) fail('usage: plan.mjs ...'); repository = undefined; - if (command === 'list') readSnapshot(); commands[command](args); } if (process.argv[1] && import.meta.url === pathToFileURL(path.resolve(process.argv[1])).href) { diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index 786fa3ea..f0722e4a 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -58,12 +58,50 @@ function issueByNumber(number) { return issue; } +// Node ids are opaque to the helper, as on GitHub. An issue id names the issue number, a label id the +// lowercase label name (repository labels are unique by case-insensitive name), and a user id the login. +function issueNodeId(issue) { + return `I_${issue.number}`; +} + +function labelNodeId(name) { + return `LA_${Buffer.from(name.toLowerCase(), 'utf8').toString('base64url')}`; +} + +function userNodeId(login) { + return `U_${login}`; +} + +function issueByNodeId(id) { + const issue = state.issues.find((entry) => issueNodeId(entry) === id); + if (!issue) die(`Could not resolve to a node with the global id of '${id}'`); + return issue; +} + +function labelNameByNodeId(id) { + const name = /^LA_[\w-]+$/.test(id) ? Buffer.from(id.slice(3), 'base64url').toString('utf8') : ''; + if (!name) die(`Could not resolve to a Label node with the global id of '${id}'`); + return name; +} + +function loginByNodeId(id) { + if (!/^U_./.test(id)) die(`Could not resolve to a User node with the global id of '${id}'`); + return id.slice(2); +} + +// The helper never sends an empty id list (an empty assigneeIds would unassign everyone), so refuse one. +function nodeIds(value, field) { + if (!Array.isArray(value) || value.length === 0 || value.some((id) => typeof id !== 'string' || !id)) + die(`${field} requires a non-empty list of ids`); + return value; +} + function labels(labels) { - return labels.map((name) => ({ name })); + return labels.map((name) => ({ id: labelNodeId(name), name })); } function assignees(logins) { - return (logins ?? []).map((login) => ({ login })); + return (logins ?? []).map((login) => ({ id: userNodeId(login), login })); } function select(object, fields) { @@ -79,12 +117,48 @@ function validateJsonFields(fields, allowed) { } } -function formFields(flag) { +// `gh api graphql` fields as cli/cli pkg/cmd/api/fields.go reads them: gh reads every -f field before any -F +// field. A -f value stays raw. An `@` -F value is the file's bytes. Any other -F value becomes an +// integer, true, false, or null, or else gets its {owner}, {repo}, {branch}, :owner, :repo, and :branch +// placeholders filled from the repository. `key[]=value` fields build an array; a bare `key[]` is empty. +function graphqlFields() { + const [owner, repo] = state.repo.nameWithOwner.split('/'); + const placeholders = { owner, repo, branch: state.repo.defaultBranchRef?.name ?? 'main' }; + const magicValue = (raw) => { + if (raw.startsWith('@')) { + try { + return fs.readFileSync(raw.slice(1), 'utf8'); + } catch (error) { + die(`open ${raw.slice(1)}: ${error.code}`); + } + } + if (/^[+-]?\d+$/.test(raw)) return Number(raw); + if (raw === 'true' || raw === 'false') return raw === 'true'; + if (raw === 'null') return null; + return raw.replace( + /:(owner|repo|branch)\b|\{([a-z]+)\}/g, + (match, colon, braced) => placeholders[colon ?? braced] ?? match, + ); + }; const fields = {}; - for (const entry of values(flag)) { - const separator = entry.indexOf('='); - if (separator === -1) die(`${flag} requires name=value`); - fields[entry.slice(0, separator)] = entry.slice(separator + 1); + for (const [flag, magic] of [ + ['-f', false], + ['-F', true], + ]) { + for (const entry of values(flag)) { + const separator = entry.indexOf('='); + const key = separator < 0 ? entry : entry.slice(0, separator); + const name = key.endsWith('[]') ? key.slice(0, -2) : key; + if (!name || /[[\]]/.test(name)) die(`invalid key: "${entry}"`); + if (separator < 0 && name === key) die(`field "${entry}" requires a value separator '=' character`); + const raw = separator < 0 ? undefined : entry.slice(separator + 1); + const value = magic && raw !== undefined ? magicValue(raw) : raw; + if (name === key) fields[name] = value; + else { + fields[name] = Array.isArray(fields[name]) ? fields[name] : []; + if (separator >= 0) fields[name].push(value); + } + } } return fields; } @@ -111,21 +185,57 @@ function graphqlValue(expression, variables) { if (expression.startsWith('$')) return variables[expression.slice(1)]; if (expression === 'true') return true; if (expression === 'false') return false; + if (expression === 'null') return null; if (/^\d+$/.test(expression)) return Number(expression); if (expression.startsWith('"')) return JSON.parse(expression); + if (expression.startsWith('[')) { + return splitArguments(expression.slice(1, -1)).map((item) => graphqlValue(item, variables)); + } + if (expression.startsWith('{')) { + const entries = [...graphqlArguments(expression.slice(1, -1))]; + return Object.fromEntries(entries.map(([key, item]) => [key, graphqlValue(item, variables)])); + } return expression; } + +// Split arguments, list items, or object fields on the commas outside nested lists, objects, and strings. +function splitArguments(text) { + const parts = []; + let depth = 0; + let quoted = false; + let start = 0; + for (let index = 0; index < text.length; index += 1) { + const char = text[index]; + if (quoted) { + if (char === '\\') index += 1; + else if (char === '"') quoted = false; + } else if (char === '"') quoted = true; + else if (char === '[' || char === '{') depth += 1; + else if (char === ']' || char === '}') depth -= 1; + else if (char === ',' && depth === 0) { + parts.push(text.slice(start, index)); + start = index + 1; + } + } + parts.push(text.slice(start)); + return parts.map((part) => part.trim()).filter(Boolean); +} + +function graphqlArguments(text) { + const args = new Map(); + for (const argument of splitArguments(text)) { + const separator = argument.indexOf(':'); + if (separator < 0) die(`invalid GraphQL argument: ${argument}`); + args.set(argument.slice(0, separator).trim(), argument.slice(separator + 1).trim()); + } + return args; +} + function graphqlSelections(query, field) { const selections = []; const pattern = new RegExp(`(?:(\\w+)\\s*:\\s*)?${field}\\s*\\(([^)]*)\\)`, 'g'); for (const match of query.matchAll(pattern)) { - const args = new Map(); - for (const argument of match[2].split(',').map((part) => part.trim()).filter(Boolean)) { - const separator = argument.indexOf(':'); - if (separator < 0) die(`invalid GraphQL argument: ${argument}`); - args.set(argument.slice(0, separator).trim(), argument.slice(separator + 1).trim()); - } - selections.push({ alias: match[1] ?? field, args }); + selections.push({ alias: match[1] ?? field, args: graphqlArguments(match[2]) }); } return selections; } @@ -175,6 +285,80 @@ function respondGraphql(data, stage) { process.exit(0); } +// A mutation payload shows the issue as stored right after its own field ran, limited to what the field selects. +function mutationPayload(field, object, issue, variables) { + const payload = { clientMutationId: null }; + const opened = new RegExp(`\\b${object}\\s*\\{`).exec(field.selection); + if (!opened) return payload; + const selection = field.selection.slice(opened.index); + const stored = /\bbody\b/.test(selection) ? { body: issue.body } : {}; + for (const page of graphqlSelections(selection, 'labels')) { + stored[page.alias] = graphqlPage(labels(issue.labels), page, variables, ['first', 'after']); + } + for (const page of graphqlSelections(selection, 'assignees')) { + stored[page.alias] = graphqlPage(assignees(issue.assignees), page, variables, ['first', 'after']); + } + payload[object] = stored; + return payload; +} + +// GitHub runs the top-level fields of a mutation serially, in document order. updateIssue replaces the body +// and the assignee list that its input names; the label mutations add or remove labels by node id. +function respondMutation(query, variables) { + // The fault flags that faulted `gh issue edit` fault the write request before any field runs. + if (state.issueEditError) die(state.issueEditError); + const pattern = /(?:(\w+)\s*:\s*)?\b(updateIssue|addLabelsToLabelable|removeLabelsFromLabelable)\s*\(([^)]*)\)/g; + const matches = [...query.matchAll(pattern)]; + if (matches.length === 0) die(`unsupported GraphQL mutation: ${query}`); + const fields = matches.map((match, index) => ({ + alias: match[1] ?? match[2], + name: match[2], + input: graphqlValue(graphqlArguments(match[3]).get('input') ?? 'null', variables), + selection: query.slice(match.index + match[0].length, matches[index + 1]?.index ?? query.length), + })); + if (state.labelRemovalErrorOnce && fields.some((field) => field.name === 'removeLabelsFromLabelable')) { + const message = state.labelRemovalErrorOnce; + delete state.labelRemovalErrorOnce; + die(message); + } + const same = (a, b) => a.toLowerCase() === b.toLowerCase(); + const data = {}; + for (const field of fields) { + const { input } = field; + if (!input || typeof input !== 'object' || Array.isArray(input)) die(`${field.name} requires an input object`); + if (field.name === 'updateIssue') { + const issue = issueByNodeId(input.id); + for (const key of Object.keys(input)) { + if (!['id', 'body', 'assigneeIds'].includes(key)) die(`unsupported UpdateIssueInput field: ${key}`); + } + if (Object.hasOwn(input, 'body')) { + if (typeof input.body !== 'string') die('UpdateIssueInput.body must be a string'); + issue.body = input.body; + } + if (Object.hasOwn(input, 'assigneeIds')) { + issue.assignees = [...new Set(nodeIds(input.assigneeIds, 'assigneeIds').map(loginByNodeId))]; + } + touch(issue); + data[field.alias] = mutationPayload(field, 'issue', issue, variables); + continue; + } + const issue = issueByNodeId(input.labelableId); + const names = nodeIds(input.labelIds, 'labelIds').map(labelNameByNodeId); + if (field.name === 'addLabelsToLabelable') { + for (const name of names) { + const stored = Object.keys(state.labels).find((label) => same(label, name)); + if (!stored) die(`Could not resolve to a Label node with the name '${name}'`); + if (!issue.labels.some((label) => same(label, stored))) issue.labels.push(stored); + } + } else { + issue.labels = issue.labels.filter((label) => !names.some((name) => same(name, label))); + } + touch(issue); + data[field.alias] = mutationPayload(field, 'labelable', issue, variables); + } + respondGraphql(data, 'mutation'); +} + function commentJson(comment, issue) { const login = typeof comment.author === 'string' ? comment.author : comment.author?.login ?? comment.user?.login; return { @@ -202,6 +386,7 @@ function apiCommentJson(comment, issue) { function issueJson(issue) { return { ...issue, + id: issueNodeId(issue), labels: labels(issue.labels), assignees: assignees(issue.assignees), comments: (issue.comments ?? []).map((comment) => commentJson(comment, issue)), @@ -215,6 +400,7 @@ function touch(issue) { const [group, command] = args; const ISSUE_JSON_FIELDS = new Set([ + 'id', 'number', 'title', 'body', @@ -271,17 +457,15 @@ if (group === 'api' && eventsEndpoint) { } if (group === 'api' && command === 'graphql') { - const query = formFields('-f').query; - if (!query) die('graphql requires -f query='); - const variables = formFields('-F'); - const [owner, name] = state.repo.nameWithOwner.split('/'); - if (variables.owner === '{owner}') variables.owner = owner; - if (variables.name === '{repo}') variables.name = name; + const { query, ...variables } = graphqlFields(); + if (typeof query !== 'string' || !query) die('graphql requires -f query='); + // A mutation addresses nodes by id, so it names no repository. + if (/^\s*mutation\b/.test(query)) respondMutation(query, variables); if (`${variables.owner}/${variables.name}` !== state.repo.nameWithOwner) die(`repository not found: ${variables.owner}/${variables.name}`); if (state.repoError) die(state.repoError); if (query.includes('viewer') && state.viewerError) die(state.viewerError); - const stage = query.includes('viewer') ? 'snapshot' : 'page'; + const stage = query.includes('viewer') ? 'snapshot' : /\bissues\s*\(/.test(query) ? 'list' : 'page'; const referenceRepo = (reference) => (typeof reference.repository === 'string' ? reference.repository : reference.repository?.nameWithOwner) ?? (reference.repository?.owner?.login && reference.repository?.name @@ -324,9 +508,11 @@ if (group === 'api' && command === 'graphql') { } const issueSelection = graphqlSelections(query, 'issue'); + const issuesSelection = graphqlSelections(query, 'issues'); const objectSelection = graphqlSelections(query, 'object'); const labelSelections = graphqlSelections(query, 'label'); - if (issueSelection.length > 1 || objectSelection.length > 1) die('graphql selects multiple issues or objects'); + if (issueSelection.length > 1 || issuesSelection.length > 1 || objectSelection.length > 1) + die('graphql selects multiple issues or objects'); if (issueSelection.length) { const requestedNumber = graphqlValue(issueSelection[0].args.get('number'), variables); const issue = issueByNumber(Number(requestedNumber)); @@ -397,22 +583,52 @@ if (group === 'api' && command === 'graphql') { repository[objectSelection[0].alias] = { associatedPullRequests: commitAssociations(oid, association[0]), }; - } else if (labelSelections.length) { - for (const selection of labelSelections) { - for (const argument of selection.args.keys()) { - if (argument !== 'name') die(`Unknown GraphQL argument: "${argument}"`); - } - const name = graphqlValue(selection.args.get('name'), variables); - const stored = Object.keys(state.labels).find((label) => label.toLowerCase() === name?.toLowerCase()); - repository[selection.alias] = stored ? { name: stored } : null; - } - } else if (graphqlSelections(query, 'repository').length !== 1) { + } else if (issuesSelection.length) { + // Repository.issues: `labels` keeps issues that carry any named label; `states` and `orderBy` as on GitHub. + const [selection] = issuesSelection; + const argument = (name, fallback) => graphqlValue(selection.args.get(name) ?? fallback, variables); + const wanted = argument('labels', '[]').map((name) => name.toLowerCase()); + const states = argument('states', '[OPEN,CLOSED]'); + const order = argument('orderBy', '{field:CREATED_AT,direction:ASC}'); + if (order.field !== 'CREATED_AT') die(`unsupported issue order: ${order.field}`); + const direction = order.direction === 'DESC' ? -1 : 1; + const labelPages = graphqlSelections(query, 'labels'); + const matched = state.issues + .filter((issue) => wanted.length === 0 || issue.labels.some((label) => wanted.includes(label.toLowerCase()))) + .filter((issue) => states.includes(issue.state.toUpperCase())) + .sort((a, b) => direction * (Date.parse(a.createdAt) - Date.parse(b.createdAt) || a.number - b.number)) + .map((issue) => { + const node = { + number: issue.number, + title: issue.title, + state: issue.state, + stateReason: issue.stateReason, + }; + for (const page of labelPages) { + node[page.alias] = graphqlPage(labels(issue.labels), page, variables, ['first', 'after']); + } + return node; + }); + repository[selection.alias] = graphqlPage(matched, selection, variables, [ + 'first', + 'after', + 'labels', + 'states', + 'orderBy', + ]); + } else if (labelSelections.length === 0 && graphqlSelections(query, 'repository').length !== 1) { die('graphql query selects no repository'); } - respondGraphql( - { ...(query.includes('viewer') && { viewer: { login: state.viewerLogin ?? '' } }), repository }, - stage, - ); + for (const selection of labelSelections) { + for (const argument of selection.args.keys()) { + if (argument !== 'name') die(`Unknown GraphQL argument: "${argument}"`); + } + const name = graphqlValue(selection.args.get('name'), variables); + const stored = Object.keys(state.labels).find((label) => label.toLowerCase() === name?.toLowerCase()); + repository[selection.alias] = stored ? { name: stored, id: labelNodeId(stored) } : null; + } + const viewer = { login: state.viewerLogin ?? '', id: state.viewerLogin ? userNodeId(state.viewerLogin) : null }; + respondGraphql({ ...(query.includes('viewer') && { viewer }), repository }, stage); } if (group === 'api' && command === 'user') { @@ -498,52 +714,6 @@ if (group === 'issue' && command === 'view') { process.exit(0); } -if (group === 'issue' && command === 'edit') { - const issue = issueByNumber(issueNumber()); - if (state.issueEditError) die(state.issueEditError); - const bodyFile = value('--body-file'); - const addedAssignees = values('--add-assignee'); - const addedLabels = values('--add-label'); - const removedLabels = values('--remove-label'); - if (removedLabels.length > 0 && state.labelRemovalErrorOnce) { - const message = state.labelRemovalErrorOnce; - delete state.labelRemovalErrorOnce; - die(message); - } - if (!bodyFile && addedAssignees.length === 0 && addedLabels.length === 0 && removedLabels.length === 0) - die('issue edit requires a body, label, or assignee change'); - if (bodyFile) issue.body = fs.readFileSync(bodyFile, 'utf8'); - const same = (a, b) => a.toLowerCase() === b.toLowerCase(); - for (const name of removedLabels) issue.labels = issue.labels.filter((label) => !same(label, name)); - for (const name of addedLabels) if (!issue.labels.some((label) => same(label, name))) issue.labels.push(name); - issue.assignees ??= []; - if (!state.dropEditAssignee) { - for (const login of addedAssignees.map((name) => name === '@me' ? state.viewerLogin : name)) { - if (!issue.assignees.includes(login)) issue.assignees.push(login); - } - } - touch(issue); - save(); - process.exit(0); -} - -if (group === 'issue' && command === 'list') { - if (!value('--state') || !value('--limit')) die('issue list requires explicit --state and --limit'); - const requiredLabel = value('--label'); - const requestedState = value('--state'); - const limit = Number(value('--limit')); - const fields = value('--json'); - validateJsonFields(fields, ISSUE_JSON_FIELDS); - const issues = state.issues - .filter((issue) => !requiredLabel || issue.labels.includes(requiredLabel)) - .filter((issue) => requestedState === 'all' || issue.state.toLowerCase() === requestedState) - .slice(0, limit) - .map((issue) => select(issueJson(issue), fields)); - emit(JSON.stringify(issues)); - save(); - process.exit(0); -} - if (group === 'issue' && command === 'close') { const issue = issueByNumber(issueNumber()); const reason = value('--reason'); diff --git a/scripts/tests/plan-cli/archive-retire-list.test.mjs b/scripts/tests/plan-cli/archive-retire-list.test.mjs index 8fdd27e3..68d8493e 100644 --- a/scripts/tests/plan-cli/archive-retire-list.test.mjs +++ b/scripts/tests/plan-cli/archive-retire-list.test.mjs @@ -102,7 +102,7 @@ test('retire repairs labels before it closes the issue, and list filters open an }); refuse( run('retire', String(retiring), '--reason', 'No longer needed'), - 'gh issue edit failed: label removal unavailable', + 'gh api graphql failed: label removal unavailable', ); assert.equal(issue(retiring).state, 'OPEN', 'a failed label repair cannot close the issue'); assert.deepEqual(issue(retiring).labels, ['plan', 'plan:drafting']); diff --git a/scripts/tests/plan-cli/records.test.mjs b/scripts/tests/plan-cli/records.test.mjs index 92a4ea6e..efca62c4 100644 --- a/scripts/tests/plan-cli/records.test.mjs +++ b/scripts/tests/plan-cli/records.test.mjs @@ -59,6 +59,27 @@ test('edit writes the normalized body to the remote issue and to the export prov ); }); +test('edit stores a body that quotes gh placeholder tokens unchanged, in one mutation request (#89)', (t) => { + const { run, loadState, issue, createPlan, exported } = planCliContext(t); + // The body travels as a gh `@file` field; gh would type-convert and placeholder-fill a plain -F value. + const quoting = createPlan('placeholder tokens'); + const quoted = 'Run `gh api repos/{owner}/{repo}/branches/{branch}` or `gh api repos/:owner/:repo`.'; + const file = exported(quoting); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8').replace('Not run.', quoted)); + const callsBefore = loadState().calls.length; + expectSuccess(run('edit', String(quoting), '--file', file), 'edit a body that quotes placeholder tokens'); + const editCalls = loadState().calls.slice(callsBefore); + assert.deepEqual( + editCalls.map((call) => call.slice(0, 2).join(' ')), + ['api graphql', 'issue view', 'api graphql'], + 'an edit sends the snapshot, the pre-write read, and one mutation request', + ); + assert.equal(issue(quoting).body, body().replace('Not run.', quoted), 'the stored body keeps every token'); + const shown = run('show', String(quoting), '--body'); + expectSuccess(shown, 'show the body that quotes placeholder tokens'); + assert.equal(shown.stdout, issue(quoting).body, 'show reads the stored body back unchanged'); +}); + test('machine path advice names each distinct machine path and skips routes, portable paths, and slash text', () => { // Machine path advice names each distinct path into a home, a mount, a named temporary or runtime directory, // a Windows drive, or a network share. URL routes, portable system paths, and slash text get none (#71). diff --git a/scripts/tests/plan-cli/status.test.mjs b/scripts/tests/plan-cli/status.test.mjs index 20549bbf..12b3545c 100644 --- a/scripts/tests/plan-cli/status.test.mjs +++ b/scripts/tests/plan-cli/status.test.mjs @@ -84,6 +84,18 @@ test('a phase label beyond the first label page is read and replaced, and other ); }); +test('status creates a phase label that the repository lacks and applies it', (t) => { + const { run, loadState, updateState, issue, createPlan } = planCliContext(t); + // The target phase label can be missing, for example after someone deleted it from the repository. + const unlabelled = createPlan('missing phase label'); + updateState((state) => { + delete state.labels['plan:planned']; + }); + expectSuccess(run('status', String(unlabelled), 'planned'), 'status with a missing phase label'); + assert.ok(Object.hasOwn(loadState().labels, 'plan:planned'), 'the status write creates the missing label'); + assert.deepEqual(issue(unlabelled).labels, ['plan', 'plan:planned']); +}); + test('step refuses to reopen a terminal row', (t) => { const { run, issue, createPlan } = planCliContext(t); // A terminal row cannot be reopened by the step command. diff --git a/scripts/tests/plan-cli/write-gates.test.mjs b/scripts/tests/plan-cli/write-gates.test.mjs index 8f0863cc..0f62e0b6 100644 --- a/scripts/tests/plan-cli/write-gates.test.mjs +++ b/scripts/tests/plan-cli/write-gates.test.mjs @@ -93,7 +93,7 @@ test('a failed remote edit keeps the export valid, so a retry with the same file state.issueEditError = transientError; }); const failedEdit = run('edit', String(retried), '--file', retryFile); - refuse(failedEdit, `gh issue edit failed: ${transientError}`); + refuse(failedEdit, `gh api graphql failed: ${transientError}`); assert.equal(failedEdit.stdout, '', 'a failed edit prints no result'); updateState((state) => { delete state.issueEditError; From 21e0faecd9c299f159eef1338cc4f1ffbe6b18a9 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:25:15 -0300 Subject: [PATCH 4/5] fix(plan-lifecycle): close the three self-review findings on #89 - The write mutation no longer selects the stored assignees. No check reads them, so the field only added a way for a write to fail. - REPOSITORY_LABELS stays a joined string, as before. - sharedFilesAdvice and printAdvice get their original formatting back; the change does not touch them, and the gate does not format this shipped file. Refs #89 --- .../plan-manager/scripts/plan.mjs | 23 +++++-------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index 905e28ff..f60f0db6 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -18,11 +18,11 @@ const ACTING_LOGIN_ERROR = 'cannot resolve the acting GitHub login (the GraphQL const PAGE_INFO = 'pageInfo{hasNextPage endCursor}'; const PULL_REQUEST_FIELDS = 'number url state mergedAt baseRefName repository{nameWithOwner}'; const PULL_REQUEST_CONNECTION = `nodes{${PULL_REQUEST_FIELDS}} ${PAGE_INFO}`; -const REPOSITORY_LABELS = PLAN_LABELS.map((name) => `${name.split(':').at(-1)}:label(name:"${name}"){name id}`); +const REPOSITORY_LABELS = PLAN_LABELS.map((name) => `${name.split(':').at(-1)}:label(name:"${name}"){name id}`).join(' '); // The pre-write read returns the body to compare and the node ids that the write mutation needs. const PRE_WRITE_FIELDS = 'id,body,updatedAt,labels,assignees'; // The last field of a write mutation returns the stored state that the post-write checks read. -const STORED_ISSUE_FIELDS = `body labels(first:100){nodes{name} ${PAGE_INFO}} assignees(first:100){nodes{login}}`; +const STORED_ISSUE_FIELDS = `body labels(first:100){nodes{name} ${PAGE_INFO}}`; // `list` reads the newest plans first, as `gh issue list` did, so the 500-plan cap keeps the same plans. const PLAN_LIST_ARGS = ',labels:["plan"],states:[OPEN,CLOSED],orderBy:{field:CREATED_AT,direction:DESC}'; const PLAN_LIST_FIELDS = 'number title state stateReason labels(first:100){nodes{name}}'; @@ -187,7 +187,7 @@ function readSnapshot( ? `latestClosure:timelineItems(last:1,itemTypes:[CLOSED_EVENT]){nodes{${CLOSED_EVENT_FIELDS}}}` : '', ].join(' '); - const query = `query($owner:String!,$name:String!${number === undefined ? '' : ',$number:Int!'}){${acting ? 'viewer{login id}' : ''} repository(owner:$owner,name:$name){nameWithOwner visibility ${archive ? 'defaultBranchRef{name}' : ''} ${labelLookup ? REPOSITORY_LABELS.join(' ') : ''} ${number === undefined ? '' : `issue(number:$number){${issueFields}}`}}}`; + const query = `query($owner:String!,$name:String!${number === undefined ? '' : ',$number:Int!'}){${acting ? 'viewer{login id}' : ''} repository(owner:$owner,name:$name){nameWithOwner visibility ${archive ? 'defaultBranchRef{name}' : ''} ${labelLookup ? REPOSITORY_LABELS : ''} ${number === undefined ? '' : `issue(number:$number){${issueFields}}`}}}`; const data = graphql(query, number === undefined ? {} : { number }); const repo = data.repository; if ( @@ -396,11 +396,7 @@ function writeIssueIfUnchanged(issue, { body, add, remove = [], labelIds } = {}) const data = body === undefined ? send() : withBodyFile(body, send); const last = fields.at(-1); const stored = data[last.mutation]?.[last.payload]; - if ( - typeof stored?.body !== 'string' || - !Array.isArray(stored.labels?.nodes) || - !Array.isArray(stored.assignees?.nodes) - ) + if (typeof stored?.body !== 'string' || !Array.isArray(stored.labels?.nodes)) fail('gh api graphql returned malformed stored plan issue'); if (body !== undefined && stored.body !== body) fail('plan issue body differs after edit'); if (!add && remove.length === 0) return; @@ -597,13 +593,7 @@ function replaceTable(sections, name, header, separator, rows) { // Steps that edit the same path need a Depends order, or parallel work collides (#78). function sharedFilesAdvice(rows) { const paths = rows.map( - (row) => - new Set( - row[3] - .split(',') - .map((part) => unquoteCode(part.trim())) - .filter((part) => part && part !== '-'), - ), + (row) => new Set(row[3].split(',').map((part) => unquoteCode(part.trim())).filter((part) => part && part !== '-')), ); const dependencies = new Map( rows.map((row) => [row[0], row[4] === '-' ? [] : row[4].split(',').map((part) => part.trim())]), @@ -708,8 +698,7 @@ function researchFilled(plan) { } function printAdvice(text, status, extra = [], options = {}) { const result = normalizePlan(text, options); - if (status !== 'drafting' && !researchFilled(parsePlan(text))) - result.advice.push('Research is not filled after drafting.'); + if (status !== 'drafting' && !researchFilled(parsePlan(text))) result.advice.push('Research is not filled after drafting.'); for (const message of [...result.advice, ...extra]) console.log(`advice: ${message}`); } // A plan-review pass covers the body that the reviewer read (#78). From 1b2fe5759d753f977222d192ab12a10346add442 Mon Sep 17 00:00:00 2001 From: Eduardo Marquez <55303379+DocksDocks@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:40:11 -0300 Subject: [PATCH 5/5] fix(plan-lifecycle): take label removal ids from the paged snapshot Code review round 1 on #89: the write path took the label names to remove from the snapshot, which pages every label, but took their ids from `gh issue view`, which reads only labels(first:100). A phase label past the first 100 got no removal id: `status` wrote the body, kept both phase labels, and refused afterwards; `archive` and `retire` on an owned plan returned early without the label check, so archive reported success and retire closed the issue with the label attached. The released helper removed by name through `gh issue edit`, so this was a regression. - The snapshot now reads label ids with the names, and the removal ids come from that complete list. The pre-write read drops labels. - The early return for "nothing left to change" is gone: names and ids now come from one list, so it could not be reached, and it was the path that skipped the label check. - The fake gh truncates `issue view` labels at 100, as real gh does. With that, the paged-label status case and a new archive case fail on the previous commit and pass now. Refs #89 --- .../productivity/plan-manager/scripts/plan.mjs | 16 +++++++--------- scripts/tests/fixtures/gh-stub/gh | 5 ++++- scripts/tests/plan-cli/archive-closer.test.mjs | 15 +++++++++++++++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs index f60f0db6..c171bb75 100755 --- a/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs +++ b/plugins/plan-lifecycle/skills/productivity/plan-manager/scripts/plan.mjs @@ -20,7 +20,7 @@ const PULL_REQUEST_FIELDS = 'number url state mergedAt baseRefName repository{na const PULL_REQUEST_CONNECTION = `nodes{${PULL_REQUEST_FIELDS}} ${PAGE_INFO}`; const REPOSITORY_LABELS = PLAN_LABELS.map((name) => `${name.split(':').at(-1)}:label(name:"${name}"){name id}`).join(' '); // The pre-write read returns the body to compare and the node ids that the write mutation needs. -const PRE_WRITE_FIELDS = 'id,body,updatedAt,labels,assignees'; +const PRE_WRITE_FIELDS = 'id,body,updatedAt,assignees'; // The last field of a write mutation returns the stored state that the post-write checks read. const STORED_ISSUE_FIELDS = `body labels(first:100){nodes{name} ${PAGE_INFO}}`; // `list` reads the newest plans first, as `gh issue list` did, so the 500-plan cap keeps the same plans. @@ -88,6 +88,8 @@ function parseJson(output, command) { fail(`${command} returned invalid JSON`); } } +// The snapshot pages every issue label with its node id, so a write removes labels from the complete list. +const ISSUE_LABEL_FIELDS = 'name id'; const ASSIGNEE_FIELDS = '... on Actor{login}'; const LABELED_EVENT_FIELDS = '... on LabeledEvent{label{name}}'; const CLOSED_EVENT_FIELDS = `... on ClosedEvent{closer{__typename ... on PullRequest{${PULL_REQUEST_FIELDS} body} ... on Commit{oid associatedPullRequests(first:100){${PULL_REQUEST_CONNECTION}}}}}`; @@ -177,7 +179,7 @@ function readSnapshot( ? '' : [ 'number title body state stateReason url createdAt updatedAt', - issueConnectionSelection('labels', 'name'), + issueConnectionSelection('labels', ISSUE_LABEL_FIELDS), issueConnectionSelection('assignedActors', ASSIGNEE_FIELDS), comments ? issueConnectionSelection('comments', 'body createdAt author{login}') : '', history @@ -231,10 +233,10 @@ function readSnapshot( typeof issue.updatedAt !== 'string' ) fail('gh api graphql returned malformed plan issue'); - issue.labels = issueNodes(number, issue.labels, 'labels', 'name'); + issue.labels = issueNodes(number, issue.labels, 'labels', ISSUE_LABEL_FIELDS); issue.assignees = issueNodes(number, issue.assignedActors, 'assignedActors', ASSIGNEE_FIELDS); if ( - issue.labels.some((label) => typeof label.name !== 'string') || + issue.labels.some((label) => typeof label.name !== 'string' || typeof label.id !== 'string' || !label.id) || issue.assignees.some((owner) => typeof owner.login !== 'string' || !owner.login) ) fail('gh api graphql returned malformed plan issue labels or assignees'); @@ -320,9 +322,7 @@ function preWriteIssue(number) { if ( !hasId(current) || typeof current.body !== 'string' || - !Array.isArray(current.labels) || !Array.isArray(current.assignees) || - !current.labels.every((label) => hasId(label) && typeof label.name === 'string') || !current.assignees.every(hasId) ) fail('gh issue view returned malformed plan issue'); @@ -362,7 +362,7 @@ function writeIssueIfUnchanged(issue, { body, add, remove = [], labelIds } = {}) fields.push({ mutation: 'addLabelsToLabelable', input: 'labelableId:$issue,labelIds:$add', payload: 'labelable' }); } const removed = new Set(remove.map(token)); - const removeIds = current.labels.filter((label) => removed.has(token(label.name))).map((label) => label.id); + const removeIds = issue.labels.filter((label) => removed.has(token(label.name))).map((label) => label.id); if (removeIds.length) { idLists.push(['remove', removeIds]); fields.push({ @@ -371,8 +371,6 @@ function writeIssueIfUnchanged(issue, { body, add, remove = [], labelIds } = {}) payload: 'labelable', }); } - // Nothing left to change: the pre-write read already shows every label to remove as absent. - if (!fields.length) return; const declarations = [ '$issue:ID!', ...(body === undefined ? [] : ['$body:String!']), diff --git a/scripts/tests/fixtures/gh-stub/gh b/scripts/tests/fixtures/gh-stub/gh index f0722e4a..e0356619 100755 --- a/scripts/tests/fixtures/gh-stub/gh +++ b/scripts/tests/fixtures/gh-stub/gh @@ -709,7 +709,10 @@ if (group === 'issue' && command === 'view') { const issue = issueByNumber(number); validateJsonFields(value('--json'), ISSUE_JSON_FIELDS); observeIssue(issue); - emit(JSON.stringify(select(issueJson(issue), value('--json')))); + // Real gh reads `labels(first:100)` for `issue view` (cli/cli api/query_builder.go), so it never + // returns a label past the first 100. + const view = issueJson(issue); + emit(JSON.stringify(select({ ...view, labels: view.labels.slice(0, 100) }, value('--json')))); save(); process.exit(0); } diff --git a/scripts/tests/plan-cli/archive-closer.test.mjs b/scripts/tests/plan-cli/archive-closer.test.mjs index 6123b11d..13107f7c 100644 --- a/scripts/tests/plan-cli/archive-closer.test.mjs +++ b/scripts/tests/plan-cli/archive-closer.test.mjs @@ -12,6 +12,21 @@ test('archive finishes a plan with terminal work, a trusted pass, and the merged assert.deepEqual(issue(landed.number).labels, ['plan']); }); +test('archive removes a stale phase label beyond the first label page of an owned plan', (t) => { + const { run, issue, updateIssue, closedPlan } = planCliContext(t); + // The owned plan needs no claim, so the label removal is the whole write. + const pagedLabels = closedPlan('phase label beyond first page'); + const unrelatedLabels = Array.from({ length: 100 }, (_, index) => `topic:${index}`); + updateIssue(pagedLabels.number, (entry) => { + entry.labels = ['plan', ...unrelatedLabels, 'plan:ongoing']; + entry.assignees = ['plan-agent']; + }); + expectSuccess(run('archive', String(pagedLabels.number)), 'archive label on second page'); + const stored = issue(pagedLabels.number); + assert.deepEqual(stored.labels, ['plan', ...unrelatedLabels]); + assert.ok(!stored.labels.some((label) => label.toLowerCase().startsWith('plan:'))); +}); + test('archive uses the latest actual closure over older closures and a later non-closure event', (t) => { const { run, updateIssue, closedPlan } = planCliContext(t); // The latest actual closure wins over older closures and a later non-closure event.