fix(permission-broker): scope broker socket by bot id, not threadId alone

[buttonsjasper360-lang]

Summary

Fixes #1017 — a delegated child turn's permission-broker socket can collide with its still-open parent's, breaking the tool-call channel itself (not just the approval card) even with Full access on both bots.

The Claude driver's session/permission-broker resource table (sessions, active maps in server/drivers/claude.ts) is a single process-wide table keyed only on threadId. Reproduced repeatedly live (@Chief → @Cliff delegation, see comments on #1017): when the delegated turn's threadId coincides with the parent's still-open one, both bind attempts collide on the exact same broker socket path. POSIX papers over this — unlinkSync silently steals the socket file out from under the still-listening parent — but Windows named pipes are exclusive and the second listen() just fails, surfacing as permission broker: ... is still held.

Change

• SendTurnInput gains an optional botId field (server/contracts.ts).
• Both call sites that build a turn payload now pass botId: bot.id (server/index.ts).
• permissionSocketPath / brokerSocketCandidates (server/drivers/claude.ts) fold botId into the socket-path digest alongside threadId.

This namespaces the broker socket per bot, so a delegated child can never share a socket with its parent (or any other bot) regardless of the exact mechanism that produces the threadId reuse — the collision is ruled out by construction rather than chased at its source.

Test plan

• pnpm exec tsc -p tsconfig.server.json --noEmit — clean
• pnpm exec vitest run server/drivers/claude.test.ts — 104 passed, 1 skipped (pre-existing, unrelated)
• pnpm exec vitest run server/index.test.ts server/delegations.test.ts server/drivers — 1122 passed, 1 skipped
• Added a regression test asserting two bots sharing a threadId get distinct broker paths

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added support for structured questions with selectable options and saved answers.
  • Added improved approval controls, including thread-aware approval modes and support for editing approvals.
  • Added bot-specific routing for turn processing.

• Bug Fixes

  • Prevented permission and broker communication conflicts when bots share conversation thread identifiers.
  • Improved reliability for concurrent parent and delegated bot actions.
  • Prevented unsupported or invalid approval settings from being applied.

[vercel]

@buttonsjasper360-lang is attempting to deploy a commit to the SupaMaus Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 71c0b12f-f924-4427-a60d-f75c6bab5a3f

📥 Commits

Reviewing files that changed from the base of the PR and between 3e4a3f4 and 97449e3.

📒 Files selected for processing (4)

• server/contracts.ts
• server/drivers/claude.test.ts
• server/drivers/claude.ts
• server/index.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR propagates botId into Claude turns, namespaces permission-broker sockets, validates approval grants, separates structured questions from permission requests, persists answers, and updates authentication and approval-mode handling.

Changes

Claude turn and approval handling

Layer / File(s)	Summary
Propagate bot identity into turns server/contracts.ts, server/index.ts, server/drivers/claude.ts	SendTurnInput accepts optional botId. Server dispatch paths pass the bot ID to Claude turns.
Namespace broker socket paths server/drivers/claude.ts, server/drivers/claude.test.ts, server/steer-unattended.e2e.test.ts	Deterministic and fallback broker paths include botId. Tests cover distinct paths for shared thread IDs and bot-aware broker connections.
Validate approvals and structured questions server/index.ts, server/drivers/claude.test.ts	Approval grants validate thread state, provider driver, busy state, and approval mode. Structured questions use questionRequest, and answered text is persisted.
Update authentication and approval settings server/drivers/claude.test.ts, server/index.ts	Authentication tests cover credential isolation and rotation. Thread settings accept edits and reject unsupported provider modes.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: milind-soni

Merge Risk: ⚪ Minimal · up to 46f28

Bot-specific broker paths are propagated through both turn-dispatch paths, with regression coverage for shared-thread bot collisions. No unresolved merge-blocking risk was identified.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue #1017 requires recovery from permission-broker leaks after failed, timed-out, or cancelled delegated turns. It also identifies approval handling that uses the broker's returned socketPath as a…	Implement and test broker cleanup for delegated-turn failure, timeout, and cancellation, or make approval-card handling use the broker's returned socketPath. Add regression coverage for the reported same-bot retry scenario. Retain the bot…
Out of Scope Changes check	⚠️ Warning	The socket namespace changes and their collision test support issue #1017. The change summary also identifies unrelated changes: authentication behavior and authentication tests, AskUserQuestion car…	Remove the unrelated authentication, AskUserQuestion, and thread-settings changes from this pull request, or move them to separate pull requests.
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: scoping permission-broker sockets by bot ID instead of thread ID alone.
Description check	✅ Passed	The description clearly explains the problem, implementation, reason, and verification results. It is mostly complete, although it does not use the template headings exactly and does not include the c…

Full details: Linked Issues check

Explanation

Issue #1017 requires recovery from permission-broker leaks after failed, timed-out, or cancelled delegated turns. It also identifies approval handling that uses the broker's returned socketPath as an alternative fix. This PR only adds botId to the socket-path namespace and tests distinct paths for different bots with one thread ID. It does not show delegated-turn cleanup, fallback socketPath handling by the approval UI, or regression tests for failure, timeout, or cancellation. The same bot can therefore still leave its deterministic path occupied and force later turns onto an approval-invisible fallback path.

Resolution

Implement and test broker cleanup for delegated-turn failure, timeout, and cancellation, or make approval-card handling use the broker's returned socketPath. Add regression coverage for the reported same-bot retry scenario. Retain the bot-ID namespace if it is required for parent and child socket isolation.

Full details: Out of Scope Changes check

Explanation

The socket namespace changes and their collision test support issue #1017. The change summary also identifies unrelated changes: authentication behavior and authentication tests, AskUserQuestion card handling and tests, and thread settings support for edits. These changes do not implement broker cleanup, returned-path handling, or bot-specific socket isolation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (1 skipped: 1 too large.)

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}