You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix: full budget-checkpoint coverage across review + adversarial (Codex #2) (#9)
The cost-controller only recorded executor token spend; self-review
(3 persona calls) and the adversarial gate were invisible to both the
budget cap and the reported total — System A could spend ~3x what the
checkpoint saw, and the kill switch couldn't fire mid-task. System B
(papa) recorded an aggregate only AFTER the thinker fan-out, with no
pre-flight check.
Changes:
- CodexCaller contract gains optional token fields; createCodexCaller
parses OpenAI's usage block, createOpenAICompatibleCodexCaller
estimates chars/4 when the server omits usage. The adversarial gate's
spend was previously untracked AT THE SOURCE (the interface returned
only { content }).
- SelfReviewResult + AdversarialReviewResult carry tokenUsage.
selfReview aggregates the three persona calls; adversarialReview
surfaces the codex call's tokens.
- loop.ts records every stage against the checkpoint: forceCheck before
the self-review fan-out, recordAndCheck after self-review and after
adversarial, kill→budget-exceeded on any over-budget result. The
reported totalTokenUsage now sums executor + review + adversarial on
ALL outcome paths (success and failure).
- papa.ts forceChecks before the thinker fan-out so a request already at
the ceiling never launches N parallel calls.
The codex model id is free-form (not a ModelTier); cost-estimator
returns $0 for ids absent from the pricing table (PR #7), so tokens are
tracked even though the codex wire-cost line is $0. Adding codex pricing
is a follow-on.
Also adds docs/design/2026-06-17-criticals-sandbox-and-budget.md — the
design for this fix (#2) AND critical #1 (sandbox hardening, Level 1
chosen). #1 ships as a separate follow-on PR.
Tests: 380 → 384. New: cloud + local codex token surfacing, loop
total-token accounting includes review+adversarial. No skips.
Refs CODEX_REVIEW.md (#2).
`loop.ts` creates one `CostCheckpoint` per task and records spend **only** for `executeTask()` (`loop.ts:~273`). The two downstream LLM stages are unaccounted:
13
+
14
+
-**`selfReview()`** (`self-review.ts:73`) runs three persona calls and returns per-persona `tokenUsage`, but the loop never records it against the checkpoint. Spend is invisible to the budget.
15
+
-**`adversarialReview()`** (`adversarial-gate.ts:16`) is worse: `CodexCaller.call()` returns `{ content }`**only — no token fields at all** (`wiring.ts``createCodexCaller`). The adversarial gate's spend is untracked *at the source*, not merely unrecorded.
16
+
17
+
System B has the same shape: `papa.ts:83` records an aggregate **after** the thinker fan-out completes, with no `forceCheck()` before fanning out — so a budget already near its ceiling still launches N parallel thinker calls.
18
+
19
+
Net effect: System A can spend ~3× (executor + 3 personas + adversarial) what the budget sees, and the kill switch can't fire mid-task. The cost-controller's headline guarantee is partially fictional.
20
+
21
+
### Fix
22
+
23
+
1.**`selfReview(execution, llm, model, checkpoint?)`** — thread the checkpoint in. After each persona call, `checkpoint.recordAndCheck(inTok, outTok, model)`; if it returns `recommendation: 'kill'`, stop the remaining personas and return a partial result flagged `budget-exhausted`. Add a `forceCheck()`*before* the persona fan-out so a task already over budget never starts review.
24
+
25
+
2.**`CodexCaller` gains a token surface.** Change the contract from `{ content }` to `{ content; inputTokens; outputTokens }`. `createCodexCaller` parses OpenAI's `usage` block; `createOpenAICompatibleCodexCaller` reuses the chars/4 estimate the LLM adapter already has. Then `adversarialReview` returns token usage and the loop records it. (This is the one interface change — small, additive, and the OpenAI-compatible side already has the estimator.)
26
+
27
+
3.**`papa.ts`** — `forceCheck()` before the fan-out; record each thinker call individually rather than one aggregate after. The plumbing already passes a checkpoint in, so this is a record-placement change.
28
+
29
+
4.**Loop records every stage.** After self-review and after adversarial, `recordAndCheck`; on `kill`, mark the task `budget-exceeded` and break (same path the executor stage already uses).
30
+
31
+
### Risk / blast radius
32
+
33
+
Low. `selfReview` and `adversarialReview` gain an optional/required param; the analyzer and canary tests that call them with mocks need the extra arg. The `CodexCaller` contract change touches `wiring.ts` (2 factories), `adversarial-gate.ts`, the loop, and their tests. No algorithm changes. ~1 day including tests. This is **implementation-ready** — no open design questions.
34
+
35
+
---
36
+
37
+
## Critical #1 — Execution sandbox hardening
38
+
39
+
### Problem (grounded in code)
40
+
41
+
ASIL runs untrusted code with trusted credentials:
42
+
43
+
-`loop.ts:~217` runs `pnpm install --frozen-lockfile`, then `pnpm -r build`, then (in the executor) `pnpm typecheck` / `pnpm test`, all inside a worktree of the **target** repo.
44
+
-`wiring.ts:~298` (`createCommandRunner`) uses `execFile` with **no `env` option → full parent-process environment inheritance**.
45
+
46
+
So any `postinstall`/`prepare` script, any build step, any test in the target repo runs with `ANTHROPIC_API_KEY`, `OPENAI_API_KEY`, `GITHUB_TOKEN`/gh creds, and the user's entire env in scope — and (until PR #8) with `pnpm install` running lifecycle scripts by default. This is remote-code-execution-with-secret-exfiltration by design for any repo ASIL is pointed at. For a tool whose pitch is "point it at a repo and walk away," that's the headline risk.
47
+
48
+
### Design — layered, choose a depth for v1
49
+
50
+
Four levels, increasing isolation and cost. They compose — each builds on the prior.
51
+
52
+
| Level | What | Stops | Cost to build | New runtime dep |
|**1 — Process hardening**|`--ignore-scripts` on install; **env allowlist** (pass only PATH, HOME, a scrubbed minimal set — never the API keys) to the CommandRunner for target-repo commands; separate the PR-creation credential (gh token) from the execution environment so it's never in scope during install/build/test | secret exfil via scripts; lifecycle-script RCE on install |~1 day, pure code | none |
56
+
|**2 — Containerized exec**| run install/build/test inside a container (Docker/Podman) with `--network=none` for the install+build+test phases, a read-only mount of the worktree except the work dir, and an empty env save the allowlist | network exfil; most filesystem escape; persistent host effects |~3–5 days | container runtime |
57
+
|**3 — microVM / gVisor**| same as 2 but with a VM/syscall-filtering boundary | kernel-level escapes | weeks | firecracker/gVisor |
58
+
59
+
### Recommendation
60
+
61
+
**Ship Level 1 as v1**, document Level 2 as an opt-in (`ASIL_SANDBOX=container`) follow-on, leave Level 3 as a note for adopters with hostile-input threat models.
62
+
63
+
Rationale: Level 1 is pure code, no infra, and removes the **highest-severity, highest-likelihood** vector — credential exfiltration. `--ignore-scripts` already half-landed culturally (PR #8 made install-failure fatal). An env allowlist on the CommandRunner is a contained change. Level 2 is the right *eventual* default for running against genuinely untrusted repos, but forcing a container runtime as a hard dependency now would hurt adoption for the common case (a team running ASIL on its own repo), and it's a clean opt-in later.
64
+
65
+
### Level 1 specifics
66
+
67
+
1.**CommandRunner env allowlist.**`createCommandRunner({ envAllowlist?: string[] })`. When set, `execFile(..., { env: pick(process.env, allowlist) })`. Default allowlist: `PATH`, `HOME`, `LANG`, `TMPDIR`, `npm_config_*` as needed for pnpm. **Never**`*_API_KEY`, `GH_TOKEN`, `GITHUB_TOKEN`. The LLM callers keep their keys because they read them at construction time in the runner *parent* process — the keys never need to be in the *child* (pnpm/git) env.
68
+
2.**`--ignore-scripts` on install** by default; `ASIL_ALLOW_INSTALL_SCRIPTS=1` to opt back in for repos that genuinely need them (rare, and the operator is then explicitly accepting the risk).
69
+
3.**Credential separation for PR creation.**`gh pr create` needs the GitHub token; install/build/test do not. Scope the gh token to only the `createPR` step's env, never the execution steps'. (The git operations that need auth — push — also only need it at push time, not during build.)
70
+
4.**Docs:** a "Running ASIL against untrusted repos" hardening section in the README + `examples/local-llm.md` sibling, stating plainly what Level 1 does and does not protect against, and pointing hostile-input users to Level 2 when it lands.
71
+
72
+
### Risk / blast radius
73
+
74
+
Medium. The env-allowlist change is the sensitive part: strip too much and pnpm/tsc/vitest break in the target repo (e.g., a repo that needs a registry token in `.npmrc` via env). Mitigation: the allowlist is configurable, and we ship a generous-but-secret-free default, with a clear error path when a build fails for missing-env reasons. ~1–1.5 days for Level 1 including tests + docs.
75
+
76
+
---
77
+
78
+
## Sequencing
79
+
80
+
1.**#2 budget coverage** first — implementation-ready, no open questions, and it's a correctness/honesty fix for the cost-controller's core promise.
81
+
2.**#1 Level 1** second — pending the depth decision below.
82
+
83
+
Each ships as its own PR against `main` (branch protection requires PRs).
84
+
85
+
## Open decision (for the user)
86
+
87
+
**How deep should the v1 sandbox go?** Level 1 (process hardening, ships now, no infra) is the recommendation; Level 2 (containerized) is the eventual default for untrusted input but adds a container-runtime dependency. This doc proceeds with Level 1 unless directed otherwise.
0 commit comments