Skip to content

Fix ACP start events for polished tools#26118

Closed
godlin-gh wants to merge 1 commit into
NousResearch:mainfrom
godlin-gh:codex/fix-acp-polished-tool-start
Closed

Fix ACP start events for polished tools#26118
godlin-gh wants to merge 1 commit into
NousResearch:mainfrom
godlin-gh:codex/fix-acp-polished-tool-start

Conversation

@godlin-gh

Copy link
Copy Markdown
Contributor

Summary

Fix ACP build_tool_start() for polished tools such as browser_navigate.

The function already imports json at module scope, but it also had a local import json in the generic fallback branch. Python treats that as a local binding for the entire function, so earlier branches that call json.dumps() before the fallback can raise UnboundLocalError.

Impact

When a polished tool start event failed, ACP live clients could miss the tool_call start update and only receive the later tool_call_update. For browser_navigate, that means clients lose the polished title like navigate: https://x.com and may fall back to a generic title.

Changes

  • Remove the redundant local import json from the generic fallback.
  • Add a regression test for browser_navigate start event formatting.

Validation

  • venv/bin/python -m pytest tests/acp/test_tools.py -q

@godlin-gh godlin-gh marked this pull request as ready for review May 15, 2026 05:03
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/acp Agent Communication Protocol adapter duplicate This issue or pull request already exists labels May 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #20356 — both PRs remove the redundant function-scope import json in build_tool_start() that shadows the module-level import and causes UnboundLocalError for polished ACP tools. #20356 references the root issue #20250.

teknium1 added a commit that referenced this pull request May 17, 2026
…tors

Adds release-note attribution mappings for the contributors from group 5:
- @haran2001 (PR #27070, #27068)
- @ms-alan (PR #26443)
- @godlin-gh (PR #26118)
- @wesleysimplicio (PR #25777, ext-email form)
- @Carry00 (PR #26851)
- @alaamohanad169-ship-it (PR #26036)
- @hawknewton (PR #26294)

(YanzhongSu PR #25879 and flamiinngo PR #27231 already mapped.)
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #27382 — your commit was cherry-picked onto current main as part of a batch salvage of low-risk new-contributor PRs. Authorship preserved (fix ACP start events for polished tools). Thanks for the contribution.

@teknium1 teknium1 closed this May 17, 2026
gweeteve pushed a commit to gweeteve/hermes-agent that referenced this pull request Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants