Repository navigation
Conversation
abd0ad3 to
d41ee06
Compare
Claude Code validationResult: Issues found Re-validated the Sixteen of the previous run's sixteen findings were re-checked: fifteen persist unchanged, one is partially fixed. The five commits since the last review ( Not covered: Plugin structure, marketplace, and version-bump scripts are run as dedicated workflow steps before this review — see the job log. Spot-checked here: Findings
Dropped after documentation check:
Not re-reported: Checks run
|
a161667 to
d022a01
Compare
d022a01 to
e01e46e
Compare
e01e46e to
bf0ab47
Compare
bf0ab47 to
1467c3f
Compare
1d0649c to
c680164
Compare
c680164 to
f04105b
Compare
f04105b to
9602fc1
Compare
9602fc1 to
d14de3d
Compare
d14de3d to
4d67843
Compare
4d67843 to
9060e9f
Compare
9060e9f to
1ae63ac
Compare
Squashed net diff of the execution layer (health check, test running, report compiling) for a clean reparent.
…fence in execution stage Continues the artifact-contract-fences migration into the execution stage: the health-checker locates the services artifact via its SERVICES fence and the runner locates the test-cases artifact via its TEST-CASES fence, instead of anchoring on markdown headers. Adds malformed-fence eval coverage for both consumers.
…on-following Invoking a Claude skill loads its SKILL.md into the same agent, which then carries out the instructions and produces the artifact itself, rather than a function call that returns a value. Reword the health-checker and test-runner agents' skill invocation steps and their downstream "the skill returns" phrasing. Also reframe playwright-cli usage to match how that skill works — its instructions are loaded once and browser actions run as `playwright-cli` CLI commands, not as a Skill() call per action. Inputs and behavior are unchanged.
…EADME - playwright-test-runner: a skill invocation loads instructions into the agent; it does not hand back a step value. Drop "or Skill(...)" from the mid-run tool-results note — the intermediate values come from Bash. - README: reword the two "Calls the playwright-cli skill" catalog rows to "Drives the browser via the playwright-cli skill's CLI", matching how that skill actually works (instructions loaded once; actions run as playwright-cli CLI commands). No behavior change.
playwright-test-runner and localhost-web-health-checker carried only the generic untrusted-content guardrail at this layer; the nonce guardrail arrived one PR later. Convert both to the per-run UNTRUSTED-SOURCE-<nonce> form so they are born consistent with the planning-phase agents. These read composed/derived artifacts rather than extracting raw source fields, so this is defense-in-depth, not a direct extraction fix. Fold the guardrail note into the 1.5.0 changelog entry; no version bump (draft).
…guardrail policy The two execution-phase agents deferred their untrusted-source rules to "the full rules given in that prompt," which nothing supplies when they run outside the orchestrated pipeline, leaving the guardrail inert. Carry a short rule core inline and point at the shared references/untrusted-source-policy.md, matching the planning-phase agents, and tailor each to what it actually reads: the health checker to the test plan, the runner to the test plan plus the runtime data it receives (email bodies, rendered page content, external-trigger and Stripe tool output). The per-run UNTRUSTED-SOURCE-<nonce> fence is reframed as optional hardening for the orchestrated path. Reword the 1.5.0 changelog bullet to match.
…se guardrails Follow the raw-source removal through the execution layer. The health checker and the test runner lose the now-meaningless "bind to the UNTRUSTED-SOURCE-<nonce> region" sentence; each keeps its tailored treat-as-data core (the health checker over the test plan, the runner over the plan plus runtime data) and the shared-policy pointer. Drop the stale fence-binding sentence from the 1.5.0 changelog bullet.
Drop the pass-inputs framing and the output-schema restatements the checking-localhost-web-health and running-playwright-tests skills already own, and condense the untrusted-source block (kept, not reduced to a bare pointer, since these agents ingest untrusted runtime data and their skills do not restate the policy). The runner's loop invariant, inline-execution rule, and resume/pause control logic are agent-owned and left intact.
…ants
The `*/...` path-glob Bash grants on the execution agents never match (the
permission matcher only matches literal path prefixes, and `${CLAUDE_PLUGIN_ROOT}`
is not expanded in agent `tools:`), so every script hit an approval prompt.
- Move script scoping to the skills' `allowed-tools`, which expand
`${CLAUDE_PLUGIN_ROOT}`/`${CLAUDE_SKILL_DIR}` and auto-approve for a subagent
that follows the skill. Add read_mailcatcher.py and ls to
running-playwright-tests (it runs both directly and forbids the mailcatcher
skill), completing coverage alongside external_trigger.py, read_admin_email.py
(already scoped there) and stripe_cli.py (scoped by using-stripe-cli).
- localhost-web-health-checker -> plain `Bash` (preflight/health-check scripts
are scoped by checking-localhost-web-health).
- playwright-test-runner -> plain `Bash` plus the working literal
`Bash(playwright-cli:*)`. Arbitrary bash still requires approval; only the
skill-scoped scripts and playwright-cli auto-run.
The "Known limits" note described the test runner's Bash grants as leading-wildcard path suffixes, but those grants were removed when script scoping moved to the skills' allowed-tools. Retitle and rewrite the note to the surviving limitation: agents grant unscoped Bash (agent tools: cannot scope to a script path), so scoping lives in the skills and an off-list command is approval-gated rather than blocked, pending a PreToolUse hook.
…wn-limits section The known-limits section and its agent-Bash entry now originate in 1.3.0; this release adds the Category 1 entry and extends the Bash entry to the execution agents. The old line also still described agent script grants, which were removed from the runner.
…checker The planning agents' PreToolUse hook now ships in 1.3.0, and the test runner's eval and run-code payloads can't be held by a command-text allowlist, so the 1.5.0 known-limits note's pending hook refers only to the health checker.
…al Api A run must know whether the flags it depends on are on without asking a person. The running Api's /config is what the web client sees, so it is the authority; secrets.json and Constants.cs are read only for the requested keys, to say whether to restart the Api, set the flag, or switch server branches.
… health check Flag state is an environment requirement like a running service, so the health check now runs the flag check between the service check and the render check, and halts with the script's fix when a flag is wrong. The policy says reading flags is permitted while editing stays forbidden.
… out of the shell unchecked The Api reads dotnet user-secrets, which setup_secrets.ps1 copies from secrets.json, so 'restart the Api' alone could never pick up a fix and left the user looping on the same message. The Resolve lines now say to apply secrets.json first. Flag keys come from planning artifacts that can carry untrusted text, so the health check now validates each key before building the command, single-quotes it after --, and --help no longer exits 0 as if every flag matched.
… /config The flag check looked for a flag's declaration only in Constants.cs, so a flag declared in a library's [FlagKeyCollection] class (such as InvoicingFeatureFlags) was misreported as unknown to the server. /config reports a flag only when a [FlagKeyCollection] class declares it and a value is configured, so for a flag it omits, the check now always inspects both secrets.json and those classes and prints a Problem line for each gap, then one line to refresh secrets and restart the dependent services.
…er contract The health check passed service names from the test plan to the shell verbatim while it regex-checked flag keys from the same untrusted plan, and never enforced the two allowed primary-URL origins before navigating. Both inputs are now checked against closed sets before any command runs, in the skill and in the health-checker agent. The skill also states its missing-playwright-cli and malformed-input halts, which its evals already grade. In the runner: mark a setup failure FAIL (the schema has no FAILED), define <bitwarden git root> as the working directory, stop asserting toasts from screenshots, and abort on missing or malformed test cases. Document that render_report.py also exits 2 on argument errors, list the flag-check grant in the tool policy, and bring the agents' version fields to 1.5.0.
…pply The runner preloaded using-stripe-cli through its skills: frontmatter but never invoked it. A preloaded skill's allowed-tools grant does not apply (verified on Claude Code 2.1.292, headless and interactive), so every stripe_cli.py call needed approval. Only an invoked skill's grant applies, and one invocation covers the rest of the agent's run. running-playwright-tests now invokes bitwarden-mailcatcher-tools:reading-mailcatcher-api and bitwarden-stripe-tools:using-stripe-cli once each, before the first step that needs them. It drops its own grant for the mailcatcher script, which now lives in another plugin, and the rule against invoking the mailcatcher skill. That rule dated from a prototype where the skill was mostly a curl walkthrough, which no longer holds. The runner no longer preloads using-stripe-cli. The exit-3 description no longer mentions MAILCATCHER_URL, whose override was removed earlier.
…r skills Invoking the Mailcatcher and Stripe skills puts their own guidance in the runner's context, and some of it assumes an interactive session: the Mailcatcher exit table says to retry, or to ask the user to start Mailcatcher or extend MAILCATCHER_ALLOWED_HOSTS, and the Stripe skill says to ask the user to attach a test clock. A subagent cannot ask the user mid-run. running-playwright-tests now says its own exit-code branching wins, and treats any "stop and ask the user" from an invoked skill as a [HUMAN] pause. Exit 1 covers all three of the reader's causes (no match, link filter miss, host allowlist rejection), not just NO_MATCH. The runner keeps using its read_admin_email.py rather than the mailcatcher skill's get_admin_email.py. The tool policy's known-limits entry now lists the runner's two scripts and the vendor grants that invoking those skills adds.
… skills in the changelog
…s with a hook The health checker lists unscoped Bash and reads an untrusted test plan, and a script-scoped grant does not work in an agent's tools:. A PreToolUse hook is the only enforcement point, so it allows one simple command at a time from the health-check skill's scripts and the playwright-cli calls render verification makes, and blocks the rest.
…ution agents The agents cannot call tools outside their allowlist, and the tool policy the runner reads through running-playwright-tests already lists the four categories, their wrappers and the stop condition.
Test cases now carry a route's `:<name>` segment, such as `:organizationId`, through as written, but the runner had no rule for filling it. It now takes the value from the browser's URL once an earlier step lands on the resource that step created, and fails the case naming the placeholder when no step did, rather than navigating to the literal segment or guessing an ID. A new behavior eval case covers the unresolved-placeholder failure.
A run spans several dispatches once a [HUMAN] step pauses it, and a resumed dispatch rebuilds only the credentials. The placeholder rule now says dispatch throughout and states that values resolved before a pause are gone, so a resumed case fails fast instead of guessing. The eval purpose now lists cases 7 and 8.
…ents key The manifest's explicit agents list replaces directory discovery, so the health checker and test runner must be listed to load. Also drop the health-checker hook docstring's note about the name forms seen in the wild, matching the planning hook.
f57a05a to
c869f1b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - checking-localhost-web-health | ||
| - playwright-cli | ||
| color: red | ||
| tools: Read, Skill, Bash |
There was a problem hiding this comment.
Bash and Skill but not its Read, and the credential file is kept out of reach by prose alone.
Details and fix
tools: Read, Skill, Bash grants three tools; the hook added in this same changeset covers two of them.
hooks/hooks.json:15sets the matcher to"Bash|Skill", so aReadcall never reaches the hook process.hooks/restrict_health_checker.py:131-135—decide()branches only ontool_name == "Skill"andtool_name == "Bash", then falls through toreturn 0, "".
The only thing keeping this agent out of the dev credential file is a sentence in the skill it loads, skills/checking-localhost-web-health/SKILL.md:53: "Do NOT Read server/dev/secrets.json yourself: it also holds the Stripe test key and the SQL password." That is guidance to a model that, by this agent's own header at lines 13–16, is reading an untrusted test plan. references/playwright-tool-policy.md:85 discloses the gap ("the hook does not restrict the agent's Read"), but disclosure is not a control.
The Bash and Skill halves are genuinely tight, which is what makes the asymmetry worth closing: the hook pins Bash to three named scripts plus four playwright-cli subcommands against two localhost origins, and pins Skill to a two-entry allowlist.
Rated IMPORTANT rather than CRITICAL deliberately. There is no egress path out of this agent — no WebFetch, no WebSearch, and external_trigger.py is not on the hook's Bash allowlist. A secret can enter the agent's context but cannot leave through any tool it holds.
Fix — extend the matcher to Read and deny the secrets path:
"matcher": "Bash|Skill|Read"and in decide(), alongside the existing two branches:
if tool == "Read" and not is_allowed_read(tool_input.get("file_path")):
return 2, READ_BLOCKwhere is_allowed_read rejects any path whose basename is secrets.json under a server/dev parent. Add the blocked case to hooks/tests/test_restrict_health_checker.py.
Reference: https://code.claude.com/docs/en/hooks
1 participant
🎟️ Tracking
📔 Objective