# Hera Review > Default, user-overridable code review methodology — the review CONTRACT (what to analyze, how to tag findings) that a broad finder follows when spawned inside a hera-spawn-review panel, or that runs standalone via /hera-review for a single-pass review outside a panel. Encodes ralph-review's review contract (behavior / delta / plan / regression / security / test-coverage audits, the canonical finding tags) without ralph's loop control, auto-fix execution, or OpenSpec-drift resolution flow — those stay with hera-spawn-review (the panel synthesizer + fix loop) or with the invoking user when this runs standalone. Named in a diligence profile's [panel] as review_skill = "hera-review" (the shipped default); a project overrides it freely with its own review_skill or review_instruction — swapping it never requires touching hera-spawn-review. - Skill: `drn/hera-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add drn/hera-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/drn/hera-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: drn (https://skillmd.com/u/drn) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/drn/hera-review --- # hera-review — default review instruction This is a review **methodology**, not an orchestrator. It tells a single reviewer what to analyze and how to tag findings. It does not spawn other agents, does not synthesize across multiple reviewers, does not decide what gets auto-fixed, and does not loop. That machinery lives in `hera-spawn-review` (the glue that injects this instruction into each broad finder and owns the synthesis + fix + re-review cycle). **Two ways this runs:** - **Injected into a panel finder.** `hera-spawn-review` reads this file's body and pastes it into a spawned sub-agent's prompt, alongside the diff packet. Your `[AUTO-FIX]` tag is a *recommendation* in that context — the panel's synthesizer re-derives the real classification and owns the final auto-fix call. Do not apply fixes yourself when running inside a panel; only produce the findings report. - **Standalone**, invoked directly (e.g. `/hera-review`) outside any panel. Same analysis and tagging; there is no synthesizer to hand off to, so just report — acting on the findings (or not) is the invoking user's call, not this instruction's. Either way, your job ends at a tagged findings report. Never edit files as part of following this instruction. ## 1. Gather what you need If a diff and file contents have already been supplied to you in the prompt above this instruction, review those — do not re-gather them. Otherwise, determine the diff scope yourself, in this order, and stop at the first match: ``` 1. Unstaged or staged changes exist → diff scope = working tree (git diff / git diff --cached) 2. Current branch != the repo's default branch → diff scope = branch vs default (git diff {default}...HEAD) 3. Otherwise → diff scope = local vs origin (git diff HEAD...origin/{default}) ``` Then collect: - The full diff for that scope. - The full contents of every changed file (not just the hunks — you need whole-file context). - Files that import, require, or otherwise reference the changed files (Grep for the symbol/path, read the hits). Regressions often live in a caller, not the changed file itself. - If `openspec/` exists: the active change's delta specs (`openspec/changes//specs//spec.md`) and, for context, the base specs (`openspec/specs//spec.md`) for every capability touched. If the change has already been archived, read the deltas from `openspec/changes/archive//specs/.../spec.md` in place instead — archives are immutable, so a divergence from an archived delta is a `[QUESTION]` (see below), never something you edit. - `design.md` and `tasks.md` from the active (or archived) change, if present, for structural context. - The project's test/lint baseline: run the test suite and linter if you can identify them; record PASS/FAIL/N/A and CLEAN/WARNINGS/ERRORS. If running them isn't practical in your context, say so explicitly rather than guessing. ## 2. Set your confidence tier ``` Active change resolved AND deltas exist → tier = "spec" (behavioral + structural + bugs/security/lint) tasks.md exists (no deltas) → tier = "plan" (structural + bugs/security/lint; deltas advisory) Neither → tier = "conservative" (bugs/security/lint only) ``` Deltas are the spec source of truth for the in-flight change; base specs are context for unchanged behavior; `tasks.md` is the structural plan. In `plan` or `conservative` tier, skip all delta-compliance analysis below and skip `[SPEC-DRIFT]` entirely — anything that would have been drift becomes a `[QUESTION]` instead. ## 3. Perform every analysis below — do not skip any **Suppression convention:** a line annotated `// expected:` (or `# expected:` in Python/shell/YAML) has already been reviewed and acknowledged. Do not re-report a finding on that line unless the surrounding logic has materially changed since the annotation was added — read the comment's reasoning before deciding. Tag these `[ACKNOWLEDGED]` (see below), not silently omitted. ### Behavior audit For every modified function, method, type, or exported symbol: what was the old behavior, what is the new behavior, is the change intentional and justified, could any caller break, are defaults / return types / error conditions / side effects altered? ### Delta compliance (spec tier only) For every behavioral change: does an active-change delta describe the expected behavior (in `## ADDED/MODIFIED/REMOVED/RENAMED Requirements`)? Does the code match the delta's requirement and scenarios? Is there new behavior the deltas don't mention (→ `[SPEC-DRIFT]`)? Does the code contradict a delta scenario (→ `[SPEC-DRIFT]`)? For `MODIFIED` requirements, does the code reflect the new wording, not the base spec's old wording? ### Plan compliance (spec and plan tiers) Does file organization match `tasks.md`/`design.md`? Are components placed where the plan specifies? Are the plan's stated patterns actually followed? ### Regression risk For each changed file: what depends on it? Could the change break a dependent? Are old edge cases still handled? Did error-handling or concurrency patterns change? ### Security audit Injection flaws, auth changes, credential exposure, input validation, XSS, insecure deserialization, vulnerable dependencies, error-message info leaks, missing rate limiting, crypto misuse, path traversal, SSRF. ### Test coverage gaps Are behavior changes covered by tests? What test cases are missing? Are error paths and edge cases tested? If a delta describes behavior the code implements correctly but no test covers it, that's a coverage gap — tag `[AUTO-FIX]` (write the test), not `[SPEC-DRIFT]`. ### Line-by-line diff review For each hunk: is removed code safe to remove? Is added code correct — trace the logic. Off-by-one errors, nil/null risks, type mismatches, boundary conditions. ## 4. Classify every finding with exactly one tag **`[AUTO-FIX]`** — you are confident this should be fixed, and one of: a delta scenario states the expected behavior and code doesn't match; a test should pass but doesn't and the fix is clear; a delta is implemented correctly but tests are missing (write them — this is a coverage gap, not drift); an obvious bug (nil pointer, off-by-one, unclosed resource, type mismatch); a security issue; a lint violation with an unambiguous fix; `tasks.md` explicitly specifies structure and the code deviates. Include: file:line, what's wrong, what the fix should be (specific enough to implement), and why you're confident (cite the delta requirement/scenario, base spec, test, or bug category). **`[QUESTION]`** — needs user judgment: deltas are silent; deltas and code contradict but it's unclear which is stale; multiple valid fixes exist; a design concern isn't addressed by deltas or `tasks.md`; a behavioral change looks intentional but has no delta coverage. Include: file:line, and a plain-language summary of what's happening and its consequence (2-3 sentences, lead with observable behavior — "the UI freezes briefly during X" not "functionA() calls sleep()") — write for someone who didn't author the code. **`[SPEC-DRIFT]`** — spec tier only. Behavioral code with no delta coverage: new behavior the active change's deltas don't mention, or changed behavior that contradicts a delta scenario. Include: file:line, what the code does, what the deltas say (or "deltas are silent"), and a draft recommendation for what the delta should say (capability, ADDED/MODIFIED, requirement text, scenarios). **`[ACKNOWLEDGED]`** — suppressed by a still-valid `// expected:` annotation. List separately for transparency; do not re-report. If the surrounding logic changed enough that the annotation may be stale, reclassify as `[QUESTION]` noting the annotation should be reviewed. **`[SKIP]`** — not actionable: stylistic preference with no delta or plan opinion, or "could be improved" with no correctness impact. ## 5. Output format ```markdown ### Findings 1. **[AUTO-FIX]** `file.go:42` — Description. Fix: . Confidence: . 2. **[QUESTION]** `handler.go:15` — Description. Needs: . 3. **[SPEC-DRIFT]** `auth.go:88` — New behavior. Deltas are silent. Recommend: "Add to openspec/changes//specs/auth/spec.md: ..." 4. **[SKIP]** `utils.go:20` — Could use a clearer variable name. ### Summary - AUTO-FIX: {N} QUESTION: {N} SPEC-DRIFT: {N} ACKNOWLEDGED: {N} SKIP: {N} - Regression confidence: HIGH / MEDIUM / LOW ``` Do not rush. Analyze every changed line. False positives are acceptable; false negatives are not — a `hera-spawn-review` panel run relies on you surfacing everything you noticed so the synthesizer (or the user, standalone) can decide what matters, not on you pre-filtering.