fix: let a sandboxed session read the agent list, which silently truncates - #642
Conversation
…cates
`claude agents --json` reads the session list from `~/.claude/jobs`, which is
not in the sandbox allow list. A sandboxed call does not fail -- it TRUNCATES.
Measured in one session, seconds apart:
sandboxed: 1 agent
unsandboxed: 14 agents, 7 of them live sessions sitting in worktrees
All seven read as dead.
That is not cosmetic. `backlog-rhythm.sh` keys `resume_implementation` off
"worktree on disk, no live session", so with a truncated listing it told the
maintainer to re-enter a worktree that a live session was actively working --
a second session on one branch, against commits the advice itself calls the
only copy. It did exactly that for #619 tonight. `implement-issue` Step 0 reads
the same list before touching a resumed branch.
An UNDER-COUNT rather than an error is the dangerous shape: nothing about a
short list looks wrong, so the wrong answer gets acted on with full confidence.
Both skills already carry a "run it unsandboxed" warning; this makes the
warning unnecessary rather than merely repeated, and fixes the case where
nobody remembered to.
NOT VERIFIED IN THIS SESSION, and it cannot be: the sandbox captures its policy
once at activation and ignores every later edit, so the numbers above are from
the OLD policy and re-measuring here would prove nothing. Per CLAUDE.md this
needs a genuinely fresh session, then `bash scripts/verify-sandbox.sh`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
| "allowWrite": [ | ||
| ".", | ||
| "~/GitHub/bess-manager", | ||
| "~/.claude/jobs", |
There was a problem hiding this comment.
Root-cause check: the PR description says claude agents --json reads the session list from ~/.claude/jobs, but the fix adds the path to filesystem.allowWrite, not allowRead. This repo's own CLAUDE.md (unchanged by this PR, line ~510) documents that reads and writes are governed separately: "There is no allow-within-deny primitive for writes (reads have one, which is why allowRead differs)". Given the doc's own repeated lesson that sandbox-knob guesses are frequently wrong unless grounded in the actual error text (see the podman example a few lines below, where filesystem.allowRead was tried and disproved for a different symptom), and given the PR explicitly could not verify this fix in-session, is there a captured error message showing this is a write denial rather than a read denial? If not, allowRead looks like the more consistent knob for a stated read operation.
There was a problem hiding this comment.
Summary
Minimal, well-scoped, honestly-caveated fix: adds ~/.claude/jobs to sandbox.filesystem.allowWrite in .claude/settings.json, plus a matching rationale entry in CLAUDE.md's "Why each non-default knob is there" section. Two files, 18 lines, no scope creep — matches the PR's own stated scope.
No linked issue (closingIssuesReferences is empty) — the PR body itself documents the investigation (the 1-vs-14 agent-count measurement), so there's no external root-cause doc to cross-check against, but the in-PR evidence for there being a real problem is solid and specific.
Open question: is allowWrite the right knob for a read operation?
Left as an inline comment on .claude/settings.json:19, repeating here since it's the crux of the review:
The PR description says (its own words): "claude agents --json reads the session list from ~/.claude/jobs" — but the fix adds the path to filesystem.allowWrite, not allowRead. CLAUDE.md itself, unchanged by this PR (~line 510), already documents that reads and writes are governed by separate mechanisms in this sandbox config: "There is no allow-within-deny primitive for writes (reads have one, which is why allowRead differs)" — and a few lines below that, the podman entry is a worked example of exactly this failure mode: filesystem.allowRead was tried, and disproved, for a different symptom, precisely because reasoning about "what ought to be blocked" instead of reading the actual error picked the wrong knob.
This PR's own "Not verified in this session, and it cannot be" section is honest about not having confirmed the fix works — which is unavoidable given the sandbox's capture-once behavior, and matches this project's established process of merge-then-verify-in-a-fresh-session for sandbox.* changes (CLAUDE.md: "Verify with bash scripts/verify-sandbox.sh in a FRESH session after any change to sandbox.*"). So I'm not blocking on that part. But the read-vs-write mismatch is answerable now, from the PR's own description and the adjacent doc text, without needing a sandboxed session — and if allowRead turns out to be the actual fix needed, this PR would land, get documented as "fixed" (removing the very warnings it says become "unnecessary rather than merely repeated"), and the truncation bug would persist silently — which is the exact dangerous shape ("nothing about a short list looks wrong, so the wrong answer is acted on with full confidence") the PR itself calls out as the reason this matters.
Ask: before or immediately after merge, confirm with the actual captured error/behavior (not just "sandboxed count == 1") whether this is a write-permission denial or a read-permission denial on ~/.claude/jobs, the way the podman/keychain/TLS entries elsewhere in that same CLAUDE.md section were each pinned to a specific measured error message rather than an assumption.
Checklist
- Matches root cause: no linked issue to check against; the PR's own evidence for the problem (1 vs 14 agents) is solid. The fix (
allowWritevsallowRead) is the open question above. - Test coverage: N/A by nature — sandbox behavior can't be tested in-session (correctly explained in the PR body), and
./scripts/quality-check.shonly asserts the Bash permission lists (deny/ask/allow), notsandbox.filesystem, so its "green" checkbox is real but doesn't bear on this specific fix's correctness. - Minimal / no scope creep: yes — one config line + matching doc rationale, consistent in style and placement with the surrounding "Why each non-default knob is there" entries.
- Rule violations: none found in
docs/agents/rules.md/.github/claude-bot.mdterms (no new classes, no hardcoded entities, no exception-string matching, no missing camelCase conversion — none of these apply to a config/doc-only change). No CHANGELOG entry, consistent with recent precedent for agent-tooling-only PRs (#635, #637, #639 also skip it). - Scope statement (rules.md Debugging Protocol step 9): PR states scope explicitly ("
.claude/settings.json(one line) andCLAUDE.md(the rationale)") — a local, minimal change, correctly categorized.
Verdict: COMMENT — the allowWrite/allowRead question is worth resolving with real evidence before trusting this as done, but it's a low-blast-radius, easily-revertible config change from an author who was unusually careful to flag what wasn't verified, so it's not a blocking rule violation.
The Stage 4 review asked the right question: `claude agents --json` READS the
session list, so why does the fix touch `allowWrite` rather than `allowRead`?
It also named the precedent that makes the question worth asking -- the podman
entry a few lines below, where `filesystem.allowRead` was tried and disproved
by reasoning about what ought to be blocked instead of reading the error.
Answered by measurement rather than argument, in a sandboxed session:
ls ~/.claude/jobs -> 16 entries # read: ALLOWED
touch ~/.claude/jobs/.probe -> Operation not permitted # write: DENIED
Reads were never blocked -- the read policy denies only `~/.claude/ide`. The
enumeration needs to write, and drops the entry rather than raising when it
cannot, which is what produces the truncation. So `allowWrite` is the correct
knob and `allowRead` would have changed nothing.
Recorded in the entry itself, in the same shape as the other knobs in that
section, each of which is pinned to a specific measured error rather than an
assumption. The next reader gets the answer without re-asking.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
Good question, and the precedent you cite is the right one to hold this against — the podman entry a few lines below is exactly a case of In a sandboxed session, against Reads were never blocked. The read policy in this sandbox denies exactly one path, So the enumeration can see the job files and still under-reports, because it needs to write (a lock or heartbeat beside each job) and drops the entry rather than raising when it cannot. That is also the mechanism behind the shape I called dangerous in the description: a write failure surfacing as a shorter list instead of an error.
Pushed as Your other two points stand and I have not tried to argue them away: |
|
@claude-bot review |
There was a problem hiding this comment.
Summary
Minimal, well-evidenced infra fix: adds ~/.claude/jobs to sandbox.filesystem.allowWrite in .claude/settings.json (1 line) and documents the rationale in CLAUDE.md in the same style as every other non-default sandbox knob already documented there.
Root cause match: Yes. The PR traces the failure precisely — claude agents --json enumerates ~/.claude/jobs, and enumeration apparently needs to write there (confirmed with a direct probe: touch ~/.claude/jobs/.probe → Operation not permitted, while ls succeeds). A sandboxed session missing that entry silently drops entries rather than erroring (1 agent vs 14, with 7 live sessions misreported as dead), which caused backlog-rhythm.sh to advise resuming a worktree a live session already held. Adding the directory to allowWrite is the direct fix for that missing permission — not a workaround routing around some other ordering/timing problem.
Test coverage: No automated test (this is a sandbox permission entry, not application code — consistent with how every other entry in that CLAUDE.md section is documented, by measured before/after behavior rather than a unit test). The PR includes concrete measured evidence (1-vs-14 agent count, and the second commit's read/write probe pair added in response to a prior review round asking "why allowWrite and not allowRead"). It's honest that the fix itself can't be re-verified in this session, since the sandbox captures its policy once at activation — that's consistent with what CLAUDE.md already documents about this class of change, and it correctly avoids claiming a green it can't produce.
Scope: Minimal and on-target — one settings entry plus the matching documentation. No scope creep. Correctly skips a CHANGELOG entry (agent tooling, no user-visible effect), consistent with the project's CHANGELOG conventions for user-facing changes only.
Rule violations: None found. This touches no Python/TypeScript application code, so the bulk of the architecture/error-handling/testing checklist items don't apply. JSON in .claude/settings.json is well-formed and the new entry follows the same tilde-path style as its neighbors (~/GitHub/bess-manager, ~/.npm, etc.).
Minor observation (non-blocking): Per rules.md Debugging Protocol step 9, the PR description doesn't explicitly label the fix as "local" — but given the change is a single config entry with a directly-matching root cause, the local-fix classification is self-evident here and doesn't need a separate callout.
No blockers. Approving.
2 participants
Problem
claude agents --jsonreads the session list from~/.claude/jobs, which is not in the sandbox allow list. A sandboxed call does not fail — it silently truncates.Measured in one session, seconds apart:
All seven read as dead.
Why it matters
backlog-rhythm.shkeysresume_implementationoff "worktree on disk, no live session". With a truncated listing it told the maintainer to re-enter a worktree that a live session was actively working — putting a second session on one branch, against commits the advice itself calls the only copy.That happened tonight, on #619.
implement-issueStep 0 reads the same list before touching a resumed branch.An under-count rather than an error is the dangerous shape: nothing about a short list looks wrong, so the wrong answer is acted on with full confidence. It also defeated two layers of documentation — both
backlogandimplement-issuealready carry a "run this unsandboxed" warning, and it still bit, because the warning only helps whoever remembers to read it.Fix
One entry:
~/.claude/jobsinsandbox.filesystem.allowWrite, with the rationale added to CLAUDE.md's per-knob list, where every other non-default entry is already explained.This makes the warnings unnecessary rather than merely repeated.
Test plan
./scripts/quality-check.shgreenThe sandbox captures its policy once at activation and ignores every later edit (CLAUDE.md, "The sandbox captures its policy ONCE"). So the numbers above come from the old policy, and re-measuring here would prove nothing — it would return the same 1 no matter what this diff says, which is exactly the trap that section documents.
Verifying needs a genuinely fresh session, then:
Flagging this rather than claiming a green I cannot produce.
Scope
.claude/settings.json(one line) andCLAUDE.md(the rationale). No CHANGELOG entry: agent tooling, no user-visible effect.Context
This is the one piece of #620 that survives. #635 superseded the rest of that PR by guarding pushes server-side with GitHub rulesets, which is a better answer than the pattern enumeration #620 was shrinking — but
~/.claude/jobswas unrelated to pushes and went with it.🤖 Generated with Claude Code