Repository navigation
feat(ce-code-review): add simplification reviewer persona - #1851
duketopceo wants to merge 1 commit into
Conversation
Adds a delete-list review lens covering ground the existing roster leaves open: unrequested surface, dependency minimalism, lean misses (dead code, reinvention of existing or stdlib capability, single-user abstractions), AI-slop artifacts, feature-flag branching debt, and avoidable orchestration. Boundary with maintainability is declared in the persona and the catalog: maintainability owns the structural health of what the diff writes; simplification owns whether the diff should be that big. Wiring: - references/personas/simplification-reviewer.md (new) - persona-catalog.md generic conditional row - select-and-route.md roster entry - finish-review.md soft-bucket demotion routing + Removable surface coverage now spans simplification/maintainability deletion findings - review-output-template.md matching coverage wording - review-skill-contract.test.ts persona lists + demotion-route pin Co-Authored-By: Paperclip <noreply@paperclip.ing>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36f841a08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ## Severity guidance | ||
|
|
||
| - **P1** — a delete-list exists that removes significant incidental complexity: unrequested public surface, a dependency for a few lines, a flag branching across the diff with no removal path, or speculative plumbing adding a whole layer. A plausible reframe that deletes whole branches or helpers is presumptive P1 — the codebase is worse for carrying the larger version. |
There was a problem hiding this comment.
When this reviewer finds a dependency used for a few lines or a behavior-preserving speculative layer, this rule labels it P1 solely because the deletion is sizeable. references/action-class-rubric.md:30-39 reserves P1 for a high-impact normal-use defect or broken contract and explicitly places a maintainability trap at P2; references/finish-review.md:37,146 then keeps every P1 primary and prevents a Ready to merge verdict. Condition P1 on a concrete P1-level consequence, and leave code-health deletions at P2/P3.
AGENTS.md reference: AGENTS.md:L114-L114
Useful? React with 👍 / 👎.
|
|
||
| ### Unrequested surface (highest priority) | ||
|
|
||
| - **Features, endpoints, config keys, flags, parameters, or exports the change's intent never called for.** Compare against the intent summary and any plan; surface beyond it is presumptive excess. New public surface is the most expensive kind — it accrues callers, tests, docs, and security review. |
There was a problem hiding this comment.
For a review whose plan came from plan: or auto-discovery rather than the PR body, this highest-priority check cannot inspect the evidence it names: the Stage 4 child receives PR metadata, the 2-3-line intent summary, files, and diff (references/subagent-template.md:175-190), but no discovered plan.path or requirements. Absence from that short summary is not proof that a feature or endpoint was unrequested, while references/intent-and-plan.md:12 already owns plan-reverse behavior checks as P3 advisory/human decisions. Pass the plan evidence with matching routing, or leave public-behavior scope at the owning Stage 6 check.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
|
|
||
| - `testing-reviewer` — test files, test infrastructure, mocks, fixtures, or harness behavior changed; or the diff changes meaningful runtime behavior without corresponding test work. Behavioral triggers include new or changed branches, state mutation, API/control-flow behavior, and error handling. Production-file presence alone and non-behavioral edits do not select it. | ||
| - `maintainability-reviewer` — a large or structural diff: substantial refactor, new abstractions, file moves, coupling/type-boundary changes, or at least 200 executable changed lines. | ||
| - `simplification-reviewer` — the diff adds production code surface that could be smaller or absent: new functions, types, modules, dependencies, config keys, endpoints, flags, or scaffolding, or the added code is materially larger than the intent requires. Owns the delete-list lens (unrequested surface, dependency minimalism, lean misses, AI-slop, flag branching debt, avoidable orchestration); structural health of what was written stays with `maintainability-reviewer`. Skip for pure deletions and docs/test/generated-only diffs. |
There was a problem hiding this comment.
On the full Stage 3 path, this list and the catalog add the persona, but the operational selection paragraph at select-and-route.md:45 still enumerates only testing, maintainability, agent-native, and learnings, and the public roster at docs/guides/ce-code-review.md:90 repeats that old set. A run following the later directive can omit the new reviewer, while the guide makes that omission look intentional. Make Stage 3 defer to the catalog (rather than maintaining another roster), and update the guide and its generic-reviewer contract pin.
AGENTS.md reference: AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
|
|
||
| You are the reviewer who asks whether this diff should exist at its current size. Other personas judge whether the code is correct and structurally sound; you judge whether the code should be there at all. Review like the senior developer who will be paged for this change at 3am: every extra line is something to read, test, debug, and someday delete. The best findings are delete-lists — cases where whole branches, helpers, flags, dependencies, or layers disappear while behavior stays the same. Do not settle for "this could be tidier"; hunt for the version of this change that is dramatically smaller. | ||
|
|
||
| **Boundary.** `maintainability` owns the structural health of what the diff writes — coupling, layering, file size, type boundaries, data-locality smells. You own the excess the diff introduces — code the change never needed. When a hunk is both, flag it once under the framing that carries the fix (usually yours, since the fix is deletion), and name the other lens in `why_it_matters` only if it adds a consequence the deletion framing misses. |
There was a problem hiding this comment.
When both conditionals run on a large structural addition, this one-sided boundary cannot ensure that a hunk is flagged once: maintainability-reviewer.md:11-25,40 still explicitly hunts branch/flag/wrapper deletion, duplicate helpers, speculative abstractions, dead code, and type holes, all of which this new persona also owns. Reviewers are independent (dispatch-reviewers.md:51), so the maintainability agent never sees this boundary and differently framed fixes need not deduplicate. Move each overlapping check to one prompt, or narrow the claimed boundary in both personas.
AGENTS.md reference: AGENTS.md:L115-L115
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Closes #1850.
Summary
Adds a
simplificationreviewer persona toce-code-review— the delete-list lens: should this diff be this big?references/personas/simplification-reviewer.md: unrequested surface, dependency minimalism, lean misses (delete / reuse / stdlib / yagni / merge / split-by-job), AI-slop artifacts, feature-flag branching debt, avoidable orchestration. Findings must name what disappears insuggested_fix.maintainability(persona + catalog): maintainability owns structural health of what the diff writes; simplification owns excess the diff introduces. A hunk flagged once, under the framing carrying the fix.finish-review.mddemotion routing +review-output-template.md— Removable surface coverage now spans deletion-orientedsimplificationormaintainabilityfindings.simplification-revieweradded to both persona lists; demotion-route pin updated to match.The persona carries the full contract sections (anchored confidence calibration through
Anchor 25 or below — suppress,What you don't flag, JSON output block).Validation
bun run test:skill-guards— 797 pass, 0 fail.Security Disclosure
No security-relevant changes.
Agent Disclosure
Devin CLI · SWE-2 Max