Rijuls review

Rijul's Review

A four-hat adversarial review — each hat an independent agent — that ends in per-role confidence scores and ranked P0–P3 findings, reconciled by a fifth deduplicator agent into one master list. Its purpose is to catch what a single-perspective read misses and to force the reviewer to justify both low confidence (what's broken/risky) and high confidence (why it's actually solid, plus residual risk) — never a bare number. A clean pass here is necessary-not-sufficient: the four hats are the same model reading the same author's framing (correlated, not statistically independent), so an independent-model (Codex) pass remains the real merge gate — this review front-loads and cheapens that pass, it does not replace it.

Each hat's full method + output shape lives in its own references/role-*.md file (passed verbatim as that hat's sub-agent prompt); the shared severity scale, per-finding required fields, and verdict vocabulary live in references/finding-format.md. This SKILL.md is the orchestrator: the Hard Rules every hat applies, the hats table, and the five-agent execution mode. See Targeting & relationship to /code-review at the bottom for the artifact forms it accepts.

Hard rules (non-negotiable)

0. A review is static inspection only unless Rijul explicitly asks for verification. Do NOT run tests, lint, format, typecheck, knip, builds, quality gates, browser smoke, database probes, dependency installation, or any other executable verification during a review. Read the diff, source, callers, tests, configs, and existing check results; reason from that evidence. You may identify a missing or inadequate test/gate as a finding, but do not execute it and do not make review completion contingent on running it. Only cross this boundary when Rijul separately and explicitly asks to test, verify, reproduce, smoke, or run gates.

1. Read everything first. No file left unread. Before scoring, open every file the artifact names or depends on — the files to be modified, the files to be created (read their siblings/templates), the files referenced as "unchanged" reference points, the call sites, the test fixtures, and the immediate collaborators of each. If the artifact cites file:line, open that file and verify the line says what's claimed. Grep to confirm "no other importers" / "only consumer" / "already does X" claims rather than trusting them. List the files you read in the output so the user can audit coverage.

2. Four hats, four independent scores. Score from each role separately. Do not average into one number — each hat sees different risk.

3. Justify low AND high.

   • < 80%: you MUST explain why — the specific gaps, unverified assumptions, missing tests, edge cases, or risks that hold the score down, and what would raise it.
   • > 90%: you MUST also explain why — what concretely makes it strong (verified claims, covered edge cases, reversibility, test coverage) AND the residual risk that keeps it from 100%.
   • 80–90%: explain the main reservation in one or two lines.

4. Verify, don't trust. Every load-bearing claim in the artifact gets independently checked against the code. Call out any claim that turns out to be wrong, stale, or unverifiable. A confidently-worded claim that you could not verify is a confidence deduction, not a pass.

5. Pyramid principle. Lead each role with the score + the one-sentence verdict, then the justification.

6. Cost, not just correctness. A change that compiles, passes every test, and is logically correct can still be a defect if it is slow by construction — most often an N+1: a per-item query or await inside a loop over a collection. Lint / typecheck / unit tests are structurally blind to it (an N+1 is correct, just slow), and it hides behind small local/test data, surfacing only at real volume. For any read over a collection, the reviewer MUST ask: does the query count stay constant as the collection grows, or scale with it? If it scales — a per-row await, an unbatched lookup, an unbounded SELECT, a filtered column with no index — that is a finding to flag, even when nothing fails.

7. jsdom-green ≠ browser-correct for native DOM/event semantics. A change that passes a jsdom unit test can still be wrong in a real browser whenever it leans on native behavior jsdom models loosely: event propagation across the React root, focus, showModal() / ::backdrop / inert, native document/window listeners, layout/measurement, or UA pseudo-classes (:modal). jsdom false-passes these. So a load-bearing claim about such behavior is verified in a real browser (Chrome MCP), not accepted because a jsdom test is green — and a jsdom test asserting such behavior is itself suspect (a "spec-faithful fixture" trap; a green test proving a browser fact can be a false negative for bugs). Corollary bug class the Engineer hat MUST check whenever a native <dialog> modal is introduced: keyboard shortcuts still fire behind it. showModal() blocks POINTER input but NOT keydowns, and React's synthetic stopPropagation() does NOT reliably stop native document/window listeners (React 18 delegates at the root container) — so every shortcut that can be live while the modal is open must stand down (guard each handler on dialog[open], e.g. isModalDialogOpen(); a dialog-boundary stopPropagation is not enough). Audit BOTH source-sets: (1) top-level global shortcuts, AND (2) shortcuts on components that stay mounted behind the modal — most-missed: an editor left mounted while a delete confirm opens over it (its Cmd+S / Cmd+K leak). The only handler you leave unguarded is one registered by the modal itself — and verify a handler's consumer scope by grep before calling it "modal-scoped," don't assume. Ordering matters: for an app-owned chord that overrides a browser-reserved key (Cmd+O / Cmd+S / Cmd+K, "/"), call preventDefault() on the matched chord FIRST, THEN the isModalDialogOpen() early-return — a guard that returns before preventDefault suppresses the app action but lets the chord fall through to the browser's native action (Open File / Save Page / address-bar / quick-find) behind the modal. Second corollary — focus-return on close. When an overlay/dialog conditionally renders its OWN trigger (!open ? <trigger> : <overlay>), do NOT set the focus-restore target by reading document.activeElement inside the open-effect — by the time that effect runs the closed-state trigger has already unmounted, so it captures <body> and closing DROPS keyboard focus. The restore target must be captured synchronously (before the trigger unmounts) or be a stable ref to the remounting trigger; also latch "was ever open" so page-load doesn't steal focus, and suppress the refocus on a route-change close (navigation shouldn't yank focus back to the trigger). jsdom implements focus()/activeElement enough to pin the ref wiring in a test, but the visible focus ring + route-change-close behavior are a real-browser check. Third corollary — custom (non-native) modals are invisible to a dialog[open] guard. A shortcut guard that queries only dialog[open] (native showModal() dialogs) does NOT see a custom role="dialog"/aria-modal overlay (a portaled <div>), so every global shortcut (Cmd+O go-to-file, "/", Cmd+S, tree-nav) still fires BEHIND it. When a change introduces a custom modal overlay, it MUST opt into the guard (e.g. a data-app-modal marker the guard also queries) — AND any shortcut that is the overlay's OWN control (the chord that toggles/closes it) must use a native-only probe, or it stands down against its own overlay and can no longer close it.

8. Verify real data flow — what the component RECEIVES, and what a shared source's change RIPPLES to. Two symmetric traps, both invisible unless you trace the actual prop/arg wiring rather than the design's prose. A design/code review MUST check both whenever a change leans on data availability or on a shared read:

   • Receipt. When the design leans on data being "available" to the code that does the work ("the full list is in memory", "the component has X"), trace the actual prop/argument into that specific component — data existing upstream (in the RSC load, a store, a parent's state) does not mean the working component receives it. Derived / filtered / narrowed props are the trap: a drag handler that computes a neighbor from its items prop is wrong if that prop is filteredItems, even though the full set sits one component up. Grep the prop's provider (<Comp items={…}>) and confirm it's the full thing, not a view.
   • Shared-source fan-out. When a change alters a shared read/source — its ORDER BY, shape, or filter — enumerate every consumer of that read and check the effect on each, especially passive pass-through consumers that inherit the result unsorted/unreshaped (a "preserves input order" / "already sorted upstream" consumer won't reveal the change at its own call site). Changing one shared sort to fix consumer A silently reorders consumers B and C that share the payload.
   • Write-stop fan-out (the mirror direction). When a change STOPS WRITING a field/column (a cohabitation migration that moves off a legacy field), enumerate every remaining READER of that field — the new/updated rows go stale or invisible to those readers until they migrate too. "Nothing else reads it" is a claim to rg the field name for, never to assume — and the reviewer must RUN rg <field> and count EVERY reader; a curated N-item list from the author is not the grep (the author's list is reliably short). When readers do remain, the change must state the consequence explicitly, not silently ship a data-visibility gap.
   • Rename fan-out includes STRING-KEYED readers (typecheck-blind). When a change RENAMES a field or — especially — an agent/LLM tool INPUT parameter (groupSlug→group), a typed-contract rename updates the typed call sites but is invisible at any consumer that reads the value by STRING KEY: a display/summary/analytics/log module doing get(input, "oldName"), a record[oldKey] lookup, a pill-label builder. Those keep reading the OLD key → silently get undefined (a blank pill, a dropped log field), no typecheck error, usually no test. So for any field/param rename, rg "oldName" across the tool-summary / analytics / logging / display layers, not just the typed consumers.
   • Writeback echo (the OUTPUT side of data flow). Receipt checks whether data reaches the component; this checks whether a mutation's result flows back. For any optimistic mutation — the component sets a local overlay, PATCHes, then clears the overlay on success — trace what the cleared state reverts to. If the overlay clears back to a prop/state that the server result was NOT propagated into (no onRowPatched-style callback to the source-of-truth state, and no refetch), the change reverts on success — the mutation persisted server-side but the UI snaps back until a manual refresh. The trap is sibling asymmetry: one surface (the board) wires the echo (onRowPatched(updated) → setItems) and its sibling (the tree) is assumed to mirror it but silently omits it. So for every optimistic-clear path: confirm the success branch echoes the server row into the source-of-truth state BEFORE (or with) clearing the overlay, on EVERY surface, not just the one you read first. This is jsdom-testable (mock the patch → assert the echo callback fires) — a green "renders" test does NOT cover it; a waitFor(echo).toHaveBeenCalled test does.
   • Render-partition mirror-drift. When a derived list is built to MIRROR what a render actually shows — a shadow/ordered-id list that must line up 1:1 with the on-screen rows (drag orderedIds ↔ measured rowRects, a virtualized index ↔ rendered items, any "same partition as the render" filter) — verify the mirror by OPENING the render's own bucketing function and diffing its item-eligibility predicate branch-by-branch, for EVERY mode (flat / each group-by / each lane) against the derived list's. Never trust the comment ("matches groupRows"), a reviewer's prose ("mirrors the render partition"), or your own memory of what a mode renders — and NEVER encode a fix from a description of the render without opening it. A mirror that drifts on ONE branch — the derived list includes an id the render omits — is invisible to gates, jsdom, and a plausible-looking filter; downstream it indexes/bisects against a row that isn't on screen. Pin each mode's partition with a per-mode unit test, not a browser smoke. Nested-tree corollary — line-index space ≠ placement space. When a drag drop-LINE's visual slot is computed from a flat/visible index but the WRITE uses tree-relative placement (sibling-after / into-child), the two spaces DIVERGE at an expanded parent: a line at targetIndex + 1 sits between the parent and its FIRST CHILD, but a "sibling after parent" write lands after the parent's ENTIRE visible subtree. So for any nested drag, verify the cue matches the write for the expanded-parent case specifically (not just flat siblings) — the "below an expanded parent" line must skip past the whole visible subtree, indent held at the target's depth. Pure-testable (computeDropIndex(orderedNodes, targetIndex) over an expanded-parent fixture).
   • Shared style / CSS-class fan-out. A CSS class (also: a Sass mixin, a styled-component, a design token, a shared component) is a shared source too. When a change restyles or repurposes a shared class to suit one caller, rg every element that renders that class across ALL surfaces and confirm the restyle doesn't break their layout — a class is not "yours" just because one caller motivated the change. The trap: repurposing .foo from position:absolute (an overlay) to an inline flex button for surface A silently relocates it on surfaces B and C that render the same class in a different container. Fix by giving the changed caller its OWN class (or a modifier), never by mutating the shared one. typecheck + jsdom + a passing Vercel build are ALL blind here (CSS isn't executed and the class still exists); only a per-surface visual check or a rg <class-name> across the repo catches it.

9. Audit EVERY prompt on EVERY execution path a model acts on — delegated sub-agents included, not just the top-level agent and not just tool describe strings. When a change migrates the model of something the agent WRITES (a data model, an id-space, an allowed-value set), a fix to the tool describe string is NOT enough: the sub-agent's system prompt frames/overrides the describe, and its own context/data loader may still feed the model the old model. The AI Education Engineer hat MUST, for any model-migration change: (a) enumerate every delegation tool (each manage* delegation tool, any run*Delegation) and trace it to the worker's buildPrompt/prompt module + its data loaders, verifying THOSE render the new model; (b) treat a tool describe edit as necessary-not-sufficient — grep for the same instruction in the worker prompt template; (c) NEVER dismiss a prompt file as "another surface's default, out of scope" without grepping who invokes it — a prompt builder is often shared across a standalone rail AND a delegation write path, so buildX-style helpers must be traced to their actual callers, not judged by filename. Bidirectional corollary — a tool's description/schema IS a second model-facing prompt; a MODE migration must reconcile BOTH the system prompt AND every tool description, and checking either alone is insufficient. One failure mode fixes the describe but not the worker prompt; the mirror failure fixes the prompt but not the descriptions. When a change migrates HOW/WHEN a tool executes (rail→delegation, streaming→auto-apply, propose→direct-write, read-only→write), the tool description strings still describe the OLD mode ("never writes / pending card / ALWAYS MANUAL") and the model follows the descriptions even under a corrected system prompt. So the AI-Ed hat MUST, for any mode migration: (a) rg the old-mode language across every tool description/parameter-doc the migrated agent will see, and require it migrated — usually by REUSING an existing description-swap wrapper (e.g. SWAPPED_DESCRIPTIONS), never raw tools — with a test that no migrated description contains the old-mode phrase; (b) treat a "reuse X / X unchanged" claim in the design as the AUTHOR'S FRAMING, not a verified fact — OPEN the reused component and enumerate its full model-facing + behavioral surface against the NEW context ("reuse buildToolset" hides 14 tool descriptions written for a different mode). "Unchanged" is where the review must look hardest, not skip.

10. A portaled / body-relocated element escapes SCOPED CSS custom properties — verify every var(--x) it (and the subtree it hosts) consumes actually RESOLVES at the element's real DOM position, not merely that the token is "defined somewhere." When a component createPortals to document.body (or is otherwise teleported/appended out of its logical container — overlay, drawer, tooltip, menu, modal), it leaves behind any --x declared on a scoped selector (.container { --x: … }) instead of at :root. Every var(--x) it then reads silently computes to initial: for background that is transparent, for color/border-color the initial/inherited value, and a whole border: 1px solid var(--x) shorthand drops. This is INVISIBLE in a browser smoke whenever the element happens to sit over content of a similar color — a transparent panel over a white page reads as a solid white panel. So the Engineer hat MUST, for any portaled/body-appended element: (a) enumerate every var(--…) in its own CSS and in the CSS module of the component it hosts; (b) for each, grep the token's DEFINING selector and confirm it is :root/html/body (an ancestor of the portal target) — not a scoped container the portal escaped; (c) treat "token defined at file:line" as necessary-not-sufficient — a definition inside .some-shell { … } does NOT reach a document.body portal, and finding one line is not the same as confirming its scope. This is the CSS analog of Hard Rule 8 (trace what the element actually RECEIVES): trace what a var() actually RESOLVES TO at render position.

11. A change that pins, overrides, hides, or replaces an element MUST NOT silently strip the INTERACTIVE AFFORDANCES it already had — enumerate them BEFORE and verify each still FUNCTIONS AFTER. "Does it render correctly?" — a screenshot or a jsdom mount — is structurally BLIND to lost interactivity: a fixed-width rail looks pixel-identical to a resizable one; a pointer-events:none overlay looks identical to a live one; an element whose drag handler is shadowed looks identical to a draggable one. So when a change touches a component that carries affordances — drag-to-resize, collapse/minimize, drag-sort, expand/collapse, hover-reveal, keyboard nav, focus — the Engineer + Senior-SWE hats MUST list each affordance the component had and confirm the change preserves it, in a REAL browser (Hard Rule 7), not merely that it "looks right." Especially suspect and an automatic flag: a CSS !important override on a property a library CONTROLS to provide behavior — flex/flex-basis on a react-resizable-panels Panel (RRP writes it inline to drive BOTH drag-resize AND the imperative collapse setLayout), a transform on a dnd-kit draggable, a width/height a ResizeObserver owns. Overriding that property with !important neutralizes the library's behavior while the element still looks correct. Ask "what did this element DO before, and does it still DO it?" — not just "does it still look the same?"

12. A column-type migration MUST widen EVERY table that stores a COPY of that value — snapshot / revision / audit / history / denormalized tables — not just the primary table + its known write sites; and a backfill respace MUST reproduce the render's EXACT visible order. When a migration changes a column's type (int4→float8, text→enum, widening a numeric), the primary write function often cascades the value into a sibling table in the same transaction — a *_revisions snapshot, an *_audit/*_history row, a denormalized copy. If that sibling column still has the OLD type, the write throws and the whole operation fails, defeating the migration. So the Engineer hat MUST, for any column-type migration: (a) rg the column name across the WHOLE schema and enumerate every table that declares it — widen/backfill each; (b) trace the primary write function (updateX, saveX) to EVERY insert/update it fires in its tx, confirming none writes the value into a stale-typed column; (c) add a test that writes the NEW-type value (e.g. a fractional for a float widen) and asserts it round-trips through BOTH the primary AND the snapshot table. Corollary (order-preservation): a backfill that renumbers rows to "preserve the current order" must reproduce the render's order EXACTLY — if the sort comparator can return 0 (ties) and the migration read has no ORDER BY, the tie rows renumber in DB-physical (arbitrary) order and visibly reorder. Carry the render's seed keys (e.g. created_at, slug) into the migration's sort, or a "no user sees a reorder" claim is false for tied rows.

13. Two blind spots this review must actively counter — conformance-to-design, and over-deferral to browser smoke. Both let a real bug through a full four-hat pass in one real case (the nested-drag line-vs-placement bug — see Hard Rule 8's nested-drag corollary), so they are now standing checks:

    • (a) Conformance ≠ correctness — trace the OBSERVABLE outcome, not just fidelity to the design. Verifying "the code implements decision row D-N" / "matches the design's Sec.X" is structurally BLIND to a bug that is IN the design. If the spec says dropIndex = i + 1 and the code writes i + 1, a conformance check passes a wrong spec. So at least one hat (Engineer or PO) MUST, for any user-facing behavior, trace the observable cause→effect from first principles, independent of the design's prose: "the user does X → the UI shows Y → the write persists Z — do Y and Z actually AGREE?" In one real case: "hover below an expanded parent → the line shows between the parent and its first child (Y) → the write lands a sibling after the parent's whole subtree (Z)" — Y ≠ Z, catchable by the thought experiment alone, missed because every hat checked code-vs-design instead. The design is an input to review, never the oracle.
    • (b) "Browser-smoke owed" is NOT a blanket deferral — split off the pure-testable core and demand a test. When a correctness question is punted to a real-browser smoke (because pointer geometry / layout / feel genuinely need a browser, Hard Rule 7), SEPARATE the DOM-dependent part from any pure composition logic riding inside it. "Does index-space X map to placement-space Y", "does the computed slot equal the written slot", "does the measured list's index mean what the write means" — these are pure functions, unit-testable, and MUST be pinned by a test, not absorbed into the smoke bucket. In one real case the whole drop-correctness question was filed under "browser-smoke owed", but the index↔placement correspondence was pure logic (computeDropIndex, now unit-tested). Ask of every "browser-only" defer: what part of this could a pure test pin? — and require it.

14. This review READS code; it does NOT run the test suite — so it is structurally BLIND to a stale EXISTING fixture the change breaks, and to a "gates green" claim that was scoped too narrow. The four hats (and any diff/design review) verify the new code against the design by reading; they never execute vitest. So when a change edits a shared source that pre-existing tests assert against — a nav data array (SHARED_NAV_LIST), an enum, a design token, a widely-imported constant/type, a fixture builder — an existing test elsewhere that pins the OLD shape fails, and no amount of reading the diff surfaces it. This is a different miss from Hard Rule 8 (that's the runtime fan-out of a shared source; this is the test-fixture fan-out). So for any change that touches a shared/global source, at least one hat MUST: (a) rg <SharedSymbol> --glob '*.test.*' across the WHOLE tree (not the feature dir) and flag EVERY existing fixture that asserts the old shape as at-risk / must-update; (b) treat a "gates green / vitest passed" claim as SUSPECT whenever the vitest was scoped — a shared-source change is only proven by the FULL sweep (pnpm -C <app> run test), and a scoped run that excludes the fixture's dir is a false green the review must call out, not trust. The hats can't run tests, but they can enumerate which existing fixtures the change invalidates and demand the full sweep before merge.

15. A server-computed now threaded into a client component is for RENDER only — a user-gesture-relative computation (now + offset on a click) MUST read a fresh Date.now() at the event, not the stale render timestamp. When an RSC threads a server now (serverNow) into a client component to keep SSR/hydration labels stable, that value is correct for render (bucketing, relative-time labels) but it is the page-render timestamp and goes stale as the tab ages. Any action offset — "snooze +15m", "remind me in an hour", any now + delta triggered by a click — MUST compute from a fresh Date.now() in the event handler, NOT the seeded render now. On a stale tab, serverNow + offset means "offset after page load," and a tab open longer than the offset computes a past instant → the server's future-only guard rejects it (a 409 the user can't explain). A Date.now() inside an event handler is hydration-safe (the hydration ban is a client clock in RENDER output, not in a handler). So the Engineer hat MUST, for any client component seeded with a server now: separate render-now (labels/buckets — serverNow is right) from action-now (click-relative offsets — must be Date.now()), and demand a test that mocks Date.now() to an instant far from the seeded serverNow and asserts the computed value is click-relative.

16. The review's job is to find what the GATES CANNOT — so map every risk to the gate that would catch it, and name the class where all offline gates are structurally blind. Types, lint, and unit/jsdom tests never exercise the real runtime substrate — the real database, a real browser, an actual deploy sequence, a live vendor wire — so any defect that lives THERE is invisible to a fully-green local suite, and an integration test that self-skips against a prod .env is not coverage of it either. The whole classes that hide in the substrate: a query/DB-shape mismatch, a hydration/native-DOM/layout bug, a migration-must-precede-code ordering hazard, a vendor-schema drift. So for each finding the reviewer MUST (a) name the single gate that would actually surface it, and (b) if that gate is a runtime one that did NOT run (browser smoke, real-DB read, staged rollout), mark the finding UNVERIFIED and demand it — never accept "typecheck + N tests pass" as clearance for a substrate-level risk.

17. Trace every externally-influenceable value to the SINK that interprets it, and gate on PROVENANCE, not ownership. A value's danger comes from what it is interpreted AS at its sink — a URL scheme (href/src/redirect), markup (HTML / dangerouslySetInnerHTML), a query (SQL), a command (shell), a filesystem path. For each such sink the reviewer asks: can ANY write-path feed this value from untrusted input? "It's the owner's own data" is the classic false-clear — it collapses the instant an agent write-path exists, because agents run on untrusted content (inbound email, fetched web, uploaded docs), so agent-produced or stored values are untrusted-by-provenance. Demand validation AT the sink (allowlist scheme/same-origin for URLs, escape for markup/SQL, canonicalize for paths), enforced at BOTH write and render, failing closed — never pass the raw value through.

18. A change that flips a SERVER-sent wire value whose validity depends on the CLIENT already running new code is a DEPLOY-ORDERING hazard — demand a staged (client-first) rollout, never a same-window flip; and it is WORST when the delivery failure is silently counted as success. Server and client deploy independently, so if the server ships a new value (a new channelId / enum / field / route it emits) before every active client has installed+opened the build that recognizes it, in-flight OLD clients mishandle it. The insidious case: the transport ACKs at a layer ABOVE the client — a vendor "ticket ok" while the DEVICE drops the message — so the server's success-gate (accepted≥1 → markFired, no receipts) marks the drop as delivered. Invisible miss, no retry, reintroducing the exact class the change was closing. So for any client↔server wire-contract change, the reviewer (SWE + PO hats) MUST ask: does an OLD client mishandle the new value, and is that mishandling counted as success anywhere? If yes → BLOCKING: require Phase A (ship the client change + keep the server on the OLD value — additive, ships now) then Phase B (flip the server value only after every active client has run Phase A — manual check for a tiny fleet, capability-metadata-on-register for a larger one). A config-plugin / earlier-startup mitigation is NOT a substitute (already-installed old clients still lack the new understanding until updated), and a client-side "shrink the window" fix only covers the updated-client-not-yet-run window, NOT the old-client/new-server window — name both.

19. Retiring a concept (dropping a table / enum / repo / vocabulary) — a symbol-deletion typecheck is NOT a comprehensive removal gate; it is BLIND to the STRING and STORED-DATA forms of the same concept. tsc names TS symbol callers (imports, typed calls), but never sees the concept expressed as: a raw SQL fragment (a column name inside a sql`…` template — e.g. sql`AND t.group_id = …`), a route-path / query-param string (/api/widget-groups, ?group=), a hardcoded slug/enum list in a form or config, stored string-column data (a column that holds the concept's slugs as text — no FK to fail, no symbol to break, the data just orphans), a test spec / JSON-DB snapshot pinning the old string, or a public-contract / package SPEC.md / interface doc that still declares the removed method/type (a prose contract, not compiled — tsc/knip/lint never read it). So when a change gates a legacy-drop on "typecheck names every surviving caller," the reviewer MUST refuse that as sufficient and demand two gates: (1) typecheck after symbol deletion, AND (2) an explicit rg sweep over EVERY string form — the column-name strings, route strings, slug literals, the storage columns, the spec/snapshot files, and the SPEC/contract docs. A concept retired only by symbol deletion leaves its string/SQL/route/stored-data/contract-doc forms live and silently broken. And never trust a "SPEC updated / comments updated" claim — rg the exact removed symbol name in the SPEC/contract yourself (a partial doc-edit that fixes some lines but leaves the interface entry is the common miss).

20. Reusing a SHARED sanitizer / render-mode as a NEW feature's security boundary — audit EVERY carve-out branch of its predicate against the new caller's context, and TEST the exception input, not the headline claim. A shared sanitizer (untrusted markdown mode, an HTML escaper, a URL flattener) was written for its ORIGINAL caller and almost always has exception branches that stay "live" (if (untrusted && !isRelativeAsset(href)) flatten — so an _assets/ ref is NOT flattened). When a new feature leans on that mode as its boundary ("it flattens all links / strips all HTML"), the reviewer MUST rg the predicate, enumerate its branches, and for each carve-out ask: does the new caller feed input that hits it, and with what prop context? A carve-out that was safe for caller A can leak for caller B (different props, e.g. no documentId to resolve an asset ref → it emits a broken/surviving <a href>). Test the EXCEPTION input (the _assets/ anchor), not just the happy path — happy-path tests pass while the carve-out ships green. A boundary claim in a design doc is a hypothesis to verify branch-by-branch, not a fact; fixing it in the shared sanitizer often closes the same latent bug in the original caller too.

21. For a PUBLIC route that serves a resource AND can be REVOKED — two checks offline gates are blind to: (a) cache-control must not outlive the revocation surface; (b) per-segment encodeURIComponent does NOT stop ../ path traversal. (a) If a resource can be unpublished/deleted/expired/revoked, its cache-control must NOT be immutable/long max-age — a shared/browser/CDN cache keeps serving it after the DB gate flips, silently defeating revocation. A header copied from a non-revocable sibling is the classic trap. Ask: "can this be revoked? then it can't be cached past the revocation" → no-store (or a TTL matching the revocation SLA), pinned in the route test. (b) encodeURIComponent("..") === ".." — dots aren't escaped — so a URL built from content/user-derived path segments via ${rel.split("/").map(encodeURIComponent).join("/")} still emits literal ..; in an untrusted body, ![](_assets/../../api/docs/x) → /api/share/<tok>/asset/../../api/docs/x → the browser normalizes it to a same-origin URL OUTSIDE the intended route (scope escape / SSRF-ish). Any path from content-derived segments must reject .././empty/\ segments → null/404 BEFORE emitting the URL (at the builder) AND at the receiving route (%2e%2e decodes to .. in a segment); test the traversal input, not just the happy path.

22. Adding an ACCESS GATE (password / auth / paywall) to a resource — gate EVERY surface that serves the resource OR its sub-resources, not just the primary one; and confirm the gating credential REACHES each. A "document" is not one endpoint: it is the reader page PLUS its image/attachment/asset routes PLUS any export/download/API endpoint — each often built in an EARLIER slice with its own public access model, and the "do-NOT-touch other slices" boundary actively steers you away from re-gating them. So gating only the primary surface (the page) leaves the sub-resources (the images) an open bypass. The reviewer MUST, for any gate change: rg every route that reads the same resource id/token and confirm the gate applies to each; verify the credential (cookie) reaches every gated surface (a cookie path:/share/<token> is NOT sent to /api/share/<token>/asset/* — no shared prefix; use path:"/" with per-resource cookie NAME for isolation); and browser-smoke the SUB-resource directly (fetch the asset/download URL logged-out, un-unlocked → confirm denied) — a page-gate smoke is BLIND to whether the image URL is still fetchable. Also fail-closed IDENTICALLY on both surfaces (mirror the exact predicate — key on hasPassword then require hash!==null && … && verify(), not hash!==null alone, so an inconsistent row denies everywhere).

23. Adding a NEW SOURCE to a marker-gated / hook-synced MATERIALIZATION (a projection, cache, denormalized mirror, search index, digest) — audit the new source against the materialization's FULL refresh lifecycle, not just the first happy-path build; and challenge any "it needs a migration." A materialization only shows what a refresh trigger last wrote. Adding a new input source to one has three failure classes offline gates + a first-build test are blind to:

    • (a) Already-materialized artifacts go STALE — usually most of prod. If the build re-fires only on a missing "built" marker / dirty flag / TTL, every artifact built BEFORE the change never picks up the new source until something forces a rebuild. "It appears on the next build" is false for the entire already-built population — which, days/weeks after launch, is nearly all of it. The reviewer MUST ask: what invalidates the marker so existing artifacts re-materialize with the new source? If nothing does, the feature is invisible to all existing data and a "self-repair when the projection is missing" gate (or a marker version bump / backfill) is REQUIRED, not optional.
    • (b) The new source needs its own incremental-sync + PRUNE path, or it leaves GHOSTS. A projector that only full-rebuilds relies on the rebuild's prune to delete vanished rows. If the new source has NO write/sync hook AND the rebuild is marker-gated, then when a new-source row is DELETED at its origin (external sync, cascade, a provider un-syncing a calendar), its stale projection is never pruned → a ghost row that outlives its source. "This source is read-only so it needs no write path" is a trap: read-only-TO-THE-USER still mutates underneath via external sync, and every such insert/edit/move/delete must reproject + prune the affected artifacts (+ subscription/account teardown). Enumerate the new source's out-of-band mutation paths and confirm each reconciles the materialization.
    • (c) Boundary inclusivity on the new range read. A new listInRange(day, day) (or any windowed read feeding the projection) must use HALF-OPEN, tz/DST-safe bounds — an inclusive upper bound admits a next-day-midnight row into the prior day. Check the comparator, don't assume it matches the sibling reads.
    • (d) Calibration — challenge "it needs a migration / the row can't distinguish X." A projected row that doesn't PERSIST a provenance/type marker does NOT mean the fact is unknowable — the read/API boundary can often RE-DERIVE it from existing data (re-resolve which source table an id lives in via a join/lookup), gate the UI, and reject the mutation server-side, all WITHOUT a schema change. When a hat concludes "the honest fix needs a prod migration," it must first prove the distinguishing fact is not a read-time join away.

The four hats

Hat	Looks for
Senior Software Engineer	Architecture fit, coupling/cohesion, SOLID, reversibility, blast radius, naming, whether the change goes through existing seams or starts a parallel system, quality-gate/knip impact, test strategy soundness, adherence to repo conventions in CLAUDE.md. AND shared-source fan-out (Hard Rule 8): when a change alters a shared read (its ORDER BY/shape/filter) OR restyles/repurposes a shared CSS class / mixin / component, enumerate every consumer — especially passive pass-through ones that inherit it unchanged — not just the one being targeted; rg the class name across all surfaces before mutating it (repurposing a shared class for one surface silently relocates it on the others). AND two CI-invisible metadata checks (Codex keeps catching these): (a) a PR that changes a package's public API or SPEC.md MUST bump its package.json version (the package's CLAUDE.md) — lint/typecheck/tests all pass without it; (b) a PR that edits a design-doc .md with a committed companion .html must regenerate the .html in the same diff — docs:check validates INDEX/frontmatter/links, never rendered content, so a stale companion drifts silently.
Engineer (implementer)	Will this actually compile and run? Concrete correctness: signatures match call sites, SSR/hydration safety, async/race conditions, the exact wire/contract shape, edge cases, error/empty/loading states, off-by-one, id/lifecycle bugs. The "I have to type this in and it must work" view. AND real data-flow (Hard Rule 8): trace the actual prop/arg into the component that does the work — a handler leaning on "the full data is here" is wrong if its prop is a filtered/derived view even when the full set exists one component up. AND native-DOM/event behavior (Hard Rule 7): does anything rely on event propagation, focus, showModal()/inert, or native document/window listeners in a way a jsdom test would false-pass — most commonly, do global keyboard shortcuts still fire behind a newly-introduced <dialog> modal? Verify in a real browser, not on green jsdom. AND data-access cost (Hard Rule 6): does any read over a collection N+1 (a per-item await/query in a loop instead of one batched read), scale its query count with row count, run unbounded, or filter on an unindexed column? Correct-but-slow is a defect — flag it even when tests pass and local data is tiny. AND portaled-element CSS scope (Hard Rule 10): for any createPortal-to-document.body overlay/drawer/tooltip/menu, verify every var(--x) it and its hosted subtree consume resolves at :root — a token defined only on a scoped .shell {} selector computes to initial (transparent background, dropped border) once the element escapes that scope, and a smoke over same-colored content hides it. AND affordance survival (Hard Rule 11): when the change pins/overrides/hides/replaces a component that had interactive affordances (drag-resize, collapse/minimize, drag-sort, keyboard nav), enumerate each and verify it still FUNCTIONS in a real browser — "renders correctly" is blind to a lost drag/resize/collapse; auto-flag any !important override on a property a library controls (RRP flex, dnd-kit transform).
AI Education Engineer	For AI/agent/LLM features: is the model/agent contract sound? Tool routing & naming, context/persistence semantics, prompt/system-message correctness, streaming behavior, token/turn limits, observability (run rows, logs), and whether the design teaches the next agent how to extend it (docs, comments, explicit seams). Also: is the artifact itself clear enough that a future agent could execute it unambiguously? AND every-prompt-every-path (Hard Rule 9): for a model-migration change, trace each delegation tool to its worker sub-agent's system prompt + data loaders — a fixed tool describe is necessary-not-sufficient; the worker prompt can still teach the dead model on the write path. Never skip a prompt file as "another surface's default" without grepping its callers. AND tool descriptions are prompts too (Hard Rule 9 bidirectional corollary): for a MODE migration (rail→delegation / auto-apply / direct-write), rg the old-mode language ("never writes", "pending card", "manual") across every tool description the migrated agent sees and require it swapped (reuse the description-swap wrapper, not raw tools) with a test; and treat any "reuse X / X unchanged" claim as the author's framing — OPEN X and enumerate its descriptions, never accept the label.
Product Owner	Does it serve the user's actual intent and the product north-star? Scope correctness (is "phase 1" really the right cut?), UX consequences the user may not have anticipated, known tradeoffs surfaced honestly, deferred-not-denied discipline, and whether shipping this leaves the product coherent.

The table is each hat's condensed lens. Each hat's full step-by-step method + its own output sections (the Engineer's edge-case matrix, the AI-Ed hat's contract map, the PO's user-promise) lives in a dedicated file, passed verbatim as that hat's sub-agent prompt:

• Hat 1 → references/role-senior-software-engineer.md
• Hat 2 → references/role-engineer.md
• Hat 3 → references/role-ai-education-engineer.md
• Hat 4 → references/role-product-owner.md
• Hat 5 (deduplicator) → references/role-deduplicator.md

All five share the severity scale + per-finding fields + verdict vocabulary in references/finding-format.md.

Procedure

1. Inventory every file/symbol the artifact references. Build the read-list.
2. Read the whole list. Open each file. For each cited claim, verify it. Grep to confirm negative claims ("no other importer", "only X renders this").
3. Each hat scores + ranks findings per its role file and the Hard Rules — a NN% with justification AND ranked P0–P3 findings (five required fields each, per references/finding-format.md). Note any claim it could not verify.
4. The deduplicator merges the four role outputs into one master list — highest severity per issue preserved, corroborating hats recorded — per references/role-deduplicator.md.
5. Final output is the four role sections verbatim + the consolidated block:

## Files read (N)
- path — one-line of what you confirmed/found

## Claims verified / corrected  (per hat, reconciled by the deduplicator)
- ✅ <claim> — confirmed at file:line
- ❌ <claim> — WRONG: <what the code actually says>
- ⚠️ <claim> — unverifiable: <why>

## Role scores (unaveraged — each hat sees different risk)

### Senior Software Engineer — NN%
Verdict sentence. Then justification (low → gaps; high → strengths + residual risk).
Ranked findings (P0→P3) + Verdict: Approve | Approve with follow-ups | Fix first.

### Engineer — NN%
…  (+ edge-case matrix)

### AI Education Engineer — NN%
…  (+ contract map)

### Product Owner — NN%
…  (+ user promise, scope/UX assessment)

## Consolidated findings (deduplicated, ranked P0 → P3)
`[P0] file:line — summary  [hats: SWE, Eng]` · failure scenario · smallest fix · regression gate.
Highest severity per issue preserved; claim conflicts + coverage gaps surfaced.

## Bottom line
Lowest score + every open P0/P1 + the single most important fix + an explicit
consolidated verdict. An open P0 anywhere ⇒ Fix first (never merge with an open decision).

6. Be honest, not generous. The score is calibrated against "would this ship clean and behave correctly in production on first try." Quality gates passing is necessary, never sufficient. If you didn't read a relevant file, your score is provisional — say so and read it. And a clean four-hat + dedup pass is necessary-not-sufficient: the hats are one model on one framing, so the independent-model (Codex) pass remains the real merge gate (Hard Rule 9).

Execution mode — five Opus sub-agents (four hats in parallel, then the deduplicator)

Run the four hats as four separate Opus sub-agents in parallel, then a fifth deduplicator agent once all four land — never one inline pass. Each hat gets its own fresh context, so it reads the source deeply for its lens without the other three hats' reads crowding the window, and the four scores stay genuinely independent (no anchoring). This is the default; fall back to inline-sequential only when sub-agents aren't available — and when you do, know that it sacrifices the independence: one context running the hats in sequence lets hat N+1 see hat N's findings in-window (the exact anchoring this mode exists to prevent), so mark the scores anchored/provisional and treat the Codex pass as doubly required.

The anti-anchoring rule is load-bearing: do NOT show one hat another hat's findings, and do NOT pre-digest findings for any hat — give each the artifact + shared read-list and let it verify against source itself. The deduplicator is the ONLY agent that sees more than one hat's output, and it sees them only AFTER all four are frozen. This is what keeps the review four genuinely independent perspectives instead of four re-phrasings of the first one.

How the caller (squad leader) orchestrates it:

1. Build the shared read-list once (the artifact + every file/symbol it references) and pass it to all four — same artifact, same commit SHA / PR / repo path, same read-list, no pre-digested findings.
2. Launch all four hats in ONE message, each an Opus (model: opus) general-purpose sub-agent, run_in_background: true, so they run concurrently. Each hat's prompt = its role file verbatim (references/role-<hat>.md) + the full Hard Rules block verbatim (this SKILL.md — read-everything, justify-low-AND-high, verify-don't-trust, pyramid, and the numbered bug-class rules) + the references/finding-format.md severity/fields/verdict spec verbatim (the hat runs against the artifact's own repo, so it can't reach this skill's folder — hand it the spec) + the artifact path + read-list, with ARTIFACT/REPOSITORY/READ-LIST filled in. Each role file already ends with "Review only. Do NOT edit files."
3. Keep posted + retry: post immediately that the four launched, surface each as it lands, auto-retry a hat ≤2× if it dies. Post progress as a table.
4. Run the deduplicator (references/role-deduplicator.md) as a fifth Opus sub-agent once all four hats return, with the four raw role outputs appended verbatim to its prompt. It merges duplicates by file:line+root-cause, preserves the highest severity per issue, records which hats corroborated each, surfaces claim conflicts + coverage gaps, and does NOT re-score or re-review the artifact.
5. Assemble the final Output shape: the four role sections verbatim + the deduplicator's consolidated block + the Bottom line. The caller does not editorialize the merge — that is the deduplicator's job.

Why five sub-agents beat one inline pass: deeper per-lens source coverage, no cross-hat anchoring on a shared number, true wall-clock parallelism, an auditable per-hat file-read list, and a merge step that keeps the highest severity without a human hand on the scale. The cost is five read/merge passes — accept it; review depth is the point. (And it is still one model on one framing — the Codex pass in Hard Rule 9 remains the real gate.)

Targeting & relationship to /code-review

rijuls-review accepts the same artifact forms the built-in /code-review does, plus design docs/plans:

Artifact	How to point the hats at it
Working diff (default)	git diff origin/main...HEAD in the current worktree — the changed files ARE the read-list seed.
A commit / range	the SHA(s); read-list = the touched files + their callers/consumers.
A GitHub PR (PR#)	gh pr diff <#> / gh pr view <#> — review a PR branch in a throwaway worktree.
A design doc / plan	the .md path; read-list = every code file/symbol the doc's decisions land in.

How it relates to the built-in /code-review: they are complementary, not duplicates.

• /code-review (harness-built-in) is the fast, inline, single-pass review of the working diff that reports via the ReportFindings tool. Reach for it for a quick lint-grade pass, and for the billed cloud /code-review ultra multi-agent run on a branch/PR (user-triggered only — you cannot launch ultra yourself).
• rijuls-review (this skill) is the deep, scored, five-agent companion: four independent hats each producing a NN% + ranked P0–P3 findings, reconciled by a deduplicator, with the generalized bug-class Hard Rules baked in. Reach for it before approving a non-trivial plan/design/PR, when Rijul asks for confidence scores or a multi-role stress-test, or as the pre-Codex gate in the plan-review-implement loop.

Rule of thumb: /code-review for a quick diff check; rijuls-review when a number and a fix-first/approve decision are on the line — then the independent Codex pass before it's locked.

Self-improvement — feed external-review findings back in

This review exists to catch what a single read misses. When it misses something and an external reviewer catches it — most often a Codex review comment Rijul relays, but any human/tool finding on a PR — that miss is a signal this review's checklist has a gap. Close it every time, unprompted (a relayed Codex comment is a standing trigger, not a one-off):

1. Fix the finding in code, with a regression test + real-runtime verification appropriate to the finding (e.g. a real-browser check when it's a native-DOM behavior — Hard Rule 7).
2. Distill the generalizable lesson — not "this PR had X" but the class of bug and the check that would have caught it (e.g. "global shortcuts fire behind a modal" → "when a <dialog> is introduced, audit every global keydown listener").
3. Write it into this skill — a Hard Rule in SKILL.md if it's a cross-cutting verification discipline every hat must apply; a clause in the owning references/role-*.md if it belongs to one perspective's method; or the references/finding-format.md if it's about severity/fields. Always with a concrete worked example.
4. Write it into your own dev-workflow / conventions skill (or CLAUDE.md) — the bug class also belongs wherever your standing engineering rules live, cross-linked, so the lesson applies beyond this one review. Keep in-flight project state out of it.

The goal: a finding an external reviewer had to catch once becomes a finding this review catches on its own next time — the hats get strictly stronger over time.