Repository navigation
Conversation
|
Lol I literally just did exactly this with my local runtime version last night. Just did a doubletake reading this |
ifauzeee
left a comment
There was a problem hiding this comment.
Review: approve with one coordination note
The change is correct and I can corroborate the diagnosis from my own testing: an omitted model parameter inherits the parent session's model on Claude Code, so the "cheaper executor" silently runs at advisor cost. Moving the requirement into the execute bullet in SKILL.md (instead of depending on closing-the-loop.md being read first) is the right fix, and the vendor-neutral wording plus host-hedge are appropriate for an Agent Skills host-agnostic skill.
Coordination note — overlap with #28
This PR edits the same SKILL.md execute bullet and the same closing-the-loop.md Dispatch section as PR #28 ("clarify executor subagents inherit the parent model"), which is still open. The two changes are compatible in intent but will conflict on the shared hunks.
Suggestion: have the author of #28 close/rebase theirs once this lands, or coordinate the merge order so only one of them touches those lines. The message in #28 ("subagent inherits your model unless configured explicitly") is a subset of this PR's mandate, so this one should win.
Minor nit
SKILL.md line ~113 still reads "dispatch a cheaper executor subagent" in the first clause of the execute bullet — the same adjective the PR body identifies as the problem. Consider tightening that phrase too, since the whole point is that "cheaper" is a property you must set, not one you can promise.
The problem
SKILL.mdtells the advisor to "dispatch a cheaper executor subagent" — but that's an adjective, not a value it can pass. The only concrete default (sonnet) lives inreferences/closing-the-loop.md, a file the skill says to read "before the first dispatch."In most agent hosts, an omitted model parameter inherits the parent session's model. So an advisor dispatching from
SKILL.mdalone passes nothing, and the "cheaper executor" runs on the advisor's own model — the exact inversion of the economics the README describes (other agent → implements, tests, ships (cheap model, executes)).Verified empirically on Claude Code:
modelpassedSKILL.mdalone would produceclaude-opus-5— inherited the advisorhaikuHaiku 4.5The override mechanism works fine. The instruction to use it is what's missing.
The failure is also intermittent and invisible: whether you get the downgrade depends on whether the advisor happened to read the reference file before dispatching. Both paths narrate "dispatching a cheaper executor subagent" in the transcript. Nothing distinguishes them except the bill.
The change
skills/improve/SKILL.md:116— theexecutebullet now states the requirement where the instruction actually lives, instead of depending on a reference file having been read first.skills/improve/references/closing-the-loop.md— the bare default becomes a mandate, names the failure mode explicitly, and adds a hedge for hosts that can't select a model per subagent.Wording is deliberately vendor-neutral ("most hosts inherit the advisor's model"), since the README says the skill works in any Agent Skills host. Concrete model names stay a scoped parenthetical example, matching the existing convention at
SKILL.md:43("in Claude Code: Explore agents").SKILL.mdgains no vendor-specific model names.Docs-only: 2 files, +10 / −2. Nothing outside the
executedispatch path changes.Not included
The audit subagents in Phase 2 (
SKILL.md:43–51) have the same underlying defect — six mandatory items listed for every subagent prompt, none of them a model, anddeepfans out up to 8 concurrent agents that all inherit the advisor's model. Left out to keep this PR single-purpose. Happy to follow up if you'd like it fixed the same way.