Skip to content

fix(nanogpt): preserve optional tool parameters - #1590

Open
huggix wants to merge 1 commit into
Zoo-Code-Org:mainfrom
huggix:codex/nanogpt-preserve-tool-optionality
Open

fix(nanogpt): preserve optional tool parameters#1590
huggix wants to merge 1 commit into
Zoo-Code-Org:mainfrom
huggix:codex/nanogpt-preserve-tool-optionality

Conversation

@huggix

@huggix huggix commented Sep 10, 2026

Copy link
Copy Markdown

Related GitHub Issue

Issue approval and assignment are pending. Opening this as a draft for maintainer feedback, not as ready for review.

Description

The NanoGPT handler uses the shared OpenAI strict-schema converter, which makes every tool property required and removes nullability.

For read_file, this changes the required fields from just path to path, mode, offset, limit, and indentation, including every nested indentation option. Valid slice reads can then fail because they omit indentation settings they do not need.

This change preserves the original parameter schemas and sets function tools to strict: false. Genuine required fields, types, bounds, nullability, and additional-property constraints remain unchanged. Caller-owned definitions are not mutated, and non-function tools pass through unchanged.

The change is limited to NanoGPT. It does not modify the shared converter, repair arguments, bypass validation, or change routing or reasoning controls. This supports the project's reliability and API compatibility goals.

Tradeoff: this stops requesting strict constrained generation. It preserves the tool's actual contract but does not guarantee that every model response will satisfy it.

Test Procedure

From the repository root:

pnpm --dir src exec vitest run api/providers/__tests__/nanogpt.spec.ts api/providers/__tests__/base-provider.spec.ts core/prompts/tools/native-tools/__tests__/read_file.spec.ts core/tools/__tests__/ReadFileTool.spec.ts
pnpm --dir src exec tsc --noEmit
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 api/providers/nanogpt.ts api/providers/__tests__/nanogpt.spec.ts
git diff --check
  • 148 tests passed across the NanoGPT handler, shared converter, native read_file schema, and ReadFileTool suites.
  • Seven added regression cases cover Auto/Tools routing, native/custom/MCP schemas, nested optional and nullable fields, genuine required fields, no mutation, absent/empty catalogs, and non-function tools. Three new cases failed before the patch.
  • TypeScript, scoped ESLint, formatting, and diff checks passed.
  • Live tests reproduced the missing-indentation failure with the old schema.
  • With the patch, seven conversations completed and two returned HTTP 429 overload responses. No patched schema failures occurred. All four combinations completed: Auto/Tools routing and slice/indentation reads.
  • Old-schema controls produced two completions, three malformed-tool stream errors, one nested-required validation failure, and two overloads. Failed attempts were retained rather than counted as passes.

The separate local live harness used the actual handler, streaming parser, 13 native tool definitions, real fixture-file execution, and tool-result continuation with alibaba/qwen3.8-flash. Completion required returning values obtained from both files. Requests used tool_choice=auto, a 4096-token cap, and no sampling/reasoning overrides or SDK retries. No missing arguments were invented. The live harness and private request artifacts are not included in this two-file diff.

These were synthetic conversations, not a replay of the customer's full history or a complete VS Code UI test. The small sample does not establish a general failure rate or fix overload errors. Local tests used Node 26.0.0 and pnpm 10.8.1; CI on the pinned Node 22.23.1 remains necessary. Dependencies were installed from the frozen lockfile with install scripts disabled; no full extension build or complete workspace test run was performed.

Pre-Submission Checklist

  • Issue approval and assignment completed.
  • Focused change limited to NanoGPT request construction.
  • Regression tests added and local checks passed.
  • Documentation impact considered; no user-facing documentation change required.
  • Human author verification and maintainer review completed before ready-for-review status.
  • Required CI passes on the pinned toolchain.

Additional Notes

No UI changes. AI assistance was used for investigation, implementation, tests, and this description. This draft contains no private support identifiers, customer arguments, credentials, or internal routing details.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e7146f3d-ed5d-45cf-8bcb-85ca16e8b145

📥 Commits

Reviewing files that changed from the base of the PR and between e5248e5 and 5a5aa6d.

📒 Files selected for processing (2)
  • src/api/providers/__tests__/nanogpt.spec.ts
  • src/api/providers/nanogpt.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (5)
Treat model, provider, MCP, path, command, and tool data as untrusted.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/nanogpt.ts
  • src/api/providers/__tests__/nanogpt.spec.ts
Require regression coverage at the lowest valid harness with behavior-focused assertions, including relevant negative, error, false/unset, and boundary cases.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/__tests__/nanogpt.spec.ts
Check strict typing and exhaustive behavior across normal, boundary, error, cancellation, retry, and compatibility paths.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/nanogpt.ts
  • src/api/providers/__tests__/nanogpt.spec.ts
Verify extension/webview contracts, cancellation and error propagation, VS Code lifecycle correctness, and behavior under retries and partial failure.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/nanogpt.ts
  • src/api/providers/__tests__/nanogpt.spec.ts
Act as an adversarial second-opinion reviewer.

⚙️ CodeRabbit configuration file

Files:

  • src/api/providers/nanogpt.ts
  • src/api/providers/__tests__/nanogpt.spec.ts
🔇 Additional comments (2)
src/api/providers/nanogpt.ts (1)

135-140: LGTM!

src/api/providers/__tests__/nanogpt.spec.ts (1)

12-12: LGTM!

Also applies to: 175-194, 196-245, 247-255, 257-268


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved NanoGPT tool handling so optional, nested, nullable, and required parameter definitions are preserved correctly.
    • Prevented tool schemas from being unintentionally made stricter, allowing optional fields to remain optional.
    • Improved compatibility with empty or missing tool lists and non-function tool types.
    • Tool definitions are now forwarded without altering the original input objects.

Walkthrough

NanoGPT now forwards tool schemas without OpenAI conversion. Function tools receive strict: false, while non-function tools and schema constraints remain unchanged. Tests cover routing, catalog states, nested parameters, and input immutability.

Changes

NanoGPT tool forwarding

Layer / File(s) Summary
Direct tool schema forwarding
src/api/providers/nanogpt.ts
createMessage maps metadata.tools directly. Function tools receive strict: false, and their declared schemas remain unchanged.
Tool forwarding validation
src/api/providers/__tests__/nanogpt.spec.ts
Tests verify routing, schema preservation, input immutability, empty catalogs, and unchanged non-function tools.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 5a5aa

NanoGPT now preserves optional tool parameters without changing shared conversion or routing behavior. Focused tests cover the affected paths, so the change is mergeable pending normal CI and maintainer checks.

🚥 Pre-merge checks | ✅ 8
✅ Passed checks (8 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Regression Evidence ✅ Passed The changed NanoGPT request construction has focused handler-level coverage. Tests cover function tools with native, nested, nullable, and required schema fields; strict:false; caller-object immutabil…
Security Boundaries ✅ Passed No changed security-boundary failure is present. src/api/providers/nanogpt.ts:138-140 only maps outbound tool definitions and sets function strict: false; it does not execute tool arguments, expos…
Persistence Integrity ✅ Passed PASS. The authoritative diff changes only NanoGPT request construction and related tests. The changed path maps metadata.tools into the outbound body.tools, shallow-copies function tools, and sets…
Lifecycle Resource Cleanup ✅ Passed PASS. The only production change is in NanoGptHandler.createMessage: it synchronously maps metadata.tools and sets function.strict to false. This path does not create or retain listeners, watc…
Title check ✅ Passed The title clearly identifies the NanoGPT fix and its primary purpose: preserving optional tool parameters.
Description check ✅ Passed The description is detailed and covers the issue context, implementation, scope, trade-offs, test procedure, results, limitations, and checklist. The linked issue, author verification, maintainer revi…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review status

Thanks for contributing. This comment tracks the review sequence and the next action.

Current step: Awaiting fresh human maintainer or CODEOWNER approval.

Automated review is complete for the latest commit but does not replace human approval.

Review-state labels are managed by this workflow; do not edit them manually.

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@huggix
huggix marked this pull request as ready for review September 10, 2026 12:57
@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 10, 2026
@github-actions github-actions Bot added awaiting-maintainer CodeRabbit approved; waiting for a human maintainer and removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-maintainer CodeRabbit approved; waiting for a human maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant