Repository navigation
fix(T-062): apply a hire manifest's tokenCap on spawn - #8
Closed
aaroncoville wants to merge 3 commits into
Closed
aaroncoville wants to merge 3 commits into
aaroncoville wants to merge 3 commits into
Conversation
added 3 commits
August 24, 2026 19:35
The per-worker token cap was derived solely from the spawn-request JSON, so a manifest's `tokenCap` was never consulted. Worse than the stated bug: undefined means unlimited, so a spawn-request that simply OMITTED tokenCap produced an UNCAPPED worker even when its manifest declared a ceiling — the safe-looking action was the unsafe one (docs/hires/manifests/ryan-developer.hire.json declares 4000000 and it was silently ignored). tokenCap now resolves through resolveHireDefaults alongside provider/model/ name/character/accent, with the same request-wins-manifest-defaults rule, and the spawn path reads the resolved value instead of the raw request. A request cap counts only when it is a positive finite number, so 0/NaN/"4000000" fall through to the manifest rather than quietly cancelling the hire's ceiling. Tests cover all three precedence cases plus the unusable-request and cap-nowhere edges; a source assertion pins the call site (index.ts imports electron and cannot be loaded in-process), using the same pattern hire-import.test.cjs uses for the hire:openFile handler. Verified by reverting each half: without the manifest fallback the manifest-only case goes RED, and with the call site back on raw.tokenCap the call-site test goes RED. The four existing full-shape deepEqual assertions gained `tokenCap: undefined` so they keep asserting the complete resolved shape — that is what makes "nothing is invented" cover the new field too. No change to MCP consent: a god-authored spawn-request still arms nothing.
The call-site test could not fail. It sliced the worker-registration block out
of src/main/index.ts as raw text and ran `assert.match(block, /const tokenCap =
eff\.tokenCap;/)`. Comment out the production line at index.ts:4691 and all 18
tests stayed green — the regex matched the commented-out copy. Fifth time this
defect class has shipped here, and the same shape QA fixed on T-061.
Severity, stated honestly: `npm run typecheck` DOES catch that exact mutation
(TS18004, no value in scope for the shorthand property `tokenCap`), so this
route could not have silently shipped. The test was still wrong — it claimed to
prove wiring and proved nothing — but it is not the T-061 case where nothing
caught it.
The mirror-image failure was live too: `assert.doesNotMatch(block, /raw\.token
Cap/)` would go RED on a comment merely mentioning `raw.tokenCap`, so the test
could both miss a regression and fire on a non-regression.
Fixed by parsing the TypeScript AST instead of the text, which is the existing
house pattern — renderer-sandbox.test.cjs pins the Electron sandbox flag the
same way, and its header says why: comments and string markers must not be able
to satisfy the contract. That is strictly stronger than T-061's comment-line
stripping, which is still blind to block comments. An AST carries no comments at
all, so both failure modes are structurally gone rather than patched.
The assertion now proves, over live nodes only: exactly one `processSpawnRequest`
exists; it holds exactly one live `tokenCap` binding; that binding's initializer
is `eff.tokenCap`; the single `liveWorkers.set` registration carries `tokenCap`
from that binding and not some other expression; and nothing in the function
reads `raw.tokenCap`. The test name and a header comment say plainly that this is
a source assertion and NOT an execution test, and list what it does not prove —
that the spawn path runs, that the value reaches the reaper at index.ts:4830, or
that `eff` is the resolveHireDefaults result. index.ts imports electron and
cannot be loaded in-process, and reimplementing the call site is forbidden here,
so a source assertion is the best available; the AST is what makes it honest.
Four meta-tests pin the analyser itself against synthetic sources, so "this test
can fail" is encoded in the suite rather than only in a commit message: a
commented-out binding reports zero live declarations, a binding reverted to
`raw.tokenCap` is reported, a longhand `tokenCap: raw.tokenCap` in the
registration is reported, and `raw.tokenCap` named only in a comment is NOT a
read. They exercise the analyser, not the call site — nothing here restates
production logic.
Also fixes a smaller fragility: the old test read 'src/main/index.ts' relative to
cwd, so it only passed when run from the repo root. Now resolved from __dirname.
Edge battery added around reqCap, the only guard on the request side:
- every unusable request cap falls through to the manifest — 0, -0, negative,
NaN, ±Infinity, numeric strings ('4000000', '0', '', ' '), null, undefined,
booleans, arrays, plain objects, and an object with a valueOf that would coerce
(it must not be coerced);
- the same unusable values with NO manifest leave the worker uncapped rather than
resolving to a bogus ceiling;
- any positive finite number is a stated ceiling, integer or not (0.5, 1e-6,
MAX_SAFE_INTEGER, MAX_AGENT_TOKEN_CAP) — pinning the current `> 0 && isFinite`
contract explicitly, so tightening it to integers-only has to argue with a test;
- a malformed manifest cap is rejected by validateHireManifest before it can
reach resolveHireDefaults, which trusts `manifest.tokenCap` unchecked. That
guard is load-bearing for the whole fix and was asserted nowhere.
Spec id and MAX_AGENT_TOKEN_CAP are imported, never copied literals.
Proven by mutation, each restored after:
- comment out index.ts:4691 -> RED (was green on all 18 before)
- eff.tokenCap -> raw.tokenCap -> RED
- registration -> tokenCap: raw.tokenCap -> RED
- delete the binding line -> RED
- reqCap drops the `> 0` guard -> RED (3 tests)
- drop `?? manifest?.tokenCap` -> RED (3 tests)
- add a comment naming raw.tokenCap -> stays GREEN (the old regex went RED)
No production change: src/main/hireSpawn.ts and src/main/index.ts are untouched.
No change to MCP consent — a god-authored spawn-request still arms nothing.
typecheck 0 errors; suite 598 of 599, sole failure the known pre-existing
'model picker options stay provider-specific'. hire-spawn.test.cjs 26 of 26,
up from 18.
Owner
Author
|
Closed per Aaron's decision: behaviour abandoned. Branch remains in git if this is ever revisited. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A hire manifest's
tokenCapwas never applied on spawn.docs/hires/manifests/ryan-developer.hire.jsondeclarestokenCap: 4000000and it was silently ignored —index.tsderived the cap solely from the spawn-request JSON.The corollary was worse than the stated bug:
undefinedmeans unlimited, so a spawn-request that omittedtokenCapproduced an uncapped worker even when its manifest declared a ceiling. The safe-looking action — leave the field out and trust the role — was the unsafe one.The fix
resolveHireDefaultsnow resolvestokenCaplike every other hire field:reqCap(raw.tokenCap) ?? manifest?.tokenCap, request-wins-over-manifest, manifest as default.reqCapaccepts only a positive finite number, so0/NaN/"4000000"fall through to the manifest rather than silently cancelling the hire's ceiling.Pipeline
oscar-t062dwight-t062qaThe defect, and what QA did about it
The call-site test verified wiring by grepping
index.ts. I commented out the real line atindex.ts:4691and all 18 tests still passed — the regex matched the commented-out line. Fifth occurrence of this class in this repo.QA deliberately departed from the T-061 precedent. Rather than stripping comment lines, it parses the TypeScript AST, because comment-stripping stays blind to
/* */blocks while an AST carries no comments at all — closing both failure modes structurally. It followed the stronger in-repo precedent (test/renderer-sandbox.test.cjsalready walks the AST to pin the Electron sandbox flag).It also closed the mirror-image failure: the old
assert.doesNotMatch(block, /raw\.tokenCap/)would falsely fail if someone merely mentionedraw.tokenCapin a comment. Both directions are now impossible — I verified each myself.It added four meta-tests running the analyser over synthetic sources, so "this test can fail" is encoded in the suite rather than only in a commit message.
Something QA found that nobody was looking for
resolveHireDefaultstrustsmanifest.tokenCapunchecked. That is only safe becausevalidateHireManifest(src/shared/hire.ts:260-263) requires a positive integer withinMAX_AGENT_TOKEN_CAP. That guard is load-bearing for the whole fix and was asserted nowhere. It is now.Verification (independently reproduced)
index.ts:4691: 25 pass / 1 fail — the AST test catches what the regex missed.raw.tokenCapnamed in a comment: 26 of 26 — no false positive.mainand tracked as T-070.npm run typecheck— 0 errors.Severity, stated honestly:
typecheckdoes catch that mutation (TS18004), so this route could not have silently shipped. The test was still wrong — it claimed to prove wiring and proved nothing — but this was not a near-miss disaster.Scope
tokenCapresolution only. No change to MCP consent: a god-authored spawn-request still arms no write/secret-tier server. The separate decision to give uncapped workers a 10M default (approved) is T-068, deliberately not bundled here.