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`