Skip to content

fix: skip windows exe patching in ci mode to avoid wine dependency#645

Merged
Nikhil (shadowfax92) merged 1 commit into
mainfrom
fix/github-server-release
Apr 3, 2026
Merged

fix: skip windows exe patching in ci mode to avoid wine dependency#645
Nikhil (shadowfax92) merged 1 commit into
mainfrom
fix/github-server-release

Conversation

@shadowfax92
Copy link
Copy Markdown
Contributor

Summary

  • The server release CI workflow (release-server.yml) fails on ubuntu-latest because patch-windows-exe.ts requires Wine to run rcedit-x64.exe, and Wine is not available on the CI runner
  • Threads the existing --ci flag through compileServerBinariescompileTarget so Windows PE metadata patching is skipped in CI mode with a warning log
  • Consistent with the existing CI isolation pattern where --ci already skips R2 uploads and production secrets

Changes

  • scripts/build/server/compile.ts — Added ci parameter to compileTarget and optional options to compileServerBinaries. When ci=true and target is Windows, logs a warning and skips patching instead of failing.
  • scripts/build/server/orchestrator.ts — Passes { ci: args.ci } to compileServerBinaries.

Test plan

  • Re-run the Release BrowserOS Server workflow on main after merging — should pass the "Build release artifacts" step
  • Verify local bun run build:server:test still works (non-CI mode, no behavior change)
  • Verify CI log shows warning: "Skipping Windows exe metadata patching in CI mode"

The server release CI workflow fails on ubuntu-latest because
patch-windows-exe.ts requires Wine to run rcedit. Thread the existing
--ci flag through compileServerBinaries so Windows PE metadata patching
is skipped in CI mode with a warning log.
@shadowfax92
Copy link
Copy Markdown
Contributor Author

Greptile (@greptileai) review

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Claude (@claude) review

@github-actions github-actions Bot added the fix label Apr 3, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Apr 3, 2026

Claude finished Nikhil (@shadowfax92)'s task in 1m 1s —— View job


Code Review

  • Gather context
  • Read changed files
  • Provide review feedback

Overall: LGTM — clean, minimal, and consistent with existing patterns.

The fix is well-scoped. The patch-windows-exe.ts script confirms why this fails in CI: it spawns wine rcedit-x64.exe on non-Windows platforms, and Wine is not available on ubuntu-latest runners. Skipping this step in CI is a sound trade-off since the PE metadata (ProductName, FileDescription, etc.) is cosmetic and doesn't affect binary functionality.

compile.ts

compileTarget(target, env, ci)           // line 53
compileServerBinaries(..., options?)     // line 85
  • The options?: { ci?: boolean } wrapper on compileServerBinaries is idiomatic — it's forward-compatible if more options are added later, and the ?? false default is correct.
  • log.warn (line 72) is the right level here: it's intentionally skipped, not silently ignored.
  • Minor: compileTarget takes ci: boolean directly while compileServerBinaries uses an object wrapper. Slight inconsistency, but not worth changing — the internal vs. external API distinction makes sense.

orchestrator.ts

{ ci: args.ci }   // line 40
  • One-line change, correct.

Potential concern (minor, not blocking)

The skipped patching means the Windows .exe shipped from CI builds will lack version metadata visible in Windows Explorer / Task Manager / installers. This is expected and documented in the test plan — just worth confirming the CI-built artifacts are only for internal testing, not distributed to end users as-is.

No issues found. Safe to merge.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 3, 2026

Greptile Summary

This PR fixes a CI failure in release-server.yml caused by patch-windows-exe.ts requiring Wine (unavailable on ubuntu-latest) by threading the existing --ci flag through compileServerBinariescompileTarget. When ci=true and the target is Windows, patching is skipped with a log.warn instead of failing. The change is minimal, backward-compatible, and consistent with the existing CI isolation pattern.

Confidence Score: 5/5

Safe to merge — the fix is minimal, backward-compatible, and correctly scoped to CI mode only.

No P0 or P1 issues found. The change threads an existing flag through two function signatures, uses the correct log.warn API, and has no other callers that need updating. All remaining observations are at most P2.

No files require special attention.

Important Files Changed

Filename Overview
packages/browseros-agent/scripts/build/server/compile.ts Adds ci boolean parameter to compileTarget and optional options to compileServerBinaries; skips Windows PE patching with a warning when ci=true. Backward-compatible, no other callers exist.
packages/browseros-agent/scripts/build/server/orchestrator.ts Threads { ci: args.ci } into compileServerBinaries. One-line change, consistent with existing CI branching pattern in the same file.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[runProdResourceBuild] -->|args.ci| B[compileServerBinaries\noptions: ci]
    B --> C[bundleServer]
    C --> D{for each target}
    D --> E[compileTarget\ntarget, env, ci]
    E --> F{target.os === 'windows'?}
    F -->|No| G[return binaryPath]
    F -->|Yes| H{ci === true?}
    H -->|Yes| I[log.warn\nSkipping Windows patching]
    H -->|No| J[runCommand\npatch-windows-exe.ts]
    I --> G
    J --> G
    G --> K[compiled artifacts]
Loading

Reviews (1): Last reviewed commit: "fix: skip windows exe patching in ci mod..." | Re-trigger Greptile

@shadowfax92 Nikhil (shadowfax92) merged commit a5f3c4d into main Apr 3, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant