diff --git a/src/main/hireSpawn.ts b/src/main/hireSpawn.ts index d8bfad58d..592aed397 100644 --- a/src/main/hireSpawn.ts +++ b/src/main/hireSpawn.ts @@ -143,19 +143,30 @@ export function mergeHireMcpDefaults( * consumer a single value to read, so "I forgot one field" stops being possible. */ export function resolveHireDefaults( - raw: { provider?: unknown; model?: unknown; name?: unknown; character?: unknown; accent?: unknown }, - manifest?: { provider?: string; model?: string; name?: string; character?: string; accent?: string } -): { provider?: string; model?: string; name?: string; character?: string; accent?: string } { + raw: { provider?: unknown; model?: unknown; name?: unknown; character?: unknown; accent?: unknown; tokenCap?: unknown }, + manifest?: { provider?: string; model?: string; name?: string; character?: string; accent?: string; tokenCap?: number } +): { provider?: string; model?: string; name?: string; character?: string; accent?: string; tokenCap?: number } { // A request value counts only when it is a non-empty string: `""` is not a // stated choice, and letting it through would mean a hire silently lost a field // to an empty one. const req = (v: unknown): string | undefined => typeof v === 'string' && v.trim() ? v.trim() : undefined; + // Same rule for the numeric ceiling: only a positive finite number is a stated + // choice, so 0/NaN/"4000000" fall through to the manifest instead of quietly + // cancelling the hire's cap. The manifest value is already range-checked by + // validateHireManifest, so nothing unbounded can arrive that way. + const reqCap = (v: unknown): number | undefined => + typeof v === 'number' && Number.isFinite(v) && v > 0 ? v : undefined; return { provider: req(raw.provider) ?? manifest?.provider, model: req(raw.model) ?? manifest?.model, name: req(raw.name) ?? manifest?.name, character: req(raw.character) ?? manifest?.character, - accent: req(raw.accent) ?? manifest?.accent + accent: req(raw.accent) ?? manifest?.accent, + // T-062: the cap used to be read straight off the spawn-request at the call + // site, so a manifest ceiling was silently ignored and a request that OMITTED + // tokenCap produced an UNCAPPED worker — the safe-looking spawn was the + // unsafe one. Resolving it here makes it obey the same request-wins rule. + tokenCap: reqCap(raw.tokenCap) ?? manifest?.tokenCap }; } diff --git a/src/main/index.ts b/src/main/index.ts index e865e472d..1653e0571 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -4686,9 +4686,11 @@ async function processSpawnRequest(filePath: string): Promise { } catch { /* window torn down */ } // Register for done-scan / idle-reap / token-cap / safe teardown (pty id == workerId). - // tokenCap is optional plumbing (default unlimited) — only a positive finite cap is kept. - const tokenCap = typeof raw.tokenCap === 'number' && Number.isFinite(raw.tokenCap) && raw.tokenCap > 0 - ? raw.tokenCap : undefined; + // tokenCap is optional plumbing (default unlimited) — only a positive finite cap is + // kept. It comes from `eff`, NOT from `raw`: reading the request directly here is + // what made a hire's declared ceiling unenforceable, so that a spawn-request which + // simply omitted tokenCap launched an UNCAPPED worker (T-062). + const tokenCap = eff.tokenCap; liveWorkers.set(workerId, { workerId, reqId, name: meta.name, slack, baseBranch, spawnedAt: Date.now(), tokenCap }); // Dispatch the objective via the standard inbox path (zero new transport), diff --git a/test/hire-spawn.test.cjs b/test/hire-spawn.test.cjs index b6fe5cb34..757de1257 100644 --- a/test/hire-spawn.test.cjs +++ b/test/hire-spawn.test.cjs @@ -15,9 +15,14 @@ const test = require('node:test'); const assert = require('node:assert/strict'); +const { readFileSync } = require('node:fs'); +const path = require('node:path'); +const ts = require('typescript'); const loadTs = require('./load-ts.cjs'); const { mergeHireMcpDefaults, resolveHireManifestPath, resolveHireDefaults } = loadTs('src/main/hireSpawn.ts'); +const { validateHireManifest, HIRE_SPEC_V1 } = loadTs('src/shared/hire.ts'); +const { MAX_AGENT_TOKEN_CAP } = loadTs('src/shared/tokenCaps.ts'); const { buildWorkerLaunch } = loadTs('src/main/workerLaunch.ts'); const { MCP_CATALOG } = loadTs('src/shared/mcpCatalog.ts'); @@ -107,14 +112,14 @@ test('the request wins on every field it states', () => { { provider: 'codex', model: 'm-req', name: 'n-req', character: 'c-req', accent: 'a-req' }, { provider: 'claude', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire' } ); - assert.deepEqual(eff, { provider: 'codex', model: 'm-req', name: 'n-req', character: 'c-req', accent: 'a-req' }); + assert.deepEqual(eff, { provider: 'codex', model: 'm-req', name: 'n-req', character: 'c-req', accent: 'a-req', tokenCap: undefined }); }); test('the manifest fills every field the request leaves unset', () => { const eff = resolveHireDefaults({}, { - provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire' + provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire', tokenCap: undefined }); - assert.deepEqual(eff, { provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire' }); + assert.deepEqual(eff, { provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire', tokenCap: undefined }); assert.equal(eff.provider, 'codex', "the exact regression: a codex hire must not resolve to claude"); }); @@ -123,10 +128,256 @@ test('an empty or whitespace request value is not a stated choice', () => { { provider: '', model: ' ', name: '', character: null, accent: undefined }, { provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire' } ); - assert.deepEqual(eff, { provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire' }); + assert.deepEqual(eff, { provider: 'codex', model: 'm-hire', name: 'n-hire', character: 'c-hire', accent: 'a-hire', tokenCap: undefined }); }); test('with no manifest at all, nothing is invented', () => { assert.deepEqual(resolveHireDefaults({ provider: 'codex' }, undefined), - { provider: 'codex', model: undefined, name: undefined, character: undefined, accent: undefined }); + { provider: 'codex', model: undefined, name: undefined, character: undefined, accent: undefined, tokenCap: undefined }); +}); + + +// ── tokenCap precedence (T-062: spend control) ────────────────────────────── +// A manifest's `tokenCap` was never consulted on spawn: the cap came solely from +// the spawn-request JSON, so a hire that declares a ceiling launched UNCAPPED +// whenever the request omitted one — and omitting it is the safe-looking choice. +// tokenCap now resolves through the same one place as every other hire field. +// The numbers below are arbitrary literals; nothing here is shared with the +// implementation, which carries no cap constant of its own. + +test('a request token cap wins over the manifest (both set)', () => { + const eff = resolveHireDefaults({ tokenCap: 250000 }, { tokenCap: 4000000 }); + assert.equal(eff.tokenCap, 250000, 'the request states a ceiling; the manifest is only a default'); +}); + +test('the manifest token cap applies when the request omits it', () => { + // THE BUG. Before the fix this resolved to undefined and the worker ran uncapped. + const eff = resolveHireDefaults({}, { tokenCap: 4000000 }); + assert.equal(eff.tokenCap, 4000000, 'a hire that declares a ceiling must never spawn uncapped'); +}); + +test('a request token cap applies with no manifest at all', () => { + const eff = resolveHireDefaults({ tokenCap: 250000 }, undefined); + assert.equal(eff.tokenCap, 250000); +}); + +test('a non-positive or non-numeric request cap is not a stated choice', () => { + // Same rule as the string fields: an unusable request value must fall back to + // the manifest, not silently disable the hire's ceiling. + for (const bad of [0, -1, NaN, Infinity, '4000000', null, true]) { + assert.equal(resolveHireDefaults({ tokenCap: bad }, { tokenCap: 4000000 }).tokenCap, 4000000, + `tokenCap ${String(bad)} must fall through to the manifest`); + } +}); + +test('with a cap nowhere, none is invented', () => { + assert.equal(resolveHireDefaults({}, {}).tokenCap, undefined); + assert.equal(resolveHireDefaults({}, undefined).tokenCap, undefined); +}); + +// ── tokenCap edge battery (T-062 QA) ──────────────────────────────────────── +// `reqCap` is the only guard on the request side, so the exact boundary between +// "a stated ceiling" and "fall through to the manifest" is worth pinning value by +// value. Nothing here is shared with the implementation: these are literals, and +// resolveHireDefaults carries no cap constant of its own. + +test('edge: every unusable request cap falls through to the manifest ceiling', () => { + const unusable = [ + 0, -0, -1, -4000000, NaN, Infinity, -Infinity, + '4000000', '0', '', ' ', // numeric strings are not numbers + null, undefined, true, false, + [], [4000000], // arrays coerce to numbers elsewhere; not here + {}, { valueOf: () => 4000000 }, // no valueOf coercion either + Number('nope') + ]; + for (const bad of unusable) { + assert.equal( + resolveHireDefaults({ tokenCap: bad }, { tokenCap: 4000000 }).tokenCap, 4000000, + `tokenCap ${JSON.stringify(bad) ?? String(bad)} must fall through to the manifest, not cancel it` + ); + } +}); + +test('edge: an unusable request cap with NO manifest leaves the worker uncapped', () => { + // The mirror of the case above, and the one that actually spends money: with + // nothing to fall back to, an unusable value must resolve to undefined rather + // than to a bogus ceiling (0 would read as "unlimited" downstream anyway). + for (const bad of [0, -1, NaN, Infinity, '4000000', null, true, {}]) { + assert.equal(resolveHireDefaults({ tokenCap: bad }, undefined).tokenCap, undefined); + assert.equal(resolveHireDefaults({ tokenCap: bad }, {}).tokenCap, undefined); + } +}); + +test('edge: any positive finite number is a stated ceiling, integer or not', () => { + // Deliberately pinning the CURRENT contract, including the odd corners: the + // guard is `> 0 && Number.isFinite`, not `Number.isInteger`. If someone tightens + // it to integers-only, this test is the one that has to be argued with first. + for (const good of [1, 0.5, 1e-6, 250000, Number.MAX_SAFE_INTEGER, MAX_AGENT_TOKEN_CAP]) { + assert.equal(resolveHireDefaults({ tokenCap: good }, { tokenCap: 4000000 }).tokenCap, good, + `${good} is a positive finite number and must win over the manifest`); + } +}); + +test('edge: a malformed manifest cap never reaches resolveHireDefaults — validateHireManifest rejects it', () => { + // resolveHireDefaults trusts `manifest.tokenCap` unchecked, which is only safe + // because the manifest was validated on the way in. That guard is load-bearing + // for the whole fix, so it is asserted here rather than assumed. + // spec id DERIVED from the module, never a copied literal. + const base = { spec: HIRE_SPEC_V1, name: 'n', description: 'd', goal: 'g' }; + for (const bad of [0, -1, 1.5, NaN, Infinity, '4000000', null, true, MAX_AGENT_TOKEN_CAP + 1]) { + const res = validateHireManifest({ ...base, tokenCap: bad }); + assert.equal(res.ok, false, `manifest tokenCap ${String(bad)} must be rejected`); + assert.ok(res.errors.some((e) => /tokenCap/.test(e)), `rejection must name tokenCap (got ${res.errors})`); + } + const good = validateHireManifest({ ...base, tokenCap: 4000000 }); + assert.equal(good.ok, true, `a well-formed cap must survive (got ${good.errors})`); + assert.equal(good.manifest.tokenCap, 4000000); +}); + + +// ── call site: SOURCE ASSERTION, NOT AN EXECUTION TEST ────────────────────── +// index.ts imports electron, so processSpawnRequest cannot be loaded or run in +// process, and reimplementing the call site in a test has shipped real defects +// here. So this inspects the real file — but through the TypeScript AST, the way +// renderer-sandbox.test.cjs pins the Electron sandbox flag, NOT through a regex +// over raw text. +// +// The regex version this replaces could not fail: commenting out the production +// line at index.ts:4691 left all 18 tests green, because `assert.match` happily +// matched the commented-out copy. It could also fire on a NON-regression, because +// `assert.doesNotMatch(block, /raw\.tokenCap/)` would trip on the words appearing +// in a comment. An AST carries no comments, so both failure modes are gone. +// +// WHAT THIS PROVES: in the live (non-commented) source of processSpawnRequest, +// `tokenCap` is bound from `eff.tokenCap` and that binding is what gets handed to +// liveWorkers.set, and nothing in that function reads `raw.tokenCap`. +// WHAT THIS DOES NOT PROVE: that the spawn path actually runs, that the value +// reaches the reaper at index.ts:4830, or that `eff` is the resolveHireDefaults +// result. Those need the electron-free integration this file cannot have. The +// behaviour of resolveHireDefaults itself is covered by the real tests above. + +const INDEX_TS = path.join(__dirname, '..', 'src', 'main', 'index.ts'); + +/** Every descendant of `node` (inclusive) matching `predicate`. */ +function collect(node, predicate) { + const out = []; + (function walk(n) { + if (predicate(n)) out.push(n); + ts.forEachChild(n, walk); + })(node); + return out; +} + +/** + * Parse `text` as TypeScript and report the shape of the token-cap wiring inside + * `processSpawnRequest`. Pure analysis over an AST, so comments are invisible to + * it by construction. Takes text (not a path) so the meta-tests below can prove + * it actually bites without touching the real file. + */ +function analyseSpawnCallSite(text) { + const source = ts.createSourceFile('index.ts', text, ts.ScriptTarget.Latest, true); + const fns = collect(source, (n) => + (ts.isFunctionDeclaration(n) || ts.isFunctionExpression(n) || ts.isArrowFunction(n)) + && ((n.name && ts.isIdentifier(n.name) && n.name.text === 'processSpawnRequest') + || (ts.isVariableDeclaration(n.parent) && ts.isIdentifier(n.parent.name) + && n.parent.name.text === 'processSpawnRequest'))); + if (fns.length !== 1) return { fnCount: fns.length }; + const fn = fns[0]; + + const decls = collect(fn, (n) => + ts.isVariableDeclaration(n) && ts.isIdentifier(n.name) && n.name.text === 'tokenCap'); + + // Live reads of `raw.tokenCap` — a mention inside a comment is not a node. + const rawReads = collect(fn, (n) => + ts.isPropertyAccessExpression(n) + && ts.isIdentifier(n.expression) && n.expression.text === 'raw' + && n.name.text === 'tokenCap'); + + // The registration call and how it names the cap. + const registrations = collect(fn, (n) => + ts.isCallExpression(n) && n.expression.getText(source) === 'liveWorkers.set'); + const capProps = registrations.flatMap((call) => { + const arg = call.arguments[1]; + if (!arg || !ts.isObjectLiteralExpression(arg)) return []; + return arg.properties.filter((p) => + p.name && ts.isIdentifier(p.name) && p.name.text === 'tokenCap'); + }); + + return { + fnCount: fns.length, + declCount: decls.length, + declInitializer: decls.length === 1 && decls[0].initializer + ? decls[0].initializer.getText(source) : undefined, + rawReadCount: rawReads.length, + registrationCount: registrations.length, + capPropCount: capProps.length, + // Shorthand `{ tokenCap }` means the local binding above; a longhand + // `{ tokenCap: X }` is recorded so a swap to something else is visible. + capPropSource: capProps.length === 1 + ? (ts.isShorthandPropertyAssignment(capProps[0]) ? 'tokenCap' : capProps[0].initializer.getText(source)) + : undefined + }; +} + +test('call site (AST source assertion, not an execution test): processSpawnRequest binds tokenCap from eff and registers that binding', () => { + const shape = analyseSpawnCallSite(readFileSync(INDEX_TS, 'utf8')); + + assert.equal(shape.fnCount, 1, 'exactly one processSpawnRequest must exist in index.ts'); + assert.equal(shape.declCount, 1, + 'processSpawnRequest must contain exactly one LIVE `tokenCap` binding — zero means the line was commented out or deleted'); + assert.equal(shape.declInitializer, 'eff.tokenCap', + 'the cap must be read from the resolved defaults, so a manifest ceiling is honoured (T-062)'); + assert.equal(shape.rawReadCount, 0, + 'reading `raw.tokenCap` is what let a hire spawn uncapped when the request omitted a cap'); + assert.equal(shape.registrationCount, 1, 'exactly one liveWorkers.set registration'); + assert.equal(shape.capPropCount, 1, 'the registration must carry a tokenCap property'); + assert.equal(shape.capPropSource, 'tokenCap', + 'the registration must pass the resolved binding, not some other expression'); +}); + +// The meta-tests: proof that the assertion above can actually fail, and that it +// does not fire on a non-regression. Synthetic sources — they do NOT reimplement +// the call site, they exercise the analyser that inspects it. + +test('call-site analyser bites: a commented-out binding reports zero live declarations', () => { + const shape = analyseSpawnCallSite(` + async function processSpawnRequest(f: string) { + // const tokenCap = eff.tokenCap; + liveWorkers.set(workerId, { workerId, tokenCap }); + }`); + assert.equal(shape.declCount, 0, 'a commented-out binding must not count as live code'); +}); + +test('call-site analyser bites: a binding moved back to raw.tokenCap is reported', () => { + const shape = analyseSpawnCallSite(` + async function processSpawnRequest(f: string) { + const tokenCap = raw.tokenCap; + liveWorkers.set(workerId, { workerId, tokenCap }); + }`); + assert.equal(shape.declInitializer, 'raw.tokenCap'); + assert.equal(shape.rawReadCount, 1); +}); + +test('call-site analyser does not false-fire: raw.tokenCap named only in a comment is not a read', () => { + // The mirror-image failure of the regex it replaces, which would have gone RED + // on this file even though the code is correct. + const shape = analyseSpawnCallSite(` + async function processSpawnRequest(f: string) { + // NOTE: this used to be raw.tokenCap, which was the bug. + const tokenCap = eff.tokenCap; + liveWorkers.set(workerId, { workerId, tokenCap }); + }`); + assert.equal(shape.rawReadCount, 0, 'a comment mentioning raw.tokenCap is not a read'); + assert.equal(shape.declInitializer, 'eff.tokenCap'); + assert.equal(shape.capPropSource, 'tokenCap'); +}); + +test('call-site analyser bites: registering a different expression is reported', () => { + const shape = analyseSpawnCallSite(` + async function processSpawnRequest(f: string) { + const tokenCap = eff.tokenCap; + liveWorkers.set(workerId, { workerId, tokenCap: raw.tokenCap }); + }`); + assert.equal(shape.capPropSource, 'raw.tokenCap', 'a longhand override must be visible, not hidden by the live binding'); + assert.equal(shape.rawReadCount, 1); });