diff --git a/docs/DESIGN.md b/docs/DESIGN.md index df5972d..5d22413 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1280,7 +1280,7 @@ An approval prompt has three outcomes, not two — `[y]es / [e]dit / [n]o` (`App This is *reject-and-steer*, not inline-edit-and-run: the user never edits a command that then executes under the badge it was approved under. **Safety is automatic** because the un-approved action is dropped and the correction is a fresh tool call that is independently classified and re-gated — steering can never smuggle a higher-risk action past the prompt. The outcome is uniform across every approval-gated tool (commands and file writes alike); no per-tool editing logic exists because nothing is edited in place. -**A plain decline ends the turn; a steer continues it.** The declined outcome carries `stop_turn = not bool(steer)` (`runtime/executor.py`). On a plain `[n]o` the tool loop truncates any remaining tool calls in that reply, records the declined result, and returns without re-invoking the model — so a declined action cannot be silently retried later in the same turn. When a plan is active, the decline surfaces a status line `Action declined; plan paused on step N.` (the active step index, via `_active_plan_step`) and leaves the plan paused for the user's next instruction. A steered `[e]dit` decline does **not** stop the turn: `stop_turn` is `False` whenever steer text is present, so the correction is fed back and the loop continues so the model can re-propose. +**A plain decline ends the turn; a steer continues it.** The declined outcome carries `stop_turn = not bool(steer)` (`runtime/executor.py`). On a plain `[n]o` the tool loop truncates any remaining tool calls in that reply, records the declined result, and returns without re-invoking the model — so a declined action cannot be silently retried later in the same turn. When a plan is active, the decline surfaces a status line `Action declined; plan paused on step N.` (the active step index, via `_plan_step(active_only=True)`) and leaves the plan paused for the user's next instruction. A steered `[e]dit` decline does **not** stop the turn: `stop_turn` is `False` whenever steer text is present, so the correction is fed back and the loop continues so the model can re-propose. **The HIGH-risk typed-`run` confirm is unchanged.** A HIGH-risk command still requires typing the literal `run` to execute; `[e]` is an additional option at that same prompt that steers without running, and Enter still cancels. Empty guidance after `[e]` is treated as a plain decline (nothing runs). A steered approval is audited as `decision="steered"` on the `approval` event (alongside `approved`/`rejected`). @@ -2463,7 +2463,7 @@ The rebuild should stay light. The goal is a reliable local harness, not a frame | Memory system | Behavior/project memory, proposals, and optimization move to v2. V1 only reads `AGENTS.md`. Scheduled for v0.3.0 (settled 2026-06-11). | | Token-budget compaction | V1 uses oldest-first truncation; selective compaction is v2. Scheduled for v0.3.0 (settled 2026-06-11). | | `trusted-local` profile | Deferred from v1, and deferred again at the 2026-06-11 v2 scoping. Revisit for v3. | -| Session resume | Shipped in v0.3.0 (settled 2026-06-11): append-only JSONL transcripts at `.shellpilot/sessions/.jsonl`, written incrementally with secrets redacted; compaction trims memory, never the transcript. `shellpilot --resume [id]` restores the latest (or named) session's history; snapshots are never restored, so read-before-write forces fresh reads. `/export` renders the transcript to markdown. Tool-call arguments are redacted recursively (matching the audit log's `_redact_value` logic, now unified in `redact_structure` in `shellpilot/memory/redaction.py`) before they reach the JSONL transcript; `/export` inherits redaction by re-reading the transcript from disk. Fixed in v0.5.2. `session_markdown` re-applies redaction at export time so transcripts written before v0.5.2 (which may contain raw secrets on disk) cannot leak through `/export`; on-disk history is deliberately left untouched. Fixed in v0.5.2 review wave. Plan state now also restores on `--resume` (v0.6.0): an `active_plan` pointer in the transcript is read at boot; if the referenced plan sidecar is live (`proposed`/`active`/`blocked`), `PlanManager.restore` reinstates it (section 11.3). **Read-side traversal guard (v0.10.1):** `SessionStore.find` now rejects any session id whose resolved parent differs from the sessions directory, closing the `--resume ../../../../etc/x` path-traversal vector; the write path was already safe via `path.stem`. **Reconciliation records:** the transcript stays append-only, so mid-turn corrections are records rather than rewrites — on load, `replace_last_message` replaces the last *assistant* record (a mid-batch decline truncates the reply's remaining tool calls, section 14.6), `truncate_last_turn` deletes from the last assistant record to the end (a mid-tool cancel, section 31.15), and `discard_last_message` pops the single trailing record (a user message that was written to the transcript and then refused by the hard context-limit gate, so `--resume` does not restore a stuck user turn with no reply). A `replace_last_message`/`truncate_last_turn` record with no assistant message present, a `discard_last_message` with an empty transcript, or an unknown record kind, is ignored. | +| Session resume | Shipped in v0.3.0 (settled 2026-06-11): append-only JSONL transcripts at `.shellpilot/sessions/.jsonl`, written incrementally with secrets redacted; compaction trims memory, never the transcript. `shellpilot --resume [id]` restores the latest (or named) session's history; snapshots are never restored, so read-before-write forces fresh reads. `/export` renders the transcript to markdown. Tool-call arguments are redacted recursively (matching the audit log, where each record value passes through `redact_structure` in `shellpilot/memory/redaction.py`) before they reach the JSONL transcript; `/export` inherits redaction by re-reading the transcript from disk. Fixed in v0.5.2. `session_markdown` re-applies redaction at export time so transcripts written before v0.5.2 (which may contain raw secrets on disk) cannot leak through `/export`; on-disk history is deliberately left untouched. Fixed in v0.5.2 review wave. Plan state now also restores on `--resume` (v0.6.0): an `active_plan` pointer in the transcript is read at boot; if the referenced plan sidecar is live (`proposed`/`active`/`blocked`), `PlanManager.restore` reinstates it (section 11.3). **Read-side traversal guard (v0.10.1):** `SessionStore.find` now rejects any session id whose resolved parent differs from the sessions directory, closing the `--resume ../../../../etc/x` path-traversal vector; the write path was already safe via `path.stem`. **Reconciliation records:** the transcript stays append-only, so mid-turn corrections are records rather than rewrites — on load, `replace_last_message` replaces the last *assistant* record (a mid-batch decline truncates the reply's remaining tool calls, section 14.6), `truncate_last_turn` deletes from the last assistant record to the end (a mid-tool cancel, section 31.15), and `discard_last_message` pops the single trailing record (a user message that was written to the transcript and then refused by the hard context-limit gate, so `--resume` does not restore a stuck user turn with no reply). A `replace_last_message`/`truncate_last_turn` record with no assistant message present, a `discard_last_message` with an empty transcript, or an unknown record kind, is ignored. | | Agent raw shell | Do not expose `raw_shell` as an agent tool in v1. Keep Manual Shell for direct user-controlled `shell=True`. | | Capability packs (Skills v2) | v0.6.0 shipped instruction-only SKILL.md discovery; v0.7.0 extends it with deterministic trigger selection, four markdown-only builtins, read-only references/templates, script manifest discovery without execution, and enriched `/skills` + `/context` visibility (section 23). | | Capability packs (heavier: tools/handlers/permissions) | Design later after core tools are stable. v3 candidate (2026-06-11). | diff --git a/shellpilot/cli/app_ui.py b/shellpilot/cli/app_ui.py index 0fc8c01..b09ea82 100644 --- a/shellpilot/cli/app_ui.py +++ b/shellpilot/cli/app_ui.py @@ -42,7 +42,7 @@ from shellpilot.cli.theme import SHELLPILOT_THEME, UNICODE_GLYPHS, Glyphs from shellpilot.memory.redaction import redact_structure from shellpilot.runtime.budget import CHARS_PER_TOKEN -from shellpilot.tools.base import workspace_display +from shellpilot.tools.base import make_workspace_path_display if TYPE_CHECKING: from shellpilot.policy.approvals import ApprovalRequest @@ -167,8 +167,7 @@ def __init__( # workspace_fn (preferred in production) is called at render time so a # mid-session /cwd change is immediately reflected; workspace is the # static fallback for test doubles that construct without a live runtime. - self._workspace = workspace - self._workspace_fn = workspace_fn + self._path_display = make_workspace_path_display(workspace, workspace_fn) self._width_fn = width_fn # Gate for the reasoning-token readout (settings.ui.show_reasoning_summary, # design section 31.14): when False, the live/done lines show plane+phrase+ @@ -601,15 +600,6 @@ def show_tool_call(self, name: str, arguments: dict[str, object]) -> None: ): self._add_renderable(renderable) - def _path_display(self, path: str) -> str: - # Resolve a `path` argument to its workspace-relative target (§14.5). - # Prefer the live workspace (workspace_fn, set in production) so a - # mid-session /cwd is honoured; fall back to the build-time workspace, - # then verbatim (a test-double with neither set — production always wires - # workspace_fn, so the path display never drifts from the action). - workspace = self._workspace_fn() if self._workspace_fn is not None else self._workspace - return workspace_display(workspace, path) if workspace is not None else path - def show_tool_result(self, name: str, success: bool, summary: str) -> None: self._add_renderable(render_tool_result(success, summary, self._glyphs)) diff --git a/shellpilot/cli/streaming.py b/shellpilot/cli/streaming.py index c123151..d71c443 100644 --- a/shellpilot/cli/streaming.py +++ b/shellpilot/cli/streaming.py @@ -332,10 +332,6 @@ def _frame(self, tick: int) -> Text: (f"{self._glyphs.ellipsis} {int(elapsed)}s", "sp.dim"), ) - def _current_label_text(self) -> str: - """Return the plain text of the most recent frame (for tests).""" - return self._frame(0).plain - def _spin(self) -> None: tick = 0 while not self._stop_event.wait(_REFRESH_SECONDS): diff --git a/shellpilot/cli/terminal.py b/shellpilot/cli/terminal.py index bf56b94..e1e17ba 100644 --- a/shellpilot/cli/terminal.py +++ b/shellpilot/cli/terminal.py @@ -91,7 +91,7 @@ from shellpilot.runtime.events import RuntimeUI, TurnStats from shellpilot.runtime.planner import TaskPlan from shellpilot.skills.loader import discover_skills -from shellpilot.tools.base import workspace_display +from shellpilot.tools.base import make_workspace_path_display def should_discard_interrupt( @@ -217,8 +217,7 @@ def __init__( # workspace_fn (preferred in production) is called at render time so a # mid-session /cwd change is immediately reflected; workspace is the # static fallback for test doubles that construct without a live runtime. - self._workspace = workspace - self._workspace_fn = workspace_fn + self._path_display = make_workspace_path_display(workspace, workspace_fn) self._stream = ResponseStream(console) self._spinner = AviationSpinner(console, glyphs, enabled=spinner) # The diff-reveal animation rides the same motion toggle as the spinner. @@ -272,15 +271,6 @@ def show_tool_call(self, name: str, arguments: dict[str, object]) -> None: label = Text.assemble(("running ", "sp.dim"), (_sanitize_line(name), "sp.emph")) self._spinner.start(label=label) - def _path_display(self, path: str) -> str: - # Resolve a `path` argument to its workspace-relative target (§14.5). - # Prefer the live workspace (workspace_fn, set in production) so a - # mid-session /cwd is honoured; fall back to the build-time workspace, - # then verbatim (a test-double with neither set — production always wires - # workspace_fn, so the path display never drifts from the action). - workspace = self._workspace_fn() if self._workspace_fn is not None else self._workspace - return workspace_display(workspace, path) if workspace is not None else path - def show_tool_result(self, name: str, success: bool, summary: str) -> None: self._spinner.stop() self._console.print(render_tool_result(success, summary, self._glyphs)) diff --git a/shellpilot/persistence/audit_store.py b/shellpilot/persistence/audit_store.py index 2a89f08..16b216b 100644 --- a/shellpilot/persistence/audit_store.py +++ b/shellpilot/persistence/audit_store.py @@ -14,12 +14,6 @@ AUDIT_VERSION = 1 -def _redact_value(value: Any, enabled: bool) -> Any: - if not enabled: - return value - return redact_structure(value) - - @dataclass class AuditLogger: """Append-only JSONL audit events; secrets redacted before write.""" @@ -39,7 +33,12 @@ def write(self, event: str, **fields: Any) -> None: "profile": self.profile, "event": event, } - record.update({key: _redact_value(value, self.redact) for key, value in fields.items()}) + record.update( + { + key: redact_structure(value) if self.redact else value + for key, value in fields.items() + } + ) self.path.parent.mkdir(parents=True, exist_ok=True, mode=0o700) fd = os.open(self.path, os.O_WRONLY | os.O_CREAT | os.O_APPEND, 0o600) with os.fdopen(fd, "a", encoding="utf-8") as handle: diff --git a/shellpilot/policy/command_policy.py b/shellpilot/policy/command_policy.py index cb2f6f6..203678d 100644 --- a/shellpilot/policy/command_policy.py +++ b/shellpilot/policy/command_policy.py @@ -426,21 +426,17 @@ def _option_present(argv: list[str], names: frozenset[str]) -> str | None: return None -def _short_option_letters(names: frozenset[str]) -> frozenset[str]: - return frozenset( - name[1:] - for name in names - if name.startswith("-") and not name.startswith("--") and len(name) == 2 - ) - - def _split_option_value(argv: list[str], names: frozenset[str]) -> str | None: """Value of a space-separated, ``=``-attached, glued, or clustered option. Clustered short options are supported when the value-taking letter is last in the cluster (``-ao out.txt``) or followed by a glued value (``-aofoo``). """ - short_letters = _short_option_letters(names) + short_letters = frozenset( + name[1:] + for name in names + if name.startswith("-") and not name.startswith("--") and len(name) == 2 + ) index = 1 while index < len(argv): token = argv[index] @@ -545,14 +541,6 @@ def _git_output_path(tokens: list[str]) -> str | None: return None -def _git_external_helper(flags: list[str]) -> str | None: - for flag in flags: - name = flag.partition("=")[0] - if name in GIT_EXTERNAL_HELPER_OPTIONS: - return f"{name} can execute configured external helpers" - return None - - def _classify_git(argv: list[str], workspace: Path) -> CommandRisk: verb, verb_args, conservative_global = _scan_git_verb(argv) flags = [token for token in argv[1:] if token.startswith("-")] @@ -590,7 +578,12 @@ def _classify_git(argv: list[str], workspace: Path) -> CommandRisk: return CommandRisk(RiskLevel.MEDIUM, (f"git {verb} changes repository state",)) if conservative_global: return CommandRisk(RiskLevel.MEDIUM, ("git uses a non-benign global option",)) - helper = _git_external_helper(flags) + helper = None + for flag in flags: + name = flag.partition("=")[0] + if name in GIT_EXTERNAL_HELPER_OPTIONS: + helper = f"{name} can execute configured external helpers" + break if helper: return CommandRisk(RiskLevel.MEDIUM, (helper,)) if verb in GIT_READONLY_VERBS or ( @@ -663,10 +656,6 @@ def _classify_tree(argv: list[str], workspace: Path) -> CommandRisk | None: return None -def _format_field_list(value: str) -> list[str]: - return [part.strip().lower() for part in value.replace(" ", ",").split(",") if part.strip()] - - def _ps_format_exposes_environment(argv: list[str]) -> bool: """True when ``-o``/``-O``/``--format`` selects env/environ columns.""" format_names = frozenset({"-o", "-O", "--format", "--Format"}) @@ -699,7 +688,9 @@ def _ps_format_exposes_environment(argv: list[str]) -> bool: index += 1 if value is None: continue - fields = _format_field_list(value) + fields = [ + part.strip().lower() for part in value.replace(" ", ",").split(",") if part.strip() + ] if any(field in {"env", "environ", "environment"} for field in fields): return True return False diff --git a/shellpilot/runtime/conversation.py b/shellpilot/runtime/conversation.py index 8ce1c10..76f4cb4 100644 --- a/shellpilot/runtime/conversation.py +++ b/shellpilot/runtime/conversation.py @@ -669,14 +669,15 @@ def _turn_stats(self, elapsed_s: float) -> TurnStats: output_tokens=self._turn_output_tokens, ) - def _pending_plan_step(self) -> tuple[int, str] | None: - """First unfinished step of the active plan, as (1-based index, title). - - Returns the first step whose status is "active", else the first - "pending" step. Returns None when there is no plan, the plan is not yet - active (e.g. still "proposed" awaiting approval, or blocked/completed), - or every step is already in a terminal state. Used by the tool loop to - decide whether a no-tool-call reply should be nudged to keep executing. + def _plan_step(self, *, active_only: bool = False) -> tuple[int, str] | None: + """Relevant step of the active plan, as (1-based index, title). + + Returns the first step whose status is "active". Unless *active_only* is + set, falls back to the first "pending" step. Returns None when there is + no plan, the plan is not yet active (e.g. still "proposed" awaiting + approval, or blocked/completed), or no matching step remains. The tool + loop uses the fallback form to nudge stalled execution and the + active-only form to report where a declined action paused the plan. """ plan = self.plan_manager.active if plan is None or plan.status != "active": @@ -687,6 +688,8 @@ def _pending_plan_step(self) -> tuple[int, str] | None: ) if active is not None: return active, plan.steps[active - 1].title + if active_only: + return None pending = next( (i for i, step in enumerate(plan.steps, start=1) if step.status == "pending"), None, @@ -695,19 +698,6 @@ def _pending_plan_step(self) -> tuple[int, str] | None: return pending, plan.steps[pending - 1].title return None - def _active_plan_step(self) -> tuple[int, str] | None: - """Currently active plan step, without falling back to pending steps.""" - plan = self.plan_manager.active - if plan is None or plan.status != "active": - return None - active = next( - (i for i, step in enumerate(plan.steps, start=1) if step.status == "active"), - None, - ) - if active is None: - return None - return active, plan.steps[active - 1].title - def _tool_loop(self) -> Message: """Model call loop with tool dispatch, budgets, and recovery (section 10.4).""" executor = ToolExecutor( @@ -798,7 +788,7 @@ def _tool_loop(self) -> Message: history_before_reply = len(self._history) self._record(reply) if not reply.tool_calls: - pending = self._pending_plan_step() + pending = self._plan_step() if pending is not None and tools and nudges_used < MAX_PLAN_NUDGES: nudges_used += 1 index, title = pending @@ -921,7 +911,7 @@ def _tool_loop(self) -> Message: self._session.replace_last_message(reply) self._record(tool_result(outcome.model_text)) if outcome.stop_turn: - active_step = self._active_plan_step() + active_step = self._plan_step(active_only=True) if active_step is not None: index, _title = active_step self._ui.show_status(f"Action declined; plan paused on step {index}.") diff --git a/shellpilot/runtime/planner.py b/shellpilot/runtime/planner.py index 0e2209f..b8db59e 100644 --- a/shellpilot/runtime/planner.py +++ b/shellpilot/runtime/planner.py @@ -165,7 +165,7 @@ class PlanManager: def __init__(self, workspace: Path, profile: str, *, max_plan_steps: int = 10) -> None: self._workspace = workspace self._profile = profile - self._max_plan_steps = max_plan_steps + self.max_plan_steps = max_plan_steps self.active: TaskPlan | None = None self.pending_revision: str | None = None # Transient completion-guard state (runtime-only; never persisted to @@ -186,14 +186,6 @@ def set_profile(self, profile: str) -> None: """New tasks stamp *profile*; an active plan keeps its recorded profile.""" self._profile = profile - @property - def max_plan_steps(self) -> int: - return self._max_plan_steps - - @max_plan_steps.setter - def max_plan_steps(self, value: int) -> None: - self._max_plan_steps = value - def artifact_path(self, plan: TaskPlan) -> Path: # Pinned to the plan's own workspace (set at create, persisted, restored), # not the mutable self._workspace — so a mid-plan /cwd keeps PLAN.md where diff --git a/shellpilot/tools/base.py b/shellpilot/tools/base.py index 960ca3d..8dc7b27 100644 --- a/shellpilot/tools/base.py +++ b/shellpilot/tools/base.py @@ -199,3 +199,15 @@ def workspace_display(workspace: Path, raw_path: str) -> str: relative = resolved.relative_to(workspace.resolve()) rel_str = relative.as_posix() return "." if rel_str == "." else rel_str + + +def make_workspace_path_display( + workspace: Path | None, workspace_fn: Callable[[], Path] | None +) -> Callable[[str], str]: + """Build a path display callback preferring the live workspace provider.""" + + def path_display(path: str) -> str: + current = workspace_fn() if workspace_fn is not None else workspace + return workspace_display(current, path) if current is not None else path + + return path_display diff --git a/tests/conftest.py b/tests/conftest.py index 4a5c561..596f613 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,8 +35,3 @@ def _hermetic_color_env(monkeypatch: pytest.MonkeyPatch) -> None: "0000000c49444154789c63f8ffff3f0005fe02fe0def46b8" # IDAT chunk "0000000049454e44ae426082" # IEND chunk ) - - -def test_tiny_png_has_valid_signature() -> None: - """TINY_PNG starts with the 8-byte PNG magic number.""" - assert TINY_PNG[:8] == b"\x89PNG\r\n\x1a\n" diff --git a/tests/fakes/fake_llm.py b/tests/fakes/fake_llm.py index 308a10a..9beb2c1 100644 --- a/tests/fakes/fake_llm.py +++ b/tests/fakes/fake_llm.py @@ -29,6 +29,17 @@ def tool_call(name: str, **arguments: Any) -> Message: return assistant("", tool_calls=(ToolCall(name=name, arguments=arguments),)) +def canonical_plan_call() -> Message: + """Script entry: the canonical plan proposal used across runtime tests.""" + return tool_call( + "propose_plan", + goal="Add a feature", + steps=["Inspect code", "Make change", "Run tests"], + assumptions=["repo is clean"], + verification=["pytest"], + ) + + @dataclass class RecordedCall: model: str diff --git a/tests/test_app_ui.py b/tests/test_app_ui.py index 9dbe629..15e41c6 100644 --- a/tests/test_app_ui.py +++ b/tests/test_app_ui.py @@ -1115,8 +1115,14 @@ def test_tool_call_path_uses_live_workspace(tmp_path: Path) -> None: ws_b = tmp_path / "sub" holder: dict[str, Path] = {"ws": ws_a} - # workspace_fn returns the live workspace from the holder. - ui = AppUI(glyphs=GLYPHS, workspace_fn=lambda: holder["ws"], width_fn=lambda: 80) + # workspace_fn returns the live workspace from the holder and must win over + # the deliberately stale static fallback. + ui = AppUI( + glyphs=GLYPHS, + workspace=ws_b, + workspace_fn=lambda: holder["ws"], + width_fn=lambda: 80, + ) # The absolute path is inside ws_a but outside ws_b. abs_path = str(ws_a / "file.txt") diff --git a/tests/test_audit.py b/tests/test_audit.py index e070d16..0095f43 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -42,6 +42,18 @@ def test_secrets_redacted_in_events(tmp_path: Path) -> None: assert "ghp_" not in event["command"] +def test_redaction_applies_to_values_not_top_level_field_names(tmp_path: Path) -> None: + logger = make_logger(tmp_path) + logger.write( + "command_result", + token="audit label", + command="export TOKEN=ghp_abcdefghijklmnopqrstuvwxyz012345", + ) + event = json.loads((tmp_path / "audit.jsonl").read_text()) + assert event["token"] == "audit label" + assert event["command"] == "export [REDACTED]" + + def test_redaction_can_be_disabled(tmp_path: Path) -> None: logger = make_logger(tmp_path, redact=False) logger.write("command_result", command="AKIAIOSFODNN7EXAMPLE") diff --git a/tests/test_command_policy.py b/tests/test_command_policy.py index cb02249..377d56b 100644 --- a/tests/test_command_policy.py +++ b/tests/test_command_policy.py @@ -507,6 +507,26 @@ def test_low_invariant_adversarial_corpus(argv: list[str], expected: RiskLevel) assert result.risk == expected, f"{argv}: {result.reasons}" +def test_tree_clustered_output_flag_outside_workspace_is_high() -> None: + result = classify_command(["tree", "-ao/tmp/out.txt"], workspace=WS) + assert result.risk == RiskLevel.HIGH + assert result.reasons == ("tree output path is outside the workspace boundary",) + + +def test_git_first_external_helper_flag_wins() -> None: + result = classify_command( + ["git", "diff", "--textconv=first", "--ext-diff=second"], workspace=WS + ) + assert result.risk == RiskLevel.MEDIUM + assert result.reasons == ("--textconv can execute configured external helpers",) + + +def test_ps_format_fields_normalize_case_and_spacing() -> None: + result = classify_command(["ps", "--format", "PID EnViRoN"], workspace=WS) + assert result.risk == RiskLevel.MEDIUM + assert result.reasons == ("ps can expose process environments",) + + # -- sensitive_path_reason (component-exact, not substring) -------------------- SENSITIVE_PATHS: list[str] = [ diff --git a/tests/test_conversation.py b/tests/test_conversation.py index 1ae2015..8cfb46a 100644 --- a/tests/test_conversation.py +++ b/tests/test_conversation.py @@ -1693,6 +1693,27 @@ def test_non_final_step_completion_still_nudges(tmp_path: Path) -> None: assert any(c.startswith(plan_prefix) for c in tool_msgs) +def test_plan_nudge_selects_first_pending_step_when_none_is_active(tmp_path: Path) -> None: + from shellpilot.runtime.conversation import PLAN_CONTINUE_NUDGE + + fake = FakeLLM(script=[answer("Working."), answer("Still working."), answer("Stopping.")]) + runtime = make_runtime(fake, FakeUI(), tmp_path) + runtime.plan_manager.create( + goal="Set up the service", + user_intent="set up the service", + steps=["Skip this", "First pending", "Later pending"], + assumptions=[], + verification=[], + ) + runtime.plan_manager.approve() + runtime.plan_manager.update_step(1, "skipped") + + runtime.run_turn("continue") + + tool_msgs = [m.content for m in runtime._history if m.role == "tool"] + assert PLAN_CONTINUE_NUDGE.format(index=2, title="First pending") in tool_msgs + + def test_empty_reply_still_routes_to_empty_nudge(tmp_path: Path) -> None: """An empty reply (no content, no tool calls) still hits the empty-reply path.""" from shellpilot.runtime.conversation import EMPTY_FIRST_NUDGE diff --git a/tests/test_explanations.py b/tests/test_explanations.py index 9560dda..a1ba3b4 100644 --- a/tests/test_explanations.py +++ b/tests/test_explanations.py @@ -24,78 +24,6 @@ WS = Path("/tmp/fake-workspace") -# -- per-reason mapping -------------------------------------------------------- - -PER_REASON_CASES: list[tuple[str, str]] = [ - ( - "sudo is privileged or system-destructive", - "Runs with elevated privileges and can make system-wide changes that may be irreversible.", - ), - ( - "bash runs raw shell syntax; use Manual Shell instead", - "Launches a raw shell that can execute arbitrary, unchecked commands.", - ), - ( - "recursive delete", - "Recursively and permanently deletes the target and everything inside it; " - "this cannot be undone.", - ), - ( - "glob delete", - "Deletes every file matching the pattern; the set is expanded at run time " - "and cannot be undone.", - ), - ( - "deletes outside the workspace", - "Deletes files outside the workspace directory.", - ), - ( - "git reset can destroy local changes", - "Can discard or overwrite uncommitted local changes, which cannot be recovered.", - ), - ( - "git branch deletion", - "Deletes a git branch.", - ), - ( - "force push rewrites remote history", - "Force-pushes, rewriting published remote history and possibly overwriting " - "others' commits.", - ), - ( - "recursive permission change", - "Recursively changes permissions or ownership across a directory tree.", - ), - ( - "find with -delete/-exec can modify files", - "Runs find with -delete/-exec, which can modify or remove matched files.", - ), - ( - "argument '.env' looks like a credential/secret path", - "Touches a path that looks like a credential or secret file.", - ), - ( - "target ../outside is outside the workspace boundary", - "Writes to a path outside the workspace directory.", - ), -] - - -@pytest.mark.parametrize(("reason", "expected"), PER_REASON_CASES) -def test_per_reason_mapping(reason: str, expected: str) -> None: - assert explain_risk((reason,)) == expected - - -def test_per_reason_from_real_classification() -> None: - # Build the reason via the real classifier rather than hardcoding it. - result = classify_command(["rm", "-rf", "build"], workspace=WS) - assert result.reasons == ("recursive delete",) - assert explain_risk(result.reasons) == ( - "Recursively and permanently deletes the target and everything inside it; " - "this cannot be undone." - ) - - # -- multi-reason composition -------------------------------------------------- diff --git a/tests/test_plan_flow.py b/tests/test_plan_flow.py index dfc34a7..0a89f38 100644 --- a/tests/test_plan_flow.py +++ b/tests/test_plan_flow.py @@ -10,7 +10,7 @@ from shellpilot.persistence.audit_store import AuditLogger from shellpilot.policy.risk import RiskLevel from shellpilot.runtime.conversation import ConversationRuntime -from tests.fakes.fake_llm import FakeLLM, answer, tool_call +from tests.fakes.fake_llm import FakeLLM, answer, canonical_plan_call, tool_call from tests.fakes.fake_ui import FakeUI @@ -31,21 +31,11 @@ def make_runtime( ) -def plan_call() -> Message: - return tool_call( - "propose_plan", - goal="Add a feature", - steps=["Inspect code", "Make change", "Run tests"], - assumptions=["repo is clean"], - verification=["pytest"], - ) - - def test_plan_proposal_approved_and_artifact_written(tmp_path: Path) -> None: # After approval, step 1 is active; the model completes the steps in-turn. fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), tool_call("update_plan", step=1, status="completed"), tool_call("update_plan", step=2, status="completed"), tool_call("update_plan", step=3, status="completed"), @@ -70,7 +60,7 @@ def test_plan_proposal_approved_and_artifact_written(tmp_path: Path) -> None: def test_plan_rejection_stops_execution(tmp_path: Path) -> None: - fake = FakeLLM(script=[plan_call(), answer("Okay, what would you like instead?")]) + fake = FakeLLM(script=[canonical_plan_call(), answer("Okay, what would you like instead?")]) ui = FakeUI(plan_answer=("n", "")) runtime = make_runtime(fake, ui, tmp_path) @@ -82,7 +72,7 @@ def test_plan_rejection_stops_execution(tmp_path: Path) -> None: def test_plan_edit_requests_revision(tmp_path: Path) -> None: - fake = FakeLLM(script=[plan_call(), answer("Here is a revised approach.")]) + fake = FakeLLM(script=[canonical_plan_call(), answer("Here is a revised approach.")]) ui = FakeUI(plan_answer=("e", "skip the tests step")) runtime = make_runtime(fake, ui, tmp_path) @@ -103,7 +93,7 @@ def test_e_then_repropose_single_task_dir_integration(tmp_path: Path) -> None: ) fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), revised_plan, tool_call("update_plan", step=1, status="completed"), tool_call("update_plan", step=2, status="completed"), @@ -149,7 +139,7 @@ def ask_plan_approval(self, plan: object, path: str) -> tuple[str, str]: def test_clear_with_pending_revision_next_propose_is_fresh(tmp_path: Path) -> None: """After /clear on a pending-revision state, next propose_plan creates a new task.""" # First turn: propose, user picks "e" - fake1 = FakeLLM(script=[plan_call(), answer("Let me revise.")]) + fake1 = FakeLLM(script=[canonical_plan_call(), answer("Let me revise.")]) ui1 = FakeUI(plan_answer=("e", "make it shorter")) runtime = make_runtime(fake1, ui1, tmp_path) runtime.run_turn("Please add the feature") @@ -196,7 +186,7 @@ def test_update_plan_completes_steps(tmp_path: Path) -> None: # active; the bounded nudge fires twice, then the turn ends on plain text. fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), tool_call("update_plan", step=1, status="completed", note="inspected"), answer("Step 1 done."), answer("Still narrating step 2."), @@ -216,7 +206,7 @@ def test_update_plan_completes_steps(tmp_path: Path) -> None: def test_blocker_tool_blocks_plan_and_instructs_protocol(tmp_path: Path) -> None: fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), tool_call("update_plan", blocker="pytest fails: ModuleNotFoundError"), answer("I hit a roadblock and recorded it."), ] diff --git a/tests/test_plan_restore.py b/tests/test_plan_restore.py index 0311310..8a3b705 100644 --- a/tests/test_plan_restore.py +++ b/tests/test_plan_restore.py @@ -10,7 +10,7 @@ from shellpilot.memory.agents_md import BehaviorInstructions from shellpilot.persistence.sessions import SessionStore from shellpilot.runtime.conversation import ConversationRuntime -from tests.fakes.fake_llm import FakeLLM, answer, tool_call +from tests.fakes.fake_llm import FakeLLM, answer, canonical_plan_call, tool_call from tests.fakes.fake_ui import FakeUI @@ -31,16 +31,6 @@ def make_runtime( ) -def plan_call() -> Message: - return tool_call( - "propose_plan", - goal="Add a feature", - steps=["Inspect code", "Make change", "Run tests"], - assumptions=["repo is clean"], - verification=["pytest"], - ) - - # --------------------------------------------------------------------------- # Test 1: pointer recorded at transition # --------------------------------------------------------------------------- @@ -50,7 +40,7 @@ def test_active_plan_pointer_recorded_at_approval(tmp_path: Path) -> None: store = SessionStore(tmp_path / "sessions", "test-sess") fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), tool_call("update_plan", step=1, status="completed"), tool_call("update_plan", step=2, status="completed"), tool_call("update_plan", step=3, status="completed"), @@ -300,7 +290,7 @@ def test_step_progress_no_extra_pointer_records(tmp_path: Path) -> None: store = SessionStore(tmp_path / "sessions", "dedup-sess") fake = FakeLLM( script=[ - plan_call(), + canonical_plan_call(), tool_call("update_plan", step=1, status="completed"), tool_call("update_plan", step=2, status="completed"), tool_call("update_plan", step=3, status="completed"), diff --git a/tests/test_render.py b/tests/test_render.py index 56cb2aa..3561875 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -232,14 +232,6 @@ def _additions_diff(count: int, name: str = "big.py") -> str: return make_diff("", after, name=name) -def test_render_diff_max_rows_param_exists() -> None: - """CI guard: render_diff keeps its keyword-only max_rows parameter.""" - import inspect - - sig = inspect.signature(render_diff) - assert "max_rows" in sig.parameters - - def test_render_diff_max_rows_caps_output_with_footer() -> None: diff = _additions_diff(30) out = rendered(render_diff(diff, GLYPHS, max_rows=10)) @@ -281,14 +273,6 @@ def test_render_diff_footer_style_is_faint() -> None: assert footer.plain.startswith(f"{GLYPHS.ellipsis} (+") -def test_render_diff_width_param_exists() -> None: - """CI guard: render_diff keeps its keyword-only width parameter (§31.16).""" - import inspect - - sig = inspect.signature(render_diff) - assert "width" in sig.parameters - - def test_render_diff_width_standardizes_panel_width() -> None: """With width set, every diff renders to the SAME panel width regardless of its content width — a standardized diff window, not one hugging its longest diff --git a/tests/test_skill_injection.py b/tests/test_skill_injection.py index ff79ffc..fa43c23 100644 --- a/tests/test_skill_injection.py +++ b/tests/test_skill_injection.py @@ -15,7 +15,7 @@ from shellpilot.skills.triggers import TriggerContext from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec from shellpilot.tools.registry import default_registry -from tests.fakes.fake_llm import FakeLLM, answer, tool_call +from tests.fakes.fake_llm import FakeLLM, answer, canonical_plan_call, tool_call from tests.fakes.fake_ui import FakeUI @@ -53,16 +53,6 @@ def _user_skill(tmp_path: Path, folder: str, body: str) -> Path: return skills_dir -def _plan_call() -> object: - return tool_call( - "propose_plan", - goal="Add a feature", - steps=["Inspect code", "Make change", "Run tests"], - assumptions=["repo is clean"], - verification=["pytest"], - ) - - def _system_texts(fake: FakeLLM) -> list[str]: return [call.messages[0].content for call in fake.calls if call.messages] @@ -72,7 +62,7 @@ def test_planning_skill_injected_only_when_plan_active(tmp_path: Path) -> None: # do. The planning skill body must appear only in the post-approval calls. fake = FakeLLM( script=[ - _plan_call(), + canonical_plan_call(), tool_call("update_plan", step=1, status="completed"), tool_call("update_plan", step=2, status="completed"), tool_call("update_plan", step=3, status="completed"), diff --git a/tests/test_streaming.py b/tests/test_streaming.py index 13ce733..12dd08e 100644 --- a/tests/test_streaming.py +++ b/tests/test_streaming.py @@ -195,12 +195,6 @@ def test_phase_for_elapsed_boundaries() -> None: assert phase_for_elapsed(1000.0).name == "long-haul" -def test_phase_for_elapsed_is_deterministic() -> None: - """phase_for_elapsed is a pure function — same input always returns same phase.""" - for elapsed in (0.0, 5.0, 10.0, 20.0, 60.0, 120.0): - assert phase_for_elapsed(elapsed).name == phase_for_elapsed(elapsed).name - - def test_phase_pools_are_populated() -> None: """Each phase pool must be non-empty and contain only lowercase strings.""" for phase in FLIGHT_PHASES: @@ -322,7 +316,7 @@ def test_labeled_frame_contains_no_flight_phrase() -> None: spinner.start(label="fueling gemma4:e4b") time.sleep(0.2) assert spinner.active - frame_text = spinner._current_label_text() + frame_text = spinner._frame(0).plain assert "fueling gemma4:e4b" in frame_text for phrase in ALL_PHRASES: assert phrase not in frame_text, f"unexpected phrase {phrase!r} in labeled frame" @@ -335,7 +329,7 @@ def test_unlabeled_frame_uses_flight_phrase() -> None: spinner = AviationSpinner(terminal_console(), GLYPHS, enabled=True) spinner.start() assert spinner.active - frame_text = spinner._current_label_text() + frame_text = spinner._frame(0).plain assert any(phrase in frame_text for phrase in ALL_PHRASES) spinner.stop() diff --git a/tests/test_terminal_ui.py b/tests/test_terminal_ui.py index e9055b2..c841c14 100644 --- a/tests/test_terminal_ui.py +++ b/tests/test_terminal_ui.py @@ -1066,7 +1066,13 @@ def test_tool_call_path_uses_live_workspace(tmp_path: Path) -> None: holder: dict[str, Path] = {"ws": ws_a} console = make_console() - ui = TerminalUI(console, glyphs=GLYPHS, spinner=False, workspace_fn=lambda: holder["ws"]) + ui = TerminalUI( + console, + glyphs=GLYPHS, + spinner=False, + workspace=ws_b, + workspace_fn=lambda: holder["ws"], + ) # The absolute path is inside ws_a but outside ws_b. abs_path = str(ws_a / "file.txt") diff --git a/tests/test_web_extract.py b/tests/test_web_extract.py index 413c501..6c9b79a 100644 --- a/tests/test_web_extract.py +++ b/tests/test_web_extract.py @@ -138,11 +138,3 @@ def test_title_inside_skip_region_ignored() -> None: html = "inner" page = extract_text(html) assert page.title == "" - - -def test_returns_extracted_page_dataclass() -> None: - page = extract_text("

hi

") - assert isinstance(page, ExtractedPage) - assert isinstance(page.title, str) - assert isinstance(page.text, str) - assert isinstance(page.truncated, bool)