iomgaa
6bdb802f01
- Add all claude skills (brainstorming, commit, debugging, TDD, etc.) - Add claude hooks (pre-commit-guard, post-edit-quality) - Add research templates (experiment plan, research brief, etc.) - Add claude tools (arxiv/semantic_scholar/openalex fetch, wiki, exa) - Add TRM4 reference implementation as algorithm fidelity baseline - Add research-wiki content (plans, index, graph, query_pack) - Update .gitignore to exclude .graphify_version runtime state
105 lines
5.3 KiB
Markdown
105 lines
5.3 KiB
Markdown
# Code Quality Reviewer Prompt Template
|
|
|
|
Use this template when running the code quality review as a read-only Codex session (`/codex:rescue --fresh --wait`). Codex reads the diff and returns a structured critique; it does not modify code. Only run this **after** spec compliance review has approved the work.
|
|
|
|
This same template is reused for the **final whole-implementation review** at the end of the plan — just widen the scope from "this task's commits" to "every commit on this branch since plan execution started," and pass the plan itself as additional context.
|
|
|
|
---
|
|
|
|
```
|
|
Codex review (read-only) — pass this as the /codex:rescue --fresh --wait prompt body:
|
|
|
|
description: "Review code quality for Task N" # or "Final whole-implementation review"
|
|
prompt: |
|
|
You are reviewing the code quality of work that has already passed spec compliance review. The implementer was a Claude Code subagent; you are Codex, and the spec review was a prior Codex pass. You are running read-only — read the diff and report; do not edit code.
|
|
|
|
## What Was Built
|
|
|
|
[Brief recap of the task — one paragraph. For the final whole-implementation review, paste the full plan summary here instead.]
|
|
|
|
## Working Directory
|
|
|
|
[Absolute path of the worktree.]
|
|
|
|
## Commits to Review
|
|
|
|
[Space-separated list of commit SHAs. For per-task review, just this task's SHAs. For the final review, every SHA on the branch since plan execution started.]
|
|
|
|
## CRITICAL: Read the Code
|
|
|
|
Do not rely on commit messages or the implementer's notes. Read `git show <sha>` for each commit. Read the surrounding files for context. The implementer is a Claude Code subagent running on a different model from a different provider; that gives you a structural advantage over single-model review, but it does not absolve you of actually reading the diff.
|
|
|
|
## Your Job
|
|
|
|
Evaluate the implementation against these dimensions. Spec compliance is already verified — do not re-litigate it.
|
|
|
|
**Architecture and decomposition**
|
|
- Are responsibilities cleanly separated, or are concerns tangled?
|
|
- Are the new units (functions, modules, classes) sized appropriately, or are any growing into "god" structures?
|
|
- Do the public interfaces between new units make sense, or are internals leaking?
|
|
|
|
**Plan conformance**
|
|
- Does the file structure match what the plan called for?
|
|
- Do the type signatures and names introduced here match what later tasks in the plan will expect to consume?
|
|
|
|
**Code quality**
|
|
- Naming: are names clear and consistent with the rest of the codebase?
|
|
- Error handling: are failures handled, surfaced, and logged appropriately? Any swallowed exceptions or silent fallbacks?
|
|
- Magic numbers / strings: are constants extracted and named?
|
|
- Duplication: is there copy-pasted logic that should be a helper?
|
|
- Dead code: any commented-out code, unreachable branches, or unused exports?
|
|
- Readability: can a new contributor follow the flow without help?
|
|
|
|
**Logging and run records**
|
|
- If the change records runtime data, does it go through the project's structured logging / `SqliteRunStore` (persisted) path rather than raw SQL or ad-hoc file writes?
|
|
- Are persisted records carrying the expected correlation IDs (patient / session / image) so they are traceable?
|
|
|
|
**Tests**
|
|
- Are tests meaningful (testing behavior, not implementation detail)?
|
|
- Are edge cases covered, or is it all happy path?
|
|
- Are there flaky patterns (timing, ordering, hidden global state)?
|
|
- Test names: do they describe behavior, not internals?
|
|
|
|
**File growth and codebase health**
|
|
- Did any file cross a reasonable size threshold in this work? If so, is the new content cohesive with what was already there, or should it have been a new file?
|
|
- Did the implementer introduce new dependencies, new build steps, or new top-level directories? Justified?
|
|
|
|
## Calibration
|
|
|
|
Only flag issues that matter. Minor stylistic preferences, formatting nits, and "I would have named it differently" are NOT blocking. Categorize what you find:
|
|
|
|
- **Critical** — Will cause real bugs, security issues, data corruption, or break a downstream task in the plan. Must be fixed.
|
|
- **Important** — Will cause maintenance pain, confusing behavior, or fragile code. Should be fixed.
|
|
- **Minor** — Worth mentioning so the implementer can address it cheaply, but not blocking.
|
|
|
|
Approval requires zero Critical and zero Important issues. Minor issues are advisory.
|
|
|
|
## Report Format
|
|
|
|
Return your review in this structure:
|
|
|
|
```
|
|
Verdict: <APPROVED | CHANGES_REQUESTED>
|
|
|
|
Strengths:
|
|
- <bullet per genuine strength — be honest, brief, and specific. If there are no real strengths, say so.>
|
|
|
|
Issues:
|
|
Critical:
|
|
- <bullet per critical issue with file:line reference and what's wrong>
|
|
- or "none"
|
|
Important:
|
|
- <bullet per important issue with file:line reference>
|
|
- or "none"
|
|
Minor:
|
|
- <bullet per minor issue with file:line reference>
|
|
- or "none"
|
|
|
|
Assessment:
|
|
<One short paragraph: overall quality, whether this is solid for downstream tasks, anything cross-cutting the controller should know.>
|
|
```
|
|
|
|
If Verdict is `APPROVED`, the controller will mark the task complete.
|
|
If Verdict is `CHANGES_REQUESTED`, the controller will send your findings to the implementer subagent via `SendMessage` and re-run this Codex review on the result.
|
|
```
|