Skip to content

Commit cd9a08f

Browse files
johanzanderclaude
andauthored
fix: unblock sandboxed worktree setup, stop E2E dirtying the tree, guard pushes server-side (#635)
* fix: unblock sandboxed worktree setup, stop E2E dirtying the tree, guard pushes server-side Three environment problems, each with a measured cause. 1. `worktree-setup.sh` could not install Playwright browsers under the sandbox. Writes are `allowOnly` and the browser cache is not in it: mkdir ~/Library/Caches/ms-playwright/probe -> Operation not permitted Probing further turned up a second blocked cache with the same shape: `~/.npm/_cacache`, which breaks every `npm install`/`npm ci`. Neither failure names the sandbox — npm blames "root-owned files" and sends you to `sudo`, and the Playwright step is bounded by a timeout that blames a slow download. Both caches are now in `allowWrite`, both are probed by `verify-sandbox.sh`, and both scripts say what the error actually means. 2. `e2e/ci-wizard-settings.json` was tracked but is pure runtime scratch: `run-e2e.sh` and all 14 wizard steps in `ci.yml` truncate it to `{}` before mounting it, so nothing ever read the committed content, while the container wrote the wizard's result back through the read-write mount. Untracked and gitignored; no consumer changes. The real `ci-bess-settings*.json` fixtures still write back — documented, not changed. 3. `git push` no longer prompts. The blanket ask existed because prefix globbing cannot reach a marker at an arbitrary argument position, but that was compensating for having no guard at the layer that can see a ref update. Four GitHub rulesets now refuse those spellings server-side (main, beta-release-*, all tags, and beta's default branch), all with empty bypass lists — load-bearing, since local pushes authenticate as the owner, not as bess-agent. Feature branches stay unprotected on purpose. `quality-check.sh` pins push in MUST_NOT_BE_GUARDED so the prompt cannot be reinstated out of caution. Sandbox settings are captured once at session start, so the allowWrite change takes effect in a fresh session; `verify-sandbox.sh` confirms it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016tGBHZEAfKzP9qoJU4NdNY * docs: correct the `git push -u` advice — it cannot set upstream under the sandbox The previous bullet recommended `git push -u` as the way to record an upstream after `checkout -b --no-track`. It cannot: `-u` writes `.git/config`, which the same denial covers. Measured while pushing this branch — the ref lands and the command then reports "unable to write upstream branch configuration", which reads as a failed push and is not one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016tGBHZEAfKzP9qoJU4NdNY * docs: stop calling the Playwright hang "the #556 hang" #556 is a CLOSED issue about caching worktree dependencies -- the work during which the hang was first observed, not a ticket tracking it. The existing comments in worktree-setup.sh say that correctly ("observed while working #556"); the shorthand added in the previous commit did not, and pointed readers at an unrelated closed issue. Replaces it with the measurement instead: 1.59.1 stalls at exactly 84 files with the 173MB zip already downloaded and the extractor idle at 0% CPU, while 1.62.1 completes on the same machine. It is extraction, not the network, and there is no issue open for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016tGBHZEAfKzP9qoJU4NdNY * fix: address code review — probe failures that reported SKIP, and a false protection claim Five findings from /code-review on this PR, plus one from #637's review that belongs in this file. verify-sandbox.sh - Both new cache probes printed SKIP, counting NO failure, when the cache directory could not be created -- which under the sandbox is precisely the blocked condition being tested. A fresh machine exited 0 reporting the config safe while npm and Playwright were both about to fail. "Cannot be created" is now folded into the same check() and FAILS. - PLAYWRIGHT_BROWSERS_PATH=0 is Playwright's documented sentinel for "no shared cache", not a path. Treating it as one ran `mkdir -p 0` inside the repo and reported a PASS that measured nothing. Now skipped explicitly. quality-check.sh / CLAUDE.md - The comment claimed GitHub refuses `git push origin --delete release-X.Y`. It does not: the live rulesets cover ~DEFAULT_BRANCH, beta-release-* and tags only, so `release-X.Y` -- the stable hotfix branch the release skill pushes and tags -- is unguarded at BOTH layers. Documented as a known residual with the fix named (a release-* ruleset, not an ask rule) rather than left as a false claim. Creating that ruleset needs approval, so it is not done here. - The feature-branch residual was justified as "damage bounded to a branch nobody has merged". That does not survive this repo's own conventions: ~20 worktrees push in parallel as the SAME identity, so a misaimed --force destroys another agent's commits and closes its PR, and the reflog that would recover it is in a different worktree. Still accepted, now for the real reason. worktree-setup.sh - Sharing compared the two LOCKFILES but never checked that the tree being shared was installed from the lockfile beside it. A dependency bump merged to main updates main's package-lock.json and not its node_modules, so every new worktree would share a tree built from the old lockfile while this script reported success -- which is how the Playwright bump (#637) would have silently reintroduced the very install hang it fixes. Adds install_matches_lockfile(), comparing package-lock.json against node_modules/.package-lock.json. Optional deps are excluded: npm legitimately omits them, and counting that as staleness made a freshly installed e2e tree report STALE over `fsevents` alone -- which would push every worktree through a full install, the exact cost the sharing exists to avoid. Verified against real trees: main/e2e MATCHES, bump worktree MATCHES, and a simulated post-#637 state (1.62.1 lockfile beside a 1.59.1 install) correctly reports STALE. It also flags main/frontend as genuinely stale -- 84 real version mismatches, so that one wants an `npm ci`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016tGBHZEAfKzP9qoJU4NdNY --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 5bd9e9d commit cd9a08f

8 files changed

Lines changed: 431 additions & 159 deletions

File tree

.claude/settings.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@
1515
"filesystem": {
1616
"allowWrite": [
1717
".",
18-
"~/GitHub/bess-manager"
18+
"~/GitHub/bess-manager",
19+
"~/Library/Caches/ms-playwright",
20+
"~/.cache/ms-playwright",
21+
"~/.npm"
1922
]
2023
}
2124
},
@@ -86,9 +89,6 @@
8689
"Bash(git -* stash store*)"
8790
],
8891
"ask": [
89-
"Bash(git push)",
90-
"Bash(git push *)",
91-
"Bash(git -* push*)",
9292
"Bash(gh api)",
9393
"Bash(gh api *)",
9494
"Bash(gh pr merge*)",

.claude/skills/verify/SKILL.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,16 @@ port to find real entity IDs/attributes to restore.
7171
## Gotchas
7272

7373
- `${BESS_SETTINGS:-./e2e/ci-bess-settings.json}` is mounted **read-write**
74-
(no `:ro`). Running the app against it can silently write settings back
75-
into the fixture (schema migrations, demo_mode defaults, etc). After
76-
tearing down: `git diff -- e2e/` and `git checkout -- e2e/` if the only
77-
changes are ones you didn't intend.
74+
(no `:ro`), and it has to be — that mount *is* how the app persists
75+
settings, so `:ro` would break the wizard rather than protect the file.
76+
Running the app against it therefore writes settings back into the fixture
77+
(schema migrations, demo_mode defaults, etc). After tearing down:
78+
`git diff -- e2e/` and `git checkout -- e2e/` if the only changes are ones
79+
you didn't intend.
80+
`e2e/ci-wizard-settings.json` is the exception and needs no such reset: it
81+
is gitignored, because every consumer truncates it to `{}` before mounting
82+
it and nothing reads its content. The `ci-bess-settings*.json` files are
83+
real fixtures whose content matters, so those still dirty the tree.
7884
- Many scenarios (e.g. `ci-wizard-entsoe.json`) pin `mock_time` to a fixed
7985
past date (`ci-normal-day.json``2025-01-15`), not "today" — the real
8086
container clock is today's date, so date-anchored service calls (Nordpool

.gitignore

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,14 @@ e2e/test-results/
7171
# Mock HA settings generated for CI (derived from scenario)
7272
backend/mock-bess-settings.json
7373

74+
# Wizard E2E scratch settings. NOT a fixture, despite living beside the real
75+
# ones: every consumer (e2e/run-e2e.sh, each wizard step in ci.yml) truncates
76+
# it to `{}` immediately before mounting it, precisely so the wizard triggers
77+
# fresh -- so nothing ever reads what is committed here. The container then
78+
# writes the wizard's result back through the read-write mount, which used to
79+
# leave the working tree dirty after every local E2E run.
80+
e2e/ci-wizard-settings.json
81+
7482
# Scratch fixtures generated by scripts/demo_health_recovery.sh
7583
.demo-scratch/
7684

CLAUDE.md

Lines changed: 196 additions & 48 deletions
Large diffs are not rendered by default.

e2e/ci-wizard-settings.json

Lines changed: 0 additions & 84 deletions
This file was deleted.

scripts/quality-check.sh

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -171,13 +171,41 @@ if ! python3 - <<'PY'
171171
import json, re, sys
172172
173173
# Patterns match the command AS WRITTEN -- prefix globbing, no normalisation.
174-
# `git push` and `gh api` are guarded by a BLANKET rule on purpose: the
175-
# dangerous shapes put their marker at an arbitrary argument position
176-
# (`git push origin main --force`, `git push origin +beta-release-9.9`,
177-
# `git push origin --delete release-X.Y`, `gh api <path> -X PUT`), which a
178-
# prefix glob cannot reach. Enumerating them left real holes twice. Narrowing
179-
# these two back to specific forms re-opens the holes, so the check requires
180-
# the blanket spelling rather than merely "some rule exists".
174+
# `gh api` is guarded by a BLANKET rule on purpose: the dangerous shapes put
175+
# their marker at an arbitrary argument position (`gh api <path> -X PUT`,
176+
# `gh api <path> -f k=v`), which a prefix glob cannot reach. Enumerating them
177+
# left real holes twice. Narrowing it back to specific forms re-opens the
178+
# holes, so the check requires the blanket spelling rather than merely "some
179+
# rule exists".
180+
#
181+
# `git push` USED to be in that same sentence and no longer is. It was never
182+
# the glob that made it safe -- the glob was a blunt instrument compensating
183+
# for having no guard at the only layer that can actually see a ref update.
184+
# The four leaks it was blanket-guarding against are now refused SERVER-SIDE
185+
# by GitHub rulesets, which evaluate the resulting ref rather than the command
186+
# string, so argument order cannot evade them:
187+
#
188+
# git push origin main --force -> non_fast_forward on ~DEFAULT_BRANCH
189+
# git push origin +beta-release-9.9 -> non_fast_forward on beta-release-*
190+
# git push origin --delete <ref> -> deletion on both of the above
191+
# git push origin v9.9.0 (force/move) -> non_fast_forward on ~ALL tags
192+
#
193+
# All rulesets are enforcement=active with an EMPTY bypass_actors list, so
194+
# they bind the repo owner too -- which matters, because local pushes
195+
# authenticate as the owner (osxkeychain), never as bess-agent.
196+
#
197+
# Do NOT re-add a `Bash(git push*)` ask on the grounds that "a guard is
198+
# missing". Check the rulesets first:
199+
#
200+
# gh api repos/johanzander/bess-manager/rulesets
201+
# gh api repos/johanzander/bess-manager-beta/rulesets
202+
#
203+
# What this deliberately does NOT cover: force-pushing or deleting a FEATURE
204+
# branch (fix/**, feat/**) on origin, nor `release-X.Y` (see MUST_NOT_BE_GUARDED
205+
# below). Accepted residuals -- but NOT because "the damage is bounded to your
206+
# own unmerged branch". ~20 worktrees push in parallel as the same identity, so
207+
# a misaimed --force destroys another agent's commits and closes its PR, with
208+
# the recovering reflog sitting in a different worktree.
181209
#
182210
# Every entry below is a rule whose deletion is the exact regression this gate
183211
# was written for -- the GitHub-reaching and history-destroying guards. Keep
@@ -191,7 +219,6 @@ REQUIRED = {
191219
"Bash(podman machine rm)", "Bash(podman system reset)",
192220
],
193221
"ask": [
194-
"Bash(git push)", "Bash(git push *)", "Bash(git -* push*)",
195222
"Bash(gh api)", "Bash(gh api *)",
196223
"Bash(gh pr merge*)", "Bash(gh repo edit*)",
197224
"Bash(gh release create*)", "Bash(gh release edit*)",
@@ -241,18 +268,9 @@ MUST_BE_GUARDED = [
241268
# git's global options may precede the subcommand -- the hook this
242269
# replaced normalised for exactly this, and CLAUDE.md teaches `git -C` as
243270
# the cross-checkout idiom, so it is the spelling most likely to be used.
244-
"git -C .claude/worktrees/x push origin main",
245-
"git -c push.default=current push beta main",
246271
"git --no-pager gc --prune=now",
247272
"git -C sub tag -d v9.9.0",
248273
"git -C sub update-ref -d refs/heads/x",
249-
# the marker sits at an arbitrary argument position
250-
"git push origin main --force",
251-
"git push origin +beta-release-9.9",
252-
"git push origin --delete release-9.9",
253-
"git push origin v9.9.0",
254-
"git push -u origin main",
255-
"git push",
256274
# history destruction, incl. the spellings that are NOT `gc`/`reflog expire`
257275
"git tag --delete v9.9.0", "git tag -d v9.9.0",
258276
"git tag -f v9.9.0 abc123",
@@ -308,6 +326,35 @@ MUST_NOT_BE_GUARDED = [
308326
"git -C .claude/worktrees/x diff -- file.py",
309327
"git -C .claude/worktrees/x apply",
310328
"git status", "git diff", "git log --oneline",
329+
# Pushing. Enforcement moved to GitHub rulesets (see the REQUIRED comment
330+
# above), so these must run UNATTENDED -- implement-issue Step 9 and every
331+
# PR-refresh push. Pinned in this direction on purpose: the failure mode
332+
# being guarded against is someone re-adding a `Bash(git push*)` ask
333+
# because it "looks unguarded", which would silently restore the stall the
334+
# rulesets were created to remove. If you believe a guard is missing, read
335+
# the rulesets before touching this list.
336+
#
337+
# These entries assert only that the commands are LOCALLY unguarded. Which
338+
# of them GitHub also refuses is a separate question, and the answer is not
339+
# "all of them" -- do not read this list as a protection matrix:
340+
#
341+
# main --force, +beta-release-*, v9.9.0 (tag) -> refused by a ruleset
342+
# --delete release-X.Y -> NOT refused; no ruleset
343+
# covers `release-*`
344+
#
345+
# That last line is a real residual, not an oversight to "fix" by putting
346+
# the prompt back: `release-X.Y` is the short-lived hotfix branch created
347+
# by the release skill (steps 2-6), it is pushed and tagged, and nothing
348+
# currently protects it at either layer. The protected TAG preserves the
349+
# released commit, so the branch is recoverable after tagging but not
350+
# before. Closing it means adding a `release-*` ruleset, not an ask rule.
351+
"git push", "git push -u origin main",
352+
"git push origin main --force",
353+
"git push origin +beta-release-9.9",
354+
"git push origin --delete release-9.9",
355+
"git push origin v9.9.0",
356+
"git -C .claude/worktrees/x push origin main",
357+
"git -c push.default=current push beta main",
311358
# Read-only stash inspection, which CLAUDE.md and rules.md both promise
312359
# keeps working. A blanket `git -* stash *` DENY caught these, and deny has
313360
# no override -- so the cross-checkout recipe was hard-blocked, not merely

scripts/verify-sandbox.sh

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,80 @@ else
178178
"add the main checkout to sandbox.filesystem.allowWrite -- nested worktrees and every symlink target are then covered by one entry. Without it vitest and vite build fail EPERM in every worktree. NOTE: a SIBLING checkout (../bess-manager-feature) is NOT under that path and needs its own entry."
179179
fi
180180

181+
# 4d. THE TWO USER-LEVEL CACHES scripts/worktree-setup.sh WRITES. Both sit well
182+
# outside the repo, so neither the "." nor the "~/GitHub/bess-manager"
183+
# entry reaches them, and the built-in allowOnly list covers only
184+
# ~/.npm/_logs -- not the package cache next to it. Setup does BOTH: it
185+
# runs `npm install` when a lockfile has diverged from the main checkout,
186+
# and then `npx playwright install chromium`.
187+
#
188+
# What makes this worth a check rather than a comment is that NEITHER
189+
# failure names the sandbox. npm reports EPERM on ~/.npm/_cacache as
190+
# "Your cache folder contains root-owned files ... please run: sudo chown
191+
# -R", sending you to `sudo` -- which is on the ask list and is entirely
192+
# the wrong fix. And worktree-setup.sh's Playwright step is bounded by a
193+
# timeout whose message blames a slow download, so a blocked cache reads
194+
# as the unrelated extraction hang described in worktree-setup.sh (which
195+
# is a real, separate failure -- do not conflate the two). Both the npm and
196+
# the Playwright blocks were observed here in one session, and the
197+
# Playwright one cost a whole worktree's setup.
198+
#
199+
# 4d-i. The Playwright browser cache. Browsers are deliberately NOT
200+
# per-worktree -- one shared user-level cache serves every checkout, so
201+
# redirecting PLAYWRIGHT_BROWSERS_PATH into the repo is not the fix.
202+
# mkdir rather than touch, and create the cache dir first: it may not
203+
# exist yet on a fresh machine, and creating it is the first thing a
204+
# real install does.
205+
# `PLAYWRIGHT_BROWSERS_PATH=0` is Playwright's documented sentinel for
206+
# "no shared cache, keep browsers inside node_modules" -- NOT a path.
207+
# Treating it as one makes the probe `mkdir -p 0` in the repo, which
208+
# succeeds and reports a PASS that measured nothing.
209+
pw_sentinel=no
210+
if [ "${PLAYWRIGHT_BROWSERS_PATH:-}" = "0" ]; then
211+
pw_sentinel=yes
212+
elif [ -n "${PLAYWRIGHT_BROWSERS_PATH:-}" ]; then
213+
pw_cache="$PLAYWRIGHT_BROWSERS_PATH"
214+
elif [ "$(uname -s)" = "Darwin" ]; then
215+
pw_cache="$HOME/Library/Caches/ms-playwright"
216+
else
217+
pw_cache="$HOME/.cache/ms-playwright"
218+
fi
219+
220+
if [ "$pw_sentinel" = yes ]; then
221+
printf 'SKIP PLAYWRIGHT_BROWSERS_PATH=0 -- browsers live in node_modules, no shared cache to probe\n'
222+
else
223+
# A cache directory that cannot even be CREATED is the blocked case, not an
224+
# inapplicable one. Reporting SKIP here (and counting no failure) is how a
225+
# fresh sandboxed machine exits 0 saying the config is safe while both npm
226+
# and Playwright are about to fail -- the exact condition this check exists
227+
# to catch. Fold it into the same `check`, so it FAILS.
228+
mkdir -p "$pw_cache" 2>/dev/null
229+
if [ -d "$pw_cache" ]; then
230+
mkdir "$pw_cache/.sandbox-probe" 2>/dev/null && pw=allowed || pw=blocked
231+
rmdir "$pw_cache/.sandbox-probe" 2>/dev/null
232+
else
233+
pw=blocked
234+
fi
235+
check "writing the Playwright browser cache is allowed" allowed "$pw" \
236+
"add the browser cache to sandbox.filesystem.allowWrite (~/Library/Caches/ms-playwright on macOS, ~/.cache/ms-playwright on Linux). Without it scripts/worktree-setup.sh cannot install browsers, and its timeout reports that as a stalled download -- indistinguishable from the separate extraction hang that timeout was written for. Do NOT point PLAYWRIGHT_BROWSERS_PATH into the repo to dodge this: one shared cache across every worktree is the design."
237+
fi
238+
239+
# 4d-ii. The npm package cache. ~/.npm/_logs is writable by default and
240+
# ~/.npm/_cacache is not, so npm looks fine right up until it has to
241+
# fetch something -- `npm install` in worktree-setup.sh (diverged
242+
# lockfile), `npm ci` in e2e/run-e2e.sh (missing frontend/dist), or any
243+
# npx that misses the local node_modules.
244+
# Same rule as above: "could not be created" IS blocked. Never SKIP it.
245+
mkdir -p "$HOME/.npm/_cacache" 2>/dev/null
246+
if [ -d "$HOME/.npm/_cacache" ]; then
247+
mkdir "$HOME/.npm/_cacache/.sandbox-probe" 2>/dev/null && npmc=allowed || npmc=blocked
248+
rmdir "$HOME/.npm/_cacache/.sandbox-probe" 2>/dev/null
249+
else
250+
npmc=blocked
251+
fi
252+
check "writing the npm package cache is allowed" allowed "$npmc" \
253+
"add ~/.npm to sandbox.filesystem.allowWrite. The built-in allow list covers ~/.npm/_logs only, so npm can write its log and not its cache. IGNORE the error npm prints -- it claims the cache 'contains root-owned files' and tells you to run 'sudo chown -R 502:20 ~/.npm'. That is npm guessing at EPERM; the files are not root-owned and sudo is not the fix."
254+
181255
# 5. gh. `gh pr create` is the closing step of implement-issue, and gh is a Go
182256
# binary: under the macOS sandbox it cannot reach trustd to verify TLS,
183257
# which surfaces as x509: OSStatus -26276.

0 commit comments

Comments
 (0)