fix(scripts): verify actual Chromium binary presence in check:playwright-browser-revision - #1965
Conversation
…ry presence (#312) The check could report ok:true even when the pinned Chromium revision had no launchable binary anywhere on disk. The "no root forced" branch skipped the disk entirely and unconditionally passed; the designated-container branch only compared directory names, so a same-named empty directory also passed. Both are now real, disk-verified checks against Playwright's own executable layout, reproduced live against this session's own container (PLAYWRIGHT_BROWSERS_PATH=/opt/pw-browsers with only chromium-1194 present while playwright-core@1.62.1 pins 1234 — the old check reported OK for that exact state). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLbw9qpfjv5CeNz6XpmteN
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLbw9qpfjv5CeNz6XpmteN
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24e8fd0409
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #10953 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Summary
check:playwright-browser-revision(ledger#312): the check could reportok: trueeven when the pinned Chromium revision had no launchable binary on disk anywhere. It now always resolves the effective browsers root (an explicitPLAYWRIGHT_BROWSERS_PATH, or Playwright's own default managed-cache directory when unset) and verifies a real executable exists for the pinned revision — not just a same-named directory — using an executable-layout table mirrored fromplaywright-core's ownEXECUTABLE_PATHS.ok: true, status: "managed-or-unconstrained". It now inspects the resolved root and returnsnot-installed/binary-missingwhen the pinned revision isn't actually present.container-aligned") branch only checked directory names. A same-named empty directory (partial/corrupt install) previously passed; it now requires a real, launchable file at the expected path for the current platform/architecture.PLAYWRIGHT_BROWSERS_PATH=/opt/pw-browserswas set withoutPLAYWRIGHT_SKIP_BROWSER_DOWNLOAD, the container only hadchromium-1194on disk, and the pinned revision (fromplaywright-core@1.62.1) is1234. The old check reported{"ok": true, "status": "managed-or-unconstrained", ...}for that exact state. The fixed check now correctly reportsnot-installedfor that same state, andcontainer-aligned/installedonly once a real1234binary is verified on disk.Verification
npm run verify:pr-local— completed:check:runtime,check:installed-lock-parity,format:changed,lint,typecheck,test(602 files / 6518 tests passed, 4 skipped),check:rag:fixtures,check:medication-interactions; build and offline RAG production contracts skipped (no build-affecting or RAG-scoped changes). The gate's final step,check:medication-lexicon-report, fails on this branch. This PR does not touch either input the check compares (data/medication-interaction-index.json,docs/medication-interaction-lexicon-review.md) —git statuswas clean apart from this PR's own two files when the check ran, and both inputs are byte-identical toorigin/main(both last touched by fix(medications): reach lithium, retire the dead z-drugs term, make lexicon review flags evidence-bearing #1923, unrelated to Playwright). Since the check is a deterministic function of those two files' content, this is a pre-existing drift onmain, not something this diff caused.tests/check-playwright-browser-revision.test.ts(10/10 passing, includes new regression tests for the exact false-OKscenario and for a same-named-but-empty revision directory) andtests/playwright-browser-preflight.test.ts(12/12 passing, unaffected — untouched file).verify:ui/verify:release(no UI, routing, or release-confidence change),eval:retrieval:quality/eval:rag(no retrieval/ranking/answer-generation change),check:production-readiness(no clinical/Supabase/deployment change).Risk and rollout
scripts/check-playwright-browser-revision.mjs(a standalone diagnostic script invoked manually vianpm run check:playwright-browser-revision, and imported only byscripts/playwright-browser-preflight.mjsfor the already-testedcontainer-revision-driftfail path) and its test file. Nothing in CI orverify:*invokescheck:playwright-browser-revisiondirectly today (confirmed by repo-wide search), so this makes an under-tested diagnostic accurate rather than changing any enforced gate's pass/fail outcome.RAG impact: N/A — no path under
src/lib/rag/**, retrieval, ranking, or the eval harness is touched.Clinical Governance Preflight: N/A — no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output is touched.
Note on solo scope: Running
classifyPullRequestFiles(['scripts/check-playwright-browser-revision.mjs', 'tests/check-playwright-browser-revision.test.ts'])fromscripts/pr-policy.mjsdirectly against this diff returns{ clinicalRisk: false, operationalRisk: false, ragRanking: false, ui: false }— neither file matches any pattern inoperationalRiskPatterns(that list is scoped to.github/workflows,package(-lock).json,next|playwright|vitest.config.*, andDockerfile/railway/nixpacksfiles, not arbitraryscripts//tests/paths). This PR is opened solo because it is a standalone ledger-sweep item from a freshorigin/mainbranch with no other currently-open, already-assembled PR in this session to bundle it with — not because the classifier flags it as risky.Notes
#312(docs/outstanding-issues.md) suggested, as a minimum fix, making the check "say plainly which browsers are present and which the locked Playwright version requires." This PR goes further, per the explicit task instructions: it verifies actual binary presence on disk rather than only directory-name/version-metadata comparison, so a falseOKis structurally impossible rather than just less likely to be misread.Generated by Claude Code