From e1e3ac4432eb0e24ae89b1ae7a4d95f0cffcc43e Mon Sep 17 00:00:00 2001 From: elkaix Date: Thu, 1 Oct 2026 06:39:39 -0400 Subject: [PATCH 1/9] docs: add regression and user-impact review to the PR workflow --- .agents/skills/gen-changesets/SKILL.md | 1 + .agents/skills/review-pr/SKILL.md | 194 ++++++++++++++++++++----- .agents/skills/review-pr/surfaces.md | 130 +++++++++++++++++ .agents/skills/write-pr/SKILL.md | 151 ++++++++++++------- .github/pull_request_template.md | 12 +- AGENTS.md | 28 ++++ 6 files changed, 427 insertions(+), 89 deletions(-) create mode 100644 .agents/skills/review-pr/surfaces.md diff --git a/.agents/skills/gen-changesets/SKILL.md b/.agents/skills/gen-changesets/SKILL.md index 7804162c1..51e6134c4 100644 --- a/.agents/skills/gen-changesets/SKILL.md +++ b/.agents/skills/gen-changesets/SKILL.md @@ -32,6 +32,7 @@ Fix occasional loss of tool call results in long conversations. Wording: - One short, user-facing English sentence that states only what changed. Drop trailing clauses that explain the cause, the benefit, or the mechanism. +- When a default flips or an existing behavior is removed, name the behavior users lose, not only the new default. State the escape hatch if one exists, e.g. `Stop hot-reloading config, skills, and AGENTS.md by default; set [watch] enabled = true or PYTHINKER_CODE_WATCH=1 to turn it back on.`; if there is none, say plainly that the old behavior is no longer available. - New features: say plainly what it is plus one line on how to use it, e.g. `Add the /foo slash command to list active sessions. Run /foo to see them.` - Experimental features: also state how to enable them (the flag, config key, or env var). - No file, class, or function names, and no PR numbers. No vague words like refactor, optimize, or improve. No real internal identifiers — use neutral placeholders such as `example.com` or `YOUR_API_KEY`. diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index a94d1d27b..7914a56ab 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -1,73 +1,199 @@ --- name: review-pr -description: Use when reviewing a pull request in the pythinker-code repository — evaluate the change against the template structure. +description: Use when reviewing a pull request in this repository — check the description against the PR template section by section, then run a separate regression and user-impact pass that produces impact levels and a review summary. disable-model-invocation: true --- # Review PR -Review a pull request by evaluating the description against the template and the actual diff, then write a structured review summary in English. +Most regressions in this repository were not logic errors. They were changes that were correct +relative to their own intent but silently altered a behavior an existing population depended on: +a provider-dialect branch narrowed, a default flipped, a system-prompt sentence deleted, an old +path's feature dropped in a refactor, a fix that changed startup timing. General review excludes +intentional behavior changes from bugs, so this skill runs that check as its own pass. + +The review criterion is one sentence: **any input that worked before the change — a config key, +an environment variable, a command-line flag, a provider response shape, session data written by +an older version, a client request, or a hook payload — must behave the same after it, unless the +PR explicitly declares the change and provides a way back.** ## Workflow -1. Fetch the PR description and metadata: +1. Fetch the PR metadata and description: ```bash gh pr view --json title,body,url,files,additions,deletions,baseRefName,headRefName ``` -2. Read the full diff: +2. Read the full diff (large PRs file by file): ```bash gh pr diff ``` -3. Read enough surrounding code to verify the claims in the PR description. +3. Read enough surrounding code to verify every claim in the description. The diff alone cannot + show who reached a branch before the change. + +4. **Pass 1: classify.** Decide the depth of pass 3; see "Pass 1: classify". + +5. **Pass 2: template section check.** See "Pass 2: template section check". + +6. **Pass 3: regression and user impact.** Run it in a fresh context when a subagent is + available: give it only the PR number, base and head, a two-to-three-sentence summary of the + change, and the path to `surfaces.md`. Do not give it the author's impact conclusions; have it + build the inventory from the diff and code first, then compare it against the author's table + when it returns. Without subagent capability, do it yourself in the same order: build the list + first, read the author's table second. See "Pass 3: regression and user impact". + +7. Write the review summary in English; format under "Output". + +## Pass 1: classify + +| Change class | Pass 3 depth | +|---|---| +| Pure internal refactor, tests, docs, test-only CI | Only evidence that nothing observable changed: which branches, defaults, and contract files are untouched | +| Release and packaging: release, native build, VS Code publishing workflows, packaging scripts, web bundle sync | Full pass; populations split by install channel and platform: npm package, per-platform native binaries, VS Code marketplace, bundled web assets | +| New feature that touches no existing path | Only check whether the new branch captures existing inputs first: does the new condition precede the old one | +| Behavior change, default flip, validation or permission tightened or loosened | Full pass | +| Prompt text: system prompt, tool descriptions, reminders, overlays, built-in skills | Full pass, and every edited sentence gets its own row | +| Replacement of an old path, large refactor, protocol swap | Full pass, and require the author to provide the old path's feature inventory | +| Protocol, disk format, SDK export, hook payload, or CLI contract change | Full pass, and name the consumers outside the repository | +| Fix that changes startup order, lifecycle, or async timing | Full pass, focused on who depended on the old order | + +A PR can belong to several classes at once; treat it as the heaviest one. + +## Pass 3: regression and user impact + +### 1. Behavior-change inventory + +List every observable behavior the diff changes, including behavior the author calls "unchanged" +and behavior the author considers "internal". Internal packages ship in the CLI release; +"internal" does not mean invisible to users. -4. Evaluate each section against the criteria below. +Each row records: behavior, before, after, change type (added / modified / removed / default +flip / tightened / loosened), and evidence (a `file:line` on each side; when one side does not +exist, give a reachable path on the other). -5. Write the review summary in English. +How to read the diff: -## Review Criteria +- For every deleted or narrowed branch, condition, default, or sentence, ask who reached it + before the change and where they go now. +- For every new branch or condition, ask which existing inputs now match it first. The classic + failure: a parser stops reading the string form of a field when the array form exists, and + gateways shaped like OpenRouter send both. +- Test changes that rewrite an old assertion to the new behavior are direct evidence of a + behavior change; map each one to an inventory row. +- Scan the diff file list against the "trigger clues" in `surfaces.md` first. Every matched + surface must have a corresponding row in the inventory, otherwise it is a miss. -### Requirement or Bug +### 2. Affected populations -- Is the linked issue valid and relevant? -- If no issue, is the requirement clearly stated in one or two sentences? +Map each row to a concrete population in `surfaces.md` and write the population's name together +with evidence that it exists: a doc page, an issue, a config example, a provider payload shape. +A row with no findable population does not become a finding; it goes to "author to confirm". -### Bug Reproduction Steps +### 3. Impact level -- For bug PRs: are the steps clear and reproducible? -- Can you follow the steps to confirm the bug exists on the base branch? +The level is decided only by the facts of the change, never by remediation state (changeset, +docs, tests). Three steps: -### Root Cause +1. Data loss, a widened security boundary, old data becoming unreadable, or a population's + feature unusable in full: L3 outright. +2. Otherwise: observable with a one-step way back (config, env, flag): L1. Observable with no + way back and the old behavior unrecoverable: L2. No observable change: L0. +3. Silent upgrade: a user-supplied input (config, env var, data, request field) is ignored or + dropped with no error signal, so users cannot discover it themselves: raise one level, at + most L3. Behavior that changed openly and is visible in the output is not silent. -- For bug PRs: is the root cause convincingly explained? -- Does the stated root cause match what you see in the diff? -- Is it clear whether this is a fundamental fix or a workaround? +Breadth has three tiers and must name the population: all users / one config or platform +population / edge environments. -### Code Changes +Level each row separately; the PR takes the highest. The reason the silent-upgrade rule exists: +with no signal there is no report, so the regression survives until someone stumbles on it. -- Does the description match the actual diff? -- Are visual outlines (diff blocks, call trees, file trees) accurate and helpful? -- Is the approach sound? Are there simpler alternatives? -- Are there edge cases the author missed? +### 4. Remediation check -### Impact Scope +Four fixed questions, each answered yes / no / not applicable, with evidence: -- Are all affected modules identified? Cross-check with the diff file list. -- Does test coverage match the claimed scope? -- Are there untested paths that carry risk? +- Escape hatch: is there a config, env, or flag that restores the old behavior. +- Changeset: does it name what users **lose**, not only the new default; when there is no way + back, does it say plainly that the old behavior is gone. +- Old data and old clients: sessions written by the previous version, migrated data, and clients + shipped in the previous release (desktop, web, VS Code, ACP) still work; rolling back to the + previous release is safe. +- Guard test: one test that pins the old behavior **for the population that keeps it**. -### Checklist +Plus one: are the documentation promises about this surface updated together with the change. -- Are all applicable items checked? -- For items marked as "not needed", do you agree? +### Handling per level + +- L3: recommend blocking. Requires a guard test plus an escape hatch, a flag gate, or a + migration; without all of them it must not merge. +- L2: a maintainer must decide explicitly in the PR; the changeset must name the lost behavior; + docs in sync. +- L1: verify the changeset and docs. +- L0: one sentence on why it is L0, with evidence. + +### Findings vs author confirmations + +- A finding needs both: `file:line` evidence on each side of the change, and a named population + with evidence it exists. Missing either one, demote to "author to confirm". When a whole file + is added or a whole implementation deleted, one side is naturally empty; give the existing side + the reachable affected `file:line`. +- At most 5 author confirmations, sorted by possible impact; report only the count beyond that. +- Do not report: problems that existed before the PR (a serious one gets a single "incidental + finding" line), problems lint and typecheck catch, style. +- None of these are evidence of "no impact": the author says "unchanged", no test references the + deleted sentence, the change is only in internal packages. + +## Pass 2: template section check + +- Related Issue: is the linked issue valid and relevant; when there is no issue, is the + requirement stated in one or two sentences? +- Problem: does it state the user need or limitation; is it consistent with the linked issue? +- What changed: does the description match the actual diff; are visual outlines (diff blocks, + call trees, file trees) accurate and helpful; is the approach sound with simpler alternatives + considered; which edge cases did the author miss? +- Behavior Changes and Affected Users: compare the author's table row by row with the + independent inventory from pass 3. A behavior the author missed is a finding; a row you cannot + reproduce goes to "author to confirm". Are populations named, or just "some users"? When the + table says `None`, does the evidence hold? Do affected modules and test coverage match the diff + file list; which risky paths are untested? +- Checklist: are all applicable boxes checked; do you agree with the "not needed" ones? ## Output -Write the review summary in English. For each section: +English, in this order: + +```markdown +## Verdict +One line: merge / changes needed / recommend blocking, plus the PR impact level and change class. + +## Regression and user impact +- Change class: ... +- Impact level: Lx (breadth: ...; silent: yes/no) + +| Behavior | Before | After | Affected population | Evidence | Level | Escape hatch | +|---|---|---|---|---|---|---| + +Remediation check: +- Escape hatch: ... +- Changeset names the lost behavior: ... +- Old data and old clients: ... +- Guard test: ... +- Docs: ... + +Author to confirm (max 5): +1. ... + +## Template section check +Section by section: adequately filled in, missing, inaccurate, or inconsistent with the diff. + +## Changes needed +Actionable list; write "none" if empty. +``` + +## Retrospective backfill -- State whether it is adequately filled in. -- Flag anything missing, inaccurate, or inconsistent with the diff. -- If the PR is ready, say so. If changes are needed, list them as actionable items. +After every regression post-mortem, add the newly discovered population or trigger clue to +`surfaces.md` and a line to its history table. An inventory that is not updated goes stale. diff --git a/.agents/skills/review-pr/surfaces.md b/.agents/skills/review-pr/surfaces.md new file mode 100644 index 000000000..a37bb7251 --- /dev/null +++ b/.agents/skills/review-pr/surfaces.md @@ -0,0 +1,130 @@ +# Populations and surfaces + +Repository-specific inventory for the `review-pr` regression pass: who depends on existing +behavior, and what a diff must touch before behaviors have to be enumerated. It goes stale; add a +line after every regression post-mortem (see `SKILL.md`, "Retrospective backfill"). + +## 1. Populations: who depends on existing behavior + +### Clients and entrypoints + +| Population | Notes | Contract location | +|---|---|---| +| TUI users | `pythinker` default entrypoint | `apps/pythinker-code/src/tui/` | +| print-mode users | `pythinker ""` headless runs, including scripts that parse `--output-format stream-json` output. Script parsers are a second-class population, but output-format changes must still be named in the changeset | `apps/pythinker-code/src/cli/` | +| desktop and web users | Both apps live in this repository (`apps/desktop`, `apps/pythinker-web`, shipped as the `apps/pythinker-code/dist-web` bundle) and consume the server's `/api/v1` REST and `/api/v1/ws`. A server-contract change must name the consumer apps and confirm their shipped clients against the previous release | `packages/agent-gateway/src/routes/`, `packages/agent-gateway/src/transport/ws/`, `docs/reference/server-api.md` | +| inspector users | `apps/pythinker-inspect` consumes the `/api/v1/debug` reflection RPC surface (loopback bind, bearer auth) | `packages/agent-gateway/src/transport/registerDebugRoutes.ts` | +| VS Code extension users | SDK event adapter; the webview has its own rendering contract | `apps/vscode/` | +| ACP client users | Zed and others, `pythinker acp`. stdio MCP is ACP's default transport and clients send it unconditionally; the capability matrix is a public promise | `packages/acp-server/`, `docs/reference/pythinker-acp.md` | +| SDK callers | `@pymodel/pythinker-code-sdk` export surface; provider implementations live behind kosong, which the CLI and the server also use | `packages/node-sdk/src/index.ts`, the `exports` map in `packages/node-sdk/package.json`, `packages/kosong/src/providers/` | +| remote-control users | `pythinker web --remote-control`; credential slot and relay follow the machine registration, behind a machine-wide single-instance lock | `packages/remote-control/` | + +### Provider dialects + +| Population | Notes | +|---|---| +| Official OAuth login | Default path; region handling | +| `openai` Chat Completions-compatible gateways | The messiest group: relay services, OpenRouter-shaped responses (`reasoning` string alongside a `reasoning_details` array), senders that only emit `reasoning_content`, strict history-message validation, third-party model `anyOf` handling | +| `anthropic`, `openai_responses`, `google-genai`, `vertexai` | Each with its own thinking, tool-call, and usage field shapes | +| Custom model configs | `api_key_env`, catalog imports, manually configured thinking parameters, `[secondary_model]` and the subagent model pool | + +Code location: `packages/agent-core-v2/src/human/llm/requester/bases/`. + +### Platforms and environments + +| Population | Known sensitivities | +|---|---| +| Windows | Deep paths and recursive watch pressure, drive roots and UNC paths, IME composition input, CSI-u keyboard sequences | +| Linux | Filesystems without hardlinks, root directories with huge file counts | +| macOS | The default development environment, most easily mistaken for "all users" | +| Network proxies | Background subagent retries | +| native binary vs npm install | Different startup paths and update mechanisms | +| install channels | npm package, per-platform native binaries, VS Code marketplace package, bundled web assets — all decided by release and packaging workflows; a platform can break without a single `src/` line changing | + +### Git repository shapes + +| Population | Known sensitivities | Contract location | +|---|---|---| +| users starting inside submodules or worktrees | `core.worktree` is set; with symlinked paths (macOS `/tmp`, a linked home, Windows mapped drives) realpath and literal paths disagree. A background git probe that fails here can silence the whole footer | `packages/agent-core-v2/src/app/git/gitService.ts` | +| git-lfs and git-crypt users | Filter drivers in `.git/config` carry `required = true`; any hardening that clears filter commands turns `git status` fatal on modified filtered files unless `required` is cleared too | same | +| large-repository users | Depend on `core.fsmonitor`; the footer's background git has a timeout budget and silently treats a timeout as clean | `packages/agent-core-v2/src/app/git/` | +| repositories requiring signed commits or hooks | Any automatic commit path (Tower) that disables hooks or signing changes the commit outcome | `packages/agent-core-v2/src/features/tower/` | + +### Configuration states + +| Population | Notes | Contract location | +|---|---|---| +| default-config users | Any default flip is a behavior change for them | `packages/agent-core-v2/docs/config-manifest.toml` | +| experimental-flag users | `[experimental]` config and `PYTHINKER_CODE_EXPERIMENTAL_*` env; flags are declared per domain via `registerFlagDefinition`; flipping a flag's `default` to true is a release to everyone | `packages/agent-core-v2/src/**/flag.ts` | +| legacy-flag users | `PYTHINKER_CODE_LEGACY_FLAG`; in the past the TUI and print mode silently ignored it | | +| custom-agent users | `--agent` policies such as `disallowedTools` | | +| users with hooks configured | Event names and payloads are a contract with external scripts; past regressions include the approval panel not rendering and calls being silently approved | `packages/agent-core-v2/src/features/externalHooks/`, `docs/customization/hooks.md` | +| skills, plugins, and MCP users | stdio MCP, user-level skill roots, `[watch]` hot reload (flipping it off also silences AGENTS.md change nudges; `/reload` covers only config and skills) | `docs/customization/` | +| environment-variable users | The same variable must behave identically across the TUI, print mode, and the server entrypoints | `docs/configuration/env-vars.md` | +| theme, keybinding, and TUI-mode customizers | `docs/customization/themes.md`, `docs/reference/keyboard.md` | | + +### Data eras + +| Population | Notes | Contract location | +|---|---|---| +| sessions written by older versions | Wire logs, turn numbering, compacted transcripts; whether new versions read them and whether a rollback still reads them | `packages/agent-core-v2/docs/wire-manifest.d.ts`, `packages/agent-core-v2/docs/state-manifest.d.ts` | +| migrated sessions | Imports from older data roots | `packages/migration-legacy/` | +| the search index | minidb snapshot and WAL | `packages/minidb/` | +| background tasks and cron persistence | Recovery after abnormal exit | | +| directory layout | Anything reading or writing `~/.pythinker-code/` | `docs/configuration/data-locations.md` | + +### External automation + +| Population | Contract | +|---|---| +| scripts parsing `stream-json` | Event shapes and ordering | +| hook scripts | Event names, payload fields, return-value semantics | +| export consumers | Export format | +| users with telemetry disabled | Opt-out honored on every entrypoint | + +### Model-behavior populations + +Prompt text changes affect all users, and no test can prove "no impact". Populations known to +depend on specific sentences: users depending on "do not touch files outside the working +directory"; users depending on "cwd is the project root"; plan-mode reminder cadence; Tower +session titles and fencing behavior. + +Prompt changes driven by external benchmark analysis trade one population's gain for another +population's loss; a PR must name both. + +## 2. Trigger clues: diff hits that force a behavior enumeration + +| Diff shows | Surface | Required check | +|---|---|---| +| `packages/agent-core-v2/src/app/agentProfileCatalog/system.md`, and any `packages/agent-core-v2/src/**/*.md` (tool descriptions, reminders, overlays, built-in skills) | model behavior | every deleted or edited sentence gets its own row: what the sentence enforced before, who depended on it, what enforces it now | +| `packages/agent-core-v2/docs/config-manifest.toml` (generated from `src/app/config/configSectionContributions.ts`, sync guarded by `test/app/config/configManifest.test.ts`) | config keys, defaults, validation | default flips and validation tightenings must be listed; changing the manifest without `docs/configuration/config-files.md` is stale docs | +| `packages/agent-core-v2/src/**/flag.ts` | experimental flags | a `default` flip to true is a release to everyone; treat as a behavior change | +| `packages/agent-core-v2/src/human/utils/watch.ts`, and any module-level default constant or `?? true` / `?? false` fallback that a config key overrides | default sources | flips must be listed; the config-path escape hatch needs a guard test, not only env; check the hatch does not depend on ConfigService being constructed before its consumers | +| `packages/agent-core-v2/docs/wire-manifest.d.ts`, `packages/agent-core-v2/docs/state-manifest.d.ts` | on-disk session format | can old sessions still be read, can the previous release read new data, is there a migration | +| `packages/agent-gateway/test/__snapshots__/apiSurface.snapshot.test.ts.snap`, `packages/agent-gateway/src/routes/`, `packages/agent-gateway/src/transport/ws/` | the server contract consumed by desktop, web, and the inspector | name the in-repo consumer apps; check whether previous-release clients still work against the new server | +| `packages/node-sdk/src/index.ts`, `packages/node-sdk/package.json` | SDK export surface | removals or signature changes are breaking | +| `packages/klient/src/` | the client facade under the SDK and ACP | a klient-only PR does not touch the SDK export surface; check behavior for SDK callers and ACP clients | +| `packages/transcript/src/contract/` | the transcript payload contract the server sends desktop, web, and VS Code | name the consumer apps; can previous-release clients parse the new payload | +| `.github/workflows/release.yml` and the native, vscode, and packaging workflows, `apps/pythinker-code/scripts/native/`, `apps/pythinker-code/scripts/check-web-assets.mjs` | release artifacts | check per install channel and platform: npm package contents, native binaries, marketplace package, bundled web assets still complete | +| `packages/acp-server/` | ACP clients | check each item of the capability matrix in `docs/reference/pythinker-acp.md` | +| `apps/pythinker-code/src/cli/` | CLI arguments, subcommands, `--output-format` | check against `docs/reference/pythinker-command.md` | +| `packages/agent-core-v2/src/features/externalHooks/` | hook events and payloads | check against `docs/customization/hooks.md` | +| `packages/agent-core-v2/src/human/llm/requester/bases/` | provider dialect branches | enumerate which provider payload shape takes which branch before and after, especially gateways that send multiple forms of a field | +| `packages/agent-core-v2/src/app/git/` | background git invocation, config overrides, timeouts | walk the git-shape populations row by row: does a failed probe silence all background git; do overridden config keys include ones that turn "disabled" into "failing" (`required`, `gpgSign`, `hooksPath`); are Windows cases skipped | +| `packages/migration-legacy/`, `packages/minidb/`, anything reading or writing the `~/.pythinker-code/` layout | data eras | is old data still readable and migratable | +| any added, removed, or re-semanticized environment-variable read, in any spelling (`process.env.X`, `process.env['X']`, dynamic keys, helpers) and any prefix | environment variables | check every entry in `docs/configuration/env-vars.md`; consistency across the TUI, print mode, and server entrypoints | +| `packages/agent-core-v2/src/app/scopes.ts`, config-ready and feature-assembly ordering | startup order and lifecycle | who depended on the old order | +| `docs/**` reference, configuration, and customization pages | public promises | behavior changed without docs, or docs changed without behavior, must be flagged either way | +| tests that rewrite an old assertion to the new behavior | direct evidence of a behavior change | map each to an inventory row | + +## 3. History: calibration for levels + +| Class | Case in this repository | What was missed | Level in hindsight | +|---|---|---|---| +| Default flip | the watch hot-reload default: a changeset named only the new default, not that editing config, skills, or AGENTS.md mid-session stopped applying and AGENTS.md change nudges went silent; the restore landed in #336 | the changeset never named the lost behavior; the config-path escape hatch had no guard test | L1 with an escape hatch, but a changed file having no effect with no signal is silent: effectively L2 | +| Safety tightening rolled back | the trust-boundary hardening batch: git probe and filter-driver hardening broke submodule and symlinked repository shapes and filtered-file status; reverted in #335 after review listed the broken populations; the local write-guard survived the revert | the landed review had not enumerated the git-shape populations up front | L3 candidates with no escape hatch; the population list is what caught it | +| Dialect branch narrowed | gateways exist that send both the string and the array form of a dual-shaped field; a parser that prefers the array stops reading the string, and the PR only said "the string is read when the array is absent" | nobody asked who sends both forms | L2 with no way back; the silently dropped input raises it to L3 for openai-compatible gateways | +| Refactor dropped a feature | a replaced path lost something the old path had: an env var, a fallback, an accepted input, a hook, a legacy flag; nobody inventoried the old path | the old path's feature list was never written down | L2 with no way back; silently ignored config raises to L3 | +| Fix changed startup timing | deferred assembly changed construction order; reverted within the day | nobody asked who depended on the old order | all users | +| Silent approval | with hooks configured, an approval surface stopped rendering and pending calls were approved silently | the hook-configured population was not in scope | L3, security boundary | +| Prompt sentence deleted | "no test references the sentence" was treated as evidence of no impact | the population receiving the prompt was never named | L2, no way back, all users; the change was openly visible, so not silent | diff --git a/.agents/skills/write-pr/SKILL.md b/.agents/skills/write-pr/SKILL.md index 14eedba4b..94ab4ba58 100644 --- a/.agents/skills/write-pr/SKILL.md +++ b/.agents/skills/write-pr/SKILL.md @@ -1,11 +1,13 @@ --- name: write-pr -description: Use when creating a pull request in the pythinker-code repository — how to fill in each section of the PR template with concise, reviewer-friendly content. +description: Use when creating or updating a pull request in this repository — how to fill in each section of the PR template with concise, reviewer-friendly content, including the behavior-change table. --- # Write PR Description -Create or update the pull request for the current branch with a description that helps the reviewer understand why the change exists and the shape of the implementation. +Create or update the pull request for the current branch with a description that helps the +reviewer understand why the change exists, the shape of the implementation, and what existing +users lose. ## Workflow @@ -17,86 +19,129 @@ Create or update the pull request for the current branch with a description that - Check the current branch for an existing PR: `gh pr view --json url,number,title,state 2>/dev/null`. - If no PR exists, inspect `git status --short --branch` and the commits on the current branch. - Commit remaining changes, push the branch with an upstream, and create the PR with `gh pr create`. - - Follow the repository's git safety protocol. + - Follow the repository's git safety rules: never commit to `main` directly; work lands through a PR. 3. Gather the context needed to explain the change: - Read the linked issue and any relevant task artifacts. - - Read the complete diff (`git diff main...HEAD`) and enough surrounding code to understand behavior and ownership. + - Read the complete diff (`git diff main...HEAD`) and enough surrounding code to understand + behavior and ownership. - Use `gh pr view` to collect PR metadata and changed files if the PR already exists. -4. Write the PR description following the template sections: - - **Requirement or Bug** — one sentence or `Resolve #`. Nothing more. - - **Bug Reproduction Steps** — bug PRs only; `N/A` for features. Write `See linked issue` when steps are already there. - - **Root Cause** — bug PRs only; `N/A` for features. State the cause and whether this is a fundamental fix or a workaround. - - **Code Changes** — use visual outline views (see below) instead of prose whenever they explain the change better. - - **Impact Scope** — list affected modules and test coverage. +4. Write the description following the template sections. The body is in English, the PR title + stays an English Conventional Commit, and changesets stay English (see the `gen-changesets` + skill); section headings follow the template verbatim: + - **Related Issue** — one line, or `Resolve #`. Nothing more. + - **Problem** — the user need or limitation in a sentence or two. Write `See linked issue` + when the issue already covers it. + - **What changed** — what you implemented and why the approach fits; prefer visual outline + views (see below) over prose whenever they explain the change better. + - **Behavior Changes and Affected Users** — fill the behavior table first, then list affected + modules and test coverage; see "Behavior Changes and Affected Users". - **Checklist** — check every box that applies. 5. Publish the description: - - Save to a temp file, then `gh pr edit --body-file ` or `gh pr create --body-file `. + - Save to a temp file, then `gh pr edit --body-file ` or + `gh pr create --body-file `. - Confirm the update succeeded. -## Visual Outline for Code Changes +## Behavior Changes and Affected Users -Prefer structural views over prose. Use the smallest combination that explains the implementation. Omit categories that did not change. +This section answers one question: after this merges, what do existing users lose? Most +regressions in this repository came from nobody answering it, not from logic errors. -Show logic or algorithm changes as pseudocode diff: +Criterion: **any input that worked before the change — a config key, an environment variable, a +command-line flag, a provider response shape, session data written by an older version, a client +request, or a hook payload — must behave the same after it, unless the change is declared here +with a way back.** + +How to write it: + +1. List every observable behavior the diff changes, one row each. Include behavior you consider + "unchanged" whose branch conditions moved: does a new condition now match existing inputs + before the old one; who reached a deleted or narrowed branch before. Internal packages ship in + the CLI release; "internal" does not mean invisible to users. The table shape (example in + English): + + | Behavior | Before | After | Who relies on the old behavior | Escape hatch | + |---|---|---|---|---| + | reasoning parsing when `reasoning_details` is an array | string `reasoning` also read | string ignored | OpenAI-compatible gateways that send both forms | none | + +2. Name populations, never "some users". Take them from + `.agents/skills/review-pr/surfaces.md`: clients (TUI, print mode, desktop, web, inspector, + VS Code, ACP, SDK), provider dialects, platforms, configuration states, data written by older + versions, external scripts. + +3. Changed prompt text (system prompt, tool descriptions, reminders): one row per sentence — what + the sentence enforced before, who relied on it, what enforces it now. "No test references the + sentence" is not evidence of no impact. + +4. Replacements and bypasses (refactors, runtime rebinding, protocol swaps): additionally list + the old path's feature inventory — which env vars it honored, which fallbacks it had, which + inputs it accepted — and where each item lives in the new path. + +5. Genuinely no observable change: write `None`, with evidence — which branches, defaults, and + contract files are untouched. + +6. After the table: affected modules and the test coverage for each row. A flipped default or a + removed behavior must have a test pinning the old behavior for the population that keeps it, + and a changeset naming what users lose (see the `gen-changesets` skill). When there is no way + back, ask a maintainer to approve it explicitly in the PR. + +## Visual Outline for What changed + +Prefer structural views over prose. Use the smallest combination that explains the +implementation. Omit categories that did not change. + +Show logic or algorithm changes as a pseudocode diff: ```diff - on(save) -- write content -+ if content is unchanged -+ return cached result -+ write new content -+ invalidate cache + on(save) +- persist immediately ++ debounce 300ms then persist ++ mark state dirty ``` Show runtime control flow as a call tree diff: ```diff - submitForm - createSession - persistPrompt -+ expandSkillMention - launchAgent -- navigateToSession -+ navigateToSession -+ subscribeToEvents + submitForm + validate + persist ++ trackAnalytics ++ if (subscribed) ++ subscribeToEvents ``` Show file responsibility changes as a shallow file tree diff: ```diff - src/ - ├── commands/ -+│ └── show-me.ts # expands the slash command - ├── sessions/ --└── transport.ts -+└── transport/ -+ ├── client.ts -+ └── stream.ts + src/ + session/ + store.ts ++ selectors.ts ++ stream/ ++ parse.ts ``` Show component or UI structure changes as a tree diff: ```diff - - useSessionEvents() - -+ - -+ + + ++ + ++ + ``` -Show component interaction, control flow, or data flow with Mermaid (especially useful for explaining bug mechanics): +Show component interaction, control flow, or data flow with Mermaid (especially useful for +explaining bug mechanics): ```mermaid sequenceDiagram - participant User - participant UI - participant Daemon - User->>UI: choose command - UI->>Daemon: send expanded prompt + UI->>Daemon: submit + Daemon->>Worker: stream + Worker-->>Daemon: chunk Daemon-->>UI: stream result ``` @@ -104,13 +149,15 @@ Show key data structure or type changes in a language-specific block: ```ts interface SessionEvents { - onTurnStart(cb: (turn: Turn) => void): void; - onTurnEnd(cb: (turn: Turn) => void): void; + delta: string; + done: boolean; } ``` Rules for visual outlines: - Use `diff` blocks when the point is what changes and the surrounding shape already exists. -- Show the complete target shape in a language-specific or `text` block when most of it is new or diff notation would obscure ownership or order. -- Tell the story in the order that makes it easiest to understand — files first, or data structures first, whichever fits. +- Show the complete target shape in a language-specific or `text` block when most of it is new or + diff notation would obscure ownership or order. +- Tell the story in the order that makes it easiest to understand — files first, or data + structures first, whichever fits. - Write as one human talking to another: simple, coherent, concise language. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 3d3acf8be..4e6a78ab3 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -24,15 +24,21 @@ See https://github.com/PyModel/pythinker-code/blob/main/CONTRIBUTING.md for more -## Impact Scope +## Behavior Changes and Affected Users - + ## Checklist - [ ] I have read the [CONTRIBUTING](https://github.com/PyModel/pythinker-code/blob/main/CONTRIBUTING.md) document. - [ ] I have linked a related issue (external PRs: issue must have a maintainer's `/approve`). - [ ] I have added tests that prove my feature works. +- [ ] The behavior-change table above is complete, and every removed behavior or flipped default is named in the changeset and either has an escape hatch or was explicitly approved by a maintainer in this PR. - [ ] Ran `gen-changesets` skill, or this PR needs no changeset. - [ ] Ran `gen-docs` skill, or this PR needs no doc update. diff --git a/AGENTS.md b/AGENTS.md index bdaf8547d..a291c245b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,6 +128,34 @@ Gate behind flags. Env: `PYTHINKER_CODE_EXPERIMENTAL_` toggles one; `PYTHI - Rules that only affect a specific directory: update the nearest sub-directory `AGENTS.md`. - Project-map entries stay at 1–2 sentences; deep package docs live in the package's own `AGENTS.md`. +## Code Review Rules + +These rules apply to every pull request review, automated or human. The user populations and contract files they refer to are listed in `.agents/skills/review-pr/surfaces.md`. + +### Enumerate changed behavior, not just bugs + +Any input that worked before the change — a config key, env var, CLI flag, provider response shape, session written by an older version, client request, or hook payload — must behave the same after it unless the PR declares the change. For every deleted or narrowed branch, condition, default, or prompt sentence, ask who reached it before and where they go now; for every new condition, ask which existing inputs now match it first. Flag a PR that calls a path "unchanged" when its branch condition moved. + +### A flipped default or removed behavior needs a named loss and an escape hatch + +Everyone on the old default is affected. Require the changeset to name the behavior users lose, not only the new default; a config, env, or flag escape hatch or a maintainer's explicit sign-off in the PR; and a test that pins the old behavior for the population that keeps it. + +### Prompt text is behavior + +Editing or deleting sentences under `packages/agent-core-v2/src/**/*.md` (system prompt, tool descriptions, reminders, overlays, built-in skills) changes agent behavior for every user who receives that prompt. "No test references the sentence" is not evidence of no impact. Require the PR to name the population that receives the text (every session, plan mode, a flag-gated feature such as Tower), what the sentence enforced, who relied on it, and what enforces it now. + +### Contract files are tripwires + +The manifests under `packages/agent-core-v2/docs/` (`config-manifest.toml`, `wire-manifest.d.ts`, `state-manifest.d.ts`), `packages/agent-gateway/test/__snapshots__/apiSurface.snapshot.test.ts.snap`, `packages/node-sdk/src/index.ts`, `packages/agent-core-v2/src/features/externalHooks/`, `packages/acp-server/`, and `apps/pythinker-code/src/cli/` are consumed outside the packages that define them: the desktop and web apps in this repository, the inspector app, the VS Code extension, ACP clients such as Zed, SDK users, hook scripts, and headless-output parsers. When they change, require the PR to name the consumers and how data and clients from the previous release keep working. + +### Ports and refactors carry the old path's feature inventory + +When a change replaces or bypasses an existing path, require a list of what the old path did — env vars honored, fallbacks, accepted inputs — and where each item lives in the new path. A silently dropped item is a regression, not a cleanup. + +### Silent failure outranks a crash + +A change that makes the product silently ignore configuration, silently approve or skip an action, or silently drop data is the most severe finding: users get no signal to report. + # GitNexus — Code Intelligence From 2e1fb595afd14b9b8b9b533e98417315593d8365 Mon Sep 17 00:00:00 2001 From: elkaix Date: Thu, 1 Oct 2026 06:59:56 -0400 Subject: [PATCH 2/9] feat(tower): interrupt tower wake turns on user input and harden tower operations --- .../src/agent/loop/loopService.ts | 3 +- .../injection/tower-mode-full-reminder.md | 6 +- .../src/features/tower/protocol/store.ts | 1316 ++++++++++------- .../src/features/tower/protocol/types.ts | 1 + .../src/features/tower/tools/inbox/inbox.md | 2 +- .../features/tower/tools/inbox/inboxTool.ts | 1 + .../features/tower/tools/mission/mission.md | 2 +- .../features/tower/tools/spawn/spawnTool.ts | 24 +- .../features/tower/tools/status/statusTool.ts | 16 +- .../features/tower/tools/teardown/teardown.md | 10 +- .../features/tower/tools/teardown/teardown.ts | 16 +- .../tower/tools/teardown/teardownTool.ts | 21 +- .../src/features/tower/towerService.ts | 98 +- .../test/agent/loop/loop.test.ts | 19 + .../test/features/tower/store.test.ts | 389 ++++- .../features/tower/tools/spawnTool.test.ts | 36 +- .../features/tower/tools/towerTools.test.ts | 174 ++- .../test/features/tower/towerService.test.ts | 319 +++- .../test/modelCatalogCatalog.test.ts | 14 +- 19 files changed, 1855 insertions(+), 612 deletions(-) diff --git a/packages/agent-core-v2/src/agent/loop/loopService.ts b/packages/agent-core-v2/src/agent/loop/loopService.ts index daeb09775..44cbb000a 100644 --- a/packages/agent-core-v2/src/agent/loop/loopService.ts +++ b/packages/agent-core-v2/src/agent/loop/loopService.ts @@ -1093,7 +1093,8 @@ export class AgentLoopService extends Disposable implements IAgentLoopService { message: { role: 'user', content: [...seededMessage.content] }, meta: { promptId: waiter.id, origin: seededMessage.origin, tracked: false }, }; - this.beginActiveTurn(waiter, entry, pending.id); + const seededTurn = this.beginActiveTurn(waiter, entry, pending.id); + this.settlePromptLaunched(waiter, seededTurn); return true; } diff --git a/packages/agent-core-v2/src/features/tower/injection/tower-mode-full-reminder.md b/packages/agent-core-v2/src/features/tower/injection/tower-mode-full-reminder.md index e4d1ec395..8861e0d3b 100644 --- a/packages/agent-core-v2/src/features/tower/injection/tower-mode-full-reminder.md +++ b/packages/agent-core-v2/src/features/tower/injection/tower-mode-full-reminder.md @@ -17,16 +17,18 @@ Working principles: ## Tower workflow 1. **Init** — `TowerInit` creates `.tower/` and records the base branch (already set up when the human enabled tower mode with `/tower `). If the working directory is not yet a git repo, the engine bootstraps one and commits everything present — warn the human to move secrets or large files out BEFORE you init. Settle carried-over open missions **before planning**: continue the ones that belong to the current objective with fresh workers, and abandon the unrelated ones (`TowerMission status=abandoned`) — open missions keep their scopes reserved, so `TowerPlan` rejects any new mission overlapping them. -2. **Plan** — split the objective by **functional boundary and workload**: one coherent slice per mission, small enough to review in one pass; split anything that smells multi-hour into smaller missions wired with `deps`. Prefer more, smaller missions — there is no fixed count. Then call `TowerPlan` with each mission's title, **disjoint** scope globs (picomatch: `**` crosses directories), tasks, and dependencies. Titles must be **printable ASCII English** — any non-ASCII character is rejected and forces a re-plan. Give every title a unique identifier word (a business code, a module name, a ticket id). Write tasks as **verifiable** items a reviewer can map to the diff, and when the human's own words carry intent your paraphrase could lose, copy the key sentences into the mission's `context` **verbatim** — when in doubt, include it. `context` supplements your paraphrase (never replaces it, never holds the full conversation history) and is the one channel that carries the human's voice to both worker and reviewer. Mark read-only investigation missions `kind: "survey"`: a survey's scope is informational (it reserves nothing, so surveys and builds may overlap the same paths), the worker must not change code, and it closes with a zero-diff `TowerMerge` — no reviewer needed. Shared files (lockfiles, central configs) belong to exactly one build mission or to your own integration work. Post the plan to the human in one compact message and launch immediately — their words are plan changes, never a gate. +2. **Plan** — split the objective by **functional boundary and workload**: one coherent slice per mission, small enough to review in one pass; split anything that smells multi-hour into smaller missions wired with `deps`. Prefer more, smaller missions — there is no fixed count. Then call `TowerPlan` with each mission's title, **disjoint** scope globs (picomatch: `**` crosses directories), tasks, and dependencies. Titles must be **printable ASCII English** — any non-ASCII character is rejected and forces a re-plan. Give every title a unique identifier word (a business code, a module name, a ticket id). Write tasks as **verifiable** items a reviewer can map to the diff, and when the human's own words carry intent your paraphrase could lose, copy the key sentences into the mission's `context` **verbatim** — when in doubt, include it. `context` supplements your paraphrase (never replaces it, never holds the full conversation history) and is the one channel that carries the human's voice to both worker and reviewer. If the human's message carries attachments (images, logs, archives), quote their `pythinker-file://` references or absolute paths in the mission's `context` too — workers resolve them with Read/ReadMediaFile from the session's attachment storage. Quote the bare attachment id without the file extension (e.g. `pythinker-file://f_7fbdcf8f-8b4f-4653-8141-a44f160e1228`, not `.../image.png`) — the extended form does not resolve. Mark read-only investigation missions `kind: "survey"`: a survey's scope is informational (it reserves nothing, so surveys and builds may overlap the same paths), the worker must not change code, and it closes with a zero-diff `TowerMerge` — no reviewer needed. Shared files (lockfiles, central configs) belong to exactly one build mission or to your own integration work. Post the plan to the human in one compact message and launch immediately — their words are plan changes, never a gate. 3. **Spawn** — one `TowerSpawn` per mission (`kind: "worker"`, background, code-built briefing), and **spawn every dependency-unblocked mission right away**: fire the `TowerSpawn` calls back to back, never trickle them out one at a time and never wait for one worker before launching the next — the fleet exists to run in parallel. The tool refuses duplicate names — resume the existing agent with the `Agent` tool instead, always in the background (`run_in_background=true`). Workers commit on their branch; their completion wakes you. Once the batch is running, **end your turn**: completions and inbox traffic arrive as notifications, so never poll `TowerInbox`/`TowerStatus` in a loop and never sit synchronously waiting on a worker. 4. **Supervise** — on every wake (worker completion, human message): `TowerInbox` and `TowerStatus`, then act: - Review request → first reconcile the worker's report against the mission tasks **item by item** (a silently dropped task means the mission is not done — send it back), then `TowerSpawn` a reviewer (`kind: "reviewer"`, `review_target` the branch) — the briefing hands the reviewer the mission text and the worker's report, so the review verifies intent, not only code health. Do not review mission code yourself. Survey missions skip review — close them with `TowerMerge` once their summary lands. - Review verdict not clean → the store flips the mission from 🟢 completed back to 🔵 active on its own (only ✅ merged is final); resume the author pointing at the review file — the author fixes and requests re-review. Round cap: at 5 rounds, or when two consecutive rounds report the same findings, stop the loop, inform the human, and redirect (reassign, split, descope). - Blocker → answer or reassign if you can; if it genuinely needs the human, inform them and keep the rest moving. - Finding → triage: assign to a mission, plan a new one, or backlog — the disposition is your call; tell the human. + - Requirement change for a busy worker → `TowerSend` is delivery, not interruption: a worker mid-turn (long build, test run, sleep) cannot see inbox messages until its turn ends. The store refuses `status=completed` while unread messages remain, so the change is caught before delivery — no silent miss is possible. - Completion report with a suspicious diff (🟢 claimed, zero changed files) → investigate before accepting. + - Dead roster agent → an unintended death (failed, lost, timed out) is a symptom, not a cue to revive: diagnose first — check why it died (the died entry's status/reason, its task state); a blind revive re-kills the worker and destroys the death scene. Resume with `Agent(resume=..., run_in_background=true)` or reassign the mission only when the cause is transient (lost contact, timeout, OOM); a systematic cause (code or environment defect) is fixed or escalated to the human before any revive. A worker or reviewer **the human stopped** is dead by intent — never resume it, never reassign its mission, never spawn a replacement; leave it stopped unless the human says otherwise. 5. **Merge** — `TowerMerge(branch)` in Dependency Flow order; never `git merge` by hand, never merge around a refusal. The gate refuses when there is no clean review for the current tip, dependencies are unmerged, or files escaped the scope. After a merge, the result lists branches that now conflict: tell those workers to rebase onto the new base, resolve, and request re-review; their moved tip makes the gate demand a fresh clean review. -6. **Teardown promptly** — when `TowerStatus` shows every mission ✅ merged and no unactioned inbox items remain, summarize what every worker produced (per mission: what was built, the branch and its merge outcome, anything left undone) and call `TowerTeardown` **right away** — never tear down without that summary. Branches and `.tower/comms/` are kept and dirty worktrees are protected — only disk is freed. Teardown does **not** exit tower mode — you remain the tower, ready for the next objective, until the human turns the mode off with `/tower off`. A `/tower teardown` from the human is the same instruction at any earlier point. +6. **Teardown promptly** — when `TowerStatus` shows every mission ✅ merged and no unactioned inbox items remain, summarize what every worker produced (per mission: what was built, the branch and its merge outcome, anything left undone) and call `TowerTeardown` **right away** — never tear down without that summary. Branches and `.tower/comms/` are kept, and a worktree is protected while its agent still has a running task, while it holds uncommitted changes (overridable with `force` — the live-agent protection is not), or when you name it in `exclude`; `dry_run` previews the exact remove/keep list first. Teardown does **not** exit tower mode — you remain the tower, ready for the next objective, until the human turns the mode off with `/tower off`. A `/tower teardown` from the human is the same instruction at any earlier point. ## Hard rules for the tower diff --git a/packages/agent-core-v2/src/features/tower/protocol/store.ts b/packages/agent-core-v2/src/features/tower/protocol/store.ts index 6995bc844..a65041977 100644 --- a/packages/agent-core-v2/src/features/tower/protocol/store.ts +++ b/packages/agent-core-v2/src/features/tower/protocol/store.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto'; -import { appendFile, mkdir, open, readFile, readdir, rename, stat, writeFile } from 'node:fs/promises'; +import { appendFile, mkdir, open, readFile, readdir, rename, rm, stat, writeFile } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import picomatch from 'picomatch'; @@ -135,6 +135,18 @@ export interface TowerAddWorktreeResult { readonly spawnBase?: string; } +export interface TowerStoreOptions { + readonly stateLockTimeoutMs?: number; + readonly stateLockPollMs?: number; +} + +export interface TowerTeardownOptions { + readonly force?: boolean; + readonly exclude?: readonly string[]; + readonly dryRun?: boolean; + readonly liveAgentIds?: ReadonlySet; +} + const FINDING_TYPES: readonly TowerFindingType[] = ['bug', 'improve', 'vuln', 'idea']; const STATUS_EMOJI: Record = { planned: '🟡', @@ -182,17 +194,79 @@ export async function assertLocalBaseBranch(repoRoot: string, base: string): Pro } export class TowerStore { - private mutation: Promise = Promise.resolve(); + private readonly stateLockTimeoutMs: number; + private readonly stateLockPollMs: number; + + constructor( + readonly repoRoot: string, + options: TowerStoreOptions = {}, + ) { + this.stateLockTimeoutMs = options.stateLockTimeoutMs ?? 10_000; + this.stateLockPollMs = options.stateLockPollMs ?? 10; + } - constructor(readonly repoRoot: string) {} + private async withStateLock(fn: () => Promise): Promise { + const lockPath = `${this.abs(STATE_FILE)}.lock`; + const deadline = Date.now() + this.stateLockTimeoutMs; + const token = randomUUID(); + while (!(await this.tryAcquireStateLock(lockPath, token))) { + if (Date.now() >= deadline) { + let holder = ''; + try { + holder = ` — held by ${(await readFile(lockPath, 'utf8')).trim()}`; + } catch { + } + throw new TowerProtocolError( + `timed out after ${String(this.stateLockTimeoutMs)}ms waiting for the tower state lock "${lockPath}"${holder} — another tower process is writing state.json; if no tower process is alive, delete the stale lock file and retry`, + ); + } + await new Promise((resolve) => { + setTimeout(resolve, this.stateLockPollMs); + }); + } + try { + return await fn(); + } finally { + await this.releaseStateLock(lockPath, token); + } + } - private exclusive(work: () => Promise): Promise { - const run = this.mutation.then(work, work); - this.mutation = run.then( - () => undefined, - () => undefined, - ); - return run; + private async releaseStateLock(lockPath: string, token: string): Promise { + let content: string; + try { + content = await readFile(lockPath, 'utf8'); + } catch { + return; + } + if (!content.includes(`token=${token}`)) return; + await rm(lockPath, { force: true }); + } + + private async tryAcquireStateLock(lockPath: string, token: string): Promise { + try { + const handle = await open(lockPath, 'wx'); + try { + await handle.writeFile( + `pid=${String(process.pid)} since=${new Date().toISOString()} token=${token}`, + 'utf8', + ); + return true; + } catch (error) { + await rm(lockPath, { force: true }); + throw error; + } finally { + await handle.close(); + } + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'EEXIST') return false; + if (code === 'ENOENT') { + throw new TowerProtocolError( + 'tower is not initialized in this repository — run TowerInit first', + ); + } + throw error; + } } async isInitialized(): Promise { @@ -225,76 +299,77 @@ export class TowerStore { } async init(sessionId?: string, base?: string): Promise { - return this.exclusive(async () => { await this.ensureRepository(base); if (!(await hasAnyCommit(this.repoRoot))) { throw new TowerProtocolError( 'the repository has no commits yet — create an initial commit first', ); } - if (await this.isInitialized()) { - const state = await this.load(); - const retiredAgents = await this.adoptForeignRoster(state, sessionId); - return { - base: state.base, - created: false, - retiredAgents, - checkout: await this.checkedOutBranch(), - ignoredBase: base !== undefined && base !== state.base ? base : undefined, - openMissions: state.missions.filter(isOpenMission).map((m) => m.id), - }; - } + await mkdir(dirname(this.abs(STATE_FILE)), { recursive: true }); + return this.withStateLock(async () => { + if (await this.isInitialized()) { + const state = await this.load(); + const retiredAgents = await this.adoptForeignRoster(state, sessionId); + return { + base: state.base, + created: false, + retiredAgents, + checkout: await this.checkedOutBranch(), + ignoredBase: base !== undefined && base !== state.base ? base : undefined, + openMissions: state.missions.filter(isOpenMission).map((m) => m.id), + }; + } - const checkout = await this.checkedOutBranch(); - let resolvedBase: string; - if (base !== undefined) { - await assertLocalBaseBranch(this.repoRoot, base); - resolvedBase = base; - } else { - if (checkout === 'HEAD') { - throw new TowerProtocolError( - 'cannot determine the base branch from a detached HEAD — pass the base branch explicitly', - ); + const checkout = await this.checkedOutBranch(); + let resolvedBase: string; + if (base !== undefined) { + await assertLocalBaseBranch(this.repoRoot, base); + resolvedBase = base; + } else { + if (checkout === 'HEAD') { + throw new TowerProtocolError( + 'cannot determine the base branch from a detached HEAD — pass the base branch explicitly', + ); + } + resolvedBase = checkout; } - resolvedBase = checkout; - } - for (const dir of [INBOX_DIR, FINDINGS_DIR, REVIEWS_DIR, MISSIONS_DIR, LOG_DIR, WORKTREES_DIR]) { - await mkdir(this.abs(dir), { recursive: true }); - } - await this.ensureGitExclude(); - - const state: TowerState = { - version: 1, - base: resolvedBase, - mode: 'branch', - createdAt: new Date().toISOString(), - sessionId, - roster: { agents: [] }, - missions: [], - }; - await this.save(state); - await writeFile(this.abs(ACTIVITY_LOG), '', 'utf8'); - await this.renderMissionsIndex(state); - await this.appendLog(TOWER_NAME, 'init', { mode: state.mode, base: resolvedBase }, MISSIONS_INDEX); - return { base: resolvedBase, created: true, retiredAgents: [], checkout, openMissions: [] }; + for (const dir of [INBOX_DIR, FINDINGS_DIR, REVIEWS_DIR, MISSIONS_DIR, LOG_DIR, WORKTREES_DIR]) { + await mkdir(this.abs(dir), { recursive: true }); + } + await this.ensureGitExclude(); + + const state: TowerState = { + version: 1, + base: resolvedBase, + mode: 'branch', + createdAt: new Date().toISOString(), + sessionId, + roster: { agents: [] }, + missions: [], + }; + await this.save(state); + await writeFile(this.abs(ACTIVITY_LOG), '', 'utf8'); + await this.renderMissionsIndex(state); + await this.appendLog(TOWER_NAME, 'init', { mode: state.mode, base: resolvedBase }, MISSIONS_INDEX); + return { base: resolvedBase, created: true, retiredAgents: [], checkout, openMissions: [] }; }); } async rebase(base: string): Promise { - return this.exclusive(async () => { - const state = await this.load(); - if (state.base === base) return; - const open = state.missions.filter(isOpenMission); - if (open.length > 0) { - throw new TowerProtocolError( - `cannot rebase the tower from "${state.base}" to "${base}" — ${String(open.length)} mission(s) are still open (${open.map((m) => m.id).join(', ')}); merge or abandon them first (or TowerTeardown and start over)`, - ); - } - await assertLocalBaseBranch(this.repoRoot, base); - const from = state.base; - await this.save({ ...state, base }); - await this.appendLog(TOWER_NAME, 'rebase', { from, to: base }); + await this.withStateLock(async () => { + const state = await this.load(); + if (state.base === base) return; + const open = state.missions.filter(isOpenMission); + if (open.length > 0) { + throw new TowerProtocolError( + `cannot rebase the tower from "${state.base}" to "${base}" — ${String(open.length)} mission(s) are still open (${open.map((m) => m.id).join(', ')}); merge or abandon them first (or TowerTeardown and start over)`, + ); + } + await assertLocalBaseBranch(this.repoRoot, base); + const from = state.base; + await this.save({ ...state, base }); + await this.appendLog(TOWER_NAME, 'rebase', { from, to: base }); }); } @@ -325,26 +400,26 @@ export class TowerStore { } async adopt(sessionId: string): Promise { - return this.exclusive(async () => { try { await readFile(this.abs(STATE_FILE), 'utf8'); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return []; throw error; } - const state = await this.load(); - return this.adoptForeignRoster(state, sessionId); + return this.withStateLock(async () => { + const state = await this.load(); + return this.adoptForeignRoster(state, sessionId); }); } async release(sessionId: string): Promise { - return this.exclusive(async () => { if (!(await this.isInitialized())) return; - const state = await this.load(); - if (state.sessionId !== sessionId) return; - state.sessionId = undefined; - await this.save(state); - await this.appendLog(TOWER_NAME, 'release', { session: sessionId }); + await this.withStateLock(async () => { + const state = await this.load(); + if (state.sessionId !== sessionId) return; + state.sessionId = undefined; + await this.save(state); + await this.appendLog(TOWER_NAME, 'release', { session: sessionId }); }); } @@ -379,9 +454,14 @@ export class TowerStore { private async save(state: TowerState): Promise { const file = this.abs(STATE_FILE); - const tmp = `${file}.tmp`; - await writeFile(tmp, `${JSON.stringify(state, null, 2)}\n`, 'utf8'); - await rename(tmp, file); + const tmp = `${file}.tmp-${String(process.pid)}-${randomUUID()}`; + try { + await writeFile(tmp, `${JSON.stringify(state, null, 2)}\n`, 'utf8'); + await rename(tmp, file); + } catch (error) { + await rm(tmp, { force: true }); + throw error; + } } async appendLog( @@ -439,28 +519,31 @@ export class TowerStore { } async registerAgent(entry: TowerRosterEntry): Promise { - return this.exclusive(async () => { - const state = await this.load(); - if (entry.name.trim().length === 0 || entry.name.trim() !== entry.name) { - throw new TowerProtocolError( - `tower agent name "${entry.name}" must not be blank or carry surrounding whitespace`, - ); - } - if (isReservedTowerAgentName(entry.name)) { - throw new TowerProtocolError( - `tower agent name "${entry.name}" is reserved by the tower protocol — pick a different name`, - ); - } - for (let index = state.roster.agents.length - 1; index >= 0; index -= 1) { - if (state.roster.agents[index]!.agentId === entry.agentId) { - state.roster.agents.splice(index, 1); + await this.withStateLock(async () => { + const state = await this.load(); + if (entry.name.trim().length === 0 || entry.name.trim() !== entry.name) { + throw new TowerProtocolError( + `tower agent name "${entry.name}" must not be blank or carry surrounding whitespace`, + ); } - } - if (this.findAgent(state, entry.name) !== undefined) { - throw new TowerProtocolError(`tower agent name "${entry.name}" is already registered`); - } - state.roster.agents.push(entry); - await this.save(state); + if (isReservedTowerAgentName(entry.name)) { + throw new TowerProtocolError( + `tower agent name "${entry.name}" is reserved by the tower protocol — pick a different name`, + ); + } + for (let index = state.roster.agents.length - 1; index >= 0; index -= 1) { + if (state.roster.agents[index]!.agentId === entry.agentId) { + state.roster.agents.splice(index, 1); + } + } + if (this.findAgent(state, entry.name) !== undefined) { + throw new TowerProtocolError(`tower agent name "${entry.name}" is already registered`); + } + state.roster.agents.push({ + ...entry, + lastInboxReadAt: entry.lastInboxReadAt ?? entry.spawnedAt, + }); + await this.save(state); }); } @@ -468,65 +551,75 @@ export class TowerStore { agentId: string, status: string, reason?: string, + sessionId?: string, ): Promise { - return this.exclusive(async () => { - const state = await this.load(); - const index = state.roster.agents.findLastIndex((agent) => agent.agentId === agentId); - const existing = state.roster.agents[index]; - if (existing === undefined) return undefined; - if (existing.diedAt !== undefined) return existing; - const entry: TowerRosterEntry = { - ...existing, - diedAt: new Date().toISOString(), - deathStatus: status, - deathReason: reason, - }; - state.roster.agents[index] = entry; - await this.save(state); - const mission = state.missions.find((m) => m.id === entry.missionId); - await this.appendLog( - TOWER_NAME, - 'died', - { - name: entry.name, - agent: agentId, - kind: entry.kind, - status, - reason: reason === undefined ? undefined : reason.replaceAll(/\s+/g, ' ').slice(0, 200), - mission: entry.missionId, - target: entry.reviewTarget, - }, - mission !== undefined ? join(MISSIONS_DIR, missionFileName(mission.id, mission.slug)) : undefined, - ); - return entry; + return this.withStateLock(async () => { + const state = await this.load(); + if (sessionId !== undefined && state.sessionId !== undefined && state.sessionId !== sessionId) { + return undefined; + } + const index = state.roster.agents.findLastIndex((agent) => agent.agentId === agentId); + const existing = state.roster.agents[index]; + if (existing === undefined) return undefined; + if (existing.diedAt !== undefined) return existing; + const entry: TowerRosterEntry = { + ...existing, + diedAt: new Date().toISOString(), + deathStatus: status, + deathReason: reason, + }; + state.roster.agents[index] = entry; + await this.save(state); + const mission = state.missions.find((m) => m.id === entry.missionId); + await this.appendLog( + TOWER_NAME, + 'died', + { + name: entry.name, + agent: agentId, + kind: entry.kind, + status, + reason: reason === undefined ? undefined : reason.replaceAll(/\s+/g, ' ').slice(0, 200), + mission: entry.missionId, + target: entry.reviewTarget, + session: sessionId, + pid: process.pid, + }, + mission !== undefined ? join(MISSIONS_DIR, missionFileName(mission.id, mission.slug)) : undefined, + ); + return entry; }); } - async clearAgentDied(agentId: string): Promise { - return this.exclusive(async () => { - const state = await this.load(); - const index = state.roster.agents.findLastIndex((agent) => agent.agentId === agentId); - const existing = state.roster.agents[index]; - if (existing === undefined || existing.diedAt === undefined) return false; - const entry: TowerRosterEntry = { - ...existing, - diedAt: undefined, - deathStatus: undefined, - deathReason: undefined, - }; - state.roster.agents[index] = entry; - await this.save(state); - await this.appendLog(TOWER_NAME, 'revived', { - name: entry.name, - agent: agentId, - kind: entry.kind, - }); - return true; + async clearAgentDied(agentId: string, sessionId?: string): Promise { + return this.withStateLock(async () => { + const state = await this.load(); + if (sessionId !== undefined && state.sessionId !== undefined && state.sessionId !== sessionId) { + return false; + } + const index = state.roster.agents.findLastIndex((agent) => agent.agentId === agentId); + const existing = state.roster.agents[index]; + if (existing === undefined || existing.diedAt === undefined) return false; + const entry: TowerRosterEntry = { + ...existing, + diedAt: undefined, + deathStatus: undefined, + deathReason: undefined, + }; + state.roster.agents[index] = entry; + await this.save(state); + await this.appendLog(TOWER_NAME, 'revived', { + name: entry.name, + agent: agentId, + kind: entry.kind, + session: sessionId, + pid: process.pid, + }); + return true; }); } async plan(input: readonly TowerPlanInput[]): Promise { - return this.exclusive(async () => { if (input.length === 0) { throw new TowerProtocolError('TowerPlan needs at least one mission'); } @@ -538,75 +631,76 @@ export class TowerStore { ); } } - const state = await this.load(); - const startIndex = state.missions.length; - - const missions: TowerMission[] = input.map((item, index) => { - const n = startIndex + index + 1; - const slug = slugify(item.title, 40); - return { - id: `M${n}`, - title: item.title, - slug, - kind: item.kind ?? 'build', - scope: [...item.scope], - branch: `feat/${slug}`, - worktree: `wt-${n}`, - deps: item.deps ?? [], - status: 'planned', - context: - item.context !== undefined && item.context.trim().length > 0 - ? item.context.trim() - : undefined, - tasks: (item.tasks ?? []).map((text) => ({ text, done: false })), - notes: [], - blockers: [], - }; - }); + return this.withStateLock(async () => { + const state = await this.load(); + const startIndex = state.missions.length; + + const missions: TowerMission[] = input.map((item, index) => { + const n = startIndex + index + 1; + const slug = slugify(item.title, 40); + return { + id: `M${n}`, + title: item.title, + slug, + kind: item.kind ?? 'build', + scope: [...item.scope], + branch: `feat/${slug}`, + worktree: `wt-${n}`, + deps: item.deps ?? [], + status: 'planned', + context: + item.context !== undefined && item.context.trim().length > 0 + ? item.context.trim() + : undefined, + tasks: (item.tasks ?? []).map((text) => ({ text, done: false })), + notes: [], + blockers: [], + }; + }); - const knownIds = new Set([...state.missions.map((m) => m.id), ...missions.map((m) => m.id)]); - for (const mission of missions) { - for (const dep of mission.deps) { - if (!knownIds.has(dep)) { - throw new TowerProtocolError(`mission ${mission.id} depends on unknown mission "${dep}"`); + const knownIds = new Set([...state.missions.map((m) => m.id), ...missions.map((m) => m.id)]); + for (const mission of missions) { + for (const dep of mission.deps) { + if (!knownIds.has(dep)) { + throw new TowerProtocolError(`mission ${mission.id} depends on unknown mission "${dep}"`); + } } } - } - const takenBranches = new Map( - state.missions.map((m): [string, TowerMission] => [m.branch, m]), - ); - for (const mission of missions) { - const existing = takenBranches.get(mission.branch); - if (existing !== undefined) { - throw new TowerProtocolError( - `mission ${mission.id} branch "${mission.branch}" is already used by ${existing.id} (${existing.status}) "${existing.title}" — change the title so its slug differs; branch-to-mission resolution must stay unambiguous`, - ); - } - if (await branchExists(this.repoRoot, mission.branch)) { - throw new TowerProtocolError( - `mission ${mission.id} branch "${mission.branch}" already exists in git but is not owned by any tower mission — the worker would start on that branch's unrelated history; change the title so its slug differs, or delete/rename the stale branch if it is a leftover`, - ); + const takenBranches = new Map( + state.missions.map((m): [string, TowerMission] => [m.branch, m]), + ); + for (const mission of missions) { + const existing = takenBranches.get(mission.branch); + if (existing !== undefined) { + throw new TowerProtocolError( + `mission ${mission.id} branch "${mission.branch}" is already used by ${existing.id} (${existing.status}) "${existing.title}" — change the title so its slug differs; branch-to-mission resolution must stay unambiguous`, + ); + } + if (await branchExists(this.repoRoot, mission.branch)) { + throw new TowerProtocolError( + `mission ${mission.id} branch "${mission.branch}" already exists in git but is not owned by any tower mission — the worker would start on that branch's unrelated history; change the title so its slug differs, or delete/rename the stale branch if it is a leftover`, + ); + } + takenBranches.set(mission.branch, mission); } - takenBranches.set(mission.branch, mission); - } - this.assertScopesDisjoint([ - ...state.missions.filter(isOpenMission), - ...missions, - ]); + this.assertScopesDisjoint([ + ...state.missions.filter(isOpenMission), + ...missions, + ]); - state.missions.push(...missions); - await this.save(state); - await this.renderMissionsIndex(state); - for (const mission of missions) { - await this.renderMissionFile(mission); - } - await this.appendLog( - TOWER_NAME, - 'plan', - { missions: missions.map((m) => m.id).join(',') }, - MISSIONS_INDEX, - ); - return missions; + state.missions.push(...missions); + await this.save(state); + await this.renderMissionsIndex(state); + for (const mission of missions) { + await this.renderMissionFile(mission); + } + await this.appendLog( + TOWER_NAME, + 'plan', + { missions: missions.map((m) => m.id).join(',') }, + MISSIONS_INDEX, + ); + return missions; }); } @@ -644,137 +738,140 @@ export class TowerStore { patch: TowerMissionPatch, options: { readonly silent?: boolean } = {}, ): Promise { - return this.exclusive(async () => { - const state = await this.load(); - const mission = state.missions.find((m) => m.id === id); - if (mission === undefined) { - throw new TowerProtocolError(`unknown mission "${id}"`); - } - if (callerName !== TOWER_NAME) { - const caller = this.findAgent(state, callerName); - if (caller?.kind !== 'worker' || caller.missionId !== id) { - throw new TowerProtocolError( - `agent "${callerName}" does not own mission ${id} — workers update only their own mission file`, - ); + return this.withStateLock(async () => { + const state = await this.load(); + const mission = state.missions.find((m) => m.id === id); + if (mission === undefined) { + throw new TowerProtocolError(`unknown mission "${id}"`); } - } - - const isNoOp = - patch.status === mission.status && - patch.note === undefined && - patch.blocker === undefined && - patch.clearBlockers === undefined && - patch.taskDone === undefined && - patch.taskDrop === undefined && - patch.owner === undefined && - patch.scope === undefined && - patch.spawnBase === undefined; - if (isNoOp) return mission; - - if (patch.spawnBase !== undefined) { if (callerName !== TOWER_NAME) { - throw new TowerProtocolError( - `agent "${callerName}" cannot record a mission spawn base — only the tower does`, - ); + const caller = this.findAgent(state, callerName); + if (caller?.kind !== 'worker' || caller.missionId !== id) { + throw new TowerProtocolError( + `agent "${callerName}" does not own mission ${id} — workers update only their own mission file`, + ); + } } - mission.spawnBase = patch.spawnBase; - } - if (patch.owner !== undefined) { - if (callerName !== TOWER_NAME) { - throw new TowerProtocolError( - `agent "${callerName}" cannot assign mission ownership — only the tower sets owner`, - ); + const isNoOp = + patch.status === mission.status && + patch.note === undefined && + patch.blocker === undefined && + patch.clearBlockers === undefined && + patch.taskDone === undefined && + patch.taskDrop === undefined && + patch.owner === undefined && + patch.scope === undefined && + patch.spawnBase === undefined; + if (isNoOp) return mission; + + if (patch.spawnBase !== undefined) { + if (callerName !== TOWER_NAME) { + throw new TowerProtocolError( + `agent "${callerName}" cannot record a mission spawn base — only the tower does`, + ); + } + mission.spawnBase = patch.spawnBase; } - mission.owner = patch.owner; - } - if (patch.scope !== undefined) { - if (callerName !== TOWER_NAME) { - throw new TowerProtocolError( - `agent "${callerName}" cannot change mission scope — only the tower widens a scope, and every change is logged`, - ); + + if (patch.owner !== undefined) { + if (callerName !== TOWER_NAME) { + throw new TowerProtocolError( + `agent "${callerName}" cannot assign mission ownership — only the tower sets owner`, + ); + } + mission.owner = patch.owner; } - this.assertScopesDisjoint([ - ...state.missions.filter((m) => m.id !== id && isOpenMission(m)), - { ...mission, scope: [...patch.scope] }, - ]); - mission.scope = [...patch.scope]; - } - if (patch.status !== undefined) { - if (patch.status === 'abandoned' && callerName !== TOWER_NAME) { - throw new TowerProtocolError( - `agent "${callerName}" cannot abandon mission ${id} — abandoning releases the mission scope, so only the tower does it`, - ); + if (patch.scope !== undefined) { + if (callerName !== TOWER_NAME) { + throw new TowerProtocolError( + `agent "${callerName}" cannot change mission scope — only the tower widens a scope, and every change is logged`, + ); + } + this.assertScopesDisjoint([ + ...state.missions.filter((m) => m.id !== id && isOpenMission(m)), + { ...mission, scope: [...patch.scope] }, + ]); + mission.scope = [...patch.scope]; } - mission.status = patch.status; - } - if (patch.note !== undefined) mission.notes.push(patch.note); - if (patch.blocker !== undefined) { - mission.blockers.push(patch.blocker); - mission.status = 'blocked'; - } - if (patch.clearBlockers === true) mission.blockers = []; - if (patch.taskDone !== undefined) { - const task = mission.tasks.find( - (t) => !t.done && t.dropped !== true && t.text.includes(patch.taskDone!), - ); - if (task === undefined) { - throw new TowerProtocolError( - `mission ${id} has no open task matching "${patch.taskDone}"`, - ); + if (patch.status !== undefined) { + if (patch.status === 'abandoned' && callerName !== TOWER_NAME) { + throw new TowerProtocolError( + `agent "${callerName}" cannot abandon mission ${id} — abandoning releases the mission scope, so only the tower does it`, + ); + } + mission.status = patch.status; } - task.done = true; - } - let taskDropLog: string | undefined; - if (patch.taskDrop !== undefined) { - const reason = patch.taskDrop.reason?.trim() ?? ''; - if (reason.length === 0) { - throw new TowerProtocolError( - `dropping a task from mission ${id} requires a reason — the drop is the escape hatch for legitimately descoped work, and the reason is recorded in the mission notes and the activity log for audit`, + if (patch.note !== undefined) mission.notes.push(patch.note); + if (patch.blocker !== undefined) { + mission.blockers.push(patch.blocker); + mission.status = 'blocked'; + } + if (patch.clearBlockers === true) mission.blockers = []; + if (patch.taskDone !== undefined) { + const task = mission.tasks.find( + (t) => !t.done && t.dropped !== true && t.text.includes(patch.taskDone!), ); + if (task === undefined) { + throw new TowerProtocolError( + `mission ${id} has no open task matching "${patch.taskDone}"`, + ); + } + task.done = true; } - const task = mission.tasks.find( - (t) => !t.done && t.dropped !== true && t.text.includes(patch.taskDrop!.text), - ); - if (task === undefined) { - throw new TowerProtocolError( - `mission ${id} has no open task matching "${patch.taskDrop.text}"`, + let taskDropLog: string | undefined; + if (patch.taskDrop !== undefined) { + const reason = patch.taskDrop.reason?.trim() ?? ''; + if (reason.length === 0) { + throw new TowerProtocolError( + `dropping a task from mission ${id} requires a reason — the drop is the escape hatch for legitimately descoped work, and the reason is recorded in the mission notes and the activity log for audit`, + ); + } + const task = mission.tasks.find( + (t) => !t.done && t.dropped !== true && t.text.includes(patch.taskDrop!.text), ); + if (task === undefined) { + throw new TowerProtocolError( + `mission ${id} has no open task matching "${patch.taskDrop.text}"`, + ); + } + task.dropped = true; + taskDropLog = `dropped task "${task.text}": ${reason}`; + mission.notes.push(taskDropLog); + } + if (patch.status === 'completed' && patch.blocker === undefined) { + await this.assertCompletable(state, mission); + if (callerName !== TOWER_NAME) { + await this.assertInboxRead(state, callerName, mission); + } } - task.dropped = true; - taskDropLog = `dropped task "${task.text}": ${reason}`; - mission.notes.push(taskDropLog); - } - if (patch.status === 'completed' && patch.blocker === undefined) { - await this.assertCompletable(state, mission); - } - await this.save(state); - await this.renderMissionsIndex(state); - await this.renderMissionFile(mission); - const taskTickOnly = - patch.taskDone !== undefined && - patch.status === undefined && - patch.note === undefined && - patch.blocker === undefined && - patch.clearBlockers === undefined && - patch.taskDrop === undefined && - patch.owner === undefined && - patch.scope === undefined && - patch.spawnBase === undefined; - if (!taskTickOnly && options.silent !== true) { - await this.appendLog(callerName, 'mission.update', { - id, - status: patch.status, - note: patch.note !== undefined ? 'added' : undefined, - blocker: patch.blocker !== undefined ? 'added' : undefined, - task_drop: taskDropLog, - owner: patch.owner, - scope: patch.scope?.join(','), - spawn_base: patch.spawnBase, - }); - } - return mission; + await this.save(state); + await this.renderMissionsIndex(state); + await this.renderMissionFile(mission); + const taskTickOnly = + patch.taskDone !== undefined && + patch.status === undefined && + patch.note === undefined && + patch.blocker === undefined && + patch.clearBlockers === undefined && + patch.taskDrop === undefined && + patch.owner === undefined && + patch.scope === undefined && + patch.spawnBase === undefined; + if (!taskTickOnly && options.silent !== true) { + await this.appendLog(callerName, 'mission.update', { + id, + status: patch.status, + note: patch.note !== undefined ? 'added' : undefined, + blocker: patch.blocker !== undefined ? 'added' : undefined, + task_drop: taskDropLog, + owner: patch.owner, + scope: patch.scope?.join(','), + spawn_base: patch.spawnBase, + }); + } + return mission; }); } @@ -800,6 +897,23 @@ export class TowerStore { } } + private async assertInboxRead( + state: TowerState, + callerName: string, + mission: TowerMission, + ): Promise { + const entry = this.findAgent(state, callerName); + if (entry === undefined) return; + const since = entry.lastInboxReadAt ?? entry.spawnedAt; + const unread = (await this.listInboxItems()).filter( + (item) => (item.to === callerName || item.to === BROADCAST_NAME) && item.sentAt > since, + ); + if (unread.length === 0) return; + throw new TowerProtocolError( + `mission ${mission.id} cannot transition to completed — ${String(unread.length)} unread inbox message(s) for ${callerName} arrived since the inbox was last read; call TowerInbox, incorporate anything new into the delivery, then retry status=completed`, + ); + } + async send(callerName: string, input: TowerSendInput): Promise { const state = await this.load(); const to = input.to.trim(); @@ -842,6 +956,27 @@ export class TowerStore { } async readInbox(callerName: string, limit: number): Promise { + const items = (await this.listInboxItems()).filter( + (item) => callerName === TOWER_NAME || item.to === callerName || item.to === BROADCAST_NAME, + ); + items.sort((a, b) => b.sentAt.localeCompare(a.sentAt)); + return items.slice(0, Math.max(1, limit)); + } + + async markInboxRead(callerName: string, newestSeenSentAt?: string): Promise { + await this.withStateLock(async () => { + const state = await this.load(); + const index = state.roster.agents.findIndex((agent) => agent.name === callerName); + const entry = state.roster.agents[index]; + if (entry === undefined) return; + const at = newestSeenSentAt ?? new Date().toISOString(); + if ((entry.lastInboxReadAt ?? entry.spawnedAt) >= at) return; + state.roster.agents[index] = { ...entry, lastInboxReadAt: at }; + await this.save(state); + }); + } + + private async listInboxItems(): Promise { let files: string[]; try { files = await readdir(this.abs(INBOX_DIR)); @@ -859,12 +994,10 @@ export class TowerStore { } const { fields, body } = parseFrontmatter(text); if (fields['type'] !== 'inbox') continue; - const to = fields['to'] ?? ''; - if (callerName !== TOWER_NAME && to !== callerName && to !== BROADCAST_NAME) continue; items.push({ file: rel, from: fields['from'] ?? 'unknown', - to, + to: fields['to'] ?? '', subject: fields['subject'] ?? '', sentAt: fields['sent_at'] ?? '', scope: fields['scope'], @@ -873,8 +1006,7 @@ export class TowerStore { body, }); } - items.sort((a, b) => b.sentAt.localeCompare(a.sentAt)); - return items.slice(0, Math.max(1, limit)); + return items; } async fileFinding(callerName: string, input: TowerFindingInput): Promise { @@ -940,106 +1072,106 @@ export class TowerStore { } async submitReview(callerName: string, input: TowerReviewInput): Promise { - return this.exclusive(async () => { - const state = await this.load(); - let callerEntry: TowerRosterEntry | undefined; - if (callerName !== TOWER_NAME) { - callerEntry = this.findAgent(state, callerName); - if (callerEntry?.kind !== 'reviewer' || callerEntry.reviewTarget !== input.target) { + return this.withStateLock(async () => { + const state = await this.load(); + let callerEntry: TowerRosterEntry | undefined; + if (callerName !== TOWER_NAME) { + callerEntry = this.findAgent(state, callerName); + if (callerEntry?.kind !== 'reviewer' || callerEntry.reviewTarget !== input.target) { + throw new TowerProtocolError( + `agent "${callerName}" is not an assigned reviewer for "${input.target}"`, + ); + } + } + if (!/^(clean|p[12]-\d+items)$/.test(input.status)) { throw new TowerProtocolError( - `agent "${callerName}" is not an assigned reviewer for "${input.target}"`, + `review status must be clean | p1-Nitems | p2-Nitems, got "${input.status}"`, + ); + } + if (!['merge', 'fix-then-merge', 'hold'].includes(input.merge)) { + throw new TowerProtocolError( + `review merge verdict must be merge | fix-then-merge | hold, got "${input.merge}"`, ); } - } - if (!/^(clean|p[12]-\d+items)$/.test(input.status)) { - throw new TowerProtocolError( - `review status must be clean | p1-Nitems | p2-Nitems, got "${input.status}"`, - ); - } - if (!['merge', 'fix-then-merge', 'hold'].includes(input.merge)) { - throw new TowerProtocolError( - `review merge verdict must be merge | fix-then-merge | hold, got "${input.merge}"`, - ); - } - - const existing = await this.reviewsFor(input.target); - const myRounds = existing.filter((r) => r.reviewer === callerName).length; - if (myRounds >= MAX_REVIEW_ROUNDS) { - throw new TowerProtocolError( - `branch "${input.target}" has already been through ${String(MAX_REVIEW_ROUNDS)} review rounds by "${callerName}" — the rework loop is not converging, so another round from the same reviewer is refused; redirect instead: reassign the work (spawn a different worker or a fresh reviewer), split the mission into smaller pieces, or descope it (TowerMission status=abandoned)`, - ); - } - const round = myRounds + 1; - const seq = await this.nextReviewSeq(); - const reviewedCommit = await branchTip(this.repoRoot, input.target); - const reviewMissionId = - callerEntry === undefined - ? resolveMissionByBranch(state, input.target)?.id - : callerEntry.reviewMissionId; - - const frontmatter = renderFrontmatter({ - date: dateDash(), - reviewer: callerName, - target: input.target, - round: String(round), - seq: String(seq), - status: input.status, - merge: input.merge, - reviewed_commit: reviewedCommit, - mission: reviewMissionId, - token_count: String(input.token_count ?? -1), - }); - const checks = (input.checks ?? []).map((c) => `- [x] ${c}`).join('\n'); - const content = [ - frontmatter, - '', - '## Findings', - '', - input.findings.trim(), - '', - '## Checks', - checks.length > 0 ? checks : '- [x] (reviewer reported no formal checks)', - '', - '## Decision', - input.decision.trim(), - '', - ].join('\n'); - const rel = await this.writeUnique( - join(REVIEWS_DIR, reviewFileName({ target: input.target, reviewer: callerName, round })), - content, - ); - await this.appendLog( - callerName, - 'review.write', - { - target: input.target, - round, - verdict: input.status, - reviewed: reviewedCommit.slice(0, 7), - token_count: input.token_count ?? -1, - }, - rel, - ); - if (input.status !== 'clean') { - const reworkMission = - reviewMissionId !== undefined - ? state.missions.find((m) => m.id === reviewMissionId) - : resolveMissionByBranch(state, input.target); - if (reworkMission !== undefined && reworkMission.status === 'completed') { - reworkMission.status = 'active'; - await this.save(state); - await this.renderMissionsIndex(state); - await this.renderMissionFile(reworkMission); - await this.appendLog( - callerName, - 'mission.rework', - { id: reworkMission.id, verdict: input.status }, - join(MISSIONS_DIR, missionFileName(reworkMission.id, reworkMission.slug)), + const existing = await this.reviewsFor(input.target); + const myRounds = existing.filter((r) => r.reviewer === callerName).length; + if (myRounds >= MAX_REVIEW_ROUNDS) { + throw new TowerProtocolError( + `branch "${input.target}" has already been through ${String(MAX_REVIEW_ROUNDS)} review rounds by "${callerName}" — the rework loop is not converging, so another round from the same reviewer is refused; redirect instead: reassign the work (spawn a different worker or a fresh reviewer), split the mission into smaller pieces, or descope it (TowerMission status=abandoned)`, ); } - } - return rel; + const round = myRounds + 1; + const seq = await this.nextReviewSeq(); + const reviewedCommit = await branchTip(this.repoRoot, input.target); + const reviewMissionId = + callerEntry === undefined + ? resolveMissionByBranch(state, input.target)?.id + : callerEntry.reviewMissionId; + + const frontmatter = renderFrontmatter({ + date: dateDash(), + reviewer: callerName, + target: input.target, + round: String(round), + seq: String(seq), + status: input.status, + merge: input.merge, + reviewed_commit: reviewedCommit, + mission: reviewMissionId, + token_count: String(input.token_count ?? -1), + }); + const checks = (input.checks ?? []).map((c) => `- [x] ${c}`).join('\n'); + const content = [ + frontmatter, + '', + '## Findings', + '', + input.findings.trim(), + '', + '## Checks', + checks.length > 0 ? checks : '- [x] (reviewer reported no formal checks)', + '', + '## Decision', + input.decision.trim(), + '', + ].join('\n'); + + const rel = await this.writeUnique( + join(REVIEWS_DIR, reviewFileName({ target: input.target, reviewer: callerName, round })), + content, + ); + await this.appendLog( + callerName, + 'review.write', + { + target: input.target, + round, + verdict: input.status, + reviewed: reviewedCommit.slice(0, 7), + token_count: input.token_count ?? -1, + }, + rel, + ); + if (input.status !== 'clean') { + const reworkMission = + reviewMissionId !== undefined + ? state.missions.find((m) => m.id === reviewMissionId) + : resolveMissionByBranch(state, input.target); + if (reworkMission !== undefined && reworkMission.status === 'completed') { + reworkMission.status = 'active'; + await this.save(state); + await this.renderMissionsIndex(state); + await this.renderMissionFile(reworkMission); + await this.appendLog( + callerName, + 'mission.rework', + { id: reworkMission.id, verdict: input.status }, + join(MISSIONS_DIR, missionFileName(reworkMission.id, reworkMission.slug)), + ); + } + } + return rel; }); } @@ -1120,148 +1252,148 @@ export class TowerStore { readonly conflictsWith: ReadonlyArray<{ readonly branch: string; readonly files: readonly string[] }>; readonly noop?: boolean; }> { - return this.exclusive(async () => { - const state = await this.load(); - const block = async (reason: string, message: string): Promise => { - await this.appendLog(TOWER_NAME, 'merge.blocked', { branch, reason }); - return new TowerProtocolError(message); - }; - const mission = resolveMissionByBranch(state, branch); - if (mission === undefined) { - const closed = state.missions.filter((m) => m.branch === branch); - if (closed.length > 0) { + return this.withStateLock(async () => { + const state = await this.load(); + const block = async (reason: string, message: string): Promise => { + await this.appendLog(TOWER_NAME, 'merge.blocked', { branch, reason }); + return new TowerProtocolError(message); + }; + const mission = resolveMissionByBranch(state, branch); + if (mission === undefined) { + const closed = state.missions.filter((m) => m.branch === branch); + if (closed.length > 0) { + throw await block( + 'branch-owned-by-closed-missions', + `merge blocked: branch "${branch}" resolves only to closed mission(s) ${closed.map((m) => `${m.id} (${m.status})`).join(', ')} — TowerMerge never flips a closed mission's status; re-plan the work under a new title if it should land`, + ); + } + throw new TowerProtocolError(`no tower mission owns branch "${branch}"`); + } + + const unmergedDeps = mission.deps.filter((dep) => { + const depMission = state.missions.find((m) => m.id === dep); + return depMission !== undefined && isOpenMission(depMission); + }); + if (unmergedDeps.length > 0) { throw await block( - 'branch-owned-by-closed-missions', - `merge blocked: branch "${branch}" resolves only to closed mission(s) ${closed.map((m) => `${m.id} (${m.status})`).join(', ')} — TowerMerge never flips a closed mission's status; re-plan the work under a new title if it should land`, + 'deps-unmerged', + `merge blocked: dependencies not merged yet (${unmergedDeps.join(', ')}) — merge in Dependency Flow order`, ); } - throw new TowerProtocolError(`no tower mission owns branch "${branch}"`); - } - const unmergedDeps = mission.deps.filter((dep) => { - const depMission = state.missions.find((m) => m.id === dep); - return depMission !== undefined && isOpenMission(depMission); - }); - if (unmergedDeps.length > 0) { - throw await block( - 'deps-unmerged', - `merge blocked: dependencies not merged yet (${unmergedDeps.join(', ')}) — merge in Dependency Flow order`, - ); - } + if (mission.kind === 'survey') { + const changed = await diffNameOnly(this.repoRoot, await this.diffBase(state, mission), branch); + if (changed.length > 0) { + throw await block( + 'read-only-survey', + `merge blocked: survey mission ${mission.id} is read-only but ${branch} has ${String(changed.length)} changed file(s): ${changed.slice(0, 5).join(', ')} — investigate the worker; if the changes are worth keeping, move them onto a build mission's branch`, + ); + } + mission.status = 'merged'; + await this.save(state); + await this.renderMissionsIndex(state); + await this.renderMissionFile(mission); + const tip = await branchTip(this.repoRoot, state.base); + await this.appendLog(TOWER_NAME, 'merge.noop', { branch, kind: 'survey' }); + return { mergeCommit: tip, conflictsWith: [], noop: true }; + } - if (mission.kind === 'survey') { - const changed = await diffNameOnly(this.repoRoot, await this.diffBase(state, mission), branch); - if (changed.length > 0) { + const reviews = await this.reviewsFor(branch); + const siblingMissions = state.missions.filter((m) => m.branch === branch && m.id !== mission.id); + const stamped = reviews.filter((r) => r.mission === mission.id); + const candidates = + stamped.length > 0 + ? reviews.filter( + (r) => + r.mission === mission.id || (r.mission === undefined && siblingMissions.length === 0), + ) + : reviews.filter((r) => r.mission === undefined); + const review = candidates.at(-1); + if (review === undefined) { throw await block( - 'read-only-survey', - `merge blocked: survey mission ${mission.id} is read-only but ${branch} has ${String(changed.length)} changed file(s): ${changed.slice(0, 5).join(', ')} — investigate the worker; if the changes are worth keeping, move them onto a build mission's branch`, + 'no-review', + `merge blocked: ${branch} has no review — assign a reviewer first`, + ); + } + if (review.status !== 'clean') { + throw await block( + 'not-clean', + `merge blocked: latest review (round ${review.round} by ${review.reviewer}) is "${review.status}" — a clean round is required`, + ); + } + const tip = await branchTip(this.repoRoot, branch); + if (review.reviewedCommit !== tip) { + throw await block( + 'tip-moved', + `merge blocked: ${branch} moved since the clean review (reviewed ${review.reviewedCommit.slice(0, 7)}, tip ${tip.slice(0, 7)}) — re-review required`, + ); + } + if (review.mission === undefined && siblingMissions.length > 0) { + throw await block( + 'review-mission-mismatch', + `merge blocked: "${branch}" is shared with other mission record(s) ${siblingMissions.map((m) => `${m.id} (${m.status})`).join(', ')}, and the latest clean review (round ${review.round} by ${review.reviewer}) predates mission-stamped reviews — re-review ${mission.id} so the gate can tell which mission was audited`, ); } - mission.status = 'merged'; - await this.save(state); - await this.renderMissionsIndex(state); - await this.renderMissionFile(mission); - const tip = await branchTip(this.repoRoot, state.base); - await this.appendLog(TOWER_NAME, 'merge.noop', { branch, kind: 'survey' }); - return { mergeCommit: tip, conflictsWith: [], noop: true }; - } - - const reviews = await this.reviewsFor(branch); - const siblingMissions = state.missions.filter((m) => m.branch === branch && m.id !== mission.id); - const stamped = reviews.filter((r) => r.mission === mission.id); - const candidates = - stamped.length > 0 - ? reviews.filter( - (r) => - r.mission === mission.id || (r.mission === undefined && siblingMissions.length === 0), - ) - : reviews.filter((r) => r.mission === undefined); - const review = candidates.at(-1); - if (review === undefined) { - throw await block( - 'no-review', - `merge blocked: ${branch} has no review — assign a reviewer first`, - ); - } - if (review.status !== 'clean') { - throw await block( - 'not-clean', - `merge blocked: latest review (round ${review.round} by ${review.reviewer}) is "${review.status}" — a clean round is required`, - ); - } - const tip = await branchTip(this.repoRoot, branch); - if (review.reviewedCommit !== tip) { - throw await block( - 'tip-moved', - `merge blocked: ${branch} moved since the clean review (reviewed ${review.reviewedCommit.slice(0, 7)}, tip ${tip.slice(0, 7)}) — re-review required`, - ); - } - if (review.mission === undefined && siblingMissions.length > 0) { - throw await block( - 'review-mission-mismatch', - `merge blocked: "${branch}" is shared with other mission record(s) ${siblingMissions.map((m) => `${m.id} (${m.status})`).join(', ')}, and the latest clean review (round ${review.round} by ${review.reviewer}) predates mission-stamped reviews — re-review ${mission.id} so the gate can tell which mission was audited`, - ); - } - - const changed = await diffNameOnly(this.repoRoot, await this.diffBase(state, mission), branch); - const outOfScope = changed.filter( - (file) => !mission.scope.some((glob) => picomatch.isMatch(file, glob)), - ); - if (outOfScope.length > 0) { - throw await block( - 'out-of-scope', - `merge blocked: ${branch} changed files outside mission ${mission.id} scope (${mission.scope.join(', ')}): ${outOfScope.join(', ')} — the tower must widen the mission scope (TowerMission scope patch) or revert those changes`, - ); - } - let checkedOut: string; - try { - checkedOut = await currentBranch(this.repoRoot); - } catch { - throw await block( - 'base-mismatch', - `merge blocked: the main checkout is in a detached HEAD state — check out the recorded base branch "${state.base}" before merging; nothing was merged`, - ); - } - if (checkedOut !== state.base) { - throw await block( - 'base-mismatch', - `merge blocked: the main checkout is on "${checkedOut}", not the recorded base "${state.base}" — switch it back (\`git checkout ${state.base}\`) and retry; nothing was merged`, + const changed = await diffNameOnly(this.repoRoot, await this.diffBase(state, mission), branch); + const outOfScope = changed.filter( + (file) => !mission.scope.some((glob) => picomatch.isMatch(file, glob)), ); - } + if (outOfScope.length > 0) { + throw await block( + 'out-of-scope', + `merge blocked: ${branch} changed files outside mission ${mission.id} scope (${mission.scope.join(', ')}): ${outOfScope.join(', ')} — the tower must widen the mission scope (TowerMission scope patch) or revert those changes`, + ); + } - const touched = await diffNameOnly(this.repoRoot, 'HEAD', branch); - if (touched.length > 0) { - const dirty = new Set((await listBaseDirtyEntries(this.repoRoot)).map((entry) => entry.path)); - const blocked = touched.filter((file) => dirty.has(file)); - if (blocked.length > 0) { + let checkedOut: string; + try { + checkedOut = await currentBranch(this.repoRoot); + } catch { throw await block( - 'base-dirty', - `merge blocked: the main checkout has uncommitted changes in file(s) this merge would overwrite: ${blocked.slice(0, 5).join(', ')} — commit or stash them first, then retry; nothing was merged`, + 'base-mismatch', + `merge blocked: the main checkout is in a detached HEAD state — check out the recorded base branch "${state.base}" before merging; nothing was merged`, + ); + } + if (checkedOut !== state.base) { + throw await block( + 'base-mismatch', + `merge blocked: the main checkout is on "${checkedOut}", not the recorded base "${state.base}" — switch it back (\`git checkout ${state.base}\`) and retry; nothing was merged`, ); } - } - const mergeCommit = await mergeNoFf(this.repoRoot, branch); - mission.status = 'merged'; - - const changedSet = new Set(changed); - const conflictsWith: Array<{ readonly branch: string; readonly files: readonly string[] }> = []; - for (const other of state.missions) { - if (other.branch === branch || !isOpenMission(other)) continue; - if (!(await branchExists(this.repoRoot, other.branch))) continue; - const otherChanged = await diffNameOnly(this.repoRoot, await this.diffBase(state, other), other.branch); - const overlap = otherChanged.filter((file) => changedSet.has(file)); - if (overlap.length > 0) { - conflictsWith.push({ branch: other.branch, files: overlap }); + const touched = await diffNameOnly(this.repoRoot, 'HEAD', branch); + if (touched.length > 0) { + const dirty = new Set((await listBaseDirtyEntries(this.repoRoot)).map((entry) => entry.path)); + const blocked = touched.filter((file) => dirty.has(file)); + if (blocked.length > 0) { + throw await block( + 'base-dirty', + `merge blocked: the main checkout has uncommitted changes in file(s) this merge would overwrite: ${blocked.slice(0, 5).join(', ')} — commit or stash them first, then retry; nothing was merged`, + ); + } } - } - await this.save(state); - await this.renderMissionsIndex(state); - await this.renderMissionFile(mission); - await this.appendLog(TOWER_NAME, 'merge', { branch, base: state.base, merge_commit: mergeCommit.slice(0, 7) }); - return { mergeCommit, conflictsWith }; + const mergeCommit = await mergeNoFf(this.repoRoot, branch); + mission.status = 'merged'; + + const changedSet = new Set(changed); + const conflictsWith: Array<{ readonly branch: string; readonly files: readonly string[] }> = []; + for (const other of state.missions) { + if (other.branch === branch || !isOpenMission(other)) continue; + if (!(await branchExists(this.repoRoot, other.branch))) continue; + const otherChanged = await diffNameOnly(this.repoRoot, await this.diffBase(state, other), other.branch); + const overlap = otherChanged.filter((file) => changedSet.has(file)); + if (overlap.length > 0) { + conflictsWith.push({ branch: other.branch, files: overlap }); + } + } + + await this.save(state); + await this.renderMissionsIndex(state); + await this.renderMissionFile(mission); + await this.appendLog(TOWER_NAME, 'merge', { branch, base: state.base, merge_commit: mergeCommit.slice(0, 7) }); + return { mergeCommit, conflictsWith }; }); } @@ -1332,30 +1464,75 @@ export class TowerStore { return { rel, spawnBase }; } - async teardown(options: { readonly force?: boolean } = {}): Promise { + async teardown(options: TowerTeardownOptions = {}): Promise { const state = await this.load(); + const dryRun = options.dryRun === true; + const force = options.force === true; + const liveAgentIds = options.liveAgentIds ?? new Set(); + const excluded = new Map(); + for (const raw of options.exclude ?? []) { + const trimmed = raw.trim().replace(/\/+$/, ''); + if (trimmed.length === 0) continue; + const short = trimmed.startsWith(`${WORKTREES_DIR}/`) + ? trimmed.slice(WORKTREES_DIR.length + 1) + : trimmed; + excluded.set(short, raw); + } const report: string[] = []; + const kept = async ( + mission: TowerMission, + rel: string, + reason: string, + log: Record, + ): Promise => { + report.push(`${dryRun ? 'would keep' : 'kept'} ${rel} (${reason})`); + if (!dryRun) { + await this.appendLog(TOWER_NAME, 'worktree.keep', { + worktree: mission.worktree, + ...log, + }); + } + }; for (const mission of state.missions) { const rel = join(WORKTREES_DIR, mission.worktree); const absPath = this.abs(rel); + const wasExcluded = excluded.delete(mission.worktree); if (!(await isRegisteredWorktree(this.repoRoot, absPath))) { report.push(`already removed ${rel}`); - await this.appendLog(TOWER_NAME, 'worktree.remove.skipped', { - worktree: mission.worktree, - reason: 'already-removed', + if (!dryRun) { + await this.appendLog(TOWER_NAME, 'worktree.remove.skipped', { + worktree: mission.worktree, + reason: 'already-removed', + }); + } + continue; + } + if (wasExcluded) { + await kept(mission, rel, 'excluded', { reason: 'excluded' }); + continue; + } + const liveAgent = state.roster.agents.find( + (agent) => agent.worktree === mission.worktree && liveAgentIds.has(agent.agentId), + ); + if (liveAgent !== undefined) { + await kept(mission, rel, `live agent: ${liveAgent.name}`, { + reason: 'live-agent', + agent: liveAgent.name, }); continue; } - if (await isWorktreeDirty(absPath)) { - if (options.force !== true) { - report.push(`kept ${rel} (uncommitted changes — rerun with force to remove)`); - await this.appendLog(TOWER_NAME, 'worktree.keep', { - worktree: mission.worktree, + if (!force) { + if (await isWorktreeDirty(absPath)) { + await kept(mission, rel, 'uncommitted changes — rerun with force to remove', { reason: 'uncommitted-changes', }); continue; } } + if (dryRun) { + report.push(`would remove ${rel}`); + continue; + } try { await worktreeRemove(this.repoRoot, absPath); report.push(`removed ${rel}`); @@ -1369,7 +1546,12 @@ export class TowerStore { }); } } - await this.appendLog(TOWER_NAME, 'teardown', { force: options.force === true ? 'yes' : undefined }); + for (const original of excluded.values()) { + report.push(`excluded worktree "${original}" matched no mission worktree`); + } + if (!dryRun) { + await this.appendLog(TOWER_NAME, 'teardown', { force: force ? 'yes' : undefined }); + } return report; } diff --git a/packages/agent-core-v2/src/features/tower/protocol/types.ts b/packages/agent-core-v2/src/features/tower/protocol/types.ts index ac031e2e4..427d2d851 100644 --- a/packages/agent-core-v2/src/features/tower/protocol/types.ts +++ b/packages/agent-core-v2/src/features/tower/protocol/types.ts @@ -11,6 +11,7 @@ export interface TowerRosterEntry { readonly worktree?: string; readonly branch?: string; readonly spawnedAt: string; + readonly lastInboxReadAt?: string; readonly diedAt?: string; readonly deathStatus?: string; readonly deathReason?: string; diff --git a/packages/agent-core-v2/src/features/tower/tools/inbox/inbox.md b/packages/agent-core-v2/src/features/tower/tools/inbox/inbox.md index 71c4e7b98..f4c3b21e9 100644 --- a/packages/agent-core-v2/src/features/tower/tools/inbox/inbox.md +++ b/packages/agent-core-v2/src/features/tower/tools/inbox/inbox.md @@ -1 +1 @@ -Read your tower inbox: messages addressed to you plus broadcasts, newest first. The tower sees all messages. Full bodies are included — reply with TowerSend. +Read your tower inbox: messages addressed to you plus broadcasts, newest first. The tower sees all messages. Full bodies are included — reply with TowerSend. Reading marks your inbox as read: the store refuses a roster agent's own TowerMission status=completed while unread messages wait, so always read before completing. diff --git a/packages/agent-core-v2/src/features/tower/tools/inbox/inboxTool.ts b/packages/agent-core-v2/src/features/tower/tools/inbox/inboxTool.ts index 5c7155549..53831f1c7 100644 --- a/packages/agent-core-v2/src/features/tower/tools/inbox/inboxTool.ts +++ b/packages/agent-core-v2/src/features/tower/tools/inbox/inboxTool.ts @@ -30,6 +30,7 @@ export class TowerInboxTool implements ITowerInboxTool { const state = await store.load(); const caller = callerName(this.scopeContext.agentId, store, state); const items = await store.readInbox(caller, args.limit ?? DEFAULT_LIMIT); + await store.markInboxRead(caller, items[0]?.sentAt); if (items.length === 0) { return { output: `inbox empty for ${caller}` }; } diff --git a/packages/agent-core-v2/src/features/tower/tools/mission/mission.md b/packages/agent-core-v2/src/features/tower/tools/mission/mission.md index 73b37d862..3cabbd543 100644 --- a/packages/agent-core-v2/src/features/tower/tools/mission/mission.md +++ b/packages/agent-core-v2/src/features/tower/tools/mission/mission.md @@ -2,6 +2,6 @@ Read or update a tower mission. With only an id, returns the mission view (status, tasks, blockers, notes). With patch fields, applies them: workers may only update the mission they own — the store rejects anything else. Use task_done to tick checklist items, task_drop (with task_drop_reason) to drop a legitimately descoped task — the reason is recorded in the mission notes and the activity log — note to log decisions, blocker when stuck (the tower watches for blocked missions). -Status lifecycle: planned → active → completed → merged. "completed" means awaiting review, not done — the branch can still be sent back for rework: when a review with a non-clean verdict lands on the mission's branch, the store flips a completed mission back to active automatically (merged never flips; planned, blocked, and paused are left alone). Only "merged" is final. The store refuses the transition to completed while the mission still has open (neither done nor dropped) tasks — the error lists them — and, for build missions, while the mission branch has no diff vs its base; survey missions are exempt from the diff check. +Status lifecycle: planned → active → completed → merged. "completed" means awaiting review, not done — the branch can still be sent back for rework: when a review with a non-clean verdict lands on the mission's branch, the store flips a completed mission back to active automatically (merged never flips; planned, blocked, and paused are left alone). Only "merged" is final. The store refuses the transition to completed while the mission still has open (neither done nor dropped) tasks — the error lists them — and, for build missions, while the mission branch has no diff vs its base; survey missions are exempt from the diff check. A roster agent's own transition to completed is also refused while its inbox holds messages it has not read yet — the error names the unread count; read them with TowerInbox and incorporate anything new into the delivery first (the tower is exempt from this check). Tower only: status=abandoned gives a mission up without merging — its scope stops reserving files for TowerPlan, its dependents may merge, and its branch drops out of conflict checks. Use it for stale missions carried over from a previous session, or for work that will not land; abandoned missions stay in MISSIONS.md (🚫) as the audit trail. diff --git a/packages/agent-core-v2/src/features/tower/tools/spawn/spawnTool.ts b/packages/agent-core-v2/src/features/tower/tools/spawn/spawnTool.ts index 52c4ac304..171f93dbf 100644 --- a/packages/agent-core-v2/src/features/tower/tools/spawn/spawnTool.ts +++ b/packages/agent-core-v2/src/features/tower/tools/spawn/spawnTool.ts @@ -279,7 +279,12 @@ export class TowerSpawnTool implements ITowerSpawnTool { isAgentTaskTerminal(settled.status) && settled.status !== 'completed' ) { - await store.markAgentDied(handle.agentId, settled.status, settled.stopReason); + await store.markAgentDied( + handle.agentId, + settled.status, + settled.stopReason, + this.sessionContext.sessionId, + ); } if (mission !== undefined) { await store.updateMission( @@ -322,7 +327,7 @@ export class TowerSpawnTool implements ITowerSpawnTool { : [`review_target: ${reviewTarget ?? ''}`]), ...notes, '', - `The ${args.kind} runs detached in the background; its completion arrives as a notification. Track progress with TowerStatus / TowerInbox; recover a dead agent with Agent(resume="${handle.agentId}", run_in_background=true, prompt="...") — never foreground: its output flows back through the tower protocol files.`, + `The ${args.kind} runs detached in the background; its completion arrives as a notification. Track progress with TowerStatus / TowerInbox. If it dies, diagnose first: check why it died (the died entry's status/reason, its task state) before reviving. Resume with Agent(resume="${handle.agentId}", run_in_background=true, prompt="...") — never foreground: its output flows back through the tower protocol files — or reassign its work, but only when the cause is transient (lost contact, timeout, OOM); a systematic cause (code or environment defect) is fixed or escalated to the human before any revive.`, ].join('\n'), }; } finally { @@ -441,9 +446,10 @@ export class TowerSpawnTool implements ITowerSpawnTool { '- NEVER create or edit files under `.tower/` by hand — the tools are the only writers.\n' + '- Ambiguity is escalated, not guessed: if the mission leaves substantive doubt about what to investigate, TowerSend(to="tower", subject="clarify-request", body=what needs pinning down) BEFORE acting — the tower relays to the human; you never ask the user directly.\n\n' + `# When the survey is done\n` + - `1. Mark the mission completed: TowerMission(id="${mission.id}", status="completed").\n` + - '2. Send the tower your summary: TowerSend(to="tower", subject="survey-summary", body=the full survey result).\n' + - '3. Finish with a structured final summary: what you covered, key facts with file:line references, open questions.' + + '1. Call TowerInbox once and fold anything new into your summary — the store refuses status="completed" while unread messages wait in your inbox.\n' + + `2. Mark the mission completed: TowerMission(id="${mission.id}", status="completed").\n` + + '3. Send the tower your summary: TowerSend(to="tower", subject="survey-summary", body=the full survey result).\n' + + '4. Finish with a structured final summary: what you covered, key facts with file:line references, open questions.' + extra ); } @@ -461,9 +467,11 @@ export class TowerSpawnTool implements ITowerSpawnTool { '- Ambiguity is escalated, not guessed: if the mission and its Context leave substantive doubt about what to build, TowerSend(to="tower", subject="clarify-request", body=what needs pinning down) BEFORE acting — the tower relays to the human; you never ask the user directly.\n\n' + `# When the mission is done\n` + "1. `git add` + `git commit` your mission's changes in the worktree (source files only — no build outputs).\n" + - `2. Mark the mission completed: TowerMission(id="${mission.id}", status="completed").\n` + - '3. Request review: TowerSend(to="tower", subject="review-request", body=what you changed and why, reconciled against the mission tasks item by item — the reviewer maps each task to your diff).\n' + - '4. Finish with a structured final summary: files changed, key decisions, open follow-ups.' + + `2. Before marking completed or sending the review-request, call TowerInbox once and incorporate anything new into the delivery — the store refuses status="completed" while unread messages wait in your inbox. Re-read your mission too (TowerMission(id="${mission.id}") with no patch fields): the tower may have changed its tasks mid-flight.\n` + + `3. Mark the mission completed: TowerMission(id="${mission.id}", status="completed").\n` + + '4. Request review: TowerSend(to="tower", subject="review-request", body=what you changed and why, reconciled against the mission tasks item by item — the reviewer maps each task to your diff).\n' + + '5. Finish with a structured final summary: files changed, key decisions, open follow-ups.\n' + + 'Long blocking operations (sleep, builds, test runs) hide inbox messages for their whole duration — split them into chunks and check TowerInbox between chunks.' + extra ); } diff --git a/packages/agent-core-v2/src/features/tower/tools/status/statusTool.ts b/packages/agent-core-v2/src/features/tower/tools/status/statusTool.ts index 21359a0e8..a09003da6 100644 --- a/packages/agent-core-v2/src/features/tower/tools/status/statusTool.ts +++ b/packages/agent-core-v2/src/features/tower/tools/status/statusTool.ts @@ -1,5 +1,11 @@ import { branchExists, branchTip } from '#/features/tower/protocol/index'; -import type { TowerMission, TowerState, TowerStore } from '#/features/tower/protocol/index'; +import type { + TowerMission, + TowerRosterEntry, + TowerState, + TowerStore, +} from '#/features/tower/protocol/index'; +import { userCancellationReason } from '#/_base/utils/abort'; import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { ITowerRateLimitService, @@ -202,10 +208,16 @@ function renderDeathWarnings(state: TowerState): string[] { const entry = deadByName.get(mission.owner); if (entry === undefined) continue; lines.push( - `- ⚠️ ${mission.id} owner ${entry.name} died (${entry.deathStatus ?? 'unknown'}) — recover with Agent(resume="${entry.agentId}", run_in_background=true, prompt="...") (never foreground: its output flows back through the tower protocol files) or reassign the mission`, + isStoppedByUser(entry) + ? `- 🛑 ${mission.id} owner ${entry.name} was stopped by the user (${entry.deathStatus ?? 'unknown'}) — dead by intent: never resume it and do not reassign the mission unless the human asks` + : `- ⚠️ ${mission.id} owner ${entry.name} died (${entry.deathStatus ?? 'unknown'}) — diagnose first: check why it died (the died entry's status/reason, its task state) before reviving anything. Resume with Agent(resume="${entry.agentId}", run_in_background=true, prompt="...") (never foreground: its output flows back through the tower protocol files) or reassign the mission only when the cause is transient (lost contact, timeout, OOM); a systematic cause (code or environment defect) is fixed or escalated to the human before any revive`, ); } if (lines.length === 0) return lines; return ['', '## Dead workers', '', ...lines]; } +function isStoppedByUser(entry: TowerRosterEntry): boolean { + return entry.deathReason?.trim() === userCancellationReason().message; +} + diff --git a/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.md b/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.md index 83161321b..23fdd5500 100644 --- a/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.md +++ b/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.md @@ -1,3 +1,11 @@ Tear down the tower workspace after all missions are merged (or abandoned). -Removes the mission worktrees — worktrees with uncommitted changes are kept and listed unless force is set. Tower mode stays active after teardown: the next objective starts with TowerInit, and the human turns the mode off explicitly with /tower off. The .tower/comms/ directory (state, inbox, findings, reviews, activity log) is always kept as the audit trail. +Removes the mission worktrees and reports one line per worktree. A worktree is kept, with the reason in the report, when: + +- its roster agent still has a running task in this session (`live agent: `) — this protection cannot be overridden, not even with force; +- it is named in exclude (`excluded`); +- it contains uncommitted changes (`uncommitted changes`) — removed only when force is set. + +Worktrees git no longer knows are reported as already removed. Set dry_run to preview the exact remove/keep decisions with their reasons before committing to them; a dry run changes nothing on disk or in state. + +Tower mode stays active after teardown: the next objective starts with TowerInit, and the human turns the mode off explicitly with /tower off. The .tower/comms/ directory (state, inbox, findings, reviews, activity log) is always kept as the audit trail. diff --git a/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.ts b/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.ts index b26b5acf5..49afd42e2 100644 --- a/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.ts +++ b/packages/agent-core-v2/src/features/tower/tools/teardown/teardown.ts @@ -8,7 +8,21 @@ export const TowerTeardownToolInputSchema = z force: z .boolean() .optional() - .describe('Remove worktrees even when they contain uncommitted changes'), + .describe( + 'Remove worktrees even when they contain uncommitted changes — a worktree whose roster agent is still running is kept regardless', + ), + exclude: z + .array(z.string()) + .optional() + .describe( + 'Worktree names to keep — they are skipped and reported, whatever their state', + ), + dry_run: z + .boolean() + .optional() + .describe( + 'Print the would-remove / would-keep list with reasons without changing anything on disk or in state', + ), }) .strict(); diff --git a/packages/agent-core-v2/src/features/tower/tools/teardown/teardownTool.ts b/packages/agent-core-v2/src/features/tower/tools/teardown/teardownTool.ts index 8515ccb3b..0555e2514 100644 --- a/packages/agent-core-v2/src/features/tower/tools/teardown/teardownTool.ts +++ b/packages/agent-core-v2/src/features/tower/tools/teardown/teardownTool.ts @@ -1,4 +1,5 @@ import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; +import { IAgentTaskService } from '#/agent/task/task'; import { ISessionManager } from '#/app/sessionManager/sessionManager'; import { TowerProtocolError } from '#/features/tower/protocol/index'; import { MAIN_AGENT_ID } from '#/session/agentLifecycle/agentLifecycle'; @@ -24,6 +25,7 @@ export class TowerTeardownTool implements ITowerTeardownTool { @ISessionContext private readonly sessionContext: ISessionContext, @ISessionManager private readonly sessions: ISessionManager, @IAgentScopeContext private readonly scopeContext: IAgentScopeContext, + @IAgentTaskService private readonly tasks: IAgentTaskService, ) {} resolveExecution(args: TowerTeardownToolInput): ToolExecution { @@ -34,7 +36,7 @@ export class TowerTeardownTool implements ITowerTeardownTool { }; } return { - description: `Tearing down tower workspace${args.force === true ? ' (force)' : ''}`, + description: `Tearing down tower workspace${args.dry_run === true ? ' (dry run)' : args.force === true ? ' (force)' : ''}`, approvalRule: this.name, execute: () => runTowerTool(async () => { @@ -52,10 +54,23 @@ export class TowerTeardownTool implements ITowerTeardownTool { `tower workspace is owned by a live session (${priorOwner}) — tearing it down would dismantle that session's fleet. Use TowerTeardown from that session, or close it first.`, ); } - const report = await store.teardown({ force: args.force }); + const liveAgentIds = new Set( + this.tasks + .list(true) + .map((task) => (task.kind === 'agent' ? task.agentId : undefined)) + .filter((agentId): agentId is string => agentId !== undefined), + ); + const report = await store.teardown({ + force: args.force, + exclude: args.exclude, + dryRun: args.dry_run, + liveAgentIds, + }); return { output: [ - 'tower teardown:', + args.dry_run === true + ? 'tower teardown (dry run — nothing was changed):' + : 'tower teardown:', ...report.map((line) => `- ${line}`), '', 'Tower mode stays active — the next objective starts with TowerInit, and the human can turn the mode off with /tower off. .tower/comms/ (state, inbox, findings, reviews, activity log) is kept as the audit trail — remove it by hand only if you are sure.', diff --git a/packages/agent-core-v2/src/features/tower/towerService.ts b/packages/agent-core-v2/src/features/tower/towerService.ts index 3c7ece2b8..adac70d75 100644 --- a/packages/agent-core-v2/src/features/tower/towerService.ts +++ b/packages/agent-core-v2/src/features/tower/towerService.ts @@ -3,10 +3,14 @@ import { join } from 'node:path'; import { Disposable, toDisposable } from '#/_base/di/lifecycle'; import { ScopeActivation, registerScopedService, type ISessionScopeHandle } from '#/_base/di/scope'; import { ILogService } from '#/_base/log/log'; +import { userCancellationReason } from '#/_base/utils/abort'; import { IAgentReminderService } from '#/features/reminder/reminderService'; import { IAgentLifecycleService } from '#/session/agentLifecycle/agentLifecycle'; import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory'; import { IAgentLoopService, type LoopNotifyHandle } from '#/agent/loop/loop'; +import { TurnStarted } from '#/agent/loop/turnEvents'; +import { TurnEnded } from '#/agent/loop/turnOps'; +import { PromptSubmitted } from '#/agent/prompt/promptEvents'; import { IAgentProfileService } from '#/agent/profile/profile'; import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { IAgentStateService } from '#/agent/state/agentState'; @@ -146,6 +150,47 @@ export class AgentTowerService extends Disposable implements IAgentTowerService }), ); } + this._register( + eventBus.subscribe(TurnStarted, (event) => { + if (this.agentCtx.agentId !== 'main') return; + if (!this.isActive) return; + if (event.agentId !== this.agentCtx.agentId) return; + if (event.origin.kind !== 'injection' || event.origin.variant !== TOWER_INBOX_WAKE_VARIANT) { + return; + } + this.wakeTurnId = event.turnId; + }), + ); + this._register( + eventBus.subscribe(TurnEnded, (event) => { + if (this.agentCtx.agentId !== 'main') return; + if (event.agentId !== this.agentCtx.agentId) return; + if (event.turnId === this.wakeTurnId) { + this.wakeTurnId = undefined; + return; + } + if (!this.wakeAbortedForUserPrompt) return; + if (this.loop === undefined || this.loop.snapshot().queue.length > 0) return; + this.wakeAbortedForUserPrompt = false; + this.inboxWakeSignals = Math.max(this.inboxWakeSignals, 1); + this.scheduleInboxWake(); + }), + ); + this._register( + eventBus.subscribe(PromptSubmitted, (event) => { + if (this.agentCtx.agentId !== 'main') return; + if (!this.isActive) return; + if (event.agentId !== this.agentCtx.agentId) return; + const turnId = this.wakeTurnId; + if (turnId === undefined) return; + queueMicrotask(() => { + if (this.wakeDisposed || !this.isActive || this.loop === undefined) return; + if (this.loop.cancel({ turnId }, userCancellationReason())) { + this.wakeAbortedForUserPrompt = true; + } + }); + }), + ); this._register( toDisposable(() => { this.wakeDisposed = true; @@ -466,27 +511,76 @@ export class AgentTowerService extends Disposable implements IAgentTowerService if (info.kind !== 'agent') return; if (info.agentId === undefined) return; if (info.status === 'completed') return; + if (!this.isActive) return; const store = new TowerStore(resolveTowerRepoRoot(this.sessionCtx.cwd)); - await store.markAgentDied(info.agentId, info.status, info.stopReason).then( + const foreignOwner = await this.resolveForeignStoreOwner(store); + if (foreignOwner !== undefined) { + this.log.info('tower: skipping roster agent death mark — tower store is owned by another session', { + event: 'TaskTerminatedNotice', + agentId: info.agentId, + sessionId: this.sessionCtx.sessionId, + owner: foreignOwner, + pid: process.pid, + }); + return; + } + this.log.info('tower: marking roster agent died', { + event: 'TaskTerminatedNotice', + agentId: info.agentId, + taskId: info.taskId, + status: info.status, + stopReason: info.stopReason, + sessionId: this.sessionCtx.sessionId, + pid: process.pid, + }); + await store.markAgentDied(info.agentId, info.status, info.stopReason, this.sessionCtx.sessionId).then( () => undefined, () => undefined, ); } private async clearTowerAgentDeath(agentId: string): Promise { + if (!this.isActive) return; const store = new TowerStore(resolveTowerRepoRoot(this.sessionCtx.cwd)); - await store.clearAgentDied(agentId).then( + const foreignOwner = await this.resolveForeignStoreOwner(store); + if (foreignOwner !== undefined) { + this.log.info('tower: skipping roster agent death clear — tower store is owned by another session', { + event: 'SubagentStarted', + agentId, + sessionId: this.sessionCtx.sessionId, + owner: foreignOwner, + pid: process.pid, + }); + return; + } + this.log.info('tower: clearing roster agent death mark', { + event: 'SubagentStarted', + agentId, + sessionId: this.sessionCtx.sessionId, + pid: process.pid, + }); + await store.clearAgentDied(agentId, this.sessionCtx.sessionId).then( () => undefined, () => undefined, ); } + private async resolveForeignStoreOwner(store: TowerStore): Promise { + const owner = await store.load().then( + (state) => state.sessionId, + () => undefined, + ); + return owner !== undefined && owner !== this.sessionCtx.sessionId ? owner : undefined; + } + private inboxWakeSignals = 0; private inboxWakeLatest: { readonly from: string; readonly subject: string } | undefined; private inboxWakeScheduled = false; private inboxWakePending = false; private inboxWakeHandle: LoopNotifyHandle | undefined; private wakeDisposed = false; + private wakeTurnId: number | undefined; + private wakeAbortedForUserPrompt = false; private onTowerInboxSent(event: TowerInboxSent): void { if (this.agentCtx.agentId !== 'main') return; diff --git a/packages/agent-core-v2/test/agent/loop/loop.test.ts b/packages/agent-core-v2/test/agent/loop/loop.test.ts index cdb485829..97908441e 100644 --- a/packages/agent-core-v2/test/agent/loop/loop.test.ts +++ b/packages/agent-core-v2/test/agent/loop/loop.test.ts @@ -1255,6 +1255,25 @@ describe('Agent loop', () => { [{ kind: 'file', name: 'note.txt', mediaType: 'text/plain', size: 21, path: '/data/note.txt' }], ]); }); + + it('settles the waiter of a notification-seeded turn', async () => { + ctx.mockNextResponse({ type: 'text', text: 'seeded answer' }); + loop.notify({ + message: { + role: 'user', + id: 'seed-1', + content: [{ type: 'text', text: 'wake up' }], + toolCalls: [], + }, + }); + await vi.waitFor(() => { + expect(loop.promptHandle('seed-1')).toBeDefined(); + }); + await expect(loop.promptHandle('seed-1')!.completion).resolves.toMatchObject({ + state: 'completed', + }); + await loop.settled(); + }); }); describe('turn telemetry', () => { diff --git a/packages/agent-core-v2/test/features/tower/store.test.ts b/packages/agent-core-v2/test/features/tower/store.test.ts index c965c11c7..407bcef5f 100644 --- a/packages/agent-core-v2/test/features/tower/store.test.ts +++ b/packages/agent-core-v2/test/features/tower/store.test.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process'; -import { mkdir, mkdtemp, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises'; +import { mkdir, mkdtemp, readdir, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { promisify } from 'node:util'; @@ -452,6 +452,28 @@ describe('markAgentDied', () => { expect(revivedLine).toContain('name=w1'); expect(revivedLine).toContain('agent=agent-w1'); }); + + it('ignores died/revived writes from a session that does not own the store', async () => { + await store.init('session-a'); + await store.registerAgent(rosterEntry({ name: 'w1', kind: 'worker', agentId: 'agent-w1' })); + + expect(await store.markAgentDied('agent-w1', 'failed', 'boom', 'session-b')).toBeUndefined(); + let state = await store.load(); + expect(state.roster.agents[0]?.diedAt).toBeUndefined(); + + await store.markAgentDied('agent-w1', 'failed', 'boom', 'session-a'); + state = await store.load(); + expect(state.roster.agents[0]?.diedAt).toBeDefined(); + + expect(await store.clearAgentDied('agent-w1', 'session-b')).toBe(false); + state = await store.load(); + expect(state.roster.agents[0]?.diedAt).toBeDefined(); + + expect(await store.clearAgentDied('agent-w1', 'session-a')).toBe(true); + const log = await readFile(join(repo, '.tower/comms/log/activity.log'), 'utf8'); + expect(log.split('\n').filter((line) => line.includes(' died '))).toHaveLength(1); + expect(log.split('\n').filter((line) => line.includes(' revived '))).toHaveLength(1); + }); }); describe('rebase', () => { @@ -762,6 +784,114 @@ describe('readInbox', () => { }); }); +describe('inbox read tracking', () => { + beforeEach(async () => { + await store.init(); + }); + + async function seedOwnedMission(spawnedAt = '2026-09-20T00:00:00.000Z'): Promise { + const [mission] = await store.plan([{ title: 'feature x', scope: ['src/feature-x/**'] }]); + const state = await store.load(); + await store.addWorktree(mission!.worktree, mission!.branch, state.base); + await commitFile(worktreeOf(mission!), 'src/feature-x/x.ts', 'x\n', `work on ${mission!.id}`); + await store.registerAgent( + rosterEntry({ + name: 'w1', + kind: 'worker', + missionId: mission!.id, + worktree: mission!.worktree, + branch: mission!.branch, + spawnedAt, + }), + ); + return mission!; + } + + it('seeds lastInboxReadAt from spawnedAt at registration', async () => { + await store.registerAgent( + rosterEntry({ name: 'w1', kind: 'worker', spawnedAt: '2026-09-20T10:00:00.000Z' }), + ); + + expect((await store.load()).roster.agents[0]?.lastInboxReadAt).toBe( + '2026-09-20T10:00:00.000Z', + ); + }); + + it('markInboxRead advances to the newest seen sent_at, falls back to now, and never regresses', async () => { + await store.registerAgent( + rosterEntry({ name: 'w1', kind: 'worker', spawnedAt: '2026-09-20T10:00:00.000Z' }), + ); + + await store.markInboxRead('w1', '2026-09-21T00:00:00.000Z'); + expect((await store.load()).roster.agents[0]?.lastInboxReadAt).toBe('2026-09-21T00:00:00.000Z'); + + await store.markInboxRead('w1', '2026-09-20T12:00:00.000Z'); + expect((await store.load()).roster.agents[0]?.lastInboxReadAt).toBe('2026-09-21T00:00:00.000Z'); + + await store.markInboxRead('w1'); + const at = (await store.load()).roster.agents[0]?.lastInboxReadAt; + expect(Date.parse(at!)).toBeGreaterThan(Date.parse('2026-09-21T00:00:00.000Z')); + + await store.markInboxRead('ghost', '2030-01-01T00:00:00.000Z'); + expect((await store.load()).roster.agents).toHaveLength(1); + }); + + it('refuses a worker completion while unread inbox messages wait and names the count', async () => { + const mission = await seedOwnedMission(); + await store.registerAgent( + rosterEntry({ name: 'w2', kind: 'worker', spawnedAt: '2026-09-20T00:00:00.000Z' }), + ); + await store.send('tower', { to: 'w1', subject: 'requirement change', body: 'add a poem' }); + await store.send('tower', { to: 'w1', subject: 'one more', body: 'and tests' }); + await store.send('tower', { to: 'w2', subject: 'not for w1', body: 'ignore me' }); + + await expect(store.updateMission('w1', mission.id, { status: 'completed' })).rejects.toThrow( + /cannot transition to completed — 2 unread inbox message\(s\) for w1.*call TowerInbox/s, + ); + expect((await store.load()).missions.find((m) => m.id === mission.id)?.status).toBe('planned'); + }); + + it('lets the worker complete once the inbox is read', async () => { + const mission = await seedOwnedMission(); + await store.send('tower', { to: 'w1', subject: 'requirement change', body: 'add a poem' }); + + await expect(store.updateMission('w1', mission.id, { status: 'completed' })).rejects.toThrow( + /unread inbox message/, + ); + + const items = await store.readInbox('w1', 20); + await store.markInboxRead('w1', items[0]?.sentAt); + + const completed = await store.updateMission('w1', mission.id, { status: 'completed' }); + expect(completed.status).toBe('completed'); + }); + + it('counts broadcasts as unread for the completing worker', async () => { + const mission = await seedOwnedMission(); + await store.send('tower', { to: 'all', subject: 'policy update', body: 'new rule' }); + + await expect(store.updateMission('w1', mission.id, { status: 'completed' })).rejects.toThrow( + /1 unread inbox message\(s\) for w1/, + ); + }); + + it('does not block on messages that predate the worker registration', async () => { + await store.send('tower', { to: 'all', subject: 'old broadcast', body: 'before spawn' }); + const mission = await seedOwnedMission(new Date().toISOString()); + + const completed = await store.updateMission('w1', mission.id, { status: 'completed' }); + expect(completed.status).toBe('completed'); + }); + + it('does not gate the tower completing a mission with unread worker messages', async () => { + const mission = await seedOwnedMission(); + await store.send('tower', { to: 'w1', subject: 'requirement change', body: 'add a poem' }); + + const completed = await store.updateMission('tower', mission.id, { status: 'completed' }); + expect(completed.status).toBe('completed'); + }); +}); + describe('findings', () => { beforeEach(async () => { await store.init(); @@ -1994,6 +2124,137 @@ describe('updateMission', () => { }); }); +describe('concurrent state writes', () => { + beforeEach(async () => { + await store.init(); + await store.plan([ + { title: 'alpha', scope: ['src/alpha/**'], tasks: ['scaffold', 'implement'] }, + ]); + }); + + async function expectNoLockOrTmpLeftovers(): Promise { + const comms = await readdir(join(repo, '.tower/comms')); + expect(comms.filter((name) => name.startsWith('state.json.'))).toEqual([]); + } + + it('serializes same-process concurrent updateMission calls without lost updates or a torn state file', async () => { + const notes = Array.from({ length: 8 }, (_, i) => `note-${String(i)}`); + let reading = true; + const reader = (async () => { + for (;;) { + if (!reading) return; + JSON.parse(await readFile(store.abs(STATE_FILE), 'utf8')); + } + })(); + + try { + await Promise.all(notes.map((note) => store.updateMission('tower', 'M1', { note }))); + } finally { + reading = false; + await reader; + } + + const state = await store.load(); + expect(state.missions[0]?.notes).toHaveLength(notes.length); + expect(state.missions[0]?.notes).toEqual(expect.arrayContaining(notes)); + JSON.parse(await readFile(store.abs(STATE_FILE), 'utf8')); + await expectNoLockOrTmpLeftovers(); + }); + + it('serializes concurrent updateMission calls from two separate TowerStore instances', async () => { + const other = new TowerStore(repo); + const fromStore = Array.from({ length: 5 }, (_, i) => `store-${String(i)}`); + const fromOther = Array.from({ length: 5 }, (_, i) => `other-${String(i)}`); + + await Promise.all([ + ...fromStore.map((note) => store.updateMission('tower', 'M1', { note })), + ...fromOther.map((note) => other.updateMission('tower', 'M1', { note })), + ]); + + const state = await store.load(); + expect(state.missions[0]?.notes).toHaveLength(fromStore.length + fromOther.length); + expect(state.missions[0]?.notes).toEqual( + expect.arrayContaining([...fromStore, ...fromOther]), + ); + JSON.parse(await readFile(store.abs(STATE_FILE), 'utf8')); + await expectNoLockOrTmpLeftovers(); + }); + + it('surfaces a held state lock as a clear error instead of a hang or a silent skip', async () => { + const lockPath = `${store.abs(STATE_FILE)}.lock`; + await writeFile(lockPath, 'pid=999999 since=2026-09-26T00:00:00.000Z', 'utf8'); + const blocked = new TowerStore(repo, { stateLockTimeoutMs: 200, stateLockPollMs: 20 }); + + await expect(blocked.updateMission('tower', 'M1', { note: 'x' })).rejects.toThrow( + /timed out.*tower state lock.*held by pid=999999/s, + ); + + expect((await store.load()).missions[0]?.notes).toEqual([]); + + await rm(lockPath, { force: true }); + await store.updateMission('tower', 'M1', { note: 'after stale lock removed' }); + expect((await store.load()).missions[0]?.notes).toEqual(['after stale lock removed']); + await expectNoLockOrTmpLeftovers(); + }); + + it('a stale release cannot delete another holder\'s state lock', async () => { + type LockDriver = { withStateLock: (fn: () => Promise) => Promise }; + const drive = (s: TowerStore, fn: () => Promise): Promise => + (s as unknown as LockDriver).withStateLock(fn); + const lockPath = `${store.abs(STATE_FILE)}.lock`; + const first = new TowerStore(repo); + const impatient = new TowerStore(repo, { stateLockTimeoutMs: 150, stateLockPollMs: 20 }); + const second = new TowerStore(repo); + + let markEntered!: () => void; + const entered = new Promise((resolve) => { + markEntered = resolve; + }); + let openGate!: () => void; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + const firstRun = drive(first, async () => { + markEntered(); + await gate; + }); + await entered; + + await expect(drive(impatient, async () => undefined)).rejects.toThrow(/tower state lock/); + + await rm(lockPath, { force: true }); + let releaseSecond!: () => void; + const secondGate = new Promise((resolve) => { + releaseSecond = resolve; + }); + const secondRun = drive(second, async () => { + await secondGate; + }); + + let heldContent = ''; + for (;;) { + try { + heldContent = await readFile(lockPath, 'utf8'); + break; + } catch { + await new Promise((resolve) => { + setTimeout(resolve, 10); + }); + } + } + expect(heldContent).toMatch(/pid=\d+ since=\S+ token=[0-9a-f-]+/); + + openGate(); + await firstRun; + + expect(await readFile(lockPath, 'utf8')).toBe(heldContent); + + releaseSecond(); + await secondRun; + await expect(readFile(lockPath, 'utf8')).rejects.toThrow(/ENOENT/); + }); +}); + describe('completion invariants', () => { beforeEach(async () => { await store.init(); @@ -2337,6 +2598,22 @@ describe('roster', () => { state = await store.load(); expect(state.roster.agents[1]?.diedAt).toBeUndefined(); }); + + it('attributes died and revived activity-log entries with the writing session and pid', async () => { + await store.registerAgent(rosterEntry({ name: 'w1', kind: 'worker', agentId: 'agent-w1' })); + + await store.markAgentDied('agent-w1', 'killed', 'stopped by the user', 'session-writer'); + const diedLog = (await store.recentLog(5)).join('\n'); + expect(diedLog).toContain(' died '); + expect(diedLog).toContain('session=session-writer'); + expect(diedLog).toContain(`pid=${String(process.pid)}`); + + expect(await store.clearAgentDied('agent-w1', 'session-writer')).toBe(true); + const revivedLog = (await store.recentLog(5)).join('\n'); + expect(revivedLog).toContain(' revived '); + expect(revivedLog).toContain('session=session-writer'); + expect(revivedLog).toContain(`pid=${String(process.pid)}`); + }); }); describe('adopt', () => { @@ -2535,6 +2812,116 @@ describe('teardown', () => { await rm(subRepo, { recursive: true, force: true }); } }); + + it('keeps worktrees whose roster agent is live, and force does not override that', async () => { + const mission = await setupMission({ + title: 'feature live', + scope: 'src/live/**', + file: 'src/live/l.ts', + content: 'l\n', + }); + await store.registerAgent( + rosterEntry({ + name: 'w-live', + kind: 'worker', + agentId: 'agent-live', + worktree: mission.worktree, + }), + ); + const live = new Set(['agent-live']); + + const report = await store.teardown({ liveAgentIds: live }); + expect(report.join('\n')).toContain( + `kept .tower/worktrees/${mission.worktree} (live agent: w-live)`, + ); + expect((await stat(worktreeOf(mission))).isDirectory()).toBe(true); + + const forced = await store.teardown({ force: true, liveAgentIds: live }); + expect(forced.join('\n')).toContain( + `kept .tower/worktrees/${mission.worktree} (live agent: w-live)`, + ); + expect((await stat(worktreeOf(mission))).isDirectory()).toBe(true); + + const settled = await store.teardown({ liveAgentIds: new Set() }); + expect(settled.join('\n')).toContain(`removed .tower/worktrees/${mission.worktree}`); + await expect(stat(worktreeOf(mission))).rejects.toThrow(); + }); + + it('skips worktrees named in exclude and reports excluded names that match nothing', async () => { + const mission = await setupMission({ + title: 'feature excluded', + scope: 'src/excluded/**', + file: 'src/excluded/e.ts', + content: 'e\n', + }); + + const report = await store.teardown({ exclude: [mission.worktree, 'wt-999'] }); + expect(report.join('\n')).toContain(`kept .tower/worktrees/${mission.worktree} (excluded)`); + expect(report.join('\n')).toContain('excluded worktree "wt-999" matched no mission worktree'); + expect((await stat(worktreeOf(mission))).isDirectory()).toBe(true); + + const after = await store.teardown(); + expect(after.join('\n')).toContain(`removed .tower/worktrees/${mission.worktree}`); + }); + + it('dry run reports the decisions without changing worktrees, state, or the log', async () => { + const clean = await setupMission({ + title: 'feature dry clean', + scope: 'src/dry-clean/**', + file: 'src/dry-clean/c.ts', + content: 'c\n', + }); + const dirty = await setupMission({ + title: 'feature dry dirty', + scope: 'src/dry-dirty/**', + file: 'src/dry-dirty/d.ts', + content: 'd\n', + }); + await writeFile(join(worktreeOf(dirty), 'uncommitted.txt'), 'dirty\n'); + const gone = await setupMission({ + title: 'feature dry gone', + scope: 'src/dry-gone/**', + file: 'src/dry-gone/g.ts', + content: 'g\n', + }); + await git(repo, 'worktree', 'remove', '--force', worktreeOf(gone)); + const live = await setupMission({ + title: 'feature dry live', + scope: 'src/dry-live/**', + file: 'src/dry-live/l.ts', + content: 'l\n', + }); + await store.registerAgent( + rosterEntry({ + name: 'w-dry-live', + kind: 'worker', + agentId: 'agent-dry-live', + worktree: live.worktree, + }), + ); + const logBefore = await store.recentLog(200); + + const report = await store.teardown({ + dryRun: true, + exclude: [gone.worktree], + liveAgentIds: new Set(['agent-dry-live']), + }); + const text = report.join('\n'); + expect(text).toContain(`would remove .tower/worktrees/${clean.worktree}`); + expect(text).toContain(`would keep .tower/worktrees/${dirty.worktree} (uncommitted changes`); + expect(text).toContain(`would keep .tower/worktrees/${live.worktree} (live agent: w-dry-live)`); + expect(text).toContain(`already removed .tower/worktrees/${gone.worktree}`); + expect(text).not.toMatch(/(^|\n)removed \.tower\/worktrees/); + expect(text).not.toContain(`excluded worktree "${gone.worktree}" matched no mission worktree`); + expect((await stat(worktreeOf(clean))).isDirectory()).toBe(true); + expect((await stat(worktreeOf(dirty))).isDirectory()).toBe(true); + expect((await stat(worktreeOf(live))).isDirectory()).toBe(true); + expect(await store.recentLog(200)).toEqual(logBefore); + + const real = await store.teardown({ liveAgentIds: new Set(['agent-dry-live']) }); + expect(real.join('\n')).toContain(`removed .tower/worktrees/${clean.worktree}`); + expect(real.join('\n')).toContain(`kept .tower/worktrees/${live.worktree} (live agent: w-dry-live)`); + }); }); describe('addWorktree branch ownership', () => { diff --git a/packages/agent-core-v2/test/features/tower/tools/spawnTool.test.ts b/packages/agent-core-v2/test/features/tower/tools/spawnTool.test.ts index b5b2703c4..e7521836c 100644 --- a/packages/agent-core-v2/test/features/tower/tools/spawnTool.test.ts +++ b/packages/agent-core-v2/test/features/tower/tools/spawnTool.test.ts @@ -307,7 +307,12 @@ describe('TowerSpawnTool', () => { expect(result.output).toContain('task_id: task-1'); expect(result.output).toContain('status: running'); expect(result.output).toContain(`worktree: ${worktreeAbs}`); - expect(result.output).toContain('Agent(resume="agent-7", run_in_background=true'); + expect(result.output).toContain('diagnose first: check why it died'); + expect(result.output).toContain('lost contact, timeout, OOM'); + expect(result.output).toContain('fixed or escalated to the human before any revive'); + expect(result.output).toMatch( + /diagnose first[\s\S]*Agent\(resume="agent-7", run_in_background=true/, + ); expect(createAgent).toHaveBeenCalledWith({ binding: { profile: 'tower-worker', model: 'pythinker-code', thinking: 'off' }, @@ -702,6 +707,35 @@ describe('TowerSpawnTool', () => { expect(prompt).toContain('subject="clarify-request"'); }); + it('briefs the worker to read the inbox and re-read its mission before completing', async () => { + const result = await execute(WORKER_ARGS); + + expect(result.isError).toBeUndefined(); + const prompt = (runAgent.mock.calls.at(-1)?.[1] as { prompt: string }).prompt; + expect(prompt).toContain('# When the mission is done'); + expect(prompt).toContain( + 'call TowerInbox once and incorporate anything new into the delivery', + ); + expect(prompt).toContain( + 'the store refuses status="completed" while unread messages wait in your inbox', + ); + expect(prompt).toContain('Re-read your mission too (TowerMission(id="M1") with no patch fields)'); + expect(prompt).toContain('split them into chunks and check TowerInbox between chunks'); + }); + + it('briefs the survey worker to read the inbox before completing', async () => { + const [survey] = await store.plan([ + { title: 'Scan apis', scope: ['src/**'], kind: 'survey' }, + ]); + + const result = await execute({ name: 'agent-scan', kind: 'worker', mission_id: survey!.id }); + + expect(result.isError).toBeUndefined(); + const prompt = (runAgent.mock.calls.at(-1)?.[1] as { prompt: string }).prompt; + expect(prompt).toContain('# When the survey is done'); + expect(prompt).toContain('Call TowerInbox once and fold anything new into your summary'); + }); + it('briefs the reviewer with the mission text and the worker self-report', async () => { const [docs] = await store.plan([ { diff --git a/packages/agent-core-v2/test/features/tower/tools/towerTools.test.ts b/packages/agent-core-v2/test/features/tower/tools/towerTools.test.ts index 80b69c6de..813dcb85f 100644 --- a/packages/agent-core-v2/test/features/tower/tools/towerTools.test.ts +++ b/packages/agent-core-v2/test/features/tower/tools/towerTools.test.ts @@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { DisposableStore } from '#/_base/di/lifecycle'; import type { ServiceIdentifier } from '#/_base/di/instantiation'; import { createServices, type TestInstantiationService } from '#/_base/di/test'; +import { userCancellationReason } from '#/_base/utils/abort'; import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { IAgentTaskService, type AgentTaskInfo } from '#/agent/task/task'; import type { AnyAgentTool } from '#/agent/toolRegistry/toolContribution'; @@ -466,6 +467,78 @@ describe('TowerTeardownTool', () => { expect(result.isError).toBeFalsy(); expect(result.output).toContain('tower teardown:'); }); + + it('keeps worktrees whose roster agent has a running task, even with force', async () => { + await initViaTool(); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'Build engine', scope: ['src/engine/**'] }], + }); + const store = new TowerStore(repo); + const state = await store.load(); + const mission = state.missions[0]!; + await store.addWorktree(mission.worktree, mission.branch, state.base); + await store.registerAgent({ + name: 'w1', + kind: 'worker', + agentId: 'agent-w1', + worktree: mission.worktree, + spawnedAt: new Date().toISOString(), + }); + liveAgentTaskIds.push('agent-w1'); + + const result = await run(ix.get(ITowerTeardownTool), { force: true }); + + expect(result.isError).toBeFalsy(); + expect(result.output).toContain( + `kept .tower/worktrees/${mission.worktree} (live agent: w1)`, + ); + expect( + (await stat(join(repo, '.tower/worktrees', mission.worktree))).isDirectory(), + ).toBe(true); + + liveAgentTaskIds.length = 0; + const settled = await run(ix.get(ITowerTeardownTool), {}); + expect(settled.output).toContain(`removed .tower/worktrees/${mission.worktree}`); + }); + + it('dry run reports the decisions and changes nothing', async () => { + await initViaTool(); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'Build engine', scope: ['src/engine/**'] }], + }); + const store = new TowerStore(repo); + const state = await store.load(); + const mission = state.missions[0]!; + await store.addWorktree(mission.worktree, mission.branch, state.base); + + const result = await run(ix.get(ITowerTeardownTool), { dry_run: true }); + + expect(result.isError).toBeFalsy(); + expect(result.output).toContain('tower teardown (dry run'); + expect(result.output).toContain(`would remove .tower/worktrees/${mission.worktree}`); + expect( + (await stat(join(repo, '.tower/worktrees', mission.worktree))).isDirectory(), + ).toBe(true); + }); + + it('keeps worktrees named in exclude', async () => { + await initViaTool(); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'Build engine', scope: ['src/engine/**'] }], + }); + const store = new TowerStore(repo); + const state = await store.load(); + const mission = state.missions[0]!; + await store.addWorktree(mission.worktree, mission.branch, state.base); + + const result = await run(ix.get(ITowerTeardownTool), { exclude: [mission.worktree] }); + + expect(result.isError).toBeFalsy(); + expect(result.output).toContain(`kept .tower/worktrees/${mission.worktree} (excluded)`); + expect( + (await stat(join(repo, '.tower/worktrees', mission.worktree))).isDirectory(), + ).toBe(true); + }); }); describe('TowerSendTool + TowerInboxTool', () => { @@ -592,6 +665,49 @@ describe('TowerSendTool + TowerInboxTool', () => { const fromWorker = await run(ix.get(ITowerSendTool), { to: 'w2', subject: 'b', body: 'x' }); expect(fromWorker.output).not.toContain('has no running task'); }); + + it('marks the caller inbox read, so a completion refused for unread messages passes after TowerInbox', async () => { + const store = new TowerStore(repo); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'Build engine', scope: ['src/engine/**'] }], + }); + await git(repo, 'checkout', '-b', 'feat/build-engine'); + await commitFile(repo, 'src/engine/engine.ts', 'export const engine = 1;\n', 'engine work'); + await git(repo, 'checkout', 'main'); + await store.registerAgent({ + name: 'w3', + kind: 'worker', + agentId: 'agent-w3', + missionId: 'M1', + branch: 'feat/build-engine', + spawnedAt: '2026-09-20T00:00:00.000Z', + }); + await store.updateMission('tower', 'M1', { status: 'active', owner: 'w3' }, { silent: true }); + const sent = await run(ix.get(ITowerSendTool), { + to: 'w3', + subject: 'requirement change', + body: 'add tests', + }); + expect(sent.isError).toBeFalsy(); + + currentAgentId = 'agent-w3'; + const refused = await run(ix.get(ITowerMissionTool), { id: 'M1', status: 'completed' }); + expect(refused.isError).toBe(true); + expect(refused.output).toContain('1 unread inbox message(s) for w3'); + expect(refused.output).toContain('call TowerInbox'); + + const inbox = await run(ix.get(ITowerInboxTool), {}); + expect(inbox.isError).toBeFalsy(); + expect(inbox.output).toContain('subject: requirement change'); + const entry = (await store.load()).roster.agents.find((agent) => agent.name === 'w3'); + expect(Date.parse(entry!.lastInboxReadAt!)).toBeGreaterThan( + Date.parse('2026-09-20T00:00:00.000Z'), + ); + + const accepted = await run(ix.get(ITowerMissionTool), { id: 'M1', status: 'completed' }); + expect(accepted.isError).toBeFalsy(); + expect(accepted.output).toContain('status: completed'); + }); }); describe('TowerStatusTool', () => { @@ -629,7 +745,63 @@ describe('TowerStatusTool', () => { expect(result.output).toContain('💀 failed'); expect(result.output).toContain('## Dead workers'); expect(result.output).toContain('M1 owner w1 died (failed)'); - expect(result.output).toContain('Agent(resume="agent-w1", run_in_background=true'); + expect(result.output).toContain('diagnose first: check why it died'); + expect(result.output).toContain('lost contact, timeout, OOM'); + expect(result.output).toMatch( + /died \(failed\) — diagnose first[\s\S]*Agent\(resume="agent-w1", run_in_background=true/, + ); + }); + + it('advises fixing or escalating a systematic death cause before any revive', async () => { + await initViaTool(); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'engine', scope: ['src/engine/**'] }], + }); + const store = new TowerStore(repo); + await store.registerAgent({ + name: 'w1', + kind: 'worker', + agentId: 'agent-w1', + missionId: 'M1', + spawnedAt: new Date().toISOString(), + }); + await store.updateMission('tower', 'M1', { status: 'active', owner: 'w1' }, { silent: true }); + await store.markAgentDied('agent-w1', 'failed', 'TS2304: Cannot find name'); + + const result = await run(ix.get(ITowerStatusTool), {}); + + expect(result.isError).toBeFalsy(); + expect(result.output).toContain('M1 owner w1 died (failed)'); + expect(result.output).toContain('systematic cause'); + expect(result.output).toContain('fixed or escalated to the human before any revive'); + }); + + it('shows a user-stopped roster agent without the recovery hint', async () => { + await initViaTool(); + await run(ix.get(ITowerPlanTool), { + missions: [{ title: 'engine', scope: ['src/engine/**'] }], + }); + const store = new TowerStore(repo); + await store.registerAgent({ + name: 'w1', + kind: 'worker', + agentId: 'agent-w1', + missionId: 'M1', + spawnedAt: new Date().toISOString(), + }); + await store.updateMission('tower', 'M1', { status: 'active', owner: 'w1' }, { silent: true }); + await store.markAgentDied('agent-w1', 'killed', userCancellationReason().message); + + const result = await run(ix.get(ITowerStatusTool), {}); + + expect(result.isError).toBeFalsy(); + expect(result.output).toContain('w1 (worker) — agent agent-w1, mission M1'); + expect(result.output).toContain('💀 killed'); + expect(result.output).toContain('## Dead workers'); + expect(result.output).toContain('M1 owner w1 was stopped by the user (killed)'); + expect(result.output).toContain('dead by intent'); + expect(result.output).not.toContain('diagnose first'); + expect(result.output).not.toContain('Agent(resume='); }); it('flags planned missions without a spawned worker in an Awaiting spawn section', async () => { diff --git a/packages/agent-core-v2/test/features/tower/towerService.test.ts b/packages/agent-core-v2/test/features/tower/towerService.test.ts index 47b2d9cb3..936b612e9 100644 --- a/packages/agent-core-v2/test/features/tower/towerService.test.ts +++ b/packages/agent-core-v2/test/features/tower/towerService.test.ts @@ -16,6 +16,10 @@ import { createReminderStub } from '../reminder/stubs'; import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory'; import type { ContextMessage } from '#/agent/contextMemory/types'; import { IAgentLoopService } from '#/agent/loop/loop'; +import { TurnStarted } from '#/agent/loop/turnEvents'; +import { TurnEnded } from '#/agent/loop/turnOps'; +import { PromptSubmitted } from '#/agent/prompt/promptEvents'; +import { isUserCancellation } from '#/_base/utils/abort'; import { runWillBeginStepHooks, stubLoopWithHooks, type StubLoop } from '../../agent/loop/stubs'; import { IAgentProfileService } from '#/agent/profile/profile'; import { IAgentToolPolicyService } from '#/agent/toolPolicy/toolPolicy'; @@ -26,7 +30,7 @@ import type { BeforeExecuteDecision, ResolvedToolExecutionHookContext, } from '#/agent/toolExecutor/toolHooks'; -import { TowerStore } from '#/features/tower/protocol/index'; +import { STATE_FILE, TowerStore, type TowerState } from '#/features/tower/protocol/index'; import { TowerSendTool } from '#/features/tower/tools/send/sendTool'; import { IAgentTowerService, @@ -611,8 +615,16 @@ describe('AgentTowerService', () => { missionId: 'M1', spawnedAt: new Date().toISOString(), }); + const infos: { msg: string; payload?: unknown }[] = []; + ix.stub(ILogService, { + ...stubLog(), + info: (msg: string, payload?: unknown) => { + infos.push({ msg, payload }); + }, + }); ix.stub(ISessionContext, { cwd: repo, sessionId: 'session-main' } as unknown as ISessionContext); - ix.get(IAgentTowerService); + const tower = ix.get(IAgentTowerService); + await tower.enter(); publishAsMain( ix, @@ -639,6 +651,22 @@ describe('AgentTowerService', () => { const state = await store.load(); expect(state.roster.agents[0]?.diedAt).toBeDefined(); expect(state.roster.agents[0]?.deathReason).toBe('provider blew up'); + + const activityLog = await readFile(join(repo, '.tower/comms/log/activity.log'), 'utf8'); + expect(activityLog).toContain(' died '); + expect(activityLog).toContain('session=session-main'); + expect(activityLog).toContain(`pid=${String(process.pid)}`); + + const notice = infos.find((entry) => entry.msg === 'tower: marking roster agent died'); + expect(notice?.payload).toMatchObject({ + event: 'TaskTerminatedNotice', + agentId: 'agent-w1', + taskId: 'agent-dead1', + status: 'failed', + stopReason: 'provider blew up', + sessionId: 'session-main', + pid: process.pid, + }); } finally { await rm(repo, { recursive: true, force: true }); } @@ -661,7 +689,8 @@ describe('AgentTowerService', () => { spawnedAt: new Date().toISOString(), }); ix.stub(ISessionContext, { cwd: repo, sessionId: 'session-main' } as unknown as ISessionContext); - ix.get(IAgentTowerService); + const tower = ix.get(IAgentTowerService); + await tower.enter(); const info = { taskId: 'agent-fine1', @@ -676,8 +705,8 @@ describe('AgentTowerService', () => { const originalMarkDied = TowerStore.prototype.markAgentDied; const markSpy = vi .spyOn(TowerStore.prototype, 'markAgentDied') - .mockImplementation(function (this: TowerStore, agentId, status, reason) { - const pending = originalMarkDied.call(this, agentId, status, reason); + .mockImplementation(function (this: TowerStore, agentId, status, reason, sessionId) { + const pending = originalMarkDied.call(this, agentId, status, reason, sessionId); deathSettled = pending.then( () => undefined, () => undefined, @@ -698,7 +727,7 @@ describe('AgentTowerService', () => { ); await vi.waitFor(() => expect(markSpy).toHaveBeenCalledTimes(1)); - expect(markSpy).toHaveBeenCalledWith('agent-stranger', 'failed', undefined); + expect(markSpy).toHaveBeenCalledWith('agent-stranger', 'failed', undefined, 'session-main'); await deathSettled; const state = await store.load(); expect(state.roster.agents[0]?.diedAt).toBeUndefined(); @@ -728,8 +757,16 @@ describe('AgentTowerService', () => { spawnedAt: new Date().toISOString(), }); await store.markAgentDied('agent-w1', 'failed', 'provider blew up'); + const infos: { msg: string; payload?: unknown }[] = []; + ix.stub(ILogService, { + ...stubLog(), + info: (msg: string, payload?: unknown) => { + infos.push({ msg, payload }); + }, + }); ix.stub(ISessionContext, { cwd: repo, sessionId: 'session-main' } as unknown as ISessionContext); - ix.get(IAgentTowerService); + const tower = ix.get(IAgentTowerService); + await tower.enter(); publishAsMain(ix, new SubagentStarted({ subagentId: 'agent-w1' })); @@ -741,6 +778,167 @@ describe('AgentTowerService', () => { expect(state.roster.agents[0]?.deathStatus).toBeUndefined(); const log = await readFile(join(repo, '.tower/comms/log/activity.log'), 'utf8'); expect(log).toContain('revived'); + expect(log).toContain('session=session-main'); + expect(log).toContain(`pid=${String(process.pid)}`); + + const notice = infos.find((entry) => entry.msg === 'tower: clearing roster agent death mark'); + expect(notice?.payload).toMatchObject({ + event: 'SubagentStarted', + agentId: 'agent-w1', + sessionId: 'session-main', + pid: process.pid, + }); + } finally { + await rm(repo, { recursive: true, force: true }); + } + }); + + it('does not record or clear deaths while tower mode is inactive', async () => { + const repo = await mkdtemp(join(tmpdir(), 'tower-death-inactive-')); + try { + await initGitRepo(repo); + await writeFile(join(repo, 'README.md'), '# fixture\n'); + await execFileAsync('git', ['add', 'README.md'], { cwd: repo }); + await execFileAsync('git', ['commit', '-m', 'initial'], { cwd: repo }); + const store = new TowerStore(repo); + await store.init('session-main'); + await store.registerAgent({ + name: 'w1', + kind: 'worker', + agentId: 'agent-w1', + sessionId: 'session-main', + spawnedAt: new Date().toISOString(), + }); + await store.markAgentDied('agent-w1', 'failed', 'provider blew up'); + const infos: { msg: string; payload?: unknown }[] = []; + ix.stub(ILogService, { + ...stubLog(), + info: (msg: string, payload?: unknown) => { + infos.push({ msg, payload }); + }, + }); + ix.stub(ISessionContext, { cwd: repo, sessionId: 'session-main' } as unknown as ISessionContext); + const tower = ix.get(IAgentTowerService); + expect(tower.isActive).toBe(false); + + const markSpy = vi.spyOn(TowerStore.prototype, 'markAgentDied'); + const clearSpy = vi.spyOn(TowerStore.prototype, 'clearAgentDied'); + try { + publishAsMain( + ix, + new TaskTerminatedNotice({ + agentId: 'main', + info: { + taskId: 'agent-dead2', + kind: 'agent', + description: 'tower worker w1: engine', + status: 'failed', + stopReason: 'provider blew up', + startedAt: 1, + endedAt: 2, + agentId: 'agent-w1', + subagentType: 'tower-worker', + }, + }), + ); + publishAsMain(ix, new SubagentStarted({ subagentId: 'agent-w1' })); + + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(markSpy).not.toHaveBeenCalled(); + expect(clearSpy).not.toHaveBeenCalled(); + } finally { + markSpy.mockRestore(); + clearSpy.mockRestore(); + } + const state = await store.load(); + expect(state.roster.agents[0]?.diedAt).toBeDefined(); + expect(infos.some((entry) => entry.msg.startsWith('tower:'))).toBe(false); + } finally { + await rm(repo, { recursive: true, force: true }); + } + }); + + it('leaves the roster untouched when another session owns the tower store', async () => { + const repo = await mkdtemp(join(tmpdir(), 'tower-death-foreign-')); + try { + await initGitRepo(repo); + await writeFile(join(repo, 'README.md'), '# fixture\n'); + await execFileAsync('git', ['add', 'README.md'], { cwd: repo }); + await execFileAsync('git', ['commit', '-m', 'initial'], { cwd: repo }); + const store = new TowerStore(repo); + await store.init('session-main'); + await store.registerAgent({ + name: 'w1', + kind: 'worker', + agentId: 'agent-w1', + sessionId: 'session-main', + spawnedAt: new Date().toISOString(), + }); + await store.registerAgent({ + name: 'w2', + kind: 'worker', + agentId: 'agent-w2', + sessionId: 'session-main', + spawnedAt: new Date().toISOString(), + }); + const infos: { msg: string; payload?: unknown }[] = []; + ix.stub(ILogService, { + ...stubLog(), + info: (msg: string, payload?: unknown) => { + infos.push({ msg, payload }); + }, + }); + ix.stub(ISessionContext, { cwd: repo, sessionId: 'session-main' } as unknown as ISessionContext); + const tower = ix.get(IAgentTowerService); + await tower.enter(); + expect(tower.isActive).toBe(true); + + const stateFile = store.abs(STATE_FILE); + const owned = JSON.parse(await readFile(stateFile, 'utf8')) as TowerState; + owned.sessionId = 'session-other'; + await writeFile(stateFile, `${JSON.stringify(owned, null, 2)}\n`); + await store.markAgentDied('agent-w2', 'failed', 'provider blew up'); + + publishAsMain( + ix, + new TaskTerminatedNotice({ + agentId: 'main', + info: { + taskId: 'agent-dead3', + kind: 'agent', + description: 'tower worker w1: engine', + status: 'failed', + stopReason: 'provider blew up', + startedAt: 1, + endedAt: 2, + agentId: 'agent-w1', + subagentType: 'tower-worker', + }, + }), + ); + publishAsMain(ix, new SubagentStarted({ subagentId: 'agent-w2' })); + + await new Promise((resolve) => setTimeout(resolve, 20)); + const state = await store.load(); + expect(state.roster.agents[0]?.diedAt).toBeUndefined(); + expect(state.roster.agents[1]?.diedAt).toBeDefined(); + + const activityLog = await readFile(join(repo, '.tower/comms/log/activity.log'), 'utf8'); + expect(activityLog.split('\n').filter((line) => line.includes(' died '))).toHaveLength(1); + expect(activityLog).not.toContain(' revived '); + + const skips = infos.filter((entry) => entry.msg.includes('skipping')); + expect(skips.map((entry) => entry.msg)).toEqual([ + 'tower: skipping roster agent death mark — tower store is owned by another session', + 'tower: skipping roster agent death clear — tower store is owned by another session', + ]); + expect(skips[0]?.payload).toMatchObject({ + event: 'TaskTerminatedNotice', + agentId: 'agent-w1', + sessionId: 'session-main', + owner: 'session-other', + pid: process.pid, + }); } finally { await rm(repo, { recursive: true, force: true }); } @@ -2727,6 +2925,111 @@ describe('AgentTowerService', () => { expect(drainWakeMessages()).toEqual([]); expect(loop.snapshot().hasPendingRequests).toBe(false); }); + + function publishUserPrompt(promptId: string): void { + publishAsMain( + ix, + new PromptSubmitted({ + agentId: 'main', + promptId, + userMessageId: promptId, + status: 'queued', + content: [{ type: 'text', text: 'hold on' }], + createdAt: new Date().toISOString(), + }), + ); + } + + it('cancels the inbox wake turn when a user prompt is submitted', async () => { + const tower = ix.get(IAgentTowerService); + await tower.enter(); + + const wakeTurn = loop.startTurn(); + publishAsMain( + ix, + new TurnStarted({ + agentId: 'main', + turnId: wakeTurn.id, + origin: { kind: 'injection', variant: TOWER_INBOX_WAKE_VARIANT }, + }), + ); + publishUserPrompt('p1'); + await flushWake(); + + expect(loop.cancels).toHaveLength(1); + expect(loop.cancels[0]?.turnId).toBe(wakeTurn.id); + expect(isUserCancellation(loop.cancels[0]?.reason)).toBe(true); + }); + + it('ignores turns seeded by other notification origins', async () => { + const tower = ix.get(IAgentTowerService); + await tower.enter(); + + const cronTurn = loop.startTurn(); + publishAsMain( + ix, + new TurnStarted({ + agentId: 'main', + turnId: cronTurn.id, + origin: { kind: 'injection', variant: 'cron' }, + }), + ); + publishUserPrompt('p1'); + await flushWake(); + + expect(loop.cancels).toEqual([]); + }); + + it('does not cancel after the wake turn has already ended', async () => { + const tower = ix.get(IAgentTowerService); + await tower.enter(); + + const wakeTurn = loop.startTurn(); + publishAsMain( + ix, + new TurnStarted({ + agentId: 'main', + turnId: wakeTurn.id, + origin: { kind: 'injection', variant: TOWER_INBOX_WAKE_VARIANT }, + }), + ); + publishAsMain(ix, new TurnEnded({ agentId: 'main', turnId: wakeTurn.id, reason: 'completed' })); + publishUserPrompt('p1'); + await flushWake(); + + expect(loop.cancels).toEqual([]); + }); + + it('re-arms the inbox wake after the interrupting user turn ends', async () => { + const tower = ix.get(IAgentTowerService); + await tower.enter(); + + publishInbox({ from: 'w1', to: 'tower', subject: 'need review' }); + await flushWake(); + expect(drainWakeMessages()).toHaveLength(1); + + const wakeTurn = loop.startTurn(); + publishAsMain( + ix, + new TurnStarted({ + agentId: 'main', + turnId: wakeTurn.id, + origin: { kind: 'injection', variant: TOWER_INBOX_WAKE_VARIANT }, + }), + ); + publishUserPrompt('p1'); + await flushWake(); + expect(loop.cancels).toHaveLength(1); + + publishAsMain(ix, new TurnEnded({ agentId: 'main', turnId: wakeTurn.id, reason: 'cancelled' })); + publishAsMain(ix, new TurnEnded({ agentId: 'main', turnId: 99, reason: 'completed' })); + await flushWake(); + + const rearmed = drainWakeMessages(); + expect(rearmed).toHaveLength(1); + expect(wakeText(rearmed[0]!)).toContain('1 new tower inbox message'); + expect(wakeText(rearmed[0]!)).toContain('w1'); + }); }); describe('roster resume veto', () => { @@ -2929,6 +3232,8 @@ describe('TowerModeInjection', () => { expect(text).toContain('Tower mode is active'); expect(text).toContain('TowerSpawn'); expect(text).toContain('TowerMerge'); + expect(text).toContain('TowerSend` is delivery, not interruption'); + expect(text).toContain('no silent miss is possible'); }); it('injects the exit reminder when tower mode turns off after being active', async () => { diff --git a/packages/agent-gateway/test/modelCatalogCatalog.test.ts b/packages/agent-gateway/test/modelCatalogCatalog.test.ts index de6309db5..d8dffd8ae 100644 --- a/packages/agent-gateway/test/modelCatalogCatalog.test.ts +++ b/packages/agent-gateway/test/modelCatalogCatalog.test.ts @@ -195,15 +195,6 @@ describe('server-v2 /api/v1 catalog browse + import endpoints', () => { return parseToml(text) as Record; } - async function waitForServerState(check: () => Promise, timeoutMs = 10000): Promise { - const deadline = Date.now() + timeoutMs; - while (Date.now() < deadline) { - if (await check()) return; - await new Promise((resolve) => setTimeout(resolve, 50)); - } - throw new Error('waitForServerState timed out'); - } - it('lists pruned directory entries with import eligibility resolved', async () => { await boot(); const { status, body } = await getJson<{ items: Array> }>( @@ -372,10 +363,7 @@ describe('server-v2 /api/v1 catalog browse + import endpoints', () => { models['openai/retired'] = { provider: 'openai', model: 'retired', max_context_size: 1 }; const { stringify: stringifyToml } = await import('smol-toml'); await writeFile(join(home as string, 'config.toml'), stringifyToml(before), 'utf-8'); - await waitForServerState(async () => { - const cfg = await getJson<{ models: Record }>('/api/v1/config'); - return 'openai/retired' in (cfg.body.data.models ?? {}); - }); + await (server as RunningServer).core.accessor.get(IConfigService).reload(); const second = await postJson('/api/v1/providers:import_catalog', { catalog_id: 'openai', From 00148ca5f305ef3ce87b547c2b971a67408feda3 Mon Sep 17 00:00:00 2001 From: elkaix Date: Thu, 1 Oct 2026 07:12:44 -0400 Subject: [PATCH 3/9] fix(agent-core-v2): fold hook output into the prompt message as marked parts --- .../src/tui/controllers/session-replay.ts | 47 +++++-- .../src/tui/utils/export-markdown.ts | 9 +- .../src/tui/utils/message-replay.ts | 74 ++++++++-- .../test/tui/export-markdown.test.ts | 16 +++ .../test/tui/message-replay.test.ts | 37 ++++- .../tui/pythinker-tui-message-flow.test.ts | 6 +- apps/vscode/src/runtime/replay-adapter.ts | 18 ++- apps/vscode/src/utils/hook-parts.ts | 21 +++ apps/vscode/src/utils/session-context.ts | 4 +- apps/vscode/test/replay-adapter.test.ts | 118 +++++----------- .../agent-core-v2/docs/state-manifest.d.ts | 12 ++ .../agent/contextMemory/compactionHandoff.ts | 9 +- .../src/agent/contextMemory/hookParts.ts | 19 +++ packages/agent-core-v2/src/agent/loop/loop.ts | 3 +- .../src/agent/loop/loopService.ts | 12 +- .../src/agent/loop/turnEvents.ts | 9 +- .../src/agent/undo/undoService.ts | 5 +- .../agent/agentExternalHooksService.ts | 23 ++-- .../externalHooks/internal/userPrompt.ts | 19 ++- .../src/features/skill/skillService.ts | 12 +- .../src/human/agent/historySchema.ts | 6 +- .../agent-core-v2/src/human/agent/origin.ts | 47 ++++++- .../agent-core-v2/src/human/llm/message.ts | 8 ++ .../src/human/test/agent/machine.test.ts | 2 +- .../agentTitlePromptSourceService.ts | 11 +- .../internal/forkTurnSlice.ts | 15 +- .../test/agent/contextMemory/context.test.ts | 21 +++ .../test/agent/prompt/promptService.test.ts | 68 ++++++++- .../test/agent/undo/undo.test.ts | 19 ++- .../externalHooks/integration.test.ts | 20 ++- .../features/externalHooks/runner.test.ts | 38 ++++++ packages/agent-gateway/src/routes/prompts.ts | 8 +- .../src/services/transcript/coreEventMap.ts | 8 +- .../services/transcript/transcriptService.ts | 9 +- packages/agent-gateway/test/prompts.test.ts | 5 + .../test/services/transcript.test.ts | 6 +- packages/transcript/src/history/groupTurns.ts | 121 +++++++++++++--- packages/transcript/test/layers.test.ts | 129 ++++++++++++------ 38 files changed, 775 insertions(+), 239 deletions(-) create mode 100644 apps/vscode/src/utils/hook-parts.ts create mode 100644 packages/agent-core-v2/src/agent/contextMemory/hookParts.ts diff --git a/apps/pythinker-code/src/tui/controllers/session-replay.ts b/apps/pythinker-code/src/tui/controllers/session-replay.ts index 6547c65d1..96b197db0 100644 --- a/apps/pythinker-code/src/tui/controllers/session-replay.ts +++ b/apps/pythinker-code/src/tui/controllers/session-replay.ts @@ -37,6 +37,7 @@ import { createReplayRenderContext, formatHookResultMessageForTranscript, isTerminalBackgroundTask, + isUserPromptSubmitHookPart, limitReplayRecordsByTurn, REPLAY_TURN_LIMIT, replayBackgroundProjection, @@ -46,6 +47,7 @@ import { pluginCommandFromOrigin, toolCallFromReplayMessage, toolResultOutput, + withoutUserPromptSubmitHookParts, type BackgroundTaskNotificationOrigin, type ReplayRenderContext, type SkillActivationProjection, @@ -410,6 +412,7 @@ export class SessionReplayRenderer { if (message.origin?.kind === 'skill_activation' && message.origin.trigger === 'user-slash') { this.advanceTurn(context); } + this.renderHookParts(context, message); return; } const pluginCommand = pluginCommandFromOrigin(message.origin); @@ -418,6 +421,7 @@ export class SessionReplayRenderer { if (message.origin?.kind === 'plugin_command' && message.origin.trigger === 'user-slash') { this.advanceTurn(context); } + this.renderHookParts(context, message); return; } @@ -426,11 +430,26 @@ export class SessionReplayRenderer { return; } this.advanceTurn(context); + this.renderHookParts(context, message); this.host.appendTranscriptEntry( - replayEntry(context, 'user', contentPartsToText(message.content), 'plain'), + replayEntry( + context, + 'user', + contentPartsToText(withoutUserPromptSubmitHookParts(message.content)), + 'plain', + ), ); } + private renderHookParts(context: ReplayRenderContext, message: ContextMessage): void { + for (const part of message.content.filter(isUserPromptSubmitHookPart)) { + this.renderHookResultEntry( + context, + formatHookResultMessageForTranscript(part.text, 'UserPromptSubmit', false), + ); + } + } + private renderBundledPrompt( context: ReplayRenderContext, message: ContextMessage, @@ -444,8 +463,10 @@ export class SessionReplayRenderer { for (const hookResult of hookResults) { this.renderHookResult(context, hookResult); } + this.renderHookParts(context, message); + const callerMessage = { ...message, content: withoutUserPromptSubmitHookParts(message.content) }; this.host.appendTranscriptEntry( - replayEntry(context, 'user', contentPartsToText(stripBundledSkillParts(message)), 'plain'), + replayEntry(context, 'user', contentPartsToText(stripBundledSkillParts(callerMessage)), 'plain'), ); } @@ -636,18 +657,20 @@ export class SessionReplayRenderer { private renderHookResult(context: ReplayRenderContext, message: ContextMessage): void { if (message.origin?.kind !== 'hook_result') return; + this.renderHookResultEntry( + context, + formatHookResultMessageForTranscript( + contentPartsToText(message.content), + message.origin.event, + message.origin.blocked === true, + ), + ); + } + + private renderHookResultEntry(context: ReplayRenderContext, formatted: string): void { this.flushAssistant(context); this.host.appendTranscriptEntry({ - ...replayEntry( - context, - 'assistant', - formatHookResultMessageForTranscript( - contentPartsToText(message.content), - message.origin.event, - message.origin.blocked === true, - ), - 'markdown', - ), + ...replayEntry(context, 'assistant', formatted, 'markdown'), hookResult: true, }); } diff --git a/apps/pythinker-code/src/tui/utils/export-markdown.ts b/apps/pythinker-code/src/tui/utils/export-markdown.ts index 1fcb89880..0a4451991 100644 --- a/apps/pythinker-code/src/tui/utils/export-markdown.ts +++ b/apps/pythinker-code/src/tui/utils/export-markdown.ts @@ -1,5 +1,7 @@ import type { ContentPart, ContextMessage, PromptOrigin, ToolCall } from '@pymodel/pythinker-code-sdk'; +import { isUserPromptSubmitHookPart, withoutUserPromptSubmitHookParts } from './message-replay'; + const HINT_KEYS = ['path', 'file_path', 'command', 'query', 'url', 'name', 'pattern'] as const; const MAX_HINT_WIDTH = 60; @@ -142,7 +144,7 @@ function formatTurnMd(messages: readonly ContextMessage[], turnNumber: number): // A daemon-ref media part is self-contained and renders as // `[image]`/`[video]` below; a standalone `` tag is user // text and exports verbatim. - for (const part of msg.content) { + for (const part of withoutUserPromptSubmitHookParts(msg.content)) { const text = formatContentPartMd(part); if (text.trim()) { lines.push(text, ''); @@ -192,7 +194,10 @@ function buildOverview( for (const msg of history) { if (msg.role === 'user' && !isInternalMessage(msg)) { const textParts = msg.content - .filter((p): p is { type: 'text'; text: string } => p.type === 'text') + .filter( + (p): p is { type: 'text'; text: string } => + p.type === 'text' && !isUserPromptSubmitHookPart(p), + ) .map((p) => p.text); topic = shorten(textParts.join(' '), 80); break; diff --git a/apps/pythinker-code/src/tui/utils/message-replay.ts b/apps/pythinker-code/src/tui/utils/message-replay.ts index f8c5a866a..c38e8dfdf 100644 --- a/apps/pythinker-code/src/tui/utils/message-replay.ts +++ b/apps/pythinker-code/src/tui/utils/message-replay.ts @@ -239,6 +239,59 @@ export function contentPartsToText(content: readonly ContentPart[]): string { return content.map(contentPartToText).join(''); } +export function isUserPromptSubmitHookPart( + part: ContentPart, +): part is Extract { + return ( + part.type === 'text' && + (part as { meta?: { source?: unknown } }).meta?.source === 'user prompt submit hook' + ); +} + +const SKILL_ACTIVATION_PART_SOURCE = 'skill activation'; + +export function isSkillActivationPart(part: ContentPart): boolean { + return ( + part.type === 'text' && + (part as { meta?: { source?: unknown } }).meta?.source === SKILL_ACTIVATION_PART_SOURCE + ); +} + +function annotateBundledSkillParts( + content: readonly ContentPart[], + bundledActivations: readonly BundledSkillActivationRef[], +): ContentPart[] { + if (bundledActivations.length === 0 || content.some(isSkillActivationPart)) { + return [...content]; + } + let index = 0; + return content.map((part) => { + const activation = bundledActivations[index]; + if ( + activation !== undefined && + part.type === 'text' && + (part as { meta?: { source?: unknown } }).meta?.source === undefined + ) { + index += 1; + return { + ...part, + meta: { source: SKILL_ACTIVATION_PART_SOURCE, activationId: activation.activationId }, + }; + } + return part; + }); +} + +interface BundledSkillActivationRef { + readonly activationId: string; +} + +export function withoutUserPromptSubmitHookParts( + content: readonly ContentPart[], +): ContentPart[] { + return content.filter((part) => !isUserPromptSubmitHookPart(part)); +} + /** * agent-core-v2's task domain persists the terminal notification under the * 'task' spelling (v1 used 'background_task'); both reach replay verbatim. @@ -250,15 +303,8 @@ export interface TaskNotificationOrigin { readonly notificationId: string; } -interface LegacyBackgroundTaskNotificationOrigin { - readonly kind: 'background_task'; - readonly taskId: string; - readonly status: BackgroundTaskStatus; - readonly notificationId: string; -} - export type BackgroundTaskNotificationOrigin = - | LegacyBackgroundTaskNotificationOrigin + | Extract | TaskNotificationOrigin; export function backgroundOrigin( @@ -313,13 +359,15 @@ export function bundledSkillsFromOrigin( } /** - * Content parts the caller actually typed: the engine prepends one rendered - * text part per bundled skill, so the caller's own parts start right after - * them. + * Content parts the caller actually typed: skill blocks are meta-marked at + * construction; legacy wires carry no marks, so the leading unmarked parts + * (one per bundled activation) are annotated first, then filtered out. */ export function stripBundledSkillParts(message: ContextMessage): readonly ContentPart[] { - const bundledCount = bundledSkillsFromOrigin(message.origin).length; - return bundledCount === 0 ? message.content : message.content.slice(bundledCount); + return annotateBundledSkillParts( + message.content, + bundledSkillsFromOrigin(message.origin), + ).filter((part) => !isSkillActivationPart(part)); } export function pluginCommandFromOrigin( diff --git a/apps/pythinker-code/test/tui/export-markdown.test.ts b/apps/pythinker-code/test/tui/export-markdown.test.ts index 1a11565d8..83302e697 100644 --- a/apps/pythinker-code/test/tui/export-markdown.test.ts +++ b/apps/pythinker-code/test/tui/export-markdown.test.ts @@ -387,6 +387,19 @@ describe('buildExportMarkdown', () => { it('filters out internal messages', () => { const msgs: ContextMessage[] = [ + { + role: 'user', + content: [ + { + type: 'text', + text: '\nhook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + } as ContentPart, + { type: 'text', text: 'clean prompt' }, + ], + toolCalls: [], + origin: { kind: 'user' }, + }, userMsg('hello', { kind: 'user' }), userMsg('injected stuff', { kind: 'injection', variant: 'system-reminder' }), assistantMsg('response'), @@ -399,6 +412,9 @@ describe('buildExportMarkdown', () => { now, }); expect(md).not.toContain('injected stuff'); + expect(md).not.toContain('hook_result'); + expect(md).not.toContain('hook note'); + expect(md).toContain('- **Topic**: clean prompt'); expect(md).toContain('hello'); expect(md).toContain('response'); }); diff --git a/apps/pythinker-code/test/tui/message-replay.test.ts b/apps/pythinker-code/test/tui/message-replay.test.ts index 251e959ec..30113941f 100644 --- a/apps/pythinker-code/test/tui/message-replay.test.ts +++ b/apps/pythinker-code/test/tui/message-replay.test.ts @@ -1200,6 +1200,34 @@ describe('PythinkerTUI resume message replay', () => { message('user', [{ type: 'text', text: hookResult }], { origin: { kind: 'hook_result', event: 'UserPromptSubmit' }, }), + message('user', [ + { + type: 'text', + text: '\nmerged hook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + } as ContentPart, + { type: 'text', text: 'merged prompt' }, + ]), + message( + 'user', + [ + { + type: 'text', + text: '\nskill hook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + } as ContentPart, + { type: 'text', text: 'Review the requested file.' }, + ], + { + origin: { + kind: 'skill_activation', + activationId: 'act-review', + skillName: 'review', + skillArgs: 'src/app.ts', + trigger: 'user-slash', + }, + }, + ), ]); const transcript = driver.state.transcriptContainer.render(120).join('\n'); @@ -1207,6 +1235,13 @@ describe('PythinkerTUI resume message replay', () => { expect(transcript).toContain('UserPromptSubmit hook'); expect(transcript).toContain('hook response 1'); expect(transcript).toContain('hook response 2'); + expect(transcript).toContain('merged hook note'); + expect(transcript).toContain('skill hook note'); + expect( + driver.state.transcriptEntries + .filter((entry) => entry.kind === 'user') + .map((entry) => entry.content), + ).toEqual(['prompt', 'merged prompt']); }); it('renders replayed compaction records as completed compaction blocks', async () => { @@ -1490,7 +1525,7 @@ describe('replayBackgroundProjection', () => { [agentTask({ model: 'k2-cheap', thinkingEffort: 'low' })], { 'k2-cheap': { - provider: 'openai', + provider: 'managed:pythinker-code', model: 'kimi-k2-cheap', displayName: 'Kimi K2 Cheap', }, diff --git a/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts b/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts index 59d305e11..cdf984286 100644 --- a/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts +++ b/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts @@ -1016,7 +1016,11 @@ describe('PythinkerTUI message flow', () => { message: { role: 'user', content: [ - { type: 'text', text: 'skill card C body' }, + { + type: 'text', + text: 'skill card C body', + meta: { source: 'skill activation', activationId: 'act-3' }, + }, { type: 'text', text: 'please /commit' }, ], toolCalls: [], diff --git a/apps/vscode/src/runtime/replay-adapter.ts b/apps/vscode/src/runtime/replay-adapter.ts index 1f7e21622..1202db1ad 100644 --- a/apps/vscode/src/runtime/replay-adapter.ts +++ b/apps/vscode/src/runtime/replay-adapter.ts @@ -15,6 +15,7 @@ import type { ToolCall, } from "../../shared/legacy-sdk"; import type { UIStreamEvent } from "../../shared/types"; +import { hookResultBody, isUserPromptSubmitHookPart, withoutUserPromptSubmitHookParts } from "../utils/hook-parts"; import { toLegacyToolName } from "./event-adapter"; import { toLegacyDisplay } from "./tool-display"; @@ -94,7 +95,8 @@ function replayAgentToWebviewEvents( const message = record.message; if (message.role === "user") { if (!isVisibleUserMessage(message.origin)) break; - const imported = importedContextReplay(message.content); + const visibleContent = withoutUserPromptSubmitHookParts(message.content); + const imported = importedContextReplay(visibleContent); completeTurn(); step = 0; turnOpen = true; @@ -103,12 +105,24 @@ function replayAgentToWebviewEvents( { type: "TurnBegin", payload: { - user_input: imported?.input ?? replayUserInput(message.content, message.origin), + user_input: imported?.input ?? replayUserInput(visibleContent, message.origin), }, }, sessionId, ), ); + const hookParts = message.content.filter(isUserPromptSubmitHookPart); + if (hookParts.length > 0) { + ensureStep(); + for (const part of hookParts) { + events.push( + withSession( + { type: "ContentPart", payload: { type: "text", text: hookResultBody(part.text) } }, + sessionId, + ), + ); + } + } if (imported !== undefined) { ensureStep(); events.push( diff --git a/apps/vscode/src/utils/hook-parts.ts b/apps/vscode/src/utils/hook-parts.ts new file mode 100644 index 000000000..bf60614d1 --- /dev/null +++ b/apps/vscode/src/utils/hook-parts.ts @@ -0,0 +1,21 @@ +import type { ContentPart } from "@pymodel/pythinker-code-sdk"; + +export function isUserPromptSubmitHookPart( + part: ContentPart, +): part is Extract { + return ( + part.type === "text" && + (part as { meta?: { source?: unknown } }).meta?.source === "user prompt submit hook" + ); +} + +export function withoutUserPromptSubmitHookParts( + content: readonly ContentPart[], +): ContentPart[] { + return content.filter((part) => !isUserPromptSubmitHookPart(part)); +} + +export function hookResultBody(text: string): string { + const match = /^\n([\s\S]*)\n<\/hook_result>$/.exec(text); + return match?.[1] ?? text; +} diff --git a/apps/vscode/src/utils/session-context.ts b/apps/vscode/src/utils/session-context.ts index b6f8e5d4f..d9be9b855 100644 --- a/apps/vscode/src/utils/session-context.ts +++ b/apps/vscode/src/utils/session-context.ts @@ -5,6 +5,8 @@ import type { ToolCall, } from "@pymodel/pythinker-code-sdk"; +import { withoutUserPromptSubmitHookParts } from "./hook-parts"; + const INTERNAL_ORIGINS = new Set([ "injection", "system_trigger", @@ -200,7 +202,7 @@ function formatPartMarkdown(part: ContentPart): string { } function stringifyParts(parts: readonly ContentPart[]): string { - return parts.map((part) => { + return withoutUserPromptSubmitHookParts(parts).map((part) => { if (part.type === "text") return part.text; if (part.type === "think") return part.think.trim() ? `\n${part.think}\n` : ""; if (part.type === "image_url") return "[image]"; diff --git a/apps/vscode/test/replay-adapter.test.ts b/apps/vscode/test/replay-adapter.test.ts index 11ca2b11d..c3f7a759e 100644 --- a/apps/vscode/test/replay-adapter.test.ts +++ b/apps/vscode/test/replay-adapter.test.ts @@ -397,6 +397,21 @@ describe("replay adapter (renders the public SDK resume state for the Webview)", }), ), record(message("user", [{ type: "text", text: "Visible prompt" }], { origin: { kind: "user" } }), 2), + record( + message( + "user", + [ + { + type: "text", + text: '\nmerged hook note\n', + meta: { contentType: "text/xml", source: "user prompt submit hook" }, + } as ContentPart, + { type: "text", text: "Merged prompt" }, + ], + { origin: { kind: "user" } }, + ), + 3, + ), ]); expect(events.filter((event) => event.type === "TurnBegin")).toEqual([ @@ -405,7 +420,25 @@ describe("replay adapter (renders the public SDK resume state for the Webview)", payload: { user_input: [{ type: "text", text: "Visible prompt" }] }, _sessionId: "session-1", }, + { + type: "TurnBegin", + payload: { user_input: [{ type: "text", text: "Merged prompt" }] }, + _sessionId: "session-1", + }, ]); + const hookIndex = events.findIndex( + (event) => event.type === "ContentPart" && JSON.stringify(event).includes("merged hook note"), + ); + expect(events[hookIndex - 1]).toEqual({ + type: "StepBegin", + payload: { n: 1 }, + _sessionId: "session-1", + }); + expect(events[hookIndex]).toEqual({ + type: "ContentPart", + payload: { type: "text", text: "merged hook note" }, + _sessionId: "session-1", + }); }); it("restores a user-invoked skill as its original slash command", () => { @@ -493,10 +526,11 @@ describe("replay adapter (renders the public SDK resume state for the Webview)", record( message("user", [{ type: "text", text: "/review" }], { origin: { - kind: "skill_activation", + kind: "plugin_command", + activationId: "activation-1", + pluginId: "review-plugin", + commandName: "review", trigger: "user-slash", - skillName: "review", - activationId: "act-1", }, }), 3, @@ -506,84 +540,6 @@ describe("replay adapter (renders the public SDK resume state for the Webview)", expect(replayRecordTurnCount(records)).toBe(2); }); - it("restores shell input and plugin commands as visible user turns", () => { - const events = replay([ - record(message("user", [{ type: "text", text: "pnpm test" }], { - origin: { kind: "shell_command", phase: "input" }, - }), 1), - record(message("user", [{ type: "text", text: "" }], { - origin: { - kind: "plugin_command", - activationId: "plugin-activation-1", - pluginId: "reviewer", - commandName: "check", - commandArgs: "focused", - trigger: "user-slash", - }, - }), 2), - ]); - - expect(events.filter((event) => event.type === "TurnBegin")).toEqual([ - { - type: "TurnBegin", - payload: { user_input: [{ type: "text", text: "pnpm test" }] }, - _sessionId: "session-1", - }, - { - type: "TurnBegin", - payload: { user_input: [{ type: "text", text: "/reviewer:check focused" }] }, - _sessionId: "session-1", - }, - ]); - }); - - it("routes AgentDynamicWorkflow XML results to the matching subagent replay", () => { - const main = resumedAgent([ - record(message("user", [{ type: "text", text: "Run workflow" }], { origin: { kind: "user" } }), 1), - record(message("assistant", [], { - toolCalls: [{ - type: "function", - id: "workflow-call-1", - name: "AgentDynamicWorkflow", - arguments: "{}", - }], - }), 2), - record(message("tool", [{ - type: "text", - text: 'done', - }], { toolCallId: "workflow-call-1" }), 5), - ]); - const child = resumedAgent([ - record(message("user", [{ type: "text", text: "child task" }], { - origin: { kind: "system_trigger", name: "subagent" }, - }), 3), - record(message("assistant", [{ type: "text", text: "workflow child answer" }]), 4), - ], { type: "sub" }); - const state: ResumedSessionState = { - sessionMetadata: { - createdAt: "", - updatedAt: "", - title: "", - isCustomTitle: false, - agents: { - main: { type: "main", parentAgentId: null }, - "sub-1": { type: "sub", parentAgentId: "main" }, - }, - custom: {}, - }, - agents: { main, "sub-1": child }, - }; - - expect(replaySessionToWebviewEvents(state, "session-1")).toContainEqual({ - type: "SubagentEvent", - payload: { - parent_tool_call_id: "workflow-call-1", - event: { type: "ContentPart", payload: { type: "text", text: "workflow child answer" } }, - }, - _sessionId: "session-1", - }); - }); - it("routes repeated runs of one subagent to their corresponding Agent calls", () => { const main = resumedAgent([ record(message("user", [{ type: "text", text: "First" }], { origin: { kind: "user" } }), 1), diff --git a/packages/agent-core-v2/docs/state-manifest.d.ts b/packages/agent-core-v2/docs/state-manifest.d.ts index 8a0e612e9..a486442b2 100644 --- a/packages/agent-core-v2/docs/state-manifest.d.ts +++ b/packages/agent-core-v2/docs/state-manifest.d.ts @@ -723,6 +723,12 @@ export interface AgentStateSnapshot { readonly content: (/* ContentPart — packages/agent-core-v2/src/human/llm/message.ts */ /* TextPart — packages/agent-core-v2/src/human/llm/message.ts */ { type: 'text'; text: string; + meta?: /* TextPartMeta — packages/agent-core-v2/src/human/llm/message.ts */ { + source?: string; + contentType?: string; + activationId?: string; + [key: string]: unknown; + }; } | /* ThinkPart — packages/agent-core-v2/src/human/llm/message.ts */ { type: 'think'; think: string; @@ -1032,6 +1038,12 @@ export interface AgentStateSnapshot { 'media.resolved': Map(message: T, text: string): T { return { ...message, - content: [{ type: 'text', text }], + content: [ + ...message.content.filter( + (part) => isUserPromptSubmitHookPart(part) || isSkillActivationPart(part), + ), + { type: 'text', text }, + ], toolCalls: [], } as unknown as T; } diff --git a/packages/agent-core-v2/src/agent/contextMemory/hookParts.ts b/packages/agent-core-v2/src/agent/contextMemory/hookParts.ts new file mode 100644 index 000000000..df0785caf --- /dev/null +++ b/packages/agent-core-v2/src/agent/contextMemory/hookParts.ts @@ -0,0 +1,19 @@ +import type { ContentPart, TextPart } from '#human/llm/message'; + +export const USER_PROMPT_SUBMIT_HOOK_SOURCE = 'user prompt submit hook'; +export const USER_PROMPT_SUBMIT_HOOK_CONTENT_TYPE = 'text/xml'; + +export function userPromptSubmitHookPart(text: string): TextPart { + return { + type: 'text', + text, + meta: { + contentType: USER_PROMPT_SUBMIT_HOOK_CONTENT_TYPE, + source: USER_PROMPT_SUBMIT_HOOK_SOURCE, + }, + }; +} + +export function isUserPromptSubmitHookPart(part: ContentPart): boolean { + return part.type === 'text' && part.meta?.source === USER_PROMPT_SUBMIT_HOOK_SOURCE; +} diff --git a/packages/agent-core-v2/src/agent/loop/loop.ts b/packages/agent-core-v2/src/agent/loop/loop.ts index b82190b04..7a28b4884 100644 --- a/packages/agent-core-v2/src/agent/loop/loop.ts +++ b/packages/agent-core-v2/src/agent/loop/loop.ts @@ -3,7 +3,7 @@ import type { IDisposable } from '#/_base/di/lifecycle'; import { Error2, isError2, type Error2Options } from '#/_base/errors/errors'; import type { ContextMessage } from '#/agent/contextMemory/types'; import type { FinishReason } from '#human/llm/finish-reason'; -import type { ContentPart } from '#human/llm/message'; +import type { ContentPart, TextPart } from '#human/llm/message'; import type { TokenUsage } from '#human/llm/usage'; import type { Hooks } from '#/hooks'; import type { UserEntry } from '#human/agent/turn'; @@ -185,6 +185,7 @@ export interface PromptLaunchResult { export interface PromptSubmitContext { readonly promptMessage: ContextMessage; readonly isSteer: boolean; + readonly hookParts: TextPart[]; block: boolean; } diff --git a/packages/agent-core-v2/src/agent/loop/loopService.ts b/packages/agent-core-v2/src/agent/loop/loopService.ts index 44cbb000a..14f5393e7 100644 --- a/packages/agent-core-v2/src/agent/loop/loopService.ts +++ b/packages/agent-core-v2/src/agent/loop/loopService.ts @@ -84,6 +84,7 @@ import { type TurnResult, } from './loop'; import { mergeSteerMessages, stripBundledSkillBlocks } from '#human/agent/origin'; +import { isUserPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; import { createUserEntry, type UserEntry } from '#human/agent/turn'; import { AssistantDelta, @@ -590,6 +591,7 @@ export class AgentLoopService extends Disposable implements IAgentLoopService { const ctx: PromptSubmitContext = { promptMessage, isSteer: false, + hookParts: [], block: false, }; await this.hooks.onBeforeSubmitPrompt.run(ctx); @@ -599,7 +601,10 @@ export class AgentLoopService extends Disposable implements IAgentLoopService { block: false, message: { role: 'user', - content: gateImageFormatParts(promptMessage.content, this.profile.getModelProviderType()), + content: [ + ...ctx.hookParts, + ...gateImageFormatParts(promptMessage.content, this.profile.getModelProviderType()), + ], }, }; } @@ -1207,7 +1212,10 @@ export class AgentLoopService extends Disposable implements IAgentLoopService { promptId: prompt.promptId, origin: prompt.origin, prompt: isDisplayablePromptOrigin(prompt.origin) - ? turnPromptText(prompt.message.content, prompt.origin) + ? turnPromptText( + prompt.message.content.filter((part) => !isUserPromptSubmitHookPart(part)), + prompt.origin, + ) : undefined, promptAttachments: turnPromptAttachments(prompt.message.content, prompt.origin), }), diff --git a/packages/agent-core-v2/src/agent/loop/turnEvents.ts b/packages/agent-core-v2/src/agent/loop/turnEvents.ts index f0163142e..bd24f6f89 100644 --- a/packages/agent-core-v2/src/agent/loop/turnEvents.ts +++ b/packages/agent-core-v2/src/agent/loop/turnEvents.ts @@ -2,6 +2,7 @@ import { z } from 'zod'; import type { PromptOrigin } from '#/agent/contextMemory/types'; +import { annotateBundledSkillParts, isSkillActivationPart } from '#human/agent/origin'; import { parseDaemonFileUrl } from '#/agent/media/mediaRef'; import { AgentEvent2, registerEvent2Class } from '#/app/event/event2'; import type { FinishReason } from '#human/llm/finish-reason'; @@ -49,10 +50,10 @@ export function turnPromptText( input: readonly ContentPart[], origin?: PromptOrigin, ): string | undefined { - const bundledBlocks = origin?.kind === 'user' ? (origin.skillActivations?.length ?? 0) : 0; - const text = input - .filter((part): part is TextPart => part.type === 'text') - .slice(bundledBlocks) + const bundledActivations = + origin?.kind === 'user' ? (origin.skillActivations ?? []) : []; + const text = annotateBundledSkillParts(input, bundledActivations) + .filter((part): part is TextPart => part.type === 'text' && !isSkillActivationPart(part)) .map((part) => part.text) .join(''); return text.length > 0 ? text : undefined; diff --git a/packages/agent-core-v2/src/agent/undo/undoService.ts b/packages/agent-core-v2/src/agent/undo/undoService.ts index 1ed2f6802..c89eafd79 100644 --- a/packages/agent-core-v2/src/agent/undo/undoService.ts +++ b/packages/agent-core-v2/src/agent/undo/undoService.ts @@ -18,6 +18,7 @@ import { isValidUndoCount, } from '#/agent/contextMemory/conversationTime'; import { IAgentFullCompactionService } from '#/agent/fullCompaction/fullCompaction'; +import { isUserPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; import { IAgentLoopService } from '#/agent/loop/loop'; import { turnKey } from '#/agent/loop/turnOps'; import { promptMetadataTextFromContentParts } from '#/agent/prompt/promptMetadataText'; @@ -241,13 +242,13 @@ export class AgentConversationUndoService const pending = this.loop.snapshot().queue.filter((item) => item.meta?.tracked === true).at(-1); let lastPrompt = pending === undefined ? undefined - : promptMetadataTextFromContentParts(pending.message.content, (pending.meta?.origin as UserPromptOrigin | undefined)?.clientMetadata); + : promptMetadataTextFromContentParts(pending.message.content.filter((part) => !isUserPromptSubmitHookPart(part)), (pending.meta?.origin as UserPromptOrigin | undefined)?.clientMetadata); if (lastPrompt === undefined) { const history = this.context.get(); for (let i = history.length - 1; i >= 0; i--) { const message = history[i]!; if (!isUndoAnchor(message)) continue; - lastPrompt = promptMetadataTextFromContentParts(message.content, message.origin?.kind === 'user' || message.origin?.kind === 'skill_activation' ? message.origin.clientMetadata : undefined); + lastPrompt = promptMetadataTextFromContentParts(message.content.filter((part) => !isUserPromptSubmitHookPart(part)), message.origin?.kind === 'user' || message.origin?.kind === 'skill_activation' ? message.origin.clientMetadata : undefined); if (lastPrompt !== undefined) break; } } diff --git a/packages/agent-core-v2/src/features/externalHooks/agent/agentExternalHooksService.ts b/packages/agent-core-v2/src/features/externalHooks/agent/agentExternalHooksService.ts index 876a14ac7..893185a9e 100644 --- a/packages/agent-core-v2/src/features/externalHooks/agent/agentExternalHooksService.ts +++ b/packages/agent-core-v2/src/features/externalHooks/agent/agentExternalHooksService.ts @@ -369,19 +369,16 @@ export class AgentExternalHooksService extends Service implements IAgentExternal const append = renderUserPromptHookResult(results); if (append !== undefined) { - this.context.append({ - role: 'user', - content: [{ type: 'text', text: append.text }], - toolCalls: [], - origin: { kind: 'hook_result', event: append.event }, - }); - void this.dispatcher.dispatch( - new HookResult({ - agentId: this.scopeContext.agentId, - hookEvent: append.event, - content: append.message, - }), - ); + ctx.hookParts.push(...append.parts); + for (const message of append.messages) { + void this.dispatcher.dispatch( + new HookResult({ + agentId: this.scopeContext.agentId, + hookEvent: append.event, + content: message, + }), + ); + } } return false; } diff --git a/packages/agent-core-v2/src/features/externalHooks/internal/userPrompt.ts b/packages/agent-core-v2/src/features/externalHooks/internal/userPrompt.ts index 1b81c9f61..694fd915b 100644 --- a/packages/agent-core-v2/src/features/externalHooks/internal/userPrompt.ts +++ b/packages/agent-core-v2/src/features/externalHooks/internal/userPrompt.ts @@ -1,3 +1,6 @@ +import { userPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; +import type { TextPart } from '#human/llm/message'; + import type { HookResult } from './types'; export function renderHookResult(event: string, message: string): string { @@ -10,9 +13,15 @@ export interface RenderedHookResult { readonly text: string; } +export interface RenderedUserPromptHookParts { + readonly event: string; + readonly messages: readonly string[]; + readonly parts: readonly TextPart[]; +} + export function renderUserPromptHookResult( results: readonly HookResult[] | undefined, -): RenderedHookResult | undefined { +): RenderedUserPromptHookParts | undefined { const messages = results ?.filter((result) => result.action !== 'block') @@ -20,11 +29,12 @@ export function renderUserPromptHookResult( .filter(isNonEmptyString) ?? []; if (messages.length === 0) return undefined; - const displayMessage = messages.join('\n\n'); return { event: 'UserPromptSubmit', - message: displayMessage, - text: messages.map((message) => renderHookResult('UserPromptSubmit', message)).join('\n'), + messages, + parts: messages.map((message) => + userPromptSubmitHookPart(renderHookResult('UserPromptSubmit', message)), + ), }; } @@ -57,6 +67,7 @@ function userPromptHookMessage(result: HookResult): string | undefined { } const message = result.message?.trim(); if (message !== undefined && message.length > 0) return message; + if (result.structuredOutput === true) return undefined; const stdout = result.stdout?.trim(); return stdout === undefined || stdout.length === 0 ? undefined : stdout; } diff --git a/packages/agent-core-v2/src/features/skill/skillService.ts b/packages/agent-core-v2/src/features/skill/skillService.ts index eb670f3d2..8c3fad444 100644 --- a/packages/agent-core-v2/src/features/skill/skillService.ts +++ b/packages/agent-core-v2/src/features/skill/skillService.ts @@ -13,6 +13,7 @@ import { IEventService } from '#/app/event/event'; import { ITelemetryService } from '#/app/telemetry/telemetry'; import { ErrorCodes, Error2 } from '#/errors'; import type { ContentPart } from '#human/llm/message'; +import { skillActivationPart } from '#human/agent/origin'; import { MAIN_AGENT_ID } from '#/session/agentLifecycle/agentLifecycle'; import { ISessionContext } from '#/session/sessionContext/sessionContext'; import { ISessionMetadata } from '#/session/sessionMetadata/sessionMetadata'; @@ -204,9 +205,10 @@ export class AgentSkillService implements IAgentSkillService { const skillArgs = input.args ?? ''; const skillContent = this.renderSkillPrompt(skill, skillArgs); + const activationId = randomUUID(); const origin: SkillActivationOrigin = { kind: 'skill_activation', - activationId: randomUUID(), + activationId, skillName: skill.name, trigger: 'user-slash', skillType: skill.metadata.type, @@ -216,16 +218,16 @@ export class AgentSkillService implements IAgentSkillService { }; return { origin, - part: { - type: 'text', - text: renderUserSlashSkillPrompt({ + part: skillActivationPart( + renderUserSlashSkillPrompt({ skillName: skill.name, skillArgs, skillContent, skillSource: skill.source, skillDir: skill.dir, }), - }, + activationId, + ), entry: { activationId: origin.activationId, skillName: origin.skillName, diff --git a/packages/agent-core-v2/src/human/agent/historySchema.ts b/packages/agent-core-v2/src/human/agent/historySchema.ts index a7ee96ea1..678981a90 100644 --- a/packages/agent-core-v2/src/human/agent/historySchema.ts +++ b/packages/agent-core-v2/src/human/agent/historySchema.ts @@ -3,7 +3,11 @@ import { z } from 'zod'; import type { PromptOrigin } from './origin'; import type { HistoryMessage } from './turn'; -const textPartSchema = z.object({ type: z.literal('text'), text: z.string() }); +const textPartSchema = z.object({ + type: z.literal('text'), + text: z.string(), + meta: z.record(z.string(), z.unknown()).optional(), +}); const thinkPartSchema = z.object({ type: z.literal('think'), think: z.string(), diff --git a/packages/agent-core-v2/src/human/agent/origin.ts b/packages/agent-core-v2/src/human/agent/origin.ts index e2b8c9394..3b1236b14 100644 --- a/packages/agent-core-v2/src/human/agent/origin.ts +++ b/packages/agent-core-v2/src/human/agent/origin.ts @@ -1,5 +1,36 @@ import { promptDisplayTextFromContentParts } from '../../agent/prompt/promptMetadataText'; -import type { ContentPart } from '#/llm/message'; +import type { ContentPart, TextPart } from '#/llm/message'; + +export const SKILL_ACTIVATION_PART_SOURCE = 'skill activation'; + +export function skillActivationPart(text: string, activationId: string): TextPart { + return { type: 'text', text, meta: { source: SKILL_ACTIVATION_PART_SOURCE, activationId } }; +} + +export function isSkillActivationPart(part: ContentPart): boolean { + return part.type === 'text' && part.meta?.source === SKILL_ACTIVATION_PART_SOURCE; +} + +export function annotateBundledSkillParts( + content: readonly ContentPart[], + bundledActivations: readonly BundledSkillActivation[], +): ContentPart[] { + if (bundledActivations.length === 0 || content.some(isSkillActivationPart)) { + return [...content]; + } + let index = 0; + return content.map((part) => { + const activation = bundledActivations[index]; + if (activation !== undefined && part.type === 'text' && part.meta?.source === undefined) { + index += 1; + return { + ...part, + meta: { source: SKILL_ACTIVATION_PART_SOURCE, activationId: activation.activationId }, + }; + } + return part; + }); +} export type SkillSource = 'project' | 'user' | 'extra' | 'builtin'; @@ -43,12 +74,14 @@ function userOriginOf(origin: PromptOrigin | undefined): UserPromptOrigin | unde return origin !== undefined && origin.kind === 'user' ? (origin as UserPromptOrigin) : undefined; } -function bundledSkillBlockCount(message: SteerMessage): number { - return userOriginOf(message.origin)?.skillActivations?.length ?? 0; +function bundledSkillActivationsOf(message: SteerMessage): readonly BundledSkillActivation[] { + return userOriginOf(message.origin)?.skillActivations ?? []; } export function stripBundledSkillBlocks(message: SteerMessage): ContentPart[] { - return message.content.slice(bundledSkillBlockCount(message)); + return annotateBundledSkillParts(message.content, bundledSkillActivationsOf(message)).filter( + (part) => !isSkillActivationPart(part), + ); } export function mergeSteerMessages(messages: readonly SteerMessage[]): { @@ -69,7 +102,11 @@ export function mergeSteerMessages(messages: readonly SteerMessage[]): { return { role: 'user', content: [ - ...messages.flatMap((message) => message.content.slice(0, bundledSkillBlockCount(message))), + ...messages.flatMap((message) => + annotateBundledSkillParts(message.content, bundledSkillActivationsOf(message)).filter( + isSkillActivationPart, + ), + ), ...messages.flatMap((message) => stripBundledSkillBlocks(message)), ], toolCalls: [], diff --git a/packages/agent-core-v2/src/human/llm/message.ts b/packages/agent-core-v2/src/human/llm/message.ts index 51dc1dc83..11230b23c 100644 --- a/packages/agent-core-v2/src/human/llm/message.ts +++ b/packages/agent-core-v2/src/human/llm/message.ts @@ -7,9 +7,17 @@ export interface ToolDescription { export type Role = 'system' | 'user' | 'assistant' | 'tool'; +export type TextPartMeta = { + source?: string; + contentType?: string; + activationId?: string; + [key: string]: unknown; +}; + export interface TextPart { type: 'text'; text: string; + meta?: TextPartMeta; } export interface ThinkPart { diff --git a/packages/agent-core-v2/src/human/test/agent/machine.test.ts b/packages/agent-core-v2/src/human/test/agent/machine.test.ts index b4b236635..d4f170d0b 100644 --- a/packages/agent-core-v2/src/human/test/agent/machine.test.ts +++ b/packages/agent-core-v2/src/human/test/agent/machine.test.ts @@ -1236,7 +1236,7 @@ describe('agent machine input.steer', () => { }); expect(steered).toEqual([['p1'], ['p2', 'p3']]); expect(actor.getSnapshot().context.notifications[1]?.message.content).toEqual([ - { type: 'text', text: 'SKILLBLOCK' }, + { type: 'text', text: 'SKILLBLOCK', meta: { source: 'skill activation', activationId: 'a1' } }, { type: 'text', text: 'p2 body' }, { type: 'text', text: 'third' }, ]); diff --git a/packages/agent-core-v2/src/session/sessionTitle/agentTitlePromptSourceService.ts b/packages/agent-core-v2/src/session/sessionTitle/agentTitlePromptSourceService.ts index 23ec0db38..ab3d88048 100644 --- a/packages/agent-core-v2/src/session/sessionTitle/agentTitlePromptSourceService.ts +++ b/packages/agent-core-v2/src/session/sessionTitle/agentTitlePromptSourceService.ts @@ -2,12 +2,14 @@ import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; import { LifecycleScope } from '#/app/scopes'; import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory'; import type { ContextMessage, PromptOrigin } from '#/agent/contextMemory/types'; +import { isUserPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; +import { annotateBundledSkillParts, isSkillActivationPart } from '#human/agent/origin'; import { IAgentLoopService } from '#/agent/loop/loop'; import { promptMetadataTextFromContentParts, promptMetadataTextFromText, } from '#/agent/prompt/promptMetadataText'; -import type { ContentPart } from '#/kosong/contract/message'; +import type { ContentPart } from '#human/llm/message'; import { IAgentTitlePromptSource, @@ -110,9 +112,12 @@ function isNaturalLanguagePrompt(message: ContextMessage): boolean { } function promptMetadataTextFromUserMessage(message: ContextMessage): string | undefined { - const bundled = message.origin?.kind === 'user' ? (message.origin.skillActivations?.length ?? 0) : 0; + const bundled = + message.origin?.kind === 'user' ? (message.origin.skillActivations ?? []) : []; return promptMetadataTextFromContentParts( - bundled === 0 ? message.content : message.content.slice(bundled), + annotateBundledSkillParts(message.content, bundled).filter( + (part) => !isSkillActivationPart(part) && !isUserPromptSubmitHookPart(part), + ), message.origin?.kind === 'user' ? message.origin.clientMetadata : undefined, ); } diff --git a/packages/agent-core-v2/src/workspace/sessionLifecycle/internal/forkTurnSlice.ts b/packages/agent-core-v2/src/workspace/sessionLifecycle/internal/forkTurnSlice.ts index 559346cab..96f6a2922 100644 --- a/packages/agent-core-v2/src/workspace/sessionLifecycle/internal/forkTurnSlice.ts +++ b/packages/agent-core-v2/src/workspace/sessionLifecycle/internal/forkTurnSlice.ts @@ -1,5 +1,11 @@ import { Error2, ErrorCodes } from '#/errors'; import { FILE_HISTORY_RECORD_PREFIX } from '#/features/fileHistory/fileHistoryOps'; +import { isUserPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; +import { + annotateBundledSkillParts, + isSkillActivationPart, + type BundledSkillActivation, +} from '#human/agent/origin'; import type { ContentPart } from '#human/llm/message'; import { promptMetadataTextFromContentParts, @@ -224,9 +230,14 @@ function promptMetadataFromTurnRecord(record: WireRecord): string | undefined { const content = message['content']; if (!Array.isArray(content)) return undefined; const activations = origin?.['skillActivations']; - const bundled = origin?.['kind'] === 'user' && Array.isArray(activations) ? activations.length : 0; + const bundled = + origin?.['kind'] === 'user' && Array.isArray(activations) + ? (activations as BundledSkillActivation[]) + : []; return promptMetadataTextFromContentParts( - (bundled === 0 ? content : content.slice(bundled)) as readonly ContentPart[], + annotateBundledSkillParts(content as readonly ContentPart[], bundled).filter( + (part) => !isSkillActivationPart(part) && !isUserPromptSubmitHookPart(part), + ), origin?.['kind'] === 'user' ? origin['clientMetadata'] : undefined, ); } diff --git a/packages/agent-core-v2/test/agent/contextMemory/context.test.ts b/packages/agent-core-v2/test/agent/contextMemory/context.test.ts index bd7f1f967..8e1c7353d 100644 --- a/packages/agent-core-v2/test/agent/contextMemory/context.test.ts +++ b/packages/agent-core-v2/test/agent/contextMemory/context.test.ts @@ -9,6 +9,7 @@ import { COMPACT_USER_MESSAGE_HEAD_TOKENS, COMPACT_USER_MESSAGE_MAX_TOKENS, selectCompactionUserMessages, + selectRecentUserMessages, type TokenEstimate, } from '#/agent/contextMemory/compactionHandoff'; import type { ContextMessage } from '#/agent/contextMemory/types'; @@ -775,6 +776,26 @@ describe('Agent context', () => { expect(zeroed.tail).toHaveLength(messages.length); expect(selectCompactionUserMessages(messages).elided).toBe(true); + + const hooked: ContextMessage = { + role: 'user', + content: [ + { + type: 'text', + text: '\nhook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { type: 'text', text: 'x'.repeat(4000) }, + ], + toolCalls: [], + }; + const truncated = selectRecentUserMessages([hooked], 10); + expect(truncated[0]?.content[0]).toMatchObject({ + type: 'text', + text: '\nhook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }); + expect(truncated[0]?.content).toHaveLength(2); }); it('falls back to a zero tokensAfter', () => { diff --git a/packages/agent-core-v2/test/agent/prompt/promptService.test.ts b/packages/agent-core-v2/test/agent/prompt/promptService.test.ts index 31ad4f64c..804dc107d 100644 --- a/packages/agent-core-v2/test/agent/prompt/promptService.test.ts +++ b/packages/agent-core-v2/test/agent/prompt/promptService.test.ts @@ -7,6 +7,7 @@ import { IFileService } from '#/app/file/fileService'; import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory'; import type { ContextMessage, PromptOrigin } from '#/agent/contextMemory/types'; import { IAgentLoopService, type PromptHandle } from '#/agent/loop/loop'; +import { TurnStarted } from '#/agent/loop/turnEvents'; import { TurnSteer } from '#/agent/loop/turnOps'; import { ISessionMediaStore } from '#/agent/media/sessionMediaStore'; import { IAgentProfileService } from '#/agent/profile/profile'; @@ -33,10 +34,18 @@ function message(text: string): ContextMessage { return { role: 'user', content: [{ type: 'text', text }], toolCalls: [], origin: { kind: 'user' } }; } -function bundledMessage(skillName: string, user: string, extra: readonly ContentPart[] = []): ContextMessage { +function bundledMessage(skillName: string, user: string, extra: readonly ContentPart[] = [], marked = false): ContextMessage { return { role: 'user', - content: [{ type: 'text', text: `${skillName}` }, { type: 'text', text: user }, ...extra], + content: [ + { + type: 'text', + text: `${skillName}`, + meta: marked ? { source: 'skill activation', activationId: `act-${skillName}` } : undefined, + }, + { type: 'text', text: user }, + ...extra, + ], toolCalls: [], origin: { kind: 'user', skillActivations: [{ activationId: `act-${skillName}`, skillName }] }, }; @@ -469,17 +478,27 @@ describe('prompt queue', () => { const entered = new Promise((resolve) => { markEntered = resolve; }); - loop.hooks.onBeforeSubmitPrompt.register('gate', async (_hookCtx, next) => { + const started: string[] = []; + ctx.get(IEventBus).subscribe(TurnStarted, (event) => { + if (event.prompt !== undefined) started.push(event.prompt); + }); + loop.hooks.onBeforeSubmitPrompt.register('gate', async (hookCtx, next) => { markEntered(); + hookCtx.hookParts.push({ + type: 'text', + text: '\nfrom hook\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }); await new Promise((resolve) => { releaseHook = resolve; }); await next(); }); + const bundled = bundledMessage('review', 'launching'); const { id } = loop.submit({ - message: { role: 'user', content: message('launching').content }, - meta: { tracked: true }, + message: { role: 'user', content: bundled.content }, + meta: { tracked: true, origin: bundled.origin as PromptOrigin }, }); await entered; expect(pendingIds(loop)).toEqual([id]); @@ -488,6 +507,31 @@ describe('prompt queue', () => { await loop.promptHandle(id)!.launched; expect(loop.snapshot().queue).toHaveLength(0); await loop.settled(); + + const history = ctx.context.get(); + expect(history[0]?.role).toBe('user'); + expect(history[0]?.content).toEqual([ + { + type: 'text', + text: '\nfrom hook\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { type: 'text', text: 'review' }, + { type: 'text', text: 'launching' }, + ]); + expect(started).toEqual(['launching']); + const turnPrompt = (await ctx.persistedWireRecords()).find( + (record) => record.type === 'turn.prompt', + ); + expect((turnPrompt as { input?: unknown } | undefined)?.input).toEqual([ + { + type: 'text', + text: '\nfrom hook\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { type: 'text', text: 'review' }, + { type: 'text', text: 'launching' }, + ]); }); it('delivers a blocked prompt’s compression captions inline in their host message', async () => { @@ -692,9 +736,11 @@ describe('prompt queue', () => { await hold.started; await enqueue(loop, { id: 'bundled', message: bundledMessage('review', 'user text') }); + await enqueue(loop, { id: 'marked-bundled', message: bundledMessage('security', 'marked text', [], true) }); expect(queued).toEqual([ { promptId: 'bundled', content: [{ type: 'text', text: 'user text' }] }, + { promptId: 'marked-bundled', content: [{ type: 'text', text: 'marked text' }] }, ]); hold.release(); @@ -775,8 +821,16 @@ describe('prompt queue', () => { (entry) => entry.origin?.kind === 'user' && entry.origin.skillActivations !== undefined, ); expect(merged?.content).toEqual([ - { type: 'text', text: 'review' }, - { type: 'text', text: 'security' }, + { + type: 'text', + text: 'review', + meta: { source: 'skill activation', activationId: 'act-review' }, + }, + { + type: 'text', + text: 'security', + meta: { source: 'skill activation', activationId: 'act-security' }, + }, { type: 'text', text: 'user A' }, { type: 'text', text: 'user B' }, ]); diff --git a/packages/agent-core-v2/test/agent/undo/undo.test.ts b/packages/agent-core-v2/test/agent/undo/undo.test.ts index 322e382d0..f69705136 100644 --- a/packages/agent-core-v2/test/agent/undo/undo.test.ts +++ b/packages/agent-core-v2/test/agent/undo/undo.test.ts @@ -9,6 +9,7 @@ import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory' import { IAgentConversationUndoParticipantRegistry } from '#/agent/contextMemory/conversationUndoParticipants'; import { ContextApplyCompaction } from '#/agent/contextMemory/contextEvents'; import { isPromptOwnedInjection, isUndoAnchor } from '#/agent/contextMemory/conversationTime'; +import { userPromptSubmitHookPart } from '#/agent/contextMemory/hookParts'; import type { ContextMessage, PromptOrigin, TaskOrigin } from '#/agent/contextMemory/types'; import { IAgentFullCompactionService } from '#/agent/fullCompaction/fullCompaction'; import { IAgentLoopService } from '#/agent/loop/loop'; @@ -623,6 +624,19 @@ describe('AgentConversationUndoService', () => { await ctx.get(IAgentConversationUndoService).undo(1); await expect(metadata.read()).resolves.toMatchObject({ lastPrompt: undefined }); + ctx.context.append({ + role: 'user', + content: [ + userPromptSubmitHookPart('\nhook note\n'), + { type: 'text', text: 'u2' }, + ], + toolCalls: [], + origin: { kind: 'user' }, + }); + ctx.appendTurnExchange('u3', 'a3'); + + await ctx.get(IAgentConversationUndoService).undo(1); + await expect(metadata.read()).resolves.toMatchObject({ lastPrompt: 'u2' }); }); it.each([undefined, 'Save button · Rename it'])('uses the newest pending prompt as lastPrompt after undo (display=%s)', async (displayText) => { @@ -640,7 +654,10 @@ describe('AgentConversationUndoService', () => { { message: { role: 'user', - content: [{ type: 'text', text: 'queued prompt' }], + content: [ + userPromptSubmitHookPart('\nhook note\n'), + { type: 'text', text: 'queued prompt' }, + ], }, meta: { promptId: 'queued', diff --git a/packages/agent-core-v2/test/features/externalHooks/integration.test.ts b/packages/agent-core-v2/test/features/externalHooks/integration.test.ts index f8916df8a..3671031b4 100644 --- a/packages/agent-core-v2/test/features/externalHooks/integration.test.ts +++ b/packages/agent-core-v2/test/features/externalHooks/integration.test.ts @@ -176,7 +176,6 @@ function stubSessionMetadata(title?: string): ISessionMetadata { setTitle: async () => {}, setArchived: async () => {}, registerAgent: async () => {}, - unregisterAgent: async () => {}, } as unknown as ISessionMetadata; } @@ -1220,6 +1219,14 @@ describe('IExternalHooksRunnerService integration', () => { origin: { kind: 'system_trigger', name: 'goal' }, }), ); + eventBus.publish( + new TurnStarted({ + agentId: 'main', + turnId: 4, + origin: { kind: 'user' }, + prompt: 'user text', + }), + ); const queuedContent = [{ type: 'text' as const, text: 'later' }]; eventBus.publish( new PromptQueued({ @@ -1255,6 +1262,17 @@ describe('IExternalHooksRunnerService integration', () => { prompt: undefined, }, }, + { + event: 'TurnStarted', + matcherValue: 'user', + inputData: { + sessionTitle: 'My Session', + turnId: 4, + originKind: 'user', + originName: undefined, + prompt: 'user text', + }, + }, { event: 'UserPromptQueued', matcherValue: queuedContent, diff --git a/packages/agent-core-v2/test/features/externalHooks/runner.test.ts b/packages/agent-core-v2/test/features/externalHooks/runner.test.ts index 58c029cff..434225c4f 100644 --- a/packages/agent-core-v2/test/features/externalHooks/runner.test.ts +++ b/packages/agent-core-v2/test/features/externalHooks/runner.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { buildHookSpawnOptions, runHook } from '#/features/externalHooks/internal/runHook'; +import { renderUserPromptHookResult } from '#/features/externalHooks/internal/userPrompt'; import { HostProcessService } from '#/os/backends/node-local/hostProcessService'; const hostProcess = new HostProcessService(); @@ -55,6 +56,43 @@ describe('runHook process runner', () => { expect(emptyHookSpecificOutput.action).toBe('allow'); expect(emptyHookSpecificOutput.message).toBeUndefined(); expect(emptyHookSpecificOutput.structuredOutput).toBe(true); + + expect(renderUserPromptHookResult([emptyObject, emptyHookSpecificOutput])).toBeUndefined(); + + const continueOnly = await runHook( + hostProcess, + nodeCommand('process.stdout.write(JSON.stringify({ continue: true }));'), + {}, + { timeout: 5 }, + ); + expect(renderUserPromptHookResult([continueOnly])).toBeUndefined(); + + const plainText = await runHook( + hostProcess, + nodeCommand('process.stdout.write("hook note");'), + {}, + { timeout: 5 }, + ); + const secondNote = await runHook( + hostProcess, + nodeCommand('process.stdout.write("second note");'), + {}, + { timeout: 5 }, + ); + const rendered = renderUserPromptHookResult([plainText, secondNote]); + expect(rendered?.messages).toEqual(['hook note', 'second note']); + expect(rendered?.parts).toEqual([ + { + type: 'text', + text: '\nhook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { + type: 'text', + text: '\nsecond note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + ]); }); it('returns block when the hook exits 2 and captures stderr as the reason', async () => { diff --git a/packages/agent-gateway/src/routes/prompts.ts b/packages/agent-gateway/src/routes/prompts.ts index 24daee233..231c527fd 100644 --- a/packages/agent-gateway/src/routes/prompts.ts +++ b/packages/agent-gateway/src/routes/prompts.ts @@ -35,6 +35,8 @@ import { type ISessionScopeHandle, type Scope, } from '@pymodel/agent-core-v2'; +import { annotateBundledSkillParts, isSkillActivationPart } from '@pymodel/agent-core-v2/human/agent/origin'; +import { isUserPromptSubmitHookPart } from '@pymodel/agent-core-v2/agent/contextMemory/hookParts'; import { ErrorCode } from '../protocol/error-codes'; import { projectPromptContentParts } from '../services/messages/messageProjection'; import { @@ -500,8 +502,10 @@ export function projectPromptSnapshot(prompt: { ? 'running' : prompt.state === 'blocked' ? 'blocked' : 'queued'; const origin = prompt.message.origin; - const bundled = origin?.kind === 'user' ? (origin.skillActivations?.length ?? 0) : 0; - const content = bundled === 0 ? prompt.message.content : prompt.message.content.slice(bundled); + const bundled = origin?.kind === 'user' ? (origin.skillActivations ?? []) : []; + const content = annotateBundledSkillParts(prompt.message.content, bundled).filter( + (part) => !isSkillActivationPart(part) && !isUserPromptSubmitHookPart(part), + ); return { prompt_id: prompt.id, user_message_id: prompt.userMessageId, diff --git a/packages/agent-gateway/src/services/transcript/coreEventMap.ts b/packages/agent-gateway/src/services/transcript/coreEventMap.ts index 6a5e9933a..e37aefe04 100644 --- a/packages/agent-gateway/src/services/transcript/coreEventMap.ts +++ b/packages/agent-gateway/src/services/transcript/coreEventMap.ts @@ -7,6 +7,8 @@ import type { CompactionStarted, } from '@pymodel/agent-core-v2/agent/fullCompaction/compactionOps'; import { daemonFileRefFromPart, type ContentPart, type ContextUndone, type CronFired, type GoalUpdated } from '@pymodel/agent-core-v2'; +import { isUserPromptSubmitHookPart } from '@pymodel/agent-core-v2/agent/contextMemory/hookParts'; +import { annotateBundledSkillParts, isSkillActivationPart } from '@pymodel/agent-core-v2/human/agent/origin'; import type { AssistantDelta, ThinkingDelta, @@ -1521,8 +1523,10 @@ export class AgentTranscriptProjector { if (frameOrigin === undefined) return []; const turn = this.currentTurn; if (turn !== undefined && turn.state !== 'running') return []; - const skip = origin.kind === 'user' ? origin.skillActivations?.length ?? 0 : 0; - const input = skip > 0 ? event.input.slice(skip) : event.input; + const input = annotateBundledSkillParts( + event.input, + origin.kind === 'user' ? (origin.skillActivations ?? []) : [], + ).filter((part) => !isSkillActivationPart(part) && !isUserPromptSubmitHookPart(part)); const files = origin.attachments ?? []; const promptIds = origin.kind === 'user' ? event.promptIds : undefined; const step = this.currentStep; diff --git a/packages/agent-gateway/src/services/transcript/transcriptService.ts b/packages/agent-gateway/src/services/transcript/transcriptService.ts index ba1e53ef2..b1df55fb1 100644 --- a/packages/agent-gateway/src/services/transcript/transcriptService.ts +++ b/packages/agent-gateway/src/services/transcript/transcriptService.ts @@ -33,6 +33,7 @@ import { groupMessagesIntoSnapshot, isPlainAgentId, turnId as exportTurnKey, + withoutUserPromptSubmitHookParts, type AgentDescriptor, type ActivityMeta, type AgentTranscript, @@ -602,7 +603,7 @@ export class TranscriptService { anchorStack.push({ taskIdsSnapshot: new Set(taskOriginTurnTaskIds), steerCount: matchedSteers.length }); } if (message?.role === 'user') { - const key = JSON.stringify(message.content); + const key = JSON.stringify(withoutUserPromptSubmitHookParts(message.content)); const kind = message.origin?.kind ?? 'user'; const pendingByKind = pendingSteers.get(key); const remaining = pendingByKind?.get(kind) ?? 0; @@ -688,7 +689,7 @@ export class TranscriptService { return snapshot; } const modes = { ...snapshot.meta.modes, tower: undefined }; - const cleared = modes.plan === undefined && modes.dynamic_workflow === undefined && modes.tower === undefined; + const cleared = modes.plan === undefined && modes.swarm === undefined && modes.tower === undefined; return { ...snapshot, meta: { ...snapshot.meta, modes: cleared ? undefined : modes } }; } @@ -697,9 +698,9 @@ export class TranscriptService { sessionId: string, agentId: string, ): Promise { + const wire = agents?.handleOf(agentId)?.accessor.get(IWireService); + if (wire === undefined) return; try { - const wire = agents?.handleOf(agentId)?.accessor.get(IWireService); - if (wire === undefined) return; await wire.flush(); } catch (error) { this.deps.logger?.warn( diff --git a/packages/agent-gateway/test/prompts.test.ts b/packages/agent-gateway/test/prompts.test.ts index 4e2b384eb..f3090f216 100644 --- a/packages/agent-gateway/test/prompts.test.ts +++ b/packages/agent-gateway/test/prompts.test.ts @@ -467,6 +467,11 @@ describe('server-v2 /api/v1 prompts', () => { message: { role: 'user', content: [ + { + type: 'text', + text: '\nhook note\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, { type: 'text', text: 'rendered skill block' }, { type: 'text', text: 'Review this change.' }, ], diff --git a/packages/agent-gateway/test/services/transcript.test.ts b/packages/agent-gateway/test/services/transcript.test.ts index 03c320388..90f2845a2 100644 --- a/packages/agent-gateway/test/services/transcript.test.ts +++ b/packages/agent-gateway/test/services/transcript.test.ts @@ -312,14 +312,16 @@ describe('AgentTranscriptProjector', () => { turnId: 0, promptId: 'prompt-1', origin: { kind: 'user' }, - prompt: 'fix the bug', + prompt: '\nliteral user text\nfix the bug', })); feed(ev({ type: 'assistant.delta', turnId: 0, delta: 'on it' })); feed(ev({ type: 'turn.ended', turnId: 0, reason: 'completed' })); const turn = turnOps('t0', tx.getItems()); expect(turn.triggerPromptId).toBe('prompt-1'); - expect(turn.prompt).toBe('fix the bug'); + expect(turn.prompt).toBe( + '\nliteral user text\nfix the bug', + ); expect(turn.state).toBe('completed'); }); diff --git a/packages/transcript/src/history/groupTurns.ts b/packages/transcript/src/history/groupTurns.ts index da88d302e..145f8b74b 100644 --- a/packages/transcript/src/history/groupTurns.ts +++ b/packages/transcript/src/history/groupTurns.ts @@ -11,8 +11,15 @@ export type HistoryMediaSource = | { readonly kind: 'base64'; readonly media_type: string; readonly data: string } | { readonly kind: 'file' | 'session_media'; readonly file_id: string }; +export interface HistoryTextPartMeta { + readonly source?: string; + readonly contentType?: string; + readonly activationId?: string; + readonly [key: string]: unknown; +} + export type HistoryContentPart = - | { readonly type: 'text'; readonly text: string } + | { readonly type: 'text'; readonly text: string; readonly meta?: HistoryTextPartMeta } | { readonly type: 'think'; readonly think: string; readonly hidden?: boolean } | { readonly type: 'image' | 'video' | 'audio'; readonly source: HistoryMediaSource; readonly name?: string } | { @@ -24,6 +31,71 @@ export type HistoryContentPart = } | { readonly type: string }; +const USER_PROMPT_SUBMIT_HOOK_SOURCE = 'user prompt submit hook'; + +export function isUserPromptSubmitHookPart(part: { readonly type: string }): boolean { + return ( + part.type === 'text' && + (part as { readonly meta?: HistoryTextPartMeta }).meta?.source === + USER_PROMPT_SUBMIT_HOOK_SOURCE + ); +} + +const SKILL_ACTIVATION_PART_SOURCE = 'skill activation'; + +export function isSkillActivationPart(part: { readonly type: string }): boolean { + return ( + part.type === 'text' && + (part as { readonly meta?: HistoryTextPartMeta }).meta?.source === + SKILL_ACTIVATION_PART_SOURCE + ); +} + +function annotateBundledSkillParts( + content: readonly T[], + bundledActivations: readonly BundledSkillActivation[], +): T[] { + if (bundledActivations.length === 0 || content.some(isSkillActivationPart)) { + return [...content]; + } + let index = 0; + return content.map((part) => { + const activation = bundledActivations[index]; + if ( + activation !== undefined && + part.type === 'text' && + (part as { readonly meta?: HistoryTextPartMeta }).meta?.source === undefined + ) { + index += 1; + return { + ...part, + meta: { source: SKILL_ACTIVATION_PART_SOURCE, activationId: activation.activationId }, + } as T; + } + return part; + }); +} + +export function withoutUserPromptSubmitHookParts( + content: readonly T[], +): readonly T[] { + if (!content.some(isUserPromptSubmitHookPart)) return content; + return content.filter((part) => !isUserPromptSubmitHookPart(part)); +} + +const USER_PROMPT_HOOK_RESULT_WRAPPER_RE = + /^\n([\s\S]*)\n<\/hook_result>$/; + +function userPromptSubmitHookMarkerPayload(part: { readonly type: string }): { + readonly hookEvent: string; + readonly content: string; +} { + const text = (part as { readonly text?: unknown }).text; + const raw = typeof text === 'string' ? text : ''; + const match = USER_PROMPT_HOOK_RESULT_WRAPPER_RE.exec(raw); + return { hookEvent: 'UserPromptSubmit', content: match?.[1] ?? raw }; +} + export interface HistoryToolCall { readonly id: string; readonly name: string; @@ -224,8 +296,29 @@ export function groupMessagesIntoSnapshot( items.push(item); }; + const extractBundledSkillMarkers = (message: HistoryMessage): HistoryMessage => { + const bundled = bundledSkillActivations(message); + const annotated = annotateBundledSkillParts(message.content ?? [], bundled); + const skillParts = annotated.filter(isSkillActivationPart); + bundled.forEach((activation, index) => { + const block = skillParts[index]; + pushMarker('skill', { + text: block !== undefined && block.type === 'text' && 'text' in block ? block.text : '', + origin: { kind: 'skill_activation', trigger: 'user-slash', ...activation }, + }); + }); + return { ...message, content: annotated.filter((part) => !isSkillActivationPart(part)) }; + }; + let prevNonTaskRole: string | undefined; - for (const message of messages) { + for (const entry of messages) { + const content = + entry.content === undefined ? undefined : withoutUserPromptSubmitHookParts(entry.content); + const hookParts = + entry.content === undefined || content === entry.content + ? [] + : entry.content.filter(isUserPromptSubmitHookPart); + const message = content === entry.content ? entry : { ...entry, content }; if (message.role === 'system') continue; const originKind = message.origin?.kind; const isTaskOrigin = @@ -234,6 +327,9 @@ export function groupMessagesIntoSnapshot( if (!isTaskOrigin) prevNonTaskRole = message.role; if (message.role === 'user') { + for (const part of hookParts) { + pushMarker('hook', userPromptSubmitHookMarkerPayload(part)); + } if (originKind !== undefined && HIDDEN_USER_ORIGINS.has(originKind)) { if (opensOwnTurn(message)) { const opening = @@ -263,16 +359,7 @@ export function groupMessagesIntoSnapshot( const steeredRemaining = steeredByKind?.get(steerKind) ?? 0; if (steeredById || (steeredByKind !== undefined && steeredRemaining > 0)) { if (!steeredById) steeredByKind!.set(steerKind, steeredRemaining - 1); - const bundled = bundledSkillActivations(message); - const parts = message.content ?? []; - bundled.forEach((activation, index) => { - const block = parts[index]; - pushMarker('skill', { - text: block !== undefined && block.type === 'text' && 'text' in block ? block.text : '', - origin: { kind: 'skill_activation', trigger: 'user-slash', ...activation }, - }); - }); - const opening = foldTurnOpeningInput({ ...message, content: parts.slice(bundled.length) }); + const opening = foldTurnOpeningInput(extractBundledSkillMarkers(message)); pendingNotificationFrames.push({ text: opening.text, taskId: undefined, @@ -312,15 +399,7 @@ export function groupMessagesIntoSnapshot( } const bundled = bundledSkillActivations(message); if (bundled.length > 0) { - const parts = message.content ?? []; - bundled.forEach((activation, index) => { - const block = parts[index]; - pushMarker('skill', { - text: block !== undefined && block.type === 'text' && 'text' in block ? block.text : '', - origin: { kind: 'skill_activation', trigger: 'user-slash', ...activation }, - }); - }); - const callerMessage = { ...message, content: parts.slice(bundled.length) }; + const callerMessage = extractBundledSkillMarkers(message); const opening = foldTurnOpeningInput(callerMessage); startTurn(mapOrigin(message), opening.text, opening.attachmentIds, triggerPromptIdOf(message)); continue; diff --git a/packages/transcript/test/layers.test.ts b/packages/transcript/test/layers.test.ts index ec60f21df..9aee495ef 100644 --- a/packages/transcript/test/layers.test.ts +++ b/packages/transcript/test/layers.test.ts @@ -498,7 +498,19 @@ describe('groupMessagesIntoSnapshot (cold path)', () => { it('groups flat messages into turns with folded tool results', () => { const snapshot = groupMessagesIntoSnapshot([ { role: 'system', content: [{ type: 'text', text: 'sys' }] }, - { role: 'user', content: [{ type: 'text', text: 'hello' }], toolCalls: [], origin: { kind: 'user' } }, + { + role: 'user', + content: [ + { + type: 'text', + text: '\nnoise\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { type: 'text', text: 'hello' }, + ], + toolCalls: [], + origin: { kind: 'user' }, + }, { role: 'assistant', content: [{ type: 'think', think: 'hmm' }, { type: 'text', text: 'checking' }], @@ -521,15 +533,19 @@ describe('groupMessagesIntoSnapshot (cold path)', () => { ]); const kinds = snapshot.items.map((i) => i.kind); - expect(kinds).toEqual(['turn', 'turn', 'marker', 'turn']); - const firstTurn = snapshot.items[0]; + expect(kinds).toEqual(['marker', 'turn', 'turn', 'marker', 'turn']); + const hookMarker = snapshot.items[0]; + if (hookMarker?.kind !== 'marker') throw new Error('expected marker'); + expect(hookMarker.marker).toBe('hook'); + expect(hookMarker.payload).toEqual({ hookEvent: 'UserPromptSubmit', content: 'noise' }); + const firstTurn = snapshot.items[1]; if (firstTurn?.kind !== 'turn') throw new Error('expected turn'); expect(firstTurn.prompt).toBe('hello'); expect(firstTurn.steps).toHaveLength(2); const tool = firstTurn.steps[0]?.frames.find((f) => f.kind === 'tool'); expect(tool?.kind === 'tool' && tool.output).toBe('file body'); expect(tool?.kind === 'tool' && tool.input).toEqual({ path: '/a' }); - const marker = snapshot.items[2]; + const marker = snapshot.items[3]; expect(marker?.kind === 'marker' && marker.marker).toBe('compaction'); }); @@ -641,12 +657,27 @@ describe('groupMessagesIntoSnapshot (cold path)', () => { [ { role: 'user', content: [{ type: 'text', text: 'active' }], toolCalls: [], origin: { kind: 'user' } }, { role: 'assistant', content: [{ type: 'text', text: 'working' }], toolCalls: [] }, - { role: 'user', content: [{ type: 'text', text: 'steered in' }], toolCalls: [], origin: { kind: 'user' } }, + { + role: 'user', + content: [ + { + type: 'text', + text: '\nnoise\n', + meta: { contentType: 'text/xml', source: 'user prompt submit hook' }, + }, + { type: 'text', text: 'steered in' }, + ], + toolCalls: [], + origin: { kind: 'user' }, + }, ], { steeredContents: new Map([[JSON.stringify([{ type: 'text', text: 'steered in' }]), new Map([['user', 1]])]]) }, ); - expect(snapshot.items.map((i) => i.kind)).toEqual(['turn']); + expect(snapshot.items.map((i) => i.kind)).toEqual(['turn', 'marker']); + const hookMarker = snapshot.items[1]; + if (hookMarker?.kind !== 'marker') throw new Error('expected marker'); + expect(hookMarker.marker).toBe('hook'); const turn = snapshot.items[0]; if (turn?.kind !== 'turn') throw new Error('expected turn'); const lastStep = turn.steps.at(-1); @@ -873,42 +904,56 @@ describe('groupMessagesIntoSnapshot (cold path)', () => { }); it('expands a bundled prompt into per-skill markers and a caller-text turn', () => { - const snapshot = groupMessagesIntoSnapshot([ - { - role: 'user', - content: [ - { type: 'text', text: 'rendered review block' }, - { type: 'text', text: 'rendered security block' }, - { type: 'text', text: 'please /skill:review and /skill:security' }, - ], - toolCalls: [], - origin: { - kind: 'user', - skillActivations: [ - { activationId: 'act-1', skillName: 'review' }, - { activationId: 'act-2', skillName: 'security', skillArgs: 'src/app.ts' }, - ], - } as { kind: string }, - }, - { role: 'assistant', content: [{ type: 'text', text: 'done' }], toolCalls: [] }, - ]); - - expect(snapshot.items.map((item) => item.kind)).toEqual(['marker', 'marker', 'turn']); - const first = snapshot.items[0]; - expect(first?.kind === 'marker' && first.marker).toBe('skill'); - expect(first?.kind === 'marker' && first.payload).toMatchObject({ - text: 'rendered review block', - origin: { kind: 'skill_activation', trigger: 'user-slash', skillName: 'review' }, - }); - const second = snapshot.items[1]; - expect(second?.kind === 'marker' && second.payload).toMatchObject({ - text: 'rendered security block', - origin: { skillName: 'security', skillArgs: 'src/app.ts' }, - }); - const turn = snapshot.items[2]; - if (turn?.kind !== 'turn') throw new Error('expected turn'); - expect(turn.prompt).toBe('please /skill:review and /skill:security'); - expect(turn.steps).toHaveLength(1); + const origin = { + kind: 'user', + skillActivations: [ + { activationId: 'act-1', skillName: 'review' }, + { activationId: 'act-2', skillName: 'security', skillArgs: 'src/app.ts' }, + ], + } as { kind: string }; + const variants: readonly (readonly HistoryContentPart[])[] = [ + [ + { type: 'text', text: 'rendered review block' }, + { type: 'text', text: 'rendered security block' }, + { type: 'text', text: 'please /skill:review and /skill:security' }, + ], + [ + { + type: 'text', + text: 'rendered review block', + meta: { source: 'skill activation', activationId: 'act-1' }, + }, + { + type: 'text', + text: 'rendered security block', + meta: { source: 'skill activation', activationId: 'act-2' }, + }, + { type: 'text', text: 'please /skill:review and /skill:security' }, + ], + ]; + for (const content of variants) { + const snapshot = groupMessagesIntoSnapshot([ + { role: 'user', content, toolCalls: [], origin }, + { role: 'assistant', content: [{ type: 'text', text: 'done' }], toolCalls: [] }, + ]); + + expect(snapshot.items.map((item) => item.kind)).toEqual(['marker', 'marker', 'turn']); + const first = snapshot.items[0]; + expect(first?.kind === 'marker' && first.marker).toBe('skill'); + expect(first?.kind === 'marker' && first.payload).toMatchObject({ + text: 'rendered review block', + origin: { kind: 'skill_activation', trigger: 'user-slash', skillName: 'review' }, + }); + const second = snapshot.items[1]; + expect(second?.kind === 'marker' && second.payload).toMatchObject({ + text: 'rendered security block', + origin: { skillName: 'security', skillArgs: 'src/app.ts' }, + }); + const turn = snapshot.items[2]; + if (turn?.kind !== 'turn') throw new Error('expected turn'); + expect(turn.prompt).toBe('please /skill:review and /skill:security'); + expect(turn.steps).toHaveLength(1); + } }); it('maps media parts on the opening user message to attachment entities, dropping base64 bytes', () => { From c6141b5f0163237681cdfc3d4d1cae2325b9fb21 Mon Sep 17 00:00:00 2001 From: elkaix Date: Thu, 1 Oct 2026 07:37:44 -0400 Subject: [PATCH 4/9] feat: cap WaitFor at 90s and let new input end the wait --- .../messages/tool-renderers/wait-for.ts | 17 +- .../src/tui/controllers/editor-keyboard.ts | 5 + .../tui/controllers/session-event-handler.ts | 6 + .../src/tui/controllers/staging-leases.ts | 42 ++- .../src/tui/controllers/streaming-ui.ts | 18 + apps/pythinker-code/src/tui/pythinker-tui.ts | 219 ++++++++--- .../tui/components/messages/tool-call.test.ts | 30 ++ .../tui/controllers/editor-keyboard.test.ts | 21 ++ .../tui/controllers/staging-leases.test.ts | 60 +++ .../tui/pythinker-tui-message-flow.test.ts | 351 +++++++++++++++++- docs/guides/interaction.md | 2 +- docs/reference/tools.md | 2 +- .../task/task-wait/task-wait-subagent.md | 2 + .../agent/tools/task/task-wait/task-wait.md | 13 +- .../agent/tools/task/task-wait/task-wait.ts | 5 +- .../tools/task/task-wait/taskWaitTool.ts | 84 ++++- .../test/agent/loop/loop.test.ts | 4 +- .../test/agent/task/taskService.test.ts | 9 +- .../test/agent/task/tools/task-tools.test.ts | 70 ++-- .../test/features/goal/goal.test.ts | 133 +------ .../agent-core-v2/test/features/goal/stubs.ts | 8 + 21 files changed, 858 insertions(+), 243 deletions(-) create mode 100644 packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait-subagent.md diff --git a/apps/pythinker-code/src/tui/components/messages/tool-renderers/wait-for.ts b/apps/pythinker-code/src/tui/components/messages/tool-renderers/wait-for.ts index bd3d70e78..d55ef2d5a 100644 --- a/apps/pythinker-code/src/tui/components/messages/tool-renderers/wait-for.ts +++ b/apps/pythinker-code/src/tui/components/messages/tool-renderers/wait-for.ts @@ -20,7 +20,7 @@ import type { ResultRenderer } from './types'; const DESCRIPTION_MAX = 72; const RUNNING_SAMPLES = 3; -type WaitForStatus = 'completed' | 'timed_out' | 'no_tasks'; +type WaitForStatus = 'completed' | 'timed_out' | 'interrupted' | 'no_tasks'; interface WaitForResultView { readonly status: WaitForStatus; @@ -77,6 +77,9 @@ export function buildWaitForHeader(options: { if (status === 'no_tasks') { return `${bullet}${currentTheme.boldFg('primary', 'No background tasks running')}${chip}`; } + if (status === 'interrupted') { + return `${bullet}${currentTheme.boldFg('primary', 'Wait interrupted by new input')}${argText}${chip}`; + } const label = taskId === undefined ? 'Waited for a background task' : 'Waited for background task'; return `${bullet}${currentTheme.boldFg('primary', label)}${argText}${chip}`; } @@ -92,7 +95,8 @@ function glanceLines(view: WaitForResultView): string[] { switch (view.status) { case 'no_tasks': return []; - case 'timed_out': { + case 'timed_out': + case 'interrupted': { if (view.runningCount === 0) return []; const summary = `${pluralizeTasks(view.runningCount)} still running`; if (view.runningSamples.length === 0) return [summary]; @@ -124,7 +128,14 @@ function pluralizeTasks(count: number): string { export function parseWaitForOutput(output: string): WaitForResultView | undefined { const status = field(output, 'wait_status'); - if (status !== 'completed' && status !== 'timed_out' && status !== 'no_tasks') return undefined; + if ( + status !== 'completed' && + status !== 'timed_out' && + status !== 'interrupted' && + status !== 'no_tasks' + ) { + return undefined; + } const waitedMs = Number(field(output, 'waited_ms') ?? 0); const finished = section(output, 'finished'); const duringWait = section(output, 'completed_during_wait'); diff --git a/apps/pythinker-code/src/tui/controllers/editor-keyboard.ts b/apps/pythinker-code/src/tui/controllers/editor-keyboard.ts index 633ae8674..5f914bf1f 100644 --- a/apps/pythinker-code/src/tui/controllers/editor-keyboard.ts +++ b/apps/pythinker-code/src/tui/controllers/editor-keyboard.ts @@ -57,6 +57,7 @@ export interface EditorKeyboardHost { }): boolean; releaseStagingMedia(mediaAttachmentIds: readonly number[]): void; recallLastQueued(): QueuedMessage | undefined; + isSteeringQueuedMessages(): boolean; showError(msg: string): void; track(event: string, props?: Record): void; updateEditorBorderHighlight(text?: string): void; @@ -321,6 +322,10 @@ export class EditorKeyboardController { host.state.appState.isCompacting ) return; + // An automatic steer of the queue is still in flight: steering more now + // could reach the model ahead of it, so the keypress is ignored for + // that brief window and the draft stays in the editor. + if (host.isSteeringQueuedMessages()) return; const text = editor.getText().trim(); const editorIsBash = editor.inputMode === 'bash'; diff --git a/apps/pythinker-code/src/tui/controllers/session-event-handler.ts b/apps/pythinker-code/src/tui/controllers/session-event-handler.ts index bc73a3471..3372ae120 100644 --- a/apps/pythinker-code/src/tui/controllers/session-event-handler.ts +++ b/apps/pythinker-code/src/tui/controllers/session-event-handler.ts @@ -122,6 +122,7 @@ export interface SessionEventHost { updateTerminalTitle(): void; sendQueuedMessage(session: Session, item: QueuedMessage): void; shiftQueuedMessage(): QueuedMessage | undefined; + steerQueuedMessagesIntoRunningTurn(): void; handleTurnStarted?(event: TurnStartedEvent): void; handleTurnEnded?(event: TurnEndedEvent): void; readonly btwPanelController: BtwPanelController; @@ -669,6 +670,11 @@ export class SessionEventHandler { } private handleToolProgress(event: ToolProgressEvent): void { + // Input queued before the wait began would otherwise sit until the wait + // returns; steering it now ends the wait so the model reads it first. + if (this.host.streamingUI.markWaitForRunning(event.toolCallId)) { + this.host.steerQueuedMessagesIntoRunningTurn(); + } const text = event.update.text; if (text === undefined || text.length === 0) return; const tc = this.host.streamingUI.getToolComponent(event.toolCallId); diff --git a/apps/pythinker-code/src/tui/controllers/staging-leases.ts b/apps/pythinker-code/src/tui/controllers/staging-leases.ts index b2ec09f7c..0ba1b37da 100644 --- a/apps/pythinker-code/src/tui/controllers/staging-leases.ts +++ b/apps/pythinker-code/src/tui/controllers/staging-leases.ts @@ -89,6 +89,10 @@ export class StagingLeaseTracker { private readonly leasesByTurn = new Map>(); /** Leases carrying a client-chosen submission id, for exact `promptId` binding. */ private readonly leasesBySubmissionId = new Map(); + /** Leases whose dispatch RPC has not settled yet (see {@link trackDispatch}). */ + private readonly inFlight = new Set(); + /** In-flight leases whose turn already ended; released once the RPC settles. */ + private readonly endedInFlight = new Set(); /** * Cache copies whose consuming turn already ended. Persisted history may * still reference their paths (skill/plugin args carry them as plain @@ -167,31 +171,53 @@ export class StagingLeaseTracker { const turnId = String(event.turnId); const leases = this.leasesByTurn.get(turnId); if (leases === undefined) return; - for (const lease of leases) this.releaseConsumed(lease); - this.leasesByTurn.delete(turnId); + for (const lease of leases) { + if (this.inFlight.has(lease)) { + this.endedInFlight.add(lease); + continue; + } + this.releaseConsumed(lease); + } + if (this.leasesByTurn.get(turnId)?.size === 0) this.leasesByTurn.delete(turnId); } /** * Track a dispatch RPC carrying staged media. When it rejects, run * `onError` and release the lease — but only while no turn has claimed it: - * a bound lease is owned by the turn and released at turn end, whatever the - * RPC's later outcome. + * a bound lease is owned by the turn and released at turn end. A turn that + * ends while the RPC is still in flight leaves the lease alone until the RPC + * settles: success releases it as consumed, failure as never consumed — + * unless `onError` handed it back to raw ownership via {@link defer}, as a + * caller that requeues the failed input does. */ trackDispatch( lease: StagingLease | undefined, request: Promise, onError: (error: unknown) => void, ): void { + if (lease !== undefined) this.inFlight.add(lease); this.track( - request - .catch((error: unknown) => { + request.then( + () => { + this.settleInFlight(lease, true); + }, + (error: unknown) => { onError(error); if (lease?.turnId === undefined) this.release(lease); - }) - .then(() => undefined), + this.settleInFlight(lease, false); + }, + ), ); } + private settleInFlight(lease: StagingLease | undefined, consumed: boolean): void { + if (lease === undefined) return; + this.inFlight.delete(lease); + if (!this.endedInFlight.delete(lease)) return; + if (consumed) this.releaseConsumed(lease); + else this.release(lease); + } + /** * Release staged media that will never be consumed (dispatch failed before * a turn claimed the lease): delete daemon uploads and cache copies now. diff --git a/apps/pythinker-code/src/tui/controllers/streaming-ui.ts b/apps/pythinker-code/src/tui/controllers/streaming-ui.ts index 4092ea7c9..c0522d199 100644 --- a/apps/pythinker-code/src/tui/controllers/streaming-ui.ts +++ b/apps/pythinker-code/src/tui/controllers/streaming-ui.ts @@ -58,6 +58,7 @@ export class StreamingUIController { private _activeThinkingComponent: ThinkingComponent | undefined = undefined; private _activeCompactionBlock: CompactionComponent | undefined = undefined; private _activeToolCalls = new Map(); + private _runningWaitForCalls = new Set(); private _streamingToolCallArguments = new Map< string, { name?: string; argumentsText: string; startedAtMs: number } @@ -147,6 +148,21 @@ export class StreamingUIController { return this._activeToolCalls.has(id); } + /** Marks a main-agent WaitFor call as actually waiting — its first + * progress update arrives only once the wait began, never for calls that + * were rejected, skipped, or had nothing to wait for. Returns whether the + * call was newly marked. */ + markWaitForRunning(toolCallId: string): boolean { + if (this._activeToolCalls.get(toolCallId)?.name !== 'WaitFor') return false; + if (this._runningWaitForCalls.has(toolCallId)) return false; + this._runningWaitForCalls.add(toolCallId); + return true; + } + + isWaitForRunning(): boolean { + return this._runningWaitForCalls.size > 0; + } + setActiveToolCall(id: string, toolCall: ToolCallBlockData): void { this._activeToolCalls.set(id, toolCall); } @@ -358,6 +374,7 @@ export class StreamingUIController { this.onToolCallEnd(toolCallId, result); } this._activeToolCalls.delete(toolCallId); + this._runningWaitForCalls.delete(toolCallId); this._streamingToolCallArguments.delete(toolCallId); return matchedCall; } @@ -542,6 +559,7 @@ export class StreamingUIController { resetToolCallState(): void { this._activeToolCalls.clear(); + this._runningWaitForCalls.clear(); } finalizeLiveTextBuffers(nextMode: LivePaneState['mode'] = 'idle'): void { diff --git a/apps/pythinker-code/src/tui/pythinker-tui.ts b/apps/pythinker-code/src/tui/pythinker-tui.ts index 62b5ada8a..15dd103a6 100644 --- a/apps/pythinker-code/src/tui/pythinker-tui.ts +++ b/apps/pythinker-code/src/tui/pythinker-tui.ts @@ -345,6 +345,7 @@ export class PythinkerTUI { private readonly cacheHint = new CacheHintController(this); /** Staged prompt media lifecycle (daemon uploads + cache copies) — see StagingLeaseTracker. */ private readonly staging: StagingLeaseTracker; + private readonly steeringQueuedMessages = new Set(); private readonly approvalController = new ApprovalController(); private readonly questionController = new QuestionController(); private readonly reverseRpcDisposers: Array<() => void> = []; @@ -1629,6 +1630,7 @@ export class PythinkerTUI { recallLastQueued(): QueuedMessage | undefined { if (this.state.queuedMessages.length === 0) return undefined; const last = this.state.queuedMessages.at(-1)!; + if (this.steeringQueuedMessages.has(last)) return undefined; this.state.queuedMessages = this.state.queuedMessages.slice(0, -1); // A recall restores the draft into the editor — it is not a discard: // consumes the retains only, keeping the staged daemon uploads alive @@ -1665,7 +1667,18 @@ export class PythinkerTUI { }, mode?: 'prompt' | 'bash', ): void { - this.state.queuedMessages.push({ + this.state.queuedMessages.push(this.toQueuedMessage(text, options, mode)); + this.track('input_queue'); + } + + private toQueuedMessage( + text: string, + options?: SendMessageOptions & { + readonly inlineSkillActivations?: readonly InlineSkillActivation[]; + }, + mode?: 'prompt' | 'bash', + ): QueuedMessage { + return { text, agentId: this.harness.interactiveAgentId, parts: options?.parts, @@ -1679,8 +1692,7 @@ export class PythinkerTUI { : undefined, mode, inlineSkillActivations: options?.inlineSkillActivations, - }); - this.track('input_queue'); + }; } beginSessionRequest(): void { @@ -1965,53 +1977,23 @@ export class PythinkerTUI { } private sendMessage(session: Session, input: string, options?: SendMessageOptions): void { - const phase = this.state.appState.streamingPhase; - // Tower mode keeps the main agent as a long-lived coordinator: while its - // turn is live, new input steers into that turn instead of queueing - // behind it, so consecutive /tower objectives are accepted immediately - // rather than serialized one turn at a time. A foreground shell command - // ('shell') has no turn to steer into and keeps queue semantics, as do - // input deferral and compaction. - const steerIntoCoordinator = - this.state.appState.towerMode && - phase !== 'idle' && - phase !== 'shell' && - !this.deferUserMessages && - !this.state.appState.isCompacting; // Submission order must survive a mid-turn compaction: objectives queued // while compacting stay queued when the turn outlives the compaction, so // steering this input ahead of them would reorder the conversation. // Prompt-only backlog rides along in the same steer batch, ahead of the // new input; a non-steerable backlog (bash, slash-skill, inline-skill // bundle) cannot, and then this input queues behind it instead. - const backlog = this.state.queuedMessages; - const backlogSteerable = backlog.every( - (m) => m.inlineSkillActivations === undefined && m.mode !== 'bash' && m.mode !== 'skill', - ); - if (steerIntoCoordinator && backlogSteerable) { + if ( + this.canSteerQueueIntoRunningTurn() && + this.state.queuedMessages.every(isSteerableQueuedMessage) + ) { // Same lease hand-off as the queue path below: the pre-dispatch lease - // defers to the raw ids on the steer item, which re-leases inside + // defers to the raw ids on the queue item, which re-leases inside // steerMessage and binds to the running turn. this.staging.defer(options?.lease); - const items: SteerInputItem[] = [ - ...backlog.map((m) => ({ - text: m.text, - parts: m.parts, - imageAttachmentIds: m.imageAttachmentIds, - videoAttachmentIds: m.videoAttachmentIds, - })), - { - text: input, - parts: options?.parts, - imageAttachmentIds: options?.imageAttachmentIds, - videoAttachmentIds: options?.videoAttachmentIds, - }, - ]; - if (backlog.length > 0) { - this.state.queuedMessages = []; - this.updateQueueDisplay(); - } - this.steerMessage(session, items); + this.state.queuedMessages.push(this.toQueuedMessage(input, options)); + this.updateQueueDisplay(); + this.steerQueuedMessagesIntoRunningTurn(); return; } if ( @@ -2028,7 +2010,99 @@ export class PythinkerTUI { this.sendMessageInternal(session, input, options); } - steerMessage(session: Session, input: readonly SteerInputItem[]): void { + // Tower mode keeps the main agent as a long-lived coordinator: while its + // turn is live, input steers into that turn instead of queueing behind it, + // so consecutive /tower objectives are accepted immediately rather than + // serialized one turn at a time. A running WaitFor steers the same way: the + // steer ends the wait at once and the model reads the new input, instead of + // the input queueing until the wait times out. A foreground shell command + // ('shell') has no turn to steer into and keeps queue semantics, as do + // input deferral and compaction. + private canSteerQueueIntoRunningTurn(): boolean { + const phase = this.state.appState.streamingPhase; + return ( + (this.state.appState.towerMode || this.streamingUI.isWaitForRunning()) && + phase !== 'idle' && + phase !== 'shell' && + !this.deferUserMessages && + !this.state.appState.isCompacting + ); + } + + /** Steers the whole queue into the running turn when it is prompt-only. + * The steered items stay at the front of the queue while the steer is in + * flight, and the queue holds (see `shiftQueuedMessage`) so nothing queued + * behind them dispatches first: success removes them, failure leaves them + * queued in place, and a queue left behind by an ended turn drains once + * the steer settles. */ + steerQueuedMessagesIntoRunningTurn(): void { + const session = this.session; + if (session === undefined || this.steeringQueuedMessages.size > 0) return; + if (!this.canSteerQueueIntoRunningTurn()) return; + const batch = [...this.state.queuedMessages]; + if (batch.length === 0 || !batch.every(isSteerableQueuedMessage)) return; + for (const message of batch) this.steeringQueuedMessages.add(message); + this.updateQueueDisplay(); + // Same expiring-upload refresh as the queue drain (`sendQueuedMessage`): + // an image whose daemon upload expired falls back to its retained bytes. + const items = batch.map((message) => { + const item = toSteerInputItem(message); + if (message.parts === undefined) return item; + return { + ...item, + parts: refreshExpiringImageFileRefs( + message.parts, + message.imageAttachmentIds ?? [], + this.imageStore, + ), + }; + }); + this.steerMessage(session, items, (steered) => { + for (const message of batch) this.steeringQueuedMessages.delete(message); + if (this.session !== session) return; + if (steered) { + const done = new Set(batch); + this.state.queuedMessages = this.state.queuedMessages.filter((m) => !done.has(m)); + } + this.updateQueueDisplay(); + if (steered && this.canSteerQueueIntoRunningTurn()) { + this.steerQueuedMessagesIntoRunningTurn(); + return; + } + // A turn that ended while the steer was in flight could not drain the + // held queue. A prompt dispatched now while the engine still runs a + // turn launched by the steer is queued behind it by the engine, so the + // order holds either way. + this.drainQueueIfIdle(); + }); + } + + isSteeringQueuedMessages(): boolean { + return this.steeringQueuedMessages.size > 0; + } + + private drainQueueIfIdle(): void { + if ( + this.state.appState.streamingPhase !== 'idle' || + this.deferUserMessages || + this.state.appState.isCompacting || + this.state.queuedMessageDispatchPending + ) { + return; + } + this.drainOneQueuedMessage(); + } + + /** `onSettled`, when given, runs once the steer settles. On a rejection its + * staged media is first handed back to raw ownership and the user entries + * added for it are removed, so the caller can keep the input queued + * instead of losing it behind an error; a steer from a session that has + * since been replaced is dropped with that session. */ + steerMessage( + session: Session, + input: readonly SteerInputItem[], + onSettled?: (steered: boolean) => void, + ): void { if (this.deferUserMessages || this.state.appState.isCompacting) { for (const item of input) { this.enqueueMessage(item.text, item); @@ -2042,8 +2116,9 @@ export class PythinkerTUI { return; } + const steeredEntries: TranscriptEntry[] = []; for (const item of input) { - this.appendTranscriptEntry({ + const entry: TranscriptEntry = { id: nextTranscriptId(), kind: 'user', turnId: this.streamingUI.getTurnContext().turnId, @@ -2053,7 +2128,9 @@ export class PythinkerTUI { item.imageAttachmentIds !== undefined && item.imageAttachmentIds.length > 0 ? item.imageAttachmentIds : undefined, - }); + }; + steeredEntries.push(entry); + this.appendTranscriptEntry(entry); } // Dedupe per item, not across the batch: each queued message retained a @@ -2079,11 +2156,43 @@ export class PythinkerTUI { ), }, ); - this.staging.trackDispatch(stagingLease, session.steer(combineSteerInput(resolvedInput)), (error) => { + const request = session.steer(combineSteerInput(resolvedInput)); + if (onSettled !== undefined) { + void request.then( + () => onSettled(true), + () => {}, + ); + } + this.staging.trackDispatch(stagingLease, request, (error) => { + if (onSettled !== undefined) { + if (this.session !== session) { + onSettled(false); + return; + } + this.staging.defer(stagingLease); + this.removeTranscriptEntries(steeredEntries); + onSettled(false); + } this.showError(`Failed to steer: ${formatErrorMessage(error)}`); }); } + private removeTranscriptEntries(entries: readonly TranscriptEntry[]): void { + const doomed = new Set(entries); + const componentsToRemove = this.state.transcriptContainer.children.filter((child) => { + const entry = getTranscriptComponentEntry(child); + return entry !== undefined && doomed.has(entry); + }); + for (const child of componentsToRemove) { + // pi-tui Container.removeChild (not a DOM node); `child.remove()` does not exist. + // oxlint-disable-next-line unicorn/prefer-dom-node-remove + this.state.transcriptContainer.removeChild(child); + if (hasDispose(child)) child.dispose(); + } + this.state.transcriptEntries = this.state.transcriptEntries.filter((e) => !doomed.has(e)); + this.state.ui.requestRender(); + } + steerSkillActivation(session: Session, skillName: string, skillArgs: string): void { // Ctrl-S on a queued slash-skill item: the activation fires into the // running turn (the engine steers it there, never the literal text). No @@ -2110,6 +2219,7 @@ export class PythinkerTUI { shiftQueuedMessage(): QueuedMessage | undefined { if (this.state.queuedMessages.length === 0) return undefined; const [first, ...rest] = this.state.queuedMessages; + if (this.steeringQueuedMessages.has(first!)) return undefined; this.state.queuedMessages = rest; return first; } @@ -3470,7 +3580,7 @@ export class PythinkerTUI { messages: queued, isCompacting: this.state.appState.isCompacting, isStreaming: this.state.appState.streamingPhase !== 'idle', - canSteerImmediately: !this.deferUserMessages, + canSteerImmediately: !this.deferUserMessages && !this.isSteeringQueuedMessages(), }), ); } @@ -4235,3 +4345,20 @@ export class PythinkerTUI { this.restoreEditor(); } } + +function isSteerableQueuedMessage(message: QueuedMessage): boolean { + return ( + message.inlineSkillActivations === undefined && + message.mode !== 'bash' && + message.mode !== 'skill' + ); +} + +function toSteerInputItem(message: QueuedMessage): SteerInputItem { + return { + text: message.text, + parts: message.parts, + imageAttachmentIds: message.imageAttachmentIds, + videoAttachmentIds: message.videoAttachmentIds, + }; +} diff --git a/apps/pythinker-code/test/tui/components/messages/tool-call.test.ts b/apps/pythinker-code/test/tui/components/messages/tool-call.test.ts index dc7b38130..a9a17f1d3 100644 --- a/apps/pythinker-code/test/tui/components/messages/tool-call.test.ts +++ b/apps/pythinker-code/test/tui/components/messages/tool-call.test.ts @@ -2249,6 +2249,36 @@ describe('ToolCallComponent', () => { ); }); + it('renders a wait ended by new input as interrupted', () => { + const component = new ToolCallComponent( + { + id: 'call_wait_interrupted', + name: 'WaitFor', + args: { task_id: 'bash-x', timeout: 60 }, + }, + { + tool_call_id: 'call_wait_interrupted', + output: [ + 'wait_status: interrupted', + 'reason: steer', + 'task_id: bash-x', + 'waited_ms: 4000', + 'timeout_ms: 60000', + '', + '[still_running]', + 'active_background_tasks: 1', + 'task_id: bash-x', + 'description: slow build', + ].join('\n'), + is_error: false, + }, + ); + + const out = strip(component.render(100).join('\n')); + expect(out).toContain('Wait interrupted by new input (bash-x)'); + expect(out).toContain('1 background task still running: slow build'); + }); + it('renders errors with the failure tense', () => { const component = new ToolCallComponent( { diff --git a/apps/pythinker-code/test/tui/controllers/editor-keyboard.test.ts b/apps/pythinker-code/test/tui/controllers/editor-keyboard.test.ts index ba1b34c50..806f62e65 100644 --- a/apps/pythinker-code/test/tui/controllers/editor-keyboard.test.ts +++ b/apps/pythinker-code/test/tui/controllers/editor-keyboard.test.ts @@ -487,6 +487,7 @@ describe('EditorKeyboardController Ctrl-S steering', () => { editorText: string; queued: Array>; skillCommandMap?: Map; + steering?: boolean; }) { const steerMessage = vi.fn(); const steerSkillActivation = vi.fn(); @@ -513,6 +514,7 @@ describe('EditorKeyboardController Ctrl-S steering', () => { steerMessage, steerSkillActivation, updateQueueDisplay, + isSteeringQueuedMessages: vi.fn(() => options.steering ?? false), validateMediaCapabilities: vi.fn(() => true), showError: vi.fn(), track: vi.fn(), @@ -566,6 +568,25 @@ describe('EditorKeyboardController Ctrl-S steering', () => { expect(updateQueueDisplay).toHaveBeenCalled(); }); + it('ignores Ctrl-S while an automatic queue steer is still in flight', () => { + const queued = [ + { text: 'already steering', agentId: 'main' }, + { text: 'later text', agentId: 'main' }, + ]; + const { host, setText, steerMessage, steerSkillActivation, onCtrlS } = createCtrlSHarness({ + editorText: 'draft', + queued, + steering: true, + }); + + onCtrlS(); + + expect(steerMessage).not.toHaveBeenCalled(); + expect(steerSkillActivation).not.toHaveBeenCalled(); + expect(setText).not.toHaveBeenCalled(); + expect(host.state.queuedMessages).toEqual(queued); + }); + it('steers plain queued messages but keeps grouped inline-skill submissions queued', () => { const { host, steerMessage, updateQueueDisplay, onCtrlS } = createCtrlSHarness({ editorText: '', diff --git a/apps/pythinker-code/test/tui/controllers/staging-leases.test.ts b/apps/pythinker-code/test/tui/controllers/staging-leases.test.ts index 50f3c26ff..0d1d9c070 100644 --- a/apps/pythinker-code/test/tui/controllers/staging-leases.test.ts +++ b/apps/pythinker-code/test/tui/controllers/staging-leases.test.ts @@ -338,6 +338,66 @@ describe('StagingLeaseTracker', () => { }); }); + describe('trackDispatch across turn end', () => { + const origin: StagingLeaseOrigin = 'user'; + + function pending(): { promise: Promise; resolve: () => void; reject: (error: Error) => void } { + let resolve!: () => void; + let reject!: (error: Error) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { promise, resolve, reject }; + } + + it('holds an in-flight lease past turn end and releases it once the dispatch succeeds', async () => { + const { tracker, deleted } = makeTracker(); + const lease = tracker.create([1], ['/cache/a'], origin); + tracker.bindToTurn(lease, '7'); + const request = pending(); + + tracker.trackDispatch(lease, request.promise, vi.fn()); + tracker.handleTurnEnded(turnEnded(7)); + expect(deleted.fileIds).toEqual([]); + + request.resolve(); + await tracker.drain(); + expect(deleted.fileIds).toEqual(['file-1']); + expect(deleted.paths).toEqual([]); + }); + + it('keeps media a failed in-flight dispatch hands back after its turn ended', async () => { + const { tracker, deleted } = makeTracker(); + const lease = tracker.create([1], ['/cache/a'], origin); + tracker.bindToTurn(lease, '7'); + const request = pending(); + + tracker.trackDispatch(lease, request.promise, () => tracker.defer(lease)); + tracker.handleTurnEnded(turnEnded(7)); + request.reject(new Error('boom')); + await tracker.drain(); + + expect(deleted.fileIds).toEqual([]); + expect(deleted.paths).toEqual([]); + }); + + it('deletes media of a failed in-flight dispatch nobody reclaims after its turn ended', async () => { + const { tracker, deleted } = makeTracker(); + const lease = tracker.create([1], ['/cache/a'], origin); + tracker.bindToTurn(lease, '7'); + const request = pending(); + + tracker.trackDispatch(lease, request.promise, vi.fn()); + tracker.handleTurnEnded(turnEnded(7)); + request.reject(new Error('boom')); + await tracker.drain(); + + expect(deleted.fileIds).toEqual(['file-1']); + expect(deleted.paths).toEqual(['/cache/a']); + }); + }); + describe('track/drain', () => { it('drain awaits in-flight cleanups and track swallows rejections', async () => { const { tracker } = makeTracker(); diff --git a/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts b/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts index cdf984286..96607936f 100644 --- a/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts +++ b/apps/pythinker-code/test/tui/pythinker-tui-message-flow.test.ts @@ -3696,12 +3696,357 @@ command = "vim" expect(session.steer).toHaveBeenCalledWith('second objective'); expect(session.prompt).not.toHaveBeenCalled(); - expect(driver.state.queuedMessages).toEqual([]); + await vi.waitFor(() => { + expect(driver.state.queuedMessages).toEqual([]); + }); expect(driver.state.transcriptEntries).toEqual([ expect.objectContaining({ kind: 'user', content: 'second objective' }), ]); }); + function waitForEvent( + type: 'tool.call.started' | 'tool.progress' | 'tool.result', + extra: Record = {}, + ): Event { + const base = { agentId: 'main', sessionId: 'ses-1', turnId: 1, toolCallId: 'call_wait' }; + if (type === 'tool.call.started') { + return { type, ...base, name: 'WaitFor', args: { timeout: 60 }, ...extra } as Event; + } + if (type === 'tool.progress') { + return { + type, + ...base, + update: { kind: 'status', text: 'Waiting 0s / 1m · 1 background task still running', replace: true }, + ...extra, + } as Event; + } + return { type, ...base, output: 'wait_status: timed_out', ...extra } as Event; + } + + function startWaitFor(driver: MessageDriver, extra: Record = {}): void { + driver.sessionEventHandler.handleEvent(waitForEvent('tool.call.started', extra), () => {}); + driver.sessionEventHandler.handleEvent(waitForEvent('tool.progress', extra), () => {}); + } + + function pendingSteer(): { session: ReturnType; resolve: () => void; reject: (error: Error) => void } { + let resolve!: () => void; + let reject!: (error: Error) => void; + const session = makeSession({ + steer: vi.fn( + () => + new Promise((res, rej) => { + resolve = res; + reject = rej; + }), + ), + }); + return { session, resolve: () => resolve(), reject: (error) => reject(error) }; + } + + it('steers fresh input into the running turn while a WaitFor call is running', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + + driver.handleUserInput('stop waiting and check this'); + + expect(session.steer).toHaveBeenCalledWith('stop waiting and check this'); + expect(session.prompt).not.toHaveBeenCalled(); + await vi.waitFor(() => { + expect(driver.state.queuedMessages).toEqual([]); + }); + }); + + it('steers messages queued before a WaitFor starts into the running turn', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('first note'); + driver.handleUserInput('second note'); + expect(driver.state.queuedMessages).toHaveLength(2); + + startWaitFor(driver); + + expect(session.steer).toHaveBeenCalledTimes(1); + expect(session.steer).toHaveBeenCalledWith('first note\n\nsecond note'); + await vi.waitFor(() => { + expect(driver.state.queuedMessages).toEqual([]); + }); + }); + + it('refreshes an expired queued image upload before steering it into a WaitFor', async () => { + const { driver, session } = await makeDriver(); + const imageStore = (driver as unknown as { imageStore: ImageAttachmentStore }).imageStore; + const attachment = imageStore.addImage( + new Uint8Array([0xaa, 0xbb]), + 'image/png', + 1, + 1, + undefined, + 'file-expired', + 1, + ); + driver.state.appState.streamingPhase = 'waiting'; + driver.state.queuedMessages.push({ + text: `describe ${attachment.placeholder}`, + agentId: 'main', + parts: [{ type: 'image_url', imageUrl: { url: 'pythinker-file://file-expired' } }], + imageAttachmentIds: [attachment.id], + }); + + startWaitFor(driver); + + expect(session.steer).toHaveBeenCalledTimes(1); + const steered = JSON.stringify(vi.mocked(session.steer).mock.calls[0]); + expect(steered).toContain('data:image/png;base64,qrs='); + expect(steered).not.toContain('pythinker-file://file-expired'); + }); + + it('does not steer the queue for a WaitFor call that never starts waiting', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('queued note'); + + driver.sessionEventHandler.handleEvent(waitForEvent('tool.call.started', { args: { timeout: 600 } }), () => {}); + driver.handleUserInput('typed while the call was rejected'); + driver.sessionEventHandler.handleEvent(waitForEvent('tool.result', { output: 'invalid arguments', isError: true }), () => {}); + + expect(session.steer).not.toHaveBeenCalled(); + expect(driver.state.queuedMessages).toEqual([ + { text: 'queued note', agentId: 'main' }, + { text: 'typed while the call was rejected', agentId: 'main' }, + ]); + }); + + it('keeps the queue in place when steering it into a WaitFor fails', async () => { + const steer = pendingSteer(); + const { driver } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('first note'); + driver.handleUserInput('second note'); + const queued = [...driver.state.queuedMessages]; + + startWaitFor(driver); + steer.reject(new Error('session closed')); + + await vi.waitFor(() => { + expect(driver.state.transcriptEntries.filter((entry) => entry.kind === 'user')).toEqual([]); + }); + expect(driver.state.queuedMessages).toEqual(queued); + }); + + it('keeps a queued video upload when steering it into a WaitFor fails', async () => { + const steer = pendingSteer(); + const { driver, harness } = await makeDriver(steer.session); + const imageStore = (driver as unknown as { imageStore: ImageAttachmentStore }).imageStore; + const attachment = imageStore.addVideo('video/mp4', '/tmp/clip.mp4'); + imageStore.completeVideo(attachment, { fileId: 'file-v1' }); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput(`describe ${attachment.placeholder}`); + + startWaitFor(driver); + steer.reject(new Error('session closed')); + await (driver as unknown as { staging: { drain(): Promise } }).staging.drain(); + driver.sessionEventHandler.handleEvent( + { type: 'turn.ended', agentId: 'main', turnId: 1, reason: 'completed' } as Event, + () => {}, + ); + await (driver as unknown as { staging: { drain(): Promise } }).staging.drain(); + + expect(harness.deleteFile).not.toHaveBeenCalledWith('file-v1'); + }); + + it('keeps submission order when a WaitFor steer fails after later input was queued', async () => { + const steer = pendingSteer(); + const { driver } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + + driver.handleUserInput('check this first'); + driver.state.queuedMessages.push({ text: 'make build', agentId: 'main', mode: 'bash' }); + steer.reject(new Error('session closed')); + + await vi.waitFor(() => { + expect(driver.state.transcriptEntries.filter((entry) => entry.kind === 'user')).toEqual([]); + }); + expect(driver.state.queuedMessages).toEqual([ + { text: 'check this first', agentId: 'main' }, + { text: 'make build', agentId: 'main', mode: 'bash' }, + ]); + }); + + it('steers input typed while an earlier WaitFor steer is in flight once it succeeds', async () => { + const steer = pendingSteer(); + const { driver } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + + driver.handleUserInput('first'); + driver.handleUserInput('second'); + expect(steer.session.steer).toHaveBeenCalledTimes(1); + steer.resolve(); + + await vi.waitFor(() => { + expect(steer.session.steer).toHaveBeenCalledTimes(2); + }); + expect(steer.session.steer).toHaveBeenNthCalledWith(2, 'second'); + expect(driver.state.queuedMessages).toEqual([{ text: 'second', agentId: 'main' }]); + }); + + it('drains input queued behind a steer that succeeds after the turn ended', async () => { + const steer = pendingSteer(); + const { driver, session } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('earlier note'); + startWaitFor(driver); + driver.state.queuedMessages.push({ text: 'later note', agentId: 'main' }); + + driver.sessionEventHandler.handleEvent( + { type: 'turn.ended', agentId: 'main', turnId: 1, reason: 'completed' } as Event, + () => {}, + ); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(session.prompt).not.toHaveBeenCalled(); + + steer.resolve(); + + await vi.waitFor(() => { + expect(session.prompt).toHaveBeenCalledTimes(1); + }); + expect(vi.mocked(session.prompt).mock.calls[0]?.[0]).toBe('later note'); + }); + + it('does not offer Ctrl-S in the queue pane while a queue steer is in flight', async () => { + const steer = pendingSteer(); + const { driver } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('queued note'); + expect(stripSgr(driver.state.queueContainer.render(120).join('\n'))).toContain( + 'ctrl-s to steer immediately', + ); + + startWaitFor(driver); + + expect(stripSgr(driver.state.queueContainer.render(120).join('\n'))).not.toContain( + 'ctrl-s to steer immediately', + ); + }); + + it('holds the queue behind an in-flight WaitFor steer across turn end', async () => { + const steer = pendingSteer(); + const { driver, session } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('earlier note'); + startWaitFor(driver); + driver.state.queuedMessages.push({ text: 'later note', agentId: 'main' }); + + driver.sessionEventHandler.handleEvent( + { type: 'turn.ended', agentId: 'main', turnId: 1, reason: 'cancelled' } as Event, + () => {}, + ); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(session.prompt).not.toHaveBeenCalled(); + + steer.reject(new Error('turn cancelled')); + + await vi.waitFor(() => { + expect(session.prompt).toHaveBeenCalledTimes(1); + }); + expect(vi.mocked(session.prompt).mock.calls[0]?.[0]).toBe('earlier note'); + expect(driver.state.queuedMessages).toEqual([{ text: 'later note', agentId: 'main' }]); + }); + + it('dispatches failed WaitFor input with its media right away when the turn ended meanwhile', async () => { + const steer = pendingSteer(); + const { driver, harness, session } = await makeDriver(steer.session); + const imageStore = (driver as unknown as { imageStore: ImageAttachmentStore }).imageStore; + const attachment = imageStore.addVideo('video/mp4', '/tmp/clip.mp4'); + imageStore.completeVideo(attachment, { fileId: 'file-v1' }); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + driver.handleUserInput(`describe ${attachment.placeholder}`); + + driver.sessionEventHandler.handleEvent( + { type: 'turn.ended', agentId: 'main', turnId: 1, reason: 'cancelled' } as Event, + () => {}, + ); + expect(harness.deleteFile).not.toHaveBeenCalledWith('file-v1'); + steer.reject(new Error('turn cancelled')); + + await vi.waitFor(() => { + expect(session.prompt).toHaveBeenCalledTimes(1); + }); + const parts = vi.mocked(session.prompt).mock.calls[0]?.[0] as Array<{ type: string; videoUrl?: { url: string } }>; + expect(parts).toContainEqual({ type: 'video_url', videoUrl: { url: 'pythinker-file://file-v1' } }); + await (driver as unknown as { staging: { drain(): Promise } }).staging.drain(); + expect(harness.deleteFile).not.toHaveBeenCalledWith('file-v1'); + expect(driver.state.queuedMessages).toEqual([]); + }); + + it('drops failed WaitFor input when the session changed meanwhile', async () => { + const steer = pendingSteer(); + const { driver } = await makeDriver(steer.session); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + driver.handleUserInput('for the old session'); + + const next = makeSession(); + (driver as unknown as { resetSessionRuntime(): void }).resetSessionRuntime(); + (driver as unknown as { session: unknown }).session = next; + steer.reject(new Error('session closed')); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(driver.state.queuedMessages).toEqual([]); + expect(next.prompt).not.toHaveBeenCalled(); + }); + + it('keeps a queue with a bash command queued when a WaitFor starts', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + driver.state.queuedMessages = [ + { text: 'note', agentId: 'main' }, + { text: 'make build', agentId: 'main', mode: 'bash' }, + ]; + + startWaitFor(driver); + + expect(session.steer).not.toHaveBeenCalled(); + expect(driver.state.queuedMessages).toHaveLength(2); + }); + + it('leaves queued messages alone when a tool other than WaitFor reports progress', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + driver.handleUserInput('queued note'); + + startWaitFor(driver, { toolCallId: 'call_bash', name: 'Bash', args: { command: 'sleep 5' } }); + + expect(session.steer).not.toHaveBeenCalled(); + expect(driver.state.queuedMessages).toEqual([{ text: 'queued note', agentId: 'main' }]); + }); + + it('queues input again once the WaitFor call has returned', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver); + driver.sessionEventHandler.handleEvent(waitForEvent('tool.result'), () => {}); + + driver.handleUserInput('after the wait'); + + expect(session.steer).not.toHaveBeenCalled(); + expect(driver.state.queuedMessages).toEqual([{ text: 'after the wait', agentId: 'main' }]); + }); + + it('queues input while only a subagent is running WaitFor', async () => { + const { driver, session } = await makeDriver(); + driver.state.appState.streamingPhase = 'waiting'; + startWaitFor(driver, { agentId: 'agent-child', toolCallId: 'call_child_wait' }); + + driver.handleUserInput('main agent is busy'); + + expect(session.steer).not.toHaveBeenCalled(); + expect(driver.state.queuedMessages).toEqual([{ text: 'main agent is busy', agentId: 'main' }]); + }); + it('prompts immediately while tower mode is active and the session is idle', async () => { const { driver, session } = await makeDriver(); driver.state.appState.towerMode = true; @@ -3754,7 +4099,9 @@ command = "vim" expect(session.steer).toHaveBeenCalledWith('objective one\n\nobjective two'); expect(session.prompt).not.toHaveBeenCalled(); - expect(driver.state.queuedMessages).toEqual([]); + await vi.waitFor(() => { + expect(driver.state.queuedMessages).toEqual([]); + }); }); it('queues fresh input behind a non-steerable backlog instead of jumping ahead', async () => { diff --git a/docs/guides/interaction.md b/docs/guides/interaction.md index b78250631..15f25f51f 100644 --- a/docs/guides/interaction.md +++ b/docs/guides/interaction.md @@ -130,7 +130,7 @@ The input box remains usable while the agent is thinking or calling tools, and s - **`Esc` / `Ctrl-C`**: interrupt the current turn - **`Ctrl-O`**: globally toggle the collapsed/expanded state of tool output and compaction summaries -When the agent is waiting for background tasks through `WaitFor`, pressing `Ctrl-S` ends that wait early. Background tasks keep running and existing tool results are preserved. If other foreground tools remain in the same batch, the agent processes your message after they return. +When the agent is waiting for background tasks through `WaitFor`, sending a message with `Enter` (or `Ctrl-S`) steers it in and ends that wait early; messages already queued when the wait starts are steered in as well. If the queue holds a shell command or a skill, the queue and new input stay queued until the turn ends instead. Background tasks keep running and existing tool results are preserved. If other foreground tools remain in the same batch, the agent processes your message after they return. ## External editor diff --git a/docs/reference/tools.md b/docs/reference/tools.md index 6f52cf464..830f00202 100644 --- a/docs/reference/tools.md +++ b/docs/reference/tools.md @@ -131,7 +131,7 @@ Background task tools manage tasks started via `Bash`, `Agent`, or `AskUserQuest **`TaskStop`** accepts a `task_id` and optional `reason` (defaults to `Stopped by TaskStop`). Safe to call on tasks that are already in a terminal state. -**`WaitFor`** suspends the current turn until a background task finishes, the timeout elapses, or a steer message arrives. Parameters: `timeout` (required, in seconds, max 600) and optional `task_id`. Without `task_id`, the wait ends as soon as any background task that was running at call time finishes; when no background tasks are running, it returns immediately. A timeout is not an error — the result lists the tasks still running, and the Agent can wait again or do other work meanwhile. Steering (`Ctrl-S` in the terminal) ends the wait early; background tasks keep running and still notify the agent on completion. A task whose result was reported by `WaitFor` does not also produce an automatic completion notification. +**`WaitFor`** suspends the current turn until a background task finishes, the timeout elapses, or a steer message arrives. Parameters: `timeout` (required, in seconds, max 90) and optional `task_id`. Without `task_id`, the wait ends as soon as any background task that was running at call time finishes; when no background tasks are running, it returns immediately. A timeout is not an error — the result lists the tasks still running, and the Agent can wait again or do other work meanwhile. A new message ends the wait early — in the terminal, pressing `Enter` while the agent waits steers the message into the turn, as does `Ctrl-S` — and messages already queued when the wait starts are steered in too (unless the queue holds a shell command or skill, which keeps the queue and new input queued until the turn ends); background tasks keep running and still notify the agent on completion. A task whose result was reported by `WaitFor` does not also produce an automatic completion notification. ## Scheduled Tasks diff --git a/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait-subagent.md b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait-subagent.md new file mode 100644 index 000000000..a4f8d5da1 --- /dev/null +++ b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait-subagent.md @@ -0,0 +1,2 @@ + +You are running as a subagent, which changes the guidance above: ending your turn is your final hand-off to the parent agent, and completion notifications that arrive after it reach no one. Do not end your turn while a background task whose result you need is still running. Keep waiting for it with WaitFor, calling it again after a timeout if needed, or run the command in the foreground with a suitable timeout instead. Still use the waiting time for other useful work on your task when you can. diff --git a/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.md b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.md index 30ebbc8fa..e93756117 100644 --- a/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.md +++ b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.md @@ -1,16 +1,19 @@ Wait for background tasks to finish without ending the current turn. -Use this when your next step depends on the result of a running background task (a sub-agent, a background bash command, or a background AskUserQuestion). The call suspends inside the current turn until the task finishes or the timeout elapses, then returns the outcome so you can keep working in the same turn. While waiting, no LLM requests are made. +Only call this tool when you really have no other work to do and your next step cannot proceed without the result of a running background task (a sub-agent, a background bash command, or a background AskUserQuestion). The call suspends inside the current turn until the task finishes or the timeout elapses, then returns the outcome so you can keep working in the same turn. While waiting, no LLM requests are made, but the user is kept waiting too. + +When background tasks finish, they notify you automatically, so you do not need to busily wait for them. Guidelines: -- Do not call WaitFor right after dispatching work whose result you do not need yet — finished background tasks notify you automatically. WaitFor is for the moment you genuinely cannot proceed without a result. -- `timeout` is required, in seconds, capped at 600. To wait longer, call WaitFor again; waking up periodically also lets you re-evaluate the situation. -- A timeout is not an error: the result lists the tasks that are still running, and you decide whether to wait again or do other work meanwhile. +- Before calling WaitFor, think about what else you can do meanwhile: another part of the task, verifying earlier work, or ending your turn with a progress update. If there is anything, do that instead. +- Do not call WaitFor right after dispatching work whose result you do not need yet. +- `timeout` is required, in seconds, capped at 90. Pick it from how long you expect the task to take, not the maximum. +- A timeout is not an error: the result lists the tasks that are still running. Prefer moving on to other work over calling WaitFor again; repeated waits keep the user waiting. - Without `task_id`, the wait ends as soon as any background task that was running at call time finishes. Tasks started during the wait are not covered by it; their completion arrives via the usual automatic notification. - With `task_id`, the wait ends when that task finishes. An unknown `task_id` is an error; a task that has already finished returns immediately. - When no background tasks are running, WaitFor returns immediately without waiting. - When the wait ends because a task finished, the result also lists other tasks that finished during the wait window, so failures surface with context. -- Waiting has no side effects on the waited tasks: WaitFor never stops a task, and interrupting the wait (for example, a user interruption) leaves every task running. +- Waiting has no side effects on the waited tasks: WaitFor never stops a task, and interrupting the wait (for example, a new user message) leaves every task running. - A finished task's result is delivered exactly once: tasks reported by WaitFor do not also produce an automatic completion notification. - You can only wait for background tasks started by this agent; task IDs belonging to other agents are unknown here. diff --git a/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.ts b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.ts index 69b001414..ef9d18df9 100644 --- a/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.ts +++ b/packages/agent-core-v2/src/agent/tools/task/task-wait/task-wait.ts @@ -2,9 +2,8 @@ import { z } from 'zod'; import { createDecorator } from '#/_base/di/instantiation'; import { type AgentTool } from '#/tool/toolContract'; -import { DEFAULT_BACKGROUND_TIMEOUT_S } from '#/agent/tools/os/bash/bash'; -export const WAIT_FOR_MAX_TIMEOUT_S = DEFAULT_BACKGROUND_TIMEOUT_S; +export const WAIT_FOR_MAX_TIMEOUT_S = 90; export const WaitForInputSchema = z.object({ timeout: z @@ -13,7 +12,7 @@ export const WaitForInputSchema = z.object({ .positive() .max(WAIT_FOR_MAX_TIMEOUT_S) .describe( - `Maximum time to wait, in seconds (1-${String(WAIT_FOR_MAX_TIMEOUT_S)}). A timeout is not an error: the tool returns the tasks that are still running, and you can call it again to keep waiting.`, + `Maximum time to wait, in seconds (1-${String(WAIT_FOR_MAX_TIMEOUT_S)}). Pick it from how long you expect the task to take, not the maximum. A timeout is not an error: the tool returns the tasks that are still running.`, ), task_id: z .string() diff --git a/packages/agent-core-v2/src/agent/tools/task/task-wait/taskWaitTool.ts b/packages/agent-core-v2/src/agent/tools/task/task-wait/taskWaitTool.ts index 81d570d91..d76c46f9a 100644 --- a/packages/agent-core-v2/src/agent/tools/task/task-wait/taskWaitTool.ts +++ b/packages/agent-core-v2/src/agent/tools/task/task-wait/taskWaitTool.ts @@ -8,17 +8,21 @@ import { } from '#/tool/toolContract'; import { registerAgentToolService } from '#/agent/toolRegistry/toolContribution'; +import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; import { IAgentTaskService } from '#/agent/task/task'; import type { AgentTaskInfo, AgentTaskOutputSnapshot } from '#/agent/task/task'; import { TERMINAL_STATUSES } from '#/agent/task/types'; import { formatPlainObject, formatTaskRecord } from '#/agent/task/tools/format'; import { formatTaskList } from '#/agent/tools/task/task-list/taskListTool'; import { IFlagService } from '#/app/flag/flag'; +import { IAgentGoalService } from '#/features/goal/goalService'; import { ITelemetryService } from '#/app/telemetry/telemetry'; +import { MAIN_AGENT_ID } from '#/session/agentLifecycle/agentLifecycle'; import { abortError, isAbortError, linkAbortSignal } from '#/_base/utils/abort'; import { WAIT_FOR_FLAG_ID } from './flag'; import { IWaitForTool, WaitForInputSchema, type WaitForInput } from './task-wait'; import WAIT_FOR_DESCRIPTION from './task-wait.md?raw'; +import WAIT_FOR_SUBAGENT_GUIDANCE from './task-wait-subagent.md?raw'; const OUTPUT_PREVIEW_BYTES = 32 * 1024; @@ -28,6 +32,12 @@ const PROGRESS_INTERVAL_MS = 1_000; type WaitForOutcome = 'completed' | 'timed_out' | 'task_not_found' | 'aborted' | 'interrupted'; +interface TurnWaitTally { + readonly turnId: number; + calls: number; + waitedMs: number; +} + function terminalReason(info: AgentTaskInfo): 'timed_out' | 'stopped' | 'failed' | undefined { if (info.status === 'timed_out') return 'timed_out'; if (info.status === 'killed' && info.stopReason !== undefined) return 'stopped'; @@ -100,7 +110,6 @@ export function startWaitProgress( const tick = (): void => { onUpdate(waitForProgressUpdate(args, tasks.list(true).length, startedAt, Date.now())); }; - tick(); const interval = setInterval(tick, PROGRESS_INTERVAL_MS); interval.unref?.(); return { @@ -114,14 +123,24 @@ export function startWaitProgress( export class WaitForTool implements IWaitForTool { declare readonly _serviceBrand: undefined; readonly name = 'WaitFor' as const; - readonly description: string = WAIT_FOR_DESCRIPTION; + readonly description: string; readonly parameters: Record = toInputJsonSchema(WaitForInputSchema); + private readonly isSubagent: boolean; + private tally: TurnWaitTally | undefined; + constructor( @IAgentTaskService private readonly tasks: IAgentTaskService, @ITelemetryService private readonly telemetry: ITelemetryService, @IFlagService private readonly flags: IFlagService, - ) {} + @IAgentGoalService private readonly goals: IAgentGoalService, + @IAgentScopeContext scopeContext: IAgentScopeContext, + ) { + this.isSubagent = scopeContext.agentId !== MAIN_AGENT_ID; + this.description = this.isSubagent + ? `${WAIT_FOR_DESCRIPTION.trimEnd()}\n${WAIT_FOR_SUBAGENT_GUIDANCE}` + : WAIT_FOR_DESCRIPTION; + } resolveExecution(args: WaitForInput): ToolExecution { return { @@ -145,6 +164,7 @@ export class WaitForTool implements IWaitForTool { output: 'WaitFor is disabled: the wait_for experimental flag is off.', }; } + const tally = this.countCall(ctx.turnId); const startedAt = Date.now(); const timeoutMs = args.timeout * 1000; const runningAtStart = this.tasks.list(true); @@ -153,16 +173,19 @@ export class WaitForTool implements IWaitForTool { if (runningAtStart.length === 0) { this.track(args, startedAt, timeoutMs, 'completed', 0); return { - output: [ - formatPlainObject({ waitStatus: 'no_tasks', waitedMs: 0, timeoutMs }), - 'No background tasks are running, so there is nothing to wait for. Finished tasks report back via automatic notification.', - ].join('\n\n'), + output: this.withRepeatWarning( + [ + formatPlainObject({ waitStatus: 'no_tasks', waitedMs: 0, timeoutMs }), + 'No background tasks are running, so there is nothing to wait for. Finished tasks report back via automatic notification.', + ].join('\n\n'), + tally, + ), isError: false, }; } } else if (this.tasks.getTask(args.task_id) === undefined) { this.track(args, startedAt, timeoutMs, 'task_not_found', 0); - return { isError: true, output: `Task not found: ${args.task_id}` }; + return { isError: true, output: this.withRepeatWarning(`Task not found: ${args.task_id}`, tally) }; } let waited: AgentTaskInfo | undefined; @@ -181,22 +204,30 @@ export class WaitForTool implements IWaitForTool { (error === ctx.steerSignal.reason || isAbortError(error)) ) { this.track(args, startedAt, timeoutMs, 'interrupted', 0); - return { output: this.formatInterrupted(args, startedAt, timeoutMs), isError: false }; + tally.waitedMs += Date.now() - startedAt; + return { + output: this.withRepeatWarning(this.formatInterrupted(args, startedAt, timeoutMs), tally), + isError: false, + }; } this.track(args, startedAt, timeoutMs, 'aborted', 0); throw error; } finally { progress.stop(); } + tally.waitedMs += Date.now() - startedAt; if (waited === undefined) { this.track(args, startedAt, timeoutMs, 'task_not_found', 0); - return { isError: true, output: `Task not found: ${args.task_id ?? ''}` }; + return { isError: true, output: this.withRepeatWarning(`Task not found: ${args.task_id ?? ''}`, tally) }; } if (!TERMINAL_STATUSES.has(waited.status)) { this.track(args, startedAt, timeoutMs, 'timed_out', 0); - return { output: this.formatTimeout(args, startedAt, timeoutMs), isError: false }; + return { + output: this.withRepeatWarning(this.formatTimeout(args, startedAt, timeoutMs), tally), + isError: false, + }; } const extras = this.collectExtras(runningAtStart, waited.taskId); @@ -205,7 +236,32 @@ export class WaitForTool implements IWaitForTool { [waited, ...extras].map((info) => ({ taskId: info.taskId, status: info.status })), ); this.track(args, startedAt, timeoutMs, 'completed', extras.length); - return { output, isError: false }; + return { output: this.withRepeatWarning(output, tally), isError: false }; + } + + private countCall(turnId: number): TurnWaitTally { + if (this.tally?.turnId !== turnId) { + this.tally = { turnId, calls: 0, waitedMs: 0 }; + } + this.tally.calls += 1; + return this.tally; + } + + private withRepeatWarning(output: string, tally: TurnWaitTally): string { + if (tally.calls < 2) return output; + const waited = formatWaitSeconds(Math.round(tally.waitedMs / 1000)); + const summary = `This is WaitFor call ${String(tally.calls)} in this turn, and you have already spent ${waited} waiting.`; + return [output, '', '[wait_warning]', `${summary} ${this.repeatWaitAdvice()}`].join('\n'); + } + + private repeatWaitAdvice(): string { + if (this.isSubagent) { + return "Stop waiting by reflex: repeated waits burn time your caller is waiting on. Before calling WaitFor again, do every part of your task that does not depend on the running background task. Wait again only for a result you truly cannot finish without — ending your turn is your final hand-off, so do not hand off without it, but never wait for tasks you do not need."; + } + if (this.goals.getGoal().goal?.status === 'active') { + return 'Stop waiting by reflex: repeated waits stall the goal. Before calling WaitFor again, do every piece of remaining goal work that does not depend on the running background task — there is almost always some. Wait again only if nothing else can proceed until it finishes; even then, WaitFor beats polling with Bash sleep or ending the turn only to be continued again.'; + } + return 'Stop calling WaitFor. Repeated waiting wastes the user\'s time and is almost never the right move. Do not call it again in this turn unless the user explicitly asked you to wait. Do something useful now — another part of the task, or verifying earlier work — or end your turn with a progress update. Finished background tasks notify you automatically, so you will not miss the result.'; } private async waitAny( @@ -255,7 +311,9 @@ export class WaitForTool implements IWaitForTool { waitedMs: Date.now() - startedAt, timeoutMs, }), - 'The wait ended before the task finished — a timeout is not an error. Call WaitFor again to keep waiting, or continue with other work; completion also arrives via automatic notification.', + this.isSubagent + ? 'The wait ended before the task finished — a timeout is not an error. If you need its result, call WaitFor again: ending your turn is your hand-off, and a completion notification after it reaches no one.' + : 'The wait ended before the task finished — a timeout is not an error. Prefer continuing with other work over waiting again; completion arrives via automatic notification.', ]; const running = this.tasks.list(true); if (running.length > 0) { diff --git a/packages/agent-core-v2/test/agent/loop/loop.test.ts b/packages/agent-core-v2/test/agent/loop/loop.test.ts index 97908441e..1ae266846 100644 --- a/packages/agent-core-v2/test/agent/loop/loop.test.ts +++ b/packages/agent-core-v2/test/agent/loop/loop.test.ts @@ -196,8 +196,8 @@ describe('Agent loop', () => { [emit] turn.step.started { "time": "