fix(desktop): assert Linux AppImage embeds Postgres packaging#2131
Conversation
📝 WalkthroughWalkthroughAdds a Node-based verifier for Linux AppImage embedded Postgres packaging, tests its file and metadata checks, and verifies release workflows run it after packaging Linux desktop artifacts. ChangesLinux AppImage packaging verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseWorkflow
participant PackagingVerifier
participant LinuxAppImage
participant Asar
ReleaseWorkflow->>PackagingVerifier: Run verification after packaging
PackagingVerifier->>LinuxAppImage: Discover unpacked resources
PackagingVerifier->>LinuxAppImage: Validate Postgres binaries and symlinks
PackagingVerifier->>Asar: List and extract app.asar
Asar-->>PackagingVerifier: Return runtime entries and package metadata
PackagingVerifier-->>ReleaseWorkflow: Report verification status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds release checks for embedded Postgres in Linux AppImages. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(desktop): harden AppImage packaging ..." | Re-trigger Greptile |
Address Greptile P1s: require hydrate soname targets from pg-symlinks.json (not just initdb/pg_ctl/postgres) and require omp-runtime dist/index.js + dist/probe.js entrypoints instead of a bare package-name substring.
| for (const entry of entries) { | ||
| if (!entry || typeof entry !== "object") continue; | ||
| const sourceRel = typeof entry.source === "string" ? entry.source : ""; | ||
| const targetRel = typeof entry.target === "string" ? entry.target : ""; | ||
| if (!sourceRel || !targetRel) continue; |
There was a problem hiding this comment.
Reject invalid link entries. This loop skips non-object entries and objects without string
source and target fields. A non-empty manifest such as [{}] therefore leaves missing at zero and reports success without checking any required ABI links. The release can pass while embedded Postgres still fails to load its shared libraries. Treat each malformed entry as a verification failure.
| for (const entry of entries) { | |
| if (!entry || typeof entry !== "object") continue; | |
| const sourceRel = typeof entry.source === "string" ? entry.source : ""; | |
| const targetRel = typeof entry.target === "string" ? entry.target : ""; | |
| if (!sourceRel || !targetRel) continue; | |
| for (const entry of entries) { | |
| if (!entry || typeof entry !== "object") { | |
| fail(`${unpackedDir}: ${platform} native/pg-symlinks.json contains an invalid link entry`); | |
| missing += 1; | |
| continue; | |
| } | |
| const sourceRel = typeof entry.source === "string" ? entry.source : ""; | |
| const targetRel = typeof entry.target === "string" ? entry.target : ""; | |
| if (!sourceRel || !targetRel) { | |
| fail(`${unpackedDir}: ${platform} native/pg-symlinks.json contains an entry without source and target paths`); | |
| missing += 1; | |
| continue; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/desktop/src/__tests__/release-workflow.test.ts`:
- Around line 66-92: Replace the substring-only assertions in the test covering
release and test-release workflows with fixture-based behavioral tests for valid
and broken Linux unpacked trees, asserting the verifier succeeds and rejects the
missing packaging cases. Import or invoke the verifier’s executable entrypoint
through the supported command path, and verify in both workflow contents that
“node scripts/verify-desktop-linux-pg-packaging.mjs” occurs after the
electron-builder packaging command rather than merely being present.
In `@scripts/verify-desktop-linux-pg-packaging.mjs`:
- Around line 216-219: Update listIncludesAsarPath to parse the
newline-delimited ASAR listing into a normalized Set and require exact path
matches, removing substring-based list.includes checks. Use the resulting
asarPaths.has checks for the bootstrap and OMP entrypoint validations so
similarly named files cannot satisfy them.
- Around line 242-255: Strengthen the verification loop around platforms and
binPath so each unpacked directory derives its expected architecture and
validates only the matching linux-* package. Require initdb, pg_ctl, and
postgres to be executable regular files, and inspect their ELF machine type when
possible to reject mismatched x64/arm64 payloads or placeholders; preserve the
existing failure reporting and SONAME-link checks.
- Around line 172-176: Update the pg-symlinks.json entry validation in the loop
over entries so malformed objects or entries with non-string or empty
source/target values increment the existing missing failure count instead of
being skipped. Preserve processing of valid entries and ensure any malformed
metadata causes verification to fail.
- Around line 28-30: Update the imports in the module containing __dirname to
explicitly import URL from the appropriate Node module, while preserving the
existing fileURLToPath and __dirname behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac0575cd-6818-42be-a2eb-fd7f3f0c7c65
📒 Files selected for processing (4)
.github/workflows/release.yml.github/workflows/test-release.ymlpackages/desktop/src/__tests__/release-workflow.test.tsscripts/verify-desktop-linux-pg-packaging.mjs
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| const __dirname = fileURLToPath(new URL(".", import.meta.url)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Import URL to fix the lint failure.
The repository lint configuration does not recognize URL as a global.
Proposed fix
-import { fileURLToPath } from "node:url";
+import { URL, fileURLToPath } from "node:url";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { fileURLToPath } from "node:url"; | |
| const __dirname = fileURLToPath(new URL(".", import.meta.url)); | |
| import { URL, fileURLToPath } from "node:url"; | |
| const __dirname = fileURLToPath(new URL(".", import.meta.url)); |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 30-30:
'URL' is not defined
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-desktop-linux-pg-packaging.mjs` around lines 28 - 30, Update
the imports in the module containing __dirname to explicitly import URL from the
appropriate Node module, while preserving the existing fileURLToPath and
__dirname behavior.
Source: Linters/SAST tools
| for (const entry of entries) { | ||
| if (!entry || typeof entry !== "object") continue; | ||
| const sourceRel = typeof entry.source === "string" ? entry.source : ""; | ||
| const targetRel = typeof entry.target === "string" ? entry.target : ""; | ||
| if (!sourceRel || !targetRel) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject malformed pg-symlinks.json entries instead of skipping them.
An array containing objects without valid source/target values currently reports success because those entries never increment missing. Fail each malformed entry so corrupted metadata cannot approve a broken package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-desktop-linux-pg-packaging.mjs` around lines 172 - 176, Update
the pg-symlinks.json entry validation in the loop over entries so malformed
objects or entries with non-string or empty source/target values increment the
existing missing failure count instead of being skipped. Preserve processing of
valid entries and ensure any malformed metadata causes verification to fail.
| function listIncludesAsarPath(list, entry) { | ||
| // asar list prints paths with a leading slash. | ||
| return list.includes(entry) || list.includes(entry.replace(/^\//, "")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match complete ASAR paths, not substrings.
includes() lets entries such as dist/main-bootstrap.cjs.map or dist/index.js.backup satisfy the checks. Parse the newline-delimited ASAR listing into a normalized Set and require exact entries.
Proposed fix
-function listIncludesAsarPath(list, entry) {
- // asar list prints paths with a leading slash.
- return list.includes(entry) || list.includes(entry.replace(/^\//, ""));
+function parseAsarPaths(list) {
+ return new Set(
+ list.split(/\r?\n/).filter(Boolean).map((path) =>
+ path.startsWith("/") ? path : `/${path}`,
+ ),
+ );
}Then use exact asarPaths.has(...) checks for the bootstrap and OMP entrypoints.
Also applies to: 273-280
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-desktop-linux-pg-packaging.mjs` around lines 216 - 219, Update
listIncludesAsarPath to parse the newline-delimited ASAR listing into a
normalized Set and require exact path matches, removing substring-based
list.includes checks. Use the resulting asarPaths.has checks for the bootstrap
and OMP entrypoint validations so similarly named files cannot satisfy them.
| const platforms = readdirSync(platformRoot).filter((n) => n.startsWith("linux-")); | ||
| if (platforms.length === 0) { | ||
| fail(`${unpackedDir}: no @embedded-postgres/linux-* packages in asar.unpacked (got: ${readdirSync(platformRoot).join(", ") || "none"})`); | ||
| } else { | ||
| ok(`${unpackedDir}: platform packages: ${platforms.join(", ")}`); | ||
| for (const platform of platforms) { | ||
| for (const bin of ["initdb", "pg_ctl", "postgres"]) { | ||
| const binPath = join(platformRoot, platform, "native", "bin", bin); | ||
| if (!existsSync(binPath)) { | ||
| fail(`${unpackedDir}: missing ${platform}/native/bin/${bin}`); | ||
| } | ||
| } | ||
| assertPlatformSonameLinks(unpackedDir, platformRoot, platform); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Verify the package architecture and executable binaries for each unpacked tree.
The verifier passes when any linux-* package contains files with the expected names. Thus an arm64 tree containing only x64 payloads—or non-executable placeholders—can pass. Derive the expected architecture from each unpacked directory and require executable regular files from its matching platform package; checking the ELF machine type would provide the strongest guarantee.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/verify-desktop-linux-pg-packaging.mjs` around lines 242 - 255,
Strengthen the verification loop around platforms and binPath so each unpacked
directory derives its expected architecture and validates only the matching
linux-* package. Require initdb, pg_ctl, and postgres to be executable regular
files, and inspect their ELF machine type when possible to reject mismatched
x64/arm64 payloads or placeholders; preserve the existing failure reporting and
SONAME-link checks.
Address Greptile P1s: require hydrate soname targets from pg-symlinks.json (not just initdb/pg_ctl/postgres) and require omp-runtime dist/index.js + dist/probe.js entrypoints instead of a bare package-name substring.
80d394f to
4bdedf5
Compare
Address Greptile P1s: require hydrate soname targets from pg-symlinks.json (not just initdb/pg_ctl/postgres) and require omp-runtime dist/index.js + dist/probe.js entrypoints instead of a bare package-name substring.
4bdedf5 to
cf9fa56
Compare
#2138) ## Summary - **Windows CI:** run the embedded Postgres smoke as non-admin `fusion-pg` (with profile prewarm) so elevated `windows-latest` runners stop failing with PostgreSQL’s admin-token refusal. Packaging still runs as the job user. - **Linux AppImage:** add a packaging content verifier for `main-bootstrap`, `@embedded-postgres` natives, and `omp-runtime` dist entrypoints; wire it into `release.yml`, `test-release.yml`, and the advisory **Desktop packaging** PR lane (after `electron-builder --dir`). - Fix eslint `no-undef` on bare `URL` in the verifier script (was red on #2131). ## Context Desktop packaging on Ubuntu was mostly green; Windows desktop builds and the AppImage packaging PR (#2131 lint) were the remaining red paths. The win-pg-diag pivot (run smoke as non-admin) proved green on CI; this ports that approach without removing main’s elevated-token product path for end-user “Run as administrator” cases (smoke simply does not take that path when the process is non-admin). ## Test plan - [x] `pnpm --filter @fusion/desktop exec vitest run src/__tests__/release-workflow.test.ts` - [x] `pnpm exec eslint scripts/verify-desktop-linux-pg-packaging.mjs` - [ ] Desktop packaging workflow on this PR - [ ] Desktop Windows Build (workflow_dispatch) - [ ] Confirm #2131 supersession if this lands the same AppImage checks <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Strengthened Linux desktop AppImage validation to confirm embedded PostgreSQL artifacts, required binaries, symlink hydration, and the expected app entrypoints are present after packaging. * Improved Windows embedded PostgreSQL smoke testing by running under a non-administrator helper user with a prewarmed profile environment. * **Tests** * Added automated packaging/release workflow verification steps (Linux and Windows) to catch embedded PostgreSQL content regressions earlier, including during artifact build and release verification. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fb61a5f to
d91a055
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/desktop/src/__tests__/linux-pg-packaging-verifier.test.ts`:
- Around line 74-82: Update the test case around
verifier.assertPlatformSonameLinks to wrap both verification scenarios and their
assertions in a try...finally block, resetting process.exitCode to undefined in
finally so cleanup occurs even when an expectation fails.
In `@packages/desktop/src/__tests__/release-workflow.test.ts`:
- Around line 115-117: Update the ordering assertion in the release workflow
test to first verify that the "Package Linux desktop artifacts" marker exists in
workflow. Then compare the verifier step’s index against that validated
packaging-step index, preserving the intended ordering check and preventing a
missing marker from passing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ee2d11a-53da-429f-b0f2-5f43516f0adc
📒 Files selected for processing (2)
packages/desktop/src/__tests__/linux-pg-packaging-verifier.test.tspackages/desktop/src/__tests__/release-workflow.test.ts
| process.exitCode = undefined; | ||
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | ||
| expect(process.exitCode).toBeUndefined(); | ||
|
|
||
| await writeFile(path.join(nativeRoot, "pg-symlinks.json"), JSON.stringify([{}])); | ||
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | ||
| expect(process.exitCode).toBe(1); | ||
| process.exitCode = undefined; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Prevent global state leakage if test assertions fail.
If an assertion (e.g., expect(process.exitCode).toBeUndefined()) throws an error, process.exitCode will not be reset. This leaks a non-zero exit code to the test worker, potentially causing it to exit prematurely and fail the CI run unpredictably. Wrap the verification logic in a try...finally block to guarantee restoration.
🛠️ Proposed fix
- process.exitCode = undefined;
- verifier.assertPlatformSonameLinks("fixture", tempDir, platform);
- expect(process.exitCode).toBeUndefined();
-
- await writeFile(path.join(nativeRoot, "pg-symlinks.json"), JSON.stringify([{}]));
- verifier.assertPlatformSonameLinks("fixture", tempDir, platform);
- expect(process.exitCode).toBe(1);
- process.exitCode = undefined;
+ try {
+ process.exitCode = undefined;
+ verifier.assertPlatformSonameLinks("fixture", tempDir, platform);
+ expect(process.exitCode).toBeUndefined();
+
+ await writeFile(path.join(nativeRoot, "pg-symlinks.json"), JSON.stringify([{}]));
+ verifier.assertPlatformSonameLinks("fixture", tempDir, platform);
+ expect(process.exitCode).toBe(1);
+ } finally {
+ process.exitCode = undefined;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process.exitCode = undefined; | |
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | |
| expect(process.exitCode).toBeUndefined(); | |
| await writeFile(path.join(nativeRoot, "pg-symlinks.json"), JSON.stringify([{}])); | |
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | |
| expect(process.exitCode).toBe(1); | |
| process.exitCode = undefined; | |
| }); | |
| try { | |
| process.exitCode = undefined; | |
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | |
| expect(process.exitCode).toBeUndefined(); | |
| await writeFile(path.join(nativeRoot, "pg-symlinks.json"), JSON.stringify([{}])); | |
| verifier.assertPlatformSonameLinks("fixture", tempDir, platform); | |
| expect(process.exitCode).toBe(1); | |
| } finally { | |
| process.exitCode = undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/desktop/src/__tests__/linux-pg-packaging-verifier.test.ts` around
lines 74 - 82, Update the test case around verifier.assertPlatformSonameLinks to
wrap both verification scenarios and their assertions in a try...finally block,
resetting process.exitCode to undefined in finally so cleanup occurs even when
an expectation fails.
| expect(workflow.indexOf("node scripts/verify-desktop-linux-pg-packaging.mjs")).toBeGreaterThan( | ||
| workflow.indexOf("Package Linux desktop artifacts"), | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check for the existence of the packaging step before comparing indices.
If the string "Package Linux desktop artifacts" is modified or removed from the workflow, indexOf will return -1. Since the verifier step is present (index > 0), expect(>0).toBeGreaterThan(-1) will incorrectly pass, masking the missing step. Assert that the packaging step exists in the workflow to ensure the ordering check is meaningful.
🛡️ Proposed fix
- expect(workflow.indexOf("node scripts/verify-desktop-linux-pg-packaging.mjs")).toBeGreaterThan(
- workflow.indexOf("Package Linux desktop artifacts"),
- );
+ const packageIndex = workflow.indexOf("Package Linux desktop artifacts");
+ expect(packageIndex).toBeGreaterThan(-1);
+ expect(workflow.indexOf("node scripts/verify-desktop-linux-pg-packaging.mjs")).toBeGreaterThan(packageIndex);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(workflow.indexOf("node scripts/verify-desktop-linux-pg-packaging.mjs")).toBeGreaterThan( | |
| workflow.indexOf("Package Linux desktop artifacts"), | |
| ); | |
| const packageIndex = workflow.indexOf("Package Linux desktop artifacts"); | |
| expect(packageIndex).toBeGreaterThan(-1); | |
| expect(workflow.indexOf("node scripts/verify-desktop-linux-pg-packaging.mjs")).toBeGreaterThan(packageIndex); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/desktop/src/__tests__/release-workflow.test.ts` around lines 115 -
117, Update the ordering assertion in the release workflow test to first verify
that the "Package Linux desktop artifacts" marker exists in workflow. Then
compare the verifier step’s index against that validated packaging-step index,
preserving the intended ordering check and preventing a missing marker from
passing.
Summary
Verification of the Fusion Linux AppImage + embedded Postgres packaging surface (follow-on to #2106 Mac packaging and #2117 Windows PG work).
What we found
Published
v0.60.0AppImages are broken for Local/embedded Postgres (pre-fix(desktop): boot embedded Postgres in packaged app and ship omp dist #2106):embedded-postgres/@embedded-postgres/*inapp.asarorapp.asar.unpackedpackage.jsonmainis stilldist/main.js(nomain-bootstrap.cjs)omp-runtimepackagedasar.unpackedonly has incidental natives (pi-tui, esbuild, node-pty)Current main (post-fix(desktop): boot embedded Postgres in packaged app and ship omp dist #2106) packaging config is correct (verified via mac
--dirpack on this host):main→dist/main-bootstrap.cjsasarUnpackofembedded-postgres+@embedded-postgres/**app.asar.unpackedomp-runtimedist present in asarLinux arm64 native PG binary smoke (
@embedded-postgres/linux-arm6415.18) in Docker: initdb → start → create DB → persist across restart → OK (requires postinstall soname symlinks fromhydrate-symlinks.js/pg-symlinks.json).Host blocker: this machine is macOS arm64 — cannot produce or execute a Linux AppImage end-to-end. Linux packaging must run on
ubuntu-latestCI.Fix in this PR
Release jobs only checked that
*.AppImagefiles existed — which is how v0.60.0 shipped empty of Postgres. Add:scripts/verify-desktop-linux-pg-packaging.mjs— inspectslinux-*-unpackedtrees for:app.asar.unpackedembedded-postgres +@embedded-postgres/linux-*binsdist/main-bootstrap.cjs+package.jsonmainomp-runtimepresencerelease.yml+test-release.ymlafter AppImage artifact checksrelease-workflow.test.tsTest plan
pnpm --filter @fusion/core test:embedded-postgres(33/33 with 60s timeout; default 15s flaked under load)electron-builder-config,build-bundling,release-workflow)Fusion-0.60.0-linux-arm64.AppImage(checksum OK; PG packaging absent)electron-builder --mac --dir: asar.unpacked has PG + bootstrap + ompbuild-desktop-linuxon this PR (runs the new verifier against real linux-unpacked)Gaps remaining (not fixed here)
/api/healthon Linuxlinux-x64.AppImagelinux-x86_64.AppImage(workflow already correct)Summary by CodeRabbit
Bug Fixes
Tests