Skip to content

fix(studio): unify verdict ledger and guard late side-thread verdicts - #2143

Merged
kantord merged 1 commit into
mainfrom
issue-2142
Oct 7, 2026
Merged

kantord merged 1 commit into
mainfrom
issue-2142

Conversation

@kantord

@kantord kantord commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

Follow-up to the #1975 review (#2142): the side thread and the main workspace answered the same permission and plan asks with diverging verdict bookkeeping. This PR fixes the cheap, clearly-correct items and leaves the contested refactors for later.

  • VerdictLedger (verdict-ledger.ts): one phase per ask (idle → inFlight → acknowledged | uncertain) replaces the side panel's three parallel Sets, which could hold contradictory states. A second begin() on a non-idle ask is refused, so a double-click sends one POST and an uncertain ask cannot be retried blindly.
  • Late-verdict guard (bug fix): a side thread can move to a successor session without remounting. A verdict that landed after the move still set notices/errors, retracted approvals, reattached a run, and cleared the new session's controlPending. View writes are now skipped when the verdict's session is no longer the viewed one; ledger writes stay unconditional. controlPending is also reset on session change.
  • Plan verdict errors in the workspace now use the same "Could not confirm this verdict: …" prefix as ordinary asks (no test pinned the bare text).
  • messageOwnsApproval / unmatchedApprovals in chat-state.ts replace the message.tools?.some(approvalMatchesToolCall) expression copied at 7 sites.
  • plan-continuation.ts: hand-rolled AbortController/deadline/Promise.race replaced with AbortSignal.any([...]). The retry loop no longer sleeps after an abort (the existing bounded-stream test caught this).
  • chat-escape.ts: dropped the redundant focusin branch; disarm is hoisted (stable useCallback) and reused by the second effect.
  • Docs/CI: gave the orphaned "Review authorization" paragraph its own heading; removed STUDIO_REQUIRE_MECATED=1 from the Playwright step (only the integration task reads it).
  • Server: failed_precondition deliberately stays in the stale set of resolvePlanAsk, with a comment saying why (see reviewer notes).

Issue relationship

Relates to #2142

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

Baseline checks

  • Linting (task lint): not run; only Studio changed. Studio biome check and tsc are clean.
  • Fast offline test suite (task test): not run; no Go changes. Studio suites pass (web 596, server 173).
  • Pre-review race suite (task test:race): not run; no Go changes.
  • Offline demo (go run ./cmd/mecademo): not run; no Go changes.
  • Markdown changed: docs generation/link checks (task docs)
  • User docs/user-facing behavior changed: site build (task site:build): not run; heading-only docs change.
  • Guarded engine API affected: compatibility check (task api:check)
  • Intentional engine API change: task api:update + engine/CHANGELOG.md

New unit tests cover the ledger (double begin refused, uncertain stays blocked until reset, asks tracked independently).

Changes

File Change
apps/web/src/features/chat/verdict-ledger.ts (+ test) New single-phase verdict ledger
apps/web/src/features/chat/side-thread-panel.tsx Use ledger; guard late verdicts after a session switch
apps/web/src/features/chat/chat-workspace.tsx Prefix plan verdict error; use messageOwnsApproval
apps/web/src/features/chat/chat-state.ts, chat-transcript.tsx messageOwnsApproval, unmatchedApprovals
apps/web/src/features/chat/plan-continuation.ts AbortSignal.any; no sleep after abort
apps/web/src/features/chat/chat-escape.ts Remove redundant branch; hoist disarm
apps/server/src/mecatl/chat.ts Comment only: why failed_precondition stays stale
user-docs/building/deployment/studio.md Heading for the authorization paragraph
.github/workflows/ci.yml Drop dead env var

User-facing change

A plan or permission verdict that completes after switching a side thread to its successor session no longer shows a stale notice/error or locks the new session's controls. Plan verdict failures in the main chat now read "Could not confirm this verdict: …".

Special notes for reviewers

  • Only the side panel uses the ledger. The workspace still has four Sets (ordinary*/plan* in-flight and uncertain), so the two surfaces still differ on stale-ask feedback (workspace shows an error, side panel is silent) and on when uncertainty is cleared. Porting the workspace is the next step and the largest remaining item.
  • Not done from Studio chat approvals: follow-ups from #1975 review #2142: usePlanVerdictControl, PendingAskCard, splitting chat-workspace.tsx, unifying settledAskKeys with approvalKey (the reducer's sessionId is optional), the stalePlanAsk code split, the remaining test gaps, and checking whether detail()'s compatibility() call adds a per-request round trip.
  • failed_precondition stays stale against the issue's suggestion: the Go server returns it (HTTP 412) when a live run owns its plan continuation, and routes/chat.test.ts requires the 409 stale_run_control mapping.
  • No regression test for the session-switch guard yet; the side-panel harness needs a successor-switch scenario.

Fully or partially written by an AI agent.

🤖 Generated with Claude Code

@kantord
kantord marked this pull request as ready for review October 7, 2026 11:15

@peppescg peppescg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. No blockers. I checked these points:

  • VerdictLedger: the move from three Sets to one phase is equivalent on every path I traced (success: inFlight → acknowledged; throw: uncertain). The one behavior change is that acknowledged now blocks in approvalDisabled, where the old code only blocked on inFlight/uncertain. The ask is already retracted and filtered at that point, so the stricter rule is fine.
  • Late-verdict guard: ledger writes stay unconditional and the keys include sessionId, so a late ack can't poison the successor session. Resetting controlPending on session change closes the lock-up.
  • chat-escape.ts: expiry is only set while armed, so it's safe for disarm() to clear it unconditionally. Removing the focusin branch is behavior-preserving, and latest.current.onHintChange is equivalent to the old options.onHintChange.
  • plan-continuation.ts: on deadline, AbortSignal.any + timeout gives the same uncertain result. The !aborted check before wait(250) is correct.
  • CI: I confirmed STUDIO_REQUIRE_MECATED is read only by server/test/integration/mecated-mock.test.ts (via task studio:test:integration), so dropping it from the Playwright step is correct.

Nits (non-blocking):

  1. verdict-ledger.ts and verdict-ledger.test.ts are the only files in features/chat/ missing the // SPDX-License-Identifier: Apache-2.0 header.
  2. With Promise.race gone, the deadline only works if watchSessionActivity honors the abort signal. The SDK does, and the bounded-stream test covers it, but this is a dependency to keep in mind.
  3. Agree with the follow-ups in the description: add a regression test for the successor-session switch and port the workspace's four Sets to the ledger.

The red checks on the first run come from a cancelled run (Analysis cancelled → Lint coverage gate). A new CI run on the same SHA is in progress.

@kantord
kantord merged commit b93b601 into main Oct 7, 2026
54 of 66 checks passed
@kantord
kantord deleted the issue-2142 branch October 7, 2026 11:47
@toolhive-release-app toolhive-release-app Bot mentioned this pull request Oct 7, 2026
2 tasks

This branch was successfully deployed

1 active deployment
Preview — 8d069671 Deployed Oct 7, 2026 by vercel[bot]
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