fix(e2e): refuse to serve a web bundle that was not built for E2E - #6243
shaurya703 wants to merge 1 commit into
Conversation
`pnpm build:web` exits 0 with a bundle that points at the wrong backend and has the E2E affordances compiled out. e2e-web-session.sh served whatever was in dist-web, so after a plain build:web every Playwright spec failed as though the product had regressed. e2e-web-build.sh now writes dist-web/.openhuman-e2e-bundle after its build, recording the VITE_* settings it compiled in. e2e-web-session.sh refuses to start without the marker, before launching the mock backend, and points at `pnpm test:e2e:web:build` — the same instruction the core binary check a few lines below gives. Vite empties dist-web on every build (emptyOutDir: true), so a plain build:web after an E2E build removes the marker. Both CI lanes build through test:e2e:web:build. ci-full.yml stages the bundle with `cp -a app/dist-web/.`, which keeps the dotfile, and its artifact cache key hashes e2e-web-build.sh, so no cached bundle from before the marker is restored. scripts/__tests__/e2e-web-bundle-guard.test.mjs runs the real scripts in a temporary tree with node, curl, pnpm, rustc and cargo stubbed. It covers an unmarked bundle refused before the mock backend starts, a marked one proceeding to the next precondition, the build writing the marker, a later plain build leaving none, and the Vite config still emptying dist-web. The scripts lane now also arms on both harness scripts and app/vite.config.*. Closes tinyhumansai#5920
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe web E2E build writes a bundle marker containing diagnostic settings. The session requires this marker before starting processes. New tests cover marker creation, refusal, removal, and Vite output behavior. CI now watches the related paths. ChangesWeb E2E bundle guard
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The harness now fails early when a non-E2E bundle is present, with clear rebuild guidance. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
A rabbit marks the bundle bright Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0092 · 130,794 in / 1,449 out · 11,464 cached (9%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash, z-ai/glm-5.2 · 488 embedded
critique: $0.0027 · 54,394 in / 532 out · 0 cached (0%) · deepseek/deepseek-v4-flash
security: $0.0052 · 52,532 in / 731 out · 11,464 cached (22%) · deepseek/deepseek-v4-flash, z-ai/glm-5.2
tests: $0.0008 · 16,222 in / 112 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0004 · 7,646 in / 74 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tinysweeper
Bot
added
the
priority: p3
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a7d28cf05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function run(tree, script) { | ||
| const env = { | ||
| ...process.env, |
There was a problem hiding this comment.
When the caller has a supported harness variable such as E2E_MOCK_PORT exported, spreading process.env passes that override into the real build script, but the marker test still hardcodes the default port 18473; for example, E2E_MOCK_PORT=29999 node --test scripts/__tests__/e2e-web-bundle-guard.test.mjs fails even though the generated marker is correct. Normalize or remove these E2E variables in the test environment so pnpm test:scripts does not depend on the invoking shell.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Needs a closer look
Three unresolved moderate issues affect marker correctness, rebuild behavior, and test determinism.
Pull request overview
Adds a marker-based guard preventing E2E from serving web bundles not built with E2E settings.
Changes:
- Marks E2E-built web bundles.
- Rejects unmarked bundles before session startup.
- Adds regression tests and expands CI script triggers.
File summaries
| File | Summary | Review findings |
|---|---|---|
scripts/__tests__/e2e-web-bundle-guard.test.mjs |
Tests marker creation, refusal, and staleness. | Moderate (1 vote): isolate inherited environment variables. |
app/scripts/e2e-web-session.sh |
Validates the bundle marker before startup. | Moderate (1 vote): use the workspace-qualified rebuild command. |
app/scripts/e2e-web-build.sh |
Writes E2E bundle metadata. | Moderate (1 vote): re-apply E2E variables after loading .env or conditionally write the marker. |
.github/workflows/ci-lite.yml |
Expands scripts-lane triggers. | No findings. |
Review details
Suppressed comments (3)
app/scripts/e2e-web-build.sh:43
- This marker is written unconditionally after the build, but
load-dotenv.shruns beforepnpm run build:weband can override the E2EVITE_*exports from lines 18–23. A local.envcontaining (for example)VITE_BACKEND_URLcan therefore produce a non-E2E bundle that still receives this marker, so the new session guard accepts exactly the class of bundle it is meant to reject. Re-apply the E2E build variables after loading.envand beforepnpm run build:web(asapp/scripts/e2e-build.sh:44-46does), or otherwise make the marker conditional on the actual E2E configuration.
cat >"$APP_DIR/dist-web/.openhuman-e2e-bundle" <<MARKER
VITE_BACKEND_URL=${VITE_BACKEND_URL}
VITE_OPENHUMAN_TARGET=${VITE_OPENHUMAN_TARGET}
VITE_OPENHUMAN_E2E_DEFAULT_CORE_MODE=${VITE_OPENHUMAN_E2E_DEFAULT_CORE_MODE}
VITE_OPENHUMAN_E2E_RESTART_APP_AS_RELOAD=${VITE_OPENHUMAN_E2E_RESTART_APP_AS_RELOAD}
VITE_OPENHUMAN_CORE_RPC_URL=${VITE_OPENHUMAN_CORE_RPC_URL}
VITE_CHAT_ATTACHMENTS=${VITE_CHAT_ATTACHMENTS}
app/scripts/e2e-web-session.sh:140
- When this script is invoked as documented from the repository root (
bash app/scripts/e2e-web-session.sh),pnpm test:e2e:web:buildresolves against the root package, which does not define that script; it is only inapp/package.json. The remediation therefore fails instead of rebuilding the bundle. Please print the workspace-qualified command.
echo "ERROR: $APP_DIR/dist-web was not built for E2E (no $(basename "$E2E_BUNDLE_MARKER")). Run pnpm test:e2e:web:build first; pnpm build:web alone omits the E2E backend and affordances." >&2
scripts/tests/e2e-web-bundle-guard.test.mjs:96
runinheritsCARGO_BIN,E2E_MOCK_PORT, andOPENHUMAN_CORE_PORTfrom the test process. A developer or CI job withCARGO_BINset can bypass the stub and run a real Cargo build, whileE2E_MOCK_PORTcan make the hard-coded127.0.0.1:18473assertion fail. Set these variables explicitly in the child environment (includingCARGO_BINto the stub path) so this self-test remains isolated and deterministic.
const env = {
...process.env,
PATH: `${tree.bin}:${process.env.PATH}`,
RUST_HOST_TRIPLE: "test-triple",
OPENHUMAN_WORKSPACE: path.join(tree.root, "workspace"),
E2E_WEB_CORE_TARGET_DIR: path.join(tree.root, "target"),
};
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
e2e-web-session.shserved whatever was indist-web, so after a plainpnpm build:webevery Playwright spec failed as though the product had regressed. It now refuses a bundle thattest:e2e:web:builddidn't produce, before starting anything.e2e-web-build.shmarks the bundles it builds withdist-web/.openhuman-e2e-bundle, recording theVITE_*settings compiled in.scripts/__tests__/e2e-web-bundle-guard.test.mjsruns both real scripts against stubbednode/curl/pnpm/rustc/cargo.scriptslane now arms on both harness scripts andapp/vite.config.*.Problem
#5920:
build:websets none of the variablese2e-web-build.shexports (VITE_BACKEND_URL,VITE_OPENHUMAN_E2E_DEFAULT_CORE_MODE,VITE_OPENHUMAN_CORE_RPC_URL, …). It produces a bundle pointed at the wrong backend with the E2E affordances compiled out, and exits 0. The session then served it without complaint.Solution
pnpm run build:webinsidee2e-web-build.sh, the only script that sets the E2E environment.Run pnpm test:e2e:web:build firstinstruction as the core-binary check a few lines further down.emptyOutDir: true, so any later plainbuild:webremoves the marker. One test pins that config, so the guarantee can't silently disappear.test:e2e:web:build.ci-full.ymlstages withcp -a app/dist-web/.and restores withcp -a repo/app/dist-web app/, both of which keep the dotfile. Its artifact cache key hashesapp/scripts/e2e-web-build.sh, so no bundle from before the marker is restored.VITE_BACKEND_URLagainstE2E_MOCK_PORT); that would be a separate decision.Submission Checklist
dist-web. Againstmain's scripts, the refusal, marker and plain-build cases fail; the other two pass by design.node --testchanges only, outside the Vitest / cargo-llvm-cov diff-cover inputs. Both changed paths in the scripts execute in the new tests.## Related— N/A: no feature behaviour changesCloses #NNNin the## RelatedsectionImpact
pnpm build:weband then the session now fails immediately with the fix spelled out, instead of producing a suite of misleading spec failures.e2e-web-build.sh.e2e-web-session.sh, and with fix(scripts): fail the json_rpc_e2e suite when test enumeration fails #6242.git merge-treeis clean pairwise and all three together; the combined tree keeps chore: validate refactor test suite after major refactor #6238's core resource monitor and this guard, and passesbash -n.Validation:
node --test scripts/__tests__/e2e-web-bundle-guard.test.mjs— 5/5node --test scripts/__tests__/*.test.mjs— the same 7 local failures as unmodifiedmain, none introducednode scripts/generate-test-inventory.mjs— 0 orphansAlready red on
main@1f58d88c5, so expected here too: Module Pin Gate, Rust Feature-Gate Smoke, and 4 module-pin registry tests in Scripts Self-Tests.Related
Summary by CodeRabbit
Bug Fixes
Tests