fix(remote): make permission approval work from Slack

[srbose]

Problem

Bash tool permission requests were effectively un-approvable from Slack. Four issues combined to make the flow fail:

1. English replies were parsed as deny. The reply parser accepted only 允许 / y / yes / 是 (and 始终允许 / always), so "allow", "approve", and "ok" all denied the request.
2. Silent swallow on delivery failure. permission.request events for remote sessions were consumed unconditionally; when remote handling returned null (missing session mapping, gateway down, send failure) the desktop dialog never appeared and the request silently auto-denied after 60s.
3. The effective window was 60s, not 5 minutes. The session-level 60s auto-deny timer ran independently of the remote flow's 5-minute timer, so users had at most a minute to reply.
4. Thread-channel mismatch. Slack reports threaded channels as C123:threadTs, so replies posted in the main channel never matched a prompt posted inside a thread (strict channel-id equality).

Changes

• src/main/remote/interaction-utils.ts (new, dependency-free): parsePermissionReply accepts y / yes / allow / approve / ok / 1 (allow once), always (allow + remember), with Chinese tokens (是 / 允许 / 始终允许) kept for parity; anything unrecognized denies (fail-closed). normalizeChannelId strips Slack thread suffixes for matching.
• session-manager: permission timers are now deferrable — new deferPermissionTimeout clears and re-arms the auto-deny timer, and handlePermissionResponse/requestPermission settle through one idempotent path.
• remote-manager:
  • English Slack prompt listing the reply options (Feishu keeps its existing Chinese prompt).
  • After the prompt is delivered, the session timer is deferred to the same 5-minute window; expiry posts a notice to the channel before denying.
  • handlePotentialInteractionResponse matches in two passes: exact channel id first, then normalized (thread-suffix-stripped) comparison; owner check and consume-once semantics preserved.
  • clearSessionBuffer settles any pending permission as deny when a session ends, so its promise doesn't hang until the 5-minute timeout.
• index.ts: falls back to the desktop permission dialog when remote handling returns null or throws.

Tests

44 new cases across three files:

• tests/interaction-utils.test.ts — reply parsing (all tokens, case/whitespace/punctuation variants, fail-closed) and channel-id normalization.
• tests/remote-manager-permission-request.test.ts — end-to-end permission flow: allow/always/deny replies, non-owner replies ignored, main-channel replies matching threaded prompts, send-failure cleanup, timer deferral, safe-tool auto-approve, session-end settlement.
• tests/session-manager-permission-timeout.test.ts — 60s default auto-deny, deferral to 5 minutes, reply-after-defer leaves no timer, deferPermissionTimeout returns false for settled/unknown ids.

Verified with npx vitest run on the three new files (44 passed), npm run typecheck, and npm run lint (0 errors).

🤖 Generated with Claude Code

[github-actions]

Findings

• [Major] Normalized channel matching can resolve the wrong pending interaction when multiple permission prompts are pending in different threads of the same Slack channel.
  The new pass 2 in handlePotentialInteractionResponse strips thread suffixes and matches any pending interaction whose channel type + normalized channel id are equal. A thread reply (C123:999.999) can therefore resolve the first pending interaction for a different thread (C123:111.111) or for the main channel; with multiple pending prompts in the same base channel, the first inserted interaction wins even though the reply was not directed at that prompt. The new test matches a reply with a different thread ts via normalized channel id documents this over-broad behavior. This can lead the owner to unintentionally approve a different tool call than the one they are responding to.
  Evidence: src/main/remote/remote-manager.ts:855-864
  Suggested fix: only normalize when at least one side is non-threaded (main-channel reply), since exact matching already handles same-thread replies:

  const isThreaded = (channelId: string) => channelId.includes(':');
  // ...
  if (
    channelInfo.channelType === channelType &&
    normalizeChannelId(channelInfo.channelId) === normalizeChannelId(channelId) &&
    (!isThreaded(channelInfo.channelId) || !isThreaded(channelId))
  ) {
    if (consume(interaction, id)) return true;
  }

• [Minor] The remote prompt advertises a 5-minute session timeout even when the configured agent executor does not implement deferPermissionTimeout.
  If agentExecutor is set elsewhere without that method, handlePermissionRequest proceeds with the session-manager default 60s timer while the Slack notice says 5 minutes, so the owner has far less time than advertised and may get a false deny.
  Evidence: src/main/remote/remote-manager.ts:736-746
  Suggested fix: make deferPermissionTimeout a required method on AgentExecutor, or fall back to using PERMISSION_TIMEOUT_MS in the timeout notice when the defer method is unavailable.

Testing

• The added unit tests cover the parsing and interaction-matching branches, but sendToRenderer fallback behavior in src/main/index.ts (when handlePermissionRequest returns null or throws) is not covered by automated tests. A small test would help protect the fallback-to-desktop path.
• Not run (automation)

Summary

Review mode: initial

Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.

Open Cowork Bot

[srbose]

Addressed the review findings in 5f10719:

[Major] Cross-thread matching — normalized channel matching now only applies when at least one side is non-threaded ((!isThreaded(channelInfo.channelId) || !isThreaded(channelId))); exact matching already covers same-thread replies. A reply posted in a different Slack thread can no longer resolve a pending permission prompt. Tests updated: the cross-thread reply is asserted as not consumed (prompt still resolvable from the correct thread), plus a new case for a threaded reply to a prompt posted in the main channel.

[Minor] 5-minute notice — deferPermissionTimeout is now a required AgentExecutor member, so the advertised reply window is always backed by the session-level timer; the stdio executor gets an explicit no-op stub. The prompt also posts an expiry notice when the session timer has already fired (defer returns false).

Testing — the permission routing previously inlined in index.ts (with its null/throw fallback to the desktop dialog) is extracted into src/main/remote/permission-route.ts, with 7 new unit tests covering allow / allow+remember / deny / null fallback / thrown-error fallback / missing response handler.

Verification: 54 permission-related unit tests pass; tsc --noEmit clean; eslint 0 errors (8 pre-existing warnings in unrelated renderer files).

[github-actions]

Review mode: follow-up after new commits

Findings

• [Major] Main-channel replies can still resolve the wrong pending permission when multiple threads of the same channel have outstanding prompts. The new normalized second pass in handlePotentialInteractionResponse (src/main/remote/remote-manager.ts, around the !isThreadedChannelId(...) condition) accepts a reply posted in the base channel as a match for any threaded prompt in that base channel. If more than one threaded prompt is pending, the first Map entry is consumed, which can approve the wrong tool call. Suggested fix: when a non-threaded reply would match multiple pending threaded interactions for the same base channel, do not consume it; either require the reply to be in the specific thread or ask for clarification.

  // in the normalized pass
  if (!isThreadedChannelId(channelId)) {
    const matches = [...this.pendingInteractions.values()].filter(
      (i) =>
        i.channelInfo.channelType === channelType &&
        normalizeChannelId(i.channelInfo.channelId) === normalizeChannelId(channelId)
    );
    if (matches.length > 1) continue; // ambiguous; do not auto-consume
  }

• [Minor] clearSessionBuffer resolves pending question interactions with the raw string '{}' instead of the parsed empty answer. Other question paths call parseQuestionResponse(...) before invoking the resolver, so the agent can receive a literal {} string instead of an empty object when a question is cleared. Suggested fix:

  resolver(
    interaction.type === 'permission'
      ? 'no'
      : this.parseQuestionResponse('{}', interaction.questions || [])
  );

• [Minor] this.agentExecutor?.deferPermissionTimeout(...) remains optional-chained even though AgentExecutor.deferPermissionTimeout is now required. If agentExecutor is undefined, deferred is undefined and the remote flow proceeds without deferring the session-level timer; a session timeout can then fire while the remote prompt is still waiting. Consider failing closed:

  const deferred = this.agentExecutor
    ? this.agentExecutor.deferPermissionTimeout(toolUseId, PERMISSION_TIMEOUT_MS)
    : false;
  if (deferred === false) { /* deny + notify */ }

Questions

• Is RemoteManager ever instantiated with stdioAgentExecutor for real permission prompts? If so, deferPermissionTimeout: () => false in src/main/index.ts would cause every remote permission request to be denied immediately because the code treats false as “session timer already fired.”

Summary

• Review mode: follow-up after new commits
• Review policy: advisory — the check reflects automation health/completion only; it does not approve the PR or resolve findings.
• The previously reported issues (normalized thread matching and the optional deferPermissionTimeout fallback) are materially addressed in this revision. The remaining concerns above are edge cases introduced or left open by the new matching/teardown logic.

Testing

• Suggested: add a unit test with two pending threaded interactions in the same base channel and a base-channel reply; assert no interaction is consumed (or a clarification is requested).
• Suggested: add a unit test for clearSessionBuffer with a pending question so the resolved value is asserted to equal parseQuestionResponse('{}', questions), not the raw '{}' string.
• Existing coverage for permission parsing and routing looks reasonable.

Open Cowork Bot