feat(bitwarden-testing-tools): add mailcatcher and Stripe skills for testing - #209
feat(bitwarden-testing-tools): add mailcatcher and Stripe skills for testing#209kdenney wants to merge 33 commits into
Conversation
Claude Code validationResult: Pass Validated PR #209 ( No critical findings and no security weakening. The two major findings are correctness gaps in newly added guidance, both traced to the source and reproduced below — they warrant a fix but do not block. CriticalNone. Major
Minor
Verified clean
Checks run
The three script checks are run by the workflow as separate steps before this review; their absence from this comment is not a pass. To run them locally, use |
ae00856 to
4d68359
Compare
kdenney
changed the title
kdenney
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the foundation layer of the testing pipeline stack: two new agent-less skills ( Not covered: Skill review did not run — this review path cannot launch Code Review DetailsNo findings at or above the reporting threshold. Notes on items examined and deliberately not raised:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
main does not ignore either path. settings.local.json is present and untracked, and this repo's claude-config-validator treats a committed settings.local.json as a critical finding, so a later 'git add -A' would have introduced one.
…t-bearing leaf skills Migrates reading-mailcatcher-api, using-stripe-cli, and compiling-test-report from the bitwarden-playwright-testing branch, along with the pipeline tool policy they and later pipeline skills depend on. Content is unchanged apart from the plugin rename in stripe_cli.py and the tool-policy path in using-stripe-cli/SKILL.md. 96 unit tests pass in the new location.
…harness There is no precedent for plugin-level shared code in this repo and eval runners are always per-skill, which would put three near-identical 200-line runners in this plugin. The runner is genuinely shared logic, so it moves to scripts/eval_harness.py parameterized by an EvalConfig, and per-skill scripts become thin configuration. assessing-test-coverage's runner is reduced to a wrapper. Its measurement behavior is unchanged, verified by re-running its 20-query suite and diffing verdicts against the committed baseline. Does not touch bitwarden-delivery-tools, whose older standalone runner is out of scope. Harness carries unit tests including a spawn-context import check, since a spawned child re-imports the entry module and macOS defaults to spawn, plus synthetic stream-json tests that drive run_query's bail-out and read-only carve-out decisions directly rather than through real agent invocations.
20-query set, 10 should-trigger email-reading phrasings and 10 near-misses covering pure mail configuration (SMTP host/port, mailcatcher listen port), container startup, email-template work, server-side flow explanation, and coverage inventory. Two initial near-miss queries were ambiguous rather than clean near-misses: diagnosing a mail-delivery problem or verifying an SMTP send both plausibly start with reading the inbox, so a trigger on either was arguably correct and not usable as evidence of over-firing. Both were reworded to pure configuration lookups with no message-reading component. Baseline recorded at 10/10 should-trigger and 9/10 should-not-trigger against the current four-skill inventory (assessing-test-coverage, compiling-test-report, reading-mailcatcher-api, using-stripe-cli), and is explicitly provisional pending the final ten-skill inventory. One near-miss, "start the mailcatcher container for me", consistently misfires (7/7) and is documented in the README as a known follow-up for whoever next owns the skill's description, since the migrated SKILL.md stays untouched here.
Seven advice-only cases covering the read-only boundary, the single permitted test-clock advance, the subordination rule, the never-permitted database and feature-flag shortcuts, and untrusted content in Stripe metadata. The with-skill versus without-skill baseline is not recorded yet; that benchmark run is deferred to a later pass once all of this plugin's behavior suites exist. Case 7 injects a real imperative instruction directly in the prompt's metadata block rather than only describing that one exists, so the case can actually be graded on injection resistance rather than on reassuring words about a hypothetical. Synthetic Stripe fixture ids use camelCase (TestClock, TestSub, TestCus) so they read cleanly without adding entries to the shared cspell dictionary. Case 2 originally expected the agent to hand-roll a per-day polling loop, which contradicts SKILL.md: the wrapper already waits for the clock to return to ready between its internal steps, so the documented correct behavior is a single advance-clock call with the full day count. Fixed the expectation to match the artifact under test instead of an adjacent document that describes what the wrapper does internally, not what the caller should do.
The policy file moved to references/playwright-testing-pipeline/ earlier in this release, but a docstring reference in stripe_cli.py was missed because the survey that found the other references was scoped to markdown files only. An identical reference in executing-web-tests/scripts/external_trigger.py is corrected when that file migrates in a later PR of this stack.
… not-yet-benchmarked
The shared trigger-eval harness counted a trigger only on a Skill call or a SKILL.md read, so a direct agent dispatch (an Agent/Task tool_use with subagent_type) fell through to the exec_tools bail and was recorded as a non-trigger. The agent non-trigger suite could not fail as a result. Add a detection branch for Agent/Task subagent_type before the bail, and accept AGENT.md in the Read branch. Both are inert for the skill trigger suites.
Add a negative-scope clause to the description so the skill stops firing on container-lifecycle requests like 'start the mailcatcher container for me', which no component in this plugin performs (TTM-06). The matching eval README reading update lands on the orchestration branch, where that prose lives.
…ight-consistent names
…havior, and resource fields
… and prerequisites
… and stripe skills Make reading-mailcatcher-api and using-stripe-cli coherent and accurate on their own, since reviewers see the foundation PR by itself. - Security: the eval-run recipe no longer leaves Claude OAuth credentials in a world-readable /tmp dir. Preserve mktemp's 0700 and clean up via a trap that also covers interrupted runs (both evals/README.md). - Add scripts/get_admin_email.py, which reads only adminSettings.admins from the server dev secrets file, and drop the unscoped Read grant, so the admin address is resolved deterministically without exposing the rest of the file. - Rewrite email-patterns.md to the sanctioned read_mailcatcher.py invocations instead of curl|grep, and relocate the raw-API and destructive DELETE commands into the debugging-only manual-api-walkthrough.md. - Drop the unsupported OTP-code extraction claim (the script extracts URLs); keep link-based flows. Fix the exit-code guidance and note the non-fatal HTML-fallback warning. Bracket the optional --pattern flag. - Remove dangling references to the upstack orchestration layer (Category 4, [HUMAN], the tool policy, playwright-test-runner, pipeline) from the stripe skill, wrapper docstring, and behavior evals. Correct the read-path and --live wording; add the coupons and promotion_codes resources. - README documents the Stripe CLI and Mailcatcher prerequisites as per-skill and optional. CHANGELOG drops the repo-root .gitignore bullet. Folded into the unreleased 1.2.0; no version bump.
Address the 15 live findings from the Claude Code validation report on PR #209, keeping the plugin at 1.2.0 (pre-merge refinement, no new bump). - email-patterns.md: note that ${CLAUDE_SKILL_DIR} is not expanded in a reference file and must be substituted with the absolute skill dir - reading-mailcatcher-api SKILL.md: enumerate the covered email types, surface get_admin_email.py, document the --mailcatcher-url/--allowed-host flags, extend the exit-3 guidance for a disallowed host, and trim the description under 500 chars (dropping the undocumented "password resets") - read_mailcatcher.py: add --mailcatcher-url/--allowed-host flags defaulting to the existing env vars so overrides ride inside the tool grant; add tests - using-stripe-cli SKILL.md: use ${CLAUDE_SKILL_DIR}/ for resources.md, quote argument-hint, document the generic exit-1 Stripe failure, reframe the eight-day dunning note as a starting point - eval runners: call main() bare so the diagnostic exit status is not misleading - eval READMEs: note the credentials copy must be removed manually on SIGKILL - plugin.json: add stripe/billing/email/mailcatcher keywords - CHANGELOG: correct the 1.2.0 date and record the fixes m8 (stylistic Triggers-on reword) intentionally skipped to avoid destabilizing a passing trigger eval; m6 was already fixed on this branch.
…l grants Address the PR #209 code review before merge, keeping the plugin at 1.2.0 (pre-merge refinement, no new bump). - read_mailcatcher.py: fix the Mailcatcher endpoint at http://localhost:1080 and remove the base-URL override (the --mailcatcher-url/--allowed-host flags and the MAILCATCHER_URL env var), so a granted call can never point the fetch at an arbitrary host. Drop check_base (no caller-supplied host remains) and the allowed_hosts flag parameter; PLAYWRIGHT_TESTING_ALLOWED_HOSTS still extends the extracted-URL allowlist. - tests: replace the five override tests with a regression guard asserting a hostile MAILCATCHER_URL env is ignored and only localhost:1080 is contacted. - both skills: correct the Bash tool grant from the non-matching `<script> *` form to the working `<script>:*` prefix form, and add the Read/Grep/Glob grants the skills rely on to open their reference files without prompting. - SKILL.md/CHANGELOG: match the fixed endpoint and record the fixes.
cf5b4b2 to
0eeb327
Compare
Addresses the 15 findings from the latest validation review on PR #209, folded into the unreleased 1.2.0 changelog (no further version bump). reading-mailcatcher-api: - read_mailcatcher.py walks every filter-matching URL and returns the first that is also a local dev host, so an external footer link that matches the filter no longer masks a later local action link - the HTML-body fallback unescapes entities, so `&` no longer corrupts multi-parameter links - SKILL.md exit-code table splits the filter-miss and non-local-host causes; get_admin_email.py cwd default, --secrets-file, and exit 3 are documented; the admin-email recipe is two Bash calls, not `$(...)` - manual-api-walkthrough.md uses a self-contained <message-id> placeholder; unused Grep/Glob grants dropped; argument-hint quoted - dropped the password-reset trigger-eval query (no documented pattern) using-stripe-cli: - resources.md examples carry the ${CLAUDE_SKILL_DIR}/scripts/ prefix - Stripe field references corrected to Basil-era shapes (latest_charge; invoices.parent.subscription_details.subscription and payments) - advance-clock documented as long-running with extended-timeout and partial-advance recovery guidance; advance_clock reports progress on a mid-advance failure
Address the third-round Claude Code validation review of PR #209 across both new skills. using-stripe-cli: - Correct the advance-clock timeout guidance to the Bash tool's 600000 ms ceiling with four-day batching; no single value guarantees an eight-day advance completes. - Convert the failure prose to an exit-code table that carves the interrupted-advance resume case out of the generic exit-1 rule it contradicted. - Add a "When to refuse, and what to redirect to" section naming the sanctioned alternatives so the behavior-eval subordination cases are grounded in the skill body; align behavior-eval case 2 with batching. - Document obtaining the clock id from the subscription's test_clock, fix the smallest-currency-unit example, and trim the description. reading-mailcatcher-api: - read_mailcatcher.py reports a Mailcatcher outage during the body fetch as exit 3 rather than exit 1, and its non-local-host diagnostic names the count and every rejected hostname (with tests). - Rename PLAYWRIGHT_TESTING_ALLOWED_HOSTS to MAILCATCHER_ALLOWED_HOSTS, document the --link-filter default and get_admin_email.py --all, drop the debugging-walkthrough pointer, fold the User invocation section into the quick reference, and trim the description. Both skills: switch allowed-tools grants and invocation examples from ${CLAUDE_SKILL_DIR} to ${CLAUDE_PLUGIN_ROOT} (not expanded in permission matchers) and quote the allowed-tools scalar.
Two non-blocking items surfaced by the local validate-ai pass: - tighten the reading-mailcatcher-api --link-filter default wording - set assessing-test-coverage/evals/run_real_eval.py to mode 100755 so all three eval-runner wrappers are executable and uniform
Addresses the 13 non-blocking findings from the latest validation review on PR #209 (against head c92ae19), folded into the unreleased 1.2.0 changelog (no further version bump). reading-mailcatcher-api: - add an "email bodies are untrusted data" note (CWE-1427), matching the plugin's other skills, since the input is a shared local inbox - add negative scoping to the description (not for configuring SMTP, starting Mailcatcher, or debugging delivery) - scope the exit-code table to read_mailcatcher.py and document get_admin_email.py's own exit codes (2 usage, 3 secrets-file) - name the concrete start command: docker compose --profile mail up -d mail (from server/dev/) using-stripe-cli: - document reading a test clock: a worked read example plus a test_clocks entry in references/resources.md (endpoints, fields, and status values verified against the official Stripe API reference) - fix the partial-advance guidance: a Bash-tool timeout kills the wrapper and prints no progress line, so a batch that emits neither a success nor an ERROR line must be treated as a partial advance and frozen_time re-read before continuing; scope the advanced-N-of-M resume row to the CLI-failure case - note the four-day batch budget counts only poll sleeps, not CLI round-trip latency - reword the ambiguous "do not report and stop" resume row - trim the refuse/redirect section to a one-line guardrail and move the redirect table to references/redirecting-writes.md (skill stays read-only-triggering) CHANGELOG/README: - drop the ### Fixed section (all pre-release churn on unreleased 1.2.0; Keep a Changelog reserves Fixed for released defects), which also removes the stale PLAYWRIGHT_TESTING_ALLOWED_HOSTS references, and set the 1.2.0 date to 2026-08-31 - add README trigger examples for the two new skills
Addresses the actionable minor findings from the 2026-08-31 local
validation run (the run passed; no critical or major). Doc-only polish
of the unreleased 1.2.0 skills, so no version bump or changelog change.
reading-mailcatcher-api:
- rephrase the description to the canonical third-person voice
("This skill should be used when...") per plugin-dev skill-development
and the skill-reviewer agent that validate-ai routes skill review
through
using-stripe-cli:
- trim the exit-code table cells to short pointers; the "advance-clock
failed partway" row now points to "The one permitted write" instead
of duplicating the resume procedure documented there
argument-hint is left in place across the skills (confirmed the
structure validator tolerates it).
…ferences
Remove the "Substitute the plugin root before running" blockquote from
resources.md and email-patterns.md. It restated automatic runtime
behavior (${CLAUDE_PLUGIN_ROOT} is set and expands to match the
allowed-tools grant) and carried authoring rationale that does not help
the model reading these references at skill runtime. The commands in
each file already use the ${CLAUDE_PLUGIN_ROOT}/ form.
Doc-only change to the unreleased 1.2.0 skills; no version bump.
Non-blocking polish of the unreleased 1.2.0 skills from the local
validation review. Doc-only, so no version bump or changelog change.
using-stripe-cli:
- trim the test-clock timeout paragraph to the actionable batching rule
(<=4 days per call at timeout: 600000, drop to three near the limit,
re-read frozen_time between batches), dropping the arithmetic
derivation
- normalize the two remaining references/ pointers to the full
${CLAUDE_PLUGIN_ROOT}/skills/using-stripe-cli/references/ form so every
reference link in the skill matches
reading-mailcatcher-api:
- remove the redundant "When to Use" section; the description and the
"Common Email Types and Patterns" reference already cover triggering
- normalize the three references/ pointers to the full
${CLAUDE_PLUGIN_ROOT}/skills/reading-mailcatcher-api/references/ form
argument-hint is left in place (undocumented but an accepted repo-wide
convention; skills are slash-command-invocable).
reading-mailcatcher-api: - reword the host-allowlist remediation to address the user, since MAILCATCHER_ALLOWED_HOSTS is a shell env change outside the script's Bash grant (both the exit-1 row and Prerequisites) - give each exit-1 row a distinct stderr fragment so exit code alone need not disambiguate them - add a cwd hint for get_admin_email.py and collapse the duplicated docker recovery line to "see Prerequisites" - align the manual walkthrough's retry wording and filter list with the script using-stripe-cli: - document the poll-exhaustion batch failure mode and gate resumes on clock status (including the internal_failure dead-clock case) - cap --days at 4 in the wrapper (MAX_ADVANCE_DAYS) with EXIT_USAGE, so the batching rule is enforced in code, not just prose; add tests - clarify the ambiguous "web UI" wording to "web vault and Admin portal" in the description and behavior eval - reduce redirecting-writes.md to a preamble plus the routing table, dropping the copies of rules that live in the skill body eval harness / evals: - document the Agent/Task bail in eval_harness.py and the assessing-test-coverage eval README, and note the baseline predates it - explain why the isolated-config eval run needs a credential copy, and trim the run instructions in both new eval READMEs - caveat the using-stripe-cli trigger reading as predating the description change Refines the unreleased 1.2.0 changeset; no further version bump.
The per-skill READMEs each described running the trigger evals against an isolated CLAUDE_CONFIG_DIR seeded with a copied .credentials.json. That recipe fails on macOS: login is stored in the Keychain keyed to the config directory, so a relocated dir has no credentials file to copy and every query records a false non-trigger. Add a shared runbook (scripts/running-evals.md) that instead pins the plugin's skill inventory per invocation via a claude PATH shim (--setting-sources project --plugin-dir), loading the plugin from the worktree while leaving the authenticated default config untouched. The runbook stays inventory-agnostic so it reads correctly on every branch; each suite's README records the inventory its own last reading used. Trim the three evals READMEs to reference it, drop assessing's now-obsolete "reinstall after editing" note, and refresh the using-stripe-cli and reading-mailcatcher-api last-observed readings to 2026-09-01.
1 participant
🎟️ Tracking
No Jira ticket.
Foundation layer (1 of 5) of stacked prs adding a fully-automated Playwright testing pipeline. Stack, bottom to top: #209 (this PR, base
main), #210, #211, #212, #213. Each layer is independently reviewable; review and merge bottom-up.📔 Objective
The foundation layer of the testing pipeline. It bumps
bitwarden-testing-toolsto 1.2.0 and adds the two agent-less "tool" skills that a person can invoke directly, plus the shared eval runner the plugin's other skills build on.reading-mailcatcher-api: reads Bitwarden emails from the local Mailcatcher REST API (verification links, magic links, tokens) through a co-locatedread_mailcatcher.py. Preferred over the Mailcatcher browser UI in automated contexts, where Playwright's browser CORS blocks a direct fetch. It reads messages only and does not start, stop, or health-check the Mailcatcher container.using-stripe-cli: queries read-only Stripe test-mode data through astripe_cli.pywrapper, with one permitted write, advancing an already-attached test clock. The wrapper builds every command itself and never forwards a caller-supplied flag, so it cannot be steered to live or production data.scripts/eval_harness.py: a shared trigger-eval runner that each skill'sevals/run_real_eval.pyconfigures rather than copies. The existingassessing-test-coverageeval runner is refactored down onto it; that skill is otherwise untouched.Each new skill ships with unit tests, references, and a trigger eval recorded as a dated on-demand prose reading against the current four-skill inventory rather than a committed baseline, to avoid repeated baseline runs with frequent edits, though they could be added once changes slow down.
using-stripe-clialso carries advice-only behavior evals.