Skip to content

fix(auth): load renewal reader Playwright safely in executables - #339

Merged
lukemaj merged 3 commits into
mainfrom
fix/323-linux-renewal-loader-linear
Oct 15, 2026
Merged

lukemaj merged 3 commits into
mainfrom
fix/323-linux-renewal-loader-linear

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

What: Linux releases load the headless renewal reader's Playwright dependency only when the reader runs.
Why: #334 introduced a static package import into the Node single-executable, which cannot resolve file-backed ESM imports and correctly fails the executable validator.
So what: Merged as 9ab53882e87a9e0a6413f74d8b313ce516721d40 after exact-head Sol/Grok success and green CI. The authorized fleet rollout with #335 is tracked on #323, cavallo then rocky then bestia.

Refs #323, follow-up to #334. This preserves cross-machine access for the current Objective. The reader uses the same lazy createRequire pattern as the existing preview browser. Dynamic import() would also fail inside Node SEA. The validator and packaging policy are unchanged.

Focused red/green: the reader packaging regression fails on 9f46c833b1 with forbidden playwright-core, then passes with the fix. The test bundles the real reader with release dependency policy, runs the unchanged executable validator, and uses an isolated Node loader hook to prove evaluation does not load Playwright while invoking the reader does, with the existing typed error. No browser launches. Reader and packaging tests: 22 passing cases across 2 files. Prior Linux executable validation, archive packaging and isolated version/serve smoke passed on the identical integrated loader tree. Current-head artifact proof is recorded here. Server typecheck and targeted lint pass. Full CI and both Sol and Grok reviews pass on c89652e3283cbaa18ebff018b77ace76befb083c. Each named status followed its findings comment. No version bump.

Upstream main/Issues/PRs searched: no renewal-specific port available; existing upstream preview boundary already uses createRequire.
Website impact: none, packaging-only repair with unchanged supported commands and session protocol. Frontend entry points, mobile, providers and wire contracts are unchanged.

Requirements and who asked: User requires focused regression proof, Linux release validator success, exact-head independent Sol/Grok success after findings comments, green CI, delegated merge and fleet deployment under #323.
Deleted: Dynamic package import, validator loosening, version bump, new loader abstraction, GUI verification and extra restarts.
Bottleneck: The reader's static dependency prevents Linux artifact activation; defer loading at its existing resource-acquisition boundary.
Checked myself: Failure record and #334 full diff/reviews, release validator mechanics, existing preview loader, failing-before packaging test and 22 passing focused cases.

Implementation: GPT-6.1 Sol (gpt-6.1-sol) through Codex in T3 Code.

@github-actions github-actions Bot added size:S vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Oct 14, 2026
@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: Exact linear head c89652e3283cbaa18ebff018b77ace76befb083c passes focused regression proof and fresh Linux release validation/archive smoke.
Why: This replaces #338's invalid merge topology without changing the loader repair or test. Current main includes #335.
So what: Both independent reviewers and full CI must pass this exact head before delegated merge; then pin one merged main for cavallo, rocky, bestia in that order.

Base f23fad2d94a8832c744fc679948fefb1d5f3b11b. No merges in origin/main..HEAD. Compared with rejected d415, only docs/map.md's PR reference differs. The reader implementation/test retain their cleared bytes. Original baseline regression failed because the unchanged import scanner found playwright-core; the new child probe additionally proves no Playwright resolution at evaluation and an actual load attempt with the existing typed error at read.

Current commands/results:

  • vp test run apps/server/src/auth/BrowserRenewalCommitments.test.ts scripts/lib/cli-external-packages.test.ts: 22 passed across 2 files, /home/luke/scratch/323-loader-linear-tests.log.
  • Frozen install with current main lockfile passed before the integration build. Reader files unchanged since successful server typecheck and scoped lint; complete current CI retains those gates.
  • vp run --filter @t3tools/desktop --filter t3 build, node apps/server/scripts/cli.ts build-exe, node scripts/build-cli-archive.ts --platform linux --arch x64 --version 0.0.45 --resource-monitor-dir /home/luke/t3-backups/upstream-runtime-0.0.45/resource-monitor, and node scripts/smoke-cli-archive.ts --archive release-cli/t3-0.0.45-linux-x64.tar.gz --expect-version 0.0.45: all passed on this exact head, /home/luke/scratch/323-loader-linear-build.log. The build-exe command ran the unchanged emitted-import validator. Smoke served isolated state on47700 and checked version/health.
  • Archive SHA-256 ba84f652d67493716522dad5ac4908c35138413a9b3d70f6a25938dc097cb5f3; executable SHA-256 fbb8fe63b6d4dd69c232098931cb101e02d4350a994cce44125cbdccbaa8a7b8.
  • git diff --check origin/main...HEAD: passed. No version bump, validator relaxation, browser launch or live activation. No delivery profile deltas.

Sol's prior integration objection was correct and is accepted: #338's merge commit failed the fork-stack gate. This candidate is linear; fresh statuses are required, not copied from an old head. Grok's original #338 round was cancelled as superseded before verdict; its old pending status was closed as error. A fresh Grok child reviews this candidate. #323 owns the authorized fleet deployment and approval work.

Requirements and who asked: User requires executable-safe renewal reading, focused tests, Linux artifact validation, exact-head Sol/Grok comments then success statuses and green CI, delegated merge, no version bump/force-push, and one combined #335 fleet rollout.
Deleted: Integration merge, dynamic package import, validator weakening, version bump, earlier-head approval reuse and extra fleet restarts.
Bottleneck: Current-head independent reviews and CI; all local Linux artifact proof now passes.
Checked myself: Exact linear topology, full three-file diff, identical reader blobs, red/green test output, current Linux build/validator/archive/smoke and both digests.

@lukemaj

lukemaj commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent Sol round 3 passes PR #339 at c89652e3283cbaa18ebff018b77ace76befb083c with no blocking findings.
Why: The loader repair preserves the existing acquisition and browser semantics, and the replacement's linear history clears round 2's fork-stack failure.
So what: This exact head passes review/independent-sol; the task owner still needs the other independent review and green full CI before merge and the authorized #323 delivery.

Reviewed exact base f23fad2d94a8832c744fc679948fefb1d5f3b11b to exact candidate c89652e3283cbaa18ebff018b77ace76befb083c, using AgentsMD operations, verification and the full code-review procedure. Read the PR body, complete three-file diff, #323 acceptance, linked failure and diagnosis, prior reviews and the current integration/proof record. I did not author this implementation. This is a fresh review and status for the replacement candidate.

Findings: none requiring correction.

  • Prior blocker resolved: The actual commit-parent chain is f23fad2d94 -> 343a3871df -> dd91051c8a -> c89652e328. git rev-list --merges <base>..<head> is empty. I inspected the exact-head Fork Stack Model run, whose recorded head is the full candidate above. It checks the PR head rather than GitHub's synthetic merge ref and passed the actual upstream-base fork gate: 85 stack commits, fork-features: OK (38 features), fork-check: OK. The enforcement script and workflow remain unchanged. The round 2 finding is cleared by evidence, not by transferring its old status.
  • Loading, errors and lifetime: BrowserRenewalCommitments.ts:13-14 creates a resolver without loading Playwright. The package require is inside the existing Effect.tryPromise acquisition at lines 74-94. Synchronous loader failures and asynchronous launch failures retain BrowserCommitmentReadError. The type query is erased, with no dynamic package import. The existing acquire/use/release lifecycle and context cleanup are preserved, matching the established preview loader boundary.
  • Behavior and security invariants: The CLI consumer still provides the same engine layer and scope. Launch options, sandbox, blocked service workers, proxy and navigation routing, private profile copy, public hash projection and schema validation are unchanged. The three-file diff does not change the executable scanner, release external policy, archive staging, session authority, contracts or versions. No additional web/desktop/mobile entry point, provider or connection-mode behavior is introduced.
  • New-main integration inspected: I traced the unchanged CLI reader path and inspected feat(server): add bare launch with native receipts #335's surrounding changes: the contracts barrel now exports bare-launch schemas, server dependencies/lockfile add @noble/hashes, server MCP wiring adds the bare-launch layer, and WebSocket handlers pass bare-read grants for launch/scheduling. These changes do not alter renewal reader acquisition or packaging. Playwright remains 1.60.0, and the unchanged external policy still stages it on disk. The map retains the new-main additions and links renewal delivery to PR fix(auth): load renewal reader Playwright safely in executables #339. The complete tree difference from rejected d41550966ae3953c957da96185f15906e939142c is only that map PR reference; compatibility was assessed from the consumers and packaging paths as well as this comparison.
  • Faithful regression and evidence: The test bundles the real reader with the actual release dependency predicates and applies the unchanged emitted-import scanner. Its isolated Node resolution hook distinguishes module evaluation from engine acquisition, confirms the preserved loader error, and stops before browser launch. I read the retained red log's forbidden playwright-core assertion and current 323-loader-linear-tests.log: 22 cases pass across the reader and CLI external-package suites. These are inspected author runs, not reviewer reruns. Exact base/head git diff --check passes. Frozen install with the integrated lockfile passed; the log records the host Node 26 versus project Node 24 engine warning. Earlier server typecheck/scoped lint are historical evidence, not a substitute for current full CI.
  • Current Linux artifact proof: I inspected completed 323-loader-linear-build.log, the unchanged validator invocation before the build-success message, archive staging and isolated version/serve smoke. I independently read both artifact SHA-256 values and matched the retained log: archive ba84f652d67493716522dad5ac4908c35138413a9b3d70f6a25938dc097cb5f3; executable fbb8fe63b6d4dd69c232098931cb101e02d4350a994cce44125cbdccbaa8a7b8. The old-head archive was not used as current proof. Full CI was still pending when inspected and remains a separate gate. Real browser reads and fleet deployment were not exercised by this reviewer.

Requirements and who asked: User requested exact-head independent round 3 review of the #323 SEA loader microfix, linear replacement topology and compatibility with #335 on current main, preserving existing browser/security/packaging behavior; reliable cross-machine access supports the current Objective.
Deleted: No design expansion, dynamic package import, validator relaxation, version bump, source/test edits, browser execution, deployment or old-status reuse.
Bottleneck: The static import and invalid merge topology are cleared in inspected evidence; the remaining delivery gates are the other exact-head review and full CI.
Checked myself: Live exact PR base/head, complete diff and parent chain, caller/contracts/dependency/route integration, actual fork-gate run and output, scanner and archive policy, retained red/green/install/current-build evidence and both current artifact digests.

Reviewer: GPT-6.1 Sol (gpt-6.1-sol), high reasoning effort, Codex harness in T3 Code. Verdict: PASS for review/independent-sol on the exact full candidate above.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB +31 B (+0.6%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB +31 B (+2.6%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB +41 B (+0.2%) 29.3 KiB ✅
Claude Live turn messages 1 2 +1 (+100.0%) 8 ✅

Baseline: f23fad2 · PR result: c89652e · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj

lukemaj commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

What: Exact head c89652e3283cbaa18ebff018b77ace76befb083c has no blocking finding. The renewal reader loads Playwright through lazy createRequire inside the existing read, and the three commits are linear on f23fad2d94a8832c744fc679948fefb1d5f3b11b.
Why: The static playwright-core import from #334 is gone from the reader and from the emitted Linux bundle, which is what the executable validator rejected.
So what: This review records review/independent-grok success on that full SHA. Merge and the #323 fleet rollout stay with the planner. No source change comes out of this review.

Reviewed only PR #339 at c89652e3283cbaa18ebff018b77ace76befb083c against f23fad2d94a8832c744fc679948fefb1d5f3b11b, using git show and git diff in the author worktree. No checkout, reset, or source edit.

Findings

No blocking finding. No optional change.

Cleared

  • Topology. Parents are 343a3871df72a1212b7982eb62ebb2f6dad9e6a7, dd91051c8a4c60c260080a8413a0b9c31059d12a, then c89652e. The first parent is the review base. git rev-list --merges f23fad2..c89652e is empty. scripts/fork-check.sh at this HEAD printed fork-check: OK against upstream merge-base 8d11fe1261. The reader files and docs/map.md are new fork files. The earlier merge-commit failure does not exist on this candidate.
  • Loader. The value import is gone. createRequire(import.meta.url) is created at module init, and require("playwright-core") runs only inside the existing Effect.tryPromise try. The catch still returns BrowserCommitmentReadError. The type assertion is erased. Emitted apps/server/dist-exe/bin.mjs line 82169 is that same call inside the launch callback. findEsmImportsOfExternalPackages on that file returned []. There is no import() of playwright-core.
  • Preserved read behavior. Launch options are unchanged: executablePath, headless, sandbox, service workers blocked, loopback-bypass proxy, and the four Chromium args. The route still fulfills only the paired origin navigation and aborts every other request. Hashing still goes through t3RenewalHash, and read still copies IndexedDB and QuotaManager into a private temp profile before launch.
  • feat(server): add bare launch with native receipts #335 integration. Bare launch is provider session launch. Its 121 files do not touch apps/server/src/auth, preview, the import scanner, the external-package policy, or apps/server/scripts/cli.ts. sessionRenewal.ts and ServerBrowserContexts.ts have the same blobs at 9f46c833 and f23fad2. The preview loader still uses the same deferred createRequire. browser renewal-commitments still calls BrowserRenewalCommitments.read and provides its layer. The built bundle's scanner is empty, so the new @noble/hashes dependency was inlined rather than left as a file-backed import.
  • Unchanged policy. The diff is the reader, its test, and the map pointer to PR fix(auth): load renewal reader Playwright safely in executables #339. No validator, external-package list, session type, or version file changes.

Proof

  • Focused log /home/luke/scratch/323-loader-linear-tests.log: 22 passed across 2 files, started 01:56:54 +0200. This HEAD has 3 reader cases plus 19 external-package cases, including keeps the executable reader importable without Playwright and loads it only on read. I did not re-run the suite. The log does not print the SHA. The commit time is 01:56:50 +0200 and the tracked worktree is clean.
  • Linux log /home/luke/scratch/323-loader-linear-build.log: build-exe reached its success line, which is after the unchanged scanner on dist-exe/bin.mjs. Archive and executable hashes on disk match the log: archive ba84f652d67493716522dad5ac4908c35138413a9b3d70f6a25938dc097cb5f3, executable fbb8fe63b6d4dd69c232098931cb101e02d4350a994cce44125cbdccbaa8a7b8. bin.mjs mtime is 01:58:13 +0200. Smoke checked --version and serve on port 47700. No browser profile was read.
  • GitHub checks on this PR are green, including Build, Test, Typecheck, Lint, Fork Stack Model, and Release Smoke. I read the check rollup, not every job log.

Read-time resolution of playwright-core inside the SEA binary was not executed. The call matches the preview loader that already ships that external package beside the executable.

Requirements and who asked: The user asked for an exact-head review of PR #339 at c89652e against f23fad2, preserving typed errors, launch options, private copy, network blocking, and hash-only output, with no dynamic import, validator change, policy change, session-authority change, or version bump, and a fresh fork-gate check after the merge commit was removed.
Deleted: Review of superseded #338, reuse of an older status, a second test run, a browser launch, and any source edit.
Bottleneck: The candidate had to be linear, and #335's bare launch had to leave the reader's loader and safety behavior intact.
Checked myself: The three-file diff, scripts/fork-check.sh, the #335 path overlap, the emitted bundle scanner, the require inside the launch try, and the artifact hashes against the linear build log.

@lukemaj
lukemaj marked this pull request as ready for review October 15, 2026 00:08
@lukemaj
lukemaj merged commit 9ab5388 into main Oct 15, 2026
31 checks passed
@lukemaj
lukemaj deleted the fix/323-linux-renewal-loader-linear branch October 15, 2026 00:09
lukemaj added a commit that referenced this pull request Oct 29, 2026
* fix(auth): load renewal reader Playwright through executable-safe require

* test(auth): keep renewal packaging probe portable and link delivery

* docs: link linear renewal packaging delivery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 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