Skip to content

feat: add readonly mode with bash safety and spawn child filtering#7

Merged
ofriw merged 107 commits into
mainfrom
feat/readonly
Jul 16, 2026
Merged

feat: add readonly mode with bash safety and spawn child filtering#7
ofriw merged 107 commits into
mainfrom
feat/readonly

Conversation

@ofriw

@ofriw ofriw commented May 27, 2026

Copy link
Copy Markdown
Contributor

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 -rf something important, or npm install unwanted 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:

  • Manual: /readonly command or Ctrl+Shift+R shortcut — instant toggle
  • Startup: pi --readonly flag — locks down from the first turn
  • Declarative: Skill files and prompt commands can set readonly: true in 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, Linux bubblewrap): 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, exec to 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, eval chains, interpreter inline code (python -c "..."), xargs wrapping — 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:

  • User types /handoff <direction> → a temporary bypass flag is set
  • Agent can call the handoff tool once (write/edit remain blocked)
  • After compaction completes, the bypass is cleared
  • The fresh context resumes in readonly mode (state persists in the session branch)
  • If the agent never calls handoff, the bypass auto-cancels after 5 turns

This means readonly is never a trap, but accidental handoffs don't happen either.

What Was Removed

  • Fragile test assertions that counted internal warnings — they broke on unrelated refactors
  • Manual state stubs in the watchdog test — now exercises the real /handoff command path

Infrastructure Along the Way

These aren't readonly-specific but live on this branch:

  • Husky pre-commit hook: Catches type errors before CI
  • Dep bump 0.78.1 → 0.79.6: SDK fixes and features
  • audit-ci migration: Controlled npm advisory management
  • Cross-platform path fixes: fileURLToPath, path.join for Windows
  • System prompt tune: Clearer "Plan then execute" guidance

Breaking 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

  • Safe exploration: Review PRs, audit dependencies, explore unfamiliar code — without risk of accidental writes
  • Policy as code: Teams can mark review/audit skills as readonly: true in their skill definitions
  • CI integration: pi --readonly for automated read-only workflows
  • Peace of mind: A safety net that's always available, never in the way

Attached is an agent optimized description of the changes in this PR - AGENT_REVIEW.md

@grzegorznowak

Copy link
Copy Markdown
Collaborator

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 Assessment

Verdict: REQUEST CHANGES

Strengths

  • The PR implements the requested opt-in readonly mode with a CLI flag, /readonly toggle, persisted branch entries, and runtime blocking for write/edit/handoff tools. Sources: work/pr_context.json:5, index.ts:63, index.ts:72, index.ts:127
  • Readonly behavior is propagated to spawned children by filtering write/edit and installing a readonly bash override when bash is inherited. Sources: spawn/index.ts:269, spawn/index.ts:274, spawn/index.ts:278
  • User-facing guidance was added through a status indicator and readonly-specific nudges so the mode is visible to users and the agent. Sources: tui.ts:54, index.ts:306, index.ts:318

In Scope Issues

  • Readonly bash still allows concrete mutation paths through git mixed commands and unparsed command substitution. Sources: readonly-bash.ts:45, readonly-bash.ts:239, readonly-bash.ts:241, readonly-bash.ts:242, readonly-bash.ts:312

    High severity · Medium likelihood

    Why: Readonly mode is sold as blocking destructive bash during exploratory sessions. git branch new-name, git tag v1, or git status $(git add .) can still mutate repository state while readonly is enabled.

    Assumptions / Preconditions: The agent invokes bash while readonly mode is active.

    Downgrade Factors: This PR describes readonly as guardrails rather than a security boundary, but these are ordinary git commands an agent may plausibly use.

    Code Trail: The classifier splits only on shell separators and quote state, not command substitution. A segment is sent to the git allowlist only when the whole trimmed segment starts with git. Separately, branch and tag mixed policies accept any non-flag argument, which includes ref-creating forms.

    Reproduction: In readonly mode, ask bash to run git branch review-temp or git status $(git add .). The classifier path can return safe instead of blocking the mutation.

  • /readonly is not safe in no-UI contexts and can throw after partially applying state. Sources: index.ts:69, index.ts:72, index.ts:73, index.ts:74, tui.ts:32

    Medium severity · Medium likelihood

    Why: The toggle mutates state and persists an entry before unconditionally calling ctx.ui.notify. In a headless command context, users can end up with readonly toggled and persisted even though the command failed.

    Assumptions / Preconditions: /readonly or the idle shortcut is invoked with ctx.hasUI === false or without ctx.ui.

    Downgrade Factors: Most interactive use likely has a UI, but other commands in this extension already support no-UI operation.

    Code Trail: toggleReadonly flips state.readonlyEnabled, sets readonlyNudgePending, and appends agenticoding-readonly; updateIndicators no-ops when there is no UI, then ctx.ui.notify is dereferenced without checking ctx.hasUI.

    Reproduction: Invoke the registered readonly command with a context shaped like { hasUI: false, getContextUsage: () => null }. State is changed before the notify call throws.

Out of Scope Issues

  • None.

Technical Assessment

Verdict: REQUEST CHANGES

Input Boundary Shape Risk Assessment

Status: Triggered
Boundary: raw tool-call input and persisted session branch entries -> stricter readonly command and rehydration assumptions.
Evidence / mitigation: event.input.command is cast directly to string before classification, and the classifier immediately expects string-like behavior; tests cover valid { command: string } inputs but not missing or malformed bash input. Branch rehydration also scans raw branch entries, though current issue evidence is strongest at the bash tool boundary. Sources: index.ts:136, index.ts:137, readonly-bash.ts:45, agenticoding.test.ts:3994

Strengths

  • The readonly classifier is factored behind a reusable isSafeReadonlyCommand API and is reused by both parent tool-call blocking and child bash override enforcement. Sources: readonly-bash.ts:307, index.ts:138, spawn/index.ts:141
  • The PR adds focused static coverage for tool blocking, child tool filtering, session rehydration, readonly nudges, shortcut gating, and state reset. Sources: agenticoding.test.ts:3922, agenticoding.test.ts:4005, agenticoding.test.ts:4183, agenticoding.test.ts:4604

In Scope Issues

  • The parent bash tool boundary does not validate command before classification. Sources: index.ts:136, index.ts:137, readonly-bash.ts:45, agenticoding.test.ts:3994

    Medium severity · Medium likelihood

    Why: A malformed bash tool payload should be blocked cleanly in readonly mode. Instead, missing command can throw, and some non-string values can be treated as empty/safe because the runtime cast does not validate shape.

    Assumptions / Preconditions: The framework or model supplies malformed bash tool input while readonly mode is active.

    Downgrade Factors: Normal bash calls likely provide { command: string }, and existing tests cover that happy path.

    Code Trail: The tool-call handler casts event.input.command to string and passes it to isSafeReadonlyCommand. splitUnquotedShellSegments then reads cmd.length, so the boundary depends on runtime string shape without a guard.

    Reproduction: Call the readonly tool_call handler with { toolName: "bash", input: {} } or a non-string command. The handler does not return a controlled readonly block result.

Out of Scope Issues

  • None.

Reusability

  • buildChildToolNames remains exported and testable, keeping child tool inheritance and readonly filtering easy to validate independently. Sources: spawn/index.ts:126, agenticoding.test.ts:4005
  • buildNudge centralizes readonly/topic guidance, with tests for readonly topic, no-topic, and boundary-hint cases. Sources: watchdog.ts:15, agenticoding.test.ts:3354, agenticoding.test.ts:3373

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

ofriw added 28 commits June 16, 2026 14:59
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.

@grzegorznowak grzegorznowak left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:1109

    High 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 for cp-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/source as the destination → marks it temp-safe → command writes /workspace/source. Likewise, git diff --output=/workspace/out is 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:169

    High 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; /new also invalidates and aborts existing children.

    Code Trail: Spawn snapshots the prompt, tool list, and custom bash definition from readonlyEnabled, but staleness is tied only to childSessionEpoch. That generation increments on reset, not on /readonly, frontmatter transitions, or branch rehydration, and those transitions do not abort liveChildSessions.

    Reproduction: Spawn an unrestricted child → keep its prompt running → invoke /readonly in 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 --readonly fallback. Sources: readonly-rehydration.ts:20, readonly-rehydration.ts:25, readonly-rehydration.ts:27, readonly-rehydration.ts:29

    High 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 authoritative false, 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 returns true → reverse scan encounters the malformed entry → returns false.

  • Kernel-sandbox wrapping changes normal bash output semantics. Sources: os-sandbox.ts:188, os-sandbox.ts:236, tests/unit/os-sandbox.test.ts:26

    Medium severity · High likelihood

    Why: Readonly commands should produce the same observable output as unrestricted commands unless a write is denied.

    Assumptions / Preconditions: sandbox-exec or 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 with echo, 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 -n can be swallowed by echo "$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:463

    Medium 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_start injects 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:37

    Medium 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:123

    Medium 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 ws and two protobufjs entries 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:188

    Medium 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 createTestPI mock, supplies an incomplete context via as any, and directly invokes the first stored hook. It never launches the Pi executable or its TUI.

    Reproduction: A regression in --readonly CLI parsing or actual input/before_agent_start ordering 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:317

    Medium 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.warn and the renderer recovery path calls console.error.

    Reproduction: Cause a render component’s flushPendingUpdates to 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

@ofriw ofriw merged commit 26e65f9 into main Jul 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants