Skip to content

fix: format mcp-client.ts to fix validation error - #1283

Open
awhite0030 wants to merge 3 commits into
Nano-Collective:mainfrom
awhite0030:jules-1134326089806078788-8d5ad583
Open

awhite0030 wants to merge 3 commits into
Nano-Collective:mainfrom
awhite0030:jules-1134326089806078788-8d5ad583

Conversation

@awhite0030

Copy link
Copy Markdown
Contributor

Description

Brief description of what this PR does

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • 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)

Root cause: The previous commit correctly fixed the issue by introducing sanitizeArgs but the new code failed pnpm test:format check due to a long line exceeding Biome's limit.
Fix: Formatted source/mcp/mcp-client.ts using pnpm test:format --write to resolve the linting error.
Validation: Ran pnpm run build, pnpm test:format, pnpm test:lint, pnpm test:types, pnpm test:knip, and targeted pnpm test:ava source/mcp/mcp-client.spec.ts, and all checks pass successfully.
Fixes #1147

Fixes #1147

actions-user and others added 3 commits September 12, 2026 02:32
When an MCP tool schema specifies an argument type as `string`, but the provided arguments contain an object instead, Nanocoder previously only sanitized the top-level keys. Deeply nested properties and array items inside these arguments were skipped and passed to the MCP server directly as objects. This commit introduces a recursive `sanitizeArgs` function to properly ensure objects are stringified anywhere in the object tree if the schema defines them as `string`. It also adds a comprehensive regression test.
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: needs work — 1 blocking, 1 important, 3 nits

@awhite0030 — there is a blocking item below.

Replaces the one-level Object.entries coercion loop in MCPClient.callTool with a recursive sanitizeArgs helper that walks arrays (via items) and nested object properties (via properties), calling ensureString when the schema declares type: 'string'. The recursion correctly resolves issue #1147 and the new mcp-client.spec.ts test exercises the deep-nesting paths it describes. The PR also bundles five badges/*.svg files whose diff swaps the canonical Nano-Collective/nanocoder URL for awhite0030/nanocoder and the star count for 0 — these SVGs are embedded in three README files and become user-visible once merged — so this needs to be split out before it can land.

🔴 blocking · scope · badges/stars.svg:1

The PR ships five regenerated badges/*.svg files (coverage.svg, forks.svg, npm-downloads-monthly.svg, repo-size.svg, stars.svg) that have nothing to do with MCP sanitization. The diff swaps the canonical org URL https://github.com/Nano-Collective/nanocoder for https://github.com/awhite0030/nanocoder and replaces the star count with 0 — i.e. the contributor ran the badge updater against their fork and committed the output. These SVGs are referenced by README.md, README.zh-CN.md and README.zh-TW.md (e.g. ![Stars](https://github.com/Nano-Collective/nanocoder/raw/main/badges/stars.svg)), so once merged the live README will advertise the contributor's personal fork with a star count of zero. These are auto-generated by .github/workflows/update-badges.yml, should not be touched by hand, and are certainly not part of "format mcp-client.ts to fix validation error". Drop the badge files from the PR (or revert them) so the diff stays scoped to the bug fix. I'm rating this blocking rather than important because it is a user-visible regression to the README, not just churn.

🟠 important · changeset · .changeset/pr-1147-nested-args.md:1

The added changeset uses the filename pr-1147-nested-args.md, which does not match the convention used by every other changeset in .changeset/ (e.g. fix-error-resolution-context.md, mcp-disconnect-keeps-custom-tools.md — none use a pr-N-... prefix). I could not read the body of the file because it does not exist on the base checkout, so verify in the PR that it carries the "@nanocollective/nanocoder": patch front-matter the project requires. The project rubric is explicit that the package name inside a changeset must resolve against the workspace; a wrong name passes the file-presence check and breaks release-prepare on every subsequent push to main.

⚪ nit · correctness · source/mcp/mcp-client.ts:43

Minor: when schema.type === 'string' and args === null, sanitizeArgs returns null (the first guard args !== null short-circuits before ensureString runs). ensureString(null) would return '', which is presumably what the caller wants when the model emits null for a string-typed field. This isn't a regression vs. the previous code (the old loop only coerced objects), so it's borderline, but worth a thought.

⚪ nit · tests · source/mcp/mcp-client.spec.ts:1219

The new regression test relies on the same executeToolCall spy seam as the pre-existing top-level sanitization test rather than asserting on the public callTool return value. That's adequate today, but a future refactor that renames the spy seam will silently break both tests' coverage without the failure being obviously attributable. Worth a one-line comment at the top of the new test noting the seam is being reused — polish, not a blocker.

⚪ nit · design · source/mcp/mcp-client.ts:39

sanitizeArgs sits in module scope as a free function, which is fine for a stateless pure helper. Three biome-ignore lint/suspicious/noExplicitAny suppressions sit in a 30-line function — a typed JSONSchema7-shaped parameter (the schema type the MCP SDK already uses elsewhere in this file) would remove the suppressions and document the expected shape at the boundary, at the cost of one extra interface.


🔴 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 13, 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] MCPClient.callTool argument sanitisation only checks top-level keys

2 participants