Skip to content

playwright fixture: fixes from the adversarial review - #93

Open
MeharPro wants to merge 1 commit into
mainfrom
claude/wizardly-hypatia-713pwa
Open

MeharPro wants to merge 1 commit into
mainfrom
claude/wizardly-hypatia-713pwa

Conversation

@MeharPro

Copy link
Copy Markdown

What

Follow-up to #72, which merged with only its first commit; these fixes were pushed 40 minutes after the merge and never reached main. An adversarial review of the fixture (6 reviewers, then 3 skeptics per finding) upheld 54 of 55 findings, none blockers, 16 major. This applies them. All changes are in packages/playwright, plus one CI step, one AGENTS.md cell and the lockfile's engines line.

The ones that matter most:

  • Console noise. The fixture logged a console error and a failed request on every page load (the agent link's aborted hello), which fails any suite that checks for those. The agent link now gets a stand-in fetch for its own address and sends nothing, also on strict-CSP pages.
  • Chaos. It fetched responses from Node and held their delivery, which bypassed context.route and routeFromHAR mocks, buffered streams, and keyed requests by response arrival, so seeds did not reproduce. It now holds the request and then calls route.fallback(); keys are assigned when the page sends the request. reorder: false sends requests in order, at least 30 ms apart.
  • Detection confidence was the action's probability; it is now the model's diagnosis probability, taken from the runtime's decide event.
  • Mode. ?genclass=heal or localStorage.genclass could raise the fixture's runtime out of observe; it is now set back.
  • Lost or stale files. A retry overwrote the first attempt's file, and files from earlier runs could count as extra seeds. Retries get their own file (-r<n>), skipped tests write none, and the default output is the project's outputDir, which Playwright empties each run.
  • Unchecked runs looked clean. The race file gets a fixture record (model option, agent state, requests chaos held, service worker, mode override), and the report notes runs the fixture could not check. Files without that record are reported exactly as before.
  • Types and packaging. test is typed from @playwright/test, typesVersions for moduleResolution: node, engines ^20.19.0 || >=22.12.0 (CommonJS projects require the ESM-only fixture), and the README's config uses defineConfig<GenClassFixtures> (without it, TypeScript rejects use.genclass).
  • CI. Wider path filters, and the runtime-resolution unit test now runs where the runtime is built. main already has the pipefail fix for the e2e step. run.mjs now fails on other signatures on the buggy page, and the spec fails on any console error or failed request.
  • Docs. README sections on request-side chaos, routes and service workers, the globals the page gets, the e2e's script-tag mapping, per-test model download, and the compat numbers with their runtime version and the 270 injected-failure findings.

The commit message lists every change.

Checks

  • npm run build -w @genclass/runtime passes (no runtime source is touched; the e2e serves its dist/)
  • npm run typecheck -w @genclass/playwright, npm run build -w @genclass/playwright, cd packages/playwright && npx vitest run: 114 passed. The CI smoke run, which loads the reporter by module name, passes
  • no packages/runtime/test/review-*.test.ts was edited
  • no model-visible text changed (nothing under packages/runtime/src/)
  • docs/runtime/API.md, the runtime's src/types.ts and packages/runtime/CHANGELOG.md: not applicable (runtime untouched); the package's README, CHANGELOG and JSDoc are updated
  • a correct app is not made worse: the runtime stays in observe; chaos, off unless given, holds matched requests for at most delayMs (10 s cap) and never changes them
  • sim/ and demos/ are untouched
  • package-lock.json is in sync (one line: the package's engines); npm ci passes

Notes for the reviewer

Measured: packages/playwright/test/e2e/run.mjs, headless Chromium 153, Playwright 1.63.0, model: "skip". Six runs of five seeds on 2026-10-25 before rebasing onto main (30 seeds per row), and one more run after:

page chaos seeds with witness:overwrote-newer on GET /api/search other signatures console errors or failed requests
buggy latency 0 to 600 ms, reorder 30 of 30 0 0
buggy latency only (reorder: false) 0 of 30 0 0
fixed latency 0 to 600 ms, reorder 0 of 30 0 0

In a separate browser check, a context.route mock still answers under chaos, a default-src 'self' page logs nothing, and ?genclass=heal is set back to observe.

The one finding not upheld: the e2e summary's staleSeeds label (judged a naming choice).

Rebased onto main at 7f96182. Conflicts were in AGENTS.md (the new package rows on main were kept) and in the workflow (the pipefail comment from main was kept). Not run: the runtime's own suites (unchanged), test/smoke/smoke.sh, the install suite, and anything on Azure.


Generated by Claude Code

An adversarial review of f1938ca (6 reviewers, 3 skeptics per finding)
upheld 54 of 55 findings. This applies them:

- Chaos holds the request, then route.fallback(), instead of fetching the
  response from Node and holding its delivery. Matched requests now go
  through the test's own routes (context.route, routeFromHAR) and the
  browser's network stack (redirects, cookies, CORS, streams); nothing is
  buffered; no actionTimeout abort. Keys and delays are assigned when the
  page sends the request, so a seed reproduces whatever the server does;
  reorder:false sends requests in order, at least 30 ms apart. The default
  filter is decided from the request: same-origin /api* or Accept: json.
  Cross-origin keys drop the port.
- The agent link gets a stand-in fetch for its own address, so the fixture
  logs no console error, failed request or CSP report (it did on every
  page load). AGENT_URL matching no longer catches 127.0.0.1:9xxx apps.
- Detections come from the runtime's decide event with the model's
  diagnosis confidence (the sink record's p was the action's probability),
  synchronously, so none are lost at the end of a test.
- The runtime is held to observe when ?genclass= or localStorage raised it.
- The race file gets a fixture record (model option, agent state, chaos
  requests held, service worker, mode override); the report notes runs the
  fixture could not check and chaos that held nothing. The blanket state
  "off" exemption is gone: model-only files are reported as before.
- Retries write their own file (-r<n>); skipped tests write none; files go
  to the project's outputDir (emptied each run, so no stale seeds).
- Witness lines state the gap correctly; opOf drops per-run op ids.
- test is typed from @playwright/test; typesVersions for node10; engines
  ^20.19.0 || >=22.12.0; startGenClass takes a Page.
- CI: the e2e step runs with pipefail (it could not fail before), wider
  path filters, the runtime-resolution unit test where the runtime is
  built. run.mjs enforces no other signature on the buggy page; the spec
  fails on any console error or failed request.
- Docs: typed config, @beta install, request-side chaos, the globals the
  page gets, service workers and routes, the e2e's script-tag mapping,
  model download per test context, compat numbers with the runtime version
  and the 270 injected-failure findings.

Measured after the change: six e2e runs (30 seeds per row): buggy page
30 of 30, latency only 0 of 30, fixed page 0 of 30; no console errors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fjo9SusiHz3LvqdFnr5wYP

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants