fix(acpx): add windowsHide to MCP proxy spawn on Windows#60678
fix(acpx): add windowsHide to MCP proxy spawn on Windows#60678KChow-ctrl wants to merge 2 commits intoopenclaw:mainfrom
Conversation
|
This PR fixes issue #60672. The MCP proxy was not setting |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12277b984f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ext === ".mp3" || | ||
| contentType === "audio/ogg" || | ||
| contentType === "audio/opus" | ||
| contentType === "audio/opus" || | ||
| contentType === "audio/mpeg" |
There was a problem hiding this comment.
Keep MP3 files out of the Opus upload path
This change routes .mp3/audio/mpeg through resolveFeishuOutboundMediaKind as fileType: "opus" + msgType: "audio", but the send path uploads the original bytes directly (no MP3→Opus transcoding before im.file.create). In practice, MP3 media will be mislabeled as Opus, which can cause Feishu to reject the upload or produce broken audio playback; the previous stream/file fallback avoided that mismatch.
Useful? React with 👍 / 👎.
| // Clean up timeout if we set one | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); |
There was a problem hiding this comment.
Clear request timeout when fetch errors
The timer is cleared only after await fetch(...) returns successfully, so connection-level failures (for example DNS/connect/TLS errors) skip this cleanup and leave the timeout pending until it fires. Under repeated failures this accumulates unnecessary timers and can keep the process/event loop alive longer than intended; clearTimeout should run from a finally path that executes on both success and error.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR bundles four independent fixes despite a title and description that only describe one of them (the acpx
Notable concerns:
Confidence Score: 3/5Safe to merge for the acpx and Ollama timeout changes; the Feishu MP3→opus type mismatch warrants verification before shipping. The core acpx fix is straightforward and low-risk. The Ollama timeout is a sensible addition. The Feishu change is the main uncertainty: uploading MP3 bytes with file_type: opus may fail on the Feishu side if the API validates codec format, which would be a user-visible regression for audio messages. If the author has confirmed this works end-to-end on Feishu, confidence rises to 4–5. extensions/feishu/src/media.ts — the MP3-as-opus upload type and its compatibility with the Feishu API needs verification. extensions/ollama/src/stream.ts — minor: timeout cleanup on error path.
|
| headers.Authorization = `Bearer ${options.apiKey}`; | ||
| } | ||
|
|
||
| // Apply request timeout if configured and no external signal is provided. | ||
| // Local Ollama may need longer timeout than the default agent timeout. | ||
| let finalSignal: AbortSignal | undefined = options?.signal; | ||
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | ||
| if (!finalSignal && defaultTimeoutMs && defaultTimeoutMs > 0) { | ||
| const timeoutController = new AbortController(); | ||
| timeoutId = setTimeout(() => timeoutController.abort(), defaultTimeoutMs); | ||
| finalSignal = timeoutController.signal; | ||
| } | ||
|
|
||
| const response = await fetch(chatUrl, { | ||
| method: "POST", | ||
| headers, | ||
| body: JSON.stringify(body), | ||
| signal: options?.signal, | ||
| signal: finalSignal, | ||
| }); | ||
|
|
||
| // Clean up timeout if we set one | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId); | ||
| } | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text().catch(() => "unknown error"); | ||
| throw new Error(`${response.status} ${errorText}`); |
There was a problem hiding this comment.
clearTimeout(timeoutId) is only reached when fetch returns without throwing. If the fetch is aborted (by the timer itself or another error), execution jumps to the catch block and the timer is never explicitly cancelled. In the timeout-fires case this is a no-op (the timer already ran), but it means the clearTimeout call is missing for non-abort errors while the timer is still pending — a minor resource leak.
Consider placing if (timeoutId) clearTimeout(timeoutId); inside the outer finally block that already calls stream.end().
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/ollama/src/stream.ts
Line: 711-738
Comment:
**Timeout timer not cancelled on fetch error path**
`clearTimeout(timeoutId)` is only reached when `fetch` returns without throwing. If the fetch is aborted (by the timer itself or another error), execution jumps to the `catch` block and the timer is never explicitly cancelled. In the timeout-fires case this is a no-op (the timer already ran), but it means the `clearTimeout` call is missing for non-abort errors while the timer is still pending — a minor resource leak.
Consider placing `if (timeoutId) clearTimeout(timeoutId);` inside the outer `finally` block that already calls `stream.end()`.
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs maintainer review before merge. Summary Reproducibility: yes. source-reproducible: #60672 gives concrete Windows ACP Codex steps, and current main still spawns the MCP proxy target without Real behavior proof Next step before merge Security Review detailsBest possible solution: Land the narrowed ACPX fix after the exact-head architecture/check-additional failure is triaged or rerun green; keep broader Windows spawn cleanup such as #48320 separate. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: #60672 gives concrete Windows ACP Codex steps, and current main still spawns the MCP proxy target without Is this the best way to solve the issue? Yes. Adding What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4fe12fc0998c. Re-review progress:
|
|
Maintainer prep update:
Verification run locally:
Note: |
|
Maintainer refresh: rebased the prepared branch onto current main after main moved.\n\nNew prepared head: |
|
Maintainer prep refresh for #60678. Prepared head: What I changed:
Verification:
Fresh CI is running on the prepared head now. Not merging until required/full checks are green. |
Fixes #60672.
Summary
windowsHide: truefor the acpx MCP proxy target process on Windows.Verification
pnpm install --frozen-lockfilepnpm test extensions/acpx/src/runtime-internals/mcp-proxy.test.ts extensions/acpx/src/runtime-internals/mcp-command-line.test.ts— 6 tests passedpnpm exec oxfmt --check --threads=1 CHANGELOG.md extensions/acpx/src/runtime-internals/mcp-proxy.mjs extensions/acpx/src/runtime-internals/mcp-proxy.test.tsgit diff --checkpnpm check:changedNotes
pnpm test:extensions acpxpassed the acpx shard itself (71 tests) but then continued iterating unrelated extension configs with no matchingacpxtest files; I stopped that broad wrapper after the acpx shard completed and used the focused acpx tests pluscheck:changedas the gate.Live Windows ACP proof was not run from this macOS maintainer workspace; the issue is source-backed and covered by focused regression tests. A
proof: overridelabel/comment records that maintainer decision.