Repository navigation
Conversation
added 4 commits
October 20, 2026 20:29
The update check picked the command with `argv.find(arg => !arg.startsWith('-'))`,
which returns the value of a preceding option rather than the command. For the
documented `playwright-cli --config path/to/config.json open example.com`, that
is `path/to/config.json`, so neither the version notice nor the stale skill
warning ever ran.
Parse argv with the same minimist options program() uses instead. Those files
are internal to playwright-core and not in its "exports" map, so resolve them
by path and fall back to the old heuristic if they ever move, keeping the
check best-effort rather than letting it break every command.
The skill ships SKILL.md plus ten reference guides, but the staleness check only compared SKILL.md, so an out-of-date file under references/ was never reported even though it misleads the agent just as much. Walk the bundled skill directory and compare every markdown file; a file missing from the installation now reads as out of date too.
update.js removed skills/playwright-cli and then copied the generated skill over it, with the whole block wrapped in a catch that reported "Generated skills directory not found" and exited 0. Any failure in fs.cp therefore left the committed skills deleted, blamed a cause that was not the real one, and still looked successful — during the documented roll flow. Stage the copy next to the target and swap it in, restoring the previous folder if the swap fails, and let real errors surface with a non-zero exit.
`install` writes .playwright/cli.config.json, so running scripts/update.js during a roll leaves an untracked directory behind.
|
TURKI156 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
program()uses, instead of taking the first argument that does not start with-SKILL.mdscripts/update.jsfrom deletingskills/playwright-clibefore the copy succeeds.playwright/directory thatinstallwritesWhy
The update check never ran when a global option preceded the command. It picked the command with
process.argv.slice(2).find(arg => !arg.startsWith('-')), which returns the value of a precedingoption rather than the command.
program()parses argv with minimist, where--configis not aboolean option and therefore consumes the next token. So for the invocation documented in the README:
program()dispatchesopen, while the update check sawpath/to/config.json, found it absent fromUPDATE_CHECK_COMMANDS, and returned early. Both the version notice and the stale-skill warning weresilently lost. The same applies to
--session name open ….minimist.jsandhelp.jsonare internal toplaywright-coreand not in itsexportsmap, so theyare resolved by path — the way
skillCheck.jsalready resolves the bundledSKILL.md— and the oldheuristic remains as a fallback, keeping the check best-effort rather than letting a moved file break
every command.
The skill check only compared
SKILL.md. The skill ships ten reference guides alongside it, and astale file under
references/misleads the agent just as much. A file missing from the installationnow reads as out of date too.
scripts/update.jscould delete the committed skills folder. It removedskills/playwright-cliand then copied the generated skill over it, with the whole block wrapped in a
catchthat printedGenerated skills directory not foundand exited 0. Any failure infs.cptherefore left the skillsdeleted, reported a cause that was not the real one, and still looked successful — during the roll
flow documented in
.claude/skills/dev/roll.md. The copy is now staged beside the target and swappedin, the previous folder is restored if the swap fails, and real errors surface with a non-zero exit.
Tests
Two tests added:
update check runs when a global option precedes the command— uses--session work list, since--sessionis a global option that takes a valueupdate check warns when an installed reference file is out of dateAll 11 tests pass locally.
Not covered
windowsHide: truewas added to thenpm viewspawn (program()already sets it on its ownspawns) but is not verified — it needs Windows
scripts/update.jshas no automated test; it was verified end to end by running it and confirmingskills/came back byte-identical with no leftover staging directories