[melodic-automation-lanes]

Closes #11902

Summary

Opened by the work-resolve-issue lane. The run's own summary:

The playwright SessionStart hook (plugins/playwright/hooks/install-python-deps.sh) now exits before installing numpy and Pillow when CLAUDE_PLUGIN_DATA has a path segment starting with claude-eval-. That is the per-run sandbox claude plugin eval creates, so a trigger-only eval case installs nothing and its measured run gets no install notice. Of the issue's two options I chose the eval-sandbox skip over a lazy install. A lazy install would break contract Rule P2 (docs/contracts/on-demand-dependencies/README.md: install from a hook, never while a skill runs; pydeps.py run never installs). The skip leaves that contract and pydeps.py unchanged, and a demo-video eval case still gets pydeps.py run's repair line. For detection: the plugin-eval docs name no environment variable or marker for an eval run, and the run's environment is an allowlist. So the hook matches the sandbox directory name instead: the issue's du evidence (config/plugins/data/playwright-inline inside each kept run directory), plugins/evals/skills/plugin-eval/reference/sandboxes.md (sandboxes named claude-eval-* holding config/), and the repo's eval trace samples (claude-eval-x/home, claude-eval-ab12/plugins). Backslashes are normalized first, so a Windows path matches too. If the undocumented name ever changes, the hook goes back to installing as it does today.

Not verified

• plugins/playwright/hooks/install-python-deps.test.sh was not executed: this lane's Bash runs only the named wrappers, so the suite is left to the PR's CI
• The eval-sandbox signal (a claude-eval-* segment in CLAUDE_PLUGIN_DATA) rests on observed sandbox layout, not on documentation: the plugin-evals page names no eval marker. A real claude plugin eval run of the playwright plugin was not made here to confirm the hook skips
• Known trade-offs from review, left as they are: a normal session whose data path has a segment starting claude-eval- also skips the install (pydeps.py run still prints the repair line), and this test suite would fail if it ran inside an eval session whose TMPDIR sits under claude-eval-*
• tidy: the ai-slop pass could not run its detector script (Bash runs only the named wrappers), so the changelog fragment got a manual rubric read only; nothing was found. tidy-diff also forbids test edits, so a redundant installs check in the new test was left in
• review-venue.txt was absent, so the review ran locally in one fresh-context subagent
• bash lint (shellcheck/shfmt) on the touched scripts was not run separately; check-affected's passing set covers what it runs here, and CI runs the rest

Fix

• plugins/playwright/hooks/install-python-deps.sh: exit 0 before any install when CLAUDE_PLUGIN_DATA has a claude-eval-* path segment (backslashes normalized), with a comment giving the reason
• plugins/playwright/hooks/install-python-deps.test.sh: new case showing a SessionStart whose data directory is inside a claude-eval-* sandbox (POSIX and backslash forms) exits 0, prints nothing and creates nothing; it runs from $WORK so a regressed hook cannot write outside the test's temp directory
• .changes/playwright/work-resolve-issue-issue-11902-9c4e1b7a.md: patch changelog fragment for the fix

Verification

The runner ran each check from the base copy against 8d3a11b:

• scripts/check-changelog-fragments.sh --check: passed

The run reports:

• read https: //code.claude.com/docs/en/plugin-evals: each run gets a temporary home, working directory and Claude Code configuration; only an allowlist of variables (PATH, locale, proxy, provider, most ANTHROPIC_* and CLAUDE_CODE_) and EVAL_ reach the run; --keep-temp keeps each run's sandbox direc
• check-changelog-fragments: All 75 changelog fragment(s) under .changes/ are valid (run before and after tidy and after the review fix)
• check-affected: 12 passed, 6 skipped over 3 changed files (run before and after tidy and after the review fix)
• cant-fail-scan --check --file plugins/playwright/hooks/install-python-deps.test.sh: PASS, no gating findings, 1 test file fully read
• fresh-context review subagent: no correctness bugs; the pattern matches POSIX and backslash claude-eval-* paths and not ordinary data paths or a my-claude-eval-x segment; the flow and exit status are unchanged on a non-match; the new test fails if the skip line is removed. Its one actionable finding
• The lane run: https://github.com/melodic-software/claude-code-plugins/actions/runs/41293811582

Left out: the items under Not verified.

Related

#11902

[github-actions]

claude-security-review reviewed 81dc6e07e13bed5bb8782803f1799f2c58480561: read 7 of 7 changed file(s); 0 finding(s), 0 as inline comments.

Security review (incremental since 8d3a11b): no security issues found

What changed since the last review

• plugins/playwright/hooks/install-python-deps.sh no longer checks the data path for a claude-eval-* segment to decide whether to skip the install. It now exits 0 before installing anything when EVAL_PLAYWRIGHT_SKIP_PYDEPS is exactly 1.
• The four playwright eval cases set env: {EVAL_PLAYWRIGHT_SKIP_PYDEPS: "1"}.
• The test suite unsets the variable at the start, then checks the skip and six values that must not skip.
• The changelog fragment is reworded to match.

What I checked

• New hook input. The variable is read once, compared to the literal 1 inside [[ ... ]], and is never expanded into a command, path or pip argument. That leaves no way to inject anything through it. The only thing it can do is stop the hook from installing numpy and Pillow. That means less code runs, not more, so no token, secret or execution path gets wider. If someone could set the variable in a user's environment, the worst case is that the demo-video packages are missing. pydeps.py run still prints its repair line and exits, so nothing fails silently and nothing new reaches a security boundary.
• Removed path-pattern skip. Taking out the claude-eval-* check means the hook installs in more cases than before. It runs the same install that already ran in every normal session, from the same source. The install source, pinning and wheel handling are unchanged in this diff.
• Eval prompt.md env keys. These are data the eval harness passes to the run under the documented EVAL_* allowlist. They grant no tools: allowed_tools is unchanged at [Read, Glob, Grep, Skill]. They also don't widen what reaches a credentialed lane.
• Test file. unset at the top and new temp-directory cases under $WORK. No security surface.
• Other checks. No plugin manifest (plugin.json or marketplace.json) changed, so there are no manifest lines to report. No changed file matches a trusted-path glob, and no trust control, merge hold or standing instruction is narrowed. The PR context contains no embedded instructions aimed at this lane.

Spend: the change reduces eval spend (no 91 MB install per trigger-only case). Nothing in it can bill the owner.

critical-findings: 0

[github-actions]

claude-review: 1 inline finding(s) on 8d3a11b38b62cb8baad6c54e9e1ed1a93d65a819; the summary is in this lane's pull request comment.

claude-lane: claude-review

[github-actions]

claude-review reviewed 81dc6e07e13bed5bb8782803f1799f2c58480561: read 7 of 7 changed file(s); 0 finding(s), 0 as inline comments.

Code review (incremental since 8d3a11b): no findings

This update drops the claude-eval-* path-segment check. In its place, the hook now skips the install when the variable EVAL_PLAYWRIGHT_SKIP_PYDEPS=1 is set, and each of the plugin's eval cases sets that variable. That is what the decision comment on #11902 asked for: a plugin-defined EVAL_* variable, set by the plugin's own eval cases, with on-demand-dependencies rule P2 and pydeps.py run left unchanged.

What I checked

• plugins/playwright/hooks/install-python-deps.sh: [[ "${EVAL_PLAYWRIGHT_SKIP_PYDEPS:-}" == 1 ]] && exit 0 runs before any other work. The right-hand 1 has no glob characters, so the match is exact, and set -u is safe because of the :- default. When the variable is unset or holds any other value, the hook runs as it did before this PR. The header comment matches the new behavior.
• Eval cases: all four plugins/playwright/evals/*/prompt.md files are in the diff, and each adds env: {EVAL_PLAYWRIGHT_SKIP_PYDEPS: "1"}. That is a mapping, and the key matches EVAL_[A-Z0-9_]*. plugins/evals/skills/validate/scripts/validate-cases.py (around L680–L696) fails any env that is not a mapping or has a key outside that pattern, so these pass. No demo-video eval case exists, so no case needs the variable left unset.
• install-python-deps.test.sh:
  • The new unset at the top keeps a variable exported by the operator from silently skipping the install cases.
  • The =1 case fails if someone removes the skip line, because it checks ! -e "$data/python".
  • The value loop (unset, '', 0, true, ' 1', 1x) shows that only an exact 1 skips. The data directory names for these values are all different: 5-unset, 0-, 1-0, 4-true, 2-_1, 2-1x.
• Changelog fragment: it now describes the env-variable behavior, says four eval cases set it, and keeps bump: patch, which fits a fix.

Closing issue #11902

• The acceptance criterion "Add a test showing that a SessionStart under the eval sandbox installs nothing" is met by the EVAL_PLAYWRIGHT_SKIP_PYDEPS=1 test case, as the decision comment redefined the eval signal.
• One question stays open, and the decision comment itself left it undecided: does a live claude plugin eval run pass a case's env to the plugin's SessionStart hook process? Neither the PR nor I verified that. A unit test cannot settle it; it takes one real eval run of a playwright case. If env does not reach the hook, the cases go back to installing, which is the old behavior rather than a breakage.

Note (not counted): the PR description in .claude-lane/pr-context.md still describes the old approach: claude-eval-* path-segment detection, the "Not verified" trade-offs for that pattern, and a test that uses POSIX and backslash sandbox paths. All of that has been replaced. Update the description so the merge record matches what ships.

I ran nothing myself. The PR's verification claims, including that check-changelog-fragments passed, come from the author and I have not confirmed them.

[github-actions]

The pr-verify verifier failed head 8d3a11b38b62cb8baad6c54e9e1ed1a93d65a819. What it wrote, as data:

Summary: Issue #11902's spec includes a trusted human's Decision comment with acceptance criteria.
  Those criteria require the hook to skip the install only when the opt-in variable
  `EVAL_PLAYWRIGHT_SKIP_PYDEPS=1` is set, the plugin's four eval cases to set it, an Exceptions row
  in the on-demand-dependencies contract, and a changelog fragment that names the variable. The PR
  does something else. It detects a `claude-eval-*` segment in the CLAUDE_PLUGIN_DATA path, which is
  the undocumented signal the decision explicitly rejected. It also leaves out the eval-case,
  contract and changelog items. The two named options to install lazily or skip in the sandbox come
  from the issue's Fix section, but the decision chose between them. CI passed, but the diff does
  not deliver the spec.
- plugins/playwright/evals/: not touched. Each of the four cases
  (`expired-login-asks-owner-to-resave`, `loads-saved-login-after-open`,
  `named-session-snapshot-click-by-ref`, `never-reads-saved-login-file`) must set `env:
  {EVAL_PLAYWRIGHT_SKIP_PYDEPS: "1"}` in its prompt.md frontmatter.
- docs/contracts/on-demand-dependencies/README.md: not touched. Add an Exceptions table row covering
  the playwright demo-video set and rule P2. It must name the variable and give the reason: eval
  runs that never use demo-video install nothing. Leave the P2 text unchanged.
- .changes/playwright/work-resolve-issue-issue-11902-9c4e1b7a.md:13: the fragment describes the
  path-based skip. It must name `EVAL_PLAYWRIGHT_SKIP_PYDEPS` and say that an eval case for
  demo-video must leave it unset.

[kyle-sexton]

pr-fix lane: please fix this finding. Per the decision on #11902, set env: {EVAL_PLAYWRIGHT_SKIP_PYDEPS: "1"} in the four non-demo-video playwright eval cases, add the Exceptions row for rule P2 in docs/contracts/on-demand-dependencies/README.md (P2 text unchanged), and reword the changelog fragment to name the variable and say a demo-video case must leave it unset; at head 8d3a11b3 none of the three is done.

[melodic-automation-lanes]

pr-unblock / address-feedback: UNCERTAIN, left open

Evidence: .changes/playwright/work-resolve-issue-issue-11902-9c4e1b7a.md:7, plugins/playwright/evals/expired-login-asks-owner-to-resave/prompt.md:8

Two of the three items in the direction are done in this round. First, the four non-demo-video playwright eval cases (expired-login-asks-owner-to-resave, loads-saved-login-after-open, named-session-snapshot-click-by-ref, never-reads-saved-login-file) set env {EVAL_PLAYWRIGHT_SKIP_PYDEPS: "1"} in their prompt.md frontmatter. Second, the changelog fragment now names the variable and says an eval case for /playwright:demo-video must leave it unset. The third item is still open: the Exceptions row for rule P2 in docs/contracts/on-demand-dependencies/README.md. This lane cannot edit that file. Its path check refuses it as outside every kept thread's scope. A person or another lane needs to add the row: it names EVAL_PLAYWRIGHT_SKIP_PYDEPS, covers the playwright demo-video set and rule P2, gives the reason (eval runs that never use demo-video install nothing), and leaves the P2 text unchanged.

[melodic-automation-lanes]

Unattended merge: native auto-merge leaves this pull request unarmed at head 8d3a11b38b62cb8baad6c54e9e1ed1a93d65a819. The lanes clear this reason on their own; the next scheduled run judges it again.

pr-merge's gate refuses its verifier verdict (verify:fail)

[melodic-automation-lanes]

pr-unblock / address-feedback finished round 1 of max 4 on 81dc6e0.

[melodic-automation-lanes]

The pr-unblock lane stopped on this pull request and needs a person: a disputed review finding is one only a person decides (a CRITICAL finding, a security-review finding with no rating the lane reads, or a thread a person opened or directed).

• Reason: feedback-disputed
• Resume key: 81dc6e07e13bed5bb8782803f1799f2c58480561 (the head the lane stopped on)

To resume, deal with the reason above, remove the needs-human label (and do-not-merge, if set), then dispatch the pr-unblock workflow with pr-number=11923 and activity=address-feedback:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=11923 -f activity=address-feedback

[kyle-sexton]

Hold: replaced by an owner-login PR because the fix lane's path check refuses the contract row the #11902 decision requires. Releases when: the replacement merges and this PR is closed.

[kyle-sexton]

Superseded by #11929, which carries this change plus the on-demand-dependencies Exceptions row the #11902 decision requires.