--- name: okstra-code-review description: >- Use when the user wants changed code reviewed and the result written to a file — either an okstra task's implementation stage, or any branch unrelated to an okstra run. The tell is a review request over a diff: "review this stage", "review my branch", "code review", "leave the review in a file". The orchestrator censuses the diff into an explicit worklist, four parallel reviewers return a verdict for every cell against this project's coding-preflight rules, and a coverage audit re-dispatches any gap. NOT for writing a PR body (okstra-pr-gen), starting a run (okstra-run), or inspecting a finished task (okstra-inspect). --- # OKSTRA Code Review Review what a diff changed — one okstra implementation stage, or any branch — against this project's coding-preflight rules, and leave the result as a file under the project. ## What makes this one-run-complete A reviewer left to build its own worklist enumerates the diff differently every run, so every run finds different things. Here the worklist — the **census** — is built **once, by you, deterministically**, before any reviewer is dispatched. Reviewers receive their slice as input; they never rebuild, reinterpret, or extend it. Each reviewer returns a **verdict for every cell it was given** (`clean`, or findings), so a skipped cell is visible instead of silently absent, and Step 3.5 re-dispatches every cell that came back without one. Same census + full verdicts = the same process every run. ## Rules The rules themselves are **not in this skill**. They live in this project's coding-preflight pack — `overview.md` (core principles plus the three-stage resource routing), `clean-code.md`, and the routed `languages/` / `frameworks/` / `architectures/` resources. Step 2 decides which of them apply; reviewers read them from the absolute paths in their brief. Never restate a rule in a brief or in the report — point at it. This skill owns review calibration only: - `references/census-rules.md` — how a diff becomes cells: the four axes, pack allocation, exclusions, and the two completion criteria. - `references/review-calibration.md` — the per-cell verdict format, severity definitions and points, when `clean` is the correct verdict, and the report's section order. ## Step 0 — Preflight Run one Bash tool call, starting with the literal token `okstra` (never wrapped in `if`/`eval`/`export`/`$(...)`/`VAR=...`/`||`/`&&`/`npx` — a non-literal leading token defeats the `Bash(okstra:*)` permission match): ```bash okstra preflight --runtime claude-code ``` Branch on the fixed first line: - `Okstra preflight: ready` → carry `Project root` as a literal string; every later step is anchored on it. - `Okstra preflight: failed` → the check only sees the cwd of the Bash call, so a project that is not the cwd can read as missing setup. Ask whether the user pointed at a specific project directory; if they did, re-run targeting it: `okstra preflight --runtime claude-code --cwd ` (`--cwd` is the sanctioned way to target a project — a leading `cd` would break the permission match). Only if this also fails do you show `Reason` and `Recovery`, then stop. If the call fails with `unknown command: preflight`, the `okstra` binary on PATH predates this skill — tell the user to update it (`npm i -g okstra@latest`), then stop (`/okstra-setup` does not update the binary). Every subsequent `okstra ` call self-bootstraps its Python path, so this skill never needs `okstra paths --shell` / `export PYTHONPATH=...`. ## Step 1 — Resolve the review target Pick the mode from what the user actually gave you: - an okstra task token (a task-id like `DEV-9184`, or a full `project-id:task-group:task-id` key) → **stage mode** - a branch name → **branch mode** - neither → ask which of the two first. Do not guess a mode. **Stage mode.** 1. A full `project-id:task-group:task-id` token is already the task key. For a bare token, run `okstra model-io task-selection-input --project-root --task-ref ` and branch on the fixed `Match count`, `Task`, and `Updated at` rows: **0** → report the task cannot be found and stop; **1** → take `Task`; **N** → list the candidates and ask via a 3-option picker (1–2 recommendations + `Enter directly`). 2. `okstra stage-map --project --text` → use the fixed `Stages` and `Done stages` count/name/value rows. 3. Choose the stage with a 3-option picker — recommend the highest done stage (the work that just finished) first, a second plausible stage if there is one, and `Enter directly` last. Pick silently only when exactly one stage exists. **Branch mode.** 1. If the user named no branch, list `git -C branch --format='%(refname:short)'` and offer a 3-option picker with the current branch as the first recommendation. 2. Detached HEAD — `git -C symbolic-ref --quiet HEAD` prints nothing — is refused: ask the user to name a branch. Do not review a bare commit. **Then call the target CLI.** One call resolves everything the rest of this skill needs; the only reason to call it again is a base the user overrides in the confirmation step below. ```bash okstra code-review target --task-key --stage --project-root --text okstra code-review target --branch [--base ] --project-root --text ``` `Status: ready` carries `Project root`, `Mode`, `Worktree path`, `Branch`, `Base commit`, `Head commit`, `Review path`, and `Round`; stage mode adds `Task key`, `Task root`, and `Stage`. Carry every field verbatim into the later steps — none of them is recomputed anywhere below. `Status: error` carries `Failure stage` and `Failure reason`. Report both and stop, unless the Exceptions table names that case. After the range is resolved, render the only project identity and range values that later model prompts may consume: ```bash okstra model-io code-review-input --project-root --base --head ``` Carry this fixed Markdown view into the census and reviewer prompts. Do not pass task manifests, target-CLI JSON, or arbitrary JSON fields to a reviewer. **You never derive the base.** Pass `--base ` only when the user named one; otherwise the CLI resolves it. `baseCommit` is a **ref, not necessarily a commit id** — a caller-supplied `--base` passes through verbatim — so use it as given in `git diff ..` and never present it as "commit ``". **Where to run git.** Use `worktreePath` when it is non-empty; otherwise run git in `projectRoot` and read the stage's `branch` ref. An empty `worktreePath` does **not** mean the stage is gone: a completed stage's registry row is `released`, so the field is empty even when the directory is still on disk. Either way the commits are on the branch. **Then show the base and confirm it — branch mode and stage mode alike.** The CLI's answer is a recommendation the user has not seen yet, and a base nobody looked at is how unrelated commits slip into a review unnoticed. Print `baseCommit` verbatim next to `git -C log -1 --oneline ` so the commit it names is legible, then ask with a 3-option picker: 1. `Use ` — **the recommendation**, the value the CLI returned. 2. `Show what this range covers` — print `git -C log --oneline ..`, then ask the same three options again. 3. `Enter directly` — always last: the user types a ref, and you call the target CLI a second time with `--base `. Every field from the second call replaces the first call's, including `reviewPath` and `round`. ## Step 2 — Census the diff 1. **Read `references/census-rules.md`** (next to this file) and follow it. It owns the cells; this step owns only the inputs and what you print. 2. Collect the diff from the work directory chosen in Step 1 — `git -C diff --name-status ..` for the file list, and `git -C diff ..` for the hunks. Before trusting that range, run `git -C rev-list --count ..`: anything but `0` is the rewritten-history case in the Exceptions table. An empty diff skips Steps 3–3.5 (also in Exceptions). 3. **Route the packs once, here — and fix both absolute paths the briefs carry.** `okstra paths --field home` prints the okstra home; read `/prompts/coding-preflight/overview.md` and walk all three stages of its routed resource selection over the changed-file list. The result is the applied pack list — the absolute paths reviewers will read. This routing happens exactly once per run; no reviewer repeats it. The second path is this skill's own calibration file. A subagent has no "next to this file" coordinate, so the brief must spell it out: the installed skill home is `~/.claude/skills/okstra-code-review/`, making the literal path `~/.claude/skills/okstra-code-review/references/review-calibration.md` — the same string whether the skill was copied in or dev-linked. Carry it, together with the pack list, into every Step 3 brief. 4. Build the cells per `references/census-rules.md` and print, in this response: one cell table per axis, the exclusion list with a reason on every entry, and the applied pack list. 5. Restate both completion criteria and show they hold: censused files + exclusions = files in the diff, and every hunk maps to a censused function or to file-level code. **Membership is mechanical; judgment only ever decides a cell's verdict.** Never drop a cell because you doubt the rule applies to it — that doubt is the reviewer's answer, and its name is `clean`. Over-inclusion costs one line; a judged exclusion is the run-to-run variance this census exists to remove. A large census is never truncated. Report the cell count and confirm before dispatching — a silent cut is a false "I looked at everything" signal. ## Step 3 — Materialize and dispatch four reviewers in parallel Every reviewer and later gap-fill is a separate auditable standalone invocation. Before dispatch, create `.okstra/agent-invocations/code-review/.instructions.md` from that reviewer's brief, then run `okstra agent-prompt materialize` with `--purpose code-review`, `--audience code-reviewer`, and the canonical `.prompt.md` path beside it. Pass the current host runtime, selected provider, and `--model-role analyser`; the returned assignment is authoritative. Run `okstra agent-prompt verify` against the returned `metadataPath` before invoking any model. For a native host call, pass the verified prompt body and `hostModelValue`. For a deterministic provider process, run `okstra worker-dispatch` with the verified prompt path and `modelExecutionValue`; never substitute one model value for the other. Dispatch the four verified calls in parallel when the host supports it. Every brief carries: - the diff, plus the work directory path so the reviewer can read whole files for context - the project layout in one or two lines (where source, tests, and — if the routing found one — domain / ports / adapters live) - **its own axis's cell list**, verbatim from the census - the absolute paths of the packs its axis reads (from step 2's routing) - the calibration path, written out in full as Step 2 fixed it — `~/.claude/skills/okstra-code-review/references/review-calibration.md`. The verdict format, the severity points, and the rules for a legitimate `clean` are defined there, not in the brief; a reviewer that cannot open this file cannot return a usable verdict, so never hand it a relative path or a "next to the skill" hint Each axis is **one rule group**, so a cell is `target × ` — never `target × `. The reviewer names the specific rule it found violated inside the verdict's `rule` field, and one cell may carry findings from several rules of its group. Axis scope — the `Reads` column names that group's rules, and a brief never restates them; the bodies are in the packs: | Axis | Cell | Reads | |---|---|---| | `structural` | non-test source file × `structural` | core principles 1, 2, 3, 6; `clean-code.md` DRY / KISS / SOLID / YAGNI; the completion sweep's domain-literal and documented-fork items; plus the architecture pack when one was routed | | `semantic` | function × `semantic` | core principles 4, 5; `clean-code.md` meaningful naming, functions-do-one-thing, the plain-English summary test, the 50-line cap, nesting depth, magic numbers, comments-explain-why | | `state-and-tests` | non-test source file × `state-and-tests`, test file × `state-and-tests` | `clean-code.md` "Mutation and state boundaries" and "Testing discipline"; plus the routed language pack's self-mock signals | | `general` | non-test source file × `general` | correctness bugs, swallowed errors, security, clear performance problems, core principle 7 (fix at the cause — nothing previously green edited to silence a failure); `clean-code.md` "Wrapping a third-party call" — a recovery branch the library already performs, a comment naming a condition the call site never establishes, a rethrow that drops the original error; plus the routed language pack's language-specific traps | Say it plainly in every brief: **the census is law** — work the cells exactly as given, return a verdict for every one, never re-derive the list. There is no length budget; dropping a finding to stay brief is the failure this skill exists to prevent. Capture each raw model return under `.okstra/agent-invocations/code-review/.tmp/`. Publish it with `okstra agent-prompt materialize-result`, publish the immutable completion marker with `okstra agent-prompt complete`, then run `okstra agent-prompt verify-completion`. Parse only the `returnedBody` emitted by that last command. A model response without a verified completion is not a review result and cannot contribute a verdict. ## Step 3.5 — Audit the coverage Diff each reviewer's verified `returnedBody` cells against the slice you handed it. Any cell without a verdict → dispatch **one gap-fill invocation per axis**, carrying only the missing cells and the same brief. Each gap-fill uses a new invocation ID and repeats the full materialize → verify → dispatch → materialize-result → complete → verify-completion boundary from Step 3. Repeat until every cell of every axis has a verdict. A missing verdict is unfinished work, never an implicit `clean`. Do not start Step 4 while a single cell is unaccounted for. ## Step 4 — Merge and write 1. **Read `references/review-calibration.md`** (next to this file) and follow its report section — you are the one writing the file, and it fixes the Coverage sentence, the per-finding line format, the Score table columns, and the total row. The reviewers were given it for their verdicts; the report obeys it too. 2. **Dedupe across axes.** The same defect surfaced by two axes stays once, under the rule that explains it best. 3. **Severity is the reviewer's.** The merge concatenates and dedupes; it never re-grades. If a verdict looks wrong, the brief was wrong — improve the brief for next run and ship this one as returned. 4. **Write the report to `reviewPath`** with the Write tool (it creates the parent directories). Frontmatter fields, in this order: ```yaml mode: stage | branch taskKey: # stage mode branch: # branch mode stage: # stage mode only round: baseCommit: headCommit: packs: [] generatedAt: ``` Body sections, in this order: `## Coverage`, `## Must-fix`, `## Should-fix`, `## Nits`, `## Score`. Empty severity sections are omitted; `Coverage` and `Score` are always present, and a review with no findings still emits the Score table with a total of 0. 5. **In the session, print only** the `reviewPath`, the count per severity, and the score total. The file is the deliverable — do not replay the findings in chat. ## Exceptions | Situation | Handling | |---|---| | `preflight` reports `Okstra preflight: failed` | retry with `--cwd `; if that also fails, tell the user to run `/okstra-setup` first and stop | | `unknown command: code-review` | the `okstra` binary predates this skill — tell the user to update it (`npm i -g okstra@latest`) and stop | | `worktreePath` is empty | not a hard stop and not a missing stage: run git in `projectRoot` against the stage's `branch` ref and continue | | `baseCommit` is not an ancestor of `headCommit` — `git -C rev-list --count ..` returns a **non-zero** count, meaning a rebase or squash rewrote the history the stage was recorded against, and `..` would drag predecessor work in backwards | code-review is read-only, so do not force a reconcile. Offer two options: review against the branch's current tip, or run `okstra git-reconcile` first and retry | | the diff is empty | dispatch no reviewers; write the "no changes" report to `reviewPath` and stop. It is a normal report, not a free-form note: the same frontmatter, `## Coverage` reading "0 changed files → 0 cells on every axis, 0 files excluded" plus the applied packs, every severity section omitted, and `## Score` carrying the table with its single total row reading 0 | | the census is large | never truncate — report the cell count and confirm before dispatching | | detached HEAD (branch mode) | refuse and ask the user to name a branch | ## Principles - **Stay in the diff.** Every finding cites a line this diff changed. A cell whose only wart sits on untouched lines verdicts `clean` — pre-existing issues are not this change's problem. - **Don't manufacture findings.** A census fully verdicted `clean` is a valid, useful result. - **No finding without a fix.** Readability findings carry a pseudocode sketch; naming findings carry a concrete alternative name. - **The census is law.** A reviewer that rebuilds its own worklist reintroduces exactly the run-to-run variance this skill exists to kill. - **Every cell gets a verdict.** `clean` is a result, not an omission; the audit treats a gap as unfinished work. - **The report prose is Korean.** Paths, identifiers, rule names, and quoted code stay verbatim.