fix(bridge): report a GOODBYE'd call as unknown-outcome, not failed

[iceteaSA]

A route GOODBYE against an in-flight request currently surfaces as a bare transport error. It reads as "the call failed", which invites a re-run.

It isn't a failure. The daemon emits route GOODBYEs after its drain wait, regardless of whether that drain completed (supervise.rs — the quiescence result is consumed only by a warn!; route release and GOODBYE emission run unconditionally). 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.

We hit this concretely: a ck module restart aft issued from an aft-served session lost the reply to its own command, and the already-produced stdout went with it. The command had succeeded.

Re-running such a call double-applies whatever it did. Since harness bash executes through this path, the realistic case is a second git push, migration, or npm publish.

Change

Append a disposition saying the outcome is UNKNOWN and that state should be verified before re-running:

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.

Deliberately distinct from BASH_TRANSPORT_DISPOSITION, whose "no background task was created … Re-run the command" is correct for a not-sent transport failure and wrong here. The GOODBYE branch is checked before the bash branch so a GOODBYE'd bash call cannot pick up the re-run guidance, and a test asserts that specific negative.

No retry behaviour changes. isRouteProvenAbsentError still gates the single in-place retry on unknown_channel / StaleRouteHandleError — errors proving the bytes never left. GOODBYE is outcome-unknown by construction and stays out. The existing "outcome-unknown request failures still surface without an in-place retry" test is untouched and still passing.

The one ugly part, stated plainly

The match is on the message literal "route closed by subc", because subc-client raises the GOODBYE as a bare SubcError with no code — there is no other discriminator on the wire. It's documented as a seam and fails open: if the string changes, callers lose the disposition rather than getting a wrong one. A stable code on that error has been requested upstream in cortexkit/subconscious; when it lands this becomes a code check and the string goes away.

Also excluded: a SubcError that does carry a code (e.g. module_reloading, which is proven-not-forwarded and legitimately retryable) is left alone, with a test pinning that.

Verification

6/6 in error-contract.test.ts, each assertion observed failing before being trusted — disabling the predicate reds exactly the three new behavioural tests and leaves the pre-existing two green. Full plugin suites at baseline (bridge 456, aft 198, pi 712, opencode 1289), lint/format clean, v0.49 audit passes, release artifacts untouched.

Wording came out of a review with the subconscious side. The specific ask was to say UNKNOWN rather than "failed" — an operator who reads "failed" re-runs it, which is the same trap in prose that a blanket retry would be in code.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Treat route GOODBYE on in‑flight calls as UNKNOWN outcome instead of a failure. This prevents unsafe retries that could double-apply mutations.

• Bug Fixes
  • Append SUBC_MODULE_RESTART_DISPOSITION to bare GOODBYE SubcError messages so they state the outcome is UNKNOWN and advise verifying state before any retry.
  • Handle GOODBYE before bash transport logic so bash calls don’t get BASH_TRANSPORT_DISPOSITION re-run guidance.
  • Retry behavior unchanged; only proven-not-sent errors (e.g., StaleRouteHandleError, unknown_channel) trigger the single in-place retry.
  • Detect via the literal “route closed by subc” from @cortexkit/subc-client; fails open if it changes. Tests cover single append, bash path, and leaving coded errors (e.g., module_reloading) untouched.

^{Written for commit d3e66a8. Summary will update on new commits.}

Greptile Summary

The PR distinguishes route GOODBYEs from proven-not-sent transport failures and appends unknown-outcome guidance without changing retry behavior.

• Detects bare route-GOODBYE SubcError instances before bash-specific transport handling.
• Warns callers to verify state rather than blindly retrying a potentially completed mutation.
• Adds coverage for bash precedence, idempotent annotation, required wording, and exclusion of coded errors.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security issues identified.

The new branch preserves the original error and existing retry decisions while ensuring matching route GOODBYEs cannot receive the contradictory bash re-run guidance.

Important Files Changed

Filename	Overview
packages/aft-bridge/src/error-contract.ts	Adds narrowly scoped, idempotent GOODBYE classification and unknown-outcome guidance ahead of existing bash transport handling; no actionable defect found.
packages/aft-bridge/src/tests/error-contract.test.ts	Adds focused tests for disposition wording, branch precedence, duplicate adaptation, and coded-error exclusion.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[adaptToolError receives error] --> B{Bare route GOODBYE SubcError?}
    B -->|Yes| C[Append UNKNOWN-outcome disposition]
    C --> D[Return original error]
    B -->|No| E{Bash transport-class error?}
    E -->|Yes| F[Append safe re-run disposition]
    E -->|No| D
    F --> D

Loading

_{Reviews (1): Last reviewed commit: "fix(bridge): report a GOODBYE'd call as ..." | Re-trigger Greptile}

[cubic-dev-ai]

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/aft-bridge/src/error-contract.ts">

<violation number="1" location="packages/aft-bridge/src/error-contract.ts:84">
P2: The GOODBYE detection rests on a fragile substring match: `error.message.includes("route closed by subc")` combined with `error.code === undefined`. This is the exact coupling the PR flags as a known limitation, and the downside of a false negative is material — the UNKNOWN/verify-before-rerun guidance would simply not be appended, recreating the blind re-run of a possibly-landed mutation that this change exists to prevent. Since the literal is the only discriminator until upstream adds a code, consider extracting it to a single named constant (shared by the matcher and the test helper) and adding a comment marking it as a temporary upstream-coupling to revisit, so a future wire-message change fails loudly in tests rather than silently. Note the check is a partial `includes`, so it also cannot distinguish a route GOODBYE from any other SubcError that happens to contain the phrase.</violation>

<violation number="2" location="packages/aft-bridge/src/error-contract.ts:108">
P3: The new GOODBYE branch appends the disposition message to the error for every command, but function docstring still claims 'Other commands retain their errors' and the file header says hosts must not rewrite the contract-owned message. That contract text is now stale: a non-bash GOODBYE error is no longer returned unchanged. The behavior is intentional per the PR, so this is mainly a documentation/contract-consistency concern — update the docstring to call out the GOODBYE exception so future readers don't assume non-bash errors are always passed through untouched.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The GOODBYE detection rests on a fragile substring match: error.message.includes("route closed by subc") combined with error.code === undefined. This is the exact coupling the PR flags as a known limitation, and the downside of a false negative is material — the UNKNOWN/verify-before-rerun guidance would simply not be appended, recreating the blind re-run of a possibly-landed mutation that this change exists to prevent. Since the literal is the only discriminator until upstream adds a code, consider extracting it to a single named constant (shared by the matcher and the test helper) and adding a comment marking it as a temporary upstream-coupling to revisit, so a future wire-message change fails loudly in tests rather than silently. Note the check is a partial includes, so it also cannot distinguish a route GOODBYE from any other SubcError that happens to contain the phrase.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/aft-bridge/src/error-contract.ts, line 84:

<comment>The GOODBYE detection rests on a fragile substring match: `error.message.includes("route closed by subc")` combined with `error.code === undefined`. This is the exact coupling the PR flags as a known limitation, and the downside of a false negative is material — the UNKNOWN/verify-before-rerun guidance would simply not be appended, recreating the blind re-run of a possibly-landed mutation that this change exists to prevent. Since the literal is the only discriminator until upstream adds a code, consider extracting it to a single named constant (shared by the matcher and the test helper) and adding a comment marking it as a temporary upstream-coupling to revisit, so a future wire-message change fails loudly in tests rather than silently. Note the check is a partial `includes`, so it also cannot distinguish a route GOODBYE from any other SubcError that happens to contain the phrase.</comment>

<file context>
@@ -52,6 +56,35 @@ export function toolErrorFromResponse(
+  return (
+    error instanceof SubcError &&
+    error.code === undefined &&
+    error.message.includes("route closed by subc")
+  );
+}
</file context>

[cubic-dev-ai]

P3: The new GOODBYE branch appends the disposition message to the error for every command, but function docstring still claims 'Other commands retain their errors' and the file header says hosts must not rewrite the contract-owned message. That contract text is now stale: a non-bash GOODBYE error is no longer returned unchanged. The behavior is intentional per the PR, so this is mainly a documentation/contract-consistency concern — update the docstring to call out the GOODBYE exception so future readers don't assume non-bash errors are always passed through untouched.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/aft-bridge/src/error-contract.ts, line 108:

<comment>The new GOODBYE branch appends the disposition message to the error for every command, but function docstring still claims 'Other commands retain their errors' and the file header says hosts must not rewrite the contract-owned message. That contract text is now stale: a non-bash GOODBYE error is no longer returned unchanged. The behavior is intentional per the PR, so this is mainly a documentation/contract-consistency concern — update the docstring to call out the GOODBYE exception so future readers don't assume non-bash errors are always passed through untouched.</comment>

<file context>
@@ -67,8 +100,20 @@ function isTransportClassError(error: unknown): boolean {
+  // 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
</file context>

[ualtinok]

Merged, with one follow-up landed right behind it (1be2dd57): the guard's bare-shape match (code === undefined) fits the shipped subc-client 0.5.0, but the current client source already stamps code: "route_closed" on the same GOODBYE failure — so the next routine client bump would have silently stopped the disposition, which is the false negative your own limitation note (and cubic's P2) worried about. The guard now matches both generations and explicitly excludes closeRoute (same code in newer clients, deliberate local close, outcome known — the guidance would be wrong there), with tests locking all three shapes. The stale adaptToolError docstring went in the same commit. Good PR — the checked-before-bash ordering and the double-pass idempotency test were exactly right.