Repository navigation
test: enforce headless token budgets and release comparisons - #255
Merged
Merged
Conversation
byapparov
changed the base branch from
main
to
feat/issue-246-tool-selection-20261021
October 21, 2026 03:54
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| " return spawn(...args)", | ||
| "}", | ||
| "Bun.spawn = checked", | ||
| "globalThis.fetch = Object.assign(async (input, init) => {", |
There was a problem hiding this comment.
🟡 Egress accounting misses non-fetch network APIs.
Suggested change
| "globalThis.fetch = Object.assign(async (input, init) => {", | |
| Either weaken the claim (document that only globalThis.fetch traffic is accounted) or additionally wrap Bun.fetch, node:http/https, node:net, node:tls, node:child_process, Bun.spawnSync and WebSocket in the guard — or run the child under a loopback-only network namespace so egress is impossible rather than merely unobserved. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/guard.ts:11):
Problem: Egress accounting misses non-fetch network APIs
Detail: The preload guard is the sole network-accounting mechanism for the CI capture path: capture.ts fails on network.some(!row.allowed) and count.py rejects an "Unaccounted network ledger" assuming network.jsonl is complete, while the README states "Only loopback fixture HTTP is allowed". But the guard only replaces globalThis.fetch (plus a preconnect stub) and Bun.spawn. Egress via Bun.fetch (a separate binding that still points at the original implementation), node:http/https, node:net/tls, node:child_process, Bun.spawnSync, or native WebSocket is neither logged nor blocked, so a CLI regression or transitive dependency phoning out through those surfaces produces an empty-but-"complete" ledger that silently passes the offline gate. The README's later "not an OS network sandbox" disclaimer lowers severity, but the ledger-completeness claim used as CI proof is still false for these paths.
Suggested fix: Either weaken the claim (document that only globalThis.fetch traffic is accounted) or additionally wrap Bun.fetch, node:http/https, node:net, node:tls, node:child_process, Bun.spawnSync and WebSocket in the guard — or run the child under a loopback-only network namespace so egress is impossible rather than merely unobserved.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The preload guard is the sole network-accounting mechanism for the CI capture path: capture.ts fails on network.some(!row.allowed) and count.py rejects an "Unaccounted network ledger" assuming network.jsonl is complete, while the README states "Only loopback fixture HTTP is allowed". But the guard only replaces globalThis.fetch (plus a preconnect stub) and Bun.spawn. Egress via Bun.fetch (a separate binding that still points at the original implementation), node:http/https, node:net/tls, node:child_process, Bun.spawnSync, or native WebSocket is neither logged nor blocked, so a CLI regression or transitive dependency phoning out through those surfaces produces an empty-but-"complete" ledger that silently passes the offline gate. The README's later "not an OS network sandbox" disclaimer lowers severity, but the ledger-completeness claim used as CI proof is still false for these paths.
"Bun.spawn = checked",
"globalThis.fetch = Object.assign(async (input, init) => {",
" const url = new URL(input instanceof Request ? input.url : String(input))",
" if (process.env.SELECTOR_FIXTURE_CODEX === 'true' && url.origin === 'https://chatgpt.com' && url.pathname === '/backend-api/codex/responses') {",
' appendFileSync(process.env.SELECTOR_FIXTURE_GUARD_LOG,JSON.stringify({origin:url.origin,path:url.pathname,allowed:true,intercepted:true})+"\\n")',| "const spawn = Bun.spawn.bind(Bun)", | ||
| "const checked = (...args) => {", | ||
| " const command = Array.isArray(args[0]) ? args[0] : args[0]?.cmd", | ||
| " if (command && /(?:^|\\/)bun$/.test(String(command[0])) && ['add','install'].includes(command[1])) throw new Error('Fixture forbids dependency installation')", |
There was a problem hiding this comment.
🟡 bun-install ban in fixture guard is trivially bypassed.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/guard.ts:7):
Problem: bun-install ban in fixture guard is trivially bypassed
Detail: The dependency-installation control in the preloaded guard only rejects spawns where argv[0] matches /(?:^|\/)bun$/ and argv[1] is exactly 'add' or 'install'. Trivially bypassed spellings fully visible in the patch: 'bun i' (alias), flags before the subcommand (['bun','--cwd','x','install']), bunx, and — most simply — any other installer or fetcher (npm, curl, wget) which the wrapper passes through unchanged. capture.ts deliberately gives the child SHELL=/bin/bash and /usr/bin:/bin on PATH, so 'bash -c "bun install"' sails through; Bun.spawnSync and node:child_process are not wrapped at all. The guard exists to keep the "offline, no unvetted code" capture honest, so its install ban is advisory only — yet test_protocol.py only exercises the one blocked spelling, giving false assurance.
Suggested fix: Normalize argv before matching (reject when leading -flags are followed by add/install/i, and match bunx), also wrap Bun.spawnSync and node:child_process, or deny any spawn whose argv[0] is not bun itself since the fixture child should never need another interpreter. At minimum document that the ban is best-effort.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The dependency-installation control in the preloaded guard only rejects spawns where argv[0] matches /(?:^|/)bun$/ and argv[1] is exactly 'add' or 'install'. Trivially bypassed spellings fully visible in the patch: 'bun i' (alias), flags before the subcommand (['bun','--cwd','x','install']), bunx, and — most simply — any other installer or fetcher (npm, curl, wget) which the wrapper passes through unchanged. capture.ts deliberately gives the child SHELL=/bin/bash and /usr/bin:/bin on PATH, so 'bash -c "bun install"' sails through; Bun.spawnSync and node:child_process are not wrapped at all. The guard exists to keep the "offline, no unvetted code" capture honest, so its install ban is advisory only — yet test_protocol.py only exercises the one blocked spelling, giving false assurance.
const guard = [
'import { appendFileSync } from "node:fs"',
"const original = globalThis.fetch",
"const spawn = Bun.spawn.bind(Bun)",
"const checked = (...args) => {",
" const command = Array.isArray(args[0]) ? args[0] : args[0]?.cmd",
" if (command && /(?:^|\\/)bun$/.test(String(command[0])) && ['add','install'].includes(command[1])) throw new Error('Fixture forbids dependency installation')",
" return spawn(...args)",
]| "Bun.spawn = checked", | ||
| "globalThis.fetch = Object.assign(async (input, init) => {", | ||
| " const url = new URL(input instanceof Request ? input.url : String(input))", | ||
| " if (process.env.SELECTOR_FIXTURE_CODEX === 'true' && url.origin === 'https://chatgpt.com' && url.pathname === '/backend-api/codex/responses') {", |
There was a problem hiding this comment.
🟡 Guard string carries dead codex branch; duplicated copy.
Suggested change
| " if (process.env.SELECTOR_FIXTURE_CODEX === 'true' && url.origin === 'https://chatgpt.com' && url.pathname === '/backend-api/codex/responses') {", | |
| Import the existing selector-fixture guard module instead of duplicating it as a string; if vendoring is intentional, strip the SELECTOR_FIXTURE_CODEX branch and add a comment tying the copy to its upstream file/commit. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/guard.ts:13):
Problem: Guard string carries dead codex branch; duplicated copy
Detail: guard.ts re-embeds the selector-fixture network guard as a source string, but keeps the codex-interception branch gated on process.env.SELECTOR_FIXTURE_CODEX === 'true' (writing to SELECTOR_FIXTURE_CODEX_LOG). Nothing in this PR ever sets either variable — capture.ts and test_protocol.py set only SELECTOR_FIXTURE_ORIGIN/SELECTOR_FIXTURE_GUARD_LOG — so lines 13–18 are dead code in this harness, and "Selector fixture" env names leak into token-budget evidence semantics. The README claims the guard is "reused from the selector fixture", but this is a byte-copy embedded as a string rather than an import of a shared module, so the two copies will drift when the selector fixture's guard evolves.
Suggested fix: Import the existing selector-fixture guard module instead of duplicating it as a string; if vendoring is intentional, strip the SELECTOR_FIXTURE_CODEX branch and add a comment tying the copy to its upstream file/commit.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
guard.ts re-embeds the selector-fixture network guard as a source string, but keeps the codex-interception branch gated on process.env.SELECTOR_FIXTURE_CODEX === 'true' (writing to SELECTOR_FIXTURE_CODEX_LOG). Nothing in this PR ever sets either variable — capture.ts and test_protocol.py set only SELECTOR_FIXTURE_ORIGIN/SELECTOR_FIXTURE_GUARD_LOG — so lines 13–18 are dead code in this harness, and "Selector fixture" env names leak into token-budget evidence semantics. The README claims the guard is "reused from the selector fixture", but this is a byte-copy embedded as a string rather than an import of a shared module, so the two copies will drift when the selector fixture's guard evolves.
" const url = new URL(input instanceof Request ? input.url : String(input))",
" if (process.env.SELECTOR_FIXTURE_CODEX === 'true' && url.origin === 'https://chatgpt.com' && url.pathname === '/backend-api/codex/responses') {",
' appendFileSync(process.env.SELECTOR_FIXTURE_GUARD_LOG,JSON.stringify({origin:url.origin,path:url.pathname,allowed:true,intercepted:true})+"\\n")',
' appendFileSync(process.env.SELECTOR_FIXTURE_CODEX_LOG,JSON.stringify(JSON.parse(String(init?.body)))+"\\n")',
" const events=[{type:'response.created',response:{id:'resp-fixture',created_at:1,model:'gpt-5.2',service_tier:null}},...]";| raise ValueError('Refuse overwriting accounting results') | ||
| if campaign['protocolVersion'] != 3 or campaign['failures'] or campaign['REPS'] < 2: | ||
| raise ValueError('Incomplete or failed release campaign') | ||
| final = campaign['cleanupChecks'][-1]['finalCampaignCheck'] |
There was a problem hiding this comment.
🟡 KeyError on signal-interrupted campaign.json.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release-count.py:20):
Problem: KeyError on signal-interrupted campaign.json
Detail: release.ts checkpoints campaign.json after every run and in its catch block, but the finalCampaignCheck cleanupChecks entry is only appended by the outer finally. The SIGINT/SIGTERM handlers in release.ts run cleanup() and process.exit(128+n) without checkpointing or appending a failure. An interrupt landing after the last per-run checkpoint (failures==[] and all arms have REPS runs) leaves campaign.json passing release-count.py's protocolVersion/failures/REPS gates, after which campaign['cleanupChecks'][-1]['finalCampaignCheck'] raises an unhandled KeyError traceback instead of the intended ValueError diagnostic — the last entry in that state is a per-run {name, removed, remaining} record with no finalCampaignCheck key.
Suggested fix: Guard the shape before indexing: take the last cleanupChecks entry, verify it is a dict containing 'finalCampaignCheck', else raise ValueError('Campaign interrupted before final cleanup verification'). Optionally have release.ts's signal handler append a failure entry and checkpoint before process.exit.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
release.ts checkpoints campaign.json after every run and in its catch block, but the finalCampaignCheck cleanupChecks entry is only appended by the outer finally. The SIGINT/SIGTERM handlers in release.ts run cleanup() and process.exit(128+n) without checkpointing or appending a failure. An interrupt landing after the last per-run checkpoint (failures==[] and all arms have REPS runs) leaves campaign.json passing release-count.py's protocolVersion/failures/REPS gates, after which campaign['cleanupChecks'][-1]['finalCampaignCheck'] raises an unhandled KeyError traceback instead of the intended ValueError diagnostic — the last entry in that state is a per-run {name, removed, remaining} record with no finalCampaignCheck key.
if campaign['protocolVersion'] != 3 or campaign['failures'] or campaign['REPS'] < 2:
raise ValueError('Incomplete or failed release campaign')
final = campaign['cleanupChecks'][-1]['finalCampaignCheck']
if final['exit'] != 0 or final['timedOut'] or final['stdout'].strip():
raise ValueError('Final campaign container cleanup not verified')| const tools = toolList(json, api) | ||
| const main = tools.length > 0 && body.includes(MARKER) | ||
| const resultEvidence = fixtureEvidence(json, api, this.scenario) | ||
| const done = scriptProgress(resultEvidence) |
There was a problem hiding this comment.
🟡 Aux requests can 400 via scriptProgress throw.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release-fixtures/token-stub.ts:154):
Problem: Aux requests can 400 via scriptProgress throw
Detail: handle() computes `const done = scriptProgress(resultEvidence)` before the request is classified as main vs aux, and scriptProgress throws "Tool results lost all scripted call IDs" whenever any tool results are present but none match ^call_(\d+). An aux request (title generation, summarization) that embeds prior tool outputs whose ids are not the stub's call_N form is rejected with HTTP 400 benchmark_validation_error, logged as an error record, and then fails the whole campaign via release.ts's `requests.some((q) => q.error)` → "Native/stub error" check. This contradicts the stub's documented contract that aux requests "get a short text reply and do not advance the script". The fail-explicit behavior should apply to main requests only.
Suggested fix: Classify first, then only advance for main: `const main = tools.length > 0 && body.includes(MARKER); const done = main ? scriptProgress(resultEvidence) : 0` — keeping the explicit throw for main requests only.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
handle() computes const done = scriptProgress(resultEvidence) before the request is classified as main vs aux, and scriptProgress throws "Tool results lost all scripted call IDs" whenever any tool results are present but none match ^call_(\d+). An aux request (title generation, summarization) that embeds prior tool outputs whose ids are not the stub's call_N form is rejected with HTTP 400 benchmark_validation_error, logged as an error record, and then fails the whole campaign via release.ts's requests.some((q) => q.error) → "Native/stub error" check. This contradicts the stub's documented contract that aux requests "get a short text reply and do not advance the script". The fail-explicit behavior should apply to main requests only.
const tools = toolList(json, api)
const main = tools.length > 0 && body.includes(MARKER)
const resultEvidence = fixtureEvidence(json, api, this.scenario)
const done = scriptProgress(resultEvidence)
const furthest = Math.max(0, ...this.log.filter((r) => r.kind === "main").map((r) => r.step))
if (main && done < furthest) throw new Error(`Tool history regressed from scripted step ${furthest} to ${done}`)| @@ -0,0 +1,87 @@ | |||
| """Account the explicit release comparison with the shared pinned offline counter.""" | |||
There was a problem hiding this comment.
⚪ release-count.py breaks Python snake_case naming.
Suggested change
| """Account the explicit release comparison with the shared pinned offline counter.""" | |
| Rename to release_count.py and update the references (RELEASING.md command, release.ts closing console.log, any README mentions). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release-count.py:1):
Problem: release-count.py breaks Python snake_case naming
Detail: All sibling Python files in script/token-budget/ use snake_case (run.py, setup.py, count.py, reference.py, test_accounting.py, test_protocol.py), but this one is kebab-case release-count.py. Kebab-case matches the repo's TS scripts (release.ts, token-stub.ts), not the Python files it sits beside and shares reference.py with; a kebab name is also unimportable as a Python module, which matters if tests ever want to exercise its analyze() the way test_accounting.py exercises count.analyze(). RELEASING.md and release.ts's closing log line already reference the filename, so renaming gets harder after merge.
Suggested fix: Rename to release_count.py and update the references (RELEASING.md command, release.ts closing console.log, any README mentions).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
All sibling Python files in script/token-budget/ use snake_case (run.py, setup.py, count.py, reference.py, test_accounting.py, test_protocol.py), but this one is kebab-case release-count.py. Kebab-case matches the repo's TS scripts (release.ts, token-stub.ts), not the Python files it sits beside and shares reference.py with; a kebab name is also unimportable as a Python module, which matters if tests ever want to exercise its analyze() the way test_accounting.py exercises count.analyze(). RELEASING.md and release.ts's closing log line already reference the filename, so renaming gets harder after merge.
|
|
||
| function toolList(json: any, api: "chat" | "responses"): Tool[] { | ||
| const raw: any[] = json.tools ?? [] | ||
| return raw.map((t) => |
There was a problem hiding this comment.
⚪ toolList marks all chat tools as function kind.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release-fixtures/token-stub.ts:222):
Problem: toolList marks all chat tools as function kind
Detail: For the chat API, every entry of json.tools is unconditionally mapped to kind "function" with name: t.function?.name possibly undefined for non-function tool types, unlike the responses branch which derives kind from t.type. fixtureCall filters only kind==="function", so a custom-type tool becomes a function candidate with an undefined name; when no read/shell tool exists the thrown error joins undefined into "no read or shell tool in: undefined,...", degrading the 400 diagnostics, and an undefined-named call becomes representable.
Suggested fix: Mirror the responses branch for chat too: kind: t.type === "function" ? ("function" as const) : ("other" as const).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
For the chat API, every entry of json.tools is unconditionally mapped to kind "function" with name: t.function?.name possibly undefined for non-function tool types, unlike the responses branch which derives kind from t.type. fixtureCall filters only kind==="function", so a custom-type tool becomes a function candidate with an undefined name; when no read/shell tool exists the thrown error joins undefined into "no read or shell tool in: undefined,...", degrading the 400 diagnostics, and an undefined-named call becomes representable.
function toolList(json: any, api: "chat" | "responses"): Tool[] {
const raw: any[] = json.tools ?? []
return raw.map((t) =>
api === "chat"
? { name: t.function?.name, params: t.function?.parameters, kind: "function" as const }
: {| import io | ||
| import json | ||
| import os | ||
| from pathlib import Path |
There was a problem hiding this comment.
⚪ Unused import importlib.util in test_accounting.py.
--- a/script/token-budget/test_accounting.py
+++ b/script/token-budget/test_accounting.py
@@ -4,7 +4,6 @@
"""Offline accounting faults using copied real captures and private environment copies."""
import contextlib
-import importlib.util
import io
import json
import os🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/test_accounting.py:7):
Problem: Unused import importlib.util in test_accounting.py
Detail: test_accounting.py imports importlib.util but never uses it: the file imports its sibling modules directly (`import reference`, `import count`) and loads nothing by path. The module-by-path loader pattern belongs to test_protocol.py (its module() helper), making this a copy-paste leftover. Dead imports conflict with the PR's own no-dead-code posture and will trip any future lint of the new Python tree.
Suggested fix: Delete the `import importlib.util` line; all other imports (contextlib, io, json, os, pathlib.Path, shutil, subprocess, sys, tempfile, unittest) are used.
Suggested patch:
--- a/script/token-budget/test_accounting.py
+++ b/script/token-budget/test_accounting.py
@@ -4,7 +4,6 @@
"""Offline accounting faults using copied real captures and private environment copies."""
import contextlib
-import importlib.util
import io
import json
import os
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
test_accounting.py imports importlib.util but never uses it: the file imports its sibling modules directly (import reference, import count) and loads nothing by path. The module-by-path loader pattern belongs to test_protocol.py (its module() helper), making this a copy-paste leftover. Dead imports conflict with the PR's own no-dead-code posture and will trip any future lint of the new Python tree.
"""Offline accounting faults using copied real captures and private environment copies."""
import contextlib
import importlib.util
import io
import json
import os|
|
||
| def test_fixed_workspace_lock_rejects_parallel_campaign_before_output(self): | ||
| RUN.FIXTURE.mkdir(mode=0o700, exist_ok=True) | ||
| fd = os.open(RUN.FIXTURE / 'lock', os.O_CREAT | os.O_RDWR | os.O_NOFOLLOW, 0o600) |
There was a problem hiding this comment.
⚪ Lock test creates shared fixture dir without validation.
Suggested change
| fd = os.open(RUN.FIXTURE / 'lock', os.O_CREAT | os.O_RDWR | os.O_NOFOLLOW, 0o600) | |
| Reuse run.py's fixture validation (or skip with a clear message when /tmp/aictrl-token-budget exists but is not owned/private by the test user), and create the lock via the same guarded os.open pattern after a stat check on FIXTURE. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/test_protocol.py:95):
Problem: Lock test creates shared fixture dir without validation
Detail: test_fixed_workspace_lock_rejects_parallel_campaign_before_output does RUN.FIXTURE.mkdir(mode=0o700, exist_ok=True) and then opens FIXTURE/'lock' with none of the ownership/symlink/mode validation run.py performs (lstat S_ISDIR, st_uid == os.getuid(), no group/other bits; O_NOFOLLOW applies only to the lock file). On a shared host where /tmp/aictrl-token-budget already exists owned by another user, mkdir(exist_ok=True) succeeds silently and the subsequent os.open(O_CREAT|O_RDWR) raises PermissionError, erroring the test instead of exercising the flock rejection path. It also creates the persistent fixed fixture directory as a side effect on machines where it did not exist, unlike every other test which stays in TemporaryDirectory.
Suggested fix: Reuse run.py's fixture validation (or skip with a clear message when /tmp/aictrl-token-budget exists but is not owned/private by the test user), and create the lock via the same guarded os.open pattern after a stat check on FIXTURE.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
test_fixed_workspace_lock_rejects_parallel_campaign_before_output does RUN.FIXTURE.mkdir(mode=0o700, exist_ok=True) and then opens FIXTURE/'lock' with none of the ownership/symlink/mode validation run.py performs (lstat S_ISDIR, st_uid == os.getuid(), no group/other bits; O_NOFOLLOW applies only to the lock file). On a shared host where /tmp/aictrl-token-budget already exists owned by another user, mkdir(exist_ok=True) succeeds silently and the subsequent os.open(O_CREAT|O_RDWR) raises PermissionError, erroring the test instead of exercising the flock rejection path. It also creates the persistent fixed fixture directory as a side effect on machines where it did not exist, unlike every other test which stays in TemporaryDirectory.
def test_fixed_workspace_lock_rejects_parallel_campaign_before_output(self):\n RUN.FIXTURE.mkdir(mode=0o700, exist_ok=True)\n fd = os.open(RUN.FIXTURE / 'lock', os.O_CREAT | os.O_RDWR | os.O_NOFOLLOW, 0o600)\n output = self.directory / 'must-not-exist'| import os | ||
| from pathlib import Path | ||
| import shutil | ||
| import signal |
There was a problem hiding this comment.
⚪ Unused import signal in test_protocol.py.
--- a/script/token-budget/test_protocol.py
+++ b/script/token-budget/test_protocol.py
@@ -8,7 +8,6 @@
import json
import os
from pathlib import Path
import shutil
-import signal
import subprocess
import sys🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/test_protocol.py:9):
Problem: Unused import signal in test_protocol.py
Detail: test_protocol.py imports `signal` but nothing in the file references it: the kill/process-group assertions use os.killpg and RUN.execute/RUN.verify (run.py has its own `import signal` for SIGKILL). Leftover from copying run.py's import block; same dead-import class as the unused importlib.util in test_accounting.py.
Suggested fix: Remove `import signal` from test_protocol.py's import block.
Suggested patch:
--- a/script/token-budget/test_protocol.py
+++ b/script/token-budget/test_protocol.py
@@ -8,7 +8,6 @@
import json
import os
from pathlib import Path
import shutil
-import signal
import subprocess
import sys
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
test_protocol.py imports signal but nothing in the file references it: the kill/process-group assertions use os.killpg and RUN.execute/RUN.verify (run.py has its own import signal for SIGKILL). Leftover from copying run.py's import block; same dead-import class as the unused importlib.util in test_accounting.py.
import ctypes
import fcntl
import importlib.util
import json
import os
from pathlib import Path
import shutil
import signal
import subprocess
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 8 · ⚪ 8 · 0/16 resolved
🤖 Fix all 16 open findings with your agent📋 Out-of-diff findings (16)
Reviewed 19 files · 0 inline · view all 16 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
Review response — PR #255Responding to this review. Judged against Issues addressed (pushed to this PR)
Review claims verified false (no change needed)Not addressed here
Optional observations retained without a code change:
Verification27 Python tests/zero skipped and four stub tests/20 assertions pass, with standalone stub and CLI typechecks. Actual baseline escaped-child/grandchild and zombie-only misclassification controls reproduce; the candidate rejects live escapees after cleanup and accepts zombie-only cases after reaping. Auxiliary foreign-history and five incomplete-manifest controls also distinguish baseline from candidate. Independent final review accepted all seven source pins; normal commit/push hooks passed (six cached typecheck tasks). Current-head hosted CI and automated review are still pending. No token benchmark or paid model run was performed. |
byapparov
changed the base branch from
feat/issue-246-tool-selection-20261021
to
main
October 21, 2026 11:44
byapparov
changed the base branch from
main
to
feat/issue-246-tool-selection-20261021
October 21, 2026 11:52
| } | ||
| } | ||
| for (const signal of ["SIGINT", "SIGTERM"] as const) | ||
| process.on(signal, () => { |
There was a problem hiding this comment.
🟡 Signal exit skips checkpoint; campaign.json lost or stale.
Suggested change
| process.on(signal, () => { | |
| In the signal handler, append an interrupt entry to failures and call checkpoint() inside the cleanup().finally() before process.exit; also make checkpoint() atomic (write campaign.json.tmp then renameSync) so an interrupt during the write cannot leave a truncated file. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release.ts:191):
Problem: Signal exit skips checkpoint; campaign.json lost or stale
Detail: The SIGINT/SIGTERM handlers run cleanup() and then process.exit() without calling checkpoint(), so an interrupt during preflight or between per-run checkpoints leaves the output directory without any campaign.json, and an interrupt mid-campaign leaves a stale checkpoint whose failures array does not record the interrupt (downstream release-count.py then reports a generic incomplete-campaign error instead of the abort). The evidence logs are retained, but the campaign summary the harness promises to leave "including failures" is lost, and because mkdirSync refuses existing directories the partial campaign can never be re-completed in place.
Suggested fix: In the signal handler, append an interrupt entry to failures and call checkpoint() inside the cleanup().finally() before process.exit; also make checkpoint() atomic (write campaign.json.tmp then renameSync) so an interrupt during the write cannot leave a truncated file.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The SIGINT/SIGTERM handlers run cleanup() and then process.exit() without calling checkpoint(), so an interrupt during preflight or between per-run checkpoints leaves the output directory without any campaign.json, and an interrupt mid-campaign leaves a stale checkpoint whose failures array does not record the interrupt (downstream release-count.py then reports a generic incomplete-campaign error instead of the abort). The evidence logs are retained, but the campaign summary the harness promises to leave "including failures" is lost, and because mkdirSync refuses existing directories the partial campaign can never be re-completed in place.
for (const signal of ["SIGINT", "SIGTERM"] as const)
process.on(signal, () => {
void cleanup().finally(() => process.exit(128 + (signal === "SIGINT" ? 2 : 15)))
})
try {
const image = JSON.parse(await docker(["image", "inspect", input.fixtureImage]))[0]| child.exited, | ||
| ]) | ||
| if (code || timeout) { | ||
| await cleanup() |
There was a problem hiding this comment.
🟡 cleanup() throw masks docker preflight stderr diagnostic.
--- a/script/token-budget/release.ts
+++ b/script/token-budget/release.ts
@@ -141,8 +141,9 @@ async function docker(args: string[]) {
])
if (code || timeout) {
- await cleanup()
- throw new Error(`Docker preflight failed (${code}, timeout=${timeout}): ${err}`)
+ const failure = new Error(`Docker preflight failed (${code}, timeout=${timeout}): ${err}`)
+ await cleanup().catch(() => {})
+ throw failure
}
return out🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release.ts:144):
Problem: cleanup() throw masks docker preflight stderr diagnostic
Detail: In docker()'s failure branch, `await cleanup()` runs before throwing the preflight error. cleanup() itself throws on any unverified removal (ps exit != 0, leftover container rows, rm error other than "No such container"), and those conditions correlate with docker being broken — the same condition that failed the preflight. When both fail, cleanup's exception replaces the intended `Docker preflight failed (code, timeout): stderr` message, so campaign.json records only the cleanup error and the docker stderr explaining the preflight failure (image/platform mismatch, missing image, etc.) is discarded.
Suggested fix: Build the preflight error first, tolerate cleanup failures on this path (cleanupChecks already records the attempt), then rethrow the preflight error so the docker stderr survives into campaign.json.
Suggested patch:
--- a/script/token-budget/release.ts
+++ b/script/token-budget/release.ts
@@ -141,8 +141,9 @@ async function docker(args: string[]) {
])
if (code || timeout) {
- await cleanup()
- throw new Error(`Docker preflight failed (${code}, timeout=${timeout}): ${err}`)
+ const failure = new Error(`Docker preflight failed (${code}, timeout=${timeout}): ${err}`)
+ await cleanup().catch(() => {})
+ throw failure
}
return out
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
In docker()'s failure branch, await cleanup() runs before throwing the preflight error. cleanup() itself throws on any unverified removal (ps exit != 0, leftover container rows, rm error other than "No such container"), and those conditions correlate with docker being broken — the same condition that failed the preflight. When both fail, cleanup's exception replaces the intended Docker preflight failed (code, timeout): stderr message, so campaign.json records only the cleanup error and the docker stderr explaining the preflight failure (image/platform mismatch, missing image, etc.) is discarded.
if (code || timeout) {
await cleanup()
throw new Error(`Docker preflight failed (${code}, timeout=${timeout}): ${err}`)
}
return out
} finally {
clearTimeout(timer)
}| proc = child | ||
| const timer = setTimeout(() => { | ||
| timeout = true | ||
| void cleanup().catch((error) => { |
There was a problem hiding this comment.
🟡 Timeout path races two concurrent cleanups.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release.ts:325):
Problem: Timeout path races two concurrent cleanups
Detail: When the 180s capture timeout fires, the timer starts cleanup() without awaiting it (void cleanup().catch(...)), and the docker CLI's subsequent exit then triggers the finally block's second `await cleanup()`. Two concurrent cleanup() instances each run `docker rm -f` followed by `docker ps -aq --filter name=...`; one instance's ps can observe the container while the other's rm is still in flight, so remaining.stdout is non-empty and cleanup throws "Unverified container cleanup" even though removal was proceeding. On a run that already failed via timeout this converts the diagnostic into a misleading cleanup failure (and can overwrite cleanupFailure), making the timeout's root cause harder to read in campaign.json.
Suggested fix: Make cleanup() a singleton promise (memoize the in-flight run, e.g. `inflightCleanup ??= doCleanup().finally(() => { inflightCleanup = undefined })`) so the timer's fire-and-forget call and the finally block's await share one rm/ps sequence instead of racing.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
When the 180s capture timeout fires, the timer starts cleanup() without awaiting it (void cleanup().catch(...)), and the docker CLI's subsequent exit then triggers the finally block's second await cleanup(). Two concurrent cleanup() instances each run docker rm -f followed by docker ps -aq --filter name=...; one instance's ps can observe the container while the other's rm is still in flight, so remaining.stdout is non-empty and cleanup throws "Unverified container cleanup" even though removal was proceeding. On a run that already failed via timeout this converts the diagnostic into a misleading cleanup failure (and can overwrite cleanupFailure), making the timeout's root cause harder to read in campaign.json.
const child = Bun.spawn(argv, { env: hostEnv, stdin: "ignore", stdout: "pipe", stderr: "pipe" })
proc = child
const timer = setTimeout(() => {
timeout = true
void cleanup().catch((error) => {
cleanupFailure = String(error)
})
}, TIMEOUT)| ownedContainer, | ||
| "--label", | ||
| `aictrl.harness-benchmark=${campaignId}`, | ||
| "--network", |
There was a problem hiding this comment.
🟡 Release capture runs unvetted binaries on host network.
Suggested change
| "--network", | |
| Replace `--network host` for capture runs with a dedicated bridge network plus `--add-host host.docker.internal:host-gateway`, and point the stub baseURL at host.docker.internal (bind the stub to that address), so only the stub port is reachable from the container while keeping the loopback-only property the README claims. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/release.ts:296-297):
Problem: Release capture runs unvetted binaries on host network
Detail: Every capture container runs the binary under test (including the previously published npm artifact, which is hash-verified but not code-reviewed) with `--network host`, permission {"*":"allow"} and no preload guard. A misbehaving or compromised release binary therefore gets unrestricted host-network egress on the operator machine. The README and metadata.network acknowledge this is not an egress sandbox, but a loopback-only alternative exists: a bridge network with `--add-host host.docker.internal:host-gateway` (or an equivalent allowlisted network) lets the container reach the stub on the host without full host networking, matching the isolation the preflight already enforces with `--network none`.
Suggested fix: Replace `--network host` for capture runs with a dedicated bridge network plus `--add-host host.docker.internal:host-gateway`, and point the stub baseURL at host.docker.internal (bind the stub to that address), so only the stub port is reachable from the container while keeping the loopback-only property the README claims.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every capture container runs the binary under test (including the previously published npm artifact, which is hash-verified but not code-reviewed) with --network host, permission {"*":"allow"} and no preload guard. A misbehaving or compromised release binary therefore gets unrestricted host-network egress on the operator machine. The README and metadata.network acknowledge this is not an egress sandbox, but a loopback-only alternative exists: a bridge network with --add-host host.docker.internal:host-gateway (or an equivalent allowlisted network) lets the container reach the stub on the host without full host networking, matching the isolation the preflight already enforces with --network none.
"--rm",
"--pull=never",
"--name",
ownedContainer,
"--label",
`aictrl.harness-benchmark=${campaignId}`,
"--network",
"host",
"--user",
"10001:10001",
"--cpus",| (out / 'controller-result.json').write_text(json.dumps(result, indent=2) + '\n') | ||
| verify(result) | ||
| records.append(result) | ||
| (output / 'campaign.json').write_text(json.dumps({'runs': records, 'workspace': str(WORKSPACE), |
There was a problem hiding this comment.
🟡 run.py writes campaign.json only after all runs pass.
Suggested change
| (output / 'campaign.json').write_text(json.dumps({'runs': records, 'workspace': str(WORKSPACE), | |
| Rewrite campaign.json incrementally after each completed capture (records.append followed by an atomic temp-file + rename write, mirroring release.ts's checkpoint()), and also write it on the failure path before re-raising, so partial campaigns remain countable as evidence. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/run.py:283):
Problem: run.py writes campaign.json only after all runs pass
Detail: run.py writes campaign.json exactly once, after every arm/repeat has succeeded. Any mid-campaign failure (verify() rejecting run 3 of 4, an execute() OSError, a KeyboardInterrupt) leaves the evidence directory with per-run controller-result.json files but no campaign.json — and since private_directory() refuses an existing output directory, the partial multi-minute campaign can never be resumed or recounted without manual surgery. This is inconsistent with release.ts, which checkpoints campaign.json after every run and on every failure path so partial evidence stays indexable.
Suggested fix: Rewrite campaign.json incrementally after each completed capture (records.append followed by an atomic temp-file + rename write, mirroring release.ts's checkpoint()), and also write it on the failure path before re-raising, so partial campaigns remain countable as evidence.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
run.py writes campaign.json exactly once, after every arm/repeat has succeeded. Any mid-campaign failure (verify() rejecting run 3 of 4, an execute() OSError, a KeyboardInterrupt) leaves the evidence directory with per-run controller-result.json files but no campaign.json — and since private_directory() refuses an existing output directory, the partial multi-minute campaign can never be resumed or recounted without manual surgery. This is inconsistent with release.ts, which checkpoints campaign.json after every run and on every failure path so partial evidence stays indexable.
verify(result)
records.append(result)
(output / 'campaign.json').write_text(json.dumps({'runs': records, 'workspace': str(WORKSPACE),
'raw_normalization': False, 'repeats': args.repeats, 'source_entry': str(args.source.resolve())}, indent=2) + '\n')
subprocess.run([sys.executable, '-I', '-B', str(ROOT / 'count.py'), str(output),| from unittest import mock | ||
|
|
||
| ROOT = Path(__file__).resolve().parent | ||
| BUN = shutil.which('bun') |
There was a problem hiding this comment.
⚪ Missing bun makes Protocol tests crash, not skip.
--- a/script/token-budget/test_protocol.py
+++ b/script/token-budget/test_protocol.py
@@ -24,6 +24,7 @@ RUN = module('budget_run', ROOT / 'run.py')
+@unittest.skipUnless(BUN, 'bun required for owned child controls')
class Protocol(unittest.TestCase):
@classmethod
def setUpClass(cls):🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #255, script/token-budget/test_protocol.py:17):
Problem: Missing bun makes Protocol tests crash, not skip
Detail: BUN = shutil.which('bun') has no None guard and no skip decorator, but Protocol.setUp immediately evaluates Path(BUN).parent and several tests spawn [BUN, ...]. On a host without bun on PATH, every Protocol test errors with TypeError("argument should be a str or an os.PathLike object ... not NoneType") instead of an actionable prerequisite message — contrast run.py's explicit "Require real bun and ripgrep on PATH" diagnostic for the same prerequisite.
Suggested fix: Decorate the Protocol class with @unittest.skipUnless(BUN, 'bun required for owned child controls') (or raise unittest.SkipTest with that message in setUpClass) so a missing prerequisite is reported as a clear skip instead of a TypeError storm.
Suggested patch:
--- a/script/token-budget/test_protocol.py
+++ b/script/token-budget/test_protocol.py
@@ -24,6 +24,7 @@ RUN = module('budget_run', ROOT / 'run.py')
+@unittest.skipUnless(BUN, 'bun required for owned child controls')
class Protocol(unittest.TestCase):
@classmethod
def setUpClass(cls):
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
BUN = shutil.which('bun') has no None guard and no skip decorator, but Protocol.setUp immediately evaluates Path(BUN).parent and several tests spawn [BUN, ...]. On a host without bun on PATH, every Protocol test errors with TypeError("argument should be a str or an os.PathLike object ... not NoneType") instead of an actionable prerequisite message — contrast run.py's explicit "Require real bun and ripgrep on PATH" diagnostic for the same prerequisite.
ROOT = Path(Path(__file__).resolve().parent)
BUN = shutil.which('bun')
def module(name, path):
spec = importlib.util.spec_from_file_location(name, path)
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 5 · ⚪ 1 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 20 files · 0 inline · view all 6 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
PR255 review response — six follow-up findingsReview
Verification: actual-source baseline defects were reproduced and the accepted12 candidate fixture scenarios passed, including interruption during preflight/capture/final inspection, prior-primary plus signal, cleanup-only and expedited production timeout errors, cleanup identity/coalescing/retry, and a normal protocol path. The normal fixture made32 real HTTP calls to the unchanged Stub/eight valid samples. That proof is reused only for exact unchanged release entry/import/helper and private-harness pins: all533 private evidence entries and four source/import/helper files were reverified. The12 lifecycle controls were not rerun for this parent integration. Seven accepted files retain680 bytes and Git modes. Limits: private fake Docker and dummy binaries; emulated tool messages, no actual native Read-tool/release qualification. Timer callback expedited from180seconds to0. The published normal merge has parents e6762b3 and accepted25446f; root read back all615 committed blobs/Git modes/link payloads. Eight fresh parent/CI checks passed: four direct noEmit compilers, four actual cold prefix captures (3441/2001 reference prefixes),27 Python tests and four actual stub tests. Root verified2593 new QA evidence entries and independent source review. Normal push hooks passed; generation/plugin-build lines in cached output are replayed logs, not new generation. Fresh CI39617644074 passed2,257tests/10skips/0failures across156files. Actual immutable-log checkout dac1821 has parents46f+ef49 and the exact published tree; all615 checkout blobs/Git modes match. The original six-poll window finished at15:02:09UTC (scheduled cap15:02:06UTC) with zero current-head formal reviews and no active AI check observed. CodeQL is untriggered because this stacked PR targets an unprotected feature branch under GitHub default-setup policy; current-head security clearance remains open. Production logs separately record this PR reaching its configured two-execution automatic-review ceiling, without a logged head SHA. No retry, counter reset or limit change was made. Earlier680 review success does not verify this new head. No model billing/quality, egress-sandbox, receiver, C1 or release clearance. The parent integration also preserves read-only workflow permissions, removes only the main-only PR target filter so existing CI can run on stacked bases, and restores the required stdin marker in the documentation example. No runtime permission or experiment-network policy was broadened. |
Base automatically changed from
feat/issue-246-tool-selection-20261021
to
main
October 21, 2026 17:32
…ate-20261021 ci: verify the integrated headless CLI candidate
This was referenced Oct 22, 2026
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
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.
Closes #248
Intent
Fail CI when a source change increases the CLI's fixed system-prompt/tool-definition overhead beyond an explicitly reviewed baseline. This protects earlier prompt reductions from accidental regression.
“Token budget” means a threshold in this repository's tests. It does not impose a token allowance or spending cap on users, trim prompts, or add an
aictrl run --token-budgetoption.Origin and scope
#160 requested a fixed-agent/model prompt-size regression test. #248 expanded it into two maintenance tasks: fixed-token CI thresholds and a comparison against the previous release. The existing 15,000-character Qwen guard remains; this PR adds a check of actual SDK requests using a pinned reference tokenizer.
Those are engineering-maintenance requirements, not evidence of customer demand for a token-budget feature. The larger release-comparison controller is a separate scope decision bundled with the CI guard.
Expected Impact on Users
Branch context: #257 merged into this branch, so this PR now also carries its combined CLI tests, instruction-isolation fix and documentation corrections. #257 describes those runtime changes. They are separate from budget tooling. This PR remains open against
main; no release is implied.Expected Outcomes
Automatic CI guard: what an engineer should expect
The
Enforce actual SDK prefix token budgetsstep starts the actual CLI against a local scripted provider, twice per configuration:defaultbash,edit,glob,grep,read,skill,task,webfetch,writefourread,bash,edit,write, selected with--toolsThe thresholds in
budgets.jsonequal the measured baseline, with zero headroom. They are controlled fixture counts, not universal limits for all models or user configurations.Example: if extending a tool description increases the default fixture to 3,500 tokens, CI fails with a diagnostic such as:
This is an illustrative failure, not a new failing test run. At or below the threshold passes the size check. An invalid/incomplete capture, unexpected catalog or differing prefix bytes between repetitions also fails.
What to do on failure: inspect
reference-counts.jsonand the raw requests in the uploadedtoken-budget-evidenceartifact. Remove accidental growth, or explicitly edit the relevant maximum inbudgets.jsonwith evidence and a justification. CI never automatically increases the limit. Even useful above-baseline growth needs review; whether that strict policy is desirable remains a decision on this PR.Exactly what is counted
The metric is pinned
tiktoken 0.12.0 / o200k_baseencoding of compact, sorted JSON containing the actual SDK system messages and serialized tool schemas, plusinstructionsif present. It excludes the fixture's user message, conversation history and generated answers. Full first-input counts are reported separately and are not the enforced thresholds. Separately tokenized system/tool diagnostics are not additive.The fixture fixes the
buildagent, SDK/provider shape, tools, working directory, date and HOME/XDG state; repeated captures must have identical prefix bytes. Setup downloads pinned Python/tokenizer data. Capture/accounting uses a scripted local model, with no paid inference or provider credentials. The result measures reference overhead, not provider bills, cache savings, speed or answer quality.Manual release comparison: a different check
release.tsandrelease-count.pycompare reviewed current/previous native binaries on the same scripted short task: four model calls and three actual Read results. They producerelease-comparison.jsonwith first-input and session input/output/total deltas. They do not reuse the 3,441/2,001 source-CI thresholds as release limits. Numerical deltas require maintainer review.The maintainer supplies binary version/hash/provenance records and an existing immutable Docker fixture image. The controller does not acquire artifacts, build/pull the image or publish a release. A previous binary without
--toolshas its minimal arm reported as unsupported.Implementation
capture.ts/run.py: actual CLI request capture, fixed-environment repetitions, deadlines and owned-process cleanup.reference.py/count.py/budgets.json: pinned offline accounting and threshold enforcement.RELEASING.mdadds the separate manual comparison step.Run the CI guard locally
From the repository root on Linux x86_64, with Python 3.12 (
venv/pip), the pinned Bun, Git/ripgrep, installed frozen JS dependencies and built SDK. Both output directories must be new:README.mddocuments prerequisites, artifacts and controls.--observereports without enforcing limits for evaluation; CI does not use it.Run the separate release comparison
After preparing the binary/image input manifest, from the repository root:
Scope Caveat
Review the CI baseline policy and release-controller scope separately. Passing technical checks does not justify every part of that machinery. These fixed Linux/provider/tool fixtures do not represent every model, plugin or workflow. Instrumented network/process checks are partial fixture controls, not OS isolation. Real-model quality, supporting receiver/lifecycle acceptance and new Windows/native release qualification remain separate gates.
Test Plan
Verification
CI39771028191 passed on current head
44ae6b6db4c09ecc88ba5290c7122eb2abfd9f85:3441/3441default and2001/2001four-tool, no violations. Full first-input counts, separately reported:3453/2013.Existing CI results were rechecked for this wording update; no new suite, Docker build, benchmark or model run was started. Prior review dispositions and local evidence retain their original revision context below.
Risks and Rollout
Strict thresholds can block intentional prompt/schema growth. The guard adds Python/tokenizer/fixture maintenance to CI; the release comparison adds a maintainer operation. Assess those costs against the existing simpler character guard. This description update does not change code, merge the PR or publish a release.
Earlier-head implementation and verification history
The previous description is retained below for evidence continuity. Its head and security/review status refer to
ef492d7before #257 merged; use the current Verification section above for44ae6b6.Closes #248
Intent and expected user impact
Contributors get a repeatable CI failure when controlled prompt/tool prefixes grow beyond reviewed reference-token budgets. Release reviewers get a separate current-versus-previous native comparison, with interruption checkpoints and primary/cleanup failures retained instead of a false success or lost diagnostic.
The existing full CI workflow now accepts stacked PR bases. A fresh run has registered for this correction on its actual parent branch; no temporary retargeting is used. Reference-token counts measure controlled harness overhead, not provider charges, cache savings or task quality.
Behavior
o200k_baseprefix ceilings: 3,441 default and 2,001 forread,bash,edit,write. Increasing a ceiling requires an explicit reviewed source edit. The existing character guard remains.Integration
Current head:
ef492d749b782de17a24b2ac208a0bed007e2f14. Normal merge parents are the accepted release correctione6762b3eand current selector parent46f98d59; the tree isc1a8538a. All 615 committed blobs, Git modes and literal symlink payloads match the reviewed freeze; the owned worktree is clean.The parent integration preserves the coordinated CLI/SDK/plugin contracts. Two bounded additional corrections remove only CI's PR target-branch filter while retaining
push: main,contents: read, jobs and pinned actions, and restore the explicit stdin marker in the CLI README example. Seven previously accepted PR255 feature files and the correctedrelease.tsretain their exact accepted bytes/modes.Verification
dac18214with parents46f98d59+ef492d7; its tree exactly equals the published head, with all615 checkout blobs/Git modes matching the accepted freeze. The budget gates and27 fault controls passed. This is an actual stacked-base run, with no temporary retargeting.Evidence limits and rollout
Local source checks used an owned scripted loopback provider and offline reference tokenizer. Matching canonical SDK outputs were reused after source/input/donor proof; no new SDK generation/build was run in that QA phase. This is not model quality, provider billing, cache or speed evidence.
The native release experiment explicitly uses reviewed artifacts and controlled host-network conditions. Instrumented fetch/spawn guards are partial fixture controls, not OS egress isolation. Supported attached receiver/lifecycle, Windows and all18 real paired C1 quality trials remain open. No merge, release, deployment or marketing publication is requested.
Historical verification
Earlier native comparison and CI39540628032 apply to the earlier ddda435 candidate; its2,021-test result and native token/session deltas do not qualify ef492d7. The680 corrective head's original30-minute window ended before formal review5599497012 arrived at12:16:42UTC. That review's six findings are now handled above. Prior temporary-base/checkout evidence remains historical; this correction retains its intended stacked base and uses the corrected ordinary CI trigger.