Skip to content

fix(acp): drop shadowing import json in build_tool_start (#20250)#20356

Closed
Beandon13 wants to merge 1 commit into
NousResearch:mainfrom
Beandon13:fix/hermes-20250-unbound-json
Closed

fix(acp): drop shadowing import json in build_tool_start (#20250)#20356
Beandon13 wants to merge 1 commit into
NousResearch:mainfrom
Beandon13:fix/hermes-20250-unbound-json

Conversation

@Beandon13

@Beandon13 Beandon13 commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The generic-fallback branch of acp_adapter.tools.build_tool_start had a function-scope import json even though json is imported at module scope. Python treats it as a local binding for the entire function body, which made the earlier json.dumps call in the _POLISHED_TOOLS branch raise UnboundLocalError for any polished tool without a dedicated branch.
  • Affected tools include discord, discord_admin, kanban_*, ha_*, browser_* (the ones not given dedicated rendering branches), feishu_*, yb_*, vision_analyze, image_generate, text_to_speech, cronjob, send_message, mixture_of_agents, clarify, etc.
  • This matches the secondary error reported in [Bug]: VS Code ACP prompt can remain in-flight indefinitely after repeated compression timeout #20250: 
    UnboundLocalError: cannot access local variable 'json' where it is not associated with a value
  File ".../acp_adapter/server.py", line 622, in _replay_session_history
    if not await _send(build_tool_start(tool_call_id, tool_name, args)):
  File ".../acp_adapter/tools.py", line 1128, in build_tool_start
  • Removed the redundant import, kept a comment explaining the foot-gun, and added a parametrized regression test covering 20 affected polished tool names.

Refs #20250 (this is the secondary build_tool_start error; the primary lifecycle issue is not addressed here).

Testing

  • scripts/run_tests.sh tests/acp/test_tools.py::TestBuildToolStart -q
  • scripts/run_tests.sh tests/acp/test_tools.py -q (full file)
  • Manual repro before fix: build_tool_start("tc", "discord", {}) → UnboundLocalError. After fix: returns a ToolCallStart.

Test output

▶ running pytest with 4 workers, hermetic env, in /tmp/hermes-20250a-fix
  (TZ=UTC LANG=C.UTF-8 PYTHONHASHSEED=0; all credential env vars unset)
bringing up nodes...
bringing up nodes...

...............................                                          [100%]
31 passed in 2.48s

Full file:

........................................................................ [ 96%]
...                                                                      [100%]
75 passed in 2.70s

@Beandon13 @claude
fix(acp): drop shadowing function-scope import json in build_tool_start
830dcc5 
The generic-fallback branch of ``acp_adapter.tools.build_tool_start`` had
a function-scope ``import json`` even though ``json`` is already imported
at module scope. Python treats this as a local binding for the entire
function body, so the earlier ``json.dumps`` call inside the
``_POLISHED_TOOLS`` branch raised ``UnboundLocalError`` for any polished
tool without a dedicated branch (discord, kanban_*, ha_*, browser_*,
feishu_*, yb_*, vision_analyze, image_generate, text_to_speech, cronjob,
send_message, mixture_of_agents, etc.).

Removes the redundant import, leaves a comment explaining why, and adds a
parametrized regression test covering 20 polished tools that exercise the
generic-fallback path.

Refs NousResearch#20250 (secondary error: build_tool_start UnboundLocalError on
'json').

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Beandon13 Beandon13 mentioned this pull request May 5, 2026
Closed
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/acp Agent Communication Protocol adapter labels May 5, 2026
This was referenced May 15, 2026
Closed
Closed
Open
@teknium1

teknium1 commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

This looks implemented on current main by a later ACP fix.

Automated hermes-sweeper review evidence:

  • acp_adapter/tools.py now imports json only at module scope and build_tool_start uses that binding in both the polished-tool branch and generic fallback (acp_adapter/tools.py:5, acp_adapter/tools.py:1218, acp_adapter/tools.py:1233).
  • Commit 6622277f11ca1dee03e868b064fb1851e5598b77 removed the function-scope import json from the generic fallback under fix ACP start events for polished tools.
  • A representative polished-tool regression test exists for browser_navigate (tests/acp/test_tools.py:228).
  • I also verified representative affected tools (discord, browser_navigate, vision_analyze, image_generate) return ToolCallStart on current main instead of raising UnboundLocalError.

Thanks for the focused fix and for tying it back to #20250.

@teknium1 teknium1 closed this Jun 11, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/acp Agent Communication Protocol adapter P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Beandon13 @teknium1 @alt-glitch