Skip to content

fix(agent_loop): apply agent_before hook return value to context (closes #537) - #714

Open
Kailigithub wants to merge 2 commits into
lsdefine:mainfrom
Kailigithub:fix/issue-537-agent-before-hook-return
Open

fix(agent_loop): apply agent_before hook return value to context (closes #537)#714
Kailigithub wants to merge 2 commits into
lsdefine:mainfrom
Kailigithub:fix/issue-537-agent-before-hook-return

Conversation

@Kailigithub

Copy link
Copy Markdown
Contributor

Problem

plugins/hooks.py::trigger() already supports returning a dict to mutate
agent context (it rebinds ctx = r when a callback returns a dict).
But agent_runner_loop() in agent_loop.py discarded the return value
of _hook('agent_before', locals()). Plugins that wanted to override
system_prompt, user_input, or initial_user_content had their
changes silently dropped.

This is the same class of issue called out in #537.

Fix

Capture the return value of _hook('agent_before', locals()):

  • When a dict is returned, apply system_prompt / user_input /
    initial_user_content overrides to the messages list.
  • When the hook returns None (the historical contract) or a non-dict
    value, the loop proceeds with the original arguments.
  • initial_user_content takes precedence over user_input, preserving
    the existing argument-resolution order at the top of the function.
 turn = 0;  handler.max_turns = max_turns
-_hook('agent_before', locals())
+_ctx = _hook('agent_before', locals())
+if isinstance(_ctx, dict):
+    if 'system_prompt' in _ctx:
+        messages[0]['content'] = _ctx['system_prompt']
+    if _ctx.get('initial_user_content') is not None:
+        messages[1]['content'] = _ctx['initial_user_content']
+    elif 'user_input' in _ctx and initial_user_content is None:
+        messages[1]['content'] = _ctx['user_input']
 while turn < handler.max_turns:

Backward compatibility

Existing read-only hooks (plugins/langfuse_tracing.py::_on_agent_before
reads user_input; plugins/project_mode.py::inject_project_context
mutates ctx['messages'] in place) keep working without modification:

  • langfuse_tracing._on_agent_before returns nothing → _ctx is None
    isinstance(_ctx, dict) is False → messages unchanged.
  • project_mode.inject_project_context mutates ctx['messages'] in
    place and returns nothing → same path as above.

No existing plugin is broken by this change.

Verification

/tmp/test_issue_537.py — 4 standalone tests against
agent_runner_loop using a stub client and BaseHandler subclass:

# Scenario Result on main Result on fix
1 Hook returns dict with system_prompt override messages[0] unchanged messages[0] updated
2 Hook returns None (legacy contract) messages unchanged messages unchanged
3 Hook returns a non-dict (e.g. string) messages unchanged messages unchanged
4 Hook returns dict with user_input override messages[1] unchanged messages[1] updated

Three-step dance confirms Test 1 fails on the unfixed code
(messages[0] == 'ORIGINAL prompt') and passes with the fix
(messages[0] == 'ORIGINAL prompt [AUGMENTED]').

$ python3 -m py_compile agent_loop.py
✓ py_compile OK

$ python3 /tmp/test_issue_537.py
✓ Test 1: system_prompt override applied via return value
✓ Test 2: legacy hook returning None leaves messages unchanged
✓ Test 3: non-dict return value is ignored (backward compatible)
✓ Test 4: user_input override applied via return value
All 4 tests PASSED.

ruff check agent_loop.py: 40 pre-existing style findings (compact
imports, 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

  • 1 file changed: agent_loop.py
  • 8 insertions, 1 deletion
  • No new tests added to tests/ (the repo currently has no pytest
    suite); verification artifact lives at /tmp/test_issue_537.py and
    is documented above for the maintainer's review.

Closes #537

@Solaris-star Solaris-star left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

看了 diff,修复方向是对的——hook 返回值被丢弃确实是个 bug。

两个小建议:

  1. messages[0]['content']messages[1]['content'] 直接用下标访问,如果 messages 结构将来变了(比如加了 system message 在 index 0 之前)会静默写错位置。用 messages[0] 没问题,但加个注释说明 index 0 = system、index 1 = user 会更安全。

  2. elif 'user_input' in _ctx and initial_user_content is None 这个条件——如果 hook 同时返回了 initial_user_contentuser_inputinitial_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
@Kailigithub
Kailigithub force-pushed the fix/issue-537-agent-before-hook-return branch from 32e4bef to f3a06ee Compare August 9, 2026 19:11
@Kailigithub

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed both suggestions in commit f3a06ee on the same branch.

1. Index 0 = system, Index 1 = user (forward-compat comment)

Added a block comment above the messages literal making the index contract explicit:

# 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. initial_user_content precedence (intent documented)

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 initial_user_content (truthy), the plugin wins; if the plugin returns only user_input, the caller's initial_user_content still wins (we don't fall through to the user_input branch in that case — that's why the initial_user_content is None guard is there).

Behavior unchanged — comments only. Verified via 6-case AST simulation:

Case system_prompt user_input initial_user_content hook ctx Final msg[0] Final msg[1]
A SYS USR INIT {system_prompt:PLUG_SYS, initial_user_content:PLUG_INIT} PLUG_SYS PLUG_INIT
B SYS USR None {initial_user_content:PLUG_INIT} SYS PLUG_INIT
C SYS USR None {user_input:PLUG_USR} SYS PLUG_USR
D SYS USR INIT {user_input:PLUG_USR} SYS INIT
E SYS USR None {} SYS USR
F SYS USR INIT {system_prompt:PLUG_SYS} PLUG_SYS INIT

(Parent commit has no hook-return handling at all, so this preserves the v1 fix and only adds the documentation.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Plugin hooks cannot modify context due to ignored return value

2 participants