Skip to content

fix(1468): a timed-out ack stops being reported as a failed mutation - #1518

Merged
artokun merged 5 commits into
mainfrom
fix/1468-ack-vs-completion
Aug 13, 2026
Merged

fix(1468): a timed-out ack stops being reported as a failed mutation#1518
artokun merged 5 commits into
mainfrom
fix/1468-ack-vs-completion

Conversation

@artokun

@artokun artokun commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Claims #1468.

panel_civitai_search and panel_exit_subgraph return isError:true after the command was applied. The reporter proved both: renderRev had advanced and the grid reported loading:true; the next panel_graph_outline showed scope root.

The mechanism I first published on the issue was wrong

Corrected publicly in the thread. Measured, not assumed:

Neither was coupled to a slow operation. Both are the #357/#694 shape: a busy-but-alive main thread missing a tighter-than-default bound. Which makes raising the bound — the fix I had argued against — the right one for the first.

Changes

1. civitai_search: private 10000BRIDGE_DEFAULT_TIMEOUT_MS. Half this codebase's own default, fencing an external wait that no longer exists.

2. exit_subgraph: settle a no-reply with one scope read. Its effect is locally observable, so it asks instead of handing back "may have been applied" plus homework.

Observation Reported as
scope:"root" decisive — the goal state holds
scope:"subgraph" not decisive; stays a failure, names the open question
probe unanswerable today's message, untouched (#1473's rule)

The ambiguous case is ambiguous on purpose: exit pops to the immediate parent (#412), so "inside a subgraph" is equally consistent with "never landed" and "landed, from a nested subgraph". Resolving that by guessing would be the same harm as the false failure, pointed the other way.

Two overclaims caught while self-reviewing and fixed:

  • the success payload says at_root, not exited — the read establishes where the canvas is, never that this command put it there;
  • the ambiguous note originally said "do NOT retry blindly", which lands directly beneath the retry_of instruction ctx.call appends for every RETRY_TOKEN_CMDS timeout. Verified against the panel's ledger: a token retry is answered from the original's entry with no second executor run (Mutation retry needs an explicit caller retry-identity (the #683 revert's unsolved half) #694); a bare repeat executes fresh. The note now draws that distinction instead of contradicting the line above it.

Only a no-reply takes this path. A relayed executor verdict already reasoned about the same uncertainty with the canvas in front of it.

Verification

  • Timeout strings measured against a real UiBridge + frozen tab, not transcribed from the template — graph_exit_subgraph does reach dispatch (it is fence-inert, panel_find_nodes is misclassified as mutating and blocked on older panels #778) rather than being refused pre-dispatch, which is the trap that would have made the whole path dead code.
  • The fixture drives the real ctx.call token-minting branch (tagged reply-timeout + dispatched rid), so the assertions run against the production message shape.
  • 6 new tests. 5/5 mutations killed, including deleting the call-site wiring while leaving the helper intact — the one that survives a helper-only suite.
  • Full suite: 490 files, 9224 passed. lint, check:vocabulary, check:unknown-collapse, vocab:export --check, asset-counts --check, docs:gen all clean.

Found while measuring, not in scope here

panel_civitai_results is the command that genuinely awaits the fetch (await state.activeReloadPromise), on the same 10 s bound. It is a read, so its timeout is not the false mutation failure this issue is about — filed separately rather than folded in.

Copilot AI balanced review requested due to automatic review settings August 13, 2026 10:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

artokun and others added 2 commits August 13, 2026 03:45
…uessing

`panel_civitai_search` and `panel_exit_subgraph` reported `isError:true` after
the command had been applied. The reporter proved both: renderRev advanced and
the grid was loading; the next graph read showed scope `root`.

The mechanism I first published on that issue was wrong, and this corrects it.
Measured, not assumed:

  - `driveSearch` does NOT await the CivitAI fetch. It fires `void reload(...)`
    and returns `{dispatched:true, renderRev}` with no await in the handler —
    panel #282, shipped at panel 0.11.0. #1468 was filed from 0.11.44.
  - `graph_exit_subgraph`'s own receipt (`confirmCanvasNavigation`) budgets
    25 polls x 40ms ~= 1s and returns early on success, clearing its 15s bound
    by 15x.

So neither was coupled to a slow operation. Both are the #357/#694 shape: a
busy-but-alive main thread missing a tight bound.

  - `civitai_search` takes `BRIDGE_DEFAULT_TIMEOUT_MS` instead of a private
    10s literal — half the codebase's own default, fencing a wait that no
    longer exists.
  - `exit_subgraph`'s effect is locally observable, so a no-reply now takes one
    scope read and reports what it found. Root is decisive and reported as
    such. "Inside a subgraph" is NOT: exit pops to the immediate parent (#412),
    so it cannot separate "never landed" from "landed, from a nested subgraph"
    — that stays a failure and says which question is open. An unanswerable
    probe claims nothing either way (#1473's rule).

The success payload says `at_root`, not `exited`: the read establishes where
the canvas IS, never that this command is what put it there.

Only a no-reply is settled this way. A relayed executor verdict already
reasoned about this uncertainty with the canvas in front of it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…redicate

P1: the settling read was not pinned to the tab the navigation was dispatched
to. `ctx.call` runs `ensureReachable` first, which silently rebinds an unpinned
current-mode session onto the sole remaining interactive tab when the bound one
has gone — the exact situation an unanswered command makes likely. A different
tab sitting at root would have been reported as this navigation having landed:
a WRONG-TARGET success, worse than the false failure being fixed. The probe now
records the dispatch tab and treats a moved binding as inconclusive.

P1: `isAckTimeout`'s looseness is safe for `workflow_open` because a match there
only opens a door — the #514 receipt correlated to the request's exact rid is
what decides. `exit_subgraph` has no receipt, so the predicate ALONE promotes an
error to success, and an acked executor error merely OPENING with the bridge's
preamble would have taken that path. Reverted `isAckTimeout` to its original
literal (workflow_open untouched) and gave this path a stricter sibling that
requires the canonical frozen-tab clause and the literal single space in
`within <N> ms` — both measured against a live UiBridge.

The root note now names the alternative reading in the message itself, not only
in a source comment: what is established is where the canvas IS, not that this
command put it there.

8 tests; 7/7 mutations killed, including deleting the dispatch-tab pin and
loosening the predicate back. The rebind test asserts the rebind OCCURRED
rather than inferring it from the verdict.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@artokun

artokun commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Scope note — panel_enter_subgraph is deliberately not included.

It sits two lines away from exit_subgraph, carries the same 15 s bound, and goes through the same confirmCanvasNavigation receipt, so leaving it out is a choice rather than an oversight and is worth stating.

Two reasons:

  1. The issue names exit, and the reporter's evidence is about exit. I have no measurement that enter misbehaves.
  2. enter is actually the easier case to settle — its goal state is fully checkable, since viewing.owner_node_id should equal the node_id that was requested, whereas exit's "inside a subgraph" reading is irreducibly ambiguous under comfy_cli_search_nodes fails instead of falling back when comfy CLI is absent #412. That makes it a clean follow-up, but it also means widening this PR would ship a second, differently-shaped verification on the back of one review.

If enter turns out to need it, the settle helper generalises to it directly — the only new part is comparing owner_node_id against the requested node rather than testing for root.

artokun and others added 2 commits August 13, 2026 04:04
…text

codex round 2 P1, and it was right to reject the regex twice. Tightening the
pattern could never work: ACKED panel errors arrive as ARBITRARY `msg.error`
text and `ctx.call` flattens both kinds into the same text-only ToolResult, so
any sentence the bridge can write, a panel error can also contain. There is no
pattern that admits the genuine no-reply and excludes a verbatim copy of it.

`markReplyTimeout`/`isReplyTimeoutTagged` already answer this exactly, on the
error object, at the only place that knows — the bridge. The information was
being dropped in translation. `ctx.call` now carries it onto the result as a
non-enumerable symbol (payload byte-identical), and the settle path keys on
that instead of parsing anything.

`isAckTimeout` is byte-identical to main again: workflow_open's looseness is
safe there because a match only opens a door and the #514 rid-correlated
receipt decides. This path has no receipt, which is the whole difference.

The test now reproduces codex's exact case — an acked error carrying the
canonical sentence VERBATIM, textually indistinguishable from a real timeout
for the sentence's full length. It is refused because the tab ANSWERED.

8 tests; 8/8 mutations killed. M8 re-implements the text predicate and dies on
that collision, which is the evidence the finding was real rather than
theoretical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ies the tag

codex round 3's only remaining finding, and one I had reached independently
while auditing the same question: the inner retry catch returned fail(err2)
unmarked. That branch is entered only when the FIRST error was a reconnect flap
or a switch refusal, so a bridge-tagged no-reply on the RETRY lands there rather
than on the outer path.

Not a correctness hole — graph_exit_subgraph is not retry-safe, so it cannot
reach that branch, and an unmarked result fails closed (nothing settles, no
false success). It is a completeness one: it would silently switch the settle
off for a real sequence, which reads as "the fix does not work" much later.

All three terminal error returns in ctx.call now carry it: inner retry, the
retry-token branch, and the outer fail.

Also recorded from the round-3 audit: the mark is invisible to JSON.stringify,
Object.keys, for..in and spread (measured), so the wire payload is unchanged.
It IS discoverable via Reflect.ownKeys/getOwnPropertySymbols — no consumer does
that, but "non-enumerable" is not "unreachable" and the distinction belongs in
the record rather than in a claim that it is invisible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@artokun
artokun marked this pull request as ready for review August 13, 2026 11:10
@artokun

artokun commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Codex gate: three rounds, all findings closed.

Round Verdict Finding Status
1 NO-SHIP P1 — settling read uncorrelated; ctx.call can silently rebind to another tab, so a different tab at root becomes success fixed — probe pinned to the dispatch tab
1 NO-SHIP P1 — loose text predicate could promote an acked error to success see round 2
2 NO-SHIP rebind closed; the "canonical" predicate still cannot separate an acked error reproducing the sentence verbatim fixed at the cause — see below
3 NO-SHIP P1 — inner retry catch dropped the tag fixed (found independently in the same audit)

Round 2 is the one that changed the design. Codex rejected message-text matching twice and was right both times: acked panel errors arrive as arbitrary msg.error text and ctx.call flattens both kinds into the same text-only result, so any sentence the bridge can write, a panel error can also contain. No regex separates them.

The bridge already answers this exactly — markReplyTimeout / isReplyTimeoutTagged, on the error object, at the only place that knows. It was simply being dropped in translation. ctx.call now carries it onto the result as a non-enumerable symbol and the settle path keys on that; no text is parsed anywhere in the new path. isAckTimeout is byte-identical to main again.

The evidence that finding was real rather than theoretical: mutation M8 re-implements the text predicate and is killed by a fixture carrying the canonical sentence verbatim.

Round 3's finding, honestly scoped: it was a completeness gap, not a correctness one — graph_exit_subgraph is not retry-safe so it cannot reach that branch, and an unmarked result fails closed. Fixed anyway, because it would have silently switched the settle off for a real sequence.

One correction to my own earlier claim in this PR: I said the symbol is invisible downstream. Measured: invisible to JSON.stringify, Object.keys, for..in and spread — but Reflect.ownKeys/getOwnPropertySymbols can see it. No consumer does that, and the wire payload is unchanged, but "non-enumerable" is not "unreachable".

Final: 490 files / 9227 tests, tsc clean, all six repo gates clean, 8/8 mutations killed.

@artokun
artokun merged commit acffcbb into main Aug 13, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants