From d221a06522296090efa1bc1907fa73f67bea2992 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 25 Jul 2026 11:02:42 +0200 Subject: [PATCH] chore(583): make per-agent model routing a hard constraint + PreToolUse gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kickoff tells the orchestrator to route agents by capability, but that rule lived only in a prose paragraph. On 2026-07-25 a session dispatched two implementers (#436, #440) with `model` omitted, silently inheriting the orchestrator tier — while following every bullet in HARD CONSTRAINTS in the same session. The bulleted list is what functions as the checklist; prose above it reads as background. - HARD CONSTRAINTS: keyed routing bullet requiring the tier to be stated in the dispatch itself, so an invisible omission becomes visible output. Prose paragraph tightened to point at the key rather than restate the table (the kickoff is pasted into every session — duplication is a per-session tax, #542). - .claude/hooks/pretooluse-agent-model.sh: PreToolUse on Agent, `ask` when a committing agent is dispatched with no explicit `model`. Narrow by design — passes through read-only/recon types, `fork` (model override ignored by the tool), and any dispatch already naming a tier, because a gate that fires on every fan-out trains one-shot dismissal. `ask` not `deny`: routing is a judgment call with no derivable right answer, unlike the H6/H10 merge gate. - New decision record `process.per-agent-model-routing`; catalog regenerated. Decision matrix verified against 9 payloads incl. a replay of the dispatch that missed. decisions-validate: OK. fixes #583 --- .claude/hooks/pretooluse-agent-model.sh | 72 +++++++++++++++++++++++++ .claude/settings.json | 5 ++ docs/decisions/README.md | 1 + docs/decisions/workflow-process.md | 49 +++++++++++++++++ docs/handoffs/chicorytv-issue-queue.md | 16 ++++-- 5 files changed, 138 insertions(+), 5 deletions(-) create mode 100755 .claude/hooks/pretooluse-agent-model.sh diff --git a/.claude/hooks/pretooluse-agent-model.sh b/.claude/hooks/pretooluse-agent-model.sh new file mode 100755 index 000000000..19c2da898 --- /dev/null +++ b/.claude/hooks/pretooluse-agent-model.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# PreToolUse / Agent — ask when a COMMITTING agent is dispatched without an explicit `model`. +# +# The kickoff prompt (docs/handoffs/chicorytv-issue-queue.md) says to route by capability: cheap/fast +# for bounded recon, mid tier for a mechanical slice against a documented contract, orchestrator tier +# for judgment-heavy work. That rule lived only in prose, and on 2026-07-25 an orchestrator dispatched +# two implementers with `model` omitted — both silently inherited the Opus orchestrator tier. Nothing +# in the session report revealed it; the operator had to ask. +# +# WHY a hook: omitting `model` is the SILENT path. Every other constraint in that kickoff has a hook, +# a CI job or a script behind it, and those were all followed in the same session — the one rule with +# no forcing function was the one that got defaulted. A check that runs beats a rule you must remember +# (the same reasoning as pretooluse-bom-guard.sh). +# +# Deliberately NARROW, because a hook that fires on every fan-out trains one-shot dismissal: +# - read-only / recon agent types -> pass through (cheap by nature, routing barely matters) +# - subagent_type "fork" -> pass through (a fork ALWAYS inherits the parent model; the +# `model` field is ignored by the tool, so asking is nonsense) +# - an explicit `model` -> pass through (the choice was made — that is the whole point) +# - a committing/implementer agent with NO model -> ask (surface the choice to a human) +# +# "ask", never "deny": routing is a judgment call with no derivable right answer, unlike the +# merge-consent gate (H6/H10) which derives a verifiable state. This gate exists to make an invisible +# default visible, not to impose a tier. +# +# Fail-open by design: any parse trouble -> allow (exit 0, no output). +set -uo pipefail + +input=$(cat) + +tool=$(printf '%s' "$input" | jq -r '.tool_name // ""' 2>/dev/null || true) +[ "$tool" = "Agent" ] || exit 0 + +# An explicit choice was made — nothing to surface. +model=$(printf '%s' "$input" | jq -r '.tool_input.model // ""' 2>/dev/null || true) +[ -z "$model" ] || exit 0 + +subagent=$(printf '%s' "$input" | jq -r '.tool_input.subagent_type // ""' 2>/dev/null || true) + +# Types that cannot commit, plus fork (whose model override is ignored by the tool). +case "$subagent" in + fork|Explore|Plan|claude-code-guide|statusline-setup) exit 0 ;; + feature-dev:code-explorer|feature-dev:code-reviewer|feature-dev:code-architect) exit 0 ;; +esac + +# Does this brief actually put an agent on the keyboard? Recon dispatched to general-purpose is +# cheap and frequent; an implementer is the case worth one prompt. +prompt=$(printf '%s' "$input" | jq -r '.tool_input.prompt // ""' 2>/dev/null || true) +[ -n "$prompt" ] || exit 0 +printf '%s' "$prompt" \ + | grep -qiE 'git commit|git push|committing agent|worktree|fixes #|implement( the| this|ing)?\b' \ + || exit 0 + +label="${subagent:-general-purpose}" +reason="Dispatching a committing agent (subagent_type: ${label}) with no explicit \`model\`. + +It will silently inherit this session's model — which may be right, but it is a default, not a choice. +Name the tier (and say so in the dispatch message), per the kickoff routing rule +\`process.per-agent-model-routing\`: + + - bounded recon / inventory / log triage -> cheapest fast tier (haiku), effort low + - mechanical slice against a documented contract -> mid tier (sonnet) + - judgment-heavy: design, compiler/parser, security, + migrations, review arbitration -> orchestrator tier (opus) + +Independent review should also prefer a DIFFERENT model family than the implementer — a cold +same-family review is worth less than a cross-family one. + +Approve as-is only if inheriting the orchestrator tier is the deliberate call." + +jq -n --arg r "$reason" '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"ask",permissionDecisionReason:$r}}' +exit 0 diff --git a/.claude/settings.json b/.claude/settings.json index 8cc4caaa1..cc1cb7769 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -39,6 +39,11 @@ "type": "command", "command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/pretooluse-agent-ram.sh\"", "timeout": 10 + }, + { + "type": "command", + "command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/pretooluse-agent-model.sh\"", + "timeout": 10 } ] }, diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f85cb29f2..f59a46bca 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -93,6 +93,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `process.lock-ownership-enumerate-producers` | Before trusting any "single owner / no double release / no cross-release" claim, grep the whole host project for every writer of that channel message (or acquirer of that lock) — the background scheduler/worker is the usual missing producer. | 2026-07-21 | [link](workflow-process.md#2026-07-21--a-lockchannel-no-cross-release-verdict-must-enumerate-every-producer-via-grep-542) | | `process.one-worktree-one-committing-agent` | Never run two committing agents concurrently on one worktree — give each parallel slice its own worktree branched off the feature branch and merge back. | 2026-07-21 | [link](workflow-process.md#2026-07-21--one-worktree-one-committing-agent-542) | | `process.parallel-session-claim` | Apply the `in-progress` label before starting an issue, and still read its dependency notes before touching shared surfaces — a claim prevents duplicate pickup, not overlapping code changes. | 2026-07-21 | [link](workflow-process.md#2026-07-21--claim-with-in-progress-before-working-claiming-is-not-collision-safety-542) | +| `process.per-agent-model-routing` | State the model tier (and effort, where the client exposes it) in the dispatch itself for every delegated agent — bounded recon → cheapest fast tier at `low`; mechanical slice against a documented contract → mid tier; judgment-heavy work → orchestrator tier; independent review → a different model family than the implementer. | 2026-07-25 | [link](workflow-process.md#2026-07-25--name-the-model-tier-for-every-dispatched-agent-a-pretooluse-gate-makes-the-silent-default-visible-583) | | `process.pr-routine-sequence` | Worktree off origin/main → implement → regenerate API artifacts → full local tests + cold review + live-E2E ALL before the push → push, open PR, arm the CI monitor at open → fixes after the push are follow-up commits, never amend/force-push. | 2026-07-21 | [link](workflow-process.md#2026-07-21--the-pr-routine-is-a-fixed-sequence-validate-locally-then-push-then-only-follow-up-commits-542) | | `process.review-disagreement-frontier-judge` | When independent reviews disagree on a gate PR, escalate to the frontier judge, and put the proposed fix approach in front of it — not just the disputed finding. | 2026-07-21 | [link](workflow-process.md#2026-07-21--review-disagreement-on-a-gate-pr-escalates-to-the-frontier-judge--and-the-proposed-fix-escalates-with-it-542) | | `process.shared-tree-readonly` | Never commit in `/Users/timothy/ersatztv` and never read its `git log`/`git status`/HEAD to infer anything about `main` — work in a worktree off `origin/main`, which is the only source of truth. | 2026-07-21 | [link](workflow-process.md#2026-07-21--the-shared-tree-at-userstimothyersatztv-is-read-only-and-tells-you-nothing-about-main-542) | diff --git a/docs/decisions/workflow-process.md b/docs/decisions/workflow-process.md index b8b7f43c3..ddf4f14cb 100644 --- a/docs/decisions/workflow-process.md +++ b/docs/decisions/workflow-process.md @@ -524,3 +524,52 @@ The dependencies API is unaffected: `POST /issues/{n}/dependencies` with `{"owne blocked-by correctly, though the bare `{index}` form returns 201 without reliably attaching — verify with the GET. (That last sentence is carried forward from an earlier revision of the handoff doc, commit `f93458c7`, where it was dropped by a later prune rather than disproved.) + +## 2026-07-25 — Name the model tier for every dispatched agent; a PreToolUse gate makes the silent default visible (#583) + +`key: process.per-agent-model-routing` · `status: active` · `since: 2026-07-25` · `supersedes: none` · `superseded-by: none` +**Rule:** State the model tier (and effort, where the client exposes it) in the dispatch itself for every delegated agent — bounded recon → cheapest fast tier at `low`; mechanical slice against a documented contract → mid tier; judgment-heavy work → orchestrator tier; independent review → a different model family than the implementer. +**Signals:** subagent model routing · `model` omitted · silent tier inheritance · orchestrator tier for a mechanical slice · capability routing · prose rule vs HARD CONSTRAINT · dispatch-time checkpoint · paths: `.claude/hooks/pretooluse-agent-model.sh`, `docs/handoffs/chicorytv-issue-queue.md` · issues: #583, #436, #440 +**Mechanics:** `Agent` tool `model` parameter; PreToolUse hook on the existing `Agent|Task` matcher in `.claude/settings.json`. + +On 2026-07-25 a session dispatched two implementers (#436, #440) with `model` omitted on both calls; +both silently inherited the Opus orchestrator tier. #440 was a mechanical SPA slice against an +already-shipped backend contract — a plausible mid-tier candidate. + +The interesting part is *why*, because it wasn't forgetfulness. **Every rule in `HARD CONSTRAINTS` was +followed in that same session** — worktree off `origin/main`, one committing agent per worktree, +parallelize on disjoint slices, local gate before push. Routing was the one instruction living only in +a prose paragraph, and it was the one that got defaulted. Treat that as the general lesson: in a +kickoff doc that is pasted into every session, **the bulleted imperative list is what actually +functions as the checklist**, and prose above it is read as background. A rule you want followed +belongs in the list, keyed, or it is advisory in practice. + +Three aggravating factors, all worth checking when writing any future rule here: + +- **Scope gap.** The low-cost-routing paragraph is written entirely about queue selection and recon + ("bounded searches, inventories, log triage, report drafting"). It never named *implementers*, and + gave no default for the bounded-but-not-trivial case — so the largest-cost dispatch fell in a gap. +- **The wrong default is the silent one.** Omitting `model` produces no artifact. Nothing in the + session report revealed the tier; the operator had to ask. Contrast the BOM trap + (`process.bom-format-detection-recipe`), where a hook fires because a memory describing the trap + demonstrably failed to prevent it twice in one day. +- **Distance from the decision point.** The rule sits ~line 84 of the kickoff; dispatch happens after + orientation, claiming, the bundle scan and doc reading. + +Hence the two-part fix: a keyed HARD CONSTRAINT that requires the tier to be **stated out loud in the +dispatch** (a self-correcting mechanism — it turns an invisible omission into visible output), plus +`.claude/hooks/pretooluse-agent-model.sh`, which `ask`s when a **committing** agent is dispatched with +no explicit `model`. + +The hook is deliberately **narrow**, and the scoping is the load-bearing design choice: it passes +through read-only/recon agent types, `fork` (whose `model` override is ignored by the tool by design, +so prompting would be nonsense), and any dispatch that already names a tier. A gate that fires on +every fan-out trains one-shot dismissal and becomes noise — which would leave it worse than no gate. +It is `ask`, never `deny`: routing is a judgment call with no derivable right answer, unlike the +H6/H10 merge gate (`release.merge-consent-autogrant`) which derives a verifiable state and can +therefore grant or refuse outright. + +**Known limitation:** review agents (`feature-dev:code-reviewer`) pass through, since they cannot +commit — so the "prefer a different model family for independent review" half of the rule remains +doc-only, unenforced. That half is the higher-value routing decision of the two; if cross-family +review drift shows up again, gate it next. diff --git a/docs/handoffs/chicorytv-issue-queue.md b/docs/handoffs/chicorytv-issue-queue.md index 912347cf6..e935249ef 100644 --- a/docs/handoffs/chicorytv-issue-queue.md +++ b/docs/handoffs/chicorytv-issue-queue.md @@ -81,11 +81,12 @@ Orchestration means: decompose, delegate independent slices, integrate their res whole, and keep canonical issue state accurate. Use the client's native agent/subagent tools; never assume a named tool, command, plugin, model-routing feature, or fork mechanism exists. -Route by capability when the client supports per-agent model selection: fast/small for bounded recon, -balanced for mechanical changes, and the strongest coding/agentic tier for judgment-heavy work. -Otherwise use the active model for every slice except the mandatory queue preflight below. Independent -review MUST start from a cold, review-only brief; prefer a different model family/client when one is -available, otherwise use a fresh agent with no implementation role. +Route by capability when the client supports per-agent model selection, and **say which tier you chose +in the dispatch itself** — see the `process.per-agent-model-routing` HARD CONSTRAINT below for the +table. Where the client cannot route per agent, use the active model for every slice except the +mandatory queue preflight below. Independent review MUST start from a cold, review-only brief; prefer +a different model family/client when one is available, otherwise use a fresh agent with no +implementation role. **Low-cost routing applies throughout the session, not only to queue selection.** Before any batch of bounded searches, inventories, log triage, URL/status sampling, or report drafting, dispatch the cheapest @@ -273,6 +274,11 @@ HARD CONSTRAINTS: plumbing merge. → `process.foreign-worktree-plumbing-merge` - **Parallelize by default** on genuinely disjoint slices (normally 2–3 delegated agents). Cap builds at 3–4 and gate on FREE RAM, not CPU. **Never set `ETV_UPDATE_GOLDENS`.** → `process.build-concurrency-limits` +- **Name the model + effort for every dispatched agent, in the dispatch itself** — bounded recon → cheapest + fast tier at `low`; mechanical slice against a documented contract → mid tier; judgment-heavy (design, + compiler/parser, security, migrations, review arbitration) → orchestrator tier; independent review → + a different model family than the implementer. Omitting it silently inherits the orchestrator tier, so + state the choice out loud. → `process.per-agent-model-routing` - **Local gate + cold-context review BEFORE the push**, never after. → `process.local-gate-before-push` - **Independent review is mandatory** for locks/concurrency, auth/security, API write-path handlers, DB migrations, or >~150 changed C# lines; a skip must be stated with its reason. → `process.independent-review-rubric`