diff --git a/.github/workflows/audit.yml b/.github/workflows/audit.yml new file mode 100644 index 000000000..62ed7d605 --- /dev/null +++ b/.github/workflows/audit.yml @@ -0,0 +1,47 @@ +# Known security advisories in the packages we ship (ATO-1415). +# +# Reads only the lockfile against the npm advisory database, so it needs no install. Dev-only +# packages (vitest, esbuild, ...) are left out: they never reach users. A high or critical +# advisory fails the check; fix it with a patch-level bump or an `overrides` entry in +# package.json, the way PR #1079 did. +# +# Kept out of the Test workflow on purpose. The advisory database changes on its own, so a +# check on every PR would turn unrelated PRs red the day a new advisory lands. On PRs it runs +# only when the dependency files change; new advisories against main are caught by the push +# and weekly runs instead. + +name: Audit + +on: + pull_request: + branches: [main] + paths: + - package.json + - package-lock.json + - .github/workflows/audit.yml + push: + branches: [main] + schedule: + - cron: "17 6 * * 1" + workflow_dispatch: + +permissions: + contents: read + +jobs: + audit: + name: npm audit (production dependencies) + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: "25.7" + check-latest: true + + - name: Audit the lockfile + run: npm audit --package-lock-only --omit=dev --audit-level=high diff --git a/README.md b/README.md index dfeff954a..1d6d7dc71 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,8 @@ The desktop app is in the same repository as `atomic-agent-desktop` (on Arch `at Debian, Ubuntu (x86-64, arm64): +> **Not in the apt repository yet.** Until the next release rebuilds the repository, `apt install atomic-agent-desktop` answers "Unable to locate package". For now, download `Atomic-Agent--amd64.deb` (or `-arm64.deb`) from the [releases page](https://github.com/AtomicBot-ai/atomic-agent/releases) and install it with `sudo apt install ./Atomic-Agent--amd64.deb`. The dnf and AUR commands below already work. + ```bash curl -fsSL https://atomic-agent-packages.fsn1.your-objectstorage.com/atomic-chat.gpg | sudo tee /usr/share/keyrings/atomic-chat.gpg >/dev/null echo "deb [signed-by=/usr/share/keyrings/atomic-chat.gpg] https://atomic-agent-packages.fsn1.your-objectstorage.com/deb stable main" \ @@ -154,6 +156,8 @@ If something isn't working: Building something with Atomic Agent, stuck on setup, or just want to share what you are working on? Grab a slot and talk to the team directly: **[cal.com/atomicagent/demo](https://cal.com/atomicagent/demo)**. No agenda required. Questions, feedback, feature requests, or a plain hello all count. We read every issue and every Discord message too, but sometimes a 15-minute call beats a week of comments. +Found a security problem? Please report it privately, not in a public issue: see [SECURITY.md](SECURITY.md). + ## Benchmarks On the public **GAIA validation Level 1** split (53 tasks), Atomic Agent and Hermes drove the **same** local `qwen-3.6-35b-a3b` (`llama-server`, UD-Q4_K_XL), with the same step budget and timeout. The only variable is the agent loop. @@ -353,7 +357,7 @@ Full TUI guide: [TUI.md](docs/user/tui.md). "localModels": { "managed": { "parallel": 3 } } ``` -`workers` (1..8, default 2) is the default fan-out width when the orchestrator does not name one; it is not a ceiling, and the orchestrator can ask for more on a given call. What bounds the width on a local leg is `localModels.managed.parallel`, the llama-server `--parallel` slot count (default `"auto"`, sized from the daemon's context; a number pins it; applied on the next daemon start). On a cloud worker leg the width is capped by `cloudWorkers` (1..32, default 4). The orchestrator model is the provider's `defaultChatModel`; on a local leg the worker model is the one the managed daemon serves. `reviewStallSteps` (default 6, `0` disables) nudges an orchestrator that keeps reading instead of delegating or replying. `turnBudgetUsd` and `turnTimeBudgetMs` (unset or `0`: no cap) cap one fusion turn's spend (the orchestrator's plus the workers', prompt-cache discount included; local tokens count as $0) and its wall-clock time: once either is reached no new delegation or worker starts, running workers are stopped, and the orchestrator is told to tell you plainly what was done and what was not. `escalation: "advisor"` (default `"off"`) lets a stuck worker (the same result three times, six read-only steps, or a first answer cut by its time limit) ask a stronger model for a short hint once; the hint reaches the worker as a mid-turn message and the worker carries on. The escalation model is `escalationProvider`, by default the orchestrator leg, and never the worker leg; each escalation is capped by `escalationMaxUsd` (default $0.25), counts toward the turn's spend cap, and its cost is shown on the worker's row. `escalationReplace: true` additionally re-runs a task that stays stuck after the hint on the escalation model (priced models only). The person can also stop or escalate one running worker by hand (`runtime.fusionWorkers`). `judge: "on"` (default `"off"`, model `judgeProvider`, by default the orchestrator leg) gives every task an acceptance checklist in its brief, built from the contract's checks, provides and declared files plus the task's deliverable; after the work, failed checks, missing provides and missing files are red whatever any model says, and a judge with a fresh context reads the files and rules on the rest. A task closes only on a green list; a red one is re-run once with the findings and reported as failed if still red. The judge's spend counts toward the turn's cap. `autoReplan: N` (default `0`, off; at most 5) makes the planner re-plan by itself: after a fan-out in which tasks did not close (failed, judged red, handed back, out of steps or time), the result lists them with the facts and its next step may only re-delegate them, so you do not have to type "continue"; tasks that depend on a failed one are held for that replan instead of being run against missing inputs. Each task gets N replans per turn; after that the planner is told to report it plainly. `isolation: "worktree"` (default `"off"`) gives every task its own workspace: a detached git worktree of the folder's current state (uncommitted and untracked files included) inside a git repo, a copy of the folder elsewhere. When the task's wave ends its changes are merged back with a three-way merge; files changed by only one task apply cleanly, a file that changed on both sides and cannot be merged is left as it was and the planner gets a merge task for it, a task that wiped a declared input is refused, and a failed task's changes are thrown away. +`workers` (1..8, default 2) is the default fan-out width when the orchestrator does not name one; it is not a ceiling, and the orchestrator can ask for more on a given call. What bounds the width on a local leg is `localModels.managed.parallel`, the llama-server `--parallel` slot count (default `"auto"`, sized from the daemon's context; a number pins it; applied on the next daemon start). On a cloud worker leg the width is capped by `cloudWorkers` (1..32, default 4). The orchestrator model is the provider's `defaultChatModel`; on a local leg the worker model is the one the managed daemon serves. `reviewStallSteps` (default 6, `0` disables) nudges an orchestrator that keeps reading instead of delegating or replying. `turnBudgetUsd` and `turnTimeBudgetMs` (unset or `0`: no cap) cap one fusion turn's spend (the orchestrator's plus the workers', prompt-cache discount included; local tokens count as $0) and its wall-clock time: once either is reached no new delegation or worker starts, running workers are stopped, and the orchestrator is told to tell you plainly what was done and what was not. `maxRoundsPerTurn` (default 6) and `maxTasksPerTurn` (default 48; `0` turns either off) cap how many delegations and worker tasks one fusion turn may start, counted rather than priced so they mean something on a subscription leg; the fan-out's approval line names the round and the task count. Fusion also stops a fan-out that keeps failing the same way: one worker that fails because the setup cannot work (the worker model does not fit the tool-call protocol, its CLI is not installed or signed in, it is out of credit), or three in a row with the same error, and no new worker starts; the next delegation on that leg in the same turn is refused, and the orchestrator is told to explain the error to you instead of retrying. `escalation: "advisor"` (default `"off"`) lets a stuck worker (the same result three times, six read-only steps, or a first answer cut by its time limit) ask a stronger model for a short hint once; the hint reaches the worker as a mid-turn message and the worker carries on. The escalation model is `escalationProvider`, by default the orchestrator leg, and never the worker leg; each escalation is capped by `escalationMaxUsd` (default $0.25), counts toward the turn's spend cap, and its cost is shown on the worker's row. `escalationReplace: true` additionally re-runs a task that stays stuck after the hint on the escalation model (priced models only). The person can also stop or escalate one running worker by hand (`runtime.fusionWorkers`). `judge: "on"` (default `"off"`, model `judgeProvider`, by default the orchestrator leg) gives every task an acceptance checklist in its brief, built from the contract's checks, provides and declared files plus the task's deliverable; after the work, failed checks, missing provides and missing files are red whatever any model says, and a judge with a fresh context reads the files and rules on the rest. A task closes only on a green list; a red one is re-run once with the findings and reported as failed if still red. The judge's spend counts toward the turn's cap. `autoReplan: N` (default `0`, off; at most 5) makes the planner re-plan by itself: after a fan-out in which tasks did not close (failed, judged red, handed back, out of steps or time), the result lists them with the facts and its next step may only re-delegate them, so you do not have to type "continue"; tasks that depend on a failed one are held for that replan instead of being run against missing inputs. Each task gets N replans per turn; after that the planner is told to report it plainly. `isolation: "worktree"` (default `"off"`) gives every task its own workspace: a detached git worktree of the folder's current state (uncommitted and untracked files included) inside a git repo, a copy of the folder elsewhere. When the task's wave ends its changes are merged back with a three-way merge; files changed by only one task apply cleanly, a file that changed on both sides and cannot be merged is left as it was and the planner gets a merge task for it, a task that wiped a declared input is refused, and a failed task's changes are thrown away. In the TUI, fusion is the last row of the composer's **Where it runs** switch (`ctrl+r`, or click the backend word): it needs two providers that can answer, one per leg, and says which one is missing otherwise. While it is on, the backend word is an orange chip and the composer and the chat bubbles take the same tint. `/runmode local|cloud|fusion` and `ctrl+g 1/2/3` pick a mode from the keyboard; `/runmode swap` trades the orchestrator and worker legs; `/runmode status` says what the mode resolves to. @@ -681,6 +685,7 @@ Everything Atomic Agent does is inspectable and interruptible: - **Failure taxonomy:** transport, grammar, model, tool, and cancellation failures classified across events, traces, metrics, TUI, sidecar, and HTTP. - **Allow always:** where a rule can be saved, every place that asks (TUI, CLI, Telegram, Discord, the desktop app) offers a third answer that saves it, for example `Always allow "git status" in this project` (`ctrl+n` in the TUI, `w` in the CLI). Rules live in `/approval-rules.json` (allow, ask, deny) and in a project's `.atomic-agent/approval-rules.json`, where only ask and deny count, so a project cannot grant itself anything. Deny wins over ask, and ask wins over allow. A shell allow rule covers simple commands only (no pipes, `&&` or redirects), while deny and ask rules apply to every command in a chain. Shell rules are saved by program and subcommand. Dangerous commands and secret files never get an allow rule. The desktop app lists saved rules in Settings › Privacy. - **Dangerous commands ask every time:** `rm -rf`, `git reset --hard`, force pushes (including `--force-with-lease` and `git push origin +main`), `git clean -fdx`, `git checkout -- .`, `git branch -D`, `find -delete`, `sudo`, `bash -c`, `eval`, `chmod 777` and similar fall in the `shell_dangerous` category. A session grant for the shell never covers them, and approving a Fusion fan-out does not let workers run them; only the top approval level skips the question. +- **HTTP requests ask per host:** "allow for this session" on an `os.http.request` prompt covers that origin (scheme, host and port) only, so a POST to another host asks again. A redirect that would carry a POST body to another host is not followed; the agent has to make a new request there. Credential headers (`Authorization`, `Cookie`, API keys) are masked in the prompt and are not sent on a redirect to another host. A GET runs without asking by default (`http.approvalMode: "writes"`), and a GET can still carry data out in its URL and headers; set `http.approvalMode` to `"always"` or use `http.hostAllowlist` if that matters to you. - **Risk check (opt-in):** with `agent.smartApproval` set to `true` in `config.json` (off by default), a small model rates actions you could grant for the whole session before you are asked. Low-risk actions run; the rest come to you as usual. Categories that cannot be granted, such as `trust_config`, and reads outside the working scope are not rated and always ask. If the check does not answer in 8 seconds, you are asked. - **No-progress guard:** repeated identical tool calls draw a warning at 3 repeats and a hard veto at 5; after 3 consecutive vetoes the agent is forced into a graceful reply. Calls are judged on the recent window and on what changed since: re-running the tests after an edit or re-reading a log after a wait is a new question, not a repeat. A deliberate poll (`sleep 60 && tail build.log`, waiting on a background job) is refused only after 20 identical answers. - **Per-session FIFO:** every surface enters the same `TurnController`; one session stays ordered while different sessions run concurrently. diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..d1459269b --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,65 @@ +# Security Policy + +Atomic Agent runs shell commands, edits files and drives a browser on your machine, so we treat security reports as our top priority. Thank you for helping keep users safe. + +## Reporting a vulnerability + +Please do not open a public issue, pull request or Discord post for a security problem. + +Report it privately in one of two ways: + +1. **GitHub private vulnerability reporting (preferred).** Open [Security → Report a vulnerability](https://github.com/AtomicBot-ai/atomic-agent/security/advisories/new) in this repository. Only the maintainers can see the report, and we can work on the fix and the advisory with you there. +2. **E-mail:** [support@atomicbot.ai](mailto:support@atomicbot.ai). Put `SECURITY` at the start of the subject line. + +Please include: + +- the version (`atomic-agent --version`, or the desktop release you installed) and your OS; +- what an attacker can do, and what they need first (a malicious web page, a crafted repository, a hostile MCP server, local access, and so on); +- steps to reproduce, ideally a minimal proof of concept; +- any logs or traces, with your own keys removed. + +## What happens next + +- We confirm we received the report within 3 business days. +- We send a first assessment (whether we can reproduce it and how severe we think it is) within 10 business days. +- We keep you updated until it is fixed, and we tell you before anything is published. +- Once a fix is released, we publish a GitHub security advisory and credit you by name or handle, unless you ask us not to. + +Please give us a reasonable time to ship a fix before you disclose the issue publicly. We aim to fix critical issues within 30 days. + +## Supported versions + +We fix security issues in the latest release of: + +- the terminal agent (`v0.x` releases, installed with the install script or a Linux package); +- the desktop app (`desktop-v0.x` releases). + +Both update in place. Please check that a problem still happens on the latest release before you report it. + +## Scope + +In scope, for example: + +- a way to run a command, write or delete a file, send a message or submit a form **without the approval prompt** the action should show; +- a prompt injection (from a web page, a file, a repository, an e-mail or an MCP tool result) that gets around an approval, a blocked command or a read-scope check; +- a way to read the agent's own secrets (`/.env`, `config.json`, provider keys) without the prompt that should guard them, or to get them past the masking of secrets in tool output; +- a way for a project folder to grant itself permissions it should not have; +- data leaving the machine in a way the [README](README.md#privacy-and-egress) does not describe, including through analytics or crash reports; +- problems in the installer, the update flow, or the signing and checksums of release files. + +Out of scope: + +- actions you approved, or that run unasked because you chose a higher approval level, a session grant or an "always allow" rule; +- at approval level 5 (full trust), or in an overnight run started with `--no-approval`, the agent doing what it is asked; +- what a skill, a shell command or an MCP server you installed and allowed does on its own; +- a model giving wrong or harmful answers without crossing a security boundary; +- issues in third-party services (model providers, MCP servers, websites) that are not caused by Atomic Agent; +- the atomicagent.io website, unless it affects the agent or its downloads (report those to the same contacts, and we will route them). + +## Safe harbor + +We will not take legal action against research done in good faith under this policy: test only on your own machine and accounts, do not access other people's data, do not degrade our services, and give us time to fix the issue before you disclose it. + +## More + +How the agent protects you (approvals, Trash instead of delete, blocked commands, telemetry and signed builds) is described at [atomicagent.io/security](https://atomicagent.io/security) and in the [Safety and Observability](README.md#safety-and-observability) section of the README. diff --git a/docs/proposals/tool-catalog-deduplication.md b/docs/proposals/tool-catalog-deduplication.md index fc4175934..40d45c329 100644 --- a/docs/proposals/tool-catalog-deduplication.md +++ b/docs/proposals/tool-catalog-deduplication.md @@ -1,6 +1,6 @@ # Remove existing Git tool descriptor duplicates -Status: proposed +Status: done (ATO-1296) Owner: src/prompt/ + src/tools/os/git/ Stage05l found 88 DEFAULT_TOOL_DESCRIPTORS entries for 85 names. os.git.checkout occurs at zero-based positions31/48, commit32/47 and push33/43. These copies predate the reorganization. [The conformance test](../../src/tools/tool-contract-conformance.test.ts) freezes complete ordered fingerprints and positions with owner/reason; changed, removed, third or newly duplicated entries fail. @@ -9,4 +9,6 @@ Removing entries during a mechanical move would change behavior: both copies con A separate change should select one descriptor from actual Git parser/schema/approval semantics, remove the second copies and retire the exact ledger entries together. Verify plain/strict native wire, tool.view under all roles, rare-tool loading, grammar offered names, Git arguments and approval. Record the expected stable-prefix byte change and one-time cache invalidation; do not merely regenerate golden outputs. Require 85 unique entries with no duplicate ledger afterward. Do not infer Git policy from readonly or change Git execution incidentally. -This is explicit behavioral debt outside mechanical05l acceptance. Other families' canonical ownership can expand separately: the current global gate proves presence/format for them, rather than semantic equality of every representation. [Stage05 acceptance](../testing/stage-05-validation.md) records that limit. +Resolved by ATO-1296. Each tool kept the copy that matches its implementation in `src/tools/os/git/` and the JSON Schema in `src/prompt/github-tool-args-schemas.ts`: commit keeps `paths` (the second copy had dropped it), push keeps `setUpstream`, and checkout keeps its schema argument order. All three keep the default tier, as the first copy (the one native wire already used) had; `compact-tool-forms.ts` still collapses push and checkout under the compact prompt. The full prompt loses the three second copies, so the stable prefix changes once. The ledger is gone and the conformance test now requires every descriptor name to be unique. + +This was explicit behavioral debt outside mechanical05l acceptance. Other families' canonical ownership can expand separately: the current global gate proves presence/format for them, rather than semantic equality of every representation. [Stage05 acceptance](../testing/stage-05-validation.md) records that limit. diff --git a/docs/testing/test-type-debt.json b/docs/testing/test-type-debt.json index 3a0531bd9..e8a1e8d4a 100644 --- a/docs/testing/test-type-debt.json +++ b/docs/testing/test-type-debt.json @@ -2679,8 +2679,6 @@ "src/tui/reduce-ui-actions.test.ts", "src/tui/reduce-ui-actions.ts", "src/tui/reducer-helpers.ts", - "src/tui/render/chat-line-builder.test.tsx", - "src/tui/render/chat-line-builder.ts", "src/tui/render/close-streaming-markdown.test.ts", "src/tui/render/close-streaming-markdown.ts", "src/tui/render/linkify-text.test.tsx", @@ -2886,7 +2884,7 @@ "src/update/run-app-update.ts", "src/version.ts" ], - "testTsx": 112, + "testTsx": 111, "diagnostics": [ { "path": "src/agent/agent-loop-fusion-seams.test.ts", @@ -5824,14 +5822,6 @@ "line": 8, "count": 1 }, - { - "path": "src/tui/render/chat-line-builder.test.tsx", - "code": 2739, - "message": "Type '{ sessionId: string; workingDir: string; llamaUrl: string; browserChannel: string; browserHeadless: false; approvalLevel: number; maxSteps: number; skillCount: number; }' is missing the following properties from type 'TuiSessionInfo': completionMaxTokens, localBackendConfigured", - "anchor": "SESSION", - "line": 12, - "count": 1 - }, { "path": "src/tui/run-mode/fusion-intro.test.ts", "code": 2741, diff --git a/docs/user/overnight.md b/docs/user/overnight.md index 3d3c2bc7b..5140a33bc 100644 --- a/docs/user/overnight.md +++ b/docs/user/overnight.md @@ -22,7 +22,7 @@ atomic-agent overnight "migrate the tests to vitest" --budget 5 --hours 8 --dir `atag overnight` is the same command. -In the foreground you get one line per step, for example `[step 12 | 14m | $0.42 of $5.00] read src/app.ts`, and a summary at the end. Ctrl+C cancels the run; a second Ctrl+C quits. The exit code is 0 only when the task completed. +In the foreground you get one line per step, for example `[step 12 | 14m | $0.42 of $5.00] read src/app.ts`, and a summary at the end. Ctrl+C cancels the run; a second Ctrl+C quits. The summary says `overnight completed` when the agent finished on its own and `overnight stopped ()` when a limit ended the run. The exit code is 0 when the agent finished, 2 when it stopped at the budget, time or step limit, made no progress, or the provider ran out of credit (including a resume that found the budget or time already used), and 1 when the run failed or was cancelled. With `--detach`, check on the run with `atomic-agent task show ` and stop it with `atomic-agent task cancel `. @@ -32,7 +32,7 @@ Nobody is there to answer, so nobody is asked. An action that needs approval is ## Limits -- **Money.** When the cap is reached the agent writes a short summary and stops with `stopped: spent $X of $Y budget`. Spend is an estimate from the provider's price list and counts the main agent loop only (not memory, compaction or Fusion workers), so a run can end slightly over the cap. Local models count as $0. +- **Money.** When the cap is reached the agent writes a short summary and stops with `stopped: spent $X of $Y budget`. Spend is an estimate from the provider's price list and counts the main agent loop only (not memory, compaction or Fusion workers), so a run can end slightly over the cap. Local models count as $0. In Fusion mode, paid workers (a cloud worker or role model, an escalation, the judge) are not in this figure at all and do not stop the run, so with them the run can end well over the cap. Cap them per turn with `llm.runMode.fusion.turnBudgetUsd`. - **Time.** The run stops after `--hours`. If both caps are set, the first one reached ends the run. - **Loops.** The loop guard leaves long normal work alone. Re-running the tests after an edit or polling a long build is fine. A poll that keeps getting the same answer is stopped after 20 identical answers, and the guard stops a real loop sooner. diff --git a/package-lock.json b/package-lock.json index 5469b6350..d9cc83398 100644 --- a/package-lock.json +++ b/package-lock.json @@ -20,7 +20,7 @@ "cron-parser": "^5.5.0", "diff": "^9.0.0", "exceljs": "^4.4.0", - "fast-xml-parser": "^5.7.1", + "fast-xml-parser": "^5.12.0", "fuzzysort": "^3.1.0", "grammy": "^1.42.0", "html-to-text": "^10.0.1", @@ -1224,9 +1224,9 @@ } }, "node_modules/@nodable/entities": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/@nodable/entities/-/entities-2.1.0.tgz", - "integrity": "sha512-nyT7T3nbMyBI/lvr6L5TyWbFJAI9FTgVRakNoBqCD+PmID8DzFrrNdLLtHMwMszOtqZa8PAOV24ZqDnQrhQINA==", + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/@nodable/entities/-/entities-3.1.0.tgz", + "integrity": "sha512-LsS/DjHr+uDM647Gru/cA8+J3a3HfhttwCKLyuoyN7yXTFCxKBtSgMQBvrW9yNPa2/zDRUNuGPaH5QUFoa4arQ==", "funding": [ { "type": "github", @@ -2257,6 +2257,18 @@ "integrity": "sha512-7UvmKalWRt1wgjL1RrGxoSJW/0QZFIegpeGvZG9kjp8vrRu55XTHbwnqq2GpXm9uLbcuhxm3IqX9OB4MZR1b2A==", "license": "MIT" }, + "node_modules/anynum": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/anynum/-/anynum-1.0.1.tgz", + "integrity": "sha512-N6//FLET/tXYNM/F6ABca1oH6fWB+KlTt909Le28WMDBk8oaT4vY17DCrwg2MvmuqUKt3Ni4N5dGJ/EoBgcO6A==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/NaturalIntelligence" + } + ], + "license": "MIT" + }, "node_modules/archiver": { "version": "5.3.2", "resolved": "https://registry.npmjs.org/archiver/-/archiver-5.3.2.tgz", @@ -4101,9 +4113,9 @@ } }, "node_modules/fast-xml-parser": { - "version": "5.7.1", - "resolved": "https://registry.npmjs.org/fast-xml-parser/-/fast-xml-parser-5.7.1.tgz", - "integrity": "sha512-8Cc3f8GUGUULg34pBch/KGyPLglS+OFs05deyOlY7fL2MTagYPKrVQNmR1fLF/yJ9PH5ZSTd3YDF6pnmeZU+zA==", + "version": "5.12.0", + "resolved": "https://registry.npmjs.org/fast-xml-parser/-/fast-xml-parser-5.12.0.tgz", + "integrity": "sha512-/pXxRBtwxY9w8z7YbgsvIPPcPvFDsTbIkWwTQJhzNJPokj+TbN6ftmcv4j/pEQDvOCNnKGDzuk5Bo2oogIOiFg==", "funding": [ { "type": "github", @@ -4112,10 +4124,12 @@ ], "license": "MIT", "dependencies": { - "@nodable/entities": "^2.1.0", - "fast-xml-builder": "^1.1.5", - "path-expression-matcher": "^1.5.0", - "strnum": "^2.2.3" + "@nodable/entities": "^3.1.0", + "fast-xml-builder": "^1.3.1", + "is-unsafe": "^2.0.3", + "path-expression-matcher": "^1.6.2", + "strnum": "^2.5.1", + "xml-naming": "^0.3.0" }, "bin": { "fxparser": "src/cli/cli.js" @@ -4902,6 +4916,18 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/is-unsafe": { + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/is-unsafe/-/is-unsafe-2.0.3.tgz", + "integrity": "sha512-ZBllczCCO267vgdFb6Tq3pv+LhHaCiGMdr+ZsuL+OopruAg7S+3ZDs1vD0RPC0uSrkjzbraxaGjtX1YBxjPeFQ==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/NaturalIntelligence" + } + ], + "license": "MIT" + }, "node_modules/is-wayland": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/is-wayland/-/is-wayland-0.1.0.tgz", @@ -7042,16 +7068,19 @@ } }, "node_modules/strnum": { - "version": "2.2.3", - "resolved": "https://registry.npmjs.org/strnum/-/strnum-2.2.3.tgz", - "integrity": "sha512-oKx6RUCuHfT3oyVjtnrmn19H1SiCqgJSg+54XqURKp5aCMbrXrhLjRN9TjuwMjiYstZ0MzDrHqkGZ5dFTKd+zg==", + "version": "2.5.1", + "resolved": "https://registry.npmjs.org/strnum/-/strnum-2.5.1.tgz", + "integrity": "sha512-UM4nTLWzQiDBseVxUGxN0wJzIk0UU89Qi7pKXz9qISo+lhviJikTuikBsK/oFRlpvYHvSdz0TVoLyQ+sEpYwxA==", "funding": [ { "type": "github", "url": "https://github.com/sponsors/NaturalIntelligence" } ], - "license": "MIT" + "license": "MIT", + "dependencies": { + "anynum": "^1.0.1" + } }, "node_modules/subsume": { "version": "4.0.0", @@ -8052,12 +8081,16 @@ "license": "MIT" }, "node_modules/uuid": { - "version": "8.3.2", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-8.3.2.tgz", - "integrity": "sha512-+NYs2QeMWy+GWFOEm9xnn6HCDp0l7QBD7ml8zLUmJ+93Q5NF0NocErnwkTkXVFNiX3/fpC6afS8Dhb/gz7R7eg==", + "version": "11.1.1", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-11.1.1.tgz", + "integrity": "sha512-vIYxrBCC/N/K+Js3qSN88go7kIfNPssr/hHCesKCQNAjmgvYS2oqr69kIufEG+O4+PfezOH4EbIeHCfFov8ZgQ==", + "funding": [ + "https://github.com/sponsors/broofa", + "https://github.com/sponsors/ctavan" + ], "license": "MIT", "bin": { - "uuid": "dist/bin/uuid" + "uuid": "dist/esm/bin/uuid" } }, "node_modules/vary": { @@ -8994,9 +9027,9 @@ } }, "node_modules/zod": { - "version": "3.25.76", - "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", - "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", + "version": "4.6.7", + "resolved": "https://registry.npmjs.org/zod/-/zod-4.6.7.tgz", + "integrity": "sha512-g8W2D2kU6a8untc053fX1uVBKzM7yqAxW/3zwk83+6RyF2+SKOuV+c0wBDhUwzV1PaGapmZH+rmcUzPf+3CGwg==", "license": "MIT", "funding": { "url": "https://github.com/sponsors/colinhacks" diff --git a/package.json b/package.json index c011c5924..59ca05ee6 100644 --- a/package.json +++ b/package.json @@ -105,7 +105,7 @@ "cron-parser": "^5.5.0", "diff": "^9.0.0", "exceljs": "^4.4.0", - "fast-xml-parser": "^5.7.1", + "fast-xml-parser": "^5.12.0", "fuzzysort": "^3.1.0", "grammy": "^1.42.0", "html-to-text": "^10.0.1", @@ -158,8 +158,10 @@ "qs": "^6.16.1", "shell-quote": "^1.9.0", "tmp": "^0.2.6", + "uuid": "^11.1.1", "ws@^7": "^7.5.11", - "ws@^8": "^8.21.0" + "ws@^8": "^8.21.0", + "zod": "^4.6.6" }, "volta": { "node": "25.7.0" diff --git a/scripts/linux-packaging.test.ts b/scripts/linux-packaging.test.ts index 198159b43..5e36daecb 100644 --- a/scripts/linux-packaging.test.ts +++ b/scripts/linux-packaging.test.ts @@ -17,6 +17,7 @@ import { maxGlibc, PACKAGE_TARGETS, packageAssets, + packagesIndexForArch, parseProductId, parseSha256File, parseSha256Sums, @@ -331,6 +332,36 @@ describe("repository layout", () => { expect(() => repoPath("SHA256SUMS.txt")).toThrow("not a package"); }); + it("splits the pool's index by Architecture, desktop debs included whatever their file name", () => { + const stanza = (pkg: string, arch: string, file: string): string => + `Package: ${pkg}\nVersion: 1\nArchitecture: ${arch}\nFilename: ${file}\nDescription: x\n more\n`; + const all = [ + stanza("atomic-agent", "amd64", "pool/main/a/atomic-agent/atomic-agent_0.7.0_amd64.deb"), + stanza("atomic-agent", "arm64", "pool/main/a/atomic-agent/atomic-agent_0.7.0_arm64.deb"), + stanza("atomic-agent-desktop", "amd64", "pool/main/a/atomic-agent-desktop/Atomic-Agent-0.0.8-amd64.deb"), + stanza("atomic-agent-desktop", "arm64", "pool/main/a/atomic-agent-desktop/Atomic-Agent-0.0.8-arm64.deb"), + stanza("some-docs", "all", "pool/main/s/some-docs/some-docs_1_all.deb"), + ].join("\n"); + const amd64 = packagesIndexForArch(all, "amd64"); + expect(amd64.match(/^Package: .*$/gm)).toEqual([ + "Package: atomic-agent", + "Package: atomic-agent-desktop", + "Package: some-docs", + ]); + expect(amd64).toContain("Filename: pool/main/a/atomic-agent-desktop/Atomic-Agent-0.0.8-amd64.deb\n"); + expect(amd64).not.toContain("arm64"); + expect(amd64.endsWith("\n\n")).toBe(true); + expect(packagesIndexForArch(all, "arm64")).toContain("Atomic-Agent-0.0.8-arm64.deb"); + expect(packagesIndexForArch(all.replaceAll("\n", "\r\n"), "arm64")).toContain("Package: atomic-agent-desktop"); + expect(packagesIndexForArch("", "amd64")).toBe(""); + }); + + it("builds the deb index without dpkg-scanpackages' --arch, which skips the desktop's file names", () => { + const script = readFileSync(new URL("./linux-repo.ts", import.meta.url), "utf8"); + expect(script).not.toMatch(/"dpkg-scanpackages",\s*\[[^\]]*"--arch"/); + expect(script).toContain("packagesIndexForArch("); + }); + it("writes a .repo that checks the metadata and, when signed, the packages", () => { const repo = renderRepoFile({ baseUrl: "https://repo.example" }); expect(repo).toContain("[atomic-agent]\n"); diff --git a/scripts/linux-packaging.ts b/scripts/linux-packaging.ts index bb0b1c505..a6aee7bf9 100644 --- a/scripts/linux-packaging.ts +++ b/scripts/linux-packaging.ts @@ -366,6 +366,27 @@ export function repoPath(file: string): string { return `rpm/packages/${file}`; } +/** + * One architecture's `Packages` index out of dpkg-scanpackages' listing of the whole pool: the + * stanzas whose Architecture is `arch` or `all`, in their order. The pool is scanned without + * `--arch` because that option only finds files named `*_.deb`, and electron-builder names + * the desktop's debs `Atomic-Agent--.deb`, so `--arch` left every desktop package + * out of the index while the files sat in the pool (ATO-1403). The Architecture field inside each + * package is what apt goes by, so filtering on it works whatever the file is called. + */ +export function packagesIndexForArch(index: string, arch: string): string { + const stanzas = index + .replace(/\r\n/g, "\n") + .split(/\n{2,}/) + .map((s) => s.trim()) + .filter((s) => s.length > 0) + .filter((s) => { + const field = /^Architecture:[ \t]*(\S+)[ \t]*$/m.exec(s); + return field !== null && (field[1] === arch || field[1] === "all"); + }); + return stanzas.length === 0 ? "" : `${stanzas.join("\n\n")}\n\n`; +} + /** The dnf repository definition users download into /etc/yum.repos.d. */ export function renderRepoFile(options: { baseUrl: string; diff --git a/scripts/linux-repo.ts b/scripts/linux-repo.ts index 671c198c1..09865ac11 100644 --- a/scripts/linux-repo.ts +++ b/scripts/linux-repo.ts @@ -43,6 +43,7 @@ import { KEY_FILE, PACKAGE_NAME, PACKAGE_TARGETS, + packagesIndexForArch, parseProductId, parseSha256File, parseSha256Sums, @@ -243,8 +244,19 @@ function buildDeb(site: string, sign: Sign): void { const deb = join(site, "deb"); const dist = join(deb, "dists", "stable"); const arches = PACKAGE_TARGETS.map((t) => t.deb); + // The whole pool once, then each architecture's stanzas: `--arch` would only find files named + // `*_.deb` and skip the desktop's `Atomic-Agent--.deb` (ATO-1403). + const all = run("dpkg-scanpackages", ["--multiversion", "pool"], { cwd: deb }); + for (const product of Object.values(PRODUCTS)) { + const debs = walk(join(deb, "pool")).filter( + (f) => f.endsWith(".deb") && product.file.test(f.split("/").pop() ?? ""), + ); + if (debs.length > 0 && !new RegExp(`^Package: ${product.name}$`, "m").test(all)) { + fail(`the pool has ${product.name} debs but dpkg-scanpackages listed none of them`); + } + } for (const arch of arches) { - const index = run("dpkg-scanpackages", ["--multiversion", "--arch", arch, "pool"], { cwd: deb }); + const index = packagesIndexForArch(all, arch); const dir = join(dist, "main", `binary-${arch}`); mkdirSync(dir, { recursive: true }); writeFileSync(join(dir, "Packages"), index); diff --git a/src/agent/agent-contract.ts b/src/agent/agent-contract.ts index a889bd376..721ae4439 100644 --- a/src/agent/agent-contract.ts +++ b/src/agent/agent-contract.ts @@ -27,10 +27,14 @@ import type { AgentMetrics } from "../tracing/agent-metrics.js"; import type { StructuredLogger } from "../tracing/structured-logger.js"; import type { ProfileClippedEvent } from "./profile-clip-warning.js"; import type { HistoryTrimmedEvent } from "./history-trim-warning.js"; +import type { ToolDisabledEvent } from "./tool-disabled-notice.js"; import type { ReviewStallSignal } from "./review-stall.js"; -/** See `AgentLoopDependencies.localEngineState`. */ -export type LocalEngineState = "alive" | "dead" | "unknown"; +/** + * See `AgentLoopDependencies.localEngineState`. `atomic_chat_app`: the link + * is served by the Atomic Chat desktop app, a process nothing here starts. + */ +export type LocalEngineState = "alive" | "dead" | "unknown" | "atomic_chat_app"; /** * The answers to "the local model ran out of memory" (ATO-334): restart @@ -118,6 +122,12 @@ export interface AgentLoopDependencies { * for the rest of the turn, and a new turn or a steer is a new answer. */ forgetDeclinedApprovals?: (sessionId: string) => void; + /** + * Deny every approval `sessionId` is waiting on, as nobody's decision + * (`ApprovalGate.denyPendingForSession`). The loop calls it once a + * task's time limit passes during a step (ATO-1367). + */ + denyPendingApprovals?: (sessionId: string, reason: string) => void; slotManager: SlotManager; grammar: string; llmComplete: (params: LlmStreamParams) => Promise; @@ -176,7 +186,11 @@ export interface AgentLoopDependencies { * certainly gone (pid dead or none recorded, no start or load under way), * so the turn ends with `LocalEngineDownError` after a short restart * grace (`LOCAL_ENGINE_RESTART_GRACE_MS`) instead of waiting out the - * whole budget. `alive` and `unknown` keep the wait. `providerId` is the + * whole budget. `atomic_chat_app`: that link is the Atomic Chat app's + * server (ATO-1278), so a refused connection (`ECONNREFUSED`) that lasts + * through a short start grace (`ATOMIC_CHAT_START_GRACE_MS`) ends the + * turn with `AtomicChatNotRunningError`; any other failure there keeps + * the wait. `alive` and `unknown` keep the wait. `providerId` is the * failing link (`readFailingLink`), `undefined` when the error does not * say. Absent (embedders, tests) means `unknown` for every link. */ @@ -806,6 +820,20 @@ export type AgentLoopEvent = budget: number; reason: string; } + | { + /** + * A provider time limit ended a long reply to step `stepIndex` and + * the step is being retried with a notice asking for a shorter + * one. Not an outage: the provider is answering. `attempt` counts + * within the step, `budget` is the ceiling after which the turn + * fails. + */ + type: "reply_time_limit_retried"; + stepIndex: number; + attempt: number; + budget: number; + reason: string; + } | { /** * The completion for step `stepIndex` came back with nothing in @@ -925,6 +953,11 @@ export type AgentLoopEvent = | ProfileClippedEvent /** The packer left older history out of the prompt (ATO-295). */ | HistoryTrimmedEvent + /** + * A tool was left out of this turn's requests: the server refused its + * JSON schema (ATO-1417). Once per tool per turn. + */ + | ToolDisabledEvent | { type: "step_started"; stepIndex: number } | { type: "step_finished"; diff --git a/src/agent/agent-loop-approval-deadline.test.ts b/src/agent/agent-loop-approval-deadline.test.ts new file mode 100644 index 000000000..3e943fbe9 --- /dev/null +++ b/src/agent/agent-loop-approval-deadline.test.ts @@ -0,0 +1,146 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { AgentLoop } from "./agent-loop.js"; +import type { RunTurnOptions } from "./agent-loop.js"; +import { APPROVAL_TIME_LIMIT_REASON } from "./turn/approval-deadline.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import type { CompletionResult } from "../llm/llama-server-client.js"; +import type { + CapabilitiesSummary, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; + +/** + * ATO-1367: a task's time limit ends a step parked on an approval + * prompt. The tool below stands in for one waiting on the gate: it + * resolves only when the loop's `denyPendingApprovals` dep is called. + */ + +function completion(content: string): CompletionResult { + return { + content, + reasoningContent: "", + stop: true, + truncated: false, + timing: { promptMs: 1, predictedMs: 1, promptTokens: 10, predictedTokens: 5 }, + cacheHitTokens: 0, + slotId: 0, + modelId: "mock", + }; +} + +const TOOLS: ToolDescriptor[] = [ + { name: "reply", summary: "Reply to the user.", argsSchema: '{"text": string}' }, + { name: "finish", summary: "Finish the session.", argsSchema: '{"summary": string}' }, + { name: "os.fs.write", summary: "Write.", argsSchema: '{"path": string}' }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +describe("AgentLoop approval wait and the task time limit (ATO-1367)", () => { + let workingDir: string; + + beforeEach(() => { + vi.useFakeTimers(); + workingDir = mkdtempSync(join(tmpdir(), "atomic-approval-deadline-")); + }); + + afterEach(() => { + vi.useRealTimers(); + rmSync(workingDir, { recursive: true, force: true }); + }); + + function setup() { + const denied: Array<{ sessionId: string; reason: string }> = []; + let release: ((reason: string) => void) | null = null; + const registry = buildDefaultToolRegistry(); + registry.register({ + name: "os.fs.write", + description: "write, waiting on an approval nobody answers", + readonly: false, + run: (_args, ctx) => + new Promise((resolve, reject) => { + release = (reason) => + resolve({ tool: "os.fs.write", status: "error", summary: `denied: ${reason}`, details: {}, truncated: false }); + ctx.signal?.addEventListener("abort", () => reject(new Error("aborted")), { once: true }); + }), + }); + let call = 0; + const agent = new AgentLoop({ + registry, + slotManager: new SlotManager(1), + grammar: 'root ::= "ok"', + toolTransport: "grammar", + toolCallAdapter: null, + supportsSlotAffinity: false, + llmComplete: async () => { + call += 1; + return completion( + call === 1 + ? JSON.stringify([{ tool: "os.fs.write", args: { path: "a.txt" } }]) + : JSON.stringify([{ tool: "reply", args: { text: "out of time" } }]), + ); + }, + denyPendingApprovals: (sessionId, reason) => { + denied.push({ sessionId, reason }); + release?.(reason); + release = null; + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: [], + }); + return { agent, denied }; + } + + function options(extra: Partial): RunTurnOptions { + return { + userMessage: "write the file", + maxSteps: 10, + taskMaxSteps: 10, + signal: new AbortController().signal, + ...extra, + }; + } + + it("denies the waiting approval at the limit and ends on a summary step", async () => { + const { agent, denied } = setup(); + const turn = agent.runTurn( + createEmptySessionState({ id: "s-approval", workingDir }), + options({ taskMaxDurationMs: 30_000 }), + ); + await vi.advanceTimersByTimeAsync(29_000); + expect(denied).toEqual([]); + await vi.advanceTimersByTimeAsync(1_000); + expect(denied).toEqual([{ sessionId: "s-approval", reason: APPROVAL_TIME_LIMIT_REASON }]); + const result = await turn; + expect(result.stopCause).toBe("time_ceiling"); + }); + + it("never denies anything on a turn without a task time limit", async () => { + const user = new AbortController(); + const { agent, denied } = setup(); + const turn = agent.runTurn( + createEmptySessionState({ id: "s-chat", workingDir }), + options({ signal: user.signal }), + ); + await vi.advanceTimersByTimeAsync(10 * 60_000); + expect(denied).toEqual([]); + user.abort(); + await turn.catch(() => undefined); + expect(denied).toEqual([]); + }); +}); diff --git a/src/agent/agent-loop-compaction-spend.test.ts b/src/agent/agent-loop-compaction-spend.test.ts new file mode 100644 index 000000000..e24c3ac15 --- /dev/null +++ b/src/agent/agent-loop-compaction-spend.test.ts @@ -0,0 +1,153 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { AgentLoop } from "./agent-loop.js"; +import type { ContextCompactionControl, CompactionStepOptions } from "./compaction-control.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; +import { compressToolResult } from "../compressor/result-compressor.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import type { CompletionResult } from "../llm/llama-server-client.js"; +import type { BuildPromptInput } from "../prompt/build-prompt-types.js"; +import type { CapabilitiesSummary, ToolDescriptor } from "../prompt/stable-prefix.js"; + +/** + * Compaction spend counts toward the task budget (ATO-1322): the + * summarizer's cost reaches the turn through `onSpend`, so a run whose + * only paid calls are compaction still stops at `taskMaxCostUsd`. + */ + +function completion(content: string): CompletionResult { + return { + content, + reasoningContent: "", + stop: true, + truncated: false, + timing: { promptMs: 1, predictedMs: 1, promptTokens: 10, predictedTokens: 5 }, + cacheHitTokens: 0, + slotId: 0, + modelId: "mock", + }; +} + +const readCall = JSON.stringify([{ tool: "os.fs.read", args: { path: "notes.txt" } }]); +const replyCall = (text: string) => JSON.stringify([{ tool: "reply", args: { text } }]); + +const TOOLS: ToolDescriptor[] = [ + { name: "reply", summary: "Reply to the user.", argsSchema: '{"text": string}' }, + { name: "finish", summary: "Finish the session.", argsSchema: '{"summary": string}' }, + { name: "os.fs.read", summary: "Read a file.", argsSchema: '{"path": string}' }, +]; + +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; + +/** A compaction control that reports `usd` on its first call and never rewrites the session. */ +function spendingCompaction(usd: number): { control: ContextCompactionControl; calls: number[] } { + const calls: number[] = []; + const control: ContextCompactionControl = { + open() {}, + close() {}, + beforeStep(_input: BuildPromptInput, options: CompactionStepOptions) { + calls.push(calls.length + 1); + if (calls.length === 1) options.onSpend?.(usd); + return undefined; + }, + }; + return { control, calls }; +} + +describe("AgentLoop compaction spend and the task budget", () => { + let workingDir: string; + + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-compaction-spend-")); + writeFileSync(join(workingDir, "notes.txt"), "hello\n", "utf8"); + }); + + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + function loop(script: (call: number) => string, compaction: ContextCompactionControl) { + const prompts: string[] = []; + let call = 0; + const registry = buildDefaultToolRegistry(); + registry.register({ + name: "os.fs.read", + description: "Read a file.", + readonly: true, + run: async () => + compressToolResult({ tool: "os.fs.read", status: "ok", output: "hello", details: {} }), + }); + const agent = new AgentLoop({ + registry, + slotManager: new SlotManager(1), + grammar: 'root ::= "ok"', + toolTransport: "grammar", + toolCallAdapter: null, + supportsSlotAffinity: false, + llmComplete: async (params) => { + prompts.push(params.prompt); + call += 1; + return completion(script(call)); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: [], + // The model itself is local and free: only compaction costs money. + priceCompletion: () => undefined, + compaction, + }); + return { agent, prompts }; + } + + it("stops at the cap when compaction alone spent it", async () => { + const { control } = spendingCompaction(1.5); + const { agent, prompts } = loop( + (call) => (call === 1 ? readCall : replyCall("stopped: compaction spent the budget")), + control, + ); + const result = await agent.runTurn(createEmptySessionState({ id: "s-compact-cap", workingDir }), { + userMessage: "do the long thing", + maxSteps: 10, + taskMaxSteps: 10, + taskMaxCostUsd: 1, + signal: new AbortController().signal, + }); + // Step 1 ran after the $1.50 compaction; step 2 is the forced summary. + expect(prompts).toHaveLength(2); + expect(prompts[0]).not.toContain("The spend budget for this task is used up"); + expect(prompts[1]).toContain("The spend budget for this task is used up ($1.50 of $1.00)"); + expect(result.stopCause).toBe("cost_ceiling"); + expect(result.spendUsd).toBeCloseTo(1.5, 6); + }); + + it("keeps going when compaction spend stays under the cap", async () => { + const { control } = spendingCompaction(0.25); + const { agent, prompts } = loop( + (call) => (call <= 2 ? readCall : replyCall("done")), + control, + ); + const result = await agent.runTurn(createEmptySessionState({ id: "s-compact-under", workingDir }), { + userMessage: "do the long thing", + maxSteps: 10, + taskMaxSteps: 10, + taskMaxCostUsd: 1, + signal: new AbortController().signal, + }); + expect(result.reason).toBe("reply"); + expect(result).not.toHaveProperty("stopCause"); + expect(prompts.some((p) => p.includes("The spend budget"))).toBe(false); + expect(result.spendUsd).toBeCloseTo(0.25, 6); + }); +}); diff --git a/src/agent/agent-loop-tool-disabled.test.ts b/src/agent/agent-loop-tool-disabled.test.ts new file mode 100644 index 000000000..78c4de98d --- /dev/null +++ b/src/agent/agent-loop-tool-disabled.test.ts @@ -0,0 +1,138 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import type { CompletionResult } from "../llm/llama-server-client.js"; +import { SlotManager } from "../llm/slot-manager.js"; +import type { + CapabilitiesSummary, + ToolDescriptor, +} from "../prompt/stable-prefix.js"; +import { createEmptySessionState } from "../session/session-state.js"; +import { buildDefaultToolRegistry } from "../tools/index.js"; + +import { AgentLoop } from "./agent-loop.js"; +import type { AgentLoopEvent, MemoryContextProvider } from "./agent-loop.js"; +import type { ToolDisabledEvent } from "./tool-disabled-notice.js"; + +/** + * ATO-1417, loop wiring. A completion whose request went out without a + * tool (`droppedTools`, the server refused its schema) becomes one + * `tool_disabled` event per tool per turn, however many completions of + * the turn report it. + */ + +const TOOLS: ToolDescriptor[] = [ + { + name: "finish", + summary: "Finish the session with a summary.", + argsSchema: '{"summary": string}', + }, +]; +const CAPS: CapabilitiesSummary = { + platform: "darwin", + arch: "arm64", + browserChannel: "chrome", + workingDir: "/work", + hasClipboard: true, + hasWmctrl: false, + hasNotifications: true, +}; +const NOOP_PROVIDER: MemoryContextProvider = { + buildMemoryContext: () => ({ recalled: [], index: [] }), +}; +const DROPPED = [ + { name: "notion.search", reason: "schema_rejected" as const }, +]; +const completion = (content: string, dropped = true): CompletionResult => ({ + content, + reasoningContent: "", + stop: true, + truncated: false, + cacheHitTokens: 0, + slotId: 0, + modelId: "mock", + timing: { promptMs: 1, predictedMs: 1, promptTokens: 10, predictedTokens: 5 }, + ...(dropped ? { droppedTools: DROPPED } : {}), +}); +const REPLY = JSON.stringify({ tool: "reply", args: { text: "ok" } }); + +describe("AgentLoop says when a tool was left out of the request", () => { + let workingDir: string; + beforeEach(() => { + workingDir = mkdtempSync(join(tmpdir(), "atomic-tool-disabled-")); + }); + afterEach(() => { + rmSync(workingDir, { recursive: true, force: true }); + }); + + const makeLoop = ( + events: AgentLoopEvent[], + complete: () => CompletionResult, + ): AgentLoop => + new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => complete(), + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: [], + memoryContextProvider: NOOP_PROVIDER, + onEvent: (event) => events.push(event), + }); + const disabled = (events: AgentLoopEvent[]): ToolDisabledEvent[] => + events.filter((e): e is ToolDisabledEvent => e.type === "tool_disabled"); + const completions = (events: AgentLoopEvent[]): number => + events.filter( + (e) => e.type === "llm_event" && e.event.type === "llm_completed", + ).length; + const turn = (loop: AgentLoop, id: string) => + loop.runTurn(createEmptySessionState({ id, workingDir }), { + userMessage: "hello", + maxSteps: 3, + signal: new AbortController().signal, + }); + + it("names the tool once, however many completions of the turn drop it", async () => { + const events: AgentLoopEvent[] = []; + // An unparseable first completion makes the turn ask again: two + // completions in one turn, both sent without the same tool. + const replies = [completion("{{garbage"), completion(REPLY)]; + const loop = makeLoop(events, () => replies.shift() ?? completion(REPLY)); + + await turn(loop, "s1"); + + expect(completions(events)).toBeGreaterThanOrEqual(2); + expect(disabled(events)).toEqual([ + { + type: "tool_disabled", + stepIndex: 0, + tool: "notion.search", + reason: "schema_rejected", + }, + ]); + }); + + it("names it again on the next turn", async () => { + const events: AgentLoopEvent[] = []; + const loop = makeLoop(events, () => completion(REPLY)); + + await turn(loop, "s2"); + await turn(loop, "s2"); + + expect(disabled(events)).toHaveLength(2); + }); + + it("says nothing when every tool went out", async () => { + const events: AgentLoopEvent[] = []; + const loop = makeLoop(events, () => completion(REPLY, false)); + + await turn(loop, "s3"); + + expect(completions(events)).toBeGreaterThan(0); + expect(disabled(events)).toEqual([]); + }); +}); diff --git a/src/agent/agent-loop.test.ts b/src/agent/agent-loop.test.ts index b4d946c93..14b4ce041 100644 --- a/src/agent/agent-loop.test.ts +++ b/src/agent/agent-loop.test.ts @@ -1578,6 +1578,113 @@ describe("AgentLoop end-to-end with mock LLM", () => { expect(waits[0]?.reason).not.toContain("Tried"); }); + it("does not count a slow compaction as a long reply (ATO-1398)", async () => { + let now = 1_000_000; + const clock = vi.spyOn(Date, "now").mockImplementation(() => now); + const controller = new AbortController(); + const events: string[] = []; + let compactions = 0; + const loop = new AgentLoop({ + registry: buildDefaultToolRegistry(), + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async () => { + throw new OpenAiHttpError( + "openai provider 504: A Timeout Occurred", + 504, + "https://api.fake.test/v1/chat/completions", + false, + null, + "fake", + undefined, + { streamError: "A Timeout Occurred" }, + ); + }, + compaction: { + open() {}, + close() {}, + beforeStep: (input) => { + compactions += 1; + if (compactions === 1) now += 70_000; + return Promise.resolve(input.session); + }, + }, + onEvent: (event) => { + events.push(event.type); + if (event.type === "provider_waiting") controller.abort(); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + try { + await loop + .runTurn(createEmptySessionState({ id: "s-slow-compaction", workingDir }), { + userMessage: "build a big page", + maxSteps: 5, + taskMaxSteps: 5, + signal: controller.signal, + }) + .catch(() => undefined); + expect(events).toContain("provider_waiting"); + expect(events).not.toContain("reply_time_limit_retried"); + } finally { + clock.mockRestore(); + } + }); + + it("asks for a shorter reply when a time limit ended a long one (ATO-1398)", async () => { + let now = 1_000_000; + const clock = vi.spyOn(Date, "now").mockImplementation(() => now); + const registry = buildDefaultToolRegistry(); + const requests: string[] = []; + let calls = 0; + const loop = new AgentLoop({ + registry, + slotManager: new SlotManager(2), + grammar: 'root ::= "ok"', + llmComplete: async (request) => { + calls += 1; + requests.push(JSON.stringify(request)); + if (calls === 1) { + now += 340_000; + throw new OpenAiHttpError( + "openai provider 504: A Timeout Occurred", + 504, + "https://api.fake.test/v1/chat/completions", + false, + null, + "fake", + undefined, + { streamError: "A Timeout Occurred" }, + ); + } + return makeCompletion( + JSON.stringify({ tool: "reply", args: { text: "recovered" } }), + ); + }, + toolDescriptors: TOOLS, + capabilities: CAPS, + skillCatalog: SKILLS, + }); + try { + const result = await loop.runTurn( + createEmptySessionState({ id: "s-mid-reply-timeout", workingDir }), + { + userMessage: "build a big page", + maxSteps: 5, + taskMaxSteps: 5, + signal: new AbortController().signal, + }, + ); + expect(result.reason).toBe("reply"); + expect(requests[0]).not.toContain("write it in parts"); + expect(requests[1]).toContain("write it in parts"); + } finally { + clock.mockRestore(); + } + }); + it("does not wait out our own request deadline (issue #490)", async () => { // A first-token timeout is `status === null` — there is no HTTP // response to carry a status — so it used to classify as an outage diff --git a/src/agent/agent-loop.ts b/src/agent/agent-loop.ts index 14b5b7fb6..fe6985c50 100644 --- a/src/agent/agent-loop.ts +++ b/src/agent/agent-loop.ts @@ -14,7 +14,8 @@ import { prepareAgentTurn, prepareTurnPolicies, prepareTurnBudgets, createTurnLo import { refreshMemoryContext, createSurfacedMemoryTracker } from "./turn/turn-memory-context.js"; export { MAX_SURFACED_NOTE_ALLOWLIST } from "./turn/turn-memory-context.js"; import { finalizeAgentTurn, finalizeFailedAgentTurn } from "./turn/turn-finalization.js"; -import { costCeilingNotice, costCeilingReached, recordStepSpend } from "./turn/turn-spend.js"; +import { armApprovalDeadline } from "./turn/approval-deadline.js"; +import { costCeilingNotice, costCeilingReached, recordCompactionSpend, recordStepSpend } from "./turn/turn-spend.js"; export { formatTaskStoppedReply } from "./turn/turn-finalization.js"; import { delegationProducedWork, recordDelegation, wouldRefuse as fusionGateWouldRefuse } from "./policies/fusion-orchestrator-mode.js"; import { observeReviewStep, reviewStallSignal, reviewStallToolSet, takeReviewStallNotice, type ReviewStallSignal } from "./review-stall.js"; @@ -50,6 +51,7 @@ import { getConfig } from "../config/index.js"; import { ProfileClipWarnings, reportProfileClip } from "./profile-clip-warning.js"; import { HistoryTrimWarnings, reportHistoryTrim } from "./history-trim-warning.js"; +import { toolDisabledEvents } from "./tool-disabled-notice.js"; import { CompactPromptLatch } from "../prompt/compact-prompt.js"; /** @@ -148,6 +150,9 @@ export class AgentLoop { const { fusionOrchestratorTurn, toolRole, claimEvidence, linkEvidence, progressNotes, unknownTools } = policies; let { fusionState, reviewStall, replan } = policies; const turn = createTurnLoopState(); + // ATO-1417: tools a provider left out of this turn's requests (the + // server refused their schema), each said once per turn. + const announcedDroppedTools = new Set(); // ATO-486: a new message on a session whose plan is still open gets // one short reminder on the turn's first step, so the model carries // the checklist on or says it is dropped. Never on a plan that is @@ -384,6 +389,9 @@ export class AgentLoop { // summary step gets at least its own five minutes — it is // reserved whichever ceiling bit, and llama-server keeps decoding // the abandoned request until it notices the closed connection. + // Reset right before the model call: a slow compaction ahead of it + // must not count as a long reply. + let requestStartedAt = Date.now(); const requestDeadline = createRequestDeadline( options.signal, finalizationStep @@ -574,6 +582,13 @@ export class AgentLoop { // spend ceiling exists for. if (event.type === "llm_completed") { recordStepSpend(turn, event.completion, contextProviderId, this.deps.priceCompletion); + for (const disabled of toolDisabledEvents( + event.completion.droppedTools, + announcedDroppedTools, + i, + )) { + this.deps.onEvent?.(disabled); + } } if (event.type === "prompt_built") { turn.lastPromptTokens = event.prompt.tokens.total; @@ -619,6 +634,7 @@ export class AgentLoop { signal: requestDeadline.signal, ...(options.providerId ? { providerId: options.providerId } : {}), ...(options.ephemeral ? { ephemeral: true } : {}), + onSpend: (usd) => recordCompactionSpend(turn, usd), }); while (maintenance) { state = await maintenance; @@ -639,10 +655,26 @@ export class AgentLoop { signal: requestDeadline.signal, ...(options.providerId ? { providerId: options.providerId } : {}), ...(options.ephemeral ? { ephemeral: true } : {}), + onSpend: (usd) => recordCompactionSpend(turn, usd), }); } } - const outcome = await executeStep(stepContext, stepDeps); + requestStartedAt = Date.now(); + // A task's time limit also ends a wait on an approval prompt + // nobody is there to answer (ATO-1367). + const disposeApprovalDeadline = + options.taskMaxDurationMs !== undefined && this.deps.denyPendingApprovals + ? armApprovalDeadline({ + msLeft: durationCeilingMs - (Date.now() - taskStartedAt), + deny: (reason) => this.deps.denyPendingApprovals?.(state.id, reason), + }) + : () => undefined; + let outcome: Awaited>; + try { + outcome = await executeStep(stepContext, stepDeps); + } finally { + disposeApprovalDeadline(); + } requestDeadline.dispose(); const durationMs = Date.now() - started; if (turn.awaitingRecovery) { @@ -996,6 +1028,7 @@ export class AgentLoop { signal: requestDeadline.signal, requested: "overflow", ...(options.providerId ? { providerId: options.providerId } : {}), ...(options.ephemeral ? { ephemeral: true } : {}), + onSpend: (usd) => recordCompactionSpend(turn, usd), }) ?? Promise.resolve(state)); } catch (compactionError) { this.deps.logger?.warn("overflow compaction failed", { ...logFields, @@ -1037,6 +1070,7 @@ export class AgentLoop { state, options, stepIndex: i, finalizationStep, noticeForThisStep, effectiveTransport, requestDeadline, resumesStoppedTask, taskStartedAt, durationCeilingMs, providerWaitCfg, recoveryStepAvailable, + requestElapsedMs: Date.now() - requestStartedAt, ...(stepModelMode !== undefined ? { modelMode: stepModelMode } : {}), ...(contextExhausted !== undefined ? { contextExhausted } : {}), }, this.deps, turn); diff --git a/src/agent/close-streaming-markdown.ts b/src/agent/close-streaming-markdown.ts index 3ac2610ee..64c667aa8 100644 --- a/src/agent/close-streaming-markdown.ts +++ b/src/agent/close-streaming-markdown.ts @@ -23,7 +23,7 @@ * (`reply-polish.ts`) reuses it, keeping only plain closer fixes. */ -const FENCE = /^ {0,3}(```|~~~)/gm; +const FENCE = /^[ \t]*(```|~~~)/gm; export function closeStreamingMarkdown(text: string): string { if (text.length === 0) return text; diff --git a/src/agent/compaction-control.ts b/src/agent/compaction-control.ts index b8452c52f..d3dab60d7 100644 --- a/src/agent/compaction-control.ts +++ b/src/agent/compaction-control.ts @@ -15,6 +15,12 @@ export interface CompactionStepOptions { providerId?: string; ephemeral?: boolean; requested?: "overflow"; + /** + * Called once per compaction attempt with what its model calls cost, + * whether the checkpoint was saved or not. The loop adds it to the + * task's spend so a budget counts summaries too (ATO-1322). + */ + onSpend?: (usd: number) => void; } /** Runtime owns subcalls and durability; the loop supplies an exact, pure prompt input. */ diff --git a/src/agent/docs/recovery.md b/src/agent/docs/recovery.md index 52d330c84..191ca6388 100644 --- a/src/agent/docs/recovery.md +++ b/src/agent/docs/recovery.md @@ -9,6 +9,8 @@ Completion parse/validation recovery, transport retry, request-size repair and p A reply cut short (on llama-server, `stop_type: "limit"` without a context overflow counts as the reply cap) is retried by truncation repair with a notice to think less and write a large file in parts. Cloud keeps the cap (two retries per step); local raises it two-fold once, unless the reply ended in a loop. A local reply whose tail repeats one block exactly (at least 256 characters and 8 copies) is aborted mid-stream and retried the same way, at the same cap. A filled window repacks once per step; six retries over back-to-back cut steps end the turn. +A provider time limit that ends a long reply mid-stream (a 408, 504 or 524 or a timeout message reported inside the stream, on a request that ran at least a minute) is not waited out: replaying the same request is cut at the same place. The step is retried with a notice to think less and write a large file in parts, twice at most, and the next timeout ends the turn. It reports `reply_time_limit_retried`, not `provider_waiting`, because the provider is answering. A timeout seconds into a stream, or with `agent.providerWait.enabled` off, keeps the ordinary outage path. + Before that retry, an answer cut at a reply cap (not the window, not a loop) is [continued](../step/answer-continuation.ts) once inside the step: the partial `reply` text is quoted back with a request for the rest only, and the two are joined. A continuation cut again ends the step with what arrived plus a line saying it was cut; one that fails any other way throws the original truncation, and the turn-level retry above runs as before. Never more than one continuation per step. A local thinking profile under the untouched default `localModels.completionMaxTokens` runs under a [lifted cap](../step/thinking-reply-cap.ts) of 32,768 on grammar links, since its think block spends the same budget as the answer; a configured value or `ATOMIC_AGENT_LLAMA_MAX_TOKENS` is kept as is. A request the model server refuses as too large for its context window (llama.cpp `exceed_context_size_error`, an OpenAI-style "maximum context length" 400/413) is compacted once per turn and the same step retried ([overflow compaction](../turn/overflow-compaction.ts)). The compaction plans against the window the server named (llama.cpp's body `n_ctx`, scaled by the agent's estimate over the server's `n_prompt_tokens` when the estimate undercounted), else most of the refused prompt's estimate; that window also drives the rest of the turn's automatic compaction, and only a window the server named is remembered (under the provider that refused) for later turns. A second refusal, or a compaction that frees no room, makes the turn's one `loop_failed` carry a plain "no longer fits this model's context window" error (the server's sentence stays on the cause), unless the turn was stopped or timed out or the refusal blames the reply reserve rather than the conversation. Each attempt and outcome is logged and a successful one emits `prompt_repacked`. @@ -17,11 +19,17 @@ A turn uses bounded continuation legs and task-wide limits, not an unlimited inf Loop gates key on the exact call: the repeat, no-progress and veto-streak counters belong to one `(tool, args)` signature, so a call failing over and over never blocks a different call that succeeds beside it (the composite batch observation only warns). An `os.fs.read` also carries the mutation epoch of its path: a write, edit, applied patch (targets from its diff headers; a dry run counts for nothing), move or restore attempted on that path, landed or failed, starts a new epoch, so the re-read a failed edit calls for is not a repeat (ATO-1000). After three failed attempts in a row on a path with nothing landed, further failures open no epoch, so identical re-reads between ever-different failing edits are vetoed again. Every other call is judged only against identical calls made since the same number of landed writes (a write itself carries no epoch, so writing the same content again is still a streak), and every call but a write or a poll only against those made since the same number of deliberate polls: `{wait: id}`, `{jobs: true}` or a command line that sleeps first ([poll-command.ts](../progress/poll-command.ts)). So re-running the tests after an edit, re-listing a directory after writing into it, or re-reading a log after a wait is a new question, while a poll stuck on one answer is refused at `POLL_NO_PROGRESS_THRESHOLD` (20) identical results instead of 5. The outcome-repeat count looks back over the history window, not the whole turn, and a de-duplicated warning re-arms once the history window has turned over, so a many-hour turn neither piles up stale counts nor grows its maps (ATO-1110). The wandering run rule counts step rounds, not calls: distinct probes of one step, parallel or run in emitted order, are one round, and the window ceiling still bounds wide fan-outs across steps (ATO-999). The `tool executed` log line names, for an os.fs call, the path it acted on and, for a failure, the first line of its error, never file contents; any failure also logs a code-style error code such as `ENOENT` when there is one; other tools log only the error name and that code, since a shell error echoes its command line and output ([fields](../tool-log-fields.ts)). -A refused connection to the managed local model (`local-llama`, managed mode, the active text provider) whose process is certainly gone is not waited out either (ATO-772). The runtime reads the engine state from disk and hands it to recovery as `localEngineState`; recovery reads no files itself. While the process is gone the turn retries every 2 seconds for a short restart grace, so the TUI or desktop supervisor can bring back a model that crashed; then it ends with `LocalEngineDownError` ("The local model isn't running.", reason code `local_engine_down`). A live engine, a start or install under way, an external server, cloud links and `local-llama` as a fallback behind a cloud provider keep the ordinary wait. +A refused connection to the managed local model (`local-llama`, managed mode, the active text provider) whose process is certainly gone is not waited out either (ATO-772). The runtime reads the engine state from disk and hands it to recovery as `localEngineState`; recovery reads no files itself. While the process is gone the turn retries every 2 seconds for a short restart grace, so the TUI or desktop supervisor can bring back a model that crashed; then it ends with `LocalEngineDownError` ("The local model isn't running.", reason code `local_engine_down`). A live engine, a start or install under way, an external server, cloud links and `local-llama` as a fallback behind a cloud provider keep the ordinary wait. The same engine reading turns a 401 from that link into `LocalPortTakenError` (reason code `local_port_taken`, ATO-1277) when the process is certainly gone: our engine is not running, so the server that refused the key is another program on the port (a second Core, Atomic Chat's own); a live or unknown engine keeps the refused-key words. + +A start or install under way means a live process holds the engine operation lock. A lock its owner left behind does not count (ATO-1275): one whose recorded owner is gone, one written before the machine last booted (pids start over after a reboot, so the number may now name an unrelated process), and one that never got a pid. The same reboot rule applies to the engine's pid file: a record from before the last boot is a dead engine. A lock held by a live process, this one included, still keeps the wait. Error reports for refused connections carry why the engine was judged as it was: `engine_liveness`, `engine_liveness_reason` (`lock_held`, `lock_held_self`, `pid_dead`, `pid_before_boot`, `pid_unreadable`, ...) and `engine_lock` (`free`, `held`, `stale_dead_owner`, `stale_before_boot`, ...), fixed words only. The engine log is not attached: its lines can carry prompt text. + +The Atomic Chat app's server is judged the same way with less to read (ATO-1278). For the `atomic-chat` preset link, while it is the active text provider, the runtime answers `atomic_chat_app`: nothing here starts that app. A connection it refuses outright (`ECONNREFUSED`) is retried every 2 seconds for a 4-second start grace, in case the app is still bringing its server up; then the turn ends with `AtomicChatNotRunningError` ("Atomic Chat is not running. Open it and try again.", reason code `atomic_chat_not_running`). An HTTP error from Atomic Chat, a reset or timeout, `atomic-chat` as a fallback behind another provider, and every other provider keep the ordinary wait. Like `local_engine_down`, it is still reported to error reporting as an `llm_failure`. A server error from the managed local model whose current run logged out of memory is not waited out either (ATO-334). The runtime reads the model's log and launch record and hands recovery `localOutOfMemory`; recovery returns `local_out_of_memory` and the loop runs `out-of-memory-recovery.ts`: restart the model with half the context window it ran with (never below 32 768 tokens, at most three restarts with no step completed between them) and replay the same step. A turn with someone watching the chat (`RunTurnOptions.localOutOfMemory: "ask"`: the TUI chat, a desktop chat) asks first through the approval surface (category `local_model_restart`, never granted for the session); an unattended turn (scheduled task, Telegram, Discord, headless run) restarts without asking. The smaller window holds until the model is next started; the config is not written. In the TUI that restart does not grow, and the growing auto window (ATO-848) stops at that size for the model for the rest of the process. Where nothing in the process can restart the model (a headless `serve`, whose model the desktop owns) the turn ends at once with the window to restart with. Every stop ends with `LocalOutOfMemoryError`, whose sentence says to pick a smaller model when a smaller window is not enough. -Every terminal failure names its cause from one shared list (ATO-384): [the failure catalog](../../llm/reliability/failure-catalog.ts) maps a stable `cause` id to a plain `phrase` and one `action` (`retry`, `switch-model`, `fix-key` for a refused key, `new-chat-summary`, `reduce-load`, `add-search-key`, `none`), and [its classifier](../../llm/reliability/classify-failure-cause.ts) picks the entry from the error's typed facts; a local server's typed HTTP answer follows its existing code ([local-server-error](../../llm/reliability/local-server-error.ts)): `local_context_exceeded` is `context_too_long`, `local_request_invalid` is `request_rejected`, a 404/405 is `local_server_not_found`, so `code` and `failure` on one frame never disagree. `loop_failed` carries it as `failure`, a tripped loop breaker's `loop_detected` carries `same_call_repeated` or `loop_detected`, and the HTTP extensions `error` frame carries it as `failure: {cause, phrase, action}` beside its existing fields. Hosts read the id and the button from there instead of matching the error sentence. +A managed local model that is still running but cuts a reply mid-stream (the connection breaks while the body is read, or llama-server sends a 5xx error event inside the stream, `LlamaServerError.midStream`) has the step retried once, at once, after the out-of-memory check and never for a cancelled turn or one of our own deadlines (ATO-1279). A second cut on the same step ends the turn instead of waiting. A 5xx on the open is not part of this: it already gets the client's retries and the outage wait. The `llm_failure` report for a managed model carries `local_fault` (`out_of_memory`, `server_crashed`, `decode_failed`, `none`), read from the current run's log by fixed strings; the log text is never sent. + +Every terminal failure names its cause from one shared list (ATO-384): [the failure catalog](../../llm/reliability/failure-catalog.ts) maps a stable `cause` id to a plain `phrase` and one `action` (`retry`, `switch-model`, `fix-key` for a refused key, `new-chat-summary`, `reduce-load`, `add-search-key`, `none`), and [its classifier](../../llm/reliability/classify-failure-cause.ts) picks the entry from the error's typed facts; a local server's typed HTTP answer follows its existing code ([local-server-error](../../llm/reliability/local-server-error.ts)): `local_context_exceeded` is `context_too_long`, `local_request_invalid` and `local_tools_unsupported` are `request_rejected`, a 404/405 is `local_server_not_found`, so `code` and `failure` on one frame never disagree. `loop_failed` carries it as `failure`, a tripped loop breaker's `loop_detected` carries `same_call_repeated` or `loop_detected`, and the HTTP extensions `error` frame carries it as `failure: {cause, phrase, action}` beside its existing fields. Hosts read the id and the button from there instead of matching the error sentence. Evidence checks prevent unverified check claims, unsourced links and fabricated transcripts from becoming a closing answer. Keep actual observations as the basis for evidence. Read-scope and declared-input guards must survive both solo and batch paths. @@ -35,6 +43,6 @@ Before that repair completion, call JSON that does not parse gets one mechanical ## Sources and tests -- [Loop](../agent-loop.ts), [detector](../loop-detector.ts), [parse recovery](../turn/parse-failure-recovery.ts), [empty recovery](../turn/empty-completion-recovery.ts), [size repair](../turn/size-rejection-recovery.ts), [overflow compaction](../turn/overflow-compaction.ts), [truncation repair](../turn/truncation-recovery.ts). +- [Loop](../agent-loop.ts), [detector](../loop-detector.ts), [parse recovery](../turn/parse-failure-recovery.ts), [empty recovery](../turn/empty-completion-recovery.ts), [size repair](../turn/size-rejection-recovery.ts), [overflow compaction](../turn/overflow-compaction.ts), [truncation repair](../turn/truncation-recovery.ts), [reply time limit](../turn/mid-reply-timeout-recovery.ts). - [Claims](../policies/claim-evidence.ts), [links](../policies/link-evidence.ts), [read coverage](../progress/read-coverage.ts), [review stall](../review-stall.ts). - [Loop tests](../agent-loop.test.ts), [detector tests](../loop-detector.test.ts), [step tests](../step-executor.test.ts), [repair cap tests](../step/step-parsing.test.ts). diff --git a/src/agent/goal/goal-controller.test.ts b/src/agent/goal/goal-controller.test.ts index f285eff4a..5a77b8d7b 100644 --- a/src/agent/goal/goal-controller.test.ts +++ b/src/agent/goal/goal-controller.test.ts @@ -188,6 +188,40 @@ describe("runGoal", () => { expect(out.goal?.status).toBe("budget_limited"); }); + it("gives each round what is left of one money budget and sums the spend (ATO-1288)", async () => { + const budgets: (number | undefined)[] = []; + const h = harness({ + runRound: async (s, _m, options) => { + budgets.push(options.taskMaxCostUsd); + return { ...replied(s, "worked"), spendUsd: 0.75 }; + }, + judgeAnswers: Array(5).fill(verdict("not_met")), + }); + const out = await runGoal(h.deps, { + session, message: "start", goal: goal(["a"]), signal, taskMaxCostUsd: 2, + }); + expect(budgets).toEqual([2, 1.25, 0.5]); + expect(out.result.spendUsd).toBeCloseTo(2.25); + expect(out.goal?.status).toBe("budget_limited"); + expect(out.goal?.statusReason ?? "").toContain("money budget"); + }); + + it("leaves rounds uncapped when the money budget is 0", async () => { + const budgets: (number | undefined)[] = []; + const h = harness({ + runRound: async (s, _m, options) => { + budgets.push(options.taskMaxCostUsd); + return replied(s, "worked"); + }, + judgeAnswers: [verdict("met")], + }); + const out = await runGoal(h.deps, { + session, message: "start", goal: goal(["a"]), signal, taskMaxCostUsd: 0, + }); + expect(budgets).toEqual([undefined]); + expect(out.result).not.toHaveProperty("spendUsd"); + }); + it("honours a pause or clear requested between rounds", async () => { const controls: ("pause" | "clear" | null)[] = [null, "pause"]; const h = harness({ diff --git a/src/agent/goal/goal-controller.ts b/src/agent/goal/goal-controller.ts index 580d45553..824f02e3e 100644 --- a/src/agent/goal/goal-controller.ts +++ b/src/agent/goal/goal-controller.ts @@ -25,7 +25,7 @@ export interface GoalControllerDeps { runRound( session: SessionState, message: string, - options: { taskMaxDurationMs?: number }, + options: { taskMaxDurationMs?: number; taskMaxCostUsd?: number }, ): Promise; /** Run the goal's shell checks in the session's folder. */ runChecks( @@ -54,6 +54,11 @@ export interface GoalRunInput { signal: AbortSignal; /** Wall-clock budget for all rounds together; absent, rounds alone limit. */ taskMaxDurationMs?: number; + /** + * Money budget for all rounds together, in USD (ATO-1288). Each round + * gets what the earlier ones left; absent or `0`, no cap. + */ + taskMaxCostUsd?: number; } export interface GoalRunOutcome { @@ -75,6 +80,11 @@ export async function runGoal( let session = input.session; let message = input.message; let last: RunTurnResult | undefined; + const costCap = input.taskMaxCostUsd !== undefined && Number.isFinite(input.taskMaxCostUsd) && input.taskMaxCostUsd > 0 + ? input.taskMaxCostUsd + : undefined; + // What the rounds spent together; `undefined` until one reports spend. + let spentUsd: number | undefined; const commit = (next: GoalState): GoalState => { goal = next; @@ -84,12 +94,12 @@ export async function runGoal( }; const stop = (status: GoalState["status"], reason: string): GoalRunOutcome => { commit(setGoalStatus(goal, status, now(), reason)); - return finish(last, session, goal); + return finish(last, session, goal, spentUsd); }; for (;;) { const control = deps.pendingControl?.(session.id) ?? null; - if (control === "clear") return finish(last, session, null); + if (control === "clear") return finish(last, session, null, spentUsd); if (control === "pause") return stop("paused", "paused by you"); if (input.signal.aborted) return stop("paused", "the turn was stopped"); if (goal.roundsUsed >= goal.maxRounds) { @@ -99,10 +109,18 @@ export async function runGoal( if (remaining !== undefined && remaining <= 0) { return stop("budget_limited", "the time limit for this run ran out"); } + const costLeft = costCap === undefined ? undefined : costCap - (spentUsd ?? 0); + if (costLeft !== undefined && costLeft <= 0) { + return stop("budget_limited", "the money budget for this run ran out"); + } commit({ ...goal, roundsUsed: goal.roundsUsed + 1, updatedAt: now() }); - last = await deps.runRound(session, message, remaining === undefined ? {} : { taskMaxDurationMs: remaining }); + last = await deps.runRound(session, message, { + ...(remaining === undefined ? {} : { taskMaxDurationMs: remaining }), + ...(costLeft === undefined ? {} : { taskMaxCostUsd: costLeft }), + }); session = last.session; + if (last.spendUsd !== undefined) spentUsd = (spentUsd ?? 0) + last.spendUsd; const abnormal = abnormalEnding(last); if (abnormal) return stop(abnormal.status, abnormal.reason); @@ -223,6 +241,7 @@ function finish( last: RunTurnResult | undefined, session: SessionState, goal: GoalState | null, + spentUsd: number | undefined, ): GoalRunOutcome { // Stopped before the first round ran: an empty result on the input session. const result: RunTurnResult = last ?? { session, reason: "cancelled", stepCount: 0 }; @@ -233,7 +252,14 @@ function finish( metadata: withGoalState(result.session.metadata, goal), }; return { - result: { ...result, session: finalSession, ...(goal === null ? {} : { goal }) }, + // The money of every round, not only the last one: a task's budget + // and its report read this figure. + result: { + ...result, + session: finalSession, + ...(goal === null ? {} : { goal }), + ...(spentUsd === undefined ? {} : { spendUsd: spentUsd }), + }, goal, }; } diff --git a/src/agent/reply-polish.test.ts b/src/agent/reply-polish.test.ts index 8aad52f7a..686b18999 100644 --- a/src/agent/reply-polish.test.ts +++ b/src/agent/reply-polish.test.ts @@ -48,6 +48,14 @@ describe("polishReply: safety", () => { }); describe("polishReply: code is never rewritten", () => { + it("treats a fence indented four or more spaces as code", () => { + const reply = + "1. Step one\n - run:\n\n ```sh\n cd /Users/me/projects/atomic/src/agent/deep/folder\n ```\n\nDone."; + expect(polishReply(reply, tui)).toBe(reply); + const segs = splitFences(reply); + expect(segs.some((s) => s.kind === "code" && s.text.includes("cd /Users/me/projects"))).toBe(true); + }); + it("keeps blank lines, repeats and filler inside a fenced block", () => { const reply = [ "Here is the script:", @@ -102,6 +110,16 @@ describe("polishReply: open markup", () => { expect(polishReply("Done **", tui)).toBe("Done"); }); + it("keeps a glob or a star at the very end", () => { + expect(polishReply("Matches everything under src/**", tui)).toBe("Matches everything under src/**"); + expect(polishReply("The pattern is: *", tui)).toBe("The pattern is: *"); + }); + + it("does not close a marker inside a word, like x**2", () => { + expect(polishReply("The area is x**2", tui)).toBe("The area is x**2"); + expect(polishReply("In Python, write 2**10", tui)).toBe("In Python, write 2**10"); + }); + it("does not close a lone star, which is as likely a glob", () => { expect(polishReply("Use the glob src/*.ts", tui)).toBe("Use the glob src/*.ts"); }); @@ -205,11 +223,11 @@ describe("polishReply: echoed question", () => { describe("polishReply: repeats", () => { it("drops a paragraph repeated right after itself", () => { - expect(polishReply("Para A.\n\nPara A.\n\nPara B.", tui)).toBe("Para A.\n\nPara B."); + expect(polishReply("Para A.\n\nPara A.\n\nPara B.", small)).toBe("Para A.\n\nPara B."); }); it("treats whitespace-only differences as the same paragraph", () => { - expect(polishReply("The build\npasses.\n\nThe build passes.", tui)).toBe("The build\npasses."); + expect(polishReply("The build\npasses.\n\nThe build passes.", small)).toBe("The build\npasses."); }); it("keeps a paragraph repeated with something between", () => { @@ -218,12 +236,18 @@ describe("polishReply: repeats", () => { }); it("drops a second half that restates the whole answer", () => { - expect(polishReply("First.\n\nSecond.\n\nFirst.\n\nSecond.", tui)).toBe("First.\n\nSecond."); + expect(polishReply("First.\n\nSecond.\n\nFirst.\n\nSecond.", small)).toBe("First.\n\nSecond."); }); it("drops the whole answer restated around a code block", () => { const once = "Use this:\n```\nnpm ci\n```"; - expect(polishReply(`${once}\n${once}`, tui)).toBe(once); + expect(polishReply(`${once}\n${once}`, small)).toBe(once); + }); + + it("keeps repeated paragraphs for a cloud model, which repeats only when asked", () => { + expect(polishReply("Para A.\n\nPara A.\n\nPara B.", tui)).toBe("Para A.\n\nPara A.\n\nPara B."); + const halves = "First.\n\nSecond.\n\nFirst.\n\nSecond."; + expect(polishReply(halves, tui)).toBe(halves); }); it("keeps two identical code blocks with no prose between", () => { @@ -278,6 +302,13 @@ describe("polishReply: home paths", () => { ); }); + it("keeps the full path when the user asked for it", () => { + const reply = "Saved to /Users/me/projects/atomic/src/agent/reply-polish.ts now"; + expect(polishReply(reply, { ...tui, userMessage: "What is the full path of the polish file?" })).toBe(reply); + expect(polishReply(reply, { ...tui, userMessage: "give me the absolute path" })).toBe(reply); + expect(polishReply(reply, { ...tui, userMessage: "Напиши полный путь к файлу" })).toBe(reply); + }); + it("leaves a short path alone", () => { expect(polishReply("see /Users/me/a.ts", tui)).toBe("see /Users/me/a.ts"); }); diff --git a/src/agent/reply-polish.ts b/src/agent/reply-polish.ts index 834433341..62842348b 100644 --- a/src/agent/reply-polish.ts +++ b/src/agent/reply-polish.ts @@ -13,13 +13,15 @@ * ("I hope this helps!", "Надеюсь, это поможет!"), at the very start * or end only, and only when something else is left; * - drops a first paragraph that only repeats the user's question; - * - drops a paragraph that repeats the one right before it (exactly or - * up to whitespace), and a reply whose second half repeats its first; + * - small models only: drops a paragraph that repeats the one right + * before it (exactly or up to whitespace), and a reply whose second + * half repeats its first; a cloud model repeats only when asked to; * - small models only: a line repeated three or more times in a row is * kept once (a decoding loop, not content); * - collapses runs of blank lines to one; * - shortens a long absolute path under the home folder to `~/...` - * when it is plain prose (not code, not a link target); + * when it is plain prose (not code, not a link target), unless the + * user's message asks for the full or absolute path; * - Telegram only: turns a markdown table into a list, since the chat * has no tables and a pipe grid wraps into noise on a phone. * @@ -82,7 +84,9 @@ interface Segment { text: string; } -const FENCE_OPEN = /^ {0,3}(`{3,}|~{3,})/; +// Any indent: a fence nested under a list item (`1. ` then a deeper +// bullet) sits four or more spaces in, and is still code. +const FENCE_OPEN = /^[ \t]*(`{3,}|~{3,})/; /** * Split into prose and fenced-code segments, line by line. An unclosed @@ -108,7 +112,7 @@ export function splitFences(text: string): Segment[] { } const marker = open[1]!; const ch = marker[0]!; - const close = new RegExp(`^ {0,3}${ch === "`" ? "`" : "~"}{${marker.length},}\\s*$`); + const close = new RegExp(`^[ \\t]*${ch === "`" ? "`" : "~"}{${marker.length},}\\s*$`); const body: string[] = [line]; let j = i + 1; let closed = false; @@ -178,14 +182,14 @@ function polish(input: string, options: PolishReplyOptions): string { let body = seg.text; if (options.smallModel === true) body = collapseRepeatedLines(body); if (options.channel === "telegram") body = tablesToLists(body); - body = shortenHomePaths(body, options.homeDir ?? safeHomedir()); + if (!asksForFullPath(options.userMessage)) body = shortenHomePaths(body, options.homeDir ?? safeHomedir()); body = collapseBlankLines(body); return { kind: "prose", text: body }; }); if (options.userMessage) segments = dropEchoedQuestion(segments, options.userMessage); segments = dropFillerAtEdges(segments); - segments = dropRepeatedParagraphs(segments); + if (options.smallModel === true) segments = dropRepeatedParagraphs(segments); let out = segments .map((s) => s.text) @@ -235,16 +239,31 @@ function closeOpenMarkup(text: string): string { const base = text.trimEnd(); if (fixed.startsWith(base)) { const suffix = fixed.slice(base.length); - return /^(?:`+|\*\*|~~)+$/.test(suffix) ? fixed : text; + if (!/^(?:`+|\*\*|~~)+$/.test(suffix)) return text; + // `x**2` or `a~~b`: a marker inside a word is maths or a name, not + // an opener, so closing it would add stray stars. + for (const marker of ["**", "~~"]) { + if (suffix.includes(marker) && openerInsideWord(base, marker)) return text; + } + return fixed; } const kept = fixed.trimEnd(); if (kept.length > 0 && base.startsWith(kept)) { + // Only a spaced-off opener ("Done **"). `src/**` and `: *` end in a + // glob, which is content and stays. const removed = base.slice(kept.length); - return /^[\s*~`]+$/.test(removed) ? kept : text; + return /^\s+(?:\*\*|~~|`+)+$/.test(removed) ? kept : text; } return text; } +/** The last `marker` in `text` sits between two word characters. */ +function openerInsideWord(text: string, marker: string): boolean { + const at = text.lastIndexOf(marker); + if (at <= 0) return false; + return /[\p{L}\p{N}_]/u.test(text[at - 1]!); +} + // --- paragraphs ------------------------------------------------------------ function paragraphsOf(text: string): string[] { @@ -455,6 +474,14 @@ function dropEchoedQuestion(segments: Segment[], userMessage: string): Segment[] // --- home paths ------------------------------------------------------------ +/** The user asked for the path as it is: "full path", "абсолютный путь". */ +const FULL_PATH_ASK = + /\b(?:full|absolute|complete|whole|exact)\s+(?:file\s+)?path|\bpath\s+in\s+full\b|(?:полн|абсолютн|цельн|точн)\S*\s+(?:\S+\s+)?пут|пут\S*\s+(?:целиком|полностью)/iu; + +function asksForFullPath(userMessage: string | null | undefined): boolean { + return typeof userMessage === "string" && FULL_PATH_ASK.test(userMessage); +} + /** Paths shorter than this are left alone: they already read fine. */ const LONG_PATH = 40; diff --git a/src/agent/step/step-batch-lifecycle.test.ts b/src/agent/step/step-batch-lifecycle.test.ts index 8b10e2d7d..2dc556646 100644 --- a/src/agent/step/step-batch-lifecycle.test.ts +++ b/src/agent/step/step-batch-lifecycle.test.ts @@ -138,6 +138,31 @@ describe("step batch lifecycle owner seams", () => { expect(policy.progressNote?.args.text).toBe("new note"); }); + // ATO-1365: the repair parse decodes wire names against the list the + // request was BUILT from (`stepDescriptors`), not the full catalog. + it("parses the repaired native batch against the descriptors the request was built from", () => { + const { ctx, deps } = fixture(); + deps.toolTransport = "native_tools"; + register(deps, "mcp.srv.foo__bar"); + register(deps, "mcp.srv.foo.bar"); + const tool = (name: string) => ({ name, tier: "frequent" as const, summary: name, argsSchema: "" }); + // Offered on the wire: both tools, so the underscored one went out + // as `mcp__srv__foo__bar_2`. The full catalog alone has no collision. + const prepared = { + stepDescriptors: [tool("mcp.srv.foo__bar"), tool("mcp.srv.foo.bar")], + stepToolDescriptors: [tool("mcp.srv.foo__bar")], + thinkingOff: false, + } as unknown as ReturnType; + const policy = createStepBatchPolicy(ctx, deps, () => false); + const completion: CompletionResult = { + ...result([]), content: "", + toolCalls: [{ id: "c1", type: "function", function: { name: "mcp__srv__foo__bar_2", arguments: "{}" } }], + }; + const parsingArgs = { ctx, deps, prepared, completion, assumesOpenReasoning: () => false, policy }; + const parsed = finishStepBatchRepair(parsingArgs, completion, { toolTransport: "native_tools", toolCallAdapter: null }); + expect(parsed.batch.calls.map((call) => call.tool)).toEqual(["mcp.srv.foo__bar"]); + }); + it("holds an unsupported check claim once, preserves the refused call and merges the next-step notice", () => { const { ctx, deps, events } = fixture(); register(deps, "reply"); diff --git a/src/agent/step/step-batch-parsing.ts b/src/agent/step/step-batch-parsing.ts index 304d04f2c..4459fecc4 100644 --- a/src/agent/step/step-batch-parsing.ts +++ b/src/agent/step/step-batch-parsing.ts @@ -84,13 +84,15 @@ export function finishStepBatchRepair( retryParseDeps: ParseDeps, ): { ok: true; batch: ToolCallBatch } { const { ctx, deps, prepared, policy, assumesOpenReasoning } = args; - const { stepToolDescriptors, thinkingOff } = prepared; + // The descriptors the request was BUILT from, as in the first parse: + // the strict-widened and wire-name maps must match the wire. + const { stepDescriptors, thinkingOff } = prepared; let parsed = tryParseToolCalls( completion, deps.profile, retryParseDeps, - stepToolDescriptors, + stepDescriptors, thinkingOff, ); parsed = admitRepairedJson(args, completion, parsed); diff --git a/src/agent/step/step-commit.ts b/src/agent/step/step-commit.ts index 67504258e..1e7dd552b 100644 --- a/src/agent/step/step-commit.ts +++ b/src/agent/step/step-commit.ts @@ -8,6 +8,7 @@ import type { BatchExecutionResult } from "../dispatch/batch-contract.js"; import type { ToolCallPayload } from "../../llm/grammar/tool-call-grammar.js"; import type { CompressedToolResult } from "../../compressor/result-compressor.js"; import { compressToolResult, fullToolResult } from "../../compressor/result-compressor.js"; +import { knownSecretsFromEnv, maskSecretsDeep } from "../../error-reporting/secret-detector.js"; import { evidenceMarks } from "./step-evidence.js"; import type { SessionState } from "../../session/session-state.js"; import { rememberConversationPackStart, recordLoadedTool, recordLatestResult, recordTurn, recordLoadedSkill, recordWorldSnapshot, recordPlan } from "../../session/session-state.js"; @@ -150,7 +151,13 @@ export function commitStepBatch(args: StepCommitArgs): StepOutcome { tool: result.tool, status: result.status, summary: result.summary, - ...(result.details !== undefined ? { details: result.details } : {}), + // The session file keeps this copy on disk; only the summary and + // the prompt-bound details were masked on the way in (ATO-1077), + // so the rest is masked here (ATO-1176). The state effects below + // still read the result as the tool wrote it. + ...(result.details !== undefined + ? { details: maskSecretsDeep(result.details, knownSecretsFromEnv()) } + : {}), }); nextSession = applyStateEffects(nextSession, result, modelMode); } diff --git a/src/agent/step/step-contract.ts b/src/agent/step/step-contract.ts index e24c2c7d4..d12db93cc 100644 --- a/src/agent/step/step-contract.ts +++ b/src/agent/step/step-contract.ts @@ -31,7 +31,7 @@ export interface LlmStreamParams { contextWindow: number | null; maxTokens?: number; }) => Pick & - Partial>; + Partial>; /** Internal policy metadata; never serialized to the provider API. */ modelModePolicy?: import("../../llm/model-mode.js").ModelModePolicy; prompt: string; @@ -107,6 +107,8 @@ export interface LlmStreamParams { temperature?: number; /** OpenAI tools payload — set when `toolTransport === "native_tools"`. */ tools?: ReadonlyArray>; + /** See `CompletionRequest.toolWireNames`. */ + toolWireNames?: ReadonlyMap; toolChoice?: unknown; parallelToolCalls?: boolean; /** diff --git a/src/agent/step/step-errors.test.ts b/src/agent/step/step-errors.test.ts index e11794b50..261cab2ba 100644 --- a/src/agent/step/step-errors.test.ts +++ b/src/agent/step/step-errors.test.ts @@ -9,6 +9,8 @@ import { RequestTooLargeError } from "../../llm/reliability/request-too-large.js import { classifyFailure } from "../../llm/reliability/classify-failure.js"; import { shouldAdvance } from "../../llm/fallback/should-advance.js"; import { contextCompactionRejection } from "../turn/compaction-recovery.js"; +import { OpenAiHttpError } from "../../llm/provider/openai/openai-http.js"; +import { parseProviderErrorBody } from "../../llm/provider/openai/parse-provider-error-body.js"; const URL = "http://127.0.0.1:9931/completion"; const ctx = { signal: new AbortController().signal } as unknown as StepContext; @@ -121,3 +123,24 @@ describe("toLlmFailure: the tool a catch-all failure names (ATO-710)", () => { expect((failure as ToolExecutionError).tool).toBe("unknown"); }); }); + +describe("toLlmFailure: Groq's 413 over the plan limit (ATO-1283)", () => { + it("reads the plan-limit sentence alone, without Groq's raw body after it", () => { + const text = JSON.stringify({ error: { + message: "Request too large for model `llama-3.3-70b-versatile` in organization `org_01abc` service tier `on_demand` on tokens per minute (TPM): Limit 6000, Requested 9531, please reduce your message size and try again.", + type: "tokens", code: "rate_limit_exceeded", + } }); + const raw = new OpenAiHttpError( + `openai provider 413: ${text.slice(0, 300)}`, 413, + "https://api.groq.com/openai/v1/chat/completions", false, null, "groq", undefined, + { body: parseProviderErrorBody(text) }, + ); + const failure = toLlmFailure(raw, ctx); + expect(failure).toBeInstanceOf(TransportError); + expect(failure.message).toBe( + "Groq refused the request (413): it is larger than your Groq plan's limit (limit 6000 tokens per minute, this request 9531). " + + "Pick a model with a higher limit or a larger Groq plan.", + ); + expect(failure.cause).toBe(raw); + }); +}); diff --git a/src/agent/step/step-errors.ts b/src/agent/step/step-errors.ts index 2597a4bbd..e5efdfa8a 100644 --- a/src/agent/step/step-errors.ts +++ b/src/agent/step/step-errors.ts @@ -3,6 +3,7 @@ import { LlmFailure, classifyFailure, CancelledError, LlamaServerError, Transpor import { ToolCallParseError } from "../../llm/grammar/tool-call-grammar.js"; import { isLlamaSizeRefusal, toRequestTooLargeError } from "../../llm/reliability/request-too-large.js"; import { SubscriptionCliInvocationError } from "../../llm/provider/subscription-cli/subscription-cli-errors.js"; +import { isGroqRequestTooLarge } from "../../llm/provider/openai/groq-plan-limit.js"; /** @@ -91,7 +92,8 @@ export function toLlmFailure(err: unknown, ctx: StepContext): LlmFailure { // A request the provider refused for its size is the one 400 whose // body the user needs to read: it names the limit. Everything else // keeps the humanized line alone. - const message = isRequestSizeRejection(err) + // Groq's plan-limit sentence already carries its numbers (ATO-1283). + const message = isRequestSizeRejection(err) && !isGroqRequestTooLarge(err) ? `${humanizeOpenAiHttpError(err)} ${requestSizeExcerpt(err.message)}` : humanizeOpenAiHttpError(err); return new TransportError(message, err.status, err.url, { diff --git a/src/agent/step/step-inference.test.ts b/src/agent/step/step-inference.test.ts index 0e2c1f0c4..cc6ba6861 100644 --- a/src/agent/step/step-inference.test.ts +++ b/src/agent/step/step-inference.test.ts @@ -255,3 +255,27 @@ describe("compact prompt decision (ATO-1116)", () => { }); }); +describe("working_dir after a live folder switch (issue #1121)", () => { + it("local mode renders the session's folder, not the bootstrap one, without preparing a workspace", () => { + const { ctx, deps } = fixture(); + let prepared = 0; + const localDeps: StepDependencies = { + ...deps, + modelMode: { mode: "local", source: "provider", providerId: "primary", modelId: null }, + prepareWorkspace: () => { prepared += 1; throw new Error("local mode must not prepare a workspace"); }, + }; + const before = prepareStepInference(ctx, localDeps); + expect(before.prompt.stablePrefix).toContain("working_dir: /fixture"); + + // The desktop folder picker moves the session; bootstrap capabilities still say /fixture. + const switched: StepContext = { ...ctx, workspace: undefined, session: { ...ctx.session, workingDir: "/switched" } }; + const after = prepareStepInference(switched, localDeps); + expect(after.prompt.stablePrefix).toContain("working_dir: /switched"); + expect(after.prompt.stablePrefix).not.toContain("working_dir: /fixture"); + expect(prepared).toBe(0); + + // Same folder again: the stable prefix is byte-identical, so the cache survives. + const again = prepareStepInference(switched, localDeps); + expect(again.prompt.stablePrefix).toBe(after.prompt.stablePrefix); + }); +}); diff --git a/src/agent/step/step-inference.ts b/src/agent/step/step-inference.ts index 74f799d1b..8897d9db8 100644 --- a/src/agent/step/step-inference.ts +++ b/src/agent/step/step-inference.ts @@ -356,7 +356,7 @@ export function prepareStepInference(ctx: StepContext, deps: StepDependencies) { deps.onEvent?.({ type: "prompt_built", prompt: next, slotId: slot.slotId }); if (next.contextBudget) assertContextCapacity(next.text + JSON.stringify(wire?.tools ?? llmParams.tools ?? []), next.contextBudget); return { prompt: next.text, messages: next.messages, contextBudget: next.contextBudget, - ...(wire ? { grammar: wire.grammar, tools: wire.tools, toolChoice: wire.toolChoice, parallelToolCalls: wire.parallelToolCalls } : {}), + ...(wire ? { grammar: wire.grammar, tools: wire.tools, toolWireNames: wire.toolWireNames, toolChoice: wire.toolChoice, parallelToolCalls: wire.parallelToolCalls } : {}), ...(template.useServerTemplate ? { chat: { system: next.stablePrefix, user: next.tail, prefixHash: hashPrefix(next.stablePrefix), enableThinking: template.enableThinking } } : { chat: undefined }) }; }; @@ -565,6 +565,11 @@ export function buildLlmStreamParams(args: { const tools = adapter.descriptorsToTools(args.toolDescriptors, { strict: args.deps.strictTools === true, }); + // Only when two tools escaped alike: the history then names a past + // call by the wire name this request offers for it (ATO-1365). + const toolWireNames = adapter.suffixedWireNames?.(args.toolDescriptors, { + strict: args.deps.strictTools === true, + }); return { ...base, // The structured prompt rides only on the native path; the seam @@ -576,6 +581,7 @@ export function buildLlmStreamParams(args: { // ignore `grammar` entirely and read `tools`, so carrying both makes // the request valid for whichever link actually serves it. tools, + ...(toolWireNames && toolWireNames.size > 0 ? { toolWireNames } : {}), // `auto` instead of `required`. Three production-observed reasons: // * Qwen-thinking providers (Alibaba gate) reject `required` outright // with `<400> InvalidParameter: tool_choice does not support being diff --git a/src/agent/step/step-parsing.test.ts b/src/agent/step/step-parsing.test.ts index 22b7e91e4..b28fdfba3 100644 --- a/src/agent/step/step-parsing.test.ts +++ b/src/agent/step/step-parsing.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from "vitest"; import { GEMMA4_THINK_PROFILE, PLAIN_INSTRUCT_PROFILE, QWEN_THINK_PROFILE } from "../../llm/model-profile.js"; import { DEFAULT_REASONING_BUDGET_TOKENS } from "../../llm/reasoning-budget.js"; -import { REPAIR_MAX_TOKENS, repairReplyCap } from "./step-parsing.js"; +import type { CompletionResult } from "../../llm/llama-server-client.js"; +import type { ToolDescriptor } from "../../prompt/stable-prefix.js"; +import { REPAIR_MAX_TOKENS, repairReplyCap, tryParseToolCalls } from "./step-parsing.js"; describe("repair completion cap", () => { it("keeps the 1024 anti-loop guard for a model without thinking", () => { @@ -34,3 +36,27 @@ describe("repair completion cap", () => { expect(repairReplyCap("native_tools", 20_000, { profile: QWEN_THINK_PROFILE, budgetTokens: 1500 })).toBe(20_000); }); }); + +// ATO-1365: a native call is decoded against the tools the request +// offered, not by textual unescape, so `mcp.srv.foo__bar` never runs as +// its dotted sibling `mcp.srv.foo.bar`. +describe("native tool-call names", () => { + const tool = (name: string): ToolDescriptor => ({ name, tier: "frequent", summary: name, argsSchema: "" }); + const descriptors = [tool("mcp.srv.foo__bar"), tool("mcp.srv.foo.bar")]; + const native = (name: string): CompletionResult => + ({ content: "", reasoningContent: "", toolCalls: [{ id: "c1", type: "function", function: { name, arguments: "{}" } }] }) as unknown as CompletionResult; + const deps = { toolTransport: "native_tools" as const, toolCallAdapter: null }; + + it("resolves each wire name to the tool it was offered for", () => { + const first = tryParseToolCalls(native("mcp__srv__foo__bar_2"), PLAIN_INSTRUCT_PROFILE, deps, descriptors, false); + expect(first.ok && first.batch.calls[0]?.tool).toBe("mcp.srv.foo__bar"); + const second = tryParseToolCalls(native("mcp__srv__foo__bar"), PLAIN_INSTRUCT_PROFILE, deps, descriptors, false); + expect(second.ok && second.batch.calls[0]?.tool).toBe("mcp.srv.foo.bar"); + }); + + it("resolves an escaped name copied into content the same way", () => { + const content = ({ content: '[{"tool":"mcp__srv__foo__bar_2","args":{}}]', reasoningContent: "" }) as unknown as CompletionResult; + const parsed = tryParseToolCalls(content, PLAIN_INSTRUCT_PROFILE, deps, descriptors, false); + expect(parsed.ok && parsed.batch.calls[0]?.tool).toBe("mcp.srv.foo__bar"); + }); +}); diff --git a/src/agent/step/step-parsing.ts b/src/agent/step/step-parsing.ts index 903d88633..9dcb5d7a4 100644 --- a/src/agent/step/step-parsing.ts +++ b/src/agent/step/step-parsing.ts @@ -6,6 +6,7 @@ import type { ModelProfile } from "../../llm/model-profile.js"; import type { ToolDescriptor } from "../../prompt/stable-prefix.js"; import { completionAssumesOpenReasoning, resolveReasoning, normalizeContent, getReasoningTagOptions, memoizeText } from "./step-reasoning.js"; import { openAiToolCallAdapter } from "../../llm/provider/openai/openai-tool-call-adapter.js"; +import type { ToolCallAdapter } from "../../llm/provider/adapters/tool-call-adapter.js"; import { parseToolCalls, extractReasoning } from "../../llm/grammar/tool-call-grammar.js"; import { BatchValidationError } from "./step-errors.js"; import { getReasoningTurnFraming } from "../../llm/model-profile.js"; @@ -156,6 +157,22 @@ export function parseDepsFor( */ const REPAIR_JSON = { repairJson: true } as const; +/** + * The request's own wire-name -> tool-name map (ATO-1365), from the same + * adapter and descriptor list the request was built from. `nameUnescape` + * alone reads `mcp__srv__foo__bar` as `mcp.srv.foo.bar` even when the + * function was offered for `mcp.srv.foo__bar`, and would run the sibling. + */ +function requestWireNames( + adapter: ToolCallAdapter, + deps: ParseDeps, + toolDescriptors: readonly ToolDescriptor[], +): ReadonlyMap | undefined { + return adapter.wireToCanonical?.(toolDescriptors, { + strict: deps.strictTools === true, + }); +} + /** * Non-throwing parser wrapper. The step executor uses it to distinguish * a malformed first attempt (retryable) from any other error shape. @@ -195,11 +212,13 @@ export function tryParseToolCalls( deps.strictTools === true && adapter.strictWidenedArgs ? adapter.strictWidenedArgs(toolDescriptors, { strict: true }) : undefined; + const wireToCanonical = requestWireNames(adapter, deps, toolDescriptors); const batch = adapter.toolCallsToBatch( completion.toolCalls, reasoning, { ...(strictWidenedArgs ? { strictWidenedArgs } : {}), + ...(wireToCanonical ? { wireToCanonical } : {}), repairJson: true, }, ); @@ -245,9 +264,12 @@ export function tryParseToolCalls( // `registry.has(...)` resolves — `nameUnescape` is a no-op for // names that are already dotted, so this is idempotent. const adapter = deps.toolCallAdapter ?? openAiToolCallAdapter; + const wireToCanonical = requestWireNames(adapter, deps, toolDescriptors); const calls = grammarBatch.calls.map((call) => ({ ...call, - tool: adapter.nameUnescape(call.tool), + tool: + wireToCanonical?.get(call.tool) ?? + adapter.nameUnescape(call.tool), })); return { ok: true, batch: { ...grammarBatch, calls } }; } @@ -297,9 +319,12 @@ export function tryParseToolCalls( ); if (grammarBatch.calls.length > 0) { const adapter = deps.toolCallAdapter ?? openAiToolCallAdapter; + const wireToCanonical = requestWireNames(adapter, deps, toolDescriptors); const calls = grammarBatch.calls.map((call) => ({ ...call, - tool: adapter.nameUnescape(call.tool), + tool: + wireToCanonical?.get(call.tool) ?? + adapter.nameUnescape(call.tool), })); return { ok: true, batch: { ...grammarBatch, calls } }; } diff --git a/src/agent/tool-disabled-notice.test.ts b/src/agent/tool-disabled-notice.test.ts new file mode 100644 index 000000000..fe36db97c --- /dev/null +++ b/src/agent/tool-disabled-notice.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it } from "vitest"; + +import { toolDisabledEvents, toolDisabledNotice } from "./tool-disabled-notice.js"; + +describe("tool_disabled (ATO-1417)", () => { + it("says which tool the model cannot use and why, in one sentence", () => { + expect(toolDisabledNotice("notion.search")).toBe( + "Tool notion.search is disabled for this turn: the model server did not accept its schema.", + ); + }); + + it("one event per tool the turn has not announced yet", () => { + const announced = new Set(); + const dropped = [ + { name: "notion.search", reason: "schema_rejected" as const }, + { name: "records.lookup", reason: "schema_rejected" as const }, + ]; + expect(toolDisabledEvents(dropped, announced, 1)).toEqual([ + { type: "tool_disabled", stepIndex: 1, tool: "notion.search", reason: "schema_rejected" }, + { type: "tool_disabled", stepIndex: 1, tool: "records.lookup", reason: "schema_rejected" }, + ]); + // The next step sends the same tools and is refused the same way. + expect(toolDisabledEvents(dropped, announced, 2)).toEqual([]); + expect([...announced]).toEqual(["notion.search", "records.lookup"]); + }); + + it("nothing for a completion that dropped nothing", () => { + const announced = new Set(); + expect(toolDisabledEvents(undefined, announced, 0)).toEqual([]); + expect(toolDisabledEvents([], announced, 0)).toEqual([]); + expect(announced.size).toBe(0); + }); +}); diff --git a/src/agent/tool-disabled-notice.ts b/src/agent/tool-disabled-notice.ts new file mode 100644 index 000000000..4ead11362 --- /dev/null +++ b/src/agent/tool-disabled-notice.ts @@ -0,0 +1,52 @@ +import type { DroppedTool } from "../llm/provider/completion-types.js"; + +/** + * ATO-1417. The provider sent a request without one of the session's + * tools because the server refused that tool's JSON schema (ATO-1399: + * LM Studio and llama.cpp reject some MCP schemas outright), so the model + * cannot call it. Before this event the only trace was a log line, and + * the person saw a model that simply never used their tool. Rides the + * `AgentLoopEvent` union so the TUI and the HTTP stream can say it in the + * chat. + * + * Emitted once per tool per turn: every step of the turn sends the same + * tool list, and so is refused and resent the same way. + */ +export interface ToolDisabledEvent { + type: "tool_disabled"; + stepIndex: number; + /** The tool's own name, as the registry knows it. */ + tool: string; + /** `schema_rejected`: the server did not accept its JSON schema. */ + reason: DroppedTool["reason"]; +} + +/** The chat line for a `tool_disabled` event, shared by the TUI and the HTTP frame. */ +export function toolDisabledNotice(tool: string): string { + return `Tool ${tool} is disabled for this turn: the model server did not accept its schema.`; +} + +/** + * The `tool_disabled` events a completion calls for: one per dropped + * tool that this turn has not announced yet. `announced` is the turn's + * own set of tool names and is updated in place. + */ +export function toolDisabledEvents( + droppedTools: readonly DroppedTool[] | undefined, + announced: Set, + stepIndex: number, +): ToolDisabledEvent[] { + if (!droppedTools || droppedTools.length === 0) return []; + const events: ToolDisabledEvent[] = []; + for (const dropped of droppedTools) { + if (!dropped.name || announced.has(dropped.name)) continue; + announced.add(dropped.name); + events.push({ + type: "tool_disabled", + stepIndex, + tool: dropped.name, + reason: dropped.reason, + }); + } + return events; +} diff --git a/src/agent/turn/approval-deadline.test.ts b/src/agent/turn/approval-deadline.test.ts new file mode 100644 index 000000000..5a511dc97 --- /dev/null +++ b/src/agent/turn/approval-deadline.test.ts @@ -0,0 +1,80 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + MAX_TIMER_MS, + APPROVAL_DEADLINE_REPEAT_MS, + APPROVAL_TIME_LIMIT_REASON, + armApprovalDeadline, +} from "./approval-deadline.js"; + +describe("armApprovalDeadline (ATO-1367)", () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it("denies pending prompts once the time is up, and keeps denying until the step ends", () => { + vi.useFakeTimers(); + const deny = vi.fn(); + const dispose = armApprovalDeadline({ msLeft: 30_000, deny }); + vi.advanceTimersByTime(29_999); + expect(deny).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(deny).toHaveBeenCalledTimes(1); + expect(deny).toHaveBeenCalledWith(APPROVAL_TIME_LIMIT_REASON); + vi.advanceTimersByTime(APPROVAL_DEADLINE_REPEAT_MS * 2); + expect(deny).toHaveBeenCalledTimes(3); + dispose(); + vi.advanceTimersByTime(APPROVAL_DEADLINE_REPEAT_MS * 4); + expect(deny).toHaveBeenCalledTimes(3); + }); + + it("never fires for a step that ends before the limit", () => { + vi.useFakeTimers(); + const deny = vi.fn(); + const dispose = armApprovalDeadline({ msLeft: 10_000, deny }); + vi.advanceTimersByTime(5_000); + dispose(); + vi.advanceTimersByTime(60_000); + expect(deny).not.toHaveBeenCalled(); + }); + + it("fires at once when the limit already passed, and survives a throwing gate", () => { + vi.useFakeTimers(); + const deny = vi.fn(() => { + throw new Error("gate down"); + }); + const dispose = armApprovalDeadline({ msLeft: -5, deny }); + vi.advanceTimersByTime(0); + expect(deny).toHaveBeenCalledTimes(1); + dispose(); + }); + + it("does nothing without a finite limit", () => { + vi.useFakeTimers(); + const deny = vi.fn(); + armApprovalDeadline({ msLeft: Number.POSITIVE_INFINITY, deny })(); + vi.advanceTimersByTime(1e9); + expect(deny).not.toHaveBeenCalled(); + }); + + it("does not fire early on a limit longer than setTimeout allows (real timers)", async () => { + const deny = vi.fn(); + const dispose = armApprovalDeadline({ msLeft: 2 ** 31, deny }); + await new Promise((resolve) => setTimeout(resolve, 30)); + expect(deny).not.toHaveBeenCalled(); + dispose(); + }); + + it("splits a very long limit into hops and fires at the real deadline", () => { + vi.useFakeTimers(); + const deny = vi.fn(); + const dispose = armApprovalDeadline({ msLeft: MAX_TIMER_MS + 1_000, deny }); + vi.advanceTimersByTime(MAX_TIMER_MS); + expect(deny).not.toHaveBeenCalled(); + vi.advanceTimersByTime(999); + expect(deny).not.toHaveBeenCalled(); + vi.advanceTimersByTime(1); + expect(deny).toHaveBeenCalledTimes(1); + dispose(); + }); +}); diff --git a/src/agent/turn/approval-deadline.ts b/src/agent/turn/approval-deadline.ts new file mode 100644 index 000000000..b9c845665 --- /dev/null +++ b/src/agent/turn/approval-deadline.ts @@ -0,0 +1,94 @@ +/** + * The task's time limit, applied to a step parked on an approval prompt + * (ATO-1367). + * + * The loop checks its wall-clock ceiling only when a step starts, and a + * tool waiting for approval waits until someone answers. An unattended + * run (the desktop's "Run overnight") with nobody to answer would then + * sit on the prompt past its limit until morning. Once the limit + * passes, every approval this session is waiting on is denied with a + * plain reason, and again every few seconds while the step runs, so a + * second prompt in the same step cannot park it either. The next step + * sees the ceiling and writes the closing summary. + * + * Only for a turn whose caller set the time limit (a task run), so an + * ordinary chat turn is never denied behind a person's back. + */ +export const APPROVAL_TIME_LIMIT_REASON = + "The time limit for this task was reached while this action waited for approval; it was not run."; + +/** How often, once past the limit, a new prompt is denied. */ +export const APPROVAL_DEADLINE_REPEAT_MS = 5_000; + +/** + * The longest delay `setTimeout` honours. Node runs a larger one after + * 1 ms, which would deny every prompt at once under a limit of more + * than ~24.8 days; such a wait is split into hops of at most this. + */ +export const MAX_TIMER_MS = 2_147_483_647; + +export interface ApprovalDeadlineTimers { + setTimeout(fn: () => void, ms: number): unknown; + clearTimeout(handle: unknown): void; + setInterval(fn: () => void, ms: number): unknown; + clearInterval(handle: unknown): void; +} + +const realTimers: ApprovalDeadlineTimers = { + setTimeout: (fn, ms) => { + const handle = setTimeout(fn, ms); + handle.unref?.(); + return handle; + }, + clearTimeout: (handle) => clearTimeout(handle as ReturnType), + setInterval: (fn, ms) => { + const handle = setInterval(fn, ms); + handle.unref?.(); + return handle; + }, + clearInterval: (handle) => clearInterval(handle as ReturnType), +}; + +/** + * Arm the limit for one step. `msLeft` is what the task has left when + * the step starts; `deny` denies the session's pending prompts. Returns + * the disposer to call when the step ends. + */ +export function armApprovalDeadline(input: { + msLeft: number; + deny: (reason: string) => void; + timers?: ApprovalDeadlineTimers; + now?: () => number; +}): () => void { + const timers = input.timers ?? realTimers; + const now = input.now ?? Date.now; + if (!Number.isFinite(input.msLeft)) return () => undefined; + const deadline = now() + Math.max(0, input.msLeft); + let repeat: unknown = null; + let pending: unknown = null; + const fire = (): void => { + try { + input.deny(APPROVAL_TIME_LIMIT_REASON); + } catch { + // A failing gate must not take the step down with it. + } + }; + const arm = (): void => { + const left = deadline - now(); + if (left > MAX_TIMER_MS) { + // Not there yet: wait one full hop and look again. + pending = timers.setTimeout(arm, MAX_TIMER_MS); + return; + } + pending = timers.setTimeout(() => { + pending = null; + fire(); + repeat = timers.setInterval(fire, APPROVAL_DEADLINE_REPEAT_MS); + }, Math.max(0, left)); + }; + arm(); + return () => { + if (pending !== null) timers.clearTimeout(pending); + if (repeat !== null) timers.clearInterval(repeat); + }; +} diff --git a/src/agent/turn/mid-reply-timeout-recovery.test.ts b/src/agent/turn/mid-reply-timeout-recovery.test.ts new file mode 100644 index 000000000..e9b718e95 --- /dev/null +++ b/src/agent/turn/mid-reply-timeout-recovery.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it } from "vitest"; +import { CancelledError, TransportError } from "../../llm/index.js"; +import { OpenAiHttpError } from "../../llm/provider/openai/openai-http.js"; +import { createEmptySessionState } from "../../session/session-state.js"; +import type { AgentLoopEvent } from "../agent-contract.js"; +import { MID_REPLY_TIMEOUT_MIN_ELAPSED_MS, composeMidReplyTimeoutNotice } from "./mid-reply-timeout-recovery.js"; +import { recoverTurnStep, type TurnRecoveryContext } from "./turn-recovery.js"; +import { createTurnLoopState } from "./turn-state.js"; + +const URL = "https://vendor/v1/chat/completions"; +const LONG = MID_REPLY_TIMEOUT_MIN_ELAPSED_MS; + +function context(overrides: Partial = {}): TurnRecoveryContext { + return { + state: createEmptySessionState({ id: "mid-reply", workingDir: "/work" }), + options: { maxSteps: 4, signal: new AbortController().signal }, + stepIndex: 2, + finalizationStep: false, + noticeForThisStep: undefined, + effectiveTransport: "native_tools", + requestDeadline: { dispose() {}, fired: () => false }, + resumesStoppedTask: false, + taskStartedAt: 0, + durationCeilingMs: 10_000_000, + providerWaitCfg: { enabled: true, maxWaitMs: 300_000 }, + recoveryStepAvailable: () => true, + requestElapsedMs: LONG, + ...overrides, + }; +} + +function midReply(status: number, message: string): TransportError { + return new TransportError(`"openrouter" reported an error in the middle of its reply (${status}): ${message}`, status, URL, { + cause: new OpenAiHttpError(`openai provider ${status}: ${message}`, status, URL, false, null, "openrouter", undefined, { streamError: message }), + }); +} +const timedOut = () => midReply(504, "A Timeout Occurred"); + +describe("recovery from a provider time limit that ends a long reply", () => { + it("retries with a notice to work in smaller pieces instead of waiting", () => { + const turn = createTurnLoopState(); + const events: AgentLoopEvent[] = []; + const decision = recoverTurnStep(timedOut(), context({ noticeForThisStep: "earlier notice" }), { onEvent: (event) => events.push(event) }, turn); + expect(decision).toEqual({ kind: "retry_same" }); + expect(turn.pendingNotice).toContain("earlier notice"); + expect(turn.pendingNotice).toContain("write it in parts"); + expect(turn.midReplyTimeoutRetry).toEqual({ stepIndex: 2, attempts: 1 }); + expect(turn.awaitingRecovery).toBe(false); + expect(turn.outageAttempts).toBe(0); + expect(events).toEqual([expect.objectContaining({ type: "reply_time_limit_retried", stepIndex: 2, attempt: 1, budget: 2 })]); + }); + + it("closes an earlier outage wait before retrying", () => { + const turn = createTurnLoopState(); + turn.awaitingRecovery = true; + turn.outageWaitedMs = 2_000; + turn.outageAttempts = 1; + const events: AgentLoopEvent[] = []; + recoverTurnStep(timedOut(), context(), { onEvent: (event) => events.push(event) }, turn); + expect(events.map((event) => event.type)).toEqual(["provider_recovered", "reply_time_limit_retried"]); + expect(events[0]).toMatchObject({ waitedMs: 2_000 }); + expect(turn.awaitingRecovery).toBe(false); + expect(turn.outageWaitedMs).toBe(0); + expect(turn.outageAttempts).toBe(0); + }); + + it("does not stack the notice on the second retry", () => { + const turn = createTurnLoopState(); + recoverTurnStep(timedOut(), context(), {}, turn); + const carried = turn.pendingNotice; + const decision = recoverTurnStep(timedOut(), context({ noticeForThisStep: carried }), {}, turn); + expect(decision).toEqual({ kind: "retry_same" }); + expect(turn.pendingNotice).toBe(carried); + expect(turn.midReplyTimeoutRetry?.attempts).toBe(2); + }); + + it("ends the turn on the third timeout of one step and says why", () => { + const turn = createTurnLoopState(); + recoverTurnStep(timedOut(), context(), {}, turn); + recoverTurnStep(timedOut(), context(), {}, turn); + const decision = recoverTurnStep(timedOut(), context(), {}, turn); + expect(decision).toMatchObject({ kind: "failure", category: "transport", cancelled: false }); + if (decision.kind !== "failure") throw new Error("expected a failure"); + expect(decision.runError.message).toContain('"openrouter"'); + expect(decision.runError.message).toContain("3 times"); + }); + + it("gives a later step its own budget", () => { + const turn = createTurnLoopState(); + recoverTurnStep(timedOut(), context(), {}, turn); + recoverTurnStep(timedOut(), context(), {}, turn); + expect(recoverTurnStep(timedOut(), context({ stepIndex: 3 }), {}, turn)).toEqual({ kind: "retry_same" }); + expect(turn.midReplyTimeoutRetry).toEqual({ stepIndex: 3, attempts: 1 }); + }); + + it("keeps a request exactly at the threshold and leaves a shorter one to the outage wait", () => { + expect(recoverTurnStep(timedOut(), context({ requestElapsedMs: LONG }), {}, createTurnLoopState())).toEqual({ kind: "retry_same" }); + const turn = createTurnLoopState(); + expect(recoverTurnStep(timedOut(), context({ requestElapsedMs: LONG - 1 }), {}, turn).kind).toBe("wait"); + expect(turn.pendingNotice).toBeUndefined(); + expect(turn.midReplyTimeoutRetry).toBeNull(); + }); + + it("leaves an unmeasured request to the outage wait", () => { + expect(recoverTurnStep(timedOut(), context({ requestElapsedMs: undefined }), {}, createTurnLoopState()).kind).toBe("wait"); + }); + + it("leaves other long failures to the outage wait", () => { + expect(recoverTurnStep(midReply(502, "Network connection lost"), context(), {}, createTurnLoopState()).kind).toBe("wait"); + const onOpen = new TransportError("gateway timeout", 504, URL); + expect(recoverTurnStep(onOpen, context(), {}, createTurnLoopState()).kind).toBe("wait"); + }); + + it("keeps the failure when provider waiting is off", () => { + const turn = createTurnLoopState(); + const decision = recoverTurnStep(timedOut(), context({ providerWaitCfg: { enabled: false, maxWaitMs: 300_000 } }), {}, turn); + expect(decision.kind).toBe("failure"); + expect(turn.midReplyTimeoutRetry).toBeNull(); + }); + + it("does not spend a retry on a stopped turn or the summary step", () => { + const controller = new AbortController(); + controller.abort(); + const stopped = createTurnLoopState(); + expect(recoverTurnStep(timedOut(), context({ options: { maxSteps: 4, signal: controller.signal } }), {}, stopped)).toMatchObject({ kind: "failure", cancelled: true }); + expect(stopped.midReplyTimeoutRetry).toBeNull(); + expect(recoverTurnStep(new CancelledError(), context(), {}, createTurnLoopState())).toMatchObject({ cancelled: true }); + const summary = createTurnLoopState(); + expect(recoverTurnStep(timedOut(), context({ finalizationStep: true }), {}, summary).kind).toBe("stop"); + expect(summary.midReplyTimeoutRetry).toBeNull(); + }); + + it("composes onto an empty notice", () => { + expect(composeMidReplyTimeoutNotice("")).toBe(composeMidReplyTimeoutNotice(undefined)); + }); +}); diff --git a/src/agent/turn/mid-reply-timeout-recovery.ts b/src/agent/turn/mid-reply-timeout-recovery.ts new file mode 100644 index 000000000..957ed09ff --- /dev/null +++ b/src/agent/turn/mid-reply-timeout-recovery.ts @@ -0,0 +1,67 @@ +import { TransportError } from "../../llm/index.js"; +import { readMidReplyTimeout } from "../../llm/reliability/mid-reply-timeout.js"; +import { WRITE_IN_PARTS } from "./truncation-recovery.js"; + +/** + * How the agent loop recovers when a provider's time limit ends a long + * reply (ATO-1398). + * + * Replaying the same request is pointless: a reply too long to finish + * inside the limit is cut at the same place again, and each attempt runs + * for minutes that `providerWait.maxWaitMs` does not count. So the retry + * changes the request: a notice asking for a much shorter reply. After + * the budget the turn ends, saying why. + * + * Only a request that ran for a while qualifies. A 504 reported seconds + * into a stream is an ordinary outage and keeps the outage wait. + */ + +/** A request that ran at least this long before the time limit ended it. */ +export const MID_REPLY_TIMEOUT_MIN_ELAPSED_MS = 60_000; + +/** Retries one step may take; the next timeout on that step ends the turn. */ +export const MID_REPLY_TIMEOUT_STEP_BUDGET = 2; + +const NOTICE = + "Your previous reply to this step ran too long: the provider stopped it on a time limit before it finished, and none of it was kept. " + + `Think less: keep your reasoning brief and make this reply much shorter. ${WRITE_IN_PARTS}`; + +export type MidReplyTimeoutPlan = + | { kind: "retry"; attempts: number } + | { kind: "give_up"; error: TransportError }; + +export function planMidReplyTimeoutRetry(input: { + error: unknown; + /** How long the failed request ran; absent means it was not measured. */ + requestElapsedMs: number | undefined; + /** Retries this step index already took. */ + stepAttempts: number; +}): MidReplyTimeoutPlan | null { + if ( + input.requestElapsedMs === undefined || + input.requestElapsedMs < MID_REPLY_TIMEOUT_MIN_ELAPSED_MS + ) { + return null; + } + const timeout = readMidReplyTimeout(input.error); + if (timeout === null) return null; + if (input.stepAttempts < MID_REPLY_TIMEOUT_STEP_BUDGET) { + return { kind: "retry", attempts: input.stepAttempts + 1 }; + } + return { + kind: "give_up", + error: new TransportError( + `"${timeout.provider}" stopped this step's reply on a time limit ${input.stepAttempts + 1} times, so the step was ended. ` + + "Try a faster model, or ask for the work in smaller steps.", + timeout.status, + timeout.url, + { cause: input.error }, + ), + }; +} + +/** Fold the notice into the step's one-shot notice, once per retried step. */ +export function composeMidReplyTimeoutNotice(existing: string | undefined): string { + if (existing === undefined || existing.length === 0) return NOTICE; + return existing.includes(NOTICE) ? existing : `${existing}\n\n${NOTICE}`; +} diff --git a/src/agent/turn/truncation-recovery.ts b/src/agent/turn/truncation-recovery.ts index 8bab99cd5..d286ab474 100644 --- a/src/agent/turn/truncation-recovery.ts +++ b/src/agent/turn/truncation-recovery.ts @@ -144,7 +144,7 @@ export function truncationRetryMaxTokens( } /** Asked of every cap-bound retry: the way out of a long reply. */ -const WRITE_IN_PARTS = +export const WRITE_IN_PARTS = 'If you are writing a large file, write it in parts: create it with the first part, then add the rest with os.fs.write mode "append" in the next steps.'; /** diff --git a/src/agent/turn/turn-recovery-local-engine.test.ts b/src/agent/turn/turn-recovery-local-engine.test.ts index 60110fca9..e6f05128d 100644 --- a/src/agent/turn/turn-recovery-local-engine.test.ts +++ b/src/agent/turn/turn-recovery-local-engine.test.ts @@ -1,9 +1,22 @@ import { describe, expect, it, vi } from "vitest"; -import { LOCAL_ENGINE_DOWN_CODE, LOCAL_ENGINE_DOWN_MESSAGE, LocalEngineDownError, TransportError } from "../../llm/index.js"; +import { + ATOMIC_CHAT_NOT_RUNNING_CODE, + ATOMIC_CHAT_NOT_RUNNING_MESSAGE, + AtomicChatNotRunningError, + LOCAL_ENGINE_DOWN_CODE, + LOCAL_ENGINE_DOWN_MESSAGE, + LOCAL_PORT_TAKEN_CODE, + LlamaServerError, + LocalEngineDownError, + LocalPortTakenError, + TransportError, +} from "../../llm/index.js"; +import { keyRefusedError } from "../../llm/llama-server-client.js"; +import { classifyFailureCause } from "../../llm/reliability/index.js"; import { attachFailingLink } from "../../llm/fallback/failed-attempts.js"; import { createEmptySessionState } from "../../session/session-state.js"; import type { AgentLoopEvent, LocalEngineState } from "../agent-contract.js"; -import { LOCAL_ENGINE_RESTART_GRACE_MS, recoverTurnStep, type TurnRecoveryContext } from "./turn-recovery.js"; +import { ATOMIC_CHAT_START_GRACE_MS, LOCAL_ENGINE_RESTART_GRACE_MS, recoverTurnStep, type TurnRecoveryContext } from "./turn-recovery.js"; import { createTurnLoopState } from "./turn-state.js"; /** @@ -150,3 +163,174 @@ describe("turn recovery: managed local engine down", () => { expect(decision).toEqual({ kind: "wait", nextRetryMs: 2_000 }); }); }); + +/** + * ATO-1278: the `atomic-chat` link is the Atomic Chat app's server. A + * connection it refuses outright, past a short start grace, ends the turn + * with `AtomicChatNotRunningError` instead of the five-minute wait. Other + * failures there, and every other provider, keep the wait. + */ +describe("turn recovery: Atomic Chat app not running", () => { + const ATOMIC_CHAT_URL = "http://127.0.0.1:1337/v1/chat/completions"; + + function refusedAtomicChat(code = "ECONNREFUSED"): TransportError { + const err = new TransportError("fetch failed", null, ATOMIC_CHAT_URL, { + cause: Object.assign(new Error(`connect ${code} 127.0.0.1:1337`), { code }), + }); + attachFailingLink(err, "atomic-chat"); + return err; + } + + /** The runtime's reading in miniature: only the atomic-chat link is the app. */ + function app() { + return vi.fn((providerId: string | undefined): LocalEngineState => + providerId === "atomic-chat" ? "atomic_chat_app" : "unknown"); + } + + it("ECONNREFUSED ends the turn after two quick retries, with its own reason code", () => { + const turn = createTurnLoopState(); + const events: AgentLoopEvent[] = []; + const deps = { localEngineState: app(), onEvent: (e: AgentLoopEvent) => events.push(e) }; + const waits: number[] = []; + const err = refusedAtomicChat(); + let ended: ReturnType | undefined; + for (;;) { + const decision = recoverTurnStep(err, context(), deps, turn); + if (decision.kind !== "wait") { + ended = decision; + break; + } + waits.push(decision.nextRetryMs); + turn.outageWaitedMs += decision.nextRetryMs; + } + expect(waits).toEqual([2_000, 2_000]); + expect(turn.outageWaitedMs).toBe(ATOMIC_CHAT_START_GRACE_MS); + expect(ended?.kind).toBe("failure"); + if (ended?.kind !== "failure") return; + expect(ended.category).toBe("transport"); + expect(ended.runError).toBeInstanceOf(AtomicChatNotRunningError); + expect(ended.runError.message).toBe(ATOMIC_CHAT_NOT_RUNNING_MESSAGE); + expect((ended.runError as AtomicChatNotRunningError).reasonCode).toBe(ATOMIC_CHAT_NOT_RUNNING_CODE); + expect((ended.runError as { cause?: unknown }).cause).toBe(err); + expect(classifyFailureCause(ended.runError, "transport").cause).toBe("atomic_chat_not_running"); + expect(events.at(-1)).toMatchObject({ type: "loop_failed" }); + }); + + it("during the grace the turn waits, so an app that is starting is picked up", () => { + const turn = createTurnLoopState(); + turn.outageWaitedMs = ATOMIC_CHAT_START_GRACE_MS - 1; + const decision = recoverTurnStep(refusedAtomicChat(), context(), { localEngineState: app() }, turn); + expect(decision).toEqual({ kind: "wait", nextRetryMs: 2_000 }); + }); + + it("with waiting disabled it ends at once with the same reason", () => { + const decision = recoverTurnStep(refusedAtomicChat(), context({ providerWaitCfg: { enabled: false, maxWaitMs: 300_000 } }), { localEngineState: app() }, createTurnLoopState()); + expect(decision.kind).toBe("failure"); + if (decision.kind === "failure") expect(decision.runError).toBeInstanceOf(AtomicChatNotRunningError); + }); + + it("an HTTP 500 from Atomic Chat is an app that answered: the wait is unchanged", () => { + const turn = createTurnLoopState(); + turn.outageWaitedMs = 60_000; + const err = new TransportError("atomic chat 500", 500, ATOMIC_CHAT_URL); + attachFailingLink(err, "atomic-chat"); + const probe = app(); + const decision = recoverTurnStep(err, context(), { localEngineState: probe }, turn); + expect(decision).toEqual({ kind: "wait", nextRetryMs: 2_000 }); + expect(probe).not.toHaveBeenCalled(); + }); + + it("a reset or timeout on the Atomic Chat link is not a refusal: the wait is unchanged", () => { + const turn = createTurnLoopState(); + turn.outageWaitedMs = 60_000; + turn.outageAttempts = 3; + const decision = recoverTurnStep(refusedAtomicChat("ECONNRESET"), context(), { localEngineState: app() }, turn); + expect(decision).toEqual({ kind: "wait", nextRetryMs: 16_000 }); + }); + + it("other providers refused the same way keep the full wait", () => { + const turn = createTurnLoopState(); + turn.outageWaitedMs = 60_000; + turn.outageAttempts = 3; + const probe = app(); + const decision = recoverTurnStep(refused("ollama", "http://127.0.0.1:11434/v1/chat/completions"), context(), { localEngineState: probe }, turn); + expect(decision).toEqual({ kind: "wait", nextRetryMs: 16_000 }); + expect(probe).toHaveBeenCalledWith("ollama"); + }); + + it("the Atomic Chat link as a fallback tail (runtime answers unknown) keeps the wait", () => { + const turn = createTurnLoopState(); + turn.outageWaitedMs = 60_000; + const decision = recoverTurnStep(refusedAtomicChat(), context(), { localEngineState: () => "unknown" }, turn); + expect(decision).toEqual({ kind: "wait", nextRetryMs: 2_000 }); + }); +}); + +/** + * ATO-1277: the managed engine's address answered 401 while our engine's + * process is not there. The server that refused the key is another + * program on the port, so the turn ends with `local_port_taken` instead + * of "the local model server refused the agent's key". + */ +describe("turn recovery: a 401 on the managed port with our engine gone", () => { + const URL = "http://127.0.0.1:9931/completion"; + + /** A standing 401 the way the llama client throws it, wrapped like `toLlmFailure` does. */ + function unauthorized(link: string, url = URL): TransportError { + const refusal = keyRefusedError(new LlamaServerError("llama-server returned http 401", 401, url)); + const err = new TransportError(refusal.message, 401, url, { cause: refusal }); + attachFailingLink(err, link); + return err; + } + + it("no pid (or a dead one) ends with LocalPortTakenError, keeping the 401, the URL and the refusal", () => { + const err = unauthorized("local-llama"); + const events: AgentLoopEvent[] = []; + const decision = recoverTurnStep(err, context(), { localEngineState: engine("dead"), onEvent: (e) => events.push(e) }, createTurnLoopState()); + expect(decision.kind).toBe("failure"); + if (decision.kind !== "failure") return; + expect(decision.category).toBe("transport"); + expect(decision.runError).toBeInstanceOf(LocalPortTakenError); + const taken = decision.runError as LocalPortTakenError; + expect(taken.reasonCode).toBe(LOCAL_PORT_TAKEN_CODE); + expect(taken.status).toBe(401); + expect(taken.url).toBe(URL); + expect((taken as { cause?: unknown }).cause).toBe(err); + expect(taken.message).toContain("Another program is using the local model's port"); + expect(taken.message).toContain("localModels.managed.port"); + expect(taken.message).not.toContain("refused the agent's key"); + expect(events.at(-1)).toMatchObject({ type: "loop_failed", failure: { cause: "local_port_taken" } }); + }); + + it("with our engine alive the refused-key words stay", () => { + const err = unauthorized("local-llama"); + const decision = recoverTurnStep(err, context(), { localEngineState: engine("alive") }, createTurnLoopState()); + expect(decision.kind).toBe("failure"); + if (decision.kind !== "failure") return; + expect(decision.runError).toBe(err); + expect(decision.runError.message).toContain("refused the agent's key"); + }); + + it("an unknown engine state (start or install under way, external mode) keeps the old error", () => { + const err = unauthorized("local-llama"); + const decision = recoverTurnStep(err, context(), { localEngineState: engine("unknown") }, createTurnLoopState()); + expect(decision.kind === "failure" && decision.runError).toBe(err); + }); + + it("a llama-server entry under another id is not the managed engine, whatever the pid says", () => { + const err = unauthorized("my-llama"); + const probe = engine("dead"); + const decision = recoverTurnStep(err, context(), { localEngineState: probe }, createTurnLoopState()); + expect(decision.kind === "failure" && decision.runError).toBe(err); + expect(probe).toHaveBeenCalledWith("my-llama"); + }); + + it("a cloud provider's 401 is not read as a taken port", () => { + const err = new TransportError("openai provider 401: bad key", 401, "https://openrouter.ai/api/v1/chat/completions"); + attachFailingLink(err, "local-llama"); + const probe = engine("dead"); + const decision = recoverTurnStep(err, context(), { localEngineState: probe }, createTurnLoopState()); + expect(decision.kind === "failure" && decision.runError).toBe(err); + expect(probe).not.toHaveBeenCalled(); + }); +}); diff --git a/src/agent/turn/turn-recovery-stream-cut.test.ts b/src/agent/turn/turn-recovery-stream-cut.test.ts new file mode 100644 index 000000000..b0acae63c --- /dev/null +++ b/src/agent/turn/turn-recovery-stream-cut.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it, vi } from "vitest"; +import { LlamaServerError, TransportError } from "../../llm/index.js"; +import { attachFailingLink } from "../../llm/fallback/failed-attempts.js"; +import { createEmptySessionState } from "../../session/session-state.js"; +import type { AgentLoopEvent, LocalEngineState } from "../agent-contract.js"; +import { recoverTurnStep, type TurnRecoveryContext } from "./turn-recovery.js"; +import { createTurnLoopState } from "./turn-state.js"; + +/** + * ATO-1279: the managed local model is running but cuts a reply off + * mid-stream (a broken connection, or a 5xx error event inside the + * stream). The step is retried once, at once; a second cut on the same + * step ends the turn instead of waiting. + */ +function context(overrides: Partial = {}): TurnRecoveryContext { + return { + state: createEmptySessionState({ id: "cut", workingDir: "/work" }), + options: { maxSteps: 4, signal: new AbortController().signal }, + stepIndex: 0, + finalizationStep: false, + noticeForThisStep: undefined, + effectiveTransport: "grammar", + requestDeadline: { dispose() {}, fired: () => false }, + resumesStoppedTask: false, + taskStartedAt: 0, + durationCeilingMs: 1_000_000, + providerWaitCfg: { enabled: true, maxWaitMs: 300_000 }, + recoveryStepAvailable: () => true, + ...overrides, + }; +} + +const URL = "http://127.0.0.1:9931/completion"; + +/** What the step executor throws for a cut stream: the client's error, wrapped. */ +function cut( + opts: { status?: number | null; midStream?: boolean; timedOut?: boolean; link?: string } = {}, +): TransportError { + const status = opts.status === undefined ? null : opts.status; + const inner = new LlamaServerError( + status === null ? "terminated" : `llama-server reported an error mid-stream (http ${status})`, + status, + URL, + opts.timedOut ?? false, + status === null ? "ECONNRESET" : undefined, + { midStream: opts.midStream ?? true }, + ); + const err = new TransportError(inner.message, status, URL, { cause: inner }); + attachFailingLink(err, opts.link ?? "local-llama"); + return err; +} + +function engine(state: LocalEngineState) { + return vi.fn((providerId: string | undefined): LocalEngineState => + providerId === "local-llama" ? state : "unknown"); +} + +describe("turn recovery: managed local model cut the reply mid-stream", () => { + it("retries the step once, then fails on the second cut of the same step without waiting", () => { + const turn = createTurnLoopState(); + const events: AgentLoopEvent[] = []; + const deps = { localEngineState: engine("alive"), onEvent: (e: AgentLoopEvent) => events.push(e) }; + + expect(recoverTurnStep(cut(), context(), deps, turn)).toEqual({ kind: "retry_same" }); + expect(turn.localStreamCutRetry).toEqual({ stepIndex: 0 }); + expect(events).toEqual([]); + + const second = recoverTurnStep(cut(), context(), deps, turn); + expect(second.kind).toBe("failure"); + if (second.kind === "failure") { + expect(second.category).toBe("transport"); + expect(second.cancelled).toBe(false); + } + expect(events.map((e) => e.type)).toEqual(["loop_failed"]); + expect(turn.outageAttempts).toBe(0); + }); + + it("retries a 5xx error event inside the stream the same way, and a later step gets its own retry", () => { + const turn = createTurnLoopState(); + const deps = { localEngineState: engine("alive") }; + expect(recoverTurnStep(cut({ status: 500 }), context(), deps, turn).kind).toBe("retry_same"); + expect(recoverTurnStep(cut({ status: 500 }), context({ stepIndex: 1 }), deps, turn).kind).toBe("retry_same"); + expect(turn.localStreamCutRetry).toEqual({ stepIndex: 1 }); + }); + + it("does not retry when the turn was cancelled", () => { + const controller = new AbortController(); + controller.abort(); + const turn = createTurnLoopState(); + const decision = recoverTurnStep( + cut(), + context({ options: { maxSteps: 4, signal: controller.signal } }), + { localEngineState: engine("alive") }, + turn, + ); + expect(decision.kind).toBe("failure"); + if (decision.kind === "failure") expect(decision.cancelled).toBe(true); + expect(turn.localStreamCutRetry).toBeNull(); + }); + + it("leaves the ordinary paths alone: a dead engine, a link that is not ours, our own deadline, a plain 500 on the open", () => { + for (const [err, state] of [ + [cut(), "dead"], + [cut({ link: "openrouter" }), "alive"], + [cut({ timedOut: true }), "alive"], + [cut({ status: 500, midStream: false }), "alive"], + [cut({ status: 400 }), "alive"], + ] as const) { + const turn = createTurnLoopState(); + const decision = recoverTurnStep(err, context(), { localEngineState: engine(state) }, turn); + expect(turn.localStreamCutRetry).toBeNull(); + expect(decision.kind).not.toBe("retry_same"); + } + }); + + it("is not taken when the out-of-memory check claims the failure first", () => { + const turn = createTurnLoopState(); + const decision = recoverTurnStep( + cut({ status: 500 }), + context(), + { + localEngineState: engine("alive"), + localOutOfMemory: { + detect: () => ({ contextWindow: 65_536 }), + canRestart: () => true, + ask: async () => "restart_smaller", + restart: async () => true, + }, + }, + turn, + ); + expect(decision.kind).toBe("local_out_of_memory"); + expect(turn.localStreamCutRetry).toBeNull(); + }); +}); diff --git a/src/agent/turn/turn-recovery.test.ts b/src/agent/turn/turn-recovery.test.ts index 5f82d2e39..0acfbe0d1 100644 --- a/src/agent/turn/turn-recovery.test.ts +++ b/src/agent/turn/turn-recovery.test.ts @@ -46,8 +46,9 @@ describe("turn recovery decisions and shared state", () => { const second = createTurnLoopState(); first.truncationRetry = { stepIndex: 1, attempts: 1, original: new Error("cut") }; first.reason = "cancelled"; - expect(Object.keys(second)).toHaveLength(21); + expect(Object.keys(second).length).toBeGreaterThanOrEqual(22); expect(second.truncationRetry).toBeNull(); + expect(second.localStreamCutRetry).toBeNull(); expect(second.reason).toBe("max_steps"); expect(second.lastBoundaryIndex).toBe(-1); }); diff --git a/src/agent/turn/turn-recovery.ts b/src/agent/turn/turn-recovery.ts index 78f8fdf67..a66da2aaf 100644 --- a/src/agent/turn/turn-recovery.ts +++ b/src/agent/turn/turn-recovery.ts @@ -2,7 +2,7 @@ import { isSubscriptionCliSetupError } from "../../llm/provider/subscription-cli import type { AgentLoopDependencies, RunTurnOptions } from "../agent-contract.js"; import type { SessionState } from "../../session/session-state.js"; import type { ToolCallTransport } from "../../llm/provider/completion-types.js"; -import { CancelledError, LlamaServerError, LlmFailure, LocalEngineDownError, TransportError, classifyFailure, isRequestSizeRejection } from "../../llm/index.js"; +import { AtomicChatNotRunningError, CancelledError, LlamaServerError, LlmFailure, LocalEngineDownError, LocalPortTakenError, TransportError, classifyFailure, isRequestSizeRejection } from "../../llm/index.js"; import type { LlmFailureCategory, TruncationDetail } from "../../llm/index.js"; import { readFailingLink } from "../../llm/fallback/failed-attempts.js"; import { describeFailedLinks } from "../../llm/fallback/failed-links.js"; @@ -11,6 +11,7 @@ import { readProviderErrorVerdict } from "../../llm/reliability/provider-error-v import { classifyFailureCause } from "../../llm/reliability/classify-failure-cause.js"; import { classifyProviderWaitCause } from "../../llm/reliability/provider-wait-cause.js"; import { composeSizeRejectionNotice, planSizeRejectionRepack } from "./size-rejection-recovery.js"; +import { MID_REPLY_TIMEOUT_STEP_BUDGET, composeMidReplyTimeoutNotice, planMidReplyTimeoutRetry } from "./mid-reply-timeout-recovery.js"; import { composeTruncationNotice, planTruncationRetry, truncationRetryMaxTokens, type TruncationRetry, type TruncationRetryPlan } from "./truncation-recovery.js"; import { PARSE_RECOVERY_BUDGET, composeParseFailureNotice, isRecoverableParseFailure } from "./parse-failure-recovery.js"; import { EMPTY_COMPLETION_RECOVERY_BUDGET, composeEmptyCompletionNotice, isRecoverableEmptyCompletion, repeatedEmptyCompletionError } from "./empty-completion-recovery.js"; @@ -39,6 +40,8 @@ export interface TurnRecoveryContext { recoveryStepAvailable: (stepIndex: number) => boolean; /** The model mode serving the step; absent (legacy wiring) reads as `local`. */ modelMode?: "local" | "cloud"; + /** How long the failed step's request ran. */ + requestElapsedMs?: number; /** * The plain error the turn ends with when it fails on a context-size * refusal after the loop's overflow compaction (ATO-1028). Replaces the @@ -506,6 +509,60 @@ export function recoverTurnStep( return { kind: "local_out_of_memory", contextWindow: outOfMemory.contextWindow, cause: runError }; } } + // A provider's time limit ended a long reply. Waiting and replaying + // the same request hits the same limit, so the retry asks for less. + // Bounded per step; a request that ran only briefly falls through to + // the outage wait below. + if (category === "transport" && !cancelled && !creditRefused && providerWaitCfg.enabled) { + const stepAttempts = + turn.midReplyTimeoutRetry?.stepIndex === i ? turn.midReplyTimeoutRetry.attempts : 0; + const plan = planMidReplyTimeoutRetry({ + error: err, + requestElapsedMs: context.requestElapsedMs, + stepAttempts, + }); + if (plan && turn.awaitingRecovery) { + // An earlier outage wait on this step is over: the provider has + // just been answering for a minute or more. + deps.onEvent?.({ type: "provider_recovered", waitedMs: turn.outageWaitedMs }); + turn.awaitingRecovery = false; + turn.outageWaitedMs = 0; + turn.outageAttempts = 0; + } + if (plan?.kind === "give_up") { + return reportTurnFailure( + plan.error, + { category, cancelled: false, creditRefused: false }, + { state, stepIndex: i }, + deps, + turn, + ); + } + if (plan?.kind === "retry") { + turn.midReplyTimeoutRetry = { stepIndex: i, attempts: plan.attempts }; + turn.pendingNotice = composeMidReplyTimeoutNotice(noticeForThisStep); + const waitedOn = readFailingLink(err); + // Its own event, not `provider_waiting`: the provider is answering, + // so nothing here may read as an outage. + deps.onEvent?.({ + type: "reply_time_limit_retried", + stepIndex: i, + attempt: plan.attempts, + budget: MID_REPLY_TIMEOUT_STEP_BUDGET, + reason: runError.message, + }); + deps.logger?.warn("provider time limit ended a long reply; retrying the step with a notice", { + sessionId: state.id, + stepIndex: i, + attempt: plan.attempts, + requestElapsedMs: context.requestElapsedMs, + error: runError.message, + ...(waitedOn !== undefined ? { providerId: waitedOn } : {}), + }); + runError = null; + return { kind: "retry_same" }; + } + } // The provider is not answering. Park the turn instead of // killing it: nothing of this step has been committed (a // completion failure throws before any tool is dispatched — @@ -522,6 +579,41 @@ export function recoverTurnStep( // buys a second helping of the same silence — see // `isOwnLlamaDeadlineExpiry`. const retryHint = verdict?.kind === "retry_after" ? verdict : null; + // The managed local model is running but cut this step's reply off + // mid-stream (ATO-1279): the connection broke while the body was read, + // or llama-server sent an error event (a 5xx) inside the stream. Not an + // outage to wait out, since the model is up; retry the step once. Same + // replay argument as the outage wait below: a completion failure throws + // before any tool is dispatched, so nothing of the step was committed. + // What the cut attempt had streamed was only a preview, and the + // surfaces already showed the failure (`step_error`). After the out-of- + // memory check above, which handles a cut that is a full memory, and + // never for a cancelled turn or one of our own deadlines. A second cut + // on the same step ends the turn instead of waiting. + let localStreamCutRepeated = false; + if ( + category === "transport" && + !cancelled && + !creditRefused && + deps.localEngineState !== undefined && + isServerStreamCut(err) + ) { + const failingLink = readFailingLink(err); + if (deps.localEngineState(failingLink) === "alive") { + if (turn.localStreamCutRetry?.stepIndex !== i) { + turn.localStreamCutRetry = { stepIndex: i }; + deps.logger?.warn("local model cut the reply mid-stream; retrying the step once", { + sessionId: state.id, + stepIndex: i, + error: runError.message, + ...(failingLink !== undefined ? { providerId: failingLink } : {}), + }); + runError = null; + return { kind: "retry_same" }; + } + localStreamCutRepeated = true; + } + } // Before parking: is the thing we would wait for coming back at all? // A refused connection to the managed local model whose process is // certainly gone (pid dead, or none recorded, and no start, load or @@ -539,6 +631,12 @@ export function recoverTurnStep( // An engine that is alive or whose state is unknown keeps the wait // below, unchanged; so does every other link, an external server // included, because the runtime answers `unknown` for them. + // + // The Atomic Chat app's server (`atomic_chat_app`, ATO-1278) is judged + // the same way with less to read: nothing here starts that app, so a + // connection it refuses outright (`ECONNREFUSED`, not a reset, a timeout + // or an HTTP error) for a short start grace means it is not open. The + // grace covers an app that is still bringing its server up. let localEngineGone = false; let localEngineDown = false; if ( @@ -551,12 +649,29 @@ export function recoverTurnStep( isWaitableOutage(err) ) { const failingLink = readFailingLink(err); - localEngineGone = deps.localEngineState(failingLink) === "dead"; + const engineState = deps.localEngineState(failingLink); + const atomicChatRefused = + engineState === "atomic_chat_app" && readErrnoCode(err) === "ECONNREFUSED"; + localEngineGone = engineState === "dead" || atomicChatRefused; const graceSpent = !providerWaitCfg.enabled || turn.outageWaitedMs >= - Math.min(LOCAL_ENGINE_RESTART_GRACE_MS, providerWaitCfg.maxWaitMs); - if (localEngineGone && graceSpent) { + Math.min( + atomicChatRefused ? ATOMIC_CHAT_START_GRACE_MS : LOCAL_ENGINE_RESTART_GRACE_MS, + providerWaitCfg.maxWaitMs, + ); + if (atomicChatRefused && graceSpent) { + localEngineDown = true; + deps.logger?.warn("Atomic Chat is not running; ending the turn instead of waiting", { + sessionId: state.id, + stepIndex: i, + waitedMs: turn.outageWaitedMs, + error: runError.message, + causeCode: "ECONNREFUSED", + ...(failingLink !== undefined ? { providerId: failingLink } : {}), + }); + runError = new AtomicChatNotRunningError(failingLink ?? "atomic-chat", err); + } else if (localEngineGone && graceSpent) { localEngineDown = true; const causeCode = readErrnoCode(err); deps.logger?.warn("local model is not running; ending the turn instead of waiting", { @@ -575,6 +690,7 @@ export function recoverTurnStep( !cancelled && !creditRefused && !localEngineDown && + !localStreamCutRepeated && providerWaitCfg.enabled && (isWaitableOutage(err) || retryHint !== null) && turn.outageWaitedMs < providerWaitCfg.maxWaitMs @@ -629,6 +745,28 @@ export function recoverTurnStep( }); return { kind: "wait", nextRetryMs }; } + // The managed local model's address refused the key while our engine's + // process is certainly not there (no pid recorded, or a dead one, and + // nothing starting): the server that answered is another program on the + // port, a second Core or Atomic Chat's own (ATO-1277). "Refused the + // agent's key, restart the model" sent people to restart a model that + // was not running. Judged by the same engine reading as the refusal + // above, so only the built-in managed link qualifies; a live or unknown + // engine keeps the key wording. + if (!cancelled && deps.localEngineState !== undefined) { + const refusal = findLlamaUnauthorized(err); + if (refusal !== null) { + const failingLink = readFailingLink(err); + if (deps.localEngineState(failingLink) === "dead") { + deps.logger?.warn("managed model port answered 401 with our engine not running; another program holds it", { + sessionId: state.id, + stepIndex: i, + ...(failingLink !== undefined ? { providerId: failingLink } : {}), + }); + runError = new LocalPortTakenError(failingLink ?? "local-llama", refusal.url, err); + } + } + } // A raised cap the provider refused — a 400 naming `max_tokens` // or the context length — is not a new failure. The turn fails // with the truncation that started it, which names the knob. @@ -742,6 +880,16 @@ function isWaitableOutage(err: unknown): boolean { return err.status >= 500 || err.status === 408 || err.status === 429; } +/** A llama-server 401 on the error or one of its causes. */ +function findLlamaUnauthorized(err: unknown): LlamaServerError | null { + let current: unknown = err; + for (let depth = 0; depth < 6 && current instanceof Error; depth += 1) { + if (current instanceof LlamaServerError) return current.status === 401 ? current : null; + current = (current as { cause?: unknown }).cause; + } + return null; +} + /** * A server answered, and with a server error. What an out-of-memory * llama-server answers every request with; a refused connection or a @@ -752,6 +900,27 @@ function isServerErrorAnswer(err: unknown): boolean { return err.status !== null && err.status >= 500; } +/** + * The server ended a stream it had answered (`LlamaServerError.midStream`): + * a broken connection, or a 5xx error event inside the stream. A 4xx + * event (a full context, say) keeps its own handling, and our own + * deadlines never carry the flag. + */ +function isServerStreamCut(err: unknown): boolean { + let current: unknown = err; + for (let depth = 0; depth < 8 && current instanceof Error; depth += 1) { + if (current instanceof LlamaServerError) { + return ( + current.midStream && + !current.timedOut && + (current.status === null || current.status >= 500) + ); + } + current = (current as { cause?: unknown }).cause; + } + return false; +} + /** * No HTTP response came back at all (a refused or reset connection, an * untyped socket failure). Only that is evidence the model process may @@ -862,6 +1031,12 @@ function isOwnLlamaDeadlineExpiry(err: unknown): boolean { export const LOCAL_ENGINE_RESTART_GRACE_MS = 8_000; /** The retry interval while that process is gone. */ const LOCAL_ENGINE_GONE_RETRY_MS = 2_000; +/** + * How long a turn keeps retrying the Atomic Chat app's server while it + * refuses connections before it ends with `AtomicChatNotRunningError` + * (ATO-1278): two quick retries, for an app that is still starting. + */ +export const ATOMIC_CHAT_START_GRACE_MS = 4_000; /** First backoff after the provider stops answering. */ const PROVIDER_WAIT_BASE_MS = 2_000; diff --git a/src/agent/turn/turn-spend.test.ts b/src/agent/turn/turn-spend.test.ts index 94c948244..5aabe7441 100644 --- a/src/agent/turn/turn-spend.test.ts +++ b/src/agent/turn/turn-spend.test.ts @@ -5,6 +5,7 @@ import { costCeilingHeadline, costCeilingNotice, costCeilingReached, + recordCompactionSpend, recordStepSpend, resolveCostCeilingUsd, } from "./turn-spend.js"; @@ -69,4 +70,15 @@ describe("turn spend ceiling helpers", () => { expect(stopped).toMatch(/^\(stopped: spent \$5\.31 of \$5\.00 budget, after 7 steps over ~2 min\.\)/); expect(stopped).toContain("agent.task.maxCostUsd"); }); + + it("adds a compaction's summary cost to the turn spend (ATO-1322)", () => { + const turn = { spendUsd: 0.5, spendPriced: false }; + recordCompactionSpend(turn, 0.25); + expect(turn).toEqual({ spendUsd: 0.75, spendPriced: true }); + recordCompactionSpend(turn, 0); + recordCompactionSpend(turn, Number.NaN); + recordCompactionSpend(turn, -1); + expect(turn.spendUsd).toBe(0.75); + expect(costCeilingReached(turn.spendUsd, 0.75)).toBe(true); + }); }); diff --git a/src/agent/turn/turn-spend.ts b/src/agent/turn/turn-spend.ts index efe130a83..b7a161083 100644 --- a/src/agent/turn/turn-spend.ts +++ b/src/agent/turn/turn-spend.ts @@ -55,6 +55,20 @@ export function recordStepSpend( turn.spendPriced = true; } +/** + * Add what a context compaction's summary calls cost (ATO-1322). They + * are side calls, not step completions, so `recordStepSpend` never sees + * them; without this a long run that compacts spends past its budget. + */ +export function recordCompactionSpend( + turn: Pick, + usd: number, +): void { + if (!Number.isFinite(usd) || usd <= 0) return; + turn.spendUsd += usd; + turn.spendPriced = true; +} + /** Whether the spend has reached a ceiling that is set. */ export function costCeilingReached(spendUsd: number, ceilingUsd: number): boolean { return ceilingUsd > 0 && spendUsd >= ceilingUsd; diff --git a/src/agent/turn/turn-state.ts b/src/agent/turn/turn-state.ts index 33ba816c2..01525bc28 100644 --- a/src/agent/turn/turn-state.ts +++ b/src/agent/turn/turn-state.ts @@ -39,6 +39,12 @@ export interface TurnLoopState { outageAttempts: number; /** Retried a step after an outage and have not yet seen it succeed. */ awaitingRecovery: boolean; + /** + * The step index already retried after the managed local model, still + * running, cut its reply mid-stream (ATO-1279). Once per step: a + * second cut on the same step ends the turn. + */ + localStreamCutRetry: { stepIndex: number } | null; /** * Restarts of the local model after it ran out of memory with no step * completed in between (ATO-334). Bounded by @@ -68,6 +74,8 @@ export interface TurnLoopState { * a step that completes without a cut resets it. */ truncationRetries: number; + /** Retries of one step after a provider's time limit ended its reply. */ + midReplyTimeoutRetry: { stepIndex: number; attempts: number } | null; /** The step index already retried after a request-size refusal. */ sizeRepackRetry: { stepIndex: number } | null; /** The loop's own estimate of the last prompt built, for the repack fallback. */ @@ -113,12 +121,14 @@ export function createTurnLoopState(): TurnLoopState { spendUsd: 0, spendPriced: false, legMadeProgress: false, + localStreamCutRetry: null, outageWaitedMs: 0, outageAttempts: 0, awaitingRecovery: false, oomRestarts: 0, truncationRetry: null, truncationRetries: 0, + midReplyTimeoutRetry: null, sizeRepackRetry: null, lastPromptTokens: 0, lastBoundaryIndex: -1, diff --git a/src/analytics/analytics-client.ts b/src/analytics/analytics-client.ts index 31764a425..704578b2d 100644 --- a/src/analytics/analytics-client.ts +++ b/src/analytics/analytics-client.ts @@ -5,6 +5,7 @@ import { POSTHOG_PLACEHOLDER_KEY, POSTHOG_PROJECT_KEY, } from "./posthog-config.js"; +import { isTeamRun } from "./read-team-run-marker.js"; import { isAnalyticsKilledByEnv } from "./read-analytics-kill-switch.js"; import type { AnalyticsDimensions } from "./resolve-analytics-dimensions.js"; @@ -42,6 +43,12 @@ export interface AnalyticsClientOptions { */ dimensions?: AnalyticsDimensions; logger?: AnalyticsLogger; + /** + * ATO-1247: this is our own machine or test run (see isTeamRun). Every + * event then carries `test_run: true` and sets the person property + * `atomic_team: true`, so PostHog can leave it out of the metrics. + */ + team?: boolean; /** Test seam — inject a fake PostHog implementation. */ posthog?: PostHogLike; } @@ -69,6 +76,7 @@ export class AnalyticsClient { private readonly platform: string; private readonly version: string; private readonly globals: Record; + private readonly team: boolean; private readonly logger?: AnalyticsLogger; constructor(options: AnalyticsClientOptions) { @@ -76,6 +84,7 @@ export class AnalyticsClient { this.platform = options.platform; this.version = options.version; this.globals = globalProperties(options.dimensions); + this.team = options.team === true; if (options.logger) this.logger = options.logger; this.posthog = options.posthog ?? @@ -160,6 +169,8 @@ export class AnalyticsClient { // a truthy `$ip` override — a `null`/falsy value is ignored and the // request IP is used instead, so the sentinel MUST be a real string. $ip: IP_PLACEHOLDER, + // ATO-1247: our own runs, tagged on the event and on the person. + ...(this.team ? { test_run: true, $set: { atomic_team: true } } : {}), }, disableGeoip: true, }; @@ -193,6 +204,8 @@ export function createAnalyticsClient(options: { posthog?: PostHogLike; /** Test seam for the kill-switch env (defaults to `process.env`). */ env?: NodeJS.ProcessEnv; + /** Test seam for the team marker (defaults to isTeamRun()). */ + team?: boolean; }): AnalyticsClient | null { if (!options.enabled) return null; if (isAnalyticsKilledByEnv(options.env)) return null; @@ -207,6 +220,7 @@ export function createAnalyticsClient(options: { installId: options.installId, platform: options.platform, version: options.version, + team: options.team ?? isTeamRun(options.env), ...(options.dimensions ? { dimensions: options.dimensions } : {}), ...(options.logger ? { logger: options.logger } : {}), ...(options.posthog ? { posthog: options.posthog } : {}), diff --git a/src/analytics/read-team-run-marker.test.ts b/src/analytics/read-team-run-marker.test.ts new file mode 100644 index 000000000..25b24c083 --- /dev/null +++ b/src/analytics/read-team-run-marker.test.ts @@ -0,0 +1,61 @@ +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { AnalyticsClient } from "./analytics-client.js"; +import { isTeamRun, TEAM_RUN_MARKER_FILE } from "./read-team-run-marker.js"; + +const homes: string[] = []; +function home(withMarker: boolean): string { + const dir = mkdtempSync(join(tmpdir(), "team-run-")); + homes.push(dir); + if (withMarker) writeFileSync(join(dir, TEAM_RUN_MARKER_FILE), ""); + return dir; +} +afterEach(() => { + for (const d of homes.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +describe("isTeamRun (ATO-1247)", () => { + it("is off with no flag and no marker file", () => { + expect(isTeamRun({}, home(false))).toBe(false); + }); + + it("is on with ATOMIC_AGENT_TEAM set to a yes value", () => { + for (const v of ["1", "true", "YES", " on "]) { + expect(isTeamRun({ ATOMIC_AGENT_TEAM: v }, home(false))).toBe(true); + } + for (const v of ["0", "false", "", "off"]) { + expect(isTeamRun({ ATOMIC_AGENT_TEAM: v }, home(false))).toBe(false); + } + }); + + it("is on when ~/.atomic-agent-team exists", () => { + expect(isTeamRun({}, home(true))).toBe(true); + }); +}); + +describe("AnalyticsClient team tag (ATO-1247)", () => { + const posthog = () => ({ capture: vi.fn(), shutdown: vi.fn().mockResolvedValue(undefined) }); + + it("tags every event of a team run and sets the person property", () => { + const ph = posthog(); + const client = new AnalyticsClient({ installId: "x", platform: "darwin", version: "1.0.0", team: true, posthog: ph }); + client.capture("message_sent", { provider: "p" }); + const props = ph.capture.mock.calls[0]![0]!.properties; + expect(props.test_run).toBe(true); + expect(props.$set).toEqual({ atomic_team: true }); + expect(props.provider).toBe("p"); + }); + + it("adds nothing for everyone else", () => { + const ph = posthog(); + const client = new AnalyticsClient({ installId: "x", platform: "darwin", version: "1.0.0", posthog: ph }); + client.capture("message_sent"); + const props = ph.capture.mock.calls[0]![0]!.properties; + expect(props).not.toHaveProperty("test_run"); + expect(props).not.toHaveProperty("$set"); + }); +}); diff --git a/src/analytics/read-team-run-marker.ts b/src/analytics/read-team-run-marker.ts new file mode 100644 index 000000000..57ed4aab2 --- /dev/null +++ b/src/analytics/read-team-run-marker.ts @@ -0,0 +1,38 @@ +import { existsSync } from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; + +/** + * ATO-1247: our own machines and test runs (QA of a release, MBP checks, + * benchmarks on Vast) are tagged so PostHog can leave them out of the + * metrics without a hand-kept list of install ids. + * + * Two ways to say "this is us", either is enough: + * - `ATOMIC_AGENT_TEAM=1` (also true / yes / on) in the environment, for + * scripted runs (benchmarks, smoke, CI); + * - a file `~/.atomic-agent-team` (any content), created once on a team + * machine, so every later run there is tagged, the desktop app opened + * from Finder included. + * + * A tagged run still sends its events, with `test_run: true` on each and the + * person property `atomic_team: true` (see AnalyticsClient). It is not an + * opt-out: `ATOMIC_AGENT_ANALYTICS=off` stays the way to send nothing. + */ +export const TEAM_RUN_ENV = "ATOMIC_AGENT_TEAM"; +export const TEAM_RUN_MARKER_FILE = ".atomic-agent-team"; + +const ON_VALUES = new Set(["1", "true", "yes", "on"]); + +/** True when this run is ours: the env flag, or the marker file in the home directory. */ +export function isTeamRun( + env: NodeJS.ProcessEnv = process.env, + home: string = homedir(), +): boolean { + const raw = env[TEAM_RUN_ENV]; + if (typeof raw === "string" && ON_VALUES.has(raw.trim().toLowerCase())) return true; + try { + return existsSync(join(home, TEAM_RUN_MARKER_FILE)); + } catch { + return false; + } +} diff --git a/src/analytics/toggle-runtime-telemetry.ts b/src/analytics/toggle-runtime-telemetry.ts index 0f13efdc2..27b01c130 100644 --- a/src/analytics/toggle-runtime-telemetry.ts +++ b/src/analytics/toggle-runtime-telemetry.ts @@ -46,6 +46,8 @@ export function buildRuntimeTelemetry(options: { logger?: AnalyticsLogger & ErrorReportLogger; env?: NodeJS.ProcessEnv; factories?: RuntimeTelemetryFactories; + /** Sentry's per-install rate-limit file (ATO-1286); memory-only when omitted. */ + sentryRateLimitPath?: string; }): RuntimeTelemetry { const factories = options.factories ?? DEFAULT_FACTORIES; const enabled = options.enabled && !isAnalyticsKilledByEnv(options.env); @@ -70,6 +72,9 @@ export function buildRuntimeTelemetry(options: { platform: process.platform, dimensions: options.dimensions, ...logger, + ...(options.sentryRateLimitPath + ? { rateLimitPath: options.sentryRateLimitPath } + : {}), }), }; } diff --git a/src/approval/README.md b/src/approval/README.md index eb790bb34..90867c401 100644 --- a/src/approval/README.md +++ b/src/approval/README.md @@ -20,7 +20,7 @@ This area owns approval policy and routing. Read [AGENTS.md](AGENTS.md) before c ## Ownership and dependencies -Gate order in `ApprovalGate.request`: the same-turn refusal (`declinedInTurn`), a matching deny rule (refused, `automatic`), a matching ask rule (skips the level and grants and prompts), the level, the session grant, a matching allow rule, then the session's refuse policy or the prompt. `resolve({ grant: "always" })` saves the allow rule built from the pending request, scoped to its project; hardline shell-guard rules fire before the gate and no rule touches them. A fan-out scope returns before the gate, so the tools ask `ruleDecisionFor` first: a deny or ask rule sends the call to the gate. A worker in a throwaway worktree matches rules against its parent's project (`SessionApprovalPolicy.ruleWorkdir`). +Gate order in `ApprovalGate.request`: the same-turn refusal (`declinedInTurn`), a matching deny rule (refused, `automatic`), a matching ask rule (skips the level and grants and prompts), the level, the session grant, a matching allow rule, then the session's refuse policy or the prompt. `resolve({ grant: "always" })` saves the allow rule built from the pending request, scoped to its project; hardline shell-guard rules fire before the gate and no rule touches them. A fan-out scope returns before the gate, so the tools ask `ruleDecisionFor` first: a deny or ask rule sends the call to the gate. A shell command the guard deems safe never asks either, so `os.shell.run` asks `ruleDecisionFor` for it too: a deny rule sends it to the gate, which refuses it (ATO-1332). Deny and ask rules match every command in `commandSegments`: past wrappers and their value flags (`sudo -u root`, `nice -n 5`, `timeout 5`, `env -u X`, `xargs -n 1`), inside a nested `sh|bash|zsh|dash|ksh|fish|csh|tcsh -c` line, `eval` and `find -exec` (`shellRuleSegments`). A script fed to a bare shell or interpreter through a pipe, a redirect or a heredoc (`printf … | sh`) cannot be read, so the request is marked `commandUnreadable` and any deny or ask command rule matches it. An allow rule still needs `commandArgv`. A worker in a throwaway worktree matches rules against its parent's project (`SessionApprovalPolicy.ruleWorkdir`). `wouldPrompt` (the fan-out's pre-check) follows the same order: a deny or allow rule means no prompt, an ask rule means a prompt whatever the level or a grant (ATO-1270). ApprovalRouter selects the handler for the owning session; level/grant logic decides whether a dispatch can proceed unattended or needs a decision. Frontends and channel bridges provide interaction, while tools consume the decision. Approval state is separate from offered prompt schemas and from the currently selected UI chat. diff --git a/src/approval/approval-gate-would-prompt.test.ts b/src/approval/approval-gate-would-prompt.test.ts new file mode 100644 index 000000000..d4297353c --- /dev/null +++ b/src/approval/approval-gate-would-prompt.test.ts @@ -0,0 +1,139 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ApprovalGate, type ApprovalRequest } from "./approval-gate.js"; +import { runWithApprovalLedger, type ToolApprovalRecord } from "./approval-ledger.js"; +import { ApprovalRulesStore } from "./approval-rules-store.js"; +import type { ApprovalLevel } from "./approval-level.js"; + +/** + * ATO-1270: `wouldPrompt` (the fan-out's pre-check) reads saved rules + * the way `request` does, and an allow rule's auto-approval lands in + * the tool call's approval ledger. + */ +describe("ApprovalGate.wouldPrompt with saved rules (ATO-1270)", () => { + let stateDir: string; + let project: string; + let store: ApprovalRulesStore; + let emitted: ApprovalRequest[]; + + function gate(level: ApprovalLevel = 1): ApprovalGate { + const g = new ApprovalGate({ emit: (r) => emitted.push(r), level }); + g.setRules(store); + return g; + } + + beforeEach(() => { + stateDir = mkdtempSync(join(tmpdir(), "would-prompt-state-")); + project = mkdtempSync(join(tmpdir(), "would-prompt-project-")); + store = new ApprovalRulesStore({ stateDir }); + emitted = []; + }); + + afterEach(() => { + rmSync(stateDir, { recursive: true, force: true }); + rmSync(project, { recursive: true, force: true }); + }); + + it("without rules it still follows the level", () => { + expect(gate(1).wouldPrompt("s", "shell")).toBe(true); + expect(gate(5).wouldPrompt("s", "shell")).toBe(false); + }); + + it("an allow rule means no prompt", () => { + store.add({ decision: "allow", tool: "os.fs.write", category: "fs_write_workspace", source: "user" }); + const g = gate(1); + expect(g.wouldPrompt("s", "fs_write_workspace", { tool: "os.fs.write", workdir: project })).toBe(false); + // Another tool is not covered. + expect(g.wouldPrompt("s", "fs_write_workspace", { tool: "os.fs.patch", workdir: project })).toBe(true); + }); + + it("a deny rule means no prompt: the call is refused unasked", async () => { + store.add({ decision: "deny", tool: "fusion.delegate", source: "user" }); + const g = gate(1); + expect(g.wouldPrompt("s", "fusion_fanout", { tool: "fusion.delegate", workdir: project })).toBe(false); + await expect( + g.request({ sessionId: "s", tool: "fusion.delegate", category: "fusion_fanout", reason: "r", workdir: project }), + ).resolves.toMatchObject({ approved: false, automatic: true }); + expect(emitted).toEqual([]); + }); + + it("an ask rule prompts even where the level would run the call", () => { + store.add({ decision: "ask", tool: "fusion.delegate", source: "user" }); + const g = gate(5); + expect(g.wouldPrompt("s", "fusion_fanout", { tool: "fusion.delegate", workdir: project })).toBe(true); + // Without the tool named, the tool-scoped rule cannot match. + expect(g.wouldPrompt("s", "fusion_fanout", { workdir: project })).toBe(false); + }); + + it("an ask rule prompts past a session grant", async () => { + store.add({ decision: "ask", category: "http", source: "user" }); + const g = new ApprovalGate({ + level: 1, + emit: (r) => { + emitted.push(r); + g.resolve({ approvalId: r.approvalId, approved: true, grant: "category" }); + }, + }); + await g.request({ sessionId: "s", tool: "web.fetch", category: "http", reason: "r" }); + expect(g.sessionGrants("s").categories).toEqual(["http"]); + expect(g.wouldPrompt("s", "http")).toBe(false); + g.setRules(store); + expect(g.wouldPrompt("s", "http", { tool: "web.fetch", workdir: project })).toBe(true); + }); + + it("an allow rule never covers a category that is never granted", () => { + store.add({ decision: "allow", category: "trust_config", source: "user" }); + expect(gate(1).wouldPrompt("s", "trust_config", { workdir: project })).toBe(true); + }); + + it("a refuse policy still answers for the session", () => { + const g = gate(1); + g.setSessionPolicy("worker", { onPrompt: "refuse", reason: "no person here" }); + expect(g.wouldPrompt("worker", "shell")).toBe(false); + }); + + it("asks nothing and records nothing", async () => { + store.add({ decision: "allow", tool: "os.fs.write", category: "fs_write_workspace", source: "user" }); + const g = gate(1); + const ledger: ToolApprovalRecord[] = []; + await runWithApprovalLedger(ledger, async () => { + g.wouldPrompt("s", "fs_write_workspace", { tool: "os.fs.write", workdir: project }); + }); + expect(ledger).toEqual([]); + expect(emitted).toEqual([]); + }); + + it("a call an allow rule approves is logged in the call's ledger", async () => { + const rule = store.add({ + decision: "allow", + tool: "os.fs.write", + category: "fs_write_workspace", + source: "user", + }); + const g = gate(1); + const ledger: ToolApprovalRecord[] = []; + const decision = await runWithApprovalLedger(ledger, () => + g.request({ + sessionId: "s", + tool: "os.fs.write", + category: "fs_write_workspace", + reason: "r", + workdir: project, + }), + ); + expect(decision.approved).toBe(true); + expect(emitted).toEqual([]); + expect(ledger).toEqual([ + expect.objectContaining({ + verdict: "auto-approved", + via: "rule", + category: "fs_write_workspace", + reason: expect.stringContaining(`rule ${rule.id}`), + }), + ]); + }); +}); diff --git a/src/approval/approval-gate.test.ts b/src/approval/approval-gate.test.ts index 0c28d2690..08a7a045f 100644 --- a/src/approval/approval-gate.test.ts +++ b/src/approval/approval-gate.test.ts @@ -206,6 +206,77 @@ describe("ApprovalGate", () => { expect(gate.sessionGrants().categories).toEqual([]); }); + it("a keyed category grant covers only that key: POST to another host still asks (ATO-1182)", async () => { + let emitted = 0; + const gate = new ApprovalGate({ + emit: (req) => { + emitted += 1; + setImmediate(() => + gate.resolve({ + approvalId: req.approvalId, + approved: true, + grant: "category", + }), + ); + }, + }); + const http = (origin: string) => + ({ + sessionId: "s", + tool: "os.http.request", + category: "http", + reason: `POST ${origin}/x`, + grantKey: origin, + }) as const; + + await gate.request(http("https://a.example")); + expect(emitted).toBe(1); + // Not a whole-category grant. + expect(gate.sessionGrants("s").categories).toEqual([]); + + // Same origin: silent. + const again = await gate.request(http("https://a.example")); + expect(again.approved).toBe(true); + expect(again.reason).toBe("auto-approved (session grant: https://a.example)"); + expect(emitted).toBe(1); + + // Another origin (another host, or the same host on another port): asks. + await gate.request(http("https://b.example")); + expect(emitted).toBe(2); + await gate.request(http("https://a.example:8443")); + expect(emitted).toBe(3); + + // Another session never rides the grant. + await gate.request({ ...http("https://a.example"), sessionId: "other" }); + expect(emitted).toBe(4); + }); + + it("a whole-category grant does not answer a keyed request (ATO-1182)", async () => { + let emitted = 0; + const gate = new ApprovalGate({ + emit: (req) => { + emitted += 1; + setImmediate(() => + gate.resolve({ + approvalId: req.approvalId, + approved: true, + grant: "category", + }), + ); + }, + }); + await gate.request({ sessionId: "s", tool: "t", category: "http", reason: "r" }); + expect(gate.sessionGrants("s").categories).toEqual(["http"]); + await gate.request({ + sessionId: "s", + tool: "os.http.request", + category: "http", + reason: "r", + grantKey: "https://a.example", + }); + expect(emitted).toBe(2); + }); + it("s grants the category for the session: same category goes silent, others still ask", async () => { let emitted = 0; const gate = new ApprovalGate({ diff --git a/src/approval/approval-gate.ts b/src/approval/approval-gate.ts index ee87def6c..a362a8289 100644 --- a/src/approval/approval-gate.ts +++ b/src/approval/approval-gate.ts @@ -9,6 +9,7 @@ import { } from "./approval-ledger.js"; import { clampApprovalLevel, + formatApprovalCategory, isAutoApprovedAt, isGrantableCategory, MIN_APPROVAL_LEVEL, @@ -26,6 +27,13 @@ import { } from "./approval-rules.js"; import type { ApprovalRulesSource } from "./approval-rules-store.js"; import { currentApprovalWorkdir } from "./approval-workdir.js"; +import { + isSiteScopedCategory, + parseSiteGrantKey, + siteGrantHost, + siteGrantKey, + type SiteGrant, +} from "./site-grants.js"; import { formatRiskNote, isRiskReviewEligible, @@ -57,6 +65,17 @@ export interface ApprovalRequest { * the shape option is not offered. */ commandShape?: string; + /** + * The narrower unit a session grant (`s`, scope `category`) covers for + * this request, when the whole category would be too broad. Set by + * `os.http.request` to the target's origin (`https://api.example.com`, + * ATO-1182): "allow for this session" on a POST to one host must not + * silence a later POST to any other host. With a key, a category grant + * is remembered for (category, key) only, and only a grant with the + * same key answers the request; a key-less whole-category grant does + * not. Absent means the grant covers the whole category, as before. + */ + grantKey?: string; /** * Parsed argv of a simple shell command (`["git", "status", "-s"]`), * the unit a saved rule's `commandPrefix` matches. Set only by the shell @@ -72,6 +91,14 @@ export interface ApprovalRequest { * stricter; an allow rule still needs `commandArgv`. */ commandSegments?: string[][]; + /** + * The call runs a script no rule can read: a shell or interpreter with + * no script of its own, fed through a pipe, a redirect or a heredoc + * (`printf 'rm -rf x' | sh`, `shellRunsUnreadableScript`). Only a deny + * or ask command rule reads it, and it then matches whatever its + * prefix: what runs could be the very command the rule names. + */ + commandUnreadable?: boolean; /** * The session's working directory, for rules scoped to a project. The * gate fills it from the dispatching tool call when the tool did not. @@ -179,6 +206,7 @@ export type RuleCheck = Pick< | "category" | "commandArgv" | "commandSegments" + | "commandUnreadable" | "targetPaths" | "affectedResources" | "workdir" @@ -206,6 +234,11 @@ export interface SessionApprovalPolicy { export interface SessionGrantsSnapshot { categories: readonly ApprovalCategory[]; shapes: readonly string[]; + /** + * Per-site browser grants (`site-grants.ts`, ATO-1181); present only + * when there is at least one. + */ + sites?: readonly SiteGrant[]; } export class ApprovalGateError extends Error { @@ -288,7 +321,13 @@ export class ApprovalGate { */ private readonly grantsBySession = new Map< string, - { categories: Set; shapes: Set } + { + categories: Set; + shapes: Set; + sites: Set; + /** Category grants narrowed by `grantKey`, see `scopedGrantId`. */ + scoped: Set; + } >(); /** Per-session prompt policies, keyed like the grants. See `SessionApprovalPolicy`. */ private readonly policiesBySession = new Map(); @@ -438,18 +477,23 @@ export class ApprovalGate { sessionGrants(sessionId?: string): SessionGrantsSnapshot { if (sessionId !== undefined) { const entry = this.grantsBySession.get(sessionId); - return { - categories: entry ? [...entry.categories] : [], - shapes: entry ? [...entry.shapes] : [], - }; + return withSites( + { + categories: entry ? [...entry.categories] : [], + shapes: entry ? [...entry.shapes] : [], + }, + entry?.sites ?? new Set(), + ); } const categories = new Set(); const shapes = new Set(); + const sites = new Set(); for (const entry of this.grantsBySession.values()) { for (const category of entry.categories) categories.add(category); for (const shape of entry.shapes) shapes.add(shape); + for (const site of entry.sites) sites.add(site); } - return { categories: [...categories], shapes: [...shapes] }; + return withSites({ categories: [...categories], shapes: [...shapes] }, sites); } request( @@ -483,7 +527,11 @@ export class ApprovalGate { return Promise.resolve({ approvalId, approved: false, - reason: `blocked by your rule ${rule.rule.id}: ${describeRule(rule.rule)}`, + reason: + `blocked by your rule ${rule.rule.id}: ${describeRule(rule.rule)}` + + (request.commandUnreadable === true + ? " (the command runs a script fed to a shell or interpreter, which a rule cannot read)" + : ""), automatic: true, }); } @@ -629,6 +677,22 @@ export class ApprovalGate { if (!isGrantableCategory(request.category)) return null; const grants = this.grantsBySession.get(request.sessionId); if (!grants) return null; + // A browser submit / off-site grant covers the one site it was made + // on, never the whole category (ATO-1181). + if (isSiteScopedCategory(request.category)) { + const host = siteGrantHost(request); + return host !== null && grants.sites.has(siteGrantKey(request.category, host)) + ? `auto-approved (session grant: ${host})` + : null; + } + if (request.grantKey !== undefined) { + // A keyed request (an HTTP call to one origin) is answered only by + // a grant for the same key, never by a whole-category grant. + if (grants.scoped.has(scopedGrantId(request.category, request.grantKey))) { + return `auto-approved (session grant: ${request.grantKey})`; + } + return null; + } if (grants.categories.has(request.category)) { return "auto-approved (session grant)"; } @@ -814,12 +878,38 @@ export class ApprovalGate { /** * Whether a request of `category` from `sessionId` would be put to a - * person now: neither the level nor a grant settles it, and no refuse - * policy answers for the session. + * person now, in the order `request` decides it: a saved deny or allow + * rule settles it without a person; an ask rule puts it to one + * whatever the level or a grant says; otherwise neither the level nor + * a grant settles it, and no refuse policy answers for the session. + * `check` names the call for the rules (tool, resources, command); + * without it only rules that name no tool or command can match. + * Asks nothing and records nothing (ATO-1270). */ - wouldPrompt(sessionId: string, category: ApprovalCategory): boolean { - const probe = { approvalId: "", sessionId, tool: "", category, reason: "" }; - if (this.autoApproval(probe) !== null) return false; + wouldPrompt( + sessionId: string, + category: ApprovalCategory, + check: Partial> = {}, + ): boolean { + const workdir = check.workdir ?? currentApprovalWorkdir(); + const rule = this.matchRule({ + tool: "", + ...check, + sessionId, + category, + ...(workdir !== undefined ? { workdir } : {}), + }); + if (rule?.decision === "deny" || rule?.decision === "allow") return false; + if (rule?.decision !== "ask") { + const probe = { + approvalId: "", + sessionId, + tool: check.tool ?? "", + category, + reason: "", + }; + if (this.autoApproval(probe) !== null) return false; + } return this.policiesBySession.get(sessionId)?.onPrompt !== "refuse"; } @@ -871,8 +961,23 @@ export class ApprovalGate { this.saveAlwaysRule(request); return; } + if (scope === "category" && isSiteScopedCategory(request.category)) { + // Per site (ATO-1181); a request naming no single host stays a + // one-off, and `canGrantCategory` does not offer it. + const host = siteGrantHost(request); + if (host !== null) { + this.grantsForSession(request.sessionId).sites.add( + siteGrantKey(request.category, host), + ); + } + return; + } const grants = this.grantsForSession(request.sessionId); if (scope === "category") { + if (request.grantKey !== undefined) { + grants.scoped.add(scopedGrantId(request.category, request.grantKey)); + return; + } grants.categories.add(request.category); return; } @@ -903,10 +1008,12 @@ export class ApprovalGate { private grantsForSession(sessionId: string): { categories: Set; shapes: Set; + sites: Set; + scoped: Set; } { let entry = this.grantsBySession.get(sessionId); if (!entry) { - entry = { categories: new Set(), shapes: new Set() }; + entry = { categories: new Set(), shapes: new Set(), sites: new Set(), scoped: new Set() }; this.grantsBySession.set(sessionId, entry); } return entry; @@ -987,14 +1094,45 @@ export class ApprovalGate { } } +/** + * The id a key-narrowed category grant is stored under. NUL cannot occur + * in a category name or an origin, so two pairs never collide. + */ +function scopedGrantId(category: ApprovalCategory, key: string): string { + return `${category}\u0000${key}`; +} + /** * Whether the approval prompt may offer `[s]` (grant the whole category * for the session) for this request. Everything is grantable except * `trust_config`. Shared by every surface (TUI modal, TUI keys, CLI - * prompt) so the offer set never drifts between them. + * prompt) so the offer set never drifts between them. A browser submit + * or off-site request is granted per site (`site-grants.ts`), so `[s]` + * is offered only when it names exactly one host. */ export function canGrantCategory(request: ApprovalRequest): boolean { - return isGrantableCategory(request.category); + if (!isGrantableCategory(request.category)) return false; + if (isSiteScopedCategory(request.category)) return siteGrantHost(request) !== null; + return true; +} + +/** + * What `[s]` grants, in the words a prompt shows after "allow": + * `browser commit on example.com` for a per-site grant, the category's + * label otherwise. + */ +export function describeCategoryGrant(request: ApprovalRequest): string { + const label = formatApprovalCategory(request.category); + const host = siteGrantHost(request); + return host !== null ? `${label} on ${host}` : label; +} + +function withSites( + snapshot: { categories: ApprovalCategory[]; shapes: string[] }, + sites: ReadonlySet, +): SessionGrantsSnapshot { + if (sites.size === 0) return snapshot; + return { ...snapshot, sites: [...sites].map(parseSiteGrantKey) }; } /** diff --git a/src/approval/approval-rules.test.ts b/src/approval/approval-rules.test.ts index 77c19a6f7..582d8ef38 100644 --- a/src/approval/approval-rules.test.ts +++ b/src/approval/approval-rules.test.ts @@ -120,6 +120,20 @@ describe("matchRules", () => { expect(matchRules(ask, line([["/usr/bin/git", "push"]]))?.decision).toBe("ask"); }); + it("a deny or ask matches a script piped into a shell whatever its prefix, an allow does not", () => { + const piped = line([["printf", "x"], ["sh"]], { commandUnreadable: true }); + expect( + matchRules([rule({ decision: "deny", commandPrefix: ["npm", "publish"] })], piped)?.decision, + ).toBe("deny"); + expect( + matchRules([rule({ decision: "ask", commandPrefix: ["rm"] })], piped)?.decision, + ).toBe("ask"); + expect(matchRules([rule({ decision: "allow", commandPrefix: ["sh"] })], piped)).toBeNull(); + expect( + matchRules([rule({ decision: "deny", commandPrefix: ["rm"] })], line([["printf", "x"], ["sh"]])), + ).toBeNull(); + }); + it("an allow never matches through segments", () => { const allow = [rule({ decision: "allow", commandPrefix: ["git"] })]; expect(matchRules(allow, line([["git", "status"]]))).toBeNull(); diff --git a/src/approval/approval-rules.ts b/src/approval/approval-rules.ts index 01baf1c66..a3c273c9d 100644 --- a/src/approval/approval-rules.ts +++ b/src/approval/approval-rules.ts @@ -1,4 +1,5 @@ import { createHash } from "node:crypto"; +import { isSiteScopedCategory } from "./site-grants.js"; import { basename, isAbsolute, resolve as resolvePath } from "node:path"; import type { ApprovalRequest } from "./approval-gate.js"; @@ -46,7 +47,8 @@ export interface ApprovalRule { * basename (case-insensitively), the rest exactly. An allow rule needs * the request's `commandArgv`; a deny or ask rule also matches any * command in `commandSegments`, so a compound line, a binary named by - * path or a dangerous command cannot step round it. + * path or a dangerous command cannot step round it, and any request + * marked `commandUnreadable` (a script piped into a shell). */ commandPrefix?: string[]; /** Every path the request names must be inside this directory. */ @@ -258,6 +260,7 @@ type MatchableRequest = Pick< | "category" | "commandArgv" | "commandSegments" + | "commandUnreadable" | "targetPaths" | "affectedResources" >; @@ -330,10 +333,10 @@ function commandMatches( request: MatchableRequest, ): boolean { if (argvHasPrefix(request.commandArgv, prefix)) return true; - return ( - rule.decision !== "allow" && - (request.commandSegments ?? []).some((words) => argvHasPrefix(words, prefix)) - ); + if (rule.decision === "allow") return false; + // A script piped into `sh` could be the very command the rule names. + if (request.commandUnreadable === true) return true; + return (request.commandSegments ?? []).some((words) => argvHasPrefix(words, prefix)); } function argvHasPrefix( @@ -440,6 +443,10 @@ export function canGrantAlways(request: ApprovalRequest): boolean { // A rule cannot say "this one file in home": it would cover any file // there, `.zshrc` included, from then on. if (request.category === "fs_write_home") return false; + // A rule has no site field, so "always" on a browser submit or an + // off-site page would cover every site for good; those are granted + // per site for the chat only (`site-grants.ts`, ATO-1181). + if (isSiteScopedCategory(request.category)) return false; if (request.category === "fs_read_outside") return alwaysReadRoot(request) !== undefined; return true; } diff --git a/src/approval/dangerous-tool.ts b/src/approval/dangerous-tool.ts index 223d3f511..0bb492cc3 100644 --- a/src/approval/dangerous-tool.ts +++ b/src/approval/dangerous-tool.ts @@ -22,10 +22,14 @@ export interface ApprovalPrompt { affectedResources?: string[]; /** Command binary (argv[0]) for shell requests; unit of a shape grant. */ commandShape?: string; + /** What a session grant on this request covers; see `ApprovalRequest.grantKey`. */ + grantKey?: string; /** Parsed argv of a simple shell command; see `ApprovalRequest.commandArgv`. */ commandArgv?: readonly string[]; /** Every simple command the call would run; see `ApprovalRequest.commandSegments`. */ commandSegments?: readonly (readonly string[])[]; + /** A script fed to a shell no rule can read; see `ApprovalRequest.commandUnreadable`. */ + commandUnreadable?: boolean; /** Session working directory; see `ApprovalRequest.workdir`. */ workdir?: string; /** @@ -150,6 +154,19 @@ export function savedRuleOverridesScope( return decision === "deny" || decision === "ask"; } +/** + * True when a saved deny rule covers this call. A command the shell + * guard deems safe never asks, so without this a "never run `echo`" + * rule would not stop `echo hi` (ATO-1332): such a call goes to the + * gate, which refuses it before any level, grant or prompt. + */ +export function savedRuleDenies( + approvals: ApprovalGate, + check: RuleCheck, +): boolean { + return (approvals.ruleDecisionFor?.(check) ?? null) === "deny"; +} + /** * Shared helper used by dangerous tools (shell.run, fs.write, * skill.run_script, …). Centralising it ensures every tool sends the same @@ -175,12 +192,14 @@ export async function requireApproval( ...(prompt.commandShape !== undefined ? { commandShape: prompt.commandShape } : {}), + ...(prompt.grantKey !== undefined ? { grantKey: prompt.grantKey } : {}), ...(prompt.commandArgv !== undefined ? { commandArgv: [...prompt.commandArgv] } : {}), ...(prompt.commandSegments !== undefined ? { commandSegments: prompt.commandSegments.map((words) => [...words]) } : {}), + ...(prompt.commandUnreadable === true ? { commandUnreadable: true } : {}), ...(prompt.workdir !== undefined ? { workdir: prompt.workdir } : {}), ...(prompt.redirectablePath !== undefined ? { redirectablePath: prompt.redirectablePath } diff --git a/src/approval/index.ts b/src/approval/index.ts index c874e81dd..dd0478cb0 100644 --- a/src/approval/index.ts +++ b/src/approval/index.ts @@ -3,6 +3,7 @@ export { ApprovalGateError, canGrantCategory, canGrantShape, + describeCategoryGrant, } from "./approval-gate.js"; export type { ApprovalDecision, diff --git a/src/approval/risk-review.ts b/src/approval/risk-review.ts index b518793c9..1627761c9 100644 --- a/src/approval/risk-review.ts +++ b/src/approval/risk-review.ts @@ -1,4 +1,4 @@ -import { randomBytes } from "node:crypto"; +import { newUntrustedNonce, wrapUntrusted } from "../tools/untrusted-text.js"; import type { ApprovalRequest } from "./approval-gate.js"; import { formatApprovalCategory, @@ -182,23 +182,17 @@ function startsWord(before: string): boolean { /** A fresh unguessable marker for the untrusted blocks of one prompt. */ export function newRiskReviewNonce(): string { - return randomBytes(8).toString("hex"); + return newUntrustedNonce(); } /** * Wrap `content` in a block the reviewer is told to read as data. The * marker carries a per-call nonce, so text inside cannot close the block * and continue as instructions; any copy of the nonce in the content is - * removed anyway. + * removed and fake block markers are defused anyway. Shared with tool + * results and the browser page (`tools/untrusted-text.ts`, ATO-1378). */ -export function wrapUntrusted(label: string, content: string, nonce: string): string { - const safe = content.split(nonce).join(""); - return [ - `<<>>`, - safe, - `<<>>`, - ].join("\n"); -} +export { wrapUntrusted }; /** System message of the review request. */ export const RISK_REVIEW_SYSTEM = diff --git a/src/approval/site-grants.test.ts b/src/approval/site-grants.test.ts new file mode 100644 index 000000000..d1a2122c8 --- /dev/null +++ b/src/approval/site-grants.test.ts @@ -0,0 +1,129 @@ +import { describe, expect, it } from "vitest"; +import { + ApprovalGate, + canGrantCategory, + describeCategoryGrant, + type ApprovalRequest, +} from "./approval-gate.js"; +import { canGrantAlways } from "./approval-rules.js"; +import { siteGrantHost } from "./site-grants.js"; + +/** + * ATO-1181: "allow in this chat" on a browser submit or an off-site + * page covers that one site, not every site. + */ + +/** A gate whose operator answers every prompt with `[s]`, counting prompts. */ +function grantingGate() { + const prompts: ApprovalRequest[] = []; + const gate = new ApprovalGate({ + emit: (req) => { + prompts.push(req); + setImmediate(() => + gate.resolve({ approvalId: req.approvalId, approved: true, grant: "category" }), + ); + }, + }); + return { gate, prompts }; +} + +function click(host: string | null, category: "browser_commit" | "browser_offsite" = "browser_commit") { + return { + sessionId: "s", + tool: "browser.click", + category, + reason: "submit control", + ...(host !== null ? { affectedResources: [host] } : {}), + } as const; +} + +describe("per-site browser grants", () => { + it("a submit grant on site A covers A again, but site B asks", async () => { + const { gate, prompts } = grantingGate(); + await gate.request(click("shop.example")); + expect(prompts).toHaveLength(1); + + const again = await gate.request(click("shop.example")); + expect(again.approved).toBe(true); + expect(again.reason).toBe("auto-approved (session grant: shop.example)"); + expect(prompts).toHaveLength(1); + + await gate.request(click("evil.example")); + expect(prompts).toHaveLength(2); + }); + + it("a subdomain is a separate site, and so is the parent domain", async () => { + const { gate, prompts } = grantingGate(); + await gate.request(click("example.com")); + await gate.request(click("mail.example.com")); + await gate.request(click("example.com.evil.test")); + expect(prompts).toHaveLength(3); + }); + + it("host case and a trailing dot do not make a new site", async () => { + const { gate, prompts } = grantingGate(); + await gate.request(click("Shop.Example")); + await gate.request(click("shop.example.")); + expect(prompts).toHaveLength(1); + }); + + it("a submit grant does not cover off-site pages on the same host, nor the reverse", async () => { + const { gate, prompts } = grantingGate(); + await gate.request(click("shop.example", "browser_commit")); + await gate.request(click("shop.example", "browser_offsite")); + expect(prompts).toHaveLength(2); + await gate.request(click("shop.example", "browser_offsite")); + expect(prompts).toHaveLength(2); + }); + + it("grants stay inside the session that made them", async () => { + const { gate, prompts } = grantingGate(); + await gate.request(click("shop.example")); + await gate.request({ ...click("shop.example"), sessionId: "other" }); + expect(prompts).toHaveLength(2); + }); + + it("a request naming no host is never granted, and [s] is not offered", async () => { + const { gate, prompts } = grantingGate(); + const noHost = click(null); + expect(canGrantCategory({ approvalId: "x", ...noHost })).toBe(false); + await gate.request(noHost); + await gate.request(noHost); + expect(prompts).toHaveLength(2); + // Nor does a host-less grant attempt leak into a hosted request. + await gate.request(click("shop.example")); + expect(prompts).toHaveLength(3); + }); + + it("the snapshot lists site grants, and clearing the session drops them", async () => { + const { gate } = grantingGate(); + await gate.request(click("shop.example")); + expect(gate.sessionGrants("s")).toEqual({ + categories: [], + shapes: [], + sites: [{ category: "browser_commit", host: "shop.example" }], + }); + gate.clearSessionGrants("s"); + expect(gate.sessionGrants("s")).toEqual({ categories: [], shapes: [] }); + }); + + it("other categories keep their whole-category grant", async () => { + const { gate, prompts } = grantingGate(); + const http = { sessionId: "s", tool: "http.fetch", category: "http" as const, reason: "r", affectedResources: ["a.example"] }; + await gate.request(http); + await gate.request({ ...http, affectedResources: ["b.example"] }); + expect(prompts).toHaveLength(1); + }); + + it("the prompt names the site; 'always' is not offered for browser submits", () => { + const request = { approvalId: "x", workdir: "/tmp/p", ...click("shop.example") }; + expect(describeCategoryGrant(request)).toContain("on shop.example"); + expect(canGrantAlways(request)).toBe(false); + }); + + it("siteGrantHost needs exactly one host", () => { + expect(siteGrantHost({ category: "browser_commit", affectedResources: ["a.example", "b.example"] })).toBeNull(); + expect(siteGrantHost({ category: "browser_commit", affectedResources: [""] })).toBeNull(); + expect(siteGrantHost({ category: "shell", affectedResources: ["a.example"] })).toBeNull(); + }); +}); diff --git a/src/approval/site-grants.ts b/src/approval/site-grants.ts new file mode 100644 index 000000000..f24a252de --- /dev/null +++ b/src/approval/site-grants.ts @@ -0,0 +1,68 @@ +/** + * Per-site session grants for the browser (ATO-1181). + * + * "Allow in this chat" on a form submit at site A used to grant the + * whole `browser_commit` category, so every later submit, delete or + * post on any other site ran unasked, including one a prompt-injected + * page steered the agent to. For the two browser categories whose + * requests name the site they act on (`affectedResources: [host]`), + * a session grant now covers that one host only: a grant on + * `example.com` does not cover `mail.example.com`, `example.org` or + * any other host, and those ask again. + * + * Hosts match exactly (case and trailing dots aside), so a subdomain is + * a separate site. A request of these categories that names no single + * host cannot be granted for the session at all: it asks every time. + * `browser_secret` (passwords, payments) stays never-grantable. + */ +import type { ApprovalCategory } from "./approval-level.js"; + +/** Categories whose session grant is per site rather than per category. */ +const SITE_SCOPED: ReadonlySet = new Set([ + "browser_commit", + "browser_offsite", +]); + +export function isSiteScopedCategory(category: ApprovalCategory): boolean { + return SITE_SCOPED.has(category); +} + +/** The shape of a request this module reads. */ +export interface SiteScopedRequest { + category: ApprovalCategory; + affectedResources?: readonly string[]; +} + +/** + * The host a session grant for `request` would cover, or `null` when the + * category is not site-scoped or the request does not name exactly one + * host. Lower-cased, trailing dots removed, like the browser's own host + * normalisation (`browser-domain-policy.ts`). + */ +export function siteGrantHost(request: SiteScopedRequest): string | null { + if (!isSiteScopedCategory(request.category)) return null; + const resources = request.affectedResources ?? []; + if (resources.length !== 1) return null; + const host = (resources[0] ?? "").trim().toLowerCase().replace(/\.+$/, ""); + if (host.length === 0 || /[\s/]/.test(host)) return null; + return host; +} + +/** Key of one per-site grant in a session's grant set. */ +export function siteGrantKey(category: ApprovalCategory, host: string): string { + return `${category} ${host}`; +} + +/** One live per-site grant, for diagnostics and the privacy panel. */ +export interface SiteGrant { + category: ApprovalCategory; + host: string; +} + +export function parseSiteGrantKey(key: string): SiteGrant { + const space = key.indexOf(" "); + return { + category: key.slice(0, space) as ApprovalCategory, + host: key.slice(space + 1), + }; +} diff --git a/src/channels/telegram/task-report-message-sections.test.ts b/src/channels/telegram/task-report-message-sections.test.ts index b4f6e4b77..d5cbbe057 100644 --- a/src/channels/telegram/task-report-message-sections.test.ts +++ b/src/channels/telegram/task-report-message-sections.test.ts @@ -167,4 +167,28 @@ describe("formatTaskReportDigest", () => { for (const r of group) expect(text).toContain(r.taskId); }); }); + + it("shows the task's spend against its cap, not only this run's (ATO-1288)", () => { + const text = formatTaskReportMessage( + makeReport({ spendUsd: 0.5, spentUsd: 4.8, maxCostUsd: 5, stopCause: "cost_ceiling" }), + ); + expect(text).toContain("spent ~$4.80 of $5.00"); + expect(text).not.toContain("spend ~$0.50"); + }); + + it("puts a task blocked on a used-up budget under Needs you (ATO-1288)", () => { + const text = formatTaskReportMessage( + makeReport({ + status: "blocked", + replyText: null, + errorCategory: "tool", + errorMessage: "budget_spent: $5.00 of $5.00 already spent by earlier attempts", + spentUsd: 5, + maxCostUsd: 5, + }), + ); + expect(text).toContain("Needs you:"); + expect(text).toContain("Stopped at the budget limit."); + expect(text).toContain("budget_spent: $5.00 of $5.00"); + }); }); diff --git a/src/channels/telegram/task-report-message.ts b/src/channels/telegram/task-report-message.ts index 5ff48462a..6659fa5bc 100644 --- a/src/channels/telegram/task-report-message.ts +++ b/src/channels/telegram/task-report-message.ts @@ -92,12 +92,16 @@ export function formatTaskReportMessage( minText: RESULT_MIN_CHARS, } : null; - const hasRun = report.summary != null || report.stopCause != null; + // A task blocked before its turn because earlier attempts used up its + // budget is a "needs you", not only an error line (ATO-1288). + const usedUp = report.status === "blocked" ? usedUpBudgetCause(report.errorMessage) : null; + const stopCause = report.stopCause ?? usedUp; + const hasRun = report.summary != null || stopCause != null; const card = hasRun ? buildTaskReportCard({ status: report.status, summary: report.summary ?? null, - stopCause: report.stopCause ?? null, + stopCause: stopCause ?? null, }) : null; if (card === null && errorLine !== null) { @@ -203,6 +207,17 @@ export function formatTaskReportDigest( return out; } +/** + * The ceiling a `budget_spent:` / `time_spent:` block names + * (`TaskRunner.runOne`), or `null` for any other error. + */ +function usedUpBudgetCause(message: string | null): string | null { + if (message === null) return null; + if (message.startsWith("budget_spent:")) return "cost_ceiling"; + if (message.startsWith("time_spent:")) return "time_ceiling"; + return null; +} + function metaLine(report: TaskReport): string { const parts = [`Attempt ${report.attempts}/${report.maxAttempts}`]; if (report.durationMs !== null) { @@ -210,7 +225,14 @@ function metaLine(report: TaskReport): string { } if (report.stepCount != null) parts.push(`${report.stepCount} steps`); if (report.stopCause != null) parts.push(`stop: ${report.stopCause}`); - if (report.spendUsd != null) parts.push(`spend ~${formatSpend(report.spendUsd)}`); + // The task's spend over all its attempts against its cap, which is + // what the cap governs (ATO-1288); this run's own spend otherwise. + if (report.spentUsd != null) { + const spent = `spent ~${formatSpend(report.spentUsd)}`; + parts.push(report.maxCostUsd != null ? `${spent} of ${formatSpend(report.maxCostUsd)}` : spent); + } else if (report.spendUsd != null) { + parts.push(`spend ~${formatSpend(report.spendUsd)}`); + } return parts.join(" · "); } diff --git a/src/channels/telegram/telegram-channel.test.ts b/src/channels/telegram/telegram-channel.test.ts index ba218f1a2..52025eddc 100644 --- a/src/channels/telegram/telegram-channel.test.ts +++ b/src/channels/telegram/telegram-channel.test.ts @@ -1244,11 +1244,16 @@ describe("TelegramChannel live-control surface", () => { const delivery = await channel.sendTaskReport(makeReport()); expect(delivery).toBe("queued"); expect(state.sendMessageCalls).toHaveLength(0); + // What `overnight` reads before it exits (ATO-1358). + const queuedId = makeReport().taskId; + expect(channel.hasPendingTaskReport(queuedId)).toBe(true); + expect(channel.hasPendingTaskReport("t-other")).toBe(false); await channel.start(); await vi.waitFor(() => { expect(state.sendMessageCalls).toHaveLength(1); }); + expect(channel.hasPendingTaskReport(queuedId)).toBe(false); expect(state.sendMessageCalls[0]!.chatId).toBe(42); expect(state.sendMessageCalls[0]!.text).toContain( "✅ Scheduled task completed", diff --git a/src/channels/telegram/telegram-channel.ts b/src/channels/telegram/telegram-channel.ts index 467fdcaef..1fd1b2f4d 100644 --- a/src/channels/telegram/telegram-channel.ts +++ b/src/channels/telegram/telegram-channel.ts @@ -1146,6 +1146,15 @@ export class TelegramChannel { }; } + /** + * Whether a report for `taskId` is still waiting for the channel to + * come up. A short-lived process (`overnight`) reads it before it + * exits, since the in-memory queue goes with it (ATO-1358). + */ + hasPendingTaskReport(taskId: string): boolean { + return this.pendingTaskReports.some((report) => report.taskId === taskId); + } + /** FIFO-append with oldest-first eviction at `TASK_REPORT_QUEUE_LIMIT`. */ private queueTaskReport(report: TaskReport): void { this.pendingTaskReports.push(report); diff --git a/src/cli/README.md b/src/cli/README.md index 72ce2d63a..901cbad93 100644 --- a/src/cli/README.md +++ b/src/cli/README.md @@ -19,7 +19,7 @@ On a Windows console, startup switches the code page to UTF-8 (65001) so non-ASC `data backup|restore|move|reset|path` (ATO-579, [data-command.ts](data-command.ts), [data-archive.ts](data-archive.ts)) acts on the state dir the terminal and the desktop share. `backup ` writes an owner-only archive with a manifest; each `*.sqlite` goes in as a snapshot through SQLite's backup API, and `models/` (unless `--with-models`), `browser-profile/` (cookies), WAL/SHM, locks and runtime records stay out. `restore --yes` unpacks into a sibling folder, refuses an archive without the manifest, carries this machine's `models/` over when the backup has none, then swaps folders and keeps the old one as `.before-restore-