refactor(api-proxy): deduplicate null tool_call type normalization#3272
Conversation
…egate to sanitizeNullToolCallTypes
🔬 Smoke Test Results
Overall: FAIL — GitHub MCP auth failed; pre-step template variables were not expanded. Author/assignees could not be fetched due to MCP auth error.
|
|
Smoke Test Results:
Status: FAIL (2/3 tests passed)
|
Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( Overall: PARTIAL — BYOK inference path confirmed working; pre-step template variables were not substituted before agent invocation.
|
Smoke Test CodexPRs: fix: remove unused exports from public API surface (batch 2); refactor: split Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the API proxy’s Copilot provider to remove a duplicated “null tool_call type” normalizer and instead reuse the shared sanitizeNullToolCallTypes utility from body-transform.js, with corresponding test updates.
Changes:
providers/copilot.js: Removes the localnormalizeNullTypeToolCallsimplementation and wiressanitizeNullToolCallTypesinto the Copilot adapter’s body transform via a wrapper that adapts return types.server.auth.test.js: Updates the former normalizer tests to targetsanitizeNullToolCallTypesand its{ body, normalizedCount, droppedCount }return shape.
Show a summary per file
| File | Description |
|---|---|
| containers/api-proxy/providers/copilot.js | Deduplicates Copilot’s tool_call type normalization by delegating to sanitizeNullToolCallTypes. |
| containers/api-proxy/server.auth.test.js | Migrates tests from the removed Copilot-specific normalizer to the shared sanitizer. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| const bodyTransform = composeBodyTransforms( | ||
| deps.bodyTransform || null, | ||
| normalizeNullTypeToolCalls | ||
| (body) => { const result = sanitizeNullToolCallTypes(body); return result ? result.body : null; } | ||
| ); |
| @@ -188,9 +189,9 @@ describe('normalizeNullTypeToolCalls', () => { | |||
| ], | |||
| })); | |||
|
|
|||
| const transformed = normalizeNullTypeToolCalls(input); | |||
| expect(transformed).not.toBeNull(); | |||
| const parsed = JSON.parse(transformed.toString('utf8')); | |||
| const result = sanitizeNullToolCallTypes(input); | |||
| expect(result).not.toBeNull(); | |||
| const parsed = JSON.parse(result.body.toString('utf8')); | |||
| expect(parsed.messages[0].tool_calls[0].type).toBe('function'); | |||
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Chroot Version Comparison
Result: FAILED — Python and Node.js versions differ between host and chroot environments.
|
|
Gemini Smoke Test: Running... Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
Smoke Test Results — FAIL
Overall: FAIL —
|
copilot.jscontained a near-identical copy ofsanitizeNullToolCallTypesfrombody-transform.js(the shared utilities module), meaning a bug fix in one wouldn't propagate to the other.Changes
providers/copilot.js: RemovesnormalizeNullTypeToolCallsentirely. ImportssanitizeNullToolCallTypesfrom../body-transformand replaces the body transform reference with a one-line wrapper that bridges the return-type difference:Also removes
normalizeNullTypeToolCallsfrom the_testingexport.server.auth.test.js: Migrates thenormalizeNullTypeToolCallsdescribe block to testsanitizeNullToolCallTypesdirectly (imported frombody-transform), updating assertions to match its{ body, normalizedCount, droppedCount }return shape.