Skip to content

fix: stabilize Windows MCP packaging and logger tests#73

Open
Sun-sunshine06 wants to merge 2 commits intoOpenCoworkAI:mainfrom
Sun-sunshine06:fix/ci-mcp-staging-and-logger-flake
Open

fix: stabilize Windows MCP packaging and logger tests#73
Sun-sunshine06 wants to merge 2 commits intoOpenCoworkAI:mainfrom
Sun-sunshine06:fix/ci-mcp-staging-and-logger-flake

Conversation

@Sun-sunshine06
Copy link
Copy Markdown
Contributor

Summary

  • stage bundled MCP outputs into .bundle-resources/mcp before packaging so electron-builder no longer copies directly from freshly-written dist-mcp files on Windows
  • add retry-based MCP staging helpers and a regression test covering the staged resource path
  • make logger context tests wait for log files to appear and stabilize before reading to reduce file I/O flakes

Validation

  • npm.cmd run typecheck
  • npm.cmd test -- --run tests/logger-context.test.ts tests/bundle-mcp-script.test.ts tests/build-windows-script.test.ts tests/windows-legacy-uninstall-remediation.test.ts
  • npm.cmd run build:win

Copy link
Copy Markdown
Collaborator

@hqhq1025 hqhq1025 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Request Changes — Critical merge conflict with current main

The MCP staging approach is sound and correctly solves the Windows EBUSY issue.

🚨 Blocking: decryptWithFallback authTag parameter

This PR modifies credentials-store.ts's decryptWithFallback signature, but it appears to be based on an older version of the file. The current main branch has authTag?: string as a third parameter (line ~212). If this PR merges as-is, the authTag parameter would be silently dropped, causing all GCM-encrypted credentials to fail decryption (ERR_CRYPTO_INVALID_TAG).

Action needed: Rebase onto current main and reconcile the decryptWithFallback signature.

Other findings

  • Medium: replaceDirectoryWithRetries silently returns on ambiguous rename failure (source disappears mid-operation). Consider asserting the destination contains expected files before returning.
  • Low: Dead code in removePathWithRetries and copyFileWithRetries — unreachable lastError throw after the loop
  • Medium: Logger test stabilization replaces stream finish event with polling, which is less reliable
  • Low: Variable shadowing logFilePath in the new getLogContent

@hqhq1025
Copy link
Copy Markdown
Collaborator

Hi @Sun-sunshine06, thanks for tackling the Windows EBUSY packaging issue!

I've cherry-picked the MCP staging mechanism from this PR into main (commit 39b825c):

Adopted:

  • scripts/bundle-mcp.js: Full staging pipeline with retry helpers
  • electron-builder.yml: dist-mcp.bundle-resources/mcp across all platforms
  • .gitignore / package.json: Supporting changes
  • tests/bundle-mcp-script.test.ts: Staging regression tests

Fixes applied on top:

  • Removed unreachable throw lastError dead code after retry loops
  • Added destination directory verification in replaceDirectoryWithRetries

Not adopted (with reasons):

  • credentials-store.ts — Based on an older version of the file. Current main has authTag as a third parameter for GCM decryption. Merging would silently drop it, breaking all GCM-encrypted credential decryption.
  • credentials-store-legacy-key.test.ts — Depends on looksSuspiciousDecryptedText() which doesn't exist in current main
  • logger-context.test.ts — Polling approach is less reliable than the current stream finish event method

Your authorship is preserved via --author on the commit. I'm going to close this PR since the relevant changes are now on main. Thanks for the contribution! 🙏

hqhq1025 pushed a commit that referenced this pull request Mar 29, 2026
Cherry-pick the MCP bundling staging mechanism from Sun-sunshine06's PR #73
to solve the Windows EBUSY issue during packaging:

- Stage dist-mcp output to .bundle-resources/mcp before electron-builder runs
- Add retry helpers for file operations (EBUSY/EPERM/ENOTEMPTY)
- Update electron-builder.yml to read from staged directory
- Export bundleMCPServers/stageBundledServers for testability
- Add regression tests for staging and builder config

Fixes applied on top of PR:
- Remove unreachable dead code in retry loops
- Add destination verification after rename in replaceDirectoryWithRetries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants