Repository navigation
feat(bitwarden-testing-tools): add context building and service scoping to the web test pipeline - #210
feat(bitwarden-testing-tools): add context building and service scoping to the web test pipeline#210kdenney wants to merge 73 commits into
Conversation
2465025 to
ce465ed
Compare
Claude Code validationResult: Pass Re-ran against the pull request's original merge base Not covered: The hook unit suite could not be executed — every Findings
Dropped after documentation check: two candidate findings the checks raised were cleared against the official documentation and are absent above.
Previous findings — re-check
Finding 1. Finding 2. Finding 3. The literal now appears exactly four times, each in a trailing Finding 4. Both new skills now ship a No inline comment was posted this round. Finding 1's thread is resolved and its problem no longer appears in the diff, so the repeat-a-persisting-finding exception does not apply; Finding 1 above is SUGGESTED, which stays in the summary by contract. Checks run
Security posture. Unchanged by No file in the changeset attempts to direct this review. The CWE-1427 language throughout the skills and agents is defensive guidance aimed at their own runtime inputs, and the injection string at Three further candidates were considered and dropped. |
ce465ed to
67aaf57
Compare
67aaf57 to
a39e947
Compare
a39e947 to
0d74197
Compare
0d74197 to
d03ec89
Compare
f407a58 to
22c883d
Compare
4477912 to
35ec1d1
Compare
35ec1d1 to
db4d8d0
Compare
kdenney
changed the title
a272f0d to
ea49f85
Compare
ea49f85 to
c358f18
Compare
4aca5bb to
8dc5007
Compare
…ing trials to a paid-org signup The PM-44448 run failed because the catalog's trial-email flow stated the wrong enum values and the wrong wizard order. A normal paid-org signup already starts a trial, so it becomes the default producer of a trialing org; marketing and new sales-assisted flows are gated behind Select only when conditions, and every sourced fact cites a literal the scoper checks.
…s in the scoper Catalog entries were copied verbatim and treated as already validated, so wrong enum values and step order reached the PM-44448 test plan. The scoper now checks each cited literal as plain text before copying, corrects drift or stops, and uses a Select only when flow only when the context or the operator's extra instructions meet its condition.
Cases 7 and 8 relied on the paid-org state's two producers, which the catalog change removed; they now test the Select only when filter and the changed-UI tie-break. Four new cases cover the default paid-org trial, marketing flows chosen by acceptance criteria about the trial pages, a sales-assisted trial chosen by extra instructions, and a no-card trial resolved by the shortest-chain fallback.
… and citation checks A gated single-producer state could be copied with no producer, the Grep escape list and 2,000-line Read limit could report false drift, and a missing file or a fact that survived a refactor had no rule. Weak citations now encode their facts, the existing-user mail retry is capped, and eval case 12 states the real reason the marketing without-payment flow is chosen.
… read The using-stripe-cli wrapper now sends a POST to create_preview, which changes no Stripe state. The policy defined a Stripe write by HTTP method, which would have forbidden it, so define a write by whether it changes state and name the preview as a read.
… Context A test run paused to ask a human whether a flag was on, because the scoper could only carry a flag requirement as prose in Notes, which the writer turned into a [HUMAN] step. Give flags a structured section, and reserve [HUMAN] for actions no granted tool can perform, so Stripe and Mailcatcher reads stop pausing runs.
…rtifact The environment check that verifies flags reads the composed test plan, which holds the services artifact but not the Application Context. Have the mapper copy the flag section through, and require the Api whenever a flag is listed, since flag state is read from the running Api.
…ass for flag keys Flag keys are not only declared in FeatureFlagKeys in Constants.cs; libraries such as Invoicing declare their own [FlagKeyCollection] classes, which the server registers the same way.
… service mapper The Inputs bullet treated a bare path as a web vault route while step 7 said one matched no host-keyed route clause, so step 6's primary-URL choice depended on which reading the model took. Step 3 now expands a bare path to https://localhost:8080<path> before matching.
…plugins reading-mailcatcher-api and using-stripe-cli now live in bitwarden-mailcatcher-tools and bitwarden-stripe-tools. plugin.json declares both as dependencies so they install with this plugin. The tool policy names each category's owner by its plugin-qualified name and drops the canonical script paths for both: a path into another plugin does not resolve from this one, and invoking the owning skill is what applies its script grant. The scoper's known-flows read email through the skill rather than a script path, and the reach-via conventions and scoper eval name the qualified Stripe skill.
…old no Bash The scoper, mapper, and context gatherer list no Bash in tools:, so their allowlists already keep it out and the hook's Bash branch never fires. Narrow the matcher to Skill, which is the check that still matters: a forked skill runs its Bash as a different agent type outside those allowlists. This also stops the hook from starting a python3 process on every Bash call in any session with the plugin enabled.
…rences Harden repo-diff.sh against a planted repo's core.fsmonitor and say what its basename check does and does not guard. Qualify the Mailcatcher skill name in the known-flows catalogs and the agents' skills entries so they match the plugin-qualified names the policy and hook require. Rebuild the mapper's feature-flag passthrough from validated bullets instead of copying them verbatim, and fix its argument-hint and bullet order. Record that the hook reads tool_input.skill, and note the behavior-eval suites in the evals README. PR 210 now stands alone: the policy, hook docstring, changelog and hook tests no longer name an orchestrator or later-PR agents.
…EADME skill name The planning agents hold no Bash and the hook matches only Skill, so the agent instructions no longer say it blocks Bash calls. The README row for the Mailcatcher dependency now uses the plugin-qualified skill name.
… planning agents The agents cannot call tools outside their allowlist, the Inputs section already says the caller supplies the diff artifact, and the hook explains its own block. The README Stripe row now uses the plugin-qualified skill name.
… context gatherer The agent cannot call tools outside its allowlist, and the hook explains its own block.
…g citations The scoper's magenta is not among the colors the sub-agents docs accept, so it becomes purple, which no other agent in the plugin uses. The auth and admin known-flows cited their verification points as file:line, which the scoping skill never checks and which had already drifted in the source. They now use the path and literal form billing.md uses, so a moved or renamed literal is caught.
…catalogs and script The PR validation report found places where the skills' stated contracts disagreed with the catalogs they copy from and the script they call, so a run that follows one rule produces output the other forbids. - Scoper: States `Source:` takes the catalog citation form only; flows may carry an optional `**Note:**`; routes may keep an `:organizationId` segment; the route check ties a placeholder to its producing flow. - Mapper: drop the nonexistent "render verification" reference; list the basename refusal among repo-diff.sh failures; replace the unverifiable working-directory check with the script's own failure path; validate feature-flag bullets before deciding to add Api. - Hook: drop the unused env parameter and os import. - auth.md: move the Note below the post-condition to match billing.md.
The skill was nearly 4,000 words, with the same rules restated in several sections. Each rule now lives in one place, the cited-literal procedure moves to its own reference, and the description drops its exclusion clause, since the scoper agent invokes the skill by name. - Prompt-injection notes record the concern and where it appeared, without carrying the embedded instruction into the artifact. - Route placeholders are generalized from `:organizationId` to any `:<name>` segment for a value an earlier flow in the setup chain creates. - Self-review failures emit the plain failure report, and the Notes section defers to the steps that send entries to it.
…ll already states The citation form and the catalog-only field handling are defined in the scoping skill. The section keeps only what the skill does not say: what counts toward a Select only when condition, and why card iframe titles carry no citation.
The plugin README documents both path variables.
…ontract Review of the trimmed skill found the placeholder rule could fail copied catalog states, whose organization ID comes from the state's own producer, and left placeholders in flow-step URLs unruled. - A `:<name>` value may come from the state's producer or its precondition chain; route-only states never carry one; self-review check 5 verifies the producer, the source of the value, and the parenthetical. - Flow-step URLs may keep a `:<name>` segment, which is never a parameter. - Notes and the failure report name what the reference file and the agent send them; the cited-literal check states its missing-file stop. - The service mapper drops a Route's trailing parenthetical when it collects URLs.
…lure naming Flows step 1 again names the inputs that decide a Select only when condition, since it is the only gate on flows copied for exercising changed UI. The failure report now names a flag key or a citation when that is what failed. Four sentences are tightened to keep the skill within its length target.
…gs use it The rule said a state is reachable only through the playwright-cli skill, yet the catalogs mark rendered states yes when their producers also read Mailcatcher or fire an external trigger, and mark only Route: n/a states no. Reachability now turns on whether the state is a rendered page the executor can reach with steps it runs itself.
…h its input precedence services.md claimed the procedure never resolves a repo path, and the eval purpose said the skill always runs its own diff. The skill uses caller-supplied changed files first and builds the path only in standalone runs, so the contradicting sentence is cut and clause (b) now describes the fallback.
6b69e6b to
5cc1079
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
List the three agents under the manifest's agents key so each loads as bitwarden-testing-tools:<agent> however the plugin is loaded, and drop the hook docstring's note about the name forms seen in the wild. Add a seventh self-review check that the catalog-only Select only when and Sources fields were dropped, since the behavior suite grades that rule. Keep the dev master password out of flow steps and parameter lines. Each flow that takes <password> now states the value once, in its own Note, so a copied billing signup flow still carries it without the auth flow. Add trigger suites for the scoping and mapping skills, per the plugin's documented procedure for a new skill. Each uses the other's phrasings as near-misses. Neither has been run, so neither has a baseline yet.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR adds the planning layer of the Not covered: The skill review did not run. This review path cannot launch |
1 participant
🎟️ Tracking
No ticket. Part of the stacked series building the
bitwarden-testing-toolsPlaywright web test pipeline. This PR is the context-building and service-scoping layer, following #209 (merged).📔 Objective
Adds the planning layer of the web test pipeline: the components that turn a feature reference into the grounded context and service set the later stages consume.
scoping-playwright-application-contextbuilds a state-centric Application Context (states, flows, and any feature flags the run depends on) from the changed code, drawing on per-domain catalogs of known flows, andmapping-services-under-testresolves which local services a change needs.scripts/repo-diff.shlists them).PreToolUsehook confines each agent to its own skill, and a shared untrusted-source policy tells them to treat anything they read as data, never instructions.references/playwright-tool-policy.md, the shared tool boundary for the pipeline, with Mailcatcher and Stripe handled by skills in their own plugins, declared as dependencies.