Repository navigation
Conversation
Claude Code validationResult: Issues found Validated the Re-run note. The branch was rebased after the previous review (every commit now carries the same Not covered: Scope. This is a stacked pull request. The CI changed-file list carries 80 paths (the whole testing-tools stack measured against Findings
Findings 1–4 already carry unresolved inline threads on this pull request; per the re-run rule they were not re-posted. Details for every finding follow.
Dropped after documentation check: six findings claimed a frontmatter field, permission form, or slash-command form behaved other than as written. All six were refuted by the official documentation.
Also dropped, on measurement rather than documentation: a claim that Checks run
What the security review foundNo secrets, no credentials, and no The shell-adjacent surfaces this PR adds are tightened rather than widened, and are worth recording as such:
Finding 1 is the one place the changeset leaves a grant open, which is why it carries the verdict. |
7661e9a to
e6ddd98
Compare
e6ddd98 to
397c7b0
Compare
d5f360f to
1645262
Compare
1645262 to
4cf8e05
Compare
9a52299 to
d599e2f
Compare
d599e2f to
e05bfbb
Compare
e05bfbb to
7e45c0b
Compare
b4ec0a8 to
db03cad
Compare
db03cad to
b10d2bd
Compare
b10d2bd to
c97db5f
Compare
3503de2 to
55c30e2
Compare
1d07e94 to
3553b08
Compare
3553b08 to
d3e1365
Compare
d3e1365 to
fa10d1b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| name: start-playwright-test | ||
| description: Use when you want UI tests planned and run against local Bitwarden web changes, starting from a Jira ticket, an implementation plan, or a description of the feature. Requires the Bitwarden local dev environment to already be running; this pipeline verifies services and required feature flags but never starts services or changes flags. Accepts a Jira ticket ID, a Jira browse URL, an implementation plan file path, or a feature description, optionally followed by extra instructions. | ||
| argument-hint: "<jira-ticket-id | jira-url | feature-plan-path | feature-description> [extra instructions]" | ||
| allowed-tools: "Agent, Read, Write, Bash(mkdir *), Bash(${CLAUDE_PLUGIN_ROOT}/scripts/repo-diff.sh *), Bash(${CLAUDE_SKILL_DIR}/scripts/open_report.py *)" |
There was a problem hiding this comment.
Write is granted unscoped on a line that carefully scopes every Bash grant.
Details and fix
Every Write this skill performs targets <cwd>/.playwright-testing-artifacts/<slug>/ (Tasks 2, 3, 4, 5, 6, 8 — there are no others). The content written is agent output the skill itself classifies as untrusted-source-derived ("Write the agent's response text verbatim").
A bare Write pre-approves writing to any path for the turn. So a directive injected into a Jira ticket or a repo file that persuades the orchestrator to write somewhere else — .claude/settings.local.json, a hook script, a workflow file — lands without a prompt. That is the one escalation this skill's own trust boundary is built to stop, and it is the only grant on this line left open: all three Bash grants are pinned to exact script paths.
Scope it with an Edit(...) rule, which is the documented path-rule form for file modification (Read and Edit rules; there is no documented Write(<path>) rule form):
allowed-tools: "Agent, Read, Edit(.playwright-testing-artifacts/**), Bash(mkdir *), Bash(${CLAUDE_PLUGIN_ROOT}/scripts/repo-diff.sh *), Bash(${CLAUDE_SKILL_DIR}/scripts/open_report.py *)"Calibration: three sibling skills in this plugin (writing-manual-test-cases, assessing-test-coverage, recommending-test-layers) also grant bare Write, so this follows existing convention rather than departing from it. It is reported because the line is new in this changeset and because this is the one skill in the plugin whose job is handling untrusted agent output.
Reference: https://code.claude.com/docs/en/permissions#read-and-edit
| "lint": "pnpm run lint:prettier && pnpm run lint:spelling && pnpm run lint:guardrail", | ||
| "lint:prettier": "prettier --check .", | ||
| "lint:spelling": "cspell lint --no-progress --no-summary --gitignore '**'", | ||
| "lint:guardrail": "bash plugins/bitwarden-testing-tools/scripts/validate-guardrail.sh", |
There was a problem hiding this comment.
Details and fix
This adds lint:guardrail and chains it into pnpm run lint on line 28. But .github/workflows/lint.yml never invokes pnpm run lint — it calls the two sub-targets individually:
lint.yml:61→pnpm run lint:prettierlint.yml:67→pnpm run lint:spelling
So validate-guardrail.sh runs only when a developer happens to type pnpm lint locally. CHANGELOG.md:15 states the check is "(run in pnpm lint) prevents drift" — in CI it does not run at all, so nothing stops a future edit from removing an agent's untrusted-source guard.
Add a step to lint.yml alongside the existing two, wired into the same Fail if any linter failed gate:
- name: Guardrail
id: guardrail
continue-on-error: true
run: pnpm run lint:guardrailThen include steps.guardrail.outcome in the gate condition at lint.yml:71.
While you are there: scripts/validate-guardrail.test.sh, evals/tests/test_run_real_eval.py, and skills/start-playwright-test/scripts/tests/test_open_report.py are all added by this PR and are likewise invoked by nothing. test_open_report.py is the security-relevant one — it covers the symlink, .., and symlinked-run-folder escapes in open_report.py.
|
|
||
| ## Inventory | ||
|
|
||
| These readings were recorded against the ten sibling skills the orchestrator competes with for selection (each reading below notes its own run date). Eight ship in this plugin: |
There was a problem hiding this comment.
Details and fix
This says "Eight ship in this plugin" and lists eight. The plugin actually ships ten skills — ls plugins/bitwarden-testing-tools/skills/ returns:
assessing-test-coverage recommending-test-layers
checking-localhost-web-health running-playwright-tests
compiling-playwright-report scoping-playwright-application-context
mapping-services-under-test start-playwright-test
writing-manual-test-cases
writing-playwright-test-cases
recommending-test-layers and writing-manual-test-cases are both absent from the list, so the stated competition is ten skills when it is really twelve (ten in-plugin plus the two dependency skills).
This matters because line 38 of this same file says "the recorded numbers are only meaningful against this exact inventory." writing-manual-test-cases is specifically the skill the write QA testing notes for this branch so a human can test it manually near-miss is aimed at — and that query timed out on all 7 runs, so its pass already carries no signal. The separation from the one sibling most likely to cross-fire is therefore unmeasured against an inventory that does not list it.
Add the two missing skills to the list, correct "Eight" to "Ten" and the dependency sentence's count, and re-run trigger-eval.json with a longer --timeout so the two timed-out near-misses produce a real reading.
|
|
||
| ### Added | ||
|
|
||
| - `start-playwright-test`, the pipeline entry point and the only orchestration skill. It accepts a Jira ticket id, a Jira browse URL, an implementation plan path, or a feature description, optionally followed by extra guidance (passed verbatim to the scoper, where it can call for a marketing-initiated or sales-assisted trial). It runs an eight-task pipeline, dispatching six agents and persisting each response verbatim to `.playwright-testing-artifacts/<slug>/`, then renders an HTML report. Tasks 3 and 4 are dispatched together and run concurrently. After gathering context it checks that each affected repo is exactly `clients`, `server`, or `billing-pricing`, then runs `scripts/repo-diff.sh` for each one, under a grant scoped to that script, and writes the changed files to `diff-<timestamp>.md`, which the scoper and mapper read in place of running the script. A required feature flag in the wrong state, reported by the health check, halts the run like any other environment failure, so a run never pauses to ask about a flag. When the report is written, it opens it in the user's browser through `skills/start-playwright-test/scripts/open_report.py`, under a grant scoped to that script; the script opens only a `report-<timestamp>.html` inside a run folder under `.playwright-testing-artifacts/`, using `open` on macOS or `xdg-open` on Linux, and skips other platforms, with unit tests. |
There was a problem hiding this comment.
Details and fix
skills/start-playwright-test/SKILL.md numbers the pipeline Tasks 1–9:
| Task 1 | Parse input |
| Task 2 | Gather context |
| Task 3 | Explore codebase |
| Task 4 | Determine required services |
| Task 5 | Build test cases |
| Task 6 | Compose test plan |
| Task 7 | Verify environment health |
| Task 8 | Execute tests |
| Task 9 | Compile report |
Three places say eight and name the wrong concurrent pair:
- This line — "eight-task pipeline … Tasks 3 and 4 are dispatched together"
README.md:137— same two claimsREADME.md:31— the skills table, "in an eight-task pipeline"
SKILL.md:118 is explicit: "Tasks 4 and 5 both need only the artifacts from Task 3, so dispatch services-under-test-mapper and playwright-test-case-writer in the same message."
Because the README table starts at Task 1 = playwright-test-context-gatherer (which is Task 2 in SKILL.md), every row of the README.md:141-148 table is off by one — it does not just undercount, it maps the wrong task number to each agent. The table is also missing a row for the diff-<timestamp>.md artifact (SKILL.md:74-92) and attributes test-results-<timestamp>.json to the runner, though SKILL.md:210,219 has the runner emit segment-<K>-<timestamp>.json with the merge script producing test-results.
Fix: change "eight-task" to "nine-task" and "Tasks 3 and 4" to "Tasks 4 and 5" in all three places, renumber the README table to match SKILL.md's Tasks 1–9, and add the diff-artifact row.
Squashed net diff of the orchestration layer (start-playwright-test) for a clean reparent.
…d sub-artifacts by fence
…tches it `checking-localhost-web-health` is a skill, which agents invoke/run; dispatch is for agents (the Agent tool). Reword the README agent row for localhost-web-health-checker accordingly. No behavior change.
… contract The untrusted-source guardrail moved its rules into a shared references/untrusted-source-policy.md, referenced by each agent, so the per-agent prose no longer contains the old "names this run's fence token" backstop string. Retarget validate-guardrail.sh to anchor each AGENT.md on the "Untrusted source content." heading and the UNTRUSTED-SOURCE-<nonce> fence binding — phrases both the current and earlier guardrail forms share, so agents can migrate one at a time without turning the stack red — and add a check that the shared policy file exists. Update the test fixtures to the new contract and add cases for a missing fence binding, a missing policy file, and a stale paragraph alongside a valid guardrail.
… the shared policy The orchestrator prepended a full copy of the untrusted-source rules to every agent dispatch, duplicating the rules that now live in references/untrusted-source-policy.md. Slim the prepended block to what only the orchestrator can supply — this run's fence token and the binding of the shared policy to the UNTRUSTED-SOURCE-<nonce> region — and defer the rule text to the policy file, so the whole pipeline has one source of truth for the guardrail. The nonce generation and fence-binding mechanism are unchanged.
…ation layer With the gatherer now distilling and discarding the raw feature source, the per-run nonce fence has nothing left to protect, so remove it end to end. - start-playwright-test: drop the run-token generation and the gen-nonce.sh grant; rewrite the dispatch guardrail to point at the shared policy with no fence or token; in Task 2 replace the "exactly one UNTRUSTED-SOURCE pair" check with a CONTEXT-fence + three-section check, and drop the non-load-bearing Source Summary note from artifact persistence. - Delete scripts/gen-nonce.sh and gen-nonce.test.sh. - validate-guardrail.sh: retarget from the UNTRUSTED-SOURCE fence to the shared-policy contract — every AGENT.md carries the "Untrusted source content." heading and names references/untrusted-source-policy.md, the orchestrator skill names it too, and the policy file exists. Drop the fence and gen-nonce checks. Update the test fixtures and cases to match. - Rewrite the gatherer's injection behavior eval: the injected imperative must now appear nowhere in the artifact (there is no fence to reproduce it into) and must not be acted on; update its README and expectation count. - Reword the 1.6.0 changelog trust-boundary bullet to the sanitize-and-discard model.
…EADME skills table The orchestration layer's README skills table omitted the compiling-playwright-report skill, though the skill ships in the plugin. Add its row so the table is complete.
Plugin agent discovery loads markdown files under agents/ as agents, so the gatherer's evals/README.md registered as an unrestricted agent of its own, with all tools and no frontmatter. Move the eval beside the orchestrator's agent suites under skills/start-playwright-test/evals/, where skill discovery reads only SKILL.md, and point the orchestrator's eval README and the 1.6.0 changelog at it.
…e orchestrator The scoper and mapper no longer hold Bash, so the orchestrator runs repo-diff.sh in the main session and writes the changed files for them.
… runs repo-diff.sh The repo names derive from untrusted feature source and go into a main-session shell command, where the planning-agent hook does not apply.
Extra instructions were only folded into dispatch prompts at the orchestrator's discretion and never reached the scoper's skill, so asking for a marketing or sales-assisted trial after the ticket ID could silently fall back to the default paid-org signup.
…ght-test The pipeline now always runs from test plan to execution without pausing, so the optional review gate and the flag that switched it on are gone.
The health check now reports how many required flags it verified, and a flag in the wrong state is one of its failures. The orchestrator must recognize both success forms so it proceeds or halts correctly.
…inishes The orchestrator now hands the written report to open_report.py, which opens it with open on macOS or xdg-open on Linux. The skill grants only that script, not an opener that would accept any file or URL, and the script refuses anything but a report-<timestamp>.html inside a run folder under .playwright-testing-artifacts.
Task 9 put the SERVICES fence's services string and base URL, agent output derived from untrusted feature source, into double-quoted render arguments, where $(...) still expands. They are now checked against tight patterns and single-quoted, and --plan-name takes the ticket key or slug rather than raw description text or a file path. Also stop the run when merge_results.py fails instead of branching on a status that was never read, drop the orchestrator's instruction to research referenced tickets itself (the gatherer owns raw source), use the namespaced compiling-playwright-report skill, narrow the agent non-trigger suite to the three orchestrator-only agents (the other three are standalone-capable by design), and bring the agents' version fields to 1.6.0.
Moves the runner's --agent flag, its unit tests, and the working-tree recipe up from the foundation layer to beside their only consumer, the agent non-trigger suite here. The recipe now loads the two vendor plugins with this one: with only this plugin's directory, claude reports its dependencies as not installed and loads none of its skills. Also restores the README's dependency rows, which this layer's first commit dropped when rebased, drops the eval README's note about the vendor suites (they moved with their plugins), and records in the orchestrator inventory that two of its sibling skills come from the dependency plugins.
…the split inventory Re-run on 2026-10-07 with the two vendor plugins loaded alongside this one: 10/10 and 10/10. One should-trigger query passed at 4/7, and two should-not-trigger queries timed out on every run, so their passes carry no signal. Recorded as measured rather than carrying the 2026-08-01 reading forward.
1 participant
🎟️ Tracking
📔 Objective