test: characterize read_file behavior for unaccepted parameters#518
Closed
wonderwhy-er wants to merge 1 commit into
Closed
test: characterize read_file behavior for unaccepted parameters#518wonderwhy-er wants to merge 1 commit into
wonderwhy-er wants to merge 1 commit into
Conversation
Adds an integration test that drives the real MCP server over stdio (like an LLM client) and asserts on the actual CallToolResult for three cases: - Unknown/unaccepted params (e.g. view_range, foo_bar) are silently stripped: the read succeeds, isError is falsy, and nothing tells the model its param was ignored. This documents the current gap. The intended future behavior is to keep the normal response but append a warning that params were stripped; when that lands, the Case 1 "no warning" assertion flips and acts as the regression anchor. - Wrong type on a known param returns a dispatcher-shaped isError: true. - Missing required param (path) returns a dispatcher-shaped isError: true. Test only; no production code changes.
Contributor
📝 WalkthroughWalkthroughA new integration test file is added that launches the real MCP server over stdio and exercises Changesread_file unknown-params integration test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration/read-file-unknown-params.js`:
- Around line 84-131: The main() function has resource leak issues because
setup() occurs before the try block and teardown() could fail before
client.close() executes. Restructure using nested try-finally blocks: move the
client creation outside, then wrap setup() in an outer try-finally that ensures
client.close() in its finally, and wrap the test cases in an inner try-finally
that ensures teardown() in its finally. This guarantees that client.close()
executes even if setup() throws, and that close() executes even if teardown()
throws, preventing stdio transport and subprocess hangs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77dc95af-0f14-49aa-b888-0beaa1d2baf3
📒 Files selected for processing (1)
test/integration/read-file-unknown-params.js
Comment on lines
+84
to
+131
| async function main() { | ||
| console.log('===== read_file Unknown-Parameter Behavior Integration Test ====='); | ||
| const client = await createMcpClient(); | ||
| const originalConfig = await setup(client); | ||
|
|
||
| try { | ||
| // --- Case 1: unknown/unaccepted parameters are silently stripped --- | ||
| const unknown = await callTool(client, 'read_file', { | ||
| path: TEST_FILE, | ||
| view_range: [5, 10], // not a real param on main | ||
| foo_bar: true, // clearly bogus | ||
| }); | ||
| const unknownText = textOf(unknown); | ||
| console.log('\n[Case 1] unknown params -> isError:', !!unknown.isError); | ||
|
|
||
| // 1a. The call is NOT rejected. | ||
| assert.ok(!unknown.isError, 'Case 1: unknown params should NOT cause isError (they are stripped)'); | ||
| // 1b. Because the params were ignored, it falls back to defaults: a read from | ||
| // the START of the file, not lines 5-10 the caller asked for. | ||
| assert.ok(/line-1\b/.test(unknownText), 'Case 1: should read from the start (params ignored)'); | ||
| assert.ok(!/^line-5$/m.test(unknownText) || /line-1\b/.test(unknownText), | ||
| 'Case 1: did not honor the requested range'); | ||
| // 1c. THE GAP: nothing in the response tells the model a param was stripped. | ||
| // When the future "append a warning" behavior lands, flip this assertion. | ||
| const mentionsStripped = /strip|ignored|unknown param|unrecognized|not a valid/i.test(unknownText); | ||
| assert.ok(!mentionsStripped, | ||
| 'Case 1 (current behavior): response contains NO warning about stripped params'); | ||
| console.log('[Case 1] PASS - params silently stripped, no warning returned (documented gap)'); | ||
|
|
||
| // --- Case 2: wrong type on a known param -> shaped isError --- | ||
| const badType = await callTool(client, 'read_file', { path: TEST_FILE, offset: 'not-a-number' }); | ||
| console.log('[Case 2] wrong type -> isError:', !!badType.isError); | ||
| assert.ok(badType.isError, 'Case 2: wrong type should return isError: true'); | ||
| assert.ok(/offset/i.test(textOf(badType)), 'Case 2: error should reference offset'); | ||
| console.log('[Case 2] PASS - wrong type surfaced to the model'); | ||
|
|
||
| // --- Case 3: missing required param -> shaped isError --- | ||
| const missing = await callTool(client, 'read_file', { offset: 2 }); | ||
| console.log('[Case 3] missing path -> isError:', !!missing.isError); | ||
| assert.ok(missing.isError, 'Case 3: missing path should return isError: true'); | ||
| console.log('[Case 3] PASS - missing required param surfaced to the model'); | ||
|
|
||
| console.log('\nAll assertions passed. Current read_file param behavior is documented.'); | ||
| } finally { | ||
| await teardown(client, originalConfig); | ||
| await client.close(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Ensure client cleanup even if setup or teardown fails.
The current structure has two resource leak scenarios:
- If
setup(client)throws after the client is created (line 87), the finally block at lines 127-130 won't execute, leavingclientunclosed. - If
teardown()throws (line 128),client.close()at line 129 won't execute.
Both scenarios could leave the stdio transport and subprocess hanging, affecting test reliability in CI environments.
🔒 Proposed fix with nested try-finally blocks
async function main() {
console.log('===== read_file Unknown-Parameter Behavior Integration Test =====');
const client = await createMcpClient();
try {
const originalConfig = await setup(client);
-
try {
- // --- Case 1: unknown/unaccepted parameters are silently stripped ---
+ // --- Case 1: unknown/unaccepted parameters are silently stripped ---
...
- console.log('\nAll assertions passed. Current read_file param behavior is documented.');
+ console.log('\nAll assertions passed. Current read_file param behavior is documented.');
+ } finally {
+ await teardown(client, originalConfig);
+ }
} finally {
- await teardown(client, originalConfig);
await client.close();
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/read-file-unknown-params.js` around lines 84 - 131, The
main() function has resource leak issues because setup() occurs before the try
block and teardown() could fail before client.close() executes. Restructure
using nested try-finally blocks: move the client creation outside, then wrap
setup() in an outer try-finally that ensures client.close() in its finally, and
wrap the test cases in an inner try-finally that ensures teardown() in its
finally. This guarantees that client.close() executes even if setup() throws,
and that close() executes even if teardown() throws, preventing stdio transport
and subprocess hangs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Test-only PR. Adds an integration test that documents what
read_filereturns today when a caller sends parameters the schema doesn't accept.It drives the real MCP server over stdio via the SDK
Client(the same path an LLM hits), so every assertion is against the actualCallToolResultthe model receives — not a direct handler call.Why
A model recently called
read_filewithview_range, which the schema doesn't define. The key was silently stripped, the defaults took over, and it read 1000 lines from the start — looking like the tool "ignored the range." This test pins down that behavior so it's visible in CI and so any future change to it is intentional.Cases asserted (current behavior on main)
view_range,foo_bar) →isErroris falsy, the read falls back to a read-from-start, and nothing in the response tells the model a param was stripped. This is the gap.offset: "x") → dispatcher-shapedisError: truereferencingoffset.path) → dispatcher-shapedisError: true.Intended follow-up (not in this PR)
The future behavior is to keep returning the normal response but append a warning that some parameters were stripped, so the model can self-correct. When that lands, the Case 1 "no warning" assertion flips — it's the regression anchor.
Notes
test/integration/, auto-discovered byrun-all-integration-tests.js; excluded from the defaultrun-all-tests.jssuite.distbuilt frommain.Summary by CodeRabbit
read_filebehavior with various parameter scenarios, including handling of unknown parameters, type validation for known parameters, and required parameter enforcement.