Skip to content

fix(desktop): guard packaged backend readiness parser - #60842

Open
embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/fix-desktop-ready-artifact-skew
Open

fix(desktop): guard packaged backend readiness parser#60842
embwl0x wants to merge 1 commit into
NousResearch:mainfrom
embwl0x:agent/fix-desktop-ready-artifact-skew

Conversation

@embwl0x

@embwl0x embwl0x commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #60772.

Summary

  • add an afterPack artifact guard that inspects the packaged desktop app.asar
  • validate the packaged dist/electron-main.mjs readiness parser accepts both HERMES_BACKEND_READY and HERMES_DASHBOARD_READY
  • keep the guard in the packaged-artifact path so source/parser skew fails packaging instead of shipping a boot-breaking app
  • add artifact-level regression coverage for the packaged parser behavior

Refresh note

Rebased onto current main after Desktop moved its platform suite to Vitest. The standalone node:test artifact regression now uses a non-Vitest filename and runs before the current Vitest electron project, preserving both test contracts.

Verification

  • node --test apps/desktop/scripts/backend-ready-artifact.node-test.mjs - 6 passed
  • npm --prefix apps/desktop run test:desktop:platforms - 37 files passed; 413 tests passed, 1 skipped, plus the 6 artifact tests
  • npm --prefix apps/desktop run typecheck
  • node --check apps/desktop/scripts/after-pack.mjs && node --check apps/desktop/scripts/backend-ready-artifact.mjs && node --check apps/desktop/scripts/backend-ready-artifact.node-test.mjs
  • git diff --check origin/main...HEAD
  • publish-gate gitleaks scan - no leaks found

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 8, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved (LGTM)

Fixes packaged Electron backend readiness parser artifact skew. The afterPack artifact guard now extracts and validates the packaged backend-ready.cjs from app.asar, ensuring both HERMES_BACKEND_READY and HERMES_DASHBOARD_READY are accepted.

Changes

  • apps/desktop/scripts/after-pack.cjs: artifact extraction
  • apps/desktop/scripts/backend-ready-artifact.cjs: packaged parser validation
  • apps/desktop/scripts/backend-ready-artifact.test.cjs: regression tests

Quality

  • 4 files, 280 additions / 7 deletions
  • Tests: backend-ready-artifact.test.cjs (113 lines), electron/backend-ready.test.cjs, test:desktop:platforms (313 passed)
  • Gitleaks clean, git diff --check clean

Reviewed by Hermes Agent

@embwl0x

embwl0x commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Final duplicate audit: #61045 currently references #60772, but that PR fixes PTY wrong-session reattachment. #60772 is specifically packaged Desktop readiness-parser artifact skew; this PR validates the final app.asar parser during packaging. The two PRs touch different failure paths and are not competing implementations. I notified #61045 of the incorrect issue reference.

@embwl0x
embwl0x force-pushed the agent/fix-desktop-ready-artifact-skew branch from 52734ad to c7e284a Compare July 10, 2026 09:30
@embwl0x
embwl0x marked this pull request as ready for review July 10, 2026 09:30
@embwl0x
embwl0x requested a review from a team July 10, 2026 09:30
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for adding a packaged-artifact guard for the reported source/artifact skew. I found no blocking correctness issue.

  • Remote main already accepts both tokens in source at apps/desktop/electron/backend-ready.ts:6, while its current afterPack hook has no artifact assertion at apps/desktop/scripts/after-pack.mjs:26-40.
  • apps/desktop/package.json:191-195 unpacks dist/**, matching the PR's primary app.asar.unpacked/dist/electron-main.mjs inspection path; the archive extraction fallback covers layouts without that unpacked file.
  • The PR base-to-current-main comparison contains no changes to the four affected Desktop packaging paths, so c7e284aceac45296331ef004677f9e979adb28ae should be mechanically salvageable.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 10, 2026
@embwl0x
embwl0x force-pushed the agent/fix-desktop-ready-artifact-skew branch from c7e284a to 1a43b03 Compare July 10, 2026 18:41
@embwl0x
embwl0x force-pushed the agent/fix-desktop-ready-artifact-skew branch from 1a43b03 to 2c83532 Compare July 13, 2026 10:50
@embwl0x

embwl0x commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this existing PR onto current main in commit 2c83532de1 to clear the newly reported conflict.

Conflict resolution retained both additions to test:desktop:platforms: the upstream electron/wsl-path-bridge.test.ts entry and the packaged-artifact regression from this PR. The other three affected files replayed cleanly.

Overlap audit: #63573 / #60763 change the Python backend to emit both readiness tokens for backward compatibility with older Desktop clients. This PR is complementary: it inspects the final packaged app.asar and fails packaging if dist/electron-main.mjs does not accept both tokens. #61045 is the unrelated PTY reattach fix and still carries an incorrect #60772 reference.

Current-main validation:

  • node --test apps/desktop/scripts/backend-ready-artifact.test.mjs: 6 passed
  • node --import tsx --test electron/backend-ready.test.ts scripts/backend-ready-artifact.test.mjs: 23 passed
  • npm --prefix apps/desktop run typecheck: passed
  • syntax checks, git diff --check, current-main ancestry, and gitleaks: passed

The repository-wide raw node --test Desktop script still hits the existing extensionless TypeScript import failures; all six new .mjs tests pass within that run.

@embwl0x
embwl0x force-pushed the agent/fix-desktop-ready-artifact-skew branch from 355cdcd to d2c9f45 Compare August 15, 2026 09:49
@embwl0x

embwl0x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the latest Desktop packaging conflict on current main in d2c9f4567.

The platform command now preserves both current main native find-in-page coverage and the packaged readiness-artifact guard. The guard still runs first, and the real afterPack hook verified the generated macOS Hermes.app app.asar before code signing completed.

Local proof: 6 artifact tests and 1,197 Electron tests passed with 2 expected skips; all TypeScript projects passed; an actual unpacked package build and signing completed; syntax, diff/security gates, and publish gate passed. Replacement GitHub CI is fully green, and GitHub reports the PR mergeable.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(desktop): guard packaged backend readiness parser

A well-scoped artifact guard with real tests and a graceful fallback between the unpacked bundle and @electron/asar extraction. A few observations:

  1. apps/desktop/scripts/backend-ready-artifact.mjs (READY_MATCHER_SOURCE, line 8): the guard checks for the exact literal HERMES_(?:BACKEND|DASHBOARD)_READY in the bundled source. Any functionally-equivalent refactor of the parser regex (e.g. building the pattern from parts, or reordering the alternation) would fail packaging even though the parser is correct — a false-positive coupling to formatting. Since the goal is "accepts both tokens", asserting both HERMES_BACKEND_READY and HERMES_DASHBOARD_READY literals appear would be at least as strong (the stale dashboard-only parser contains only one) and robust to regex-format changes.

  2. The guard is a text-level heuristic: it proves the regex literal survived bundling, not that the parser logic executes correctly (an inverted condition or dead code around the same literal would pass). That is an acceptable smoke check for the skew bug, but consider documenting the limitation in the script header so future maintainers do not treat it as functional verification.

  3. Minor: resolvePackagedAsarPath's darwin fallback uses productFilename || 'Hermes'. electron-builder's appOutDir on macOS is normally already the .app path (handled by the .endsWith('.app') branch), but if a custom product name does not match the actual bundle name, the guard throws and blocks packaging with a confusing error — a path-existence check (or a clearer message naming the expected path) would help.

@alt-glitch alt-glitch added platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists and removed P3 Low — cosmetic, nice to have labels Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows Desktop can timeout when packaged Electron readiness parser is older than Python backend

5 participants