From 4af626f4fcd89e96ef0eda79d744a31f9f196fe3 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 05:54:54 -0400 Subject: [PATCH 01/27] fix(security): escalate git mutating verbs and option-encoded paths off auto-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The balanced profile auto-runs LOW-risk commands with no prompt. Two holes let model-proposed argv reach AUTO under indirect injection: - git's --output is a global diff-formatting option honoured by every diff-emitting verb (diff/show/log/stash show), so `git log --output=/etc/x` was an arbitrary out-of-workspace write/truncate primitive at LOW. A single hoisted check scans the whole invocation for an --output/-O path value and routes it through the workspace boundary (outside HIGH, inside MEDIUM), placed after every HIGH determination so it only ever escalates a would-be LOW result. Mutating branch/remote subcommands (branch , remote add/set-url) classify MEDIUM; bare and listing forms stay LOW. - option-encoded paths (grep --file=/etc/passwd, patch --output=/etc/x) were skipped by the boundary check's leading-dash filter; the substring after the first = is now boundary-checked when path-like. Glued short forms (-f/path) remain a documented residual. DESIGN.md §36.1 updated. --- docs/DESIGN.md | 2 + shellpilot/policy/command_policy.py | 71 +++++++++++++++++++--- tests/test_command_policy.py | 94 ++++++++++++++++++++++++++++- 3 files changed, 158 insertions(+), 9 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7e9ae6b..7571dc9 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -2981,6 +2981,8 @@ The dedicated `read_file`/`search_text` tools enforce the workspace boundary and - **Reader executables now honour the boundary.** A reader executable (`cat`/`head`/`tail`/`grep`/`rg`/`wc`/`file`/`stat`/`du`) whose path argument resolves **outside the workspace**, or names a secret marker, is escalated to `RiskLevel.HIGH` (always-ask) instead of returning LOW/auto-run. Because reader commands carry `SideEffect.VARIABLE` they can never auto-run silently, so the deterministic over-ask (HIGH → ASK) is the conservative match for the file tools' boundary, rather than threading the `allow_sensitive_reads` setting through the command path. - **`git` global options feed classification.** A `git` invocation carrying a non-benign global option before the verb (`--git-dir`/`--work-tree`/`--exec-path`/`-C`/`-c` and the like) is treated as at least MEDIUM (ASK), closing an out-of-workspace read primitive that the previous first-non-dash-token verb derivation skipped past. Only `--no-pager`/`--literal-pathspecs` remain benign. +- **Mutating verbs and `--output` under the read-only allowlist no longer auto-run.** The `GIT_READONLY_VERBS` LOW return is read-*only*, but two forms slipped through it under `balanced`. First, `git branch ` / `git remote add|set-url …` (a non-flag positional names a branch/remote to create, rename, add, or repoint — a state mutation) now classify at least MEDIUM (ASK), while bare `git branch`/`git remote`/`git remote -v` and listing forms (`git branch --list `, `-a`/`-r`) stay LOW. Second, `--output=` is a **global diff-formatting option honoured by every diff-emitting verb** (`diff`/`show`/**`log`**/**`stash show`**/…), so `git log --output=/etc/x` was an arbitrary out-of-workspace file-write/truncate primitive at LOW→AUTO just like `git diff --output=`. A single hoisted check now scans the whole invocation for an `--output`/`-O` path value (the `=`, space-separated, and glued forms) and routes it through the same workspace-boundary check as the write/reader paths: outside → HIGH (always-ask), inside → MEDIUM. The check is placed **after** every HIGH determination (`reset`/`clean`, `branch -d/-D`, force `push`) and **before** any read-only LOW return, so it only escalates a would-be-LOW command and never downgrades an already-HIGH one (`git branch -D x --output=in_ws.txt` stays HIGH). `-O` is git's order-file option, not an `--output` short form; it is covered conservatively (an out-of-workspace order-file read is still a boundary crossing worth escalating). The git path previously never consulted the boundary check at all. +- **Option-encoded paths are checked, not skipped.** `_path_arg_outside_workspace` skipped every `-`-prefixed token, so a path hidden in an option value (`grep --file=/etc/passwd .`, `patch --output=/etc/x`) bypassed the boundary entirely and fell through to the LOW reader/write allowlist. For a `-`-token containing `=`, the substring after the first `=` is now run through the same resolve-and-compare check when it looks path-like (contains `/` or starts with `..`); a non-path option value (`--color=auto`, `--include=*.py`) is still ignored, so an in-workspace option path keeps its LOW/MEDIUM classification and only out-of-workspace targets escalate. **Accepted residual:** only `--opt=PATH` and space-separated (`--opt PATH`) forms are boundary-checked; a path glued to a short flag (`grep -f/etc/passwd`, `cp -t/outside`) is still skipped, so the glued short form remains a known gap — a glued out-of-workspace reader path (`grep -f/etc/passwd .`) still classifies LOW and can auto-run under `balanced`. The high-value `--opt=PATH` primitive (the common, model-natural form) is closed; the glued short form is the lower-value remainder and is left as a documented residual rather than parsing every command's short-flag grammar. - **`pytest` requires approval.** `pytest` and `python -m pytest` execute arbitrary project Python (conftest/collected modules) at collection time, so the previous LOW carve-out is removed: they now classify MEDIUM (ASK in `balanced`), symmetric with `python script.py`. - **Path-qualified basenames are distrusted.** When `argv[0]` contains a path separator (`./grep`, `/abs/grep`), the bare-name LOW allowlist no longer applies; a path-qualified executable is classified at least MEDIUM, so a workspace-staged file sharing a trusted command's name cannot auto-run. diff --git a/shellpilot/policy/command_policy.py b/shellpilot/policy/command_policy.py index bfc954c..f1e2305 100644 --- a/shellpilot/policy/command_policy.py +++ b/shellpilot/policy/command_policy.py @@ -168,17 +168,26 @@ def _path_arg_outside_workspace(argv: list[str], workspace: Path) -> str | None: root = workspace.resolve() for token in argv[1:]: if token.startswith("-"): - # skip flags/options - continue + # A flag may still hide a path in its value (e.g. --file=/etc/passwd, + # --output=/etc/x). Check the substring after the first '=' when it + # looks path-like; otherwise the flag carries no path to check. + if "=" not in token: + continue + value = token.split("=", 1)[1] + if "/" not in value and not value.startswith(".."): + continue + candidate = value + else: + candidate = token try: - if token.startswith("/"): - target = Path(token).resolve() + if candidate.startswith("/"): + target = Path(candidate).resolve() else: - target = (workspace / token).resolve() + target = (workspace / candidate).resolve() except OSError: continue if root != target and root not in target.parents: - return f"target {token} is outside the workspace boundary" + return f"target {candidate} is outside the workspace boundary" return None @@ -223,7 +232,34 @@ def _is_git_push_short_force_flag(flag: str) -> bool: return False -def _classify_git(argv: list[str]) -> CommandRisk: +GIT_BRANCH_READONLY_FLAGS: Final = frozenset( + {"-l", "--list", "-a", "--all", "-r", "--remotes", "--contains", "--merged", "--points-at"} +) + + +def _git_output_path(tokens: list[str]) -> str | None: + """The path value of a ``--output`` (or ``-O`` order-file) option, else None. + + ``--output`` is a global diff-formatting option honoured by every + diff-emitting verb (``diff``/``show``/``log``/``stash show``/…), so the whole + invocation is scanned rather than a single verb. ``-O`` is git's order-file + option, not an ``--output`` short form; it is covered conservatively (an + out-of-workspace order-file is still a boundary crossing worth escalating). + """ + index = 0 + while index < len(tokens): + token = tokens[index] + if token in ("--output", "-O"): + return tokens[index + 1] if index + 1 < len(tokens) else None + if token.startswith("--output="): + return token[len("--output=") :] + if token.startswith("-O") and len(token) > 2: + return token[2:] + index += 1 + 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("-")] if verb in GIT_HIGH: @@ -239,6 +275,25 @@ def _classify_git(argv: list[str]) -> CommandRisk: ): return CommandRisk(RiskLevel.HIGH, ("force push rewrites remote history",)) return CommandRisk(RiskLevel.MEDIUM, ("git push publishes commits",)) + # --output writes an arbitrary file from any diff-emitting verb, so scan the + # whole invocation. Placed after every HIGH determination above and before + # any read-only LOW return so it only ever escalates a would-be-LOW command + # (never downgrades an already-HIGH one, e.g. `branch -D x --output=in_ws`). + output_path = _git_output_path(argv[1:]) + if output_path is not None: + if _path_arg_outside_workspace(["git", output_path], workspace): + return CommandRisk( + RiskLevel.HIGH, + (f"git {verb or '?'} output path is outside the workspace boundary",), + ) + return CommandRisk(RiskLevel.MEDIUM, (f"git {verb or '?'} writes output to a file",)) + if verb in ("branch", "remote"): + # A non-flag positional names a branch/remote to create/rename/add/set — + # a state mutation, not a read-only listing (which carries no name or a + # listing flag like --list/-a). Bare `git branch`/`git remote -v` stay LOW. + listing = verb == "branch" and any(flag in GIT_BRANCH_READONLY_FLAGS for flag in flags) + if not listing and any(not arg.startswith("-") for arg in verb_args): + return CommandRisk(RiskLevel.MEDIUM, (f"git {verb} changes repository state",)) if conservative_global: return CommandRisk(RiskLevel.MEDIUM, ("git uses a non-benign global option",)) if verb in GIT_READONLY_VERBS and verb != "stash": @@ -284,7 +339,7 @@ def classify_command(argv: list[str], *, workspace: Path) -> CommandRisk: if executable == "rm": return _classify_rm(argv, workspace) if executable == "git": - risk = _classify_git(argv) + risk = _classify_git(argv, workspace) if secret: return CommandRisk(RiskLevel.HIGH, (*risk.reasons, secret)) return risk diff --git a/tests/test_command_policy.py b/tests/test_command_policy.py index c043fb8..954c69a 100644 --- a/tests/test_command_policy.py +++ b/tests/test_command_policy.py @@ -4,7 +4,11 @@ import pytest -from shellpilot.policy.command_policy import classify_command, sensitive_path_reason +from shellpilot.policy.command_policy import ( + _path_arg_outside_workspace, + classify_command, + sensitive_path_reason, +) from shellpilot.policy.risk import RiskLevel WS = Path("/tmp/fake-workspace") @@ -317,6 +321,94 @@ def test_mv_outside_behavior_unchanged_after_rename() -> None: # -- end reader-command boundary tests ---------------------------------------- +# -- git mutating verbs under read-only LOW (#1) ------------------------------ + + +@pytest.mark.parametrize( + ("argv", "expected"), + [ + # --output is a global diff-formatting option honoured by every + # diff-emitting verb (diff/show/log/stash show), not just diff/show, so + # every one is an out-of-workspace write/truncate primitive under LOW. + (["git", "diff", "--output=/tmp/x"], RiskLevel.HIGH), + (["git", "show", "--output=/tmp/x"], RiskLevel.HIGH), + (["git", "diff", "--output", "/tmp/x"], RiskLevel.HIGH), + (["git", "diff", "-O/tmp/x"], RiskLevel.HIGH), + (["git", "log", "--output=/tmp/x"], RiskLevel.HIGH), + (["git", "log", "-p", "--output=/tmp/x"], RiskLevel.HIGH), + (["git", "log", "--output", "/tmp/x"], RiskLevel.HIGH), + (["git", "log", "-O/tmp/x"], RiskLevel.HIGH), + (["git", "stash", "show", "-p", "--output=/tmp/x"], RiskLevel.HIGH), + # In-workspace --output is still a write -> MEDIUM (ask), never LOW/auto. + (["git", "diff", "--output=out.diff"], RiskLevel.MEDIUM), + (["git", "log", "--output=out.diff"], RiskLevel.MEDIUM), + # branch/remote with a non-flag positional mutate state -> MEDIUM. + (["git", "branch", "newtopic"], RiskLevel.MEDIUM), + (["git", "remote", "add", "o", "u"], RiskLevel.MEDIUM), + (["git", "remote", "set-url", "o", "u"], RiskLevel.MEDIUM), + ], + ids=lambda case: str(case), +) +def test_git_mutating_verbs_escalate(argv: list[str], expected: RiskLevel) -> None: + result = classify_command(argv, workspace=WS) + assert result.risk == expected, f"{argv}: {result.reasons}" + + +def test_git_output_check_does_not_downgrade_high_verb() -> None: + # An already-HIGH determination (branch delete) must win over the + # output-path check even when --output points inside the workspace. + result = classify_command(["git", "branch", "-D", "topic", "--output=in_ws.txt"], workspace=WS) + assert result.risk == RiskLevel.HIGH, result.reasons + + +@pytest.mark.parametrize( + "argv", + [ + # No-regression: bare/listing read-only forms must stay LOW (auto-run). + ["git", "branch"], + ["git", "branch", "--list"], + ["git", "branch", "--list", "topic"], + ["git", "branch", "-r"], + ["git", "remote"], + ["git", "remote", "-v"], + ["git", "diff", "--stat"], + ["git", "log"], + ["git", "log", "-p"], + ["git", "log", "--stat"], + ["git", "stash", "show"], + ["git", "stash", "list"], + ], + ids=lambda case: str(case), +) +def test_git_readonly_verbs_stay_low(argv: list[str]) -> None: + result = classify_command(argv, workspace=WS) + assert result.risk == RiskLevel.LOW, f"{argv}: {result.reasons}" + + +# -- option-encoded paths evade the workspace boundary (#14) ------------------- + + +def test_grep_option_encoded_outside_path_is_high() -> None: + result = classify_command(["grep", "--file=/etc/passwd", "."], workspace=WS) + assert result.risk == RiskLevel.HIGH + assert any("outside the workspace" in reason for reason in result.reasons) + + +def test_patch_option_encoded_outside_path_detected() -> None: + assert _path_arg_outside_workspace(["patch", "--output=/etc/x"], WS) is not None + + +def test_grep_option_encoded_inside_path_stays_low() -> None: + result = classify_command(["grep", "--file=./patterns.txt", "."], workspace=WS) + assert result.risk == RiskLevel.LOW + + +def test_grep_non_path_option_value_stays_low() -> None: + # --color=auto / --include=*.py are not path-like; no false-positive escalation. + result = classify_command(["grep", "--color=auto", "TODO", "."], workspace=WS) + assert result.risk == RiskLevel.LOW + + def test_reasons_are_present_for_risky_commands() -> None: result = classify_command(["rm", "-rf", "build"], workspace=WS) assert result.reasons From 7a17b9aac6a8ddb7f283c8fb0bac4d579b9323ae Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 06:00:32 -0400 Subject: [PATCH 02/27] fix(security): hard-bound command output per read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A newline-less stream (e.g. `head -c /dev/zero`, or cat-ing a crafted single-line file) made the text-mode line iterator buffer the whole stream into one string before the capture cap was ever consulted, and the first line was appended regardless of the cap -- unbounded memory plus render amplification, reachable at auto-run under balanced (cat/head/tail classify LOW). Replace the line iterator with a readline(MAX_READ_CHARS=65536) loop so each read is bounded; the existing total cap then bounds capture and the first-line-always-appended case is fixed. Streaming (emit_line) and the truncated flag are unchanged. DESIGN.md §13.1 updated. --- docs/DESIGN.md | 2 +- shellpilot/tools/command.py | 8 +++- tests/test_run_command.py | 87 +++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7571dc9..de55ce7 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1088,7 +1088,7 @@ Execution: - Use `subprocess.Popen(argv, shell=False)`. - Stream stdout/stderr. -- Bound captured output. +- Bound captured output; each read is hard-capped at 65 536 chars so a newline-less stream cannot materialise an unbounded string before the total capture cap is consulted. - Enforce timeout. - Kill process group on timeout. - Return exit code and captured output. diff --git a/shellpilot/tools/command.py b/shellpilot/tools/command.py index b07bc59..cad8a50 100644 --- a/shellpilot/tools/command.py +++ b/shellpilot/tools/command.py @@ -26,6 +26,9 @@ from shellpilot.tools.filesystem import ALL_PROFILES DEFAULT_TIMEOUT_SECONDS = 600 +# Maximum chars read in a single readline() call so a newline-less stream cannot +# materialise an unbounded string before the total-capture cap is consulted. +MAX_READ_CHARS = 65_536 # Exit codes that mean "ran fine, found nothing" rather than failure (section 24.3). EXPECTED_NONZERO: dict[str, frozenset[int]] = { "grep": frozenset({1}), @@ -133,7 +136,10 @@ def run_command_process( def reader() -> None: nonlocal captured_chars, truncated assert process.stdout is not None - for line in process.stdout: + while True: + line = process.stdout.readline(MAX_READ_CHARS) + if not line: + break if emit_line is not None: emit_line(line.rstrip("\n")) if captured_chars < max_capture_chars: diff --git a/tests/test_run_command.py b/tests/test_run_command.py index 2232513..ddb2e4a 100644 --- a/tests/test_run_command.py +++ b/tests/test_run_command.py @@ -12,6 +12,7 @@ from shellpilot.runtime.executor import ToolExecutor from shellpilot.tools.base import ToolContext from shellpilot.tools.command import ( + MAX_READ_CHARS, RUN_COMMAND, CommandRequest, _precheck_run_command, @@ -590,3 +591,89 @@ def _fake_outcome() -> Any: from shellpilot.tools.command import CommandOutcome return CommandOutcome(exit_code=0, output="", timed_out=False, truncated=False) + + +# --------------------------------------------------------------------------- +# Newline-less output: per-read chunk bound (#17) +# --------------------------------------------------------------------------- + + +def test_newline_less_runaway_is_truncated(tmp_path: Path) -> None: + """A newline-less 5 MB stream is hard-bounded; truncated flag is set. + + Pre-fix: the text-mode line iterator materialises the entire 5 MB string + before the cap is consulted, so captured output == 5 MB and truncated stays + False. Post-fix: readline(MAX_READ_CHARS) caps each read so total capture + stays within max_capture_chars + MAX_READ_CHARS. + """ + cap = 200_000 + outcome = run_command_process( + CommandRequest( + argv=[ + sys.executable, + "-c", + "import sys; sys.stdout.write('x' * 5_000_000); sys.stdout.flush()", + ], + cwd=tmp_path, + timeout_seconds=30, + ), + max_capture_chars=cap, + ) + assert outcome.truncated is True, "truncated must be set for newline-less runaway" + assert len(outcome.output) <= cap + MAX_READ_CHARS, ( + f"output length {len(outcome.output)} exceeds cap + MAX_READ_CHARS " + f"({cap} + {MAX_READ_CHARS} = {cap + MAX_READ_CHARS})" + ) + + +def test_newline_less_small_output_untruncated(tmp_path: Path) -> None: + """A short newline-less stream (under the cap) is captured fully, not truncated.""" + outcome = run_command_process( + CommandRequest( + argv=[ + sys.executable, + "-c", + "import sys; sys.stdout.write('hello'); sys.stdout.flush()", + ], + cwd=tmp_path, + timeout_seconds=30, + ), + max_capture_chars=200_000, + ) + assert not outcome.truncated + assert "hello" in outcome.output + + +def test_multiline_normal_output_untruncated(tmp_path: Path) -> None: + """Normal multi-line output well under the cap is captured in full.""" + outcome = run_command_process( + CommandRequest( + argv=[sys.executable, "-c", "for i in range(10): print(f'line {i}')"], + cwd=tmp_path, + timeout_seconds=30, + ), + max_capture_chars=200_000, + ) + assert not outcome.truncated + for i in range(10): + assert f"line {i}" in outcome.output + + +def test_newline_rich_over_cap_still_truncates(tmp_path: Path) -> None: + """Newline-rich output that exceeds the cap still sets truncated (regression guard).""" + cap = 500 + outcome = run_command_process( + CommandRequest( + argv=[ + sys.executable, + "-c", + "for i in range(200): print('x' * 20)", + ], + cwd=tmp_path, + timeout_seconds=30, + ), + max_capture_chars=cap, + ) + assert outcome.truncated is True + # Each readline stops at the '\n' (line is 21 chars), so total <= cap + 21. + assert len(outcome.output) <= cap + 21 From 389e528bf44331a2309cfa7550c5a5d88515617a Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 06:06:23 -0400 Subject: [PATCH 03/27] fix(security): stop ambient proxy env from rerouting httpx traffic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three httpx clients (Ollama, web fetch, web search) defaulted to trust_env=True, so ambient HTTP_PROXY/ALL_PROXY/NO_PROXY (and ~/.netrc) could silently reroute traffic -- including the loopback Ollama prompt stream. That breaks the local-first invariant that the hardcoded base_url cannot be redirected by the environment, corrupts the egress audit's recorded destination, and defeats the hostname SSRF guard (a proxy does its own DNS). Set trust_env=False on all three clients. Tradeoff documented: a user behind a mandatory corporate proxy loses web_search/web_fetch connectivity (web tools are opt-in, off by default); an explicit [tools] http_proxy knob is a possible future follow-up, not built here. DESIGN.md §36.10 added. --- docs/DESIGN.md | 10 ++++++++++ shellpilot/llm/ollama.py | 1 + shellpilot/web/fetch.py | 1 + shellpilot/web/search.py | 1 + tests/test_ollama_client.py | 10 ++++++++++ tests/test_web_fetch.py | 10 ++++++++++ tests/test_web_search.py | 10 ++++++++++ 7 files changed, 43 insertions(+) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index de55ce7..9b19bb5 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -3024,3 +3024,13 @@ The opt-in cloud feature (section 15.2) rests on four enforced controls, summari ### 36.9 Approval-Path Display Integrity An approval is only meaningful if the path it shows is the path that will be acted on. The audit found the user-facing path display was the **raw model argument**: the tool-call line and approval-panel head rendered `arguments["path"]` verbatim (`executor._display_for`, `show_tool_call`), and the diff-panel title showed only the resolved *basename* (`unified_diff` used `path.name`). A model could therefore pass an argument that *displays* as one in-workspace file while `resolve_in_workspace` *acts on* another (`..` segments, `./x/../y`, symlink, trailing junk) — the user approves a write under a misleading path. The boundary itself was never weak (the action was always resolved and bounded); the gap was cosmetic, within-workspace, and it is exactly the trust an approval rests on. Closed by the **display-integrity invariant** (section 14.5): all three displays now derive from the single `workspace_display` helper, which is layered over the canonical `resolve_in_workspace` — so the displayed path is always the resolved, workspace-relative target the action uses, and a boundary-escaping path shows an honest `` marker, never a fabricated-looking path. Display-only change: the resolution and boundary checks are untouched. (`run_command` shows its literal `argv` — not a resolution case — and is unchanged.) + +### 36.10 Ambient Proxy Isolation (`trust_env=False`) + +All three `httpx.Client` instances — `OllamaClient` (`llm/ollama.py`), `PageFetcher` (`web/fetch.py`), and `DuckDuckGoProvider` (`web/search.py`) — are constructed with `trust_env=False`. This prevents httpx from reading `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, `NO_PROXY`, or `~/.netrc` from the ambient environment. + +Without this flag, a parent process, shell profile, or system administrator that sets an ambient proxy env var could silently reroute any of these clients — including the **loopback Ollama client**, whose `base_url` is otherwise hardcoded to `http://localhost:11434` precisely so that a malicious environment cannot redirect where prompts are sent (the F7 invariant noted in `ollama.py`). An ambient proxy would also corrupt the egress audit record: the `model_request` audit event would record the destination as "local" while bytes actually left the device through the proxy, making the locality signal untrustworthy. For the web clients, the v0.10.0 hostname SSRF guard (`§36.6`) validates the destination before dialling; a proxy performs its own DNS resolution, so the locally validated IP is never the one actually contacted. + +`trust_env=False` restores all three invariants cleanly: the loopback Ollama endpoint cannot be redirected, the egress audit destination is always truthful, and the SSRF guard is not bypassed. The fix is three one-line additions with no other behaviour change. + +**Known tradeoff — corporate mandatory proxy:** a user whose only path to the public internet is a mandatory corporate HTTP proxy will find `web_search` and `web_fetch` unable to connect, because `trust_env=False` means those clients do not pick up the proxy from the environment. Web tools are opt-in and off by default (`[tools] web = false`); only users who explicitly enable them are affected. An explicit `[tools] http_proxy` config knob — passed as the `proxy=` argument to the affected web clients — is a possible future follow-up but is not implemented here. Ollama runs on loopback and needs no proxy; the tradeoff only affects the two web clients. diff --git a/shellpilot/llm/ollama.py b/shellpilot/llm/ollama.py index d78f7ed..1263abd 100644 --- a/shellpilot/llm/ollama.py +++ b/shellpilot/llm/ollama.py @@ -91,6 +91,7 @@ def __init__( base_url=base_url or resolve_base_url(), timeout=httpx.Timeout(timeout_seconds, read=generate_timeout_seconds), transport=transport, + trust_env=False, ) self._reasoning = reasoning # Per-model fallback cache: models that rejected the think flag. diff --git a/shellpilot/web/fetch.py b/shellpilot/web/fetch.py index 28438cc..c5a02a7 100644 --- a/shellpilot/web/fetch.py +++ b/shellpilot/web/fetch.py @@ -182,6 +182,7 @@ def __init__( timeout=httpx.Timeout(timeout_seconds, read=read_timeout_seconds), follow_redirects=False, transport=transport, + trust_env=False, ) self._max_bytes = max_bytes self._max_chars = max_chars diff --git a/shellpilot/web/search.py b/shellpilot/web/search.py index 0099cb0..37eb4ea 100644 --- a/shellpilot/web/search.py +++ b/shellpilot/web/search.py @@ -193,6 +193,7 @@ def __init__( timeout=httpx.Timeout(timeout_seconds), headers={"User-Agent": _USER_AGENT}, transport=transport, + trust_env=False, ) def search(self, query: str, *, max_results: int = 5) -> list[SearchResult]: diff --git a/tests/test_ollama_client.py b/tests/test_ollama_client.py index 64729b1..c9c434a 100644 --- a/tests/test_ollama_client.py +++ b/tests/test_ollama_client.py @@ -120,3 +120,13 @@ def raise_connect(request: httpx.Request) -> httpx.Response: client = make_client(httpx.MockTransport(raise_connect)) with pytest.raises(OllamaUnreachableError): client.preload("gemma4:e4b") + + +def test_client_ignores_ambient_proxy_env() -> None: + """The httpx client must not honour ambient proxy env vars. + + Loopback Ollama traffic cannot be redirected by HTTP_PROXY/HTTPS_PROXY/ALL_PROXY + in the environment — trust_env=False enforces this invariant (F7 / §36.3). + """ + client = make_client(httpx.MockTransport(lambda r: httpx.Response(200, json={}))) + assert client._client.trust_env is False diff --git a/tests/test_web_fetch.py b/tests/test_web_fetch.py index 64ccf43..03624b0 100644 --- a/tests/test_web_fetch.py +++ b/tests/test_web_fetch.py @@ -605,3 +605,13 @@ def selective_getaddrinfo( assert len(calls) == 1, ( f"Expected exactly 1 transport call, got {len(calls)}: {[str(r.url) for r in calls]}" ) + + +def test_page_fetcher_ignores_ambient_proxy_env() -> None: + """PageFetcher's httpx client must not honour ambient proxy env vars. + + Web fetch traffic cannot be silently redirected through an ambient proxy — + trust_env=False keeps the egress audit's destination truthful (§36.3). + """ + fetcher = PageFetcher(transport=_html_transport("ok")) + assert fetcher._client.trust_env is False diff --git a/tests/test_web_search.py b/tests/test_web_search.py index 4176d0b..3f66f3b 100644 --- a/tests/test_web_search.py +++ b/tests/test_web_search.py @@ -136,3 +136,13 @@ def handler(request: httpx.Request) -> httpx.Response: ) assert req.url.params.get("q") == "shellpilot web" + + +def test_duckduckgo_provider_ignores_ambient_proxy_env() -> None: + """DuckDuckGoProvider's httpx client must not honour ambient proxy env vars. + + Web search traffic cannot be silently redirected through an ambient proxy — + trust_env=False keeps the egress audit's destination truthful (§36.3). + """ + provider = DuckDuckGoProvider(transport=_make_transport(_DDG_EMPTY_HTML)) + assert provider._client.trust_env is False From 131c68fa06a0f5ccf86cd9d8966ac56924bd2ce5 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 06:22:42 -0400 Subject: [PATCH 04/27] fix(security): redact key-named and JSON-shaped secrets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit redact_structure walked dict values but ignored keys, so a secret stored under a sensitive key whose value matched no pattern (e.g. {"api_key": "1234567890"}) persisted verbatim into session JSONL, audit events, /export, and the cloud-egress copy. The value-pattern regex also missed the quoted-JSON-key form ("api_key": "...") because [=:] could not match the key's closing quote. - Redact string and numeric (int/float, excluding bool) scalar values under a sensitive dict key (password, token, api_key, access_token, aws_secret_access_key, ... matched case-insensitively with -/_ normalised); containers still recurse; bool/None are left intact. - Make the value-pattern separator tolerate a quote so quoted-JSON keys match. Redaction stays best-effort: image/base64 and novel formats are still missed, and a benign field literally named token/secret reads [REDACTED] in persisted copies (over-masking beats leaking; in-memory history is never mutated). DESIGN.md §15.1 and §36 updated. --- docs/DESIGN.md | 4 +-- shellpilot/memory/redaction.py | 50 ++++++++++++++++++++++++++--- tests/test_redaction.py | 58 +++++++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 9b19bb5..7f017ba 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1307,7 +1307,7 @@ Reads of these are gated deterministically, never by model judgement. The `read_ When the model endpoint is **remote**, the entire prompt (system prompt, AGENTS.md, memory, file contents, command output) leaves the device — the prompt itself is an exfiltration channel that no per-action approval gate intercepts. The runtime owns a single locality signal (`_is_egressing()`, which delegates to the shared `is_egressing(model, base_url)` predicate in `config/model.py` — true when the model `base_url` is not loopback **or** the model is a cloud model — `is_cloud_model(name)`, the Ollama cloud tag (`:cloud`, or a sized `-cloud` such as `:31b-cloud`, matched on the tag after the final `:`), which egresses to the provider even through a localhost Ollama proxy) and applies two controls at the one chokepoint where every **conversational** model request passes (`conversation.py` tool loop): -- **Outbound redaction (best-effort defence-in-depth, not a guarantee).** On an egressing turn, when `privacy.redact_secrets` is on, a **redacted copy** of the outbound messages is sent (`redact_secrets` on content, `redact_structure` on tool-call arguments) — the in-memory history is never mutated. A **loopback turn is sent byte-identical** (no copy, zero behaviour change). This is regex-based and conservative: **novel secret formats are missed**, and **image/base64 data is not redactable here and egresses unredacted**. It reduces accidental credential leakage to a provider; it is not a confidentiality guarantee. Local-first remains the only full privacy posture. +- **Outbound redaction (best-effort defence-in-depth, not a guarantee).** On an egressing turn, when `privacy.redact_secrets` is on, a **redacted copy** of the outbound messages is sent (`redact_secrets` on content, `redact_structure` on tool-call arguments) — the in-memory history is never mutated. A **loopback turn is sent byte-identical** (no copy, zero behaviour change). The value patterns mask common credential shapes (AWS/GitHub/Slack tokens, JWTs, bearer headers, PEM blocks, `key=value` assignments); in addition `redact_structure` masks **string and numeric (int/float, excluding `bool`) scalar values** under a **sensitive dict key** (`password`/`passwd`/`secret`/`token`/`api_key`/`apikey`/`access_key`/`accesskey`/`client_secret`/`private_key`/`access_token`/`refresh_token`/`auth_token`/`session_token`/`aws_secret_access_key`/`bearer_token`, compared case-insensitively with `-`→`_`), and the value-pattern's separator now tolerates a quote so **quoted-JSON-key forms** (`"api_key": "…"`) match. Container values (list/dict) under a sensitive key are still walked, not masked wholesale; `bool`/`None` scalars are deliberately left intact (`bool` is a Python subclass of `int`). This is a deliberate **over-mask**: a benign field literally *named* `token`/`secret` will read `[REDACTED]` in the persisted/egress copy — over-masking is preferred to leaking, and the in-memory history is never mutated. This remains regex-based and best-effort: **novel secret formats are still missed**, and **image/base64 data is not redactable here and egresses unredacted**. It reduces accidental credential leakage to a provider; it is not a confidentiality guarantee. Local-first remains the only full privacy posture. - **Egress visibility (audit).** Every egressing model request emits a `model_request` audit event (host/model/counts only — never message bodies; section 22), and every `SideEffect.NETWORK` tool call that actually runs emits a `web_egress` event. Together these record *what left the device* without recording its contents. **Accepted residual — `/memory compact`.** One model call bypasses this chokepoint: the `/memory compact` preference optimization (`SlashDispatcher._memory_compact`, `slash.py`) calls the Ollama client directly, outside the `conversation.py` tool loop, so on an egressing session it carries no `model_request` audit event and no outbound-redaction pass. This is an **accepted residual**, not a consent failure: per-session consent (§15.2) is granted before any model call, so this call is already covered by the boundary; and what it sends is stored preferences only (short behaviour strings — not history, file contents, or command output), which §16.4 already requires must not contain secrets. The open gap is audit-completeness (a silent egress the trail does not count) and the missing best-effort redaction pass. Routing `_memory_compact` through the chokepoint to close the audit-undercount gap is a planned hardening follow-up. (Context compaction — `/compact`, §20.2 — is unaffected: it makes no model call, only in-memory message dropping.) @@ -3006,7 +3006,7 @@ A project `AGENTS.md` is injected as standing instructions with the same authori A single locality predicate, `is_egressing(model, base_url)` (`config/model.py`), is the one source of truth for whether a session is off-box — true for a cloud model (the Ollama cloud tag — `:cloud` or a sized `-cloud` — via `is_cloud_model`) or a non-loopback `base_url` (`is_loopback_url`, `llm/ollama.py`). At the conversational chokepoint (`conversation.py` tool loop; the `/memory compact` residual is noted in section 15.1): -- **Best-effort outbound redaction** runs on egressing turns when `privacy.redact_secrets` is on — a **redacted copy** of the outbound messages is sent (the in-memory history is never mutated); a loopback turn is sent byte-identical. It is regex-based defence-in-depth (misses novel formats, cannot redact image/base64), not a confidentiality guarantee. +- **Best-effort outbound redaction** runs on egressing turns when `privacy.redact_secrets` is on — a **redacted copy** of the outbound messages is sent (the in-memory history is never mutated); a loopback turn is sent byte-identical. It is regex-based defence-in-depth covering common credential shapes plus sensitive dict keys and quoted-JSON-key forms (§15.1); it still misses novel formats and cannot redact image/base64, so it is not a confidentiality guarantee. - **Egress-visibility audit** emits a `model_request` event per egressing turn (host/model/counts only — never message bodies) and a `web_egress` event before each `SideEffect.NETWORK` tool call (section 22), recording *what left the device* without its contents. ### 36.6 DNS-Rebinding SSRF Defense in `web_fetch` diff --git a/shellpilot/memory/redaction.py b/shellpilot/memory/redaction.py index d5cd99e..c68be21 100644 --- a/shellpilot/memory/redaction.py +++ b/shellpilot/memory/redaction.py @@ -11,6 +11,40 @@ REDACTED = "[REDACTED]" +# Dict keys whose scalar values are secrets regardless of value shape. Compared +# case-insensitively with "-" normalised to "_" (so "API-Key" matches "api_key"). +_SENSITIVE_KEYS = frozenset( + { + "password", + "passwd", + "secret", + "token", + "api_key", + "apikey", + "access_key", + "accesskey", + "client_secret", + "private_key", + "access_token", + "refresh_token", + "auth_token", + "session_token", + "aws_secret_access_key", + "bearer_token", + } +) + + +def _is_sensitive_key(key: str) -> bool: + return key.lower().replace("-", "_") in _SENSITIVE_KEYS + + +def _is_redactable_scalar(item: object) -> bool: + # Mask string and numeric scalars under a sensitive key. bool is a subclass + # of int, so True/False (and None) are deliberately left intact. + return isinstance(item, str) or (isinstance(item, (int, float)) and not isinstance(item, bool)) + + _PATTERNS: list[re.Pattern[str]] = [ # AWS access key ids and secret-looking assignments re.compile(r"\bAKIA[0-9A-Z]{16}\b"), @@ -23,10 +57,11 @@ re.compile(r"\beyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{5,}\b"), # Bearer headers re.compile(r"(?i)\bbearer\s+[A-Za-z0-9._~+/=-]{16,}"), - # key=value style credentials (allow prefixes like DB_PASSWORD, MY_API_KEY) + # key=value style credentials (allow prefixes like DB_PASSWORD, MY_API_KEY). + # The optional quote before the separator also matches JSON keys ("api_key":). re.compile( r"(?i)(password|passwd|secret|token|api[_-]?key|access[_-]?key)" - r"\s*[=:]\s*['\"]?[^\s'\"]{6,}['\"]?" + r"['\"]?\s*[=:]\s*['\"]?[^\s'\"]{6,}['\"]?" ), # PEM private key blocks re.compile( @@ -46,13 +81,20 @@ def redact_secrets(text: str) -> str: def redact_structure(value: object) -> object: """Recursively redact secrets from arbitrarily nested str/dict/list values. - Non-string scalars (int, float, bool, None, …) pass through unchanged. + String and numeric scalars (int/float, excluding bool) under a sensitive + dict key are masked outright; everything else recurses, so non-string + scalars (bool, None, …) outside a sensitive key pass through unchanged. Callers decide whether redaction is enabled; this function always redacts. """ if isinstance(value, str): return redact_secrets(value) if isinstance(value, dict): - return {key: redact_structure(item) for key, item in value.items()} + return { + key: REDACTED + if isinstance(key, str) and _is_sensitive_key(key) and _is_redactable_scalar(item) + else redact_structure(item) + for key, item in value.items() + } if isinstance(value, list): return [redact_structure(item) for item in value] return value diff --git a/tests/test_redaction.py b/tests/test_redaction.py index 4d4ed66..622f08f 100644 --- a/tests/test_redaction.py +++ b/tests/test_redaction.py @@ -1,6 +1,6 @@ """Tests for secret redaction (design section 15).""" -from shellpilot.memory.redaction import REDACTED, redact_secrets +from shellpilot.memory.redaction import REDACTED, redact_secrets, redact_structure def test_aws_access_key_redacted() -> None: @@ -42,3 +42,59 @@ def test_pem_block_redacted() -> None: def test_ordinary_text_untouched() -> None: text = "Run pytest -q in the workspace; the token count is 42." assert redact_secrets(text) == text + + +def test_structure_redacts_value_under_sensitive_key() -> None: + # The value alone matches no value-pattern; only the key marks it secret. + assert redact_structure({"api_key": "1234567890"}) == {"api_key": REDACTED} + + +def test_structure_redacts_common_sensitive_keys() -> None: + for key in ("password", "token", "secret"): + assert redact_structure({key: "supersecretvalue"}) == {key: REDACTED} + + +def test_structure_sensitive_key_normalizes_case_and_hyphen() -> None: + assert redact_structure({"API-Key": "1234567890"}) == {"API-Key": REDACTED} + + +def test_json_shaped_secret_in_string_redacted() -> None: + assert REDACTED in redact_secrets('"api_key": "1234567890"') + + +def test_structure_benign_dict_unchanged() -> None: + benign = {"name": "alice", "count": 5} + assert redact_structure(benign) == benign + + +def test_structure_container_under_sensitive_key_recurses() -> None: + # Containers under a sensitive key are walked, not force-masked. + assert redact_structure({"token": ["a", "b"]}) == {"token": ["a", "b"]} + assert redact_structure({"secret": {"inner": "x"}}) == {"secret": {"inner": "x"}} + + +def test_structure_numeric_scalar_under_sensitive_key_redacted() -> None: + assert redact_structure({"api_key": 1234567890}) == {"api_key": REDACTED} + assert redact_structure({"api_key": 1234567890.5}) == {"api_key": REDACTED} + + +def test_structure_bool_and_none_under_sensitive_key_unchanged() -> None: + # bool is a subclass of int; True/False and None must never be masked. + assert redact_structure({"secret": True}) == {"secret": True} + assert redact_structure({"token": None}) == {"token": None} + assert redact_structure({"flag": True}) == {"flag": True} + assert redact_structure({"x": None}) == {"x": None} + + +def test_structure_redacts_expanded_token_keys() -> None: + assert redact_structure({"access_token": "ya29.xxxxxxxxxx"}) == {"access_token": REDACTED} + + +def test_structure_nested_sensitive_key_redacted() -> None: + nested = {"config": {"password": "hunter2pass"}} + assert redact_structure(nested) == {"config": {"password": REDACTED}} + + +def test_benign_prose_not_redacted() -> None: + assert redact_secrets("the api docs are here") == "the api docs are here" + assert redact_secrets("time: now") == "time: now" From 1aa3212e1a12c5a40fe875da3c6a172f160b932f Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 06:35:17 -0400 Subject: [PATCH 05/27] fix(planner): finalize on skipped final step; pin artifact path to plan workspace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness bugs in the plan state machine: - update_step only finalized the plan inside the `status == "completed"` branch, so a skipped final step left the plan `active` forever -- it kept injecting, the end-of-plan summary never fired, the session pointer was never cleared, and --resume rehydrated an uncompleteable plan. The finalize check (all steps completed or skipped) now runs after any status change; only a completion still advances the next pending step. - artifact_path derived from the mutable _workspace, so a /cwd during a live plan relocated PLAN.md (orphaning the original) and made /plan path point at a non-existent file. It now derives from the plan's pinned workspace, matching render_plan_markdown; _workspace governs only new-task creation. DESIGN.md §11.4 note added. --- docs/DESIGN.md | 4 ++ shellpilot/runtime/planner.py | 14 ++++- tests/test_planner.py | 112 ++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 3 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7f017ba..d3dde09 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -788,6 +788,10 @@ Recommended workflow: next step in the same turn. The final completion result instructs the model to summarize the outcome instead. +A plan finalizes to `completed` once every step is `completed` or `skipped`. This is +re-checked after **any** step status change, so a skipped final step finishes the plan just +as a completed one does; only a *completion* advances the next pending step to active. + Completing a step is deterministically guarded: if the step's last side-effecting action failed or was denied and nothing has succeeded since, `update_plan(completed)` returns a corrective failure telling the model to apply the change successfully or record a blocker diff --git a/shellpilot/runtime/planner.py b/shellpilot/runtime/planner.py index 0c0e291..3055519 100644 --- a/shellpilot/runtime/planner.py +++ b/shellpilot/runtime/planner.py @@ -200,7 +200,10 @@ def set_workspace(self, workspace: Path) -> None: self._workspace = workspace def artifact_path(self, plan: TaskPlan) -> Path: - return project_state_dir(self._workspace) / "tasks" / plan.task_id / "PLAN.md" + # 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 + # it was written instead of orphaning it under the new boundary (§11.3). + return project_state_dir(plan.workspace) / "tasks" / plan.task_id / "PLAN.md" def _write(self, plan: TaskPlan) -> None: plan.updated = _now_iso() @@ -322,8 +325,13 @@ def update_step(self, index: int, status: str, note: str = "") -> str: nxt = next((s for s in self.active.steps if s.status == "pending"), None) if nxt is not None: nxt.status = "active" - elif all(s.status in ("completed", "skipped") for s in self.active.steps): - self.active.status = "completed" + # Finalize after ANY status change: a skipped final step must complete the + # plan too, otherwise it stays active forever (keeps injecting, the + # end-of-plan summary never fires, the session pointer is never cleaned). + if self.active.steps and all( + s.status in ("completed", "skipped") for s in self.active.steps + ): + self.active.status = "completed" self._write(self.active) return "" diff --git a/tests/test_planner.py b/tests/test_planner.py index 0b55574..ab7224a 100644 --- a/tests/test_planner.py +++ b/tests/test_planner.py @@ -75,6 +75,118 @@ def test_step_completion_advances_and_finishes(tmp_path: Path) -> None: assert "- [x] Inspect current code" in text +def test_skipped_final_step_completes_plan(tmp_path: Path) -> None: + """A skipped final step must finalize the plan, not leave it active forever.""" + manager = PlanManager(tmp_path, "balanced") + manager.create( + goal="Two-step task", + user_intent="u", + steps=["Do the work", "Optional cleanup"], + assumptions=[], + verification=[], + ) + manager.approve() + assert manager.active is not None + + assert manager.update_step(1, "completed") == "" + assert manager.update_step(2, "skipped") == "" + + assert manager.active.status == "completed" + + +def test_middle_skipped_last_completed_completes_plan(tmp_path: Path) -> None: + """Skipping a middle step then completing the last finalizes the plan.""" + manager = make_plan(tmp_path) + manager.approve() + assert manager.active is not None + + manager.update_step(1, "completed") + manager.update_step(2, "skipped") + assert manager.active.status == "active" # step 3 still pending + manager.update_step(3, "completed") + + assert manager.active.status == "completed" + + +def test_all_completed_plan_still_completes(tmp_path: Path) -> None: + """No-regression: a normal all-completed plan still finishes.""" + manager = make_plan(tmp_path) + manager.approve() + assert manager.active is not None + + manager.update_step(1, "completed") + manager.update_step(2, "completed") + manager.update_step(3, "completed") + + assert manager.active.status == "completed" + + +def test_completion_promotes_next_pending_step(tmp_path: Path) -> None: + """No-regression: completing a step still promotes the first pending step to active.""" + manager = make_plan(tmp_path) + manager.approve() + assert manager.active is not None + + manager.update_step(1, "completed") + assert manager.active.steps[1].status == "active" + assert manager.active.status == "active" + + +def test_skip_does_not_promote_or_complete_with_pending_left(tmp_path: Path) -> None: + """No-regression: a skip neither advances the next step nor completes a plan + that still has a pending step.""" + manager = make_plan(tmp_path) + manager.approve() + assert manager.active is not None + + # Skip the active first step; steps 2 and 3 remain pending. + manager.update_step(1, "skipped") + + # Skip alone does not promote the next pending step (only completion does). + assert manager.active.steps[1].status == "pending" + # A pending step remains, so the plan is NOT marked completed. + assert manager.active.status == "active" + + +def test_artifact_path_pinned_to_plan_workspace(tmp_path: Path) -> None: + """An active plan keeps its artifact path even after the workspace boundary moves.""" + workspace_a = tmp_path / "ws_a" + workspace_a.mkdir() + workspace_b = tmp_path / "ws_b" + workspace_b.mkdir() + + manager = PlanManager(workspace_a, "balanced") + plan = manager.create( + goal="g", user_intent="u", steps=["A", "B"], assumptions=[], verification=[] + ) + before = manager.artifact_path(plan) + + manager.set_workspace(workspace_b) + + # The active plan's artifact path is pinned to its creation workspace. + assert manager.artifact_path(plan) == before + assert before.exists() + + +def test_new_task_after_set_workspace_uses_new_boundary(tmp_path: Path) -> None: + """set_workspace governs NEW tasks: a plan created after the move lands under ws_b.""" + workspace_a = tmp_path / "ws_a" + workspace_a.mkdir() + workspace_b = tmp_path / "ws_b" + workspace_b.mkdir() + + manager = PlanManager(workspace_a, "balanced") + manager.set_workspace(workspace_b) + plan_b = manager.create( + goal="g2", user_intent="u", steps=["X"], assumptions=[], verification=[] + ) + + path_b = manager.artifact_path(plan_b) + assert str(workspace_b) in str(path_b) + assert str(workspace_a) not in str(path_b) + assert path_b.exists() + + def test_invalid_step_index_reports(tmp_path: Path) -> None: manager = make_plan(tmp_path) error = manager.update_step(9, "completed") From 87bfbc661ae4c6be980a93db6a4c8fa4936920b3 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 06:47:36 -0400 Subject: [PATCH 06/27] fix(security): reject truncated Ollama streams; guard session-id read traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _stream_chat accepted a stream that ended cleanly without a final done:true chunk (an OOM-killed/cancelled runner, or a buffering proxy truncating on a clean boundary) as a complete reply. It now tracks the done sentinel and raises OllamaResponseError on an incomplete stream; legit early stops (done_reason length/load/stop) and tool-call-only replies all carry done, so they pass. The error surfaces at the REPL as a model error, not a crash. - SessionStore.find built its path from a raw --resume id, so --resume ../../etc/cron.d/evil could load any on-disk .jsonl into history. It now returns None when the candidate escapes the sessions directory (the save path was already basename-contained via path.stem). DESIGN.md §24.6 and §25.2 updated. --- docs/DESIGN.md | 3 +- shellpilot/llm/ollama.py | 5 +++ shellpilot/persistence/sessions.py | 2 ++ tests/test_ollama_client.py | 56 ++++++++++++++++++++++++++++++ tests/test_sessions.py | 35 +++++++++++++++++++ 5 files changed, 100 insertions(+), 1 deletion(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d3dde09..ed1e569 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -2401,6 +2401,7 @@ Most rows here concern the v2 memory system. The prompt-injection and secret row | Model loops tool calls | Enforce turn/tool budgets and replan or stop. | | Reasoning mode unavailable | Per-model fallback: add the model to `_no_think`; retry once without `think`; other models keep sending `think`. `_reasoning` (the config-level flag) is never mutated. | | Reasoning-only turn (think text, empty content) | The streamed `thinking` field is now accumulated and captured on the reply message (it is never echoed back to the API and never rendered), so a turn that reasons and then emits nothing is observable in the audit log rather than silently empty; the runtime nudges such a reply once the model has already run a tool (section 10.4). | +| Stream ends without `done` sentinel | Rejected as incomplete: `_stream_chat` raises `OllamaResponseError` after the `with` block closes without seeing a chunk whose `done` field is truthy. Legitimate early stops (`done_reason = "length"` or `"load"`) still carry the sentinel and are accepted normally. | ### 24.7 Privacy And Log Edge Cases @@ -2437,7 +2438,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). | +| 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`. | | 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/llm/ollama.py b/shellpilot/llm/ollama.py index 1263abd..ac56d2b 100644 --- a/shellpilot/llm/ollama.py +++ b/shellpilot/llm/ollama.py @@ -220,6 +220,7 @@ def _stream_chat( content_parts: list[str] = [] thinking_parts: list[str] = [] tool_calls: list[ToolCall] = [] + done_seen = False try: with self._client.stream("POST", "/api/chat", json=payload) as response: if response.status_code >= 400: @@ -234,6 +235,8 @@ def _stream_chat( raise OllamaResponseError(f"invalid stream chunk: {line[:200]}") from exc if chunk.get("error"): raise OllamaResponseError(str(chunk["error"])) + if chunk.get("done"): + done_seen = True message = chunk.get("message") or {} token = message.get("content") or "" if token: @@ -250,6 +253,8 @@ def _stream_chat( parsed = _decode_tool_call(raw_call) if parsed is not None: tool_calls.append(parsed) + if not done_seen: + raise OllamaResponseError("stream ended before completion (no done sentinel)") except httpx.TransportError as exc: raise OllamaUnreachableError(f"Ollama API unreachable: {exc}") from exc return Message( diff --git a/shellpilot/persistence/sessions.py b/shellpilot/persistence/sessions.py index ae3ca98..4d16e7f 100644 --- a/shellpilot/persistence/sessions.py +++ b/shellpilot/persistence/sessions.py @@ -61,6 +61,8 @@ def latest(directory: Path) -> Path | None: @staticmethod def find(directory: Path, session_id: str) -> Path | None: candidate = directory / f"{session_id}.jsonl" + if candidate.parent.resolve() != directory.resolve(): + return None return candidate if candidate.is_file() else None @staticmethod diff --git a/tests/test_ollama_client.py b/tests/test_ollama_client.py index c9c434a..54866da 100644 --- a/tests/test_ollama_client.py +++ b/tests/test_ollama_client.py @@ -1,14 +1,17 @@ """Tests for the Ollama HTTP client (no live Ollama; httpx.MockTransport only).""" import json +from typing import Any import httpx import pytest +from shellpilot.llm.messages import Message from shellpilot.llm.ollama import ( DEFAULT_BASE_URL, LocalModel, OllamaClient, + OllamaResponseError, OllamaUnreachableError, resolve_base_url, ) @@ -130,3 +133,56 @@ def test_client_ignores_ambient_proxy_env() -> None: """ client = make_client(httpx.MockTransport(lambda r: httpx.Response(200, json={}))) assert client._client.trust_env is False + + +# --------------------------------------------------------------------------- +# Fix #4: truncated stream (no done sentinel) is rejected +# --------------------------------------------------------------------------- + + +def _streaming_transport(chunks: list[dict[str, Any]]) -> httpx.MockTransport: + """MockTransport that responds to /api/chat with the given NDJSON lines.""" + body = "\n".join(json.dumps(c) for c in chunks) + "\n" + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/api/chat": + return httpx.Response(200, content=body.encode()) + return httpx.Response(404) + + return httpx.MockTransport(handler) + + +def test_truncated_stream_raises_response_error() -> None: + """A stream that closes without a done:true chunk raises OllamaResponseError.""" + chunks: list[dict[str, Any]] = [ + {"message": {"role": "assistant", "content": "hello"}, "done": False}, + {"message": {"role": "assistant", "content": " world"}, "done": False}, + # NOTE: no {"done": true} chunk — simulates a truncated/OOM-killed stream + ] + client = make_client(_streaming_transport(chunks)) + with pytest.raises(OllamaResponseError, match="done"): + client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + + +def test_complete_stream_returns_assembled_message() -> None: + """A stream ending with done:true is accepted and returns the full message.""" + chunks: list[dict[str, Any]] = [ + {"message": {"role": "assistant", "content": "hello"}, "done": False}, + {"message": {"role": "assistant", "content": " world"}, "done": False}, + {"message": {"role": "assistant", "content": ""}, "done": True, "done_reason": "stop"}, + ] + client = make_client(_streaming_transport(chunks)) + msg = client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + assert msg.content == "hello world" + assert msg.role == "assistant" + + +def test_done_reason_length_stream_is_accepted() -> None: + """A stream truncated by context length (done_reason='length') still carries done:true.""" + chunks: list[dict[str, Any]] = [ + {"message": {"role": "assistant", "content": "partial answer"}, "done": False}, + {"message": {"role": "assistant", "content": ""}, "done": True, "done_reason": "length"}, + ] + client = make_client(_streaming_transport(chunks)) + msg = client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + assert msg.content == "partial answer" diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 79bc61f..6cec95d 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -508,3 +508,38 @@ def test_recent_newest_first_and_capped(tmp_path: Path) -> None: os.utime(s.path, (1000.0 + i, 1000.0 + i)) recent = SessionStore.recent(sessions, limit=3) assert [label for label, _ in recent] == ["msg 4", "msg 3", "msg 2"] + + +# --------------------------------------------------------------------------- +# Fix #6: SessionStore.find read-side path traversal guard +# --------------------------------------------------------------------------- + + +def test_find_rejects_traversal_id(tmp_path: Path) -> None: + """find() returns None for a session_id that escapes the sessions directory via '..'.""" + sessions_dir = tmp_path / "sessions" + sessions_dir.mkdir(parents=True) + # Plant a sibling .jsonl one level up that would be reachable via traversal. + sibling = tmp_path / "passwd.jsonl" + sibling.write_text('{"type":"meta","model":"evil"}\n', encoding="utf-8") + + assert SessionStore.find(sessions_dir, "../passwd") is None + + +def test_find_rejects_absolute_id(tmp_path: Path) -> None: + """find() returns None for an absolute-path session_id (pathlib replaces the base).""" + sessions_dir = tmp_path / "sessions" + sessions_dir.mkdir(parents=True) + target = tmp_path / "target.jsonl" + target.write_text('{"type":"meta"}\n', encoding="utf-8") + + # str(target.with_suffix("")) is an absolute path; / with it replaces sessions_dir. + assert SessionStore.find(sessions_dir, str(target.with_suffix(""))) is None + + +def test_find_normal_id_still_works(tmp_path: Path) -> None: + """find() still returns the path for a valid session id inside the sessions dir.""" + store = make_store(tmp_path) + store.record_message(Message(role="user", content="hello")) + found = SessionStore.find(store.path.parent, "20260611-100000-ab12") + assert found is not None and found == store.path From 0fb2fafd0851614d9a2f3667d59e0f179cfed26c Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 10:23:02 -0400 Subject: [PATCH 07/27] fix(cli): guard slash/manual-shell turns and validate --resume before preload - The `!` one-shot, the slash dispatch, and the manual-shell command loop ran outside the normal-turn try/except, so Ctrl+C (or a model error) during them escaped run_interactive and crashed the REPL, skipping the session_end audit. Each now catches KeyboardInterrupt/OllamaError and continues; in the manual shell, Ctrl+C aborts the running command rather than the whole loop. - A bad --resume was only discovered after the cloud-consent prompt and the model preload + context probe. The session existence check (pure filesystem, no egress) is hoisted above the consent gate, so a typo'd or stale --resume returns 1 immediately with no consent prompt and no preload. Consent still precedes the first egress -- the invariant is intact. --- shellpilot/cli/manual_shell.py | 7 +- shellpilot/cli/terminal.py | 90 +++++++++++-------- tests/test_phase5_flows.py | 26 ++++++ tests/test_terminal_ui.py | 154 +++++++++++++++++++++++++++++++++ 4 files changed, 239 insertions(+), 38 deletions(-) diff --git a/shellpilot/cli/manual_shell.py b/shellpilot/cli/manual_shell.py index 57b6bad..bc06c62 100644 --- a/shellpilot/cli/manual_shell.py +++ b/shellpilot/cli/manual_shell.py @@ -60,7 +60,12 @@ def manual_shell_loop( continue if line == EXIT_COMMAND: break - exit_code = run_manual_command(line, cwd, audit) + try: + exit_code = run_manual_command(line, cwd, audit) + except KeyboardInterrupt: + # Ctrl+C aborts the running command, not the shell loop. + console.print("[yellow]Interrupted.[/yellow]") + continue if exit_code != 0: console.print(f"[dim]exit code {exit_code}[/dim]") if audit is not None: diff --git a/shellpilot/cli/terminal.py b/shellpilot/cli/terminal.py index 24f534d..9e19b3a 100644 --- a/shellpilot/cli/terminal.py +++ b/shellpilot/cli/terminal.py @@ -472,6 +472,25 @@ def load() -> LoadedConfig: console.print(f"[red]Model {chosen} is not installed.[/red] Try: ollama pull {chosen}") return 1 + # Fail fast on a missing --resume target BEFORE any consent prompt or model + # load: resolving the session file is pure filesystem (no egress), so a + # typo'd or stale --resume must not make the user consent and wait through a + # preload only to then hit "session not found". + sessions_dir = SessionStore.sessions_dir(workspace) + resume_path: Path | None = None + if resume is not None: + resume_path = ( + SessionStore.latest(sessions_dir) + if resume == "latest" + else SessionStore.find(sessions_dir, resume) + ) + if resume_path is None: + console.print( + f"[red]No saved session to resume[/red] " + f"({'none found' if resume == 'latest' else resume}) in {sessions_dir}." + ) + return 1 + # Cloud-egress consent boundary (design section 15.2): a cloud/remote model # must clear allow_cloud + per-session consent BEFORE any prompt-bearing call # touches it. Placed strictly before _preload — the first egress point — so a @@ -537,26 +556,13 @@ def _preload(model_name: str) -> None: host=(urlsplit(settings.model.base_url).hostname or "").rstrip("."), ) - sessions_dir = SessionStore.sessions_dir(workspace) # Capture prior sessions for the banner BEFORE this session's meta is - # written, so the current (empty) session never lists itself. + # written, so the current (empty) session never lists itself. sessions_dir + # and the --resume existence check were resolved earlier (before consent). recent_sessions = [ (label, _relative_age(mtime)) for label, mtime in SessionStore.recent(sessions_dir) ] - restored = None - if resume is not None: - session_path = ( - SessionStore.latest(sessions_dir) - if resume == "latest" - else SessionStore.find(sessions_dir, resume) - ) - if session_path is None: - console.print( - f"[red]No saved session to resume[/red] " - f"({'none found' if resume == 'latest' else resume}) in {sessions_dir}." - ) - return 1 - restored = SessionStore.load(session_path) + restored = SessionStore.load(resume_path) if resume_path is not None else None session = SessionStore( sessions_dir, restored.session_id if restored is not None else SessionStore.new_session_id(), @@ -677,28 +683,38 @@ def _preload(model_name: str) -> None: continue if not line: continue - if line.startswith("!"): - # `!` runs one command through the audited manual-shell path - # (raw shell=True, exactly like /shell); a bare `!` opens the shell - # loop. This is a human-only escape — model output never reaches this - # reader — so it carries the same trust as /shell. Live workspace - # honours a prior /cwd. - workspace = runtime.status().workspace - command = line[1:].strip() - if command: - run_manual_command(command, workspace, audit) - else: - manual_shell_loop(console, workspace, audit) + # The `!` and `/` branches run outside the normal-turn guard below, so + # a Ctrl+C (or model error) here is caught at this level — otherwise it + # escapes run_interactive and crashes the REPL, skipping session_end. + try: + if line.startswith("!"): + # `!` runs one command through the audited manual-shell path + # (raw shell=True, exactly like /shell); a bare `!` opens the + # shell loop. This is a human-only escape — model output never + # reaches this reader — so it carries the same trust as /shell. + # Live workspace honours a prior /cwd. + workspace = runtime.status().workspace + command = line[1:].strip() + if command: + run_manual_command(command, workspace, audit) + else: + manual_shell_loop(console, workspace, audit) + continue + if line.startswith("/"): + action = dispatcher.handle(line) + if action is SlashAction.EXIT: + break + if action is SlashAction.MANUAL_SHELL: + # Fetch the live workspace from the runtime so that a prior + # /cwd set is honoured, rather than using the stale local + # captured at startup. + manual_shell_loop(console, runtime.status().workspace, audit) + continue + except KeyboardInterrupt: + ui.show_status("Interrupted.") continue - if line.startswith("/"): - action = dispatcher.handle(line) - if action is SlashAction.EXIT: - break - if action is SlashAction.MANUAL_SHELL: - # Fetch the live workspace from the runtime so that a prior - # /cwd set is honoured, rather than using the stale local - # captured at startup. - manual_shell_loop(console, runtime.status().workspace, audit) + except OllamaError as exc: + ui.show_error(f"Model call failed: {exc}") continue try: staged_paths = attachments.take() diff --git a/tests/test_phase5_flows.py b/tests/test_phase5_flows.py index 4b8522c..fbe9ccc 100644 --- a/tests/test_phase5_flows.py +++ b/tests/test_phase5_flows.py @@ -3,6 +3,7 @@ import json from pathlib import Path +import pytest from rich.console import Console from shellpilot.cli.manual_shell import BANNER, manual_shell_loop, run_manual_command @@ -131,6 +132,31 @@ def test_manual_shell_loop_banner_and_exit(tmp_path: Path) -> None: assert kinds == ["manual_shell_enter", "manual_shell_command", "manual_shell_exit"] +def test_manual_shell_loop_survives_keyboard_interrupt( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Ctrl+C during a manual command is caught; the loop continues, never propagates.""" + import shellpilot.cli.manual_shell as manual_shell_mod + + console = Console(record=True, width=100) + audit = make_audit(tmp_path) + lines = iter(["sleep 100", "/exit-shell"]) + + def _boom(command: str, cwd: Path, audit: object) -> int: + raise KeyboardInterrupt + + monkeypatch.setattr(manual_shell_mod, "run_manual_command", _boom) + + # Must return normally — the interrupt is caught and the loop moves on to the + # next line — rather than propagating out of manual_shell_loop. + manual_shell_loop(console, tmp_path, audit, read_line=lambda: next(lines)) + + assert "Interrupted" in console.export_text() + # The loop reached /exit-shell and shut down cleanly. + kinds = [e["event"] for e in read_events(tmp_path)] + assert kinds == ["manual_shell_enter", "manual_shell_exit"] + + def test_banner_matches_design_wording() -> None: assert "shell=True" in BANNER assert "/exit-shell" in BANNER diff --git a/tests/test_terminal_ui.py b/tests/test_terminal_ui.py index a66eadf..4e0ff84 100644 --- a/tests/test_terminal_ui.py +++ b/tests/test_terminal_ui.py @@ -3,6 +3,7 @@ from __future__ import annotations import io +import json from collections.abc import Iterator from pathlib import Path @@ -784,3 +785,156 @@ def read(self, context: object) -> str: assert bang_calls == ["echo hi"], "'!' must run via the manual-shell path" assert model_turns == [], "a '!' line must NOT be sent to the model" assert rc == 0 + + +def _boot_fake_paths(tmp_path: Path) -> object: + from shellpilot.persistence.paths import AppPaths + + return AppPaths( + config_dir=tmp_path / "cfg", + data_dir=tmp_path / "data", + state_dir=tmp_path / "state", + cache_dir=tmp_path / "cache", + ) + + +def _make_fake_client(model: str, preload_calls: list[str] | None = None) -> type: + from shellpilot.llm.ollama import LocalModel + + class _FakeClient: + def __init__(self, *args: object, **kwargs: object) -> None: + pass + + def health(self) -> bool: + return True + + def list_models(self) -> list[LocalModel]: + return [LocalModel(name=model, size_bytes=1)] + + def preload(self, name: str, *, keep_alive: str = "5m") -> None: + if preload_calls is not None: + preload_calls.append(name) + + def model_context_length(self, name: str) -> int: + return 8192 + + return _FakeClient + + +def test_slash_branch_survives_keyboard_interrupt( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Ctrl+C during a slash command (e.g. `/plan revise`) is caught in the REPL. + + The interrupt must not escape `run_interactive` — the session ends cleanly + and the `session_end` audit still fires. + """ + import shellpilot.cli.terminal as terminal_mod + + model = "gemma4:e4b" + fake_paths = _boot_fake_paths(tmp_path) + monkeypatch.setattr(terminal_mod, "OllamaClient", lambda *a, **k: _make_fake_client(model)()) + monkeypatch.setattr(terminal_mod.AppPaths, "default", classmethod(lambda cls: fake_paths)) + + class _Reader: + def __init__(self) -> None: + self._lines = iter(["/plan revise x"]) + + def read(self, context: object) -> str: + try: + return next(self._lines) + except StopIteration: + raise EOFError from None + + monkeypatch.setattr(terminal_mod, "make_input", lambda *a, **k: _Reader()) + + def _boom(self: object, line: str) -> object: + raise KeyboardInterrupt + + monkeypatch.setattr(terminal_mod.SlashDispatcher, "handle", _boom) + monkeypatch.setattr(terminal_mod.sys.stdin, "isatty", lambda: True) + monkeypatch.setattr(Console, "is_terminal", property(lambda self: True)) + + # Must return normally, not propagate the KeyboardInterrupt out of the REPL. + rc = terminal_mod.run_interactive(tmp_path, model_override=model) + + assert rc == 0 + events = [ + json.loads(line) for line in (fake_paths.state_dir / "audit.jsonl").read_text().splitlines() + ] + assert any(e["event"] == "session_end" for e in events), "session_end audit must survive" + + +def test_resume_existence_checked_before_preload( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A bad --resume fails fast: no model preload happens before the existence check.""" + import shellpilot.cli.terminal as terminal_mod + + model = "gemma4:e4b" + preload_calls: list[str] = [] + fake_paths = _boot_fake_paths(tmp_path) + monkeypatch.setattr( + terminal_mod, "OllamaClient", lambda *a, **k: _make_fake_client(model, preload_calls)() + ) + monkeypatch.setattr(terminal_mod.AppPaths, "default", classmethod(lambda cls: fake_paths)) + monkeypatch.setattr(terminal_mod.sys.stdin, "isatty", lambda: True) + monkeypatch.setattr(Console, "is_terminal", property(lambda self: True)) + + rc = terminal_mod.run_interactive(tmp_path, resume="nope", model_override=model) + + assert rc == 1 + assert preload_calls == [], "the model must not preload before the resume existence check" + + +def test_resume_existing_session_still_loads( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """The happy path is unchanged: an existing --resume target loads and the model warms.""" + import shellpilot.cli.terminal as terminal_mod + from shellpilot.persistence.sessions import SessionStore + + model = "gemma4:e4b" + preload_calls: list[str] = [] + fake_paths = _boot_fake_paths(tmp_path) + + sessions_dir = SessionStore.sessions_dir(tmp_path) + sessions_dir.mkdir(parents=True) + sid = "20260101-000000-abcd" + (sessions_dir / f"{sid}.jsonl").write_text( + json.dumps( + { + "type": "meta", + "session_id": sid, + "model": model, + "profile": "balanced", + "workspace": str(tmp_path), + } + ) + + "\n" + + json.dumps({"type": "message", "role": "user", "content": "hello"}) + + "\n", + encoding="utf-8", + ) + + monkeypatch.setattr( + terminal_mod, "OllamaClient", lambda *a, **k: _make_fake_client(model, preload_calls)() + ) + monkeypatch.setattr(terminal_mod.AppPaths, "default", classmethod(lambda cls: fake_paths)) + + class _Reader: + def read(self, context: object) -> str: + raise EOFError + + monkeypatch.setattr(terminal_mod, "make_input", lambda *a, **k: _Reader()) + monkeypatch.setattr(terminal_mod.sys.stdin, "isatty", lambda: True) + monkeypatch.setattr(Console, "is_terminal", property(lambda self: True)) + + rc = terminal_mod.run_interactive(tmp_path, resume=sid, model_override=model) + + assert rc == 0 + assert preload_calls == [model], "an existing resume target still warms the model" + events = [ + json.loads(line) for line in (fake_paths.state_dir / "audit.jsonl").read_text().splitlines() + ] + assert any(e["event"] == "session_resume" for e in events), "resume load must still happen" From 25f9909d8afad82c411e9116962ebdc7dd870f59 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 10:47:33 -0400 Subject: [PATCH 08/27] fix(security): hide in-workspace secrets in write/patch previews; rebuild project memory on /cwd - write_file/patch_file previews and result diffs replace an existing in-workspace sensitive file's contents (e.g. a workspace .env) with a "[sensitive file contents hidden]" placeholder when allow_sensitive_reads != "always", so the on-disk secret never renders in the approval preview or returns to the model. Contents-only; write risk and the always-ask side-effect gate are unchanged. - set_workspace rebuilds the path-scoped project MemoryStore for the new workspace, so after /cwd set the prior workspace's facts stop injecting (and stop egressing under cloud); the shared global store is preserved. - the auto-compact-off hard-limit gate counts this turn's incoming image tokens (IMAGE_TOKEN_ESTIMATE per image), not only text and history images. --- docs/DESIGN.md | 7 +++ shellpilot/runtime/conversation.py | 21 +++++++- shellpilot/tools/patch.py | 39 +++++++++++++- tests/test_compaction.py | 39 ++++++++++++++ tests/test_conversation.py | 45 ++++++++++++++++ tests/test_write_tools.py | 86 ++++++++++++++++++++++++++++++ 6 files changed, 233 insertions(+), 4 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index ed1e569..5b6e52e 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -515,6 +515,8 @@ hard_limit_tokens = floor(model_context_tokens * 0.90) `clamp(min, max, value)` means use `value` but never lower than `min` or higher than `max`. +When automatic compaction is off, a turn is refused if it would cross `hard_limit_tokens`. That pre-turn gate counts the estimated prompt (system prompt + history, including history images) plus this turn's incoming text **and** its incoming images (`IMAGE_TOKEN_ESTIMATE` per image), so an image-heavy turn cannot slip past a limit that text-only turns respect. The term is zero for text-only turns, leaving them unchanged. + Note the floor case: at the 8192-token fallback, after the system prompt, tool schemas, and behavior instructions, the working prompt budget is roughly 3-4k tokens. Small-context operation is a first-class mode, not a degraded one: shorter tool results, more aggressive truncation, and no long conversational tails. If the selected model exposes a much larger context window, the runtime can scale up conversation and tool context, but it should still cap noisy data. Larger context should improve continuity, not encourage dumping full command logs into every prompt. @@ -1307,6 +1309,8 @@ Reads of these are gated deterministically, never by model judgement. The `read_ `search_text` applies the same gate to directory traversal: files whose path components name a secret are skipped (their contents are never read) unless `allow_sensitive_reads = "always"`, and the tool result appends a deterministic note naming up to three skipped files and pointing at `read_file` or the `"always"` setting. An explicit sensitive path passed as the search root is gated by the classifier exactly like `read_file`; once that gate authorizes it (auto under `"always"`, on approval under `"ask"`, never under `"never"`), the approved sensitive root is searched in full — the traversal skip applies only to sensitive files encountered incidentally under a non-sensitive root. Listing directory names (`list_dir`) is not a content read and is never gated. +The write tools (`write_file`, `patch_file`, section 12.5) build their preview by reading the existing target, so an in-workspace sensitive file (e.g. a workspace `.env`) on overwrite, append, or patch would otherwise render its existing secret contents in the approval diff — and return them to the model on approval. When the resolved, in-workspace target's path components name a secret and `allow_sensitive_reads != "always"`, the diff is replaced with a `[sensitive file contents hidden]` placeholder at every preview and result site, so the existing on-disk secret never appears pre-approval. Out-of-workspace targets are already rejected by the workspace boundary (section 24.1) and cannot be previewed or written. This hides the contents only; write tools keep their existing risk and always-ask side-effect gate — the HIGH read-classifier is deliberately *not* bolted onto them, since write risk and read secrecy are separate concerns. + ### 15.1 Egress Chokepoint (v0.10.0) When the model endpoint is **remote**, the entire prompt (system prompt, AGENTS.md, memory, file contents, command output) leaves the device — the prompt itself is an exfiltration channel that no per-action approval gate intercepts. The runtime owns a single locality signal (`_is_egressing()`, which delegates to the shared `is_egressing(model, base_url)` predicate in `config/model.py` — true when the model `base_url` is not loopback **or** the model is a cloud model — `is_cloud_model(name)`, the Ollama cloud tag (`:cloud`, or a sized `-cloud` such as `:31b-cloud`, matched on the tag after the final `:`), which egresses to the provider even through a localhost Ollama proxy) and applies two controls at the one chokepoint where every **conversational** model request passes (`conversation.py` tool loop): @@ -1480,6 +1484,8 @@ Example: } ``` +The project store is path-scoped to `workspace/.shellpilot/memory.json` and stamped with `project_id_for(workspace)`. Because the file follows the workspace, a `/cwd set ` (section 14.1) rebuilds the project `MemoryStore` for the new workspace inside the single `set_workspace` chokepoint — so the previous workspace's facts stop injecting into the prompt (and, under cloud, stop egressing). The shared global store is preserved across the change. + ## 17. Configuration Configuration should be layered and explicit. @@ -2390,6 +2396,7 @@ Most rows here concern the v2 memory system. The prompt-injection and secret row | Prompt injection in repo docs | Do not store instructions from project files as behavior memory without user approval. | | Secret-like content | Redact from memory proposals and logs. | | Too many memory proposals | Batch and ask once, or defer low-value suggestions. | +| Workspace change (`/cwd set`) | Rebuild the project memory store for the new path; the prior workspace's facts are never injected (or egressed) after the switch. | ### 24.6 Ollama And Model Edge Cases diff --git a/shellpilot/runtime/conversation.py b/shellpilot/runtime/conversation.py index e721cb3..340293d 100644 --- a/shellpilot/runtime/conversation.py +++ b/shellpilot/runtime/conversation.py @@ -16,8 +16,9 @@ from shellpilot.llm.ollama import encode_tool, is_loopback_url from shellpilot.memory.agents_md import BehaviorInstructions from shellpilot.memory.redaction import redact_secrets, redact_structure -from shellpilot.memory.store import MemoryStores +from shellpilot.memory.store import MemoryStore, MemoryStores, project_id_for from shellpilot.persistence.audit_store import AuditLogger +from shellpilot.persistence.paths import project_state_dir from shellpilot.persistence.sessions import SessionStore from shellpilot.persistence.snapshots import SnapshotStore from shellpilot.policy.risk import SideEffect @@ -263,6 +264,19 @@ def set_model(self, model: str) -> None: def set_workspace(self, workspace: Path) -> None: self._workspace = workspace self.plan_manager.set_workspace(workspace) + if self._memory is not None: + # Project memory is path-scoped (workspace/.shellpilot/memory.json), + # so a /cwd change must rebuild the project store for the new path — + # otherwise the previous workspace's facts keep injecting (and, under + # cloud, egressing). The shared global store is preserved as-is. + self._memory = dataclasses.replace( + self._memory, + project_store=MemoryStore( + project_state_dir(workspace) / "memory.json", + project_id=project_id_for(workspace), + redact=self._settings.privacy.redact_secrets, + ), + ) if self._audit is not None: # Update the logger's workspace field BEFORE writing the event so # the change record itself (and all subsequent events) carry the new @@ -476,7 +490,10 @@ def run_turn(self, text: str, *, images: Sequence[ImageRef] = ()) -> str: return "" if not self._settings.runtime.auto_compact and ( - self.estimated_prompt_tokens() + estimate_tokens(text) > self.budget.hard_limit_tokens + self.estimated_prompt_tokens() + + estimate_tokens(text) + + IMAGE_TOKEN_ESTIMATE * len(images) + > self.budget.hard_limit_tokens ): self._ui.show_status( "Context is over the hard limit and automatic compaction is off. " diff --git a/shellpilot/tools/patch.py b/shellpilot/tools/patch.py index 993452f..59e08d1 100644 --- a/shellpilot/tools/patch.py +++ b/shellpilot/tools/patch.py @@ -19,6 +19,7 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.persistence.json_store import atomic_write_text +from shellpilot.policy.command_policy import sensitive_path_reason from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.tools.base import ( ToolContext, @@ -32,6 +33,28 @@ OPERATIONS = ("replace_exact", "insert_before", "insert_after", "delete_exact") WRITE_MODES = ("create", "overwrite", "append") MAX_PREVIEW_LINES = 60 +# Stand-in shown instead of a real diff when the target is an in-workspace +# sensitive file, so existing secret contents never render in the approval +# preview or the diff returned to the model (design section 15). +SENSITIVE_DIFF_PLACEHOLDER = "[sensitive file contents hidden]\n" + + +def _hides_sensitive_contents(context: ToolContext, path: Path) -> bool: + """True when *path* is an in-workspace sensitive file whose existing contents + must not be disclosed in a pre-approval diff (design section 15). + + ``path`` is the resolved, in-workspace target (out-of-workspace paths are + already rejected by ``resolve_in_workspace``), so a workspace-relative form + always exists. ``allow_sensitive_reads == "always"`` opts back into showing + the real before-content, mirroring read_file's sensitive-read gate. + """ + if context.allow_sensitive_reads == "always": + return False + try: + relative = path.relative_to(context.workspace.resolve()) + except ValueError: + return False + return sensitive_path_reason(relative) is not None def apply_edit(text: str, operation: str, old: str, new: str) -> tuple[str | None, str]: @@ -141,7 +164,11 @@ def _patch_file(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: assert context.snapshots is not None context.snapshots.record(path, new_text.encode("utf-8")) display = workspace_display(context.workspace, str(arguments["path"])) - diff = unified_diff(display, text, new_text) + diff = ( + SENSITIVE_DIFF_PLACEHOLDER + if _hides_sensitive_contents(context, path) + else unified_diff(display, text, new_text) + ) return ToolResult( success=True, summary=f"patched {arguments['path']} ({arguments['operation']})", @@ -164,6 +191,8 @@ def _patch_preview(context: ToolContext, arguments: dict[str, Any]) -> str: ) if new_text is None: return f"(cannot preview: {edit_error})" + if _hides_sensitive_contents(context, path): + return SENSITIVE_DIFF_PLACEHOLDER display = workspace_display(context.workspace, str(arguments["path"])) return unified_diff(display, text, new_text) @@ -212,7 +241,11 @@ def _write_file(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: _write_preserving(path, new_text) if context.snapshots is not None: context.snapshots.record(path, new_text.encode("utf-8")) - diff = unified_diff(workspace_display(context.workspace, raw_path), before, new_text) + diff = ( + SENSITIVE_DIFF_PLACEHOLDER + if mode != "create" and _hides_sensitive_contents(context, path) + else unified_diff(workspace_display(context.workspace, raw_path), before, new_text) + ) return ToolResult( success=True, summary=f"wrote {raw_path} ({mode}, {len(new_text)} chars)", @@ -237,6 +270,8 @@ def _write_preview(context: ToolContext, arguments: dict[str, Any]) -> str: else: before = path.read_bytes().decode("utf-8", errors="replace") after = before + content if mode == "append" else content + if _hides_sensitive_contents(context, path): + return SENSITIVE_DIFF_PLACEHOLDER return unified_diff(workspace_display(context.workspace, raw_path), before, after) diff --git a/tests/test_compaction.py b/tests/test_compaction.py index cd9c371..2573503 100644 --- a/tests/test_compaction.py +++ b/tests/test_compaction.py @@ -116,3 +116,42 @@ def test_auto_compact_off_allows_normal_turns(tmp_path: Path) -> None: ) assert runtime.run_turn("hello") == "hi" assert len(fake.calls) == 1 + + +def test_auto_compact_off_counts_incoming_image_tokens(tmp_path: Path) -> None: + """The pre-turn hard-limit gate must count THIS turn's incoming images, not + only text and history images (design section 10.5).""" + import base64 + import hashlib + + from shellpilot.llm.messages import ImageRef + from shellpilot.runtime.conversation import IMAGE_TOKEN_ESTIMATE + from tests.conftest import TINY_PNG + + ref = ImageRef( + path="/tmp/img.png", + sha256=hashlib.sha256(TINY_PNG).hexdigest(), + data_b64=base64.b64encode(TINY_PNG).decode(), + ) + runtime, ui, fake = make_runtime( + tmp_path, context_tokens=4096, auto_compact=False, script=[answer("seen")] + ) + # An empty-text, no-image turn is comfortably under the hard limit; carrying + # enough images to close that gap must trip the gate (text stays empty). + headroom = runtime.budget.hard_limit_tokens - runtime.estimated_prompt_tokens() + assert headroom > 0 + image_count = headroom // IMAGE_TOKEN_ESTIMATE + 1 + reply = runtime.run_turn("", images=tuple(ref for _ in range(image_count))) + assert reply == "" + assert fake.calls == [] # the gate fired before any model call + assert any("hard limit" in status.lower() for status in ui.statuses) + + +def test_auto_compact_off_text_near_limit_unaffected(tmp_path: Path) -> None: + """A text-only turn under the hard limit is allowed exactly as before.""" + runtime, _, fake = make_runtime( + tmp_path, context_tokens=4096, auto_compact=False, script=[answer("seen")] + ) + assert runtime.estimated_prompt_tokens() < runtime.budget.hard_limit_tokens + assert runtime.run_turn("hello") == "seen" + assert len(fake.calls) == 1 diff --git a/tests/test_conversation.py b/tests/test_conversation.py index 5e7c4d5..4c3b5f3 100644 --- a/tests/test_conversation.py +++ b/tests/test_conversation.py @@ -472,6 +472,51 @@ def test_image_token_estimate_counted(tmp_path: Path) -> None: assert estimate_with_image >= estimate_no_image + IMAGE_TOKEN_ESTIMATE +def test_set_workspace_rebuilds_project_memory(tmp_path: Path) -> None: + """Changing workspace (/cwd) rebuilds the project memory store for the new + path, so the previous workspace's facts stop injecting (design section 16); + the shared global store is preserved.""" + from shellpilot.memory.store import MemoryStore, MemoryStores, project_id_for + from shellpilot.persistence.paths import project_state_dir + + workspace_a = tmp_path / "a" + workspace_b = tmp_path / "b" + workspace_a.mkdir() + workspace_b.mkdir() + + global_store = MemoryStore(tmp_path / "global-memory.json") + stores = MemoryStores( + global_store=global_store, + project_store=MemoryStore( + project_state_dir(workspace_a) / "memory.json", + project_id=project_id_for(workspace_a), + ), + ) + stores.project_store.add_fact(kind="config", value="postgres-a", label="db", source="user") + + runtime = ConversationRuntime( + llm=FakeLLM(script=[]), + settings=Settings(), + workspace=workspace_a, + behavior=BehaviorInstructions(global_text=None, project_text=None), + ui=FakeUI(), + memory=stores, + ) + assert "postgres-a" in runtime.context_snapshot().system_text() + + runtime.set_workspace(workspace_b) + + rendered_b = runtime.context_snapshot().system_text() + assert "postgres-a" not in rendered_b # A's fact no longer injected into B + assert runtime.memory is not None + assert runtime.memory.global_store is global_store # shared global untouched + + runtime.memory.project_store.add_fact( + kind="config", value="postgres-b", label="db", source="user" + ) + assert "postgres-b" in runtime.context_snapshot().system_text() + + def test_update_plan_after_clear_reports_no_active_plan(tmp_path: Path) -> None: """After clear, the update_plan tool handler returns 'no active plan'.""" fake = FakeLLM( diff --git a/tests/test_write_tools.py b/tests/test_write_tools.py index 1cab01b..1dc235a 100644 --- a/tests/test_write_tools.py +++ b/tests/test_write_tools.py @@ -197,6 +197,92 @@ def test_write_preview_diff_header_uses_resolved_relative_path(tmp_path: Path) - assert "sub/../sub/new.txt" not in diff +# -- in-workspace sensitive-file pre-approval previews (design section 15) ------- + + +def _sensitive_ctx(workspace: Path, snapshots: SnapshotStore, mode: str) -> ToolContext: + return ToolContext( + workspace=workspace, + max_result_tokens=2000, + snapshots=snapshots, + allow_sensitive_reads=mode, + ) + + +def test_write_preview_hides_in_workspace_sensitive_before_content(tmp_path: Path) -> None: + """Overwriting an in-workspace sensitive file (.env) must not render the + existing secret contents in the pre-approval diff (design section 15).""" + (tmp_path / ".env").write_text("API_KEY=supersecret123\nDB_PASSWORD=hunter2\n") + diff = WRITE_FILE.preview( # type: ignore[misc] + ctx(tmp_path), # allow_sensitive_reads defaults to "ask" + {"path": ".env", "content": "API_KEY=x\n", "mode": "overwrite"}, + ) + assert "supersecret123" not in diff + assert "hunter2" not in diff + assert "[sensitive file contents hidden]" in diff + + +def test_write_preview_shows_non_sensitive_before_content(tmp_path: Path) -> None: + """A normal in-workspace overwrite still shows its real before-content diff.""" + (tmp_path / "notes.txt").write_text("alpha\nbeta\n") + diff = WRITE_FILE.preview( # type: ignore[misc] + ctx(tmp_path), + {"path": "notes.txt", "content": "gamma\n", "mode": "overwrite"}, + ) + assert "alpha" in diff + assert "[sensitive file contents hidden]" not in diff + + +def test_write_preview_shows_sensitive_when_allow_always(tmp_path: Path) -> None: + """allow_sensitive_reads='always' opts back into the real before-content.""" + (tmp_path / ".env").write_text("API_KEY=visiblesecret\n") + diff = WRITE_FILE.preview( # type: ignore[misc] + _sensitive_ctx(tmp_path, SnapshotStore(), "always"), + {"path": ".env", "content": "API_KEY=x\n", "mode": "overwrite"}, + ) + assert "visiblesecret" in diff + + +def test_write_file_result_diff_hides_sensitive_before(tmp_path: Path) -> None: + """The diff returned to the model after writing an in-workspace sensitive file + omits the prior secret contents.""" + (tmp_path / ".env").write_text("TOKEN=abc123secret\n") + context = read_then_ctx(tmp_path, ".env") + result = WRITE_FILE.handler( + context, {"path": ".env", "content": "TOKEN=new\n", "mode": "overwrite"} + ) + assert result.success + assert "abc123secret" not in result.content + assert "abc123secret" not in result.metadata.get("diff", "") + assert "[sensitive file contents hidden]" in result.content + + +def test_patch_preview_hides_sensitive_before(tmp_path: Path) -> None: + """Patching an in-workspace sensitive file does not reveal its contents.""" + (tmp_path / ".env").write_text("SECRET=topsecret\nKEEP=1\n") + context = read_then_ctx(tmp_path, ".env") + diff = PATCH_FILE.preview( # type: ignore[misc] + context, + {"path": ".env", "operation": "replace_exact", "old": "KEEP=1", "new": "KEEP=2"}, + ) + assert "topsecret" not in diff + assert "[sensitive file contents hidden]" in diff + + +def test_patch_file_result_diff_hides_sensitive(tmp_path: Path) -> None: + """The patch result diff returned to the model omits the secret contents.""" + (tmp_path / ".env").write_text("SECRET=topsecret\nKEEP=1\n") + context = read_then_ctx(tmp_path, ".env") + result = PATCH_FILE.handler( + context, + {"path": ".env", "operation": "replace_exact", "old": "KEEP=1", "new": "KEEP=2"}, + ) + assert result.success + assert "topsecret" not in result.content + assert "topsecret" not in result.metadata.get("diff", "") + assert "[sensitive file contents hidden]" in result.content + + def test_stale_snapshot_rejected(tmp_path: Path) -> None: (tmp_path / "f.py").write_text("x = 1\n") context = read_then_ctx(tmp_path, "f.py") From a2f21fa9c9b110cd5e622ef0c1cd662bffc765af Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 10:55:11 -0400 Subject: [PATCH 09/27] fix(security): validate uddg redirect scheme, clamp max_results, floor reserved builtin names - web/search.py: the decoded DuckDuckGo uddg redirect target is returned only when its scheme is http(s); a javascript:/file:/data: target now yields "" (skipped) instead of being emitted as a result. - tools/web.py: guard int(max_results) against non-numeric input and clamp it to [1, 10] so a bad or oversized argument can neither raise nor request an oversized fetch. - skills/loader.py: union the static builtin-trigger map into the reserved name set so a partial builtin-discovery failure cannot shrink the set and let a user skill masquerade as a builtin. --- shellpilot/skills/loader.py | 2 +- shellpilot/tools/web.py | 6 +++++- shellpilot/web/search.py | 3 ++- tests/test_skills.py | 17 +++++++++++++++ tests/test_web_search.py | 43 +++++++++++++++++++++++++++++++++++++ tests/test_web_tools.py | 42 ++++++++++++++++++++++++++++++++++++ 6 files changed, 110 insertions(+), 3 deletions(-) diff --git a/shellpilot/skills/loader.py b/shellpilot/skills/loader.py index ad447e5..66a8cbc 100644 --- a/shellpilot/skills/loader.py +++ b/shellpilot/skills/loader.py @@ -638,7 +638,7 @@ def merge_skills(builtin: list[Skill], user: list[Skill]) -> list[Skill]: invalid with error="reserved builtin name". Builtin names are harness machinery; a local folder must not be able to override them. """ - builtin_names: set[str] = {s.name for s in builtin} + builtin_names: set[str] = {s.name for s in builtin} | set(_BUILTIN_TRIGGER_MAP) merged_user: list[Skill] = [] for skill in user: if skill.name in builtin_names: diff --git a/shellpilot/tools/web.py b/shellpilot/tools/web.py index 3a6fb1b..f3442ef 100644 --- a/shellpilot/tools/web.py +++ b/shellpilot/tools/web.py @@ -41,7 +41,11 @@ def _search(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: summary="query is required", content="Provide a non-empty query string.", ) - max_results = int(arguments.get("max_results", 5)) + try: + max_results = int(arguments.get("max_results", 5)) + except (TypeError, ValueError): + max_results = 5 + max_results = max(1, min(max_results, 10)) try: results = provider.search(query, max_results=max_results) except WebSearchError as exc: diff --git a/shellpilot/web/search.py b/shellpilot/web/search.py index 37eb4ea..7e151f4 100644 --- a/shellpilot/web/search.py +++ b/shellpilot/web/search.py @@ -165,7 +165,8 @@ def _resolve_url(href: str) -> str: qs = parse_qs(parts.query) uddg = qs.get("uddg", []) if uddg: - return uddg[0] + target = uddg[0] + return target if urlsplit(target).scheme in ("http", "https") else "" return "" # Already an absolute http(s) link diff --git a/tests/test_skills.py b/tests/test_skills.py index e4e76ef..79991da 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -1102,6 +1102,23 @@ def test_new_builtin_names_are_reserved(tmp_path: Path) -> None: assert collision.error == "reserved builtin name" +def test_merge_reserves_builtin_names_even_when_builtin_list_is_empty() -> None: + """Reserved builtin names must be enforced even when builtin discovery returned nothing. + + If the builtin list is empty (e.g. due to a discovery failure) and the + reserved set is derived only from that list, a user skill named "planning" + would pass through valid — which is wrong. The static trigger map + provides a floor so the reserved set is never smaller than the full + builtin name set. + """ + user = [_make_skill("planning", root="user")] + merged = merge_skills(builtin=[], user=user) + user_results = [s for s in merged if s.root == "user"] + assert len(user_results) == 1 + assert user_results[0].valid is False + assert "reserved builtin name" in user_results[0].error + + # --------------------------------------------------------------------------- # Fix 1: manifest.json byte-cap and entry-count cap (boot-DoS hardening) # --------------------------------------------------------------------------- diff --git a/tests/test_web_search.py b/tests/test_web_search.py index 3f66f3b..6cb1eb3 100644 --- a/tests/test_web_search.py +++ b/tests/test_web_search.py @@ -146,3 +146,46 @@ def test_duckduckgo_provider_ignores_ambient_proxy_env() -> None: """ provider = DuckDuckGoProvider(transport=_make_transport(_DDG_EMPTY_HTML)) assert provider._client.trust_env is False + + +# --------------------------------------------------------------------------- +# _resolve_url: scheme validation of decoded uddg targets +# --------------------------------------------------------------------------- + + +def test_resolve_url_rejects_javascript_scheme_in_uddg() -> None: + """A DDG redirect whose uddg value has a javascript: scheme must return empty string. + + The decoder validates only the DDG wrapper URL's scheme, not the decoded + target. A javascript: or file: target must be rejected before it can be + emitted as a search result. + """ + from urllib.parse import quote + + from shellpilot.web.search import _resolve_url # module-private, tested directly + + javascript_target = "javascript:alert(1)" + href = f"//duckduckgo.com/l/?uddg={quote(javascript_target)}&rut=x" + assert _resolve_url(href) == "" + + +def test_resolve_url_rejects_file_scheme_in_uddg() -> None: + """A DDG redirect whose uddg value has a file: scheme must return empty string.""" + from urllib.parse import quote + + from shellpilot.web.search import _resolve_url + + file_target = "file:///etc/passwd" + href = f"//duckduckgo.com/l/?uddg={quote(file_target)}&rut=x" + assert _resolve_url(href) == "" + + +def test_resolve_url_passes_through_https_uddg_target() -> None: + """A DDG redirect with a valid https: uddg target is returned unchanged.""" + from urllib.parse import quote + + from shellpilot.web.search import _resolve_url + + target = "https://example.com/page" + href = f"//duckduckgo.com/l/?uddg={quote(target)}&rut=x" + assert _resolve_url(href) == target diff --git a/tests/test_web_tools.py b/tests/test_web_tools.py index ff2e180..c470741 100644 --- a/tests/test_web_tools.py +++ b/tests/test_web_tools.py @@ -176,6 +176,48 @@ def test_web_search_empty_query_returns_failed_result(tmp_path: Path) -> None: assert provider.calls == [] # provider never called +def test_web_search_non_numeric_max_results_falls_back_to_default(tmp_path: Path) -> None: + """A non-numeric max_results value must not raise; the provider is called with 5.""" + provider = _MockSearchProvider([]) + specs = make_web_tools(provider, _empty_fetcher()) + search_spec = next(s for s in specs if s.name == "web_search") + + ctx = ToolContext(workspace=tmp_path, max_result_tokens=2000) + search_spec.handler(ctx, {"query": "test", "max_results": "abc"}) + + assert len(provider.calls) == 1 + _, received_max = provider.calls[0] + assert received_max == 5 + + +def test_web_search_excessive_max_results_is_clamped(tmp_path: Path) -> None: + """max_results values above 10 must be clamped to 10.""" + provider = _MockSearchProvider([]) + specs = make_web_tools(provider, _empty_fetcher()) + search_spec = next(s for s in specs if s.name == "web_search") + + ctx = ToolContext(workspace=tmp_path, max_result_tokens=2000) + search_spec.handler(ctx, {"query": "test", "max_results": 999}) + + assert len(provider.calls) == 1 + _, received_max = provider.calls[0] + assert received_max == 10 + + +def test_web_search_zero_max_results_is_clamped_to_one(tmp_path: Path) -> None: + """max_results of 0 or negative must be clamped to 1.""" + provider = _MockSearchProvider([]) + specs = make_web_tools(provider, _empty_fetcher()) + search_spec = next(s for s in specs if s.name == "web_search") + + ctx = ToolContext(workspace=tmp_path, max_result_tokens=2000) + search_spec.handler(ctx, {"query": "test", "max_results": -3}) + + assert len(provider.calls) == 1 + _, received_max = provider.calls[0] + assert received_max == 1 + + # --------------------------------------------------------------------------- # web_fetch: result formatting # --------------------------------------------------------------------------- From 077104232647f5c6c39c0eb39c377db445e4231b Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 10:58:35 -0400 Subject: [PATCH 10/27] docs: reconcile DESIGN with the CLI interrupt and resume fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - §21: document the interrupt-resilience contract — Ctrl+C during a running manual-shell command aborts the command (not the loop), and a KeyboardInterrupt or model-call error while handling a slash command or a ! escape returns to the prompt instead of ending the session. - §36.8: note that a --resume target is existence-resolved before the cloud consent gate, so a stale/typo'd id fails fast with no consent prompt, preload, or cloud round-trip. - fix three trust_env test docstrings that cited §36.3 (Egress/Safety Setter Invariant); ambient-proxy isolation is documented at §36.10. --- docs/DESIGN.md | 4 ++++ tests/test_ollama_client.py | 2 +- tests/test_web_fetch.py | 2 +- tests/test_web_search.py | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 5b6e52e..d0bec24 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -2125,6 +2125,8 @@ Type /exit-shell to return. A `!` prefix is a one-shot escape into the same audited manual-shell path: `!` runs a single command (raw shell, identical to `/shell`) without entering the shell loop, while a bare `!` opens the loop. It is a human-only escape — the prefix is recognized only at the interactive prompt reader, so model output can never reach it. +Inside the shell loop, Ctrl+C during a running command aborts that command and returns to the `manual$` prompt rather than leaving the shell; Ctrl+C at an empty prompt (or EOF) exits back to the agent CLI. The unified CLI applies the same discipline to interactive turns: a `KeyboardInterrupt` or a model-call error raised while handling a slash command or a `!` escape is caught and returns to the prompt instead of tearing down the session (a clean `/exit` is the only path that ends it). + ## 22. Logging And Audit Audit logs should be local and structured. @@ -3033,6 +3035,8 @@ The two highest-value at-rest artifacts — session transcripts and the audit lo The opt-in cloud feature (section 15.2) rests on four enforced controls, summarized here as the security spine: (1) `[model] allow_cloud` defaults to **`false`** and can be enabled only by a deliberate act — a `config.toml` edit or a confirm-gated `/config set`, never an ambient env var (section 36.3); (2) a **fail-closed per-session consent gate** (`_resolve_cloud_consent`) runs at boot strictly before any prompt-bearing call and declines on a non-TTY, EOF, Enter, or anything but an explicit yes — so a decline egresses no prompt data; (3) an **unspoofable active-cloud indicator** (boot banner, persistent bottom status bar [§31.11], `/status` locality) derived from `is_egressing` on the live model, never from model output, with the bar's user-controlled workspace path sanitized at the render sink (§36.2); and (4) a `cloud_consent_granted` audit event recording the consent. Consent is *the* boundary: once granted, the prompt egresses, with the best-effort redaction above layered behind it. Local-first remains the only full-privacy posture. +On the resume path specifically, a `--resume ` target is existence-resolved (the model-free filesystem lookup) *before* the consent gate runs, so a typo'd or stale session id fails fast — no consent prompt, no model preload, no cloud round-trip — because the gate's prompt-bearing work is never reached for a resume that cannot load. + ### 36.9 Approval-Path Display Integrity An approval is only meaningful if the path it shows is the path that will be acted on. The audit found the user-facing path display was the **raw model argument**: the tool-call line and approval-panel head rendered `arguments["path"]` verbatim (`executor._display_for`, `show_tool_call`), and the diff-panel title showed only the resolved *basename* (`unified_diff` used `path.name`). A model could therefore pass an argument that *displays* as one in-workspace file while `resolve_in_workspace` *acts on* another (`..` segments, `./x/../y`, symlink, trailing junk) — the user approves a write under a misleading path. The boundary itself was never weak (the action was always resolved and bounded); the gap was cosmetic, within-workspace, and it is exactly the trust an approval rests on. Closed by the **display-integrity invariant** (section 14.5): all three displays now derive from the single `workspace_display` helper, which is layered over the canonical `resolve_in_workspace` — so the displayed path is always the resolved, workspace-relative target the action uses, and a boundary-escaping path shows an honest `` marker, never a fabricated-looking path. Display-only change: the resolution and boundary checks are untouched. (`run_command` shows its literal `argv` — not a resolution case — and is unchanged.) diff --git a/tests/test_ollama_client.py b/tests/test_ollama_client.py index 54866da..ba11bbf 100644 --- a/tests/test_ollama_client.py +++ b/tests/test_ollama_client.py @@ -129,7 +129,7 @@ def test_client_ignores_ambient_proxy_env() -> None: """The httpx client must not honour ambient proxy env vars. Loopback Ollama traffic cannot be redirected by HTTP_PROXY/HTTPS_PROXY/ALL_PROXY - in the environment — trust_env=False enforces this invariant (F7 / §36.3). + in the environment — trust_env=False enforces this invariant (F7 / §36.10). """ client = make_client(httpx.MockTransport(lambda r: httpx.Response(200, json={}))) assert client._client.trust_env is False diff --git a/tests/test_web_fetch.py b/tests/test_web_fetch.py index 03624b0..a4fef31 100644 --- a/tests/test_web_fetch.py +++ b/tests/test_web_fetch.py @@ -611,7 +611,7 @@ def test_page_fetcher_ignores_ambient_proxy_env() -> None: """PageFetcher's httpx client must not honour ambient proxy env vars. Web fetch traffic cannot be silently redirected through an ambient proxy — - trust_env=False keeps the egress audit's destination truthful (§36.3). + trust_env=False keeps the egress audit's destination truthful (§36.10). """ fetcher = PageFetcher(transport=_html_transport("ok")) assert fetcher._client.trust_env is False diff --git a/tests/test_web_search.py b/tests/test_web_search.py index 6cb1eb3..3732305 100644 --- a/tests/test_web_search.py +++ b/tests/test_web_search.py @@ -142,7 +142,7 @@ def test_duckduckgo_provider_ignores_ambient_proxy_env() -> None: """DuckDuckGoProvider's httpx client must not honour ambient proxy env vars. Web search traffic cannot be silently redirected through an ambient proxy — - trust_env=False keeps the egress audit's destination truthful (§36.3). + trust_env=False keeps the egress audit's destination truthful (§36.10). """ provider = DuckDuckGoProvider(transport=_make_transport(_DDG_EMPTY_HTML)) assert provider._client.trust_env is False From 1a56a3f362741e985519e7d7b2b55a5a96fd787d Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 11:17:57 -0400 Subject: [PATCH 11/27] refactor: remove dead code (superseded renderers, test-only wrapper, unreachable branches) - planner.py: delete unused render_plan_terminal (superseded by render_plan_markdown). - cli/render.py: delete the dead approval_block/approval_head cascade; the live approval path renders via approval_info/approval_cwd. - runtime/conversation.py: delete the test-only _endpoint_is_loopback wrapper and repoint its tests to is_loopback_url. - tools/base.py: remove an unreachable bool-vs-int branch in validate_args (bool is a subclass of int, so the outer guard never enters it; the live integer-type check is unchanged). - cli/slash.py: flatten a comma-split loop in command_words that always iterated once (no HELP_ROWS command string contains a comma); drop a dead args-is-None guard in _logs. - runtime/planner.py: drop a redundant pending_revision-is-None conjunct that is always true in every state reachable at that elif. --- shellpilot/cli/render.py | 12 ------------ shellpilot/cli/slash.py | 13 ++++++------- shellpilot/runtime/conversation.py | 10 +--------- shellpilot/runtime/planner.py | 20 +------------------- shellpilot/tools/base.py | 2 -- tests/test_conversation.py | 20 +++++++++----------- tests/test_render.py | 20 -------------------- 7 files changed, 17 insertions(+), 80 deletions(-) diff --git a/shellpilot/cli/render.py b/shellpilot/cli/render.py index 5fe3493..93a6d71 100644 --- a/shellpilot/cli/render.py +++ b/shellpilot/cli/render.py @@ -289,10 +289,6 @@ def badge(level: str, *, plain: bool = False) -> Text: return Text(f" {label} ", style=_BADGE_STYLES.get(level.lower(), "sp.badge.medium")) -def approval_head(request: ApprovalRequest, glyphs: Glyphs) -> Text: - return Text.assemble((f"{glyphs.bullet} ", ""), (request.display, "")) - - def approval_info(request: ApprovalRequest, *, plain_badge: bool = False) -> Text: info = Text(" ") info.append_text(badge(request.risk.value, plain=plain_badge)) @@ -307,14 +303,6 @@ def approval_cwd(request: ApprovalRequest) -> Text: return Text(f" CWD: {request.cwd}", style="sp.dim") -def approval_block(request: ApprovalRequest, glyphs: Glyphs, *, plain_badge: bool = False) -> Group: - return Group( - approval_head(request, glyphs), - approval_info(request, plain_badge=plain_badge), - approval_cwd(request), - ) - - def plan_step_line(index: int, step: PlanStep, glyphs: Glyphs) -> Text: title = _sanitize_line(step.title) if step.status == "completed": diff --git a/shellpilot/cli/slash.py b/shellpilot/cli/slash.py index b3954ba..87bb895 100644 --- a/shellpilot/cli/slash.py +++ b/shellpilot/cli/slash.py @@ -84,13 +84,12 @@ class SlashAction(Enum): def command_words() -> list[str]: - """Completion phrases derived from HELP_ROWS: split combined rows, drop .""" + """Completion phrases derived from HELP_ROWS: drop placeholders.""" words: list[str] = [] for entry, _ in HELP_ROWS: - for raw in entry.split(","): - phrase = " ".join(part for part in raw.split() if not part.startswith("<")) - if phrase and phrase not in words: - words.append(phrase) + phrase = " ".join(part for part in entry.split() if not part.startswith("<")) + if phrase and phrase not in words: + words.append(phrase) return words @@ -630,12 +629,12 @@ def _profile(self, args: list[str]) -> None: return self._console.print("Usage: /profile | /profile use ") - def _logs(self, args: list[str] | None = None) -> None: + def _logs(self, args: list[str]) -> None: audit = self._runtime.audit if audit is None: self._console.print("[dim]Audit logging is not active in this session.[/dim]") return - show_all = args is not None and args and args[0] == "all" + show_all = args[:1] == ["all"] session_filter = None if show_all else audit.session_id events = audit.tail(15, session_id=session_filter) if not events: diff --git a/shellpilot/runtime/conversation.py b/shellpilot/runtime/conversation.py index 340293d..dfb5db5 100644 --- a/shellpilot/runtime/conversation.py +++ b/shellpilot/runtime/conversation.py @@ -13,7 +13,7 @@ from shellpilot.config.model import Settings, is_egressing from shellpilot.llm.client import LLMClient from shellpilot.llm.messages import ImageRef, Message, tool_result, user -from shellpilot.llm.ollama import encode_tool, is_loopback_url +from shellpilot.llm.ollama import encode_tool from shellpilot.memory.agents_md import BehaviorInstructions from shellpilot.memory.redaction import redact_secrets, redact_structure from shellpilot.memory.store import MemoryStore, MemoryStores, project_id_for @@ -293,14 +293,6 @@ def _endpoint_host(self) -> str: """Host of the model endpoint (for audit); empty when unparseable.""" return (urlsplit(self._base_url).hostname or "").rstrip(".") - def _endpoint_is_loopback(self) -> bool: - """True when the model endpoint is on this box (loopback = local). - - Delegates to the shared ``is_loopback_url`` helper so the runtime egress - chokepoint and the CLI boot consent gate use one definition of off-box. - """ - return is_loopback_url(self._base_url) - def _is_egressing(self) -> bool: """True when a model request leaves this device. diff --git a/shellpilot/runtime/planner.py b/shellpilot/runtime/planner.py index 3055519..7444b55 100644 --- a/shellpilot/runtime/planner.py +++ b/shellpilot/runtime/planner.py @@ -146,23 +146,6 @@ def render_plan_markdown(plan: TaskPlan) -> str: return "\n".join(parts) -def render_plan_terminal(plan: TaskPlan) -> str: - lines = [f"Goal: {plan.goal}", ""] - if plan.assumptions: - lines.append("Assumptions:") - lines.extend(f"- {item}" for item in plan.assumptions) - lines.append("") - lines.append("Plan:") - for index, step in enumerate(plan.steps, start=1): - marker = {"completed": "✓", "active": "→", "skipped": "·"}.get(step.status, " ") - lines.append(f"{index}. [{marker}] {step.title}") - if plan.verification: - lines.append("") - lines.append("Verification:") - lines.extend(f"- {item}" for item in plan.verification) - return "\n".join(lines) - - def compact_plan_state(plan: TaskPlan) -> str: """Small plan-state block injected into the model context (section 11.3).""" steps = "; ".join( @@ -446,8 +429,7 @@ def _propose(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: ) plan = manager.active elif ( - manager.pending_revision is None - and manager.active is not None + manager.active is not None and manager.active.status in ("proposed", "active") and manager.active.goal == goal and [step.title for step in manager.active.steps] == steps diff --git a/shellpilot/tools/base.py b/shellpilot/tools/base.py index cd0ad41..337b139 100644 --- a/shellpilot/tools/base.py +++ b/shellpilot/tools/base.py @@ -110,8 +110,6 @@ def validate_args(spec: ToolSpec, arguments: dict[str, Any]) -> str | None: return f"unknown argument '{name}' for {spec.name}" expected = _JSON_TYPES.get(str(schema.get("type", "string"))) if expected is not None and not isinstance(value, expected): - if expected is int and isinstance(value, bool): - return f"argument '{name}' must be an integer" return f"argument '{name}' must be of type {schema.get('type')}" if expected is int and isinstance(value, bool): return f"argument '{name}' must be an integer" diff --git a/tests/test_conversation.py b/tests/test_conversation.py index 4c3b5f3..51689a7 100644 --- a/tests/test_conversation.py +++ b/tests/test_conversation.py @@ -925,6 +925,8 @@ def _make_runtime_with_base_url( def test_endpoint_loopback_detection_local(tmp_path: Path) -> None: """localhost / 127.x / ::1 / 0.0.0.0 / empty host are loopback (not egressing).""" + from shellpilot.llm.ollama import is_loopback_url + for url in ( "http://localhost:11434", "http://127.0.0.1:11434", @@ -934,19 +936,21 @@ def test_endpoint_loopback_detection_local(tmp_path: Path) -> None: "http://foo.localhost:11434", ): runtime = _make_runtime_with_base_url(FakeLLM(script=[]), FakeUI(), tmp_path, url) - assert runtime._endpoint_is_loopback() is True, url + assert is_loopback_url(url) is True, url assert runtime._is_egressing() is False, url def test_endpoint_loopback_detection_remote(tmp_path: Path) -> None: """A non-loopback host is remote → egressing.""" + from shellpilot.llm.ollama import is_loopback_url + for url in ( "https://ollama.com", "https://api.example.com:443", "http://10.0.0.5:11434", # private but not loopback → still off this box ): runtime = _make_runtime_with_base_url(FakeLLM(script=[]), FakeUI(), tmp_path, url) - assert runtime._endpoint_is_loopback() is False, url + assert is_loopback_url(url) is False, url assert runtime._is_egressing() is True, url @@ -1102,14 +1106,6 @@ def test_is_loopback_url_shared_helper() -> None: assert is_loopback_url(url) is False, url -def test_endpoint_is_loopback_uses_shared_helper(tmp_path: Path) -> None: - """_endpoint_is_loopback delegates to the shared helper (no behaviour change).""" - runtime = _make_runtime_with_base_url( - FakeLLM(script=[]), FakeUI(), tmp_path, "https://ollama.com" - ) - assert runtime._endpoint_is_loopback() is False - - def _make_runtime_with_model( fake: FakeLLM, ui: FakeUI, tmp_path: Path, model: str, *, base_url: str ) -> ConversationRuntime: @@ -1126,6 +1122,8 @@ def _make_runtime_with_model( def test_cloud_model_egresses_on_localhost(tmp_path: Path) -> None: """A '-cloud' model egresses even through a loopback Ollama proxy.""" + from shellpilot.llm.ollama import is_loopback_url + runtime = _make_runtime_with_model( FakeLLM(script=[]), FakeUI(), @@ -1133,7 +1131,7 @@ def test_cloud_model_egresses_on_localhost(tmp_path: Path) -> None: "nemotron-3-nano:30b-cloud", base_url="http://localhost:11434", ) - assert runtime._endpoint_is_loopback() is True + assert is_loopback_url("http://localhost:11434") is True assert runtime._is_egressing() is True diff --git a/tests/test_render.py b/tests/test_render.py index e6016c3..afc58c0 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -9,7 +9,6 @@ from rich.console import Console, RenderableType from shellpilot.cli.render import ( - approval_block, badge, context_line, output_truncation, @@ -21,8 +20,6 @@ word_highlight_ranges, ) from shellpilot.cli.theme import SHELLPILOT_THEME, UNICODE_GLYPHS -from shellpilot.policy.approvals import ApprovalRequest -from shellpilot.policy.risk import RiskLevel from shellpilot.runtime.planner import PlanStep, TaskPlan GLYPHS = UNICODE_GLYPHS @@ -217,23 +214,6 @@ def test_badge_chips_and_plain_degradation() -> None: assert badge("blocked", plain=True).plain == "[BLOCKED]" -def test_approval_block_composes_display_reasons_and_cwd() -> None: - request = ApprovalRequest( - kind="command", - display="rm -rf build/", - risk=RiskLevel.HIGH, - reasons=("recursive delete",), - cwd=Path("/tmp/ws"), - purpose="Removes stale build output.", - ) - out = rendered(approval_block(request, GLYPHS)) - assert "rm -rf build/" in out - assert " HIGH " in out - assert "recursive delete" in out - assert "Removes stale build output." in out - assert "/tmp/ws" in out - - def test_plan_panel_gate_and_step_lines() -> None: plan = TaskPlan( task_id="20260611-031500-demo-task", From 8d387f59b3f5b1f8275fdeef264a0ef9fe85392d Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 11:35:49 -0400 Subject: [PATCH 12/27] refactor(skills): dedupe loader.py path/traversable function pairs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builtin skills load via importlib.resources (Traversable) and user skills via filesystem Path, which forced six near-identical _*_path/_*_traversable function pairs. Collapse three of them while keeping both code paths and every load-time safety gate: - one _read_resource_bytes over Path | Traversable. - one _parse_manifest_bytes core for the shared JSON-decode / list-check / slice / per-entry-build branches; each caller keeps its own preamble, the per-caller too-large/unreadable messages, the injected exception-catch breadth (narrow for Path, broad for package resources), and — for the Path caller only — the _is_safe_path escape gates. - one _discover_markdown_resources normalizing on .name ops; the path-only _is_safe_path check is injected as a predicate (None = allow-all for trusted package resources). This unifies the .md filter on name.endswith(".md"), so a file named literally ".md" is now discovered consistently under both roots. - inline the dead _triggers_for_skill_name delegate. _script_entry_error_path keeps its two extra escape checks and stays unmerged with the traversable variant. Pure refactor; skill discovery output is unchanged. --- shellpilot/skills/loader.py | 213 +++++++++++++++--------------------- tests/test_skills.py | 22 ++++ 2 files changed, 111 insertions(+), 124 deletions(-) diff --git a/shellpilot/skills/loader.py b/shellpilot/skills/loader.py index 66a8cbc..d7fcedb 100644 --- a/shellpilot/skills/loader.py +++ b/shellpilot/skills/loader.py @@ -73,10 +73,6 @@ def _assign_builtin_reference_triggers( ) -def _triggers_for_skill_name(name: str) -> tuple[SkillTrigger, ...]: - return _triggers_for_builtin_skill_name(name) - - def _parse_frontmatter(text: str) -> tuple[dict[str, str], str, bool, str]: """Parse SKILL.md frontmatter. @@ -177,79 +173,27 @@ def _resource_from_bytes( ) -def _read_resource_bytes_path(entry: Path) -> bytes: +def _read_resource_bytes(entry: Path | importlib.resources.abc.Traversable) -> bytes: with entry.open("rb") as handle: return handle.read(MAX_RESOURCE_BYTES) -def _read_resource_bytes_traversable( - entry: importlib.resources.abc.Traversable, -) -> bytes: - with entry.open("rb") as handle: - return handle.read(MAX_RESOURCE_BYTES) - - -def _discover_markdown_resources_path( +def _discover_markdown_resources( + skill_root: Path | importlib.resources.abc.Traversable, *, - skill_root: Path, dirname: str, kind: str, max_tokens: int, + catch: type[BaseException] | tuple[type[BaseException], ...], + safe_check: Callable[[Any], bool] | None = None, ) -> tuple[tuple[SkillResource, ...], tuple[str, ...]]: - resource_dir = skill_root / dirname - if not resource_dir.is_dir(): - return (), () - - warnings: list[str] = [] - try: - candidates = sorted( - ( - entry - for entry in resource_dir.iterdir() - if entry.is_file() and entry.suffix == ".md" - ), - key=lambda entry: entry.stem, - ) - except OSError: - return (), (f"{dirname}/: could not read resources",) - resources: list[SkillResource] = [] - for entry in candidates[:MAX_RESOURCES_PER_KIND]: - rel_path = f"{dirname}/{entry.name}" - if not _is_safe_path(skill_root, entry): - warnings.append(f"{rel_path}: skipped unsafe path") - continue - try: - data = _read_resource_bytes_path(entry) - except OSError: - warnings.append(f"{rel_path}: could not read resource") - continue - try: - resources.append( - _resource_from_bytes( - kind=kind, - name=entry.stem, - rel_path=rel_path, - data=data, - max_tokens=max_tokens, - ) - ) - except UnicodeDecodeError: - warnings.append(f"{rel_path}: could not decode resource") - if len(candidates) > MAX_RESOURCES_PER_KIND: - warnings.append( - f"{dirname}: found {len(candidates)} markdown resources; " - f"loaded first {MAX_RESOURCES_PER_KIND} sorted by name" - ) - return tuple(resources), tuple(warnings) - + """Discover ``.md`` resources under ``dirname``. -def _discover_markdown_resources_traversable( - *, - skill_root: importlib.resources.abc.Traversable, - dirname: str, - kind: str, - max_tokens: int, -) -> tuple[tuple[SkillResource, ...], tuple[str, ...]]: + Serves both user (``Path``) and builtin (``Traversable``) roots by working on + ``entry.name``. ``safe_check`` is the path-only resolve-escape guard (builtin + package resources pass ``None`` — trusted, never resolve-checked); ``catch`` is + the iterdir/read exception breadth (path narrow ``OSError``; builtin broad). + """ resource_dir = skill_root / dirname if not resource_dir.is_dir(): return (), () @@ -264,14 +208,17 @@ def _discover_markdown_resources_traversable( ), key=lambda entry: Path(entry.name).stem, ) - except Exception: # noqa: BLE001 — advisory only + except catch: return (), (f"{dirname}/: could not read resources",) resources: list[SkillResource] = [] for entry in candidates[:MAX_RESOURCES_PER_KIND]: rel_path = f"{dirname}/{entry.name}" + if safe_check is not None and not safe_check(entry): + warnings.append(f"{rel_path}: skipped unsafe path") + continue try: - data = _read_resource_bytes_traversable(entry) - except Exception: # noqa: BLE001 — unreadable resource is advisory only + data = _read_resource_bytes(entry) + except catch: warnings.append(f"{rel_path}: could not read resource") continue try: @@ -423,6 +370,47 @@ def _script_from_manifest_entry( ) +def _parse_manifest_bytes( + manifest_bytes: bytes, + *, + entry_maker: Callable[[Any], SkillScript], + catch: type[BaseException] | tuple[type[BaseException], ...], +) -> tuple[tuple[SkillScript, ...], tuple[str, ...]]: + """Decode/validate the manifest bytes and build the scripts. + + Shared by the path and traversable callers; each reads the bytes and applies + its own size guard first. ``catch`` is the decode/parse exception breadth the + caller wants (path stays narrow; traversable is intentionally broad). + """ + try: + raw_manifest = json.loads(manifest_bytes.decode("utf-8")) + except catch as exc: + return ( + ( + _invalid_script( + name="manifest", + entry="scripts/manifest.json", + error=f"malformed scripts/manifest.json: {exc}", + ), + ), + (), + ) + if not isinstance(raw_manifest, list): + return ( + ( + _invalid_script( + name="manifest", + entry="scripts/manifest.json", + error="scripts/manifest.json must contain a list", + ), + ), + (), + ) + entries = raw_manifest[:MAX_RESOURCES_PER_KIND] + scripts = tuple(entry_maker(entry) for entry in entries) + return scripts, () + + def _script_from_manifest_entry_path(skill_root: Path, raw: Any) -> SkillScript: return _script_from_manifest_entry( raw, @@ -476,8 +464,8 @@ def _discover_scripts_path(skill_root: Path) -> tuple[tuple[SkillScript, ...], t ) try: - raw_manifest = json.loads(manifest.read_text(encoding="utf-8")) - except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + manifest_bytes = manifest.read_bytes() + except OSError as exc: return ( ( _invalid_script( @@ -489,21 +477,11 @@ def _discover_scripts_path(skill_root: Path) -> tuple[tuple[SkillScript, ...], t (), ) - if not isinstance(raw_manifest, list): - return ( - ( - _invalid_script( - name="manifest", - entry="scripts/manifest.json", - error="scripts/manifest.json must contain a list", - ), - ), - (), - ) - - entries = raw_manifest[:MAX_RESOURCES_PER_KIND] - scripts = tuple(_script_from_manifest_entry_path(skill_root, entry) for entry in entries) - return scripts, () + return _parse_manifest_bytes( + manifest_bytes, + entry_maker=lambda raw: _script_from_manifest_entry_path(skill_root, raw), + catch=(UnicodeDecodeError, json.JSONDecodeError), + ) def _script_from_manifest_entry_traversable( @@ -560,33 +538,11 @@ def _discover_scripts_traversable( ), (), ) - try: - raw_manifest = json.loads(manifest_bytes.decode("utf-8")) - except Exception as exc: # noqa: BLE001 — advisory invalid script, not fatal - return ( - ( - _invalid_script( - name="manifest", - entry="scripts/manifest.json", - error=f"malformed scripts/manifest.json: {exc}", - ), - ), - (), - ) - if not isinstance(raw_manifest, list): - return ( - ( - _invalid_script( - name="manifest", - entry="scripts/manifest.json", - error="scripts/manifest.json must contain a list", - ), - ), - (), - ) - entries = raw_manifest[:MAX_RESOURCES_PER_KIND] - scripts = tuple(_script_from_manifest_entry_traversable(skill_root, entry) for entry in entries) - return scripts, () + return _parse_manifest_bytes( + manifest_bytes, + entry_maker=lambda raw: _script_from_manifest_entry_traversable(skill_root, raw), + catch=Exception, + ) def _discover_resources_path( @@ -594,17 +550,24 @@ def _discover_resources_path( *, max_tokens: int, ) -> _DiscoveryResult: - references, ref_warnings = _discover_markdown_resources_path( - skill_root=skill_root, + def safe_check(entry: Path) -> bool: + return _is_safe_path(skill_root, entry) + + references, ref_warnings = _discover_markdown_resources( + skill_root, dirname="references", kind="reference", max_tokens=max_tokens, + catch=OSError, + safe_check=safe_check, ) - templates, template_warnings = _discover_markdown_resources_path( - skill_root=skill_root, + templates, template_warnings = _discover_markdown_resources( + skill_root, dirname="templates", kind="template", max_tokens=max_tokens, + catch=OSError, + safe_check=safe_check, ) scripts, script_warnings = _discover_scripts_path(skill_root) return references, templates, scripts, ref_warnings + template_warnings + script_warnings @@ -615,17 +578,19 @@ def _discover_resources_traversable( *, max_tokens: int, ) -> _DiscoveryResult: - references, ref_warnings = _discover_markdown_resources_traversable( - skill_root=skill_root, + references, ref_warnings = _discover_markdown_resources( + skill_root, dirname="references", kind="reference", max_tokens=max_tokens, + catch=Exception, ) - templates, template_warnings = _discover_markdown_resources_traversable( - skill_root=skill_root, + templates, template_warnings = _discover_markdown_resources( + skill_root, dirname="templates", kind="template", max_tokens=max_tokens, + catch=Exception, ) scripts, script_warnings = _discover_scripts_traversable(skill_root) return references, templates, scripts, ref_warnings + template_warnings + script_warnings @@ -684,7 +649,7 @@ def discover_skills( description="", body="", root="builtin", - triggers=_triggers_for_skill_name(entry.name), + triggers=_triggers_for_builtin_skill_name(entry.name), est_tokens=0, valid=False, error="could not read SKILL.md", @@ -703,7 +668,7 @@ def discover_skills( skill = replace( raw, root="builtin", - triggers=_triggers_for_skill_name(entry.name), + triggers=_triggers_for_builtin_skill_name(entry.name), references=references, templates=templates, scripts=scripts, diff --git a/tests/test_skills.py b/tests/test_skills.py index 79991da..3328283 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -738,6 +738,28 @@ def fail_read_bytes(self: Path) -> bytes: assert skill.references[0].text == "Bounded read" +def test_discover_resource_filter_uses_name_endswith(tmp_path: Path) -> None: + """User and builtin roots share one filter: ``entry.name.endswith(".md")``. + + A file named literally ``.md`` is therefore discovered (its ``.suffix`` is + empty, so an older ``suffix == ".md"`` filter would have excluded it). This + pins the unified filter against a regression back to suffix-based matching. + """ + skill_dir = _user_skill_dir(tmp_path) + refs = skill_dir / "references" + refs.mkdir() + (refs / ".md").write_text("Dotfile resource", encoding="utf-8") + (refs / "normal.md").write_text("Normal resource", encoding="utf-8") + + skill = _only_user_skill(tmp_path) + + assert [resource.rel_path for resource in skill.references] == [ + "references/.md", + "references/normal.md", + ] + assert skill.warnings == () + + def test_discover_ignores_disallowed_top_level_dirs(tmp_path: Path) -> None: skill_dir = _user_skill_dir(tmp_path) docs = skill_dir / "docs" From bfa1639faa1262ff3a33106afdaf3716cdc808a5 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 11:46:40 -0400 Subject: [PATCH 13/27] refactor(tools): centralize ALL_PROFILES in tools/base.py ALL_PROFILES was defined in 5 tool modules and re-stated the canonical config.model.VALID_PROFILES, so adding a profile needed 6 edits and a missed copy would silently de-register a tool. Define it once in tools/base.py as frozenset(VALID_PROFILES) and import it everywhere; command/environment/planner drop their now-unused filesystem import. A guard test pins ALL_PROFILES == frozenset(VALID_PROFILES). --- shellpilot/runtime/planner.py | 3 +-- shellpilot/tools/base.py | 5 +++++ shellpilot/tools/command.py | 3 +-- shellpilot/tools/environment.py | 3 +-- shellpilot/tools/filesystem.py | 3 +-- shellpilot/tools/images.py | 3 +-- shellpilot/tools/memory_tools.py | 4 +--- shellpilot/tools/patch.py | 3 ++- shellpilot/tools/search.py | 10 ++++++++-- shellpilot/tools/skill_tools.py | 4 +--- shellpilot/tools/web.py | 4 +--- tests/test_config.py | 12 ++++++++++++ 12 files changed, 35 insertions(+), 22 deletions(-) diff --git a/shellpilot/runtime/planner.py b/shellpilot/runtime/planner.py index 7444b55..979a7ae 100644 --- a/shellpilot/runtime/planner.py +++ b/shellpilot/runtime/planner.py @@ -19,8 +19,7 @@ from shellpilot.persistence.json_store import atomic_write_json, atomic_write_text from shellpilot.persistence.paths import project_state_dir from shellpilot.policy.risk import RiskLevel, SideEffect -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec -from shellpilot.tools.filesystem import ALL_PROFILES +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec STEP_STATUSES = ("pending", "active", "completed", "skipped") PLAN_STATUSES = ("proposed", "active", "blocked", "completed", "cancelled") diff --git a/shellpilot/tools/base.py b/shellpilot/tools/base.py index 337b139..9536e7d 100644 --- a/shellpilot/tools/base.py +++ b/shellpilot/tools/base.py @@ -7,11 +7,16 @@ from pathlib import Path from typing import Any +from shellpilot.config.model import VALID_PROFILES from shellpilot.llm.messages import ToolDefinition from shellpilot.persistence.snapshots import SnapshotStore from shellpilot.policy.command_policy import CommandRisk from shellpilot.policy.risk import RiskLevel, SideEffect +# Single canonical set derived from config. Importing modules use this instead +# of re-stating the literal so a future profile add requires only one edit here. +ALL_PROFILES: frozenset[str] = frozenset(VALID_PROFILES) + class ToolError(Exception): """A tool could not run; the message is safe to show the model.""" diff --git a/shellpilot/tools/command.py b/shellpilot/tools/command.py index cad8a50..1ba93d0 100644 --- a/shellpilot/tools/command.py +++ b/shellpilot/tools/command.py @@ -22,8 +22,7 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.policy.command_policy import CommandRisk, classify_command from shellpilot.policy.risk import RiskLevel, SideEffect -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec -from shellpilot.tools.filesystem import ALL_PROFILES +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec DEFAULT_TIMEOUT_SECONDS = 600 # Maximum chars read in a single readline() call so a newline-less stream cannot diff --git a/shellpilot/tools/environment.py b/shellpilot/tools/environment.py index 18a29f1..163ae37 100644 --- a/shellpilot/tools/environment.py +++ b/shellpilot/tools/environment.py @@ -8,8 +8,7 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.policy.risk import RiskLevel, SideEffect -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec -from shellpilot.tools.filesystem import ALL_PROFILES +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec def _env_info(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: diff --git a/shellpilot/tools/filesystem.py b/shellpilot/tools/filesystem.py index 85a4b91..84339d5 100644 --- a/shellpilot/tools/filesystem.py +++ b/shellpilot/tools/filesystem.py @@ -10,6 +10,7 @@ from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.runtime.budget import truncate_to_tokens from shellpilot.tools.base import ( + ALL_PROFILES, ToolContext, ToolResult, ToolSpec, @@ -20,8 +21,6 @@ MAX_DIR_ENTRIES = 500 _BINARY_SNIFF_BYTES = 8192 -ALL_PROFILES = frozenset({"supervised", "balanced"}) - def is_binary(path: Path) -> bool: with path.open("rb") as handle: diff --git a/shellpilot/tools/images.py b/shellpilot/tools/images.py index f8ae742..67aae48 100644 --- a/shellpilot/tools/images.py +++ b/shellpilot/tools/images.py @@ -19,6 +19,7 @@ from shellpilot.llm.messages import ImageRef, ToolDefinition from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.tools.base import ( + ALL_PROFILES, ToolContext, ToolResult, ToolSpec, @@ -26,8 +27,6 @@ resolve_in_workspace, ) -ALL_PROFILES = frozenset({"supervised", "balanced"}) - def make_view_image_tool( stage: Callable[[ImageRef], None], diff --git a/shellpilot/tools/memory_tools.py b/shellpilot/tools/memory_tools.py index a20ec24..580b62e 100644 --- a/shellpilot/tools/memory_tools.py +++ b/shellpilot/tools/memory_tools.py @@ -14,9 +14,7 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.memory.store import MemoryFormatError, MemoryStores from shellpilot.policy.risk import RiskLevel, SideEffect -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec - -ALL_PROFILES = frozenset({"supervised", "balanced"}) +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec PREVIEW_TOKENS = 2000 diff --git a/shellpilot/tools/patch.py b/shellpilot/tools/patch.py index 59e08d1..5bdef5c 100644 --- a/shellpilot/tools/patch.py +++ b/shellpilot/tools/patch.py @@ -22,13 +22,14 @@ from shellpilot.policy.command_policy import sensitive_path_reason from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.tools.base import ( + ALL_PROFILES, ToolContext, ToolResult, ToolSpec, resolve_in_workspace, workspace_display, ) -from shellpilot.tools.filesystem import ALL_PROFILES, is_binary +from shellpilot.tools.filesystem import is_binary OPERATIONS = ("replace_exact", "insert_before", "insert_after", "delete_exact") WRITE_MODES = ("create", "overwrite", "append") diff --git a/shellpilot/tools/search.py b/shellpilot/tools/search.py index 38c08b9..edd3ba1 100644 --- a/shellpilot/tools/search.py +++ b/shellpilot/tools/search.py @@ -9,8 +9,14 @@ from shellpilot.policy.command_policy import sensitive_path_reason from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.runtime.budget import truncate_to_tokens -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec, resolve_in_workspace -from shellpilot.tools.filesystem import ALL_PROFILES, _classify_path_arg, is_binary +from shellpilot.tools.base import ( + ALL_PROFILES, + ToolContext, + ToolResult, + ToolSpec, + resolve_in_workspace, +) +from shellpilot.tools.filesystem import _classify_path_arg, is_binary MAX_MATCHES = 100 SKIP_DIRS = {".git", ".venv", "node_modules", "__pycache__", ".shellpilot", ".mypy_cache"} diff --git a/shellpilot/tools/skill_tools.py b/shellpilot/tools/skill_tools.py index 9d84f75..5381129 100644 --- a/shellpilot/tools/skill_tools.py +++ b/shellpilot/tools/skill_tools.py @@ -13,9 +13,7 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.policy.risk import RiskLevel, SideEffect from shellpilot.skills.model import Skill -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec - -ALL_PROFILES = frozenset({"supervised", "balanced"}) +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec def make_skill_read_tool(skills: Sequence[Skill]) -> ToolSpec: diff --git a/shellpilot/tools/web.py b/shellpilot/tools/web.py index f3442ef..87cbaff 100644 --- a/shellpilot/tools/web.py +++ b/shellpilot/tools/web.py @@ -18,13 +18,11 @@ from shellpilot.llm.messages import ToolDefinition from shellpilot.policy.risk import RiskLevel, SideEffect -from shellpilot.tools.base import ToolContext, ToolResult, ToolSpec +from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec from shellpilot.web.errors import WebFetchError, WebSearchError from shellpilot.web.fetch import PageFetcher from shellpilot.web.search import SearchProvider -ALL_PROFILES = frozenset({"supervised", "balanced"}) - def make_web_tools(provider: SearchProvider, fetcher: PageFetcher) -> list[ToolSpec]: """Build web_search and web_fetch ToolSpecs backed by *provider* and *fetcher*. diff --git a/tests/test_config.py b/tests/test_config.py index 5860ca5..75e4a1d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1099,3 +1099,15 @@ def test_is_egressing_combines_cloud_model_and_base_url() -> None: assert is_egressing("gemma4:e4b", remote) is True # Both signals firing is still egressing. assert is_egressing("x-cloud", remote) is True + + +def test_all_profiles_matches_valid_profiles() -> None: + """ALL_PROFILES in tools.base must stay in sync with VALID_PROFILES in config.model. + + A future profile add only requires updating VALID_PROFILES; this test + catches any silent desync between the two constants. + """ + from shellpilot.config.model import VALID_PROFILES + from shellpilot.tools.base import ALL_PROFILES + + assert ALL_PROFILES == frozenset(VALID_PROFILES) From a325e97eadc3543e4bbd724a6d2b5281d50a1387 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 11:46:40 -0400 Subject: [PATCH 14/27] refactor(persistence): drop unused FileSnapshot fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit size, line_count, and taken_at were computed on every read/patch/write (including a whole-buffer newline scan and datetime.now()) but never read — only sha256 is used, by validate(). Reduce FileSnapshot to path + sha256, drop the datetime import, and sync DESIGN section 12.4 to the two fields actually retained. The read-before-write/no-stale-write contract is unchanged. --- docs/DESIGN.md | 4 ---- shellpilot/persistence/snapshots.py | 12 +----------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index d0bec24..7a60098 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -989,11 +989,7 @@ Before the model can edit an existing file, the runtime must have read that file The snapshot should include: - Resolved path. -- File size. - Content hash. -- Read timestamp. -- Line count. -- The exact text or bounded read windows provided to the model. This prevents blind edits where the model guesses file contents, overwrites concurrent changes, or writes a file it has not inspected. diff --git a/shellpilot/persistence/snapshots.py b/shellpilot/persistence/snapshots.py index a0ef450..c03638d 100644 --- a/shellpilot/persistence/snapshots.py +++ b/shellpilot/persistence/snapshots.py @@ -8,7 +8,6 @@ import hashlib from dataclasses import dataclass -from datetime import UTC, datetime from pathlib import Path @@ -20,9 +19,6 @@ def content_hash(data: bytes) -> str: class FileSnapshot: path: Path sha256: str - size: int - line_count: int - taken_at: str class SnapshotStore: @@ -32,13 +28,7 @@ def __init__(self) -> None: self._snapshots: dict[Path, FileSnapshot] = {} def record(self, path: Path, data: bytes) -> FileSnapshot: - snapshot = FileSnapshot( - path=path, - sha256=content_hash(data), - size=len(data), - line_count=data.count(b"\n") + (1 if data and not data.endswith(b"\n") else 0), - taken_at=datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%SZ"), - ) + snapshot = FileSnapshot(path=path, sha256=content_hash(data)) self._snapshots[path] = snapshot return snapshot From bbbd9b69affdb2e5ace5e18a455255e19f9d85d6 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 11:57:51 -0400 Subject: [PATCH 15/27] refactor(cli): import and constant hygiene - carry size_bytes on ImageRef so /attach reads the validated byte length instead of base64-decoding the image again just to measure it; drops the now-unused base64 import from slash.py. - root the four shared theme hexes in theme.py; banner.py and status_bar.py import them instead of each restating the literals. - hoist needlessly-deferred stdlib imports (dataclasses, json, urlsplit in slash.py; os in commands.py) to module scope; the terminal<->slash cycle-breaker import stays deferred. - env scrub uses str.startswith(tuple) directly, matching its sibling. --- shellpilot/cli/attachments.py | 2 +- shellpilot/cli/banner.py | 25 +++++++++++-------------- shellpilot/cli/commands.py | 3 +-- shellpilot/cli/input.py | 4 ++-- shellpilot/cli/slash.py | 17 ++++------------- shellpilot/cli/status_bar.py | 11 ++++------- shellpilot/cli/theme.py | 17 ++++++++++++----- shellpilot/llm/messages.py | 1 + shellpilot/tools/command.py | 6 +----- tests/test_compaction.py | 1 + tests/test_conversation.py | 3 +++ tests/test_image_messages.py | 2 +- tests/test_sessions.py | 2 +- 13 files changed, 43 insertions(+), 51 deletions(-) diff --git a/shellpilot/cli/attachments.py b/shellpilot/cli/attachments.py index 42d9d00..76fd289 100644 --- a/shellpilot/cli/attachments.py +++ b/shellpilot/cli/attachments.py @@ -39,7 +39,7 @@ def load_image(path: Path) -> ImageRef: ) sha256 = hashlib.sha256(raw).hexdigest() data_b64 = base64.b64encode(raw).decode() - return ImageRef(path=str(path), sha256=sha256, data_b64=data_b64) + return ImageRef(path=str(path), sha256=sha256, data_b64=data_b64, size_bytes=len(raw)) @dataclass diff --git a/shellpilot/cli/banner.py b/shellpilot/cli/banner.py index 53d6ad3..fbb2dd6 100644 --- a/shellpilot/cli/banner.py +++ b/shellpilot/cli/banner.py @@ -17,6 +17,7 @@ from shellpilot import __version__ from shellpilot.cli.render import _sanitize_line +from shellpilot.cli.theme import COLOR_ACCENT, COLOR_DIM, COLOR_FAINT, COLOR_WARN # --------------------------------------------------------------------------- # Jet art — locked v2 block art, embedded verbatim (do NOT read from disk). @@ -68,12 +69,8 @@ _JET_WIDTH = len(_JET_BLOCKS[0]) # 28 — every row is this wide. -# Theme color constants (aligned with shellpilot/cli/theme.py). -_COLOR_ACCENT = "#98c379" # green — local model -_COLOR_WARN = "#e5c07b" # amber — cloud model +# Banner-only color constants (shared theme colors imported from theme.py above). _COLOR_CYAN = "#7fb3c8" # section headers -_COLOR_DIM = "#6b6b6b" # sp.dim — item text -_COLOR_FAINT = "#444444" # sp.faint — hints / rules _COLOR_COCKPIT = "#080808" # near-black for cockpit cells _GRADIENT_TOP = (0x7C, 0x7C, 0x7C) # #7c7c7c — nose _GRADIENT_BOT = (0x3A, 0x3A, 0x3A) # #3a3a3a — tail @@ -131,7 +128,7 @@ def _build_jet() -> Text: def _build_left_col(model: str, *, is_cloud: bool, profile: str) -> Group: - model_style = f"bold {_COLOR_WARN if is_cloud else _COLOR_ACCENT}" + model_style = f"bold {COLOR_WARN if is_cloud else COLOR_ACCENT}" locality = "cloud" if is_cloud else "local" # The jet is centered as a fixed-width BLOCK via Align (width=_JET_WIDTH), # not per line — per-line centering skews because Rich strips trailing @@ -142,7 +139,7 @@ def _build_left_col(model: str, *, is_cloud: bool, profile: str) -> Group: Align.center(_build_jet(), width=_JET_WIDTH), Text(""), Text(model, style=model_style, justify="center"), - Text(f"{profile} · {locality}", style=_COLOR_DIM, justify="center"), + Text(f"{profile} · {locality}", style=COLOR_DIM, justify="center"), ) @@ -158,12 +155,12 @@ def _section(header: str, items: Sequence[tuple[str, str]]) -> Text: for lead, desc in items: t.append("\n") t.append(f"{lead:<{_ITEM_PAD}}", style="bold") - t.append(desc, style=_COLOR_DIM) + t.append(desc, style=COLOR_DIM) return t def _rule() -> Text: - return Text("─" * _RULE_WIDTH, style=_COLOR_FAINT) + return Text("─" * _RULE_WIDTH, style=COLOR_FAINT) def _workflow_section(skills: Sequence[str]) -> Text: @@ -171,11 +168,11 @@ def _workflow_section(skills: Sequence[str]) -> Text: t.append("Workflow skills", style=f"bold {_COLOR_CYAN}") t.append("\n") if skills: - t.append(" · ".join(skills), style=_COLOR_DIM) + t.append(" · ".join(skills), style=COLOR_DIM) else: - t.append(" · ".join(_AVAILABLE_WORKFLOW_SKILLS), style=_COLOR_DIM) + t.append(" · ".join(_AVAILABLE_WORKFLOW_SKILLS), style=COLOR_DIM) t.append("\n") - t.append("/skills to enable", style=_COLOR_FAINT) + t.append("/skills to enable", style=COLOR_FAINT) return t @@ -184,12 +181,12 @@ def _recent_section(recent_sessions: Sequence[tuple[str, str]]) -> Text: t.append("Recent sessions", style=f"bold {_COLOR_CYAN}") for label, age in recent_sessions: t.append("\n") - t.append("● ", style=_COLOR_DIM) + t.append("● ", style=COLOR_DIM) # The label is a snippet of a past session's first USER message — untrusted, # possibly-pasted input. Strip control/ANSI bytes at this render sink so a # stored escape sequence cannot repaint the terminal on boot (Group B). t.append(_sanitize_line(label)) - t.append(f" ({age})", style=_COLOR_DIM) + t.append(f" ({age})", style=COLOR_DIM) return t diff --git a/shellpilot/cli/commands.py b/shellpilot/cli/commands.py index c65e11f..2d4c85e 100644 --- a/shellpilot/cli/commands.py +++ b/shellpilot/cli/commands.py @@ -3,6 +3,7 @@ from __future__ import annotations import argparse +import os import sys from collections.abc import Sequence from pathlib import Path @@ -66,8 +67,6 @@ def run_cli(argv: Sequence[str] | None = None) -> int: def _run_config(workspace: Path, action: str) -> int: - import os - from rich.console import Console from shellpilot.cli.slash import render_config diff --git a/shellpilot/cli/input.py b/shellpilot/cli/input.py index eab9d52..62b2278 100644 --- a/shellpilot/cli/input.py +++ b/shellpilot/cli/input.py @@ -22,8 +22,8 @@ from rich.console import Console from shellpilot.cli.render import context_line -from shellpilot.cli.status_bar import COLOR_ACCENT, COLOR_WARN, status_bar -from shellpilot.cli.theme import Glyphs +from shellpilot.cli.status_bar import status_bar +from shellpilot.cli.theme import COLOR_ACCENT, COLOR_WARN, Glyphs PT_STYLE = Style.from_dict( { diff --git a/shellpilot/cli/slash.py b/shellpilot/cli/slash.py index 87bb895..906afd7 100644 --- a/shellpilot/cli/slash.py +++ b/shellpilot/cli/slash.py @@ -2,11 +2,13 @@ from __future__ import annotations -import base64 +import dataclasses +import json import os from collections.abc import Callable from enum import Enum from pathlib import Path +from urllib.parse import urlsplit from rich.console import Console from rich.table import Table @@ -283,8 +285,6 @@ def _locality_line(self, model: str) -> Text: endpoint, or 'cloud' for a -cloud model proxied through loopback); local in dim otherwise (design section 15.2). """ - from urllib.parse import urlsplit - from shellpilot.llm.ollama import is_loopback_url base_url = self._loaded.settings.model.base_url @@ -338,7 +338,6 @@ def _model(self, args: list[str]) -> None: # consent BEFORE set_model/_preload touch it. On reject: no switch, # no preload, no egress. from shellpilot.cli.terminal import _resolve_cloud_consent - from shellpilot.config.model import is_egressing base_url = self._loaded.settings.model.base_url egressing = is_egressing(name, base_url) @@ -355,8 +354,6 @@ def _model(self, args: list[str]) -> None: if self._preload is not None: self._preload(name) if egressing and self._runtime.audit is not None: - from urllib.parse import urlsplit - self._runtime.audit.write( "cloud_consent_granted", model=name, @@ -604,8 +601,6 @@ def _diff(self) -> None: self._console.print(render_diff(diff, self._glyphs)) def _profile(self, args: list[str]) -> None: - import dataclasses - from shellpilot.config.model import VALID_PROFILES settings = self._runtime.settings @@ -722,8 +717,6 @@ def _audit_memory(self, summary: str) -> None: self._runtime.audit.write("memory_update", summary=summary) def _memory_compact(self, stores: object) -> None: - import json - from shellpilot.llm.messages import Message from shellpilot.memory.store import MemoryStores, Preference from shellpilot.prompts.memory import MEMORY_COMPACT_PROMPT @@ -949,7 +942,7 @@ def _attach(self, args: list[str]) -> None: queue.stage(candidate) # Use the already-loaded ref so a file vanishing between calls can't crash. - size = len(base64.b64decode(ref.data_b64)) + size = ref.size_bytes human_size = ( f"{size / 1024 / 1024:.1f} MB" if size >= 1024 * 1024 else f"{size / 1024:.1f} KB" ) @@ -959,8 +952,6 @@ def _attach(self, args: list[str]) -> None: ) def _compact(self, args: list[str]) -> None: - import dataclasses - if args and args[0] == "status": status = self._runtime.status() budget = status.budget diff --git a/shellpilot/cli/status_bar.py b/shellpilot/cli/status_bar.py index b1e145f..0ad8c5e 100644 --- a/shellpilot/cli/status_bar.py +++ b/shellpilot/cli/status_bar.py @@ -26,15 +26,12 @@ from prompt_toolkit.formatted_text import FormattedText from shellpilot.cli.render import _abbreviate_home, _sanitize_line +from shellpilot.cli.theme import COLOR_ACCENT, COLOR_DIM, COLOR_FAINT, COLOR_WARN -# Theme color constants (hex strings; aligned with shellpilot/cli/theme.py). -# prompt_toolkit cannot read the rich Theme, so the bar mirrors the same hexes, -# the same pattern cli/banner.py uses for its rich-independent jet art. -COLOR_ACCENT = "#98c379" # green — local model / low ctx -COLOR_WARN = "#e5c07b" # amber — cloud model / mid ctx / egress emphasis +# prompt_toolkit cannot read the rich Theme, so the bar uses the shared hex +# constants from theme.py directly. COLOR_ERROR is status-bar-only (not in +# the four shared theme colors) so it remains defined here. COLOR_ERROR = "#e06c75" # red — high ctx -COLOR_DIM = "#6b6b6b" # sp.dim — ordinary segment text -COLOR_FAINT = "#444444" # sp.faint — separators # Context-utilization color thresholds (percent full): green below MID, amber # from MID up to HI, red at/above HI. diff --git a/shellpilot/cli/theme.py b/shellpilot/cli/theme.py index f5e0e97..b252eda 100644 --- a/shellpilot/cli/theme.py +++ b/shellpilot/cli/theme.py @@ -15,14 +15,21 @@ from shellpilot.config.model import Settings +# Single-source hex values for the four shared theme colors. Other modules +# (banner.py, status_bar.py) import these constants so the values can't drift. +COLOR_ACCENT = "#98c379" +COLOR_WARN = "#e5c07b" +COLOR_DIM = "#6b6b6b" +COLOR_FAINT = "#444444" + SHELLPILOT_THEME = Theme( { "sp.emph": "bold bright_white", - "sp.dim": "#6b6b6b", - "sp.faint": "#444444", - "sp.accent": "#98c379", - "sp.success": "#98c379", - "sp.warn": "#e5c07b", + "sp.dim": COLOR_DIM, + "sp.faint": COLOR_FAINT, + "sp.accent": COLOR_ACCENT, + "sp.success": COLOR_ACCENT, + "sp.warn": COLOR_WARN, "sp.error": "#e06c75", "sp.risk.high": "bold #e06c75", "sp.badge.medium": "bold white on #3a3a3a", diff --git a/shellpilot/llm/messages.py b/shellpilot/llm/messages.py index 4ae7949..860806d 100644 --- a/shellpilot/llm/messages.py +++ b/shellpilot/llm/messages.py @@ -32,6 +32,7 @@ class ImageRef: path: str # original filesystem path (display/reference) sha256: str # hex digest of the raw bytes data_b64: str # base64-encoded raw bytes (for Ollama wire encoding) + size_bytes: int # byte length of the raw image (avoids re-decoding b64 to measure) @dataclass(frozen=True) diff --git a/shellpilot/tools/command.py b/shellpilot/tools/command.py index 1ba93d0..59e7e21 100644 --- a/shellpilot/tools/command.py +++ b/shellpilot/tools/command.py @@ -75,11 +75,7 @@ def subprocess_env() -> dict[str, str]: prompts. PATH and all other variables pass through untouched so that activated-venv workflows continue to work. """ - env = { - k: v - for k, v in os.environ.items() - if not any(k.startswith(prefix) for prefix in _STRIP_PREFIXES) - } + env = {k: v for k, v in os.environ.items() if not k.startswith(_STRIP_PREFIXES)} env.update(_NON_INTERACTIVE) return env diff --git a/tests/test_compaction.py b/tests/test_compaction.py index 2573503..65a01be 100644 --- a/tests/test_compaction.py +++ b/tests/test_compaction.py @@ -132,6 +132,7 @@ def test_auto_compact_off_counts_incoming_image_tokens(tmp_path: Path) -> None: path="/tmp/img.png", sha256=hashlib.sha256(TINY_PNG).hexdigest(), data_b64=base64.b64encode(TINY_PNG).decode(), + size_bytes=len(TINY_PNG), ) runtime, ui, fake = make_runtime( tmp_path, context_tokens=4096, auto_compact=False, script=[answer("seen")] diff --git a/tests/test_conversation.py b/tests/test_conversation.py index 51689a7..d021cc8 100644 --- a/tests/test_conversation.py +++ b/tests/test_conversation.py @@ -389,6 +389,7 @@ def test_user_turn_audit_counts_images(tmp_path: Path) -> None: path=str(tmp_path / "shot.png"), sha256=hashlib.sha256(TINY_PNG).hexdigest(), data_b64=base64.b64encode(TINY_PNG).decode(), + size_bytes=len(TINY_PNG), ) fake = FakeLLM(script=[answer("with image"), answer("without image")]) ui = FakeUI() @@ -423,6 +424,7 @@ def test_run_turn_passes_images_into_user_message(tmp_path: Path) -> None: path=str(tmp_path / "sample.png"), sha256=hashlib.sha256(TINY_PNG).hexdigest(), data_b64=data_b64, + size_bytes=len(TINY_PNG), ) fake = FakeLLM(script=[answer("I see a white pixel.")]) @@ -457,6 +459,7 @@ def test_image_token_estimate_counted(tmp_path: Path) -> None: path="/tmp/img.png", sha256=hashlib.sha256(TINY_PNG).hexdigest(), data_b64=data_b64, + size_bytes=len(TINY_PNG), ) fake_no_image = FakeLLM(script=[answer("plain reply")]) diff --git a/tests/test_image_messages.py b/tests/test_image_messages.py index fa8175b..e5ead2c 100644 --- a/tests/test_image_messages.py +++ b/tests/test_image_messages.py @@ -17,7 +17,7 @@ def _make_image_ref(data: bytes, path: str = "/tmp/test.png") -> ImageRef: sha256 = hashlib.sha256(data).hexdigest() data_b64 = base64.b64encode(data).decode() - return ImageRef(path=path, sha256=sha256, data_b64=data_b64) + return ImageRef(path=path, sha256=sha256, data_b64=data_b64, size_bytes=len(data)) def stream_body(*chunks: dict[str, Any]) -> bytes: diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 6cec95d..b6f5f4f 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -152,7 +152,7 @@ def test_session_markdown_renders_transcript(tmp_path: Path) -> None: def _make_image_ref(data: bytes, path: str = "/tmp/shot.png") -> ImageRef: sha256 = hashlib.sha256(data).hexdigest() data_b64 = base64.b64encode(data).decode() - return ImageRef(path=path, sha256=sha256, data_b64=data_b64) + return ImageRef(path=path, sha256=sha256, data_b64=data_b64, size_bytes=len(data)) def test_record_message_stores_image_refs_not_bytes(tmp_path: Path) -> None: From 80569d1b23180cec13139be086ebcf764859f5a5 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 12:06:29 -0400 Subject: [PATCH 16/27] refactor(runtime): hoist invariant compaction estimate; dedupe skill decisions - compact_now computes the invariant system-token estimate once instead of rebuilding the full system prompt on every threshold check during a compaction pass (the system context cannot change while only history is dropped); the now-callerless _over_threshold is removed. - hoist the misleading lazy load_plan import to the module-level planner import (no circularity existed). - context.assemble builds its four SkillDecision records through one _skill_decision helper, factoring the five identical fields while each site keeps its own values. - assemble gates injection on the already-computed matched triggers (bool(matched) equals the old any_fires check) instead of scanning the triggers a second time; the dead _skill_should_inject helper is removed. Pure refactor: the injected system prompt and compaction behavior are unchanged. --- shellpilot/runtime/context.py | 78 +++++++++++++++--------------- shellpilot/runtime/conversation.py | 26 ++++++---- 2 files changed, 56 insertions(+), 48 deletions(-) diff --git a/shellpilot/runtime/context.py b/shellpilot/runtime/context.py index 32a456a..81cc0f5 100644 --- a/shellpilot/runtime/context.py +++ b/shellpilot/runtime/context.py @@ -18,7 +18,7 @@ from shellpilot.runtime.budget import estimate_tokens from shellpilot.skills.model import Skill, SkillResource, SkillTrigger, is_on_demand -from shellpilot.skills.triggers import TriggerContext, any_fires, fires +from shellpilot.skills.triggers import TriggerContext, fires @dataclass(frozen=True) @@ -70,11 +70,6 @@ def _skill_block_text(skill: Skill) -> str: return f"## Skill: {skill.name}\n{skill.body}" -def _skill_should_inject(skill: Skill, *, ctx: TriggerContext) -> bool: - """Trigger predicate used by both the live prompt and /skills Active column.""" - return any_fires(skill.triggers, skill.name, ctx) - - def _matched_triggers(skill: Skill, *, ctx: TriggerContext) -> tuple[SkillTrigger, ...]: return tuple(trigger for trigger in skill.triggers if fires(trigger, skill.name, ctx)) @@ -130,6 +125,27 @@ def _script_summary(skill: Skill) -> str: return f"{len(skill.scripts)} {noun} (execution unsupported)" +def _skill_decision( + skill: Skill, + *, + injected: bool, + matched: tuple[SkillTrigger, ...], + reason: str, + injected_refs: tuple[SkillResource, ...], +) -> SkillDecision: + return SkillDecision( + skill=skill.name, + root=skill.root, + injected=injected, + triggers=skill.triggers, + matched_triggers=matched, + reason=reason, + resource_summary=_resource_summary(skill, injected_refs), + script_summary=_script_summary(skill), + warnings=skill.warnings, + ) + + class ContextAssembler: """Pure (no I/O). Builds the structured system-prompt snapshot.""" @@ -168,23 +184,19 @@ def assemble( budget_blown = False for skill in all_skills: if not skill.valid: - decisions_by_id[id(skill)] = SkillDecision( - skill=skill.name, - root=skill.root, + decisions_by_id[id(skill)] = _skill_decision( + skill, injected=False, - triggers=skill.triggers, - matched_triggers=(), + matched=(), reason=f"invalid: {skill.error}", - resource_summary=_resource_summary(skill, ()), - script_summary=_script_summary(skill), - warnings=skill.warnings, + injected_refs=(), ) for skill in _ordered_valid_skills(skills): text = _skill_block_text(skill) source = skill.root matched = _matched_triggers(skill, ctx=trigger_ctx) - if not _skill_should_inject(skill, ctx=trigger_ctx): + if not matched: reason = _skill_not_injected_reason(skill, ctx=trigger_ctx) skill_blocks.append( ContextBlock( @@ -195,16 +207,12 @@ def assemble( reason=reason, ) ) - decisions_by_id[id(skill)] = SkillDecision( - skill=skill.name, - root=skill.root, + decisions_by_id[id(skill)] = _skill_decision( + skill, injected=False, - triggers=skill.triggers, - matched_triggers=matched, + matched=matched, reason=reason, - resource_summary=_resource_summary(skill, ()), - script_summary=_script_summary(skill), - warnings=skill.warnings, + injected_refs=(), ) continue references = _triggered_references(skill, ctx=trigger_ctx) @@ -220,16 +228,12 @@ def assemble( reason="skipped: skill budget", ) ) - decisions_by_id[id(skill)] = SkillDecision( - skill=skill.name, - root=skill.root, + decisions_by_id[id(skill)] = _skill_decision( + skill, injected=False, - triggers=skill.triggers, - matched_triggers=matched, + matched=matched, reason="skipped: skill budget", - resource_summary=_resource_summary(skill, ()), - script_summary=_script_summary(skill), - warnings=skill.warnings, + injected_refs=(), ) continue cumulative += group_tokens @@ -260,16 +264,12 @@ def assemble( on_demand_names.append(resource.name) if on_demand_names: readable.append((skill.name, on_demand_names)) - decisions_by_id[id(skill)] = SkillDecision( - skill=skill.name, - root=skill.root, + decisions_by_id[id(skill)] = _skill_decision( + skill, injected=True, - triggers=skill.triggers, - matched_triggers=matched, + matched=matched, reason="", - resource_summary=_resource_summary(skill, references), - script_summary=_script_summary(skill), - warnings=skill.warnings, + injected_refs=references, ) index_injected = bool(injected_names) diff --git a/shellpilot/runtime/conversation.py b/shellpilot/runtime/conversation.py index dfb5db5..f3471b0 100644 --- a/shellpilot/runtime/conversation.py +++ b/shellpilot/runtime/conversation.py @@ -27,7 +27,13 @@ from shellpilot.runtime.context import ContextAssembler, ContextSnapshot from shellpilot.runtime.events import RuntimeUI, TurnStats from shellpilot.runtime.executor import ExecutionOutcome, ToolExecutor -from shellpilot.runtime.planner import PlanManager, TaskPlan, compact_plan_state, make_plan_tools +from shellpilot.runtime.planner import ( + PlanManager, + TaskPlan, + compact_plan_state, + load_plan, + make_plan_tools, +) from shellpilot.skills.model import Skill from shellpilot.skills.triggers import TriggerContext from shellpilot.tools.images import make_view_image_tool @@ -242,8 +248,6 @@ def restore_active_plan(self, task_id: str | None) -> None: """Restore an active plan from a prior session. None → no-op.""" if task_id is None: return - from shellpilot.runtime.planner import load_plan - plan = load_plan(self._workspace, task_id) if plan is None: return @@ -416,9 +420,6 @@ def status(self) -> RuntimeStatus: history_messages=len(self._history), ) - def _over_threshold(self) -> bool: - return self.estimated_prompt_tokens() > self.budget.compact_at_tokens - def _old_region(self) -> int: """Messages before this index are compactable; the recent window is not.""" return max(0, len(self._history) - MIN_KEPT_MESSAGES) @@ -433,12 +434,19 @@ def compact_now(self) -> int: metadata live outside history and are never touched. """ changed = 0 + # System context is invariant during compaction (only self._history + # mutates), so estimate it once and re-test only the changing history sum. + system_tokens = self._context_snapshot().est_system_tokens + + def over() -> bool: + return system_tokens + self.history_token_estimate()[0] > self.budget.compact_at_tokens + # Digestion may reach everything except the in-flight exchange (last 2 # messages); snapshot staleness checks still force a fresh read before # any write, so losing exact tool text is safe. Drops below stay gated # by the wider MIN_KEPT_MESSAGES window. for index in range(max(0, len(self._history) - 2)): - if not self._over_threshold(): + if not over(): break message = self._history[index] if message.role == "tool": @@ -446,7 +454,7 @@ def compact_now(self) -> int: if digest != message.content: self._history[index] = Message(role="tool", content=digest) changed += 1 - while self._over_threshold(): + while over(): drop_index = next( (i for i in range(self._old_region()) if self._history[i].role != "user"), None, @@ -460,7 +468,7 @@ def compact_now(self) -> int: while drop_index < len(self._history) and self._history[drop_index].role == "tool": self._history.pop(drop_index) changed += 1 - while self._over_threshold(): + while over(): user_indices = [i for i, m in enumerate(self._history) if m.role == "user"] if len(user_indices) <= 1: break From 7396ec5e8d2e7cf7eb630ae53e01e979f7bed406 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 12:11:43 -0400 Subject: [PATCH 17/27] refactor(llm,web): dedupe /api/show probe and collapse dead imports - ollama.py: extract _api_show for the shared POST /api/show + swallow-all error handling; model_context_length and model_capabilities keep their own defaults (None / ()). - tools/web.py: drop the dead deferred re-imports in default_web_tools; DuckDuckGoProvider rides the existing module-level web.search import. - web/fetch.py: fold the charset default to response.encoding or "utf-8". --- shellpilot/llm/ollama.py | 25 +++++++++++++++---------- shellpilot/tools/web.py | 7 ++----- shellpilot/web/fetch.py | 4 +--- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/shellpilot/llm/ollama.py b/shellpilot/llm/ollama.py index ac56d2b..6f61102 100644 --- a/shellpilot/llm/ollama.py +++ b/shellpilot/llm/ollama.py @@ -124,18 +124,26 @@ def list_models(self) -> list[LocalModel]: for item in models ] + def _api_show(self, model: str) -> dict[str, Any] | None: + """POST /api/show; return the parsed body, or None on any HTTP/transport error.""" + try: + response = self._client.post("/api/show", json={"model": model}) + response.raise_for_status() + except httpx.HTTPError: + return None + result: dict[str, Any] = response.json() + return result + def model_context_length(self, model: str) -> int | None: """Maximum context length from model metadata, or None when undetectable. Note the Ollama context trap (design section 10.5): this is the model's maximum, NOT the runtime context. chat() must always set num_ctx. """ - try: - response = self._client.post("/api/show", json={"model": model}) - response.raise_for_status() - except httpx.HTTPError: + payload = self._api_show(model) + if payload is None: return None - info = response.json().get("model_info") or {} + info = payload.get("model_info") or {} for key, value in info.items(): if key.endswith(".context_length") and isinstance(value, int): return value @@ -147,12 +155,9 @@ def model_capabilities(self, model: str) -> tuple[str, ...]: Returns an empty tuple on any HTTP or transport error so that capability probing never crashes a session. """ - try: - response = self._client.post("/api/show", json={"model": model}) - response.raise_for_status() - except httpx.HTTPError: + payload = self._api_show(model) + if payload is None: return () - payload: dict[str, Any] = response.json() return tuple(payload.get("capabilities") or ()) def preload(self, model: str, *, keep_alive: str = "5m") -> None: diff --git a/shellpilot/tools/web.py b/shellpilot/tools/web.py index 87cbaff..7f92fc2 100644 --- a/shellpilot/tools/web.py +++ b/shellpilot/tools/web.py @@ -21,7 +21,7 @@ from shellpilot.tools.base import ALL_PROFILES, ToolContext, ToolResult, ToolSpec from shellpilot.web.errors import WebFetchError, WebSearchError from shellpilot.web.fetch import PageFetcher -from shellpilot.web.search import SearchProvider +from shellpilot.web.search import DuckDuckGoProvider, SearchProvider def make_web_tools(provider: SearchProvider, fetcher: PageFetcher) -> list[ToolSpec]: @@ -161,7 +161,4 @@ def _fetch(context: ToolContext, arguments: dict[str, Any]) -> ToolResult: def default_web_tools() -> list[ToolSpec]: """Production web tools backed by DuckDuckGoProvider and PageFetcher.""" - from shellpilot.web.fetch import PageFetcher as _PageFetcher - from shellpilot.web.search import DuckDuckGoProvider as _DuckDuckGoProvider - - return make_web_tools(_DuckDuckGoProvider(), _PageFetcher()) + return make_web_tools(DuckDuckGoProvider(), PageFetcher()) diff --git a/shellpilot/web/fetch.py b/shellpilot/web/fetch.py index c5a02a7..590c807 100644 --- a/shellpilot/web/fetch.py +++ b/shellpilot/web/fetch.py @@ -252,9 +252,7 @@ def fetch(self, url: str) -> FetchedPage: raw_bytes = b"".join(chunks) # Determine charset - charset: str = "utf-8" - if response.encoding: - charset = response.encoding + charset = response.encoding or "utf-8" body = raw_bytes.decode(charset, errors="replace") final_url = str(response.url) From 45705debbfb8a5ccd9b0134030ef1d1649f90bce Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 12:18:57 -0400 Subject: [PATCH 18/27] refactor(cli,persistence): single-stat recent(); drop DiffReveal.row_count - persistence/sessions.py: recent() derives each file's mtime once instead of stat-ing twice, removing a latent sort-vs-display divergence window. - memory/store.py: _next_id is a staticmethod (it uses no instance state). - cli/streaming.py + terminal.py: reveal() returns the long-diff predicate (a pure row-count test, independent of whether animation is enabled) so the diff is parsed one fewer time per approval; the single-caller row_count() is removed. --- shellpilot/cli/streaming.py | 20 +++++++++----------- shellpilot/cli/terminal.py | 3 +-- shellpilot/memory/store.py | 3 ++- shellpilot/persistence/sessions.py | 11 ++++++----- tests/test_streaming.py | 13 ++++++------- 5 files changed, 24 insertions(+), 26 deletions(-) diff --git a/shellpilot/cli/streaming.py b/shellpilot/cli/streaming.py index da553ea..e069836 100644 --- a/shellpilot/cli/streaming.py +++ b/shellpilot/cli/streaming.py @@ -206,21 +206,18 @@ def __init__(self, console: Console, glyphs: Glyphs, *, enabled: bool) -> None: self._glyphs = glyphs self._enabled = enabled and console.is_terminal - def row_count(self, diff_text: str) -> int: - """Number of rendered diff rows — pure, cheap, no Panel built.""" - rows, _ = _diff_rows(diff_text, self._glyphs) - return len(rows) - - def reveal(self, diff_text: str, *, max_rows: int) -> None: + def reveal(self, diff_text: str, *, max_rows: int) -> bool: """Animate a scrolling reveal of *diff_text*, then return. - No-op when motion is off, on a non-terminal, or for a diff at or below - ``ANIMATE_THRESHOLD`` rows. The caller prints the settled (capped) panel - next regardless — capping is a display choice, not part of the motion. + Returns True when the diff exceeds ``ANIMATE_THRESHOLD`` rows (a pure + row-count predicate, independent of whether animation is enabled), so the + caller can decide whether to cap the settled panel to ``WINDOW_ROWS``. + Animation itself is a no-op when motion is off or on a non-terminal. """ rows, _ = _diff_rows(diff_text, self._glyphs) - if not self._enabled or len(rows) <= self.ANIMATE_THRESHOLD: - return + long_diff = len(rows) > self.ANIMATE_THRESHOLD + if not self._enabled or not long_diff: + return long_diff total = len(rows) ticks = max(1, math.floor(self.TOTAL_DURATION / _REFRESH_SECONDS)) chunk = max(1, math.ceil(total / ticks)) @@ -245,6 +242,7 @@ def reveal(self, diff_text: str, *, max_rows: int) -> None: # scrollback and double-rendering under the settled panel. live.update("", refresh=False) live.stop() + return long_diff def _frame(self, rows: list[Text], revealed: int, max_rows: int) -> Group: """A reveal frame: the last ``max_rows`` of the rows revealed so far.""" diff --git a/shellpilot/cli/terminal.py b/shellpilot/cli/terminal.py index 9e19b3a..2e8e4de 100644 --- a/shellpilot/cli/terminal.py +++ b/shellpilot/cli/terminal.py @@ -286,10 +286,9 @@ def ask_approval(self, request: ApprovalRequest) -> ApprovalReply: self._console.print() if request.diff: cap = DiffReveal.WINDOW_ROWS - long_diff = self._diff_reveal.row_count(request.diff) > DiffReveal.ANIMATE_THRESHOLD # Long diffs scroll-reveal (motion only when enabled+TTY) then settle # into a capped window; short diffs print the full panel unchanged. - self._diff_reveal.reveal(request.diff, max_rows=cap) + long_diff = self._diff_reveal.reveal(request.diff, max_rows=cap) self._console.print( Padding( render_diff(request.diff, self._glyphs, max_rows=cap if long_diff else None), diff --git a/shellpilot/memory/store.py b/shellpilot/memory/store.py index f3dc7c8..52b3e3d 100644 --- a/shellpilot/memory/store.py +++ b/shellpilot/memory/store.py @@ -129,7 +129,8 @@ def _load(self) -> None: ) ) - def _next_id(self, prefix: str, existing: list[str]) -> str: + @staticmethod + def _next_id(prefix: str, existing: list[str]) -> str: highest = 0 for entry_id in existing: _, _, number = entry_id.partition("_") diff --git a/shellpilot/persistence/sessions.py b/shellpilot/persistence/sessions.py index 4d16e7f..ae7533f 100644 --- a/shellpilot/persistence/sessions.py +++ b/shellpilot/persistence/sessions.py @@ -75,11 +75,12 @@ def recent(directory: Path, limit: int = 3) -> list[tuple[str, float]]: """ if not directory.is_dir(): return [] - files = sorted(directory.glob("*.jsonl"), key=lambda p: p.stat().st_mtime, reverse=True) - out: list[tuple[str, float]] = [] - for path in files[:limit]: - out.append((SessionStore._session_label(path), path.stat().st_mtime)) - return out + pairs = sorted( + ((p, p.stat().st_mtime) for p in directory.glob("*.jsonl")), + key=lambda pm: pm[1], + reverse=True, + ) + return [(SessionStore._session_label(p), mtime) for p, mtime in pairs[:limit]] @staticmethod def _session_label(path: Path) -> str: diff --git a/tests/test_streaming.py b/tests/test_streaming.py index 09addc0..59a0686 100644 --- a/tests/test_streaming.py +++ b/tests/test_streaming.py @@ -486,13 +486,12 @@ def test_diff_reveal_nontty_is_noop(monkeypatch: pytest.MonkeyPatch) -> None: assert _SpyLive.instances == [] -def test_diff_reveal_row_count_is_pure() -> None: - reveal = DiffReveal(terminal_console(), GLYPHS, enabled=True) - diff = _additions_diff(30) - first = reveal.row_count(diff) - second = reveal.row_count(diff) - assert first == second == 30 - assert reveal.row_count("") >= 0 +def test_diff_reveal_long_diff_predicate_is_independent_of_enabled() -> None: + # reveal() returns True for a long diff even when animation is disabled (non-TTY), + # so the settled panel still applies the WINDOW_ROWS cap on non-interactive output. + reveal = DiffReveal(plain_console(), GLYPHS, enabled=True) + assert reveal.reveal(_additions_diff(30), max_rows=DiffReveal.WINDOW_ROWS) is True + assert reveal.reveal(_additions_diff(5), max_rows=DiffReveal.WINDOW_ROWS) is False def test_diff_reveal_long_diff_animates_and_clears_before_stop( From 7ba06e9bc7d739b2314892f44d9eb91ba634611c Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 12:25:52 -0400 Subject: [PATCH 19/27] refactor(policy): remove the self-cancelling git stash classifier pair "stash" was in GIT_READONLY_VERBS but _classify_git immediately excluded it with `and verb != "stash"`, so the membership did nothing: bare `git stash` was already classified MEDIUM via the mutating-verb fall-through, and only `git stash list`/`show` are readonly via their own special-case. Remove both the dead member and the compensating guard together (removing only the guard would wrongly make `git stash` read-only). Classification is byte-identical across every stash form. Adds a stash-classification guard test and a back-compat test pinning that the deprecated model.family field still loads. --- shellpilot/policy/command_policy.py | 3 +-- tests/test_command_policy.py | 18 ++++++++++++++++++ tests/test_config.py | 11 +++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/shellpilot/policy/command_policy.py b/shellpilot/policy/command_policy.py index f1e2305..38567a2 100644 --- a/shellpilot/policy/command_policy.py +++ b/shellpilot/policy/command_policy.py @@ -55,7 +55,6 @@ "remote", "rev-parse", "ls-files", - "stash", } ) GIT_HIGH: Final = frozenset({"reset", "clean"}) @@ -296,7 +295,7 @@ 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",)) - if verb in GIT_READONLY_VERBS and verb != "stash": + if verb in GIT_READONLY_VERBS: return CommandRisk(RiskLevel.LOW, ()) if verb == "stash" and verb_args and verb_args[0] in ("list", "show"): return CommandRisk(RiskLevel.LOW, ()) diff --git a/tests/test_command_policy.py b/tests/test_command_policy.py index 954c69a..0beb792 100644 --- a/tests/test_command_policy.py +++ b/tests/test_command_policy.py @@ -385,6 +385,24 @@ def test_git_readonly_verbs_stay_low(argv: list[str]) -> None: assert result.risk == RiskLevel.LOW, f"{argv}: {result.reasons}" +@pytest.mark.parametrize( + ("argv", "expected"), + [ + # Only `stash list`/`stash show` are read-only listings (auto-run LOW). + (["git", "stash", "list"], RiskLevel.LOW), + (["git", "stash", "show"], RiskLevel.LOW), + # Bare `git stash` and `git stash pop` mutate the working tree/index, so + # they stay MEDIUM (ask) and never auto-run under the balanced profile. + (["git", "stash"], RiskLevel.MEDIUM), + (["git", "stash", "pop"], RiskLevel.MEDIUM), + ], + ids=lambda case: str(case), +) +def test_git_stash_classification(argv: list[str], expected: RiskLevel) -> None: + result = classify_command(argv, workspace=WS) + assert result.risk == expected, f"{argv}: {result.reasons}" + + # -- option-encoded paths evade the workspace boundary (#14) ------------------- diff --git a/tests/test_config.py b/tests/test_config.py index 75e4a1d..2024910 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -102,6 +102,17 @@ def test_unknown_key_reports_dotted_key(tmp_path: Path) -> None: load_config(user_config_file=user, project_config_file=tmp_path / "missing.toml", env={}) +def test_deprecated_model_family_still_loads(tmp_path: Path) -> None: + # `family` is deprecated since v0.4.0 (ignored), but kept so an old user + # config that still sets it loads without raising — config.toml errors are + # fatal, so an unknown-key regression would break existing configs. + user = write_toml(tmp_path / "user.toml", '[model]\nfamily = "gemma"\n') + loaded = load_config( + user_config_file=user, project_config_file=tmp_path / "missing.toml", env={} + ) + assert loaded.settings.model.family == "gemma" + + def test_invalid_profile_rejected(tmp_path: Path) -> None: user = write_toml(tmp_path / "user.toml", '[runtime]\nsecurity_profile = "trusted-local"\n') with pytest.raises(ConfigError, match="trusted-local"): From d2fb52177353fc51590400ee47baab1bc896638f Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:03:05 -0400 Subject: [PATCH 20/27] fix(security): redact prefixed secret keys in structured redaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _is_sensitive_key matched only exact normalized keys, so api_key redacted but env/config-style prefixed forms (OPENAI_API_KEY, DB_PASSWORD, MY_API_KEY, the X-Api-Key header) leaked their values verbatim into the persisted session log and markdown export — those values match no value-pattern. Match the exact key OR a conventional prefixed form where the sensitive token is the final "_"-delimited segment. Deliberate conservative over-mask, consistent with the documented stance; the in-memory history is never mutated. DESIGN redaction contract updated in-commit. --- docs/DESIGN.md | 2 +- shellpilot/memory/redaction.py | 6 +++++- tests/test_redaction.py | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 7a60098..ea0405e 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1311,7 +1311,7 @@ The write tools (`write_file`, `patch_file`, section 12.5) build their preview b When the model endpoint is **remote**, the entire prompt (system prompt, AGENTS.md, memory, file contents, command output) leaves the device — the prompt itself is an exfiltration channel that no per-action approval gate intercepts. The runtime owns a single locality signal (`_is_egressing()`, which delegates to the shared `is_egressing(model, base_url)` predicate in `config/model.py` — true when the model `base_url` is not loopback **or** the model is a cloud model — `is_cloud_model(name)`, the Ollama cloud tag (`:cloud`, or a sized `-cloud` such as `:31b-cloud`, matched on the tag after the final `:`), which egresses to the provider even through a localhost Ollama proxy) and applies two controls at the one chokepoint where every **conversational** model request passes (`conversation.py` tool loop): -- **Outbound redaction (best-effort defence-in-depth, not a guarantee).** On an egressing turn, when `privacy.redact_secrets` is on, a **redacted copy** of the outbound messages is sent (`redact_secrets` on content, `redact_structure` on tool-call arguments) — the in-memory history is never mutated. A **loopback turn is sent byte-identical** (no copy, zero behaviour change). The value patterns mask common credential shapes (AWS/GitHub/Slack tokens, JWTs, bearer headers, PEM blocks, `key=value` assignments); in addition `redact_structure` masks **string and numeric (int/float, excluding `bool`) scalar values** under a **sensitive dict key** (`password`/`passwd`/`secret`/`token`/`api_key`/`apikey`/`access_key`/`accesskey`/`client_secret`/`private_key`/`access_token`/`refresh_token`/`auth_token`/`session_token`/`aws_secret_access_key`/`bearer_token`, compared case-insensitively with `-`→`_`), and the value-pattern's separator now tolerates a quote so **quoted-JSON-key forms** (`"api_key": "…"`) match. Container values (list/dict) under a sensitive key are still walked, not masked wholesale; `bool`/`None` scalars are deliberately left intact (`bool` is a Python subclass of `int`). This is a deliberate **over-mask**: a benign field literally *named* `token`/`secret` will read `[REDACTED]` in the persisted/egress copy — over-masking is preferred to leaking, and the in-memory history is never mutated. This remains regex-based and best-effort: **novel secret formats are still missed**, and **image/base64 data is not redactable here and egresses unredacted**. It reduces accidental credential leakage to a provider; it is not a confidentiality guarantee. Local-first remains the only full privacy posture. +- **Outbound redaction (best-effort defence-in-depth, not a guarantee).** On an egressing turn, when `privacy.redact_secrets` is on, a **redacted copy** of the outbound messages is sent (`redact_secrets` on content, `redact_structure` on tool-call arguments) — the in-memory history is never mutated. A **loopback turn is sent byte-identical** (no copy, zero behaviour change). The value patterns mask common credential shapes (AWS/GitHub/Slack tokens, JWTs, bearer headers, PEM blocks, `key=value` assignments); in addition `redact_structure` masks **string and numeric (int/float, excluding `bool`) scalar values** under a **sensitive dict key** (`password`/`passwd`/`secret`/`token`/`api_key`/`apikey`/`access_key`/`accesskey`/`client_secret`/`private_key`/`access_token`/`refresh_token`/`auth_token`/`session_token`/`aws_secret_access_key`/`bearer_token`, compared case-insensitively with `-`→`_`, matching either the exact key **or** a conventional prefixed form where the token is the final `_`-delimited segment — so `OPENAI_API_KEY`, `DB_PASSWORD`, and the `X-Api-Key` header all match), and the value-pattern's separator now tolerates a quote so **quoted-JSON-key forms** (`"api_key": "…"`) match. Container values (list/dict) under a sensitive key are still walked, not masked wholesale; `bool`/`None` scalars are deliberately left intact (`bool` is a Python subclass of `int`). This is a deliberate **over-mask**: a benign field literally *named* `token`/`secret` will read `[REDACTED]` in the persisted/egress copy — over-masking is preferred to leaking, and the in-memory history is never mutated. This remains regex-based and best-effort: **novel secret formats are still missed**, and **image/base64 data is not redactable here and egresses unredacted**. It reduces accidental credential leakage to a provider; it is not a confidentiality guarantee. Local-first remains the only full privacy posture. - **Egress visibility (audit).** Every egressing model request emits a `model_request` audit event (host/model/counts only — never message bodies; section 22), and every `SideEffect.NETWORK` tool call that actually runs emits a `web_egress` event. Together these record *what left the device* without recording its contents. **Accepted residual — `/memory compact`.** One model call bypasses this chokepoint: the `/memory compact` preference optimization (`SlashDispatcher._memory_compact`, `slash.py`) calls the Ollama client directly, outside the `conversation.py` tool loop, so on an egressing session it carries no `model_request` audit event and no outbound-redaction pass. This is an **accepted residual**, not a consent failure: per-session consent (§15.2) is granted before any model call, so this call is already covered by the boundary; and what it sends is stored preferences only (short behaviour strings — not history, file contents, or command output), which §16.4 already requires must not contain secrets. The open gap is audit-completeness (a silent egress the trail does not count) and the missing best-effort redaction pass. Routing `_memory_compact` through the chokepoint to close the audit-undercount gap is a planned hardening follow-up. (Context compaction — `/compact`, §20.2 — is unaffected: it makes no model call, only in-memory message dropping.) diff --git a/shellpilot/memory/redaction.py b/shellpilot/memory/redaction.py index c68be21..ad8933e 100644 --- a/shellpilot/memory/redaction.py +++ b/shellpilot/memory/redaction.py @@ -36,7 +36,11 @@ def _is_sensitive_key(key: str) -> bool: - return key.lower().replace("-", "_") in _SENSITIVE_KEYS + # Exact match, or a conventional prefixed form (OPENAI_API_KEY, DB_PASSWORD, + # MY_API_KEY) where the sensitive token is the final "_"-delimited segment(s). + # NOTE: "_"-delimited convention only; camelCase (myApiKey) is out of scope. + norm = key.lower().replace("-", "_") + return norm in _SENSITIVE_KEYS or any(norm.endswith(f"_{s}") for s in _SENSITIVE_KEYS) def _is_redactable_scalar(item: object) -> bool: diff --git a/tests/test_redaction.py b/tests/test_redaction.py index 622f08f..610ee05 100644 --- a/tests/test_redaction.py +++ b/tests/test_redaction.py @@ -90,6 +90,25 @@ def test_structure_redacts_expanded_token_keys() -> None: assert redact_structure({"access_token": "ya29.xxxxxxxxxx"}) == {"access_token": REDACTED} +def test_structure_redacts_prefixed_sensitive_keys() -> None: + # Conventional env-var/config style keys carry the secret token as the final + # "_"-delimited segment(s). Their values match no value-pattern, so the key + # matcher must catch them or they leak verbatim (e.g. into the session log). + probe = { + "OPENAI_API_KEY": "sk-supersecret123456", + "DB_PASSWORD": "hunter2secret", + "MY_API_KEY": "1234567890", + "AWS_SECRET_ACCESS_KEY": "wJalrXUtnFEMI", + } + assert redact_structure(probe) == {key: REDACTED for key in probe} + + +def test_structure_token_substring_key_not_redacted() -> None: + # A key that only contains a sensitive token mid-word is not a secret holder. + assert redact_structure({"password_strength": "weak"}) == {"password_strength": "weak"} + assert redact_structure({"token_count": 5}) == {"token_count": 5} + + def test_structure_nested_sensitive_key_redacted() -> None: nested = {"config": {"password": "hunter2pass"}} assert redact_structure(nested) == {"config": {"password": REDACTED}} From 2dfefd75c32fce761bb330b96b03791f3e055d0a Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:03:13 -0400 Subject: [PATCH 21/27] test(banner): render at a width that can't trigger console-width wrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test_tips_section_present asserts the contiguous tip string "to confirm a high-risk command"; on some Rich versions/runners the banner Panel measured wider than the 120-col render width and Rich shrank columns, wrapping the tip across lines and failing the assertion. The Panel is expand=False, so a wider console is a no-op on output where it already fits — raise the _export default width to 200 to keep the panel at its natural width. Coverage unchanged. --- tests/test_banner.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/test_banner.py b/tests/test_banner.py index 4bde9a9..b4b66ba 100644 --- a/tests/test_banner.py +++ b/tests/test_banner.py @@ -8,7 +8,12 @@ _JET_GLYPHS = ("▀", "▄", "█") -def _export(panel: Panel, *, styles: bool = False, width: int = 120) -> str: +def _export(panel: Panel, *, styles: bool = False, width: int = 200) -> str: + # Render width must stay above the banner's natural width. The panel is + # expand=False, so a generous console width is a no-op on the output, but a + # console narrower than the panel's measured width makes Rich shrink columns + # and wrap long lines (e.g. the Tips text) — a runner/Rich-version-dependent + # fragility, not a banner change. 200 keeps the panel at its natural width. console = Console(record=True, width=width, force_terminal=True) console.print(panel) return console.export_text(styles=styles) From 09f510679ea4e4a70ab5629b2ac9d8d09c640851 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:03:35 -0400 Subject: [PATCH 22/27] fix(command): hard-bound captured output to max_capture_chars The reader appended a whole readline chunk (up to MAX_READ_CHARS) whenever captured_chars was under the cap, so a single newline-less line overshot the cap by up to one chunk and left truncated=False. Slice the final chunk to the remaining budget, hard-bounding total capture to exactly max_capture_chars and setting truncated. Existing bound test tightened from soft (<= 101) to the exact bound. DESIGN output-bound note updated in-commit. --- docs/DESIGN.md | 2 +- shellpilot/tools/command.py | 14 +++++++++++--- tests/test_run_command.py | 8 ++++++-- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index ea0405e..b473fc7 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1090,7 +1090,7 @@ Execution: - Use `subprocess.Popen(argv, shell=False)`. - Stream stdout/stderr. -- Bound captured output; each read is hard-capped at 65 536 chars so a newline-less stream cannot materialise an unbounded string before the total capture cap is consulted. +- Bound captured output; each read is hard-capped at 65 536 chars so a newline-less stream cannot materialise an unbounded string before the total capture cap is consulted, and the final captured chunk is sliced to the remaining budget so total capture never exceeds the cap (and `truncated` is set). - Enforce timeout. - Kill process group on timeout. - Return exit code and captured output. diff --git a/shellpilot/tools/command.py b/shellpilot/tools/command.py index 59e7e21..82da24f 100644 --- a/shellpilot/tools/command.py +++ b/shellpilot/tools/command.py @@ -137,11 +137,19 @@ def reader() -> None: break if emit_line is not None: emit_line(line.rstrip("\n")) - if captured_chars < max_capture_chars: + # Hard-bound total capture: a single newline-less chunk can be up to + # MAX_READ_CHARS, so slice it to the remaining budget rather than + # appending it whole (which overshot the cap and left truncated=False). + remaining = max_capture_chars - captured_chars + if remaining <= 0: + truncated = True + elif len(line) > remaining: + captured.append(line[:remaining]) + captured_chars = max_capture_chars + truncated = True + else: captured.append(line) captured_chars += len(line) - else: - truncated = True thread = threading.Thread(target=reader, daemon=True) thread.start() diff --git a/tests/test_run_command.py b/tests/test_run_command.py index ddb2e4a..b87fe66 100644 --- a/tests/test_run_command.py +++ b/tests/test_run_command.py @@ -87,15 +87,19 @@ def test_timeout_kills_process_group(tmp_path: Path) -> None: def test_output_capture_is_bounded(tmp_path: Path) -> None: + # A single newline-less chunk far larger than the cap must be hard-bounded to + # exactly max_capture_chars with truncated=True — not appended whole (which + # overshot the cap by up to one chunk and left truncated=False). outcome = run_command_process( CommandRequest( - argv=[sys.executable, "-c", "print('x' * 100 + '\\n', end='');" * 1], + argv=[sys.executable, "-c", "print('x' * 1000, end='')"], cwd=tmp_path, timeout_seconds=30, ), max_capture_chars=50, ) - assert len(outcome.output) <= 101 # one buffered line may land before the cap + assert len(outcome.output) == 50 + assert outcome.truncated def test_streaming_emits_lines(tmp_path: Path) -> None: From 1a946dd185dd2ddc7f16ec3868fd806b797af72b Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:03:47 -0400 Subject: [PATCH 23/27] fix(ollama): typed errors on malformed or wrong-shaped responses list_models() called response.json() and walked .get("models") unguarded, so a non-JSON body raised JSONDecodeError and a malformed schema (e.g. {"models": "bad"}) raised AttributeError. _api_show() returned the raw json(), so a non-dict body crashed model_context_ length/model_capabilities despite their never-crash contract. Wrap list_models parse+shape -> OllamaResponseError; _api_show returns None on a JSON error or non-dict body. --- shellpilot/llm/ollama.py | 22 ++++++++++++++-------- tests/test_ollama_client.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/shellpilot/llm/ollama.py b/shellpilot/llm/ollama.py index 6f61102..8020189 100644 --- a/shellpilot/llm/ollama.py +++ b/shellpilot/llm/ollama.py @@ -117,12 +117,15 @@ def list_models(self) -> list[LocalModel]: raise OllamaUnreachableError(f"Ollama API unreachable: {exc}") from exc except httpx.HTTPError as exc: raise OllamaResponseError(f"Ollama API error: {exc}") from exc - payload: dict[str, Any] = response.json() - models = payload.get("models") or [] - return [ - LocalModel(name=str(item.get("name", "")), size_bytes=int(item.get("size", 0))) - for item in models - ] + try: + payload = response.json() + models = payload.get("models") or [] + return [ + LocalModel(name=str(item.get("name", "")), size_bytes=int(item.get("size", 0))) + for item in models + ] + except (ValueError, TypeError, AttributeError) as exc: + raise OllamaResponseError(f"Ollama API returned a malformed model list: {exc}") from exc def _api_show(self, model: str) -> dict[str, Any] | None: """POST /api/show; return the parsed body, or None on any HTTP/transport error.""" @@ -131,8 +134,11 @@ def _api_show(self, model: str) -> dict[str, Any] | None: response.raise_for_status() except httpx.HTTPError: return None - result: dict[str, Any] = response.json() - return result + try: + result = response.json() + except ValueError: + return None # malformed JSON body — metadata probing must never crash + return result if isinstance(result, dict) else None def model_context_length(self, model: str) -> int | None: """Maximum context length from model metadata, or None when undetectable. diff --git a/tests/test_ollama_client.py b/tests/test_ollama_client.py index ba11bbf..678e083 100644 --- a/tests/test_ollama_client.py +++ b/tests/test_ollama_client.py @@ -73,6 +73,36 @@ def test_list_models_tolerates_empty_payload() -> None: assert client.list_models() == [] +def test_list_models_raises_response_error_on_invalid_json() -> None: + """A 200 with a non-JSON body raises the typed error, not a raw JSONDecodeError.""" + transport = httpx.MockTransport(lambda request: httpx.Response(200, content=b"<>")) + client = make_client(transport) + with pytest.raises(OllamaResponseError): + client.list_models() + + +def test_list_models_raises_response_error_on_wrong_shape() -> None: + """A malformed schema (models is a string, not a list) raises the typed error.""" + transport = httpx.MockTransport(lambda request: httpx.Response(200, json={"models": "bad"})) + client = make_client(transport) + with pytest.raises(OllamaResponseError): + client.list_models() + + +def test_model_capabilities_empty_on_malformed_show_json() -> None: + """Metadata probing never crashes a session: malformed /api/show JSON -> ().""" + transport = httpx.MockTransport(lambda request: httpx.Response(200, content=b"not json")) + client = make_client(transport) + assert client.model_capabilities("gemma4:e4b") == () + + +def test_model_context_length_none_on_non_dict_show_json() -> None: + """A non-dict /api/show body yields None rather than an AttributeError.""" + transport = httpx.MockTransport(lambda request: httpx.Response(200, json=["not", "a", "dict"])) + client = make_client(transport) + assert client.model_context_length("gemma4:e4b") is None + + # --------------------------------------------------------------------------- # A9: preload tests # --------------------------------------------------------------------------- From 23cafd4164009c19b4fcfbf36d24517ef2745509 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:03:53 -0400 Subject: [PATCH 24/27] fix(web): don't flag exact-byte-limit fetches as truncated The byte-capture loop used len(chunk) >= remaining, so a chunk that exactly filled the remaining budget set truncated=True even though nothing was cut. Use > : an exact-fit chunk appends whole and is not truncated; if more data follows, the next iteration (remaining == 0) still flags it. remaining can never go negative. --- shellpilot/web/fetch.py | 5 ++++- tests/test_web_fetch.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/shellpilot/web/fetch.py b/shellpilot/web/fetch.py index 590c807..00742f4 100644 --- a/shellpilot/web/fetch.py +++ b/shellpilot/web/fetch.py @@ -242,7 +242,10 @@ def fetch(self, url: str) -> FetchedPage: byte_truncated = False for chunk in response.iter_bytes(): remaining = self._max_bytes - bytes_read - if len(chunk) >= remaining: + # Only truncate when a chunk EXCEEDS the remaining budget; a + # chunk that exactly fills it is complete, not truncated. If + # more data follows, the next iteration (remaining == 0) flags it. + if len(chunk) > remaining: chunks.append(chunk[:remaining]) byte_truncated = True break diff --git a/tests/test_web_fetch.py b/tests/test_web_fetch.py index a4fef31..cb70401 100644 --- a/tests/test_web_fetch.py +++ b/tests/test_web_fetch.py @@ -299,6 +299,18 @@ def test_caps_download_size_and_flags_truncation() -> None: assert page.truncated +def test_body_exactly_at_byte_limit_not_truncated() -> None: + # A body exactly max_bytes long is complete — nothing was cut, so truncated + # must be False (the off-by-one was a >= where > is correct). + body = b"A" * 100 + transport = _bytes_transport(body, content_type="text/plain; charset=utf-8") + fetcher = PageFetcher(max_bytes=100, transport=transport) + + page = fetcher.fetch("https://example.com/exact.txt") + assert page.text == "A" * 100 + assert not page.truncated + + def test_rejects_binary_content_type() -> None: fetcher = PageFetcher(transport=_bytes_transport(b"\x00\x01\x02")) with pytest.raises(WebFetchError, match="content type"): From 3fa084c34a8672cf512472d3ec20626bfaedbbad Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:48:27 -0400 Subject: [PATCH 25/27] test(banner): assert tips against flattened right column (wrap-independent) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier width bump (2dfefd7, 120->200) did not prevent the Rich line-wrap of the Tips text on all Rich versions/runners (the panel is expand=False; how Rich measures and wraps it is version-dependent), so test_tips_section_present still failed for some reviewers. Width was the wrong lever. Extract the panel's right column (slice between the interior divider and the right border) and collapse whitespace, then assert the tip strings against that — so the contiguous-phrase check survives wherever Rich wrapped. Render width reverts to 120 (no net change vs main); coverage is unchanged (still the exact phrase). --- tests/test_banner.py | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/test_banner.py b/tests/test_banner.py index b4b66ba..a4cf7bf 100644 --- a/tests/test_banner.py +++ b/tests/test_banner.py @@ -1,5 +1,7 @@ """Tests for shellpilot.cli.banner — boot-banner renderer.""" +import re + from rich.console import Console from rich.panel import Panel @@ -8,17 +10,26 @@ _JET_GLYPHS = ("▀", "▄", "█") -def _export(panel: Panel, *, styles: bool = False, width: int = 200) -> str: - # Render width must stay above the banner's natural width. The panel is - # expand=False, so a generous console width is a no-op on the output, but a - # console narrower than the panel's measured width makes Rich shrink columns - # and wrap long lines (e.g. the Tips text) — a runner/Rich-version-dependent - # fragility, not a banner change. 200 keeps the panel at its natural width. +def _export(panel: Panel, *, styles: bool = False, width: int = 120) -> str: console = Console(record=True, width=width, force_terminal=True) console.print(panel) return console.export_text(styles=styles) +def _right_column_text(text: str) -> str: + """Right-column content with wrapping flattened (jet column excluded). + + The banner is a two-column panel. How Rich wraps a long right-column line + depends on the terminal width and Rich version, and the left-column jet art + interleaves between the wrapped halves — so a contiguous-substring assertion + on the raw render is width/version-fragile. Slice each content row to its + right column (between the interior divider and the right border) and collapse + whitespace, so content checks are independent of where Rich wrapped. + """ + cols = [parts[-2] for line in text.splitlines() if len(parts := line.split("│")) >= 3] + return re.sub(r"\s+", " ", " ".join(cols)).strip() + + def _jet_left_cells(text: str) -> list[str]: """Left-column slice of each jet row, trailing space KEPT. @@ -80,11 +91,14 @@ def test_command_section_present() -> None: def test_tips_section_present() -> None: - text = _export(render_banner("m", is_cloud=False, profile="balanced")) - assert "Tips" in text - assert "for slash commands" in text - assert "to run a shell command" in text - assert "to confirm a high-risk command" in text + # Assert against the flattened right column: the Tips lines are long enough + # to wrap at narrow widths / on some Rich versions, and the contiguous-string + # check must survive that wrap (the original P1 merge-blocker). + rc = _right_column_text(_export(render_banner("m", is_cloud=False, profile="balanced"))) + assert "Tips" in rc + assert "for slash commands" in rc + assert "to run a shell command" in rc + assert "to confirm a high-risk command" in rc def test_workflow_skills_section_enabled() -> None: From 2e2d7d7f4be1a9679e483fc6903baa5cb7990673 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 13:48:38 -0400 Subject: [PATCH 26/27] fix(ollama): typed errors on malformed streaming chunk shapes _stream_chat() assumed valid-JSON stream chunks were dict-shaped, so a top-level JSON array chunk, a string message, or a string tool_calls field each raised a raw AttributeError instead of OllamaResponseError (the list_models/_api_show hardening did not cover the streaming path). Guard that the chunk and message are dicts and tool_calls is a list -> OllamaResponseError. Also make _decode_tool_call total: a non-dict element inside an otherwise-valid tool_calls list is skipped (None), the same way a dict call with a missing name is skipped, rather than leaking a raw exception. --- shellpilot/llm/ollama.py | 20 +++++++++++-- tests/test_ollama_client.py | 59 +++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/shellpilot/llm/ollama.py b/shellpilot/llm/ollama.py index 8020189..1bbe006 100644 --- a/shellpilot/llm/ollama.py +++ b/shellpilot/llm/ollama.py @@ -244,11 +244,15 @@ def _stream_chat( chunk = json.loads(line) except json.JSONDecodeError as exc: raise OllamaResponseError(f"invalid stream chunk: {line[:200]}") from exc + if not isinstance(chunk, dict): + raise OllamaResponseError(f"unexpected stream chunk shape: {line[:200]}") if chunk.get("error"): raise OllamaResponseError(str(chunk["error"])) if chunk.get("done"): done_seen = True message = chunk.get("message") or {} + if not isinstance(message, dict): + raise OllamaResponseError(f"unexpected stream message shape: {line[:200]}") token = message.get("content") or "" if token: content_parts.append(token) @@ -260,7 +264,10 @@ def _stream_chat( thinking = message.get("thinking") or "" if thinking: thinking_parts.append(thinking) - for raw_call in message.get("tool_calls") or []: + raw_calls = message.get("tool_calls") or [] + if not isinstance(raw_calls, list): + raise OllamaResponseError(f"unexpected tool_calls shape: {line[:200]}") + for raw_call in raw_calls: parsed = _decode_tool_call(raw_call) if parsed is not None: tool_calls.append(parsed) @@ -303,8 +310,15 @@ def encode_tool(tool: ToolDefinition) -> dict[str, Any]: } -def _decode_tool_call(raw: dict[str, Any]) -> ToolCall | None: - function = raw.get("function") or {} +def _decode_tool_call(raw: object) -> ToolCall | None: + # A non-dict element inside an otherwise-valid tool_calls list is one bad + # call, not a malformed stream — skip it (None) the same way a dict call + # with a missing name/arguments is skipped, rather than raising. + if not isinstance(raw, dict): + return None + function = raw.get("function") + if not isinstance(function, dict): + return None name = function.get("name") arguments = function.get("arguments") if isinstance(arguments, str): diff --git a/tests/test_ollama_client.py b/tests/test_ollama_client.py index 678e083..c11aaf1 100644 --- a/tests/test_ollama_client.py +++ b/tests/test_ollama_client.py @@ -182,6 +182,17 @@ def handler(request: httpx.Request) -> httpx.Response: return httpx.MockTransport(handler) +def _raw_stream_transport(body: str) -> httpx.MockTransport: + """MockTransport that returns a verbatim /api/chat NDJSON body (any shape).""" + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path == "/api/chat": + return httpx.Response(200, content=body.encode()) + return httpx.Response(404) + + return httpx.MockTransport(handler) + + def test_truncated_stream_raises_response_error() -> None: """A stream that closes without a done:true chunk raises OllamaResponseError.""" chunks: list[dict[str, Any]] = [ @@ -216,3 +227,51 @@ def test_done_reason_length_stream_is_accepted() -> None: client = make_client(_streaming_transport(chunks)) msg = client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) assert msg.content == "partial answer" + + +# --------------------------------------------------------------------------- +# Malformed stream-chunk shapes raise the typed error, not a raw AttributeError +# --------------------------------------------------------------------------- + + +def test_stream_non_dict_chunk_raises_response_error() -> None: + """A top-level JSON array chunk raises OllamaResponseError, not AttributeError.""" + client = make_client(_raw_stream_transport('["not", "a", "dict"]\n')) + with pytest.raises(OllamaResponseError, match="shape"): + client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + + +def test_stream_non_dict_message_raises_response_error() -> None: + """A chunk whose `message` is a string raises OllamaResponseError.""" + body = json.dumps({"message": "oops", "done": True}) + "\n" + client = make_client(_raw_stream_transport(body)) + with pytest.raises(OllamaResponseError, match="shape"): + client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + + +def test_stream_non_list_tool_calls_raises_response_error() -> None: + """A chunk whose `message.tool_calls` is a string raises OllamaResponseError.""" + body = json.dumps({"message": {"content": "", "tool_calls": "oops"}, "done": True}) + "\n" + client = make_client(_raw_stream_transport(body)) + with pytest.raises(OllamaResponseError, match="shape"): + client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + + +def test_stream_non_dict_tool_call_element_is_skipped() -> None: + """A non-dict element in a valid tool_calls list is skipped, not raised.""" + body = ( + json.dumps( + { + "message": { + "content": "hi", + "tool_calls": ["oops", {"function": {"name": "x", "arguments": {}}}], + }, + "done": True, + } + ) + + "\n" + ) + client = make_client(_raw_stream_transport(body)) + msg = client.chat("gemma4:e4b", [Message(role="user", content="hi")], num_ctx=2048) + assert msg.content == "hi" + assert [c.name for c in msg.tool_calls] == ["x"] From af5521f40b4fa3aff6a05bb0d410e51d9c5a63d3 Mon Sep 17 00:00:00 2001 From: Lavindeep Dhillon Date: Sat, 27 Jun 2026 14:04:10 -0400 Subject: [PATCH 27/27] chore(release): v0.10.1 Post-v0.10.0 hardening and cleanup: resolves a wave of external-review logic and security findings and lands behavior-preserving internal cleanups. No model-facing behavior change; a default localhost session is unchanged. --- README.md | 8 +++++--- docs/DESIGN.md | 8 ++++---- shellpilot/__init__.py | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 718471e..bb8b1e7 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ ShellPilot is developed and tested on **macOS** (Apple Silicon) and is **continu ## A session -A representative session in the default `balanced` profile (risk badges are colored chips in the terminal, shown here as text). On launch, ShellPilot v0.10.0 prints a panel banner — a block-art logo alongside Commands, Tips, Workflow-skills, and Recent-sessions sections — then drops to the prompt; the transcript below picks up there: +A representative session in the default `balanced` profile (risk badges are colored chips in the terminal, shown here as text). On launch, ShellPilot v0.10.1 prints a panel banner — a block-art logo alongside Commands, Tips, Workflow-skills, and Recent-sessions sections — then drops to the prompt; the transcript below picks up there: ```text ~/my-project · gemma4:e4b · balanced · ● local 6% ctx @@ -284,14 +284,16 @@ python scripts/benchmark_model.py --model gemma4:e4b --trials 10 ## Status and roadmap -Current release: **v0.10.0** — opt-in cloud models. Recent milestones: +Current release: **v0.10.1** — post-v0.10.0 hardening and cleanup. Recent milestones: - **v0.7.x** — Skills v2 with trigger-driven built-in guidance and read-only resources; instant high-risk approvals generated deterministically from classifier reasons. - **v0.8.x** — web-grounding quality and hardening for small local models: fetch-before-answer, discover-first query shaping, fetch-recovery, current-generation checks; planner hardening for a single end-of-plan summary and idempotent re-proposals. - **v0.9.0** — progressive disclosure: a `skill_read` tool and a readable-docs menu let skills carry depth without inflating every prompt. - **v0.10.0** — security hardening pass (policy tightening, terminal-output sanitization, DNS rebinding guard, audit/session file modes, config-key hardening, AGENTS.md TOFU); opt-in cloud models behind `[model] allow_cloud` with a per-session consent gate, fail-closed non-TTY path, best-effort outbound redaction, `cloud_consent_granted` and `model_request` audit events, honest system-prompt when egressing, and an unmistakable active-cloud indicator (☁ status bar + amber model name + `/status` locality); a persistent status bar (directory · model · profile · locality + context %); four opt-in workflow skills (debugging, verification, code-review, git-workflow); boot banner with cheat-sheet and amber/green model-locality color; streamlined one-key boot picker; reject-and-steer `[e]dit` tool/command approvals; a `!` one-shot manual-shell escape; resolved-path display in approval panels; full-width diff bars; `/config` command consolidation; approval diff scrolling reveal for long diffs. -Next up is **v0.10.1** — a full-screen TUI input dock (framed input box, queueable input, completion-menu integration) deferred from v0.10.0. Later candidates include controlled skill-script execution under its own safety design, a `trusted-local` profile, and `/undo`. +- **v0.10.1** — post-v0.10.0 hardening and cleanup: a wave of external-review logic and security fixes (classifier escalation of git mutating verbs and option-encoded paths, hard-bounded command output, `trust_env=False` on every HTTP client, broader secret redaction including prefixed and JSON-shaped keys, planner stuck-state and artifact-path fixes, Ollama stream done-sentinel and typed errors on malformed responses, session-id traversal guard, exact-byte web-fetch boundary) plus behavior-preserving internal cleanups. No model-facing behavior change — a default session is unchanged. + +Next up is a full-screen TUI input dock (framed input box, queueable input, completion-menu integration), deferred from v0.10.0. Later candidates include controlled skill-script execution under its own safety design, a `trusted-local` profile, and `/undo`. ## License diff --git a/docs/DESIGN.md b/docs/DESIGN.md index b473fc7..8cc4af5 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1,12 +1,12 @@ # ShellPilot Design -Status: Current implementation design through v0.10.0, with historical rebuild notes retained -Date: 2026-06-21 +Status: Current implementation design through v0.10.1, with historical rebuild notes retained +Date: 2026-06-27 Repository: `/Users/lavin/Projects/ShellPilot` ## 1. Purpose -This document defines ShellPilot's design as a modular, local-first Python AI harness. Early sections retain the original rebuild rationale from 2026-06-10; later release-settled sections describe the current implementation through v0.10.0. +This document defines ShellPilot's design as a modular, local-first Python AI harness. Early sections retain the original rebuild rationale from 2026-06-10; later release-settled sections describe the current implementation through v0.10.1. The rebuilt project should feel closer to a local coding and shell partner than a menu-driven chatbot. The user should be able to open one CLI conversation, ask questions, ask for project inspection, request command execution, approve plans for complex work, and use a manual shell when they want direct control. @@ -18,7 +18,7 @@ This project is a local developer productivity harness. Security-related feature ## 2. Original Repo Baseline -This section is historical context from the initial rebuild plan. The repository has since shipped the rebuilt architecture through v0.10.0, but these observations explain why the current boundaries exist. +This section is historical context from the initial rebuild plan. The repository has since shipped the rebuilt architecture through v0.10.1, but these observations explain why the current boundaries exist. The pre-rebuild repository already proved several valuable ideas: diff --git a/shellpilot/__init__.py b/shellpilot/__init__.py index 5f08291..c512b3b 100644 --- a/shellpilot/__init__.py +++ b/shellpilot/__init__.py @@ -1,3 +1,3 @@ """ShellPilot: a local-first AI shell harness.""" -__version__ = "0.10.0" +__version__ = "0.10.1"