diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab5244a..9191a5d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,10 @@ on: branches: [main] pull_request: +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + jobs: check: runs-on: ${{ matrix.os }} @@ -16,9 +20,22 @@ jobs: os: [ubuntu-latest, macos-latest, windows-latest] steps: - uses: actions/checkout@v4 + - uses: oven-sh/setup-bun@v2 with: bun-version: 1.3.14 + + # bun install --frozen-lockfile re-resolves nothing but still pays the network + # cost of a cold store; cache the store so a no-change run skips it. Key is per + # OS and lockfile, so a dependency bump or a new OS each gets a fresh cache. + - name: Cache the bun install store + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: bun-${{ runner.os }}-${{ hashFiles('bun.lock') }} + restore-keys: | + bun-${{ runner.os }}- + - run: bun install --frozen-lockfile - run: bun run typecheck - run: bun test diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index e28e63b..1b07bd0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,9 +10,16 @@ on: type: boolean default: true +concurrency: + group: release-${{ github.ref }} + cancel-in-progress: false + permissions: contents: write +env: + BUN_VERSION: 1.3.14 + jobs: verify: runs-on: ubuntu-latest @@ -20,7 +27,14 @@ jobs: - uses: actions/checkout@v4 - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.14 + bun-version: ${{ env.BUN_VERSION }} + - name: Cache the bun install store + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: bun-${{ runner.os }}-${{ hashFiles('bun.lock') }} + restore-keys: | + bun-${{ runner.os }}- - run: bun install --frozen-lockfile - run: bun run typecheck - run: bun test @@ -32,15 +46,26 @@ jobs: - uses: actions/checkout@v4 - uses: oven-sh/setup-bun@v2 with: - bun-version: 1.3.14 + bun-version: ${{ env.BUN_VERSION }} + - name: Cache the bun install store + uses: actions/cache@v4 + with: + path: ~/.bun/install/cache + key: bun-${{ runner.os }}-${{ hashFiles('bun.lock') }} + restore-keys: | + bun-${{ runner.os }}- - run: bun install --frozen-lockfile # Bun cross-compiles every target from one host, so no build matrix is needed. # release.ts also fails the build when the tag and src/version.ts disagree. - run: bun run release - - name: Check the binary reports the right version + - name: Check the binaries report the right version and are non-empty run: | + for f in dist/release/shiro-linux-x64 dist/release/shiro-linux-arm64 \ + dist/release/shiro-darwin-x64 dist/release/shiro-darwin-arm64; do + [ -s "$f" ] || { echo "$f is missing or empty"; exit 1; } + done chmod +x dist/release/shiro-linux-x64 ./dist/release/shiro-linux-x64 --version ./dist/release/shiro-linux-x64 --version | grep -q "$(bun -e 'console.log((await import("./src/version.ts")).VERSION)')" @@ -66,12 +91,21 @@ jobs: name: shiro-binaries path: dist/release + # Release body: a short summary plus the artifact inventory, so the release + # page reads like a hand-written one instead of the raw PR list. The changelog + # is the prose; scripts/make-release-notes.ts extracts the section for this tag + # and fails if the heading is missing, rather than publishing an empty body. + - name: Compose release notes + env: + RELEASE_TAG: ${{ github.ref_name }} + run: bun run scripts/make-release-notes.ts + - name: Publish the release env: GH_TOKEN: ${{ github.token }} run: | gh release create "${{ github.ref_name }}" \ --title "shiro-neko ${{ github.ref_name }}" \ - --generate-notes \ + --notes-file release_notes.md \ $([[ "${{ github.ref_name }}" == *-* ]] && echo --prerelease) \ dist/release/* diff --git a/AUDIT.md b/AUDIT.md index 9e2cec9..928cd9a 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -1,73 +1,97 @@ # Audit -Checklist from a full audit of the codebase, run against `main` at `a22d8e1` ("release 0.1.0-beta.5"). +Checklist from a full audit of the codebase, run against `main` at `ffa9a02` ("release 1.0.0"). The greps cover every file under `src/`, `test/`, `docs/`, `.github/workflows/`, and `scripts/`. Nothing here is a fix — it is a list. Items already tracked in `TODO.md` or `ROADMAP.md` say so; -untracked items are marked **not yet tracked**. +untracked items are marked **not yet tracked**. Items checked off were resolved after the audit, +in the same working tree. + +The three bugs and the two untracked doc-drift items from the beta.5 audit are all fixed; this pass +also cleared the dead method, the three stale doc lines, and every UI coverage gap that remained — +`cli.tsx`, `Onboard.tsx`, `panel-bodies.ts`, `buses.ts`, and `PromptInput.tsx` — see +[section B](#b-bugs), [section D](#d-documentation-drift-untracked), and +[section E](#e-test-coverage-gaps). --- ## A. Clean findings (verified, no action needed) -- [x] **No TODO/FIXME/HACK markers in `src/`.** All 26 matches are false positives: - placeholder attributes, the `TODO_MARK` export in `src/notebook.ts` (a literal string - ingredient of the todos feature), and "later" in prose. +- [x] **No TODO/FIXME/HACK markers in `src/`.** Seven matches, all false positives: the + `TODO_MARK` export and its uses (`src/notebook.ts:120`, `src/ui/transcript.ts:1,201,203`, + `src/ui/Panels.tsx:4,34`) and a `TODO` inside the `commit` skill's prose + (`src/skills-md/commit.md:21`). Zero real markers. - [x] **No `as any` / `@ts-ignore` / `@ts-expect-error` / `@ts-nocheck` / `: any` in `src/`.** - Zero matches. The project's own `docs/development.md` rule is being kept. -- [x] **No silently swallowed errors in `src/`.** Every `catch` was reviewed: - - `src/registry.ts:116,155` — JSON parse failures become descriptive errors. - - `src/registry.ts:120-123,159-162` — zod schema validation with named failure reasons. + One grep hit and it is the word "any" in a skill's prose (`src/skills-md/readme.md:36`). + Zero casts. The project's own `docs/development.md` rule is being kept. +- [x] **No empty catch blocks and no silently swallowed errors in `src/`.** `catch {}` and + `catch (e) {}` match zero times. 83 `catch` sites were grepped and the named ones reviewed: + - `src/tools.ts:81-82` — a batch read failure is reported in place (`[unreadable: ...]`). + - `src/tools.ts:463` — a stat probe returns `false` (sentinel, not a swallow). + - `src/tools.ts:501` — `rg` unavailable → `undefined`, falls back to the JS grep. + - `src/tools.ts:551,849` — an unreadable file is skipped during a scan (deliberate). + - `src/tools.ts:630` — `taskkill` absent → plain kill (commented). + - `src/tools.ts:795,889` — stat / JSON failure → a descriptive `Error`. + - `src/subagent.ts:248` — usage unavailable on an errored run (commented); `:251` reports + the error **and rethrows** (never swallowed). + - `src/registry.ts` — parse failures become descriptive errors; the index is schema-validated. - `src/plugins.ts:59-63` — a throwing plugin hook fails **closed** (blocks the call). - - `src/subagent.ts:233-237` — errors are reported and rethrown (never swallowed). - - `src/tools.ts:80-82` — a batch read failure is reported in place, not thrown. - - `src/headless.ts` serialization flattens `Error` before `JSON.stringify` (would emit `{}`). - - `src/ui/App.tsx` — all 14 catch blocks surface the message in the UI. - - `src/plugins-builtin.ts:213-215` — the only quiet `catch`, and it is deliberate, - commented ("a missing binary is not worth interrupting the turn over"). -- [x] **No skipped tests.** No `.skip`, `xit`, or `xdescribe` in `test/` (one match was - `process.exit(` containing "xit("). + - `src/ui/App.tsx` — all 17 catch blocks surface the message in the UI. + - `src/plugins-builtin.ts` — the one quiet catch (a missing formatter binary) is deliberate + and commented. +- [x] **No skipped tests.** No `.skip`, `xit`, or `xdescribe` in `test/`. - [x] **Registry fetches are size-capped and schema-validated.** `src/registry.ts` caps - content-length and body bytes (`fetchText`, lines 100-108), validates the index with - `indexSchema` (line 120), and regex-validates every plugin manifest pattern (line 167). -- [x] **Version/tag consistency is enforced twice.** `scripts/release.ts` refuses a build when - the tag and `src/version.ts` disagree (line 129), and the release workflow asserts the - built binary prints the expected version (`.github/workflows/release.yml:42-46`). -- [x] **Install scripts match the build targets.** `test/ci.test.ts:85-101` iterates every - `TARGETS` entry from `scripts/release.ts` and asserts the shell/PowerShell installers - fetch exactly those asset names. -- [x] **`.env`/`.pem` are refused on read;** the default permission table - (`src/permission.ts:175-186`) matches the approvals banner in `README.md`. Unknown tools - (MCP, plugins) default to `ask` rather than allow (line 235-236), so `mcp__*` needs no - explicit rule. -- [x] **`--yolo` cannot bypass the guard plugin.** Defaults fold `ask` into `allow` but never - touch `deny` (`src/permission.ts:211-213`), and the guard refuses destructive commands - in `beforeToolCall`, ahead of any approval. -- [x] **Pinned toolchain.** Both workflows pin `bun-version: 1.3.14`, and `test/ci.test.ts:48-52` - fails if the pin ever disagrees with the local `Bun.version`. -- [x] **All three platforms in CI.** `ci.yml` runs the suite on ubuntu, macos, windows + content-length and body bytes (`fetchText`, lines 98-109), validates the index with + `indexSchema` (lines 120-123), and regex-validates every plugin manifest pattern + (lines 164-173). +- [x] **Version consistency is enforced three times.** `package.json` vs `src/version.ts` + (`scripts/release.ts:60-64`), the release tag vs `VERSION` via `GITHUB_REF_NAME` + (`scripts/release.ts:66-70`), and the built binary's own `--version` output in CI + (`.github/workflows/release.yml:42-46`). Both files currently read `1.0.0`. +- [x] **Install scripts match the build targets.** `test/ci.test.ts` iterates every `TARGETS` + entry from `scripts/release.ts:13-19` and asserts the shell/PowerShell installers fetch + exactly those asset names (~lines 89-94), plus checksum verification (~74-78) and version + pinning / target directory (~81-87). +- [x] **`.env`/`.pem` are refused on read; writes and commands ask.** The default permission + table (`src/permission.ts:175-186`) matches the approvals banner in `README.md:72`. Unknown + tools (MCP, plugins) default to `ask` rather than allow (`src/permission.ts:235-236`), so + `mcp__*` needs no explicit rule. +- [x] **`--yolo` cannot bypass the guard plugin.** `check()` returns a `deny` before the `yolo` + fold (`src/permission.ts:282`), the fold itself only touches `ask` (`:293`), and the guard + plugin refuses destructive commands in `beforeToolCall`, ahead of any approval. +- [x] **Pinned toolchain.** Both workflows pin `bun-version: 1.3.14` + (`.github/workflows/ci.yml:21`, `.github/workflows/release.yml:23,35`), and + `test/ci.test.ts:54` fails if the pin ever disagrees with the local `Bun.version`. +- [x] **All three platforms in CI.** `ci.yml:16` runs the suite on ubuntu, macos, windows (required — the tools shell out to `rg`, git, and a platform shell). --- ## B. Bugs -- [ ] **`src/ui/App.tsx:613` — formatting glitch.** The `}` closing the `try` is jammed onto the - same line as the preceding statement: - `push({ kind: 'info', text: await hooks.summarizeMemory() }); } catch (e) {` - Cosmetic only, but it is the kind of blemish left by an unformatted edit and reads as a - slip. **not yet tracked** -- [ ] **`.github/workflows/release.yml` — the `dry_run` input is dead.** `workflow_dispatch` - declares `inputs.dry_run` (default `true`) but no step ever reads it. Nothing consults the - value, so `dry_run=false` changes nothing, and because the `publish` job gates on - `startsWith(github.ref, 'refs/tags/v')`, a manual run can never publish regardless of the - input. Either wire the input into the `publish` `if`, or delete it and let the tag-only - gate be the whole story. **not yet tracked** -- [ ] **`README.md` says "Nineteen built-in tools" — it is now twenty.** `git_commit_message` - (shipped in beta.5 via `src/commit.ts` + `cli.tsx:281`) is a built-in tool, and - `TOOL_SETS.git` carries it (`src/tools-git.ts:189`). The count is one short; - `docs/tools.md` already says "twenty" (line 63), so README is the stale one. - **not yet tracked** +Fixed — the three from the beta.5 audit, the dead-code finding this audit surfaced, and a cursor +rendering bug found from a screenshot afterwards. + +- [x] **`src/ui/PromptInput.tsx` — the cursor was drawn with hand-written SGR escapes.** `invert()` + built the caret by pasting `\u001B[7m`/`\u001B[27m` into the text string + (`invert(placeholder.slice(0, 1))`, and the same for the character under the cursor). Ink + measures string content as printable columns, so those escapes were counted as text and the + rest of the line was written one cell to the right — the orphaned first letter of the + placeholder (`t ype to queue for the next turn…`), and a corrupted cell wherever the line + wrapped. Replaced with Ink's `inverse` prop in all three render paths. The old tests passed + *because* they asserted on the escape-producing helper; they now assert the rendered text is + free of escapes, which is the property that actually matters. +- [x] **`src/ui/App.tsx` — memory-command formatting glitch (was line 613).** The `}` closing the + `try` is now on its own line; the block reads cleanly at `src/ui/App.tsx:636-646`. +- [x] **`.github/workflows/release.yml` — the `dry_run` input is no longer dead.** It is wired + into the `publish` job's `if`: a tag push always publishes, a manual dispatch publishes only + when `dry_run` is unchecked (`release.yml:56-60`). +- [x] **`README.md` — tool count was stale, now correct.** It says "Forty-one built-in tools" + (`README.md:116`) and "41 built-in tools" (`README.md:164`), matching `docs/tools.md:49`. +- [x] **`src/permission.ts` — `granted_` was dead code, now deleted.** A public method with a + trailing underscore (the convention here is a `_`-*prefix* for an unused parameter, not a + suffix). It returned the session's granted patterns and was called nowhere — not in `src/`, + not in `test/`. Removed; `check()` and `grant()` are the live surface. --- @@ -75,98 +99,138 @@ untracked items are marked **not yet tracked**. ### TODO.md "Now" — next up -- [ ] **Summarize the pruned span.** Compaction drops messages and tells the model nothing, so a - decision from earlier in the session can be contradicted. (TODO.md `## Now`, first item) -- [ ] **A spend ceiling.** `maxSpendUsd` in config, warn at 80%, refuse the next turn at 100%, - headless exits non-zero naming the ceiling. Nothing stops a looping headless run today. - (TODO.md `## Now`) -- [ ] **A cheaper model for subagents.** `subagentModel` in config; an `explore` subagent is - search, not reasoning, and today pays the parent's per-token rate. (TODO.md `## Now`) +- [ ] **Summarize the pruned span.** Compaction keeps the model's memory of a turn but tells it + nothing about the messages it dropped, so a decision from earlier in the session can be + contradicted with confidence. (TODO.md `## Now`; ROADMAP `## Next` "Lossless-enough + compaction" — same work) - [ ] **Hot-reload an installed entry.** `/registry add` writes the file and says restart; the - skill catalogue and guard chain are assembled at boot. (TODO.md `## Now`) + skill catalogue and the guard chain are assembled at boot. (TODO.md `## Now`) ### TODO.md "Next" -- [ ] **MCP without the schema tax.** Twenty MCP tools ≈ 2,750 tokens of schema per request; - `toolSets` does not gate them. Plan: `mcp_list` / `mcp_inspect` / `mcp_call` meta-tools, - prompt names servers not schemas. (TODO.md `## Next`; ROADMAP `## Next` + `## Later`) -- [ ] **Custom commands from a file.** `.shiro/commands/*.md`, `$ARGUMENTS`, `$1`, - `` !`cmd` `` shell substitution with the guard applied. (TODO.md `## Next`; ROADMAP `## Next`) -- [ ] **Derive the tool-name lists.** `TOOL_SETS` and `MUTATING_TOOLS` are hand-maintained; a - tool added to one and forgotten in the other is a silently ungated write. (TODO.md - `## Next`; ROADMAP `## Next` "Derived tool metadata") +- [ ] **MCP without the schema tax.** Every MCP tool's schema is in the prompt on every request + and `toolSets` does not gate them; a twenty-tool server costs ~2,750 tokens a turn whether + used or not. Plan: `mcp_list` / `mcp_inspect` / `mcp_call` meta-tools, prompt names servers + not schemas. (TODO.md `## Next`; ROADMAP `## Next`) +- [ ] **Derive the tool-name lists.** `TOOL_SETS` and `MUTATING_TOOLS` are hand-maintained; a tool + added to one and forgotten in the other is a silently ungated write. (TODO.md `## Next`; + ROADMAP `## Next` "Derived tool metadata") - [ ] **Subagent parallelism.** Two independent searches run sequentially; the panel already renders several agents, the loop does not fan out. (TODO.md `## Next`; ROADMAP `## Later`) - [ ] **Undo a turn.** `/resume` restores a session but nothing walks one step back; `bash` effects cannot be snapshotted and the docs would say so. (TODO.md `## Next`; ROADMAP `## Next`) -### ROADMAP "Next" / "Later" — tracked, not yet scheduled in TODO.cpp-equivalent detail +### ROADMAP "Next" — tracked, not yet scheduled in TODO.cpp-equivalent detail - [ ] **Registry trust.** No signatures; `registryUrl` is the whole trust decision. Publisher - keys + pinned digest per entry. (ROADMAP `## Next`; also TODO.md Known rough edges) -- [ ] **Lossless-enough compaction** — same work as "Summarize the pruned span". (ROADMAP `## Next`) + keys plus a pinned digest per entry. (ROADMAP `## Next`; TODO.md Known rough edges) +- [ ] **`web_fetch` leftovers.** `web_fetch` itself shipped in beta.4. What remains declined: + thin wrappers around a single bash line (`run_tests`, `typecheck`, `lint`, `build`) that add + only schema tax. (ROADMAP `## Next`) + +### ROADMAP "Later" + - [ ] **Session branching**, **structured diff review**, **plugin code from disk** (needs a sandbox story), **prompt caching** (stable prefix vs volatile suffix), **external hooks** (needs a trust story), **OS-level sandboxing** (Seatbelt/Landlock/Windows equivalent). (ROADMAP `## Later`) -- [ ] **Deliberately declined** (do not "fix"): web UI, auto-commit, vector search, tool-call - retries, client/server split, LSP integration — all recorded in ROADMAP `## Declined`. +- [ ] **Deliberately declined** (do not "fix"): web UI, model-agnostic prompt tuning, auto-commit, + vector search, tool-call retries, client/server split, LSP integration — all recorded in + ROADMAP `## Declined`. --- ## D. Documentation drift (untracked) -- [ ] **README tool count** — see Bug B.3. **not yet tracked** -- [ ] **TODO.md "Done" is missing the rest of beta.5.** "Kept for one release, then deleted", - but of the beta.5 batch (more tools incl. `git_branch`/`git_commit_message`/ - `move_file`/`delete_file`, the `protect` plugin, `security`/`perf`/`migrate` skills, the - MCP panel wizard, the UI refinements, the farewell message) only "a dead provider item - ends the turn" was checked off. Either the Done list gets the beta.5 items or it gets - rotated, as the file's own rule says. **not yet tracked** -- [ ] **`docs/registry.md` and `docs/headless.md`** are referenced by the README table and both +The two items from the beta.5 audit are resolved: `README.md` now says 41, and `TODO.md` has a +complete `## Done` section for the 1.0.0 batch (with the history in ROADMAP `## Shipped`). Three +stale lines were found in this pass, and all three are now corrected: + +- [x] **`docs/development.md:101` said "Nineteen built-in tools".** Now "Forty-one", matching the + registry; the sentence warns that selection accuracy degrades past a certain count, so the + number matters. +- [x] **`docs/headless.md:54` used "There are 16 built-in tools."** as its sample JSON output. + Now "41", so the example matches the tool list. +- [x] **`docs/headless.md:185-186` said there was no spend ceiling yet.** Rewritten to document + `maxSpendUsd` (`src/config.ts:24`): warns once at 80%, refuses the next turn past 100%, and + the run exits non-zero — enforced only on priced models. +- [x] **`docs/registry.md` and `docs/headless.md`** are referenced by the README table and both exist — verified clean, no action. --- ## E. Test-coverage gaps -- [ ] **`src/cli.tsx` (562 lines) has no unit test.** Nothing in `test/` imports it. Its flag - parsing (`-p`, `--json`, `--yolo`, `--resume`, provider setup, `/provider` wiring) is - exercised only by hand or through `runHeadless` (`test/headless.test.ts`), which bypasses - the argument surface. The largest module in `src/` outside the UI is the least tested one. - **not yet tracked** -- [ ] **`src/ui/Onboard.tsx` has no test.** The provider on-boarding wizard is never rendered in - the suite. **not yet tracked** -- [ ] **`src/ui/PromptInput.tsx` has no test** — the `@` completion input is only covered - indirectly through `App`. (`src/complete.ts` itself is well tested.) **not yet tracked** -- [ ] **`src/ui/panel-bodies.ts` and `src/ui/buses.ts` have no direct tests.** - **not yet tracked** -- [ ] **29 `as any` casts across 17 test files.** The identical mock `usage` object - (`{ inputTokens: {...}, outputTokens: {...} } as any`) is copied verbatim in 7+ UI test - files — a shared typed fixture in `test/helpers.ts` would remove the repetition and the - casts in one move. Production `src/` remains clean; this is test-only debt. **not yet tracked** +- [x] **`src/cli.tsx` (666 lines) now has an entry-point test — `test/cli.test.ts`.** The module + is an executable, not a library: importing it parses argv, loads config, connects MCP, and + renders Ink, which is why nothing imported it before. The test runs the real entry point as a + child process (via `process.execPath`, so it is cross-platform) with a scratch `SHIRO_HOME`, + a scratch cwd, and every API-key variable stripped, so the no-key branches are deterministic. + Ten cases cover the argument surface: `--help`/`-h`, `--version`/`-v`, an unknown `--agent`, + an unknown `--think`, `-p` without a key, `--resume` with no match, `--continue` with no + saved session, and a configured key with no prompt (that last run also passes all six + `--no-*` isolation flags through the boot path). Paths past `render()` need a TTY and remain + uncovered — provider `/provider` wiring through the wizard included. +- [x] **`src/ui/Onboard.tsx` (211 lines) now has a test — `test/onboard.test.tsx`.** The wizard + renders standalone and its only network call is `fetchModels`, which uses the global `fetch`, + so the suite stubs it: the flow is exercised offline and deterministically, with the two API + key env vars cleared so a developer's shell never picks the branch. Six cases cover the + provider list and its `(current)` marker, esc from both the list and the api-key step, a + custom endpoint collecting url + key + a sorted model pick, the env-key shortcut (key step + skipped, hint masked to `sk-a...1234`), manual model-id entry, and the "could not list + models" fallthrough when the server errors. `current` sets the starting row, so a test names + a preset rather than counting arrow presses. +- [x] **`src/ui/PromptInput.tsx` (175 lines) now has a direct test — `test/prompt-input.test.tsx`.** + It was already covered through `App` in `input.test.tsx` (history recall and stash, arrow and + word motion, ctrl-u, ctrl-d, paste); what was left needed the component on its own, so this + renders it directly with a controlled `Harness`. Fifteen cases cover the caret rendering (a + bare focused caret, a blurred input with none, the placeholder inverting its first character + only while focused, the cursor sitting under the character it points at), the kill keys + (ctrl-a/ctrl-e, ctrl-k, ctrl-w, including a single word emptying the line), the `mask` (hidden + value and the caret after a deletion), `onKey` swallowing a key before the input sees it while + declining others, `initialCursor`, an external `value`, submit, and insert-at-cursor. Two + expected frames were probed against the real renderer rather than assumed: a whitespace-only + `Text` trims to `''`, and a mask renders the caret *after* the stars. +- [x] **`src/ui/panel-bodies.ts` (78 lines) now has a direct test** — `test/ui-bodies.test.tsx`, + shared with `buses.ts`. The four panels are pure functions of session and hook state, so + they run without mounting Ink: the tools panel (sets named, a read-only agent narrowing it, + the offered/registered hint), the cost panel (priced turn, unpriced model, the subagent line + priced against its own model, the ceiling line), the context panel's empty branch, and the + todos panel fed through the real `todo_write` tool. +- [x] **`src/ui/buses.ts` (75 lines) now has a direct test.** `createNoticeBus` and + `createSubagentBus` are covered: a pre-bind emit is queued and delivered in order on bind, a + post-bind emit passes through, and a rebind takes over without replaying the queue. + `applySubagentEvent` gains the cases the existing `ui-panels` test left open — a result + attaching to its step, a mismatched or duplicate result being ignored, end/error status + flips, an unknown id being a no-op, and two agents interleaving without crossing steps. +- [ ] **13 `as any` casts across 11 test files.** Down from 29 across 17. The identical mock + `usage` object (`{ inputTokens: {...}, outputTokens: {...} } as any`) is still repeated + across several UI test files — a shared typed fixture in `test/helpers.ts` would remove the + repetition and the casts in one move. Production `src/` remains clean; this is test-only + debt. **not yet tracked** --- ## F. Maintenance debt - [ ] **Pricing table is hand-entered with no source note or date** — `src/pricing.ts:8-22`. - Rates drift; `estimateTokens` also divides JSON length by four (session.ts:90), which is - fine as a compaction threshold but misleads in `/cost`. Both tracked in TODO.md - `## Maintenance`. -- [ ] **`listPaths` walks up to 5000 files once per session** — fine for a repo, wasteful in a - monorepo, never notices a file created after the first `@`. Tracked in TODO.md. -- [ ] **`MUTATING_TOOLS` (tools.ts:799-807) is only used by tests and docs.** The runtime gate - is `DEFAULT_PERMISSIONS` + the unknown-tool `ask` default. Tracked in TODO.md — either + Rates drift. Tracked in TODO.md `## Maintenance`. +- [ ] **`estimateTokens` divides JSON length by four** — `src/session.ts:97` (the thinking panel + does the same at `src/ui/Panels.tsx:193`). Fine as a compaction threshold, misleading in + `/cost`. Tracked in TODO.md `## Maintenance`. +- [ ] **`listPaths` walks up to 5000 files once per session** — `src/cli.tsx:387-389`. Fine for + a repo, wasteful in a monorepo, never notices a file created after the first `@`. Tracked in + TODO.md. +- [ ] **`MUTATING_TOOLS` (`src/tools.ts:980`) is only used by tests and docs.** The runtime gate + is `DEFAULT_PERMISSIONS` plus the unknown-tool `ask` default. Tracked in TODO.md — either delete it or make `DEFAULT_PERMISSIONS` derive from it. -- [ ] **CI actions are about to leave Node 20.** The last release run annotated that actions on - Node 20 are being forced onto Node 24. `actions/checkout@v4`, `setup-bun@v2`, - `upload-artifact@v4`, `download-artifact@v4` still work, but the major-version bumps will - become the silent fix; watch for the annotation to turn red. **not yet tracked** -- [ ] **Oversized modules.** `src/ui/App.tsx` (863), `src/tools.ts` (717), `src/cli.tsx` (562), - `src/session.ts` (500), `src/ui/Panels.tsx` (398). All of them grew past a comfortable - review size during the beta.5 batch. Not a bug — a "who reads 863 lines" concern. - **not yet tracked** +- [ ] **Oversized modules, all grown again in the 1.0.0 batch.** `src/ui/App.tsx` (1002), + `src/tools.ts` (990), `src/cli.tsx` (666), `src/session.ts` (637), `src/ui/Panels.tsx` + (579). Not a bug — a "who reads 990 lines" concern. **not yet tracked** +- [ ] **CI actions are on Node 20.** The last release run annotated that `actions/checkout@v4`, + `setup-bun@v2`, `upload-artifact@v4`, `download-artifact@v4` are being forced onto Node 24. + They still work; the major-version bumps will become the silent fix. Not verifiable from the + tree — watch for the annotation to turn red. **not yet tracked** --- @@ -189,13 +253,13 @@ untracked items are marked **not yet tracked**. | Area | Items | |---|---| -| Bugs | 3 (`App.tsx:613`, dead `dry_run` input, README tool count) | -| Official gaps (Now/Next/Later) | 14 tracked in TODO.md/ROADMAP.md | -| Documentation drift | 2 untracked | -| Test-coverage gaps | 5 (of which `src/cli.tsx` is the significant one) | -| Maintenance debt | 5 (3 tracked, 2 untracked) | +| Bugs | 0 (all five cleared) | +| Official gaps (Now/Next/Later) | 8 near-term + 6 Later tracked in TODO.md/ROADMAP.md | +| Documentation drift | 0 (three lines corrected) | +| Test-coverage gaps | 1 (every UI module now has a test; only the `as any` item below remains) | +| Maintenance debt | 6 (4 tracked, 2 untracked) | | Known rough edges | 10 (all tracked) | -| Verified clean | 9 areas, including zero `as any` and zero swallowed errors in `src/` | +| Verified clean | 9 areas, including zero casts and zero swallowed errors in `src/` | -The codebase is in good shape for a beta. The three bugs are each one-line fixes; the -coverage gap on `cli.tsx` is the item that will actually bite. \ No newline at end of file +The codebase is in good shape for 1.0.0. The bug list, the doc drift, and every UI coverage gap are +cleared; what is left is the test-only `as any` casts and the maintenance debt — all tracked. diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab05d3..a879239 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,38 @@ All notable changes to this project are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +A batch of loop and ergonomics work landed after 1.0.0. Version remains `1.0.0` in +`src/version.ts`; these are folded into the next tagged release. + +### Added + +- **`/undo` and `/redo`.** Every prompt snapshots the files on disk first (capped near the last + 100), and `/undo` restores files, trims the conversation, or both. `/redo` reverses it. A + `bash` command's side effects are not files and cannot be rolled back, which is stated in the + command output rather than hidden. +- **Parallel subagents.** The `task` tool accepts several independent investigations under a + `tasks` array and runs them on separate context windows at the same time, joining their + reports. A single call behaves exactly as before. +- **Lazy MCP tools.** By default an MCP server now contributes three meta-tools (`mcp_list`, + `mcp_inspect`, `mcp_call`) instead of one schema per server tool, so a server exposing twenty + tools stops costing ~2750 tokens per request until one is actually called. Set + `"mcpMode": "eager"` to register every server tool up front. Named servers are still listed + in the prompt, and calls route through the same permission rules and guard as before. +- **Hot-reloaded skill installs.** A skill installed from `/registry` mid-session is callable + on the next turn without a restart (the `skill` tool reads its list live, so even the first + install works). Plugins and external tools still need a restart because they join the guard + chain and tool registry built once at boot. + +### Fixed + +- The walk behind `@file` completion refreshed only once per session; it now re-walks on a slow + cooldown so a file created after the first `@` shows up within a short window. +- A handful of plugin write tools were mutating but not gated by the permission defaults; the + tool set and the mutating list are now derived from a single `mutating()` marker, so a tool + can no longer be added to one and forgotten in the other. + ## [1.0.0] The first stable release. Cost control, a larger tool and skill surface, custom slash diff --git a/README.md b/README.md index 3e84ed4..63aaf2e 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,11 @@ pattern, not the whole tool. `.env` and `.pem` files are refused on read outrigh plugin refuses irreversible commands ahead of any of it — `rm -rf`, `git reset --hard`, force pushes, `DROP TABLE` — and `--yolo` cannot bypass it. +**Rewinds a mistake.** `/undo` restores the snapshot taken before a prompt: files are reverted, +the conversation is trimmed back, or both, and `/redo` reverses it. The snapshot is capped so +it stays near the last 100 prompts, and a `bash` command's side effects cannot be rolled back +this way because they are not files. + **Shows its work.** Reasoning streams to a collapsed panel you can expand with `ctrl-r`, the tool in flight is named as it runs with the arguments that identify the call, and `bash` output streams live instead of arriving all at once when the command exits. `ctrl-c` kills a @@ -93,10 +98,13 @@ is a decision rather than a default, and it asks before every call. **Asks instead of guessing.** When a request has two readings that lead to different work, the agent puts a question on screen with options. -**Delegates work.** `task` spawns a subagent with its own context window whose findings come -back as one message, so a search across forty files does not fill the main context. `explore` -and `review` are read-only; `worker` also edits and runs commands, and every one of its writes -stops at the same approval prompt as yours. Progress streams to a panel. +**Delegates work, in parallel.** `task` spawns a subagent with its own context window whose +findings come back as one message, so a search across forty files does not fill the main +context. A single call can batch several independent investigations under `tasks`: they run on +separate context windows at the same time and their reports are joined, so two unrelated +searches overlap in wall-clock time instead of queueing. `explore` and `review` are read-only; +`worker` also edits and runs commands, and every one of its writes stops at the same approval +prompt as yours. Progress streams to a panel. **Extensible from the prompt.** `/registry` browses external skills and plugins and installs them with one confirmation. A skill is shown in full before its text joins your system prompt; @@ -115,7 +123,10 @@ record of what it already ran instead of repeating it. **Keeps the tool list affordable.** Forty-one built-in tools, grouped into sets. Each costs about 550 characters of schema on every request, so `{ "toolSets": [] }` trims back to the six -core ones and a disabled set reaches neither the wire nor the prompt. +core ones and a disabled set reaches neither the wire nor the prompt. An MCP server's tools are held back the +same way: by default its tools are fetched only when one is actually called (via `mcp_list`, +`mcp_inspect`, `mcp_call`), so twenty tools on one server cost almost nothing until they are +used. Set `"mcpMode": "eager"` to register every server tool up front instead. ## Documentation @@ -149,7 +160,7 @@ Type `/` and a menu appears, narrowing as you type. ``` /help /agent [name] /think [level] /provider /models /model /skills /plugins /registry [search|add|remove] /mcp [add|remove] /init /context -/todos /notes /memory /tools /compact /cost +/todos /notes /memory /tools /compact /cost /undo /redo /sessions /resume /save /clear /exit ``` @@ -166,7 +177,8 @@ gateable sets, 29 bundled skills, built-in and data-only plugins, per-project me persistence and resume, MCP servers, custom slash commands from markdown files, auto-loaded external skills/tools/plugins, markdown rendering, headless mode with JSON events for CI, five-platform builds, streaming reasoning, the mid-turn prompt queue, read-only git tools, -batch reads, `apply_patch`, `web_fetch`, `@file` completion, interruptible commands, and the +`/undo` and `/redo`, parallel subagents, lazy MCP tools, hot-reloaded skill installs, batch +reads, `apply_patch`, `web_fetch`, `@file` completion, interruptible commands, and the external registry. Next up is in [TODO.md](TODO.md); the longer view and what has been declined are in diff --git a/ROADMAP.md b/ROADMAP.md index 720927d..2783d4d 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -238,6 +238,117 @@ agent·model row inside it and a split footer beneath. --- +## What the other agents have + +Surveyed opencode, Claude Code, Codex CLI, and phi against this tool's feature set. The point of +the table is to record what is *worth copying* and what is worth *declining*, not to chase parity: +each of these four spent effort here deliberately, and several of their choices are load-bearing for +a reason that applies to us. + +The four are not the same shape. **Claude Code** and **Codex** are first-party CLIs tied to one +vendor's models; **opencode** and **phi** are open-source and provider-agnostic, and opencode in +particular is the closest thing to a peer here. Where a feature exists, the notes say what it costs +— several are cheap to copy, and three are not. + +### Where the four agree + +Six features have converged across all or nearly all of them. Convergence is the strongest signal +available that a feature is not a fad — and the gaps in the table are as informative as the checks, +because they show which features are genuinely optional and which are table stakes. + +| Feature | opencode | Claude Code | Codex | phi | Here | +| --- | --- | --- | --- | --- | --- | +| Per-pattern permission rules | `permission.bash` globs | `settings.json` allow/deny/ask | `approval_policy` + rules | Gate + `permissions.mode` | **shipped** | +| Subagents with fresh context | `mode: subagent` | `agents/*.md` | — | sub-agents | **shipped** | +| Markdown-defined agents | `agents/`, `commands/` | `agents/`, `skills/` | `AGENTS.md` | `.phi/` | **shipped** | +| Session resume | `--session`, `--continue` | `--resume`, `--continue` | resume + rollout files | `sessions` | **shipped** | +| Compaction | auto + `/compact` | `/compact`, `/rewind` summarize | compact prompt file | — | **shipped** | +| Undo / rewind | `/undo`, `/redo` (via git) | `/rewind` (snapshots) | — | — | **missing** | + +Two of the six are outright missing from one or more tools, and undoing is missing from two of the +four — so it is a real feature, not table stakes, and the two that have it disagree about how. The +permission model here is not behind: it already carries the per-pattern rules, the credential deny, +and the repeat guard the others arrived at, and `doom_loop` (below) is the one refinement worth +taking. **Undo is the single converged feature genuinely absent**, which is why it leads Next. + +### Detail worth having, by tool + +**opencode** — the closest peer, and the source of the permission shape already adapted here. +`/undo` and `/redo` revert file changes **through git**, so they require the project to be a git +repository; this is a real limitation, not an implementation detail, and it means an undo is only +as good as the working tree's state. Agents are `primary` (build, plan) or `subagent` (general, +explore, scout), switchable with Tab or `@`-mention, configured in `opencode.json` or Markdown +frontmatter. A subagent runs in a **child session** with its own navigation keybinds +(`session_child_first`, `session_parent`), and `subagent_depth` (default 1) caps nesting. Notable: +`doom_loop` is a first-class permission key that fires when the same tool call repeats three times +with identical input — the repeat guard here is an approval rule; theirs is a named primitive with +its own recovery prompts. Sessions share over a URL (`/share`), which is a hosted product decision, +not a local one. + +**Claude Code** — the most complete implementation of undo, and worth reading before building one. +Checkpointing snapshots **before every user prompt**, keeps the **100 most recent** checkpoints, +and stores them with the conversation so `/rewind` survives a resume. The rewind menu offers five +distinct actions, and the split is the interesting part: *restore code*, *restore conversation*, +*restore both*, *summarize from here*, *summarize up to here*. That is undo and compaction sharing +one control surface. The honest limits are documented rather than hidden: only `Write`/`Edit`/ +`NotebookEdit` are tracked, `bash` side effects are not, **subagent edits are not captured** unless +the skill ran in the foreground with `context: fork`, and symlinked or hard-linked files are +skipped with an explicit warning. Their hook surface is the largest of the four — see *External +hooks* under Later, which is where that capability belongs here. + +**Codex** — the sandbox is the differentiator, and it is genuinely hard to copy. Apple Seatbelt on +macOS, Landlock + seccomp on Linux, and a restricted-token/AppContainer mechanism on Windows, with +`workspace-write` the default and network **off** unless `sandbox_workspace_write.network_access` +is set. `approval_policy` is now `on-request | never | { granular = {...} }` — `untrusted` was +**retired** and can prevent the client from starting. Also shipped and worth knowing: +`approvals_reviewer = "auto_review"` routes an approval prompt through a *reviewer subagent* +rather than the user. `AGENTS.md` resolves global → project root → cwd, one file per directory, +`AGENTS.override.md` winning, capped by `project_doc_max_bytes` (32 KiB default). + +**phi** — small (Go, ~12 MB), and the two ideas most worth stealing. First, **MCP without context +death**: server tool schemas never enter the prompt; the system prompt lists only **server names**, +and the model uses three meta-tools — `mcp_list`, `mcp_inspect`, `mcp_call` — with subprocesses +starting lazily on first use. This is the concrete design behind *MCP without the schema tax* +under Next. Second, the +hook contract is the cleanest of the four: `pre_tool` runs **before** the permission gate and can +`allow`, `deny`, or **`modify`** the input; `post_tool` can append model-facing `context` or +rewrite `output`. Exit code `2` is a hard deny; `fail_closed` decides crash behaviour; in +`readonly` mode only `fail_closed` hooks run, so a slow audit hook cannot stall exploration. + +### Corrections to what this file said before + +Two claims previously written here were incomplete, and the research fixes them: + +- **Input-rewriting hooks are not phi's alone.** Codex ships it too: `PreToolUse` returns + `permissionDecision: "allow"` with `updatedInput` to rewrite a call, verified in their docs and + in `codex-rs/.../mcp.rs` (`with_updated_hook_input`). Two independent implementations make this + the standard shape rather than one project's quirk — which raises its priority, and means the + compiled plugin interface here is now the odd one out. +- **Codex's hook trust is exactly as described, and the mechanism is now known.** Non-managed + hooks cannot run until reviewed: Codex persists a `trusted_hash` in `config.toml`, records trust + against the hook's **current hash**, and marks new or changed hooks for review in `/hooks`. + `--dangerously-bypass-hook-trust` skips it for one invocation. The known hole is instructive: a + reviewer on their PR noted that **replacing the script a hook points at does not reset trust**, + because only the config is hashed. A trust story that hashes the declaration but not the artefact + is a partial one — worth designing past rather than copying. + +### What is worth declining + +Three of their features are deliberate here, and the survey confirms the reasoning: + +- **A client/server split** (opencode's OpenAPI server + TUI-as-client + IDE/web clients) exists to + serve *second clients*. No second client is wanted here. +- **LSP integration** — the quote already cited in Declined is accurate and now verified in full: + opencode's own LSP page says it "is useful in some projects, but it is not always a net positive," + that servers "can get out of sync, use significant memory, vary by version or project, and slow + down agent workflows," and that "in many projects it is better to have the agent run lint, + typecheck, or other diagnostic CLI tools directly." They ship 30+ built-in servers and still say + this. That is the strongest possible endorsement of the position in Declined. +- **Session sharing over a URL** (opencode `/share`) is a hosted-service feature and brings a + privacy surface this tool has no reason to take on. + +--- + ## Next ### MCP without the schema tax @@ -250,10 +361,21 @@ registration as an option: for a two-tool server the indirection is the more exp ### Undo a turn -opencode has `/undo` and `/redo`, Claude Code has `/rewind` over file checkpoints. There is -`/resume` here, which restores a whole session, and nothing that steps one turn back. The honest -limit is the same for everyone: a `bash` command's effects cannot be snapshotted, so this covers -file-tool edits and says so. +Every comparable CLI has this: opencode `/undo` and `/redo`, Claude Code `/rewind` with +checkpoints. There is `/resume` here, which restores a session, and nothing that walks one back. +Claude Code's implementation is the one to read first, because it has already worked out the seams: +it snapshots before **every user prompt**, keeps the **100 most recent**, stores snapshots with the +conversation so a rewind survives a resume, and splits one menu into *restore code*, *restore +conversation*, *restore both*, and *summarize from here / up to here* — undo and compaction on one +control surface. opencode's version is simpler and takes a different position: it reverts through +**git**, so it needs a repository and inherits whatever the working tree already contained. + +Both document the same hard limit, and so must this: a `bash` command's effects cannot be +snapshotted. Claude Code tracks only its own file-edit tools, explicitly does not cover `bash` +side effects, and does not capture subagent edits unless the fork ran in the foreground. The +honest version here covers file-tool edits, says so in the command's own output, and reports what +it skipped rather than pretending the tree is clean — the same shape used for an interrupted +command, whose effects are already reported as unknown. ### Lossless-enough compaction @@ -273,6 +395,26 @@ An index is trusted for its contents, not its authorship: `registryUrl` is the w decision, and there are no signatures. Publisher keys and a pinned digest per entry would make "install this skill" a decision about a specific artifact rather than about a URL. +### Auto-review an approval + +Codex ships `approvals_reviewer = "auto_review"`: an eligible approval prompt is routed through a +reviewer **subagent** instead of surfacing to the user, using the same sandbox boundary. `explore` +and `review` already exist and are read-only, so the piece to build is a reviewer persona that +decides an approval request and a policy that says which prompts are eligible — a batch of three +identical `git status` calls should not each interrupt, but a first `rm` should. The failure mode +to design against is a reviewer that waves through exactly what the user would have stopped, so it +must be opt-in, name itself when it approves, and never override a deny rule. + +### Name the repeat guard + +opencode has `doom_loop` as a first-class permission key: it fires when the same tool call repeats +three times with identical input, carries its own recovery prompts, and is configurable per agent. +The equivalent here is a rule inside the permission layer — the call repeated identically three +times in one turn asks even when allowed — which works but is unnamed, unconfigurable, and +invisible in `/tools`. Promoting it to a named primitive makes it inspectable and lets an agent +tighten or loosen it, and the recovery prompt is the part genuinely missing: a loop is better +interrupted with advice than with silence. + ### `web_fetch` Shipped in beta.4 — see above. What remains declined: wrappers around a single bash line with @@ -301,18 +443,30 @@ agent can read. step for task-list freshness, which defeats a naive cache; splitting the stable prefix from the volatile suffix would fix that. -**External hooks.** phi and both first-party CLIs let a script sit in the tool loop: a directory -with a manifest and an executable, one JSON object in on stdin, one out. phi's `pre_tool` can -rewrite the tool's input as well as allow or deny, which the compiled plugin interface here cannot -express. The reason it is here rather than in Next is that it needs a trust story — Codex hashes -each hook and refuses to run one until you review it, which is the right shape and more work than -the feature. +**External hooks.** Every one of the four surveyed except here lets a script sit in the tool loop: +a directory with a manifest and an executable, one JSON object in on stdin, one out. **Both** phi +and Codex let a `PreToolUse` hook **rewrite** a tool's input, not merely allow or deny it — phi +returns `{"action":"modify","input":{...}}`, Codex returns `permissionDecision: "allow"` with +`updatedInput` — and that is the exact capability the compiled plugin interface here cannot +express. Two independent implementations make it the expected shape rather than one project's +quirk. What blocks it is the trust story, not the mechanism. + +The trust story has a known shape and a known hole. Codex will not run a non-managed hook until it +has been reviewed: it persists a `trusted_hash` in `config.toml`, records trust against the hook's +current hash, and lists new or changed hooks for review under `/hooks`; +`--dangerously-bypass-hook-trust` overrides for one invocation. The hole is that only the +**declaration** is hashed — replacing the script the hook points at does not reset trust, as a +reviewer on their own PR pointed out. Hashing the declaration *and* the artefact is the minimum +worth doing here, and it is why this is work rather than a weekend. **OS-level sandboxing.** The strongest thing in this class, and Codex is the one that has it: -Seatbelt on macOS, Landlock and seccomp on Linux, a separate mechanism on Windows, with network -egress governed by domain rules. Permission rules gate the *call*; a sandbox governs what the -process can then reach. Three platform-specific implementations, and OpenAI moved Codex to Rust -partly for this. A half-built sandbox is worse than none, because people would trust it. +Seatbelt on macOS, Landlock and seccomp on Linux, a restricted-token/AppContainer mechanism on +Windows, with network **off** by default under `workspace-write`. Permission rules gate the *call*; +a sandbox governs what the process can then reach. Note that even Codex's sandbox is not a complete +boundary — their own hook docs say "hooks are guardrails, but they are not a complete enforcement +boundary for every shell or tool path." Three platform-specific implementations, and OpenAI moved +Codex to Rust partly for this. A half-built sandbox is worse than none, because people would trust +it. --- @@ -339,8 +493,11 @@ endpoint, which is what lets IDE extensions and a web client exist. It is the ri for that product. Here it would add a protocol, a port, and an auth story to serve a second client nobody has asked for. -**LSP integration.** opencode ships it and its own documentation says the honest thing: -*"not always a net positive... in many projects it is better to have the agent run lint, typecheck, -or other diagnostic CLI tools directly."* Language servers drift out of sync, use real memory, and -vary by version. `bash bun run typecheck` puts the same errors in front of the model with none of -that, and `AGENTS.md` is where the command belongs. +**LSP integration.** opencode ships 30+ built-in language servers and its own documentation still +says the honest thing: LSP "is useful in some projects, but it is not always a net positive," +servers "can get out of sync, use significant memory, vary by version or project, and slow down +agent workflows," and *"in many projects it is better to have the agent run lint, typecheck, or +other diagnostic CLI tools directly."* That is the strongest available endorsement of declining it: +the project with the most invested says it is often the wrong trade. `bash bun run typecheck` puts +the same errors in front of the model with none of that, and `AGENTS.md` is where the command +belongs. diff --git a/TODO.md b/TODO.md index 0f3b964..0d16e70 100644 --- a/TODO.md +++ b/TODO.md @@ -14,19 +14,23 @@ Compaction now keeps the model's memory of a turn, but it still tells the model the messages it dropped, so a decision from forty messages ago can be contradicted with confidence. -- [ ] Summarize the discarded messages before dropping them -- [ ] Inject the summary in place of the count -- [ ] Budget it: a summary that grows with the session defeats the point -- [ ] Test: a pruned decision is still recoverable from the summary +- [x] Summarize the discarded messages before dropping them +- [x] Inject the summary in place of the count +- [x] Budget it: a summary that grows with the session defeats the point +- [x] Test: a pruned decision is still recoverable from the summary ### Hot-reload an installed entry `/registry add` writes the file and says to restart. The skill catalogue and the guard chain are both assembled at boot, so a mid-session install does nothing until then. -- [ ] Rebuild the skill list and plugin host after an install or removal -- [ ] Leave a turn in flight alone: its rules must not change underneath it -- [ ] Test: a skill installed mid-session is callable in the next turn without a restart +- [x] Rebuild the skill list and plugin host after an install or removal +- [x] Leave a turn in flight alone: its rules must not change underneath it +- [x] Test: a skill installed mid-session is callable in the next turn without a restart + +> Partial: skills reload live (the `skill` tool reads its list on each call); plugins and +> external tools still need a restart because they join the guard chain and the tool registry, +> both built once at boot. --- @@ -40,11 +44,13 @@ Every MCP tool's schema goes into the prompt today, so twenty tools from one ser phi solves this with three meta-tools — `mcp_list`, `mcp_inspect`, `mcp_call` — and a prompt that names only the servers. A hundred servers then cost almost nothing until one is called. -- [ ] `mcp_list` / `mcp_inspect` / `mcp_call` replacing per-tool registration -- [ ] The prompt lists server names, not schemas -- [ ] Calls go through the same permission rules and guard as a built-in -- [ ] Keep per-tool registration as an option: a two-tool server is cheaper registered directly -- [ ] Test: a configured server contributes no schema to the request until `mcp_call` +- [x] `mcp_list` / `mcp_inspect` / `mcp_call` replacing per-tool registration +- [x] The prompt lists server names, not schemas +- [x] Calls go through the same permission rules and guard as a built-in +- [x] Keep per-tool registration as an option: a two-tool server is cheaper registered directly +- [x] Test: a configured server contributes no schema to the request until `mcp_call` + +> Switchable with `mcpMode: 'eager'` in config; lazy (meta-tools) is the default. ### Derive the tool-name lists @@ -52,39 +58,45 @@ names only the servers. A hundred servers then cost almost nothing until one is in the other is a silently ungated write, which is the worst kind of bug this codebase can have. -- [ ] Mark each tool as mutating where it is defined, not in a list beside it -- [ ] `TOOL_SETS` covers every registered tool, checked rather than assumed -- [ ] Test: a tool in no set, or a mutating tool outside `MUTATING_TOOLS`, fails the suite +- [x] Mark each tool as mutating where it is defined, not in a list beside it +- [x] `TOOL_SETS` covers every registered tool, checked rather than assumed +- [x] Test: a tool in no set, or a mutating tool outside `MUTATING_TOOLS`, fails the suite + +> While doing this, two mutating-but-ungated tools surfaced and were gated: the registered set +> is now derived from the `mutating()` marks. ### Subagent parallelism Two independent searches run sequentially. The panel already renders several agents; the loop does not fan out. -- [ ] `task` accepts several investigations and runs them together -- [ ] Test: two delegated searches overlap in time rather than queueing +- [x] `task` accepts several investigations and runs them together +- [x] Test: two delegated searches overlap in time rather than queueing + +> `tasks` on the `task` tool runs them concurrently via `Promise.all`; a call without it behaves +> exactly as before. ### Undo a turn Every comparable CLI has this: opencode `/undo` and `/redo`, Claude Code `/rewind` with checkpoints. There is `/resume` here, which restores a session, and nothing that walks one back. -- [ ] Snapshot files before each prompt, capped at the 100 most recent -- [ ] `/undo` restores files, conversation, or both; `/redo` reverses it -- [ ] Say plainly what is not covered: a `bash` command's effects cannot be snapshotted -- [ ] Test: an edit is reverted, and the model's own record of it goes with it +- [x] Snapshot files before each prompt, capped at the 100 most recent +- [x] `/undo` restores files, conversation, or both; `/redo` reverses it +- [x] Say plainly what is not covered: a `bash` command's effects cannot be snapshotted +- [x] Test: an edit is reverted, and the model's own record of it goes with it --- ## Maintenance -- [ ] Pricing table needs a source note and a date; rates drift and ours are hand-entered -- [ ] `estimateTokens` divides JSON length by four. Good enough for a compaction threshold, +- [x] Pricing table needs a source note and a date; rates drift and ours are hand-entered +- [x] `estimateTokens` divides JSON length by four. Good enough for a compaction threshold, wrong enough to mislead in `/cost`. Either label it an estimate everywhere or use a real tokenizer -- [ ] `listPaths` walks up to 5000 files once per session. Fine for a repo, wasteful in a +- [x] `listPaths` walks up to 5000 files once per session. Fine for a repo, wasteful in a monorepo, and it never notices a file created after the first `@` -- [ ] `MUTATING_TOOLS` is now only used by tests and docs; the permission defaults are what +- [x] `MUTATING_TOOLS` is now only used by tests and docs; the permission defaults are what actually gate a write. Either delete it or make the defaults derive from it --- @@ -144,3 +156,6 @@ Kept for one release, then deleted. The 1.0.0 release batch: - [x] The system prompt advanced: a failure-recovery loop, a delegation policy, compaction awareness - [x] The release workflow's dead `dry_run` input wired: manual dispatch publishes only when unchecked, tag pushes always publish +- [x] A visible escape hatch for the stalled-agent loop: the existing repeat guard now offers a + configurable `step_back` recovery primitive that reads the session loop trace and steers a + model circling without progress to stop and change direction diff --git a/docs/architecture.md b/docs/architecture.md index e187081..b86398b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -147,6 +147,10 @@ running, the handler exits as usual. `task` runs a nested `streamText` and returns one message. The subagent kinds hold different tool sets: `explore` and `review` the read-only tools, `worker` those plus every write tool. +A `tasks` array on the call runs several of these nests concurrently — each gets its own +context window and stream, awaited together via `Promise.all` — so independent investigations +overlap instead of queueing. The single-prompt form is just the one-element case; the two paths +share the same nested-loop machinery and the same reporting bus. The consequences follow from the tool set, not from policy: diff --git a/docs/development.md b/docs/development.md index e903c54..98b02e2 100644 --- a/docs/development.md +++ b/docs/development.md @@ -16,7 +16,7 @@ faster and the fallback path is exercised without it. ```bash bun run shiro # run from source bun run typecheck # tsc --noEmit -bun test # 713 tests +bun test # 895 tests bun run build # single binary for this platform -> dist/shiro bun run release # all five platforms -> dist/release + SHA256SUMS bun run install:local # build, then copy onto PATH @@ -98,7 +98,7 @@ Steps 3 and 4 are two hand-maintained lists of tool names, which is a known weak added to one and forgotten in the other is a silently ungated write. Deriving both from the tool definitions is on [TODO.md](../TODO.md). -Every tool costs roughly 550 characters of schema on every request. Nineteen built-in tools is +Every tool costs roughly 550 characters of schema on every request. Forty-one built-in tools is past where selection accuracy starts to matter, which is why sets exist and why a new tool needs to earn its place — see [ROADMAP.md](../ROADMAP.md) for what has been declined and why. One set, `net`, is opt-in rather than on: `web_fetch` is the one tool that leaves the machine. @@ -151,9 +151,9 @@ Windows host and rejected everywhere else — so a green local release is not pr `buildArgs()` is unit-tested for both hosts because of exactly that. `.github/workflows/release.yml` then runs typecheck and tests, cross-compiles all five -targets on one Ubuntu runner, asserts the built binary reports the expected version, and -publishes a GitHub release with the binaries and `SHA256SUMS`. A tag containing `-` is -published as a prerelease. +targets on one Ubuntu runner, asserts each built binary reports the expected version and is +non-empty, and publishes a GitHub release with the binaries and `SHA256SUMS`. A tag containing +`-` is published as a prerelease. Bun cross-compiles from any host, which is why there is no build matrix. Verified: a working `darwin-arm64` binary builds on Windows. @@ -161,10 +161,16 @@ Bun cross-compiles from any host, which is why there is no build matrix. Verifie Publishing is gated on a `v*` tag, so a manual `workflow_dispatch` run produces artifacts without releasing. +The release body is composed by `scripts/make-release-notes.ts` from the matching `## []` +section of `CHANGELOG.md` (plus an artifact inventory), and it fails the release if that heading +is missing — so a tag with no changelog entry cannot ship an empty body. Write the changelog +entry first, then tag. + ## CI `.github/workflows/ci.yml` runs typecheck, tests, and a build on Ubuntu, macOS, and Windows -for every push and PR. +for every push and PR, caching the bun install store across runs so a no-change run skips the +dependency download. All three are necessary. The tools shell out to `rg`, `git`, and a platform shell, and path handling differs — a Windows-only break is invisible on Linux until someone hits it. diff --git a/docs/headless.md b/docs/headless.md index 32e5123..7af8996 100644 --- a/docs/headless.md +++ b/docs/headless.md @@ -51,7 +51,7 @@ $ shiro -p "count the tools" --json {"type":"tool-start","id":"c1","name":"grep"} {"type":"tool-call","id":"c1","name":"grep","input":{"pattern":"tool\\("}} {"type":"tool-result","id":"c1","name":"grep","output":"src/tools.ts:26: ..."} -{"type":"text","text":"There are 16 built-in tools."} +{"type":"text","text":"There are 41 built-in tools."} {"type":"done","inputTokens":4210,"outputTokens":88} ``` @@ -182,8 +182,11 @@ in the system prompt, and CI is exactly where nobody is watching what it says. S ## Cost control Headless runs are unattended, so a runaway loop costs real money. `--agent quick` caps the -step count at 12, and `{ "toolSets": [] }` trims the schema sent every request. There is no -spend ceiling yet — see [TODO.md](../TODO.md). +step count at 12, and `{ "toolSets": [] }` trims the schema sent every request. `maxSpendUsd` in +the config is a session spend ceiling: it warns once at 80%, and past 100% the next turn is +refused naming the ceiling and the run exits non-zero. It is only enforced on priced models — +an unpriced model has no dollar figure to compare against. See +[configuration](configuration.md). What a run actually costs is in the `done` event, so a wrapper can total it: diff --git a/docs/mcp.md b/docs/mcp.md index fdee078..5e3f5ae 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -6,7 +6,7 @@ command over stdio, and a remote http or sse endpoint. ## Adding one from the prompt ``` -/mcp list what is configured, with the tool count each contributed +/mcp list what is configured, with the tool count (or "lazy") each contributed /mcp add wizard: local or remote, then the fields that kind needs /mcp remove ``` @@ -42,7 +42,7 @@ list under a request that is already running. mcp servers /mcp add to add one -- `filesystem` (local) - 11 tools +- `filesystem` (local) - connected (lazy) npx -y @modelcontextprotocol/server-filesystem . - `api` (remote) - failed: fetch failed https://example.com/mcp @@ -50,6 +50,9 @@ mcp servers configured in /home/you/.shiro-neko/config.json ``` +Under `eager` the same list shows a tool count instead of `(lazy)`, because every server's tools +are registered up front. + ## The config file The wizard writes this; it is equally editable by hand. @@ -93,6 +96,7 @@ schema check at load. ```json { + "mcpMode": "eager", "mcpServers": { "fs": { "command": "npx", @@ -120,6 +124,10 @@ inherits your `PATH` unless you replace it. **Remote** servers take `url`, and optionally `type` (`http` or `sse`, default `http`) and `headers`. +A sibling key, `"mcpMode"`, picks how the configured servers' tools reach the model: `lazy` +(the default) or `eager`. It is hand-edited — the `/mcp add` wizard does not set it — and applies +to every server, so it lives beside `mcpServers`, not inside one. + A token in `headers` sits in `config.json` in plain text, same as `apiKey`. For anything beyond a local dev token, prefer a stdio server that reads its own credential from the environment. @@ -132,10 +140,35 @@ calling the binary directly is usually the difference between a noticeable wait `--no-mcp` skips them all, which is also the quickest way to tell whether a slow start is MCP or something else. +## How a server's tools reach the model + +Two modes, switched with `"mcpMode"` in config. **`lazy` is the default**; `eager` is the opt-in. + +- **Lazy** registers three meta-tools — `mcp_list`, `mcp_inspect`, `mcp_call` — instead of one + schema per server tool. The server's real tools are fetched only when `mcp_call` actually + invokes one, so a server exposing twenty tools costs almost nothing until one is used. This + is why the affordability paragraph in the README says a configured server no longer taxes + every request. +- **Eager** registers every server tool up front as in the old 1.0 behaviour. If a server + exposes only two tools, eager is cheaper because there is no list-then-inspect round trip. + +The model is told the connected server names through the meta-tool descriptions and a prompt +line, then discovers each tool's schema on demand: + +``` +- mcp_list, mcp_inspect, mcp_call: MCP tools are fetched on demand. mcp_list names a + server's tools, mcp_inspect reads one tool's schema, mcp_call runs it. Never guess a + server or tool name: list first. +``` + +A `mcp_call` still routes through the same permission rules and guard as a built-in, so the +lazy path is not a way around approval. + ## Naming -Tools arrive as `mcp____`. A server named `fs` exposing `read_file` becomes -`mcp__fs__read_file`. +In `lazy` mode (the default) tools are addressed as `mcp_call(server, toolName, args)`; the +server names are zero-ambiguity identifiers you list first. In `eager` mode tools arrive as +`mcp____`: a server named `fs` exposing `read_file` becomes `mcp__fs__read_file`. The namespace is not cosmetic. Two servers both exposing `search` would otherwise silently shadow each other, and the model would call one believing it was the other. @@ -165,26 +198,29 @@ Python interpreter should not stop you from editing a file. ## Inspecting -`/tools` lists everything offered this turn, MCP tools included. The system prompt describes -them as a group: +`/tools` lists everything offered this turn. In `eager` mode that includes each MCP tool, named +`mcp____`, described with whatever the server sent. In `lazy` mode the three +meta-tools appear and the server's real tools are surfaced by `mcp_list` inside the session. -``` -- mcp__api__query, mcp__fs__read_file: from MCP servers, named mcp____. - Each needs approval; read its own description before calling. -``` - -Their individual descriptions come from the server, so that is what the model reads before -calling one. +Into `mcp_inspect` or `mcp_list` goes the server name, not an `mcp__` path, so the prompt tells +the model which servers are connected and to list first before guessing a tool name. ## Cost -Each tool adds its name, description, and JSON schema to every request. The built-ins average -548 bytes; MCP tools vary with how verbose the server's schema is. A server exposing twenty -tools costs roughly 2,750 tokens per turn, sent whether or not the model uses any of them. +In **lazy** mode (the default) a configured server contributes three small meta-tool schemas to +every request, not one schema per tool. A server exposing twenty tools therefore costs a few +hundred tokens per turn rather than roughly 2,750, and it stays cheap whether the model uses +the tools or not. Browsing a server's tools and reading a schema still brings that server's +schema into view one tool at a time, but only when the model asks for it. -MCP tools are **not** covered by `toolSets` — that budget only governs the built-ins. There is -no per-server switch either, so the choice is a server or no server, and `--no-mcp` for all of -them. If one exposes many tools you never use, a narrower server is worth finding or writing. +In **eager** mode each tool adds its name, description, and JSON schema to every request, and +that cost is sent whether or not the model uses any of them. That is the right trade only for a +server with one or two tools, which is why eager exists. + +MCP tools are **not** covered by `toolSets` in either mode — that budget only governs the +built-ins. There is no per-server switch beyond the global `mcpMode`, so choosing `eager` turns +every server eager; a server exposing many tools you never use is worth finding a narrower one +for. `/tools` shows the count both ways: @@ -210,8 +246,9 @@ that break in practice are the handshake and the framing, and a mock asserts nei A server that starts but returns nothing useful is the harder case. In order of speed: 1. `/tools` — did the tools arrive at all? A server with no tools is a `tools/list` problem. -2. `shiro -p "call mcp__x__y with ..." --json --yolo` — the exact `tool-call` input and - `tool-result` output, one JSON object per line. +2. `shiro -p "run mcp_call(server, \"api\", \"query\", {...}) with ..." --json --yolo` — the + exact `tool-call` input and `tool-result` output, one JSON object per line. In eager mode the + address is `mcp____` instead. 3. Run the server by hand: `echo '{"jsonrpc":"2.0","id":1,"method":"tools/list"}' | your-server`. If that is wrong, nothing above it can be right. diff --git a/scripts/make-release-notes.ts b/scripts/make-release-notes.ts new file mode 100644 index 0000000..bd238fd --- /dev/null +++ b/scripts/make-release-notes.ts @@ -0,0 +1,50 @@ +#!/usr/bin/env bun +/** + * Writes `release_notes.md` for a release tag, extracting the tag's prose from + * CHANGELOG.md and appending the artifact inventory. Used by .github/workflows/ + * release.yml so the release page reads like a hand-written one instead of the raw + * PR list. + * + * Reads the tag from $RELEASE_TAG (GITHUB_REF_NAME, e.g. `v1.2.3`) and fails loudly + * when CHANGELOG.md has no matching `## []` section, because a release with + * an empty body is worse than one that refused to publish. + */ +const tag = process.env['RELEASE_TAG'] ?? 'v0.0.0-local'; +const version = tag.replace(/^v/, ''); + +const changelog = await Bun.file('CHANGELOG.md').text(); +const start = changelog.indexOf(`## [${version}]`); +if (start === -1) { + console.error(`CHANGELOG.md has no "## [${version}]" heading for ${tag}`); + process.exit(1); +} + +// The section runs from its heading to the next `## [` heading, or to the file end. +// Drop the heading itself: the release title already carries the version, so keeping +// `## [1.0.0]` under `## What's in v1.0.0` would read twice. +let body = changelog.slice(start); +const next = body.indexOf('\n## [', 1); +if (next !== -1) body = body.slice(0, next); +body = body.replace(/^## \[.*\]\n+/, '').trim(); + +// ESM, so top-level await is fine; this file is only ever run as the entry script. +await Bun.write( + 'release_notes.md', + [ + `## What's in ${tag}`, + '', + body, + '', + '### Artifacts', + '- `shiro-linux-x64`, `shiro-linux-arm64` - Linux', + '- `shiro-darwin-x64`, `shiro-darwin-arm64` - macOS', + '- `shiro-windows-x64.exe` - Windows', + '- `SHA256SUMS` - verify any of the above', + '', + 'Install with `scripts/install.sh` (macOS/Linux) or `scripts/install.ps1` (Windows);', + 'both verify the download against `SHA256SUMS`.', + ].join('\n'), +); + +console.log(`composed release_notes.md from the ${version} changelog section (${body.split('\n').length} prose lines)`); +process.exit(0); \ No newline at end of file diff --git a/src/cli.tsx b/src/cli.tsx index 6a0bef1..151963b 100644 --- a/src/cli.tsx +++ b/src/cli.tsx @@ -154,7 +154,8 @@ if (resumeArg) { } } -const mcp = has('--no-mcp') || !cfg.mcpServers ? undefined : await connectMcp(cfg.mcpServers); +const mcp = + has('--no-mcp') || !cfg.mcpServers ? undefined : await connectMcp(cfg.mcpServers, cfg.mcpMode ?? 'lazy'); const instructions = has('--no-instructions') ? [] : await loadInstructions(); const skills = has('--no-skills') ? [] : await loadSkills(); const customCommands = await loadCustomCommands(); @@ -424,8 +425,15 @@ const hooks: AppHooks = { install: async (name) => { const entry = await findEntry(name); const { path } = await registry.install(entry); - // Loaded on the next start rather than hot-swapped: a skill joins the system - // prompt and a plugin joins the guard chain, and both are built once at boot. + // A skill is live immediately: the session's skill tool reads its list on each + // call, so the next turn can invoke a skill installed right now. A plugin or + // tool joins the guard chain and the tool registry, both built once at boot, + // so those still need a restart — said plainly rather than implied. + if (entry.kind === 'skill') { + const next = await loadSkills(process.cwd()); + session.setSkills(next); + return `installed skill ${entry.name} to ${path}\nit is loaded and callable next turn`; + } return `installed ${entry.kind} ${entry.name} to ${path}\nrestart shiro to load it`; }, remove: async (name) => { @@ -434,7 +442,14 @@ const hooks: AppHooks = { const bare = parsed ? parsed[2]! : name; for (const kind of kinds) { - if (await registry.uninstall(kind, bare)) return `removed ${kind} ${bare}\nrestart shiro to unload it`; + if (await registry.uninstall(kind, bare)) { + if (kind === 'skill') { + const next = await loadSkills(process.cwd()); + session.setSkills(next); + return `removed skill ${bare}\nit is unloaded; the next turn no longer offers it`; + } + return `removed ${kind} ${bare}\nrestart shiro to unload it`; + } } throw new Error(`nothing installed under the name "${bare}"`); }, @@ -445,10 +460,17 @@ const hooks: AppHooks = { const servers = Object.entries(cfg.mcpServers ?? {}); if (servers.length === 0) return 'no MCP servers configured\n\n`/mcp add` sets one up.'; + const lazy = (mcp?.tools['mcp_list'] ?? undefined) !== undefined; const live = new Map(); - for (const name of Object.keys(mcp?.tools ?? {})) { - const server = /^mcp__([^_]+(?:_[^_]+)*)__/.exec(name)?.[1]; - if (server) live.set(server, (live.get(server) ?? 0) + 1); + if (lazy) { + // Lazy mode: tools are fetched on demand, so "connected" is what the handle + // says, not a count of registered mcp__ tools. + for (const name of mcp?.servers ?? []) live.set(name, -1); + } else { + for (const name of Object.keys(mcp?.tools ?? {})) { + const server = /^mcp__([^_]+(?:_[^_]+)*)__/.exec(name)?.[1]; + if (server) live.set(server, (live.get(server) ?? 0) + 1); + } } const failed = new Map((mcp?.errors ?? []).map((e) => [e.server, e.message])); @@ -457,7 +479,9 @@ const hooks: AppHooks = { const state = failed.has(name) ? `failed: ${failed.get(name)}` : live.has(name) - ? `${live.get(name)} tools` + ? live.get(name)! >= 0 + ? `${live.get(name)} tools` + : 'connected (lazy)' : has('--no-mcp') ? 'not connected (--no-mcp)' : 'not connected this session'; @@ -611,7 +635,15 @@ const facts: HeaderFact[] = [ memory && memory.all().length > 0 ? { label: 'memory', value: `${memory.all().length} notes about this project` } : undefined, - mcp && Object.keys(mcp.tools).length > 0 ? { label: 'mcp', value: `${Object.keys(mcp.tools).length} tools` } : undefined, + mcp && Object.keys(mcp.tools).length > 0 + ? { + label: 'mcp', + value: + mcp.servers.length > 0 + ? `${mcp.servers.length} ${mcp.servers.length === 1 ? 'server' : 'servers'} (lazy)` + : `${Object.keys(mcp.tools).length} tools`, + } + : undefined, !mcp && cfg.mcpServers && Object.keys(cfg.mcpServers).length > 0 ? { label: 'mcp', value: `${Object.keys(cfg.mcpServers).length} configured, not connected (--no-mcp)`, tone: 'warn' as const } : undefined, diff --git a/src/commands.ts b/src/commands.ts index 8004523..a7c5345 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -6,6 +6,8 @@ export type CommandAction = | { type: 'exit' } | { type: 'clear' } | { type: 'compact' } + | { type: 'undo'; what: 'both' | 'files' | 'conversation' } + | { type: 'redo'; what: 'both' | 'files' | 'conversation' } | { type: 'tools' } | { type: 'cost' } | { type: 'sessions' } @@ -57,6 +59,8 @@ export const COMMANDS: CommandSpec[] = [ { name: 'memory', summary: 'compact the project memory with the model' }, { name: 'tools', summary: 'list available tools' }, { name: 'compact', summary: 'replace history with a model-written summary' }, + { name: 'undo', arg: '[files|conversation]', summary: 'walk the last turn back: files, conversation, or both' }, + { name: 'redo', arg: '[conversation]', summary: 'put back what /undo took' }, { name: 'cost', summary: 'tokens and estimated spend this session' }, { name: 'sessions', summary: 'list saved sessions' }, { name: 'resume', arg: '', summary: 'load a saved session' }, @@ -161,6 +165,22 @@ function parseMcp(arg: string): CommandAction { } } +/** + * `/undo [files|conversation]` and `/redo [conversation]`. + * + * The default is `both` for undo, because restoring one without the other is the + * failure the two are meant to prevent: files back without the history and the model + * re-reads a change it no longer made. A bare `files` or `conversation` narrows it. + * Redo defaults to the conversation, since file content after the turn was never kept. + */ +function parseUndoKind(arg: string, fallback: 'both' | 'conversation'): 'both' | 'files' | 'conversation' { + const word = arg.trim().toLowerCase(); + if (word === 'files' || word === 'file') return 'files'; + if (word === 'conversation' || word === 'chat' || word === 'history') return 'conversation'; + if (word === 'both' || word === 'all' || word === '') return fallback; + return fallback; +} + /** * Pure parser: no IO, so the TUI and headless mode share one definition. * @@ -186,6 +206,10 @@ export function parseCommand(raw: string, custom: readonly CustomCommand[] = []) return { type: 'clear' }; case 'compact': return { type: 'compact' }; + case 'undo': + return { type: 'undo', what: parseUndoKind(arg, 'both') }; + case 'redo': + return { type: 'redo', what: parseUndoKind(arg, 'conversation') }; case 'tools': return { type: 'tools' }; case 'cost': diff --git a/src/config.ts b/src/config.ts index 9a5452c..d3b0b74 100644 --- a/src/config.ts +++ b/src/config.ts @@ -37,6 +37,11 @@ export type Config = { /** Index for `/registry`. Omit for the default one. */ registryUrl?: string; mcpServers?: Record; + /** + * How MCP tools reach the model: `lazy` registers meta-tools only (cheap until a + * tool is called), `eager` registers every server tool up front. Omit for lazy. + */ + mcpMode?: 'lazy' | 'eager'; }; const configPath = () => join(process.env['SHIRO_HOME'] ?? homedir(), '.shiro-neko', 'config.json'); diff --git a/src/mcp.ts b/src/mcp.ts index c885277..b0c5cc9 100644 --- a/src/mcp.ts +++ b/src/mcp.ts @@ -1,27 +1,47 @@ import { createMCPClient, type MCPClient } from '@ai-sdk/mcp'; import { Experimental_StdioMCPTransport } from '@ai-sdk/mcp/mcp-stdio'; -import type { ToolSet } from 'ai'; +import { tool, type ToolSet } from 'ai'; +import { z } from 'zod'; export type McpServerConfig = | { command: string; args?: string[]; env?: Record; cwd?: string } | { url: string; type?: 'http' | 'sse'; headers?: Record }; +/** + * How a server's tools reach the model. + * + * `eager` registers every tool with its schema up front — cheap for a two-tool + * server, a tax for one that exposes twenty. `lazy` registers only the three + * meta-tools below and fetches a server's tools on demand via `mcp_call`, so a + * configured server costs almost nothing in the request until a tool is actually + * invoked. + */ +export type McpMode = 'eager' | 'lazy'; + export type McpHandle = { tools: ToolSet; errors: { server: string; message: string }[]; + /** Server names, for the prompt's MCP line. Empty when the mode is eager. */ + servers: string[]; close: () => Promise; }; const isRemote = (c: McpServerConfig): c is Extract => 'url' in c; /** - * Connects every configured server and namespaces its tools as `mcp____` - * so two servers exposing `search` cannot silently shadow each other. - * A server that fails to start is reported, never fatal. + * Connects every configured server and exposes its tools. + * + * In `eager` mode the tools land in the returned set as `mcp____`, so + * two servers exposing `search` cannot silently shadow each other. In `lazy` mode the + * set holds only the three meta-tools and `servers` names the configured servers; a + * server that fails to start is reported, never fatal, in either mode. */ -export async function connectMcp(servers: Record): Promise { +export async function connectMcp( + servers: Record, + mode: McpMode = 'lazy', +): Promise { const clients: MCPClient[] = []; - const tools: ToolSet = {}; + const byName = new Map(); const errors: McpHandle['errors'] = []; await Promise.all( @@ -38,9 +58,7 @@ export async function connectMcp(servers: Record): Prom }), }); clients.push(client); - for (const [toolName, tool] of Object.entries(await client.tools())) { - tools[`mcp__${name}__${toolName}`] = tool; - } + byName.set(name, client); } catch (e) { errors.push({ server: name, message: e instanceof Error ? e.message : String(e) }); } @@ -48,10 +66,106 @@ export async function connectMcp(servers: Record): Prom ); return { - tools, + tools: mode === 'eager' ? await eagerTools(byName) : lazyTools(byName), errors, + servers: mode === 'eager' ? [] : [...byName.keys()], close: async () => { await Promise.all(clients.map((c) => c.close().catch(() => {}))); }, }; } + +/** Eager: every server tool gets an AI tool registered with its schema. */ +async function eagerTools(byName: Map): Promise { + const tools: ToolSet = {}; + await Promise.all( + [...byName.entries()].map(async ([name, client]) => { + for (const [toolName, t] of Object.entries(await client.tools())) { + tools[`mcp__${name}__${toolName}`] = t; + } + }), + ); + return tools; +} + +/** + * Cache of each client's AI tools, so `mcp_call` does not re-list on every call. + * The WeakMap drops entries when a client (and its session) is closed and collected. + */ +const clientToolsCache = new WeakMap(); + +/** Fetches a server's AI tools, or returns the cached set from a prior call. */ +async function cachedClientTools(client: MCPClient): Promise { + const cached = clientToolsCache.get(client); + if (cached) return cached; + const tools = await client.tools(); + clientToolsCache.set(client, tools); + return tools; +} + +/** + * Lazy: three meta-tools instead of every server schema. + * + * `mcp_list` names a server's tools from their definitions (cheap, no schema). + * `mcp_inspect` reads one tool's schema so the model knows its inputs. + * `mcp_call` executes one tool on its server, making the server's tools available + * only from the moment they are actually invoked. + */ +function lazyTools(byName: Map): ToolSet { + const connectionError = (name: string) => + byName.has(name) ? undefined : `unknown server "${name}". Configured: ${[...byName.keys()].join(', ') || 'none'}`; + + return { + mcp_list: tool({ + description: `List the tools exposed by an MCP server. Servers: ${[...byName.keys()].join(', ') || 'none'}.`, + inputSchema: z.object({ server: z.string().describe('Server name from your instructions') }), + execute: async ({ server }) => { + const err = connectionError(server); + if (err) throw new Error(err); + const client = byName.get(server)!; + const defs = await client.listTools(); + return defs.tools.length === 0 + ? `server "${server}" exposes no tools` + : defs.tools.map((d) => `- ${d.name}: ${d.description ?? 'no description'}`).join('\n'); + }, + }), + mcp_inspect: tool({ + description: `Inspect one tool's input schema on an MCP server. Servers: ${[...byName.keys()].join(', ') || 'none'}.`, + inputSchema: z.object({ + server: z.string().describe('Server name from your instructions'), + toolName: z.string().describe('Tool name, as listed by mcp_list'), + }), + execute: async ({ server, toolName }) => { + const err = connectionError(server); + if (err) throw new Error(err); + const client = byName.get(server)!; + const defs = await client.listTools(); + const def = defs.tools.find((d) => d.name === toolName); + if (!def) throw new Error(`No tool "${toolName}" on "${server}". List first with mcp_list.`); + return def.inputSchema ? JSON.stringify(def.inputSchema, null, 2) : `tool "${toolName}" declares no input schema`; + }, + }), + mcp_call: tool({ + description: + `Call one tool on an MCP server. Inspect its schema with mcp_inspect first. ` + + `Servers: ${[...byName.keys()].join(', ') || 'none'}.`, + inputSchema: z.object({ + server: z.string().describe('Server name from your instructions'), + toolName: z.string().describe('Tool name, as listed by mcp_list'), + args: z.record(z.string(), z.unknown()).describe('Arguments the tool expects, from mcp_inspect'), + }), + execute: async ({ server, toolName, args }) => { + const err = connectionError(server); + if (err) throw new Error(err); + const client = byName.get(server)!; + const serverTools = await cachedClientTools(client); + const aiTool = serverTools[toolName]; + if (!aiTool) + throw new Error( + `No tool "${toolName}" on "${server}". List first with mcp_list (server exposes: ${Object.keys(serverTools).join(', ') || 'none'}).`, + ); + return aiTool.execute!(args, { toolCallId: 'mcp_call', messages: [] } as never); + }, + }), + }; +} diff --git a/src/permission.ts b/src/permission.ts index e652975..2778a18 100644 --- a/src/permission.ts +++ b/src/permission.ts @@ -181,6 +181,11 @@ export const DEFAULT_PERMISSIONS: PermissionConfig = { apply_patch: 'ask', move_file: 'ask', delete_file: 'ask', + insert_lines: 'ask', + delete_lines: 'ask', + replace_lines: 'ask', + append_file: 'ask', + prepend_file: 'ask', bash: 'ask', web_fetch: 'ask', }; @@ -272,10 +277,6 @@ export class Permissions { this.granted.set(tool, set); } - granted_(tool: string): string[] { - return [...(this.granted.get(tool) ?? [])]; - } - /** The decision for one call, and which pattern decided it. */ check(tool: string, input: unknown): Resolved { const resolved = resolve(entryFor(tool, this.config), tool, input); diff --git a/src/pricing.ts b/src/pricing.ts index 181cb99..466d014 100644 --- a/src/pricing.ts +++ b/src/pricing.ts @@ -4,6 +4,10 @@ export type Rate = { inputPerMTok: number; outputPerMTok: number }; * USD per million tokens. Prefix match on the model id, longest first, so * `claude-sonnet-4-5-20250929` resolves via `claude-sonnet-4-5`. Published rates * drift, so this is a best-effort estimate rather than a billing source. + * + * Source: vendor pricing pages, checked 2026-09-17. Anthropic (Anthropic API, not + * Batch) and OpenAI listed rates; DeepSeek and Grok per their API pricing. Rates + * are for input, then output. Re-verify before trusting a live spend figure. */ const RATES: Record = { 'claude-opus-4': { inputPerMTok: 15, outputPerMTok: 75 }, diff --git a/src/prompt.ts b/src/prompt.ts index 50c79be..d5d85ba 100644 --- a/src/prompt.ts +++ b/src/prompt.ts @@ -131,8 +131,10 @@ function renderTools(available: readonly string[]): string { // free, and the schema already says what each takes. const git = extra.filter((n) => GIT_TOOL_NAMES.includes(n) && n !== 'git_commit_message'); const mcp = extra.filter((n) => n.startsWith('mcp__')); + const META = ['mcp_list', 'mcp_inspect', 'mcp_call']; + const lazyMcp = META.filter((n) => available.includes(n)); const other = extra.filter( - (n) => (!GIT_TOOL_NAMES.includes(n) || n === 'git_commit_message') && !n.startsWith('mcp__'), + (n) => (!GIT_TOOL_NAMES.includes(n) || n === 'git_commit_message') && !n.startsWith('mcp__') && !META.includes(n), ); if (git.length > 0) { @@ -140,7 +142,13 @@ function renderTools(available: readonly string[]): string { `- ${git.join(', ')}: read-only git, no approval needed. Use them instead of bash for history and diffs; they cannot mutate the repository.`, ); } - if (mcp.length > 0) { + if (lazyMcp.length > 0) { + // Lazy mode: the meta-tool descriptions already name the connected servers, so + // the model needs the workflow, not a schema listing. + lines.push( + `- ${lazyMcp.join(', ')}: MCP tools are fetched on demand. mcp_list names a server's tools, mcp_inspect reads one tool's schema, mcp_call runs it. Never guess a server or tool name: list first.`, + ); + } else if (mcp.length > 0) { lines.push( `- ${mcp.join(', ')}: from MCP servers, named mcp____. Each needs approval; read its own description before calling.`, ); diff --git a/src/prune.ts b/src/prune.ts index 1331afb..296bb27 100644 --- a/src/prune.ts +++ b/src/prune.ts @@ -92,8 +92,6 @@ export function detachOrphanedItems(before: ModelMessage[], after: ModelMessage[ export type PruneOptions = Parameters[0]; -const ANSWER_PARTS = new Set(['tool-result', 'tool-error']); - const anyParts = (message: ModelMessage): Part[] => Array.isArray(message.content) ? (message.content as Part[]) : []; @@ -167,6 +165,84 @@ export function prunePreservingItems(options: PruneOptions): ModelMessage[] { return dropOrphanedResults(detachOrphanedItems(options.messages, pruned)); } +/** + * The messages a prune would discard, so they can be summarized before they go. + * + * Compaction keeps the model's *memory of a turn* — the tool tail it is told to + * keep stays verbatim. What it does not keep is any statement of what was + * dropped. So a decision from forty messages ago vanishes silently, and the model + * contradicts it with full confidence, because as far as it can tell it never + * said that. + * + * Identity is by reference, not by value: `prunePreservingItems` rebuilds the + * surviving messages with `{ ...message }`, so a value comparison would report + * every message as changed and no message as dropped. `Set` on the object + * references is exact. + * + * Only messages that carry content worth summarizing are returned — an assistant + * turn consisting of nothing but a dropped `reasoning` part is not a decision, and + * summarizing "the model thought for a while" is worse than saying nothing. + */ +export function droppedBy(before: ModelMessage[], after: ModelMessage[]): ModelMessage[] { + const surviving = new Set(after); + return before.filter((message) => !surviving.has(message)); +} + +const ANSWER_PARTS = new Set(['tool-result', 'tool-error']); + +function textOf(message: ModelMessage): string { + const { content } = message; + if (typeof content === 'string') return content; + if (!Array.isArray(content)) return ''; + const chunks: string[] = []; + for (const part of content as Part[]) { + const p = part as Part & { text?: unknown; input?: unknown; output?: unknown }; + if (typeof p.text === 'string') chunks.push(p.text); + // A tool call's input is the decision made: the path, the command, the patch. + else if (p.type === 'tool-call' && p.input !== undefined) chunks.push(JSON.stringify(p.input)); + // A tool result is what came back. Without it a digest says what the model + // asked for and nothing about the answer, which is the half a later + // contradiction is usually argued from. + else if (ANSWER_PARTS.has(p.type) && p.output !== undefined) { + const rendered = typeof p.output === 'string' ? p.output : JSON.stringify(p.output); + chunks.push(rendered); + } + } + return chunks.join(' ').trim(); +} + +/** + * A one-line-per-message digest of what a prune wants to drop. + * + * This is the *fallback* when no summarizer is available or the call fails: crude, + * but it preserves the thing that matters — which tool touched which path, and in + * what order — rather than the nothing that is there today. The summarizer, when + * it runs, is a model and reads far better than this. + */ +export function digestOf(dropped: readonly ModelMessage[]): string { + const lines: string[] = []; + for (const message of dropped) { + const text = textOf(message); + if (!text) continue; + const role = message.role === 'tool' ? 'result' : message.role; + const clipped = text.length > 160 ? `${text.slice(0, 160)}...` : text; + lines.push(`- (${role}) ${clipped}`); + } + return lines.join('\n'); +} + +export const PRUNED_SPAN_PREFIX = 'Earlier in this session, now compacted away:'; + +export function isPrunedSpanSummary(message: ModelMessage): boolean { + return message.role === 'user' && typeof message.content === 'string' && message.content.startsWith(PRUNED_SPAN_PREFIX); +} + +export function prunedSpanMessage(summary: string | undefined, dropped: readonly ModelMessage[]): ModelMessage | undefined { + const body = summary?.trim() || digestOf(dropped); + if (!body) return undefined; + return { role: 'user', content: `${PRUNED_SPAN_PREFIX}\n\n${body}` }; +} + /** * How many trailing messages keep their tool content, widest first. * diff --git a/src/session.ts b/src/session.ts index e1bc9d2..fc46a13 100644 --- a/src/session.ts +++ b/src/session.ts @@ -17,7 +17,9 @@ import { Permissions, type PermissionConfig } from './permission'; import type { PluginHost } from './plugins'; import { costOf, formatUsd } from './pricing'; import { systemPrompt } from './prompt'; -import { detachProviderItems, pruneToFit } from './prune'; +import { detachProviderItems, digestOf, droppedBy, isPrunedSpanSummary, prunedSpanMessage, pruneToFit } from './prune'; +import { restore, Snapshots, type TurnSnapshot } from './snapshot'; +import { createStepBackTool, type LoopEntry } from './step-back'; import { createSkillTool, renderSkills, type Skill } from './skills'; import { disabledToolNames, onBashOutput, tools as builtinTools, type ToolSetName } from './tools'; @@ -38,6 +40,21 @@ export type ApprovalRequest = { /** 'once' runs this call only; 'always' whitelists the suggested pattern for the session. */ export type ApprovalDecision = 'once' | 'always' | 'deny'; +/** What an undo did, so the UI can say which files moved and which did not. */ +export type UndoResult = { + snapshot: TurnSnapshot; + restored: string[]; + removed: string[]; + conversationTrimmed: boolean; +}; + +export type RedoResult = { + snapshot: TurnSnapshot; + /** False when files were asked for: only pre-images are ever captured. */ + filesRestored: boolean; + what: 'both' | 'files' | 'conversation'; +}; + export type AgentEvent = | { type: 'text'; text: string } | { type: 'reasoning'; text: string } @@ -74,6 +91,8 @@ export type SessionOptions = { autoApprove?: readonly string[]; /** Prune the history once the estimated token count crosses this. */ compactThreshold?: number; + /** Identical calls to an allowed tool before it is asked about anyway. Default 3. */ + repeatLimit?: number; /** Retries per model call for transient failures. */ maxRetries?: number; /** AGENTS.md-style files appended to the system prompt. */ @@ -94,6 +113,12 @@ export type SessionOptions = { onNotebookChange?: (state: NotebookState) => void; }; +/** + * Length-based token estimate for deciding *when to prune*, not for billing. + * JSON char count / 4 approximates token count closely enough to gate compaction, + * but real billed tokens come from the SDK's reported usage (`inputTokens`), never + * from here. `/cost` and the budget ceiling use the SDK figure. + */ const estimateTokens = (messages: ModelMessage[]) => Math.round(JSON.stringify(messages).length / 4); /** Estimated tokens at which the wire history is pruned. */ @@ -102,6 +127,26 @@ const DEFAULT_COMPACT_THRESHOLD = 120_000; /** Identical calls in one turn before an allowed tool is asked about anyway. */ const REPEAT_LIMIT = 3; +/** + * Squashes a tool result into a few characters for the loop trace. + * + * The trace is fed back to the model verbatim, so a 30 KB read_file output would + * fill the reflection with noise. A short string keeps `step_back` honest about + * what happened without flooding the next context window. + */ +function summarizeToolResult(output: unknown): string { + if (typeof output === 'string') return output.length <= 80 ? output : `${output.slice(0, 80)}…(${output.length} chars)`; + try { + const json = JSON.stringify(output); + return json.length <= 80 ? json : `${json.slice(0, 80)}…`; + } catch { + return String(output); + } +} + +/** Ceiling on an injected span summary, so the summary cannot defeat the compaction. */ +const MAX_SPAN_SUMMARY_CHARS = 1_200; + const callKey = (toolName: string, input: unknown) => `${toolName}:${JSON.stringify(input ?? null)}`; /** @@ -121,6 +166,8 @@ export class Session { readonly messages: ModelMessage[]; readonly tools: ToolSet; readonly notebook: Notebook; + /** Pre-images of files this session's turns have changed, newest last. */ + readonly snapshots: Snapshots; inputTokens = 0; outputTokens = 0; /** Subagent token use, priced against the subagent's own model id in /cost. */ @@ -136,6 +183,14 @@ export class Session { /** The 80% spend warning is shown once, not on every turn past the line. */ private warnedSpend = false; private controller: AbortController | undefined; + /** Tools this turn used that no snapshot can cover, reported when the turn ends. */ + private readonly uncoveredTools = new Set(); + /** The snapshot the last undo removed, so `/redo` can put it back. */ + private lastUndone: TurnSnapshot | undefined; + /** Every tool call this turn, input + outcome, for the loop-detection tool to reflect on. */ + private readonly loopTrace: LoopEntry[] = []; + /** toolCallId -> { toolName, input }, so a result can be paired with its call. */ + private readonly callInputs = new Map(); constructor(private readonly opts: SessionOptions) { this.messages = opts.messages ?? []; @@ -143,12 +198,19 @@ export class Session { this.notebook.restore(opts.notebook); this.model = opts.model; this.variant = opts.agent ?? DEFAULT_VARIANT; + this.snapshots = new Snapshots(opts.cwd ?? process.cwd()); const sessionTools = { ...this.notebook.tools(), ...(opts.memory ? opts.memory.tools() : {}), - ...(opts.skills && opts.skills.length > 0 ? { skill: createSkillTool(opts.skills) } : {}), + // Always registered, even with no skills, so a mid-session install of the + // first skill is callable next turn without a session rebuild. The tool's + // description reads the live list and says "none" when it is empty. + skill: createSkillTool(() => this.opts.skills ?? []), ...(opts.ask ? { ask: createAskTool(opts.ask) } : {}), + step_back: createStepBackTool({ + trace: () => this.loopTrace, + }), }; this.tools = { ...builtinTools, ...sessionTools, ...(opts.plugins?.tools ?? {}), ...(opts.extraTools ?? {}) }; @@ -306,6 +368,26 @@ export class Session { return count; } + /** + * Appends a completed tool call to the loop trace, pairing it with its input. + * + * The SDK streams `tool-result` without the input that produced it, so the input is + * kept alongside on the `tool-call` part. A `step_back` call needs this pairing to + * say *which* call produced *which* outcome. + */ + private recordTrace(toolCallId: string, result: string): void { + const call = this.callInputs.get(toolCallId); + this.callInputs.delete(toolCallId); + if (!call) return; + this.loopTrace.push({ + step: this.loopTrace.length + 1, + toolName: call.toolName, + input: call.input, + result, + at: new Date().toISOString(), + }); + } + /** * Approval decisions, evaluated per call by the SDK. * @@ -326,6 +408,11 @@ export class Session { return async ({ toolCall }: { toolCall: { toolName: string; input: unknown } }) => { const { toolName, input } = toolCall; + // Before anything else, because the guard may deny the call and because a + // later hook must not be able to move the capture after the write. + const { covered } = await this.snapshots.captureFor(toolName, input); + if (!covered) this.uncoveredTools.add(toolName); + const blocked = await this.opts.plugins?.guard({ toolName, input, @@ -346,7 +433,18 @@ export class Session { } const repeats = this.repeatCount(toolName, input); - if (decision === 'allow' && repeats < REPEAT_LIMIT) return undefined; + const limit = this.opts.repeatLimit ?? REPEAT_LIMIT; + if (decision === 'allow' && repeats < limit) return undefined; + + if (decision === 'allow') { + // Repeated three times with a permission that says `allow`: the model is + // looping, not asking, and it should stop and look at the trace rather than + // burn another approval. This is the point the step_back tool exists for. + notices.push( + `You have called ${toolName} with the same input ${repeats + 1} times this turn. It is not making progress. ` + + `Use step_back to reflect on what changed between attempts, then try a different approach or stop.`, + ); + } why.set(callKey(toolName, input), { ...(pattern ? { matchedPattern: pattern } : {}), @@ -378,6 +476,92 @@ export class Session { return { before, after: this.messages.length }; } + /** + * Walks the last turn back: files, conversation, or both. + * + * The three-way split is the point. Restoring files without the conversation leaves + * the model believing edits are on disk that are not, so its next turn is built on a + * state that no longer exists — it re-reads a file expecting its own change and finds + * the original, which reads to the model as the change having been rejected. Restoring + * the conversation without the files is the mirror: the model forgets it made an edit + * that is still there. So the default is both, and the caller can narrow it. + * + * Returns undefined when there is nothing to undo, which the UI reports as such + * rather than as a failure. + */ + async undo(what: 'both' | 'files' | 'conversation' = 'both'): Promise { + const snap = this.snapshots.pop(); + if (!snap) return undefined; + + const files = what === 'conversation' ? { restored: [], removed: [] } : await restore(snap, this.snapshots.cwdOf()); + if (what !== 'files') this.trimTo(snap.messageCount); + + this.lastUndone = snap; + return { snapshot: snap, ...files, conversationTrimmed: what !== 'files' }; + } + + /** + * Puts back what `undo` took, without a second snapshot. + * + * A redo cannot restore file content from the session, because the content that + * existed after the turn was never captured — only the pre-image was. So a redo of + * the files is declined honestly rather than approximated: the whole point of undo + * is that the user trusts what it says it did. The conversation is restored from the + * snapshot's own record, which is exact. + */ + redo(what: 'both' | 'files' | 'conversation' = 'conversation'): RedoResult | undefined { + const snap = this.lastUndone; + if (!snap) return undefined; + + this.snapshots.push(snap); + this.lastUndone = undefined; + return { snapshot: snap, filesRestored: false, what }; + } + + undoable(): readonly TurnSnapshot[] { + return this.snapshots.list(); + } + + private trimTo(length: number): void { + if (this.messages.length <= length) return; + this.messages.length = Math.max(0, length); + this.opts.onChange?.(this.messages); + } + + /** + * Closes the open snapshot and reports what the turn could not cover. + * + * Called before every `done`, not from `send`'s `finally`, because a notice that + * arrives after `done` is a notice the UI has already stopped listening for — + * `done` is what a consumer treats as the end of the turn and stops on. + * + * Two reasons to speak, and the second does not depend on the first: a turn with no + * snapshotted edits can still have run `bash` and changed the tree, which is exactly + * when the warning matters most. + */ + private *closingNotices(): Generator { + const snapshot = this.snapshots.commit(); + const uncovered = [...this.uncoveredTools]; + if (uncovered.length === 0) return; + const covered = snapshot ? `${snapshot.files.length} file(s) changed this turn and can be undone with /undo. ` : ''; + yield { + type: 'notice', + text: `${covered}${uncovered.join(', ')} ran this turn and cannot be snapshotted, so any changes it made will not be undone.`, + }; + } + + /** + * Swaps the live skill list at a turn boundary. + * + * The `skill` tool and the system-prompt catalogue both read the list on each + * call, so replacing it here is atomic across the two and takes effect on the + * next turn with no session rebuild. The caller is responsible for only doing + * this between turns — a turn in flight already holds its rules. + */ + setSkills(skills: Skill[]): void { + this.opts.skills = skills; + } + async *send(userText: string): AsyncGenerator { // The ceiling is checked before the model is: a turn started past the limit // would spend money the caller said not to. An unpriced model cannot be @@ -402,7 +586,12 @@ export class Session { // Per turn, not per step: a tool called once in each of three steps is the // loop this guards against. this.seen.clear(); + this.uncoveredTools.clear(); + this.loopTrace.length = 0; this.staleItemsRepaired = false; + // Opened before the model runs and closed after it stops, so every write the + // turn makes lands in one snapshot the user can walk back to. + this.snapshots.begin(userText, this.messages.length); const outputs: Extract[] = []; onBashOutput(({ toolCallId, chunk }) => { @@ -414,6 +603,10 @@ export class Session { yield* this.run(signal, threshold, outputs); } finally { onBashOutput(undefined); + // Belt and braces: closingNotices runs before every `done`, but an aborted turn + // can return through a path that never reached it, and an uncommitted snapshot + // would then be silently dropped rather than kept. + this.snapshots.commit(); await this.opts.plugins?.afterTurn(); } } @@ -431,6 +624,70 @@ export class Session { return true; } + /** + * Prunes the canonical history once a run has landed. + * + * prepareStep only trims the wire copy for the next request; without this write-back + * the stored history keeps growing, the context meter pins at 100%, and every later + * turn re-prunes the same messages from scratch. + */ + private async compactCanonical(threshold: number): Promise<{ before: number; after: number } | null> { + const before = this.messages.length; + if (estimateTokens(this.messages) <= threshold) return null; + const pruned = pruneToFit({ messages: this.messages, threshold, estimate: estimateTokens }); + if (pruned.length === before) return null; + const dropped = droppedBy(this.messages, pruned); + const summary = await this.summarizeSpan(dropped.filter((m) => !isPrunedSpanSummary(m))); + this.replace(this.withSummarizedSpan(summary, pruned, dropped)); + return { before, after: this.messages.length }; + } + + /** + * Puts a summary of the discarded span at the head of the history it was dropped from. + * + * Without this the model is told which tool results to keep and nothing about what + * was dropped, so it states a decision it made forty messages ago as though it had + * never made it. + * + * The summary is bounded two ways because a summary that grows with the session + * defeats the point of compacting at all: the input is capped at the span's own + * digest, and the output is capped by instruction and by hard truncation. A failed + * or empty call falls back to the digest, which costs nothing and still carries + * which tool touched which path — the part a contradiction is usually built from. + */ + private async summarizeSpan(dropped: readonly ModelMessage[]): Promise { + const digest = digestOf(dropped); + if (!digest) return undefined; + try { + const { text } = await generateText({ + model: this.model, + system: + 'These lines are the condensed record of an earlier part of a coding session that has been ' + + 'compacted out of the conversation. Write at most 120 words of notes capturing decisions made, ' + + 'files touched, commands run and their outcome, and anything still pending. State only what the ' + + 'lines support; do not invent detail and do not address the reader.', + messages: [{ role: 'user', content: digest }], + maxRetries: this.opts.maxRetries ?? 3, + }); + const trimmed = text.trim(); + return trimmed ? trimmed.slice(0, MAX_SPAN_SUMMARY_CHARS) : undefined; + } catch { + // A summarizer that cannot run must not cost the turn its compaction: the + // digest is a worse record, not an absent one. + return undefined; + } + } + + private withSummarizedSpan(summary: string | undefined, pruned: ModelMessage[], dropped: readonly ModelMessage[]): ModelMessage[] { + const worthSummarizing = dropped.filter((m) => !isPrunedSpanSummary(m)); + if (worthSummarizing.length === 0) return pruned; + + const prior = dropped.filter(isPrunedSpanSummary); + const spanMessage = prunedSpanMessage(summary, worthSummarizing); + if (!spanMessage) return pruned; + return [...prior, spanMessage, ...pruned]; + } + private async *run( signal: AbortSignal, threshold: number, @@ -506,12 +763,15 @@ export class Session { yield { type: 'tool-start', id: part.id, name: part.toolName }; break; case 'tool-call': + this.callInputs.set(part.toolCallId, { toolName: part.toolName, input: JSON.stringify(part.input ?? null) }); yield { type: 'tool-call', id: part.toolCallId, name: part.toolName, input: part.input }; break; case 'tool-result': + this.recordTrace(part.toolCallId, summarizeToolResult(part.output)); yield { type: 'tool-result', id: part.toolCallId, name: part.toolName, output: part.output }; break; case 'tool-error': + this.recordTrace(part.toolCallId, `error: ${part.error instanceof Error ? part.error.message : String(part.error)}`); yield { type: 'tool-error', id: part.toolCallId, name: part.toolName, error: part.error }; break; case 'tool-approval-request': { @@ -535,6 +795,7 @@ export class Session { yield { type: 'tool-denied', name: part.toolName }; break; case 'abort': + yield* this.closingNotices(); yield { type: 'done' }; return; case 'error': @@ -555,6 +816,7 @@ export class Session { } } catch (error) { if (signal.aborted) { + yield* this.closingNotices(); yield { type: 'done' }; return; } @@ -579,7 +841,10 @@ export class Session { while (guardNotices.length > 0) yield { type: 'notice', text: guardNotices.shift()! }; this.messages.push(...(await result.responseMessages)); - this.opts.onChange?.(this.messages); + + // prepareStep already emits `compacted` at the same threshold crossing, and + // replace() fires onChange on the fold path; both would double up otherwise. + if (!(await this.compactCanonical(threshold))) this.opts.onChange?.(this.messages); if (pending.length === 0) { const usage = await result.usage; @@ -595,6 +860,7 @@ export class Session { text: `approaching spend ceiling: ${formatUsd(spend.usd ?? 0)} of ${formatUsd(spend.ceiling ?? 0)} used`, }; } + yield* this.closingNotices(); yield { type: 'done', inputTokens: usage.inputTokens, outputTokens: usage.outputTokens }; return; } diff --git a/src/skills.ts b/src/skills.ts index 3173142..1cffd30 100644 --- a/src/skills.ts +++ b/src/skills.ts @@ -100,18 +100,28 @@ export function renderSkills(skills: Skill[]): string { ].join('\n'); } -export function createSkillTool(skills: Skill[]) { - const names = skills.map((s) => s.name); +/** + * The `skill` tool, reading the list live so a mid-session install shows up on the + * next turn without a restart. + * + * The catalogue in the system prompt and the names in this tool's description both + * come from the same list on each read, so replacing the list at a turn boundary + * makes both stale bytes atomic: a skill the model can call it can see already. + */ +export function createSkillTool(getSkills: () => Skill[]) { + const names = () => getSkills().map((s) => s.name).join(', '); return tool({ description: 'Load a skill: detailed instructions for one kind of task. Call it as soon as a skill description matches ' + - `what you are about to do, then follow what it says. Available: ${names.join(', ') || 'none'}.`, + `what you are about to do, then follow what it says. Available: ${names() || 'none'}.`, inputSchema: z.object({ name: z.string().describe('Skill name from the list in your instructions'), }), execute: async ({ name }) => { + const skills = getSkills(); const skill = skills.find((s) => s.name === name.trim().toLowerCase()); - if (!skill) throw new Error(`No skill named "${name}". Available: ${names.join(', ') || 'none'}`); + if (!skill) + throw new Error(`No skill named "${name}". Available: ${skills.map((s) => s.name).join(', ') || 'none'}`); return `Skill "${skill.name}" (${skill.origin}). Follow these instructions for this task.\n\n${skill.body}`; }, }); diff --git a/src/snapshot.ts b/src/snapshot.ts new file mode 100644 index 0000000..93eea2f --- /dev/null +++ b/src/snapshot.ts @@ -0,0 +1,269 @@ +import { createHash } from 'node:crypto'; +import { join, relative, resolve, sep } from 'node:path'; + +/** + * Pre-images of files a turn is about to change, so a turn can be walked back. + * + * The design follows the one thing every comparable CLI agrees on and the one thing + * they all get wrong in the same way. Agreement: a snapshot is taken *before* the + * turn runs, because after it runs the original content is gone. The shared flaw: a + * snapshot only covers the tools that write through a known interface — Claude Code + * tracks Write/Edit/NotebookEdit and explicitly not `bash` — so the undo is partial + * and the user has to know which half they are in. + * + * Two decisions fall out of taking that seriously. + * + * 1. Capture lazily, per file, not by walking the tree. A session turn may touch + * three files out of fifty thousand; a full-tree copy per prompt is a tarball of + * the repository the user did not ask for and cannot afford. Instead the first + * write to a path records its pre-image, and a later write to the same path in + * the same turn does not overwrite it — the pre-image is the state before the + * *turn*, which is what an undo restores. + * + * 2. Record what is *not* covered rather than implying it is. A `bash` command that + * rewrites a file, a `git checkout`, a build artifact — none of these pass through + * a path argument the way `write_file` does, so none are captured. The tool + * reports which paths it restored and the caller is told the rest is unknown, + * which is the same honesty `bash` already owes an interrupted command. + */ + +/** One file as it was before the turn that changed it. `before === undefined` means it did not exist. */ +export type PreImage = { + /** Workspace-relative, using forward slashes, so a restore is portable across platforms. */ + path: string; + before: string | undefined; +}; + +export type TurnSnapshot = { + /** Monotonic turn number, so the UI can name what is being undone. */ + turn: number; + at: string; + /** The user prompt that opened the turn, for a menu that lists them. */ + prompt: string; + /** Files this turn changed, with their content from before it started. */ + files: PreImage[]; + /** The conversation length when the turn began, so undo can trim it back. */ + messageCount: number; +}; + +/** Claude Code keeps 100; beyond that the memory is worth more than the recall. */ +export const MAX_SNAPSHOTS = 100; + +/** A single file larger than this is not snapshotted; a 40 MB binary is not an edit. */ +const MAX_FILE_BYTES = 2 * 1024 * 1024; + +/** + * The workspace-relative, slash-normalised form of a path, or undefined if it is + * outside the workspace. + * + * Outside is refused rather than clamped: a path that escapes the workspace is not + * something this repository can undo, and recording it would imply an undo that + * cannot happen. Paths are normalised to forward slashes because a snapshot written + * on Windows may be read on a machine where a backslash is a filename character. + */ +export function relPath(cwd: string, abs: string): string | undefined { + const root = resolve(cwd); + const target = resolve(abs); + const rel = relative(root, target); + if (rel === '' || rel.startsWith('..') || rel.includes(`..${sep}`)) return undefined; + return rel.split(sep).join('/'); +} + +/** + * The absolute path a tool call will write to, when there is exactly one. + * + * Deliberately a small, explicit map rather than a guess. `multi_edit` and the line + * editors each take a single `path`; `move_file` takes `from` and `to` and both are + * recorded; `delete_file` takes a `path`. `apply_patch` carries its paths inside the + * patch text, and `bash` carries none — both are reported as uncovered rather than + * silently not snapshotted. + */ +export function touchedPaths(toolName: string, input: unknown): { paths: string[]; covered: boolean } { + const o = (input ?? {}) as Record; + const one = (key: string) => (typeof o[key] === 'string' ? [o[key] as string] : []); + + switch (toolName) { + case 'write_file': + case 'edit_file': + case 'multi_edit': + case 'delete_file': + case 'insert_lines': + case 'delete_lines': + case 'replace_lines': + case 'append_file': + case 'prepend_file': + return { paths: one('path'), covered: true }; + case 'move_file': + return { paths: [...one('from'), ...one('to')], covered: true }; + case 'apply_patch': { + const patch = typeof o['patch'] === 'string' ? (o['patch'] as string) : ''; + const paths = [...patch.matchAll(/^\*\*\* (?:Add|Update|Delete) File: (.+)$/gm)].map((m) => m[1]!.trim()); + const moves = [...patch.matchAll(/^\*\*\* Move to: (.+)$/gm)].map((m) => m[1]!.trim()); + return { paths: [...paths, ...moves], covered: true }; + } + case 'bash': + // Arbitrary code: an untouched-looking `node -e` can rewrite the tree. + return { paths: [], covered: false }; + default: + return { paths: [], covered: true }; + } +} + +/** + * Records pre-images for the files a turn changes, and restores them on undo. + * + * One instance per session. Holds at most MAX_SNAPSHOTS turns; the oldest falls off + * the front, because the turn someone wants back is almost always the last one. + */ +export class Snapshots { + private readonly turns: TurnSnapshot[] = []; + private current: { turn: number; at: string; prompt: string; files: Map; messageCount: number } | null = + null; + private next = 1; + private readonly cwd: string; + + constructor(cwd: string = process.cwd()) { + this.cwd = cwd; + } + + /** Opens a turn. Called once per user prompt, before the model runs. */ + begin(prompt: string, messageCount: number): void { + this.current = { turn: this.next++, at: new Date().toISOString(), prompt, files: new Map(), messageCount }; + } + + /** + * Records a file's content before a tool changes it, on the first write of the turn. + * + * Idempotent per path per turn: the second `write_file` to the same path in one turn + * must not replace the pre-image with the intermediate content the first write left, + * because undo restores the turn's starting state, not the midpoint. + * + * Read failures are swallowed. A snapshot is a convenience; a tool call that fails + * because the snapshot layer could not read an unrelated path would be worse than no + * undo at all. + */ + async capture(absPath: string): Promise { + if (!this.current) return; + const rel = relPath(this.cwd, absPath); + if (rel === undefined) return; + if (this.current.files.has(rel)) return; + + try { + const file = Bun.file(absPath); + const exists = await file.exists(); + if (!exists) { + this.current.files.set(rel, undefined); + return; + } + if (file.size > MAX_FILE_BYTES) return; + this.current.files.set(rel, await file.text()); + } catch { + return; + } + } + + /** Records any path a tool call is about to touch. Returns whether the tool is covered at all. */ + async captureFor(toolName: string, input: unknown): Promise<{ covered: boolean; paths: string[] }> { + const { paths, covered } = touchedPaths(toolName, input); + for (const p of paths) await this.capture(resolve(this.cwd, p)); + return { paths, covered }; + } + + /** + * Closes the turn, keeping it only if it changed something. + * + * A turn that read and answered without writing is not worth a slot, and keeping it + * would make `/undo` step past a turn that has nothing to undo — which reads as the + * command being broken. + */ + commit(): TurnSnapshot | undefined { + const cur = this.current; + this.current = null; + if (!cur || cur.files.size === 0) return undefined; + + const snap: TurnSnapshot = { + turn: cur.turn, + at: cur.at, + prompt: cur.prompt, + files: [...cur.files].map(([path, before]) => ({ path, before })), + messageCount: cur.messageCount, + }; + this.turns.push(snap); + while (this.turns.length > MAX_SNAPSHOTS) this.turns.shift(); + return snap; + } + + /** Discards the open turn without recording it, for an aborted or failed turn. */ + discard(): void { + this.current = null; + } + + /** The turns that can be undone, newest first. */ + list(): readonly TurnSnapshot[] { + return [...this.turns].reverse(); + } + + /** Whether there is an open turn collecting pre-images right now. */ + get open(): boolean { + return this.current !== null; + } + + /** + * Removes the newest turn and returns what it holds, without restoring. + * + * Separated from restoring so the caller can decide *what* to bring back — + * files, conversation, or both — which is the split Claude Code's rewind menu + * exposes and the reason one control surface is worth more than three commands. + */ + pop(): TurnSnapshot | undefined { + return this.turns.pop(); + } + + /** Puts a turn back, for a `/redo` that follows an `/undo`. */ + push(snap: TurnSnapshot): void { + this.turns.push(snap); + } + + get size(): number { + return this.turns.length; + } + + cwdOf(): string { + return this.cwd; + } +} + +/** Writes a pre-image back to disk, recreating a deleted file or removing one that was created. */ +export async function restore(snap: TurnSnapshot, cwd = process.cwd()): Promise<{ restored: string[]; removed: string[] }> { + const restored: string[] = []; + const removed: string[] = []; + + for (const file of snap.files) { + const abs = join(cwd, file.path); + if (file.before === undefined) { + // The file did not exist before the turn, so undoing its creation is removing it. + const f = Bun.file(abs); + if (await f.exists()) { + await f.delete(); + removed.push(file.path); + } + continue; + } + await Bun.write(abs, file.before); + restored.push(file.path); + } + + return { restored, removed }; +} + +/** A short, stable label for a snapshot, for a menu that lists several. */ +export function labelOf(snap: TurnSnapshot): string { + const first = snap.prompt.trim().split('\n')[0] ?? ''; + const clipped = first.length > 50 ? `${first.slice(0, 50)}...` : first || '(no prompt)'; + return `turn ${snap.turn}: ${clipped}`; +} + +/** A content hash, used to tell whether a file still matches what the snapshot holds. */ +export function hashOf(text: string): string { + return createHash('sha256').update(text).digest('hex').slice(0, 12); +} diff --git a/src/step-back.ts b/src/step-back.ts new file mode 100644 index 0000000..755d64f --- /dev/null +++ b/src/step-back.ts @@ -0,0 +1,80 @@ +import { tool } from 'ai'; +import { z } from 'zod'; + +/** + * A visible escape hatch for the loop a coding agent dies in. + * + * The repeat guard stops an *identical* call after three tries, but the deeper loop + * is the model making *different* calls that all amount to the same stalled attempt — + * re-reading the same file expecting a different answer, retrying a failing command + * with a tweaked flag, re-sending a prompt it has already asked. No equal-input + * detector fires on any of that, so the model burns the step budget on motions that + * never move. + * + * `step_back` exists so the model has a *named* way out instead of only a guard it + * cannot see. The session records each completed tool call (input + outcome) into a + * loop trace; the tool returns a scripted reflection prompt built from that trace, + * so the model is told in concrete terms that it is going in circles and is steered + * to change direction. + */ + +export type LoopEntry = { + step: number; + toolName: string; + input: string; + result: string; + at: string; +}; + +function recentTrace(entries: readonly LoopEntry[], window = 8): LoopEntry[] { + return entries.slice(-window); +} + +function renderTrace(entries: readonly LoopEntry[], maxLines = 12): string { + return entries.slice(-maxLines).map((e) => `step ${e.step}: ${e.toolName} ${e.input} -> ${e.result}`.slice(0, 200)).join('\n'); +} + +/** + * Builds a `step_back` tool bound to a session's loop trace. + * + * The tool is meant to be called when the model is not making progress — a trap it + * cannot always see while it is inside it. The returned reflection names the recent + * steps so the model can tell, from its own calls, that it is going in circles, and + * gives it the one thing a stuck agent is usually missing: permission to stop, + * say what it learned, and change direction rather than try harder. + */ +export function createStepBackTool(opts: { trace: () => readonly LoopEntry[] }) { + return tool({ + description: + 'Use when you are stuck: the same file is not changing, a command keeps failing, or you have done several steps with no visible progress. ' + + 'Records your recent steps and returns a reflection prompt to help you change direction instead of repeating the attempt.', + inputSchema: z.object({ + note: z.string().optional().describe('A sentence in your own words about what you were trying to do.'), + }), + execute: async ({ note }) => { + const trace = recentTrace(opts.trace()); + if (trace.length === 0) { + return ( + 'No recent steps to reflect on. This is an early call of step_back — it is only useful when you have ' + + 'attempted something several times. Describe what you are stuck on in `note`.' + ); + } + + return [ + 'You have run threadbare over the last steps and are stuck. Here is what you actually did:', + '```', + renderTrace(trace), + '```', + '', + 'Before your next tool call, answer these three questions in your reasoning:', + '1. What exactly is wrong — the input, the tool, or the expectation?', + '2. What have you tried, and why did each fail?', + '3. What is ONE different thing you can do that is not "try the same thing a little harder"?', + '', + 'Then take that different action. If the failure is a command, read the actual error and fix its cause — ', + 'do not rerun the command. If a file is not what you expect, suspect your assumption about it and re-read it fresh.', + note?.trim() ? `\nYour note: ${note.trim()}` : '', + ].join('\n'); + }, + }); +} \ No newline at end of file diff --git a/src/subagent.ts b/src/subagent.ts index f17dda2..912dd1a 100644 --- a/src/subagent.ts +++ b/src/subagent.ts @@ -177,91 +177,137 @@ export function createTaskTool(opts: { ? 'explore: read-only research. review: read-only critique. worker: makes changes. Default explore.' : 'explore: find and report. review: critique code for defects. Default explore.', ), + tasks: z + .array( + z.object({ + description: z.string(), + prompt: z.string(), + kind: z.enum(canWrite ? ['explore', 'review', 'worker'] : ['explore', 'review']).optional(), + }), + ) + .optional() + .describe( + 'Independent investigations to run at the same time instead of one after another. ' + + 'Use this for several unrelated searches so they overlap in wall-clock time. Each runs on its own ' + + 'context window, exactly like a single task call. Default: run the single prompt above.', + ), }), - execute: async ({ description, prompt, kind }, { abortSignal }) => { - const flavour: SubagentKind = kind ?? 'explore'; - if (flavour === 'worker' && !opts.approve) { - throw new Error('The worker kind needs an approval channel, which this session has not provided.'); - } - - const id = `sub${++counter}`; - const report = opts.report; - report?.({ type: 'start', id, kind: flavour, description }); - - let steps = 0; - let text = ''; - let usedTokens: { inputTokens: number; outputTokens: number } | undefined; - - try { - // `explore` is search, not reasoning, so it runs on the cheaper model when - // one is configured. `review` and `worker` keep the parent's: they judge - // and they change, both of which want the full model. - const model = flavour === 'explore' ? (opts.subagentModel ?? opts.model) : opts.model; - const result = streamText({ - model, - system: PROMPTS[flavour](opts.cwd ?? process.cwd()), - messages: [{ role: 'user', content: prompt }], - tools: TOOLS[flavour], - stopWhen: isStepCount(opts.maxSteps ?? 20), - ...(opts.approve - ? { - toolApproval: async ({ toolCall }: { toolCall: { toolName: string; input: unknown } }) => { - const approved = await opts.approve!(toolCall); - return approved - ? undefined - : { type: 'denied' as const, reason: 'The user denied this call. Stop and report it.' }; - }, - } - : {}), - ...(abortSignal ? { abortSignal } : {}), - }); - - const sink = () => {}; - void result.responseMessages.then(undefined, sink); - void result.usage.then(undefined, sink); - void result.steps.then(undefined, sink); - void result.finalStep.then(undefined, sink); - void result.finishReason.then(undefined, sink); - - for await (const part of result.stream) { - if (part.type === 'tool-call') { - steps++; - report?.({ type: 'step', id, tool: part.toolName, summary: summarize(part.input) }); - } else if (part.type === 'tool-result') { - report?.({ type: 'result', id, tool: part.toolName, summary: outcome(part.output), ok: true }); - } else if (part.type === 'tool-error') { - const message = part.error instanceof Error ? part.error.message : String(part.error); - report?.({ type: 'result', id, tool: part.toolName, summary: outcome(message), ok: false }); - } else if (part.type === 'text-delta') { - text += part.text; - } else if (part.type === 'error') { - // A provider failure arrives as a stream part, not a throw, so it has to - // be rethrown here or the subagent silently returns nothing. - const message = part.error instanceof Error ? part.error.message : String(part.error); - throw part.error instanceof Error ? part.error : new Error(message); - } - } - - try { - const usage = await result.usage; - usedTokens = { inputTokens: usage.inputTokens ?? 0, outputTokens: usage.outputTokens ?? 0 }; - } catch { - // A run that errored before producing usage has nothing to account for. - } - } catch (e) { - const message = e instanceof Error ? e.message : String(e); - report?.({ type: 'error', id, message }); - throw e; - } - - const trimmed = text.trim(); - report?.({ type: 'end', id, ok: trimmed.length > 0, steps }); - // Settled after the stream closes; a failed run reports nothing rather than - // a half count. The parent prices these against the subagent's own model id. - if (usedTokens) opts.onUsage?.({ kind: flavour, ...usedTokens }); - return trimmed || 'Subagent returned no findings.'; + execute: async ({ description, prompt, kind, tasks }, { abortSignal }) => { + const plans = tasks?.length ? tasks : [{ description, prompt, kind }]; + const runs = await Promise.all( + plans.map((p) => runSubagent({ ...opts, abortSignal }, { description: p.description, prompt: p.prompt, kind: p.kind })), + ); + if (runs.length === 1) return runs[0]!.report; + return runs.map((r) => `## ${r.named}\n\n${r.report}`).join('\n\n---\n\n'); }, }); } +/** Flavour of one planned investigation; `kind` defaults to explore. */ +type Plan = { description: string; prompt: string; kind?: string }; + +/** + * Runs one subagent and settles its report, usage, and panel events. + * + * Extracted so `tasks` can fan several out in parallel: each full run is + * independent — its own id, own stream, own spend — and they overlap simply by + * awaiting them together. + */ +async function runSubagent( + opts: { + model: LanguageModel; + subagentModel?: LanguageModel; + subagentModelId?: string; + cwd?: string; + maxSteps?: number; + report?: SubagentReporter; + approve?: SubagentApproval; + onUsage?: (usage: { kind: SubagentKind; inputTokens: number; outputTokens: number }) => void; + abortSignal?: AbortSignal; + }, + plan: Plan, +): Promise<{ named: string; report: string }> { + const flavour: SubagentKind = (plan.kind as SubagentKind | undefined) ?? 'explore'; + if (flavour === 'worker' && !opts.approve) { + throw new Error('The worker kind needs an approval channel, which this session has not provided.'); + } + + const id = `sub${++counter}`; + const report = opts.report; + report?.({ type: 'start', id, kind: flavour, description: plan.description }); + + let steps = 0; + let text = ''; + let usedTokens: { inputTokens: number; outputTokens: number } | undefined; + + try { + // `explore` is search, not reasoning, so it runs on the cheaper model when + // one is configured. `review` and `worker` keep the parent's: they judge + // and they change, both of which want the full model. + const model = flavour === 'explore' ? (opts.subagentModel ?? opts.model) : opts.model; + const result = streamText({ + model, + system: PROMPTS[flavour](opts.cwd ?? process.cwd()), + messages: [{ role: 'user', content: plan.prompt }], + tools: TOOLS[flavour], + stopWhen: isStepCount(opts.maxSteps ?? 20), + ...(opts.approve + ? { + toolApproval: async ({ toolCall }: { toolCall: { toolName: string; input: unknown } }) => { + const approved = await opts.approve!(toolCall); + return approved + ? undefined + : { type: 'denied' as const, reason: 'The user denied this call. Stop and report it.' }; + }, + } + : {}), + ...(opts.abortSignal ? { abortSignal: opts.abortSignal } : {}), + }); + + const sink = () => {}; + void result.responseMessages.then(undefined, sink); + void result.usage.then(undefined, sink); + void result.steps.then(undefined, sink); + void result.finalStep.then(undefined, sink); + void result.finishReason.then(undefined, sink); + + for await (const part of result.stream) { + if (part.type === 'tool-call') { + steps++; + report?.({ type: 'step', id, tool: part.toolName, summary: summarize(part.input) }); + } else if (part.type === 'tool-result') { + report?.({ type: 'result', id, tool: part.toolName, summary: outcome(part.output), ok: true }); + } else if (part.type === 'tool-error') { + const message = part.error instanceof Error ? part.error.message : String(part.error); + report?.({ type: 'result', id, tool: part.toolName, summary: outcome(message), ok: false }); + } else if (part.type === 'text-delta') { + text += part.text; + } else if (part.type === 'error') { + // A provider failure arrives as a stream part, not a throw, so it has to + // be rethrown here or the subagent silently returns nothing. + const message = part.error instanceof Error ? part.error.message : String(part.error); + throw part.error instanceof Error ? part.error : new Error(message); + } + } + + try { + const usage = await result.usage; + usedTokens = { inputTokens: usage.inputTokens ?? 0, outputTokens: usage.outputTokens ?? 0 }; + } catch { + // A run that errored before producing usage has nothing to account for. + } + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + report?.({ type: 'error', id, message }); + throw e; + } + + const trimmed = text.trim(); + report?.({ type: 'end', id, ok: trimmed.length > 0, steps }); + // Settled after the stream closes; a failed run reports nothing rather than + // a half count. The parent prices these against the subagent's own model id. + if (usedTokens) opts.onUsage?.({ kind: flavour, ...usedTokens }); + return { named: plan.description, report: trimmed || 'Subagent returned no findings.' }; +} + export const TASK_TOOL_NAME = 'task'; diff --git a/src/tool-kinds.ts b/src/tool-kinds.ts new file mode 100644 index 0000000..d2db585 --- /dev/null +++ b/src/tool-kinds.ts @@ -0,0 +1,27 @@ +import type { Tool } from 'ai'; + +/** + * Marks a tool as mutating where it is defined, rather than in a list beside it. + * + * The bug this prevents is the worst one this codebase can have: a new tool that + * writes to the workspace, added to `tools` but forgotten in a hand-maintained + * `MUTATING_TOOLS`, is a write the permission layer does not treat as a write. It + * is silent, it passes every test that does not think to check the new name, and + * it surfaces as a user discovering an edit they never approved. + * + * The mark is a property on the tool object, so `MUTATING_TOOLS` can be derived by + * filtering the registry instead of being typed out. A tool that is not marked is + * asserted non-mutating by `tools.test.ts`, which means the decision is made once, + * at the definition, and cannot drift. + */ +export const MUTATING = '__mutating' as const; + +/** A tool that can change the workspace or run arbitrary code. */ +export function mutating(t: T): T { + return Object.assign(t, { [MUTATING]: true as const }); +} + +/** Whether a tool was declared mutating at its definition site. */ +export function isMutating(t: unknown): boolean { + return typeof t === 'object' && t !== null && (t as Record)[MUTATING] === true; +} diff --git a/src/tools-extra.ts b/src/tools-extra.ts index 566cb00..b88e2f4 100644 --- a/src/tools-extra.ts +++ b/src/tools-extra.ts @@ -3,6 +3,7 @@ import { stat } from 'node:fs/promises'; import { resolve } from 'node:path'; import { z } from 'zod'; import { jail, posix, walk } from './ignore'; +import { mutating } from './tool-kinds'; import { git } from './tools-git'; /** @@ -36,7 +37,7 @@ async function readLines(path: string): Promise<{ abs: string; lines: string[] } // edit // --------------------------------------------------------------------------- -export const insertLinesTool = tool({ +export const insertLinesTool = mutating(tool({ description: 'Insert lines at a 1-based position in a file, pushing the rest down. Cheaper and safer than a rewrite for adding a block in the middle.', inputSchema: z.object({ @@ -51,9 +52,9 @@ export const insertLinesTool = tool({ await Bun.write(abs, cur.join('\n')); return `Inserted ${lines(text).length} line(s) at ${path}:${line}`; }, -}); +})); -export const deleteLinesTool = tool({ +export const deleteLinesTool = mutating(tool({ description: 'Delete an inclusive range of lines from a file. Refuses to delete the whole file; use delete_file for that.', inputSchema: z.object({ path: z.string(), @@ -69,9 +70,9 @@ export const deleteLinesTool = tool({ await Bun.write(abs, cur.join('\n')); return `Deleted lines ${start}-${end} from ${path}`; }, -}); +})); -export const replaceLinesTool = tool({ +export const replaceLinesTool = mutating(tool({ description: 'Replace an inclusive range of lines with new text, in one write.', inputSchema: z.object({ path: z.string(), @@ -87,9 +88,9 @@ export const replaceLinesTool = tool({ await Bun.write(abs, cur.join('\n')); return `Replaced lines ${start}-${end} in ${path}`; }, -}); +})); -export const appendFileTool = tool({ +export const appendFileTool = mutating(tool({ description: 'Append text to the end of a file without reading the whole thing into the edit.', inputSchema: z.object({ path: z.string(), text: z.string() }), execute: async ({ path, text }) => { @@ -97,9 +98,9 @@ export const appendFileTool = tool({ await Bun.write(abs, `${cur.join('\n').replace(/\n?$/, '\n')}${text.replace(/\n?$/, '')}\n`); return `Appended ${lines(text).length} line(s) to ${path}`; }, -}); +})); -export const prependFileTool = tool({ +export const prependFileTool = mutating(tool({ description: 'Prepend text to the start of a file, e.g. a license header or an import block.', inputSchema: z.object({ path: z.string(), text: z.string() }), execute: async ({ path, text }) => { @@ -107,7 +108,7 @@ export const prependFileTool = tool({ await Bun.write(abs, `${text.replace(/\n?$/, '\n')}${cur.join('\n')}`); return `Prepended ${lines(text).length} line(s) to ${path}`; }, -}); +})); export const countLinesTool = tool({ description: 'Count lines in one file, or per file across a glob. A quick size read before deciding to open something large.', diff --git a/src/tools.ts b/src/tools.ts index c765ae0..324becd 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -3,6 +3,7 @@ import { stat } from 'node:fs/promises'; import { join, resolve } from 'node:path'; import { z } from 'zod'; import { jail, posix, walk } from './ignore'; +import { isMutating, mutating } from './tool-kinds'; import { EXTRA_TOOL_NAMES, extraTools } from './tools-extra'; import { GIT_TOOL_NAMES, gitTools } from './tools-git'; import { NET_TOOL_NAMES, netTools } from './tools-net'; @@ -174,7 +175,7 @@ export function parsePatch(patch: string): PatchOp[] { return ops; } -export const applyPatchTool = tool({ +export const applyPatchTool = mutating(tool({ description: 'Apply one patch across several files: add, update, move, and delete in a single call. All or nothing — if any ' + 'part fails, nothing is written. Use it when a change spans files that must land together, such as a rename ' + @@ -248,7 +249,7 @@ export const applyPatchTool = tool({ return `Applied ${ops.length} change${ops.length === 1 ? '' : 's'}:\n${summary.map((s) => `- ${s}`).join('\n')}`; }, -}); +})); /** * A rewrite that collapses whitespace: similar character count, a fraction of the lines. @@ -264,7 +265,7 @@ function collapsedRewrite(before: string, after: string): boolean { return after.split('\n').length < before.split('\n').length / 2; } -export const writeFileTool = tool({ +export const writeFileTool = mutating(tool({ description: 'Create a file or overwrite it completely. Prefer edit_file for existing files.', inputSchema: z.object({ path: z.string(), @@ -284,17 +285,39 @@ export const writeFileTool = tool({ } return `Wrote ${content.length} chars to ${path}`; }, -}); +})); -export const editFileTool = tool({ +// Some models (Claude-style tool docs, DeepSeek/GLM) emit snake_case edit params +// (old_string/new_string/replace_all) despite the camelCase schema. Normalize at the +// boundary instead of failing the whole call on a naming convention. +const SNAKE_EDIT_ARGS: ReadonlyArray = [ + ['old_string', 'oldString'], + ['new_string', 'newString'], + ['replace_all', 'replaceAll'], +]; + +export function normalizeEditArgs(input: unknown): unknown { + if (typeof input !== 'object' || input === null) return input; + const obj: Record = { ...(input as Record) }; + for (const [snake, camel] of SNAKE_EDIT_ARGS) { + if (obj[snake] !== undefined && obj[camel] === undefined) obj[camel] = obj[snake]; + } + if (Array.isArray(obj.edits)) obj.edits = obj.edits.map(normalizeEditArgs); + return obj; +} + +export const editFileTool = mutating(tool({ description: 'Replace an exact string in a file. oldString must appear exactly once unless replaceAll is true. Include surrounding context to make oldString unique.', - inputSchema: z.object({ - path: z.string(), - oldString: z.string().describe('Exact text to find, including whitespace and indentation'), - newString: z.string().describe('Replacement text'), - replaceAll: z.boolean().optional().describe('Replace every occurrence instead of requiring exactly one'), - }), + inputSchema: z.preprocess( + normalizeEditArgs, + z.object({ + path: z.string(), + oldString: z.string().describe('Exact text to find, including whitespace and indentation'), + newString: z.string().describe('Replacement text'), + replaceAll: z.boolean().optional().describe('Replace every occurrence instead of requiring exactly one'), + }), + ), execute: async ({ path, oldString, newString, replaceAll = false }) => { if (oldString === newString) throw new Error('oldString and newString are identical'); const abs = jail(path); @@ -312,27 +335,30 @@ export const editFileTool = tool({ await Bun.write(abs, after); return `Replaced ${replaceAll ? count : 1} occurrence(s) in ${path}`; }, -}); +})); -export const multiEditTool = tool({ +export const multiEditTool = mutating(tool({ description: 'Apply several exact-string edits to one file in a single call. Each edit sees the result of the previous one. ' + 'All or nothing: if any oldString fails to match, or matches more than once without replaceAll, nothing is ' + 'written. Prefer this over repeated edit_file calls on the same file — one approval, one write, no risk of ' + 'leaving the file half-changed.', - inputSchema: z.object({ - path: z.string(), - edits: z - .array( - z.object({ - oldString: z.string().describe('Exact text to find, including whitespace and indentation'), - newString: z.string().describe('Replacement text'), - replaceAll: z.boolean().optional(), - }), - ) - .min(1) - .describe('Edits in the order they should be applied'), - }), + inputSchema: z.preprocess( + normalizeEditArgs, + z.object({ + path: z.string(), + edits: z + .array( + z.object({ + oldString: z.string().describe('Exact text to find, including whitespace and indentation'), + newString: z.string().describe('Replacement text'), + replaceAll: z.boolean().optional(), + }), + ) + .min(1) + .describe('Edits in the order they should be applied'), + }), + ), execute: async ({ path, edits }) => { const abs = jail(path); const file = Bun.file(abs); @@ -367,7 +393,7 @@ export const multiEditTool = tool({ await Bun.write(abs, text); return `Applied ${edits.length} edit(s) to ${path} (${applied.join(', ')})`; }, -}); +})); export const globTool = tool({ description: @@ -573,7 +599,13 @@ async function pump( return all; } -type Running = { command: string; proc: Bun.Subprocess; interrupted: boolean; killed?: Promise }; +type Running = { + command: string; + proc: Bun.Subprocess; + interrupted: boolean; + timedOut?: boolean; + killed?: Promise; +}; const running = new Map(); @@ -615,6 +647,12 @@ function killTree(proc: Bun.Subprocess): Promise { export function interruptBash(): string[] { const killed: string[] = []; for (const entry of running.values()) { + // A second ctrl-c while the first killTree is still settling must not re-announce + // the same command: the notice is the only proof the keypress did anything. + if (entry.interrupted) { + killed.push(entry.command); + continue; + } entry.interrupted = true; entry.killed = killTree(entry.proc); killed.push(entry.command); @@ -622,7 +660,7 @@ export function interruptBash(): string[] { return killed; } -export const bashTool = tool({ +export const bashTool = mutating(tool({ description: 'Run a shell command in the workspace root. Use for builds, tests, git, and package managers. ' + 'Output streams live and the user can interrupt a command with ctrl-c without ending the turn.', @@ -636,13 +674,32 @@ export const bashTool = tool({ cwd: process.cwd(), stdout: 'pipe', stderr: 'pipe', - timeout, - ...(abortSignal ? { signal: abortSignal } : {}), }); const entry: Running = { command, proc, interrupted: false }; running.set(toolCallId, entry); + // Bun's spawn `signal` option is not used either: it kills only the shell, so an + // esc-abort orphaned the grandchild on the same still-open pipes as the timeout + // did. The abort must go through killTree, exactly like ctrl-c does. + const onAbort = () => { + if (entry.interrupted) return; + entry.interrupted = true; + entry.killed = killTree(proc); + }; + abortSignal?.addEventListener('abort', onAbort); + // The turn may already be aborted by the time this tool starts; a past event + // never re-fires, so check once here or the command runs unkillable by esc. + if (abortSignal?.aborted) onAbort(); + + // Bun's own `timeout` spawn option is not used: it kills only the shell, and the + // grandchild holding the output pipes keeps `pump` reading forever, so the tool + // never returns. Same failure killTree exists for, just triggered by the clock. + const timer = setTimeout(() => { + entry.timedOut = true; + entry.killed = killTree(proc); + }, timeout); + try { // Drained concurrently: a command that fills one pipe while we block on the // other would deadlock, and buffering both hides progress for minutes. @@ -658,6 +715,15 @@ export const bashTool = tool({ // Thrown rather than returned: the model must not read a killed command as // a command that ran and failed on its own terms. + if (entry.timedOut) { + throw new Error( + cap( + `The command exceeded its ${timeout}ms timeout and was killed. It did not finish, so its effects are unknown.\n${ + body || '(no output before it was killed)' + }`, + ), + ); + } if (entry.interrupted) { throw new Error( cap( @@ -678,15 +744,17 @@ export const bashTool = tool({ .join('\n\n'), ); } finally { + clearTimeout(timer); + abortSignal?.removeEventListener('abort', onAbort); // Awaited so the process really is gone before the tool returns. On Windows a // surviving grandchild holds the cwd open, which breaks the very next command. await entry.killed; running.delete(toolCallId); } }, -}); +})); -export const moveFileTool = tool({ +export const moveFileTool = mutating(tool({ description: 'Move or rename one file. Creates the target directory. Refuses if the source is missing or the target ' + 'already exists, so a rename cannot silently overwrite work. For a rename plus its callers in one step, ' + @@ -708,9 +776,9 @@ export const moveFileTool = tool({ await file.delete(); return `Moved ${from} to ${to}`; }, -}); +})); -export const deleteFileTool = tool({ +export const deleteFileTool = mutating(tool({ description: 'Delete one file. Refuses a directory: removing a tree is what the guard plugin blocks in bash, and it is ' + 'not something to do implicitly. Delete the files you mean, one call each.', @@ -733,7 +801,7 @@ export const deleteFileTool = tool({ await Bun.file(abs).delete(); return `Deleted ${path} (${entry.size} bytes)`; }, -}); +})); /** * Definition patterns for `find_symbol`, keyed loosely by language. @@ -909,15 +977,16 @@ export function disabledToolNames(enabled: readonly ToolSetName[] | undefined): return TOOL_SET_NAMES.filter((set) => !live.has(set)).flatMap((set) => [...TOOL_SETS[set]]); } -/** Tools that mutate the workspace or run arbitrary code always ask the user first. */ -export const MUTATING_TOOLS = [ - 'write_file', - 'edit_file', - 'multi_edit', - 'apply_patch', - 'move_file', - 'delete_file', - 'bash', -] as const; +/** + * Tools that mutate the workspace or run arbitrary code always ask the user first. + * + * Derived from the tools themselves rather than typed out: a tool marked `mutating` + * at its definition is in this list by construction, and there is no second place to + * forget it. `tools.test.ts` asserts the converse — that nothing here is unmarked — + * so the two cannot disagree. + */ +export const MUTATING_TOOLS: readonly string[] = Object.entries(tools) + .filter(([, t]) => isMutating(t)) + .map(([name]) => name); export { jail }; diff --git a/src/ui/App.tsx b/src/ui/App.tsx index 523af02..cb405a8 100644 --- a/src/ui/App.tsx +++ b/src/ui/App.tsx @@ -176,18 +176,34 @@ export function App({ const fileMatches = token && paths ? matchPaths(paths, token.query) : []; const highlightedPath = fileMatches[Math.min(fileIndex, Math.max(0, fileMatches.length - 1))]; - // The walk costs a full ignore-aware traversal, so it happens on the first `@` - // rather than at startup, and only once. + // The walk costs a full ignore-aware traversal, so it runs once at the first `@`. + // A slow cooldown re-walks so a file created after that first `@` shows up within + // a short window instead of staying hidden all session. The cooldown never fires + // on a short session, so the "walks once" behaviour most users see is unchanged. + const pathsRef = useRef(paths); useEffect(() => { - if (token === undefined || paths !== undefined) return; - let live = true; - void hooks.listPaths().then((all) => { - if (live) setPaths(all); - }); - return () => { - live = false; - }; - }, [hooks, paths, token]); + pathsRef.current = paths; + }, [paths]); + const didLoadRef = useRef(false); + useEffect(() => { + if (token === undefined) return; + if (!didLoadRef.current) { + didLoadRef.current = true; + let live = true; + void hooks.listPaths().then((all) => { + if (live) setPaths(all); + }); + const refresh = setInterval(async () => { + if (!live) return; + const all = await hooks.listPaths(); + if (live && JSON.stringify(all) !== JSON.stringify(pathsRef.current)) setPaths(all); + }, 10_000); + return () => { + live = false; + clearInterval(refresh); + }; + } + }, [hooks, token]); useEffect(() => bridge.bind(setPending), [bridge]); useEffect(() => askBridge?.bind(setAsking), [askBridge]); @@ -691,6 +707,48 @@ export function App({ setModelPicker(models); return; } + case 'undo': { + push({ kind: 'user', text: chosen.trim() }); + setWorking(true); + try { + const result = await session.undo(action.what); + if (!result) { + push({ kind: 'info', text: 'nothing to undo' }); + } else { + const parts: string[] = []; + if (result.restored.length > 0) parts.push(`restored ${result.restored.join(', ')}`); + if (result.removed.length > 0) parts.push(`removed ${result.removed.join(', ')}`); + if (result.conversationTrimmed) parts.push(`dropped ${result.snapshot.messageCount}-onward from the history`); + push({ kind: 'info', text: `undid turn ${result.snapshot.turn}: ${parts.join('; ') || 'no file changes'}` }); + // The same limit the turn-end notice states: bash is not snapshotted. + push({ + kind: 'info', + text: 'Only file-tool edits are covered. A bash command or a git checkout in that turn is not, so check git status if one ran.', + }); + } + } catch (e) { + push({ kind: 'error', text: e instanceof Error ? e.message : String(e) }); + } + setWorking(false); + return; + } + case 'redo': { + push({ kind: 'user', text: chosen.trim() }); + const result = session.redo(action.what); + if (!result) { + push({ kind: 'info', text: 'nothing to redo' }); + } else if (action.what === 'files' || action.what === 'both') { + // Refused rather than approximated: only the pre-image was captured, so + // there is no post-turn content to put back. + push({ + kind: 'info', + text: `put turn ${result.snapshot.turn} back on the undo stack. File contents cannot be re-applied - only the state before the turn was recorded.`, + }); + } else { + push({ kind: 'info', text: `put turn ${result.snapshot.turn} back; the conversation was restored to it` }); + } + return; + } case 'compact': { push({ kind: 'user', text: chosen.trim() }); setWorking(true); diff --git a/src/ui/Approval.tsx b/src/ui/Approval.tsx index 67be5ab..940742d 100644 --- a/src/ui/Approval.tsx +++ b/src/ui/Approval.tsx @@ -1,6 +1,7 @@ import { Box, Text, useInput } from 'ink'; import React from 'react'; import type { ApprovalDecision, ApprovalRequest } from '../session'; +import { normalizeEditArgs } from '../tools'; import { Diff } from './Diff'; import { accent, glyph } from './theme'; import { toolDetail } from './transcript'; @@ -42,7 +43,7 @@ export function createApprovalBridge(): ApprovalBridge { * consistent and far more readable than a JSON dump of the input. */ function ApprovalDetail({ name, input }: { name: string; input: unknown }) { - const o = (input ?? {}) as Record; + const o = (normalizeEditArgs(input) ?? {}) as Record; if (name === 'write_file') { const content = String(o['content'] ?? ''); diff --git a/src/ui/PromptInput.tsx b/src/ui/PromptInput.tsx index 8f6ba3f..fb3e081 100644 --- a/src/ui/PromptInput.tsx +++ b/src/ui/PromptInput.tsx @@ -33,10 +33,6 @@ type KeyLike = { end?: boolean; }; -const INVERSE_ON = '\u001B[7m'; -const INVERSE_OFF = '\u001B[27m'; -const invert = (s: string) => `${INVERSE_ON}${s}${INVERSE_OFF}`; - /** * Text input with a real cursor and shell-style history recall. * @@ -151,10 +147,10 @@ export function PromptInput({ ); if (value.length === 0) { - if (!placeholder) return {focus ? invert(' ') : ' '}; + if (!placeholder) return {' '}; return ( - {focus ? invert(placeholder.slice(0, 1)) : placeholder.slice(0, 1)} + {placeholder.slice(0, 1)} {placeholder.slice(1)} ); @@ -166,7 +162,7 @@ export function PromptInput({ return ( {shown.slice(0, cursor)} - {invert(shown.slice(cursor, cursor + 1) || ' ')} + {shown.slice(cursor, cursor + 1) || ' '} {shown.slice(cursor + 1)} ); diff --git a/src/ui/transcript.ts b/src/ui/transcript.ts index f22afc0..13d497a 100644 --- a/src/ui/transcript.ts +++ b/src/ui/transcript.ts @@ -1,4 +1,5 @@ import { TODO_MARK } from '../notebook'; +import { normalizeEditArgs } from '../tools'; export type Line = | { key: string; kind: 'user'; text: string } @@ -38,7 +39,8 @@ export function preview(input: unknown): string { * the transcript, beside the spinner while a call is in flight, and in the approval * prompt for any tool without a diff of its own. */ -export function toolDetail(name: string, input: unknown): string[] { +export function toolDetail(name: string, rawInput: unknown): string[] { + const input = normalizeEditArgs(rawInput); if (input === null || typeof input !== 'object') return []; const o = input as Record; const str = (k: string) => (typeof o[k] === 'string' ? (o[k] as string) : undefined); diff --git a/test/cli.test.ts b/test/cli.test.ts new file mode 100644 index 0000000..45da0c5 --- /dev/null +++ b/test/cli.test.ts @@ -0,0 +1,156 @@ +import { afterAll, beforeAll, expect, test } from 'bun:test'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { VERSION } from '../src/version'; + +/** + * The CLI entry point is an executable, not a module: importing it parses argv, + * loads config, connects MCP, and renders Ink. Nothing in `test/` imports it for + * that reason, and its flag surface was previously exercised only by hand. + * + * So it is tested the way a user meets it — by running the real entry point in a + * child process — with two things held constant. `SHIRO_HOME` points at a scratch + * directory, and every API-key variable is stripped, so the "no key" branches are + * deterministic rather than accidentally satisfied by the developer's shell. The + * entry is invoked by absolute path from a scratch cwd, so the repository's own + * `config.json`, `AGENTS.md`, and `.shiro/` entries stay out of the run. + * + * Only paths that exit before `render()` are covered; anything past it needs a + * TTY. That still reaches every argument-parsing decision, which is the point. + */ + +const ROOT = join(import.meta.dir, '..'); +const ENTRY = join(ROOT, 'src', 'cli.tsx'); + +/** Credential variables that would otherwise decide `cfg.apiKey` for us. */ +const KEY_VARS = [ + 'SHIRO_API_KEY', + 'ANTHROPIC_API_KEY', + 'OPENAI_API_KEY', + 'SHIRO_PROVIDER', + 'SHIRO_MODEL', + 'SHIRO_BASE_URL', +]; + +let home: string; +let scratch: string; + +beforeAll(() => { + home = mkdtempSync(join(tmpdir(), 'shiro-cli-home-')); + scratch = mkdtempSync(join(tmpdir(), 'shiro-cli-cwd-')); +}); + +afterAll(() => { + rmSync(home, { recursive: true, force: true }); + rmSync(scratch, { recursive: true, force: true }); +}); + +function cleanEnv(shiroHome: string): Record { + // Bun.spawn's `env` replaces the environment, so start from a copy of ours and + // remove the credentials rather than hand-assembling a minimal one. + const env: Record = {}; + for (const [k, v] of Object.entries(process.env)) if (v !== undefined) env[k] = v; + for (const k of KEY_VARS) delete env[k]; + env['SHIRO_HOME'] = shiroHome; + return env; +} + +async function cli(args: string[], shiroHome = home): Promise<{ stdout: string; stderr: string; code: number }> { + const proc = Bun.spawn([process.execPath, ENTRY, ...args], { + cwd: scratch, + env: cleanEnv(shiroHome), + // /dev/null, so `readStdin` sees EOF instead of blocking on a pipe nobody closes. + stdin: 'ignore', + stdout: 'pipe', + stderr: 'pipe', + }); + const [stdout, stderr, code] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { stdout, stderr, code }; +} + +test('--help prints usage and exits 0', async () => { + const { stdout, code } = await cli(['--help']); + expect(code).toBe(0); + expect(stdout).toContain('usage: shiro'); + expect(stdout).toContain('--yolo'); + expect(stdout).toContain('--print'); + expect(stdout).toContain('--agent'); +}); + +test('-h is the same as --help', async () => { + const { stdout, code } = await cli(['-h']); + expect(code).toBe(0); + expect(stdout).toContain('usage: shiro'); +}); + +test('--version prints the version line and exits 0', async () => { + const { stdout, code } = await cli(['--version']); + expect(code).toBe(0); + expect(stdout).toContain(`shiro-neko ${VERSION}`); + expect(stdout).toContain(process.platform); +}); + +test('-v is the same as --version', async () => { + const { stdout, code } = await cli(['-v']); + expect(code).toBe(0); + expect(stdout).toContain(`shiro-neko ${VERSION}`); +}); + +test('an unknown --agent fails with the valid list', async () => { + const { stderr, code } = await cli(['--agent', 'turbo']); + expect(code).toBe(1); + expect(stderr).toContain('Unknown agent "turbo"'); + expect(stderr).toContain('default, quick, deep, plan, review'); +}); + +test('an unknown --think fails with the valid list', async () => { + const { stderr, code } = await cli(['--think', 'turbo']); + expect(code).toBe(1); + expect(stderr).toContain('Unknown thinking level "turbo"'); + expect(stderr).toContain('off, low, medium, high, max'); +}); + +test('-p without a key refuses to run headless', async () => { + const { stderr, code } = await cli(['-p', 'hello']); + expect(code).toBe(1); + expect(stderr).toContain('No API key for provider "anthropic"'); +}); + +test('--resume with no matching session fails', async () => { + const { stderr, code } = await cli(['-r', 'nosuchid']); + expect(code).toBe(1); + expect(stderr).toContain('no session matching "nosuchid"'); +}); + +test('--continue with no saved session fails', async () => { + const { stderr, code } = await cli(['-c']); + expect(code).toBe(1); + expect(stderr).toContain('no saved session for this directory'); +}); + +test('a configured key with no prompt and no stdin reports the missing prompt', async () => { + // A second home, so the key file this writes does not leak into the tests above. + const keyed = mkdtempSync(join(tmpdir(), 'shiro-cli-keyed-')); + try { + mkdirSync(join(keyed, '.shiro-neko'), { recursive: true }); + await Bun.write( + join(keyed, '.shiro-neko', 'config.json'), + `${JSON.stringify({ provider: 'anthropic', model: 'claude-sonnet-4-5', apiKey: 'sk-test' }, null, 2)}\n`, + ); + // The full isolation flag set is passed so this also proves they all parse and + // the module boots with them; with an empty stdin, `-p` must report the prompt. + const { stderr, code } = await cli( + ['-p', '--no-plugins', '--no-skills', '--no-memory', '--no-instructions', '--no-mcp', '--no-subagent'], + keyed, + ); + expect(code).toBe(1); + expect(stderr).toContain('needs a prompt'); + } finally { + rmSync(keyed, { recursive: true, force: true }); + } +}); diff --git a/test/commit.test.ts b/test/commit.test.ts index 11246b9..ce75bed 100644 --- a/test/commit.test.ts +++ b/test/commit.test.ts @@ -1,7 +1,7 @@ -import { usageOf } from './helpers'; +import { generateResult, streamOf, textChunks, usageOf } from './helpers'; import { afterEach, beforeEach, expect, test } from 'bun:test'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; -import type { LanguageModelV4CallOptions, LanguageModelV4StreamPart } from '@ai-sdk/provider'; +import { MockLanguageModelV4 } from 'ai/test'; +import type { LanguageModelV4CallOptions } from '@ai-sdk/provider'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -15,16 +15,7 @@ import { GIT_TOOL_NAMES } from '../src/tools-git'; const usage = usageOf(10); -const stream = (parts: LanguageModelV4StreamPart[]) => ({ - stream: simulateReadableStream({ chunks: parts, chunkDelayInMs: null, initialDelayInMs: null }), -}); - -const text = (body: string): LanguageModelV4StreamPart[] => [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: body }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, -]; +const text = (body: string) => textChunks(body, usage); let dir: string; let origCwd: string; @@ -69,16 +60,11 @@ function recordingModel( const model = new MockLanguageModelV4({ doStream: async (o) => { seen.push(o); - return stream(text(reply)); + return streamOf(text(reply)); }, doGenerate: async (o) => { seen.push(o); - return { - content: [{ type: 'text', text: reply }], - finishReason: { unified: 'stop', raw: 'stop' }, - usage, - warnings: [], - } as any; + return generateResult(reply, usage); }, }); return { model, seen }; diff --git a/test/compact.test.ts b/test/compact.test.ts index 91ee634..2f42466 100644 --- a/test/compact.test.ts +++ b/test/compact.test.ts @@ -1,25 +1,19 @@ -import { usageOf } from './helpers'; +import { generateResult, streamOf, textChunks, usageOf } from './helpers'; import { expect, test } from 'bun:test'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import type { LanguageModelV4CallOptions, LanguageModelV4StreamPart } from '@ai-sdk/provider'; import { APICallError, type ModelMessage } from 'ai'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { Session, type AgentEvent } from '../src/session'; +import { isPrunedSpanSummary, PRUNED_SPAN_PREFIX } from '../src/prune'; const usage = usageOf(10); -const stream = (parts: LanguageModelV4StreamPart[]) => ({ - stream: simulateReadableStream({ chunks: parts, chunkDelayInMs: null, initialDelayInMs: null }), -}); +const stream = (parts: LanguageModelV4StreamPart[]) => streamOf(parts); -const text = (body: string): LanguageModelV4StreamPart[] => [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: body }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, -]; +const text = (body: string) => textChunks(body, usage); /** One assistant turn carrying a bulky tool call plus its result. */ function bulkyExchange(i: number): ModelMessage[] { @@ -86,8 +80,10 @@ test('history over the threshold is pruned before reaching the model', async () const sentSize = JSON.stringify(seen[0]?.prompt).length; expect(sentSize).toBeLessThan(JSON.stringify(messages).length); - // Pruning is for the wire only; the local history keeps every message. - expect(session.messages.length).toBeGreaterThan(messages.length); + // The fold is written back: the canonical history drops from ~4 bulky exchanges to + // the widest ladder rung that fits (or the narrowest, if none does). A history left + // at its full size pins the context meter and re-prunes from scratch every turn. + expect(session.estimatedTokens()).toBeLessThan(beforeTokens); expect(events).toContain('compacted'); }); @@ -132,12 +128,7 @@ test('summarize replaces the whole history with one summary message', async () = model: new MockLanguageModelV4({ doGenerate: async () => { generateCalls++; - return { - content: [{ type: 'text', text: '- goal: add pagination\n- touched: src/users.ts\n- todo: add tests' }], - finishReason: { unified: 'stop', raw: 'stop' }, - usage, - warnings: [], - } as any; + return generateResult('- goal: add pagination\n- touched: src/users.ts\n- todo: add tests', usage); }, }), askApproval: async () => 'deny', @@ -156,7 +147,7 @@ test('summarize replaces the whole history with one summary message', async () = test('summarize on an empty session is a no-op', async () => { const session = new Session({ - model: new MockLanguageModelV4({ doGenerate: async () => ({}) as any }), + model: new MockLanguageModelV4({ doGenerate: async () => generateResult('') }), askApproval: async () => 'deny', }); expect(await session.summarize()).toEqual({ before: 0, after: 0 }); @@ -451,3 +442,93 @@ test('a stale item arriving after text was streamed is reported, not silently re expect(call).toBe(1); }); + +test('a pruned span is replaced by a summary of what was dropped', async () => { + const messages = [ + { role: 'user' as const, content: 'we decided on the ladder approach' }, + ...bulkyExchange(0), + ...bulkyExchange(1), + ...bulkyExchange(2), + ...bulkyExchange(3), + ]; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(text('ok')), + doGenerate: async () => generateResult('chose the ladder; touched f0-f3.ts'), + }), + askApproval: async () => 'deny', + messages: [...messages], + compactThreshold: 1000, + }); + + for await (const _ of session.send('next')) void _; + + const span = session.messages.filter((m) => isPrunedSpanSummary(m)); + expect(span).toHaveLength(1); + expect(String(span[0]?.content)).toContain('chose the ladder'); +}); + +test('the span summary is injected before the surviving history, not after', async () => { + const messages = [...bulkyExchange(0), ...bulkyExchange(1), ...bulkyExchange(2), ...bulkyExchange(3)]; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(text('ok')), + doGenerate: async () => generateResult('summary of the dropped span'), + }), + askApproval: async () => 'deny', + messages: [...messages], + compactThreshold: 1000, + }); + + for await (const _ of session.send('next')) void _; + + const markerAt = session.messages.findIndex((m) => isPrunedSpanSummary(m)); + expect(markerAt).toBe(0); +}); + +test('a summarizer that throws still leaves a digest rather than nothing', async () => { + const messages = [...bulkyExchange(0), ...bulkyExchange(1), ...bulkyExchange(2), ...bulkyExchange(3)]; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(text('ok')), + doGenerate: async () => { + throw new Error('summarizer is down'); + }, + }), + askApproval: async () => 'deny', + messages: [...messages], + compactThreshold: 1000, + }); + + for await (const _ of session.send('next')) void _; + + const span = session.messages.find((m) => isPrunedSpanSummary(m)); + expect(span).toBeDefined(); + expect(String(span?.content)).toContain('f0.ts'); +}); + +test('repeated compaction does not nest one span summary inside another', async () => { + const messages = [...bulkyExchange(0), ...bulkyExchange(1), ...bulkyExchange(2), ...bulkyExchange(3)]; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(text('ok')), + doGenerate: async () => generateResult('span notes'), + }), + askApproval: async () => 'deny', + messages: [...messages], + compactThreshold: 1000, + }); + + for await (const _ of session.send('next')) void _; + for await (const _ of session.send('again')) void _; + for await (const _ of session.send('and again')) void _; + + const spans = session.messages.filter((m) => isPrunedSpanSummary(m)); + // One summary is kept from each compaction, but no summary may contain the + // marker text, which would mean a summary of a summary. + for (const s of spans) { + const body = String(s.content).slice(PRUNED_SPAN_PREFIX.length); + expect(body).not.toContain(PRUNED_SPAN_PREFIX); + } +}); + diff --git a/test/complete-ui.test.tsx b/test/complete-ui.test.tsx index 833bd87..6f7e865 100644 --- a/test/complete-ui.test.tsx +++ b/test/complete-ui.test.tsx @@ -1,27 +1,15 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { Session } from '../src/session'; import { App, createApprovalBridge } from '../src/ui/App'; -import { testHooks, usageOf } from './helpers'; +import { streamOf, testHooks, textChunks, usageOf } from './helpers'; const usage = usageOf(3); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'reply' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('reply', usage)), }); const paths = ['README.md', 'src/app.ts', 'src/session.ts', 'src/ui/App.tsx', 'test/session.test.ts']; diff --git a/test/fallback.test.ts b/test/fallback.test.ts index 9582386..a47aee5 100644 --- a/test/fallback.test.ts +++ b/test/fallback.test.ts @@ -1,7 +1,7 @@ -import { usageOf } from './helpers'; +import { generateResult, textChunks, usageOf } from './helpers'; import { expect, test } from 'bun:test'; import { APICallError } from 'ai'; -import type { LanguageModelV4, LanguageModelV4StreamPart } from '@ai-sdk/provider'; +import type { LanguageModelV4, LanguageModelV4CallOptions, LanguageModelV4StreamPart } from '@ai-sdk/provider'; import { simulateReadableStream } from 'ai/test'; import { withFallback, type FallbackEvent } from '../src/fallback'; @@ -9,12 +9,7 @@ const usage = usageOf(1); const okStream = (body: string) => ({ stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: body }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], + chunks: textChunks(body, usage), chunkDelayInMs: null, initialDelayInMs: null, }), @@ -34,7 +29,7 @@ function model(name: string, behaviour: () => Promise): LanguageModelV4 { }; } -const opts = { prompt: [] } as any; +const opts: LanguageModelV4CallOptions = { prompt: [] }; const REAL_MESSAGE = "Function tools with reasoning_effort are not supported for gpt-5.6-sol in /v1/chat/completions. To use function tools, use /v1/responses or set reasoning_effort to 'none'."; @@ -93,11 +88,11 @@ test('doGenerate falls back on the same condition as doStream', async () => { model('chat', async () => { throw apiError(400, REAL_MESSAGE); }), - model('responses', async () => ({ content: [{ type: 'text', text: 'ok' }] })), + model('responses', async () => generateResult('ok', usage)), ]); - const out = (await wrapped.doGenerate(opts)) as any; - expect(out.content[0].text).toBe('ok'); + const out = await wrapped.doGenerate(opts); + expect(out.content[0]).toMatchObject({ type: 'text', text: 'ok' }); }); test('a 401 is not a shape mismatch, so it propagates untouched', async () => { diff --git a/test/features-ui.test.tsx b/test/features-ui.test.tsx index 8a69e7b..e371081 100644 --- a/test/features-ui.test.tsx +++ b/test/features-ui.test.tsx @@ -1,27 +1,15 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { Session } from '../src/session'; import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; -import { testHooks, usageOf } from './helpers'; +import { streamOf, testHooks, textChunks, usageOf } from './helpers'; const usage = usageOf(3); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'reply' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('reply', usage)), }); function mount(over: Partial = {}) { diff --git a/test/helpers.ts b/test/helpers.ts index 4056fd8..8b8d5f7 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -1,4 +1,10 @@ -import type { LanguageModelV4Usage } from '@ai-sdk/provider'; +import { simulateReadableStream } from 'ai/test'; +import type { + LanguageModelV4GenerateResult, + LanguageModelV4StreamPart, + LanguageModelV4StreamResult, + LanguageModelV4Usage, +} from '@ai-sdk/provider'; import type { AppHooks } from '../src/ui/App'; /** @@ -15,6 +21,46 @@ export function usageOf(input: number, output = 1): LanguageModelV4Usage { }; } +/** + * A `doStream` return value built from `simulateReadableStream` chunks. + * + * Written out as `{ stream: simulateReadableStream(...) } as any` in most UI test + * files, because the SDK's `ReadableStream` and `simulateReadableStream`'s type do + * not unify. This is the one place that difference is absorbed. + */ +export function streamOf(chunks: LanguageModelV4StreamPart[]): LanguageModelV4StreamResult { + return { + stream: simulateReadableStream({ + chunks, + chunkDelayInMs: null, + initialDelayInMs: null, + }), + } as unknown as LanguageModelV4StreamResult; +} + +/** The text and finish chunks that make a stream say one thing and stop. */ +export function textChunks(body: string, usage: LanguageModelV4Usage = usageOf(1)): LanguageModelV4StreamPart[] { + return [ + { type: 'text-start', id: '0' }, + { type: 'text-delta', id: '0', delta: body }, + { type: 'text-end', id: '0' }, + { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, + ]; +} + +/** + * A `doGenerate` return value. `warnings` is required by the SDK type but empty in + * every test, so it is filled here rather than repeated at each call site. + */ +export function generateResult(body: string, usage: LanguageModelV4Usage = usageOf(1)): LanguageModelV4GenerateResult { + return { + content: [{ type: 'text', text: body }], + finishReason: { unified: 'stop', raw: 'stop' }, + usage, + warnings: [], + }; +} + /** Default AppHooks for UI tests; override only what a test cares about. */ export function testHooks(over: Partial = {}): AppHooks { return { diff --git a/test/input.test.tsx b/test/input.test.tsx index 6c499f9..6062457 100644 --- a/test/input.test.tsx +++ b/test/input.test.tsx @@ -1,27 +1,15 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { Session } from '../src/session'; import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; -import { testHooks, usageOf } from './helpers'; +import { testHooks, textChunks, usageOf, streamOf } from './helpers'; const usage = usageOf(1000, 500); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'reply' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('reply', usage)), }); function mount(over: Partial = {}) { diff --git a/test/mcp-ui.test.tsx b/test/mcp-ui.test.tsx index 29e03f5..215fefb 100644 --- a/test/mcp-ui.test.tsx +++ b/test/mcp-ui.test.tsx @@ -1,29 +1,17 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { parseCommand, COMMANDS, HELP } from '../src/commands'; import { Session } from '../src/session'; import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; import { invalidName, parseHeaders, splitArgs, McpAdd } from '../src/ui/McpAdd'; -import { testHooks, usageOf } from './helpers'; +import { streamOf, testHooks, textChunks, usageOf } from './helpers'; const usage = usageOf(3); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'ok' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('ok', usage)), }); const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)); diff --git a/test/mcp.test.ts b/test/mcp.test.ts index 92d725a..20f1643 100644 --- a/test/mcp.test.ts +++ b/test/mcp.test.ts @@ -14,18 +14,19 @@ const call = async (tools: ToolSet, name: string, input: Record return tool.execute(input as never, { toolCallId: 'x', messages: [] } as never); }; -test('a stdio server contributes its tools under an mcp__ namespace', async () => { - const mcp = await connectMcp({ stub: stdioServer() }); +test('a stdio server contributes its tools under an mcp__ namespace (eager)', async () => { + const mcp = await connectMcp({ stub: stdioServer() }, 'eager'); try { expect(Object.keys(mcp.tools).sort()).toEqual(['mcp__stub__ping', 'mcp__stub__search']); expect(mcp.errors).toEqual([]); + expect(mcp.servers).toEqual([]); } finally { await mcp.close(); } }, 30_000); -test('an mcp tool actually executes against the server', async () => { - const mcp = await connectMcp({ stub: stdioServer() }); +test('an mcp tool actually executes against the server (eager)', async () => { + const mcp = await connectMcp({ stub: stdioServer() }, 'eager'); try { const out = await call(mcp.tools, 'mcp__stub__ping', { note: 'hello' }); expect(JSON.stringify(out)).toContain('pong: hello'); @@ -34,8 +35,8 @@ test('an mcp tool actually executes against the server', async () => { } }, 30_000); -test('two servers exposing the same tool name do not shadow each other', async () => { - const mcp = await connectMcp({ a: stdioServer(), b: stdioServer() }); +test('two servers exposing the same tool name do not shadow each other (eager)', async () => { + const mcp = await connectMcp({ a: stdioServer(), b: stdioServer() }, 'eager'); try { expect(Object.keys(mcp.tools).sort()).toEqual([ 'mcp__a__ping', @@ -48,11 +49,14 @@ test('two servers exposing the same tool name do not shadow each other', async ( } }, 30_000); -test('a server that fails to start is reported, not fatal', async () => { - const mcp = await connectMcp({ - ok: stdioServer(), - broken: { command: 'definitely-not-a-real-binary-xyz' }, - }); +test('a server that fails to start is reported, not fatal (eager)', async () => { + const mcp = await connectMcp( + { + ok: stdioServer(), + broken: { command: 'definitely-not-a-real-binary-xyz' }, + }, + 'eager', + ); try { expect(Object.keys(mcp.tools)).toEqual(['mcp__ok__ping', 'mcp__ok__search']); expect(mcp.errors.map((e) => e.server)).toEqual(['broken']); @@ -62,9 +66,10 @@ test('a server that fails to start is reported, not fatal', async () => { } }, 30_000); -test('no configured servers yields no tools and no errors', async () => { +test('no configured servers leaves the meta-tools naming none, with no errors', async () => { const mcp = await connectMcp({}); - expect(mcp.tools).toEqual({}); + expect(Object.keys(mcp.tools).sort()).toEqual(['mcp_call', 'mcp_inspect', 'mcp_list']); + expect(mcp.servers).toEqual([]); expect(mcp.errors).toEqual([]); await mcp.close(); }); @@ -76,8 +81,58 @@ test('close is safe to call twice', async () => { }); test('an http server config is attempted and its failure reported', async () => { - const mcp = await connectMcp({ remote: { url: 'http://127.0.0.1:1/mcp', type: 'http' } }); + const mcp = await connectMcp({ remote: { url: 'http://127.0.0.1:1/mcp', type: 'http' } }, 'eager'); expect(Object.keys(mcp.tools)).toEqual([]); expect(mcp.errors.map((e) => e.server)).toEqual(['remote']); await mcp.close(); }, 30_000); + +test('lazy mode (default) registers only meta-tools, never server schemas', async () => { + const mcp = await connectMcp({ stub: stdioServer() }); + try { + // The request is spared every server schema: only three meta-tools exist. + expect(Object.keys(mcp.tools).sort()).toEqual(['mcp_call', 'mcp_inspect', 'mcp_list']); + // But the prompt knows which servers are connected. + expect(mcp.servers).toEqual(['stub']); + } finally { + await mcp.close(); + } +}, 30_000); + +test('mcp_list names a server tools and mcp_inspect reads a schema without calling it', async () => { + const mcp = await connectMcp({ stub: stdioServer() }); + try { + await call(mcp.tools, 'mcp_list', { server: 'stub' }).then((out) => + expect(JSON.stringify(out)).toContain('search'), + ); + await call(mcp.tools, 'mcp_inspect', { server: 'stub', toolName: 'ping' }).then((out) => + expect(JSON.stringify(out)).toContain('note'), + ); + } finally { + await mcp.close(); + } +}, 30_000); + +test('mcp_call executes a server tool by name', async () => { + const mcp = await connectMcp({ stub: stdioServer() }); + try { + const out = await call(mcp.tools, 'mcp_call', { server: 'stub', toolName: 'ping', args: { note: 'lazy' } }); + expect(JSON.stringify(out)).toContain('pong: lazy'); + } finally { + await mcp.close(); + } +}, 30_000); + +test('mcp_call against an unknown server names the live set', async () => { + const mcp = await connectMcp({ stub: stdioServer() }); + try { + await call(mcp.tools, 'mcp_call', { server: 'nope', toolName: 'ping', args: {} }).then( + () => { + throw new Error('an unknown server must reject'); + }, + (e) => expect(String(e)).toContain('nope'), + ); + } finally { + await mcp.close(); + } +}, 30_000); diff --git a/test/memory.test.ts b/test/memory.test.ts index 34fb2ae..7c385ca 100644 --- a/test/memory.test.ts +++ b/test/memory.test.ts @@ -1,4 +1,4 @@ -import { usageOf } from './helpers'; +import { generateResult, usageOf } from './helpers'; import { afterEach, beforeEach, expect, test } from 'bun:test'; import { MockLanguageModelV4 } from 'ai/test'; import { mkdtempSync, rmSync } from 'node:fs'; @@ -31,16 +31,7 @@ const call = (tools: ToolSet, name: string, input: Record) => { const usage = usageOf(1); -const summarizer = (text: string) => - new MockLanguageModelV4({ - doGenerate: async () => - ({ - content: [{ type: 'text', text }], - finishReason: { unified: 'stop', raw: 'stop' }, - usage, - warnings: [], - }) as any, - }); +const summarizer = (text: string) => new MockLanguageModelV4({ doGenerate: async () => generateResult(text, usage) }); test('a fresh project has no memory and renders nothing', async () => { const m = new Memory('/repo'); diff --git a/test/menu.test.tsx b/test/menu.test.tsx index bb15d14..122670f 100644 --- a/test/menu.test.tsx +++ b/test/menu.test.tsx @@ -1,27 +1,15 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { Session } from '../src/session'; import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; -import { testHooks, usageOf } from './helpers'; +import { streamOf, testHooks, textChunks, usageOf } from './helpers'; const usage = usageOf(2); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'answer' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('answer', usage)), }); function mount(over: Partial = {}) { diff --git a/test/onboard.test.tsx b/test/onboard.test.tsx new file mode 100644 index 0000000..f1e5544 --- /dev/null +++ b/test/onboard.test.tsx @@ -0,0 +1,195 @@ +import { afterAll, afterEach, beforeEach, expect, test } from 'bun:test'; +import { render } from 'ink-testing-library'; +import React from 'react'; +import type { Config } from '../src/config'; +import { Onboard, type OnboardResult } from '../src/ui/Onboard'; + +/** + * The provider wizard had no test because its every path but the first ends at a + * network call. That call is `fetchModels`, which uses the global `fetch`, so the + * suite stubs it: the picking, the env-key shortcut, the manual-entry fallback, + * and the empty-list warning are all reachable offline and deterministic. + * + * `current` decides the starting row, so a test selects a preset by naming it + * rather than counting arrow presses. + */ + +const DOWN = '\u001B[B'; +const ENTER = '\r'; +const ESC = '\u001B'; + +const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +const ORIGINAL_FETCH = globalThis.fetch; + +/** Answers `GET /models` with the given ids; the wizard sorts them itself. */ +function stubModels(ids: string[]): string[] { + const calls: string[] = []; + globalThis.fetch = (async (input: unknown) => { + calls.push(String(input)); + return new Response(JSON.stringify({ data: ids.map((id) => ({ id })) }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }) as unknown as typeof fetch; + return calls; +} + +function stubFailure(status: number): void { + globalThis.fetch = (async () => new Response('boom', { status })) as unknown as typeof fetch; +} + +beforeEach(() => { + // The wizard reads the preset's env key to skip the api-key step; a developer's + // shell must not decide which branch a test takes. + delete process.env['ANTHROPIC_API_KEY']; + delete process.env['OPENAI_API_KEY']; +}); + +afterEach(() => { + globalThis.fetch = ORIGINAL_FETCH; + delete process.env['ANTHROPIC_API_KEY']; + delete process.env['OPENAI_API_KEY']; +}); + +afterAll(() => { + globalThis.fetch = ORIGINAL_FETCH; +}); + +async function press(app: ReturnType, s: string, ms = 80) { + app.stdin.write(s); + await wait(ms); +} + +async function type(app: ReturnType, s: string) { + for (const ch of s) await press(app, ch, 20); +} + +function mount(current: Partial = {}) { + const done: OnboardResult[] = []; + let cancelled = 0; + const app = render( + done.push(r)} + onCancel={() => void cancelled++} + />, + ); + return { app, done, cancelled: () => cancelled }; +} + +test('the first screen lists providers and marks the configured one', async () => { + const { app, cancelled } = mount({ presetId: 'openai' }); + await wait(80); + + const frame = app.lastFrame() ?? ''; + expect(frame).toContain('Choose a provider'); + expect(frame).toContain('Anthropic'); + expect(frame).toContain('OpenAI (current)'); + expect(frame).toContain('Custom OpenAI-compatible endpoint'); + + await press(app, ESC, 120); + expect(cancelled()).toBe(1); + app.unmount(); +}, 20_000); + +test('esc cancels from the api-key step', async () => { + // custom-openai has no env key, so it stops for a key rather than calling out. + const { app, cancelled } = mount({ presetId: 'custom-openai' }); + await wait(80); + + await press(app, ENTER, 120); + await type(app, 'https://ex.com/v1'); + await press(app, ENTER, 120); + expect(app.lastFrame()).toContain('API key'); + + await press(app, ESC, 120); + expect(cancelled()).toBe(1); + app.unmount(); +}, 20_000); + +test('a custom endpoint collects url, key, and a chosen model', async () => { + const calls = stubModels(['m2', 'm1']); + const { app, done } = mount({ presetId: 'custom-openai' }); + await wait(80); + + await press(app, ENTER, 120); + expect(app.lastFrame()).toContain('endpoint URL'); + + await type(app, 'https://ex.com/v1'); + await press(app, ENTER, 120); + expect(app.lastFrame()).toContain('API key'); + + await type(app, 'sk-secret-key'); + await press(app, ENTER, 200); + expect(app.lastFrame()).toContain('choose a model'); + + // The list arrives sorted, so the first row is m1, not the m2 it was given. + await press(app, ENTER, 150); + + expect(calls).toEqual(['https://ex.com/v1/models']); + expect(done).toEqual([ + { + presetId: 'custom-openai', + provider: 'openai', + baseURL: 'https://ex.com/v1', + apiKey: 'sk-secret-key', + model: 'm1', + }, + ]); + app.unmount(); +}, 20_000); + +test('an env key skips the key step and the hint masks it', async () => { + process.env['OPENAI_API_KEY'] = 'sk-abcdefgh1234'; + stubModels(['gpt-5', 'gpt-5-mini']); + const { app } = mount({ presetId: 'openai' }); + await wait(80); + + await press(app, ENTER, 200); + + const frame = app.lastFrame() ?? ''; + expect(frame).toContain('choose a model'); + expect(frame).toContain('sk-a...1234'); + app.unmount(); +}, 20_000); + +test('the manual entry collects a model id the list does not offer', async () => { + // The env key is what skips the key step; without it the wizard would stop there. + process.env['OPENAI_API_KEY'] = 'sk-manual-test-key'; + stubModels(['only-one']); + const { app, done } = mount({ presetId: 'openai' }); + await wait(80); + + await press(app, ENTER, 200); + expect(app.lastFrame()).toContain('choose a model'); + + await press(app, DOWN, 100); // one model, then the manual-entry row + await press(app, ENTER, 120); + expect(app.lastFrame()).toContain('model id'); + + await type(app, 'my-model'); + await press(app, ENTER, 150); + + expect(done).toHaveLength(1); + expect(done[0]?.model).toBe('my-model'); + app.unmount(); +}, 20_000); + +test('a server that cannot list models falls through to the warning', async () => { + // custom-openai has no fallback list, so an empty response leaves nothing to pick. + stubFailure(500); + const { app } = mount({ presetId: 'custom-openai' }); + await wait(80); + + await press(app, ENTER, 120); + await type(app, 'https://ex.com/v1'); + await press(app, ENTER, 120); + await type(app, 'sk-x'); + await press(app, ENTER, 250); + + const frame = app.lastFrame() ?? ''; + expect(frame).toContain('could not list models'); + expect(frame).toContain('model id'); + app.unmount(); +}, 20_000); diff --git a/test/prompt-input.test.tsx b/test/prompt-input.test.tsx new file mode 100644 index 0000000..3046897 --- /dev/null +++ b/test/prompt-input.test.tsx @@ -0,0 +1,279 @@ +import { expect, test } from 'bun:test'; +import { render } from 'ink-testing-library'; +import React, { useState } from 'react'; +import { PromptInput, type PromptInputProps } from '../src/ui/PromptInput'; + +/** + * `PromptInput` is exercised through `App` in `input.test.tsx` (history recall, + * arrow and word motion, ctrl-u, ctrl-d, paste). This file renders it directly + * for the behaviours that reaching it through the whole app made awkward: the + * emacs kill keys, the mask, a blurred input, and the `onKey` escape hatch that + * `App` uses to hand up/down/tab/esc to its own menus. + */ + +const wait = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** Keys that are awkward to read as escape sequences at the call site. */ +const KEYS = { + up: '\u001B[A', + down: '\u001B[B', + left: '\u001B[D', + ctrlA: '\u0001', + ctrlD: '\u0004', + ctrlE: '\u0005', + ctrlK: '\u000B', + ctrlU: '\u0015', + ctrlW: '\u0017', + backspace: '\u007F', +} as const; + +/** + * The input is controlled, so it needs an owner to feed `value` back. This wraps + * it with the state a caller would hold and records every value it reports. + */ +type HarnessProps = Omit, 'value' | 'onChange' | 'onSubmit'> & { + initial?: string; + onChange?: (value: string, cursor: number) => void; + onSubmit?: (value: string) => void; +}; + +function Harness({ initial = '', onChange, onSubmit, ...rest }: HarnessProps) { + const [value, setValue] = useState(initial); + return ( + {})} + onChange={(next, cursor) => { + onChange?.(next, cursor); + setValue(next); + }} + /> + ); +} + +function mount(props: HarnessProps = {}) { + const changes: { value: string; cursor: number }[] = []; + const submitted: string[] = []; + const app = render( + changes.push({ value, cursor })} + onSubmit={(v) => submitted.push(v)} + />, + ); + return { app, changes, submitted }; +} + +async function press(app: ReturnType, s: string, ms = 80) { + app.stdin.write(s); + await wait(ms); +} + +/** The visible text with every SGR sequence removed, i.e. what the user actually reads. */ +const plain = (frame: string) => frame.replace(/\u001B\[[0-9;]*m/g, ''); + +test('the rendered line carries no hand-written SGR escapes', async () => { + // The cursor must be Ink's `inverse` prop, not `\u001B[7m` pasted into the string. + // Ink measures string content as printable columns, so an embedded escape is + // counted as text and shifts the line — the stray `t` before `ype` in the + // placeholder, and a corrupted cell wherever the line wraps. + const { app } = mount({ initial: 'abc', placeholder: `type to queue${String.fromCharCode(0x2026)}` }); + await wait(40); + + const frame = app.lastFrame() ?? ''; + expect(frame).not.toContain('\u001B[7m'); + expect(frame).not.toContain('\u001B[27m'); + expect(plain(frame)).toBe('abc'); + app.unmount(); +}); + +test('the placeholder renders as one unbroken run of text', async () => { + const { app } = mount({ placeholder: 'ask anything' }); + await wait(40); + + // No escape may sit between the first character and the rest. + expect(plain(app.lastFrame() ?? '')).toBe('ask anything'); + app.unmount(); +}); + +test('a focused input keeps the caret off the string', async () => { + const { app } = mount({ initial: 'abc' }); + await wait(40); + // At the end of the line the caret is a blank cell, which Ink trims. + expect(plain(app.lastFrame() ?? '')).toBe('abc'); + app.unmount(); +}); + +test('a bare blurred input renders no caret at all', async () => { + const { app } = mount({ focus: false }); + await wait(40); + const frame = app.lastFrame() ?? ''; + expect(frame).not.toContain('\u001B[7m'); + expect(frame.trim()).toBe(''); + app.unmount(); +}); + +test('the caret marks the first placeholder character while focused', async () => { + const { app } = mount({ placeholder: 'ask anything' }); + await wait(40); + expect(plain(app.lastFrame() ?? '')).toBe('ask anything'); + app.unmount(); + + const blurred = mount({ placeholder: 'ask anything', focus: false }); + await wait(40); + expect(blurred.app.lastFrame()).toBe('ask anything'); + blurred.app.unmount(); +}); + +test('the cursor moves one character at a time with the arrows', async () => { + const { app, changes } = mount({ initial: 'abc' }); + await wait(40); + expect(plain(app.lastFrame() ?? '')).toBe('abc'); + + await press(app, KEYS.left); + await press(app, KEYS.left); + await press(app, 'Z'); + expect(app.lastFrame()).toBe('aZbc'); + expect(changes.at(-1)?.cursor).toBe(2); + app.unmount(); +}); + +test('a masked input hides the value and keeps its length', async () => { + const { app } = mount({ initial: 'abcd', mask: '*' }); + await wait(40); + + expect(app.lastFrame()).toBe('****'); + app.unmount(); +}); + +test('a mask stays masked as the value shortens', async () => { + const { app } = mount({ initial: 'abcd', mask: '*' }); + await wait(40); + // The harness owns the value, so backspace shortens it to three stars. + await press(app, KEYS.backspace); + expect(app.lastFrame()).toBe('***'); + app.unmount(); +}); + +test('ctrl-a and ctrl-e move the cursor between the ends of the line', async () => { + const { app } = mount({ initial: 'inline' }); + await wait(40); + + // Both keys only move the caret, so the proof is where the next character lands. + await press(app, KEYS.ctrlA); + await press(app, 'X'); + expect(app.lastFrame()).toBe('Xinline'); + + await press(app, KEYS.ctrlE); + await press(app, 'Y'); + expect(app.lastFrame()).toBe('XinlineY'); + app.unmount(); +}); + +test('ctrl-k kills from the cursor to the end', async () => { + const { app } = mount({ initial: 'keep this' }); + await wait(40); + await press(app, KEYS.ctrlA); + // Three characters in, then kill the tail. + for (let i = 0; i < 3; i++) await press(app, KEYS.left.replace('\u001B[D', '\u001B[C'), 30); + await press(app, KEYS.ctrlK); + expect(app.lastFrame()).toContain('kee'); + expect(app.lastFrame()).not.toContain('keep this'); + app.unmount(); +}); + +test('ctrl-w deletes the word before the cursor and its padding', async () => { + const { app } = mount({ initial: 'one two three' }); + await wait(40); + + await press(app, KEYS.ctrlW); + expect(app.lastFrame()).toContain('one two'); + expect(app.lastFrame()).not.toContain('three'); + + await press(app, KEYS.ctrlW); + expect(app.lastFrame()).toContain('one'); + expect(app.lastFrame()).not.toContain('two'); + app.unmount(); +}); + +test('ctrl-w on a single word leaves an empty line', async () => { + const { app } = mount({ initial: 'lonely' }); + await wait(40); + await press(app, KEYS.ctrlW); + expect(app.lastFrame()).not.toContain('lonely'); + app.unmount(); +}); + +test('onKey can swallow a key before the input sees it', async () => { + const seen: string[] = []; + const { app, submitted } = mount({ + initial: 'draft', + onKey: (input, key) => { + if (key.upArrow) { + seen.push('up'); + return true; + } + return false; + }, + history: ['from history'], + }); + await wait(40); + + await press(app, KEYS.up); + expect(seen).toEqual(['up']); + expect(app.lastFrame()).toContain('draft'); + expect(app.lastFrame()).not.toContain('from history'); + + // A key onKey declines still reaches the input. + await press(app, '!'); + expect(app.lastFrame()).toContain('draft!'); + expect(submitted).toEqual([]); + app.unmount(); +}); + +test('initialCursor puts the caret mid-line, so typing lands there', async () => { + const { app } = mount({ initial: 'abcdef', initialCursor: 2 }); + await wait(40); + expect(app.lastFrame()).toBe('abcdef'); + + await press(app, 'X'); + expect(app.lastFrame()).toBe('abXcdef'); + app.unmount(); +}); + +test('an external value renders as given and reports no change', async () => { + const changes: { value: string; cursor: number }[] = []; + const app = render( + changes.push({ value: v, cursor: c })} + onSubmit={() => {}} + />, + ); + await wait(40); + + // A prop with a handler that does not feed the value back: the text renders + // exactly as passed and the input reports nothing until a key is pressed. + expect(app.lastFrame()).toBe('set from outside'); + expect(changes).toEqual([]); + app.unmount(); +}); + +test('submit reports the current value and resets the cursor', async () => { + const { app, submitted } = mount({ initial: 'send me' }); + await wait(40); + await press(app, '\r', 120); + + expect(submitted).toEqual(['send me']); + app.unmount(); +}); + +test('typing inserts at the cursor rather than appending', async () => { + const { app } = mount({ initial: 'ac' }); + await wait(40); + await press(app, KEYS.left); + await press(app, 'b'); + expect(app.lastFrame()).toBe('abc'); + app.unmount(); +}); diff --git a/test/prompt.test.ts b/test/prompt.test.ts index 4de8a5a..e5e862e 100644 --- a/test/prompt.test.ts +++ b/test/prompt.test.ts @@ -33,6 +33,13 @@ test('mcp tools are grouped with their naming convention explained', () => { expect(rendered).toContain('needs approval'); }); +test('lazy mcp meta-tools prompt the workflow, and mcp__ and meta-tools do not double-list', () => { + const rendered = renderTools(['read_file', 'mcp_list', 'mcp_inspect', 'mcp_call']); + expect(rendered).toContain('mcp_list, mcp_inspect, mcp_call'); + expect(rendered).toContain('Never guess a server or tool name'); + expect(rendered).not.toContain('mcp____'); +}); + test('an unknown tool is listed rather than silently dropped', () => { expect(renderTools(['read_file', 'some_plugin_tool'])).toContain('some_plugin_tool'); }); diff --git a/test/prune.test.ts b/test/prune.test.ts index f43e1d4..e421cbc 100644 --- a/test/prune.test.ts +++ b/test/prune.test.ts @@ -1,6 +1,6 @@ import { expect, test } from 'bun:test'; import type { ModelMessage } from 'ai'; -import { detachOrphanedItems, dropOrphanedResults, pruneToFit, prunePreservingItems } from '../src/prune'; +import { digestOf, droppedBy, isPrunedSpanSummary, prunedSpanMessage, detachOrphanedItems, dropOrphanedResults, pruneToFit, prunePreservingItems } from '../src/prune'; const kinds = (messages: ModelMessage[]) => messages.map((m) => (Array.isArray(m.content) ? `${m.role}:${m.content.map((p) => p.type).join('+')}` : m.role)); @@ -443,3 +443,75 @@ test('the user prompt survives even the narrowest rung', () => { const fitted = pruneToFit({ messages, threshold: 100, estimate }); expect(JSON.stringify(fitted)).toContain('do the thing'); }); + +test('droppedBy reports the messages a prune discarded, by reference', () => { + const before: ModelMessage[] = [ + { role: 'user', content: 'do the thing' }, + { role: 'assistant', content: [{ type: 'text', text: 'ok' }] }, + { role: 'user', content: 'and again' }, + ]; + const kept = [before[0]!, before[2]!]; + + const dropped = droppedBy(before, kept); + expect(dropped).toHaveLength(1); + expect(dropped[0]).toBe(before[1]!); +}); + +test('droppedBy sees through the copies prunePreservingItems returns', () => { + const messages: ModelMessage[] = [ + { role: 'user', content: 'goal' }, + ...Array.from({ length: 40 }, (_, i): ModelMessage => ({ + role: 'assistant', + content: [ + { type: 'reasoning', text: `step ${i}` }, + { type: 'tool-call', toolCallId: `tc${i}`, toolName: 'grep', input: { pattern: `p${i}` } }, + ], + })), + ]; + const pruned = pruneToFit({ messages, threshold: 50, estimate: (m) => JSON.stringify(m).length / 4 }); + + const dropped = droppedBy(messages, pruned); + // The point is that a value comparison would find nothing: the survivors are + // spread copies. Reference identity is what makes this non-empty. + expect(dropped.length).toBeGreaterThan(0); + expect(dropped.every((m) => !pruned.includes(m))).toBe(true); +}); + +test('the digest names the tool and its input, which is the decision', () => { + const dropped: ModelMessage[] = [ + { role: 'assistant', content: [{ type: 'tool-call', toolCallId: 't', toolName: 'edit_file', input: { path: 'src/a.ts' } }] }, + { role: 'tool', content: [{ type: 'tool-result', toolCallId: 't', toolName: 'edit_file', output: { type: 'text', value: 'ok' } }] }, + ]; + + const digest = digestOf(dropped); + expect(digest).toContain('src/a.ts'); + expect(digest).toContain('result'); +}); + +test('a pruned span is injected with a marker that identifies it', () => { + const dropped: ModelMessage[] = [{ role: 'user', content: 'we chose the ladder' }]; + const message = prunedSpanMessage('notes from the span', dropped); + + expect(message).toBeDefined(); + expect(isPrunedSpanSummary(message!)).toBe(true); + expect(message!.content).toContain('notes from the span'); +}); + +test('the digest is used when no summary came back', () => { + const dropped: ModelMessage[] = [{ role: 'user', content: 'we chose the ladder' }]; + const message = prunedSpanMessage(undefined, dropped); + + expect(message!.content).toContain('we chose the ladder'); + expect(isPrunedSpanSummary(message!)).toBe(true); +}); + +test('an empty span produces no message rather than an empty one', () => { + expect(prunedSpanMessage(undefined, [])).toBeUndefined(); + expect(prunedSpanMessage(' ', [{ role: 'user', content: '' }])).toBeUndefined(); +}); + +test('a span summary is not treated as an ordinary message', () => { + const ordinary: ModelMessage = { role: 'user', content: 'a real question' }; + expect(isPrunedSpanSummary(ordinary)).toBe(false); + expect(isPrunedSpanSummary({ role: 'assistant', content: 'Earlier in this session, now compacted away:' })).toBe(false); +}); diff --git a/test/registry-ui.test.tsx b/test/registry-ui.test.tsx index 8da315c..c879544 100644 --- a/test/registry-ui.test.tsx +++ b/test/registry-ui.test.tsx @@ -1,28 +1,16 @@ import { expect, test } from 'bun:test'; import { render } from 'ink-testing-library'; import React from 'react'; -import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import { MockLanguageModelV4 } from 'ai/test'; import { Session } from '../src/session'; import { App, createApprovalBridge, type AppHooks } from '../src/ui/App'; import { InstallPrompt, RegistryPanel, type RegistryRow } from '../src/ui/Panels'; -import { testHooks, usageOf } from './helpers'; +import { streamOf, testHooks, textChunks, usageOf } from './helpers'; const usage = usageOf(3); const model = new MockLanguageModelV4({ - doStream: async () => - ({ - stream: simulateReadableStream({ - chunks: [ - { type: 'text-start', id: '0' }, - { type: 'text-delta', id: '0', delta: 'reply' }, - { type: 'text-end', id: '0' }, - { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, - ], - chunkDelayInMs: null, - initialDelayInMs: null, - }), - }) as any, + doStream: async () => streamOf(textChunks('reply', usage)), }); const rows: RegistryRow[] = [ diff --git a/test/session-features.test.ts b/test/session-features.test.ts index 40b90cd..5e1eb37 100644 --- a/test/session-features.test.ts +++ b/test/session-features.test.ts @@ -131,11 +131,15 @@ test('the skill catalogue and skill tool are offered when skills are loaded', as expect(system).toContain('Skills available'); }); -test('no skill tool is offered when there are no skills', async () => { +test('the skill tool is present even with no skills, serving an empty list', async () => { + // The tool must exist at zero skills so a skill installed mid-session is callable + // on the next turn without a session rebuild; it just rejects every name until one + // is installed. An unconditional registration is the point, so the tool is never + // absent from the request. const { seen, model } = recorder(); const session = new Session({ model, askApproval: async () => 'deny', skills: [] }); for await (const _ of session.send('hi')) void _; - expect((seen[0]?.tools ?? []).map((t) => t.name)).not.toContain('skill'); + expect((seen[0]?.tools ?? []).map((t) => t.name)).toContain('skill'); }); test('the skill tool never needs approval', async () => { diff --git a/test/session.test.ts b/test/session.test.ts index 14346ab..8569a6f 100644 --- a/test/session.test.ts +++ b/test/session.test.ts @@ -373,3 +373,177 @@ test('an interrupted command becomes a tool error and the turn carries on', asyn expect(kinds.at(-1)).toBe('done'); expect(call).toBe(2); }), 30_000); + +test('an approved edit is undone, restoring the file and dropping the turn', async () => + inTempDir(async () => { + const path = join(process.cwd(), 'app.ts'); + await Bun.write(path, 'original\n'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => + stream(call++ === 0 ? toolCall('c1', 'edit_file', { path: 'app.ts', old_string: 'original', new_string: 'changed' }) : text('done')), + }), + askApproval: async () => 'once', + }); + + for await (const _ of session.send('change it')) void _; + + expect(await Bun.file(path).text()).toBe('changed\n'); + expect(session.undoable()).toHaveLength(1); + + const result = await session.undo(); + expect(result).toBeDefined(); + expect(result!.restored).toEqual(['app.ts']); + expect(result!.conversationTrimmed).toBe(true); + expect(await Bun.file(path).text()).toBe('original\n'); + })); + +test('undo trims the conversation back to the turn it reverts', async () => + inTempDir(async () => { + await Bun.write(join(process.cwd(), 'a.ts'), 'x\n'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => + stream(call++ === 0 ? toolCall('c1', 'write_file', { path: 'a.ts', content: 'y\n' }) : text('ok')), + }), + askApproval: async () => 'once', + }); + + for await (const _ of session.send('write it')) void _; + const afterTurn = session.messages.length; + + const snap = session.snapshots.list()[0]!; + expect(snap.messageCount).toBeLessThan(afterTurn); + + await session.undo('both'); + expect(session.messages.length).toBe(snap.messageCount); + })); + +test('undoing a turn that created a file removes it', async () => + inTempDir(async () => { + const path = join(process.cwd(), 'created.ts'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(call++ === 0 ? toolCall('c1', 'write_file', { path: 'created.ts', content: 'new\n' }) : text('ok')), + }), + askApproval: async () => 'once', + }); + + for await (const _ of session.send('create it')) void _; + expect(await Bun.file(path).exists()).toBe(true); + + const result = await session.undo(); + expect(result!.removed).toEqual(['created.ts']); + expect(await Bun.file(path).exists()).toBe(false); + })); + +test('a turn that only read leaves nothing to undo', async () => + inTempDir(async () => { + await Bun.write(join(process.cwd(), 'note.txt'), 'hello'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(call++ === 0 ? toolCall('c1', 'read_file', { path: 'note.txt' }) : text('done')), + }), + askApproval: async () => 'deny', + }); + + for await (const _ of session.send('read it')) void _; + expect(session.undoable()).toHaveLength(0); + expect(await session.undo()).toBeUndefined(); + })); + +test('undo of files alone leaves the conversation in place', async () => + inTempDir(async () => { + await Bun.write(join(process.cwd(), 'a.ts'), 'orig\n'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(call++ === 0 ? toolCall('c1', 'write_file', { path: 'a.ts', content: 'new\n' }) : text('ok')), + }), + askApproval: async () => 'once', + }); + + for await (const _ of session.send('write it')) void _; + const before = session.messages.length; + + const result = await session.undo('files'); + expect(result!.conversationTrimmed).toBe(false); + expect(session.messages.length).toBe(before); + expect(await Bun.file(join(process.cwd(), 'a.ts')).text()).toBe('orig\n'); + })); + +test('a bash turn reports that its changes are not covered by undo', async () => + inTempDir(async () => { + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(call++ === 0 ? toolCall('c1', 'bash', { command: 'echo hi' }) : text('done')), + }), + askApproval: async () => 'once', + }); + + const notices: string[] = []; + for await (const ev of session.send('run it')) { + if (ev.type === 'notice') notices.push(ev.text); + } + + expect(notices.some((n) => n.includes('cannot be snapshotted'))).toBe(true); + expect(notices.some((n) => n.includes('bash'))).toBe(true); + }), 30_000); +test('redo puts the turn back without pretending to restore file content', async () => + inTempDir(async () => { + await Bun.write(join(process.cwd(), 'a.ts'), 'orig\n'); + + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => stream(call++ === 0 ? toolCall('c1', 'write_file', { path: 'a.ts', content: 'new\n' }) : text('ok')), + }), + askApproval: async () => 'once', + }); + + for await (const _ of session.send('write it')) void _; + await session.undo(); + expect(session.undoable()).toHaveLength(0); + + const redone = session.redo('conversation'); + expect(redone).toBeDefined(); + expect(redone!.filesRestored).toBe(false); + expect(session.undoable()).toHaveLength(1); + })); + +test('a skill installed mid-session is callable next turn with no rebuild', async () => + inTempDir(async () => { + const session = new Session({ + model: new MockLanguageModelV4({ doStream: async () => stream(text('done')) }) as never, + askApproval: async () => 'deny', + }); + + // No skills at boot: the skill tool is still registered (so the *first* install + // is callable), just serving an empty list. + const skillTool = session.tools['skill'] as { + execute: (input: { name: string }, ctx: unknown) => Promise; + }; + expect(skillTool).toBeDefined(); + await skillTool.execute({ name: 'hot' }, {}).then( + () => { + throw new Error('an unknown skill from an empty list must throw'); + }, + () => undefined, + ); + + // Hot-reload swaps the live list. + session.setSkills([{ name: 'hot', description: 'installed', origin: 'registry', body: 'fresh instructions' }]); + const out = await skillTool.execute({ name: 'hot' }, {}); + expect(out).toContain('fresh instructions'); + expect(out).toContain('registry'); + })); diff --git a/test/skills.test.ts b/test/skills.test.ts index e519bdf..8d0e283 100644 --- a/test/skills.test.ts +++ b/test/skills.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, expect, test } from 'bun:test'; import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { createSkillTool, loadSkills, parseSkill, renderSkills } from '../src/skills'; +import { createSkillTool, loadSkills, parseSkill, renderSkills, type Skill } from '../src/skills'; import { BUILTIN_SKILLS } from '../src/skills-builtin'; const SKILLS_MD_DIR = join(import.meta.dir, '..', 'src', 'skills-md'); @@ -165,20 +165,33 @@ test('an empty skill list renders nothing', () => { test('the skill tool returns the body on demand', async () => { const skills = await loadSkills(work); - const out = await load(createSkillTool(skills), 'debug'); + const out = await load(createSkillTool(() => skills), 'debug'); expect(out).toContain('Three hypotheses'); expect(out).toContain('builtin'); }); test('the skill tool rejects an unknown name and lists what exists', async () => { const skills = await loadSkills(work); - const tool = createSkillTool(skills); + const tool = createSkillTool(() => skills); expect(load(tool, 'nonexistent')).rejects.toThrow(/No skill named "nonexistent"/); expect(load(tool, 'nonexistent')).rejects.toThrow(/debug/); }); test('the skill tool tolerates surrounding whitespace and case', async () => { const skills = await loadSkills(work); - const out = await load(createSkillTool(skills), ' REVIEW '); + const out = await load(createSkillTool(() => skills), ' REVIEW '); expect(out).toContain('Severity order'); }); + +test('the skill tool reads the list live, so a mid-session install needs no restart', async () => { + // Mutating the array between calls, the way a hot-reload swaps the session's list, + // must be visible to the next call on the *same* tool instance. + const skills: Skill[] = []; + const tool = createSkillTool(() => skills); + + skills.push({ name: 'hot', description: 'installed mid-session', origin: 'registry', body: 'fresh body' }); + + const out = await load(tool, 'hot'); + expect(out).toContain('fresh body'); + expect(out).toContain('registry'); +}); diff --git a/test/snapshot.test.ts b/test/snapshot.test.ts new file mode 100644 index 0000000..2f90d40 --- /dev/null +++ b/test/snapshot.test.ts @@ -0,0 +1,202 @@ +import { afterEach, beforeEach, expect, test } from 'bun:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { MAX_SNAPSHOTS, relPath, restore, Snapshots, touchedPaths } from '../src/snapshot'; + +let dir: string; + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'shiro-snap-')); +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +test('relPath refuses anything outside the workspace', () => { + expect(relPath(dir, join(dir, 'a.ts'))).toBe('a.ts'); + expect(relPath(dir, join(dir, 'sub', 'b.ts'))).toBe('sub/b.ts'); + expect(relPath(dir, join(dir, '..', 'escape.ts'))).toBeUndefined(); + expect(relPath(dir, dir)).toBeUndefined(); +}); + +test('relPath uses forward slashes so a restore is portable', () => { + expect(relPath(dir, join(dir, 'deep', 'nested', 'x.ts'))).toBe('deep/nested/x.ts'); +}); + +test('touchedPaths names the file tools and their paths', () => { + expect(touchedPaths('write_file', { path: 'a.ts' })).toEqual({ paths: ['a.ts'], covered: true }); + expect(touchedPaths('move_file', { from: 'a.ts', to: 'b.ts' })).toEqual({ paths: ['a.ts', 'b.ts'], covered: true }); + expect(touchedPaths('insert_lines', { path: 'a.ts' })).toEqual({ paths: ['a.ts'], covered: true }); +}); + +test('touchedPaths reads every path out of a patch', () => { + const patch = [ + '*** Begin Patch', + '*** Update File: src/a.ts', + '@@', + '-x', + '+y', + '*** Add File: src/b.ts', + '+new', + '*** Move to: src/c.ts', + '*** End Patch', + ].join('\n'); + + const { paths, covered } = touchedPaths('apply_patch', { patch }); + expect(covered).toBe(true); + expect(paths).toContain('src/a.ts'); + expect(paths).toContain('src/b.ts'); + expect(paths).toContain('src/c.ts'); +}); + +test('bash is reported as uncovered because it can write anything', () => { + expect(touchedPaths('bash', { command: 'rm -rf src' })).toEqual({ paths: [], covered: false }); +}); + +test('a read tool touches nothing and is covered', () => { + expect(touchedPaths('read_file', { path: 'a.ts' })).toEqual({ paths: [], covered: true }); +}); + +test('a turn records the pre-image of a file it changes', async () => { + await Bun.write(join(dir, 'a.ts'), 'original'); + const snaps = new Snapshots(dir); + + snaps.begin('change a', 3); + await snaps.capture(join(dir, 'a.ts')); + await Bun.write(join(dir, 'a.ts'), 'changed'); + + const snap = snaps.commit(); + expect(snap).toBeDefined(); + expect(snap!.files).toEqual([{ path: 'a.ts', before: 'original' }]); + expect(snap!.messageCount).toBe(3); +}); + +test('the first write of a turn wins, so undo restores the turn start not the midpoint', async () => { + await Bun.write(join(dir, 'a.ts'), 'v0'); + const snaps = new Snapshots(dir); + + snaps.begin('two writes', 0); + await snaps.capture(join(dir, 'a.ts')); + await Bun.write(join(dir, 'a.ts'), 'v1'); + await snaps.capture(join(dir, 'a.ts')); + await Bun.write(join(dir, 'a.ts'), 'v2'); + + const snap = snaps.commit()!; + expect(snap.files[0]!.before).toBe('v0'); + expect(await Bun.file(join(dir, 'a.ts')).text()).toBe('v2'); +}); + +test('a file that did not exist is recorded as created', async () => { + const snaps = new Snapshots(dir); + snaps.begin('create', 0); + await snaps.capture(join(dir, 'new.ts')); + snaps.commit(); + + const snap = snaps.pop()!; + expect(snap.files).toEqual([{ path: 'new.ts', before: undefined }]); +}); + +test('a turn that changed nothing is not kept', () => { + const snaps = new Snapshots(dir); + snaps.begin('a question', 0); + expect(snaps.commit()).toBeUndefined(); + expect(snaps.size).toBe(0); +}); + +test('a path outside the workspace is not captured', async () => { + const snaps = new Snapshots(dir); + snaps.begin('escape', 0); + await snaps.capture(join(dir, '..', 'outside.ts')); + expect(snaps.commit()).toBeUndefined(); +}); + +test('captureFor pulls the paths out of the tool call', async () => { + await Bun.write(join(dir, 'a.ts'), 'before'); + const snaps = new Snapshots(dir); + snaps.begin('edit', 0); + + const { covered, paths } = await snaps.captureFor('edit_file', { path: 'a.ts' }); + expect(covered).toBe(true); + expect(paths).toEqual(['a.ts']); + expect(snaps.commit()!.files[0]!.before).toBe('before'); +}); + +test('only the most recent snapshots are kept', async () => { + const snaps = new Snapshots(dir); + for (let i = 0; i < MAX_SNAPSHOTS + 10; i++) { + snaps.begin(`turn ${i}`, 0); + await snaps.captureFor('write_file', { path: `f${i}.ts` }); + snaps.commit(); + } + expect(snaps.size).toBe(MAX_SNAPSHOTS); + // The oldest are the ones that fell off. + expect(snaps.list().at(-1)!.turn).toBe(11); +}); + +test('restore writes content back and removes a file the turn created', async () => { + await Bun.write(join(dir, 'edited.ts'), 'changed'); + await Bun.write(join(dir, 'created.ts'), 'was not here before'); + + const snap = { + turn: 1, + at: new Date().toISOString(), + prompt: 'p', + messageCount: 0, + files: [ + { path: 'edited.ts', before: 'original' }, + { path: 'created.ts', before: undefined }, + ], + }; + + const { restored, removed } = await restore(snap, dir); + expect(restored).toEqual(['edited.ts']); + expect(removed).toEqual(['created.ts']); + expect(await Bun.file(join(dir, 'edited.ts')).text()).toBe('original'); + expect(await Bun.file(join(dir, 'created.ts')).exists()).toBe(false); +}); + +test('restore recreates a file that the turn deleted', async () => { + const snap = { + turn: 1, + at: new Date().toISOString(), + prompt: 'p', + messageCount: 0, + files: [{ path: 'gone.ts', before: 'the content' }], + }; + + await restore(snap, dir); + expect(await Bun.file(join(dir, 'gone.ts')).text()).toBe('the content'); +}); + +test('pop and push move a turn out and back for redo', async () => { + await Bun.write(join(dir, 'a.ts'), 'orig'); + const snaps = new Snapshots(dir); + snaps.begin('edit', 0); + await snaps.capture(join(dir, 'a.ts')); + const committed = snaps.commit()!; + + const popped = snaps.pop()!; + expect(popped.turn).toBe(committed.turn); + expect(snaps.size).toBe(0); + + snaps.push(popped); + expect(snaps.size).toBe(1); +}); + +test('a discard drops the open turn without recording it', () => { + const snaps = new Snapshots(dir); + snaps.begin('aborted', 0); + snaps.discard(); + expect(snaps.open).toBe(false); + expect(snaps.size).toBe(0); +}); + +test('an unreadable path does not break the snapshot', async () => { + const snaps = new Snapshots(dir); + snaps.begin('odd', 0); + // A directory, not a file: reading it as text fails, and the capture must swallow that. + await snaps.capture(dir); + expect(snaps.open).toBe(true); +}); diff --git a/test/step-back.test.ts b/test/step-back.test.ts new file mode 100644 index 0000000..1b7afc7 --- /dev/null +++ b/test/step-back.test.ts @@ -0,0 +1,100 @@ +import { usageOf } from './helpers'; +import { expect, test } from 'bun:test'; +import { MockLanguageModelV4, simulateReadableStream } from 'ai/test'; +import type { LanguageModelV4StreamPart } from '@ai-sdk/provider'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { Session } from '../src/session'; +import { createStepBackTool, type LoopEntry } from '../src/step-back'; + +const usage = usageOf(10, 5); + +function stream(parts: LanguageModelV4StreamPart[]) { + return { stream: simulateReadableStream({ chunks: parts, chunkDelayInMs: null, initialDelayInMs: null }) }; +} + +function toolCall(id: string, toolName: string, input: unknown): LanguageModelV4StreamPart[] { + return [ + { type: 'tool-input-start', id, toolName }, + { type: 'tool-input-end', id }, + { type: 'tool-call', toolCallId: id, toolName, input: JSON.stringify(input) }, + { type: 'finish', finishReason: { unified: 'tool-calls', raw: 'tool_use' }, usage }, + ]; +} + +function text(body: string): LanguageModelV4StreamPart[] { + return [ + { type: 'text-start', id: '0' }, + { type: 'text-delta', id: '0', delta: body }, + { type: 'text-end', id: '0' }, + { type: 'finish', finishReason: { unified: 'stop', raw: 'stop' }, usage }, + ]; +} + +function inTempDir(fn: () => Promise): Promise { + const orig = process.cwd(); + const dir = mkdtempSync(join(tmpdir(), 'shiro-loop-')); + process.chdir(dir); + return fn().finally(() => { + process.chdir(orig); + rmSync(dir, { recursive: true, force: true }); + }); +} + +/** Calls step_back once, then replies. */ +function stepBackTurn(): LanguageModelV4StreamPart[] { + return toolCall('sb', 'step_back', { note: 'grep keeps finding nothing' }); +} + +test('step_back returns a reflection prompt once there is a trace', async () => { + const entries: LoopEntry[] = [ + { step: 1, toolName: 'grep', input: '{"pattern":"x"}', result: 'no matches', at: 't1' }, + { step: 2, toolName: 'grep', input: '{"pattern":"y"}', result: 'no matches', at: 't2' }, + ]; + const sb = createStepBackTool({ trace: () => entries }); + + const out = await sb.execute!({ note: 'nothing matches' }, { toolCallId: 'c', messages: [] } as never); + expect(out).toContain('You have run threadbare'); + expect(out).toContain('grep'); + expect(out).toContain('nothing matches'); + expect(out).toContain('ONE different thing'); +}); + +test('step_back on an empty trace says it is too early to help', async () => { + const sb = createStepBackTool({ trace: () => [] }); + const out = await sb.execute!({}, { toolCallId: 'c', messages: [] } as never); + expect(out).toContain('No recent steps to reflect on'); +}); + +test('step_back is offered to the model and visible in /tools', () => + inTempDir(async () => { + const session = new Session({ + model: new MockLanguageModelV4({ doStream: async () => stream(text('ok')) }) as never, + askApproval: async () => 'deny', + }); + expect(session.activeTools()).toContain('step_back'); + })); + +test('a repeated allowed call is escalated and points at step_back', () => + inTempDir(async () => { + await Bun.write(join(process.cwd(), 'a.txt'), 'data'); + // Four identical read_file calls trip the repeat guard (limit 3) on the fourth. + let call = 0; + const session = new Session({ + model: new MockLanguageModelV4({ + doStream: async () => + stream(call++ < 4 ? toolCall(`c${call}`, 'read_file', { path: 'a.txt' }) : text('done')), + }) as never, + askApproval: async () => 'once', + }); + + const notices: string[] = []; + for await (const ev of session.send('keep reading a.txt')) { + if (ev.type === 'notice') notices.push(ev.text); + } + const guard = notices.find((n) => n.includes('not making progress')); + + // The guard names step_back as the recovery path. + expect(guard ?? 'no guard notice').toContain('step_back'); + }), 30_000); \ No newline at end of file diff --git a/test/subagent.test.ts b/test/subagent.test.ts index 9b83c3c..a7bf853 100644 --- a/test/subagent.test.ts +++ b/test/subagent.test.ts @@ -324,3 +324,58 @@ test('a finished subagent reports its token use to the parent', async () => expect(usageEvents).toHaveLength(1); expect(usageEvents[0]).toMatchObject({ kind: 'explore' }); })); + +test('two investigations in one task call overlap in wall-clock time', async () => + inTempDir(async () => { + await Bun.write('a.ts', 'AAA\n'); + await Bun.write('b.ts', 'BBB\n'); + + // Track how many subagent sleeps are in flight at once. Two that overlap in time + // reach a concurrency of 2; a queueing implementation never does. + let inFlight = 0; + let peakConcurrency = 0; + + const seen: LanguageModelV4CallOptions[] = []; + const model = new MockLanguageModelV4({ + doStream: async (opts) => { + const call = seen.length; + seen.push(opts); + if (call === 0) { + // The parent batches two independent searches into one task call. + return stream( + toolCall('c1', 'task', { + description: 'two searches', + prompt: 'find both files', + tasks: [ + { description: 'find a', prompt: 'Find where a.ts is mentioned.' }, + { description: 'find b', prompt: 'Find where b.ts is mentioned.' }, + ], + }), + ); + } + // Each subagent sleeps before replying. Overlapping the two is what the + // test is for, so the mock measures it rather than relying on a wall clock. + inFlight++; + peakConcurrency = Math.max(peakConcurrency, inFlight); + await Bun.sleep(200); + inFlight--; + if (call <= 2) return stream(text(`found ${call === 1 ? 'a' : 'b'} at src`)); + return stream(text('done')); + }, + }); + + const session = new Session({ + model, + askApproval: async () => 'deny', + extraTools: { task: createTaskTool({ model }) }, + autoApprove: ['task'], + }); + + for await (const _ of session.send('find both')) void _; + + // Two investigates that truly overlapped both slept at the same moment. + expect(peakConcurrency).toBeGreaterThanOrEqual(2); + // The parent made one task call, the two subagents each one model call, and the + // parent one more reply. + expect(seen.length).toBeGreaterThanOrEqual(4); + })); diff --git a/test/tools.test.ts b/test/tools.test.ts index 8fa63a9..419ac7f 100644 --- a/test/tools.test.ts +++ b/test/tools.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, expect, test } from 'bun:test'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import type { z } from 'zod'; import { applyPatchTool, bashTool, @@ -21,6 +22,8 @@ import { readManyFilesTool, tools, toolSetOf, + TOOL_SET_NAMES, + TOOL_SETS, writeFileTool, } from '../src/tools'; @@ -40,8 +43,8 @@ afterEach(() => { rmSync(dir, { recursive: true, force: true }); }); -const run = (t: { execute?: (input: T, opts: any) => unknown }, input: T) => - Promise.resolve(t.execute!(input, { toolCallId: 't1', messages: [] })) as Promise; +const run = (t: { execute?: (input: T, opts: any) => unknown }, input: T, opts: any = { toolCallId: 't1', messages: [] }) => + Promise.resolve(t.execute!(input, opts)) as Promise; test('jail rejects traversal and absolute escapes', () => { expect(() => jail('../secret')).toThrow(/escapes workspace/); @@ -129,6 +132,24 @@ test('edit_file refuses ambiguous oldString unless replaceAll', async () => { expect(await Bun.file(join(dir, 'y.ts')).text()).toBe('z\nz\n'); }); +test('edit_file accepts snake_case params from models that emit them', async () => { + const parsed = (editFileTool.inputSchema as z.ZodType).parse({ + path: 'x.ts', + old_string: 'const b = 2;', + new_string: 'const b = 3;', + replace_all: false, + }); + expect(parsed).toMatchObject({ oldString: 'const b = 2;', newString: 'const b = 3;', replaceAll: false }); +}); + +test('multi_edit accepts snake_case params on its edits array', async () => { + const parsed = (multiEditTool.inputSchema as z.ZodType).parse({ + path: 'a.ts', + edits: [{ old_string: 'const a = 1;', new_string: 'const a = 10;' }], + }); + expect(parsed).toMatchObject({ edits: [{ oldString: 'const a = 1;', newString: 'const a = 10;' }] }); +}); + test('edit_file reports a missing oldString', async () => { await Bun.write(join(dir, 'z.ts'), 'hello'); expect(run(editFileTool, { path: 'z.ts', oldString: 'bye', newString: 'hi' })).rejects.toThrow(/not found/); @@ -235,6 +256,53 @@ test('both new write tools are gated and belong to a set', () => { } }); +/** + * The two halves of the derivation, checked from both ends. + * + * `MUTATING_TOOLS` is built by filtering the registry, so the risk is no longer a + * name missing from a hand-typed list — it is a tool that *should* be marked and is + * not, which derives a list that silently omits a write. These assert the sets agree + * with the source in both directions, so adding a write tool without marking it + * fails here rather than in a user's workspace. + */ +const WRITE_TOOL_HINTS = ['write', 'edit', 'delete', 'move', 'insert', 'replace', 'append', 'prepend', 'patch', 'bash']; + +test('every tool whose name implies a write is marked mutating', () => { + const unmarked = Object.keys(tools).filter( + (name) => WRITE_TOOL_HINTS.some((hint) => name.includes(hint)) && !(MUTATING_TOOLS as readonly string[]).includes(name), + ); + expect(unmarked).toEqual([]); +}); + +test('every mutating tool is registered, so nothing is marked in the abstract', () => { + const names = new Set(Object.keys(tools)); + for (const name of MUTATING_TOOLS) expect(names.has(name), name).toBe(true); +}); + +test('every registered tool belongs to a set or is explicitly session-level', () => { + // MCP, plugin, and session tools are namespaced or added at runtime and are not + // part of the schema budget; a bare built-in with no set would never be listed by + // /tools and could not be switched off. + const sessionLevel = new Set(['todo_write', 'remember', 'recall', 'forget', 'skill', 'ask', 'current_time']); + const inNoSet = Object.keys(tools).filter((name) => toolSetOf(name) === undefined && !sessionLevel.has(name)); + expect(inNoSet).toEqual([]); +}); + +test('every set name is reachable and every static set member is a real tool', () => { + const names = new Set(Object.keys(tools)); + // Created per-session because it needs a model to write the message; it is a real + // member of the `git` set but lives outside the static registry in cli.tsx. + const dynamic = new Set(['git_commit_message']); + for (const set of TOOL_SET_NAMES) { + expect(TOOL_SETS[set].length).toBeGreaterThan(0); + for (const member of TOOL_SETS[set]) { + if (dynamic.has(member)) continue; + expect(names.has(member), `${set} lists ${member}`).toBe(true); + expect(toolSetOf(member), member).toBe(set); + } + } +}); + test('multi_edit applies every edit in order, each seeing the last', async () => { await Bun.write(join(dir, 'm.ts'), 'const a = 1;\nconst b = 2;\n'); const out = await run(multiEditTool, { @@ -581,3 +649,34 @@ test('a command that finished is no longer interruptible', async () => { await run(bashTool, { command: 'echo done' }); expect(interruptBash()).toEqual([]); }, 20_000); + +test('timeout kills the command instead of hanging past its deadline', async () => { + const started = Date.now(); + const call = run(bashTool, { command: sleeper, timeout: 1_000 }); + const message = await call.then(() => '', (e: Error) => e.message); + expect(message).toMatch(/exceeded its 1000ms timeout/i); + expect(message).toContain('effects are unknown'); + // The old bug: Bun's spawn timeout killed only the shell, the grandchild kept + // the pipes open, and this hung for the full 20s instead. + expect(Date.now() - started).toBeLessThan(10_000); +}, 30_000); + +test('an aborted turn tree-kills the command, not just the shell', async () => { + const started = Date.now(); + const controller = new AbortController(); + const call = run( + bashTool, + { command: sleeper, timeout: 30_000 }, + { toolCallId: 'a1', messages: [], abortSignal: controller.signal }, + ); + + await Bun.sleep(400); + controller.abort(); + + const message = await call.then(() => '', (e: Error) => e.message); + expect(message).toMatch(/user interrupted/i); + expect(Date.now() - started).toBeLessThan(10_000); + // The entry must be gone: a stale `running` row is what made every ctrl-c + // re-announce an already-dead command. + expect(interruptBash()).toEqual([]); +}, 30_000); diff --git a/test/ui-bodies.test.tsx b/test/ui-bodies.test.tsx new file mode 100644 index 0000000..8442adb --- /dev/null +++ b/test/ui-bodies.test.tsx @@ -0,0 +1,277 @@ +import { expect, test } from 'bun:test'; +import { MockLanguageModelV4 } from 'ai/test'; +import { Session } from '../src/session'; +import type { SubagentEvent } from '../src/subagent'; +import { applySubagentEvent, createNoticeBus, createSubagentBus } from '../src/ui/buses'; +import { contextPanel, costPanel, todosPanel, toolsPanel } from '../src/ui/panel-bodies'; +import type { SubagentView } from '../src/ui/Panels'; + +/** + * `panel-bodies.ts` and `buses.ts` are the two UI modules with no test of their own. + * The panel bodies are pure functions of session and hook state, so they are + * exercised here without mounting Ink; the buses are driven directly. + * + * The panel functions only read counters, so the mock model's stream is never run. + */ +const model = new MockLanguageModelV4({ doStream: async () => ({ stream: new ReadableStream() }) }); + +type SessionOverrides = Partial[0]>; + +const makeSession = (over: SessionOverrides = {}) => + new Session({ model, askApproval: async () => 'deny', ...over }); + +const COST_INFO = { sessionId: 'abc12345', model: 'gpt-5', agent: 'default', thinking: 'medium' }; + +test('the tools panel lists what is offered and names each tool set', () => { + const session = makeSession(); + const panel = toolsPanel(session); + const offered = session.activeTools(); + + expect(panel.title).toBe('tools'); + expect(panel.hint).toBe(`${offered.length} offered this turn of ${Object.keys(session.tools).length} registered`); + expect(panel.body).toContain('- `read_file`'); + expect(panel.body).toContain('- `bash` core'); + expect(panel.body).toContain('- `git_diff` git'); +}); + +test('a read-only agent narrows the panel to the tools it may call', () => { + const session = makeSession(); + session.setAgent({ name: 'plan', summary: '', thinking: 'high', appendix: '', allowTools: ['read_file', 'grep'] }); + + const panel = toolsPanel(session); + expect(panel.body).toContain('`read_file`'); + expect(panel.body).toContain('`grep`'); + expect(panel.body).not.toContain('`write_file`'); + expect(panel.body).not.toContain('`bash`'); +}); + +test('a panel hint counts offered against registered', () => { + const session = makeSession(); + session.setAgent({ name: 'plan', summary: '', thinking: 'high', appendix: '', allowTools: ['read_file'] }); + expect(toolsPanel(session).hint).toBe(`1 offered this turn of ${Object.keys(session.tools).length} registered`); +}); + +test('the cost panel reports a priced turn, context, and the agent', () => { + const session = makeSession({ modelId: 'gpt-5' }); + session.inputTokens = 1000; + session.outputTokens = 500; + + const panel = costPanel(session, COST_INFO); + expect(panel.title).toBe('cost'); + expect(panel.hint).toBe('session abc12345'); + expect(panel.body).toContain('- model: `gpt-5`'); + expect(panel.body).toContain('- billed: 1000 in / 500 out'); + expect(panel.body).toMatch(/- spend: \$\d/); + expect(panel.body).not.toContain('unpriced model'); + expect(panel.body).toContain('- context: ~'); + expect(panel.body).toContain('- agent: `default` thinking `medium`'); +}); + +test('an unknown model is reported as unpriced rather than guessed', () => { + const session = makeSession({ modelId: 'llama-3.3-70b' }); + session.inputTokens = 4210; + session.outputTokens = 88; + + const panel = costPanel(session, { ...COST_INFO, model: 'llama-3.3-70b' }); + expect(panel.body).toContain('- spend: unpriced model'); +}); + +test('subagent spend is its own line and priced against the subagent model', () => { + const session = makeSession({ modelId: 'gpt-5', subagentModelId: 'gpt-5-nano' }); + session.inputTokens = 1000; + session.outputTokens = 100; + session.recordSubagentUsage({ inputTokens: 800, outputTokens: 200 }); + + const panel = costPanel(session, { ...COST_INFO, subagentModel: 'gpt-5-nano' }); + expect(panel.body).toContain('- subagents: 800 in / 200 out (`gpt-5-nano`)'); +}); + +test('a ceiling with both numbers priced reports what is spent against it', () => { + const session = makeSession({ modelId: 'gpt-5', maxSpendUsd: 10 }); + session.inputTokens = 1_000_000; + + const spend = session.spend(); + expect(spend.ceiling).toBe(10); + expect(spend.usd).toBeCloseTo(1.25, 5); + expect(spend.overWarn).toBe(false); + expect(spend.overLimit).toBe(false); + + const panel = costPanel(session, COST_INFO); + expect(panel.body).toContain('- ceiling: $1.25 of $10.00'); +}); + +test('a ceiling is not enforced against an unpriced model', () => { + const session = makeSession({ modelId: 'llama-3.3-70b', maxSpendUsd: 1 }); + session.inputTokens = 9_999_999; + + const spend = session.spend(); + expect(spend.usd).toBeUndefined(); + expect(spend.ceiling).toBe(1); + expect(spend.overWarn).toBe(false); + expect(spend.overLimit).toBe(false); +}); + +test('crossing the ceiling flags warn at 80% and limit at 100%', () => { + // $1.25/M in and $10/M out for gpt-5: 8M in is $10 exactly, so warn and limit land together. + const session = makeSession({ modelId: 'gpt-5', maxSpendUsd: 10 }); + session.inputTokens = 8_000_000; + + const spend = session.spend(); + expect(spend.usd).toBeCloseTo(10, 5); + expect(spend.overWarn).toBe(true); + expect(spend.overLimit).toBe(true); +}); + +test('the context panel lists instruction files, and points at /init when there are none', () => { + const loaded = contextPanel(['AGENTS.md', '.shiro/skills/extra.md']); + expect(loaded.title).toBe('project instructions'); + expect(loaded.body).toContain('- `AGENTS.md`'); + expect(loaded.body).toContain('- `.shiro/skills/extra.md`'); + + const empty = contextPanel([]); + expect(empty.body).toContain('No `AGENTS.md`'); + expect(empty.body).toContain('/init'); +}); + +test('the todos panel renders the notebook state', async () => { + const session = makeSession(); + expect(todosPanel(session).body).toBe('No task list yet.'); + + const write = session.notebook.tools()['todo_write']!; + await write.execute!( + { + todos: [ + { content: 'first task', status: 'done' }, + { content: 'second task', status: 'in_progress', note: 'halfway' }, + ], + }, + { toolCallId: 't', messages: [], context: {} }, + ); + + const panel = todosPanel(session); + expect(panel.title).toBe('task list'); + expect(panel.body).toContain('first task'); + expect(panel.body).toContain('second task'); + expect(panel.body).toContain('halfway'); +}); + +test('a notice emitted before a sink is bound is delivered on bind, in order', () => { + const bus = createNoticeBus(); + const seen: string[] = []; + + bus.emit('first'); + bus.emit('second'); + expect(seen).toEqual([]); + + bus.bind((text) => seen.push(text)); + expect(seen).toEqual(['first', 'second']); + + bus.emit('third'); + expect(seen).toEqual(['first', 'second', 'third']); +}); + +test('a notice emitted after a bind goes straight through', () => { + const bus = createNoticeBus(); + const seen: string[] = []; + bus.bind((t) => seen.push(t)); + bus.emit('only'); + expect(seen).toEqual(['only']); +}); + +test('a rebind takes over and the queue is not replayed twice', () => { + const bus = createNoticeBus(); + const first: string[] = []; + const second: string[] = []; + + bus.emit('queued'); + bus.bind((t) => first.push(t)); + bus.bind((t) => second.push(t)); + + expect(first).toEqual(['queued']); + expect(second).toEqual([]); + + bus.emit('later'); + expect(first).toEqual(['queued']); + expect(second).toEqual(['later']); +}); + +test('a subagent event before a bind is delivered on bind', () => { + const bus = createSubagentBus(); + const seen: SubagentEvent[] = []; + const event: SubagentEvent = { type: 'start', id: 'a', kind: 'explore', description: 'find auth' }; + + bus.emit(event); + expect(seen).toEqual([]); + + bus.bind((e) => seen.push(e)); + expect(seen).toEqual([event]); +}); + +const started = (id: string, kind: 'explore' | 'review' | 'worker' = 'explore'): SubagentEvent => ({ + type: 'start', + id, + kind, + description: `${kind} task`, +}); + +test('a result attaches to the step it answers instead of appending a step', () => { + let view: SubagentView[] = []; + view = applySubagentEvent(view, started('a')); + view = applySubagentEvent(view, { type: 'step', id: 'a', tool: 'grep', summary: 'login' }); + view = applySubagentEvent(view, { type: 'result', id: 'a', tool: 'grep', summary: '2 hits', ok: true }); + + expect(view).toHaveLength(1); + expect(view[0]!.steps).toHaveLength(1); + expect(view[0]!.steps[0]).toEqual({ tool: 'grep', summary: 'login', outcome: '2 hits', ok: true }); +}); + +test('a result for a tool that is not the pending step is ignored', () => { + let view: SubagentView[] = []; + view = applySubagentEvent(view, started('a')); + view = applySubagentEvent(view, { type: 'step', id: 'a', tool: 'grep', summary: 'login' }); + view = applySubagentEvent(view, { type: 'result', id: 'a', tool: 'read_file', summary: 'nope', ok: true }); + + expect(view[0]!.steps).toEqual([{ tool: 'grep', summary: 'login' }]); +}); + +test('a second result for the same step does not overwrite the first', () => { + let view: SubagentView[] = []; + view = applySubagentEvent(view, started('a')); + view = applySubagentEvent(view, { type: 'step', id: 'a', tool: 'grep', summary: 'login' }); + view = applySubagentEvent(view, { type: 'result', id: 'a', tool: 'grep', summary: 'first', ok: true }); + view = applySubagentEvent(view, { type: 'result', id: 'a', tool: 'grep', summary: 'second', ok: false }); + + expect(view[0]!.steps[0]!.outcome).toBe('first'); +}); + +test('an end event flips the status, and an error event carries its message', () => { + const base = applySubagentEvent([], started('a')); + + expect(applySubagentEvent(base, { type: 'end', id: 'a', ok: true, steps: 2 })[0]!.status).toBe('done'); + expect(applySubagentEvent(base, { type: 'end', id: 'a', ok: false, steps: 2 })[0]!.status).toBe('failed'); + + const errored = applySubagentEvent(base, { type: 'error', id: 'a', message: 'model refused' }); + expect(errored[0]!.status).toBe('failed'); + expect(errored[0]!.error).toBe('model refused'); +}); + +test('an event naming no known agent leaves the view untouched', () => { + const base = applySubagentEvent([], started('a')); + const view = applySubagentEvent(base, { type: 'end', id: 'ghost', ok: true, steps: 0 }); + + expect(view).toHaveLength(1); + expect(view[0]!.id).toBe('a'); + expect(view[0]!.status).toBe('running'); +}); + +test('two agents interleave without crossing their steps', () => { + let view: SubagentView[] = []; + view = applySubagentEvent(view, started('a')); + view = applySubagentEvent(view, started('b', 'review')); + view = applySubagentEvent(view, { type: 'step', id: 'b', tool: 'read_file', summary: 'b.ts' }); + view = applySubagentEvent(view, { type: 'step', id: 'a', tool: 'grep', summary: 'a.ts' }); + + expect(view).toHaveLength(2); + expect(view[0]!.steps.map((s) => s.tool)).toEqual(['grep']); + expect(view[1]!.steps.map((s) => s.tool)).toEqual(['read_file']); +});