Split plan-eng-review-arch-finding: one-way architecture forks vs two-way code-derived defect findings (the only safely-tunable class is currently unreachable)

[alexvibecommit]

Summary

plan-eng-review-arch-finding is declared door_type: 'one-way' and absorbs two behaviorally different question types. One genuinely deserves the one-way gate. The other is the single highest-value candidate for never-ask in the whole registry, and the current classification makes it permanently untunable.

Proposal: split it into plan-eng-review-arch-fork (one-way) and plan-eng-review-code-defect (two-way).

Evidence

Hand-audited from one /plan-eng-review session on a real infrastructure plan. Every question that would have fired as plan-eng-review-arch-finding sorted cleanly into two columns:

Genuinely one-way — architecture forks, data-model changes, surface-area removal

Question	Outcome
Retire 3 MCP tools + drop traces	overrode the recommendation
New ingest_sessions persistent object	took recommendation
Drop traces, ship prompt-text search instead	overrode the recommendation

Code-derived defect findings — "the code says this is broken, here is the fix"

Question	Outcome
N+1 in loadCandidateSequence → cache + batch	took recommendation
min_machines_running 0→1, swap decompress lib	took recommendation
Server-side tail buffer for dropped records	took recommendation
readable_repository_ids gate on new tables	took recommendation
(+4 more of the same shape)	took recommendation

Right column: 8 for 8. Left column: contains both of the session's overrides.

The reason the split is clean is not agreement rate, it is information asymmetry. In the right column the reviewer has read code the human has not, and the finding carries its own answer — there is no judgment left for the human to add. In the left column the human holds context the reviewer does not: product direction, what a surface is worth keeping, what is cheap to walk back.

Why this blocks the feature rather than merely limiting it

Filtering the registry for what a /plan-eng-review user could actually tune today:

• plan-eng-review-arch-finding — one-way, so never-ask is refused. Also see the separate issue where such a write silently succeeds and never fires.
• plan-eng-review-scope-reduce — two-way, but this is the override class. Tuning it is the wrong move.
• plan-eng-review-outside-voice — two-way, and it was overridden. Wrong move.
• plan-eng-review-test-gap and plan-eng-review-todo-add — two-way, high agreement, and arguably still the wrong move: the value of "should this become a TODO?" is the pause it forces, not the answer. Auto-approving TODO adds converts a backlog into a graveyard faster. Same argument, milder, for test scope: auto-approving it hides the cost from the person paying for it.

So the tunable set on the flagship review skill is either two ids that probably should not be tuned, or zero. The one class where auto-decide is unambiguously safe — a defect the reviewer found in code the human has not read, with a fix that follows from the defect — is the one the registry blocks.

Suggested shape

'plan-eng-review-arch-fork': {
  category: 'approval',
  door_type: 'one-way',
  signal_key: 'architecture-care',
  // architecture forks, data-model changes, dropping or retiring a shipped surface
},
'plan-eng-review-code-defect': {
  category: 'approval',
  door_type: 'two-way',
  signal_key: 'architecture-care',
  // defect found by reading code, where the fix follows from the defect:
  // N+1, missing index, RLS gap, resource limit, dropped-record path
},

The boundary that matters is who holds the missing context, not severity. A code defect can be serious and still be two-way, because the human adds nothing by being asked. An architecture fork can be small and still be one-way, because the human's answer is the whole point.

Two guards worth keeping if this lands:

• The keyword classifier in scripts/one-way-doors.ts already catches schema migration, breaking change, data model change, drop table. Those patterns should keep overriding plan-eng-review-code-defect when a "defect fix" turns out to require a migration, which is exactly the leak the split could otherwise open.
• never-ask on plan-eng-review-code-defect should still surface the decision in the review output, just without stopping for it. The point is removing the stop, not removing the record.

Caveat on the evidence

This is one session, hand-audited from a transcript, n=8 on the right column. It is not logged data, and per-id capture in this install has been producing unregistered singleton ids (separate issue), so nobody has clean data on this yet. I would not want the split adopted on this alone.

Filing it now because the classification is a prerequisite for collecting the data that would justify it: while everything lands on a single one-way id, no amount of accumulated evidence can distinguish the two classes.

[alexvibecommit]

Cross-refs, since these came out of the same audit and the dependency order matters for triage:

• Question Tuning preamble points at a relative scripts/question-registry.ts that does not resolve from a project cwd, so agents silently fabricate question_ids (21/21 unregistered in one session) #2489 — agents fabricate question_ids because the registry path in the preamble does not resolve. Blocks this issue in practice: while ids are heading-derived singletons, no amount of session data can distinguish the two classes this split proposes, so the split cannot be justified empirically until that lands.
• gstack-question-preference --write accepts never-ask on one-way ids: preference stores, --stats reports it configured, --check refuses it forever #2488 — --write accepts never-ask on a one-way id and it silently never fires. Directly relevant here: today the only way to express "auto-decide code-defect findings" is a never-ask on plan-eng-review-arch-finding, which writes cleanly, reports as configured, and does nothing.

Also adjacent, already open, both hit the same data path:

• question-log-hook: user answers lost on native AskUserQuestion (answers-object shape unparsed → __unknown__ + poisoned followed_recommendation) #2336 / plan-tune question-log hook records answers as __unknown__ on native AskUserQuestion; followed_recommendation always false #2206 — question-log-hook records user_choice: "__unknown__" on native AskUserQuestion and poisons followed_recommendation to false. Confirmed still live on 1.60.1.0; a fresh capture on my install recorded __unknown__ + followed_recommendation: false on a question where the recommendation was taken. Means agreement rate is currently unmeasurable from logs, which is the metric anyone would use to evaluate this split.
• question-preference-hook returns permissionDecision:"defer" — hangs every AskUserQuestion on non-interactive hosts #2464 / AskUserQuestion silently never renders in Claude Desktop: question-preference-hook returns print-mode-only permissionDecision "defer" #2404 / AskUserQuestion always fails on Claude desktop: question-preference-hook returns invalid permissionDecision 'defer' #2340 / Conductor + Claude: plan-tune PreToolUse hook breaks native AskUserQuestion (auto-installed in Conductor) #2207 — question-preference-hook returning defer.