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. review_diff.py owns the review flow (candidate, attempts, verdict, result, terminal output); review_diff_clients.py holds one adapter per CLI behind one interface, so a client change stays there and the flow never names a CLI. stdout stays the machine-readable lines, progress goes to stderr, and exit 1 means recorded findings only.
• 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