From d3e66a89635f225d5f4bc305e7978cc733d65e4d Mon Sep 17 00:00:00 2001 From: iceteaSA <171169159+iceteaSA@users.noreply.github.com> Date: Sun, 9 Aug 2026 17:29:41 +0200 Subject: [PATCH] fix(bridge): report a GOODBYE'd call as unknown-outcome, not failed A route GOODBYE against an in-flight request surfaced as a bare transport error, which reads as "the call failed" and invites a re-run. The daemon emits route GOODBYEs after its drain wait regardless of whether that drain completed, so a call in flight at GOODBYE was admitted BEFORE the gate closed and may already have run to completion with only its reply lost. Re-running it double-applies a mutation that already landed -- for a bash call that is a second `git push` or migration. Append a disposition that says the outcome is UNKNOWN and that state should be verified before re-running. Deliberately distinct from BASH_TRANSPORT_DISPOSITION, whose "no task was created, re-run the command" is true for a not-sent failure and false here; the GOODBYE branch is checked first so a GOODBYE'd bash call cannot pick up the wrong guidance. No retry behaviour changes. isRouteProvenAbsentError still gates the single in-place retry on errors proving the bytes never left, and GOODBYE is outcome-unknown by construction, so it stays out. The match is on the message literal because subc-client raises the GOODBYE as a bare SubcError with no code; it fails open (caller loses the disposition) rather than misclassifying, and a stable code has been requested upstream. --- .../src/__tests__/error-contract.test.ts | 60 ++++++++++++++++++- packages/aft-bridge/src/error-contract.ts | 49 ++++++++++++++- 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/packages/aft-bridge/src/__tests__/error-contract.test.ts b/packages/aft-bridge/src/__tests__/error-contract.test.ts index 06d6153c..f1b16009 100644 --- a/packages/aft-bridge/src/__tests__/error-contract.test.ts +++ b/packages/aft-bridge/src/__tests__/error-contract.test.ts @@ -1,8 +1,18 @@ /// import { describe, expect, test } from "bun:test"; +import { SubcError } from "@cortexkit/subc-client"; import { BridgeTransportTimeoutError, isBridgeTransportTimeout } from "../bridge.js"; -import { adaptToolError, BASH_TRANSPORT_DISPOSITION } from "../error-contract.js"; +import { + adaptToolError, + BASH_TRANSPORT_DISPOSITION, + SUBC_MODULE_RESTART_DISPOSITION, +} from "../error-contract.js"; + +/** Shaped exactly as subc-client raises it: bare SubcError, no code. */ +function routeGoodbyeError(): SubcError { + return new SubcError("route closed by subc (GOODBYE)"); +} describe("adaptToolError", () => { test("adds bash transport disposition guidance while preserving the error", () => { @@ -29,4 +39,52 @@ describe("adaptToolError", () => { expect(original.message).toBe("read transport timed out"); expect(original.message).not.toContain(BASH_TRANSPORT_DISPOSITION); }); + + test("a route GOODBYE reports an UNKNOWN outcome, never a failure", () => { + const original = routeGoodbyeError(); + + const adapted = adaptToolError("write", original); + + expect(adapted).toBe(original); + expect(original.message).toContain(SUBC_MODULE_RESTART_DISPOSITION); + // The wording is the safety property: an operator or agent that reads + // "failed" re-runs the call, which double-applies a mutation that may + // already have landed before the daemon dropped the reply. + expect(original.message).toContain("UNKNOWN"); + expect(original.message).toContain("never blind-retry a mutation"); + expect(original.message).not.toContain("Re-run the command."); + }); + + test("a GOODBYE'd bash call gets the unknown-outcome text, not the re-run text", () => { + // BASH_TRANSPORT_DISPOSITION asserts no task was created and says to re-run. + // That is true for a not-sent transport failure and FALSE for a GOODBYE, + // where the command may already have executed. + const original = routeGoodbyeError(); + + adaptToolError("bash", original); + + expect(original.message).toContain(SUBC_MODULE_RESTART_DISPOSITION); + expect(original.message).not.toContain(BASH_TRANSPORT_DISPOSITION); + }); + + test("disposition is appended once when the error passes through twice", () => { + const original = routeGoodbyeError(); + + adaptToolError("read", original); + adaptToolError("read", original); + + const occurrences = original.message.split(SUBC_MODULE_RESTART_DISPOSITION).length - 1; + expect(occurrences).toBe(1); + }); + + test("a coded SubcError is left alone — only the bare GOODBYE shape matches", () => { + // module_reloading is proven-not-forwarded and retryable; it must not be + // dressed up as an unknown outcome. + const coded = new SubcError("route closed by subc (GOODBYE)", "module_reloading"); + + const adapted = adaptToolError("write", coded); + + expect(adapted).toBe(coded); + expect(coded.message).not.toContain(SUBC_MODULE_RESTART_DISPOSITION); + }); }); diff --git a/packages/aft-bridge/src/error-contract.ts b/packages/aft-bridge/src/error-contract.ts index 8668268e..c57c7d06 100644 --- a/packages/aft-bridge/src/error-contract.ts +++ b/packages/aft-bridge/src/error-contract.ts @@ -6,7 +6,11 @@ * rewrite the contract-owned message while doing so. */ -import { isConsumerReconnectTransient, StaleRouteHandleError } from "@cortexkit/subc-client"; +import { + isConsumerReconnectTransient, + StaleRouteHandleError, + SubcError, +} from "@cortexkit/subc-client"; import { isBridgeTransportTimeout } from "./bridge.js"; import { SubcRootGenerationExpiredError, SubcRootReapedError } from "./subc-transport.js"; @@ -52,6 +56,35 @@ export function toolErrorFromResponse( export const BASH_TRANSPORT_DISPOSITION = "The transport to the AFT daemon was interrupted; no background task was created for this command and no task ID exists. Re-run the command. Do not poll bash_status for it."; +/** + * Agent-facing guidance for a call the daemon GOODBYE'd mid-flight. + * + * Deliberately does NOT say the call failed. The daemon emits route GOODBYEs + * after its drain wait regardless of whether that drain completed, so a call + * in flight at GOODBYE was admitted BEFORE the gate closed and may already + * have run to completion with only its reply lost. "Failed" reads as an + * invitation to re-run, which double-applies a mutation that already landed. + */ +export const SUBC_MODULE_RESTART_DISPOSITION = + "The AFT daemon module restarted while this call was in flight, so its outcome is UNKNOWN: it may or may not have executed. Verify actual state before re-running, and never blind-retry a mutation."; + +/** + * A route GOODBYE delivered against an in-flight request. + * + * COUPLING: subc-client raises this as a bare `SubcError` carrying no code + * (client.ts, the `FrameType.Goodbye` branch), so the message literal is the + * only discriminator available. Asked upstream for a stable `code` on that + * error; until it exists this match is the seam and will fail open (no + * disposition appended) rather than misclassify. + */ +function isRouteGoodbyeError(error: unknown): boolean { + return ( + error instanceof SubcError && + error.code === undefined && + error.message.includes("route closed by subc") + ); +} + function isTransportClassError(error: unknown): boolean { return ( isBridgeTransportTimeout(error) || @@ -67,8 +100,20 @@ function isTransportClassError(error: unknown): boolean { * object, class, code, or retry behavior. Other commands retain their errors. */ export function adaptToolError(command: string, error: unknown): unknown { - if (command !== "bash" || !isTransportClassError(error)) return error; if (!(error instanceof Error)) return error; + + // Checked before the bash branch, and applied to every command: a GOODBYE'd + // call has an unknown outcome, so BASH_TRANSPORT_DISPOSITION's "no task was + // created, re-run the command" would be actively wrong here. + if (isRouteGoodbyeError(error)) { + if (error.message.includes(SUBC_MODULE_RESTART_DISPOSITION)) return error; + error.message = error.message + ? `${error.message} ${SUBC_MODULE_RESTART_DISPOSITION}` + : SUBC_MODULE_RESTART_DISPOSITION; + return error; + } + + if (command !== "bash" || !isTransportClassError(error)) return error; if (error.message.includes(BASH_TRANSPORT_DISPOSITION)) return error; error.message = error.message ? `${error.message} ${BASH_TRANSPORT_DISPOSITION}`