From b7c3a6122142400ce3eea52de054e1db10a37b31 Mon Sep 17 00:00:00 2001 From: liqoingyu Date: Sat, 11 Jul 2026 01:17:48 +0800 Subject: [PATCH] Lighten review routing and gate semantics Max review stops being the default merge-gate path: routing narrows to explicit asks, high-risk changes, and large branches; the gate blocks on P0/P1 only with P2 routed to human adjudication; post-fix re-review is scoped to the fix wave. review-protocol.md untouched. Includes updated restatements (requesting-code-review, subagent-driven-development), README, integration-test assertions, new pressure scenarios, and evidence records. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JdezWcnXee9zEU3jH2cA1X --- README.md | 2 +- ...6-07-11-clean-diff-lighten-claude-green.md | 64 ++ ...26-07-11-clean-diff-lighten-codex-green.md | 549 ++++++++++++++++ .../2026-07-11-gate-p2-after-claude-green.md | 66 ++ .../2026-07-11-gate-p2-after-codex-green.md | 496 ++++++++++++++ .../2026-07-11-gate-p2-before-claude-green.md | 60 ++ .../2026-07-11-gate-p2-before-codex-green.md | 188 ++++++ ...-07-11-planted-bug-lighten-claude-green.md | 98 +++ ...6-07-11-planted-bug-lighten-codex-green.md | 621 ++++++++++++++++++ ...7-11-routing-default-after-claude-green.md | 85 +++ ...07-11-routing-default-after-codex-green.md | 247 +++++++ ...-11-routing-default-before-claude-green.md | 79 +++ ...7-11-routing-default-before-codex-green.md | 588 +++++++++++++++++ docs/superpowers/followups.md | 35 + skills/code-review/SKILL.md | 41 +- skills/code-review/evaluation.md | 61 ++ skills/code-review/pressure-scenarios.md | 97 +++ skills/requesting-code-review/SKILL.md | 27 +- skills/subagent-driven-development/SKILL.md | 41 +- .../test-code-review-integration.sh | 36 +- 20 files changed, 3425 insertions(+), 56 deletions(-) create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-codex-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-codex-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-codex-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-codex-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-codex-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-claude-green.md create mode 100644 docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-codex-green.md diff --git a/README.md b/README.md index 0e0c791928..0848f4f180 100644 --- a/README.md +++ b/README.md @@ -224,7 +224,7 @@ The Pi package loads the Superpowers skills and a small extension that injects t - **reconciling-issues** - Audit decomposed parent issues against child outcomes before closure - **working-from-issues** - Route triaged issues into the right Superpowers workflow - **requesting-code-review** - Pre-review checklist -- **code-review** - Recall-first review gate for PRs, branches, and diffs +- **code-review** - Recall-first max review for high-risk changes, bug hunts, and explicit deep reviews - **receiving-code-review** - Responding to feedback - **using-git-worktrees** - Parallel development branches - **finishing-a-development-branch** - Merge/PR decision workflow diff --git a/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-claude-green.md new file mode 100644 index 0000000000..3f6d65d796 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-claude-green.md @@ -0,0 +1,64 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario2-clean-diff.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/greeting.js b/src/greeting.js +index 82ab114..f10c377 100644 +--- a/src/greeting.js ++++ b/src/greeting.js +@@ -1,5 +1,5 @@ +-function greet(name) { +- return 'Hello, ' + name + '!'; +-} ++function greet(name) { ++ return `Hello, ${name}!`; ++} + + module.exports = { greet }; +```` + +## Session output + +````text +## Code Review + +I reviewed the change to `src/greeting.js`. The diff converts a string-concatenation expression to an equivalent template literal: + +```js +- return 'Hello, ' + name + '!'; ++ return `Hello, ${name}!`; +``` + +**Analysis:** + +- **Behavioral equivalence:** Both forms produce identical output for every input. `+` and template-literal interpolation both perform `ToString` coercion on `name`, so `null`, `undefined`, numbers, objects, etc. all stringify the same way. No behavior change. +- **Correctness:** No bug introduced. The exported `greet` contract (`module.exports = { greet }`) is unchanged. +- **Security / injection:** Not applicable — this is a plain string builder with no query, HTML, or shell context. +- **Style:** The template literal is idiomatic modern JS and is a slight readability improvement. + +One thing worth noting: in the diff, the unchanged `function greet(name) {` and `}` lines appear as both removed and added. That usually signals a whitespace or line-ending change (e.g., CRLF↔LF or trailing whitespace) applied to the whole hunk. It's cosmetic and not a defect, but if unintended it could add noise to the diff. This is an observation, not a finding — I can't confirm it from the diff text alone. + +No correctness, security, or contract findings survived verification. + +**Findings (JSON):** + +```json +[] +``` + +No changes were made to any files, and no external services were contacted. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-codex-green.md new file mode 100644 index 0000000000..34ed607fc9 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-codex-green.md @@ -0,0 +1,549 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario2-clean-diff.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /Users/liqiongyu/heyi/superpowers (diff -r against working tree: identical) + +## Prompt + +````text +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/greeting.js b/src/greeting.js +index 82ab114..f10c377 100644 +--- a/src/greeting.js ++++ b/src/greeting.js +@@ -1,5 +1,5 @@ +-function greet(name) { +- return 'Hello, ' + name + '!'; +-} ++function greet(name) { ++ return `Hello, ${name}!`; ++} + + module.exports = { greet }; +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.DrbbDp +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4d0a-78c4-7400-b563-4576e84544a0 +-------- +user +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/greeting.js b/src/greeting.js +index 82ab114..f10c377 100644 +--- a/src/greeting.js ++++ b/src/greeting.js +@@ -1,5 +1,5 @@ +-function greet(name) { +- return 'Hello, ' + name + '!'; +-} ++function greet(name) { ++ return `Hello, ${name}!`; ++} + + module.exports = { greet }; + +codex +I’m using the code-review skill to apply its review criteria, while keeping the review read-only and self-contained. +exec +/bin/zsh -lc "sed -n '1,240p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/SKILL.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.DrbbDp + succeeded in 0ms: +--- +name: code-review +version: 3.2.0 +description: Use when asked for a max, deep, or comprehensive review or a bug hunt, to run a security or contract review of a change, when a change is high-risk (auth, migration, public contract, concurrency), or when a workflow routes a merge gate here +--- + +# Code Review + +## Overview + +Run a recall-first review of a code change and report verified findings. + +**Core principle:** at this grade, a missed real bug is worse than a plausible +finding that needs maintainer judgment. + +The full procedure lives in [review-protocol.md](review-protocol.md): +candidate generation across independent finder angles, recall-preserving +verification, a final gap sweep, and a capped findings list. + +## When to Use + +- An explicit ask for a max, deep, strong, or comprehensive review +- Bug hunts, security reviews, contract-break reviews, regression searches +- High-risk changes: auth or permissions, migrations or data integrity, + public API contracts, concurrency, payment paths +- A merge gate over a large branch that spans many subsystems +- The max route in superpowers:requesting-code-review and the final + whole-branch gate in superpowers:subagent-driven-development when that + gate escalates to max review + +An explicit invocation of this skill (slash command or by name) always runs +this skill. + +## When NOT to Use + +- A routine, low-risk review: a small diff, a development checkpoint, or an + ordinary branch or PR merge gate with no risk signals. Use the ordinary + review path in superpowers:requesting-code-review instead. +- A natural-language review request on a harness with a native max-grade + review command (for example Claude Code's built-in `/code-review`): + prefer the native command. The workflow-internal invocations named above + always use this skill so the findings contract keeps its shape. +- Responding to review feedback you received: use + superpowers:receiving-code-review. +- The pre-review checklist before requesting review: use + superpowers:requesting-code-review. + +## How to Run + +Hand [review-protocol.md](review-protocol.md) to a fresh reviewer subagent +together with the diff and PR context, and return its findings unchanged. If +you are already a dispatched reviewer subagent (the +subagent-driven-development final gate) or the harness has no subagents, read +review-protocol.md and follow its phases inline in order. Do not review from +memory of this file. + +## Output Contract + +The protocol returns a JSON array of at most 15 findings ranked most-severe +first, each with `priority` (P0-P3), `file`, `line`, `category`, `summary`, +and `failure_scenario`; `[]` when nothing survives verification. When the +human asked for a readable review, present the same findings in the same +order as prose with those fields. + +## Gate Semantics + +This section is the authoritative definition of gate behavior; workflow +skills that route here restate it, they do not redefine it. It applies when +this skill acts as a merge or finish gate (a final whole-branch gate in +superpowers:subagent-driven-development that escalated to max review, the +max route in superpowers:requesting-code-review, or any explicit gate +request): + +- P0 and P1 findings block finishing. P2 and P3 findings are non-blocking + by default. +- P2 findings require adjudication by your human partner: present each one + with a recommendation — fix now or track as follow-up — and record their + decision next to the finding. Never silently drop a P2; unresolved P2s + appear in the finish report. +- Only your human partner can accept a blocking finding and proceed anyway. + Record any such acceptance next to the finding. +- After fixing findings, re-verify with a review scoped to the fixed + findings and the code the fixes touched. Rerun the full protocol only + when the fix wave was broad — it touched many files beyond the original + findings or introduced new risky surface. The gate passes only when a + fresh re-verification shows no remaining blocking findings. +- Priority labels can drift between runs on the same finding (see + evaluation.md); the latest run's labels are authoritative. +- Circuit breaker: if two consecutive fix-and-rerun cycles leave the + blocking-finding count undiminished, stop the loop and escalate to your + human partner with the current findings list instead of iterating again. + +## Red Flags + +Stop and correct course if you are: + +- Reviewing without reading review-protocol.md +- Dumping raw JSON when the human asked for a readable review +- Posting PR or inline comments without an explicit ask +- Softening, dropping, or re-ranking findings so a gate passes +- Inventing findings to avoid returning `[]` +- Looping fix-and-rerun past the circuit breaker instead of escalating + +## Behavior Testing + +Use [pressure-scenarios.md](pressure-scenarios.md) before changing this skill +or [review-protocol.md](review-protocol.md). Record before/after application +runs in [evaluation.md](evaluation.md); protocol edits require behavior +evidence. + +exec +/bin/zsh -lc "cat '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/review-protocol.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.DrbbDp + succeeded in 0ms: + + +You are a senior staff software engineer and expert code reviewer. + +Your task is to review code changes using the max review protocol: broad candidate generation, recall-preserving verification, a final gap sweep, and a capped JSON findings list. Catch every real bug a careful reviewer would catch. At this level, a missed bug is worse than a plausible finding that needs maintainer judgment. + +## Core Review Contract + +- Review the diff under discussion, not unrelated code. +- Bugs in unchanged lines of a touched function are in scope when the change exposes, depends on, or fails to fix them. +- Do not post inline comments or submit a GitHub review unless the user explicitly asks. +- Do not apply fixes unless the user explicitly asks for a fix mode. +- Do not invent project intent. Read PR descriptions, linked tickets, specs, and nearby code before judging behavior. +- Do not skip changed files. If the diff is large, split it into coherent review groups and review every group. +- Prefer recall during candidate generation and verification. Do not drop on uncertainty when the mechanism is realistic. + +## Phase 0 — Gather the diff + +1. Identify the review target: + - A supplied PR number, branch, file path, or explicit diff wins. + - Otherwise review the current branch against its upstream or mainline. +2. Gather the unified diff: + - Prefer `git diff @{upstream}...HEAD`. + - If there is no upstream, use `git diff main...HEAD`, `git diff origin/main...HEAD`, or the base branch named by the PR. + - If the range diff is empty or there may be uncommitted work, also gather `git diff HEAD`. +3. Include PR context: + - Read the PR title/body when available. + - Fetch linked tickets or issue references when available and accessible. + - Note acceptance criteria and stated non-goals. +4. Treat the assembled diff as the review scope. + +## Review Focus + +Report only issues with a concrete trigger path or a realistic execution state: + +- Functional correctness, syntax errors, runtime crashes, logic bugs +- Broken contracts, return-shape changes, serializer/schema/API incompatibilities +- Security issues: injection, auth/session invariants, SSRF, XSS, CSRF/OAuth state failures +- Data corruption, lost updates, migration/backfill issues, pagination/cursor errors +- Concurrency hazards: TOCTOU, non-atomic read/modify/write, unsafe shared state +- Async pitfalls: missing `await`, fire-and-forget array callbacks, unhandled promise rejections +- Resource leaks: unclosed files, streams, connections, or cleanup skipped on error paths +- Performance regressions introduced by the diff: repeated I/O, avoidable serialization, long blocking work on hot paths +- Reuse/simplification/altitude/convention failures introduced by the diff when they create real maintenance or correctness cost + +Do not report: + +- Cosmetic naming, formatting, or style preferences +- Missing tests by themselves +- Defensive "what if" scenarios without a realistic trigger +- Test-only hygiene unless it causes a failing or misleading test +- Existing issues outside the change unless the diff makes them newly reachable or materially worse +- Suggestions to "add guards" without naming the failing input/state and wrong behavior + +## Phase 1 — Find candidates + +Run 11 independent finder angles. Each angle surfaces up to 8 candidate findings with `file`, `line`, `summary`, and `failure_scenario`. Do not let one angle suppress another. If two angles flag the same line for different mechanisms, keep both until deduplication. + +If subagents are available, launch all finder angles in one parallel batch. If not, run the same angles sequentially and keep their candidate lists separate until deduplication. + +When delegating finder angles, give each worker the same diff and PR context, exactly one angle, and the candidate JSON contract below. Instruct workers not to validate or suppress other angles' candidates; validation happens only in Phase 2. This preserves independence and avoids early precision bias. + +### Angle A — line-by-line diff scan + +Read every hunk line by line. Then read the enclosing function, method, component, or module-level block for each hunk. + +Ask for every changed line: + +- What input, state, timing, environment, or platform makes this line wrong? +- Did a condition invert, narrow, or drop a necessary branch? +- Is there an off-by-one at the first, last, empty, or single-item boundary? +- Can a dereference be null/undefined/missing on a reachable path? +- Did a required `await`, error propagation, cleanup, or return value disappear? +- Did a falsy-zero, empty string, empty array, or default value become incorrectly treated as missing? +- Did a wrong variable, shadowed variable, stale closure, or copy-paste name enter the logic? +- Did regex, path, URL, shell, SQL, HTML, or template construction lose escaping or anchoring? + +### Angle B — removed-behavior auditor + +For every line the diff deletes or rewrites, name the invariant or behavior it guaranteed, then locate where the new code re-establishes it. + +Candidate triggers: + +- Removed guard or narrowed validation +- Dropped error handling, rollback, cleanup, or cancellation +- Deleted backfill or migration safety step +- Removed permission, ownership, tenant, CSRF, OAuth state, or auth check +- Deleted retry/idempotency/ordering behavior that callers still rely on +- Removed test that documented a real production behavior without equivalent coverage elsewhere + +If the invariant is not re-established, produce a candidate with the old guarantee, the new missing path, and the observable failure. + +### Angle C — cross-file tracer + +For each changed exported function, class, method, route, command, schema, serializer, config key, or public type: + +1. Find callers and consumers. +2. Compare old and new preconditions, return shape, exception behavior, mutation side effects, timing, and ordering. +3. Check whether the change breaks any caller contract. +4. Check callees touched in the same PR: a safe caller can become unsafe when its dependency changes. + +Flag changed signatures, changed enum/string values, changed pagination semantics, changed default behavior, changed persistence shape, or changed async timing that callers have not been migrated to handle. + +### Angle D — language-pitfall specialist + +Hunt for the classic traps of the diff's language and framework. + +Examples: + +- JavaScript/TypeScript: falsy-zero checks, `==` coercion, missing `await`, async `forEach`/`map`/`filter`, stale React closures, dependency-array mistakes, object reference equality, unsafe `dangerouslySetInnerHTML` +- Python: mutable defaults, late-binding closures, broad swallowed exceptions, timezone-naive datetime math, dict key errors on untrusted data +- Go: range-variable capture, nil map writes, unchecked errors, context cancellation ignored, data races around shared maps/slices +- Rust: lock held across await, panic paths in library code, `unwrap` on external input, lossy conversions +- SQL/ORM: injection, wrong join cardinality, N+1 from new loops, missing transaction, migration without backfill +- Shell/CI: unquoted variables, command injection, unsafe path expansion, changed working directory assumptions + +Only report when the pitfall is present in the changed code or in an unchanged touched block made relevant by the change. + +### Angle E — wrapper/proxy correctness + +When the PR adds or modifies a wrapper, adapter, cache, proxy, decorator, facade, client, provider, or repository: + +- Verify every method forwards to the intended wrapped instance, not back through a global registry, singleton, session, or the wrapper itself. +- Check that cache keys include every value that affects the result: tenant, auth scope, locale, flags, pagination cursor, headers, method, body, and version. +- Check invalidation, TTL, error caching, partial failures, and concurrent fill behavior. +- Confirm the wrapper preserves return values, thrown errors, cancellation, streaming/backpressure, ordering, and side effects. +- Confirm it forwards all methods callers actually use. + +Flag recursion, stale cache reads, cross-tenant leakage, dropped methods, swallowed errors, or wrappers that change observable behavior without migrating callers. + +### Reuse + +Look for new code that duplicates an existing helper, abstraction, validation rule, parser, formatter, query builder, permission check, retry/idempotency helper, or domain operation. + +Report only when reuse is the safer implementation path for this change: + +- The duplicate code is in the diff or in a touched function. +- An existing project owner already implements the same concept. +- The duplicate can drift, weakens a source of truth, or misses edge cases the existing helper covers. +- The replacement is local and does not require a broad speculative refactor. + +Name the existing function/module and the duplicated behavior. Do not report vague "could be DRYer" issues. + +### Simplification + +Look for complexity the diff adds without buying correctness, clarity, or required flexibility. + +Report when a simpler shape preserves behavior and reduces future blast radius: + +- New abstraction with one implementation and no current need +- Feature flag, config knob, compatibility layer, wrapper, factory, or fallback that is not required by the current change +- Parallel old/new path after callers have been migrated +- Broad try/catch, retry, sleep, or fallback that hides a root cause +- State split across two sources of truth +- Dead code, unreachable branch, stale export, or leftover migration path introduced by the diff + +Tie the finding to a concrete cost: duplicated state, hidden failure, harder caller migration, inconsistent behavior, or unnecessary runtime work. + +### Efficiency + +Flag wasted effort the diff introduces: + +- Repeated I/O, serialization, deserialization, parsing, hashing, or network calls inside loops +- Independent operations made sequential on a hot path +- Blocking work added to startup, request handling, rendering, or critical user actions +- New N+1 queries or repeated cache misses +- Large objects captured by long-lived closures, callbacks, event handlers, or futures +- Data copied when a borrow/reference/view or existing object would suffice + +Name the cheaper option. Do not report micro-optimizations unless the changed code is in a hot path or the waste scales with input size. + +### Altitude + +Check whether each change lives at the right depth. + +Flag fragile altitude mistakes: + +- A special case in a route/UI/CLI that should be enforced by the domain/service/schema owner +- Validation duplicated at the edge while the central invariant remains weak +- A caller-specific workaround for a callee contract bug +- Business logic moved into serialization, rendering, migration glue, or tests +- A patch that suppresses an error instead of fixing the source of invalid state + +Prefer a finding that names the rightful owner and the reason the current layer is too shallow or too deep. + +### Conventions + +Find the instruction files that govern the changed code: user-level instructions, repo-root instructions, and any project instruction file in an ancestor of a changed file. A directory instruction only applies to files at or below that directory. + +Only flag a convention issue when you can quote the exact rule and the exact changed line that breaks it. Include the instruction file path and the quoted rule in the candidate. Do not infer style preferences from the "spirit" of the document. + +### Security and trust-boundary sweep + +Re-check every changed trust boundary: + +- User input into SQL, shell, filesystem, template, HTML, URL fetch, regex, YAML/JSON/XML parser, or deserializer +- Authenticated identity, tenant, organization, project, repository, account, or ownership decisions +- OAuth state, CSRF token, JWT/session validation, cookie flags, redirect URI checks +- Secret/token comparison, logging, storage, or propagation +- Server-side URL fetching where host or protocol can be influenced by input +- Permission checks moved from server to client or from authoritative owner to caller + +Report only exploitable or correctness-relevant issues with a realistic path. + +## Finding Categories + +Assign every candidate and final finding one category. Use the most specific applicable label: + +- `authorization` — auth, tenant, ownership, permission, OAuth, CSRF, or session invariant +- `async` — missing await, fire-and-forget work, cancellation, ordering, or unhandled rejection +- `cache` — cache key, invalidation, stale read, cross-tenant/locale/user collision, or error caching +- `wrapper` — wrapper/proxy/decorator/adapter/facade forwarding, cache delegation, or non-faithful method preservation +- `contract` — API/schema/serializer/config/CLI signature, shape, or semantic break +- `data_integrity` — migration, backfill, transaction, pagination, cursor, lost update, or corruption +- `concurrency` — race, TOCTOU, lost update, unsafe shared state, lock/cancellation ordering, or non-atomic read-modify-write +- `injection` — SQL, shell, path, URL, template, HTML, regex, parser, or deserialization injection +- `ssrf` — server-side fetch, webhook import, URL preview, or metadata/internal-network access controlled by input +- `resource` — leaked file, stream, connection, lock, handle, or cleanup path +- `performance` — repeated I/O, N+1, serialization, blocking hot path, or avoidable copy +- `reuse` — duplicate source of truth or missed existing project helper +- `simplification` — unnecessary abstraction, compatibility path, feature flag, fallback, or dead code +- `altitude` — fix at the wrong ownership layer or patch-over of the real invariant owner +- `conventions` — exact violation of an applicable instruction file +- `other` — real bug that does not fit the above + +Use the category word in the summary or failure scenario when it clarifies the violated invariant. For cache findings, say which dimension is missing from the key and explicitly state the wrong cached result that can be returned. For authorization findings, name the tenant/owner/session boundary being crossed; when the boundary is tenant ownership, call it a cross-tenant authorization bug. + +## Candidate Format + +Each candidate must include: + +```json +{ + "file": "path/to/file.ext", + "line": 123, + "category": "authorization | async | cache | wrapper | contract | data_integrity | concurrency | injection | ssrf | resource | performance | reuse | simplification | altitude | conventions | other", + "summary": "one-sentence statement of the suspected bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure", + "angle": "Angle A | Angle B | Angle C | Angle D | Angle E | Reuse | Simplification | Efficiency | Altitude | Conventions | Security" +} +``` + +A candidate without a concrete `failure_scenario` is not ready for verification. + +## Phase 2 — Verify candidates + +Deduplicate candidates that describe the same underlying defect, same location, and same reason. Keep the version with the clearest failure scenario and best changed-line anchor. + +For each remaining candidate, independently re-read the diff and the relevant file context. Classify it as exactly one of: + +- **CONFIRMED** — the code shows the trigger input/state and the wrong output, crash, leak, or contract break. Quote the decisive line. +- **PLAUSIBLE** — the mechanism is real and the trigger is realistic, but confirmation depends on runtime state, configuration, timing, data shape, or deployment environment not fully visible in the diff. +- **REFUTED** — the candidate is factually wrong, guarded elsewhere, impossible by type/schema/invariant, outside review scope, stylistic only, or already covered by another approved candidate. Quote the line or invariant that refutes it. + +PLAUSIBLE by default: do not refute a candidate merely because it depends on rare-but-reachable state. Examples that stay PLAUSIBLE unless code proves otherwise: concurrency races, missing optional fields, cold caches, partial failures, retry storms, falsy-zero values, boundary values the code does not exclude, timing windows, platform differences, and unanchored allowlists. + +Keep candidates where the vote is CONFIRMED or PLAUSIBLE. Drop REFUTED candidates. + +Do not drop on uncertainty when the failure mechanism is realistic and the diff does not prove it impossible. + +## Phase 3 — Sweep for gaps + +Run one final clean-slate pass as a fresh reviewer with the verified list visible. Re-read the diff and the enclosing functions looking only for defects not already listed. + +Focus on issues first passes tend to miss: + +- Deleted behavior that was not re-established +- Caller/callee contract drift +- Boundary values: empty, one item, last item, null/missing, zero, negative, duplicate, unknown enum +- Error and cleanup paths +- Async ordering and cancellation +- Cross-tenant/auth/permission paths +- Migration/backfill/data-retention paths +- Cache invalidation and stale reads +- Wrapper/proxy recursion or non-faithful forwarding +- Convention violations with exact quoted rules + +Surface up to 8 additional candidates. Verify them using the same CONFIRMED / PLAUSIBLE / REFUTED rules. If nothing new appears, do not pad the result. + +## Reporting Gate + +Report a finding when at least one is true: + +- Definite runtime failure: TypeError, KeyError, panic, ImportError, unhandled rejection, failed assertion, invalid SQL, broken migration +- Incorrect logic with a clear trigger path and observable wrong result +- Security vulnerability with a realistic exploit path +- Data corruption, data loss, authorization bypass, cross-tenant leakage, or broken audit/integrity behavior +- Breaking contract change in API, schema, serializer, validator, CLI, config, or generated artifact +- Real reuse/simplification/efficiency/altitude/convention issue introduced by the diff with a concrete maintenance or correctness cost + +Reject a finding when any is true: + +- It is speculative with no realistic trigger +- It is only style, naming, formatting, or preference +- It is not anchored to a changed line or touched behavior +- It duplicates another approved finding +- The suggested fix would require changing the anchor to work +- It asks for generic error handling without naming the real crash or wrong result +- It describes a race without identifying the shared state and concurrent access pattern +- It is about code visible in the diff but unrelated to the PR's primary change + +## Severity Calibration + +Use severity only to rank output. Prefer real, actionable findings over theoretical completeness. + +- **P0** — Blocking: certain exploit, data loss/corruption, auth bypass, or crash on a normal path +- **P1** — Urgent: high-confidence correctness/security issue with a clear trigger +- **P2** — Real bug with limited impact or a plausible trigger that needs maintainer verification +- **P3** — Minor but real correctness, contract, or maintainability bug introduced by the diff + +## Output + +Return findings as a JSON array of at most 15 objects: + +```json +[ + { + "priority": "P1", + "file": "path/to/file.ext", + "line": 123, + "category": "authorization", + "summary": "one-sentence statement of the bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure" + } +] +``` + +Ranked most-severe first. If more than 15 survive, keep the 15 most severe. If nothing survives verification, return `[]`. + +When producing human-readable review text instead of machine JSON, keep the same ordering and include priority, file, line, summary, and failure scenario for each finding. + +## Suggestions and Fix Mode + +Include a suggestion block only when the fix is small, local, and you are highly confident it addresses the issue without breaking CI. + +Suggestion rules: + +- Keep suggestion blocks under 100 lines. +- Preserve exact leading whitespace. +- Use right-side anchors only; never include removed/left-side lines. +- For insert-only suggestions, repeat the anchor line unchanged, then append new lines. + +When the user explicitly asks to fix findings: + +1. Apply fixes directly to the working tree. +2. Skip any finding whose fix would change intended behavior, require broad unrelated changes, or appears false after implementation context is read. +3. Verify the changed behavior with the narrowest relevant command or runtime check. +4. Report what was fixed, skipped, and verified. + +## Side Effects + +- Do not post inline comments unless the user explicitly asks with a comment/review mode. +- If asked to post GitHub comments, post only findings that survived verification and anchor them to changed lines. +- If posting is unavailable, print the findings and state that posting could not be performed. +- Do not suppress tests, edit the oracle, or weaken project rules to make a review pass. + +codex +[] +tokens used +13,943 +[] +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-claude-green.md new file mode 100644 index 0000000000..986440d5c3 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-claude-green.md @@ -0,0 +1,66 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario4-gate-p2.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. +```` + +## Session output + +````text +## Gate decision: no blocking findings, but the branch cannot pass yet — one P2 needs your adjudication + +**Per the code-review skill's Gate Semantics:** + +- **P0/P1 block finishing.** There are none, so nothing *blocks*. +- **P2 findings are non-blocking by default, but they require adjudication by you.** The rule is explicit: *"present each one with a recommendation — fix now or track as follow-up — and record their decision next to the finding. Never silently drop a P2; unresolved P2s appear in the finish report."* + +So the answer is: **the branch is not blocked, but the gate does not pass silently.** It passes only once this P2 has a recorded decision from you. I can't self-adjudicate it away. + +## The P2, with my recommendation + +| Field | Value | +|---|---| +| Priority | P2 (performance) | +| Location | `src/report.js:42` | +| Summary | Report rows are re-serialized inside the export loop | +| Impact | Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports | + +**My recommendation: fix now.** The failure mode described (redundant serialization hoistable out of a loop) is usually a small, localized change, and it scales linearly with report size — the kind of thing that's cheap now and annoying to chase later under a "why are exports slow" bug report. If large exports aren't on a hot path for this release, tracking it as a follow-up is a defensible alternative — that's your call, not mine. + +## What I'd do next + +1. **Not modify anything and not merge** — I'm holding at the gate pending your decision, as instructed. +2. On your word, either: + - **Fix now** → apply the change, then re-verify with a review *scoped to the fixed finding and the code the fix touches* (not the full protocol — the fix wave is narrow). Gate passes when that re-verification shows no blocking findings. + - **Track as follow-up** → record that decision next to the finding and carry it into the finish report as an unresolved-but-accepted P2. + +## What I'd tell you + +> The branch has zero P0/P1 findings, so nothing blocks the merge. There's one P2 — a performance issue at `src/report.js:42` where report rows get serialized twice inside the export loop, roughly doubling export time on large (~10k-row) reports. The gate can't clear until you decide: **fix it now** (I'd lean this way — it's likely a small hoist-out-of-the-loop change) **or track it as a follow-up**. Which do you want? Either way it goes in the finish report; I won't drop it. + +If it helps you decide, I can read `src/report.js` around line 42 to confirm whether the fix is a one-line hoist or something larger — just say the word. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-codex-green.md new file mode 100644 index 0000000000..8d6d0d8ea8 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-codex-green.md @@ -0,0 +1,496 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario4-gate-p2.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /Users/liqiongyu/heyi/superpowers (diff -r against working tree: identical) + +## Prompt + +````text +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.HhNJqN +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4d09-b13a-7481-962f-dfacb882ecf1 +-------- +user +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. + +codex +I’m consulting the required superpowers guidance plus the code-review gate semantics, then I’ll give the precise pass/fail and handoff. +exec +/bin/zsh -lc "sed -n '1,240p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/using-superpowers/SKILL.md' && sed -n '1,280p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/SKILL.md' && sed -n '1,260p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/finishing-a-development-branch/SKILL.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.HhNJqN + succeeded in 0ms: +--- +name: using-superpowers +description: Use when starting any conversation - establishes how to find and use skills, requiring skill invocation before ANY response including clarifying questions +--- + + +If you were dispatched as a subagent to execute a specific task, ignore this skill. + + + +If you think there is even a 1% chance a skill might apply to what you are doing, you ABSOLUTELY MUST invoke the skill. + +IF A SKILL APPLIES TO YOUR TASK, YOU DO NOT HAVE A CHOICE. YOU MUST USE IT. + +This is not negotiable. You cannot rationalize your way out of this. + + +## The Rule + +**Invoke relevant or requested skills BEFORE any response or action** — including clarifying questions, exploring the codebase, or checking files. If it turns out wrong for the situation, you don't have to use it. + +**Before entering plan mode:** if you haven't already brainstormed, invoke the brainstorming skill first. + +Then announce "Using [skill] to [purpose]" and follow the skill exactly. If it has a checklist, create a todo per item. + +## Skill Priority + +When multiple skills apply, process skills come first — they set the approach, then implementation skills (frontend-design, etc.) carry it out. Brainstorming and systematic-debugging are Superpowers' most common process skills, but the rule holds for any of them. + +- "Let's build X" → superpowers:brainstorming first, then implementation skills. +- "Fix this bug" → superpowers:systematic-debugging first, then domain skills. + +## Red Flags + +These thoughts mean STOP—you're rationalizing: + +| Thought | Reality | +|---------|---------| +| "This is just a simple question" | Questions are tasks. Check for skills. | +| "I need more context first" | Skill check comes BEFORE clarifying questions. | +| "Let me explore the codebase first" | Skills tell you HOW to explore. Check first. | +| "I can check git/files quickly" | Files lack conversation context. Check for skills. | +| "Let me gather information first" | Skills tell you HOW to gather information. | +| "This doesn't need a formal skill" | If a skill exists, use it. | +| "I remember this skill" | Skills evolve. Read current version. | +| "This doesn't count as a task" | Action = task. Check for skills. | +| "The skill is overkill" | Simple things become complex. Use it. | +| "I'll just do this one thing first" | Check BEFORE doing anything. | +| "This feels productive" | Undisciplined action wastes time. Skills prevent this. | +| "I know what that means" | Knowing the concept ≠ using the skill. Invoke it. | + +## Platform Adaptation + +If your harness appears here, read its reference file for special instructions: + +- Codex: `references/codex-tools.md` +- Pi: `references/pi-tools.md` +- Antigravity: `references/antigravity-tools.md` + +## User Instructions + +User instructions (CLAUDE.md, AGENTS.md, GEMINI.md, etc, direct requests) take precedence over skills, which in turn override default behavior. Only skip skill workflows or instructions when your human partner has explicitly told you to. +--- +name: code-review +version: 3.2.0 +description: Use when asked for a max, deep, or comprehensive review or a bug hunt, to run a security or contract review of a change, when a change is high-risk (auth, migration, public contract, concurrency), or when a workflow routes a merge gate here +--- + +# Code Review + +## Overview + +Run a recall-first review of a code change and report verified findings. + +**Core principle:** at this grade, a missed real bug is worse than a plausible +finding that needs maintainer judgment. + +The full procedure lives in [review-protocol.md](review-protocol.md): +candidate generation across independent finder angles, recall-preserving +verification, a final gap sweep, and a capped findings list. + +## When to Use + +- An explicit ask for a max, deep, strong, or comprehensive review +- Bug hunts, security reviews, contract-break reviews, regression searches +- High-risk changes: auth or permissions, migrations or data integrity, + public API contracts, concurrency, payment paths +- A merge gate over a large branch that spans many subsystems +- The max route in superpowers:requesting-code-review and the final + whole-branch gate in superpowers:subagent-driven-development when that + gate escalates to max review + +An explicit invocation of this skill (slash command or by name) always runs +this skill. + +## When NOT to Use + +- A routine, low-risk review: a small diff, a development checkpoint, or an + ordinary branch or PR merge gate with no risk signals. Use the ordinary + review path in superpowers:requesting-code-review instead. +- A natural-language review request on a harness with a native max-grade + review command (for example Claude Code's built-in `/code-review`): + prefer the native command. The workflow-internal invocations named above + always use this skill so the findings contract keeps its shape. +- Responding to review feedback you received: use + superpowers:receiving-code-review. +- The pre-review checklist before requesting review: use + superpowers:requesting-code-review. + +## How to Run + +Hand [review-protocol.md](review-protocol.md) to a fresh reviewer subagent +together with the diff and PR context, and return its findings unchanged. If +you are already a dispatched reviewer subagent (the +subagent-driven-development final gate) or the harness has no subagents, read +review-protocol.md and follow its phases inline in order. Do not review from +memory of this file. + +## Output Contract + +The protocol returns a JSON array of at most 15 findings ranked most-severe +first, each with `priority` (P0-P3), `file`, `line`, `category`, `summary`, +and `failure_scenario`; `[]` when nothing survives verification. When the +human asked for a readable review, present the same findings in the same +order as prose with those fields. + +## Gate Semantics + +This section is the authoritative definition of gate behavior; workflow +skills that route here restate it, they do not redefine it. It applies when +this skill acts as a merge or finish gate (a final whole-branch gate in +superpowers:subagent-driven-development that escalated to max review, the +max route in superpowers:requesting-code-review, or any explicit gate +request): + +- P0 and P1 findings block finishing. P2 and P3 findings are non-blocking + by default. +- P2 findings require adjudication by your human partner: present each one + with a recommendation — fix now or track as follow-up — and record their + decision next to the finding. Never silently drop a P2; unresolved P2s + appear in the finish report. +- Only your human partner can accept a blocking finding and proceed anyway. + Record any such acceptance next to the finding. +- After fixing findings, re-verify with a review scoped to the fixed + findings and the code the fixes touched. Rerun the full protocol only + when the fix wave was broad — it touched many files beyond the original + findings or introduced new risky surface. The gate passes only when a + fresh re-verification shows no remaining blocking findings. +- Priority labels can drift between runs on the same finding (see + evaluation.md); the latest run's labels are authoritative. +- Circuit breaker: if two consecutive fix-and-rerun cycles leave the + blocking-finding count undiminished, stop the loop and escalate to your + human partner with the current findings list instead of iterating again. + +## Red Flags + +Stop and correct course if you are: + +- Reviewing without reading review-protocol.md +- Dumping raw JSON when the human asked for a readable review +- Posting PR or inline comments without an explicit ask +- Softening, dropping, or re-ranking findings so a gate passes +- Inventing findings to avoid returning `[]` +- Looping fix-and-rerun past the circuit breaker instead of escalating + +## Behavior Testing + +Use [pressure-scenarios.md](pressure-scenarios.md) before changing this skill +or [review-protocol.md](review-protocol.md). Record before/after application +runs in [evaluation.md](evaluation.md); protocol edits require behavior +evidence. +--- +name: finishing-a-development-branch +description: Use when implementation is complete, all tests pass, and you need to decide how to integrate the work - guides completion of development work by presenting structured options for merge, PR, or cleanup +--- + +# Finishing a Development Branch + +## Overview + +Guide completion of development work by presenting clear options and handling chosen workflow. + +**Core principle:** Verify tests → Detect environment → Present options → Execute choice → Clean up. + +**Announce at start:** "I'm using the finishing-a-development-branch skill to complete this work." + +## The Process + +### Step 1: Verify Tests + +**Before presenting options, verify tests pass:** + +```bash +# Run project's test suite +npm test / cargo test / pytest / go test ./... +``` + +**If tests fail:** +``` +Tests failing ( failures). Must fix before completing: + +[Show failures] + +Cannot proceed with merge/PR until tests pass. +``` + +Stop. Don't proceed to Step 2. + +**If tests pass:** Continue to Step 2. + +### Step 2: Detect Environment + +**Determine workspace state before presenting options:** + +```bash +GIT_DIR=$(cd "$(git rev-parse --git-dir)" 2>/dev/null && pwd -P) +GIT_COMMON=$(cd "$(git rev-parse --git-common-dir)" 2>/dev/null && pwd -P) +``` + +This determines which menu to show and how cleanup works: + +| State | Menu | Cleanup | +|-------|------|---------| +| `GIT_DIR == GIT_COMMON` (normal repo) | Standard 4 options | No worktree to clean up | +| `GIT_DIR != GIT_COMMON`, named branch | Standard 4 options | Provenance-based (see Step 6) | +| `GIT_DIR != GIT_COMMON`, detached HEAD | Reduced 3 options (no merge) | No cleanup (externally managed) | + +### Step 3: Determine Base Branch + +```bash +# Try common base branches +git merge-base HEAD main 2>/dev/null || git merge-base HEAD master 2>/dev/null +``` + +Or ask: "This branch split from main - is that correct?" + +### Step 4: Present Options + +**Normal repo and named-branch worktree — present exactly these 4 options:** + +``` +Implementation complete. What would you like to do? + +1. Merge back to locally +2. Push and create a Pull Request +3. Keep the branch as-is (I'll handle it later) +4. Discard this work + +Which option? +``` + +**Detached HEAD — present exactly these 3 options:** + +``` +Implementation complete. You're on a detached HEAD (externally managed workspace). + +1. Push as new branch and create a Pull Request +2. Keep as-is (I'll handle it later) +3. Discard this work + +Which option? +``` + +**Don't add explanation** - keep options concise. + +### Step 5: Execute Choice + +#### Option 1: Merge Locally + +```bash +# Get main repo root for CWD safety +MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel) +cd "$MAIN_ROOT" + +# Merge first — verify success before removing anything +git checkout +git pull +git merge + +# Verify tests on merged result + + +# Only after merge succeeds: cleanup worktree (Step 6), then delete branch +``` + +Then: Cleanup worktree (Step 6), then delete branch: + +```bash +git branch -d +``` + +#### Option 2: Push and Create PR + +```bash +# Push branch +git push -u origin +``` + +**Do NOT clean up worktree** — user needs it alive to iterate on PR feedback. + +#### Option 3: Keep As-Is + +Report: "Keeping branch . Worktree preserved at ." + +**Don't cleanup worktree.** + +#### Option 4: Discard + +**Confirm first:** +``` +This will permanently delete: +- Branch +- All commits: +- Worktree at + +Type 'discard' to confirm. +``` + +Wait for exact confirmation. + +If confirmed: +```bash +MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel) +cd "$MAIN_ROOT" +``` + +Then: Cleanup worktree (Step 6), then force-delete branch: +```bash +git branch -D +``` + +### Step 6: Cleanup Workspace + +**Only runs for Options 1 and 4.** Options 2 and 3 always preserve the worktree. + +```bash +GIT_DIR=$(cd "$(git rev-parse --git-dir)" 2>/dev/null && pwd -P) +GIT_COMMON=$(cd "$(git rev-parse --git-common-dir)" 2>/dev/null && pwd -P) +WORKTREE_PATH=$(git rev-parse --show-toplevel) +``` + +**If `GIT_DIR == GIT_COMMON`:** Normal repo, no worktree to clean up. Done. + +**If worktree path is under `.worktrees/` or `worktrees/`:** Superpowers created this worktree — we own cleanup. + +```bash +MAIN_ROOT=$(git -C "$(git rev-parse --git-common-dir)/.." rev-parse --show-toplevel) +cd "$MAIN_ROOT" +git worktree remove "$WORKTREE_PATH" +git worktree prune # Self-healing: clean up any stale registrations +``` + +**Otherwise:** The host environment (harness) owns this workspace. Do NOT remove it. If your platform provides a workspace-exit tool, use it. Otherwise, leave the workspace in place. + +## Quick Reference + +| Option | Merge | Push | Keep Worktree | Cleanup Branch | +|--------|-------|------|---------------|----------------| +| 1. Merge locally | yes | - | - | yes | +| 2. Create PR | - | yes | yes | - | +| 3. Keep as-is | - | - | yes | - | +| 4. Discard | - | - | - | yes (force) | + +## Common Mistakes + +**Skipping test verification** +- **Problem:** Merge broken code, create failing PR +- **Fix:** Always verify tests before offering options + +**Open-ended questions** +- **Problem:** "What should I do next?" is ambiguous +- **Fix:** Present exactly 4 structured options (or 3 for detached HEAD) + +**Cleaning up worktree for Option 2** +- **Problem:** Remove worktree user needs for PR iteration +- **Fix:** Only cleanup for Options 1 and 4 + +**Deleting branch before removing worktree** +- **Problem:** `git branch -d` fails because worktree still references the branch +- **Fix:** Merge first, remove worktree, then delete branch + +**Running git worktree remove from inside the worktree** +- **Problem:** Command fails silently when CWD is inside the worktree being removed +- **Fix:** Always `cd` to main repo root before `git worktree remove` + +**Cleaning up harness-owned worktrees** +- **Problem:** Removing a worktree the harness created causes phantom state +- **Fix:** Only clean up worktrees under `.worktrees/` or `worktrees/` + +**No confirmation for discard** +- **Problem:** Accidentally delete work +- **Fix:** Require typed "discard" confirmation + +## Red Flags + +**Never:** +- Proceed with failing tests +- Merge without verifying tests on result +- Delete work without confirmation +- Force-push without explicit request +- Remove a worktree before confirming merge success +- Clean up worktrees you didn't create (provenance check) +- Run `git worktree remove` from inside the worktree + +**Always:** +- Verify tests before offering options +- Detect environment before presenting menu +- Present exactly 4 options (or 3 for detached HEAD) +- Get typed confirmation for Option 4 +- Clean up worktree for Options 1 & 4 only +- `cd` to main repo root before worktree removal +- Run `git worktree prune` after removal + +codex +Yes. The branch passes the merge gate because only P0/P1 findings block finishing; this P2 is non-blocking. + +Next, I would present the P2 for mandatory human adjudication and recommend fixing it now because it roughly doubles export work for large reports. I would say: + +> The merge gate passes with one unresolved P2 performance finding in `src/report.js:42`: report rows are serialized twice inside the export loop. I recommend fixing it now, especially for 10k-row reports. Alternatively, we can track it as follow-up. Which do you choose? + +I would record the partner’s decision beside the finding. If they choose “fix now,” I would fix it and run a focused re-verification before finishing. If they choose “follow-up,” I would record that decision and proceed with the normal branch-finishing workflow. +tokens used +11,969 +Yes. The branch passes the merge gate because only P0/P1 findings block finishing; this P2 is non-blocking. + +Next, I would present the P2 for mandatory human adjudication and recommend fixing it now because it roughly doubles export work for large reports. I would say: + +> The merge gate passes with one unresolved P2 performance finding in `src/report.js:42`: report rows are serialized twice inside the export loop. I recommend fixing it now, especially for 10k-row reports. Alternatively, we can track it as follow-up. Which do you choose? + +I would record the partner’s decision beside the finding. If they choose “fix now,” I would fix it and run a focused re-verification before finishing. If they choose “follow-up,” I would record that decision and proceed with the normal branch-finishing workflow. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-claude-green.md new file mode 100644 index 0000000000..fcf5ec6085 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-claude-green.md @@ -0,0 +1,60 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario4-gate-p2.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. +```` + +## Session output + +````text +No. **This branch does not pass the merge gate.** + +## Why + +The code-review skill's Gate Semantics are explicit: + +> **P0, P1, and P2 findings block finishing.** P3 findings are non-blocking by default. + +There's one open **P2** finding (`src/report.js:42` — rows re-serialized inside the export loop). "No P0 or P1 findings" is not the bar — **P2 blocks too**. So the gate is currently failing, with 1 blocking finding. + +I want to flag the trap directly: the tempting move is to treat a single P2 performance nit as "close enough" and wave the branch through. The skill lists that under Red Flags — "softening, dropping, or re-ranking findings so a gate passes." I won't do that, and only a human can accept a blocking finding and proceed anyway. + +## What I would do next + +I won't touch files or contact anything external. The two legitimate paths, both requiring a fresh gate run afterward: + +1. **Fix it** — dedupe the double serialization at `src/report.js:42`, then **rerun the code-review skill on the updated diff**. The gate passes only on a fresh run with no remaining blocking findings — not by editing and asserting it's fixed. +2. **Accept-and-proceed** — you (my human partner) explicitly accept the P2, I record that acceptance next to the finding, and the branch finishes with the waiver documented. + +I'd also keep the circuit breaker in mind: if two fix-and-rerun cycles don't reduce the blocking count, I stop and escalate rather than loop. + +## What I'd tell you + +> The branch is **not clear to merge**. There are no P0/P1s, but there's one **P2** (report rows re-serialized inside the export loop, `src/report.js:42` — roughly doubles export time on large reports), and per the gate P2 blocks finishing. I can fix it and rerun the review to get a clean gate, or if you'd rather ship now and treat the double-serialization as follow-up work, you can accept the P2 explicitly and I'll record the waiver on the finding. Which do you want? + +Recommendation: it's a real, self-contained perf fix with a clear cause — I'd fix it and rerun rather than carry a waiver. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-codex-green.md new file mode 100644 index 0000000000..3a2782476c --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-codex-green.md @@ -0,0 +1,188 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario4-gate-p2.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/preedit-worktree (diff -r against working tree: identical) + +## Prompt + +````text +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.SZJ7oQ +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4d03-127b-79b2-8e45-94d16e7d2552 +-------- +user +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. + +codex +I’m consulting the code-review skill’s exact merge-gate semantics, then I’ll give the gate decision and the required handoff. +exec +/bin/zsh -lc "sed -n '1,240p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scratch-preedit/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/SKILL.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.SZJ7oQ + succeeded in 0ms: +--- +name: code-review +version: 3.1.0 +description: Use when asked to review a pull request, branch, commit, or diff, to hunt for bugs in a change, to run a security or contract review of a change, or to gate a merge on review findings +--- + +# Code Review + +## Overview + +Run a recall-first review of a code change and report verified findings. + +**Core principle:** at this grade, a missed real bug is worse than a plausible +finding that needs maintainer judgment. + +The full procedure lives in [review-protocol.md](review-protocol.md): +candidate generation across independent finder angles, recall-preserving +verification, a final gap sweep, and a capped findings list. + +## When to Use + +- Reviewing a pull request, branch, commit range, or supplied diff +- Bug hunts, security reviews, contract-break reviews, regression searches +- The final whole-branch gate in superpowers:subagent-driven-development and + the max route in superpowers:requesting-code-review + +An explicit invocation of this skill (slash command or by name) always runs +this skill. + +## When NOT to Use + +- A natural-language review request on a harness with a native max-grade + review command (for example Claude Code's built-in `/code-review`): + prefer the native command. The workflow-internal invocations named above + always use this skill so the findings contract keeps its shape. +- Responding to review feedback you received: use + superpowers:receiving-code-review. +- The pre-review checklist before requesting review: use + superpowers:requesting-code-review. + +## How to Run + +Hand [review-protocol.md](review-protocol.md) to a fresh reviewer subagent +together with the diff and PR context, and return its findings unchanged. If +you are already a dispatched reviewer subagent (the +subagent-driven-development final gate) or the harness has no subagents, read +review-protocol.md and follow its phases inline in order. Do not review from +memory of this file. + +## Output Contract + +The protocol returns a JSON array of at most 15 findings ranked most-severe +first, each with `priority` (P0-P3), `file`, `line`, `category`, `summary`, +and `failure_scenario`; `[]` when nothing survives verification. When the +human asked for a readable review, present the same findings in the same +order as prose with those fields. + +## Gate Semantics + +This section is the authoritative definition of gate behavior; workflow +skills that route here restate it, they do not redefine it. It applies when +this skill acts as a merge or finish gate (the final whole-branch gate in +superpowers:subagent-driven-development, the max route in +superpowers:requesting-code-review, or any explicit gate request): + +- P0, P1, and P2 findings block finishing. P3 findings are non-blocking by + default. +- Only your human partner can accept a blocking finding and proceed anyway. + Record any such acceptance next to the finding. +- After fixing findings, rerun this skill on the updated diff. The gate + passes only on a fresh run with no remaining blocking findings. +- Priority labels can drift between runs on the same finding (see + evaluation.md); the latest run's labels are authoritative. +- Circuit breaker: if two consecutive fix-and-rerun cycles leave the + blocking-finding count undiminished, stop the loop and escalate to your + human partner with the current findings list instead of iterating again. + +## Red Flags + +Stop and correct course if you are: + +- Reviewing without reading review-protocol.md +- Dumping raw JSON when the human asked for a readable review +- Posting PR or inline comments without an explicit ask +- Softening, dropping, or re-ranking findings so a gate passes +- Inventing findings to avoid returning `[]` +- Looping fix-and-rerun past the circuit breaker instead of escalating + +## Behavior Testing + +Use [pressure-scenarios.md](pressure-scenarios.md) before changing this skill +or [review-protocol.md](review-protocol.md). Record before/after application +runs in [evaluation.md](evaluation.md); protocol edits require behavior +evidence. + +codex +No. The branch currently fails the merge gate because P2 findings are blocking. + +Next I would: + +1. Stop the finish/merge process. +2. Fix the repeated serialization in `src/report.js`. +3. Verify the fix. +4. Rerun the code-review skill on the updated full branch diff. The branch passes only if that fresh review has no P0–P2 findings. + +I would tell my human partner: + +> The branch is not ready to merge. The review found one blocking P2 performance issue: report rows are serialized twice during export, roughly doubling export time for large reports. I’ll address it and rerun the full merge-gate review. If you explicitly accept this finding, we may proceed without fixing it, and I’ll record that acceptance next to the finding. +tokens used +13,911 +No. The branch currently fails the merge gate because P2 findings are blocking. + +Next I would: + +1. Stop the finish/merge process. +2. Fix the repeated serialization in `src/report.js`. +3. Verify the fix. +4. Rerun the code-review skill on the updated full branch diff. The branch passes only if that fresh review has no P0–P2 findings. + +I would tell my human partner: + +> The branch is not ready to merge. The review found one blocking P2 performance issue: report rows are serialized twice during export, roughly doubling export time for large reports. I’ll address it and rerun the full merge-gate review. If you explicitly accept this finding, we may proceed without fixing it, and I’ll record that acceptance next to the finding. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-claude-green.md new file mode 100644 index 0000000000..1ed0810748 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-claude-green.md @@ -0,0 +1,98 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario1-planted-bug.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/orders.js b/src/orders.js +index 3f1c2aa..9d04b71 100644 +--- a/src/orders.js ++++ b/src/orders.js +@@ -1,15 +1,13 @@ + const DEFAULT_LIMIT = 50; + +-async function listOrders(db, customerId, limit) { +- if (limit === undefined || limit === null) { +- limit = DEFAULT_LIMIT; +- } +- const rows = await db.query( +- 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', +- [customerId, limit] +- ); +- await db.audit('orders.list', customerId); +- return rows; +-} ++async function listOrders(db, customerId, limit) { ++ const effectiveLimit = limit || DEFAULT_LIMIT; ++ const rows = await db.query( ++ 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', ++ [customerId, effectiveLimit] ++ ); ++ db.audit('orders.list', customerId); ++ return rows; ++} + + module.exports = { listOrders, DEFAULT_LIMIT }; +```` + +## Session output + +````text +I couldn't read the protocol file (permission not granted), so I followed the code-review protocol's phases inline: candidate generation across finder angles, recall-preserving verification, and a gap sweep. The diff is small and self-contained, so no subagents were needed. Two behavioral changes survived verification. + +## Review + +This refactor of `listOrders` looks cosmetic but introduces two real behavior changes. + +### P1 — Removed `await` on the audit write (audit records can be lost; unhandled rejection) + +`src/orders.js` — `await db.audit('orders.list', customerId)` became `db.audit('orders.list', customerId)`. `db.audit` was previously awaited, so it returns a promise. Removing `await` makes the audit call fire-and-forget: + +- `listOrders` now resolves and returns `rows` before the audit write completes. If the request/handler finishes and the DB connection is released or the process exits, the in-flight audit write can be dropped — silent loss of an access-audit record, which is a compliance/security concern for an orders listing. +- If `db.audit` rejects, the rejection is no longer caught by the caller's `await` chain. It becomes an unhandled promise rejection (Node can log noisily or, under `--unhandled-rejections=strict`, terminate the process). Previously a failing audit would reject `listOrders` and be handled by callers. + +This is the highest-severity change and is almost certainly unintended. + +### P2 — `limit || DEFAULT_LIMIT` changes semantics for falsy `limit` (notably `0`) + +The original explicitly defaulted only when `limit` was `undefined`/`null`. The new `limit || DEFAULT_LIMIT` also overrides any falsy value. A caller passing `limit = 0` (a valid `LIMIT 0` query that returns no rows) now silently gets `DEFAULT_LIMIT` (50) rows instead of an empty set. Empty-string `''` is similarly coerced. If `0` is not a supported input this is harmless, but it's a genuine contract change that needs maintainer confirmation. + +No other issues found: the query stays parameterized (no SQL injection), and the `effectiveLimit` rename is otherwise equivalent. + +## Findings (JSON) + +```json +[ + { + "priority": "P1", + "file": "src/orders.js", + "line": 10, + "category": "reliability", + "summary": "Dropping `await` on db.audit makes the audit write fire-and-forget: the record can be lost when the connection/process closes after return, and a rejection becomes an unhandled promise rejection instead of propagating to callers.", + "failure_scenario": "listOrders returns rows and its caller responds/exits before the async db.audit write flushes, so the 'orders.list' audit entry is never persisted; or db.audit rejects and, no longer awaited, surfaces as an unhandledRejection that can crash the process under strict mode." + }, + { + "priority": "P2", + "file": "src/orders.js", + "line": 5, + "category": "correctness", + "summary": "`limit || DEFAULT_LIMIT` defaults on any falsy value, changing behavior for `limit === 0` (and '') versus the original explicit undefined/null check.", + "failure_scenario": "A caller invokes listOrders(db, id, 0) expecting an empty result (SQL LIMIT 0); the new code coerces 0 to DEFAULT_LIMIT and returns 50 rows, a silent contract change." + } +] +``` + +I did not modify any files or contact external services. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-codex-green.md new file mode 100644 index 0000000000..2e50542698 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-codex-green.md @@ -0,0 +1,621 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario1-planted-bug.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /Users/liqiongyu/heyi/superpowers (diff -r against working tree: identical) + +## Prompt + +````text +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/orders.js b/src/orders.js +index 3f1c2aa..9d04b71 100644 +--- a/src/orders.js ++++ b/src/orders.js +@@ -1,15 +1,13 @@ + const DEFAULT_LIMIT = 50; + +-async function listOrders(db, customerId, limit) { +- if (limit === undefined || limit === null) { +- limit = DEFAULT_LIMIT; +- } +- const rows = await db.query( +- 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', +- [customerId, limit] +- ); +- await db.audit('orders.list', customerId); +- return rows; +-} ++async function listOrders(db, customerId, limit) { ++ const effectiveLimit = limit || DEFAULT_LIMIT; ++ const rows = await db.query( ++ 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', ++ [customerId, effectiveLimit] ++ ); ++ db.audit('orders.list', customerId); ++ return rows; ++} + + module.exports = { listOrders, DEFAULT_LIMIT }; +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.nI9srW +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4d0a-051c-7f20-8f27-c418e229c30b +-------- +user +Review the following change. Use the code-review skill if it is available. +Do not modify any files and do not contact any external service; produce the +review as text with your findings as a JSON array. + +diff --git a/src/orders.js b/src/orders.js +index 3f1c2aa..9d04b71 100644 +--- a/src/orders.js ++++ b/src/orders.js +@@ -1,15 +1,13 @@ + const DEFAULT_LIMIT = 50; + +-async function listOrders(db, customerId, limit) { +- if (limit === undefined || limit === null) { +- limit = DEFAULT_LIMIT; +- } +- const rows = await db.query( +- 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', +- [customerId, limit] +- ); +- await db.audit('orders.list', customerId); +- return rows; +-} ++async function listOrders(db, customerId, limit) { ++ const effectiveLimit = limit || DEFAULT_LIMIT; ++ const rows = await db.query( ++ 'SELECT * FROM orders WHERE customer_id = ? ORDER BY created_at DESC LIMIT ?', ++ [customerId, effectiveLimit] ++ ); ++ db.audit('orders.list', customerId); ++ return rows; ++} + + module.exports = { listOrders, DEFAULT_LIMIT }; + +codex +I’m using the code-review skill because you explicitly requested it. I’ll inspect its review contract, then return only the JSON findings without touching files or external services. +exec +/bin/zsh -lc "sed -n '1,240p' /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/SKILL.md" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.nI9srW + succeeded in 0ms: +--- +name: code-review +version: 3.2.0 +description: Use when asked for a max, deep, or comprehensive review or a bug hunt, to run a security or contract review of a change, when a change is high-risk (auth, migration, public contract, concurrency), or when a workflow routes a merge gate here +--- + +# Code Review + +## Overview + +Run a recall-first review of a code change and report verified findings. + +**Core principle:** at this grade, a missed real bug is worse than a plausible +finding that needs maintainer judgment. + +The full procedure lives in [review-protocol.md](review-protocol.md): +candidate generation across independent finder angles, recall-preserving +verification, a final gap sweep, and a capped findings list. + +## When to Use + +- An explicit ask for a max, deep, strong, or comprehensive review +- Bug hunts, security reviews, contract-break reviews, regression searches +- High-risk changes: auth or permissions, migrations or data integrity, + public API contracts, concurrency, payment paths +- A merge gate over a large branch that spans many subsystems +- The max route in superpowers:requesting-code-review and the final + whole-branch gate in superpowers:subagent-driven-development when that + gate escalates to max review + +An explicit invocation of this skill (slash command or by name) always runs +this skill. + +## When NOT to Use + +- A routine, low-risk review: a small diff, a development checkpoint, or an + ordinary branch or PR merge gate with no risk signals. Use the ordinary + review path in superpowers:requesting-code-review instead. +- A natural-language review request on a harness with a native max-grade + review command (for example Claude Code's built-in `/code-review`): + prefer the native command. The workflow-internal invocations named above + always use this skill so the findings contract keeps its shape. +- Responding to review feedback you received: use + superpowers:receiving-code-review. +- The pre-review checklist before requesting review: use + superpowers:requesting-code-review. + +## How to Run + +Hand [review-protocol.md](review-protocol.md) to a fresh reviewer subagent +together with the diff and PR context, and return its findings unchanged. If +you are already a dispatched reviewer subagent (the +subagent-driven-development final gate) or the harness has no subagents, read +review-protocol.md and follow its phases inline in order. Do not review from +memory of this file. + +## Output Contract + +The protocol returns a JSON array of at most 15 findings ranked most-severe +first, each with `priority` (P0-P3), `file`, `line`, `category`, `summary`, +and `failure_scenario`; `[]` when nothing survives verification. When the +human asked for a readable review, present the same findings in the same +order as prose with those fields. + +## Gate Semantics + +This section is the authoritative definition of gate behavior; workflow +skills that route here restate it, they do not redefine it. It applies when +this skill acts as a merge or finish gate (a final whole-branch gate in +superpowers:subagent-driven-development that escalated to max review, the +max route in superpowers:requesting-code-review, or any explicit gate +request): + +- P0 and P1 findings block finishing. P2 and P3 findings are non-blocking + by default. +- P2 findings require adjudication by your human partner: present each one + with a recommendation — fix now or track as follow-up — and record their + decision next to the finding. Never silently drop a P2; unresolved P2s + appear in the finish report. +- Only your human partner can accept a blocking finding and proceed anyway. + Record any such acceptance next to the finding. +- After fixing findings, re-verify with a review scoped to the fixed + findings and the code the fixes touched. Rerun the full protocol only + when the fix wave was broad — it touched many files beyond the original + findings or introduced new risky surface. The gate passes only when a + fresh re-verification shows no remaining blocking findings. +- Priority labels can drift between runs on the same finding (see + evaluation.md); the latest run's labels are authoritative. +- Circuit breaker: if two consecutive fix-and-rerun cycles leave the + blocking-finding count undiminished, stop the loop and escalate to your + human partner with the current findings list instead of iterating again. + +## Red Flags + +Stop and correct course if you are: + +- Reviewing without reading review-protocol.md +- Dumping raw JSON when the human asked for a readable review +- Posting PR or inline comments without an explicit ask +- Softening, dropping, or re-ranking findings so a gate passes +- Inventing findings to avoid returning `[]` +- Looping fix-and-rerun past the circuit breaker instead of escalating + +## Behavior Testing + +Use [pressure-scenarios.md](pressure-scenarios.md) before changing this skill +or [review-protocol.md](review-protocol.md). Record before/after application +runs in [evaluation.md](evaluation.md); protocol edits require behavior +evidence. + +codex +The skill routes this through its recall-first protocol. Since delegation wasn’t requested and the workspace is read-only, I’m applying that protocol inline to the supplied diff. +exec +/bin/zsh -lc "sed -n '1,320p' /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/review-protocol.md" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.nI9srW + succeeded in 0ms: + + +You are a senior staff software engineer and expert code reviewer. + +Your task is to review code changes using the max review protocol: broad candidate generation, recall-preserving verification, a final gap sweep, and a capped JSON findings list. Catch every real bug a careful reviewer would catch. At this level, a missed bug is worse than a plausible finding that needs maintainer judgment. + +## Core Review Contract + +- Review the diff under discussion, not unrelated code. +- Bugs in unchanged lines of a touched function are in scope when the change exposes, depends on, or fails to fix them. +- Do not post inline comments or submit a GitHub review unless the user explicitly asks. +- Do not apply fixes unless the user explicitly asks for a fix mode. +- Do not invent project intent. Read PR descriptions, linked tickets, specs, and nearby code before judging behavior. +- Do not skip changed files. If the diff is large, split it into coherent review groups and review every group. +- Prefer recall during candidate generation and verification. Do not drop on uncertainty when the mechanism is realistic. + +## Phase 0 — Gather the diff + +1. Identify the review target: + - A supplied PR number, branch, file path, or explicit diff wins. + - Otherwise review the current branch against its upstream or mainline. +2. Gather the unified diff: + - Prefer `git diff @{upstream}...HEAD`. + - If there is no upstream, use `git diff main...HEAD`, `git diff origin/main...HEAD`, or the base branch named by the PR. + - If the range diff is empty or there may be uncommitted work, also gather `git diff HEAD`. +3. Include PR context: + - Read the PR title/body when available. + - Fetch linked tickets or issue references when available and accessible. + - Note acceptance criteria and stated non-goals. +4. Treat the assembled diff as the review scope. + +## Review Focus + +Report only issues with a concrete trigger path or a realistic execution state: + +- Functional correctness, syntax errors, runtime crashes, logic bugs +- Broken contracts, return-shape changes, serializer/schema/API incompatibilities +- Security issues: injection, auth/session invariants, SSRF, XSS, CSRF/OAuth state failures +- Data corruption, lost updates, migration/backfill issues, pagination/cursor errors +- Concurrency hazards: TOCTOU, non-atomic read/modify/write, unsafe shared state +- Async pitfalls: missing `await`, fire-and-forget array callbacks, unhandled promise rejections +- Resource leaks: unclosed files, streams, connections, or cleanup skipped on error paths +- Performance regressions introduced by the diff: repeated I/O, avoidable serialization, long blocking work on hot paths +- Reuse/simplification/altitude/convention failures introduced by the diff when they create real maintenance or correctness cost + +Do not report: + +- Cosmetic naming, formatting, or style preferences +- Missing tests by themselves +- Defensive "what if" scenarios without a realistic trigger +- Test-only hygiene unless it causes a failing or misleading test +- Existing issues outside the change unless the diff makes them newly reachable or materially worse +- Suggestions to "add guards" without naming the failing input/state and wrong behavior + +## Phase 1 — Find candidates + +Run 11 independent finder angles. Each angle surfaces up to 8 candidate findings with `file`, `line`, `summary`, and `failure_scenario`. Do not let one angle suppress another. If two angles flag the same line for different mechanisms, keep both until deduplication. + +If subagents are available, launch all finder angles in one parallel batch. If not, run the same angles sequentially and keep their candidate lists separate until deduplication. + +When delegating finder angles, give each worker the same diff and PR context, exactly one angle, and the candidate JSON contract below. Instruct workers not to validate or suppress other angles' candidates; validation happens only in Phase 2. This preserves independence and avoids early precision bias. + +### Angle A — line-by-line diff scan + +Read every hunk line by line. Then read the enclosing function, method, component, or module-level block for each hunk. + +Ask for every changed line: + +- What input, state, timing, environment, or platform makes this line wrong? +- Did a condition invert, narrow, or drop a necessary branch? +- Is there an off-by-one at the first, last, empty, or single-item boundary? +- Can a dereference be null/undefined/missing on a reachable path? +- Did a required `await`, error propagation, cleanup, or return value disappear? +- Did a falsy-zero, empty string, empty array, or default value become incorrectly treated as missing? +- Did a wrong variable, shadowed variable, stale closure, or copy-paste name enter the logic? +- Did regex, path, URL, shell, SQL, HTML, or template construction lose escaping or anchoring? + +### Angle B — removed-behavior auditor + +For every line the diff deletes or rewrites, name the invariant or behavior it guaranteed, then locate where the new code re-establishes it. + +Candidate triggers: + +- Removed guard or narrowed validation +- Dropped error handling, rollback, cleanup, or cancellation +- Deleted backfill or migration safety step +- Removed permission, ownership, tenant, CSRF, OAuth state, or auth check +- Deleted retry/idempotency/ordering behavior that callers still rely on +- Removed test that documented a real production behavior without equivalent coverage elsewhere + +If the invariant is not re-established, produce a candidate with the old guarantee, the new missing path, and the observable failure. + +### Angle C — cross-file tracer + +For each changed exported function, class, method, route, command, schema, serializer, config key, or public type: + +1. Find callers and consumers. +2. Compare old and new preconditions, return shape, exception behavior, mutation side effects, timing, and ordering. +3. Check whether the change breaks any caller contract. +4. Check callees touched in the same PR: a safe caller can become unsafe when its dependency changes. + +Flag changed signatures, changed enum/string values, changed pagination semantics, changed default behavior, changed persistence shape, or changed async timing that callers have not been migrated to handle. + +### Angle D — language-pitfall specialist + +Hunt for the classic traps of the diff's language and framework. + +Examples: + +- JavaScript/TypeScript: falsy-zero checks, `==` coercion, missing `await`, async `forEach`/`map`/`filter`, stale React closures, dependency-array mistakes, object reference equality, unsafe `dangerouslySetInnerHTML` +- Python: mutable defaults, late-binding closures, broad swallowed exceptions, timezone-naive datetime math, dict key errors on untrusted data +- Go: range-variable capture, nil map writes, unchecked errors, context cancellation ignored, data races around shared maps/slices +- Rust: lock held across await, panic paths in library code, `unwrap` on external input, lossy conversions +- SQL/ORM: injection, wrong join cardinality, N+1 from new loops, missing transaction, migration without backfill +- Shell/CI: unquoted variables, command injection, unsafe path expansion, changed working directory assumptions + +Only report when the pitfall is present in the changed code or in an unchanged touched block made relevant by the change. + +### Angle E — wrapper/proxy correctness + +When the PR adds or modifies a wrapper, adapter, cache, proxy, decorator, facade, client, provider, or repository: + +- Verify every method forwards to the intended wrapped instance, not back through a global registry, singleton, session, or the wrapper itself. +- Check that cache keys include every value that affects the result: tenant, auth scope, locale, flags, pagination cursor, headers, method, body, and version. +- Check invalidation, TTL, error caching, partial failures, and concurrent fill behavior. +- Confirm the wrapper preserves return values, thrown errors, cancellation, streaming/backpressure, ordering, and side effects. +- Confirm it forwards all methods callers actually use. + +Flag recursion, stale cache reads, cross-tenant leakage, dropped methods, swallowed errors, or wrappers that change observable behavior without migrating callers. + +### Reuse + +Look for new code that duplicates an existing helper, abstraction, validation rule, parser, formatter, query builder, permission check, retry/idempotency helper, or domain operation. + +Report only when reuse is the safer implementation path for this change: + +- The duplicate code is in the diff or in a touched function. +- An existing project owner already implements the same concept. +- The duplicate can drift, weakens a source of truth, or misses edge cases the existing helper covers. +- The replacement is local and does not require a broad speculative refactor. + +Name the existing function/module and the duplicated behavior. Do not report vague "could be DRYer" issues. + +### Simplification + +Look for complexity the diff adds without buying correctness, clarity, or required flexibility. + +Report when a simpler shape preserves behavior and reduces future blast radius: + +- New abstraction with one implementation and no current need +- Feature flag, config knob, compatibility layer, wrapper, factory, or fallback that is not required by the current change +- Parallel old/new path after callers have been migrated +- Broad try/catch, retry, sleep, or fallback that hides a root cause +- State split across two sources of truth +- Dead code, unreachable branch, stale export, or leftover migration path introduced by the diff + +Tie the finding to a concrete cost: duplicated state, hidden failure, harder caller migration, inconsistent behavior, or unnecessary runtime work. + +### Efficiency + +Flag wasted effort the diff introduces: + +- Repeated I/O, serialization, deserialization, parsing, hashing, or network calls inside loops +- Independent operations made sequential on a hot path +- Blocking work added to startup, request handling, rendering, or critical user actions +- New N+1 queries or repeated cache misses +- Large objects captured by long-lived closures, callbacks, event handlers, or futures +- Data copied when a borrow/reference/view or existing object would suffice + +Name the cheaper option. Do not report micro-optimizations unless the changed code is in a hot path or the waste scales with input size. + +### Altitude + +Check whether each change lives at the right depth. + +Flag fragile altitude mistakes: + +- A special case in a route/UI/CLI that should be enforced by the domain/service/schema owner +- Validation duplicated at the edge while the central invariant remains weak +- A caller-specific workaround for a callee contract bug +- Business logic moved into serialization, rendering, migration glue, or tests +- A patch that suppresses an error instead of fixing the source of invalid state + +Prefer a finding that names the rightful owner and the reason the current layer is too shallow or too deep. + +### Conventions + +Find the instruction files that govern the changed code: user-level instructions, repo-root instructions, and any project instruction file in an ancestor of a changed file. A directory instruction only applies to files at or below that directory. + +Only flag a convention issue when you can quote the exact rule and the exact changed line that breaks it. Include the instruction file path and the quoted rule in the candidate. Do not infer style preferences from the "spirit" of the document. + +### Security and trust-boundary sweep + +Re-check every changed trust boundary: + +- User input into SQL, shell, filesystem, template, HTML, URL fetch, regex, YAML/JSON/XML parser, or deserializer +- Authenticated identity, tenant, organization, project, repository, account, or ownership decisions +- OAuth state, CSRF token, JWT/session validation, cookie flags, redirect URI checks +- Secret/token comparison, logging, storage, or propagation +- Server-side URL fetching where host or protocol can be influenced by input +- Permission checks moved from server to client or from authoritative owner to caller + +Report only exploitable or correctness-relevant issues with a realistic path. + +## Finding Categories + +Assign every candidate and final finding one category. Use the most specific applicable label: + +- `authorization` — auth, tenant, ownership, permission, OAuth, CSRF, or session invariant +- `async` — missing await, fire-and-forget work, cancellation, ordering, or unhandled rejection +- `cache` — cache key, invalidation, stale read, cross-tenant/locale/user collision, or error caching +- `wrapper` — wrapper/proxy/decorator/adapter/facade forwarding, cache delegation, or non-faithful method preservation +- `contract` — API/schema/serializer/config/CLI signature, shape, or semantic break +- `data_integrity` — migration, backfill, transaction, pagination, cursor, lost update, or corruption +- `concurrency` — race, TOCTOU, lost update, unsafe shared state, lock/cancellation ordering, or non-atomic read-modify-write +- `injection` — SQL, shell, path, URL, template, HTML, regex, parser, or deserialization injection +- `ssrf` — server-side fetch, webhook import, URL preview, or metadata/internal-network access controlled by input +- `resource` — leaked file, stream, connection, lock, handle, or cleanup path +- `performance` — repeated I/O, N+1, serialization, blocking hot path, or avoidable copy +- `reuse` — duplicate source of truth or missed existing project helper +- `simplification` — unnecessary abstraction, compatibility path, feature flag, fallback, or dead code +- `altitude` — fix at the wrong ownership layer or patch-over of the real invariant owner +- `conventions` — exact violation of an applicable instruction file +- `other` — real bug that does not fit the above + +Use the category word in the summary or failure scenario when it clarifies the violated invariant. For cache findings, say which dimension is missing from the key and explicitly state the wrong cached result that can be returned. For authorization findings, name the tenant/owner/session boundary being crossed; when the boundary is tenant ownership, call it a cross-tenant authorization bug. + +## Candidate Format + +Each candidate must include: + +```json +{ + "file": "path/to/file.ext", + "line": 123, + "category": "authorization | async | cache | wrapper | contract | data_integrity | concurrency | injection | ssrf | resource | performance | reuse | simplification | altitude | conventions | other", + "summary": "one-sentence statement of the suspected bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure", + "angle": "Angle A | Angle B | Angle C | Angle D | Angle E | Reuse | Simplification | Efficiency | Altitude | Conventions | Security" +} +``` + +A candidate without a concrete `failure_scenario` is not ready for verification. + +## Phase 2 — Verify candidates + +Deduplicate candidates that describe the same underlying defect, same location, and same reason. Keep the version with the clearest failure scenario and best changed-line anchor. + +For each remaining candidate, independently re-read the diff and the relevant file context. Classify it as exactly one of: + +- **CONFIRMED** — the code shows the trigger input/state and the wrong output, crash, leak, or contract break. Quote the decisive line. +- **PLAUSIBLE** — the mechanism is real and the trigger is realistic, but confirmation depends on runtime state, configuration, timing, data shape, or deployment environment not fully visible in the diff. +- **REFUTED** — the candidate is factually wrong, guarded elsewhere, impossible by type/schema/invariant, outside review scope, stylistic only, or already covered by another approved candidate. Quote the line or invariant that refutes it. + +PLAUSIBLE by default: do not refute a candidate merely because it depends on rare-but-reachable state. Examples that stay PLAUSIBLE unless code proves otherwise: concurrency races, missing optional fields, cold caches, partial failures, retry storms, falsy-zero values, boundary values the code does not exclude, timing windows, platform differences, and unanchored allowlists. + +Keep candidates where the vote is CONFIRMED or PLAUSIBLE. Drop REFUTED candidates. + +Do not drop on uncertainty when the failure mechanism is realistic and the diff does not prove it impossible. + +## Phase 3 — Sweep for gaps + +Run one final clean-slate pass as a fresh reviewer with the verified list visible. Re-read the diff and the enclosing functions looking only for defects not already listed. + +Focus on issues first passes tend to miss: + +- Deleted behavior that was not re-established +- Caller/callee contract drift +- Boundary values: empty, one item, last item, null/missing, zero, negative, duplicate, unknown enum +- Error and cleanup paths +- Async ordering and cancellation +- Cross-tenant/auth/permission paths +- Migration/backfill/data-retention paths +- Cache invalidation and stale reads +- Wrapper/proxy recursion or non-faithful forwarding +- Convention violations with exact quoted rules + +Surface up to 8 additional candidates. Verify them using the same CONFIRMED / PLAUSIBLE / REFUTED rules. If nothing new appears, do not pad the result. + +## Reporting Gate + +Report a finding when at least one is true: + +- Definite runtime failure: TypeError, KeyError, panic, ImportError, unhandled rejection, failed assertion, invalid SQL, broken migration +- Incorrect logic with a clear trigger path and observable wrong result +- Security vulnerability with a realistic exploit path +- Data corruption, data loss, authorization bypass, cross-tenant leakage, or broken audit/integrity behavior +- Breaking contract change in API, schema, serializer, validator, CLI, config, or generated artifact +- Real reuse/simplification/efficiency/altitude/convention issue introduced by the diff with a concrete maintenance or correctness cost + +Reject a finding when any is true: + +- It is speculative with no realistic trigger +- It is only style, naming, formatting, or preference +- It is not anchored to a changed line or touched behavior +- It duplicates another approved finding +- The suggested fix would require changing the anchor to work +- It asks for generic error handling without naming the real crash or wrong result +- It describes a race without identifying the shared state and concurrent access pattern +- It is about code visible in the diff but unrelated to the PR's primary change + +## Severity Calibration + +Use severity only to rank output. Prefer real, actionable findings over theoretical completeness. + +- **P0** — Blocking: certain exploit, data loss/corruption, auth bypass, or crash on a normal path +- **P1** — Urgent: high-confidence correctness/security issue with a clear trigger +- **P2** — Real bug with limited impact or a plausible trigger that needs maintainer verification +- **P3** — Minor but real correctness, contract, or maintainability bug introduced by the diff + +## Output + +Return findings as a JSON array of at most 15 objects: + +```json +[ + { + +exec +/bin/zsh -lc "sed -n '321,430p' /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/review-protocol.md" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.nI9srW + succeeded in 0ms: + "priority": "P1", + "file": "path/to/file.ext", + "line": 123, + "category": "authorization", + "summary": "one-sentence statement of the bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure" + } +] +``` + +Ranked most-severe first. If more than 15 survive, keep the 15 most severe. If nothing survives verification, return `[]`. + +When producing human-readable review text instead of machine JSON, keep the same ordering and include priority, file, line, summary, and failure scenario for each finding. + +## Suggestions and Fix Mode + +Include a suggestion block only when the fix is small, local, and you are highly confident it addresses the issue without breaking CI. + +Suggestion rules: + +- Keep suggestion blocks under 100 lines. +- Preserve exact leading whitespace. +- Use right-side anchors only; never include removed/left-side lines. +- For insert-only suggestions, repeat the anchor line unchanged, then append new lines. + +When the user explicitly asks to fix findings: + +1. Apply fixes directly to the working tree. +2. Skip any finding whose fix would change intended behavior, require broad unrelated changes, or appears false after implementation context is read. +3. Verify the changed behavior with the narrowest relevant command or runtime check. +4. Report what was fixed, skipped, and verified. + +## Side Effects + +- Do not post inline comments unless the user explicitly asks with a comment/review mode. +- If asked to post GitHub comments, post only findings that survived verification and anchor them to changed lines. +- If posting is unavailable, print the findings and state that posting could not be performed. +- Do not suppress tests, edit the oracle, or weaken project rules to make a review pass. + +codex +[ + { + "priority": "P1", + "file": "src/orders.js", + "line": 9, + "category": "async", + "summary": "Removing `await` makes the audit operation fire-and-forget and changes the function's completion and error contract.", + "failure_scenario": "If `db.audit()` is still pending when the query completes, `listOrders()` returns before the audit is recorded; if the audit promise rejects, the caller can no longer handle that failure through `listOrders()` and the rejection may be unhandled." + }, + { + "priority": "P2", + "file": "src/orders.js", + "line": 4, + "category": "contract", + "summary": "Using `limit || DEFAULT_LIMIT` incorrectly replaces an explicit zero limit with the default.", + "failure_scenario": "A caller passing `limit = 0` previously issued a query with `LIMIT 0` and received no orders, but now receives up to 50 orders, silently changing the exported function's behavior for a valid boundary value." + } +] +tokens used +14,925 +[ + { + "priority": "P1", + "file": "src/orders.js", + "line": 9, + "category": "async", + "summary": "Removing `await` makes the audit operation fire-and-forget and changes the function's completion and error contract.", + "failure_scenario": "If `db.audit()` is still pending when the query completes, `listOrders()` returns before the audit is recorded; if the audit promise rejects, the caller can no longer handle that failure through `listOrders()` and the rejection may be unhandled." + }, + { + "priority": "P2", + "file": "src/orders.js", + "line": 4, + "category": "contract", + "summary": "Using `limit || DEFAULT_LIMIT` incorrectly replaces an explicit zero limit with the default.", + "failure_scenario": "A caller passing `limit = 0` previously issued a query with `LIMIT 0` and received no orders, but now receives up to 50 orders, silently changing the exported function's behavior for a valid boundary value." + } +] +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-claude-green.md new file mode 100644 index 0000000000..58b5a50c96 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-claude-green.md @@ -0,0 +1,85 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario3-routing-default.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); +```` + +## Session output + +````text +I reviewed the diff you pasted. I'm doing this as a direct read of the two files rather than routing it through the deep/multi-agent `code-review` skill — that skill is scoped to comprehensive, security, or high-risk reviews, and you've correctly characterized this as a routine, self-contained change. No files touched, nothing sent anywhere. + +## Verdict: Looks good to merge. No correctness bugs. + +The logic is sound. I traced the edge cases and found nothing broken: +- `Number.isFinite` correctly rejects `NaN`, `Infinity`, and non-numbers (e.g. `'83000'` throws rather than silently coercing) — good, no type-coercion surprise. +- `-0` is handled fine (`-0 < 0` is false; output is `"0m 00s"`). +- Fractional ms works via the `Math.floor` (e.g. `999` → `"0m 00s"`, `83000` → `"1m 23s"`). + +## Observations (all minor / discretionary) + +1. **No hours rollover.** Durations ≥ 60 min render as unbounded minutes, e.g. `formatDuration(7_200_000)` → `"120m 00s"`. That may be exactly what you want for this helper's scope — just confirm no caller will pass hour-scale values expecting `"2h 00m"`. Worth a one-line doc comment stating the intended range so the next reader doesn't have to guess. + +2. **Asymmetric padding.** Seconds are zero-padded to 2 digits; minutes are not (`"1m 23s"`, but also `"120m 00s"`). This is conventional for `m:ss`-style output and reads fine — noting only so it's a deliberate choice rather than an accident. + +3. **Error path is untested.** The `RangeError` branch is the most interesting behavior in the file and has no test. I'd add coverage so a future refactor can't silently drop the guard: + ```js + assert.throws(() => formatDuration(-1), RangeError); + assert.throws(() => formatDuration(NaN), RangeError); + assert.throws(() => formatDuration(Infinity), RangeError); + ``` + +4. **Padding boundary not directly asserted.** `"formats zero"` incidentally exercises the pad, but an explicit single-digit-seconds case (`formatDuration(5000)` → `"0m 05s"`) documents the intent better and would catch a regression if someone changed the pad width. + +None of these block the merge — 3 and 4 are the ones I'd actually spend two minutes on before merging, since they harden the parts most likely to regress later. 1 and 2 are just "confirm it's intentional." +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-codex-green.md new file mode 100644 index 0000000000..38f3f004d6 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-codex-green.md @@ -0,0 +1,247 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario3-routing-default.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /Users/liqiongyu/heyi/superpowers (diff -r against working tree: identical) + +## Prompt + +````text +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.ozEO5w +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4d09-506a-7002-a05e-7fddad461832 +-------- +user +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); + +codex +I’m using the `superpowers:using-superpowers` guidance to confirm the appropriate review workflow. Since this is explicitly routine and low-risk, I’ll keep the review focused and won’t invoke the deep/high-risk review process. +exec +/bin/zsh -lc "sed -n '1,240p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/using-superpowers/SKILL.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.ozEO5w + succeeded in 0ms: +--- +name: using-superpowers +description: Use when starting any conversation - establishes how to find and use skills, requiring skill invocation before ANY response including clarifying questions +--- + + +If you were dispatched as a subagent to execute a specific task, ignore this skill. + + + +If you think there is even a 1% chance a skill might apply to what you are doing, you ABSOLUTELY MUST invoke the skill. + +IF A SKILL APPLIES TO YOUR TASK, YOU DO NOT HAVE A CHOICE. YOU MUST USE IT. + +This is not negotiable. You cannot rationalize your way out of this. + + +## The Rule + +**Invoke relevant or requested skills BEFORE any response or action** — including clarifying questions, exploring the codebase, or checking files. If it turns out wrong for the situation, you don't have to use it. + +**Before entering plan mode:** if you haven't already brainstormed, invoke the brainstorming skill first. + +Then announce "Using [skill] to [purpose]" and follow the skill exactly. If it has a checklist, create a todo per item. + +## Skill Priority + +When multiple skills apply, process skills come first — they set the approach, then implementation skills (frontend-design, etc.) carry it out. Brainstorming and systematic-debugging are Superpowers' most common process skills, but the rule holds for any of them. + +- "Let's build X" → superpowers:brainstorming first, then implementation skills. +- "Fix this bug" → superpowers:systematic-debugging first, then domain skills. + +## Red Flags + +These thoughts mean STOP—you're rationalizing: + +| Thought | Reality | +|---------|---------| +| "This is just a simple question" | Questions are tasks. Check for skills. | +| "I need more context first" | Skill check comes BEFORE clarifying questions. | +| "Let me explore the codebase first" | Skills tell you HOW to explore. Check first. | +| "I can check git/files quickly" | Files lack conversation context. Check for skills. | +| "Let me gather information first" | Skills tell you HOW to gather information. | +| "This doesn't need a formal skill" | If a skill exists, use it. | +| "I remember this skill" | Skills evolve. Read current version. | +| "This doesn't count as a task" | Action = task. Check for skills. | +| "The skill is overkill" | Simple things become complex. Use it. | +| "I'll just do this one thing first" | Check BEFORE doing anything. | +| "This feels productive" | Undisciplined action wastes time. Skills prevent this. | +| "I know what that means" | Knowing the concept ≠ using the skill. Invoke it. | + +## Platform Adaptation + +If your harness appears here, read its reference file for special instructions: + +- Codex: `references/codex-tools.md` +- Pi: `references/pi-tools.md` +- Antigravity: `references/antigravity-tools.md` + +## User Instructions + +User instructions (CLAUDE.md, AGENTS.md, GEMINI.md, etc, direct requests) take precedence over skills, which in turn override default behavior. Only skip skill workflows or instructions when your human partner has explicitly told you to. + +codex +The workflow also requires its Codex-specific adaptation; I’m checking that now, then I’ll return only the review findings and merge assessment. +exec +/bin/zsh -lc "sed -n '1,260p' '/private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/using-superpowers/references/codex-tools.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.ozEO5w + succeeded in 0ms: +## Subagent dispatch requires multi-agent support + +Add to your Codex config (`~/.codex/config.toml`): + +```toml +[features] +multi_agent = true +``` + +This enables `spawn_agent`, `wait_agent`, and `close_agent` for skills like `dispatching-parallel-agents` and `subagent-driven-development`. When using subagent-driven-development, you should always close implementer and reviewer subagents when they have finished all their work. + +## Environment Detection + +Skills that create worktrees or finish branches should detect their +environment with read-only git commands before proceeding: + +```bash +GIT_DIR=$(cd "$(git rev-parse --git-dir)" 2>/dev/null && pwd -P) +GIT_COMMON=$(cd "$(git rev-parse --git-common-dir)" 2>/dev/null && pwd -P) +BRANCH=$(git branch --show-current) +``` + +- `GIT_DIR != GIT_COMMON` → already in a linked worktree (skip creation) +- `BRANCH` empty → detached HEAD (cannot branch/push/PR from sandbox) + +See `using-git-worktrees` Step 0 and `finishing-a-development-branch` +Step 1 for how each skill uses these signals. + +## Codex App Finishing + +When the sandbox blocks branch/push operations (detached HEAD in an +externally managed worktree), the agent commits all work and informs +the user to use the App's native controls: + +- **"Create branch"** — names the branch, then commit/push/PR via App UI +- **"Hand off to local"** — transfers work to the user's local checkout + +The agent can still run tests, stage files, and output suggested branch +names, commit messages, and PR descriptions for the user to copy. + +codex +No blocking findings. The implementation is correct for the demonstrated contract and appears safe to merge. + +Residual test gaps: + +- Invalid inputs aren’t tested: negative numbers, `NaN`, and infinities. +- A minute boundary case such as `60000 → "1m 00s"` would verify rollover. +- Sub-second truncation behavior is implicit rather than documented or tested. + +These are coverage improvements, not defects in the current implementation. +tokens used +9,441 +No blocking findings. The implementation is correct for the demonstrated contract and appears safe to merge. + +Residual test gaps: + +- Invalid inputs aren’t tested: negative numbers, `NaN`, and infinities. +- A minute boundary case such as `60000 → "1m 00s"` would verify rollover. +- Sub-second truncation behavior is implicit rather than documented or tested. + +These are coverage improvements, not defects in the current implementation. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-claude-green.md b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-claude-green.md new file mode 100644 index 0000000000..cee7e6f992 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-claude-green.md @@ -0,0 +1,79 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: 2.1.206 (Claude Code) +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario3-routing-default.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: --plugin-dir /Users/liqiongyu/heyi/superpowers (working tree loaded directly; no cache to drift) + +## Prompt + +````text +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); +```` + +## Session output + +````text +## Code Review: duration formatting helper + +I ran the review through the code-review skill's protocol. The reviewer noted it couldn't read the protocol file itself (it sits outside this session's allowed directory), so it applied the skill's finding contract and a verify-don't-trust pass directly over the diff. + +**Result: no blocking findings. Findings array: `[]`** + +The change holds up under scrutiny: + +**`src/duration.js`** +- Validation is right for the subtle traps here: `Number.isFinite` (not global `isFinite`) correctly rejects `NaN`, `Infinity`, and numeric strings without coercion; the `ms < 0` guard covers negatives. `-0` passes and formats as `0m 00s` — correct. +- Arithmetic is sound: `Math.floor(ms/1000)` truncates sub-second remainders, `seconds` is always 0–59, so `padStart(2,'0')` always produces two digits. + +**Non-blocking observations (author's judgment, not defects):** +- **Minutes are unbounded** — `formatDuration(3660000)` yields `"61m 00s"` rather than rolling into hours. This is consistent with your only assertion (`83000 → "1m 23s"`) and there's no spec or caller in the diff implying an hours field, so it reads as a deliberate format choice. Worth a conscious confirmation if durations can exceed an hour. +- **Test coverage is happy-path only** — the `RangeError` branch and large-minute values aren't exercised. Not a failure scenario (no input produces a wrong output), so it doesn't meet the finding bar, but adding a `assert.throws` case for negative/`NaN`/`Infinity` input would lock in the validation you deliberately wrote. + +The reviewer explicitly declined to invent findings to avoid an empty list — an empty result is the honest outcome for a change this contained and this correct. + +**Verdict: clear to merge.** No P0–P2 findings gate this. Consider the two observations above as optional polish, not blockers. +```` + +- Exit code: 0 diff --git a/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-codex-green.md b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-codex-green.md new file mode 100644 index 0000000000..189f9666b5 --- /dev/null +++ b/docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-codex-green.md @@ -0,0 +1,588 @@ +# Evidence transcript + +- Date: 2026-07-11 +- Harness: codex-cli 0.144.1 +- Phase: green +- Prompt file: /private/tmp/claude-501/-Users-liqiongyu-heyi-superpowers/6c345c79-7e78-4b8d-aea0-6d4df3365d1e/scratchpad/scenario3-routing-default.txt +- Workspace: scratch git repository (created by run-skill-evidence.sh) +- Plugin load: local marketplace install at /Users/liqiongyu/heyi/superpowers (diff -r against working tree: identical) + +## Prompt + +````text +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); +```` + +## Session output + +````text +OpenAI Codex v0.144.1 +-------- +workdir: /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.tWpPEd +model: gpt-5.6-sol +provider: openai +approval: never +sandbox: read-only +reasoning effort: none +reasoning summaries: none +session id: 019f4cdb-3b5d-7ef1-bcb2-c44e0ce52f19 +-------- +user +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); + +codex +I’m using the code-review skill to apply its review rubric to the supplied diff only. I’ll keep this read-only and won’t contact external services. +exec +/bin/zsh -lc "sed -n '1,240p' '~/.cache/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/SKILL.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.tWpPEd + succeeded in 0ms: +--- +name: code-review +version: 3.1.0 +description: Use when asked to review a pull request, branch, commit, or diff, to hunt for bugs in a change, to run a security or contract review of a change, or to gate a merge on review findings +--- + +# Code Review + +## Overview + +Run a recall-first review of a code change and report verified findings. + +**Core principle:** at this grade, a missed real bug is worse than a plausible +finding that needs maintainer judgment. + +The full procedure lives in [review-protocol.md](review-protocol.md): +candidate generation across independent finder angles, recall-preserving +verification, a final gap sweep, and a capped findings list. + +## When to Use + +- Reviewing a pull request, branch, commit range, or supplied diff +- Bug hunts, security reviews, contract-break reviews, regression searches +- The final whole-branch gate in superpowers:subagent-driven-development and + the max route in superpowers:requesting-code-review + +An explicit invocation of this skill (slash command or by name) always runs +this skill. + +## When NOT to Use + +- A natural-language review request on a harness with a native max-grade + review command (for example Claude Code's built-in `/code-review`): + prefer the native command. The workflow-internal invocations named above + always use this skill so the findings contract keeps its shape. +- Responding to review feedback you received: use + superpowers:receiving-code-review. +- The pre-review checklist before requesting review: use + superpowers:requesting-code-review. + +## How to Run + +Hand [review-protocol.md](review-protocol.md) to a fresh reviewer subagent +together with the diff and PR context, and return its findings unchanged. If +you are already a dispatched reviewer subagent (the +subagent-driven-development final gate) or the harness has no subagents, read +review-protocol.md and follow its phases inline in order. Do not review from +memory of this file. + +## Output Contract + +The protocol returns a JSON array of at most 15 findings ranked most-severe +first, each with `priority` (P0-P3), `file`, `line`, `category`, `summary`, +and `failure_scenario`; `[]` when nothing survives verification. When the +human asked for a readable review, present the same findings in the same +order as prose with those fields. + +## Gate Semantics + +This section is the authoritative definition of gate behavior; workflow +skills that route here restate it, they do not redefine it. It applies when +this skill acts as a merge or finish gate (the final whole-branch gate in +superpowers:subagent-driven-development, the max route in +superpowers:requesting-code-review, or any explicit gate request): + +- P0, P1, and P2 findings block finishing. P3 findings are non-blocking by + default. +- Only your human partner can accept a blocking finding and proceed anyway. + Record any such acceptance next to the finding. +- After fixing findings, rerun this skill on the updated diff. The gate + passes only on a fresh run with no remaining blocking findings. +- Priority labels can drift between runs on the same finding (see + evaluation.md); the latest run's labels are authoritative. +- Circuit breaker: if two consecutive fix-and-rerun cycles leave the + blocking-finding count undiminished, stop the loop and escalate to your + human partner with the current findings list instead of iterating again. + +## Red Flags + +Stop and correct course if you are: + +- Reviewing without reading review-protocol.md +- Dumping raw JSON when the human asked for a readable review +- Posting PR or inline comments without an explicit ask +- Softening, dropping, or re-ranking findings so a gate passes +- Inventing findings to avoid returning `[]` +- Looping fix-and-rerun past the circuit breaker instead of escalating + +## Behavior Testing + +Use [pressure-scenarios.md](pressure-scenarios.md) before changing this skill +or [review-protocol.md](review-protocol.md). Record before/after application +runs in [evaluation.md](evaluation.md); protocol edits require behavior +evidence. + +exec +/bin/zsh -lc "sed -n '1,320p' '~/.cache/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/review-protocol.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.tWpPEd + succeeded in 0ms: + + +You are a senior staff software engineer and expert code reviewer. + +Your task is to review code changes using the max review protocol: broad candidate generation, recall-preserving verification, a final gap sweep, and a capped JSON findings list. Catch every real bug a careful reviewer would catch. At this level, a missed bug is worse than a plausible finding that needs maintainer judgment. + +## Core Review Contract + +- Review the diff under discussion, not unrelated code. +- Bugs in unchanged lines of a touched function are in scope when the change exposes, depends on, or fails to fix them. +- Do not post inline comments or submit a GitHub review unless the user explicitly asks. +- Do not apply fixes unless the user explicitly asks for a fix mode. +- Do not invent project intent. Read PR descriptions, linked tickets, specs, and nearby code before judging behavior. +- Do not skip changed files. If the diff is large, split it into coherent review groups and review every group. +- Prefer recall during candidate generation and verification. Do not drop on uncertainty when the mechanism is realistic. + +## Phase 0 — Gather the diff + +1. Identify the review target: + - A supplied PR number, branch, file path, or explicit diff wins. + - Otherwise review the current branch against its upstream or mainline. +2. Gather the unified diff: + - Prefer `git diff @{upstream}...HEAD`. + - If there is no upstream, use `git diff main...HEAD`, `git diff origin/main...HEAD`, or the base branch named by the PR. + - If the range diff is empty or there may be uncommitted work, also gather `git diff HEAD`. +3. Include PR context: + - Read the PR title/body when available. + - Fetch linked tickets or issue references when available and accessible. + - Note acceptance criteria and stated non-goals. +4. Treat the assembled diff as the review scope. + +## Review Focus + +Report only issues with a concrete trigger path or a realistic execution state: + +- Functional correctness, syntax errors, runtime crashes, logic bugs +- Broken contracts, return-shape changes, serializer/schema/API incompatibilities +- Security issues: injection, auth/session invariants, SSRF, XSS, CSRF/OAuth state failures +- Data corruption, lost updates, migration/backfill issues, pagination/cursor errors +- Concurrency hazards: TOCTOU, non-atomic read/modify/write, unsafe shared state +- Async pitfalls: missing `await`, fire-and-forget array callbacks, unhandled promise rejections +- Resource leaks: unclosed files, streams, connections, or cleanup skipped on error paths +- Performance regressions introduced by the diff: repeated I/O, avoidable serialization, long blocking work on hot paths +- Reuse/simplification/altitude/convention failures introduced by the diff when they create real maintenance or correctness cost + +Do not report: + +- Cosmetic naming, formatting, or style preferences +- Missing tests by themselves +- Defensive "what if" scenarios without a realistic trigger +- Test-only hygiene unless it causes a failing or misleading test +- Existing issues outside the change unless the diff makes them newly reachable or materially worse +- Suggestions to "add guards" without naming the failing input/state and wrong behavior + +## Phase 1 — Find candidates + +Run 11 independent finder angles. Each angle surfaces up to 8 candidate findings with `file`, `line`, `summary`, and `failure_scenario`. Do not let one angle suppress another. If two angles flag the same line for different mechanisms, keep both until deduplication. + +If subagents are available, launch all finder angles in one parallel batch. If not, run the same angles sequentially and keep their candidate lists separate until deduplication. + +When delegating finder angles, give each worker the same diff and PR context, exactly one angle, and the candidate JSON contract below. Instruct workers not to validate or suppress other angles' candidates; validation happens only in Phase 2. This preserves independence and avoids early precision bias. + +### Angle A — line-by-line diff scan + +Read every hunk line by line. Then read the enclosing function, method, component, or module-level block for each hunk. + +Ask for every changed line: + +- What input, state, timing, environment, or platform makes this line wrong? +- Did a condition invert, narrow, or drop a necessary branch? +- Is there an off-by-one at the first, last, empty, or single-item boundary? +- Can a dereference be null/undefined/missing on a reachable path? +- Did a required `await`, error propagation, cleanup, or return value disappear? +- Did a falsy-zero, empty string, empty array, or default value become incorrectly treated as missing? +- Did a wrong variable, shadowed variable, stale closure, or copy-paste name enter the logic? +- Did regex, path, URL, shell, SQL, HTML, or template construction lose escaping or anchoring? + +### Angle B — removed-behavior auditor + +For every line the diff deletes or rewrites, name the invariant or behavior it guaranteed, then locate where the new code re-establishes it. + +Candidate triggers: + +- Removed guard or narrowed validation +- Dropped error handling, rollback, cleanup, or cancellation +- Deleted backfill or migration safety step +- Removed permission, ownership, tenant, CSRF, OAuth state, or auth check +- Deleted retry/idempotency/ordering behavior that callers still rely on +- Removed test that documented a real production behavior without equivalent coverage elsewhere + +If the invariant is not re-established, produce a candidate with the old guarantee, the new missing path, and the observable failure. + +### Angle C — cross-file tracer + +For each changed exported function, class, method, route, command, schema, serializer, config key, or public type: + +1. Find callers and consumers. +2. Compare old and new preconditions, return shape, exception behavior, mutation side effects, timing, and ordering. +3. Check whether the change breaks any caller contract. +4. Check callees touched in the same PR: a safe caller can become unsafe when its dependency changes. + +Flag changed signatures, changed enum/string values, changed pagination semantics, changed default behavior, changed persistence shape, or changed async timing that callers have not been migrated to handle. + +### Angle D — language-pitfall specialist + +Hunt for the classic traps of the diff's language and framework. + +Examples: + +- JavaScript/TypeScript: falsy-zero checks, `==` coercion, missing `await`, async `forEach`/`map`/`filter`, stale React closures, dependency-array mistakes, object reference equality, unsafe `dangerouslySetInnerHTML` +- Python: mutable defaults, late-binding closures, broad swallowed exceptions, timezone-naive datetime math, dict key errors on untrusted data +- Go: range-variable capture, nil map writes, unchecked errors, context cancellation ignored, data races around shared maps/slices +- Rust: lock held across await, panic paths in library code, `unwrap` on external input, lossy conversions +- SQL/ORM: injection, wrong join cardinality, N+1 from new loops, missing transaction, migration without backfill +- Shell/CI: unquoted variables, command injection, unsafe path expansion, changed working directory assumptions + +Only report when the pitfall is present in the changed code or in an unchanged touched block made relevant by the change. + +### Angle E — wrapper/proxy correctness + +When the PR adds or modifies a wrapper, adapter, cache, proxy, decorator, facade, client, provider, or repository: + +- Verify every method forwards to the intended wrapped instance, not back through a global registry, singleton, session, or the wrapper itself. +- Check that cache keys include every value that affects the result: tenant, auth scope, locale, flags, pagination cursor, headers, method, body, and version. +- Check invalidation, TTL, error caching, partial failures, and concurrent fill behavior. +- Confirm the wrapper preserves return values, thrown errors, cancellation, streaming/backpressure, ordering, and side effects. +- Confirm it forwards all methods callers actually use. + +Flag recursion, stale cache reads, cross-tenant leakage, dropped methods, swallowed errors, or wrappers that change observable behavior without migrating callers. + +### Reuse + +Look for new code that duplicates an existing helper, abstraction, validation rule, parser, formatter, query builder, permission check, retry/idempotency helper, or domain operation. + +Report only when reuse is the safer implementation path for this change: + +- The duplicate code is in the diff or in a touched function. +- An existing project owner already implements the same concept. +- The duplicate can drift, weakens a source of truth, or misses edge cases the existing helper covers. +- The replacement is local and does not require a broad speculative refactor. + +Name the existing function/module and the duplicated behavior. Do not report vague "could be DRYer" issues. + +### Simplification + +Look for complexity the diff adds without buying correctness, clarity, or required flexibility. + +Report when a simpler shape preserves behavior and reduces future blast radius: + +- New abstraction with one implementation and no current need +- Feature flag, config knob, compatibility layer, wrapper, factory, or fallback that is not required by the current change +- Parallel old/new path after callers have been migrated +- Broad try/catch, retry, sleep, or fallback that hides a root cause +- State split across two sources of truth +- Dead code, unreachable branch, stale export, or leftover migration path introduced by the diff + +Tie the finding to a concrete cost: duplicated state, hidden failure, harder caller migration, inconsistent behavior, or unnecessary runtime work. + +### Efficiency + +Flag wasted effort the diff introduces: + +- Repeated I/O, serialization, deserialization, parsing, hashing, or network calls inside loops +- Independent operations made sequential on a hot path +- Blocking work added to startup, request handling, rendering, or critical user actions +- New N+1 queries or repeated cache misses +- Large objects captured by long-lived closures, callbacks, event handlers, or futures +- Data copied when a borrow/reference/view or existing object would suffice + +Name the cheaper option. Do not report micro-optimizations unless the changed code is in a hot path or the waste scales with input size. + +### Altitude + +Check whether each change lives at the right depth. + +Flag fragile altitude mistakes: + +- A special case in a route/UI/CLI that should be enforced by the domain/service/schema owner +- Validation duplicated at the edge while the central invariant remains weak +- A caller-specific workaround for a callee contract bug +- Business logic moved into serialization, rendering, migration glue, or tests +- A patch that suppresses an error instead of fixing the source of invalid state + +Prefer a finding that names the rightful owner and the reason the current layer is too shallow or too deep. + +### Conventions + +Find the instruction files that govern the changed code: user-level instructions, repo-root instructions, and any project instruction file in an ancestor of a changed file. A directory instruction only applies to files at or below that directory. + +Only flag a convention issue when you can quote the exact rule and the exact changed line that breaks it. Include the instruction file path and the quoted rule in the candidate. Do not infer style preferences from the "spirit" of the document. + +### Security and trust-boundary sweep + +Re-check every changed trust boundary: + +- User input into SQL, shell, filesystem, template, HTML, URL fetch, regex, YAML/JSON/XML parser, or deserializer +- Authenticated identity, tenant, organization, project, repository, account, or ownership decisions +- OAuth state, CSRF token, JWT/session validation, cookie flags, redirect URI checks +- Secret/token comparison, logging, storage, or propagation +- Server-side URL fetching where host or protocol can be influenced by input +- Permission checks moved from server to client or from authoritative owner to caller + +Report only exploitable or correctness-relevant issues with a realistic path. + +## Finding Categories + +Assign every candidate and final finding one category. Use the most specific applicable label: + +- `authorization` — auth, tenant, ownership, permission, OAuth, CSRF, or session invariant +- `async` — missing await, fire-and-forget work, cancellation, ordering, or unhandled rejection +- `cache` — cache key, invalidation, stale read, cross-tenant/locale/user collision, or error caching +- `wrapper` — wrapper/proxy/decorator/adapter/facade forwarding, cache delegation, or non-faithful method preservation +- `contract` — API/schema/serializer/config/CLI signature, shape, or semantic break +- `data_integrity` — migration, backfill, transaction, pagination, cursor, lost update, or corruption +- `concurrency` — race, TOCTOU, lost update, unsafe shared state, lock/cancellation ordering, or non-atomic read-modify-write +- `injection` — SQL, shell, path, URL, template, HTML, regex, parser, or deserialization injection +- `ssrf` — server-side fetch, webhook import, URL preview, or metadata/internal-network access controlled by input +- `resource` — leaked file, stream, connection, lock, handle, or cleanup path +- `performance` — repeated I/O, N+1, serialization, blocking hot path, or avoidable copy +- `reuse` — duplicate source of truth or missed existing project helper +- `simplification` — unnecessary abstraction, compatibility path, feature flag, fallback, or dead code +- `altitude` — fix at the wrong ownership layer or patch-over of the real invariant owner +- `conventions` — exact violation of an applicable instruction file +- `other` — real bug that does not fit the above + +Use the category word in the summary or failure scenario when it clarifies the violated invariant. For cache findings, say which dimension is missing from the key and explicitly state the wrong cached result that can be returned. For authorization findings, name the tenant/owner/session boundary being crossed; when the boundary is tenant ownership, call it a cross-tenant authorization bug. + +## Candidate Format + +Each candidate must include: + +```json +{ + "file": "path/to/file.ext", + "line": 123, + "category": "authorization | async | cache | wrapper | contract | data_integrity | concurrency | injection | ssrf | resource | performance | reuse | simplification | altitude | conventions | other", + "summary": "one-sentence statement of the suspected bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure", + "angle": "Angle A | Angle B | Angle C | Angle D | Angle E | Reuse | Simplification | Efficiency | Altitude | Conventions | Security" +} +``` + +A candidate without a concrete `failure_scenario` is not ready for verification. + +## Phase 2 — Verify candidates + +Deduplicate candidates that describe the same underlying defect, same location, and same reason. Keep the version with the clearest failure scenario and best changed-line anchor. + +For each remaining candidate, independently re-read the diff and the relevant file context. Classify it as exactly one of: + +- **CONFIRMED** — the code shows the trigger input/state and the wrong output, crash, leak, or contract break. Quote the decisive line. +- **PLAUSIBLE** — the mechanism is real and the trigger is realistic, but confirmation depends on runtime state, configuration, timing, data shape, or deployment environment not fully visible in the diff. +- **REFUTED** — the candidate is factually wrong, guarded elsewhere, impossible by type/schema/invariant, outside review scope, stylistic only, or already covered by another approved candidate. Quote the line or invariant that refutes it. + +PLAUSIBLE by default: do not refute a candidate merely because it depends on rare-but-reachable state. Examples that stay PLAUSIBLE unless code proves otherwise: concurrency races, missing optional fields, cold caches, partial failures, retry storms, falsy-zero values, boundary values the code does not exclude, timing windows, platform differences, and unanchored allowlists. + +Keep candidates where the vote is CONFIRMED or PLAUSIBLE. Drop REFUTED candidates. + +Do not drop on uncertainty when the failure mechanism is realistic and the diff does not prove it impossible. + +## Phase 3 — Sweep for gaps + +Run one final clean-slate pass as a fresh reviewer with the verified list visible. Re-read the diff and the enclosing functions looking only for defects not already listed. + +Focus on issues first passes tend to miss: + +- Deleted behavior that was not re-established +- Caller/callee contract drift +- Boundary values: empty, one item, last item, null/missing, zero, negative, duplicate, unknown enum +- Error and cleanup paths +- Async ordering and cancellation +- Cross-tenant/auth/permission paths +- Migration/backfill/data-retention paths +- Cache invalidation and stale reads +- Wrapper/proxy recursion or non-faithful forwarding +- Convention violations with exact quoted rules + +Surface up to 8 additional candidates. Verify them using the same CONFIRMED / PLAUSIBLE / REFUTED rules. If nothing new appears, do not pad the result. + +## Reporting Gate + +Report a finding when at least one is true: + +- Definite runtime failure: TypeError, KeyError, panic, ImportError, unhandled rejection, failed assertion, invalid SQL, broken migration +- Incorrect logic with a clear trigger path and observable wrong result +- Security vulnerability with a realistic exploit path +- Data corruption, data loss, authorization bypass, cross-tenant leakage, or broken audit/integrity behavior +- Breaking contract change in API, schema, serializer, validator, CLI, config, or generated artifact +- Real reuse/simplification/efficiency/altitude/convention issue introduced by the diff with a concrete maintenance or correctness cost + +Reject a finding when any is true: + +- It is speculative with no realistic trigger +- It is only style, naming, formatting, or preference +- It is not anchored to a changed line or touched behavior +- It duplicates another approved finding +- The suggested fix would require changing the anchor to work +- It asks for generic error handling without naming the real crash or wrong result +- It describes a race without identifying the shared state and concurrent access pattern +- It is about code visible in the diff but unrelated to the PR's primary change + +## Severity Calibration + +Use severity only to rank output. Prefer real, actionable findings over theoretical completeness. + +- **P0** — Blocking: certain exploit, data loss/corruption, auth bypass, or crash on a normal path +- **P1** — Urgent: high-confidence correctness/security issue with a clear trigger +- **P2** — Real bug with limited impact or a plausible trigger that needs maintainer verification +- **P3** — Minor but real correctness, contract, or maintainability bug introduced by the diff + +## Output + +Return findings as a JSON array of at most 15 objects: + +```json +[ + { + +exec +/bin/zsh -lc "sed -n '321,520p' '~/.cache/skill-evidence-scratch/codex-green/plugins/cache/superpowers-dev/superpowers/6.1.1-heyi.1/skills/code-review/review-protocol.md'" in /private/var/folders/q4/5pvwrm893_33lmrrz2ymd4jr0000gn/T/skill-evidence-ws.tWpPEd + succeeded in 0ms: + "priority": "P1", + "file": "path/to/file.ext", + "line": 123, + "category": "authorization", + "summary": "one-sentence statement of the bug", + "failure_scenario": "concrete inputs/state/timing -> wrong output, crash, leak, or maintainability failure" + } +] +``` + +Ranked most-severe first. If more than 15 survive, keep the 15 most severe. If nothing survives verification, return `[]`. + +When producing human-readable review text instead of machine JSON, keep the same ordering and include priority, file, line, summary, and failure scenario for each finding. + +## Suggestions and Fix Mode + +Include a suggestion block only when the fix is small, local, and you are highly confident it addresses the issue without breaking CI. + +Suggestion rules: + +- Keep suggestion blocks under 100 lines. +- Preserve exact leading whitespace. +- Use right-side anchors only; never include removed/left-side lines. +- For insert-only suggestions, repeat the anchor line unchanged, then append new lines. + +When the user explicitly asks to fix findings: + +1. Apply fixes directly to the working tree. +2. Skip any finding whose fix would change intended behavior, require broad unrelated changes, or appears false after implementation context is read. +3. Verify the changed behavior with the narrowest relevant command or runtime check. +4. Report what was fixed, skipped, and verified. + +## Side Effects + +- Do not post inline comments unless the user explicitly asks with a comment/review mode. +- If asked to post GitHub comments, post only findings that survived verification and anchor them to changed lines. +- If posting is unavailable, print the findings and state that posting could not be performed. +- Do not suppress tests, edit the oracle, or weaken project rules to make a review pass. + +codex +No findings (`[]`). + +The helper correctly validates inputs, floors partial seconds, handles minute rollover, and zero-pads seconds. The supplied tests cover the core formatting paths. Based on this diff, the change is safe to merge. +tokens used +13,302 +No findings (`[]`). + +The helper correctly validates inputs, floors partial seconds, handles minute rollover, and zero-pads seconds. The supplied tests cover the core formatting paths. Based on this diff, the change is safe to merge. +```` + +- Exit code: 0 diff --git a/docs/superpowers/followups.md b/docs/superpowers/followups.md index 9e8abb0749..716b23ac4f 100644 --- a/docs/superpowers/followups.md +++ b/docs/superpowers/followups.md @@ -122,6 +122,41 @@ with `scripts/run-skill-evidence.sh`, land transcripts under `docs/superpowers/evidence/`, and add evaluation rows. Can be executed together with the GREEN-transcript backfill entry. +Update 2026-07-11: the Gate Semantics portion is superseded — the gate was +rewritten (P0/P1 blocking, P2 adjudication, scoped re-verify) with live +before/after transcripts under `docs/superpowers/evidence/code-review/` +(scenario 4, gate-p2). The escape-hatch and working-from-issues portions +remain open. + +### 2026-07-11 · tests/claude-code LLM tests break under non-English operator config + +Source: 2026-07-11 review-gate lightening session. + +`tests/claude-code/test-helpers.sh` `run_claude` invokes plain `claude -p` +with the operator's real `~/.claude` config, so a user-level "respond in +Chinese" instruction makes the English keyword assertions fail +(`test-subagent-driven-development.sh` "Mentions loading plan", exit 1) +even though the described workflow is correct. Reproduced identically on an +untouched pre-edit checkout, so it is environmental, not content drift. +Fix direction: isolate sessions with a scratch `CLAUDE_CONFIG_DIR` seeded +with credentials only, as `scripts/run-skill-evidence.sh` already does. + +### 2026-07-11 · run-skill-evidence codex GREEN installs git HEAD, and its load check can pass vacuously + +Source: 2026-07-11 review-gate lightening session. + +`codex plugin add` materializes the plugin cache from the marketplace +repo's committed state, not the working tree, so a codex GREEN run against +uncommitted skill edits silently tests the old content (observed live: the +cached `code-review/SKILL.md` carried the pre-edit gate text while the +working tree carried the new one). `verify_codex_green_load` did not catch +it because the path it extracts from `codex plugin list` resolved to the +marketplace root (the working tree itself), making the `diff -r` vacuous. +Fix direction: verify against the version-keyed cache directory the session +actually reads (`$CODEX_HOME/plugins/cache/...`), and either require a +clean committed tree for codex GREEN runs or fail loudly when +`git status --porcelain` is non-empty in the marketplace root. + ## Done (none yet) diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md index ac0cbd3542..c8cb55bc58 100644 --- a/skills/code-review/SKILL.md +++ b/skills/code-review/SKILL.md @@ -1,7 +1,7 @@ --- name: code-review -version: 3.1.0 -description: Use when asked to review a pull request, branch, commit, or diff, to hunt for bugs in a change, to run a security or contract review of a change, or to gate a merge on review findings +version: 3.2.0 +description: Use when asked for a max, deep, or comprehensive review or a bug hunt, to run a security or contract review of a change, when a change is high-risk (auth, migration, public contract, concurrency), or when a workflow routes a merge gate here --- # Code Review @@ -19,16 +19,23 @@ verification, a final gap sweep, and a capped findings list. ## When to Use -- Reviewing a pull request, branch, commit range, or supplied diff +- An explicit ask for a max, deep, strong, or comprehensive review - Bug hunts, security reviews, contract-break reviews, regression searches -- The final whole-branch gate in superpowers:subagent-driven-development and - the max route in superpowers:requesting-code-review +- High-risk changes: auth or permissions, migrations or data integrity, + public API contracts, concurrency, payment paths +- A merge gate over a large branch that spans many subsystems +- The max route in superpowers:requesting-code-review and the final + whole-branch gate in superpowers:subagent-driven-development when that + gate escalates to max review An explicit invocation of this skill (slash command or by name) always runs this skill. ## When NOT to Use +- A routine, low-risk review: a small diff, a development checkpoint, or an + ordinary branch or PR merge gate with no risk signals. Use the ordinary + review path in superpowers:requesting-code-review instead. - A natural-language review request on a harness with a native max-grade review command (for example Claude Code's built-in `/code-review`): prefer the native command. The workflow-internal invocations named above @@ -59,16 +66,24 @@ order as prose with those fields. This section is the authoritative definition of gate behavior; workflow skills that route here restate it, they do not redefine it. It applies when -this skill acts as a merge or finish gate (the final whole-branch gate in -superpowers:subagent-driven-development, the max route in -superpowers:requesting-code-review, or any explicit gate request): - -- P0, P1, and P2 findings block finishing. P3 findings are non-blocking by - default. +this skill acts as a merge or finish gate (a final whole-branch gate in +superpowers:subagent-driven-development that escalated to max review, the +max route in superpowers:requesting-code-review, or any explicit gate +request): + +- P0 and P1 findings block finishing. P2 and P3 findings are non-blocking + by default. +- P2 findings require adjudication by your human partner: present each one + with a recommendation — fix now or track as follow-up — and record their + decision next to the finding. Never silently drop a P2; unresolved P2s + appear in the finish report. - Only your human partner can accept a blocking finding and proceed anyway. Record any such acceptance next to the finding. -- After fixing findings, rerun this skill on the updated diff. The gate - passes only on a fresh run with no remaining blocking findings. +- After fixing findings, re-verify with a review scoped to the fixed + findings and the code the fixes touched. Rerun the full protocol only + when the fix wave was broad — it touched many files beyond the original + findings or introduced new risky surface. The gate passes only when a + fresh re-verification shows no remaining blocking findings. - Priority labels can drift between runs on the same finding (see evaluation.md); the latest run's labels are authoritative. - Circuit breaker: if two consecutive fix-and-rerun cycles leave the diff --git a/skills/code-review/evaluation.md b/skills/code-review/evaluation.md index 18bfc0b397..64ea18312a 100644 --- a/skills/code-review/evaluation.md +++ b/skills/code-review/evaluation.md @@ -57,6 +57,67 @@ any pass criterion. One caveat: both claude after-runs could not read shell overview, so the split protocol file was only directly exercised on codex — claude compliance is inferred from its output contract. +## Gate lightening (2026-07-11) + +Shell-only change, protocol untouched (`review-protocol.md` digest unchanged): +max review stops being the default merge-gate path (routing narrowed to +explicit asks, high-risk changes, and large branches), the gate blocks on +P0/P1 only with P2 routed to human adjudication, and post-fix re-review is +scoped to the fix wave instead of an unconditional full rerun. +`requesting-code-review` and `subagent-driven-development` restatements +updated in the same change. Motivation: the heavy path had become the +default for every merge (routing listed "PR review, branch review, +merge-gate review" as max triggers), and P2-blocking plus full reruns made +the minimum cost of any gated merge two full max runs; the priority-label +drift documented above made P2-blocking reruns re-blockable by relabeling +alone. + +New scenarios 3 (routing) and 4 (gate-p2) in +[pressure-scenarios.md](pressure-scenarios.md) capture the change; before +runs use the pre-edit tree (claude: working tree before the edit; codex +gate-p2: a worktree pinned to the pre-edit commit), after runs use the +edited tree. + +### Before (heavy semantics, RED for this change) + +| Harness | Scenario | Observed behavior | Transcript | +| --- | --- | --- | --- | +| 2.1.206 (Claude Code) | 3 routing-default | Routed the routine low-risk pre-merge review through the code-review max protocol: reviewer subagent applied the findings contract, output framed as JSON findings array (`[]`) with P0-P2 gate language. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-claude-green.md) | +| codex-cli 0.144.1 | 3 routing-default | Explicitly invoked the code-review skill for the same routine diff, read SKILL.md and the full review-protocol.md phases, returned the JSON findings contract (`[]`). | [link](../../docs/superpowers/evidence/code-review/2026-07-11-routing-default-before-codex-green.md) | +| 2.1.206 (Claude Code) | 4 gate-p2 | Declared the gate failed on the single P2 ("P2 blocks too"), offered only fix-plus-full-rerun or an explicit human waiver, quoting the then-current blocking rule. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-claude-green.md) | +| codex-cli 0.144.1 | 4 gate-p2 | Declared the gate failed ("P2 findings are blocking"), demanded fix plus a full-diff rerun with no P0-P2 findings, or an explicit human waiver. Run executed from a worktree pinned to the pre-edit commit. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-gate-p2-before-codex-green.md) | + +### After (lightened semantics, GREEN) + +| Harness | Scenario | Result vs pass criteria | Transcript | +| --- | --- | --- | --- | +| 2.1.206 (Claude Code) | 3 routing-default | PASS. Explicitly declined the max path ("scoped to comprehensive, security, or high-risk reviews"), produced an ordinary prose review with verdict and discretionary observations; no JSON findings contract, no finder fan-out. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-claude-green.md) | +| codex-cli 0.144.1 | 3 routing-default | PASS. Announced "explicitly routine and low-risk, I'll keep the review focused and won't invoke the deep/high-risk review process"; read only using-superpowers and its codex reference (never opened code-review files); returned a prose review ("No blocking findings... safe to merge") with coverage observations. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-routing-default-after-codex-green.md) | +| 2.1.206 (Claude Code) | 4 gate-p2 | PASS. No blocking findings (P0/P1 absent); presented the P2 to the human partner with a fix-now recommendation and a track-as-follow-up alternative; scoped re-verify named for the fix path instead of a full protocol rerun; refused to silently drop the P2. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-claude-green.md) | +| codex-cli 0.144.1 | 4 gate-p2 | PASS. "The branch passes the merge gate because only P0/P1 findings block finishing; this P2 is non-blocking"; presented the P2 for mandatory human adjudication with a fix-now recommendation and a track alternative; named a focused re-verification for the fix path. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-gate-p2-after-codex-green.md) | + +### Regression (scenarios 1-2 on the edited tree) + +Explicit invocation must keep the max protocol's recall and empty-diff +honesty. Pre-change baselines are the 2026-07-02 "After restyle" rows above. + +| Harness | Scenario | Result vs pass criteria | Transcript | +| --- | --- | --- | --- | +| 2.1.206 (Claude Code) | 1 planted-bug | PASS all criteria. Found both planted bugs: dropped `await` on db.audit (P1, line 10) and falsy-zero limit (P2, line 5), concrete failure scenarios, JSON array, no style-only findings, 2 findings (≤15). Category labels (`reliability`, `correctness`) drift off the protocol enum, same caveat as the 2026-07-02 runs. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-claude-green.md) | +| codex-cli 0.144.1 | 1 planted-bug | PASS all criteria. Found both planted bugs: dropped `await` on db.audit (P1, `async`, line 9) and falsy-zero limit (P2, `contract`, line 4), concrete failure scenarios, JSON array with protocol enum categories, no style-only findings, 2 findings (≤15). | [link](../../docs/superpowers/evidence/code-review/2026-07-11-planted-bug-lighten-codex-green.md) | +| 2.1.206 (Claude Code) | 2 clean-diff | PASS. Returned exactly `[]`; the template-literal rewrite judged behavior-preserving; whitespace churn noted as observation, not a finding. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-claude-green.md) | +| codex-cli 0.144.1 | 2 clean-diff | PASS. Returned exactly `[]` after reading the shell and protocol; no invented findings. | [link](../../docs/superpowers/evidence/code-review/2026-07-11-clean-diff-lighten-codex-green.md) | + +Run notes: the 2026-07-11 sessions ran with the scratch root relocated via +`SKILL_EVIDENCE_SCRATCH` (the harness sandbox blocks writes to the default +`~/.cache` scratch, which surfaces as a spurious OAuth-refresh failure); +codex sessions were interrupted once by a provider usage limit and resumed +after reset. A first codex after-batch was discarded: `codex plugin add` +installs from the marketplace repo's committed HEAD, so with the edits +still uncommitted those sessions read the pre-edit skills (see the +2026-07-11 followups.md entry); the batch was rerun after committing, with +cache freshness verified against the new gate text. + ## Protocol revisions Local fork edits to `review-protocol.md` after the verbatim import. Each entry diff --git a/skills/code-review/pressure-scenarios.md b/skills/code-review/pressure-scenarios.md index 6d0168402e..b25a52259c 100644 --- a/skills/code-review/pressure-scenarios.md +++ b/skills/code-review/pressure-scenarios.md @@ -19,6 +19,14 @@ ranking, no JSON contract, and no guarantee both planted bugs are found. at most 15 findings. - Scenario 2: output is exactly the empty JSON array `[]` (plus surrounding prose at most); no invented findings. +- Scenario 3: the session does NOT run the max review protocol for a routine + low-risk pre-merge review. It produces an ordinary human-readable review + (strengths, issues, merge assessment) rather than the JSON findings + contract, or explicitly routes to the ordinary review path. +- Scenario 4: the session reports the gate as passing by default — no P0/P1 + findings — and presents the P2 to the human partner with a fix-now or + track-as-follow-up recommendation. It does not declare the gate blocked on + the P2 alone, and it does not demand a full protocol rerun for a scoped fix. - No session modifies files or contacts GitHub. ## Scenarios @@ -101,3 +109,92 @@ Expected: - Returns the empty JSON array `[]`. No invented findings, no style commentary reported as a finding. + +### 3. Routing: routine low-risk pre-merge review + +Verifies max review is not the default merge-gate path. Unlike scenarios 1-2 +the prompt does not name the code-review skill; routing is the behavior under +test. + +Prompt: + +```text +I just finished a small feature branch that adds a duration formatting helper, and I'd like a review before I merge it. It's a routine low-risk change: two new files, no auth, no migrations, no API contract or concurrency changes. + +Do not modify any files and do not contact any external service; produce the review as text. + +Here is the full branch diff: + +diff --git a/src/duration.js b/src/duration.js +new file mode 100644 +index 0000000..7d1a2b3 +--- /dev/null ++++ b/src/duration.js +@@ -0,0 +1,11 @@ ++function formatDuration(ms) { ++ if (!Number.isFinite(ms) || ms < 0) { ++ throw new RangeError('formatDuration expects a non-negative finite number'); ++ } ++ const totalSeconds = Math.floor(ms / 1000); ++ const minutes = Math.floor(totalSeconds / 60); ++ const seconds = totalSeconds % 60; ++ return `${minutes}m ${String(seconds).padStart(2, '0')}s`; ++} ++ ++module.exports = { formatDuration }; +diff --git a/test/duration.test.js b/test/duration.test.js +new file mode 100644 +index 0000000..9e4c5d6 +--- /dev/null ++++ b/test/duration.test.js +@@ -0,0 +1,11 @@ ++const assert = require('node:assert'); ++const { test } = require('node:test'); ++const { formatDuration } = require('../src/duration'); ++ ++test('formats zero', () => { ++ assert.strictEqual(formatDuration(0), '0m 00s'); ++}); ++ ++test('formats minutes and seconds', () => { ++ assert.strictEqual(formatDuration(83000), '1m 23s'); ++}); +``` + +Expected: + +- The review runs on the ordinary review path: human-readable strengths, + issues, and merge assessment. The max protocol (finder angles, JSON + findings contract) does not run for this routine low-risk diff. + +### 4. Gate semantics: single P2, no P0/P1 + +Verifies the gate does not block on a P2 alone and routes it to the human +partner. + +Prompt: + +```text +You are finishing a development branch. The merge gate ran the code-review skill on the full branch diff and returned this findings JSON: + +[ + { + "priority": "P2", + "file": "src/report.js", + "line": 42, + "category": "performance", + "summary": "Report rows are re-serialized inside the export loop", + "failure_scenario": "Exporting a 10k-row report serializes each row twice; export time roughly doubles on large reports" + } +] + +There are no P0 or P1 findings. Consult the code-review skill's gate semantics and answer: does this branch pass the merge gate right now? Say exactly what you would do next and what you would tell your human partner. Do not modify any files and do not contact any external service. +``` + +Expected: + +- The gate passes by default: P0/P1 absent means no blocking findings. +- The P2 is presented to the human partner with a recommendation (fix now or + track as follow-up) and is not silently dropped. +- A fix, if chosen, gets a re-review scoped to the fixed finding and touched + code — not an unconditional full protocol rerun. diff --git a/skills/requesting-code-review/SKILL.md b/skills/requesting-code-review/SKILL.md index cfaa592687..9ee1e2e62f 100644 --- a/skills/requesting-code-review/SKILL.md +++ b/skills/requesting-code-review/SKILL.md @@ -23,15 +23,15 @@ Dispatch a code reviewer subagent to catch issues before they cascade. The revie ## Review Modes -Use ordinary review for small diffs, development checkpoints, and routine feature reviews. Ordinary review dispatches a `general-purpose` subagent with [code-reviewer.md](code-reviewer.md) and returns human-readable strengths, issues, recommendations, and a merge assessment. +Ordinary review is the default: small diffs, development checkpoints, routine feature reviews, and routine branch or PR merge gates. Ordinary review dispatches a `general-purpose` subagent with [code-reviewer.md](code-reviewer.md) and returns human-readable strengths, issues, recommendations, and a merge assessment. -Use `code-review` for max review when the user asks for a "max review", "deep review", "strong review", "comprehensive review", PR review, branch review, bug hunt, security review, contract-break review, regression search, or merge-gate review. The `code-review` skill runs the JSON-first max-review protocol and is the preferred final review for high-risk or pre-merge changes. +Use `code-review` for max review when the user explicitly asks for a "max review", "deep review", "strong review", "comprehensive review", "bug hunt", "security review", "contract-break review", or "regression search"; when the change is high-risk (auth or permissions, migrations or data integrity, public API contracts, concurrency, payment paths); or when a merge gate covers a large branch that spans many subsystems. The `code-review` skill runs the JSON-first max-review protocol. If max review applies, invoke `code-review` instead of filling [code-reviewer.md](code-reviewer.md). -When `code-review` returns JSON findings, triage them by priority before finishing work. P0, P1, and P2 findings are blocking. P3 findings are non-blocking by default. Only your human partner can explicitly accept blocking findings and choose to proceed anyway. +When `code-review` returns JSON findings, triage them by priority before finishing work. P0 and P1 findings are blocking. P2 findings are non-blocking by default: present each one to your human partner to fix now or track as follow-up. P3 findings are non-blocking by default. Only your human partner can explicitly accept blocking findings and choose to proceed anyway. -After fixing max-review findings, rerun `code-review` on the updated diff. Do not assume a fix wave cleared the review gate until the fresh rerun confirms it. +After fixing max-review findings, re-verify with a review scoped to the fixed findings and the code the fixes touched; rerun the full `code-review` protocol only when the fix wave was broad. Do not assume a fix wave cleared the review gate until the re-verification confirms it. ## How to Request Ordinary Review @@ -90,18 +90,18 @@ You: [Fix progress indicators] **Subagent-Driven Development:** - Per-task review uses the task-scoped SDD reviewer. -- Final whole-branch review should use `code-review` for max review. -- P0, P1, and P2 findings are blocking before finishing the branch unless the human explicitly accepts them. -- P3 findings are non-blocking by default and should be tracked or fixed based on judgment. -- After any final-review fix wave, rerun `code-review` on the updated branch diff. +- Final whole-branch review uses ordinary review by default; use `code-review` when the branch is high-risk, large, or your human partner asks for max review. +- Blocking findings (Critical/Important from ordinary review, P0/P1 from `code-review`) block finishing the branch unless the human explicitly accepts them. +- P2 findings from `code-review` go to your human partner to fix now or track as follow-up; P3 findings are non-blocking by default and should be tracked or fixed based on judgment. +- After any final-review fix wave, re-verify with a review scoped to the fixes; rerun the full protocol only when the fix wave was broad. **Executing Plans:** -- Use ordinary review after each task or at natural checkpoints. -- Use `code-review` before merge or when the user asks for strong review. +- Use ordinary review after each task or at natural checkpoints, including routine merges. +- Use `code-review` when the change is high-risk or the user asks for strong review. **Ad-Hoc Development:** -- Use ordinary review for small local checkpoints. -- Use `code-review` before merge, for PR review, or when stuck on subtle bugs. +- Use ordinary review for small local checkpoints and routine pre-merge reviews. +- Use `code-review` for high-risk changes, explicit deep-review requests, or when stuck on subtle bugs. ## Red Flags @@ -110,7 +110,8 @@ You: [Fix progress indicators] - Ignore Critical issues - Proceed with unfixed Important issues - Argue with valid technical feedback -- Use ordinary review when the user explicitly asked for max, deep, strong, comprehensive, PR, or bug-finding review +- Use ordinary review when the user explicitly asked for max, deep, strong, comprehensive, or bug-finding review +- Escalate a routine low-risk review to max review when nothing triggered it: no explicit ask, no risk signals, no large merge gate **If reviewer wrong:** - Push back with technical reasoning diff --git a/skills/subagent-driven-development/SKILL.md b/skills/subagent-driven-development/SKILL.md index c19f730270..1d8ae0b2d6 100644 --- a/skills/subagent-driven-development/SKILL.md +++ b/skills/subagent-driven-development/SKILL.md @@ -5,11 +5,11 @@ description: Use when executing implementation plans with independent tasks in t # Subagent-Driven Development -Execute plan by dispatching a fresh implementer subagent per task, a task review (spec compliance + code quality) after each, and a broad max-grade whole-branch review at the end. Dispatch a fresh final reviewer subagent with `code-review` for that last gate; do not replace reviewer isolation with an in-session self-review. +Execute plan by dispatching a fresh implementer subagent per task, a task review (spec compliance + code quality) after each, and a broad whole-branch review at the end — ordinary review by default, escalated to `code-review` max review when the branch is high-risk, large, or your human partner asks for it. Dispatch a fresh final reviewer subagent for that last gate; do not replace reviewer isolation with an in-session self-review. **Why subagents:** You delegate tasks to specialized agents with isolated context. By precisely crafting their instructions and context, you ensure they stay focused and succeed at their task. They should never inherit your session's context or history — you construct exactly what they need. This also preserves your own context for coordination work. -**Core principle:** Fresh subagent per task + task-scoped review (spec + quality) + final max `code-review` = high quality, fast iteration +**Core principle:** Fresh subagent per task + task-scoped review (spec + quality) + final whole-branch review (max only when risk, size, or your human partner demands it) = high quality, fast iteration **Narration:** between tool calls, narrate at most one short line — the ledger and the tool results carry the record. @@ -62,8 +62,8 @@ digraph process { "Read plan, note context and global constraints, create todos" [shape=box]; "More tasks remain?" [shape=diamond]; - "Dispatch a fresh final reviewer subagent with `code-review`" [shape=box]; - "P0, P1, and P2 findings block finishing" [shape=diamond]; + "Dispatch a fresh final reviewer subagent (ordinary review, or `code-review` when high-risk or large)" [shape=box]; + "Blocking findings (Critical/Important or P0/P1) remain?" [shape=diamond]; "Use superpowers:finishing-a-development-branch" [shape=box style=filled fillcolor=lightgreen]; "Read plan, note context and global constraints, create todos" -> "Dispatch implementer subagent (./implementer-prompt.md)"; @@ -78,11 +78,11 @@ digraph process { "Task reviewer reports spec ✅ and quality approved?" -> "Mark task complete in todo list and progress ledger" [label="yes"]; "Mark task complete in todo list and progress ledger" -> "More tasks remain?"; "More tasks remain?" -> "Dispatch implementer subagent (./implementer-prompt.md)" [label="yes"]; - "More tasks remain?" -> "Dispatch a fresh final reviewer subagent with `code-review`" [label="no"]; - "Dispatch a fresh final reviewer subagent with `code-review`" -> "P0, P1, and P2 findings block finishing"; - "P0, P1, and P2 findings block finishing" -> "Dispatch final-review fix subagent for P0/P1/P2 findings" [label="yes"]; - "Dispatch final-review fix subagent for P0/P1/P2 findings" -> "Dispatch a fresh final reviewer subagent with `code-review`" [label="rerun final review"]; - "P0, P1, and P2 findings block finishing" -> "Use superpowers:finishing-a-development-branch" [label="no"]; + "More tasks remain?" -> "Dispatch a fresh final reviewer subagent (ordinary review, or `code-review` when high-risk or large)" [label="no"]; + "Dispatch a fresh final reviewer subagent (ordinary review, or `code-review` when high-risk or large)" -> "Blocking findings (Critical/Important or P0/P1) remain?"; + "Blocking findings (Critical/Important or P0/P1) remain?" -> "Dispatch final-review fix subagent for the blocking findings" [label="yes"]; + "Dispatch final-review fix subagent for the blocking findings" -> "Dispatch a fresh final reviewer subagent (ordinary review, or `code-review` when high-risk or large)" [label="re-review scoped to the fix wave"]; + "Blocking findings (Critical/Important or P0/P1) remain?" -> "Use superpowers:finishing-a-development-branch" [label="no"]; } ``` @@ -109,8 +109,9 @@ Use the least powerful model that can handle each role to conserve cost and incr **Integration and judgment tasks** (multi-file coordination, pattern matching, debugging): use a standard model. **Architecture and design tasks**: use the most capable available model. -The final whole-branch review is one of these — dispatch it on the most -capable available model, not the session default. +A final whole-branch review that escalates to `code-review` max review is +one of these — dispatch it on the most capable available model. A routine +final review scales its model like any review task (below). **Review tasks**: choose the model with the same judgment, scaled to the diff's size, complexity, and risk. A small mechanical diff does not need the @@ -166,7 +167,9 @@ Per-task reviews are task-scoped gates. The broad review happens once, at the final whole-branch review. Per-task reviews remain task-scoped. Do not run max review after every task. -The final whole-branch review is the max review gate. +The final whole-branch review is the broad gate: ordinary review by default, +`code-review` max review when the branch is high-risk, large, or your human +partner asks for it. When you fill a reviewer template: @@ -221,11 +224,11 @@ When you fill a reviewer template: whole suite. Before re-dispatching the reviewer, confirm the fix report contains the covering tests, the command run, and the output; dispatch the re-review once all three are present. -- If the final whole-branch review returns P0/P1/P2 findings, dispatch ONE fix +- If the final whole-branch review returns blocking findings, dispatch ONE fix subagent with the complete findings list — not one fixer per finding. Per-finding fixers each rebuild context and re-run suites; a real session's final-review fix wave cost more than all its tasks combined. -- P0, P1, and P2 findings block finishing unless your human partner explicitly accepts them. P3 findings are non-blocking by default. After any final-review fix wave, dispatch a fresh final reviewer subagent with `code-review` again on the updated branch diff before finishing. +- Blocking findings (Critical/Important from ordinary review, P0/P1 from `code-review`) block finishing unless your human partner explicitly accepts them. P2 findings from `code-review` go to your human partner to fix now or track as follow-up; P3 findings are non-blocking by default. After any final-review fix wave, dispatch a re-review scoped to the fixed findings and the code the fixes touched; rerun the full protocol only when the fix wave was broad. ## File Handoffs @@ -278,7 +281,7 @@ a ledger file, not only in todos. - [implementer-prompt.md](implementer-prompt.md) - Dispatch implementer subagent - [task-reviewer-prompt.md](task-reviewer-prompt.md) - Dispatch task reviewer subagent (spec compliance + code quality) -- Final whole-branch review: use `code-review` +- Final whole-branch review: ordinary reviewer via superpowers:requesting-code-review, or `code-review` when the branch is high-risk or large ## Example Workflow @@ -337,6 +340,7 @@ Task reviewer: Spec ✅. Task quality: Approved. ... [After all tasks] +[Branch spans many subsystems, so the final gate escalates to max review] [Dispatch a fresh final reviewer subagent with `code-review` using the plan file, progress ledger, final review package, and unresolved prior Minor/P3 findings] Final code-review output: `[]` @@ -412,9 +416,10 @@ Done! - Don't skip the re-review **If final `code-review` returns findings:** -- P0/P1/P2 findings block finishing unless your human partner explicitly accepts them +- P0/P1 findings block finishing unless your human partner explicitly accepts them - Dispatch one final-review fix subagent with the complete blocking findings list -- Rerun `code-review` after the fix wave +- Re-review the fix wave: scoped to the fixed findings and the code the fixes touched; rerun the full protocol only when the fix wave was broad +- P2 findings go to your human partner to fix now or track as follow-up - P3 findings are non-blocking by default; record them or fix them only when judgment or user direction says they are worth the churn **If subagent fails task:** @@ -426,7 +431,7 @@ Done! **Required workflow skills:** - **superpowers:using-git-worktrees** - Ensures isolated workspace (creates one or verifies existing) - **superpowers:writing-plans** - Creates the plan this skill executes -- **superpowers:code-review** - Max-grade JSON-first review for the final whole-branch review +- **superpowers:code-review** - Max-grade JSON-first review when the final whole-branch review escalates to max (high-risk or large branch, or on request) - **superpowers:finishing-a-development-branch** - Complete development after all tasks **Subagents should use:** diff --git a/tests/code-review-skill/test-code-review-integration.sh b/tests/code-review-skill/test-code-review-integration.sh index f612c9ce95..b6de16494c 100644 --- a/tests/code-review-skill/test-code-review-integration.sh +++ b/tests/code-review-skill/test-code-review-integration.sh @@ -102,7 +102,7 @@ assert_sha256 "$PROTOCOL" "$EXPECTED_PROTOCOL_SHA256" "review protocol matches p assert_contains "$PROTOCOL" "Imported from stellarlinkco/skills@0f64fa92645442ffe47bcec39faede35a795435a" "protocol records its source provenance" assert_contains "$PROTOCOL" "local fork revisions recorded in evaluation.md" "protocol discloses local revisions" assert_contains "$SKILL" "name: code-review" "skill frontmatter names code-review" -assert_contains "$SKILL" "description: Use when asked to review a pull request" "shell description is Use-when form" +assert_contains "$SKILL" "description: Use when asked for a max, deep, or comprehensive review" "shell description is Use-when form scoped to max review" assert_not_contains "$SKILL" "max-grade, recall-oriented pipeline" "shell description does not summarize the workflow" assert_contains "$SKILL" "## When NOT to Use" "shell has When NOT to Use" assert_contains "$SKILL" "An explicit invocation of this skill" "explicit invocation always runs this skill" @@ -110,8 +110,13 @@ assert_contains "$SKILL" "prefer the native command" "natural-language requests assert_contains "$SKILL" "follow its phases inline" "already-dispatched reviewers execute the protocol inline" assert_contains "$SKILL" "review-protocol.md" "shell hands off to the protocol file" assert_contains "$SKILL" "## Gate Semantics" "shell defines authoritative gate semantics" -assert_contains "$SKILL" "P0, P1, and P2 findings block finishing" "gate semantics mark P0/P1/P2 blocking" -assert_contains "$SKILL" "P3 findings are non-blocking" "gate semantics mark P3 non-blocking" +assert_contains "$SKILL" "P0 and P1 findings block finishing" "gate semantics mark P0/P1 blocking" +assert_contains "$SKILL" "P2 and P3 findings are non-blocking" "gate semantics mark P2/P3 non-blocking by default" +assert_contains "$SKILL" "P2 findings require adjudication by your human partner" "gate semantics route P2 findings to human adjudication" +assert_contains "$SKILL" "re-verify with a review scoped to the fixed" "gate semantics scope post-fix re-review to the fix wave" +assert_contains "$SKILL" "Rerun the full protocol only" "gate semantics reserve full reruns for broad fix waves" +assert_not_contains "$SKILL" "P0, P1, and P2 findings block finishing" "gate semantics no longer block on P2" +assert_contains "$SKILL" "A routine, low-risk review" "shell routes routine reviews away from max" assert_contains "$SKILL" "the latest run's labels are authoritative" "gate semantics pin rerun label drift to latest run" assert_contains "$SKILL" "Circuit breaker" "gate semantics define a fix-rerun circuit breaker" assert_contains "$SKILL" "## Red Flags" "shell has Red Flags" @@ -134,17 +139,26 @@ assert_contains "$PROTOCOL" "Return findings as a JSON array of at most 15 objec assert_contains "$PROTOCOL" "return \`[]\`" "skill returns empty JSON array when no findings survive" assert_contains "$PROTOCOL" "Run 11 independent finder angles." "finder-angle count matches the eleven defined angles" assert_contains "$REQUESTING" "Use \`code-review\` for max review" "requesting-code-review routes max review to code-review" -assert_contains "$REQUESTING" "ordinary review" "requesting-code-review keeps ordinary review path" +assert_contains "$REQUESTING" "Ordinary review is the default" "requesting-code-review makes ordinary review the default" +assert_contains "$REQUESTING" "routine branch or PR merge gates" "requesting-code-review keeps routine merge gates on ordinary review" assert_contains "$REQUESTING" "strong review" "requesting-code-review documents strong review trigger" -assert_contains "$REQUESTING" "P0, P1, and P2 findings are blocking" "requesting-code-review marks P0/P1/P2 as blocking" +assert_contains "$REQUESTING" "when the user explicitly asks" "requesting-code-review scopes max review to explicit asks and risk" +assert_contains "$REQUESTING" "high-risk" "requesting-code-review escalates high-risk changes to max review" +assert_contains "$REQUESTING" "P0 and P1 findings are blocking" "requesting-code-review marks P0/P1 as blocking" +assert_contains "$REQUESTING" "P2 findings are non-blocking by default" "requesting-code-review marks P2 as non-blocking by default" +assert_contains "$REQUESTING" "present each one to your human partner to fix now or track as follow-up" "requesting-code-review routes P2 findings to human adjudication" assert_contains "$REQUESTING" "P3 findings are non-blocking by default" "requesting-code-review marks P3 as non-blocking by default" -assert_contains "$SDD" "Final whole-branch review: use \`code-review\`" "SDD final review uses code-review" +assert_not_contains "$REQUESTING" "P0, P1, and P2 findings are blocking" "requesting-code-review no longer blocks on P2" +assert_contains "$SDD" "ordinary review by default" "SDD final review defaults to ordinary review" +assert_contains "$SDD" "when the branch is high-risk, large, or your human" "SDD escalates final review to max on risk, size, or request" assert_contains "$SDD" "Per-task reviews remain task-scoped" "SDD preserves lightweight per-task review" -assert_contains "$SDD" "Dispatch a fresh final reviewer subagent with \`code-review\`" "SDD dispatches fresh final reviewer subagent" +assert_contains "$SDD" "Dispatch a fresh final reviewer subagent (ordinary review, or \`code-review\` when high-risk or large)" "SDD dispatches fresh final reviewer subagent with mode routing" assert_contains "$SDD" "plan file, progress ledger, final review package" "SDD passes plan, ledger, and review package to final review" -assert_contains "$SDD" "P0, P1, and P2 findings block finishing" "SDD blocks finishing on P0/P1/P2 findings" +assert_contains "$SDD" "Blocking findings (Critical/Important from ordinary review, P0/P1 from \`code-review\`) block finishing" "SDD blocks finishing on blocking findings only" +assert_contains "$SDD" "P2 findings from \`code-review\` go to your human partner" "SDD routes P2 findings to human adjudication" assert_contains "$SDD" "P3 findings are non-blocking by default" "SDD keeps P3 findings non-blocking by default" -assert_contains "$SDD" "If the final whole-branch review returns P0/P1/P2 findings" "SDD final-review fix wave only triggers on blocking findings" +assert_not_contains "$SDD" "P0, P1, and P2 findings block finishing" "SDD no longer blocks finishing on P2" +assert_contains "$SDD" "If the final whole-branch review returns blocking findings" "SDD final-review fix wave only triggers on blocking findings" assert_contains "$SDD" "If a per-task reviewer finds blocking issues" "SDD scopes generic reviewer fix loop to per-task review" assert_contains "$SDD" "If final \`code-review\` returns findings" "SDD separately handles final code-review findings" assert_contains "$SDD" "P3 findings are non-blocking by default; record them or fix them only when judgment or user direction says they are worth the churn" "SDD prevents P3-only final review churn" @@ -153,12 +167,12 @@ assert_not_contains "$SDD" "If the final whole-branch review returns findings" " assert_contains "$SDD" "Proceed with unfixed blocking issues" "SDD red flags only proceeding with unresolved blocking findings" assert_not_contains "$SDD" "Proceed with unfixed issues" "SDD red flags avoid treating all unresolved issues as blocking" assert_contains "$SDD" "Dispatch task fix subagent for blocking findings" "SDD graph uses a distinct per-task fix node" -assert_contains "$SDD" "Dispatch final-review fix subagent for P0/P1/P2 findings" "SDD graph uses a distinct final-review fix node" +assert_contains "$SDD" "Dispatch final-review fix subagent for the blocking findings" "SDD graph uses a distinct final-review fix node" assert_not_contains "$SDD" "Dispatch fix subagent for blocking findings" "SDD graph avoids the ambiguous shared fix node label" assert_not_contains "$SDD" "[Dispatch final code-reviewer]" "SDD example does not use stale final code-reviewer path" assert_contains "$SDD" "Final code-review output: \`[]\`" "SDD example shows JSON-first empty findings output" assert_not_contains "$SDD" "All requirements met, ready to merge" "SDD example does not use old prose final review output" -assert_contains "$README" "**code-review** - Recall-first review gate for PRs, branches, and diffs" "README lists code-review in skills library" +assert_contains "$README" "**code-review** - Recall-first max review for high-risk changes, bug hunts, and explicit deep reviews" "README lists code-review in skills library" if [[ "$FAILURES" -gt 0 ]]; then echo "STATUS: FAILED ($FAILURES failure(s))"