git:20261103.2941d76 to git:20261103.26fb191

# 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`