fix(agent_loop): apply agent_before hook return value to context (closes #537) - #714
fix(agent_loop): apply agent_before hook return value to context (closes #537)#714Kailigithub wants to merge 2 commits into
Conversation
Solaris-star
left a comment
There was a problem hiding this comment.
看了 diff,修复方向是对的——hook 返回值被丢弃确实是个 bug。
两个小建议:
-
messages[0]['content']和messages[1]['content']直接用下标访问,如果 messages 结构将来变了(比如加了 system message 在 index 0 之前)会静默写错位置。用messages[0]没问题,但加个注释说明 index 0 = system、index 1 = user 会更安全。 -
elif 'user_input' in _ctx and initial_user_content is None这个条件——如果 hook 同时返回了initial_user_content和user_input,initial_user_content优先。这个优先级是有意为之的吗?如果是,值得在注释里写一句。
改动本身很小,逻辑没问题。
lsdefine#537) plugins/hooks.py::trigger() already supports returning a dict to mutate agent context, but agent_runner_loop() discarded the return value of _hook('agent_before', ...). Plugins returning updated system_prompt, user_input, or initial_user_content had their changes silently dropped. When the hook returns a dict, apply system_prompt to messages[0] and user_input (or initial_user_content when set) to messages[1]. Backward compatible: hooks returning None or non-dict values leave messages unchanged, so existing read-only hooks (logging, telemetry, langfuse) keep working without modification. Verification: /tmp/test_issue_537.py — 4 cases (system_prompt override, legacy None-return, non-dict return, user_input override). All pass on the fixed code, Test 1 fails on main (messages[0] == 'ORIGINAL prompt' without '[AUGMENTED]' suffix).
lsdefine#537 Address review comments on PR lsdefine#714 from Solaris-star (CONTRIBUTOR): 1. Document that messages[0] = system, messages[1] = user is part of the contract — future refactors that prepend a tool/system message at index 0 must update both the list literal and the hook-override indices to avoid silent writes to the wrong slot. 2. Document that 'initial_user_content' (when returned by the hook and truthy) takes precedence over 'user_input'. The plugin override wins over the caller's explicit initial_user_content — this matches the call-site precedence 'initial_user_content if initial_user_content is not None else user_input' and is the intended behavior: plugins are the last word on first-turn content. Behavior is unchanged — comments only. Verified via 6-case AST simulation that the patch preserves the original precedence semantics: A. plugin overrides both → plugin wins B. plugin overrides initial_user_content when caller's was None C. plugin overrides user_input when no initial_user_content D. plugin returns user_input but caller has initial_user_content → caller's wins E. no plugin override → caller values used F. plugin returns system_prompt only
32e4bef to
f3a06ee
Compare
|
Thanks for the review — addressed both suggestions in commit 1. Index 0 = system, Index 1 = user (forward-compat comment) Added a block comment above the # messages[0] = system, messages[1] = user; agent_before hooks may
# override either via the returned dict. Index positions are part of the
# contract — if a system message is ever prepended in index 0 (e.g. by
# a future tool/system split), update these indices accordingly.
messages = [...]So if anyone later adds an extra system/tool message at index 0, both the literal and the two override indices have to be updated together — the comment makes that coupling visible. 2. The precedence is intentional and matches the call-site default: # initial_user_content takes precedence over user_input when both
# are returned by the hook. The caller passed `initial_user_content`
# explicitly as the resolved first-turn content; if a plugin
# override disagrees, the plugin override wins (plugin is the
# last word). This matches the default precedence at call site:
# `initial_user_content if initial_user_content is not None else user_input`.In other words: plugin override is the last word on first-turn content, regardless of what the caller passed. If the plugin returns Behavior unchanged — comments only. Verified via 6-case AST simulation:
(Parent commit has no hook-return handling at all, so this preserves the v1 fix and only adds the documentation.) |
Problem
plugins/hooks.py::trigger()already supports returning a dict to mutateagent context (it rebinds
ctx = rwhen a callback returns a dict).But
agent_runner_loop()inagent_loop.pydiscarded the return valueof
_hook('agent_before', locals()). Plugins that wanted to overridesystem_prompt,user_input, orinitial_user_contenthad theirchanges silently dropped.
This is the same class of issue called out in #537.
Fix
Capture the return value of
_hook('agent_before', locals()):system_prompt/user_input/initial_user_contentoverrides to themessageslist.None(the historical contract) or a non-dictvalue, the loop proceeds with the original arguments.
initial_user_contenttakes precedence overuser_input, preservingthe existing argument-resolution order at the top of the function.
Backward compatibility
Existing read-only hooks (
plugins/langfuse_tracing.py::_on_agent_beforereads
user_input;plugins/project_mode.py::inject_project_contextmutates
ctx['messages']in place) keep working without modification:langfuse_tracing._on_agent_beforereturns nothing →_ctxisNone→
isinstance(_ctx, dict)is False → messages unchanged.project_mode.inject_project_contextmutatesctx['messages']inplace and returns nothing → same path as above.
No existing plugin is broken by this change.
Verification
/tmp/test_issue_537.py— 4 standalone tests againstagent_runner_loopusing a stubclientandBaseHandlersubclass:system_promptoverrideNone(legacy contract)user_inputoverrideThree-step dance confirms Test 1 fails on the unfixed code
(
messages[0] == 'ORIGINAL prompt') and passes with the fix(
messages[0] == 'ORIGINAL prompt [AUGMENTED]').ruff check agent_loop.py: 40 pre-existing style findings (compactimports, semicolon-separated statements — intentional per
CONTRIBUTING.md's "Compact and visually uniform"). The fix introduces
zero new ruff findings (verified by
git stash+ re-check).Scope
agent_loop.pytests/(the repo currently has no pytestsuite); verification artifact lives at
/tmp/test_issue_537.pyandis documented above for the maintainer's review.
Closes #537