Skip to content

Studio chat approvals: follow-ups from #1975 review #2142

Closed as duplicate

Description

@kantord

Follow-ups from the review of #1975 (Studio chat approvals, plan review, Escape). None block merge. Parent: #1848.

Worth fixing (non-blocking)

  • Plan-verdict logic is duplicated and has diverged. chat-workspace.tsx (planUnavailableReason, respondToPlan, separate in-flight/uncertain sets, viewedSessionId guard) vs side-thread-panel.tsx (plan folded into the ordinary path, no uncertain state for plans). After an ambiguous failure, retry/Escape can be allowed on one surface and blocked on the other. Extract one usePlanVerdictControl hook.
  • chat-workspace.tsx is ~3400 lines. Move plan-continuation follow/reattach state into plan-continuation.ts.
  • Pending-ask card ternary is copied at 3 sites (chat-transcript.tsx, chat-workspace.tsx, side-thread-panel.tsx). Extract a PendingAskCard.
  • resolvePlanAsk maps generic failed_precondition to 409 "stale" (apps/server/src/mecatl/chat.ts). Keep only stale_run_control, ask_not_pending, not_awaiting_plan, or log the underlying code.
  • Docs: in user-docs/building/deployment/studio.md, the new Review tool calls and plans heading sits above the unrelated "Review authorization" paragraph.
  • Test gaps: 500ms draft-clear boundary (499/500ms); held Escape (repeat: true) cannot fire ask deny / plan iterate / panel / stop; verdict keys with a dialog or menu open (or state in docs that only Escape applies); a planted secret in ask args/diff.

Nitpicks

  • Add unmatchedApprovals() to chat-state.ts (the message.tools?.some(approvalMatchesToolCall) check is repeated 7 times).
  • Use one key helper: settledAskKeys uses runId\0askId, approvalKey uses a JSON tuple with sessionId.
  • plan-continuation.ts: replace the hand-rolled AbortController/deadline/Promise.race with AbortSignal.any([signal, AbortSignal.timeout(ms)]).
  • chat-escape.ts: drop the redundant focusin branch in changed(); hoist disarm() and reuse it in the second useEffect.
  • stalePlanAsk reuses code stale_run_control, so clients cannot tell a stale plan ask from a stale permission ask.
  • runtime.ts copyCapabilities hardcodes storageMigration: false and derives bash from shell; add a comment or fix the schema. Check detail()'s new client.server.compatibility() call does not add a round trip per request.
  • ci.yml: STUDIO_REQUIRE_MECATED=1 on the Playwright step is read only by mecated-mock.test.ts, not by chat-approval.spec.ts. Drop it or make the spec honour it.
  • Docs: the Escape paragraph duplicates the Keyboard shortcuts page; state once and link.
  • Tests: malformed/missing args at card level, unmatched callId permission ask, double-click sends one POST, confirm useRealTimers in afterEach in chat-escape.test.tsx.

Fully or partially written by an AI agent.

Activity

  1. self-assigned this
    on Oct 7, 2026
  2. kantord commented on Oct 14, 2026

    @kantord
    MemberAuthor

    Merged into #2303: all items were copied there (section B). Closing as a duplicate. Fully or partially written by an AI agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions