# skills/code-review/scripts Agent Contract

Scripts here provide bounded, provider-neutral review/challenge wrappers and
parsers for Claude, Kimi, OpenCode, and Codex.

Rules:

- Preserve the packet boundary and structured output validation. A wrapper may
  expose pathless read/search tools over its frozen, hash-bound packet; verify
  their arguments, returned bytes and completed lifecycle. This does not permit
  arbitrary commands or workspace access. A malformed, timeout, or inconclusive
  wrapper result is not a pass.
- **`review_diff.py` is the one runner without a packet.** Its reviewer reads
  the repository and `git diff` itself, because packet intake was where review
  calls failed (`specs/diff-first-review/plan.md`). Its boundary is the host's
  own read-only mode instead: Codex runs `-s read-only` under a private
  `CODEX_HOME` with no MCP servers, hooks or plugins, kept outside the system
  temp dir so the sandbox helper can start; Claude gets only `Read`, `Grep` and
  `Glob` with no shell (git can write files through `--output`), no setting
  sources and no MCP servers; OpenCode runs a generated `ccl-review` agent
  that denies every permission but reading, under private config/data/state
  dirs with project config and plugins off, from a private empty git project,
  because OpenCode imports `.opencode/plugin` code from its project and parents; Kimi Code runs under a private
  home whose global tool allowlist is `Read`, `Grep`, `Glob`, without hooks,
  MCP or workspace trust. Each attempt builds its own private home and is
  read only after a clean exit, so the try-next loop carries nothing from a
  failed attempt into the verdict. The Codex reviewer may run commands inside its
  read-only sandbox, which is the one exception to the no-target-code rule
  below. The reviewer can read files the user can read and sends what it reads
  to its provider; that is the accepted trade-off. Do not
  add a write-capable tool, a shell for Claude, or inherited config to it, and
  keep the result envelope that CI and the pull-request reminder read.
- **Never pin a lane to a CLI version's vocabulary — reading it or writing it.**
  `parse_probe_result.py`
  gates on *shape*, not on field/value names: the isolation proof is the exact
  `tools` allowlist plus the tool_use scan, which no init field can bypass.
  Field-name and value whitelists were tried twice (`fast_mode_state`, then
  `capabilities`) and both times a routine CLI release took every Claude lane
  down while proving nothing. So: unknown init fields are tolerated when scalar
  or empty-container, and `agents`/`capabilities` are list-of-strings checks
  with no value vocabulary. Only `permissionMode` stays value-pinned (it widens
  what the runtime may do with no tool added).
  The write side is the same class and was missed once: config a wrapper
  GENERATES and then submits to the runtime's own validator as an admission
  precondition carries a key that release may not know, and rejecting it turns
  the whole lane off. So a generated setting that a per-invocation environment
  variable already carries is belt, not the gate — on rejection regenerate the
  strict subset without it and revalidate, unconditionally rather than by
  matching the runtime's error wording, which is the same pin relocated. Only a
  setting with no second, independent path to the same property may fail the
  lane closed, and admission must never widen on the retry.
- **Skill, command and plugin lists are vocabulary, not a boundary.**
  `slash_commands`, `terminal_slash_commands`, `skills` and `plugins` may hold
  any value and are never judged by name, shape, or origin; only `mcp_servers`
  must still be empty. A third attempt at a vocabulary boundary -- a snapshot
  of the host's own built-in names, later backed by a baseline invocation --
  turned every CLI release that shipped a new skill or command into a Claude
  lane outage. Do not add a name list, a baseline, or an "unclassifiable entry"
  class back; nothing in those lists is invocable past the pinned `tools` set.
  The same holds for the installed CCL plugin: an absent or unverifiable
  registry costs the owner-skill binding (`native_skill_binding=unavailable`),
  never the review.
- **Keep unsafe *values* out of the schema-drift class.** An unrecognized shape
  means this lane cannot verify isolation → fallback-eligible. A known field
  carrying an unsafe value (`permissionMode: "bypassPermissions"`) is a breached
  boundary → terminal. They report through different reasons and different
  buckets (`unknown_fields` vs `unsafe_values`) precisely so a real privilege
  escalation cannot inherit the drift class's softer `next_action`. Any new
  value-pinned field goes in `unsafe_values`.
- When every Claude run goes inconclusive with "unrecognized surface-shaped init
  field", the CLI added a NON-EMPTY list/dict under a name the parser does not
  know — the one shape that could carry a new invocable surface. That reason is
  fallback-eligible (`capability_missing`), not a terminal boundary violation,
  so review routes to another client meanwhile. Recipe: (1) capture a normal
  wrapper invocation and diff its formal review stream's init event against the
  parser's known fields; do not add a separate inference probe; (2) decide
  whether the new container is a model-invocable surface — if it is, it belongs
  in the required-empty set, not the allowlist; (3) if it is host metadata, add
  the name to `KNOWN_SAFE_INIT_METADATA_FIELDS` (or give it a shape check),
  never a value whitelist; (4) add RED/GREEN fixtures to
  `test_parse_probe_result.sh` and record the RED round in the slice's own plan
  under `specs/` (see `specs/012-challenge-index-default/plan.md` under
  "Test / register coverage" for the shape); (5) full dual-track before landing.
- **A fail-closed stop must carry the evidence that justifies it.** When a lane
  refuses to cascade *because* the output may hold a real finding
  (`concern_evidence`), a boolean plus a fixed reason string is not enough: the
  operator is told a finding exists but not what it says, so the documented
  reject-and-rerun remediation runs blind and a false positive is
  indistinguishable from a real one. Emit a bounded, line-selected excerpt
  (`concern_excerpt.py`) — matched lines only, never the raw verdict, which may
  echo the packet and would widen the evidence row past the class a schema-valid
  `findings` result already persists. Wrapper run dirs are deleted on exit and the
  gate unlinks the frozen packet, so anything not in the emitted payload is gone;
  the OpenCode timeout path may additionally retain only its explicitly requested,
  curated private diagnostic copy under the contract in
  `../references/timeout-auth-and-capabilities.md`.
  `test_concern_excerpt.sh` is the firing path; keep new stop paths covered by it.
- **Timeout diagnostics separate portable summary from sensitive raw evidence.**
  The ordinary payload may keep bounded counts/types/booleans needed to classify
  a stall, but never raw event, log, prompt, diff, session-id, credential, or model
  text. Raw OpenCode artifacts require an explicit private diagnostic directory,
  restrictive permissions with no extended ACL, a sensitivity marker,
  caller-owned retention, and a test proving credential bindings are excluded.
  A requested owner profile alone
  does not prove a native-skill stall: require positive structured stream-part
  evidence or keep the generic timeout classification.
- Keep review and challenge lanes distinct; do not let one satisfy the other.
- Do not execute target-repo code or grant broad tools from the reviewer lane.
- Parser changes need tests with positive, finding, malformed, and inconclusive
  cases.
- A recipe step here must never cite a repo path that does not exist in this repo — for the specs/ namespace that is now machine-checked and blocking — and a step whose target is absent is not a step, because a contributor cannot run it. Backticked paths in prose are invisible to `check-markdown-links.py`,
  which resolves only Markdown inline-link destinations in tracked `*.md`; the
  gap that leaves is now closed for one namespace by
  `scripts/check-spec-references.py`, which runs in `make test` and CI and fails
  when a backticked specs/ token does not resolve. Verify each cited path
  resolves before landing, and when one is already dead, repoint it at what the
  repo does rather than creating the missing target — back-filling it fabricates
  evidence for a round nobody ran.
- **A path TEMPLATE is not written as a citation.** That checker has no
  exemption: a backticked token beginning specs/ must resolve, full stop, because
  the exemption that used to carve out angle-bracket shapes produced five
  separate bypasses and was deleted. Write a shape by naming it without the
  prefix and leaving the prefix in prose — a `<NNN>-<slug>/plan.md` file under
  specs/ — or put it in a fenced block. Keep the backticks: an unbackticked angle
  bracket is swallowed as an HTML tag when the Markdown renders.

Validation:

- `bash skills/code-review/scripts/test_classify_envelope.sh`
- `bash skills/code-review/scripts/test_concern_excerpt.sh`
- `bash skills/code-review/scripts/test_claude_review_probe.sh`
- `bash skills/code-review/scripts/test_parse_opencode_review.sh`
- `bash skills/code-review/scripts/test_parse_probe_result.sh`
- `bash skills/code-review/scripts/test_parse_review_json.sh`
- `bash skills/code-review/scripts/test_opencode_review_retry.sh`
- `bash skills/code-review/scripts/test_opencode_review_concurrency.sh`
- `bash skills/code-review/scripts/test_review_gate.sh`
- `bash skills/code-review/scripts/test_review_client_order.sh`
- `bash skills/code-review/scripts/test_cli_review_wrappers.sh`
- `python3 skills/code-review/scripts/test_kimi_packet_mcp.py`
- `python3 skills/code-review/scripts/test_review_client_compat.py`
- `bash skills/code-review/scripts/test_code_review_identity.sh`
- `bash skills/code-review/scripts/test_review_diff.sh`