Skip to content

fix: catch XML validation errors and unknown tool errors in processToolUse - #1337

Closed
awhite0030 wants to merge 5 commits into
Nano-Collective:mainfrom
awhite0030:fix-process-tool-use-error-527446006132810128
Closed

awhite0030 wants to merge 5 commits into
Nano-Collective:mainfrom
awhite0030:fix-process-tool-use-error-527446006132810128

Conversation

@awhite0030

@awhite0030 awhite0030 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

Root cause: The checks for __xml_validation_error__, uninitialized tool registry, and unknown tools in processToolUse were placed outside of the try/catch block. Consequently, when triggered, they threw uncaught exceptions instead of returning a properly formatted ToolResult error object for the LLM.

Fix: Moved the initial validation checks inside the try block in source/message-handler.ts so exceptions thrown by these checks are properly caught and formatted into a { role: 'tool', isError: true, content: 'Error: ...' } structure. Tests were updated in source/message-handler.spec.ts to expect ToolResult outputs rather than thrown exceptions for these cases. A changeset was added.

Validation:

$ pnpm run build
$ tsc && tsc-alias && cp source/commands/contributors.json dist/commands/contributors.json && chmod +x dist/cli.js

$ pnpm test:format
Checked 530 files in 538ms. No fixes applied.

$ pnpm test:lint
Checked 530 files in 352ms. No fixes applied.

$ pnpm test:types
$ tsc --noEmit

$ pnpm test:knip
$ knip

$ pnpm test:ava source/message-handler.spec.ts
  30 tests passed

Fixes #1303

Fixes #1303

Type of Change

  • Bug fix
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

actions-user and others added 5 commits September 12, 2026 02:32
…olUse

Move the checks for `__xml_validation_error__`, missing tool registry,
and unknown tools inside the try/catch block of `processToolUse`.

This prevents these validation cases from throwing uncaught errors and
ensures they are correctly caught and wrapped in a formatted ToolResult
with `isError: true` so the model can self-correct on the next turn.
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important, 1 nit

@awhite0030 — there is a blocking item below.

The fix itself is correct and minimal: moving the XML-validation-error, uninitialized-registry, and unknown-tool checks inside the try block routes their exceptions through the existing catch so the model sees an isError: true ToolResult instead of an unhandled rejection. Tests were updated to assert the new contract. The change resolves issue #1303 as described. However, the PR also commits five badges/*.svg files modified by the contributor running the badge workflow against their fork, which leaves the repo with badges pointing at awhite0030/nanocoder (e.g. stars.svg shows 0 stars from the contributor's fork). This is unrelated, would corrupt the README-rendered badges on main, and must be removed.

🔴 blocking · scope · badges/stars.svg

The diff for badges/stars.svg replaces the upstream URL Nano-Collective/nanocoder with the contributor's fork awhite0030/nanocoder and a star count of 0. These SVGs are rendered directly in README.md (and the zh-CN/zh-TW variants) via ![Stars](https://github.com/Nano-Collective/nanocoder/raw/main/badges/stars.svg) — leaving this in the PR would make the public README show the contributor's fork as the canonical repo with zero stars. The contributor ran the update-badges.yml workflow against their fork; the resulting fork-specific artifacts have no business landing on main. All five modified badges (coverage.svg, forks.svg, npm-downloads-monthly.svg, repo-size.svg, stars.svg) must be reverted before merging. The next scheduled badge run on the upstream repo will regenerate them correctly.

🟠 important · completeness · source/message-handler.spec.ts:117

Issue #1303 lists three checks that throw outside the try block: __xml_validation_error__, uninitialized toolRegistryGetter, and unknown tool. The PR updates tests for two of the three (XML validation and unknown tool). The third — the uninitialized-registry check — has no regression test; the file even carries a comment at lines 117–119 explicitly deferring it ("Testing uninitialized registry state is not feasible without module-level access"). The module-level getter can be reset to null via setToolRegistryGetter(null as any) or by clearing the variable through a delete-style helper added for the test, and the resulting Error: Tool registry not initialized ToolResult should be asserted just like the other two cases. Leaving the third arm untested means a future edit could re-introduce the same bug for that branch without the suite catching it.

⚪ nit · warranted · source/hooks/chat-handler/conversation/conversation-loop.tsx:558

Worth flagging for awareness, not blocking: in the interactive loop, filterValidToolCalls partitions __xml_validation_error__ into unknownToolCalls (see source/hooks/chat-handler/utils/tool-filters.ts:51), and the ACP path uses partitionUnknownToolCalls to do the same — so processToolUse is no longer reached for that tool name in either production flow. The defensive fix here is still correct and the existing code comment is now accurate, but the issue's "Steps to Reproduce" (induce the model to emit malformed XML under /tune tool-mode: xml) will not actually exercise this path on the current codebase. A maintainer may want to either (a) confirm the path is reachable through some caller I missed, or (b) simplify processToolUse by relying solely on the partition step. The PR is fine to merge either way; the question is whether the check belongs here at all.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:needs-work nc-review found blocking findings label Sep 16, 2026
@akramcodez akramcodez closed this Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:needs-work nc-review found blocking findings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] processToolUse throws unhandled error on __xml_validation_error__ and missing tools instead of returning error ToolResult

3 participants