review-pr

Estrelas8

Forks0

Atualizado26 de julho de 2026 às 05:28

Review an open GitHub PR on the Irreden Engine repo and post a structured review. Use when the user says "review PR <N>", "review #<N>", "review the last PR", "check my PR", "review any new PRs", "review the PR queue", "check for new PRs to review", "review the open PRs", or otherwise asks for a code review — and also invoke automatically from a persistent reviewer-loop session when `gh pr list` surfaces a PR that has not yet been reviewed by this fleet. Designed to be invoked by a dedicated reviewer agent (dispatched into a pool worktree) whose job is to look at another agent's work with a fresh context. Writes a structured review covering ownership, ECS invariants, allocation hot paths, naming, tests, and project-specific smells, then posts it as a PR comment.

Instalação

Instalar com Codex ou Claude Copie este prompt, cole no Codex, Claude ou outro assistente e deixe que ele revise a página da skill e instale para você.

Fonte

jakildev

jakildev/IrredenEngine

Abrir repositório GitHub Ver repositórios do creator

Download

Ocupações relacionadasSOC

Baseado na classificação ocupacional SOC

Analistas de garantia de qualidade de software e testadoresInformática e Matemática·SOC 15-1253

Explorador de arquivos

4 arquivos

SKILL.md

readonly

Mais deste repositório

mesmo repositório

simplify

jakildev/IrredenEngine

Polish the dirty working tree before committing — catch Irreden-Engine smells that a reviewer would otherwise flag. Use whenever you're about to commit, after addressing review feedback, after amending a commit, or whenever the user says "simplify", "clean up", "polish", "self-review", or "review my changes". Also auto-invoked by commit-and-push before the commit message is drafted. The skill finds per-entity getComponent in tick functions, allocation in hot loops, ECS naming convention slips, opportunities to reuse existing helpers, dead code, debug logs left behind, tautological comments, triple-nested loops over voxel volumes, renderer leaks from creation code, CPU-side SDF grid evaluation, linear-search in save/load hot paths, and stale or drifting CLAUDE.md / role / skill docs. Dispatches a parallel reuse-detection subagent fan-out so the reuse pass runs concurrently with the main checks. Applies safe fixes inline and reports anything that needs human judgment. Saves a review-fix-rereview round-trip.

2026-07-268

commit-and-push

jakildev/IrredenEngine

Stage, commit, push a feature branch, and open a GitHub PR against master for the Irreden Engine repo. Use whenever the user says "commit", "commit my changes", "commit and push", "open a PR", "make a PR", "wrap up this chunk", or otherwise indicates the current slice of work is ready for review. The skill assumes the parallel-agent workflow where work happens on short-lived feature branches, another agent reviews the PR, and the user merges via the GitHub UI. NEVER commit directly to master.

2026-07-218

platform-catchup

jakildev/IrredenEngine

Process the backlog of `fleet:needs-<host>-smoke` PRs on the current host. Builds a representative demo cohort against current `origin/master`, runs each with `--auto-screenshot`, and on green batch-swaps `fleet:needs-<host>-smoke` → `fleet:verified-<host>` on every eligible merged PR. On red (build break), files / opens a fix PR for the offender and holds labels on PRs that touch the implicated source paths until the fix lands. On partial (per-demo runtime hang or crash but build green), sweeps the rest, then fix-forwards the offending demo this session (issue only as the documented fallback). Runs on Linux, macOS, AND native Windows (MSYS2 mingw64) — Windows is now the primary validation host. Use when the human cues "platform-catchup", "catch up smoke tests", or "catch up Windows builds". Skill is **cue-only**, never auto-run — multi-target builds and ~40-demo runs are expensive, the human chooses when to spend them.

2026-07-158

render-debug-loop

jakildev/IrredenEngine

Build, run, and visually evaluate a rendering demo in an automated loop. Captures screenshots across a demo's configured shot list (zoom, camera offset, render-mode combinations), reads each image, diagnoses rendering issues against a topic-indexed reference (trixel / SDF shapes, lighting, backend parity), applies fixes, and repeats. Use whenever iterating on render pipeline bugs, shape alignment, lighting, parity drift, or any visual regression in the engine.

2026-07-158

render-verify

jakildev/IrredenEngine

Automated pass/fail render-regression harness for Irreden Engine demos. Wraps build → run → capture → compare → report: builds a demo, runs it with `--auto-screenshot`, then compares each shot against a committed reference image using a stdlib-only Python comparator. Reports per-shot pass/fail with diff images written for mismatches. Use after any render pipeline change to catch visual regressions automatically, or as a sanity pass on `master` before cutting releases.

2026-07-158

attach-screenshots

jakildev/IrredenEngine

Capture before/after screenshots for an engine rendering PR and commit them to `docs/pr-screenshots/<branch>/` so the PR body can embed them via raw GitHub URLs. Runs an auto-screenshot-capable demo (default `IRShapeDebug`) once against origin/master and once against the dirty working tree — or, in `--two-ref` mode, between two committed refs for the feedback-AMEND path — pairs the outputs by shot label, and prints a markdown snippet the worker pastes into the PR body. Invoke when a PR touches `engine/render/`, `engine/prefabs/irreden/render/`, any `.glsl`/`.metal` shader, or `creations/demos/*/src/`, so reviewers can see the visual delta without running the executable themselves.

2026-07-148

name

review-pr

description

review-pr (Irreden Engine)

The flow lives in docs/agents/skills/review-pr.md. Read it first, then apply the engine deltas below — most importantly the engine review checklist (the shared flow's step 4) and the exact verdict-label swap commands (step 5b). See docs/design/skill-sharing.md for why.

Deltas (Irreden Engine)

Delta key	Engine value
repo	`jakildev/IrredenEngine` (inline comments: `gh api repos/jakildev/IrredenEngine/pulls/<N>/comments`)
claim tool	`fleet-claim` (bail release: `fleet-claim review-release <N> <worktree>`)
default branch	`master`
review checklist	the engine checklist below
acceptance grader	`review-acceptance` — dispatch per shared-flow step 4b; verdict mapping below
smoke procedure	`procedures/cross-host-smoke.md`
re-review procedure	`procedures/re-review.md`
stacked-review procedure	`procedures/stacked-pr-review.md`
fleet doc	`docs/agents/FLEET.md` — "Model split"

Engine review checklist (shared-flow step 4)

Go through each explicitly. For every item, confirm compliance or raise an issue.

ECS invariants — check against .claude/rules/cpp-ecs-smells.md.

Ownership / lifetime

shared_ptr where unique_ptr would do.
Raw owning pointers (raw pointer = non-owning, always).
Storing references or pointers to ECS component storage across ticks — archetype changes invalidate addresses.
Capturing this or references to World managers in lambdas that outlive the World (e.g. lua callbacks registered before World teardown).
Stored g_*Manager pointer or reference in any object whose lifetime can outlive World (background threads, sol2 callback closures, long-lived caches) — see engine/world/CLAUDE.md and engine/CLAUDE.md.
New mutable namespace-scope variable in a header (inline or extern) outside a module ir_*.hpp entry point — state with no owner, teardown, or reset participation. Belongs on a manager, a singleton component (IREntity::singleton<T>), or a .cpp anonymous namespace; sanctioned patterns + detection grep in .claude/rules/cpp-globals.md.

Render pipeline

CPU frame-data struct out of sync with its GLSL layout(std140) counterpart. vec3 members pad to 16 bytes; array elements stride to 16 bytes; members crossing a 16-byte boundary need alignas(16). If either the C++ struct (in engine/render/include/irreden/render/) or its shader uniform block changed, cross-reference both sides.
shader references binding = N but the C++ kBufferIndex_* constant was not updated — the mismatch is silent. Confirm every bind-point index agrees on both sides.
New shader file not following the c_ / v_ / f_ / g_ prefix.
Canvas allocation before the canvas entity exists.
Compute dispatch size doesn't match voxelDispatchGridForCount().
A shader that writes indirect-dispatch dims from a runtime count without capping numGroupsX at 1024 and spilling to numGroupsY (mirror writeDispatchDims() in c_voxel_visibility_compact.glsl) — an uncapped 1-D grid is undefined past 65535 groups.
A new runtime uniform mode-branch added to a hot compute kernel (voxel stage 1/2, c_shapes_to_trixel, lighting / sun-shadow / particle kernels) — predicated, not skipped; should be a compile-time #if specialization from a shared body (docs/design/gpu-stage-timing-cost-model.md §2).
A rebased shader-kernel/encoding PR whose fork predates a carrier or encoding migration on master: verify every encode*/decode* call's arity and that the migration's carrier is threaded on the enabled path — byte-identity at default doesn't prove it (engine/render/CLAUDE.md §"Verifying render changes").
new *.glsl added without a matching *.metal counterpart (if parity is intentionally deferred, the PR body must acknowledge it and reference a follow-up task).

Lighting (if the diff touches system_*ao*, system_*shadow*, system_*flood*, system_*fog*, system_build_light_occlusion_grid*, or any c_compute_*shadow*.glsl / .metal)

Check 1 (grid coverage): system_build_light_occlusion_grid.hpp iterates the full voxel pool — it does not include cull_viewport_state.hpp and does not call visibleIsoViewport. Off-screen geometry participates in lighting by design.
Check 2 (shadow-ring extent): if chunk streaming is involved, the resident-chunk set extends past the view frustum by maxCasterHeight × cot(sunAltitude) in the sun-projection direction.
Check 3 (light-seed expansion): the flood-fill seed gather does not filter by visibleIsoViewport without expanding by C_LightSource::radius_. Off-screen light sources within radius must still seed on-screen tiles.
Check 4 (AO/shadow guard band): when chunk streaming is active, the resident chunk set includes a 1-chunk guard band in all six directions for correct AO neighbor-voxel sampling.

Math / coordinates (see .claude/rules/cpp-math.md for the full glm→IRMath substitution table)

Mixing 3D world coords with iso 2D coords without going through IRMath::pos3DtoPos2DIso or a named helper.
Hard-coded x + y + z where IRMath::pos3DtoDistance exists.
PlaneIso axis swapped.

Serialization (when the diff touches engine/asset/, engine/prefabs/irreden/voxel/, or engine/world/)

A struct annotated // IRAsset: serialized gained, lost, or renamed a field without bumping static constexpr uint16_t kSaveVersion = N; in the same diff. Fix: increment kSaveVersion and add a reader migration keyed on (structType, oldVersion) in the format's load function. (Save Format Extensibility Rule #3.)
A new serialized record type with no // IRAsset: serialized annotation — the simplify and review-pr checks cannot guard it without the annotation. Add the annotation and set kSaveVersion = 1 on the struct.
A loader that mutates live state (entity splice, setParent, id-watermark advance) with a fallible decode positioned after its first mutation — a mid-apply decode failure leaves the world partially mutated. All decoding must complete (into staged buffers) before the first live mutation; see engine/asset/CLAUDE.md Extensibility Rule #5 (recurred #2228 → #2230).
SaveSerialize<C>::write reading a per-frame-derived field instead of the authored source-of-truth. When a system re-derives the component's live representation each frame (rotation re-voxelize, GPU-owned state), the primary field holds a derived arrangement and the authored data sits in a companion field (*Source* / *Snapshot* / pending*, e.g. rotationSourceVoxels_) — verify the write reads the companion (PR #2240).

Naming / style — follow the table in docs/agents/CLAUDE-BASELINE.md §Naming.

Private members take m_; public members take a trailing _. The reversed form — m_ on a public member, or a trailing _ on a private member — is the single most common slip. A private member that already carries m_ is correct; do not flag it (this inversion produced a false-positive nit on PR #1706).
Block comments that narrate change / investigation history — issue-by-issue forensics, Before #X / now Y, was a misdiagnosis, retired (T-323) — instead of stating a durable invariant. Durable why stays; history moves to the commit / PR / a docs/design/ doc with at most a one-line // see #N backref. Single-sourced in CLAUDE-BASELINE.md §Style; most common in render-pipeline diffs.

Tests / build

Code change with no corresponding test change where a test existed.
New public surface with no covering test — judge per surface, not per PR: a PR that tests one surface while shipping others uncovered still fails this check; flag each uncovered surface as needs-fix unless the human explicitly waived tests ("no tests" stays a human-explicit waiver). Specifically call out:
- a new Lua binding needs a test/script/lua_*_test.cpp exercising the sol2 seam (the existing lua_*_test.cpp files there are the precedent);
- a facility with two registration paths (free-function params AND System<N> spec-member detection) needs both paths covered. Worked example (#2425): the per-system cadence gate shipped 13 solid SystemManager tests, but the new IRSystem.* cadence Lua surface and the kCadence/kCadenceOffset spec-member path shipped with zero coverage — under the old "a new test exists" wording that was unflaggable; under per-surface wording each gap is a needs-fix hook. Suite layout and authoring conventions: test/CLAUDE.md.
Build or format-check not run before opening the PR (check the commit message, or run fleet-build --target format-changed yourself if cheap).
Verification claims green over an unclean exit: a PR body or run log that reports a crash / RESULT=CRASH (teardown included) but presents the verification as passing — needs-fix per docs/agents/FLEET.md §"Clean-exit policy". A crash observed but out of reach must be filed with forensics AND the lane reported failed.

Acceptance vs the originating issue (shared-flow step 4b)

When the PR body carries Closes #N and issue N has a ## Plan with ### Acceptance criteria, dispatch the acceptance grader (review-acceptance) with the PR + issue numbers and fold its fragment into the review as ### Acceptance (issue #N).
Verdict mapping: any unmet criterion is at least needs-fix — blocker if the miss means the shipped mechanism doesn't function at all. A missing ## Acceptance evidence PR-body section whose criteria you can still establish from the diff is a nit; a criterion you cannot establish is unmet, not a nit.
unverifiable on rows are not penalized — confirm the smoke labels from step 5c cover the lane the grader named.

Opportunistic fixes (fix-forward covenant)

A clearly-sectioned ## Opportunistic fixes block in the PR body is the expected shape under FLEET.md §"Fix-forward", not scope creep — review the bundled fixes on their merits. Ask for a split only when a bundled fix materially raises the PR's risk (schema changes, cross-module refactors, behavior changes beyond what the section claims).

Opus-only items (Sonnet should not attempt these — escalate via the verdict footer if any of these surfaces are touched)

GPU buffer lifetime across frames. An SSBO/UBO bound on frame N and read on frame N+1 without a fence or explicit double-buffer swap; async readback recycled before completion (use-after-free).
Archetype-mutation race during structural-change deferral. A tick that calls addComponent/removeComponent/removeEntity must use the deferred variant; touching the live archetype while a parallel system iterates invalidates component addresses silently.
Race between flushStructuralChanges and async GPU readback. The readback's destination (or the entity it indexes) may vanish if the flush runs first — verify the readback's lifetime is decoupled (stable ID or its own refcount).
Allocator behavior in long-lived caches. An unordered_map<EntityId,T> with a poor hash, or a never-shrunk vector<T>, grows unbounded — confirm an eviction path or clear teardown order.
Hot-path register pressure. A per-entity tick touching >8 components or carrying >12 live locals across a loop body approaches the register file; flag tick paths that grew significantly, especially in engine/render/ or engine/world/.

Creation- or implementation-specific criteria

If the PR touches a subdirectory under creations/ (or any implementation layered on the engine), read the nearest CLAUDE.md there in addition to this engine checklist. The engine checklist is the baseline; the creation's CLAUDE.md is the delta. Apply both. If the subdirectory has a dedicated REVIEW.md, prefer it for review-specific rules.

Flow docs (markdown-only PRs skip the code checklist — still check this)

A bash fence in a changed .md that runs gh pr edit|create / gh issue create with --body "$var" instead of --body-file, or that consumes a $var in a --body/substitution line never assigned in the same fence — a worker running it literally executes --body "" and wipes the PR body (#2342; the --body-file rule generalizes REVIEWER-PROTOCOL.md §Posting the review body to flow docs).

Fleet scripts (if the diff touches scripts/fleet/)

Inline comments — layout diagrams, role-list blocks, --help banners — still naming the old identifiers after a rename. grep the renamed-from names across the touched scripts; comment blocks drift silently because nothing compiles them.
A ~/.fleet/state/state.json consumer judging cache freshness from file mtime instead of the in-file generated_at (UTC epoch; st_mtime fallback only when generated_at is missing/unparseable) — followers rewrite the file each tick with the leader's generated_at preserved, so mtime lies. Shared helper: fleet_poll_topology.state_age_seconds; rule + rationale in docs/agents/FLEET-RUNTIME.md (recurred: fleet-dispatcher, #2231).

Verdict-label swap commands (shared-flow step 5b)

Run the matching command as your very next bash call after gh pr review:

# approve, no nits in body:
gh pr edit <N> --remove-label "fleet:needs-fix" --remove-label "fleet:blocker" --remove-label "fleet:has-nits" --remove-label "fleet:awaiting-upstream-review" --remove-label "fleet:stacked-rebase" --remove-label "fleet:needs-base-update" --add-label "fleet:approved"

# approve WITH a non-empty Nits section:
gh pr edit <N> --remove-label "fleet:needs-fix" --remove-label "fleet:blocker" --remove-label "fleet:awaiting-upstream-review" --remove-label "fleet:stacked-rebase" --remove-label "fleet:needs-base-update" --add-label "fleet:approved" --add-label "fleet:has-nits"

# needs-fix (nits roll into the fix work; no separate label):
gh pr edit <N> --remove-label "fleet:approved" --remove-label "fleet:blocker" --remove-label "fleet:has-nits" --remove-label "fleet:awaiting-upstream-review" --remove-label "fleet:stacked-rebase" --remove-label "fleet:needs-base-update" --add-label "fleet:needs-fix"

# blocker:
gh pr edit <N> --remove-label "fleet:approved" --remove-label "fleet:needs-fix" --remove-label "fleet:has-nits" --remove-label "fleet:awaiting-upstream-review" --remove-label "fleet:stacked-rebase" --remove-label "fleet:needs-base-update" --add-label "fleet:blocker"

fleet:has-nits rides on top of fleet:approved and tells the author "approved, clean up the nits before this lands" — author roles poll for it.

Feedback-fix model class: the dispatcher routes the fix iteration from your labels — nits-only feedback runs on sonnet, fleet:needs-fix / fleet:blocker runs on the opus class. If your findings indicate the approach itself is wrong (not fixable with scoped edits), additionally --add-label "fleet:fable" so the fix dispatches on the fable class — or escalate fleet:design-blocked if it needs an architect decision.

Engine notes

Step 1's metadata fetch uses gh api repos/jakildev/IrredenEngine/pulls/<N>/comments for inline comments; gh pr view --json does not include them.
Bail-label release uses fleet-claim review-release <N> <your-worktree-name>.
The procedure files (procedures/*.md) say "SKILL.md step N" — read it as step N of the shared flow in docs/agents/skills/review-pr.md (the step numbers are preserved); this wrapper is the entry point that points there.