fix: unblock sandboxed worktree setup, stop E2E dirtying the tree, guard pushes server-side - #635
Conversation
…ard 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
… 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
#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
…alse 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
…iewer" The Stage 4 bot only ever acts when triggered by an `@claude-bot review` comment. So "green, no verdict yet" has two completely different meanings, and only one of them belongs to the reviewer: request NEWER than last push -> the bot is genuinely working: reviewer request OLDER, or absent -> nobody has asked: DISPATCHER Collapsing those parked six of eleven open PRs on someone who had not been asked and was never going to act. Measured on the live fleet: #637, #635 never requested at all #620 requested 17:40:53, pushed 18:33:05 #619 requested 10:50:40, pushed 21:53:00 #614 requested 06:55:38, pushed 07:08:20 #490 requested 08-15 13:41, pushed 08-16 15:30 Every one had been reported as `awaiting-review [reviewer]`. After this change the same fleet shows zero PRs waiting on the reviewer and zero waiting on the maintainer — the pipeline owes an action on all of them. That is the failure this whole branch is about, seen from the other side. The gate stops a confused loop asking too often; this surfaces the loop that stopped asking at all. Both are the same lost state — whether the last verdict has been consumed — and both are recoverable from the PR rather than from a session that died. `comments` joins the field set for this. It fits inside the GraphQL node budget at --limit 30, which is already bounded by `commits`. Verified by mutation: disabling the request-feed check reddens test_a_review_never_requested_is_the_dispatchers_turn_not_the_reviewers and test_a_push_after_the_last_request_owes_a_new_round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude-bot review |
There was a problem hiding this comment.
Summary
Self-reported (no linked issue) — three environment problems each backed by measured evidence in the PR body (sandbox EPERM traces, git-diff dirtying, push-guard gap analysis). This is the PR's second review round; the last commit (8a1971d) already addressed 5 findings from a prior /code-review plus one from #637's review, including correcting an earlier false claim that GitHub rulesets cover release-X.Y branch deletion (they don't — now honestly disclosed as a residual gap).
Verified independently:
e2e/ci-wizard-settings.jsondeletion: confirmed viagit grepthat every consumer (14ci.ymlsteps +run-e2e.sh:96) truncates it to{}before mounting — nothing reads the committed content, so deleting it and adding it to.gitignoreis safe.scripts/quality-check.sh's embedded python permission-gate: extracted and ran it standalone against this branch's.claude/settings.json— passes clean (76 command shapes checked, 20 require deny, 26 must stay unattended), confirmingMUST_NOT_BE_GUARDED/MUST_BE_GUARDED/MUST_BE_DENIEDare consistent with the actual settings diff (thegit push*ask lines are correctly gone fromask, and re-asserted as required-unattended).scripts/worktree-setup.sh's newinstall_matches_lockfile()python heredoc:ast.parseclean, logic checked — comparespackage-lock.jsonvsnode_modules/.package-lock.jsonper-package version, skips optional deps (correctly, to avoid false STALE onfsevents-only installs), and defaults to "matches" when either file is missing/unparseable (documented as intentional — no evidence, no forced reinstall).- All three touched shell scripts pass
bash -n. The loneshellcheckhit (SC2001,quality-check.sh:455) is on an untouched pre-existing line, not introduced by this diff. release-X.Yresidual: cross-checked against.claude/skills/release/SKILL.mdsteps 92–98 — the branch name matches exactly, and step 97 already requires "explicit user approval before each push/tag/release," so the disclosed gap (unguarded at both local-ask and GitHub-ruleset layers) is mitigated in practice by an existing manual approval gate, not silently open.
One thing I could not verify: the entire safety case for removing the blanket Bash(git push*) ask rests on four GitHub rulesets (Protect Main Branch, Protect beta release branches, Protect release tags, Protect beta main) actually existing with enforcement=active and an empty bypass_actors list. gh api repos/johanzander/bess-manager/rulesets is permission-gated for this bot and I could not run it. Since this is the load-bearing claim of the PR — if the rulesets aren't exactly as described, git push --force/--delete on protected refs becomes unguarded at both layers — please confirm with gh api repos/johanzander/bess-manager/rulesets and gh api repos/johanzander/bess-manager-beta/rulesets before merging. Given this is already the second review pass and the PR explicitly names this as the verification step (and states "pushing this branch ran unattended, confirming the rule change"), I'm not blocking on it, but it's worth a manual confirm since I structurally can't do it.
Scope: minimal and on-topic — sandbox permissions, .gitignore, script docs/comments, and the worktree lockfile-staleness fix. No unrelated code touched. No Python/TS source under core/, backend/, or frontend/ is touched, so most of the architecture/API-layer checklist doesn't apply here.
Rule violations: none found.
No blockers — approving.
The `Protect stable hotfix branches` ruleset was created after #635 merged (id 20972632: `refs/heads/release-*`, non_fast_forward, enforcement=active, empty bypass_actors). CLAUDE.md and quality-check.sh still carried the warning that `release-X.Y` was unguarded at both layers, which is no longer true -- and a doc asserting a gap that has been closed misleads exactly as much as one asserting a guard that does not exist. Adds the row to the ruleset table and replaces the warning with the reason for the one asymmetry in it: `release-*` blocks non_fast_forward and deliberately NOT deletion. The branch is pushed and tagged, so rewriting it must fail; deleting it once the release is out is ordinary cleanup and must not. The tag ruleset one row up is what makes dropping the deletion guard safe -- the published tag pins the released commit. Careful with the summary line above the table: `--delete release-X.Y` is still permitted, so "refuse every line above" would be false. It refuses every REWRITE. Written that way once in this commit's first draft and corrected before pushing. Claude-Session: https://claude.ai/code/session_016tGBHZEAfKzP9qoJU4NdNY Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three environment problems reported after a worktree session stalled. Each has a measured cause, not a guessed one.
1. The sandbox blocks the caches
worktree-setup.shwritessandbox.filesystem.allowWritewas[".", "~/GitHub/bess-manager"]. Writes areallowOnly, and the Playwright browser cache is not in it:Probing turned up a second cache with the same shape:
~/.npm/_cacache. Only~/.npm/_logsis granted by default, so npm can write its log and not its cache — breakingnpm installinworktree-setup.shandnpm ciinrun-e2e.sh.Neither failure names the sandbox, which is why this cost a whole worktree setup:
sudo chown -R 502:20 ~/.npm" — a guess. Nothing is root-owned, andsudois on the ask list.worktree-setup.sh's own timeout, whose message blames a slow download — indistinguishable from the real Speed up local verification: cache/symlink worktree dependencies instead of reinstalling #556 hang.Both caches are added to
allowWrite, probed byverify-sandbox.sh, and both scripts now say what the error actually means.2. E2E wrote settings back into a tracked file
e2e/ci-wizard-settings.jsonlived beside the real fixtures but is pure runtime scratch. Every consumer —run-e2e.sh:96and all 14 wizard steps inci.yml— doesecho '{}' >it immediately before mounting, precisely so the wizard triggers fresh. Nothing ever read the committed content. The container then wrote the wizard's result back through the read-write mount, dirtying the tree after every local run.Untracked and gitignored. No consumer changes, because every one already creates the file.
The real
ci-bess-settings*.jsonfixtures do have content that matters and still write back. That mount has to stay read-write — it is how the app persists settings — so:rowould break the wizard rather than protect the file. Documented in theverifyskill, not changed here.3.
git pushis guarded server-side instead of by a promptThe blanket
Bash(git push*)ask existed because prefix globbing cannot reach a marker at an arbitrary argument position. But the glob was compensating for having no guard at the only layer that sees a ref update rather than a command string.Four GitHub rulesets now refuse those spellings, all
enforcement=activewith emptybypass_actors:bess-manager~DEFAULT_BRANCHbess-managerbeta-release-*bess-manager~ALLtagsbess-manager-beta~DEFAULT_BRANCHThe empty bypass list is load-bearing: local pushes authenticate as the repo owner (osxkeychain;
gh auth status→johanzander), never asbess-agent. A ruleset exempting admins would exempt every push this machine makes.Accepted residual: force-pushing or deleting a feature branch (
fix/**,feat/**) stays possible. Bounded to an unmerged branch, and it keeps ordinary work prompt-free.Tag creation stays allowed so
releasecan tag; only deletion and force-update are blocked. Removing a mistaken tag now needs a deliberate ruleset edit.quality-check.shpins push inMUST_NOT_BE_GUARDED, so the prompt cannot be reinstated out of caution.maintobeta/main, which the new rule refuses. CLAUDE.md documents the disable → push → re-enable recipe. Do not add a bypass actor for it.Verification
scripts/quality-check.sh— 0 errors, 0 warnings (76 command shapes checked)verify-sandbox.sh— both new probes correctly FAIL against the old captured policy, proving they detect the conditionOut of scope — found while investigating
The Playwright install hang is reproducible and version-specific. Playwright 1.59.1 (pinned in
e2e/package.json) stalls twice at exactly 84 files / 448K, unsandboxed, with the download already complete (173MB in$TMPDIR) andoopDownloadBrowserMain.jsidle at 0% CPU with every libuv worker parked in__psynch_cvwait. Playwright 1.62.1 completed cleanly on the same machine, same network, minutes apart. So it is extraction, not the network — and bumping the pin looks like the actual fix.Note there is no issue tracking this hang.
worktree-setup.shcites #556, but only as the work it was first observed during; #556 itself is the closed "Speed up local verification: cache/symlink worktree dependencies" issue. This PR drops that shorthand and records the measurement in CLAUDE.md instead.The bump is not done here: it moves the E2E runner under 14 CI wizard scenarios and deserves its own PR.