fix(runtime): populate ModelID in after_llm_call hook payload#2911
Open
kimizuka wants to merge 1 commit into
Open
fix(runtime): populate ModelID in after_llm_call hook payload#2911kimizuka wants to merge 1 commit into
kimizuka wants to merge 1 commit into
Conversation
The ModelID field on hooks.Input is documented as populated for after_llm_call (pkg/hooks/types.go:177-186), but executeAfterLLMCallHooks never sets it, so hook handlers reading model_id always observed an empty string. Thread the model identifier into the dispatch so the payload matches the documented contract. The harness path already had modelID in scope; the loop path uses modelID.String() from the turn's resolved modelsdev.ID. Add a regression test that captures the after_llm_call Input via a builtin hook and asserts ModelID equals the provider's canonical id. Signed-off-by: kimizuka <f.kimizuka@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Hook handlers wired to
after_llm_callreadmodel_idand always observedthe empty string, even though
pkg/hooks/types.go:177-186documentsModelIDas populated for this event:
Anyone writing a cost/audit/telemetry sidecar that trusts the doc and keys off
model_idends up with no way to distinguish responses across models in thesame session.
Root cause
executeAfterLLMCallHooks(pkg/runtime/hooks.go:446-453) never set the fieldon the dispatched
hooks.Input:json:"model_id,omitempty"on the zero-string then elides the key from thewire payload entirely, so handlers can't even branch on "is the field present".
The sibling
executeBeforeLLMCallHooksalready populatesModelIDfrom aparameter (
pkg/runtime/loop.go:530), so this is an after-only impl gap, not aruntime-wide one.
Fix
Thread the model identifier into the dispatch so the payload matches the
documented contract. Both callsites already have the identifier in scope:
pkg/runtime/hooks.go: addmodelIDparameter toexecuteAfterLLMCallHooksand set
Input.ModelIDon the dispatch.pkg/runtime/harness.go:192: pass the existing localmodelID.pkg/runtime/loop.go:572: passmodelID.String()from the resolvedmodelsdev.ID.No public API or wire-format addition — the field has been on
hooks.Inputanddocumented all along; this PR just stops eliding it.
For harness agents,
modelIDis the harness label (e.g.claude-code) ratherthan the
<provider>/<model>canonical form the types.go doc describes. That'spre-existing: the sibling
executeBeforeLLMCallHooksin the harness path isdispatched with the same
modelID(pkg/runtime/harness.go:50). This PRpreserves the before/after symmetry and introduces no new asymmetry.
Tests
pkg/runtime/after_llm_call_test.goaddsTestAfterLLMCallHook_PopulatesModelID,which registers a builtin
after_llm_callhook against a fixed-idmockProvider,runs one turn, and asserts the captured
Input.ModelIDequals the provider'scanonical id. Verified the test fails on the unfixed tree
(
expected "test/mock-model" / actual "") and passes with the fix.Verification
go vet ./pkg/runtime/— cleangolangci-lint run ./pkg/runtime/...— 0 issuesgo test ./pkg/runtime/ ./pkg/hooks/...— all greenOut of scope
docs/configuration/hooks/index.md:258-259lists per-event payload fields butdoesn't mention
model_idfor eitherbefore_llm_call(already populated incode) or
after_llm_call(this PR). That's a pre-existing docs gap coveringboth events; a follow-up docs PR (matching the spirit of #2569) is a better
venue for catching them up together than fixing only the after-side here.