diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e0aa8e6..d04b67d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -7,7 +7,7 @@ How to pick up work and get it merged. The shared plan lives in 1. Browse [open issues](https://github.com/Faisal-Fayaz/sidekick/issues) — they're grouped by [milestones](https://github.com/Faisal-Fayaz/sidekick/milestones) - (`v0.3.0` = now; later work parks as bullets in `ROADMAP.md`). + (later work parks as bullets in `ROADMAP.md`). 2. Comment on the issue so we don't duplicate effort, then work from a branch (`feature/` or `fix/`). 3. Starting work? Assign the issue to yourself *before* coding — the @@ -126,7 +126,9 @@ GitHub auto-generates release notes from merge subjects, so write them as - [ ] Version bumped on a `release/*` branch only (CI enforces this). - [ ] `ROADMAP.md` `Done` records the release, `Next` points past it. -- [ ] Test-count badge in `README.md` matches `pytest --collect-only`. -- [ ] Full CI green on the release PR (lint, mypy, tests × 6, badge, build). +- [ ] Full CI green on the release PR: `lint`, `test-count`, `build`, `coverage`, + `website`, tests × 6. (`coverage` is required and gated at the `fail_under` + floor in `pyproject.toml`; the old test-count *badge* gate was removed + deliberately in #319 — the count is reported, not asserted.) - [ ] After merge: release workflow success, PyPI shows the version, GitHub Release exists with artifacts. diff --git a/README.md b/README.md index a3ed372..bd25846 100644 --- a/README.md +++ b/README.md @@ -150,16 +150,16 @@ The `opencode` preset points at OpenCode Zen, opencode's gateway with a set of f | Command | What | |---|---| -| `sk` / `sk tui [--continue]` | Fullscreen chat, fresh session each launch | -| `sk chat [--continue]` | Fallback plain-text REPL (dumb terminals, screen readers, TUI issues) | -| `sk talk [-d SECS] [--stt-model base] [--device hw:2,0]` | Push-to-talk voice chat (CPU transcription, Enter to record/stop) | +| `sk` / `sk tui [--continue] [--model M] [--allow LIST] [--deny LIST]` | Fullscreen chat, fresh session each launch. `sk --version` prints the version and exits; `--cwd DIR` and `--profile NAME` work on every command | +| `sk chat [--continue] [--session S] [-y|--yes] [--model M] [--allow LIST] [--deny LIST] [--no-stream]` | Fallback plain-text REPL (dumb terminals, screen readers, TUI issues) | +| `sk talk [-d SECS] [--stt-model base] [--device hw:2,0] [--session S] [-y] [--install]` | Push-to-talk voice chat (CPU transcription, Enter to record/stop; `--install` skips the install prompt) | | `sk mic-test [-d SECS] [--device hw:2,0]` | Mic level check: peak dB + silent/quiet/good verdict | | `/sessions`, `/resume `, `/sessions delete `, `/fork [n]` | List, switch, delete, branch past sessions | | `/yolo`, `/confirm`, `/readonly` | Approval modes: auto-approve writes / ask every time / block all file writes (research mode) | | `/compact [hint]`, `/diff`, `/review` | Summarize history on demand / inspect working-tree diff / review it from inside a session | | `/plan`, `/build` | Plan mode: propose without writing (blocked writes) / back to build mode | | `/rewind [n]` | Undo an agent file edit — snapshots write/edit/delete targets (`shell` mutations are not tracked, use git for those) | -| `sk run "task" [--yes] [--plan] [--read-only] [--deny LIST] [--model auto\|fast\|smart\|name] [--json] [--bg] [--allow LIST]` | Single-shot agent run (auto-router picks the model; `--json` emits one machine-readable document + exit codes, use with `--yes` unattended; `--bg` detaches, returns a job id, notifies on completion; `--allow shell:pytest,write_file` skips prompts for listed tools; `--plan` proposes without writing) | +| `sk run "task" [--yes] [--plan] [--read-only] [--deny LIST] [--model auto\|fast\|smart\|name] [--json] [--bg] [--allow LIST] [--session S] [--no-stream]` | Single-shot agent run (auto-router picks the model; `--json` emits one machine-readable document + exit codes, use with `--yes` unattended; `--bg` detaches, returns a job id, notifies on completion; `--allow shell:pytest,write_file` skips prompts for listed tools; `--plan` proposes without writing) | | `sk jobs [-n N]` | List background jobs from `sk run --bg` | | `sk brief [-p PATH] [--smart]` | Morning digest: system + git + todos + memories, instant without LLM | | `sk digest [--force]` | Teammate pilot: brief + overnight failures, desktop nudge or log | @@ -167,10 +167,10 @@ The `opencode` preset points at OpenCode Zen, opencode's gateway with a set of f | `sk search QUERY [--session S]` | Full-text search across past transcripts | | `sk todo add/list/done/clear` | Todos | | `sk history` / `sk oops` | Shell log / explain last failure | -| `sk imagine "prompt" [--out f.png]` | Generate an image via the provider images endpoint | -| `sk export [SESSION] [--out f.md]` | Session transcript as Markdown (turns + tool calls) | +| `sk imagine "prompt" [--out f.png] [--size WxH] [--model M]` | Generate an image via the provider images endpoint | +| `sk export [SESSION] [--out f.md] [--force]` | Session transcript as Markdown (turns + tool calls). **Refuses to overwrite an existing `--out` unless `--force`** | | `sk egress [list\|allow HOST\|deny HOST\|test URL]` | Egress policy: what the model may fetch, and what it was blocked from ([docs](docs/egress.md)) | -| `sk audit [--session S] [--format md\|json]` | Compliance log: tool runs, approve/deny, local-vs-egress | +| `sk audit [--session S] [--format md\|json] [-n N]` | Compliance log: tool runs, approve/deny, local-vs-egress (`-n` clamped 1–1000) | | `sk stats [--session S] [--format md\|json]` | Usage + cost estimates from audit rows (turns, tools, tokens); pair with spend caps for BYO-key budgets | | `sk hook-install [--write]` | Bash/zsh logging hook | | `sk hooks [--check]` | List event hooks + live dry-run (see `docs/hooks.md`) | @@ -219,11 +219,18 @@ sk egress test https://example.com/x # check a URL without fetching it ``` This covers every destination the *model* can cause a fetch of: `read_url`, -`web_search` (including each search hit), and provider-supplied image URLs. With -an empty allowlist, all three are refused. Your model API endpoint and the MCP -servers you configured are unaffected — you chose those, they are not something -a prompt can talk the agent into adding. Every allow and deny is written to the -ledger with the host and reason, visible via `sk egress` and `sk audit`. +`web_search`, and provider-supplied image URLs. With an empty allowlist, all +three are refused. The allowlist is checked on the initial URL **and on every +redirect hop**, so an allowlisted host cannot redirect the agent somewhere +unlisted. `web_search` itself only requests `html.duckduckgo.com` and returns +result URLs as text — it never fetches a hit, so following one is `read_url`, +which is governed on its own. + +Your model API endpoint and the MCP servers you configured are unaffected — you +chose those, they are not something a prompt can talk the agent into adding. +Fetch decisions are written to the ledger with the host and reason, visible via +`sk egress` and `sk audit`. (Adding or removing an allowlist entry is a config +change, not a fetch, so it is not itself a ledger row.) **The ceiling, stated plainly:** this governs fetches made *through the agent's tools*. A `shell` command running `curl` is not covered — no string denylist can @@ -244,7 +251,24 @@ The eval harness (`tests/test_eval.py`) locks in every past quality bug as an of ## Config -`~/.sidekick/config.toml` (`provider`, `model`, `base_url` override, `api_key`, …). Env overrides: `SIDEKICK_PROVIDER`, `SIDEKICK_MODEL`, `SIDEKICK_BASE_URL`, `SIDEKICK_API_KEY`, `SIDEKICK_SPEND_CAP` (per-session USD cap, `0` = unlimited). Data stays home: `history.db`, `skills/`, `nudges.log`, `input_history`, `tui-errors.log`. +`~/.sidekick/config.toml` (`provider`, `model`, `base_url` override, `api_key`, …). Data stays home: `history.db`, `skills/`, `nudges.log`, `input_history`, `tui-errors.log`. + +| Env var | Effect | +|---|---| +| `SIDEKICK_PROVIDER` / `SIDEKICK_MODEL` | override the configured provider/model | +| `SIDEKICK_BASE_URL` / `SIDEKICK_API_KEY` | override the endpoint / key for this process | +| `SIDEKICK_SPEND_CAP` | per-session USD cap, `0` = unlimited | +| `SIDEKICK_REASONING_EFFORT` | reasoning effort for backends that support it | +| `SIDEKICK_PROFILE` | load a named config profile (see `sk config --profiles`) | +| `OPENCODE_API_KEY` | key for the `opencode` provider | +| `SIDEKICK_TRUST_REPO=1` | **opt in to repo-supplied slash commands** | + +> **`SIDEKICK_TRUST_REPO` — why your repo's commands do nothing.** A cloned repo +> ships its own `.sidekick/commands/*.md`, and a `!`cmd`` expansion inside one +> reaches `sh -c` with no approval prompt. Those templates are therefore ignored +> unless you set `SIDEKICK_TRUST_REPO=1` in your own shell. It is read from the +> environment and never from repo config, so a repository cannot self-authorize +> (#287). Commands in `~/.sidekick/commands/` are always loaded. **History budget:** the prompt budget bounds the **whole assembled prompt**, not just history — system prompt, auto-context, tool schemas, and a reservation for the reply are all measured and subtracted, then history gets what is left (#308). `history_budget_tokens` (default 3000) caps that share; over-budget sessions compact to a rolling summary via the current model (DB history stays complete). Lower it for small-context models. diff --git a/ROADMAP.md b/ROADMAP.md index fb3270c..19de1d0 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -24,7 +24,7 @@ How we decide what gets in: - **Zero-dep bias.** Prefer stdlib solutions (FTS5 over vectors, `argparse`-grade simplicity) so it runs on a 4GB box. - **Safety gates before features.** Writes need approval, destructive patterns hard-refused, SSRF guards on fetchers. New capabilities ship with regression tests (`tests/test_security.py`). -## Next — v0.28.0 (unplanned — propose by PR) +## Next — v0.30.0 (unplanned — propose by PR) ## Later @@ -74,6 +74,8 @@ Explicit non-goals: a cloud-hosted version, frontier feature parity, native mobi Shipped, most recent first. Details in [Releases](https://github.com/Faisal-Fayaz/sidekick/releases). +- `v0.29.0` — unparsed tool-call markup is no longer posted as an answer (#367/#368): the prose is kept, the markup is dropped, and the turn is bounded at 2 corrective retries before a plain error. Alongside it on main but **not yet published**: the `~/.sidekick` 0700/0600 fix (#301) and API-key masking (#269). +- `v0.28.0` — **burned.** Existed in `src/sk/__init__.py` for three minutes and was superseded before tagging, so the number can never be published. Listed in `BURNED_VERSIONS` in `tests/test_release_integrity.py`; a new gap now fails that test. - `v0.27.0` — **security release.** Hostile-repo RCE chain closed: project files can no longer redirect traffic (`provider`/`model`) or self-approve shell (`[project].approved_commands`), and repo-supplied commands need `SIDEKICK_TRUST_REPO=1`. Irreversible shell commands are refused *before* approval (#298), so `--yes`/`/yolo`/`--allow-writes` no longer bypass the blocklist — **behaviour change**. Protected write paths (shell rc, credential dirs, `.git/hooks`, the agent's own config) are denied (#305). Credentials redacted from the audit trail, shell log and notifications (#300). MCP gets an environment allowlist, URL validation, a fail-closed trust tier and cross-origin header protection (#302, #303). One SSRF chokepoint for every outbound fetch, closing the CGNAT/cloud-metadata gap (#304). Compaction no longer silently no-ops and no longer discards its own retained tail (#306, #307). TUI prompt no longer sent twice; no more duplicate `tool_call_id`s (#309, #310). Fixed an order-dependent test failure (#317). Tests 828 → 981; layered on #264/#265/#266 rather than duplicating them. - `v0.26.0` — security + robustness fix batch (#220–#229), QA regression suite (#231), retry breaker (#230), tester skill (#236), session-deny + edit-first (#237, #238), research delegate (#239), partial-output errors (#240), empty-toollist fallback, folding removal. - `v0.25.0` — image input (#178), JS extraction (#182), profiles (#189), TUI forgiveness batch (#161, #163–#168), dispatch write-block (#208), reasoning/control hygiene (#206). diff --git a/docs/egress.md b/docs/egress.md index a14c5b9..4621da5 100644 --- a/docs/egress.md +++ b/docs/egress.md @@ -53,9 +53,16 @@ Rules: | Path | Tool | |---|---| | A page the model asks to read | `read_url` | -| The search backend, and every hit it follows | `web_search` | +| The search backend, and every redirect hop it follows | `web_search` | | A URL a provider returns for an image | image download | +> **`web_search` fetches exactly one host.** It requests only +> `html.duckduckgo.com` and returns result titles and URLs *as text*. It never +> fetches a search hit, so a result pointing anywhere is not a governed +> destination — following one is `read_url`, which is governed on its own and on +> every redirect hop. An earlier version of this table said "every hit it +> follows", which described the threat model rather than the code. + **Not governed** — traffic you configured yourself: | Path | Why | diff --git a/docs/mcp-client.md b/docs/mcp-client.md index c2ae774..b2d96e8 100644 --- a/docs/mcp-client.md +++ b/docs/mcp-client.md @@ -33,6 +33,8 @@ env = { SQLITE_MCP_RO = "1" } | `env` | no | `{}` | extra env vars (stdio only, merged over the process env) | | `headers` | no | `{}` | extra HTTP headers (url only) | | `timeout` | no | `30` | seconds per request, clamped to 1–300 | +| `trust` | no | `untrusted` | `untrusted` or `full` — see below | +| `inherit_env` | no | `[]` | glob patterns of extra env vars a stdio server may also read from the parent process (#302) | \*Exactly one of `command`/`url`. Setting both refuses the entry (fail fast — fix the config instead of guessing). @@ -40,6 +42,39 @@ fix the config instead of guessing). Server names must match `[A-Za-z0-9_-]+` (no `__`); invalid entries are skipped. Check wiring with `sk mcp-servers` (live tool count or the error). +## `trust`: why your tools are refused + +This is the gate most people hit first, and it was previously undocumented — +so the error named a config key that appeared nowhere in this file. + +There are **three** gates on an MCP tool call, and the first one is not +approval: + +1. **`trust`** (config, default `untrusted`, fail closed) +2. **`readOnlyHint`** (tool annotation, fail closed) +3. **approval** (`--allow`, `--read-only`, `sk chat`) + +On an `untrusted` server, a tool is available only if it declares a **literal** +`readOnlyHint: true`. Anything else — a missing annotation, `readOnlyHint: "true"`, +a string, an empty annotations dict — is refused, and the error tells you to set +`trust = "full"`. + +```toml +[mcp_servers.db] +command = "/usr/local/bin/sqlite-mcp" +args = ["--db", "/home/you/app.db"] +trust = "full" # tools no longer need readOnlyHint: true +``` + +`untrusted` is the default and the right one: a spawned server inherits the +ability to do whatever the model asks of it, and only a server you wrote should +opt out of per-tool annotation. `inherit_env` has the same shape of risk — +patterns like `AWS_*` hand a stdio server credentials from your shell — so it is +also opt-in per server. + +Only `full` and `untrusted` are accepted; **anything else silently normalises to +`untrusted`** rather than erroring, so a typo fails closed. + ## How tools appear Each server tool becomes `mcp____` with the server's diff --git a/docs/plugins.md b/docs/plugins.md index 4cb9153..7ee324f 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -1,104 +1,151 @@ -# Plugin ecosystem design (closes #48, constrains #49) +# Plugins — user-defined tools How user-defined tools plug into sidekick without touching code. -Status: **design** — no implementation here; #49 implements against this doc. + +Status: **shipped**. This document describes what the code does. Where an +earlier draft of this file described intent, the difference is called out +explicitly under [Deliberate differences](#deliberate-differences) — the `params` +syntax in particular is not what that draft showed, and following the draft +silently produces a tool with no parameters. ## Goals -- A tool declared in a config/skill file behaves like a builtin: listed in +- A tool declared in a skill file behaves like a builtin: listed in `TOOLS_SCHEMA`, dispatched through `_gated_dispatch`, audited in `tool_runs`. -- Zero new dependencies. Unparseable declarations error clearly and never - brick the tool (same leniency rule as `_parse_spend_cap` in `config.py`). -- Hostile packs fail closed: a cloned skill repo is untrusted input, same - threat class as `.sidekick.toml` project files (`PROJECT_BLOCKED_KEYS`). +- Zero new dependencies. Unparseable declarations error clearly and never brick + the tool. +- Hostile packs fail closed: a cloned skill repo is untrusted input, same threat + class as `.sidekick.toml` project files (`PROJECT_BLOCKED_KEYS`). ## Non-goals -- Arbitrary code execution from packs (no Python entrypoints in v1). -- A package manager / version solver (that's #50, the registry). +- Arbitrary code execution from packs (no Python entrypoints). +- A package manager / version solver — that is the registry issue. - Daemon-side or TUI-specific tool surfaces (CLI/agent loop only). -## Manifest format (`TOOLS.md` next to `SKILL.md`) +## Where declarations live + +`TOOLS.md` next to `SKILL.md`, loaded automatically from +`~/.sidekick/skills/*/TOOLS.md`. There is **no `--tools` flag** — an earlier +draft proposed one; auto-load won because a flag is friction for no gain when +the only source is a directory the user already controls. -Packs declare tools in a `TOOLS.md` file in the skill dir, YAML-ish -frontmatter (parsed with the existing `parse_frontmatter`, no yaml dep): +## Manifest format + +YAML-ish frontmatter, parsed with the existing `parse_frontmatter` (no yaml dep): ```markdown --- tool: weather description: Current weather for a city (keyless wttr.in). approval: ask # ask | auto (auto = reads only, never writes) -kind: url-template # url-template | shell-template (v1 kinds) +kind: url-template # url-template | shell-template url: https://wttr.in/{city}?format=3 -params: - city: {type: string, required: true, max_len: 80} +params: city: string required --- -Optional long-form docs for the model (capped at 2KB, like skill bodies). +Optional long-form docs for the model (capped at 2KB). +``` + +### `params` is an inline string, not a mapping + +This is the one that bites. `params` takes a **comma-separated inline list**: + +``` +params: city: string required, units: string +``` + +A YAML block mapping does **not** work: + +```yaml +params: # ← ignored, silently + city: {type: string, required: true} ``` -Field rules: +The parser only handles `params` when the value is a string, so a mapping leaves +the parameter set empty, `{city}` is never substituted, and the tool is still +listed in the schema with no required arguments. Nothing warns. Only `string` +and `integer` are recognised; `max_len` is always 500 and is not configurable +per declaration. + +The shipped examples in `docs/examples/*/TOOLS.md` use the working inline form. | Field | Rule | |---|---| -| `tool` | `^[a-z][a-z0-9_]{1,30}$`, must not collide with builtin names (builtins win, pack tool ignored with a warning) | +| `tool` | `^[a-z][a-z0-9_]{1,30}$`; must not collide with a builtin name (builtins win, the pack tool is ignored with a warning) | | `description` | ≤200 chars, shown in the prompt index | -| `approval` | `ask` (default, joins `APPROVAL_TOOLS`) or `auto` (read-only; any write-shaped kind forces `ask`) | -| `kind` | v1: `url-template` or `shell-template` only; unknown kinds ignored with a warning | +| `approval` | `ask` (default, joins `APPROVAL_TOOLS`) or `auto` (read-only) | +| `kind` | `url-template` or `shell-template`; unknown kinds are ignored with a warning | | `url` / `cmd` | template with `{param}` slots; params validated before substitution | -| `params` | JSON-Schema-subset (`type: string|integer`, `required`, `max_len`); unknown keys ignored | +| `params` | inline `"name: type [required]"` list, comma separated | -## Entrypoints (v1 kinds) +## Entrypoints - **`url-template`** — substitutes params, then runs the *existing* - `tool_read_url` pipeline verbatim: SSRF guard (`_url_blocked`), - content-type whitelist, 1MB/15K caps, `max_redirects=3`. A plugin URL - fetcher can never be more permissive than `read_url`. + `tool_read_url` pipeline verbatim: the egress allowlist and SSRF guard, + content-type whitelist, size caps, ≤3 redirects. A plugin URL fetcher can + never be more permissive than `read_url`. + + Note that `read_url` is **egress-gated**, so a `url-template` plugin is + refused unless its host is on `egress_allow` — see `docs/egress.md`. This is + the same rule that applies to the builtin tool, not an extra plugin + restriction. + - **`shell-template`** — substitutes params, then runs the *existing* - `tool_exec` allowlist pipeline (`_check_cmd`): binary must be in - `ALLOWED_BINARIES`, metachars rejected, `find -exec/-delete` and - non-read-only `git` subcommands refused. Anything outside the allowlist - falls back to the `shell` tool, which keeps its approval gate + - hard-refusal patterns. Shell-template tools are always `approval: ask`. - -No new executor code: both kinds are thin wrappers over existing tools, -so every property test in `tests/test_properties.py` covers plugins free. - -## Sandbox rules (untrusted packs) - -1. **Name fence** — `tool` names are namespaced per pack internally - (`pack.tool`); collisions with builtins or other packs resolve to - builtin-first, then first-installed-wins, with a warning listing losers. -2. **Project-file rule** — like `PROJECT_BLOCKED_KEYS`, tool declarations - are *never* read from `.sidekick.toml` project files (global - `~/.sidekick/skills/` + explicit `--tools` paths only). A hostile repo - cannot smuggle tool definitions into your session. -3. **Budget fences** — ≤10 tools per pack, ≤30 packs total (mirrors - `MAX_FILES`/30-pack caps in `skills.py`); bodies >2KB truncated. -4. **Secret fence** — declarations carry no secrets; keys stay in keyring / - env / config (`effective_api_key`), never in pack files. -5. **Audit parity** — plugin calls log `tool_runs` rows with provider/host - like builtins, so `sk audit` / `sk stats` cover them. - -## Schema mapping - -At startup the loader compiles each valid declaration to one -`TOOLS_SCHEMA` entry (`type: function`, `function: {name, description, -parameters}`) plus a dispatcher closure over the kind pipeline above. -Invalid declarations are skipped with a warning surfaced by -`sk config --show`-style diagnostics (exact command: #49's call). - -`--allow` (#40) applies unchanged: `weather` as a bare entry allows the -whole plugin tool; `shell:` scopes keep working because shell-template -calls route through the `shell` tool identity. - -## Open questions for #49 - -1. `--tools PATH` flag vs auto-load from `~/.sidekick/skills/*/TOOLS.md` — - recommendation: auto-load (zero friction), flag for ad-hoc files. -2. Should `approval: auto` plugin tools appear in the prompt index by - default, or only on relevance like `SKILL.md` bundles? (Prompt-budget - pressure grows with every tool.) -3. Registry (#50) signature story: checksum pinning for remote packs — - defer to #50, local packs need nothing. -4. Per-pack kill switch (`enabled: false`) — cheap, include in #49? + `tool_exec` allowlist pipeline: the binary must be in `ALLOWED_BINARIES`, + metacharacters are rejected, and `find -exec/-delete` plus non-read-only + `git` subcommands are refused. + + An earlier draft said a command outside the allowlist "falls back to the + `shell` tool, which keeps its approval gate + hard-refusal patterns". **It + does not.** There is no fallback and no escalation: `tool_exec` returns its + own `Blocked: …` error string, and that error is what the model sees. The + command does not run. (Asserted in `tests/test_plugins.py`.) + + Shell-template tools are always `approval: ask`, whatever the manifest says. + +No new executor code: both kinds are thin wrappers over existing tools. + +## Sandbox rules + +1. **Name fence** — `tool` names are **not** namespaced per pack. Collisions + resolve to builtin-first, then first-installed-wins, with a warning naming + the losers. (An earlier draft described `pack.tool` namespacing; it was + never implemented, because the collision warning turned out to be enough.) +2. **Project-file rule** — tool declarations are never read from `.sidekick.toml` + project files. Only `~/.sidekick/skills/`. A hostile repo cannot smuggle tool + definitions into your session. +3. **Budget fences** — ≤10 tools per pack (`MAX_TOOLS_PER_PACK`), ≤30 packs + (`MAX_PACKS`). Bodies are capped at 2000 chars (`MAX_BODY_CHARS`) — though + note that only name and description reach the schema, so pack prose is not + sent to the model at all. +4. **Secret fence** — declarations carry no secrets; keys stay in the keyring / + env / config (`effective_api_key`). +5. **Audit parity** — plugin calls log `tool_runs` rows like builtins, so + `sk audit` and `sk stats` cover them. + +## Schema mapping and diagnostics + +At load time each valid declaration compiles to one `TOOLS_SCHEMA` entry plus a +dispatcher closure over the kind pipeline above. + +Invalid declarations are skipped **with a warning shown by `sk plugins`**. An +earlier draft said they surfaced via `sk config --show`; they do not — that +command only prints `cfg.project_warnings`. Use `sk plugins` to see +declarations that failed to load and why. + +`--allow` applies unchanged: `weather` as a bare entry allows the whole plugin +tool. + +## Deliberate differences + +Where the shipped behaviour differs from the original design document, and why: + +| Original | Shipped | Why | +|---|---|---| +| `params` as a YAML mapping | inline comma-separated string | no yaml dep; the mapping form fails silently, so the string form is the only honest option — **but it should warn** when handed a mapping, and does not | +| outside-allowlist shell → `shell` tool fallback | `tool_exec` returns a `Blocked:` error | an implicit escalation to a *more* powerful tool is a worse default than refusing. The refusal is visible; the old wording was not true | +| `pack.tool` namespacing | no namespacing, collision warning instead | namespacing made prompts unreadable; the warning is sufficient | +| `--tools PATH` flag | auto-load only | one source directory, no flag needed | +| warnings via `sk config --show` | warnings via `sk plugins` | `--show` is about project files; plugin warnings are a different concern | +| registry / `enabled: false` kill switch | not implemented | still open | \ No newline at end of file diff --git a/packaging/aur/PKGBUILD b/packaging/aur/PKGBUILD index 5b3ede0..488ad5d 100644 --- a/packaging/aur/PKGBUILD +++ b/packaging/aur/PKGBUILD @@ -1,12 +1,14 @@ # Maintainer: Irfanwani # Arch User Repository package for the sidekick-agent PyPI package. -# Publish flow: bump version in src/sk/__init__.py, tag vX.Y.Z, build + upload to AUR. -# Per release, refresh pkgver + source URL + sha256sums below from -# https://pypi.org/pypi/sidekick-agent/json (sdist url + sha256 entries). +# Publish flow: bump __version__ in src/sk/__init__.py and merge to main; the +# release workflow publishes to PyPI and creates the vX.Y.Z tag itself. Do NOT +# push a tag by hand -- on.push filters on branches, so a tag push runs no +# workflow at all. Then refresh pkgver + source URL + sha256sums below from +# https://pypi.org/pypi/sidekick-agent//json and upload to AUR. pkgname=python-sidekick-agent _pkgname=sidekick-agent -pkgver=0.26.0 +pkgver=0.29.0 pkgrel=1 pkgdesc="Local-first terminal companion agent (Ollama-powered) — CLI 'sk'" arch=("any") @@ -23,8 +25,8 @@ depends=( ) makedepends=("python-build" "python-installer" "python-wheel" "python-setuptools") optdepends=("python-faster-whisper: local voice transcription for 'sk talk'") -source=("${_pkgname}-${pkgver}.tar.gz::https://files.pythonhosted.org/packages/b3/c7/5f90094ee731258f331fe0c74c3cdeb5258515bb2357cff5389192cff89f/${_pkgname}-${pkgver}.tar.gz") -sha256sums=("bb98d0d11b5f5c9969402bfd02f11c40164cdc98c22453c833944e4e22f398f1") +source=("${_pkgname}-${pkgver}.tar.gz::https://files.pythonhosted.org/packages/00/2a/b7312453fd348ef4b39f6ad9cc592eae24dad5e7e28d4779e8f7bb313c32/${_pkgname}-${pkgver}.tar.gz") +sha256sums=("070be0416971804d53c511a9ddcdad424da2948a52674942e6df8d60b9bb2571") build() { python -m build --wheel --no-isolation diff --git a/tests/test_docs_consistency.py b/tests/test_docs_consistency.py new file mode 100644 index 0000000..bb1c37a --- /dev/null +++ b/tests/test_docs_consistency.py @@ -0,0 +1,183 @@ +"""Documentation and packaging agree with the code (#370). + +Version numbers drift across five places — `__init__.py`, the tags, +`ROADMAP.md`, `website/`, and the AUR `PKGBUILD` — and had already drifted in +every one of them: `pkgver` was three releases behind with a hardcoded sdist +hash for the wrong version, `ROADMAP.md` pointed `Next` at a version that had +already shipped, and the website's roadmap was eight releases stale. + +These assert the mechanical agreements. They deliberately do not try to check +that prose is *true* — a doc can be confidently wrong in a way no test can see, +which is why the high-severity ones were fixed by reading the code first. +""" + +from __future__ import annotations + +import pathlib +import re + +import pytest + +import sk + +ROOT = pathlib.Path(__file__).resolve().parent.parent + + +def _read(rel: str) -> str: + return (ROOT / rel).read_text() + + +@pytest.fixture(scope="module") +def version() -> str: + return sk.__version__ + + +# --- version copies --------------------------------------------------------- + + +def test_roadmap_next_points_past_the_current_version(version): + """`Next` pointing at an already-shipped version is how v0.28.0 got burned.""" + next_line = next( + (ln for ln in _read("ROADMAP.md").splitlines() if ln.startswith("## Next")), + "", + ) + assert next_line, "ROADMAP.md has no '## Next' section" + m = re.search(r"v(\d+)\.(\d+)\.(\d+)", next_line) + assert m, f"unparsable Next line: {next_line!r}" + assert m.group(0) > f"v{version}", ( + f"ROADMAP.md says {next_line!r} but v{version} is already the current " + f"version; Next must point past it" + ) + + +def test_roadmap_done_records_the_current_version(version): + done = _read("ROADMAP.md") + assert f"v{version}" in done, ( + f"ROADMAP.md 'Done' never mentions v{version}; the release checklist in " + f"CONTRIBUTING.md requires it" + ) + + +def test_roadmap_documents_the_burned_version(): + """A number that can never be published should say so where Next is derived.""" + assert "v0.28.0" in _read("ROADMAP.md") + assert "burned" in _read("ROADMAP.md").lower() + + +def test_website_next_matches_roadmap_next(): + roadmap = _read("ROADMAP.md") + tag = re.search(r"## Next — (v\d+\.\d+\.\d+)", roadmap) + assert tag, "could not read the roadmap's Next version" + assert tag.group(1) in _read("website/src/pages/Roadmap.tsx"), ( + f"website still advertises a different Next version than ROADMAP.md ({tag.group(1)})" + ) + + +def test_aur_pkgver_is_not_behind_the_source(version): + """A stale `pkgver` means `yay -Syu` keeps serving an old package, and the + hardcoded sdist hash pins a URL that only matches the old version.""" + aur = _read("packaging/aur/PKGBUILD") + m = re.search(r"^pkgver=(\S+)", aur, re.M) + assert m, "PKGBUILD has no pkgver" + assert m.group(1) == version, ( + f"PKGBUILD pins {m.group(1)} but the source is {version}; refresh it and the " + f"sdist sha256 from https://pypi.org/pypi/sidekick-agent/{version}/json" + ) + + +def test_aur_source_and_hash_agree_with_pkgver(): + """The sdist path segment and the sha256 are both version-specific. + + The pinned URL is a `files.pythonhosted.org/packages////…` path, + and that hash directory is derived from the file, so bumping `pkgver` without + refreshing the URL and the digest yields a package that cannot build. The + filename must be templated on `pkgver` (not hardcoded) so the two track. + """ + aur = _read("packaging/aur/PKGBUILD") + + source = re.search(r'^source=\("\$\{_pkgname\}-\$\{pkgver\}\.tar\.gz::(\S+?)"\)$', aur, re.M) + assert source, "PKGBUILD source line is not in the expected templated form" + url = source.group(1) + assert "${_pkgname}-${pkgver}.tar.gz" in url, f"source URL filename is not templated: {url}" + assert "files.pythonhosted.org/packages/" in url + + sha = re.search(r'^sha256sums=\("([0-9a-f]{64})"\)$', aur, re.M) + assert sha, "PKGBUILD needs a 64-hex sha256sums entry for the pinned sdist" + # A literal version anywhere outside pkgver means one of the two was edited + # without the other. + stray = re.findall(r"sidekick[_-]agent-(?!\$\{)(\d+\.\d+\.\d+)", aur) + assert not stray, ( + f"PKGBUILD hardcodes sdist version(s) {stray} outside ${{pkgver}}; refresh " + f"pkgver, the URL path and the sha256 together" + ) + + +# --- documented behaviour that does not exist ------------------------------- + + +def test_mcp_doc_documents_the_keys_that_refuse_tools(): + """Default trust is untrusted, and a refusal tells the user to set it. A key + named only in an error message is undiscoverable.""" + doc = _read("docs/mcp-client.md") + assert "`trust`" in doc, "docs/mcp-client.md has no `trust` key documented" + assert "inherit_env" in doc, "docs/mcp-client.md has no `inherit_env` key documented" + assert "untrusted" in doc and "readOnlyHint" in doc + + +def test_plugins_doc_is_not_written_as_a_design_document(): + plugins_doc = _read("docs/plugins.md") + assert "Status: **shipped**" in plugins_doc, ( + "docs/plugins.md still reads as an unimplemented design doc" + ) + assert "#49 implements against this doc" not in plugins_doc + + +def test_plugins_doc_shows_the_param_form_the_parser_accepts(): + """`params` only parses from an inline string. Documenting the mapping form + produced a tool whose `{slots}` were never substituted, with no warning.""" + plugins_doc = _read("docs/plugins.md") + assert re.search(r"^params: \w+: (string|integer)", plugins_doc, re.M), ( + "docs/plugins.md does not show the inline params form the parser accepts" + ) + # And the shipped examples must use it, since the README points users there. + for ex in (ROOT / "docs" / "examples").glob("*/TOOLS.md"): + text = ex.read_text() + assert not re.search(r"^params:\s*$", text, re.M), ( + f"{ex} uses the mapping form, which the parser ignores" + ) + + +def test_plugins_doc_does_not_promise_a_shell_fallback(): + """There is no escalation from tool_exec to the shell tool; the refusal is + the whole point.""" + body = _read("docs/plugins.md").split("## Deliberate differences")[0] + assert "falls back to the `shell` tool, which keeps its approval gate" not in body + + +def test_egress_doc_no_longer_claims_something_false(): + egress_doc = _read("docs/egress.md") + assert "on every redirect hop" in egress_doc + # It must not claim the search tool follows result links. + assert "every hit it follows" not in egress_doc + assert "including each search hit" not in _read("README.md") + + +def test_readme_documents_trust_repo(): + """An undocumented security gate means a user cannot find out why their + repo's commands silently do nothing.""" + readme = _read("README.md") + assert "SIDEKICK_TRUST_REPO" in readme + + +def test_readme_documents_the_version_flag(): + assert "sk --version" in _read("README.md") + + +def test_contributing_does_not_reference_removed_gates(): + """Both were deleted deliberately; the checklist still asked for them, so a + release PR would have been judged against gates that no longer exist.""" + contributing = _read("CONTRIBUTING.md") + assert "Test-count badge in `README.md` matches" not in contributing + assert "tests × 6, badge, build" not in contributing + # The checklist must name coverage, which is now a required check. + assert "coverage" in contributing diff --git a/website/src/data/content.ts b/website/src/data/content.ts index 72dc275..ae2682a 100644 --- a/website/src/data/content.ts +++ b/website/src/data/content.ts @@ -312,6 +312,9 @@ export const FAQS = [ export const ROADMAP_NEXT: { v: string; items: string[] }[] = []; export const ROADMAP_DONE = [ + 'v0.29.0 — unparsed tool-call markup no longer posted as an answer (#367); MCP tool lists cached per connection (#314)', + 'v0.28.0 — burned: bumped then superseded before tagging, never publishable', + 'v0.27.0 — security release: hostile-repo RCE chain closed (#298), protected write paths (#305), credential redaction (#300), MCP trust tier (#302/#303), SSRF chokepoint (#304)', 'v0.21.0 — cross-provider picker (#139), theme pack: opencode/dracula/tokyonight + F2 picker (#138)', 'v0.20.0 — build/plan modes (#131), TUI model picker (#130)', 'v0.19.0 — agent-capability batch: read-only mode (#98), session slash parity (#100), checkpoints + /rewind (#103), project memory (#105), MCP client (#107)', diff --git a/website/src/pages/Roadmap.tsx b/website/src/pages/Roadmap.tsx index 0e78d14..4bf130b 100644 --- a/website/src/pages/Roadmap.tsx +++ b/website/src/pages/Roadmap.tsx @@ -17,7 +17,7 @@ export default function Roadmap() {
- next — v0.21.0 · unplanned, propose by PR + next — v0.30.0 · unplanned, propose by PR {ROADMAP_NEXT.map((r) => (

{r.v}