diff --git a/.changeset/974-graphify-budget-missing-value.md b/.changeset/974-graphify-budget-missing-value.md new file mode 100644 index 000000000..2fbc4cefb --- /dev/null +++ b/.changeset/974-graphify-budget-missing-value.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 986 +--- +**`graphify query --budget` with no value now errors instead of silently ignoring the budget** — a trailing `--budget` parsed as `NaN` and was treated as 'no budget', so the query ran unbounded with no warning. (#974) diff --git a/scripts/lint-regression-test-names.allowlist.json b/scripts/lint-regression-test-names.allowlist.json index 0027a5407..ec95d452c 100644 --- a/scripts/lint-regression-test-names.allowlist.json +++ b/scripts/lint-regression-test-names.allowlist.json @@ -260,5 +260,6 @@ "bug-947-hermes-gsd-prefix.test.cjs", "bug-948-state-noop-write-guard.test.cjs", "bug-950-quick-summary-status-complete.test.cjs", + "bug-974-graphify-budget-missing-value.test.cjs", "bug-978-milestone-complete-force.test.cjs" ] diff --git a/scripts/lint-test-file-count.allowlist.json b/scripts/lint-test-file-count.allowlist.json index ac766c5f1..ea2ebaa20 100644 --- a/scripts/lint-test-file-count.allowlist.json +++ b/scripts/lint-test-file-count.allowlist.json @@ -1,5 +1,5 @@ { - "_doc": "Baseline of modules currently exceeding the 2-test-file limit. Each entry locks in TODAY's exact test filenames as the allowlisted set (identity ratchet). Adding a NEW test file to a capped module fails (novel). Removing one requires pruning this list (stale, ratchet-down). When a cluster drops to ≤ 2, remove its entry entirely. New entries require justification in PR description.", + "_doc": "Baseline of modules currently exceeding the 2-test-file limit. Each entry locks in TODAY's exact test filenames as the allowlisted set (identity ratchet). Adding a NEW test file to a capped module fails (novel). Removing one requires pruning this list (stale, ratchet-down). When a cluster drops to \u2264 2, remove its entry entirely. New entries require justification in PR description.", "modules": { "audit": { "files": [ @@ -27,6 +27,7 @@ "graphify": { "files": [ "bug-622-graphify-optional-graph-html.test.cjs", + "bug-974-graphify-budget-missing-value.test.cjs", "graphify-auto-update.slow.test.cjs", "graphify-command-cutover.test.cjs", "graphify-query.test.cjs", diff --git a/src/graphify-command-router.cts b/src/graphify-command-router.cts index 67216482d..c736f4f09 100644 --- a/src/graphify-command-router.cts +++ b/src/graphify-command-router.cts @@ -64,7 +64,15 @@ function routeGraphifyCommand({ args, cwd, raw, error, _graphify }: RouteGraphif return; } const budgetIdx = args.indexOf('--budget'); - const budget = budgetIdx !== -1 ? parseInt(args[budgetIdx + 1], 10) : null; + let budget: number | null = null; + if (budgetIdx !== -1) { + const rawBudget = args[budgetIdx + 1]; + if (rawBudget === undefined || Number.isNaN(parseInt(rawBudget, 10))) { + error('Usage: gsd-tools graphify query [--budget ]', ERROR_REASON.USAGE); + return; + } + budget = parseInt(rawBudget, 10); + } core.output(g.graphifyQuery(cwd, term, { budget }), raw); } else if (subcommand === 'status') { core.output(g.graphifyStatus(cwd), raw); diff --git a/tests/bug-974-graphify-budget-missing-value.test.cjs b/tests/bug-974-graphify-budget-missing-value.test.cjs new file mode 100644 index 000000000..b86578403 --- /dev/null +++ b/tests/bug-974-graphify-budget-missing-value.test.cjs @@ -0,0 +1,362 @@ +'use strict'; +/** + * bug-974-graphify-budget-missing-value.test.cjs + * + * Regression guard: `gsd-tools graphify query --budget` with no value + * following --budget must emit a USAGE error and exit non-zero. + * + * Bug: args[budgetIdx + 1] was undefined → parseInt(undefined, 10) → NaN. + * NaN is falsy so the budget guard `if (options.budget)` silently skipped + * trimming and the query ran unbounded with no warning. (#974) + * + * Test categories: + * 1. BEHAVIORAL (recording mock) — direct routeGraphifyCommand call, no I/O + * 2. SUBPROCESS — end-to-end via runGsdTools + * 3. PROPERTY — budget arg parser: non-numeric/absent → usage error, valid int → passes + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); + +const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { + enableGraphify, + writeGraphJson, + SAMPLE_GRAPH, +} = require('./helpers/graphify.cjs'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const { routeGraphifyCommand } = require('../gsd-core/bin/lib/graphify-command-router.cjs'); + +// ─── Recording-mock helpers (mirrors graphify-command-cutover.test.cjs) ──────── + +function makeGraphifyMock() { + const calls = []; + function recorder(name, ...fnArgs) { + const sentinel = { _mock: name, args: fnArgs }; + calls.push(sentinel); + return sentinel; + } + return { + calls, + mock: { + graphifyQuery: (cwd, term, opts) => recorder('graphifyQuery', cwd, term, opts), + graphifyStatus: (cwd) => recorder('graphifyStatus', cwd), + graphifyDiff: (cwd) => recorder('graphifyDiff', cwd), + graphifyBuild: (cwd) => recorder('graphifyBuild', cwd), + writeSnapshot: (cwd) => recorder('writeSnapshot', cwd), + }, + }; +} + +function makeErrorRecorder() { + const calls = []; + const fn = (msg, reason) => calls.push({ msg, reason }); + fn.calls = calls; + return fn; +} + +function runJsonErrors(args, tmpDir, env = {}) { + const result = runGsdTools(args, tmpDir, { ...env, GSD_JSON_ERRORS: '1' }); + assert.strictEqual(result.success, false, + `Expected failure with GSD_JSON_ERRORS=1 for args: ${args.join(' ')}\n` + + `stdout: ${result.output}\nstderr: ${result.error}`); + let parsed; + try { + parsed = JSON.parse(result.error); + } catch (e) { + throw new Error( + `GSD_JSON_ERRORS=1 must emit valid JSON on stderr.\n` + + `Args: ${args.join(' ')}\nstderr: ${result.error}\nparse error: ${e.message}`, + ); + } + return parsed; +} + +function assertTypedError(parsed, expectedReason, label) { + assert.strictEqual(parsed.ok, false, `${label}: error object must have ok: false`); + assert.strictEqual(parsed.reason, expectedReason, + `${label}: reason must be "${expectedReason}", got: ${parsed.reason}`); + assert.ok(typeof parsed.message === 'string' && parsed.message.length > 0, + `${label}: message must be a non-empty string`); +} + +// ─── 1. BEHAVIORAL — direct routeGraphifyCommand (recording mock, no I/O) ───── + +describe('bug #974: --budget missing value → usage error (unit, recording mock)', () => { + const CWD = '/fake/cwd'; + const RAW = false; + + // (a) --budget as last arg (no value following) + test('(a) --budget last arg (missing value) → error(USAGE); graphifyQuery NOT called', () => { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget'], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 1, + `error must be called exactly once; got ${errFn.calls.length} calls`); + assert.strictEqual(errFn.calls[0].reason, 'usage', + `reason must be 'usage'; got: ${errFn.calls[0].reason}`); + assert.ok( + errFn.calls[0].msg.includes('graphify query'), + `usage message must mention "graphify query"; got: ${errFn.calls[0].msg}`, + ); + assert.strictEqual(calls.length, 0, + 'graphifyQuery must NOT be called when --budget has no value'); + }); + + // (b) --budget with a non-numeric value + test('(b) --budget foo (non-numeric value) → error(USAGE); graphifyQuery NOT called', () => { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget', 'foo'], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 1, + `error must be called exactly once; got ${errFn.calls.length} calls`); + assert.strictEqual(errFn.calls[0].reason, 'usage', + `reason must be 'usage'; got: ${errFn.calls[0].reason}`); + assert.strictEqual(calls.length, 0, + 'graphifyQuery must NOT be called when --budget value is non-numeric'); + }); + + // (c) --budget 0 → 0 is a valid integer, must NOT error + test('(c) --budget 0 (explicit zero) → budget passed as 0, no error', () => { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget', '0'], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 0, + `error must NOT be called for --budget 0; got: ${JSON.stringify(errFn.calls)}`); + assert.strictEqual(calls.length, 1, + 'graphifyQuery MUST be called for --budget 0'); + assert.strictEqual(calls[0]._mock, 'graphifyQuery'); + assert.strictEqual(calls[0].args[2].budget, 0, + `budget must be 0, got: ${calls[0].args[2].budget}`); + }); + + // (d) valid --budget 500 → regression: still works after fix + test('(d) --budget 500 (valid positive integer) → budget: 500, no error', () => { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget', '500'], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 0, + `error must NOT be called for --budget 500; got: ${JSON.stringify(errFn.calls)}`); + assert.strictEqual(calls.length, 1); + assert.strictEqual(calls[0]._mock, 'graphifyQuery'); + assert.strictEqual(calls[0].args[2].budget, 500, + `budget must be integer 500; got: ${calls[0].args[2].budget}`); + assert.strictEqual(typeof calls[0].args[2].budget, 'number', + 'budget must be a number, not string'); + }); + +}); + +// ─── 2. SUBPROCESS — end-to-end via runGsdTools ─────────────────────────────── + +describe('bug #974: --budget missing value → usage error (subprocess)', () => { + let tmpDir; + let planningDir; + + beforeEach(() => { + tmpDir = createTempProject(); + planningDir = path.join(tmpDir, '.planning'); + enableGraphify(planningDir); + writeGraphJson(planningDir, SAMPLE_GRAPH); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('subprocess: --budget last arg → non-zero exit', () => { + const result = runGsdTools( + ['graphify', 'query', 'AuthService', '--budget'], + tmpDir, + ); + assert.strictEqual(result.success, false, + `--budget with no value must exit non-zero; stderr: ${result.error}`); + }); + + test('subprocess: --budget last arg → stderr contains usage hint', () => { + const result = runGsdTools( + ['graphify', 'query', 'AuthService', '--budget'], + tmpDir, + ); + assert.ok( + result.error.includes('Usage') || result.error.includes('graphify query'), + `stderr must contain usage hint; got: ${result.error}`, + ); + }); + + test('subprocess: --budget foo (non-numeric) → non-zero exit', () => { + const result = runGsdTools( + ['graphify', 'query', 'AuthService', '--budget', 'foo'], + tmpDir, + ); + assert.strictEqual(result.success, false, + `--budget with non-numeric value must exit non-zero; stderr: ${result.error}`); + }); + + test('subprocess: --budget last arg → GSD_JSON_ERRORS=1 → usage reason', () => { + const parsed = runJsonErrors( + ['graphify', 'query', 'AuthService', '--budget'], + tmpDir, + ); + assertTypedError(parsed, 'usage', '--budget missing value json-error'); + assert.ok( + parsed.message.includes('graphify query'), + `message must include "graphify query"; got: ${parsed.message}`, + ); + }); + + test('subprocess: --budget foo → GSD_JSON_ERRORS=1 → usage reason', () => { + const parsed = runJsonErrors( + ['graphify', 'query', 'AuthService', '--budget', 'foo'], + tmpDir, + ); + assertTypedError(parsed, 'usage', '--budget non-numeric json-error'); + }); + + // Regression: valid --budget still works after the fix + test('subprocess: --budget 500 (valid) → success, term echoed', () => { + const result = runGsdTools( + ['graphify', 'query', 'AuthService', '--budget', '500'], + tmpDir, + ); + assert.ok(result.success, + `--budget 500 must still succeed after fix; error: ${result.error}`); + const parsed = JSON.parse(result.output); + assert.strictEqual(parsed.term, 'AuthService', + 'term must be echoed in query response'); + assert.ok('nodes' in parsed, 'response must include nodes array'); + assert.ok('total_nodes' in parsed, 'response must include total_nodes field'); + }); +}); + +// ─── 3. PROPERTY — budget parse: non-numeric/absent → usage error, valid int → passes +// Per RULESET.TESTS.property-based-testing: parsing/budget-limit modules +// must include ≥1 fast-check property test. + +describe('bug #974: budget arg parser property tests', () => { + const CWD = '/fake/cwd'; + const RAW = false; + + // Property (a): any non-numeric, non-parseable string for --budget → usage error + // + // The budget-VALUE generator is constrained to genuinely non-numeric values. + // The .filter(v => Number.isNaN(parseInt(v, 10))) at the end is the authoritative + // gate: it excludes any value that parseInt(v, 10) would accept (e.g. "+5", "-3", + // " 7", "0x1a", "007"). This makes the property universally true rather than + // relying on a runtime skip inside the property body. + // The term is fixed as "myterm" — this property is about the BUDGET VALUE, not + // the term. + test('property: non-numeric --budget value always produces usage error', () => { + fc.assert( + fc.property( + fc.oneof( + // Alphabetic-start strings (parseInt('abc') = NaN) + fc.stringMatching(/^[a-zA-Z][a-zA-Z0-9_-]{0,20}$/), + // Empty string (parseInt('') = NaN) + fc.constant(''), + // Strings starting with special chars — filtered below for safety + fc.stringMatching(/^[!@#$%^&*()_+={}\][|\\:;"'<,>?/~`][^\s]{0,10}$/), + ).filter(v => Number.isNaN(parseInt(v, 10))), + (badValue) => { + const { mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget', badValue], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 1, + `--budget "${badValue}" (NaN) must produce exactly one error call`); + assert.strictEqual(errFn.calls[0].reason, 'usage', + `--budget "${badValue}" error must have reason 'usage'; got: ${errFn.calls[0].reason}`); + }, + ), + // Explicit seed + numRuns for CI determinism. + { seed: 1001, numRuns: 200 }, + ); + }); + + // Example (b): --budget as last arg (missing value) → usage error, for fixed valid terms. + // Previously a property fuzz over a validTerm generator; replaced with deterministic + // examples because the regex `/^[A-Za-z0-9][A-Za-z0-9_.-]{0,29}$/` admitted single + // digits like "0" which are falsy and trigger the router's `if (!term)` guard instead + // of the budget-missing-value path, causing a spurious test failure (seed 1004, term + // "0"). These properties are about the BUDGET contract; the term is a fixed bystander. + test('example: --budget as last arg always produces usage error (fixed terms)', () => { + const FIXED_TERMS = ['someterm', 'AuthService', 'a1', 'graph-node_1', 'MyComponent']; + for (const term of FIXED_TERMS) { + const { mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', term, '--budget'], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 1, + `--budget as last arg: term=${JSON.stringify(term)}: must produce exactly one error call; got ${errFn.calls.length}`); + assert.strictEqual(errFn.calls[0].reason, 'usage', + `--budget as last arg: term=${JSON.stringify(term)}: reason must be 'usage'; got ${errFn.calls[0].reason}`); + } + }); + + // Property (c): valid positive integer strings → budget passed through as number, no error + test('property: positive integer string for --budget is always accepted', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 10_000_000 }), + (n) => { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', 'myterm', '--budget', String(n)], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 0, + `--budget ${n} (valid positive integer) must not produce an error`); + assert.strictEqual(calls.length, 1, 'graphifyQuery must be called'); + assert.strictEqual(calls[0].args[2].budget, n, + `budget must equal ${n}; got: ${calls[0].args[2].budget}`); + assert.strictEqual(typeof calls[0].args[2].budget, 'number', + `budget must be a number, not string "${calls[0].args[2].budget}"`); + }, + ), + // Explicit seed + numRuns for CI determinism. + { seed: 1003, numRuns: 200 }, + ); + }); + + // Example (d): absence of --budget flag yields budget:null and no error, for fixed valid terms. + // Previously a property fuzz over a validTerm generator; replaced with deterministic + // examples for the same reason as example (b): single-digit terms like "0" are falsy + // and trigger the router's term-missing usage error, not the budget-absent path. + // These examples fix the term as clearly valid multi-char identifiers. + test('example: absence of --budget flag yields budget:null and no error (fixed terms)', () => { + const FIXED_TERMS = ['someterm', 'AuthService', 'a1', 'graph-node_1', 'MyComponent']; + for (const term of FIXED_TERMS) { + const { calls, mock } = makeGraphifyMock(); + const errFn = makeErrorRecorder(); + routeGraphifyCommand({ + args: ['graphify', 'query', term], + cwd: CWD, raw: RAW, error: errFn, _graphify: mock, + }); + assert.strictEqual(errFn.calls.length, 0, + `no --budget flag must not produce an error for term "${term}"`); + assert.strictEqual(calls.length, 1, + `graphifyQuery must be called for term "${term}"; got calls.length=${calls.length}`); + assert.strictEqual(calls[0].args[2].budget, null, + `absent --budget must pass budget:null; got: ${calls[0].args[2].budget}`); + } + }); +});