fix(codex): sandbox the review path, fail the gate closed, order timeouts wrapper-first

Closes #2496, #2524, #2477 — three defects in the class "a guard that
reports success while doing nothing", all in codex/SKILL.md.tmpl:

(a) Review sandbox. The default `codex review` path was the only codex call
with no sandbox override, inheriting ~/.codex/config.toml's default — write
access on a trusted project — while Important Rules claimed read-only.
Top-level `codex review` has no -s/--sandbox flag (verified on 0.147.0), so
the invocation now pins `-c 'sandbox_mode="read-only"'`, the same form the
consult-resume path already uses.

(b) Fail-closed verdict gate. The old rule ("no [P1] found → PASS") could
not fail on the default path: native `codex review` output carries no
bracketed tags, and a non-zero exit, expired auth, timeout, or empty result
also contains no [P1] — all read as PASS. The gate is now an ordered,
fail-closed check: non-zero exit → FAIL; empty output → FAIL; [P0]/[P1]
(bracketed or codex's native labels) → FAIL with count; NO severity tags at
all → FAIL requiring a human read; PASS is only reachable through the
explicit tagged-advisory-only branch. [P0] is recognized as blocking, and
the review-log findings count includes it.

(c) Bash gate above the wrapper. Step 2A instructed `timeout: 300000` under
a 330s wrapper, and Challenge's 300s gate sat under a 600s wrapper — the
harness killed the call before the wrapper could emit its diagnosable
exit-124 message. Every Bash gate now sits strictly ABOVE its wrapper:
360000 over the 330s review wrapper, 660000 over the 600s challenge/consult
wrappers, with the ordering rationale stated at each site.

Also from #2477/#2524: a new Error Handling entry for the model-entitlement
400 ("The '<model>' model is not supported...") pointing at the `model =`
pin and `[notice.model_migrations]` in ~/.codex/config.toml and saying
exactly which override to retry with (-m for exec-based modes,
`-c model="..."` for review mode, which rejects -m); the Model & Reasoning
section no longer documents `-m` for `/codex review`.

Static assertions in test/codex-hardening.test.ts pin (a)-(c) across both
the .tmpl and the generated SKILL.md: every scoped review invocation carries
sandbox_mode="read-only" and never -s; the default-PASS sentence is banned
and the fail-closed branches are present; and per-section, every Bash
`timeout: N` is strictly greater than every wrapper budget, with 2A/2B/2C
all required to be inspected. Generated SKILL.md regenerated via
gen:skill-docs in this commit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

test/codex-hardening.test.ts

@@ -475,3 +475,80 @@ describe('codex timeout wrapper: /review + /ship diff passes', () => {
     });
   }
 });
+
+// Regression guards for #2496 / #2524 / #2477 — three "guard reports success
+// while doing nothing" defects in codex/SKILL.md:
+//   (a) the default `codex review` path set NO sandbox override, inheriting
+//       whatever ~/.codex/config.toml grants (write access on trusted
+//       projects) while the skill's Important Rules claimed read-only;
+//   (b) the severity-tag verdict gate could not fail on the default path — a
+//       non-zero exit, empty output, or untagged output all satisfied the
+//       "no [P1] found → PASS" branch as written;
+//   (c) Step 2A's Bash tool gate (300000 ms) sat BELOW the 330s wrapper
+//       budget, so the harness killed the call before the wrapper could emit
+//       its diagnosable exit-124 message — the same inversion #1036 fixed for
+//       /review and /ship.
+// Asserted across both the .tmpl source and the generated SKILL.md so a regen
+// or hand-edit of one but not the other can't silently reopen any of them.
+describe('codex SKILL.md.tmpl: review sandbox + fail-closed gate + timeout ordering', () => {
+  for (const relPath of ['codex/SKILL.md.tmpl', 'codex/SKILL.md']) {
+    const read = () => fs.readFileSync(path.join(ROOT, relPath), 'utf-8');
+
+    test(`${relPath}: (a) every scoped codex review invocation pins sandbox_mode="read-only"`, () => {
+      const invocations = read()
+        .split('\n')
+        .filter((l) => /_gstack_codex_timeout_wrapper\s+\d+\s+codex\s+review\b/.test(l));
+      expect(invocations.length).toBeGreaterThanOrEqual(1);
+      for (const line of invocations) {
+        expect(line).toContain('sandbox_mode="read-only"');
+        // `codex review` has no -s/--sandbox flag (verified 0.147.0) — the
+        // config override is the only lever. `-s read-only` here would fail
+        // at argv parsing, which check (b) would then read as a gate FAIL.
+        expect(line).not.toMatch(/\s-s\s+read-only\b/);
+      }
+    });
+
+    test(`${relPath}: (b) the verdict gate fails closed — no default-PASS path`, () => {
+      const content = read();
+      // The old rule inferred PASS from the absence of a substring:
+      expect(content).not.toContain(
+        'If no `[P1]` markers are found (only `[P2]` or no findings) — the gate is **PASS**',
+      );
+      // The new rule: FAIL on non-zero exit, empty output, and untagged
+      // output; [P0] recognized as blocking; PASS reachable only through the
+      // explicit tagged-advisory-only branch.
+      expect(content).toContain('The gate FAILS CLOSED');
+      expect(content).toContain('`_CODEX_EXIT` is non-zero (including 124) → **GATE: FAIL**');
+      expect(content).toContain('empty or whitespace-only → **GATE: FAIL**');
+      expect(content).toContain('untagged output');
+      expect(content).toContain('`[P0]`');
+      expect(content).toContain('PASS is only reachable through check 5');
+    });
+
+    test(`${relPath}: (c) every Bash gate sits strictly above its section's wrapper budgets`, () => {
+      // Split on `## ` headings; within any section that declares BOTH a Bash
+      // tool gate (`timeout: N` in ms) and a wrapper budget
+      // (`_gstack_codex_timeout_wrapper S codex`), every gate must be strictly
+      // greater than every wrapper budget so the wrapper fires first.
+      const sections = read().split(/\n## /);
+      const inspected: string[] = [];
+      for (const section of sections) {
+        const gates = [...section.matchAll(/timeout:\s*(\d{4,})/g)].map((m) => Number(m[1]));
+        const wrappers = [...section.matchAll(/_gstack_codex_timeout_wrapper\s+(\d+)\s+codex\b/g)].map(
+          (m) => Number(m[1]) * 1000,
+        );
+        if (gates.length === 0 || wrappers.length === 0) continue;
+        inspected.push(section.split('\n')[0]);
+        for (const gate of gates) {
+          for (const wrapper of wrappers) {
+            expect(gate).toBeGreaterThan(wrapper);
+          }
+        }
+      }
+      // Review (2A), Challenge (2B), and Consult (2C) must all have been
+      // inspected — each declares both numbers. If a refactor drops either
+      // number from a section, this count catches the silent skip.
+      expect(inspected.length).toBeGreaterThanOrEqual(3);
+    });
+  }
+});