Skip to content

fix(desktop): serve done() and preview harness from memory for sandboxed Chrome - #461

Merged
Sun-sunshine06 merged 1 commit into
OpenCoworkAI:mainfrom
Lxr-max:fix/done-verify-sandboxed-chrome
Oct 4, 2026
Merged

Sun-sunshine06 merged 1 commit into
OpenCoworkAI:mainfrom
Lxr-max:fix/done-verify-sandboxed-chrome

Conversation

@Lxr-max

@Lxr-max Lxr-max commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Summary

The done() runtime verifier wrote its harness HTML to os.tmpdir() (/tmp/codesign-done-verify-*/verify.html) and pointed system Chrome at that file:// URL. Chrome builds that run with a private /tmp, such as Snap Chromium (Ubuntu's chromium-browser/chromium are Snap wrappers) and Flatpak browsers, can't see that file. Every verification then fails with net::ERR_FILE_NOT_FOUND and the run ends in GENERATION_INCOMPLETE, even when App.jsx is fine. The agent preview tool has the same problem because it writes preview.html into its temp profile dir.

This PR serves both harness documents from memory through the request interception that is already in place (req.respond(...) for the exact document URL). The file:// URL stays the same, so relative workspace assets (<base href>), the file-URL allowlist, and error messages work as before. Nothing gets written to the temp directory anymore, so the verifier no longer needs a filesystem it shares with the browser. The disposable userDataDir stays in tmpdir(). Chrome creates it inside its own namespace, and puppeteer reads the DevTools endpoint from stderr.

Root cause reproduction (Linux): I used bwrap --dev-bind / / --tmpfs /tmp google-chrome, which gives Chrome a private /tmp the way Snap's per-snap /tmp does, as CODESIGN_CHROME_PATH:

before after
makeRuntimeVerifier() / workspace verifier resource failed: net::ERR_FILE_NOT_FOUND [file:///tmp/codesign-done-verify-…/verify.html] + runtime verifier load failed: net::ERR_FILE_NOT_FOUND … (the same two errors as in the issue) [] for a valid artifact, and a real ReferenceError is still reported for a broken one
runPreview ok: false, net::ERR_FILE_NOT_FOUND at file:///tmp/codesign-preview-…/preview.html ok: true, workspace-relative SVG loads

With the normal (non-sandboxed) Chrome, the results are the same before and after.

Type of change

  • Bug fix
  • New feature
  • Refactor (no behavior change)
  • Documentation
  • Build / CI / tooling
  • Breaking change

Linked issue

Closes #455

Checklist

  • I checked the linked issue / relevant context before starting
  • pnpm lint && pnpm typecheck && pnpm test passes locally
  • Added/updated tests for the change
  • Added a changeset (pnpm changeset) if user-visible
  • Updated docs if behavior changed (no documented behavior changes)

Tests

  • New apps/desktop/src/main/harness-document.test.ts (mocked puppeteer, runs in CI without Chrome):
    • verifier: the navigation target does not exist on disk, it is fulfilled via respond() with the generated document, and it is never continue()d
    • preview: the same check for preview.html
    • isHarnessDocumentRequest matches only the exact document URL
    • These tests fail on main and pass with this change.
  • The existing real-Chrome suites (done-verify.workspace.test.ts, preview-runtime.test.ts) still pass, including relative workspace assets and blocking files outside the workspace.

PRINCIPLES §5b

  • Compatibility ✅ No IPC, schema, config, or error-format changes. The document URL and asset resolution are the same, and it works with Snap, Flatpak, and regular Chrome on all platforms.
  • Upgradeability ✅ Nothing is persisted and there is no migration. The change is limited to the two harness loaders.
  • No bloat ✅ No new dependencies. It removes a temp-dir write and cleanup per verification. About 60 lines of production code change (+43/−16), and most of the diff is tests.
  • Elegance ✅ It reuses the request interception that both harnesses already install, so one small shared helper (isHarnessDocumentRequest + respondWithHarnessDocument) covers both without changing the sandbox or allowlist.

The done() verifier and the preview tool wrote their harness HTML under
os.tmpdir() and navigated system Chrome to that file:// URL. Chrome builds
with a private /tmp (Snap Chromium, Flatpak) cannot see that file, so every
verification failed with net::ERR_FILE_NOT_FOUND and generation ended in
GENERATION_INCOMPLETE.

Fulfil the harness document request through the existing request
interception instead, keeping the same file:// URL so relative workspace
assets and the file-URL allowlist behave exactly as before. Nothing is
written to the temp directory any more.

Refs OpenCoworkAI#455
@github-actions github-actions Bot added docs Documentation area:desktop apps/desktop (Electron shell, renderer) labels Oct 3, 2026

@github-actions github-actions 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.

Findings

No blockers, majors, or minors found. The change is correct and narrowly scoped: both harness loaders serve the document from memory at the same file:// URL through the request interceptors they already install, so <base href>, relative workspace assets, the file-URL allowlist, and error formatting are all unchanged. This matches the stated root cause (sandboxed Chrome with a private /tmp cannot read a file written under os.tmpdir()).

Verification notes from the diff:

  • apps/desktop/src/main/done-verify.ts:277 — verify.html is no longer written; srcdoc is passed into verifyWithSystemChrome(...) and served via respondWithHarnessDocument. Parameter order at both call sites (verifyWithSystemChrome(verifyUrl, verifyPath, srcdoc, workspaceRoot, context?.signal) and handleVerifierRequest(req, verifyPath, html, workspaceRoot)) lines up with the new signatures.
  • apps/desktop/src/main/preview-runtime.ts:420 isHarnessDocumentRequest and :434 respondWithHarnessDocument are checked before the allowlist in both handlers and sit inside the existing try blocks, so a respond() rejection still degrades to abort().
  • Cleanup is correct: the finally { await rm(tempDir, ...) } is removed and no orphan temp directory is created, while mkdtemp/rm remain live for the disposable userDataDir (no unused imports).
  • .changeset/done-verify-sandboxed-chrome.md is present with a patch bump for @open-codesign/desktop.
  • apps/desktop/src/main/harness-document.test.ts targets the exact regression mechanism: the navigation URL is asserted absent on disk and fulfilled via respond() with the generated body, and continue() is asserted never called. isHarnessDocumentRequest edge cases (…verify.html.bak, https:, non-URL) are covered.

Questions

  • I could not fetch issue #455 from the public context provided in this run, so I could not independently confirm its acceptance criteria. The PR body says Closes #455; can a maintainer confirm the issue is limited to the done verifier / preview harness document-load failure (no additional runtime paths such as title generation or export in scope)?

Summary

Review mode: initial

Directionally sound and ready to merge; no material issues found. The design explicitly trades an on-disk harness file (which was the failure source) for in-memory interception, and the tests plus the author's real-Chrome/bwrap checks are proportional to the risk.

Residual observations (non-blocking):

  • The harness now depends entirely on request interception, so a rare req.respond() failure on the navigation surfaces as ERR_FAILED/aborted rather than falling back to disk. This is an intentional consequence of removing the temp write and is covered by the same interception the allowlist already required — no action needed unless you want a clearer error message on that path.
  • isHarnessDocumentRequest compares fileURLToPath(new URL(rawUrl)) === documentPath as an exact string. This round-trips on the producing side, but if Windows CI exists it is worth confirming that drive-letter case normalization in Chrome's req.url() does not diverge from pathToFileURL(tmpdir()).
  • I could not verify #455's acceptance criteria in this run; the diff does touch the expected paths, so the closure claim is plausible.

Testing

Not run (automation). The added harness-document.test.ts is the right unit-level guard. If a Windows runner is available, add one assertion that isHarnessDocumentRequest returns true for a pathToFileURL(join(tmpdir(), ...)) round-trip to lock in cross-platform URL equality.

Open-CoDesign Bot

@Lxr-max

Lxr-max commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I checked #455 and the other Chrome entry points. No code change.

Scope of #455. The report is GENERATION_INCOMPLETE after done() verification, with both errors on file:///tmp/codesign-done-verify-…/verify.html (net::ERR_FILE_NOT_FOUND). That is the temp harness the verifier used to write and then open. Title generation does not load a document in Chrome. PDF and PPTX export render with page.setContent (packages/exporters/src/pdf.ts, pptx.ts, rendered-html.ts); their temp directories are Chrome profiles, and the page HTML is passed in memory. The preview tool was the other writer of a harness file under os.tmpdir() (preview.html) navigated to as file://, so Snap/Flatpak Chrome fails there for the same reason. Closes #455 matches that pair of loaders.

req.respond() on the navigation. Agreed that a failed respond() surfaces as an aborted load. Both handlers already call it inside the existing try, and a rejection still falls through to abort(). The allowlist needed interception before this change. I am leaving the error path as it is.

Windows drive-letter equality. PR CI is ubuntu-latest only (.github/workflows/ci.yml). isHarnessDocumentRequest compares fileURLToPath of the URL we just built with pathToFileURL against that same path. harness-document.test.ts already asserts that round-trip, and rejects a .bak suffix, an https: URL, and a non-URL. I do not have a Windows runner here to observe Chrome rewriting req.url(), so I am not adding a case-fold on top of the path we generate and pass to page.goto.

@Sun-sunshine06
Sun-sunshine06 merged commit 716437c into OpenCoworkAI:main Oct 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:desktop apps/desktop (Electron shell, renderer) docs Documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GENERATION_FAILED (fp: bcb438c0)

2 participants