fix: recognize failed V2 ACP tool results as failures - #3
Open
ranxianglei wants to merge 2 commits into
Open
ranxianglei wants to merge 2 commits into
ranxianglei wants to merge 2 commits into
Conversation
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
- 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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes ranxianglei#426
Problem
The pinned
@opencode/plugin@2.0.3Promise adapter has no safe native error channel, socreateV2Toolreturns resolved results for caught execution failures and the host records them asstate.status === "completed"even when the content saysACP compress failed: .... ACP's internal V2 projection preserved thatcompletedstatus, so:messageHasCompress(lib/messages/query.ts) treated a failed compression as success;lib/messages/inject/inject.ts) advancedlastPerMessageNudgeTokens/ setcompressBaselineSetafter a failed compression;lib/state/rebuild.ts) replayed the failed call and rebuilt a block that never existed;hideFailedCompressCallsnever hid the failed call (it only matchesstatus === "error").The installed E2E fake-provider classifier also missed these failure texts (
ACP .* execution faileddoes not matchACP compress failed:).Fix
errorResult()inlib/v2/tools.tsadds explicitacpFailed: truemetadata to every resolved error result (additive — existing per-reason flags unchanged).lib/v2/projection/acp-failure.ts: canonical ACP tool-name list, metadata key, position-0-anchored failure-text pattern, predicates — shared by the projection and the E2E fake provider so they cannot drift.toolState()projects a host-completedresult for an ACP tool to an internalstatus: "error"when explicit failure metadata is present or the output matches the historical failure-text pattern (pre-fix host history). It exposes nooutputfield, so provider-owned lowered results are never rewritten: origin correlation keeps the lowered output untouched and the patcher's status-change guard sees identical states on both sides.inspectToolResultsinscripts/e2e/fake-llm-server.tsclassifies ACP results through the shared anchored pattern (legacy regex retained for non-ACP tools).installed-v2.tsnudge-growth stage now asserts the observed compress results are genuinelycompleted, not merely present.No nudge code changes were needed:
messageHasCompressAttemptis status-agnostic (failed attempts still reset pending-nudge state), whilemessageHasCompressrequirescompleted— once the projection emitserrorfor failures, success baselines stop advancing and V2 aligns with V1 semantics exactly.Verification
tests/v2-failed-tool-recognition.test.ts(pure module, projection shapes, cold rebuild, warm multi-turn nudge baselines withpreserveRecentMessages: 20); verified they fail with the projection fix reverted (bug-detection check per AGENTS.md §5.7.3).tests/soft-block.test.ts, which hardcodesmkdirSync('/tmp/...')at import time and crashes on this sandbox's read-only/tmpmount — environmental, unrelated to this diff (passes in CI).