v0.10.1 — post-v0.10.0 hardening and cleanup#4
Merged
Conversation
…ff auto-run 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 <name>, 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.
A newline-less stream (e.g. `head -c <huge> /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.
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.
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.
…an workspace 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.
… traversal - _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.
… 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.
…uild 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.
…r 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.
- §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.
…ity findings Hardens the command classifier read-path (git --output/option-encoded paths, reader-exec boundary), bounds command output per read, isolates httpx from ambient proxy env, broadens secret redaction (key-named and JSON-shaped), fixes planner finalize-on-skipped-final-step and artifact path pinning, rejects truncated Ollama streams, guards session-id read traversal, makes CLI turns interrupt-resilient and validates --resume before consent, rebuilds project memory on /cwd, counts incoming image tokens, hides in-workspace secrets in write/patch previews, validates uddg redirect schemes, clamps max_results, and floors reserved builtin names. DESIGN updated throughout.
…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.
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.
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).
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.
- 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.
…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.
- 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".
…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.
"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.
Removes dead code and drift hazards across the codebase without changing any behavior, safety classification, or the gemma4 baseline prompt: loader.py path/traversable dedup, ALL_PROFILES rooted once in tools/base, dead FileSnapshot fields, theme-color and deferred-import hygiene, invariant compaction-estimate and SkillDecision dedup, /api/show probe dedup, single-stat recent(), DiffReveal parse reduction, and the self-cancelling git stash classifier pair.
_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.
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.
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.
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.
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.
…ndent) 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).
_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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
v0.10.1 — post-v0.10.0 hardening and cleanup
A patch release on top of v0.10.0. Resolves a wave of external-review logic and security findings, adds robustness fixes, and lands behavior-preserving internal cleanups. No model-facing behavior change — a default localhost session is unchanged.
External-review logic & security fixes
trust_env=Falseon every httpx client (ollama / fetch / search) so an ambient proxy env var can't reroute traffic.OPENAI_API_KEY,DB_PASSWORD,MY_API_KEY).donesentinel); typedOllamaResponseErrors on malformed or wrong-shaped responses, including streaming chunk shapes (non-dict chunk / message /tool_calls); a non-dict element inside a validtool_callslist is skipped, not raised.--resume— reject any session id whose resolved parent escapes the sessions directory (path-traversal guard).--resumebefore preload./cwd; image-token gate; write-preview placeholder; reserved-name union; uddg redirect-scheme validation;max_resultsclamp; web fetch no longer flags exact-byte-limit bodies as truncated.Cleanups (behavior-preserving)
/api/showprobe + skill-decision dedup, loader path/traversable dedup, and import/constant hygiene. No behavior change.Verification
ruff check,ruff format --check,mypy shellpilot --strict,pytest— 1255 passing.__version__→ 0.10.1, README roadmap, DESIGN status line.