fix(agents): repair malformed tool-call args on openai-completions#70294
Conversation
Greptile SummaryThis PR fixes the malformed tool-call argument repair wrapper being gated only to Confidence Score: 5/5Safe to merge — all changes are well-scoped, the prior Kimi-scoping concern is resolved, and new gate tests lock in the intended transport/provider matrix. All remaining findings are P2 or lower. The core fix is correct and covered by four focused tests. The ancillary changes (builtInTools removal, resolveApiKeyForProvider fallback, plugin-ID preservation) are internally consistent and have matching test updates. No files require special attention. Reviews (2): Last reviewed commit: "docs: credit openai-completions repair P..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21301ee53b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ca1a763 to
008f4a9
Compare
008f4a9 to
f2d511f
Compare
43f910b to
f47a60e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47a60e2c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -71,12 +85,15 @@ async function resolveGeneratedImagePath(params: { | |||
| } | |||
|
|
|||
| async function ensureImageGenerationConfigured(env: QaSuiteRuntimeEnv) { | |||
| const snapshot = await readConfigSnapshot(env); | |||
There was a problem hiding this comment.
Retry pre-patch config snapshot reads during gateway restarts
Calling readConfigSnapshot(env) directly here introduces a new hard-fail point that bypasses patchConfig’s restart/hash-conflict retry logic (runConfigMutation in suite-runtime-gateway.ts). If ensureImageGenerationConfigured runs while the gateway is restarting (a normal QA flow race), this await can throw on config.get (e.g., closed/restarting gateway) and abort media setup before patchConfig gets a chance to recover, making the scenario flaky compared with the previous implementation.
Useful? React with 👍 / 👎.
f47a60e to
08b2f52
Compare
|
Maintainer review: no blocker from my pass. The repair gate now has the shape I want: Kimi repair remains scoped to Recommendation: merge once the remaining pending |
08b2f52 to
cbd2613
Compare
|
Just to flag, as far as I can tell, #70294 doesn't cover two other wire-level issues on the Kimi/SGLang + openai-completions route:
|
Summary
openai-completionscould emit streamed tool-call arguments in shapes the existing repair wrapper knows how to fix, but the runner only enabled that wrapper foranthropic-messages+ Kimi.shouldRepairMalformedToolCallArguments, kept the existing Kimi provider behavior, and additionally enabled the repair wrapper foropenai-completionstransports. Added focused guard tests and generalized the repair log wording.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
run/attempt.tsonly enabled it whenparams.model.api === "anthropic-messages"and the provider normalized to Kimi.openai-completions, so the repair path stayed dormant even though the runtime already had logic for this class of malformed stream.Regression Test Plan (if applicable)
src/agents/pi-embedded-runner/run/attempt.tool-call-argument-repair.test.tssrc/agents/pi-embedded-runner/run/attempt.test.tsanthropic-messagesstill enables repair,openai-completionsnow enables repair even for non-Kimi provider ids, and unrelated transports remain off.src/agents/pi-embedded-runner/run/attempt.test.tsalready covers the live wrapper behavior after enablement.User-visible / Behavior Changes
openai-completionsbackends such as Kimi/SGLang now get the existing malformed streamed tool-call argument repair path, reducing empty-argument or unusable-argument tool dispatch failures.Diagram (if applicable)
Security Impact (required)
Yes, explain risk + mitigation: N/ARepro + Verification
Environment
openai-completions; existing Kimianthropic-messagespath preservedSteps
openai-completionsthat streams fragmented tool-call arguments.run/attempt.ts.Expected
openai-completionsand preserves existing Kimi behavior.Actual
Evidence
Human Verification (required)
pnpm check:changedpnpm tsgopnpm test src/agents/pi-embedded-runner/run/attempt.tool-call-argument-repair.test.ts src/agents/pi-embedded-runner/run/attempt.test.tsanthropic-messagesopenai-completionsnow returns true even when the provider id is not normalized to Kimiopenai-responsesremains disabledReview Conversations
Compatibility / Migration
Risks and Mitigations
openai-completionsroute is broader than a Kimi-only gate and could affect other OpenAI-compatible backends.openai-completionstransports remain unchanged.