Skip to content

[codex] Structure desktop build script failures#3452

Merged
juliusmarminge merged 2 commits into
mainfrom
codex/desktop-build-errors
Jun 21, 2026
Merged

[codex] Structure desktop build script failures#3452
juliusmarminge merged 2 commits into
mainfrom
codex/desktop-build-errors

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Jun 21, 2026

Copy link
Copy Markdown
Member

Replaces the remaining generic BuildScriptError paths in the desktop artifact builder with schema-backed, structurally useful failures.

This keeps the macOS passkey and Linux icon fallback work from #3303 and #3447 intact, preserves immediate causes at transformation boundaries, keeps command output diagnostics structured, and uses distinct tags where callers need distinct failure predicates. Existing user-facing messages remain stable.

Validation:

  • vp test scripts/build-desktop-artifact.test.ts (22 tests)
  • vp check
  • vp run typecheck

Note

Medium Risk
Behavior change for any code catching BuildScriptError; desktop build/signing failure paths are refactored but logic is largely preserved with clearer error shapes.

Overview
Removes the generic BuildScriptError from the desktop artifact builder and routes each failure through schema-backed tagged error classes with stable message getters and typed fields (paths, platform/arch, kind, reason, command/exitCode, optional stdout/stderr tails).

runCommand now requires a label and fails with BuildCommandFailedError on non-zero exit; Linux icon resize aggregates those as structured causes under LinuxIconResizeError. Mock update port env parsing maps decode failures to InvalidMockUpdateServerPortError with classified reasons (not-numeric, not-integer, out-of-range). macOS passkey resolution uses MacPasskeySigningConfigurationResolutionError.fromCause to rethrow known signing errors unchanged.

Missing inputs, dependency resolution, icons, bundled client assets, provisioning profile, and post-build dist/artifact checks each get distinct error tags instead of string-only messages. Tests assert the new types and fields.

Reviewed by Cursor Bugbot for commit 57a31f9. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Replace generic BuildScriptError with structured tagged error classes in the desktop build script

  • Removes the single BuildScriptError class from build-desktop-artifact.ts and replaces all call sites with specific tagged error classes (e.g. BuildCommandFailedError, InvalidMockUpdateServerPortError, MacPasskeySigningConfigurationResolutionError, MissingDesktopBuildInputError, and ~10 others).
  • Each new error carries structured fields (exit code, paths, platform, reason, cause, etc.) and a stable message getter, making failures easier to classify and handle programmatically.
  • runCommand now requires an explicit label parameter and emits BuildCommandFailedError with stdout/stderr tails on non-zero exit.
  • Test suite in build-desktop-artifact.test.ts is updated to assert on the new error types, their fields, and classified reasons.
  • Behavioral Change: callers that previously caught BuildScriptError will no longer match; they must handle the new specific error types.

Macroscope summarized 57a31f9.

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5851f4fa-37b8-434a-837b-a96e1d94af54

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/desktop-build-errors

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jun 21, 2026
@juliusmarminge
juliusmarminge force-pushed the codex/desktop-build-errors branch from 9c1aa21 to 3f0e34d Compare June 21, 2026 00:47

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Mock port reason mislabels valid integers
    • Added an explicit range check so that when Number() produces a finite integer within 1-65535 but Schema.NumberFromString still rejected the input (due to format like '+8080' or '0x50'), the function now correctly returns 'not-numeric' instead of the misleading 'out-of-range'.

Create PR

Or push these changes by commenting:

@cursor push 94bdd12b4f
Preview (94bdd12b4f)
diff --git a/scripts/build-desktop-artifact.ts b/scripts/build-desktop-artifact.ts
--- a/scripts/build-desktop-artifact.ts
+++ b/scripts/build-desktop-artifact.ts
@@ -910,7 +910,9 @@
 ): typeof InvalidMockUpdateServerPortReason.Type {
   const parsed = Number(configuredPort);
   if (!Number.isFinite(parsed)) return "not-numeric";
-  return Number.isInteger(parsed) ? "out-of-range" : "not-integer";
+  if (!Number.isInteger(parsed)) return "not-integer";
+  if (parsed >= 1 && parsed <= 65535) return "not-numeric";
+  return "out-of-range";
 }
 
 const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) =>

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 3f0e34d. Configure here.

Comment thread scripts/build-desktop-artifact.ts
@macroscopeapp

macroscopeapp Bot commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved

Refactors build script error handling from a generic error class to multiple structured error types with richer metadata. Changes are confined to build scripts and tests with no production runtime impact.

No code changes detected at 57a31f9. Prior analysis still applies.

You can customize Macroscope's approvability policy. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Jun 21, 2026
@juliusmarminge
juliusmarminge force-pushed the codex/desktop-build-errors branch 3 times, most recently from 153182c to 357a773 Compare June 21, 2026 01:43
@macroscopeapp
macroscopeapp Bot dismissed their stale review June 21, 2026 01:43

Dismissing prior approval to re-evaluate 357a773

juliusmarminge and others added 2 commits June 20, 2026 18:49
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
@juliusmarminge
juliusmarminge force-pushed the codex/desktop-build-errors branch from 357a773 to 57a31f9 Compare June 21, 2026 01:49
@juliusmarminge
juliusmarminge merged commit 60dc4af into main Jun 21, 2026
15 checks passed
@juliusmarminge
juliusmarminge deleted the codex/desktop-build-errors branch June 21, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant