4th-review remediation (FR4-01..15): settlement-ordering billing fixes + isolation + stricter controls

.github/trigger-deploy

@@ -30,3 +30,6 @@ f21-ratelimit e1f992f
 f13-f21-ci 669033a
 audit-hardening 94b7fcb
 guard-falsepos-fix 1f8ea44
+fourth-review 59e2782
+gateway-cache-settle c90565e
+stricter-form 6985691

.github/trigger-test

@@ -45,3 +45,6 @@ f21-ratelimit e1f992f
 f13-f21-ci 669033a
 audit-hardening 94b7fcb
 guard-falsepos-fix 1f8ea44
+fourth-review 59e2782
+gateway-cache-settle c90565e
+stricter-form 6985691

.github/workflows/deploy.yml

@@ -927,8 +927,8 @@ jobs:
       - name: Bounded rate-limit maps + global waitlist ceiling (F21 — stale one-time-IP keys swept, distributed flood capped — offline)
         run: node scripts/test-rate-sweep.js
 
-      - name: Idempotent render credit (F13 — capacity-refused paid render retried without a second charge; single-use, TTL, eviction — offline)
-        run: node scripts/test-render-credit.js
+      - name: Settlement-aware idempotency (FR4-01 — a non-200/unsettled response is NOT cached; @x402/express settles AFTER the handler)
+        run: node scripts/test-idempotency-settlement.js
 
       - name: /health envelope + .flags shape (boolean discipline locks the Yahoo-relay-flap class)
         run: node scripts/test-health-envelope.js
@@ -1133,6 +1133,9 @@ jobs:
       - name: Tollbooth atomic replay + origin binding (F05/F18 — Durable Object single-use claim + PoW origin/method binding — offline)
         run: node scripts/test-tollbooth-replay.js
 
+      - name: Tollbooth verifier timeout/cancellation (FR4-10 — x402VerifierFromExpress honors the AbortSignal; a late settle can't grant — offline)
+        run: node scripts/test-tollbooth-verifier.js
+
       - name: Tollbooth dashboard runtime smoke (boots CLI, hits /__tollbooth + /__tollbooth/stats over HTTP)
         run: node scripts/test-tollbooth-runtime.js
 

CLAUDE.md

@@ -66,8 +66,21 @@ because /v1 settles before the handler and an empty balance = charged-but-failed
 - Unit/offline: `scripts/test-memory.js`, `test-find.js`, `test-revenue-scan.js`, `test-util-kit.js`, `test-discovery.js`, `tollbooth/test.js`+`edge.test.js`+`features.test.js`.
 - Raise the MCP free-tier limit for sweeps: `AGENT402_MCP_MAX_PER_MIN=999999 AGENT402_MCP_MAX_PER_HOUR=9999999`.
 
+## x402 settlement ordering (CRITICAL — get this right)
+The installed **`@x402/express` v2.16 runs the handler FIRST, then settles**, and
+ONLY settles a `<400` response — for any handler `statusCode >= 400` it CANCELS
+settlement (`reason: "handler_failed"`) so the buyer is **NOT charged**; if
+settlement of a `<400` response fails, it discards the buffered body and returns a
+402. So: **a 4xx/5xx (incl. a capacity 503 or an upstream 502) is never charged**,
+and a 200 is only charged if settlement then succeeds. Do NOT assume "settles
+before the handler" (an earlier, wrong belief that produced the F13 free-render
+bypass and the pre-settlement idempotency cache — both since fixed). Anything that
+caches, credits, or bills based on handler status BEFORE settlement is unsafe;
+key such logic off the FINAL (post-settlement) response, e.g. `res.on("finish")`
+with `res.statusCode === 200`. (`node_modules/@x402/express/dist/esm/index.mjs`.)
+
 ## Notable features (current)
-- **Idempotency:** opt-in `Idempotency-Key` header; cache key = `sha256(METHOD /path + key + gate-credential)`; replays a paid result without re-charging; no-op without the header. Hooks `res.json` only — streamed responses are never replayable.
+- **Idempotency:** opt-in `Idempotency-Key` header; cache key = `sha256(METHOD /path + key + gate-credential)`. **Settlement-aware (FR4-01):** the body is captured at `res.json` but COMMITTED to the cache only on `res.on("finish")` when the FINAL `statusCode === 200` — i.e. after `@x402/express` has settled — so an unsettled 200 (settlement-failure → 402) is never cached/replayed. No-op without the header; streamed responses are never replayable. `scripts/test-idempotency-settlement.js`.
 - **Tollbooth:** charge modes (`bots`/`all`/`strict`), adaptive PoW, analytics (`gate.stats()` + `/__tollbooth/stats` + `/__tollbooth` dashboard), deploy templates (Cloudflare/Next.js/Docker). Defaults preserve original behavior.
 - **Buyer SDK (`agent402-client`):** `find()` + `call()` with auto-payment (PoW free / x402 paid), caching, idempotent retries, non-custodial.
 - **LLM gateway (`src/tools/llm-gateway-kit.js`, OpenAI wire paths):** five tiers —

docs/audit-closeout-f01-f25.md

@@ -5,8 +5,9 @@ is one of: **Fixed** (shipped in code, live), **Accepted** (won't-fix with
 rationale), or **Owner action** (cannot be closed from this repo — needs a
 GitHub-admin, DNS/registrar, or product decision; exact steps below).
 
-As of this writing, 22 of 25 are Fixed (F13 flag-gated OFF), 1 is Accepted, and 2 need owner action
-(plus the owner half of F01).
+As of this writing (post 4th review): 22 of 25 Fixed, F13 REMOVED (its premise was
+wrong — see the 4th-review section below), F22 Accepted, and F17/F25 + the owner
+half of F01 need owner action.
 
 ## Ledger
 
@@ -24,7 +25,7 @@ As of this writing, 22 of 25 are Fixed (F13 flag-gated OFF), 1 is Accepted, and
 | F10 | Med | Fixed | waitlist drops PII fallback, fails closed |
 | F11 | Med | Fixed | route-aware Postgres TLS (`src/db-ssl.js`) |
 | F12 | Med | Fixed | E2B output cap + concurrency ceiling |
-| F13 | Med | Fixed (code, flag-gated OFF) | idempotent render credit (`src/render-credit.js`); enable with `RENDER_CREDIT_ENABLED` |
+| F13 | Med | **REMOVED (was wrong)** — see §4th-review FR4-02 | the render-credit premise (a charged 503) can't occur under `@x402/express`; feature deleted |
 | F14 | Med | Fixed | MCP abort + await-before-release |
 | F15 | Med | Fixed | PostHog proxy timeout + concurrency + byte cap |
 | F16 | Med | Fixed | `Dockerfile.mcp` digest-pinned + non-root |
@@ -42,10 +43,9 @@ As of this writing, 22 of 25 are Fixed (F13 flag-gated OFF), 1 is Accepted, and
 
 ## Owner-action items — exact steps
 
-These need an owner action from outside the repo (GitHub-admin, DNS/registrar, or
-flipping a reviewed prod flag). Each is copy-paste ready. F13 is fully built and
-only needs its flag enabled; F01 (owner half), F17, and F25 are the genuine
-external actions.
+These need an owner action from outside the repo (GitHub-admin, DNS/registrar).
+Each is copy-paste ready. (F13 is no longer here — it was removed; see the
+4th-review section.)
 
 ### F01 (owner half) — GitHub branch rulesets + protected environment
 
@@ -67,35 +67,6 @@ jobs): add a required reviewer (yourself is fine), set the deployment-branch
 policy to allow `main` **and** `claude/sweet-brown-i99jl3`, and move the
 deploy/publish secrets to env scope.
 
-### F13 — render capacity refused after settlement — BUILT (approach B), flag-gated OFF
-
-Approach **B (idempotent durable credit)** is implemented in `src/render-credit.js`
-and wired into the paywall in `src/server.js`, **flagged OFF by default** so
-billing is byte-identical until enabled. On a capacity refusal (503) of a
-*settled* paid render/screenshot, the server mints a one-time **credit token** (a
-256-bit bearer secret) returned only in that buyer's own 503 response
-(`X-Render-Credit` header + JSON `renderCredit`), bound to the exact request
-(route + body). A retry presenting the token via `X-Render-Credit` skips the gate
-(PoW + replay guard + USDC paywall) and is served once; if that retry also fails,
-a fresh token is re-issued so the paid credit is never lost. 10-minute TTL,
-FIFO-capped.
-
-Security (adversarially reviewed): the token is unguessable and delivered only to
-the paying caller, so knowing a (public) wallet cannot forge or steal it; it is
-bound to route+body so it can't be spent on a different or costlier render;
-verified live that a forged token still 402s. **Single-use is enforced atomically
-at admission** via `claim()` (validate-and-remove in one synchronous step), NOT a
-validate-now/consume-on-finish pattern — so a burst of concurrent retries with
-the same token can't each be served (the earlier draft had that double-spend; the
-test now asserts exactly one of 100 concurrent claims wins). Unit-tested
-(`scripts/test-render-credit.js`, 24 cases: atomic single-use / no double-spend,
-forgery + cross-request rejection, TTL, re-issue on failed delivery, eviction).
-
-**To turn it on (owner, after review):** set `RENDER_CREDIT_ENABLED=true` on the
-main Railway service. Rollback = unset it (billing reverts, no data). Approach A
-(reserve-before-settle) writeup remains in `docs/f13-capacity-options.md` if the
-ordering-change route is ever preferred.
-
 ### F17 — publish DMARC (DNS)
 
 MX is Zoho; SPF is currently `~all` (soft-fail) with no DMARC.
@@ -143,3 +114,29 @@ finding.
 If we later choose to close it: add a per-request nonce, remove `'unsafe-inline'`
 from `script-src` first (styles second), and roll out behind
 `Content-Security-Policy-Report-Only` to catch misses before enforcing.
+
+---
+
+## 4th review (A402-FR4-01..14) — dispositions
+
+The 4th review's key correction: **`@x402/express` v2.16 settles AFTER the handler
+and CANCELS settlement for any `>=400` response** (see CLAUDE.md "x402 settlement
+ordering"). Several fixes below follow from that.
+
+| ID | Sev | Disposition | Where |
+|---|---|---|---|
+| FR4-01 | High | **Fixed** — idempotency is settlement-aware: cache commits on `finish` only when final status is 200 | `src/server.js` idempotency middleware; `scripts/test-idempotency-settlement.js` |
+| FR4-02 | High | **Fixed** — render-credit REMOVED (a 503 is never charged, so a credit was a free-render bypass); `RENDER_CREDIT_ENABLED=false` in prod + code deleted | `src/server.js` (F13 block/import gone), `src/render-credit.js` + test deleted |
+| FR4-03 | High | **Fixed** — skill-pack inline `render` now routes through the worker | `src/server.js` `SKILL_INLINE_HANDLERS.render` |
+| FR4-04 | High | **Fixed** — Tollbooth enforcing mode FAILS CLOSED without an atomic replay store (unless `TOLLBOOTH_ALLOW_NON_ATOMIC_REPLAY=true`) | `tollbooth/worker.js` |
+| FR4-05 | Med | **Fixed** — worker guard also blocks URL/DSN/DB/LEDGER/redis/mongo names (Railway metadata URLs exempt) | `worker/server.js` `forbiddenSecretsIn` |
+| FR4-06 | Med | **Fixed** — `workerEnabled()` requires BOTH url+token; `assertWorkerConfig()` fails boot on partial config; bounded worker response read | `src/worker-client.js`, `src/server.js` boot |
+| FR4-07 | Med | **Fixed** — `httpHeadersHandler` cancels the body instead of draining it | `src/tools/network-kit2.js` |
+| FR4-08 | Med | **Fixed** — x402 quote/audit/RPC reads are byte-capped (`boundedText`) | `src/tools/x402-kit.js` |
+| FR4-09 | Med | **Fixed** — code-run error name+value go through the aggregate output budget | `src/tools/code-run-kit.js`; `scripts/test-code-run-caps.js` |
+| FR4-10 | Med | **Fixed** — Tollbooth README verifier example honors the `AbortSignal` | `tollbooth/README.md` |
+| FR4-11 | Med | **Residual (platform)** — Chromium `--no-sandbox` + no egress firewall after RCE; secretless worker + egress proxy bound impact. Owner/platform work (`docs/worker-isolation-plan.md`). |
+| FR4-12 | Low | **Fixed** — worker `isMain` uses resolved-path compare (Windows-safe) | `worker/server.js` |
+| FR4-13 | Low | **Accepted** — CSP `unsafe-inline` (see F22). |
+| FR4-14 | Low | **Owner action** — GitHub ruleset / env / DNS evidence (see F01/F17/F25). |
+| FR4-15 | Low | **Fixed (self-found)** — the LLM-gateway prompt/embeddings cache also wrote pre-settlement (same class as FR4-01); now deferred to a final-200 commit via `req.__deferredCache` | `src/server.js` binder, `src/tools/llm-gateway-kit.js` |

scripts/test-code-run-caps.js

@@ -4,6 +4,9 @@
 // before creation. Offline unit test of the cap logic + config.
 //
 //   node scripts/test-code-run-caps.js
+import { readFileSync } from "node:fs";
+import { join, dirname } from "node:path";
+import { fileURLToPath } from "node:url";
 import { __test } from "../src/tools/code-run-kit.js";
 
 const { capUtf8, TIERS, E2B_MAX_CONCURRENT } = __test;
@@ -42,6 +45,24 @@ const ok = (c, m) => { c ? pass++ : fail++; console.log(`${c ? "ok" : "FAIL"} -
   ok(budget === 0, "aggregate budget fully consumed, never negative");
 }
 
+// FR4-09: the thrown-error name + value (execution.error.value) go through the
+// SAME aggregate budget as stdout/stderr/result/traceback — a multi-megabyte
+// error message must not bypass the cap.
+{
+  let budget = 1000;
+  const take = (v) => { const c = capUtf8(v, Math.max(0, budget)); budget -= c.used; return c; };
+  const stdout = take("s".repeat(300));
+  const errName = take("Error");
+  const errMsg = take("E".repeat(5_000_000)); // huge thrown error value
+  const traceback = take("t".repeat(5_000_000));
+  const total = stdout.used + errName.used + errMsg.used + traceback.used;
+  ok(errMsg.truncated, "a multi-megabyte error message is truncated");
+  ok(total <= 1000, `error name+message+traceback stay inside the aggregate cap (used ${total} <= 1000)`);
+  // The handler wires these fields through take() (guards against regression).
+  const src = readFileSync(join(dirname(fileURLToPath(import.meta.url)), "..", "src", "tools", "code-run-kit.js"), "utf8");
+  ok(/const errMsg = execution\.error \? take\(execution\.error\.value/.test(src), "handler routes execution.error.value through take() (budgeted)");
+}
+
 // Config sanity.
 {
   ok(TIERS["code-run"].maxOutputBytes === 256 * 1024 && TIERS["code-run-pro"].maxOutputBytes === 1024 * 1024, "tiers carry an output byte cap");

scripts/test-idempotency-settlement.js

@@ -0,0 +1,86 @@
+// FR4-01: idempotency must be SETTLEMENT-AWARE. @x402/express (v2.16) runs the
+// handler first and only settles a <400 response; on settlement failure it
+// rewrites the buffered 200 into a 402. The old idempotency cache committed at
+// res.json() time (handler completion, BEFORE settlement), so a 200 whose
+// payment never settled could be replayed. The fix commits only on 'finish' when
+// the FINAL status is 200 (post-settlement reality).
+//
+// This boots the real server (FREE_MODE) and proves the caching now keys on the
+// FINAL response status: a 200 is cached + replayed, a NON-200 (stand-in for a
+// settlement-failure 402) is NOT cached. Uses X-Pow-Solution as the credential so
+// idemHashKey engages without needing a live payment.
+//
+//   node scripts/test-idempotency-settlement.js
+import { spawn } from "node:child_process";
+import { readFileSync } from "node:fs";
+import { join, dirname } from "node:path";
+import { fileURLToPath } from "node:url";
+
+let pass = 0, fail = 0;
+const ok = (c, m) => { c ? pass++ : fail++; console.log(`${c ? "ok" : "FAIL"} - ${m}`); };
+const ROOT = join(dirname(fileURLToPath(import.meta.url)), "..");
+
+// Same-class as FR4-01: the LLM-gateway prompt/embeddings cache must ALSO commit
+// only after settlement. The handlers now stash on req.__deferredCache and the
+// route binder commits on a final 200 (an e2e needs upstream keys; lock the wiring).
+{
+  const gw = readFileSync(join(ROOT, "src", "tools", "llm-gateway-kit.js"), "utf8");
+  const srv = readFileSync(join(ROOT, "src", "server.js"), "utf8");
+  ok(/req\.__deferredCache \?\?= \[\]\)\.push\(w\)/.test(gw) && (gw.match(/__deferredCache/g) || []).length >= 2, "gateway chat+embeddings handlers STASH the cache write on req.__deferredCache (not a direct pre-settlement write)");
+  ok(/res\.on\("finish", \(\) => \{[\s\S]*?if \(res\.statusCode !== 200\) return;[\s\S]*?req\.__deferredCache/.test(srv), "route binder commits req.__deferredCache only on a final 200 (post-settlement)");
+}
+
+const PORT = 3959;
+const BASE = `http://127.0.0.1:${PORT}`;
+
+const srv = spawn(process.execPath, ["src/server.js"], {
+  env: { ...process.env, FREE_MODE: "true", PORT: String(PORT), NODE_ENV: "test" },
+  stdio: ["ignore", "ignore", "inherit"],
+});
+const until = async (fn, ms = 15000) => {
+  const t0 = Date.now();
+  for (;;) { try { if (await fn()) return true; } catch { /* retry */ } if (Date.now() - t0 > ms) return false; await new Promise((r) => setTimeout(r, 200)); }
+};
+
+try {
+  const up = await until(async () => (await fetch(`${BASE}/health`)).ok);
+  if (!up) { console.error("server did not start"); srv.kill("SIGKILL"); process.exit(1); }
+
+  const call = (path, { key, body, method = "POST" }) => fetch(`${BASE}${path}`, {
+    method,
+    headers: { "content-type": "application/json", "idempotency-key": key, "x-pow-solution": "test-credential" },
+    body: body === undefined ? undefined : JSON.stringify(body),
+  });
+
+  // 1. A 200 IS cached and replayed on retry with the same key+credential.
+  {
+    const r1 = await call("/api/hash", { key: "k-ok", body: { text: "idem-settlement-test" } });
+    ok(r1.status === 200, "first hash call returns 200");
+    const r2 = await call("/api/hash", { key: "k-ok", body: { text: "idem-settlement-test" } });
+    ok(r2.status === 200 && r2.headers.get("x-idempotent-replay") === "true", "identical retry is served from cache (X-Idempotent-Replay: true)");
+  }
+
+  // 2. A NON-200 final response (stand-in for a settlement-failure 402) is NOT
+  //    cached — the whole point of FR4-01. /api/render with no url returns 400
+  //    before any Chromium work; the retry must re-run (no replay), not serve a
+  //    cached body.
+  {
+    const r1 = await call("/api/render", { key: "k-bad", body: {} });
+    ok(r1.status >= 400 && r1.status < 500, `non-200 request returns ${r1.status} (not cached)`);
+    const r2 = await call("/api/render", { key: "k-bad", body: {} });
+    ok(r2.headers.get("x-idempotent-replay") !== "true", "a non-200 response is NOT replayed from cache (settlement-failure bypass closed)");
+    ok(r2.status === r1.status, "the retry re-runs the handler and gets the same non-200 status");
+  }
+
+  // 3. Sanity: a different body under the same key does not collide.
+  {
+    const a = await call("/api/hash", { key: "k-body", body: { text: "aaa" } });
+    const b = await call("/api/hash", { key: "k-body", body: { text: "bbb" } });
+    ok(a.status === 200 && b.status === 200 && b.headers.get("x-idempotent-replay") !== "true", "different body under the same key is not a cache hit");
+  }
+} finally {
+  srv.kill("SIGKILL");
+}
+
+console.log(`\n${fail ? "FAILED" : "OK"}: ${pass} passed, ${fail} failed`);
+process.exit(fail ? 1 : 0);