feat: add safe long-running support chat workflow - #2982
feat: add safe long-running support chat workflow#2982alectimison-maker wants to merge 14 commits into
Conversation
|
@alectimison-maker is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Strengths: thread_key + composer_ref binding, pre-send persist, outgoing bubble verification, OTP/PIN fail-closed, chat_observe only in Ask mode, Chrome/Firefox parity, untrusted-content gate. Findings:
Note: Resolution evidence relying solely on |
Seven fixes from review of the chat support workflow. Chrome and Firefox copies stay byte-identical. - collectMessages capped with slice(0, MAX_ITEMS), keeping the oldest 200 bubbles while the kernel keeps the last 200. Past 200 messages the observer returned the top of the transcript forever and nextAction never reached reply. - ignoredMessageNode disqualified any node under an [aria-live] or [role="alert"] ancestor. Intercom- and Zendesk-shaped transcripts are one aria-live region, so every message was filtered out. The status-region check now applies to the node itself; interactive and navigational ancestors still disqualify. - markerFor treated an absent aria-current as present, since '' !== 'false'. threadKey bound to the first row of the conversation rail instead of the open thread and stopped changing on thread switch, defeating the thread_changed guard. - chat_send accepted a caller-supplied composer_ref whenever the observation had none, since the drift comparison was vacuous. The observed ref is now required and must match exactly. - message_too_long was unreachable: normalizeChatText had already sliced to the cap, so a 6,000-character body was truncated and dispatched instead of rejected. Normalize one character past the cap so the check fires. - pendingOutbound cleared only on a matching outgoing bubble, so a dispatch that never rendered blocked that text forever. attemptedAt now ages the record out after 10 minutes. - The pin and auth heuristics matched as substrings, so shipping, spinner, pinned, author and authorize each tripped a fail-closed needs_user_input stop. Both are word-bounded now, on inferred signals only. node test/run.js: 2203 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0196wte9FS1SnsrKvrQUbB7m
|
Reviewed this at high effort and pushed 0cfb1df to the branch with seven fixes. Both the Chrome and Firefox copies are patched and still byte-identical; Fixed in 0cfb1dfOrdering — Live regions — Thread binding — in Composer drift guard —
Stuck Substring heuristics — Left for youThese five are judgment calls about the design rather than mechanical fixes, so I didn't touch them.
🤖 Generated with Claude Code |
|
btw @alectimison-maker have we used the /watch slashcommand here? i think it could be used here. |
|
Implemented the remaining design findings in
I kept the WebMCP page-error compatibility adjustment because Chrome 153 reports the protocol tool exception without a duplicate Playwright
Validation:
|
|
Follow-up in Final rerun: |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved send-race, recipient-preflight, thread-binding, draft-loss, and idempotency defects can cause blocked, stale, or duplicate messages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a durable, cross-browser support-chat workflow with observation, guarded sending, persistence, resumable waits, and safety stops.
Changes:
- Adds mirrored chat observation and state-machine implementations.
- Integrates
chat_observe/chat_send, permission gating, persistence, and scheduling. - Adds parity, safety, durability, and protocol tests.
File summaries
| File | Description |
|---|---|
test/webmcp-e2e.mjs |
Updates exception assertions for current Chrome behavior. |
test/run.js |
Adds chat workflow, persistence, safety, and parity tests. |
src/chrome/src/content/content.js |
Exposes composer refs and chat observation dispatch. |
src/chrome/src/content/chat-observation.js |
Implements Chrome DOM chat observation. |
src/chrome/src/agent/tools.js |
Defines and exposes chat tools. |
src/chrome/src/agent/permission-gate.js |
Adds chat tool capability requirements. |
src/chrome/src/agent/mutation-tools.js |
Classifies chat sends as mutations. |
src/chrome/src/agent/chat-workflow.js |
Implements chat state, deltas, and deduplication. |
src/chrome/src/agent/agent.js |
Integrates chat execution, persistence, and resume handling. |
src/chrome/manifest.json |
Registers the chat observation content script. |
src/firefox/src/content/content.js |
Exposes composer refs and chat observation dispatch. |
src/firefox/src/content/chat-observation.js |
Implements Firefox DOM chat observation. |
src/firefox/src/agent/tools.js |
Defines and exposes chat tools. |
src/firefox/src/agent/permission-gate.js |
Adds chat tool capability requirements. |
src/firefox/src/agent/mutation-tools.js |
Classifies chat sends as mutations. |
src/firefox/src/agent/chat-workflow.js |
Implements chat state, deltas, and deduplication. |
src/firefox/src/agent/agent.js |
Integrates chat execution, persistence, and resume handling. |
src/firefox/manifest.json |
Registers the chat observation content script. |
Review details
Suppressed comments (10)
src/chrome/src/agent/chat-workflow.js:426
- Mandatory:
chat_sendclears and replaces the composer, but this decision only checks availability. If the user has typed a draft while the workflow was waiting, the next send silently destroys it. Fail closed when the freshly observed composer is non-empty (or explicitly reconcile its exact contents before submission).
if (snapshot.composer.available !== true) {
return { ok: false, reason: 'composer_unavailable', error: 'The active conversation composer is not available.' };
}
src/firefox/src/agent/chat-workflow.js:426
- Mandatory:
chat_sendclears and replaces the composer, but this decision only checks availability. If the user has typed a draft while the workflow was waiting, the next send silently destroys it. Fail closed when the freshly observed composer is non-empty (or explicitly reconcile its exact contents before submission).
if (snapshot.composer.available !== true) {
return { ok: false, reason: 'composer_unavailable', error: 'The active conversation composer is not available.' };
}
src/chrome/src/content/chat-observation.js:382
- Mandatory: this URL-only fallback is not an exact conversation binding. A support widget or SPA can switch to another case while keeping the same URL and no exposed identity, so
threadKeyremains unchanged and the send-time drift check authorizes the wrong thread. Fail closed unless a conversation-specific marker or independently verified identity is available, or add an adapter-provided stable thread identity.
const threadKey = compact(
conversationId
? `dom:${conversationId}`
: (url && conversationIdentity ? `${url}#${conversationIdentity}` : url || conversationIdentity || 'document'),
240,
src/firefox/src/content/chat-observation.js:382
- Mandatory: this URL-only fallback is not an exact conversation binding. A support widget or SPA can switch to another case while keeping the same URL and no exposed identity, so
threadKeyremains unchanged and the send-time drift check authorizes the wrong thread. Fail closed unless a conversation-specific marker or independently verified identity is available, or add an adapter-provided stable thread identity.
const threadKey = compact(
conversationId
? `dom:${conversationId}`
: (url && conversationIdentity ? `${url}#${conversationIdentity}` : url || conversationIdentity || 'document'),
240,
src/chrome/src/agent/chat-workflow.js:180
- The fallback IDs are not stable across the 200-message sliding window because occurrence numbering restarts after truncation. For example, snapshots containing 200 and then 201 identical no-ID messages both normalize to the same occurrence IDs 0–199, so the newly appended message produces no delta. Preserve a stable per-bubble identity before truncation or reconcile no-ID messages against prior snapshots.
for (const raw of (Array.isArray(source.messages) ? source.messages : []).slice(-MAX_MESSAGES)) {
const provisionalText = canonicalChatText(raw?.text ?? raw?.content ?? raw?.message);
const provisionalDirection = normalizeDirection(raw?.direction ?? raw?.authorRole ?? raw?.role);
const occurrence = seenOccurrences.get(`${provisionalDirection}\u001f${provisionalText}`) || 0;
seenOccurrences.set(`${provisionalDirection}\u001f${provisionalText}`, occurrence + 1);
src/firefox/src/agent/chat-workflow.js:180
- The fallback IDs are not stable across the 200-message sliding window because occurrence numbering restarts after truncation. For example, snapshots containing 200 and then 201 identical no-ID messages both normalize to the same occurrence IDs 0–199, so the newly appended message produces no delta. Preserve a stable per-bubble identity before truncation or reconcile no-ID messages against prior snapshots.
for (const raw of (Array.isArray(source.messages) ? source.messages : []).slice(-MAX_MESSAGES)) {
const provisionalText = canonicalChatText(raw?.text ?? raw?.content ?? raw?.message);
const provisionalDirection = normalizeDirection(raw?.direction ?? raw?.authorRole ?? raw?.role);
const occurrence = seenOccurrences.get(`${provisionalDirection}\u001f${provisionalText}`) || 0;
seenOccurrences.set(`${provisionalDirection}\u001f${provisionalText}`, occurrence + 1);
src/chrome/src/agent/agent.js:17816
- The recipient preflight is missing the text that the eventual
set_fielddispatch will submit._probeMessageRecipientGuardderivesmessageBodyfromargs.textfor submit-scopedset_field; with the normal empty composer this call therefore produces an empty body and fails verification, sochat_sendcannot work on any adapter with recipient guarding enabled. Preflight the exact dispatch arguments.
src/firefox/src/agent/agent.js:15565 - The recipient preflight is missing the text that the eventual
set_fielddispatch will submit._probeMessageRecipientGuardderivesmessageBodyfromargs.textfor submit-scopedset_field; with the normal empty composer this call therefore produces an empty body and fails verification, sochat_sendcannot work on any adapter with recipient guarding enabled. Preflight the exact dispatch arguments.
src/chrome/src/agent/chat-workflow.js:327 - Clearing
pendingOutboundon thread drift discards the only durable record of an uncertain send exactly when its result has not been verified. After re-binding the original thread, the same text can be dispatched again even if the first send succeeded. Preserve the pending record keyed to the original thread and require explicit reconciliation before allowing a rebind or retry.
state: transitionChatState(session.state, { type: 'thread_changed' }),
userInput: { required: true, reason: 'thread_changed', message: 'The active conversation changed. Select and verify the intended thread before continuing.' },
stopReason: 'thread_changed',
pendingOutbound: null,
};
src/firefox/src/agent/chat-workflow.js:327
- Clearing
pendingOutboundon thread drift discards the only durable record of an uncertain send exactly when its result has not been verified. After re-binding the original thread, the same text can be dispatched again even if the first send succeeded. Preserve the pending record keyed to the original thread and require explicit reconciliation before allowing a rebind or retry.
state: transitionChatState(session.state, { type: 'thread_changed' }),
userInput: { required: true, reason: 'thread_changed', message: 'The active conversation changed. Select and verify the intended thread before continuing.' },
stopReason: 'thread_changed',
pendingOutbound: null,
};
- Files reviewed: 18/18 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const prior = this.chatSessions.get(tabId) || createChatSession(); | ||
| const observed = advanceChatSession(prior, before); | ||
| this.chatSessions.set(tabId, observed.session); | ||
| this._persist(tabId); | ||
| const workflow = () => this._chatWorkflowView(observed); |
| // A dispatch that never produced a visible bubble must not block the same text | ||
| // forever. `attemptedAt` ages the pending record out so the workflow can retry. | ||
| const PENDING_OUTBOUND_TTL_MS = 10 * 60 * 1000; |
| const collectMessages = (root, composer) => { | ||
| const candidates = queryMany(root, MESSAGE_SELECTORS) |
| const prior = this.chatSessions.get(tabId) || createChatSession(); | ||
| const observed = advanceChatSession(prior, before); | ||
| this.chatSessions.set(tabId, observed.session); | ||
| this._persist(tabId); | ||
| const workflow = () => this._chatWorkflowView(observed); |
| // A dispatch that never produced a visible bubble must not block the same text | ||
| // forever. `attemptedAt` ages the pending record out so the workflow can retry. | ||
| const PENDING_OUTBOUND_TTL_MS = 10 * 60 * 1000; |
| const collectMessages = (root, composer) => { | ||
| const candidates = queryMany(root, MESSAGE_SELECTORS) |
Two stale-state bugs from review of webbrain-one#2982, rebuilt onto the author's newer Copilot-review base (249a927, 6f15704) so the fixes survive both the observer's truncation and the removed TTL. The fallback id for a bubble the page does not identify is positional, and both layers numbered occurrences after truncating: the observer after its 200-item cap, the kernel after its own. The numbering restarts at 0 as older bubbles age out, so past the cap a genuinely new duplicate normalizes onto an id seenMessageIds already holds and produces no delta. The observer now assigns the ordinal over the whole deduped transcript before its cap and reports it; the kernel prefers a supplied ordinal and otherwise counts before its own cap. The Copilot review moved the kernel to count the full array, but the observer still slices before the kernel sees it, so the DOM path needs the observer change; the kernel preference keeps an observer-supplied ordinal from being recounted. pendingOutbound.key embeds the reply anchor the message was sent under, but outgoingMessageKeys recomputes that anchor from DOM order. A counterparty message that renders after a send can move the anchor so the recomputed key never matches; without the TTL, the pending record then claims the send indeterminately and blocks every later dispatch. A newly visible outgoing bubble carrying the pending text proves delivery without depending on the anchor, so advanceChatSession now clears the pending record on that too. Explicit reconciliation stays available for the genuinely-uncertain case. Tests: three assertions in the workflow block (anchor drift, duplicate ids past the cap, observer-assigned ordinal honored) and one in the observation block (numbering before the cap). Each fails on the author's head and passes with the fix; the over-cap kernel delta is already handled by the author's kernel and is kept as regression coverage.
|
Rebuilt the two outstanding review fixes from 1. Fallback ids past the retention cap. The Copilot round moved the kernel to count occurrences over the full transcript, which fixes sources that feed >200 raw messages. But the observer still slices to its 200-item cap before numbering ( 2. Anchor drift on the pending record. I kept the Copilot base's decisions (no TTL, block on any pending, reconcile tool, composer-empty guard, strict thread identity) and layered only these two fixes. Each new assertion was verified to fail on the pre-rebase head and pass now; the kernel-side duplicate-delta case is kept as regression coverage since the author's kernel already handles it. Separate CI heads-up: the tab-chat persistence suite still hits |
…-undelivered send Code review of the chat_observe / chat_send kernel turned up several ways a session could get stuck in a state only an explicit user reconciliation could clear, plus a pause that could be escaped without the rebind the tool contract requires. - _sendChatWorkflow left the pending record in place when the pre-dispatch persist failed, even though nothing had been dispatched. Every later send then failed with send_pending, making the error's own "retry after storage is available" advice impossible to follow. Restore the non-pending session and report the delta the call already consumed. - A dispatch that proves it never reached the page (ref not found, content action timeout, thrown error) now clears pendingOutbound before the verification observation instead of stranding the workflow. - advanceChatSession lifts a thread_changed pause only when the bound threadKey is observed again. Previously any counterparty_replied or outgoing_verified event moved the state out of needs_user_input while stopReason stayed thread_changed forever, leaving the rebind precondition permanently satisfied so the session could be reset at any later point. - chat_send and the rebind path compare against the normalized threadKey that chat_observe reported, not the raw content-script value. bounded() maps control characters to spaces and compact() does not, so a conversation id containing one made both permanently unusable on that page. - The failed-after-observation branch reports the session's own threadKey and resolutionEvidence rather than reading them off the failure object, which is exactly when the model needs them to reconcile an uncertain send. - collectMessages collapses ancestor/descendant message nodes on containment plus text containment, so a wrapper and its bubble that each carry an id, or a wrapper whose innerText adds a timestamp, no longer report twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FH7NV9Zop2f6JPka8V1h5g
|
Thanks for rebuilding these fixes on top of the current head and preserving the newer commits. The two fixes make sense to me:
I also agree that the ~6 MB image Thanks for the thorough review. |
Summary
Closes #2981.
Adds a durable, event-driven support chat workflow for long-running conversations:
chat_observeandchat_sendtools with exact thread/composer binding, fresh pre-send observation, recipient guard integration, and independently verified outgoing bubbles.schedule_resumedurable for chat waits and guides the model to resume after 60–120 seconds usingchat_observeand only the new-message delta.Implementation was split into six focused commits on this single PR.
Verification
npm test2203 passed, 0 failed60/6011 passed, 0 failednpm run build:zip(Chrome, Edge, Firefox)