Skip to content

chore(testing): remove the one-off browser-tools benchmark - #6525

Merged
kyle-sexton merged 36 commits into
mainfrom
claude/gracious-wright-1adkab
Oct 7, 2026
Merged

kyle-sexton merged 36 commits into
mainfrom
claude/gracious-wright-1adkab

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

No related issue: repository cleanup the owner asked for after #6477 merged.

Summary

#6477 added a browser-CLI benchmark under plugins/testing/benchmarks/browser-tools/. That directory sits inside the shipped testing plugin, so every install would carry a fixture server, CDP drivers, and raw run logs that no user runs. The benchmark was a one-off. Its verdict, playwright-cli stays the default and agent-browser is not adopted, is already recorded where it is used: ADR 0056 under Alternatives considered, plus the playwright README and ## Gotchas. This PR removes the benchmark and the clutter that came with it.

Fix

  • Delete plugins/testing/benchmarks/ (52 files) and its unreleased changelog fragment.
  • Add a bump: none fragment for testing. It records that the plugin ships unchanged from 0.23.0, so the release bot deletes it and releases nothing.
  • .github/workflows/pr-require-checks.yml: drop the Spell-check exclusion that only covered the benchmark's results folder.
  • ADR 0056: the agent-browser entry keeps the result and states that the benchmark is not kept. Its revisit trigger is now a behavior change in agent-browser.
  • /playwright:playwright: the Gotchas evidence line cites ADR 0056 instead of the deleted FINDINGS.md. The plugin goes to 0.10.2, on top of the 0.10.1 docs-link release that landed on main meanwhile, with a CHANGELOG entry.

Verification

  • git grep benchmarks/browser-tools returns nothing.
  • Repo gates run locally on the head merged with current main all pass:
    • changelog fragments (--check, --check-required)
    • changelog parity (--check-bump, --check-order, --check-preserved)
    • changed skills, docs-only gate, lane coverage, ADR numbers
    • purged em dashes, declared prerequisites, stale-base overlap
    • plan-test-lanes (0 unmapped)
    • markdownlint and typos on the changed files

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK

claude added 30 commits October 7, 2026 03:31
…vs playwright-cli

Fixture server with 20 hostile-UI fixtures and 12 agent task pages, tool-neutral
graders with plain-CDP reference solutions, a scripted L1 runner for both CLIs,
an L2 trial harness with per-run tool isolation and a transcript parser, and a
clean local runner on claude -p --bare.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
…ot its name

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
Hide the fixture state readout and the hydration readiness line from agents,
shorten per-run session ids for agent-browser's socket path limit, accept the
coupon defect's visible symptom in the T04 grader, and drop an F19 step that
waited on hidden text. Records the 25-trial pilot results.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
The maintainer comments in each fixture describe its trap, and an agent that
read the page source got the answer (T09's even named the injection test).
Results gathered while they were served move to a superseded file and are not
scored; the full run restarts on fresh trials.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
aggregate.mjs turns a dated results directory into summary.json and
summary.md under the pre-registered win rule. The README documents the
layers, rubric, grading and how to re-run the suite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
Partial results committed early so a container restart cannot lose them.
The run continues; this file is replaced when it completes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
All 125 L2 trials are in (120 default-arm, 5 content-boundaries arm), every
one passed, so the pre-registered rule names no reliability winner and the
routing rows stay. The Alternatives entry records the secondary differences
and links the suite. Results now carry a <scratch> placeholder instead of
container paths. Adds the testing plugin's changelog fragment.

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

Records what the numbers do not show, each with the trials behind it, and
scores the judged rubric rows. The scripted-layer row fills in when that
run completes.

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

The aggregator's log line used an em dash, and the pilot and superseded
result files still carried this container's scratch paths; both now use
the same <scratch> placeholder as the scored results.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
312 scripted runs (26 fixture variants, 2 delays, 3 repeats): both tools
pass 138 of 156 on different fixtures, no row flaky; agent-browser is about
3.5x faster per fixture. summary.md and summary.json now cover both layers,
FINDINGS.md gains the scripted-layer section, and the aggregator prints
readable L1 failure rows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
Runs each vendored skill's own update script in its read-only check mode
every Monday and on dispatch, and fails the run when one reports upstream
drift, so GitHub's failed-workflow notification reaches the maintainers
with no Claude routine or account involved. Informational: outside
ci-status, read-only token, no secrets, opens nothing.

The scripts disagree on exit codes, so each matrix row states how its
script signals drift; a CLI missing from the runner never counts. A local
run of every row today flags firecrawl, playbooks boris and
skill-authoring, and context7, and passes playwright and dometrain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
…dd reference contents

Audit of /playwright:playwright against the skill-authoring guidance and
the repo's own rules. The skill gate now passes with no warnings.

- Gotchas observed in the 2026-10-07 browser-tools benchmark: fill does
  not blur, refs are refused after a re-render, clicks can land before
  hydration, short toasts outrun per-command startup, uncaught exceptions
  show in console, and Linux containers lack the chrome channel.
- Sessions close by name: kill-all and close-all reach every playwright-cli
  browser on the machine, so they stop being routine setup.
- SKILL.md points at upstream defaults with an as-of date and recheck
  trigger instead of restating them; adds a Next section.
- Contents lists on the six reference files over 100 lines, plus
  snapshot --boxes and generate-locator from upstream 0.1.22.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
… 0056 issue reference

The README states why the plugin keeps wrapping Microsoft's skill: the
upstream 0.1.22 skill's gaps against the skill-authoring guidance, as of
2026-10-07, with the recheck trigger, as issue 6448 asks for Playwright.
ADR 0056's agent-browser entry no longer says issue 6475 tracks
agent-browser; that issue probes playwright-cli's own attach --extension.

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

run-bare.sh drives the browser-tools benchmark with `claude -p`. It is a
maintainer harness, not something the plugin runs on a user's machine, so the
call carries the convention's inline prereq-ok exemption instead of a
plugin-wide prerequisite declaration.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
The uncaught-exceptions and Linux chrome-channel gotchas sat on one line,
so they rendered as a single garbled bullet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
- Void a trial that invokes the compared CLI it was not assigned:
  parseTranscript takes the trial's tool and flags calls to the other one.
  Re-parsing all 125 graded trials voids none, so the results stand.
- Look a bare CLI name up on PATH before following its symlink;
  readlink -f alone resolved it against the working directory, so
  run-bare.sh could not find the shipped skill without *_BIN set.
- Point the README's ADR 0056 link at the file's real name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
The unassigned-CLI check matched only a segment starting with the other
CLI, a /<cli> path, or npx. Env-var prefixes, wrappers (env, command,
exec, which), subshells and $(...) slipped past. It now matches the CLI
name as a whole word wherever a command or path tail can sit. Re-parsing
the 125 graded trials still voids none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
- exec-bit: aggregate.mjs and fixtures/server.mjs carry a shebang, so
  they are now executable.
- editorconfig: l1.json ends with a newline, and run-l1.mjs writes one.
- typos: the fixtures' `sav` answer-code prefix and a `quantit` regex
  carry the inline spellchecker:disable-line directive; one British
  spelling is corrected. Benchmark results are recorded run output, so
  the Spell-check step excludes plugins/testing/benchmarks/*/results/**,
  next to the vendor and judge-calibration exclusions. Touching that
  workflow puts two existing comment misspellings in scope, now fixed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
The whole-word match also fired on mentions of the other CLI in echo
text, grep patterns and comments. commandWords() now drops quoted text
(keeping a double-quoted command substitution) and comments, splits the
string into simple commands (including subshells and $(...)), skips
VAR=value prefixes and wrappers that run their argument (env, command,
exec, npx, which, sudo, time, nohup), and compares the basename of the
command word. 26 cases pass; re-parsing the 125 graded trials voids none.

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

commandWords() dropped every quoted string, so "agent-browser" status
slipped past. A quoted single word is now unquoted in place, and quoted
text with whitespace becomes a placeholder word, so a quoted mention
in an argument (echo "agent-browser", grep -v "agent-browser") stays
clean. 31 cases pass; re-parsing the 125 graded trials voids none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
Quote stripping ran before comment stripping, so a quoted CSS id such
as '#x' was unquoted to #x and then read as a comment, deleting a
chained call to the other CLI. Quotes and comments are now matched in
one left-to-right pass, and a quoted string is unquoted only when it
is a plain word, so 'x;agent-browser' cannot become a command split.

The case table moves into lib/transcript.test.mjs (38 cases), which
the affected-tests selector now maps transcript.mjs to. Re-parsing the
125 graded trials still voids none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
commandWords() skipped a wrapper's flags one token at a time, so a
flag's separate value (env -C /tmp, sudo -u root) was read as the
command and the unassigned CLI behind it was missed. After a wrapper,
the command is now the first later word naming a compared CLI, else
the first non-flag word. 44 cases pass; re-parsing the 125 graded
trials voids none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
plan-test-lanes refused transcript.test.mjs because no lane runs a bare
Node suite: run-plugin-tests.sh discovers only *.test.sh. The wrapper
follows the fingerprint.test.sh pattern: find node, run the suite,
forward its exit status.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
claude added 6 commits October 7, 2026 13:38
…ention

Scanning every word after a wrapper for a compared CLI voided trials
like `time grep -l agent-browser *.log`, where grep is the command.
commandWords() now skips a wrapper chain one flag at a time and lets a
flag listed in VALUE_FLAGS (env -u/-C/-S, sudo -u/-g/-D..., time -f/-o,
exec -a, npx -p/-c) consume its value, so the command word is resolved
as Bash runs it. 50 cases pass; re-parsing the 125 graded trials voids
none.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
The benchmark lived inside the shipped testing plugin, so every install
carried a fixture server, CDP drivers and raw run logs no user runs. Its
verdict is already recorded where it is used: ADR 0056 (Alternatives
considered) and the playwright README and Gotchas.

- Delete plugins/testing/benchmarks/ and its unreleased changelog
  fragment, so testing ships unchanged from 0.23.0.
- Drop the Spell-check exclusion that only covered its results.
- ADR 0056 and the playwright Gotchas evidence line now point at the
  recorded result instead of the deleted files; playwright 0.10.1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
…t-1adkab

# Conflicts:
#	plugins/playwright/CHANGELOG.md
The removal touches plugins/testing/, so the fragment gate needs a
fragment; bump: none records that testing ships unchanged from 0.23.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Py2vMPypYELv5S61e1sCyK
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 7, 2026 15:44
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-07T15:46:25.773123Z b71e063 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 44s —— View job


Security review (/review:security-review)

  • Gather context (PR diff, changed files, custom instructions)
  • Run skip gate / criteria check
  • Post result

Result: no security review needed — skip gate applies.

This PR is a pure deletion + doc-pointer cleanup, with no security-relevant surface:

  • 52 files deleted under plugins/testing/benchmarks/browser-tools/ (fixture HTML pages, a CDP driver, a fixture server, run scripts, and recorded run output/results). Pure removal, no new code paths.
  • .github/workflows/pr-require-checks.yml (diff): only narrows the spell-check exclude: list by dropping the now-dead plugins/testing/benchmarks/*/results/** entry (the directory it exempted no longer exists). That widens scan coverage, not a weakened control.
  • ADR 0056 and plugins/playwright/skills/playwright/SKILL.md Gotchas: citations repointed from the deleted FINDINGS.md to the ADR's "Alternatives considered" section. Content preserved, no guardrail removed — checked against the instruction-surface-deletion lens and this doesn't touch a CLAUDE.md/AGENTS.md/rules file/hook allow-deny list.
  • Changelog fragments, plugin version bump (playwright 0.10.1 → 0.10.2): metadata only.

No injection surfaces, no auth/access-control changes, no secrets/token handling touched. Nothing to report.

@claude

claude Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 9s —— View job


Code review (/review:code-review)

  • Gather context (PR diff vs origin/main, PR body, CLAUDE.md/AGENTS.md conventions)
  • Review non-deletion files for correctness and convention alignment
  • Verify no dangling references to the removed benchmark
  • Verify changelog-fragment and version-bump mechanics
  • Report findings

Scope. 59 files changed, +17/-8679. 52 of the deletions are the removed plugins/testing/benchmarks/browser-tools/ tree (fixtures, CDP drivers, run logs) — no review surface there. The actual review surface is 7 files: the new/removed .changes/testing/ fragments, the CI workflow's spell-check exclusion, ADR 0056, and the playwright plugin's version/changelog/SKILL.md.

Checks performed:

  • git grep -n "benchmarks/browser-tools" and git grep -n "FINDINGS.md" on the HEAD tree both return nothing — no dangling path references to the deleted directory.
  • The testing plugin is listed in scripts/fragment-plugins.txt, so the bump: none fragment (.changes/testing/claude-gracious-wright-1adkab-a175df31.md) is the correct release mechanic, and plugins/testing/.claude-plugin/plugin.json correctly stays at 0.23.0 (unreleased fragment added and removed in the same unreleased window, never shipped). Confirmed against scripts/lib/changelog-fragments.sh / scripts/check-changelog-fragments.sh, which require a non-empty reason line for bump: none — present here.
  • playwright is not in scripts/fragment-plugins.txt, so the direct plugin.json version bump (0.10.1 → 0.10.2) + CHANGELOG.md entry is the correct mechanic for that plugin, and the two are consistent with each other.
  • ADR 0056's updated "Alternatives considered" entry for agent-browser replaces the now-deleted-file pointer with a prose statement that the benchmark isn't kept, plus a new revisit trigger ("when agent-browser waits on clicks and reports uncaught page exceptions") — consistent with the benchmark's own stated finding (agent-browser's console/errors missed an uncaught exception playwright-cli caught every rep).
  • /playwright:playwright's ## Gotchas evidence line now cites ADR 0056 instead of the deleted FINDINGS.md, satisfying the "name a plugin by /plugin:skill, point at a doc by its live location" pattern.
  • The CI workflow's spell-check exclude glob drops only the benchmark-results clause; the remaining two exclusions (vendor/**, judge-calibration cases) are untouched and the YAML remains well-formed.

One pre-existing line, plugins/playwright/README.md:82 ("adds gotchas observed in the marketplace's browser-tools benchmark"), still references the removed benchmark by name, but that file isn't part of this diff and the line carries no path/link, so it isn't a broken reference — out of scope for this review (pre-existing content on an untouched line).

No findings. The deletion is self-contained, the release/changelog mechanics for both affected plugins are used correctly, and the cross-references that needed updating (ADR 0056, playwright SKILL.md) were updated correctly.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through b71e063; a later push is reviewed from there.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through b71e063; a later push is reviewed from there.

@kyle-sexton
kyle-sexton enabled auto-merge October 7, 2026 15:47
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 7, 2026
Merged via the queue into main with commit e917a47 Oct 7, 2026
66 of 79 checks passed
@kyle-sexton
kyle-sexton deleted the claude/gracious-wright-1adkab branch October 7, 2026 15:58
@gitguardian

gitguardian Bot commented Oct 7, 2026

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
37941187 Triggered Username Password 6428664 plugins/testing/benchmarks/browser-tools/lib/cases.mjs View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secret safely. Learn here the best practices.
  3. Revoke and rotate this secret.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

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