From 0177aa0c1464bb5291926b254d5c0b0113139397 Mon Sep 17 00:00:00 2001 From: ranxianglei Date: Thu, 17 Sep 2026 11:45:12 +0800 Subject: [PATCH 1/2] fix: recognize failed V2 ACP tool results as failures (#426) The pinned @opencode/plugin@2.0.3 Promise adapter has no safe native error channel, so createV2Tool returns resolved results for caught failures and the host records them as status completed. ACP's internal V2 projection preserved that completed status, so messageHasCompress, nudge success baselines, and cold-start reconstruction treated failed compressions as successes; the installed E2E fake provider classifier missed the same failure texts. - errorResult() now adds acpFailed:true metadata to every resolved error result (additive; existing per-reason flags unchanged) - new pure module lib/v2/projection/acp-failure.ts holds the canonical tool-name list, metadata key, anchored failure-text pattern, and predicates shared by the projection and the E2E fake provider - toolState projects a completed host state for an ACP tool to an internal status:error when explicit failure metadata is present or the output matches the historical failure-text pattern; it exposes no output field, so provider-owned lowered results are never rewritten (origin correlation + patcher invariants hold) - fake-llm-server inspectToolResults classifies ACP results via the shared anchored pattern; legacy regex retained for other tools - installed-v2 nudge-growth asserts genuine completed status for both compress results - tests: 18 new tests (pure module, projection, cold rebuild, warm nudge baselines with preserveRecentMessages:20); verified they fail with the projection fix reverted --- .../DESIGN.md | 199 +++++ .../REQ.md | 108 +++ .../WORKLOG.md | 152 ++++ lib/v2/projection/acp-failure.ts | 76 ++ lib/v2/projection/shared.ts | 19 + lib/v2/tools.ts | 16 +- scripts/e2e/fake-llm-server.ts | 6 + scripts/e2e/installed-v2.ts | 12 + tests/v2-failed-tool-recognition.test.ts | 689 ++++++++++++++++++ 9 files changed, 1268 insertions(+), 9 deletions(-) create mode 100644 devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md create mode 100644 devlog/2026-09-17_v2-failed-tool-recognition/REQ.md create mode 100644 devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md create mode 100644 lib/v2/projection/acp-failure.ts create mode 100644 tests/v2-failed-tool-recognition.test.ts diff --git a/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md b/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md new file mode 100644 index 00000000..b7219b59 --- /dev/null +++ b/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md @@ -0,0 +1,199 @@ +# DESIGN - Recognize Failed V2 ACP Tool Results as Failures + +- Task ID: `2026-09-17_v2-failed-tool-recognition` +- References: https://github.com/ranxianglei/opencode-acp/issues/426 + +## 1. Context & Root Cause + +On OpenCode V2, ACP's five direct tools (`compress`, `decompress`, +`search_context`, `acp_status`, `acp_context_recap`) execute through the +pinned Promise adapter (`@opencode/plugin@2.0.3`, +`node_modules/@opencode/plugin/dist/promise/tool.d.ts`): + +```ts +execute(input: Input, context: ToolContext): Promise> +``` + +- The native Effect-based `Tool.Info` CAN return a typed error + (`Effect`), but the adapter lifts Promise tools via + `Effect.promise(() => tool.execute(...))`, so a rejection becomes an untyped + defect — structured metadata is lost and the turn risks failing outright. +- Therefore `createV2Tool` returns **resolved** results with error text in + `content` for every caught failure (permission ask/deny, lifecycle deny, + subagent deny, invalid input, execution exceptions). +- The host persists resolved results as tool parts with + `state.status === "completed"`. ACP's internal projection + (`lib/v2/projection/shared.ts -> toolState`) mapped that verbatim to an + internal `completed` state, so every downstream consumer — which all treat + `completed` as success — was wrong about failures: + - `messageHasCompress` (`lib/messages/query.ts`) → nudge success-baseline + advancement in `lib/messages/inject/inject.ts`; + - `collectCompressInvocations` (`lib/state/rebuild.ts`) → cold-start block + reconstruction; + - `hideFailedCompressCalls` (`lib/compress/hide-failed.ts`) → never hides + the failed call (it only matches `status === "error"`). + +The fake-provider classifier used by installed E2E +(`scripts/e2e/fake-llm-server.ts -> inspectToolResults`) had the same blind +spot in its text regex (`ACP .* execution failed` does not match +`ACP compress failed:`), so E2E observations also reported `completed`. + +## 2. Design Decisions + +### D1 — Explicit failure metadata at the source (additive) + +`errorResult()` in `lib/v2/tools.ts` is the single choke point through which +all resolved error results flow. It now merges +`acpFailed: true` into the result metadata, keeping every existing per-reason +flag (`acpError`, `acpPermission`, `acpDisabled`, `acpSubAgent`) intact: + +```ts +return { content: message, metadata: { [ACP_FAILURE_METADATA_KEY]: true, ...metadata } } +``` + +Additive, never replacing: existing consumers of the specific flags are +unaffected, and any future error path added via `errorResult()` is covered +automatically. + +### D2 — Recognition at projection time, not in the shared transform + +The patcher (`lib/v2/projection/patch.ts`) rejects any shared transform that +changes a tool part's status relative to the original projection +(`ambiguous-origin` "changed state"). Hence the reclassification must happen +where the projection is built — `toolState()` — so both sides of every later +comparison already agree on `error`. + +In `toolState()`, when host status is `completed` and the tool is one of the +five ACP tools: + +```ts +if (isAcpFailedToolOutput(stringValue(tool.name), rawState, normalizedOutput)) { + return { + state: { + status: "error", + input, + error: normalizedOutput, + ...(metadata ? { metadata } : {}), + }, + opaqueResult: output === undefined, + error: normalizedOutput, // sets origin.normalizedError only + } +} +``` + +Deliberately **no `output` field** on the returned record: +`normalize.ts` only sets `origin.normalizedOutput` when one exists, so the +origin keeps no output fingerprint of the provider-owned text, and the patcher +skips output comparison entirely (`loweredToolCorrelationIsExact` treats an +undefined origin output as "correlation by identity"). The lowered/host result +is therefore never rewritten — the issue's hard constraint. + +### D3 — Two-tier recognition, metadata authoritative + +`isAcpFailedToolOutput(toolName, rawState, neutralizedOutput)`: + +1. `toolName` must be one of the five ACP names (allowlist — non-ACP tools + with error-looking or even quoted ACP text are untouched, including + metadata-carrying bash outputs). +2. Explicit `metadata.acpFailed === true` → failed (authoritative, covers any + current/future message shape). +3. Else the anchored pattern against the normalized output covers + **compatible historical failure output** from pre-fix host history. + +`ACP_FAILURE_OUTPUT_PATTERN` is anchored at position zero and intentionally +lacks the multiline flag: + +``` +/^(?:ACP (?:[a-z][a-z_]* failed|is shutting down|is currently disabled|direct tools are disabled|could not verify the session parent|could not resolve the active agent permission|tool execution is disabled|cannot request an interactive permission)|Invalid [a-z][a-z_]* input:)/ +``` + +Coverage: `ACP failed: …` (catch-all + quality-gate rejections, which +arrive wrapped in the catch-all form), all four deny/lifecycle constants, the +bili-proxy disable notice, and `Invalid input: …`. Verified non-matches: +every known success prefix (`Compressed N messages into …`, +`[Compressed conversation section]…`, `No active compression blocks.`, +`No matches found …`, recap restore text, acp_status usage lines) and any +failure text appearing after the first line (quoted history inside restored +or searched content). + +### D4 — Shared pure module as the single definition + +`lib/v2/projection/acp-failure.ts` is dependency-free and exports the name +list, metadata key, pattern, and predicates. Consumers: +`lib/v2/tools.ts` (re-exports the name list, preserving the existing public +import path used by `tests/v2-lifecycle.test.ts`), +`lib/v2/projection/shared.ts`, `scripts/e2e/fake-llm-server.ts`, and the unit +tests. One definition ⇒ the classifier and the projection cannot drift apart. + +### D5 — Nudges: no code change needed + +`messageHasCompressAttempt` is status-agnostic (any status counts as an +attempt) while `messageHasCompress` requires `completed`. Once the projection +emits `error` for failures: + +- failed attempts still enter the attempt branch → pending-nudge anchors and + `lastNudgeShownTokens` reset (the #216 feedback-loop protection is retained); +- the success sub-check (`messageHasCompress`) no longer fires → + `lastPerMessageNudgeTokens` / `compressBaselineSet` are NOT advanced; +- `hideFailedCompressCalls` now works on V2 as on V1 (hides all but the most + recent failed call so the model can retry). + +This aligns V2 with V1 semantics exactly, where failures already arrive with +`status: "error"`. + +### D6 — E2E observations require genuine success + +- `inspectToolResults`: ACP-named results are classified with the shared + anchored pattern first; the legacy loose regex remains for non-ACP tools + (bash stderr `error:`, etc.). +- `installed-v2.ts` nudge-growth stage: beyond asserting a compress observation + EXISTS, it now asserts its status is `"completed"` — a failed compression + can no longer satisfy a success-expecting scenario. + +## 3. Data Flow (after fix) + +``` +host history (V2) internal projection downstream +───────────────────── ─────────────────── ────────── +tool part completed toolState(): + content: "ACP compress failed…" name ∈ ACP list + metadata: {acpFailed:true} ──► metadata.acpFailed → status:"error" ─► nudge attempt branch + or (no output exposed) rebuild: skipped + pre-fix history: ▲ hide-failed: active + no metadata, text match ──► pattern fallback messageHasCompress: false +``` + +## 4. Verified Invariants (why this is safe) + +1. **Status-change rejection**: `patch.ts` rejects transforms that change a + tool part's status vs the original projection. Reclassification happens at + projection time ⇒ both sides identical ⇒ no `ambiguous-origin` rejection. +2. **Provider-owned output untouched**: reclassified branch exposes no + `output` ⇒ `origin.normalizedOutput === undefined` ⇒ output comparison + skipped in correlation and patching; error text is identical on both sides + ⇒ the error-text change check cannot fire. Outgoing messages byte-identical. +3. **Removal path independent of status**: `hideFailedCompressCalls` removals + operate on call/result pointers in the outgoing build, unaffected by the + internal status value. +4. **Consumers tolerate missing output on error parts**: every + `part.state.output` reader in `lib/` is guarded by `status === "completed"` + or a typeof check (enforce-budget, truncate-tools, utils, rebuild, + protected-content, quality-gate, token-utils, decompress logic). +5. **No persisted-format impact**: `fingerprintMessage` (fork matching) is + computed on the fly from current messages, never persisted; block state + files are unchanged in shape. + +## 5. Alternatives Considered + +- **Rewrite host/provider output to an error shape** — rejected: violates the + issue constraint, breaks exact correlation with the lowered session, and the + patcher would reject it anyway (invariant 1). +- **Reject the promise (native error channel)** — rejected for the pinned + adapter: rejection becomes an untyped Effect defect (metadata loss, turn + failure risk). Revisit if the pin moves (follow-up in WORKLOG §7). +- **Detect failures in each downstream consumer** — rejected: scatters the + rule across query/rebuild/nudge/hide modules; a single projection-time + recognition point gives V1-equivalent semantics everywhere at once. +- **Looser (multiline, case-insensitive) pattern** — rejected: risk of + demoting genuine success outputs that quote historical failure text + (decompress/recap/search_context restore exactly such content). diff --git a/devlog/2026-09-17_v2-failed-tool-recognition/REQ.md b/devlog/2026-09-17_v2-failed-tool-recognition/REQ.md new file mode 100644 index 00000000..9ff28868 --- /dev/null +++ b/devlog/2026-09-17_v2-failed-tool-recognition/REQ.md @@ -0,0 +1,108 @@ +# REQ - Recognize Failed V2 ACP Tool Results as Failures + +- Task ID: `2026-09-17_v2-failed-tool-recognition` +- Home Repo: `opencode-acp` +- Created: 2026-09-17 +- Status: InProgress +- Priority: P1 +- References: https://github.com/ranxianglei/opencode-acp/issues/426 + +## 1. Background & Problem Statement + +- **Context**: On OpenCode V2 (pinned `@opencode/plugin` `2.0.3`), ACP's direct + tools run through the Promise tool adapter (`@opencode/plugin/promise/tool`). + That adapter's `execute` signature is `(input, context) => Promise` + and has **no safe native error-return channel**: rejecting the promise turns a + structured failure into an untyped Effect defect (losing metadata and risking + turn-level failure), so `createV2Tool` returns _resolved_ results with error + text in `content` for every caught execution failure. +- **Current behavior**: The host records those resolved error results as tool + parts with `state.status === "completed"` even when the content says + `ACP compress failed: ...`. ACP's internal V2 projection + (`lib/v2/projection/shared.ts -> toolState`) preserves that `completed` + status without inspecting ACP failure metadata or failure text. Downstream: + - `messageHasCompress` (`lib/messages/query.ts`) treats the failed call as a + successful compression; + - nudge baseline logic (`lib/messages/inject/inject.ts`) advances + `lastPerMessageNudgeTokens` / sets `compressBaselineSet` after a _failed_ + compression; + - cold-start reconstruction (`lib/state/rebuild.ts -> +collectCompressInvocations`) replays the failed call and rebuilds a block + that never existed; + - `hideFailedCompressCalls` never hides the failed call because its status is + not `error`. +- **Also affected**: the installed E2E fake provider + (`scripts/e2e/fake-llm-server.ts -> inspectToolResults`) classifies tool + result status by text pattern; its regex misses `ACP failed:` (it only + matches `ACP .* execution failed`) plus several other deny/lifecycle texts, + so E2E observations report `completed` for genuinely failed ACP calls. +- **Expected behavior**: + - All resolved error results from `createV2Tool` carry explicit ACP failure + metadata (`acpFailed: true`) alongside their existing specific flags. + - The internal V2 projection recognizes explicit failure metadata — and + compatible historical failure output from pre-fix host history — as an + internally _failed_ tool (status `error`) **without rewriting the actual + provider-owned outgoing output**. + - Failed-attempt handling for nudges is retained (a failed attempt still + resets pending-nudge state) but must NOT advance success baselines. + - Cold/warm failed compression does not reconstruct a block. + - Successful ACP tool outputs remain recognized as completed. + - E2E observations require genuine compression success where success is + expected. + - V1 behavior is unchanged. + +## 2. Reproduction + +- **Environment**: OpenCode V2 `2.0.3`, Linux, local ACP checkout at fork tip + `1fe36e09`. +- **Minimal reproduction**: + 1. Run a V2 session with ACP enabled and trigger any caught ACP tool + failure (e.g. a `compress` call whose pipeline throws, producing + `ACP compress failed: ...`). + 2. Inspect host session history: the tool part has + `state.status === "completed"` with the error text in `content`. + 3. Observe ACP internals treating it as success: nudge baseline advanced, + and on restart `collectCompressInvocations` replays the part and + reconstructs a phantom block. + 4. In installed E2E, `inspectToolResults` reports the same result as + `completed`, so scenario assertions pass despite the failure. +- **Relevant configuration**: V2 server plugin loaded through the normal + `opencode.json` `plugins` array; pinned `@opencode/plugin` `2.0.3`. + +## 3. Constraints & Non-Goals + +- **Constraints**: + - Do NOT rewrite provider-owned outgoing/host message content — recognition + happens in the internal projection only. + - Keep existing per-reason metadata flags (`acpError`, `acpPermission`, + `acpDisabled`, `acpSubAgent`) intact for compatibility; add, don't replace. + - No new runtime dependencies. Pure-function module for the shared + recognition logic so unit tests and the E2E fake provider can reuse it. + - Do NOT change `version` in package.json (non-release branch). + - Internal `dcp` naming / persisted state format compatibility preserved. +- **Non-goals**: + - Changing the pinned `@opencode/plugin` adapter to use the native Effect + error channel (out of scope; revisit if the pin moves). + - Changing V1 tool result handling at all. + - Adding new issue entries for this work (issue #426 is the ticket). + +## 4. Acceptance Criteria + +1. Every resolved error result produced by `createV2Tool` includes + `acpFailed: true` in its metadata (verified by unit test over the helper). +2. `toolState` in `lib/v2/projection/shared.ts` maps a `completed` host state + for one of the five ACP tools to an internal `status: "error"` state when + either explicit failure metadata is present OR the normalized output starts + with a known ACP failure prefix (historical compat). Successful ACP outputs + stay `completed`; non-ACP tools are untouched. +3. Unit tests prove: failed cold compression does not advance + `lastPerMessageNudgeTokens` / `compressBaselineSet` and does not rebuild a + block; failed warm compression likewise; failed attempts still clear pending + nudge anchors; successful compressions are recognized exactly as before. +4. `scripts/e2e/fake-llm-server.ts` classifies all ACP failure texts (explicit + `ACP failed:`, lifecycle/deny/subagent/proxy messages) as `error`. +5. Installed E2E driver asserts genuine `completed` status for the + nudge-growth compress results (success required, not just observed). +6. `npm run typecheck`, `npm run test`, `npm run build` all pass. +7. Devlog REQ/WORKLOG (+DESIGN, since projection data flow changes) committed + with the code. diff --git a/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md b/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md new file mode 100644 index 00000000..144fc1d0 --- /dev/null +++ b/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md @@ -0,0 +1,152 @@ +# WORKLOG - Recognize Failed V2 ACP Tool Results as Failures + +- Task ID: `2026-09-17_v2-failed-tool-recognition` +- Home Repo: `opencode-acp` +- Status: Done +- Updated: 2026-09-17 + +## 1. Summary + +- **What was done**: `createV2Tool` resolved error results now carry explicit + `acpFailed: true` metadata; the internal V2 projection (`toolState`) maps a + host-`completed` result for one of the five ACP tools to an internal + `status: "error"` state when that metadata is present or the output matches + the anchored ACP failure-text pattern (historical compat) — without exposing + any provider-owned `output`, so outgoing messages are never rewritten. The + installed E2E fake provider reuses the same pattern for its classifier, and + the nudge-growth driver now asserts genuine `completed` status for compress + results. +- **Why**: The pinned `@opencode/plugin@2.0.3` Promise adapter has no safe + native error channel, so caught failures were recorded by the host as + `completed` and downstream logic (nudge success baselines, cold-start block + reconstruction, `messageHasCompress`) treated failures as successes. +- **Behavior / compatibility changes**: Yes — internal V2 projection only. + Failed V2 ACP tool results are now visible to ACP internals as failed tools + (same shape V1 already produces for real errors). Host/provider-owned output + is byte-identical before and after. Successful ACP outputs, non-ACP tools, + native host error states, and all V1 paths are unchanged. Existing per-reason + metadata flags are preserved (`acpFailed` is additive). +- **Risk level**: Low — recognition is confined to the projection boundary; + patcher invariants (status-change rejection, output correlation) verified in + DESIGN.md §4. + +## 2. Change Log + +### Commits + +| Commit | Description | +| ------- | ------------------------------------------------------------ | +| `` | fix: recognize failed V2 ACP tool results as failures (#426) | + +### Key Files + +- `lib/v2/projection/acp-failure.ts` — NEW pure module: canonical ACP tool-name + list, `ACP_FAILURE_METADATA_KEY`, anchored `ACP_FAILURE_OUTPUT_PATTERN`, + `isAcpToolName`, `isAcpFailedToolOutput`. Dependency-free so the unit tests, + the projection, and the E2E fake provider all share one definition. +- `lib/v2/tools.ts` — imports/re-exports `V2_ACP_TOOL_NAMES` from the new + module; `errorResult()` merges `acpFailed: true` into every resolved error + result's metadata (single choke point covering all current and future error + paths). +- `lib/v2/projection/shared.ts` — `toolState()` completed branch: recognized + ACP failure → internal `{status:"error", input, error, metadata}` with NO + `output` field (load-bearing: keeps origin correlation exact and prevents + the patcher from touching provider-owned output). +- `scripts/e2e/fake-llm-server.ts` — `inspectToolResults` classifies ACP-named + tool results with the shared anchored pattern; legacy regex retained for + non-ACP tools. +- `scripts/e2e/installed-v2.ts` — nudge-growth stage asserts the observed + compress result status is genuinely `"completed"` after each nudge. +- `tests/v2-failed-tool-recognition.test.ts` — NEW: 18 tests across four + sections (pure module, projection, cold rebuild, warm nudge baselines). +- `devlog/2026-09-17_v2-failed-tool-recognition/{REQ,WORKLOG,DESIGN}.md`. + +## 3. Design & Implementation Notes + +See `DESIGN.md` for the full data-flow analysis and invariants. Key points: + +- Recognition happens at **projection time** (host history → internal parts), + not in the shared transform: the patcher rejects transforms that change a + tool part's status relative to the original projection, so both sides of the + comparison must already agree on `error`. +- Explicit metadata is authoritative; the text pattern is only a fallback for + pre-fix host history ("compatible historical failure output"). +- The pattern is anchored at position zero with no multiline flag, so quoted + historical failure text inside a larger success output can never + misclassify it. + +## 4. Testing & Verification + +### Build & Test Commands + +```sh +npm run typecheck +node --import tsx --test tests/*.test.ts +npm run build +``` + +### Test Coverage + +- New test file: `tests/v2-failed-tool-recognition.test.ts` (18 tests): + - acp-failure module: name list, all 9 failure shapes match at position 0, + mid-output occurrences rejected, success outputs rejected, metadata + precedence, non-ACP exclusion. + - Projection: metadata-flagged → error; historical-text-only → error; + successful ACP → completed; non-ACP error-looking text untouched; + multi-line quoted failure stays completed; native error/running states + unchanged; reclassified state exposes no `output`. + - Cold rebuild: failed compress part → 0 blocks rebuilt; mixed failed + + successful → exactly 1 block from the successful call. + - Warm nudges (multi-turn, shared state, `preserveRecentMessages: 20` per + AGENTS.md §5.7.1): failed attempt clears turn/iteration anchors and + `lastNudgeShownTokens`, leaves `lastPerMessageNudgeTokens` unchanged and + `compressBaselineSet` false; control proves a successful compress advances + the baseline and sets the flag. +- Bug-detection check (AGENTS.md §5.7.3): with the `shared.ts` change reverted, + the two recognition pinning tests fail; restored → all pass. +- Full suite: 1429 tests, 1428 pass, 1 fail — `tests/soft-block.test.ts` + crashes at import in this sandbox because it hardcodes `mkdirSync('/tmp/...')` + and `/tmp` is read-only here (environmental; passes in CI with a writable + `/tmp`). Unrelated to this change. +- Typecheck: pass. Build: pass. Format: pass. + +### Results + +- **PASS** (modulo the environmental soft-block sandbox failure above). + +## 5. Risk Assessment & Rollback + +- **Risk points**: + - False positives would demote successful ACP calls to errors. Mitigated by + position-zero anchoring, the explicit allowlist of five tool names, and + tests asserting every known success prefix stays completed. + - Patcher correlation breakage. Mitigated: no `output` exposed on the + reclassified branch ⇒ `origin.normalizedOutput` stays undefined ⇒ output + comparison skipped; error text identical on both sides ⇒ no rejection. +- **Rollback method**: + - Revert commit(s): `` + - Rollback impact: restores pre-fix behavior (failures treated as completed); + no persisted-state migration involved, so rollback is clean. +- **Compatibility notes** (data format, config schema): No persisted format or + config changes. `acpFailed` metadata appears in host-recorded tool state for + new sessions only; old history is covered by the text fallback. + +## 6. Lessons Learned + +- The Promise adapter's missing error channel makes "resolved but failed" a + first-class input shape for the V2 surface — any future V2 tool must return + through `errorResult()` (or otherwise set `acpFailed`) or it will be + misread as success. +- Nudge unit tests need three harness details that are easy to get wrong: + `state.modelContextLimit` set, an established baseline + (`lastPerMessageNudgeTokens = 0`), and handler invocations ending on a user + message (the pipeline runs before the assistant responds). +- `getCurrentTokenUsage` derives usage from the last assistant + `info.tokens` (Bug 17), not raw content — fixtures must carry realistic token + info or nudges silently never fire. + +## 7. Follow-ups + +- [ ] If the `@opencode/plugin` pin moves, revisit using the native Effect + error channel (`Effect`) instead of resolved + error-results (non-goal of this iteration). diff --git a/lib/v2/projection/acp-failure.ts b/lib/v2/projection/acp-failure.ts new file mode 100644 index 00000000..b5c905e4 --- /dev/null +++ b/lib/v2/projection/acp-failure.ts @@ -0,0 +1,76 @@ +/** + * Shared recognition of failed ACP tool results on OpenCode V2. + * + * The pinned Promise tool adapter (`@opencode/plugin@2.0.3`) has no safe + * native error-return channel: a rejected promise becomes an untyped Effect + * defect, losing structured metadata and risking turn-level failure. + * `createV2Tool` therefore returns RESOLVED results with the error text in + * `content`, and the host records those tool parts with + * `state.status === "completed"`. + * + * This module centralizes failure recognition so the internal V2 projection + * can report such tools as failed WITHOUT rewriting provider-owned output: + * - explicit failure metadata (`acpFailed: true`) written by + * `createV2Tool`'s `errorResult` is authoritative; + * - the anchored output pattern recognizes compatible historical records + * created before that metadata existed. + * + * The module is intentionally dependency-free so unit tests, the projection, + * and the installed E2E fake provider can all reuse the same recognition. + */ + +export const V2_ACP_TOOL_NAMES = [ + "compress", + "decompress", + "search_context", + "acp_status", + "acp_context_recap", +] as const + +export type V2AcpToolName = (typeof V2_ACP_TOOL_NAMES)[number] + +/** Metadata key marking a resolved ACP error result as a failure. */ +export const ACP_FAILURE_METADATA_KEY = "acpFailed" as const + +/** + * Position-0 anchored prefixes of every resolved ACP error result produced + * by `createV2Tool`: + * - `ACP failed: ...` (execution catch-all) + * - `ACP is shutting down; ...` (lifecycle deny) + * - `ACP is currently disabled because a /bili/ proxy is active.` + * - `ACP direct tools are disabled for child sessions ...` + * - `ACP could not verify the session parent; ...` + * - `ACP could not resolve the active agent permission; ...` + * - `ACP tool execution is disabled by the active agent or ACP configuration...` + * - `ACP cannot request an interactive permission on OpenCode V2.0.3. ...` + * - `Invalid input: ...` + * + * Deliberately NOT multiline-anchored: quoted historical failure text inside + * a larger successful output must never match. + */ +export const ACP_FAILURE_OUTPUT_PATTERN = + /^(?:ACP (?:[a-z][a-z_]* failed|is shutting down|is currently disabled|direct tools are disabled|could not verify the session parent|could not resolve the active agent permission|tool execution is disabled|cannot request an interactive permission)|Invalid [a-z][a-z_]* input:)/ + +export function isAcpToolName(name: string | undefined): name is V2AcpToolName { + return typeof name === "string" && (V2_ACP_TOOL_NAMES as readonly string[]).includes(name) +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value) +} + +/** + * Decide whether a host-recorded completed ACP tool result is actually a + * failure. Explicit failure metadata wins; otherwise fall back to the + * historical output-text pattern. Non-ACP tools never match. + */ +export function isAcpFailedToolOutput( + toolName: string | undefined, + rawState: Record, + neutralizedOutput: string, +): boolean { + if (!isAcpToolName(toolName)) return false + if (isRecord(rawState.metadata) && rawState.metadata[ACP_FAILURE_METADATA_KEY] === true) + return true + return ACP_FAILURE_OUTPUT_PATTERN.test(neutralizedOutput) +} diff --git a/lib/v2/projection/shared.ts b/lib/v2/projection/shared.ts index 7c8b5b1e..7056bb6a 100644 --- a/lib/v2/projection/shared.ts +++ b/lib/v2/projection/shared.ts @@ -14,6 +14,7 @@ import type { V2ProjectionOptions, } from "./types" import { isAcpOwnedId, isAcpOwnedNoticeId, isAcpSyntheticId } from "../../synthetic-ids" +import { isAcpFailedToolOutput } from "./acp-failure" export function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value) @@ -516,6 +517,24 @@ export function toolState(tool: Record): { const normalizedOutput = output ?? content.map((item) => (isRecord(item) ? (stringValue(item.text) ?? "") : "")).join("\n") + if (isAcpFailedToolOutput(stringValue(tool.name), rawState, normalizedOutput)) { + // The pinned V2 Promise adapter has no safe native error channel, + // so createV2Tool returns resolved results for caught failures and + // the host records them as completed. Project them internally as + // failed tools without rewriting provider-owned output: no + // `output` field is exposed, so origin correlation and patching + // leave the lowered result untouched. + return { + state: { + status: "error", + input, + error: normalizedOutput, + ...(isRecord(rawState.metadata) ? { metadata: rawState.metadata } : {}), + }, + opaqueResult: output === undefined, + error: normalizedOutput, + } + } return { state: { status: "completed", diff --git a/lib/v2/tools.ts b/lib/v2/tools.ts index 0ace89cf..5ad2da46 100644 --- a/lib/v2/tools.ts +++ b/lib/v2/tools.ts @@ -23,18 +23,13 @@ import { } from "../host-permissions" import type { V2HostAdapter } from "./host" import type { V2OperationTracker } from "./lifecycle" +import { ACP_FAILURE_METADATA_KEY, V2_ACP_TOOL_NAMES } from "./projection/acp-failure" + +export { V2_ACP_TOOL_NAMES } type V2Context = Parameters[0] export type V2ToolEditor = Parameters[0]>[0] -export const V2_ACP_TOOL_NAMES = [ - "compress", - "decompress", - "search_context", - "acp_status", - "acp_context_recap", -] as const - type V2ToolContent = Exclude, string>[number] const PERMISSION_ASK_MESSAGE = @@ -49,8 +44,11 @@ function errorMessage(error: unknown): string { return error instanceof Error ? error.message : String(error) } +// The pinned Promise adapter has no safe native error channel, so resolved +// results are the only failure channel the host records reliably. Mark every +// one explicitly so the internal projection can recognize it as a failure. function errorResult(message: string, metadata: Record): V2ToolResult { - return { content: message, metadata } + return { content: message, metadata: { [ACP_FAILURE_METADATA_KEY]: true, ...metadata } } } function resultMetadata( diff --git a/scripts/e2e/fake-llm-server.ts b/scripts/e2e/fake-llm-server.ts index fe3392a3..713b0841 100644 --- a/scripts/e2e/fake-llm-server.ts +++ b/scripts/e2e/fake-llm-server.ts @@ -21,6 +21,7 @@ */ import { readFileSync, writeFileSync, existsSync } from "fs" +import { ACP_FAILURE_OUTPUT_PATTERN, isAcpToolName } from "../../lib/v2/projection/acp-failure" declare const Bun: { serve(options: { @@ -751,7 +752,12 @@ function inspectToolResults(messages: any[]): ToolResultObservation[] { const text = extractMessageText(message) const id = typeof message?.tool_call_id === "string" ? message.tool_call_id : undefined const name = normalizeToolName(message?.name) ?? (id ? namesByCallId.get(id) : undefined) + // ACP tool failures are resolved results on V2 (no native error + // channel), so recognize them through the same anchored pattern the + // internal projection uses; the legacy regex covers other tools. + const acpFailure = isAcpToolName(name) && ACP_FAILURE_OUTPUT_PATTERN.test(text) const status = + acpFailure || /(?:^|\n)\s*(?:error:|ACP cannot request|ACP tool execution is disabled|ACP .* execution failed|permission .* blocked|invalid .* input|COMPRESSION REJECTED|QUALITY GATE FAILURE)/i.test( text, ) diff --git a/scripts/e2e/installed-v2.ts b/scripts/e2e/installed-v2.ts index 1c0e96bc..ae69f3e3 100755 --- a/scripts/e2e/installed-v2.ts +++ b/scripts/e2e/installed-v2.ts @@ -953,6 +953,12 @@ async function stageNudgeGrowth(): Promise { "first nudge has a real compress result observation", firstPostObservation.toolResultStatuses?.some((tool) => tool.name === "compress") === true, ) + // A failed resolved V2 tool result must not count as compression success. + record( + "first nudge compress result is a genuine completed success", + firstPostObservation.toolResultStatuses?.find((tool) => tool.name === "compress") + ?.status === "completed", + ) const firstCompressed = await waitForState( sessionID, "first nudge compression commit", @@ -1034,6 +1040,12 @@ async function stageNudgeGrowth(): Promise { secondNudgeObservation, ) if (!secondPostObservation) throw new Error("second nudge produced no post-tool observation") + // A failed resolved V2 tool result must not count as compression success. + record( + "second nudge compress result is a genuine completed success", + secondPostObservation.toolResultStatuses?.find((tool) => tool.name === "compress") + ?.status === "completed", + ) const secondCompressed = await waitForState( sessionID, "second nudge compression commit", diff --git a/tests/v2-failed-tool-recognition.test.ts b/tests/v2-failed-tool-recognition.test.ts new file mode 100644 index 00000000..ba173c4e --- /dev/null +++ b/tests/v2-failed-tool-recognition.test.ts @@ -0,0 +1,689 @@ +/** + * Issue #426 — failed V2 ACP tool results must be recognized as failures. + * + * The pinned V2 Promise adapter has no safe native error channel, so + * createV2Tool returns resolved results for caught execution failures and + * the host records them with status "completed". These tests pin the fix: + * - explicit `acpFailed` metadata (or compatible historical failure output) + * is projected internally as a failed tool without touching provider-owned + * output; + * - cold rebuild does not reconstruct blocks from failed compress calls; + * - warm nudge handling treats failed attempts as attempts (anchors reset) + * but never advances success baselines; successful compress still does. + */ + +import assert from "node:assert/strict" +import { describe, it } from "node:test" +import { mkdtempSync } from "node:fs" +import { tmpdir } from "node:os" +import { join } from "node:path" +import "./test-env" +import { + ACP_FAILURE_METADATA_KEY, + ACP_FAILURE_OUTPUT_PATTERN, + V2_ACP_TOOL_NAMES, + isAcpFailedToolOutput, + isAcpToolName, +} from "../lib/v2/projection/acp-failure" +import { toolState } from "../lib/v2/projection/shared" +import { rebuildCompressionState } from "../lib/state/rebuild" +import { createSessionState } from "../lib/state/state" +import type { SessionState } from "../lib/state/types" +import { Logger } from "../lib/logger" +import type { PluginConfig } from "../lib/config" +import { createChatMessageTransformHandler } from "../lib/hooks" +import { createTestRegistry } from "./registry-stub" +import type { WithParts } from "@opencode-ai/plugin" + +// ─── (a) acp-failure pure module ──────────────────────────────────────────── + +const FAILURE_MESSAGES = [ + "ACP cannot request an interactive permission on OpenCode V2.0.3. Set the active agent's `compress` permission to `allow` or `deny`, then retry.", + "ACP tool execution is disabled by the active agent or ACP configuration. Choose `allow` for the `compress` permission to enable it.", + "ACP direct tools are disabled for child sessions when `allowSubAgents` is false.", + "ACP could not verify the session parent; direct tool execution was blocked.", + "ACP is shutting down; this operation was not executed.", + "ACP is currently disabled because a /bili/ proxy is active.", + "ACP could not resolve the active agent permission; execution was blocked.", + "Invalid compress input: content must have at least one entry", + "ACP compress failed: simulated execution failure", + "ACP decompress failed: block b7 not found", +] + +const SUCCESS_MESSAGES = [ + "Compressed 4 messages into [Compressed conversation section].", + "[Compressed conversation section]\n\nSummary of m00001-m00004.", + "No active compression blocks.", + 'No matches found for query "decoder".', + "Restored 3 messages from b5.", + "Context usage 42% — 84k / 200k tokens.", +] + +describe("acp-failure module", () => { + it("exposes exactly the five ACP tool names", () => { + assert.deepEqual( + [...V2_ACP_TOOL_NAMES], + ["compress", "decompress", "search_context", "acp_status", "acp_context_recap"], + ) + }) + + it("matches every known failure shape at position zero", () => { + for (const message of FAILURE_MESSAGES) { + assert.ok( + ACP_FAILURE_OUTPUT_PATTERN.test(message), + `should match failure shape: ${message.slice(0, 40)}...`, + ) + } + }) + + it("never matches failure text appearing mid-output", () => { + const quoted = "Previous attempt said:\nACP compress failed: timeout\nRetrying now." + assert.ok(!ACP_FAILURE_OUTPUT_PATTERN.test(quoted)) + const recap = + "[Compressed conversation section]\n\nEarlier run noted ACP decompress failed: b3 missing." + assert.ok(!ACP_FAILURE_OUTPUT_PATTERN.test(recap)) + }) + + it("never matches genuine success outputs", () => { + for (const message of SUCCESS_MESSAGES) { + assert.ok(!ACP_FAILURE_OUTPUT_PATTERN.test(message), `must not match: ${message}`) + } + }) + + it("classifies tool names precisely", () => { + for (const name of V2_ACP_TOOL_NAMES) { + assert.equal(isAcpToolName(name), true) + } + assert.equal(isAcpToolName("bash"), false) + assert.equal(isAcpToolName("read"), false) + assert.equal(isAcpToolName(undefined), false) + }) + + it("treats explicit acpFailed metadata as authoritative", () => { + const rawState = { status: "completed", metadata: { [ACP_FAILURE_METADATA_KEY]: true } } + // Metadata wins even when the text looks like a success. + assert.equal( + isAcpFailedToolOutput( + "compress", + rawState, + "Compressed 4 messages into [Compressed conversation section].", + ), + true, + ) + // Metadata wins even with empty output. + assert.equal(isAcpFailedToolOutput("acp_status", { ...rawState }, ""), true) + // Absent metadata falls back to the pattern. + assert.equal( + isAcpFailedToolOutput("compress", { status: "completed" }, FAILURE_MESSAGES[8]), + true, + ) + assert.equal( + isAcpFailedToolOutput("compress", { status: "completed" }, SUCCESS_MESSAGES[0]), + false, + ) + // Explicit false metadata must not override... it simply means "not flagged"; + // recognition then depends on the output pattern. + assert.equal( + isAcpFailedToolOutput( + "compress", + { metadata: { [ACP_FAILURE_METADATA_KEY]: false } }, + SUCCESS_MESSAGES[0], + ), + false, + ) + }) + + it("ignores non-ACP tools regardless of content or metadata", () => { + assert.equal( + isAcpFailedToolOutput( + "bash", + { metadata: { [ACP_FAILURE_METADATA_KEY]: true } }, + FAILURE_MESSAGES[8], + ), + false, + ) + assert.equal(isAcpFailedToolOutput(undefined, {}, FAILURE_MESSAGES[8]), false) + }) +}) + +// ─── (b) projection: toolState recognizes failed ACP results ──────────────── + +function completedHostState(overrides: Record = {}) { + return { + status: "completed", + input: { topic: "work", content: [{ startId: "m00001", endId: "m00004", summary: "s" }] }, + title: "compress", + metadata: {}, + content: [ + { type: "text", text: "Compressed 4 messages into [Compressed conversation section]." }, + ], + time: { start: 1, end: 2 }, + ...overrides, + } +} + +describe("toolState projection of V2 ACP results", () => { + it("projects a completed result with acpFailed metadata as an internal error", () => { + const result = toolState({ + name: "compress", + state: completedHostState({ + metadata: { [ACP_FAILURE_METADATA_KEY]: true, acpError: "execution" }, + content: [ + { type: "text", text: "ACP compress failed: simulated execution failure" }, + ], + }), + }) + assert.equal(result.state.status, "error") + assert.equal(result.state.error, "ACP compress failed: simulated execution failure") + assert.deepEqual(result.state.input, { + topic: "work", + content: [{ startId: "m00001", endId: "m00004", summary: "s" }], + }) + assert.deepEqual(result.state.metadata, { + [ACP_FAILURE_METADATA_KEY]: true, + acpError: "execution", + }) + // No provider-owned output is exposed: origin correlation and patching + // must leave the lowered result untouched. + assert.equal(result.output, undefined) + assert.equal(result.error, "ACP compress failed: simulated execution failure") + assert.equal(result.opaqueResult, false) + }) + + it("recognizes historical failure output without metadata", () => { + for (const message of FAILURE_MESSAGES) { + const result = toolState({ + name: "compress", + state: completedHostState({ content: [{ type: "text", text: message }] }), + }) + assert.equal( + result.state.status, + "error", + `expected error for: ${message.slice(0, 40)}...`, + ) + assert.equal(result.output, undefined) + } + }) + + it("keeps successful ACP results completed with their output", () => { + for (const message of SUCCESS_MESSAGES) { + const result = toolState({ + name: "compress", + state: completedHostState({ content: [{ type: "text", text: message }] }), + }) + assert.equal(result.state.status, "completed", `expected completed for: ${message}`) + assert.equal(result.state.output, message) + assert.equal(result.error, undefined) + } + }) + + it("leaves non-ACP completed results untouched even with error-looking text", () => { + const result = toolState({ + name: "bash", + state: completedHostState({ + content: [{ type: "text", text: "ACP compress failed: quoted from log" }], + }), + }) + assert.equal(result.state.status, "completed") + assert.equal(result.state.output, "ACP compress failed: quoted from log") + assert.equal(result.error, undefined) + }) + + it("does not misclassify multi-line success output that quotes failure text later", () => { + const text = + "Restored 3 messages from b5.\nHistorical note: ACP decompress failed: b3 missing." + const result = toolState({ + name: "decompress", + state: completedHostState({ content: [{ type: "text", text }] }), + }) + assert.equal(result.state.status, "completed") + assert.equal(result.state.output, text) + }) + + it("preserves native host error states unchanged", () => { + const result = toolState({ + name: "compress", + state: { + status: "error", + input: {}, + error: { message: "native provider error" }, + }, + }) + assert.equal(result.state.status, "error") + assert.equal(result.state.error, "native provider error") + assert.equal(result.state.output, undefined) + }) + + it("preserves running states unchanged", () => { + const result = toolState({ + name: "compress", + state: { status: "running", input: {} }, + }) + assert.equal(result.state.status, "running") + }) +}) + +// ─── (c) cold rebuild skips failed compress calls ─────────────────────────── + +function buildRebuildConfig(): PluginConfig { + return { + enabled: true, + autoUpdate: true, + debug: false, + logLevel: "silent", + pruneNotification: "off", + pruneNotificationType: "chat", + commands: { enabled: true, protectedTools: [] }, + experimental: { allowSubAgents: false, customPrompts: false }, + protectedFilePatterns: [], + compress: { + mode: "range", + permission: "allow", + showCompression: false, + summaryBuffer: true, + candidates: false, + maxContextLimit: 150000, + minContextLimit: 50000, + minCompressRange: 0, + nudgeFrequency: 5, + iterationNudgeThreshold: 15, + nudgeForce: "soft", + protectedTools: ["task"], + protectTags: false, + protectUserMessages: false, + preserveRecentMessages: 0, + preserveRecentTokens: 0, + preserveLastUserMessage: false, + maxSummaryLengthHard: 4000, + maxVisibleSegments: 3, + }, + gc: { + algorithm: "truncate", + promotionThreshold: 5, + maxBlockAge: 15, + maxOldGenSummaryLength: 3000, + majorGcThresholdPercent: "100%", + batchCleanup: { lowThreshold: "60%", highThreshold: "75%", forceThreshold: "90%" }, + }, + qualityGate: { enabled: false, algorithm: "rouge-recall-v1", algorithms: {} }, + messageFilters: { enabled: false, filters: {} }, + } +} + +function rebuildUserMessage(id: string, sessionID: string): WithParts { + return { + info: { + id, + sessionID, + role: "user", + agent: "assistant", + time: { created: Date.now() }, + model: { providerID: "test-provider", modelID: "test-model" }, + } as WithParts["info"], + parts: [{ type: "text", text: `user ${id}`, id: `${id}-p1`, sessionID, messageID: id }], + } +} + +function rebuildAssistantMessage(id: string, sessionID: string, extraParts: any[]): WithParts { + return { + info: { + id, + sessionID, + role: "assistant", + agent: "assistant", + parentID: "parent-placeholder", + modelID: "test-model", + providerID: "test-provider", + mode: "normal", + path: { cwd: "/", root: "/" }, + summary: false, + cost: 0, + tokens: { input: 100, output: 50, reasoning: 0, cache: { read: 0, write: 0 } }, + time: { created: Date.now() }, + } as WithParts["info"], + parts: [ + { type: "step-start", id: `${id}-ss`, sessionID, messageID: id }, + { type: "text", text: `assistant ${id}`, id: `${id}-p1`, sessionID, messageID: id }, + ...extraParts, + ], + } +} + +// Post-projection internal shapes: a failed V2 compress arrives here as an +// error-status part (no output); a successful one as completed + output. +function failedCompressPart(callID: string, sessionID: string, messageId: string) { + return { + type: "tool", + tool: "compress", + callID, + id: `part-${callID}`, + sessionID, + messageID: messageId, + state: { + status: "error", + input: { + topic: "work", + content: [{ startId: "m00001", endId: "m00004", summary: "s" }], + }, + error: "ACP compress failed: simulated execution failure", + }, + } +} + +function successfulCompressPart(callID: string, sessionID: string, messageId: string) { + return { + type: "tool", + tool: "compress", + callID, + id: `part-${callID}`, + sessionID, + messageID: messageId, + state: { + status: "completed", + input: { + topic: "work", + content: [{ startId: "m00001", endId: "m00004", summary: "s" }], + }, + output: "Compressed 4 messages into [Compressed conversation section].", + }, + } +} + +describe("cold rebuild after failure", () => { + it("does not reconstruct a block from a failed compress call", () => { + const SID = "rebuild-fail-cold" + const config = buildRebuildConfig() + const logger = new Logger(false) + const state = createSessionState() + state.sessionId = SID + const messages: WithParts[] = [ + rebuildUserMessage("u1", SID), + rebuildAssistantMessage("a1", SID, [failedCompressPart("c-fail", SID, "a1")]), + rebuildUserMessage("u2", SID), + rebuildAssistantMessage("a2", SID, []), + ] + const blocks = rebuildCompressionState(state, messages, config, logger) + assert.equal(blocks, 0) + assert.equal(state.prune.messages.blocksById.size, 0) + }) + + it("reconstructs only the successful call when one failed and one succeeded", () => { + const SID = "rebuild-mixed-cold" + const config = buildRebuildConfig() + const logger = new Logger(false) + const state = createSessionState() + state.sessionId = SID + const messages: WithParts[] = [ + rebuildUserMessage("u1", SID), + rebuildAssistantMessage("a1", SID, [failedCompressPart("c-fail", SID, "a1")]), + rebuildUserMessage("u2", SID), + rebuildAssistantMessage("a2", SID, [successfulCompressPart("c-ok", SID, "a2")]), + rebuildUserMessage("u3", SID), + rebuildAssistantMessage("a3", SID, []), + ] + const blocks = rebuildCompressionState(state, messages, config, logger) + assert.equal(blocks, 1) + assert.equal(state.prune.messages.blocksById.size, 1) + }) +}) + +// ─── (d) warm nudge baselines across turns ────────────────────────────────── + +const NUDGE_SID = "v2-fail-nudge-warm" + +function buildNudgeConfig(overrides: Partial = {}): PluginConfig { + const base: PluginConfig = { + enabled: true, + autoUpdate: true, + debug: false, + logLevel: "silent", + pruneNotification: "off", + pruneNotificationType: "chat", + commands: { enabled: true, protectedTools: [] }, + experimental: { allowSubAgents: false, customPrompts: false }, + protectedFilePatterns: [], + compress: { + mode: "message", + permission: "allow", + showCompression: false, + summaryBuffer: true, + candidates: false, + maxContextLimit: 150000, + minContextLimit: 50000, + minCompressRange: 0, + nudgeFrequency: 5, + iterationNudgeThreshold: 15, + nudgeForce: "soft", + protectedTools: ["task"], + protectTags: false, + protectUserMessages: false, + // Production default per AGENTS.md §5.7.1 — the recent zone must + // actually protect messages for this scenario to mirror production. + preserveRecentMessages: 20, + preserveRecentTokens: 0, + preserveLastUserMessage: false, + maxSummaryLengthHard: 4000, + maxVisibleSegments: 3, + }, + gc: { + algorithm: "truncate", + promotionThreshold: 5, + maxBlockAge: 15, + maxOldGenSummaryLength: 3000, + majorGcThresholdPercent: "100%", + batchCleanup: { lowThreshold: "60%", highThreshold: "75%", forceThreshold: "90%" }, + }, + qualityGate: { enabled: false, algorithm: "rouge-recall-v1", algorithms: {} }, + messageFilters: { enabled: false, filters: {} }, + } + return { ...base, ...overrides } +} + +function nudgeUserMessage(id: string, text: string): WithParts { + return { + info: { + id, + sessionID: NUDGE_SID, + role: "user", + agent: "assistant", + time: { created: Date.now() }, + model: { providerID: "test-provider", modelID: "test-model" }, + } as WithParts["info"], + parts: [{ type: "text", text, id: `${id}-p1`, sessionID: NUDGE_SID, messageID: id }], + } +} + +function nudgeAssistantMessage( + id: string, + text: string, + extraParts: any[] = [], + tokenOverrides: { input?: number; output?: number } = {}, +): WithParts { + return { + info: { + id, + sessionID: NUDGE_SID, + role: "assistant", + agent: "assistant", + parentID: "parent-placeholder", + modelID: "test-model", + providerID: "test-provider", + mode: "normal", + path: { cwd: "/", root: "/" }, + summary: false, + cost: 0, + tokens: { + input: tokenOverrides.input ?? 100, + output: tokenOverrides.output ?? 50, + reasoning: 0, + cache: { read: 0, write: 0 }, + }, + time: { created: Date.now() }, + } as WithParts["info"], + parts: [ + { type: "step-start", id: `${id}-ss`, sessionID: NUDGE_SID, messageID: id }, + { type: "text", text, id: `${id}-p1`, sessionID: NUDGE_SID, messageID: id }, + ...extraParts, + ], + } +} + +function bashToolPart(messageId: string, callID: string, chars: number) { + return { + type: "tool", + tool: "bash", + callID, + id: `part-${callID}`, + sessionID: NUDGE_SID, + messageID: messageId, + state: { status: "completed", input: { command: "build" }, output: "x".repeat(chars) }, + } +} + +/** + * History deep enough that, with preserveRecentMessages: 20, more than 20 + * messages exist and the pre-recent-zone portion holds well over + * EFFECTIVE_MIN_COMPRESSIBLE_TOKENS (1250) of compressible content. The final + * assistant message carries realistic prompt sizes so current-token usage + * (derived from the last assistant info.tokens, Bug 17) sits past the min + * nudge limit. + */ +function buildHistory(pairs: number, charsPerOutput: number): WithParts[] { + const messages: WithParts[] = [] + for (let i = 0; i < pairs; i += 1) { + const uid = `h-u${i}` + const aid = `h-a${i}` + const isLast = i === pairs - 1 + messages.push(nudgeUserMessage(uid, `history user ${i}`)) + messages.push( + nudgeAssistantMessage( + aid, + `history assistant ${i}`, + [bashToolPart(aid, `h-c${i}`, charsPerOutput)], + isLast ? { input: 100000, output: 50000 } : {}, + ), + ) + } + return messages +} + +function setupNudgePipeline(stateOverrides: Partial = {}) { + const tempDir = mkdtempSync(join(tmpdir(), "acp-v2fail-nudge-")) + process.env.XDG_DATA_HOME = tempDir + process.env.XDG_CONFIG_HOME = tempDir + + const state = createSessionState() + state.sessionId = NUDGE_SID + // Matches the working e2e-blocks-nudges harness: the transform pipeline + // resolves limits against a known model window. + state.modelContextLimit = 200000 + Object.assign(state, stateOverrides) + + const config = buildNudgeConfig() + const logger = new Logger(false) + const handler = createChatMessageTransformHandler( + { session: { get: async () => ({ data: { parentID: null } }) } }, + createTestRegistry(state), + logger, + config, + { + reload() {}, + getRuntimePrompts() { + return { + system: "ACP system", + compressRange: "compress range", + compressMessage: "compress message", + contextLimitNudge: "nudge", + turnNudge: "turn nudge", + iterationNudge: "iteration nudge", + manualExtension: "", + subagentExtension: "", + } + }, + }, + { global: undefined, agents: {} }, + ) + return { state, handler, tempDir } +} + +async function runTurn( + handler: ReturnType["handler"], + messages: WithParts[], +) { + const output = { messages } + await handler({}, output) + return output.messages +} + +describe("warm nudge baseline behavior around compress attempts", () => { + it("failed compress resets pending-nudge state but never advances the success baseline", async () => { + const { state, handler } = setupNudgePipeline() + // An established baseline is required for the nudge path to fire + // (matches the e2e-blocks-nudges harness); mid-session it is always set. + state.nudges.lastPerMessageNudgeTokens = 0 + const history = buildHistory(26, 12000) + const u1 = nudgeUserMessage("u-turn1", "keep going") + const a1 = nudgeAssistantMessage("a-turn1", "working") + const u2 = nudgeUserMessage("u-turn2", "context is huge, please compress") + // Post-projection internal shape of a failed V2 compress: status + // "error", no output. + const a2 = nudgeAssistantMessage("a-turn2", "attempting compression", [ + failedCompressPart("c-fail-warm", NUDGE_SID, "a-turn2"), + ]) + + // LLM call 1: context has grown past the min limit → nudge fires. + await runTurn(handler, [...history, u1]) + assert.notEqual( + state.nudges.lastNudgeShownTokens, + undefined, + "precondition: nudge must fire before the attempt for the scenario to be meaningful", + ) + const baselineAfterNudge = state.nudges.lastPerMessageNudgeTokens + assert.equal(state.nudges.compressBaselineSet, false) + + // LLM call 2: the plain response lands, no compress yet. + await runTurn(handler, [...history, u1, a1]) + assert.equal(state.nudges.compressBaselineSet, false) + + // LLM call 3: the failed compress attempt is in the current turn. + await runTurn(handler, [...history, u1, a1, u2, a2]) + + // Failed-attempt handling preserved: pending-nudge state cleared so a + // later turn can nudge again. + assert.equal(state.nudges.turnNudgeAnchors.size, 0) + assert.equal(state.nudges.iterationNudgeAnchors.size, 0) + assert.equal(state.nudges.lastNudgeShownTokens, undefined) + assert.equal(state.nudges.shouldInjectThisTurn, false) + // Success baseline NOT advanced by the failure. + assert.equal(state.nudges.lastPerMessageNudgeTokens, baselineAfterNudge) + assert.equal(state.nudges.compressBaselineSet, false) + }) + + it("successful compress advances the success baseline (control)", async () => { + const { state, handler } = setupNudgePipeline() + // Same established-baseline precondition as the failure scenario above. + state.nudges.lastPerMessageNudgeTokens = 0 + const history = buildHistory(26, 12000) + const u1 = nudgeUserMessage("u-turn1", "keep going") + const a1 = nudgeAssistantMessage("a-turn1", "working") + const u2 = nudgeUserMessage("u-turn2", "context is huge, please compress") + const a2 = nudgeAssistantMessage("a-turn2", "compressing now", [ + successfulCompressPart("c-ok-warm", NUDGE_SID, "a-turn2"), + ]) + + await runTurn(handler, [...history, u1]) + assert.notEqual( + state.nudges.lastNudgeShownTokens, + undefined, + "precondition: nudge fires before the attempt", + ) + + await runTurn(handler, [...history, u1, a1]) + + await runTurn(handler, [...history, u1, a1, u2, a2]) + + assert.equal(state.nudges.shouldInjectThisTurn, false) + assert.equal(state.nudges.compressBaselineSet, true) + assert.notEqual(state.nudges.lastPerMessageNudgeTokens, undefined) + }) +}) From bc5f56cc05534fb17e1bbd3a62b809c82a4baacf Mon Sep 17 00:00:00 2001 From: ranxianglei Date: Thu, 17 Sep 2026 12:43:40 +0800 Subject: [PATCH 2/2] fix: address dual-review findings on V2 failed-tool recognition (#426) - rename misleading isAcpFailedToolOutput param neutralizedOutput -> outputText - strengthen warm-nudge control assertion: success baseline must advance past its seeded value, not merely become defined - add two full-projection integration tests driving host-shaped completed records through normalizeV2ProjectedHistory (internal part status error without output; origin.normalizedOutput stays undefined so the patcher can never rewrite provider-owned output) - document shared-tools soft-string failure limitation (DESIGN.md section 6) --- .../DESIGN.md | 24 +++ .../WORKLOG.md | 32 +++- lib/v2/projection/acp-failure.ts | 4 +- tests/v2-failed-tool-recognition.test.ts | 179 +++++++++++++++++- 4 files changed, 232 insertions(+), 7 deletions(-) diff --git a/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md b/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md index b7219b59..8f9c0f25 100644 --- a/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md +++ b/devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md @@ -197,3 +197,27 @@ tool part completed toolState(): - **Looser (multiline, case-insensitive) pattern** — rejected: risk of demoting genuine success outputs that quote historical failure text (decompress/recap/search_context restore exactly such content). + +## 6. Known Limitations + +The shared ACP tools report some soft failures as plain resolved text instead +of going through `errorResult`: + +- `decompress` returns `resolved.error` as ordinary output + (`lib/compress/decompress.ts:451`; error strings built around :230–:322, e.g. + `Error: No active compression blocks overlap the range …` at :322). +- `search_context` returns `"Error: query is required."` + (`lib/compress/search.ts:561`). +- `acp_context_recap` returns informational strings such as + `"No active compression blocks."` (`lib/compress/recap.ts:44`). +- `acp_status` appends `"(unable to fetch messages)"` on fetch failure + (`lib/compress/status.ts:701`). + +These stay `completed` on both runtimes — identical classification to V1, where +plain results were never native errors either — so this is not a regression, +and there is zero functional impact today because the #426 logic keys only off +compress success/failure (nudge baselines, rebuild replay, hide-failed). +Extending the anchored position-0 pattern to a generic `Error:` prefix was +rejected as too broad for a shared recognition rule. If these tools ever need +failure semantics, they should gain explicit `errorResult`-style metadata like +the compress path has. diff --git a/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md b/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md index 144fc1d0..4db354b3 100644 --- a/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md +++ b/devlog/2026-09-17_v2-failed-tool-recognition/WORKLOG.md @@ -57,8 +57,9 @@ non-ACP tools. - `scripts/e2e/installed-v2.ts` — nudge-growth stage asserts the observed compress result status is genuinely `"completed"` after each nudge. -- `tests/v2-failed-tool-recognition.test.ts` — NEW: 18 tests across four - sections (pure module, projection, cold rebuild, warm nudge baselines). +- `tests/v2-failed-tool-recognition.test.ts` — NEW: 20 tests across five + sections (pure module, projection, cold rebuild, warm nudge baselines, full + projection integration). - `devlog/2026-09-17_v2-failed-tool-recognition/{REQ,WORKLOG,DESIGN}.md`. ## 3. Design & Implementation Notes @@ -75,6 +76,22 @@ See `DESIGN.md` for the full data-flow analysis and invariants. Key points: historical failure text inside a larger success output can never misclassify it. +### Post-review adjustments (dual-agent review of PR #3) + +- Renamed `isAcpFailedToolOutput`'s third parameter + `neutralizedOutput` → `outputText`: the function only pattern-tests the + joined output text; nothing is neutralized. +- Strengthened the warm-nudge control assertion: the success baseline must + advance past its seeded value (0), not merely become defined — the old + `notEqual(undefined)` check would also pass if the baseline had stayed at + the seed. +- Added two full-projection integration tests driving host-shaped completed + records through `normalizeV2ProjectedHistory` (both reviewers' main gap: + most tests exercised the pure module or downstream consumers rather than + the changed projection code path end-to-end). +- Documented soft-string failures of shared tools as a known limitation + (DESIGN.md §6): classified identically on V1, no functional impact today. + ## 4. Testing & Verification ### Build & Test Commands @@ -87,7 +104,7 @@ npm run build ### Test Coverage -- New test file: `tests/v2-failed-tool-recognition.test.ts` (18 tests): +- New test file: `tests/v2-failed-tool-recognition.test.ts` (20 tests): - acp-failure module: name list, all 9 failure shapes match at position 0, mid-output occurrences rejected, success outputs rejected, metadata precedence, non-ACP exclusion. @@ -101,7 +118,14 @@ npm run build AGENTS.md §5.7.1): failed attempt clears turn/iteration anchors and `lastNudgeShownTokens`, leaves `lastPerMessageNudgeTokens` unchanged and `compressBaselineSet` false; control proves a successful compress advances - the baseline and sets the flag. + the baseline past its seeded value and sets the flag. + - Full projection integration (via `normalizeV2ProjectedHistory`): a + host-shaped completed record with `acpFailed` metadata — and the + historical text-only variant — project to a valid history whose internal + compress part has status `error` with no `output`, while + `origin.normalizedOutput` stays undefined (provider-owned output is + never fingerprinted, so the patcher cannot rewrite it) and + `origin.normalizedError` carries the failure text. - Bug-detection check (AGENTS.md §5.7.3): with the `shared.ts` change reverted, the two recognition pinning tests fail; restored → all pass. - Full suite: 1429 tests, 1428 pass, 1 fail — `tests/soft-block.test.ts` diff --git a/lib/v2/projection/acp-failure.ts b/lib/v2/projection/acp-failure.ts index b5c905e4..02ba6b52 100644 --- a/lib/v2/projection/acp-failure.ts +++ b/lib/v2/projection/acp-failure.ts @@ -67,10 +67,10 @@ function isRecord(value: unknown): value is Record { export function isAcpFailedToolOutput( toolName: string | undefined, rawState: Record, - neutralizedOutput: string, + outputText: string, ): boolean { if (!isAcpToolName(toolName)) return false if (isRecord(rawState.metadata) && rawState.metadata[ACP_FAILURE_METADATA_KEY] === true) return true - return ACP_FAILURE_OUTPUT_PATTERN.test(neutralizedOutput) + return ACP_FAILURE_OUTPUT_PATTERN.test(outputText) } diff --git a/tests/v2-failed-tool-recognition.test.ts b/tests/v2-failed-tool-recognition.test.ts index ba173c4e..3a5c3ce2 100644 --- a/tests/v2-failed-tool-recognition.test.ts +++ b/tests/v2-failed-tool-recognition.test.ts @@ -34,6 +34,12 @@ import type { PluginConfig } from "../lib/config" import { createChatMessageTransformHandler } from "../lib/hooks" import { createTestRegistry } from "./registry-stub" import type { WithParts } from "@opencode-ai/plugin" +import { Message } from "@opencode/ai" +import type { Message as AiMessage } from "@opencode/ai" +import { DateTime } from "effect" +import { Info as SessionMessageInfo } from "@opencode/schema/session-message" +import type { Info as SessionMessageInfoValue } from "@opencode/schema/session-message" +import { normalizeV2ProjectedHistory, type V2Projection } from "../lib/v2/projection" // ─── (a) acp-failure pure module ──────────────────────────────────────────── @@ -684,6 +690,177 @@ describe("warm nudge baseline behavior around compress attempts", () => { assert.equal(state.nudges.shouldInjectThisTurn, false) assert.equal(state.nudges.compressBaselineSet, true) - assert.notEqual(state.nudges.lastPerMessageNudgeTokens, undefined) + // The baseline was seeded to 0 above; a genuine advancement must move + // it past the seed, not merely define it. + assert.ok( + (state.nudges.lastPerMessageNudgeTokens ?? -1) > 0, + "success baseline advanced beyond the seeded value", + ) + }) +}) + +// ─── (e) full projection integration ──────────────────────────────────────── + +const PROJ_MODEL = { id: "model-a", providerID: "provider-a" } +const PROJ_SID = "v2-fail-projection" +const PROJ_CALL_ID = "call-acp-426" +const PROJ_COMPRESS_INPUT = { + topic: "proj", + content: [{ startId: "m00001", endId: "m00004", summary: "s" }], +} +const FAILED_COMPRESS_TEXT = "ACP compress failed: simulated execution failure" + +function projUserSource(id: string): Record { + return { type: "user", id, time: { created: 1 }, text: "compress the earlier turns" } +} + +function projAssistantSource( + id: string, + toolStateRecord: Record, +): Record { + return { + type: "assistant", + id, + time: { created: 2 }, + agent: "code", + model: PROJ_MODEL, + content: [ + { + type: "tool", + id: PROJ_CALL_ID, + name: "compress", + executed: false, + state: toolStateRecord, + time: { created: 2 }, + }, + ], + } +} + +function projValidate(projected: readonly unknown[]): readonly unknown[] { + for (const message of projected) { + // Public transport encodes DateTime values as epoch millis; Info.make + // validates that form without coupling the test to npm's layout. + SessionMessageInfo.make(toProjSchemaValue(message) as SessionMessageInfoValue) + } + return projected +} + +function toProjSchemaValue(value: unknown): unknown { + if (Array.isArray(value)) return value.map((entry) => toProjSchemaValue(entry)) + if (!isProjRecord(value)) return value + return Object.fromEntries( + Object.entries(value).map(([key, entry]) => { + if (key !== "time" || !isProjRecord(entry)) return [key, toProjSchemaValue(entry)] + return [ + key, + Object.fromEntries( + Object.entries(entry).map(([timeKey, timeValue]) => [ + timeKey, + typeof timeValue === "number" + ? DateTime.makeUnsafe(timeValue) + : toProjSchemaValue(timeValue), + ]), + ), + ] + }), + ) +} + +function isProjRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value) +} + +function runProjection(toolStateRecord: Record): V2Projection { + const projected = projValidate([ + projUserSource("msg_proj_user"), + projAssistantSource("msg_proj_asst", toolStateRecord), + ]) + const outgoing: AiMessage[] = [ + Message.make({ id: "msg_proj_user", role: "user", content: "compress the earlier turns" }), + Message.make({ + id: "msg_proj_asst", + role: "assistant", + content: [ + { + type: "tool-call" as const, + id: PROJ_CALL_ID, + name: "compress", + input: PROJ_COMPRESS_INPUT, + }, + { + type: "tool-result" as const, + id: PROJ_CALL_ID, + name: "compress", + result: { type: "text" as const, value: FAILED_COMPRESS_TEXT }, + }, + ], + }), + ] + return normalizeV2ProjectedHistory(projected, outgoing, { + sessionID: PROJ_SID, + agent: "code", + currentModel: PROJ_MODEL, + }) +} + +interface ProjCompressPart { + type: "tool" + tool: string + state: { status: string; error?: string; output?: string } +} + +describe("full projection of host-shaped completed ACP failures", () => { + function assertFailureProjection(projection: V2Projection) { + assert.equal( + projection.valid, + true, + `projection must stay valid: ${projection.rejection?.message ?? ""}`, + ) + const part = projection.messages + .flatMap((message) => message.parts) + .find((candidate) => candidate.type === "tool" && candidate.tool === "compress") as + ProjCompressPart | undefined + assert.ok(part, "internal projection must contain the compress tool part") + assert.equal(part.state.status, "error") + assert.equal(part.state.error, FAILED_COMPRESS_TEXT) + assert.ok(!("output" in part.state), "failed result must not carry a success output") + const entry = projection.entries.find( + (candidate) => candidate.sourceMessageId === "msg_proj_asst", + ) + assert.ok(entry, "provenance must cover the assistant source") + const origin = entry.origins.find((candidate) => candidate.callId === PROJ_CALL_ID) + assert.ok(origin, "provenance must cover the tool call") + // No output fingerprint is recorded for the reclassified result, so + // the patcher can never rewrite the provider-owned host output. + assert.equal(origin.normalizedOutput, undefined) + assert.equal(origin.normalizedError, FAILED_COMPRESS_TEXT) + } + + it("projects a completed record carrying acpFailed metadata as an error", () => { + assertFailureProjection( + runProjection({ + status: "completed", + input: PROJ_COMPRESS_INPUT, + content: [{ type: "text", text: FAILED_COMPRESS_TEXT }], + time: { start: 2, end: 3 }, + metadata: { + [ACP_FAILURE_METADATA_KEY]: true, + acpError: "execution", + tool: "compress", + }, + }), + ) + }) + + it("projects historical completed failure output without metadata as an error", () => { + assertFailureProjection( + runProjection({ + status: "completed", + input: PROJ_COMPRESS_INPUT, + content: [{ type: "text", text: FAILED_COMPRESS_TEXT }], + time: { start: 2, end: 3 }, + }), + ) }) })