Sequence tools after same-message patches; report tool failures to the room

[jurgenwerk]

When generating cards using AI assistant, it sometimes happened that the "show card" tool request coming right after a code patch just kept on spinning and never completes. The fix is for the tool to wait until the patches are done applying.

Claude's detailed description:

Fixes the stuck-forever tool spinner seen when the assistant sends code patches and a tool request in one message. The tool (typically show-card for the card a patch creates) executed while the patches were still being applied, could hang on a card that was not loadable yet, and nothing ever reached the room — the spinner never cleared and the bot waited forever.

message with patches + tool request
  → tool drain requeues until the message's patches settle
  → execute bounded by a timeout (hang → error)
  → error/timeout posts a 'failed' tool result event
      → spinner clears, bot reacts, UI offers Retry

The 'failed' status already existed in the wire schema and the prompt builder; the host just never sent it. Three acceptance tests cover the sequencing (spying the patch status at the moment the tool executes), the failure event, and the timeout; the sequencing test is mutation-verified against the unguarded drain.

[github-actions]

Preview deployments

• Host staging preview

• Host production preview

Host Test Results

253 tests   251 ✅  6m 2s ⏱️
  1 suites    2 💤
  1 files      0 ❌

Results for commit 4a9c795.

Realm Server Test Results

    1 files  ±0      1 suites  ±0   15m 53s ⏱️ -51s
2 173 tests ±0  2 173 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 253 runs  ±0  2 253 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 4a9c795. ± Comparison against earlier commit 6172636.

[Copilot]

Pull request overview

Sequences tool execution after same-message patches and reports failures through Matrix events to prevent stuck tool spinners.

Changes:

• Defers tools until code patches and indexing settle.
• Adds execution timeouts and failed result events.
• Adds failure UI and acceptance coverage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/host/app/services/tool-service.ts	Implements sequencing, timeouts, and failure reporting.
packages/host/app/lib/matrix-classes/message-tool.ts	Adds the failed tool status.
packages/host/app/components/matrix/room-message-tool.gts	Displays room-reported failures with Retry.
packages/host/tests/acceptance/code-patches-test.gts	Tests sequencing, failures, and timeouts.
packages/host/tests/acceptance/tools-test.gts	Updates retry-event assertions.

Suppressed comments (1)

packages/host/app/services/tool-service.ts:988

• This catch also handles failures after the tool has already succeeded. Once line 952 adds the request to executedToolRequestIds, a failure in skill refresh, context collection, or sending the applied event lands here and publishes failed; however, Retry immediately returns at lines 878-880 because the request is recorded as executed. This leaves the room reporting failure even though side effects completed, with a nonfunctional Retry action. Separate execution failures from post-execution/result-publication failures and retry publishing the applied result rather than rerunning or marking the tool failed.

      try {
        await this.matrixService.sendToolResultEvent({
          roomId: command.message.roomId,
          invokedToolFromEventId: eventId,
          toolCallId: commandRequestId!,
          status: 'failed',
          failureReason: error.message,
        });

---

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a9c795106

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Prevent timed-out tools from continuing alongside retries

When a side-effecting tool takes longer than the 120-second bound, Promise.race rejects but performTool() continues running; the catch then marks the call failed, clears currentlyExecutingToolRequestIds, and exposes Retry. If the user retries before the original operation settles, both executions can mutate state, potentially duplicating custom-tool actions or overwriting a card after the successful retry. The timeout needs cancellation or another guard that prevents retry while the original execution remains live.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Clear stale collision redirects for later direct patches

When one create patch collision-renames foo.gts, this map permanently records foo.gts -> foo-1.gts. If a later message normally edits the original foo.gts, finalFileIdentifier === fileUrl, so this block leaves the old redirect intact; the subsequent invalidation wait looks under foo-1.gts instead of the newly tracked foo.gts entry and lets that message's tools race indexing again. Remove the previous mapping when the write lands at the requested URL, or consume redirects after their corresponding wait.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid blocking every room on one invalidation wait

If a .gts or .json patch's index event is delayed or lost, this awaited Promise.all holds the single serialized drainToolProcessingQueue for the full ENV.cardRenderTimeout (30 seconds by default). Tools already queued—or queued afterward—in unrelated rooms cannot be processed during that interval because subsequent drains await flushToolProcessingQueue, so one realm's indexing problem stalls tool execution globally. Perform these per-message waits without blocking the shared drain, or requeue only the affected message.

Useful? React with 👍 / 👎.

[habdelra]

[Claude Code 🤖] This change is a concurrency/ordering fix in a shared drain plus a new terminal status on the wire, so I reviewed it along three axes: whether the sequencing actually orders what it claims to, what the new failed event means to every consumer that reads tool results, and what the new awaits and timeouts do to the drain that all rooms share. Everything below was traced against the checked-out branch.

Bottom line: the diagnosis is right and the three-layer structure is the correct shape for it — no blocking issues. Four things I'd act on: one regression where a post-success bookkeeping failure now publishes failed for a write that landed, one availability regression where the new index wait parks the shared drain (with a latent 30 s test trap alongside it), one narrow stale-redirect bug, and two comments that claim more than the code does.

What lands right

• failed really is terminal now. All three auto-execution gates agree — the ready-tool loop, invalidateAutoExecutableToolsForStuckProcessing, and messageHasUnresolvedTools. A reload that replays room history therefore cannot re-run a failed side-effecting tool unattended. This closes the automated finding from the earlier commit, which is now outdated.
• The bot really does un-stick. getShouldRespond in packages/runtime-common/ai/prompt.ts counts any tool result event with a recognised msgtype as resolving its request — status is not consulted. So a failed result releases the wait exactly as the description claims; I traced it rather than assuming it.
• Latest-result-wins is load-bearing and lands on both consumers. toResultMessages and gatherPatchedCards were the two places where a stale failed would have outlived a successful retry, and getToolResults preserves history order so findLast means what it should. The neighbouring predicates correctly need no change: allRelevantToolsResolved wants any result, hasAppliedChanges wants specifically applied.
• Gating the patch wait on act mode is right, not a gap. drainCodePatchProcessingQueue only auto-applies in act mode, so waiting outside it would stall tools on patches nobody is applying.
• Excluding blocks with no resolvable file URL from "unsettled" is correct — executeReadyCodePatches skips them, so they never leave ready and would otherwise burn the whole retry budget waiting on nothing.
• 'failed' is already a first-class ApplyButtonState with its own indicator, and MessageTool.status reports 'applying' while a retry is in flight, so the new alert branch can't linger over a running retry.

Recommendations

1. Don't report failed for a tool that already executed — see the thread on the catch block in tool-service.ts; a didExecute flag and a best-effort applied re-send is the smallest fix.
2. Don't hold the shared drain on the index wait, and give it an isTesting() bound — see the thread on the Promise.all over patchedFileUrls; the requeue machinery directly above it is the drop-in pattern.
3. Clear patchedFileRedirects when a write lands at the requested URL — one-line suggestion in that thread.
4. Reword the checkCorrectness headroom comment: the tool takes one invalidation window, not two.
5. Extend the withTimeout comment to state that a timed-out execute may still commit, so Retry can double-apply; real cancellation is a follow-up, not this PR.
6. Add a .gts variant of the sequencing test — the index-wait layer currently has no coverage at all, for the reason explained in that thread.

Adjacent, out of scope

• Cross-message sequencing is still open. The wait is derived from the message's own patch blocks, so if the bot lands patches in one message and the show-card tool in the next one, patchedFileUrls is empty for that second message and its tools run immediately — even though the invalidation for the just-patched file is still outstanding in aiAssistantInvalidations. Worth checking the field traces to confirm the observed shape really is always same-message; if it isn't, keying the wait on live room-level invalidation entries (bounded) would cover both shapes with one rule.
• patchedFileRedirects has no cap. resetState clears it and nothing else prunes it. With recommendation 3 applied it stays bounded in practice, but aiAssistantClientRequestIdsByRoom already gets a LimitedSet for the same reason; matching that would be consistent.
• RoomResource.sortedEvents sorts matrixRoom.events in place. get events() hands back the live _events array and .sort() mutates it, behind a @cached getter. Harmless for this PR — both orders put the newest result last, which is why the new newest-first scan is safe — but it means "insertion order" is not a property anything downstream can rely on. For whoever touches that getter next.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] This catch also covers everything that happens after the tool succeeded, so a bookkeeping failure now publishes failed for a call whose side effects landed.

The mechanism. In run, once performTool() resolves the task keeps going inside the same try: executedToolRequestIds.add(commandRequestId) → matrixService.updateSkillsAndToolsIfNeeded(roomId) → operatorModeStateService.getSummaryForAIBot() → sendToolResultEvent({ status: 'applied' }). Three of those four do network work. updateSkillsAndToolsIfNeeded in matrix-service.ts re-loads every enabled skill source through the store, re-uploads the changed ones, and writes a room state event — a transient realm or matrix error there throws after the tool's write has already committed, and lands right here.

Why it matters more than it used to. Before this PR that path set failedToolState locally and sent nothing; the room saw no terminal event (the hang this PR is removing). Now the room gets status: 'failed' with that error's message, and getShouldRespond in packages/runtime-common/ai/prompt.ts counts any tool result as resolution — so the bot wakes up, is told the call failed, and can reasonably re-issue the same tool against an effect that already exists.

The Retry the UI offers alongside it is inert in this specific case: run's top guard returns early because executedToolRequestIds already holds the id, and that guard sits above failedToolState.delete, so the alert doesn't even clear. That half is pre-existing — publishing failed to the room for a committed write is what's new.

The way out. Separate "the tool never ran" from "the tool ran, the aftermath failed":

let didExecute = false;
// …
[resultCard] = await all([...]);
didExecute = true;
this.executedToolRequestIds.add(commandRequestId!);

and in this catch, when didExecute is true, best-effort re-send status: 'applied' instead of 'failed'. The room still gets its terminal event — which is the whole point of the change — but a truthful one. If even that send fails, logging and leaving the local failed state is closer to correct than telling the model a committed write failed.

Scope. Regression introduced here (layered on a pre-existing dead-Retry). Non-blocking for the main fix, but it's the one path where the new event actively misinforms the model, so I'd fold it in.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] Two things about this wait — the second one will bite a future test author rather than a user.

The drain is global, so this await does stall every room. The comment says the parallel-and-conditional shape avoids that, but neither mitigation touches the case that matters. toolProcessingEventQueue holds ${roomId}|${eventId} for every room (queueEventForToolProcessing), the enclosing while (toolSpecs.length > 0) walks that mixed list one entry at a time, and every later drain pass begins with await this.flushToolProcessingQueue. The Promise.all here is parallel only across one message's own files, and messageHasUnresolvedTools only skips messages with nothing runnable. A single message whose incremental index event is late or lost parks tool processing for all rooms for the full ENV.cardRenderTimeout — 30 s by default (packages/host/config/environment.js) — including rooms with no patches at all. Either the code or the comment should change; right now the comment reads as if the hazard were handled.

The bound has no test arm. Every sibling constant in this file is shortened under isTesting() — STUCK_PROCESSING_TIMEOUT_MS, MAX_TOOL_FINALIZATION_RETRIES, MAX_TOOL_PATCH_WAIT_RETRIES, TOOL_EXECUTE_TIMEOUT_MS. ENV.cardRenderTimeout is not: it resolves to DEFAULT_CARD_RENDER_TIMEOUT_MS (30 000) in the test build too, and nothing in the host test config or CI sets RENDER_TIMEOUT_MS. This wait runs inside toolProcessingWaiter.beginAsync, so the first acceptance test that patches a .gts/.json in an act-mode message carrying a tool request, without producing a matching incremental index event, blocks settled() for 30 s and presents as a hang or a flake rather than a failed assertion.

Suggested shape. Reuse the requeue machinery immediately above instead of awaiting. aiAssistantInvalidations already stores a record per key, so setting a flag next to each deferred.fulfill() — the subscribe callback in trackAiAssistantCardRequest and the five-minute sweep — makes "has this landed?" a synchronous question:

if (
  patchedFileUrls.length > 0 &&
  this.messageHasUnresolvedTools(message) &&
  this.hasPendingPatchInvalidations(roomId!, patchedFileUrls)
) {
  // push compoundKey back, debounce, continue — exactly the patch wait above
}

That keeps the ordering guarantee, bounds it with the same retry budget, and never holds the shared drain. If you'd rather keep the await, give it its own constant with an isTesting() arm and a much smaller production value, and reword the comment to say what the code actually does.

Scope. Regression (latency/availability, plus a latent test trap), non-blocking. An automated reviewer flagged the cross-room stall; the call chain above is the verification of it.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] The redirect is written but never cleared, so a later patch that lands at the requested URL keeps consulting a stale rename — and silently loses the index wait this PR adds.

Why the map exists (for anyone reading this cold): PatchCodeTool.run registers the invalidation under finalFileIdentifier, i.e. where the write actually landed after determineFinalFileUrl may have collision-renamed a create. The drain, meanwhile, only knows codeData.fileUrl — the URL written in the code block. Hence this bridge.

The failure. Once a create-collision records …/foo.gts → …/foo-1.gts, the entry survives until resetState(). A later message that edits foo.gts normally has finalFileIdentifier === fileUrl, so this branch doesn't run and the old entry stays. waitForInvalidationAfterAIAssistantRequest then rewrites the key to …::…/foo-1.gts — whose aiAssistantInvalidations entry was deleted the moment the first wait succeeded — existing is undefined, and the function returns immediately. The second message's tools run with no index wait at all, which is the race this PR exists to close.

The fix, mechanical:

Suggested change

	if (finalFileIdentifier && finalFileIdentifier !== fileUrl) {
	// The invalidation tracker keys on where the write actually landed;
	// waits keyed on the requested URL resolve through this redirect.
	this.patchedFileRedirects.set(
	this.invalidationKey(roomId, fileUrl),
	finalFileIdentifier,
	);
	}
	let requestedKey = this.invalidationKey(roomId, fileUrl);
	if (finalFileIdentifier && finalFileIdentifier !== fileUrl) {
	// The invalidation tracker keys on where the write actually landed;
	// waits keyed on the requested URL resolve through this redirect.
	this.patchedFileRedirects.set(requestedKey, finalFileIdentifier);
	} else {
	// The write landed at the requested URL, so a redirect left by an
	// earlier collision-rename of this file no longer describes it.
	this.patchedFileRedirects.delete(requestedKey);
	}

Scope. Regression, narrow (needs a create-collision followed by a direct edit of the same path in the same room), non-blocking. Raised by an automated reviewer; the call chain above is the verification.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] Confirmation plus one thing the comment should say out loud.

What's right, and worth not breaking later. Promise.race attaches handlers to both promises immediately, so a performTool() rejection arriving after the timeout already won is still handled — no stray unhandled rejection escapes to the test suite or the console. And clearTimeout in the finally means the loser's timer can't fire after a normal win, which is what keeps the isDestroying guard in run's catch from being load-bearing in the common case. Nothing is emitted when the abandoned execute eventually settles either: the race has already settled so its value is dropped, and executedToolRequestIds was never written, so there is no risk of a second result event for the same request.

What remains. The abandoned execute is still live and its side effects still land. run's finally clears currentlyExecutingToolRequestIds and the new failure UI offers Retry, so a merely-slow-but-eventually-successful tool can commit twice — an idempotent re-write of the same attributes for patchCardInstance, but a genuine duplicate for a custom tool that creates something. Both automated reviewers flagged this and both are right about the mechanism.

I'd still keep the timeout: the alternative is the stuck spinner and the indefinitely-waiting bot that this PR exists to remove, and that is strictly worse. Two things worth doing instead of changing the design here:

1. Extend this comment past "we cannot cancel it" to name the consequence — that a timed-out execute may still commit, and that Retry can therefore double-apply. The next person to read this needs that sentence before they decide whether their tool is safe to auto-retry.
2. Follow-up, not this PR: thread an AbortSignal through Command.execute so the timeout can actually cancel, at which point Retry is unambiguous.

Scope. Confirmation + follow-up. Non-blocking.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] The constant is safe; the reason given for it isn't what the code does.

CheckCorrectnessTool.run takes exactly one invalidation wait per invocation, not two. collectModuleErrors (target type file ending in .gts) and collectCardErrors (target type card, including a .json re-classified by checkIfFileIsACardInstance) are the two arms of a single if / else if, and each calls waitForInvalidationAfterAIAssistantRequest once with cardIndexingTimeout — which is ENV.cardRenderTimeout (packages/host/app/tools/check-correctness.ts). So 2 * ENV.cardRenderTimeout is double a window that is only ever entered once.

The headroom is still fine — generous rather than wrong — because what actually consumes the rest of the budget is the work after the wait: prerenderModule, or refreshCard with its two store.waitForCardLoad calls plus getWithoutCache. Suggest rewording to name that, e.g. "one index-invalidation window plus the prerender/refresh work that follows it", so the arithmetic stays checkable against the tool.

Scope. Comment accuracy, non-blocking.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] Confirmation — the ordering this scan depends on holds under both states the array can be in, which is not obvious from here.

Room.addEvent (packages/host/app/lib/matrix-classes/room.ts) appends to _events in arrival order and replaces edits in place, so within a room a failed result precedes its applied retry positionally. Separately, RoomResource.sortedEvents calls .sort((a, b) => a.origin_server_ts - b.origin_server_ts) on that same array — Array.prototype.sort mutates in place, so once anything has read sortedEvents the array is also timestamp-ascending. Both orders put the newest matching result last, so walking backwards picks the right one either way. Also good that the loop now uses e.content?.[...] — the old .find dereferenced e.content unguarded on every event in the room.

Two notes for whoever edits this next:

This is the rebuild path, not the live one. When a result event arrives live, updateMessageCommandResult takes the builderContext.toolResultEvent fast path above and assigns toolCallStatus unconditionally, so the last event to arrive wins there. That agrees with this scan only because both now mean "latest wins" — if either side ever flips back to first-wins, the live view and the post-reload view of a retried tool diverge with nothing failing.

Which makes the pair worth pinning. The new packages/ai-bot/tests/prompt-construction-test.ts case covers the ai-bot half of latest-wins; the host half has no equivalent. A rendering or unit test that builds a message from a history containing failed then applied for one request and asserts messageTool.status === 'applied' (and no stale failureReason) would cover the reload path this scan owns.

Scope. Confirmation + suggested test. Non-blocking.

---

Generated by Claude Code

[habdelra]

[Claude Code 🤖] This test pins the first sequencing layer but is structurally blind to the second one — the harder half of the change.

The patch targets hello.txt, and trackAiAssistantCardRequest returns before registering anything when the file is neither .gts nor .json. So no invalidation entry is ever created for this message, patchedFileUrls yields one URL but waitForInvalidationAfterAIAssistantRequest finds no existing and returns on its first statement. What the assertions therefore prove is exactly the requeue-until-patch-status-settles loop — which they prove well: spying store.patch and reading getCodePatchStatus at the moment of execution is a much stronger assertion than event ordering alone, and the patchResultIndex < toolResultIndex check backs it up.

What is left unexercised is everything added in the index-wait layer: the Promise.all over patchedFileUrls, the patchedFileRedirects lookup, and the rule that a timed-out wait must not consume the deferred (so checkCorrectness can still wait on it). Those are the subtlest parts of the diff and none of them run in any test in this file. A .gts variant of this same test — patch a .gts, assert the tool executed only after the tracked incremental index event fired — would pin the layer; a second case that lets the wait time out and then asserts a subsequent waitForInvalidationAfterAIAssistantRequest still blocks would pin the non-consumption rule.

Separately, 'failed' being terminal for auto-execution is now enforced in three places (the ready-tool loop, invalidateAutoExecutableToolsForStuckProcessing, and messageHasUnresolvedTools) with no test behind any of them. The regression that guard prevents is specifically a reload replaying room history and re-running a failed side-effecting tool unattended, so a test that rebuilds a room whose history already contains a failed result and asserts no new tool result event is dispatched would be the one that matters.

Scope. Test coverage, non-blocking. Not asking for all of it in this PR — the .gts sequencing case is the one I'd prioritise, since it's the layer with the most moving parts.

---

Generated by Claude Code