[byapparov]

Follow-up to merged PR #1. No linked issue; issue tracking is disabled for this fork.

Problem

A single-host installation test cannot reproduce a setup callback that returns to a different host from the application's OAuth cookie. Manually copying a Cookie header across hosts also hides that failure.

Behavior

Add a browser regression and reusable HTTPS fixture that exercise the emulator's OAuth and App installation pages, explicit repository selection, and configured setup_url across two loopback hosts on port 3005. The browser demonstrates rejection when the host-only cookie is absent, then successful completion after returning to the initiating host. The installation token can access only the selected repository.

Scope

• Verify opaque state, installation parameters and existing encoded/repeated callback query values.
• Keep TLS verification enabled using a generated CA trusted only by a temporary Firefox profile. Requests use browser-managed cookies; signed fixture state contains a hash of the cookie value.
• Add pnpm test:cross-host, pnpm demo:cross-host, documentation and CI browser prerequisites. Playwright is a development dependency.

Rollout and risks

Targets github-only after the installation flow merged in #1. The diff is limited to this browser regression and its supporting fixture, documentation and CI setup. The fixture illustrates the callback boundary; it does not run the production aictrl application or establish a complete hosted-client signup result.

Test plan

• pnpm test:cross-host: real Firefox browser proof passed (cookie absent: HTTP 400; cookie restored: HTTP 200).
• Installation-flow regression: 23 tests passed.
• Build, type check, lint and formatting passed; lint retains existing warnings.
• MJS syntax checks, actionlint and git diff --check passed.
• Independent review passed.
• GitHub-hosted CI passed, including the browser proof, full package tests, smoke tests and packaging: https://github.com/aictrl-dev/github-emulator/actions/runs/41720480264

[aictrl-dev]

Code review

Verdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 3 · ⚪ 1 · 0/4 resolved

• 🟡 .github/workflows/ci.yml:36 — CI re-downloads Playwright Firefox every run
• 🟡 docs/cross-host-installation.md:24 — Doc omits --with-deps that CI uses for Firefox
• ⚪ examples/cross-host-demo.mjs:47-51 — verifyState slices before guarding index < 0
• 🟡 examples/cross-host-demo.mjs:157-165 — JWT and HTTP helpers duplicated from demo.mjs

🤖 Fix all 4 open findings with your agent

Fix the following code review findings on aictrl-dev/github-emulator PR #2 (head branch).
Run the relevant tests/linters after each change.

1. .github/workflows/ci.yml:36 — CI re-downloads Playwright Firefox every run
   Detail: The new step `pnpm exec playwright install --with-deps firefox` downloads the full Firefox build plus apt dependencies on every CI run with no cache for the browser binaries. The job already caches pnpm's store (cache: pnpm), so the omission is inconsistent and adds a fresh multi-tens-of-MB download and apt resolution to every push and PR.
   Suggested fix: Cache the Playwright browsers directory before the install step, e.g. actions/cache@v4 with path ~/.cache/ms-playwright keyed on the playwright version from package.json (or pin PLAYWRIGHT_BROWSERS_PATH to a cached path).
2. docs/cross-host-installation.md:24 — Doc omits --with-deps that CI uses for Firefox
   Detail: The documented local prerequisite command is `pnpm exec playwright install firefox`, while the CI job added in this PR runs `pnpm exec playwright install --with-deps firefox` (.github/workflows/ci.yml:36). A user on the Debian/Ubuntu environment the doc itself targets (it instructs apt-installing libnss3-tools) who follows the doc verbatim can end up with a Firefox missing its system libraries and a failing browser launch. Doc and CI should state the same effective prerequisite.
   Suggested fix: Either change the documented command to `pnpm exec playwright install --with-deps firefox` or add a note that on Debian/Ubuntu the --with-deps flag (or the equivalent apt packages) is required, so local instructions match what CI does.
3. examples/cross-host-demo.mjs:47-51 — verifyState slices before guarding index < 0
   Detail: In verifyState, `state.slice(0, index)` and `state.slice(index + 1)` are computed before the `index < 0` check, so a dot-less state silently slices with -1 (dropping the last character). The current outcome is still safe because the MAC length comparison rejects the mismatch, but the validity guard should precede any use of the index so the invariant stays explicit and future edits to the comparison logic cannot mask an invalid index.
   Suggested fix: Move the guard to the top of verifyState: `const index = state.lastIndexOf("."); if (index < 0) return undefined;` before computing body/actual/expected, and drop `index < 0` from the combined condition.
4. examples/cross-host-demo.mjs:157-165 — JWT and HTTP helpers duplicated from demo.mjs
   Detail: startCrossHostDemo re-implements the RS256 App-JWT minting block, the generatedSecrets lookup, and the json/redirect response helpers that already exist in examples/demo.mjs (JWT: lines 140-141; secrets lookup: line 80; json/redirect: lines 29-36), and examples/lib/local-https.mjs re-implements demo.mjs's closeServer including the identical ERR_SERVER_NOT_RUNNING handling (demo.mjs lines 11-13 vs local-https.mjs close()). This PR itself introduces examples/lib/ as the shared-module location, so the copies will drift when the token flow changes.
   Suggested fix: Extract the App-JWT minting into examples/lib (e.g. mintAppJwt(key, appId)) plus the small json/redirect/closeServer helpers (e.g. examples/lib/http.mjs), and import them from both examples/demo.mjs and examples/cross-host-demo.mjs instead of keeping two hand-maintained copies.

📋 Out-of-diff findings (4)

Sev	Location	Finding
🟡	.github/workflows/ci.yml:36	CI re-downloads Playwright Firefox every run
🟡	docs/cross-host-installation.md:24	Doc omits --with-deps that CI uses for Firefox
⚪	examples/cross-host-demo.mjs:47-51	verifyState slices before guarding index < 0
🟡	examples/cross-host-demo.mjs:157-165	JWT and HTTP helpers duplicated from demo.mjs

Reviewed 12 files · 0 inline · view all 4 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback

---

aictrl · AI code review for fast-moving teams · aictrl.dev

[byapparov]

Review response for PR #2

Verified all four findings from this review. Fixed three in commit cf8da7a; declined the browser-cache suggestion.

Issues addressed (pushed to this PR)

• Firefox prerequisites: docs/cross-host-installation.md now uses pnpm exec playwright install --with-deps firefox, matching CI and installing the required system libraries. (commit cf8da7a)
• State separator guard: verifyState returns immediately when the separator is missing, before slicing or computing the MAC. The previous guard already rejected malformed state; this makes the order explicit. (commit cf8da7a)
• Duplicated demo helpers: both demos share App private-key lookup, RS256 JWT minting, JSON responses and redirects from examples/lib. HTTP and HTTPS fixtures also share server shutdown. (commit cf8da7a)

Review claims verified false (no change needed)

• Missing Firefox cache as a defect: fresh browser downloads are expected here. Playwright recommends against browser caching: cache restoration can take comparable time to downloading, and Linux system dependencies still need installation. The pnpm package cache serves a different purpose. There is no measured bottleneck supporting an additional cache in this PR.

Not addressed here

None deferred. Three FIX, one IGNORE.

Validation

Passed 266 package tests, the OAuth/App-token/webhook smoke test, and the real Firefox cross-host regression on port 3005. Build, type check, lint, formatting, MJS syntax and whitespace checks passed. Lint retains 17 existing warnings with zero errors. GitHub CI and CodeQL passed on cf8da7a. All four original review threads have been answered and resolved. No new automated review has been posted for this revision.

Verdict persistence: 4 matched, 4 written, 4 verified, 0 failed, 0 unrecorded.