-
Notifications
You must be signed in to change notification settings - Fork 46k
fix(copilot): multi-layer defence against Write tool truncation errors #12749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -929,3 +929,101 @@ async def fake_tool_fn(_args: dict) -> dict: | |
| stashed = pop_pending_tool_output("fake_tool_normal") | ||
| assert stashed is not None | ||
| assert '"is_dry_run": true' in stashed | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Partial truncation detection (Layer 2 of write-tool-truncation fix) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestPartialTruncationDetection: | ||
| """When the API truncates a write tool call mid-JSON, `content` may survive | ||
| but `file_path` is lost. The wrapper must detect this and return actionable | ||
| guidance instead of letting the call through to fail with an opaque error. | ||
| """ | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_write_file_partial_truncation_detected(self): | ||
| """write_file with content but no file_path returns truncation error.""" | ||
|
|
||
| async def fake_tool_fn(_args: dict) -> dict: | ||
| raise AssertionError("Should not be called") | ||
|
|
||
| wrapper = _make_truncating_wrapper( | ||
| fake_tool_fn, | ||
| "write_file", | ||
| input_schema={"required": ["file_path", "content"]}, | ||
| ) | ||
| result = await wrapper({"content": "some data"}) | ||
| assert result["isError"] is True | ||
| text = result["content"][0]["text"] | ||
| assert "truncated" in text | ||
| assert "file_path" in text | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_edit_file_partial_truncation_detected(self): | ||
| """edit_file with new_string but no file_path returns truncation error.""" | ||
|
|
||
| async def fake_tool_fn(_args: dict) -> dict: | ||
| raise AssertionError("Should not be called") | ||
|
|
||
| wrapper = _make_truncating_wrapper( | ||
| fake_tool_fn, | ||
| "edit_file", | ||
| input_schema={"required": ["file_path", "old_string", "new_string"]}, | ||
| ) | ||
| result = await wrapper({"new_string": "replacement"}) | ||
| assert result["isError"] is True | ||
| text = result["content"][0]["text"] | ||
| assert "truncated" in text | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_write_file_with_file_path_passes_through(self): | ||
| """write_file with file_path present should NOT trigger truncation guard.""" | ||
| called = False | ||
|
|
||
| async def fake_tool_fn(_args: dict) -> dict: | ||
| nonlocal called | ||
| called = True | ||
| return { | ||
| "content": [{"type": "text", "text": "ok"}], | ||
| "isError": False, | ||
| } | ||
|
|
||
| normal_session = MagicMock() | ||
| normal_session.dry_run = False | ||
| set_execution_context(user_id="test", session=normal_session, sandbox=None, sdk_cwd="/tmp/test") # type: ignore[arg-type] | ||
|
|
||
| wrapper = _make_truncating_wrapper( | ||
| fake_tool_fn, | ||
| "write_file", | ||
| input_schema={"required": ["file_path", "content"]}, | ||
| ) | ||
| result = await wrapper({"file_path": "/home/user/f.txt", "content": "data"}) | ||
| assert called | ||
| assert result["isError"] is False | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_non_write_tool_not_affected(self): | ||
| """Partial truncation guard only applies to write/edit tools.""" | ||
| called = False | ||
|
|
||
| async def fake_tool_fn(_args: dict) -> dict: | ||
| nonlocal called | ||
| called = True | ||
| return { | ||
| "content": [{"type": "text", "text": "ok"}], | ||
| "isError": False, | ||
| } | ||
|
|
||
| normal_session = MagicMock() | ||
| normal_session.dry_run = False | ||
| set_execution_context(user_id="test", session=normal_session, sandbox=None, sdk_cwd="/tmp/test") # type: ignore[arg-type] | ||
|
|
||
| wrapper = _make_truncating_wrapper( | ||
| fake_tool_fn, | ||
| "bash_exec", | ||
| input_schema={"required": ["command"]}, | ||
| ) | ||
| await wrapper({"content": "some data"}) | ||
| assert called | ||
|
Comment on lines
+939
to
+1029
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Please add a The new wrapper logic also branches on 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -623,3 +623,46 @@ class _FakeFileInfo: | |
| # Normal workspace path must have produced a content response. | ||
| assert isinstance(result, WorkspaceFileContentResponse) | ||
| assert base64.b64decode(result.content_base64) == fake_content | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Truncation error message (Layer 4 of write-tool-truncation fix) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestWriteTruncationMessage: | ||
| """When write_workspace_file receives empty args due to API output-token | ||
| truncation, the error message must give actionable step-by-step guidance.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_empty_args_returns_actionable_truncation_message( | ||
| self, ephemeral_dir | ||
| ): | ||
| """Calling write with no args at all should mention cat > and cat >>.""" | ||
| write_tool = WriteWorkspaceFileTool() | ||
| result = await write_tool._execute( | ||
| user_id="test-user", | ||
| session=make_session(), | ||
| filename=None, # type: ignore[arg-type] | ||
| ) | ||
| assert isinstance(result, ErrorResponse) | ||
| assert "truncated" in result.message.lower() | ||
| assert "cat >" in result.message | ||
| assert "cat >>" in result.message | ||
| assert "source_path" in result.message | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_missing_filename_with_content_gives_simple_error( | ||
| self, ephemeral_dir | ||
| ): | ||
| """When content is present but filename is missing, it's not truncation — | ||
| just a missing required field.""" | ||
| write_tool = WriteWorkspaceFileTool() | ||
| result = await write_tool._execute( | ||
| user_id="test-user", | ||
| session=make_session(), | ||
| filename=None, # type: ignore[arg-type] | ||
| content="some content", | ||
| ) | ||
|
Comment on lines
+638
to
+666
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Remove the new These tests add two As per coding guidelines, "Do not use linter suppressors — no 🤖 Prompt for AI Agents |
||
| assert isinstance(result, ErrorResponse) | ||
| assert "filename" in result.message.lower() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard blocks valid
write_workspace_file(content=...)calls.write_workspace_filedoes not have afile_pathargument, so this condition treats every normal inline-content call as “partial truncation” and short-circuits before the tool runs. It also misses the other valid payload inputs for that tool (content_base64andsource_path).Proposed fix
🤖 Prompt for AI Agents