fix(playwright): document video-start --size and correct the frame-size guidance - #1586
Conversation
…ze guidance A bare `playwright-cli video-start <name>.webm` — the skill's canonical example — records at 800x450 regardless of viewport intent, because the video frame size is derived from the viewport at browser-context creation and then fitted into an 800x800 box. `resize` afterwards does not change it. The `--size` flag that fixes this appeared nowhere in the skill. - Add a "Frame size — two levers, not one" section to reference/tracing-and-video.md: PLAYWRIGHT_MCP_VIEWPORT_SIZE on `open` governs what the page renders at, `video-start --size` governs the output frame, and a correct recording needs both matched. Tabulates the measured outcome of every partial combination. - Make the canonical video example carry both levers, at a neutral illustrative resolution. - Add a video-recording exception to SKILL.md "Defaults (accept, don't override)". The 1280x720 viewport row is correct and stays; what was missing was the carve-out. Frontmatter untouched. - Drop the "1280x720 WebM is ~5 MB/minute" cost claim: the CLI never emits 1280x720 by default and the figure is unsourced. Verified against @playwright/cli 0.1.17 help.json (the pinned version), playwright.dev recordVideo.size and Videos docs, and an ffprobe-measured five-scenario repro. Closes #1575 Closes #1576 Closes #1577 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 2s —— View job PR Review —
|
|
Claude finished @kyle-sexton's task in 1m 41s —— View job Security Review — PR #1586
SummaryThis is a documentation-only PR. The four changed files are FindingsNo CRITICAL or IMPORTANT security findings. One low-confidence observation worth noting for completeness: SUGGESTION — Confidence: LOW
The table shows the env-var lever as: without quotes around This is not a regression — the same unquoted-template pattern already exists elsewhere in the skill ( Areas checked and found clean
|
![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: c3d62e4395
ℹ️ 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".
…ell env syntax MD051 flagged both `#frame-size-two-levers-not-one` links as invalid: an em dash in a heading slugs to a DOUBLE hyphen, so `## Frame size — two levers, not one` anchors as `#frame-size--two-levers-not-one`. Rename the heading to the parenthetical form, which strips to single hyphens and matches the links already in place — the same convention codebase-health's `## Remediation (delegated to other plugins)` uses. Also note that the `VAR=value <command>` env prefix in the new example is POSIX-only, with the PowerShell `$env:` equivalent, since this skill ships a Windows quirks reference and Windows consumers are a known audience. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 58s —— View job Security Review — PR #1586
SummaryThis is a documentation-only PR. All four changed files are skill prose, changelog, and a manifest version bump — no executable code, no GitHub Actions modifications, no new dependencies, no remote trust delegations. The security surface is minimal. FindingsNo CRITICAL or IMPORTANT findings. The one observation worth logging for completeness: SUGGESTION — Confidence: LOW
The lever table shows the env-var form as: without quotes around the substitution site. The concrete examples ( This pattern is not new to this PR — it matches the pre-existing Areas checked and found clean
|
|
Claude finished @kyle-sexton's task in 2m 3s —— View job PR Review —
|
| Snippet | Line | Missing |
|---|---|---|
playwright-cli video-chapter ... |
54 | -s=demo |
playwright-cli video-show-actions ... |
59 | -s=demo |
playwright-cli click e1 (in show-actions block) |
60 | -s=demo |
playwright-cli fill e2 "test" |
61 | -s=demo |
playwright-cli video-hide-actions |
62 | -s=demo |
These were pre-existing omissions, but this PR made them consequential by naming the session above. Fix this →
2. Placeholder casing inconsistency — cosmetic
The lever table (line 81–82) uses <W>x<H> (uppercase). The outcomes table row at line 96 uses resize <w> <h> (lowercase). resize <W> <H> would match the convention above.
3. "both levers, matched" outcome row — minor
The outcomes table shows "the size you asked for" rather than a concrete measurement. Every other row gives a specific pixel result; a closing row like "1920×1200 — correct" would make the payoff explicit and parallel the other rows.
CLAUDE.md compliance
- Fresh-docs mandate: This change is confined to skill prose bodies with frontmatter untouched — outside scope. Load-bearing claims cited to
help.json, upstream README, and playwright.dev. ✓ - Repo-agnostic:
1440x900presented as illustrative; docs teach the pattern. ✓ - Versioned:
plugin.jsonbumped + changelog entry. ✓ - No PII / secrets: Documentation only. ✓
Security
Documentation-only change. No code execution paths, no new trust delegations, no secrets introduced. No security concerns.
Verdict
LGTM with one request to address before merge: the session selector gap (finding 1) is a functional issue introduced by this PR that would break annotation snippets for agents following the newly-named demo flow. Findings 2 and 3 are cosmetic and non-blocking.
Review catch. Naming the session in the basic video example left the immediately following `video-chapter` and `video-show-actions` snippets on the bare (unnamed) session, so a reader copying them in sequence would target a different daemon than the one recording — the exact isolation failure `reference/sessions.md` warns about, and contrary to this skill's own "always use named sessions for multi-step work" convention. The tracing example above is left bare: it is pre-existing, internally self-consistent, and untouched by this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 37s —— View job PR Review —
|
| Commit | What changed |
|---|---|
c3d62e4 |
Core fix: --size documented, canonical example updated, SKILL.md carve-out added, unsourced cost figure removed |
61d99f0 |
Repairs: anchor fragment, PowerShell env syntax note |
6a496c6 |
Carries -s=demo through video-chapter, video-show-actions, click e1, fill e2, video-hide-actions snippets |
plugin.json — Version bump 0.4.0 → 0.5.0
Correct. Precedent from this plugin's own changelog: 0.3.x = doc corrections → patch; 0.4.0 = newly-surfaced upstream flags → minor. Surfacing --size (a previously undocumented upstream flag) matches that pattern exactly. permalink
CHANGELOG.md
Correct Keep a Changelog format (Added / Changed / Fixed). All three entries accurately describe what changed and why. Consistent with earlier versions. permalink
SKILL.md — Video-recording exception paragraph
The carve-out at line 78 is well-placed and correct:
- Inserted directly after the Defaults table — the exact point a reader first encounters "don't override" guidance
- Accurately describes the two-lever mechanism and why a bare
video-startrecords at 800×450 - The "per-command prefix, not a project-settings entry" clarification resolves the apparent contradiction with the surrounding guidance
- Cross-link uses the correct relative path from
SKILL.md's location ✓ - Frontmatter untouched ✓
reference/tracing-and-video.md — Main change
Session selector fix (commit 6a496c6) — fully correct. Every video command snippet in the file now carries -s=demo, matching the named session opened in the basic example. Previous state left video-chapter, video-show-actions, click, fill, and video-hide-actions targeting the unnamed session — copying them in sequence would have dispatched to a different daemon than the active recording. All five lines now consistent. permalink
Basic video example (lines 40–46): Fixed on all three dimensions — session name, viewport env var, --size. Internally consistent with SKILL.md Quick Start pattern.
Frame size section (lines 68–108): Clear structure and honest attribution. Measured outcomes table is versioned to @playwright/cli 0.1.14. PowerShell note is a useful addition. saveVideo correctly marked unverified.
Workflow checklist step 3 (lines 127–128): Updated correctly with --size and cross-reference.
Known costs (lines 136–138): Removing the unsourced ~5 MB/minute figure and replacing with a proportionality statement is the right call.
Remaining cosmetic observations (non-blocking, pre-existing)
1. Placeholder casing in outcomes table — line 96: resize <w> <h> uses lowercase while the lever table above uses <W>x<H>. resize <W> <H> would be consistent.
2. "both levers, matched" row — line 99: All other rows show a specific measured pixel result; this row says "the size you asked for". A concrete value like 1920×1200 — correct would parallel the other rows.
Neither requires action before merge.
CLAUDE.md compliance
- Fresh-docs mandate: Skill prose bodies with frontmatter untouched — outside scope. Load-bearing claims cited to
help.json, upstream README, and playwright.dev. ✓ - Repo-agnostic:
1440x900is illustrative; the docs teach the pattern. ✓ - Versioned:
plugin.jsonbumped + changelog entry. ✓ - No PII / secrets: Documentation only. ✓
Security
Documentation-only change. No code execution paths, no remote trust delegations, no secrets introduced. No security concerns.
Verdict
LGTM. The blocking issue from the previous review (session selector gap in annotation snippets) is fully resolved in 6a496c6. The substantive fix — teaching agents the two-lever pattern required to get the video frame size they intend — is correct, well-evidenced, and appropriately scoped.
|
Claude finished @kyle-sexton's task in 1m 31s —— View job Security Review — PR #1586 (head:
|
| Addition | Assessment |
|---|---|
PowerShell env-var guidance ($env:PLAYWRIGHT_MCP_VIEWPORT_SIZE = '<W>x<H>') |
Single-quoted literal; correct template form for PS — no injection vector |
External links (playwright.dev/docs/videos, playwright.dev/docs/api/class-browser) |
Legitimate upstream documentation URLs |
saveVideo config note |
Correctly flagged unverified; no trust delegation |
-s=demo session selectors (head commit) |
Hardcoded literal; no user-controlled input path |
Areas checked and found clean
| Area | Verdict |
|---|---|
| Secret / credential exposure | None introduced |
| Command injection via shell patterns | No new vectors; unquoted env-var template is pre-existing and low-risk |
| Path traversal | No user-controlled paths introduced |
| GitHub Actions (workflow files) | No files changed |
| Supply-chain / dependency pins | plugin.json version bump only; no new packages |
| Trust delegation (remote MCP, external scripts) | None introduced |
| Logic / trust-boundary flaws in skill instructions | None; saveVideo path correctly marked unverified; resize non-effect correctly documented |
1 participant
Closes #1575
Closes #1576
Closes #1577
The problem
The
playwrightskill's canonical video example is a bareplaywright-cli video-start demo.webm. That deterministically produces an 800x450 file. Anoperator following the skill verbatim — including setting a large viewport with
resize— gets800x450 recordings every time, and nothing in the skill explains why or how to change it. The
downstream report was nine E2E screen recordings, all 800x450, after a 1920x1200
resize.Which fix is correct, and why (stated explicitly)
The task framing offers two valid resolutions: make the guarantee true, or stop claiming it.
--sizedoes exist upstream — verified inhelp.jsonextracted from@playwright/cli@0.1.17,the exact version this skill's frontmatter pins:
So the guarantee is deliverable, and this PR makes it true rather than retracting it. Worth being
explicit about what "behavior" means for a skill: this skill's behavior is the commands it
instructs an agent to run. Changing the canonical example to pass
--sizeis therefore thebehavior fix, not a prose paper-over of a broken one. There is no separate code path left unfixed.
The one place I did choose retraction over repair is the cost figure — see defect 3.
Verdicts on the three reported defects
1.
--sizeundocumented — REAL (#1575)Confirmed three ways: the installed CLI's
--help,help.jsonfrom the pinned 0.1.17, and anffprobe-measured repro. The string
--sizeappeared nowhere in the skill.2. "Defaults (accept, don't override)" steers wrong — REAL, but NARROWER than reported
The report framed this as the section contradicting the fix. On inspection it is more precisely an
omission, and two sub-claims do not survive:
| Viewport | 1280x720 |row is factually correct — that is the CLI's default viewport, perthe
@playwright/cli@0.1.17README (PLAYWRIGHT_MCP_VIEWPORT_SIZE— "specify browser viewport sizein pixels, for example "1280x720""). It is not a defect and it stays.
PLAYWRIGHT_MCP_*env vars to project settings". The video fix is aper-command env prefix on
open, which that sentence does not forbid. Not a contradiction.What was genuinely missing is a documented exception: an agent reading "accept, don't override" next
to a 1280x720 row reasonably concludes the viewport is not a knob to touch. So the fix here is one
added carve-out paragraph with a cross-link — not the section rework the report suggested. Demolishing
the section would be overreach the evidence does not support.
3. "1280x720 WebM is ~5 MB/minute" — REAL
Two errors in one line. The CLI never emits 1280x720 by default, and the
~5 MB/minutefigure isunsourced — it appears in no upstream skill, no
@playwright/cliREADME, and no official Playwrightdoc. Deliberately removed rather than re-anchored to 800x450: correcting only the resolution
would relocate the fabrication instead of fixing it, and short clips of a static page cannot ground a
replacement number. Replaced with a qualitative statement (size scales with frame area and on-screen
motion).
Evidence
Empirical repro, ffprobe on the resulting
.webm,@playwright/cli0.1.14 on Windows:open, barevideo-startvp8, 800, 450open,resize 1920 1200, barevideo-startvp8, 800, 450—resizedoes not move itPLAYWRIGHT_MCP_VIEWPORT_SIZE=1920x1200 open, barevideo-startvp8, 800, 500open,video-start --size "1920x1200"vp8, 1920, 1200— but a frame extracted at n=20 shows the 1280x720 render in the top-left corner, rest padded greyvp8, 1920, 1200, correctThat last row refines the original report, which described it as an "upscaled 1280-wide render". It is
not upscaled — it is letterboxed top-left, exactly as
https://playwright.dev/docs/videos describes: "The video of the viewport is placed in the top-left
corner of the output video, scaled down to fit if necessary."
Sources, all fetched or executed this session:
help.jsonfrom@playwright/cli@0.1.17(npm tarball, the pinned version) —--sizepresent,identical text to 0.1.14;
flags: { size: "string" }@playwright/cli@0.1.17README —PLAYWRIGHT_MCP_VIEWPORT_SIZEenv var and format; the.playwright/cli.config.jsonschema showingbrowser.contextOptionsand a top-levelsaveVideo: { width, height }recordVideo.size: "If notspecified the size will be equal to
viewportscaled down to fit into 800x800. Ifviewportis notconfigured explicitly the video size defaults to 800x450."
video size."
Repo-convention compliance
0.4.0→0.5.0. Precedent in this plugin's own CHANGELOG: 0.3.1/0.3.2 were pure doccorrections → patch; 0.4.0 folded in newly-surfaced upstream commands and flags → minor. Surfacing
--sizematches the 0.4.0 pattern.made.
check-skill.shconfirms "all 10 base-ref trigger phrase(s) preserved".vendor/untouched. It is a verbatim upstream baseline for drift detection; editing it wouldmake the next
update --checkreport false drift. Upstream's own shipped skill omits--sizetoo —that is an upstream gap, noted as reportable, not patched here.
1920x1200, the reporting operator's personal preference.Not baked in — the docs use a neutral illustrative
1440x900and teach the pattern (set viewportat
openAND pass--size, matched), since the number is the consumer's call.with frontmatter untouched, so it falls outside that scope. The load-bearing claims are grounded in
the pinned package's own
help.jsonand README plus playwright.dev, all obtained this session andcited above. No stale
docs.claude.comURLs exist underplugins/playwright.Checks run locally
check-skill.sh playwright— PASS, 0 errors (1 pre-existing advisory warning: no Gotchas surface)validate-plugins.sh— all manifests + catalog passcheck-changelog-parity.sh --checkand--check-bump origin/main— passcheck-skill-portability.sh origin/main,check-contract-slice-prune.sh --check-diff origin/main,check-shell-portability.sh origin/main,generate-catalog.mjs --check— passDeliberately deferred
## Gotchassection. The report suggested one for "resize does not affect videoframe size". That fact now lives in two places a reader actually hits — the SKILL.md Defaults
exception (the exact spot that previously steered wrong) and the measured-outcomes table. A separate
section would duplicate it to silence an advisory warning that predates this PR.
.playwright/cli.config.jsonroute. The README schema showsbrowser.contextOptions(accepts
viewport) and a top-levelsaveVideo: { width, height }, and confirmssaveVideogovernswhole-session auto-save — a different mechanism from on-demand
video-start. Documented as such andexplicitly flagged unverified, since I did not measure it.
--sizeomission upstream tomicrosoft/playwright-cli.Not verified
Official Playwright docs do not anywhere state that video frame size is fixed at context creation
and immune to a later resize. I searched
docs/src/videos.mdand thePage.setViewportSizesection ofdocs/src/api/class-page.mdin full. The claim is therefore attributed to measurement in this repo'swording ("
resizedoes not change the video frame size" — what ffprobe showed), never asserted asdocumented upstream behavior.
Related
docs.claude.comURLs tocode.claude.comrepo-wide. Merged beforethis branch was cut, so there was no collision. Verified independently that no stale
docs.claude.comURL remains underplugins/playwright; this PR adds none.microsoft/playwright-cli— its own shipped skill(
vendor/references/video-recording.md) omits--sizetoo. Not closed by this PR and not patchedhere, since
vendor/is a verbatim drift-detection baseline; reportable upstream as follow-up.with no architecture or contract-surface decision attached.
🤖 Generated with Claude Code