feat: add readonly mode with bash safety and spawn child filtering#7
Conversation
…acklist and git allowlist
…with TUI indicator
…and spawn child filtering
…only when no branch entries exist
…, and TUI warning
…ion, and redirect analysis
…ges, and CLI flag behavior
|
Summary: GH-2’s readonly-mode safety value is aligned, but concrete bash mutation bypasses and a no-UI toggle crash mean the PR should not merge as-is. Business / Product AssessmentVerdict: REQUEST CHANGES Strengths
In Scope Issues
Out of Scope Issues
Technical AssessmentVerdict: REQUEST CHANGES Input Boundary Shape Risk AssessmentStatus: Triggered Strengths
In Scope Issues
Out of Scope Issues
Reusability
review generated with CURe v. 0.7.3 · multi-stage - stages: 4 · sha 6540936 · model gpt-5.5/high · tok 8m/82k/8m · session agenticoding-pi-agenticoding-pr7-20260528-053241-21cc · 16m32s |
… temp-dir path checking
…dren, and watchdog throttling
Replaces npm audit with audit-ci to allowlist unavoidable shrinkwrap-pinned vulns (ws, protobufjs, undici via pi-coding-agent) with expiry tracking. Adds config-invariants tests to validate allowlist freshness and CI ordering.
…ws USERPROFILE support
…nagement and eligibility-aware nudges
…ligibility is met
…and system prompt
grzegorznowak
left a comment
There was a problem hiding this comment.
Thanks @ofriw . Posting the full CURe output for reference and approving the PR.
None of it is actually a blocker imho, though most of it is spot-on.
I remember we decided to don't worry about win OS, so the only really critical one can be ignored. 🦾
If you wanted to drill down into any of this please do of course.
Summary: GH-2 offers valuable safer exploration and review workflows, but authority gaps can still permit non-temp writes or retain unrestricted child agents after readonly appears active.
Business / Product Assessment
Verdict: REQUEST CHANGES
Strengths
- The requested manual, startup, and declarative activation surfaces are substantially implemented through the flag, command/shortcut, and frontmatter-intent pipeline. Sources:
index.ts:208,index.ts:240,index.ts:303 - Parent tool enforcement blocks write/edit, narrows handoff, and applies a shared bash guard while readonly is active. Sources:
index.ts:263,index.ts:276,index.ts:290 - Successful handoffs construct a fresh brief that explicitly restores readonly mode and clears the temporary bypass. Sources:
handoff/compact.ts:29,handoff/format.ts:26
In Scope Issues
-
The fallback classifier permits concrete, statically recognizable non-temp writes. Sources:
readonly-bash.ts:415,readonly-bash.ts:588,readonly-bash.ts:936,readonly-bash.ts:1109High severity · High likelihood
Why: On Windows or whenever the OS sandbox is unavailable, ordinary commands can modify the workspace while readonly reports that outside-temp writes are blocked.
Assumptions / Preconditions: The OS sandbox is unavailable and the agent invokes one of the misclassified forms.
Downgrade Factors: A working macOS/Linux kernel sandbox still rejects the filesystem write.
Code Trail: The fallback allows any command accepted by
classifyBashCommand. Destination extraction forcp-family commands uses only the last non-option argument, so destination-bearing flags are ignored. Git inspection commands are accepted solely by subcommand, without checking output options. The shell scanners also process backslashes before single-quote state, which can hide real separators and redirects.Reproduction:
cp -t /workspace /tmp/source→ classifier treats/tmp/sourceas the destination → marks it temp-safe → command writes/workspace/source. Likewise,git diff --output=/workspace/outis accepted as immutable. -
Enabling readonly does not revoke authority from children already running or awaiting session construction. Sources:
index.ts:214,spawn/index.ts:288,spawn/index.ts:308,state.ts:169High severity · Medium likelihood
Why: The parent can display readonly as enabled while an existing child continues using write, edit, and unrestricted bash.
Assumptions / Preconditions: A child was spawned while unrestricted and remains active, or its session factory is pending, when readonly changes.
Downgrade Factors: Newly spawned children sample the current readonly state correctly;
/newalso invalidates and aborts existing children.Code Trail: Spawn snapshots the prompt, tool list, and custom bash definition from
readonlyEnabled, but staleness is tied only tochildSessionEpoch. That generation increments on reset, not on/readonly, frontmatter transitions, or branch rehydration, and those transitions do not abortliveChildSessions.Reproduction: Spawn an unrestricted child → keep its prompt running → invoke
/readonlyin the parent → parent reports readonly enabled → child continues with its original write/edit/bash authority. -
A malformed latest persistence entry silently disables readonly and suppresses the
--readonlyfallback. Sources:readonly-rehydration.ts:20,readonly-rehydration.ts:25,readonly-rehydration.ts:27,readonly-rehydration.ts:29High severity · Low likelihood
Why: Corrupt or legacy session data can turn a safety control off without notifying the operator.
Assumptions / Preconditions: The latest matching custom branch entry has missing, null, or non-boolean
data.enabled.Downgrade Factors: Entries written by the current normal toggle path contain a proper boolean.
Code Trail: Reverse scanning stops at the first matching custom entry and returns whether its value is exactly
true. Invalid values therefore become an authoritativefalse, rather than being skipped so an older valid entry or CLI flag can apply.Reproduction: Branch entries: older
{enabled:true}, then newer{data:null}; CLI flag returnstrue→ reverse scan encounters the malformed entry → returnsfalse. -
Kernel-sandbox wrapping changes normal bash output semantics. Sources:
os-sandbox.ts:188,os-sandbox.ts:236,tests/unit/os-sandbox.test.ts:26Medium severity · High likelihood
Why: Readonly commands should produce the same observable output as unrestricted commands unless a write is denied.
Assumptions / Preconditions:
sandbox-execor bubblewrap is available.Downgrade Factors: Exit status is preserved, and fallback-only execution does not use this wrapper.
Code Trail: Both wrappers run the entire command inside
output=$(... 2>&1), which buffers output, merges stderr into stdout, and removes trailing newlines. The result is then emitted withecho, which is not byte-preserving and can interpret values such as-n. Tests cover filesystem effects but not stream separation or output fidelity.Reproduction: A command producing interleaved stdout/stderr becomes one buffered stdout stream; a command whose complete output is
-ncan be swallowed byecho "$output". -
The PR is not fully opt-in or non-breaking as described: it drops 0.78 peer compatibility and changes every agent’s primer even when readonly is off. Sources:
work/pr_context.json:5,package.json:18,system-prompt.ts:14,index.ts:463Medium severity · High likelihood
Why: Maintainers and users need an accurate compatibility contract before release.
Assumptions / Preconditions: A host remains on the 0.78 SDK line, or an existing workflow is sensitive to the newly mandated planning/delegation behavior.
Downgrade Factors: The change is acceptable if the release intentionally requires Pi 0.79 and the global prompt tuning is an approved product change.
Code Trail: All Pi peer ranges move from the non-overlapping 0.78 line to
^0.79.6. Separately, the changed primer tells every agent to phase work, delegate large subtasks, consider verification spawns, and stop at a human checkpoint;before_agent_startinjects that primer unconditionally.Reproduction: Install into a 0.78 host → peer requirements cannot be satisfied. Start a normal readonly-off session → the revised primer is still appended.
Out of Scope Issues
- None.
Technical Assessment
Verdict: REQUEST CHANGES
Input Boundary Shape Risk Assessment
Status: Triggered
Boundary: Raw bash tool input → shell classification/sandbox wrapping; raw persisted session entries → boolean readonly authority.
Evidence / mitigation: Bash input has a useful runtime type check, but stricter shell-shape assumptions miss valid destination and quoting forms; persistence stops at matching entries without validating their data shape. Sources: readonly-bash.ts:1092, readonly-rehydration.ts:20
Strengths
- Parent and child bash paths reuse the same guard, including explicit rejection of non-string command input. Sources:
readonly-bash.ts:1092,spawn/index.ts:183 - Temp-directory decisions share a canonical path and include symlink-aware resolution for paths that do not yet exist. Sources:
temp-dir.ts:14,resolve-path.ts:11 - The PR adds extensive focused tests and a four-cell cross-platform CI matrix, and all currently hosted cells pass. Sources:
.github/workflows/test.yml:31,tests/unit/readonly-bash-classifier.test.ts:24,tests/unit/readonly-handoff.test.ts:48
In Scope Issues
-
The declared Node runtime floor is lower than the SDK’s actual requirement, and CI does not exercise the declared minimum precisely. Sources:
package.json:14,package-lock.json:602,.github/workflows/test.yml:37Medium severity · Medium likelihood
Why: Consumers should not be told that an unsupported Node runtime is valid.
Assumptions / Preconditions: A user runs Node 22.0–22.18.
Downgrade Factors: Many current Node 22 installations will already be at or above 22.19.
Code Trail: The extension declares
node >=22, while the locked Pi 0.79.6 package requires>=22.19.0. The CI cell requests generic"22", which resolves to a current Node 22 release rather than testing 22.0.Reproduction: Install under Node 22.0 → the extension’s engine accepts it → its required Pi SDK rejects or cannot support that runtime.
-
Three audit exceptions described as path-scoped are advisory-wide. Sources:
audit-ci.jsonc:5,audit-ci.jsonc:8,tests/unit/config-invariants.test.ts:114,tests/unit/config-invariants.test.ts:123Medium severity · Low likelihood
Why: A future vulnerable dependency path should fail CI instead of being hidden by an exception intended only for the pinned Pi dependency.
Assumptions / Preconditions: The same advisory later appears through a direct or unrelated dependency before the exceptions expire.
Downgrade Factors: The exceptions expire on 2026-09-01 and currently document the intended upstream path.
Code Trail: The
wsand twoprotobufjsentries contain only advisory IDs, unlike the explicitly scoped undici keys containing|dependency-path. The invariant labels the allowlist path-scoped but makes the path suffix optional and hardcodes the broad keys.Reproduction: Add another vulnerable dependency affected by one of those advisory IDs → audit-ci matches the bare advisory exception regardless of dependency path.
-
The expanded E2E suite does not exercise the real Pi host, CLI, event dispatcher, or TUI. Sources:
tests/e2e/test-host.ts:28,tests/e2e/test-host.ts:37,tests/e2e/test-host.ts:54,tests/e2e/test-host.ts:188Medium severity · Not Assessed likelihood
Why: The highest-risk behavior depends on real SDK event ordering, CLI parsing, and TUI stream integrity, which this harness cannot validate.
Assumptions / Preconditions: Real Pi behavior differs from the test mock in registration, dispatch ordering, context shape, or stream handling.
Downgrade Factors: The process isolation still provides useful integration coverage of extension code and request framing.
Code Trail: The child process registers against the unit-test
createTestPImock, supplies an incomplete context viaas any, and directly invokes the first stored hook. It never launches the Pi executable or its TUI.Reproduction: A regression in
--readonlyCLI parsing or actualinput/before_agent_startordering can leave every mock-host test green because those host paths are bypassed.
Out of Scope Issues
-
Pre-existing runtime console diagnostics conflict with the repository-wide TUI safety rule introduced on this branch. Sources:
AGENTS.md:3,runtime-singletons.ts:78,spawn/renderer.ts:317Medium severity · Low likelihood
Why: If these error paths execute inside Pi, their output can corrupt the shared ANSI TUI.
Assumptions / Preconditions: A pending singleton lock or renderer flush failure reaches the diagnostic call during an interactive session.
Downgrade Factors: The calls predate this PR and execute only on exceptional paths.
Code Trail: The new policy prohibits all console diagnostics, but the singleton replacement path calls
console.warnand the renderer recovery path callsconsole.error.Reproduction: Cause a render component’s
flushPendingUpdatesto throw → the scheduler writes directly to stderr while the TUI owns the same process streams.
Reusability
- Centralized readonly copy keeps status, handoff, child-authority, and error wording aligned. Sources:
readonly-copy.ts:8,readonly-copy.ts:51 - Pure authority helpers such as branch rehydration and readonly tool-name filtering are independently testable and reusable. Sources:
readonly-rehydration.ts:16,spawn/index.ts:147 - Canonical temp and real-path helpers provide a reusable basis for consistent filesystem-boundary decisions. Sources:
temp-dir.ts:14,resolve-path.ts:11
review generated with CURe v. 0.8.2 · multi-stage - stages: 5 · sha fd28a3a · model gpt-5.6-sol/high · tok 33m/219k/33m · session agenticoding-pi-agenticoding-pr7-20260716-052958-bbdd · 38m52s
Note: This PR was generated by an AI agent. If you'd like to talk with other humans, drop by our Discord!
Readonly Mode — Safety Net for AI Agent Operations
Context
The agent had unrestricted filesystem access. Any operation — reading config, exploring code, reviewing a PR — could accidentally overwrite files,
rm -rfsomething important, ornpm installunwanted dependencies. There was no way to constrain it to read-only operations short of not using it.What This Changes
This PR introduces a readonly mode — a guardrail that, when active, prevents the agent from modifying the filesystem. The agent can still read everything, navigate, search, debug, and plan. It just can't write (except to the OS's temp dir).
Three ways to activate it:
/readonlycommand orCtrl+Shift+Rshortcut — instant togglepi --readonlyflag — locks down from the first turnreadonly: truein their YAML frontmatter — readonly activates automatically when that skill/command is invoked. Think "review my PR" = readonly, "implement this feature" = unrestricted.How It Works (Two-Layer Enforcement)
Layer 1 — OS Sandbox (macOS
sandbox-exec, Linuxbubblewrap): Wraps bash commands in a kernel-enforced sandbox that denies file writes outside the OS temp directory. This is the real enforcement — the OS stops writes at the syscall level.Layer 2 — Command Pattern Classifier (all platforms, fallback): Parses bash commands and classifies each segment — allows reads and temp-directory writes, blocks anything that modifies the project. Recursively unwraps
sudo,env,eval,execto inspect the real payload. Blocks package managers (npm install,pip install, etc.) unconditionally — they write to unpredictable locations.On macOS/Linux both layers work together (classifier runs first for a clear block reason, then sandbox wraps if allowed).
Critical — please read: Pattern classification is theoretically impossible to make complete. Shell injection,
evalchains, interpreter inline code (python -c "..."),xargswrapping — bypasses are inherent to the problem, not bugs in the implementation. The classifier is a best-effort guardrail, not a security boundary. On macOS/Linux, the OS sandbox provides real enforcement underneath. On Windows, only the classifier applies — which means readonly there is fundamentally a best-effort guardrail and cannot be hardened to real enforcement without OS-level sandboxing support.The Hard Part — Handoff Integration
Readonly mode creates a tension: if handoff is blocked, the user is trapped in a session they can't escape. The solution is a narrow, explicit bypass:
/handoff <direction>→ a temporary bypass flag is setThis means readonly is never a trap, but accidental handoffs don't happen either.
What Was Removed
/handoffcommand pathInfrastructure Along the Way
These aren't readonly-specific but live on this branch:
0.78.1 → 0.79.6: SDK fixes and featuresfileURLToPath,path.joinfor WindowsBreaking Changes? None.
Readonly defaults to off. Every new module is opt-in additive. Existing sessions, skills, prompts, and workflows are completely unaffected.
Value to the Project
readonly: truein their skill definitionspi --readonlyfor automated read-only workflowsAttached is an agent optimized description of the changes in this PR - AGENT_REVIEW.md