Skip to content

fix(panel): fence ask_user / request_secret to the conversation whose turn asked for them - #711

Merged
artokun merged 5 commits into
mainfrom
fix/fence-interactive-cards-to-conversation
Aug 7, 2026
Merged

fix(panel): fence ask_user / request_secret to the conversation whose turn asked for them#711
artokun merged 5 commits into
mainfrom
fix/fence-interactive-cards-to-conversation

Conversation

@artokun

@artokun artokun commented Aug 7, 2026

Copy link
Copy Markdown
Owner

What

ask_user (a question card) and request_secret (a masked token input) are the two
cards the panel renders on the agent's behalf that collect a value from the user and
return it as the tool result
. On main both painted unconditionally:

onAsk(msg)    { const p = paintQuestion(msg);  return p; },
onSecret(msg) { const p = paintSecret(msg);    return p; },

while onThinking, sitting between them in the same object, is fenced — "a late
thinking frame arriving AFTER a local interrupt must not resurrect the indicator the
user just dismissed."

For these two the consequence isn't a stray indicator, it's a stray value: an
abandoned or superseded turn could paint a secure input into whatever conversation the
tab happened to be showing, and the token typed there came back as the result of a turn
belonging to a different conversation. mcp #897 made agent sessions
orchestrator-scoped, so "which conversation is on screen" and "which turn this frame
belongs to" are now genuinely separable — which is what makes it reachable.

This PR fences both.

Origin

Found by the independent gate on PR #680 and re-verified on origin/main.

It deliberately does NOT address #680's structural blocker. The panel publishes
shared conversation state off sendFrame() returning true, which only means the local
socket accepted bytes — not that the orchestrator applied the transition. Both the panel
author and the gate concluded separately that the panel cannot fix that alone; it needs
orchestrator-confirmed session transitions, which do not exist yet. This is a standalone
fix that lands on its own and leaves #680 exactly where it is.

The fence

New pure module web/js/lib/interactive-card-fence.js. Paint only when a turn is in
flight in this tab and the conversation that turn was captured under is the
conversation on screen. Why nothing weaker works:

candidate why it's insufficient
agentWorking alone bare "some turn is in flight" — exactly what #381 already found insufficient for attribution, which is why liveTurnThreadId exists. Still the load-bearing half for the reported shape: every abandon path (new chat, opening an older conversation, workflow switch, backend switch, Disconnect, Esc, cancelling a queued message) routes through endTurnLocally().
CURRENT_THREAD_KEY / thread?.id alone describes only what's on screen; carries zero frame provenance, so it can't tell "this turn owns the screen" from "this is a late frame for something else" — the one distinction needed.
commandRidLedger / commandEpoch the rid dedupes one command from another; the epoch separates a restarted orchestrator from its predecessor (#694). An abandoned turn and the live turn share both the epoch and the socket. (The socket half is already covered upstream by isActive().)
the frame itself request_secret carries only label/hint; ask_user carries an ask_id that is a fresh random UUID per call, for correlation, not a conversation. A conversation id on the wire is the fix this should have — it's an orchestrator protocol change, not a panel change.

The owner-less case

liveTurnThreadId is captured as thread?.id ?? null, so it's null for a turn that
began on a view with no conversation yet. Such a turn isn't owner-less — it owns the
conversation it creates. My own gate got this wrong twice, and both wrong answers
are written down in the module header:

  • refusing whenever owner !== shown false-refuses that legitimate card (the agent
    emits a progress say, which mints the conversation, then asks);
  • painting whenever owner === null opens a hole the other way — loadThread()'s
    cross-workflow BLOCKED branch calls detachInvalidCurrentThread({rebind:true}) and
    returns without endTurnLocally();
  • "created after the turn started" isn't enough either — a conversation created in
    another tab after this turn began can sync into this tab's history and be rebound
    onto the screen by that same detach path. Newer than the turn, and still not the
    turn's.

So the discriminator is provenance, not age: lastMintedThreadId is written only by
record()'s thread-creation branch (the sole place a conversation is created) and reset
by onTurn("working"), so a non-null value means exactly "record() created this
conversation during the turn now running."
A conversation that merely appeared can
never satisfy it. It's module-scoped on purpose — record() runs from many points
inside the panel builder closure, and a let partway down that closure would be in its
temporal dead zone for any call reaching record() earlier.

What the agent sees when a card is refused

An explicit ok:false — not silence, not a fabricated success, and not a card painted
somewhere else. Tone matches command-liveness.js's redactSensitiveReply, including
its rule that the panel reports what it OBSERVED, never a guess:

the panel did not show the secure token input for "request_secret": that ComfyUI tab
has no turn in flight, so nothing there owns this card. That is what the panel
OBSERVED, not a diagnosis — the usual cause is that the turn was ended in that tab (an
interrupt, a new chat, an older conversation reopened, a workflow or backend switch, a
disconnect), but a turn start the panel has not registered yet looks the same. Nothing
was shown, nothing was collected and nothing was stored — a secure input must never be
painted into whatever conversation the tab happens to be showing: the token typed
there would come back as THIS turn's result, putting a secret in a conversation the
user never chose to put it in. Do not retry this in a loop: say in plain text what you
need and why, and ask again after the user's next message in the tab you want to ask
in.

No value can be logged on this path: the refusal runs before any card exists, so
there is nothing to leak. A refused request_secret also consumes the Settings
"set at" marker rather than leaving it armed for an unrelated later secret.

Known residuals (documented, pinned by tests, deliberately not fixed here)

Both have one root cause — the turn frame carries a state and no turn identity:

  1. a straggler turn:working arriving past STALE_WORKING_GUARD_MS is
    indistinguishable from a fresh turn's, so onTurn adopts the conversation then on
    screen. Narrower than the defect being fixed (which required no precondition at all),
    but not zero.
  2. the mirror: a genuinely fresh turn whose turn:working lands inside that window is
    discarded, so a legitimate card is refused. It fails closed, with the honest
    error above.

Closing either means putting a turn/conversation id on the wire, which subsumes both —
a comfyui-mcp protocol change, not a panel change. Each has a KNOWN RESIDUAL: test so
the residual is visible and a future turn-id has a test to flip.

Verification

  • npm run test:unit2554 pass, 0 fail. npm run typecheck clean.
    node --check on both files; the panel re-verified as a real ES module (parses and
    links, no duplicate top-level declaration).
  • 41 tests in browser_tests/unit/interactive-card-fence.test.mjs: the pure decision,
    the refusal wording, source-level pins on the shipped handlers, and a lifecycle
    section that wires the real endTurnLocally and onTurn bodies to the real fence
    and handlers over one closure, so agentWorking / liveTurnThreadId are produced by
    shipped code rather than injected.
  • 26 mutations verified — each breaks one behaviour, fails a specific named test,
    and is restored. Including the pre-fix state: removing the fence from both handlers
    fails the load-bearing tests with a card painted into the wrong conversation.
  • 3 adversarial gate rounds on substance plus a narrow re-gate of the newest commit.
    Every finding was either fixed (round 2's false refusal, round 3's cross-tab hole,
    round 3's brittle pins) or accepted and documented (the two turn-identity residuals).
  • No control bytes in any changed file; no git-binary files.
  • pyproject.toml and PANEL_VERSION are untouched — no Comfy Registry publish.

Do not merge without a human look.

🤖 Generated with Claude Code

artokun and others added 5 commits August 6, 2026 18:05
… turn asked

Found by the independent gate on PR #680 and re-verified on origin/main.

The panel renders two INTERACTIVE cards on the agent's behalf that COLLECT a
value from the user and hand it back as the tool result: `request_secret` (a
masked token input) and `ask_user` (a question card). Both painted
unconditionally, while the handler sitting between them in the same object
(`onThinking`) is fenced on `agentWorking` precisely so a late frame from a
turn the user already ended cannot act on a screen it no longer owns.

For these two the consequence is not a stray indicator but a stray VALUE: an
abandoned or superseded turn could paint a secure input into whatever
conversation the tab happened to be showing, and the token typed there came
back as the result of a turn belonging to a DIFFERENT conversation. mcp #897
made agent sessions orchestrator-scoped, so "which conversation is on screen"
and "which turn this frame belongs to" are now genuinely separable.

The fence is the PAIR (agentWorking && liveTurnThreadId === thread?.id): a turn
must be in flight in this tab, AND the conversation captured as that turn's
owner at turn start must be the conversation on screen. Neither half suffices
alone -- see the header of web/js/lib/interactive-card-fence.js for why the
rid/epoch on the dispatch path and the frame's own fields cannot discriminate.

A refused card answers the agent with an explicit ok:false naming what was
refused, stating that nothing was shown, collected or stored, and giving the
one next step that works -- the tone command-liveness.js already uses for this
class. Nothing is painted anywhere else, and no value can be logged because the
refusal runs before any card exists.

Deliberately does NOT address PR #680's structural blocker (the panel publishes
shared conversation state off sendFrame() returning true, which needs
orchestrator-confirmed session transitions that do not exist yet).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fecycle tests, residuals documented

Round 1 of my independent codex gate raised three findings.

SEVERE (a straggler turn:working past onTurn's guard re-authorizes whatever
conversation is on screen) and IMPORTANT (the mirror: a genuinely fresh turn's
turn:working landing INSIDE that guard is discarded, so a legitimate card is
refused) share one root cause: the `turn` frame carries a state and no turn
identity. Closing either means putting a turn/conversation id on the wire, which
is a comfyui-mcp protocol change and out of scope for this panel fix. Both are
now documented in the module header AND pinned by a test each, so the residual is
visible and a future turn-id has a test to flip. The fix remains strictly better
than origin/main in both directions: the refused case fails CLOSED with an honest
error, and the SEVERE case needs a precondition main required nothing for.

The IMPORTANT finding also showed the refusal wording asserting a cause the panel
never observed ("has already ended"), which is exactly wrong in that case.
command-liveness.js's rule is 'reports what we OBSERVED, never a guess' — the
no_live_turn text now states the observation and offers causes as examples. The
next-step advice no longer implies a retry will help either.

MINOR (the handler tests injected idealized state, so they proved the predicate
rather than shipped lifecycle behaviour): added a section that wires the REAL
endTurnLocally and onTurn bodies to the REAL fence and handlers over one closure
with a fake clock, so agentWorking/liveTurnThreadId are produced by shipped code.

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

Gate round 2 found a real false refusal I had argued away in the design.

A turn that begins on a view with no conversation captures liveTurnThreadId =
null. Its own first output then runs record(), which MINTS the conversation now
on screen. Refusing whenever owner !== shown therefore refused a card that
belongs to the visible turn, in the visible conversation.

The obvious inverse — paint whenever the owner is null — re-opens a hole in the
other direction: loadThread()'s cross-workflow BLOCKED branch calls
detachInvalidCurrentThread({rebind:true}) and RETURNS without endTurnLocally(),
so a thread-less live turn can find an OLD conversation on screen.

The discriminator is whether the shown conversation came into existence DURING
this turn. onTurn('working') now stamps liveTurnStartedAt (cleared on done) and
the fence compares it against thread.createdAt, which record()'s mint stamps off
the same Date.now(). Missing or unusable timestamps fail CLOSED. A source-level
test pins both stamps to the same clock and pins that record() does NOT
retroactively adopt the thread as the turn owner, so the comparison cannot
quietly become dead code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Gate round 3 broke round 2's discriminator, correctly. Comparing the shown
conversation's createdAt against the turn start proves it is NEWER than the turn,
not that this turn created it: a conversation minted in ANOTHER TAB after the turn
began can sync into this tab's history and be rebound onto the screen by
detachInvalidCurrentThread(), and it would have passed.

Replaced with the fact itself. record()'s mint branch — the only place a
conversation is ever created — records the id in a module-level
`lastMintedThreadId`, and onTurn('working') resets it, so a non-null value means
exactly 'record() created this conversation during the turn now running'. The
owner-less branch paints only against that exact id. A conversation that merely
APPEARED can never satisfy it, whatever its age. liveTurnStartedAt and the
timestamp comparison are gone.

The marker is module-scoped deliberately: record() runs from many points inside
the panel builder closure, and a `let` declared partway down that closure would be
in its temporal dead zone for any call reaching record() earlier. Only one panel
is mounted at a time.

Also from round 3:
 - MINOR: the 'liveTurnStartedAt is 0 between turns' comment was untrue because
   endTurnLocally() left it stale — moot, the variable no longer exists.
 - MINOR: the source pins were loose enough to miss a plausible refactor. The mint
   pin is now brace-bounded to record()'s own thread-creation branch (verified: an
   assignment moved to a sibling branch now fails it, where before it passed), the
   record() slice is sanity-checked to actually span record(), and every
   assignment to lastMintedThreadId in the file is enumerated.

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

Narrow re-gate of the previous commit found the marker itself sound (no forgery
path, remount-safe, nothing dangling) but the SOURCE PINS loose in two ways:

 - the writer enumeration only matched a literal `= `, so a later
   `lastMintedThreadId ||= replacement.id` in a rebind path would have slipped
   through while letting a conversation nobody minted vouch for itself;
 - the reset pin accepted a reset hoisted above onTurn's 'working' branch, which
   would fire on 'done' too — a weaker meaning than 'since this turn began'.

Both now fail loudly (mutation-verified: each hypothetical regression applied to
the panel source fails this test).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 7, 2026 01:48

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.

Pull request overview

Fences interactive cards to the visible conversation that owns the active turn, preventing cross-conversation answers or secret disclosure.

Changes:

  • Adds a pure card-fence classifier and refusal messages.
  • Applies fencing to question and secret handlers.
  • Adds extensive unit and lifecycle regression coverage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
web/js/lib/interactive-card-fence.js Defines ownership checks and refusal messages.
web/js/comfyui-mcp-panel.js Integrates turn provenance fencing into card handlers.
browser_tests/unit/interactive-card-fence.test.mjs Tests decisions, handlers, lifecycle, and known residuals.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +21963 to +21966
// Consumed BEFORE the fence so a REFUSED request also clears the marker —
// leaving it armed would attach this button's slot to whatever unrelated
// secret the agent collects next. (paintSecret never touches it, so reading
// it one step earlier is otherwise behaviour-neutral.)
@artokun
artokun merged commit 4925be7 into main Aug 7, 2026
2 checks passed
artokun added a commit that referenced this pull request Aug 7, 2026
Every one of these came from a via-panel report filed today, and none of them
has reached users: 0.11.42 shipped the 0.50.0 tool-surface migration, and the
eight fixes below landed after it.

  #711  ask_user / request_secret are fenced to the conversation whose turn
        asked for them — an abandoned turn could paint a secure-input card into
        whatever conversation the tab was showing, and the token typed there
        came back as THAT turn's result
  #695  socket-vs-widget is classified structurally, so comma-joined union
        datatypes ("IMAGE,MASK") stop failing closed — 17 more node classes are
        addable, 0 newly refused
  #700  the backend node definition is snapshotted rather than read by
        reference, so a refresh can no longer mutate the "backend truth" the
        add-node guard compares against (LoadImage became permanently unaddable)
  #706  a ComfyUI-Manager security refusal no longer reports as "not reachable",
        and no longer carries the flag that authorizes a mutation re-send
  #710  panel_show_media plays audio instead of painting a broken image, and a
        kind it cannot present is no longer counted as painted
  #705  a CivitAI outage surfaces the upstream reason instead of a bare status,
        so it stops reading as a broken search
  #696/#701/#702/#663  a content difference is wrong-canvas evidence only when
        it is STRUCTURAL — the guard was comparing full serialize() against a
        tracker that only snapshots on USER input, so any node-driven widget
        write looked like a different graph
  #698  a widget that structurally cannot hold a value says so, instead of
        reporting a retryable-sounding "did not retain the requested value"

PANEL_VERSION and pyproject version bumped together; the Comfy Registry
publishes off the pyproject change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
artokun added a commit that referenced this pull request Aug 7, 2026
Every one of these came from a via-panel report filed today, and none of them
has reached users: 0.11.42 shipped the 0.50.0 tool-surface migration, and the
eight fixes below landed after it.

  #711  ask_user / request_secret are fenced to the conversation whose turn
        asked for them — an abandoned turn could paint a secure-input card into
        whatever conversation the tab was showing, and the token typed there
        came back as THAT turn's result
  #695  socket-vs-widget is classified structurally, so comma-joined union
        datatypes ("IMAGE,MASK") stop failing closed — 17 more node classes are
        addable, 0 newly refused
  #700  the backend node definition is snapshotted rather than read by
        reference, so a refresh can no longer mutate the "backend truth" the
        add-node guard compares against (LoadImage became permanently unaddable)
  #706  a ComfyUI-Manager security refusal no longer reports as "not reachable",
        and no longer carries the flag that authorizes a mutation re-send
  #710  panel_show_media plays audio instead of painting a broken image, and a
        kind it cannot present is no longer counted as painted
  #705  a CivitAI outage surfaces the upstream reason instead of a bare status,
        so it stops reading as a broken search
  #696/#701/#702/#663  a content difference is wrong-canvas evidence only when
        it is STRUCTURAL — the guard was comparing full serialize() against a
        tracker that only snapshots on USER input, so any node-driven widget
        write looked like a different graph
  #698  a widget that structurally cannot hold a value says so, instead of
        reporting a retryable-sounding "did not retain the requested value"

PANEL_VERSION and pyproject version bumped together; the Comfy Registry
publishes off the pyproject change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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