fix(ce-simplify-code): stop entrenching constructs that shouldn't exist

[chreho]

Summary

ce-simplify-code can converge on a local optimum at the expense of the global one. Each reviewer evaluates the diff as structured and proposes the best cleanup of that structure — deduplicate these blocks, optimize this call — without ever asking whether the structure itself is the problem. When the highest-value simplification is "this construct shouldn't exist" (it can be derived from a source of truth, or a downstream layer already provides its guarantee), pattern-level polish doesn't just miss that answer — it actively moves the code away from it: every merge and micro-optimization built around the construct adds inertia, making the better architecture more expensive to reach later.

The three reviewers each operate inside the author's design: reuse hunts importable code, quality hunts patterns, efficiency hunts wasted work. None of them asks whether a construct the diff introduces needs to exist at all. When a diff introduces a hand-maintained mirror of another artifact — the motivating case was a field allowlist copying a GraphQL schema whose serving layer already projects responses to the selection set — the pass does the opposite of simplifying: the quality reviewer merges the duplicated serializers around the list, the efficiency reviewer optimizes model_dump(include=...) against it, and the construct comes out more entrenched than before. The reviewer that looked straight at it cleared it by inferring intent ("deliberate public-API allowlist") without verifying that the downstream layer already enforced the same boundary. On the planted-issue eval below, the baseline pass produces zero necessity findings and two entrenchment fixes; the construct's drift risk (schema gains a field → API silently returns nothing for it) ships unflagged.

This PR gives the pass a way to notice the global option and surface it rather than silently optimize past it — as rubric lines in the two relevant personas plus a synthesis rule in Step 3. No new persona, no new dispatch.

What changed

• code-reuse-reviewer.md — new rubric item 5: guarantees provided by the platform/framework/downstream layer count as "existing functionality" (gateway/GraphQL projection, serializer field enumeration, framework validation). Findings whose removal preserves the external contract but changes internal-boundary shape must be reported with that tradeoff stated, not silently fixed and not silently dropped.
• code-quality-reviewer.md — rule 3 (copy-paste) now checks elimination before proposing a merge (consolidating an unnecessary thing entrenches it); new guard: do not clear a suspicious construct by inferring intent — an unverified justification is a question finding, and a hand-maintained list mirroring another artifact is a standing red flag.
• Both personas' return format — now carries question findings (no concrete fix required) and a brief note of suspicious constructs considered-and-cleared, so the orchestrator's synthesis has the data its trigger references.
• SKILL.md Step 3 — synthesis before fixing: when ≥2 findings touch one construct (including one reviewer clearing it while another cleans up around it), resolve the necessity question before applying pattern-level fixes that would entrench it. Behavior preservation is judged at the observable contract boundary, with explicit proof required when internal shape changes. Design-level "shouldn't exist" findings are reported with tradeoffs both ways, never auto-applied — they trade on judgment the user owns (payload limits, defense-in-depth, future consumers).
• SKILL.md Step 5 + docs/skills/ce-simplify-code.md — design-level findings lead the summary instead of being buried under applied polish.

Design decisions

• Bake-in, not a fourth persona. The necessity lens overlaps the reuse and quality dimensions rather than forming an independent one, and a fourth reviewer would add a dispatch to every invocation of a skill that runs inside ce-work/lfg. Closing the gap at its owning layers keeps reviewer count and dispatch cost unchanged.
• The Step 3 conversational contract is preserved. The existing "do not argue with the finding or raise questions to the user" rule is unchanged for normal findings. The design-level channel is deliberately narrow — it exists only for fixes that cannot clear the behavior-preservation bar as internal-shape changes, which today are silently dropped. Reporting them in the summary is strictly more information at zero dispatch cost.
• Consider (flagging for maintainer judgment): whether Step 3's synthesis clause should also mark applied fixes on a reported construct as contingent — the eval showed the pass may still apply a merge on a construct whose necessity it just escalated; defensible under apply-and-verify, but redone work if the user later removes the construct.

Eval evidence

Planted-issue fixture, A/B: synthetic Python/graphql-core repo ("library catalog"). One diff plants: (P1) hand-maintained schema-mirroring field tuples where the executor provably projects; (P2) copy-paste serializers; (P3) a false-positive trap — a hand-rolled JSON coercer where model_dump(mode="json") is NOT behavior-equivalent (Decimal→str, tz datetime→Z suffix). Seven Sonnet runs, identical scope prompts, only the persona/orchestration text varies. Rows 5–6 re-run the amended personas at this PR's final text (after review-round fixes); row 7 is an end-to-end integration run — both amended reviewers' verbatim outputs fed through the amended Step 3 synthesis and Step 5 summary.

Run	P1 necessity surfaced	P2 copy-paste	P3 false-positive guard
baseline reuse	no — silent	ruled out of scope	held
baseline quality	partial — drift noted "awareness only"; fix relocates the list and merges around it	merge, no elimination check	held
amended reuse	yes — names the executor's projection as the duplicated guarantee, states the internal-boundary tradeoff, frames as a design call	contingent: "raise only if P1 not adopted"	held (+ independently found the datetime Z/+00:00 divergence)
amended quality	yes — verified via server wiring and caller search before judging "not load-bearing"	elimination before merge, explicit	held
final reuse (re-run)	yes — executed graphql-core live: validation rejects internal-field queries; full-dump return leaks nothing; tradeoff stated	contingent aside, deferred to quality dimension	held (+ cleared a non-planted trap: the String scalar falls back to str(), space not T, so the hand-rolled .isoformat() is load-bearing)
final quality (re-run)	yes — same empirical proof, independent; used the new considered-and-cleared channel	elimination first; explicit warning that merging as-they-stand entrenches the allowlist	held
integration (reuse + quality → Step 3 synthesis → Step 5)	yes — clustered 3 findings incl. the clear-around signal; resolved "shouldn't exist as a hand-maintained mirror"	merge skipped as entrenchment of an unresolved construct	design-level routing held despite proven external-contract equivalence (the "necessary but not sufficient" rule); Step 5 led with the design finding, tradeoffs both ways, 0 fixes auto-applied

Necessity detection: 0/2 baseline → 2/2 amended → 2/2 at final text. False-positive guard: 7/7. No regression on legitimate findings — the amended runs each also produced a valid extra finding the baselines missed. The integration row is the sharpest evidence for the Step 3 wording: on this input the pre-review text licensed auto-applying the removal (both reviewers had proof), and the pre-PR baseline applied the entrenching merge; the final text routes the decision to the user and applies nothing. Re-runs used a fixture copy with the results file removed (answer-key contamination guard; the original four runs predated that file).

Fixture repo (files, planted diff, run matrix, and a base64 tarball of the full two-commit history): https://gist.github.com/chreho/56e057603e912253ecbec3d1ba8aba86

Validation

• npx --yes bun@1.2.20 test --timeout 30000 (2,030 passed, 0 failed, 76 files)
• npx --yes bun@1.2.20 run release:validate (metadata in sync: 0 agents, 30 skills, 0 MCP servers)
• git diff --check (clean)
• claude plugin validate on both manifests: pass (local CLI 2.1.110 predates --strict; the diff touches only skill/docs markdown, no manifests, so CI's strict pass is expected unchanged)
• Behavioral validation is the seven-run planted-issue eval above (per-persona A/B plus the end-to-end Step 3/Step 5 integration run)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4fbbe37f13

ℹ️ 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".

[chatgpt-codex-connector]

Make cleared constructs visible to synthesis

The synthesis rule depends on detecting when one reviewer clears a construct while another proposes cleanup around it, but the reviewers are only instructed to return findings or say there is nothing to flag, so this state is not present in the aggregated findings. In that scenario the orchestrator cannot form the intended cluster and may still apply the cleanup/merge that entrenches the construct; require reviewers to emit explicit clears for suspicious constructs or remove this case from the synthesis trigger.

Useful? React with 👍 / 👎.

[chreho]

Fixed in f2afac5. Both personas' return format now carries an explicit clear/question channel: findings may be question findings (no concrete fix required), and a nothing-to-flag report must briefly note suspicious constructs considered-and-cleared with the reason. The synthesis trigger's clears clause now references data the reviewers actually emit.