fix: recognize failed V2 ACP tool results as failures

devlog/2026-09-17_v2-failed-tool-recognition/DESIGN.md

@@ -0,0 +1,223 @@
+# 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<Tool.Result<Output>>
+```
+
+- The native Effect-based `Tool.Info` CAN return a typed error
+  (`Effect<Result, Tool.Error>`), 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 <tool> 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 <tool> 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).
+
+## 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.

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<Tool.Result>`
+  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 <tool> 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 <tool> 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.