Skip to content

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

Open
ofriw wants to merge 10 commits into
mainfrom
feat/readonly
Open

feat: add readonly mode with bash safety and spawn child filtering#7
ofriw wants to merge 10 commits into
mainfrom
feat/readonly

Conversation

@ofriw
Copy link
Copy Markdown
Contributor

@ofriw ofriw commented May 27, 2026

Note: This PR was generated by an AI agent. If you'd like to talk with other humans, drop by our Discord!


Readonly Mode — review summary

This PR adds a readonly mode to the extension that blocks write/edit tool calls and destructive bash commands, both for the parent agent and spawned children. It's a safety mechanism — think "guard rails" rather than a security boundary — designed to prevent accidental file mutations during exploratory or debugging sessions. Closes #2.

What changed (~1210 lines across 6 files):

  • readonly-bash.ts (new) — bash classifier using a destructive-command blacklist (rm, mv, sudo, redirects, package managers, editors, etc.) and a three-tier git allowlist that distinguishes immutable (diff/log/status) from mutable (add/commit/push) subcommands, with mixed-command predicates for things like reflog, branch, and stash.
  • index.ts — toggle command (/readonly), keyboard shortcut (ctrl+shift+r, idle-gated), CLI flag (--readonly), runtime tool_call blocking for write/edit/destructive-bash, one-shot ON/OFF nudges injected via the context hook, and rehydration logic for session_start/session_tree.
  • state.ts — two new fields (readonlyEnabled, readonlyNudgePending), initialized in createState() and cleared in resetState().
  • tui.ts — 🔒 readonly status indicator on the status bar.
  • spawn/index.ts — child tool filtering (drops write/edit when readonly is active), appends a readonly bash override that rejects destructive commands via spawnHook, and rewrites child authority wording from "same authority as the parent" to "read-only authority".
  • agenticoding.test.ts — 18 new test blocks covering bash classification, toggle/TUI behavior, tool_call blocking, spawn child filtering, session rehydration, CLI flag override, nudge delivery semantics, shortcut gating, and state reset.

Key design decision: rather than the canonical pi pattern of calling setActiveTools() to remove write/edit from the parent's tool list, this uses runtime enforcement via the tool_call event handler. This keeps the parent's active tool set stable across toggles — avoiding cache invalidation of the parent's tool definitions (which setActiveTools() would trigger). The tradeoff is that the LLM may still see write/edit in its tool list even when they're blocked, but the context nudges compensate by making the LLM aware of the restriction.

No breaking changes — readonly mode is opt-in via /readonly, ctrl+shift+r, or --readonly. Existing behavior is unaffected when disabled. Session persistence uses appendEntry entries on the branch, which cleanly survive /resume, /fork, and /clone boundaries.


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

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.

feat: Readonly Mode

2 participants