Skip to content

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

Closed
lukemaj wants to merge 3 commits into
mainfrom
fix/323-linux-renewal-loader
Closed

lukemaj wants to merge 3 commits into
mainfrom
fix/323-linux-renewal-loader

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: Linux artifact proof passes; complete exact-head Sol/Grok review and CI, then merge under delegated authority. The user expanded delivery to the same main on cavallo, rocky, then bestia under #323, including #335.

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. Linux release executable validation, archive packaging and isolated version/serve smoke pass on fa9c72f75351746f7d9aece2ebf824f67f4a906c, recorded in the artifact proof. Server typecheck and targeted lint pass. Independent reviews and CI retain their exact-head gate. 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 the vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. label Oct 14, 2026
@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: Exact head fa9c72f75351746f7d9aece2ebf824f67f4a906c passes Linux executable import validation, full release archive construction and isolated archive smoke.
Why: The reader now uses filesystem-backed createRequire only at resource acquisition. Both static package ESM and dynamic package ESM remain rejected by the unchanged validator.
So what: Named independent Sol/Grok reviews and full CI remain the merge gates. Bestia has not restarted; post-restart steps will be recorded on #323 before activation.

Proof on this head, Node 26.8.2, pnpm 11.10.0, isolated task worktree /home/luke/dev/toolboxmd/chromeria-worktrees/323-linux-renewal-loader:

  • vp test run apps/server/src/auth/BrowserRenewalCommitments.test.ts scripts/lib/cli-external-packages.test.ts: 22 pass across 2 files. Baseline reader packaging test fails with expected ['playwright-core'] to deeply equal []; same test passes after fix, and its child process confirms no Playwright load until engine read, preserving typed loader failure.
  • tsc-rs --noEmit -p apps/server/tsconfig.json: exit 0. Targeted lint exit 0, with the pre-existing inline-schema warning outside the changed lines.
  • vp run --filter @t3tools/desktop --filter t3 build: passed.
  • node apps/server/scripts/cli.ts build-exe: passed its unchanged emitted-import validator.
  • 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: passed.
  • node scripts/smoke-cli-archive.ts --archive release-cli/t3-0.0.45-linux-x64.tar.gz --expect-version 0.0.45: passed version and isolated serve health on 47700, never live state.
  • Archive SHA-256 8e58f77506e4e83e6e0ee0ac33184a34842ead1080680a12e48bb738773f5aed; executable SHA-256 c77d95317cd86ffa816164ca9cc33f0982f939756e2ec63c69f6861ab1e12af2.

Raw proof retained in /home/luke/scratch/323-loader-{red,green-final,types,build}.log. Real browser execution remains outside this packaging proof. Post-merge main artifact identity will be verified separately before activation.

Requirements and who asked: User's focused test, Linux release validator proof, dual named exact-head reviews and CI, with no version bump.
Deleted: Dynamic package import, validator changes, version bump, real browser launch and treating focused checks as full CI.
Bottleneck: Artifact failure is cleared; exact-head independent review and CI remain.
Checked myself: Red/green output, source and full diff, successful typecheck/lint, actual executable validator run, archive smoke and both artifact digests.

@lukemaj
lukemaj marked this pull request as ready for review October 14, 2026 23:48
@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent Sol review found no blocking findings in PR #338 at fa9c72f75351746f7d9aece2ebf824f67f4a906c.
Why: Playwright now loads inside the existing resource-acquisition effect, preserving executable import validation and browser behavior.
So what: This review passes; the task owner still needs the other independent review and green full CI before delivery.

Reviewed exact base 9f46c833b19223e38a1dfb654d2735e54d47c1f7 to exact head fa9c72f75351746f7d9aece2ebf824f67f4a906c. Read the PR body, complete three-file diff, #323 acceptance, Linux failure, and diagnosis, using AgentsMD operations and its full review procedure. I did not author the implementation.

Findings: none requiring correction.

  • Loading and errors: BrowserRenewalCommitments.ts:13-14 creates a resolver without loading Playwright. The package require occurs only inside Effect.tryPromise at lines 74-93, when the engine read runs. Both synchronous resolution errors and asynchronous launch errors follow the existing BrowserCommitmentReadError mapping. The type query is erased. This matches the established ServerBrowserContexts.ts:12-15 boundary and introduces no dynamic package import.
  • Browser semantics: The existing CLI consumer still supplies the same layer. Launch options, sandbox, proxy, service-worker blocking, intercepted navigation, public-hash projection, private profile copying, schema validation and context cleanup are unchanged. No new client, provider or wire-contract behavior is introduced.
  • Packaging and faithful regression: The test at BrowserRenewalCommitments.test.ts:140-197 bundles the real engine with the actual release external-dependency policy, then applies the unchanged executable scanner. Its isolated Node resolution hook proves zero Playwright loads during module evaluation and a load attempt during read, failing before any browser launches. Static and dynamic external package imports are rejected by the scanner. The runtime external list, archive staging and validator remain unchanged; existing policy includes Playwright in the runtime dependency tree. This focused test does not itself execute a SEA or prove a real browser read.
  • Evidence inspected: Retained 323-loader-red.log shows the packaging assertion failing with ['playwright-core']; 323-loader-green-final.log records 22 passing cases across the two named suites. These are inspected author runs, not reviewer reruns. git diff --check passes. The completed 323-loader-build.log records successful Linux SEA validation, archive creation, and --version plus isolated serve smoke. Recorded SHA-256: archive 8e58f77506e4e83e6e0ee0ac33184a34842ead1080680a12e48bb738773f5aed, executable c77d95317cd86ffa816164ca9cc33f0982f939756e2ec63c69f6861ab1e12af2. Full CI remains pending and independent of this review verdict. No real browser, deployment, live-state write, implementation edit or test rerun was performed by this reviewer.

Requirements and who asked: User requested exact-head independent review of the Linux SEA loader repair for #323, with unchanged validator and existing browser semantics; restoring cross-machine access supports the current Objective.
Deleted: No design expansion, loader abstraction, validator relaxation, dynamic package import, version bump or implementation changes.
Bottleneck: The static Playwright import blocked Linux executable validation; the existing acquisition boundary now owns its filesystem-backed load.
Checked myself: Complete exact-commit diff, CLI caller, preview loader, release bundle policy, validator invocation, archive dependency staging, retained red/green/build evidence, and live PR base/head identities.

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 head above.

@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: #338 now integrates current main f23fad2d94a8832c744fc679948fefb1d5f3b11b, including #335, at exact head d41550966ae3953c957da96185f15906e939142c.
Why: The user's expanded fleet rollout requires one candidate that contains both bare launch and the Linux packaging fix, with current integration proof.
So what: Fresh exact-head Sol/Grok review and CI are required; old-head verdicts do not authorize merge. The merged-main deployment target remains unselected until that gate passes.

Main merged without conflicts. Both reader implementation/test blobs are unchanged from fa9c72f75351746f7d9aece2ebf824f67f4a906c; docs/map.md retains bare launch and adds PR #338 to paired renewal. The merge diff against current main still contains only those three files. The 22 focused cases pass again; Linux artifact proof is rebuilding with the combined candidate and its refreshed frozen lockfile. No force-push, version bump or deployment. Sol's old-head PASS remains recorded; Grok is finishing the old-head review, then a new delegated round will review this head.

Requirements and who asked: User added #335 in the same main deployed sequentially on cavallo, rocky, bestia, with dual exact-head reviews and CI retained.
Deleted: Deploying an old-base artifact or reusing old-head statuses.
Bottleneck: Combined-candidate review/CI and Linux artifact proof.
Checked myself: Live #335 merge, clean merge of main, unchanged reader blobs, three-file PR diff and 22 passing focused tests.

@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: Combined candidate d41550966ae3953c957da96185f15906e939142c, including #335, passes fresh Linux release proof.
Why: The integration must be proved on current main, rather than rely on the pre-#335 artifact.
So what: Fresh independent statuses and CI remain required on this exact head before delegated merge and fleet activation.

pnpm install --frozen-lockfile --offline, 22 focused tests, full server/web/desktop build, node apps/server/scripts/cli.ts build-exe with the unchanged validator, Linux release archive construction and isolated version/serve smoke all passed. Archive SHA-256 9b5772b3f2ab495d663e6a9e70c4352988d0c01242235580de89a609322eb8ba; executable SHA-256 fbb8fe63b6d4dd69c232098931cb101e02d4350a994cce44125cbdccbaa8a7b8. Logs /home/luke/scratch/323-loader-main-{install,tests,build}.log. No delivery profile declares additional deltas. No version bump, production server against live state, browser launch or activation.

Requirements and who asked: User's combined #335/fix rollout and current-candidate Linux validator proof.
Deleted: Earlier artifact as deployment proof and any validator relaxation.
Bottleneck: Fresh dual independent review and CI on the combined exact head.
Checked myself: Completed logs, 22 passing cases, actual validator invocation, archive smoke and both digests.

@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent Sol round 2 finds one blocking integration finding at d41550966ae3953c957da96185f15906e939142c; no loader implementation findings.
Why: Integrating main with a merge commit violates the fork's enforced merge-free stack and has already failed the exact-head Fork Stack Model check.
So what: Replace the merge integration with a linear integration onto current main, then obtain fresh exact-head proof and review; this candidate does not pass review/independent-sol.

Reviewed exact base f23fad2d94a8832c744fc679948fefb1d5f3b11b to exact head d41550966ae3953c957da96185f15906e939142c, using AgentsMD operations, verification and the full review procedure. Read the complete three-file diff, PR body, #323 acceptance, linked failure/diagnosis, prior Sol review and new integration record. I did not author the implementation. This is a fresh verdict, not a transferred old-head status.

Blocking finding

[P2] Remove the merge commit from the candidate's fork stack. Location: candidate commit d41550966ae3953c957da96185f15906e939142c and the invariant enforced by scripts/fork-check.sh:47-52. The candidate has two parents, including current main, and git log --merges f23fad2d94a8832c744fc679948fefb1d5f3b11b..d41550966ae3953c957da96185f15906e939142c identifies this merge. .github/workflows/fork.yml deliberately checks the PR head itself, rather than GitHub's synthetic merge ref, and requires this workflow alongside CI. The actual current-head job exited 1 with:

fork-check: FAIL: merge commits in the fork stack (rebase, do not merge):
d415509 Merge remote-tracking branch 'origin/main' into fix/323-linux-renewal-loader

The tree can be correct while this candidate remains ineligible under the project's rebase policy and green-CI requirement. Integrate the same repair linearly onto current main, preserving the three-file outcome and the validator. Do not weaken the gate. The resulting SHA requires its own review/status.

Cleared implementation and compatibility checks

  • Playwright is required only inside the existing Effect.tryPromise acquisition at BrowserRenewalCommitments.ts:74-94. Module evaluation creates a resolver without loading the package. The erased type query introduces no dynamic package import. Synchronous require failures and asynchronous launch failures retain BrowserCommitmentReadError; the existing acquire/use/release lifetime remains intact.
  • CLI browser renewal-commitments still supplies the same layer and scope. Launch options, sandbox, blocked service workers, proxy and routing, private profile copying, public hash projection, validation and cleanup are unchanged. Existing ServerBrowserContexts uses the same resolver pattern. Validator, release external policy, session authority and versions are unchanged by this PR.
  • I inspected the new-main integration beyond the matching reader blobs: feat(server): add bare launch with native receipts #335's contracts barrel adds bare-launch schemas, server dependencies/lockfile add @noble/hashes, and server route wiring adds the bare-launch layer. None changes the reader's call path, Playwright version (1.60.0), external-package policy, executable scanner or archive staging. The map retains both bare-launch edges and the PR fix(auth): load renewal reader Playwright safely in executables #338 renewal reference. No additional web/desktop/mobile entry-point, provider, connection-mode or wire-contract change is introduced by this repair.
  • The regression bundles the real engine with the release dependency predicates and scans its emitted imports. Its child-process resolution hook distinguishes module evaluation from actual engine acquisition and checks the preserved loader error without launching a browser. The retained initial red log fails on forbidden playwright-core. Current-candidate 323-loader-main-tests.log records 22 passing cases across the two focused suites. These are inspected author runs, not reviewer reruns; git diff --check on the exact base/head passed.
  • The completed current-candidate 323-loader-main-build.log now records successful full build, unchanged executable import validation, Linux archive construction and isolated version/serve smoke. Archive SHA-256: 9b5772b3f2ab495d663e6a9e70c4352988d0c01242235580de89a609322eb8ba; executable: fbb8fe63b6d4dd69c232098931cb101e02d4350a994cce44125cbdccbaa8a7b8. Frozen install log passes. Initial-candidate typecheck/lint and prior archive proof remain historical evidence, not complete current-head CI. Real browser reads and deployment were not exercised by this reviewer.

Requirements and who asked: User requested exact-head independent round 2 review of the #323 SEA loader microfix, including compatibility with #335 on current main and unchanged browser/security/packaging semantics; 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 loader packaging failure is cleared in inspected evidence, but merge integration now deterministically fails the fork-stack gate.
Checked myself: Exact live PR base/head, complete three-file diff, integrated caller/contracts/dependency changes, actual release scanner and staging, current red/green/build/install logs, merge ancestry and exact-head failed CI output.

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

@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: #339 replaces this PR with the same repair on a linear branch from current main.
Why: My integration merge violated the fork's merge-free stack gate, as independently confirmed by Sol. That finding is accepted. The implementation and test are unchanged; only the delivery map points to the replacement PR.
So what: Close #338 as superseded, retain its diagnosis and proof history, and require fresh exact-head Sol/Grok comments/statuses plus green CI on #339. No force-push, version bump or validator weakening. Fleet deployment remains on #323.

Requirements and who asked: User's no-force-push constraint, fork rebase policy, exact-head independent gates and combined #335 fleet rollout.
Deleted: The integration merge and reuse of old review statuses.
Bottleneck: Linear replacement exact-head proof/review/CI.
Checked myself: Sol's actual failed fork-check finding, conflict-free cherry-picks onto current main, identical reader blobs and the replacement PR.

@lukemaj lukemaj closed this Oct 14, 2026
@lukemaj

lukemaj commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

What: The renewal reader loads Playwright only when a commitment read runs, through the same lazy createRequire the preview browser already uses, and the Linux executable validator stays unchanged.
Why: #334's static playwright-core import is a file-backed ESM import, which the single-executable validator correctly rejects, and that rejection stopped the bestia artifact.
So what: No blocking finding on fa9c72f75351746f7d9aece2ebf824f67f4a906c. Full CI stays a separate gate. The PR tip moved during this review; this verdict does not cover that later SHA.

Requirements and who asked: #323 review brief: lazy createRequire at the reader's resource acquisition, validator unchanged, and a check of test faithfulness, eager load, typed errors, packaging, and existing browser semantics.
Deleted: Validator edits, dynamic import(), a new loader, a version bump, implementation changes, and review of the later main merge.
Bottleneck: The reader's static package import fails executable validation before bestia can be activated.
Checked myself: Diff 9f46c833b19223e38a1dfb654d2735e54d47c1f7..fa9c72f75351746f7d9aece2ebf824f67f4a906c, the validator, the preview loader, the CLI startup import, the preserved launch options, and the 01:48 dist-exe/bin.mjs plus /home/luke/scratch/323-loader-build.log.

Verdict

No required fix. The candidate does what #323 asked for this packaging failure.

Cleared

  • The validator is untouched. findEsmImportsOfExternalPackages still flags static imports and import() of non-builtins (scripts/lib/cli-executable-imports.ts). apps/server/scripts/cli.ts still scans dist-exe/bin.mjs and logs Built dist-exe/t3 only after that scan passes. The 01:48 bundle contains requirePlaywright$1("playwright-core") inside the read try and no import or import() of playwright-core. The log then records smoke --version and serve on port 47700. Digests printed there: t3 c77d95317cd86ffa816164ca9cc33f0982f939756e2ec63c69f6861ab1e12af2, archive 8e58f77506e4e83e6e0ee0ac33184a34842ead1080680a12e48bb738773f5aed.
  • Package load is lazy and matches preview. BrowserRenewalCommitments.ts:14 builds createRequire(import.meta.url) at init. BrowserRenewalCommitments.ts:76 calls require("playwright-core") only inside the acquire callback. ServerBrowserContexts.ts:13-15 uses the same split, and calls it from launch. bin.ts loads binCli.ts for serve and --version, and binCli.ts imports browserCommand, which imports this reader, so the smoked process initialized createRequire inside the executable.
  • Browser launch semantics are the base options: headless, chromiumSandbox: true, serviceWorkers: "block", the loopback proxy, and the same four Chromium args (BrowserRenewalCommitments.ts:79-90).
  • A failure to load Playwright stays a BrowserCommitmentReadError. The require sits in Effect.tryPromise, and the catch at line 93 wraps the cause. The new test's rejection check looks at error.cause.message, which holds only when that wrapper is the rejection.
  • The new test (BrowserRenewalCommitments.test.ts:140) bundles the real reader with shouldBundleCliDependency and isExternalCliDependency, scans that bundle with the real validator, and uses a child-process resolve hook to require zero Playwright loads on import and one load on read. Its pack call uses config: false, so it is the dependency policy and validator, while the 01:48 vp pack bundle is the release shape. Both agree. The reader file has 3 cases and scripts/lib/cli-external-packages.test.ts has 19, which is the reported 22. I did not re-execute them.
  • fa9c72f7 only makes the probe paths portable (fileURLToPath / pathToFileURL) and points docs/map.md at this PR. The diff has no version bump.

Unproven

  • These 22 cases remain the author's run. I did not re-execute them.
  • Full CI is still open.
  • This verdict is only fa9c72f75351746f7d9aece2ebf824f67f4a906c. The PR tip is now merge d41550966ae3953c957da96185f15906e939142c of origin/main f23fad2d94 (feat(server): add bare launch with native receipts #335). The renewal source, the new test, the validator, and the preview loader are identical there. docs/map.md only adds the bare-launch node. That merge is outside this review.

Reviewer did not author the implementation. Context: review/independent-grok.

@lukemaj
lukemaj deleted the fix/323-linux-renewal-loader branch October 15, 2026 00:09
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