# Jitx Code Review > Same-model self-critique pass for JITX Python code just written in this workspace. Use when the user asks to "review my JITX code", "self-critique", "check JITX conventions", "find string-hacking", "check framework-boundary issues", "audit before merge", or any equivalent. Mandatory for complete-board tier at task acceptance (folds into Think Twice); user-invoked for single-task work. Catches the architectural failure modes that grep gates and static linters miss — parallel string-keyed models, reflection-as-iteration (regardless of whether on self), owner-shaped data misplaced in design code, build-spec-then-iterate, module-import-time parallel models, and framework-boundary-bypass (the "framework does it, therefore so can I" trap). Applies an ownership test to every banned-pattern hit or proposed exception. Produces severity-tagged findings (CRITICAL / WARNING / NOTE) that fold into the task acceptance block. - Skill: `jitx-inc/jitx-code-review` (Agent Skill, multi-file: 3 files) - Install (CLI): `npx skillmds@latest add jitx-inc/jitx-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/jitx-inc/jitx-code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: AI & ML - Author: JITx-Inc (https://skillmd.com/u/jitx-inc) - Updated: 2026-09-10 - Page: https://skillmd.com/skills/jitx-inc/jitx-code-review --- # JITX Code Review Task-level architectural self-review for JITX Python code just written in the current workspace. The reviewer agent reads the code with a checklist-bound, evidence-anchored framing — different from the framing under which the code was written — which is what catches what the in-session author rationalized as fine. This skill is the *per-task same-model pre-pass* in the Think Twice flow (see `jitx/references/task-execution.md` Part A Step 4 and `jitx/references/outside-voice-review.md` "Same-model passes precede codex"). It catches architectural and code-craft smells (string-hacking, parallel models, naming hygiene). Codex outside-voice — which runs only for trigger-list task classes (MCU/FPGA, RF, power, safety, high-speed digital, battery) — catches the JITX-engineering-domain issues (datasheet-vs-code, impedance/return-path, voltage-divider math). Phase 3b's same-model pre-pass is the **four-pass design audit** (not this skill) — see `jitx/references/completion-blocks.md` "Phase 3b Design Audit Block". By the time Phase 3b runs, every Phase 1/2/3 task has already passed its per-task `jitx-code-review` and the findings live in the task acceptance blocks. ## When this skill runs - **Mandatory** for every accepted task in complete-board tier. The `jitx` base skill invokes this at the Think Twice step in `task-execution.md` Part A Step 4 (before the orchestrator's acceptance review). The task acceptance block's `JITX code review (self):` field carries the finding count + disposition; missing or `not run` defaults to `block`. - **User-invoked** for single-task tier. A user who wants self-critique on their single-skill work invokes this directly ("review my JITX code"). Each subskill's SKILL.md mentions the option but does not auto-trigger. - **Not a replacement** for codex outside-voice. Codex still runs at Phase 3b (mandatory complete-board) and per-task for the trigger list (MCU/FPGA, RF, power converter, safety, high-speed digital, battery — see `jitx/references/outside-voice-review.md`). ## Inputs the review takes 1. **The code under review.** Default: the workspace's `/` directory (everything just written for the current task). The orchestrator may scope tighter (single file) when invoking from Think Twice. 2. **The task acceptance block draft** (if running mid-Think-Twice). Tells the reviewer what was supposed to be built and what evidence the author cites. 3. **The relevant rule sources.** Read at review time: - `jitx/SKILL.md` Don'ts (always) - `jitx/references/architectural-patterns.md` (always — this is the worked-counter-example source for the dominant failure modes) - Subskill SKILL.mds for the work in scope (component-modeler if a component file changed, etc.) - `jitx-code-review/references/checklist.md` (the pattern index over the architecture doctrine) The review is **evidence-anchored**: every finding cites `file:line` and quotes a rule sentence (from SKILL.md or a reference file). Findings without a citation are downgraded to `NOTE`. ## Workflow 1. **Identify the scope.** Files / directories under review. Default `/`, but a scoped run can target a single file. 2. **Read the rule sources** above. The point of reading them in-skill is so the reviewer cites verbatim — not paraphrases. 3. **Walk the pattern checklist** (`references/checklist.md`). For each pattern, look for instances; for each instance, capture severity and citation. 4. **Apply the architectural pass** (`jitx/references/architectural-patterns.md`). The dominant failure modes are encoded there as worked counter-examples (Bad/Good + rationale); check each one against the code under review. 5. **Apply the ownership test to every banned-pattern hit or proposed exception** (see "Ownership test" below). The pattern checklist names patterns; the ownership test catches the framework-boundary-bypass failure where the AI rationalizes a banned pattern as "OK because the framework does it." 6. **Emit the findings block** in the format below. 7. **Hand back to the caller** (orchestrator in Think Twice, or user for single-task). The orchestrator folds findings into the task acceptance block; the user decides whether to fix or accept-with-rationale. The skill does **not** modify code. Findings → caller → fix decision → next iteration. ## Ownership test (mandatory for every banned-pattern hit or proposed exception) When the code uses a banned pattern (`getattr`, `type(...)`, `_protected_method()`, etc.), or when the agent/author proposes a carve-out ("this `getattr` is OK because…", "this is the boundary call into the framework…"), the reviewer must answer four questions before accepting the carve-out: 1. **What object owns the invariant** the code is navigating? (A numbering scheme, a layer-to-via map, a protocol pin assignment, etc.) 2. **Is this code inside that object's class or a subclass of it?** If yes, the same-class exception applies and the pattern is legitimately allowed (carve-out is real). If no, see step 3. 3. **Is the caller using a public method on the owning object?** If yes, that's the right shape — no banned-pattern use needed. If no, see step 4. 4. **If no public method exists, can a subclass adapter expose one?** If yes, that's the fix — add a public method on the subclass that delegates to the framework's protected method (the "method calling another method on the same class" carve-out of the no-leading-underscore-from-elsewhere rule). All design-side callers go through the adapter. If the answer is "outside the owner, copying internals" or "wrapping the banned pattern in a helper to make it look like one boundary call," classify the finding as **`framework-boundary-bypass`** (see `references/checklist.md`) and recommend the subclass-adapter fix. This step exists because rule text alone wasn't sufficient: in a real review, the AI followed the no-getattr rule literally (one wrapped `getattr` call) but missed the rule's intent (don't replicate framework internals in design code) and produced a design coupled to a numbering scheme that the reviewer caught. ## Severity scheme Same vocabulary as `outside-voice-review.md` for consistency: - **CRITICAL** — code clearly violates a rule, or contains a pattern that's named as illegal in `jitx/SKILL.md` Don'ts or `architectural-patterns.md`. Must be fixed before the task can be `accepted`. Examples: `getattr(self, f"...")`, module-level loop populating a global dict, design file with `_SIGNAL_LAYER_TO_VIA` that duplicates the substrate. - **WARNING** — looks like the failure pattern but has ambiguity (e.g., a `list[dict]` that might be a legitimate parameter-staging pattern, an `f"X_{i}"` that might be a log message). Each WARNING needs a disposition: `fix` or `accept with rationale: `. - **NOTE** — observation worth recording but not blocking. Code-craft hygiene that's better-done-otherwise (vestigial `from __future__ import annotations`, named constant used exactly once, suboptimal naming). Combined with the codex outside-voice precedence rule: **any CRITICAL or WARNING finding here changes the combined verdict to `issues-pending`** even if codex says `clean`. Same rule, two same-tier reviewers. ## Output format ```markdown ## JITX code review — /circuits/.py> **Reviewer:** jitx-code-review (same-model self-critique) **Scope:** **Rule sources read:** jitx/SKILL.md, jitx/references/architectural-patterns.md, ### CRITICAL - **** at `:` — . Rule: "" (from ). Disposition required. - ... ### WARNING - **** at `:` — . Disposition: fix | accept with rationale: . - ... ### NOTE - **** at `:` — . - ... ### Summary CRITICAL: | WARNING: | NOTE: ``` The `` is from the checklist (e.g., `string-keyed-model`, `reflection-as-iteration`, `owner-shaped-data-misplaced`, `parallel-data-model`). Pattern tags are orthogonal to severity — a single pattern can produce CRITICAL or WARNING findings depending on the specific instance. ## What this skill is NOT - **Not a cross-repo audit tool.** This skill is in-workspace and same-model only — it reviews the code just written here, not a remote codebase against external documentation. - **Not a replacement for static analysis.** `pyright` and `ruff check` still run as part of the task acceptance block. They catch type / lint issues; this skill catches architectural smells they can't see. - **Not a build verifier.** `jitx build` still runs. This skill is purely a code-quality pass. ## Compliance-theater watch The skill can become ritual if the reviewer agent passes everything as `clean` without doing the work. Concrete signals the orchestrator should treat as suspicious: - `CRITICAL: 0 | WARNING: 0 | NOTE: 0` on a parametric / generator task where the architectural patterns are likely to surface. - Findings without `file:line` citations or rule quotes. - All findings tagged `NOTE` when the pattern checklist named CRITICAL-eligible patterns. - Bulk dispositions (`all accepted, all framework code`) on review-required grep-gate hits — each hit needs a per-line rationale. - A prior finding marked `fix`/`resolved`/`accepted` with no cited evidence — no fix commit or `file:line` showing the change, and no stated acceptance rationale. "Resolved" with neither is the author-side form of compliance theater (e.g. a reviewer reopening a thread marked resolved with "not sure what the resolution is"). A `resolved` disposition must point at the diff that resolved it or say why it's accepted as-is. A valid review output should let a reader open the cited file at the cited line and verify the finding in under a minute.