Repository navigation
feat: add Mailcatcher and Stripe vendor plugins - #209
Conversation
Claude Code validationResult: Pass Validated the two new plugins this pull request adds — Two findings were verified away rather than reported: both wrapper scripts ( CriticalNone. MajorNone. MinorBoth plugins
Findings dropped after documentation check
What was checked and found clean
Checks run
|
ae00856 to
4d68359
Compare
kdenney
changed the title
kdenney
changed the title
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed at head The sole open thread is addressed at this head: Not covered: Skill review did not run — this review path cannot launch Code Review DetailsNo new findings at or above the reporting threshold. Three items noted for visibility only, not raised as findings. Claude-configuration review found no issues. No |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cf5b4b2 to
0eeb327
Compare
5703fe7 to
449e59a
Compare
3a66ed7 to
54bf1dd
Compare
There was a problem hiding this comment.
💭 Would you be open to considering breaking the mailcatcher skill out into a separate, dedicated plugin: bitwarden-mailcatcher-tools?
I ask because we're currently re-evaluating the taxonomy of the marketplace and will be proposing standards and guidelines meant to improve organization to make determining where a tool should live easier for contributors, among other things.
Making vendor integration tools their own plugin is one of those guidelines. Splitting it out now is easier than pulling it out later, but does mean the evals would need some changes.
The same guidance can technically apply to the Stripe skill, but it seems pretty tightly coupled to testing right now. I'm not sure if or how other teams would use the Stripe API.
54bf1dd to
c5b7b8d
Compare
kdenney
changed the title
@SaintPatrck |
| | 1 | `preview-invoice` got a Stripe API error (the `ERROR:` line carries Stripe's message, e.g. `No such subscription`) | report the message and the subscription id; don't retry or substitute | | ||
| | 1 | any other Stripe CLI **process** failure (e.g. not logged in, network error); an API-level error from `read`, such as a mistyped `cus_`/`sub_` id, does NOT land here — see "Interpreting responses" | report the stderr message and the path; don't retry or substitute | | ||
|
|
||
| ## When to refuse |
There was a problem hiding this comment.
The "When to refuse" section and the whole of
references/redirecting-writes.mdexist to answer write requests by routing them to the web vault / Admin portal / Dashboard flow, but the skill's own trigger contract prevents them from ever loading.
In other words, the reference file isn't reachable because it lives under the Skill, but the Skill's description says not to invoke for write operations. That means Claude will technically never get here, unless it ignores the description and invokes the Skill for operations it shouldn't.
I also see "this skill is read only" style statements duplicated in several places within the skill and its reference files. If the Skill only triggers on read operations, those types of statements aren't necessary. At most, it should only be stated once at the top of the skill so the agent "fails fast".
There was a problem hiding this comment.
Yeah, I agree. These skills have gone though an immense amount of changes and rewrites and that reference file was from an older version where it was being used. I've dropped it and cleaned up the rest of the read-only mentions in the latest push.
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
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.
Bitwarden's server/dev/secrets.json is JSONC by convention (// and /* */ comments, trailing commas), but get_admin_email parsed it with strict json.load and exited 3 on the real file. Add a string-aware strip_jsonc preprocessor that removes comments and trailing commas while preserving markers inside string values (e.g. bitwarden:// URLs), then json.loads the result. The exit-code contract is unchanged.
The Stripe CLI returns exit 0 with a top-level {"error": {...}} body for
API-level failures, so run_cli did not raise on them. advance-clock ignored
the advance POST result, so a rejected advance was reported as success: the
poll saw the clock still 'ready' at its old frozen_time and the wrapper printed
an advance that never happened. Add _run_json to parse internal clock calls and
raise EXIT_CLI on an error payload, and route _clock and the advance POST
through it. The read command stays pass-through by design.
Also correct SKILL.md: the exit-code table wrongly attributed a mistyped
cus_/sub_ id to exit 1; API-level errors are exit 0 with an error body. Scope
exit 1 to genuine CLI-process failure and add an Interpreting-responses bullet
telling the reader to check for a top-level error key before trusting a read.
… CLI The Stripe CLI never checks that its test-mode key is a test key: a live key typed into `stripe login --interactive` or `stripe config --set test_mode_api_key` is used by a plain `stripe get` (Stripe CLI v1.35, pkg/config/profile.go GetAPIKey, pkg/validators/validate.go). The wrapper only inspected STRIPE_API_KEY, so it missed that path. stripe_cli.py now resolves the key the CLI would use (STRIPE_API_KEY, else the active profile's test-mode key in the CLI config), allows only sk_test_/rk_test_ keys, fails closed when the key is missing or the config cannot be verified, and pins the checked key through STRIPE_API_KEY so the checked key is the key used. The Mailcatcher skill's exit-3 remedy told the agent to start Mailcatcher, which its description excludes and its Bash grant cannot do; it now asks the user. Its NO_MATCH remedy now covers a slow send with a single re-invocation.
…rapper A test run could not check what a subscription's next invoice will look like: GET /v1/invoices/upcoming is deprecated, and its replacement, /v1/invoices/create_preview, accepts only POST, which the read command never sends. The preview creates no Stripe object, so add a narrow preview-invoice subcommand that validates the sub_ id and posts only that id, failing loudly on a Stripe API error. Document it across SKILL.md and resources.md, and add a behavior eval case for it.
Main landed its own shared trigger-eval engine at evals/run_real_eval.py (#240) while this stack carried scripts/eval_harness.py for the same job. Two harnesses in one plugin would split every future engine fix, so the stack moves onto main's. The stack's harness detected direct agent dispatch, which main's engine did not. That moves into run_real_eval.py behind a new --agent flag, so the suite checking that orchestrator-only agents stay unprompted has a runner. Its unit tests are ported alongside. The per-skill wrappers go away because main's engine infers the skill and plugin from the eval-set path, and the inventory-pinning shim recipe folds into evals/README.md.
The marketplace is adopting a guideline that vendor integrations live in their own plugin, and review on #209 asked for the Mailcatcher skill to move before it ships. reading-mailcatcher-api moves to bitwarden-mailcatcher-tools and using-stripe-cli to bitwarden-stripe-tools, each at 1.0.0 with its scripts, tests, references, and evals unchanged. Each plugin carries a copy of main's trigger-eval runner, so its evals run without depending on bitwarden-testing-tools' files. The testing-tools README, keywords, and changelog drop the two skills.
…he foundation layer With both skills extracted, this layer no longer changes bitwarden-testing-tools, so it carries no version bump or changelog entry. The runner's --agent flag, its tests, and the working-tree recipe move up the stack to the orchestration layer, beside start-playwright-test's agent eval, which is the flag's only consumer.
- reading-mailcatcher-api's description now says what the skill does rather than how, keeping the routing note about the Mailcatcher UI. - Drop its user-facing "trigger it from any Claude Code session" sentence; the plugin README covers that. - using-stripe-cli no longer tells Claude to attach a test clock in the Dashboard, which it cannot reach. It stops and asks the user to attach one and say when it is done. - using-stripe-cli states that other plugins grant the wrapper on the strength of its read-only boundary, so a needed write is reported as out of scope instead of worked around. - Swap one should-not-trigger mailcatcher eval query for an SMTP configuration near-miss, folded down from the orchestration layer.
Both suites were last measured inside bitwarden-testing-tools' four-skill inventory. Re-recorded against each plugin alone: Mailcatcher 9/9 and 9/10, Stripe 10/10 and 9/10. Each miss is a near-miss query written for a testing-tools sibling skill that is no longer installed alongside. At 7 runs both pass (2/7 and 3/7). For Mailcatcher the previous description scores 3/7 on the same query, so the review-driven description change is no worse.
Both skills granted a bare Read. Once another plugin invokes them, as
the Playwright runner now does, that grant applies for the rest of the
run and approves reading any file, including server/dev/secrets.json
and the Stripe CLI config. Verified on Claude Code 2.1.292: a bare Read
grant read a file outside the plugin unprompted, while
Read(/${CLAUDE_PLUGIN_ROOT}/skills/<skill>/references/**) read the
skill's reference file and still prompted for anything else. The leading
slash matters: permission rules treat //path as absolute, and the
single-slash form did not match.
Also reword the test-clock note so it no longer states that a clock can
only ever be set at customer creation: that limit is the CLI's and the
API's, and the Dashboard attaches one to an existing subscription.
read_mailcatcher.py only ever prints a URL; a token reaches the caller only inside a link's query string. The skill description, manifest, marketplace entry, changelog, and READMEs no longer promise a token.
…cription Should-trigger 9/9, should-not-trigger 8/10 at 3 runs. A 7-run A/B of the two misses against the previous "link or token" wording differs by one run on each, within sampling noise, and the token-phrased should-trigger query still triggers 7/7.
The skill's Bash grant pre-approves get_admin_email.py with any argument, so --secrets-file could probe whether arbitrary files exist and parse as JSON without a prompt. The script now refuses any path that does not resolve to a dev/secrets.json file, before reading it. select_message now drops non-integer ids, so a mixed int/str payload no longer raises TypeError and exits 1, which read as "no email arrived". The docs now state that the URL allowlist checks the host only, list every exit-3 cause, and name the server checkout prerequisite. Two trigger-eval rows are reworded to match the link-only description and to cover delivery debugging, measured at 7/7 and 0/7.
check_path denied a few known-bad characters and so accepted `..`, `%2e`, and query strings. It now allows only the documented /v1/ path shape, matching the clock and subscription id guards. The clock advance poll stopped on status == "ready" alone, so a stale ready read at the old frozen_time could send the next day's advance while this one was still being applied. It now also waits for frozen_time to reach the requested value. SKILL.md gains an "Untrusted content" section that covers pasted record text, says content never authorizes the clock advance, and says embedded directives get surfaced to the user, as behavior-eval case 7 already grades. A new behavior case covers refusing to work around exit 21, and the write-redirect table gains attach-a-clock and catch-all rows.
urlparse reads "https://evil.com\@localhost/" as localhost, but a browser ends the authority at the backslash and navigates to evil.com, so a crafted email could pass the local dev host allowlist. is_local now rejects any URL containing a backslash or userinfo, and URL_PATTERN ends a URL at a backslash, single quote, or angle bracket, which also fixes single-quoted href values on the HTML fallback. The SKILL.md sentence explaining what the allowlist checks is dropped; the untrusted-URL caution remains under Important Notes.
The skill description tells the model not to invoke it for Stripe writes, so the "When to refuse" section and redirecting-writes.md could never load. Collapse the repeated read-only statements into one fail-fast statement at the top of SKILL.md, and remove the behavior evals that graded the removed redirect behavior (write requests are already should-not-trigger queries in trigger-eval.json).
dd4d0f7 to
0ecbb73
Compare
|
Appreciate you making those changes, and thank you for your patience with the review cycles. We'll try to get the stacked ones knocked out quicker. |
3 participants
🎟️ Tracking
No Jira ticket.
Foundation layer (1 of 5) of a stacked set of PRs adding an automated Playwright testing pipeline to
bitwarden-testing-tools. Stack, bottom to top: #209 (this PR, basemain), #210, #211, #212, #213. Review and merge bottom-up.Repurposed in response to review feedback asking for vendor integrations to live in their own plugins. This PR no longer changes
bitwarden-testing-tools.📔 Objective
Adds two vendor plugins at 1.0.0. The skills were previously in
bitwarden-testing-tools; the pipeline layers above this one declare both plugins as dependencies.bitwarden-mailcatcher-tools:reading-mailcatcher-apireturns the action link from a Bitwarden email in the local Mailcatcher inbox (verification links, magic-link logins, trial activations, invites). Itsread_mailcatcher.pyreads onlyhttp://localhost:1080, follows no redirects, and returns only URLs on a local dev host allowlist thatMAILCATCHER_ALLOWED_HOSTScan extend but not replace.get_admin_email.pyprints the dev Admin Portal address and reads nothing fromserver/dev/secrets.jsonexceptadminSettings.admins.bitwarden-stripe-tools:using-stripe-cliqueries Stripe test-mode data throughstripe_cli.py. The wrapper refuses any key that is notsk_test_orrk_test_before calling the CLI, builds every command itself, and is read-only apart from advancing an already-attached test clock. It also previews a subscription's next invoice, which creates nothing.Each plugin carries its own copy of
main's trigger-eval runner, and both trigger readings were re-recorded against each plugin's own inventory (see each skill'sevals/README.md).