Skip to content

feat(acp): bridge workflow-gate asks to the ACP permission channel - #3952

Merged
Yeachan-Heo merged 2 commits into
Yeachan-Heo:devfrom
snowykr:feat/acp-ask-permission-bridge
Aug 7, 2026
Merged

feat(acp): bridge workflow-gate asks to the ACP permission channel#3952
Yeachan-Heo merged 2 commits into
Yeachan-Heo:devfrom
snowykr:feat/acp-ask-permission-bridge

Conversation

@snowykr

@snowykr snowykr commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What

Workflow-gate asks (ralplan approval, deep-interview questions, ultragoal checkpoints) now surface through the ACP permission channel when the client does not advertise ACP form elicitation (e.g. Paseo). A selector ask is sent as a session/request_permission request whose options are the answer choices; the client's selected optionId maps back to the answer. This follows the documented Paseo pattern ("encode a single-choice question as multiple options of the same allow kind").

Why

Without it, a plain ACP client without elicitation.form had no ask channel: the ui capability is only registered when the client advertises form elicitation (acp-agent.ts acpProviderRegistrations), so the gate waited forever. The SDK already exposes workflow.gate_answer / workflow.plan_approve with an ACP disposition of generic_safe, so only the ask→client channel was missing.

Related: #3922 (previous PR; this resolves its Scope "Not covered" item 1 — human-decision workflow gates).

Scope

  • packages/coding-agent/src/sdk/bus/index.ts: new createSdkPermissionAskAnswerSource + wiring in the existing permission capability handler. No new ACP capability is registered (avoids clients rejecting unknown capabilities); the bridge installs only when the permission capability is active (prompt mode) and no ui source is installed (form-eliciting clients keep the richer ui channel).
  • Selector asks only: free-text asks (custom_editor / clarification_editor) have no permission-option representation and stay unanswered (unchanged from today).
  • Auto-approval follows the client's permission mode, so gates never self-approve under prompt.

Testing

Fresh upstream dev checkout, macOS darwin-arm64:

  • bun test packages/coding-agent/test/sdk-acp-ask-permission-source.test.ts → 3 pass (optionId→answer mapping, cancellation, non-selector not bridged)
  • Regression set (sdk-ask-answer-source, tools/ask, acp-*, sdk-acp-*, sdk-q29, sdk-host-wiring) → 226 pass, 0 fail
  • tsc --noEmit clean, biome check clean, git diff --check clean
  • Live ACP round trip against the checkout: initialize → session/new → session/prompt returned the expected "OK" answer (run path incl. provider registration unaffected)
  • Dogfood E2E (ask gate through a real ACP session): prompted the agent to use the ask tool; it surfaced as session/request_permission with the question + answer options; replying with an option let the agent proceed and echo the chosen answer. This surfaced and fixed two headless crashes: the ask tool's unconditional TUI theme reads (theme.status/theme.checkbox) and the bridge's malformed permission toolCall (missing required toolCallId/title).
  • Review round 1 (addressed): serialize the typed ACP permission schema (toolCallId/title/toolName, option optionId/name), parse the nested { outcome: { outcome, optionId } } response, map enabled navigation controls (Next/Done) to permission options so multi-select asks can commit, and dispose the ask source with the permission lease so headless asks fall back to the workflow-gate path.
  • Review round 2 (addressed): omit the free-text Other/clarification transition options (selecting them previously aborted the whole ask via the editor path), encode multi-select selectedOptions state in option names, and restore the permission ask source when the ui lease is removed while the permission lease stays live.
  • Review round 3 (addressed): the headless multi-select rendering no longer dereferences the TUI theme ([x]/[ ] checkbox fallbacks) and matches raw remote choices directly against option labels, so deep-interview / multi-select gates surface as session/request_permission instead of crashing (verified live: multi-select ask emitted the permission request with the question and options).
  • Review round 4 (addressed): the ask timeout now travels to the bridge (AskAnswerRequest.timeoutMs) and auto-selects the recommended/first option when a headless permission ask is left unanswered outside plan mode, and recommendedIndex is shown as (Recommended) in the ACP-visible option name.
  • Review round 5 (addressed): on timeout the bridge aborts the underlying reverse request (timeout abort signal, Promise.withResolvers) and returns without an answer, delegating the decision to AskTool's own auto-select-on-timeout policy and timed-out settlement (multi-select toggling and deep-interview autoSelectOnTimeout: false preserved); the selector classifies a post-timeout remote-cancellation failure as a timeout. Also fixes the broker ownership race (provider probe vs agent launch on a cold broker) by reusing the broker that wins the lock when a concurrently spawned broker exits cleanly.
  • Review round 7 (addressed): the bridge removes only the synthetic trailing transition entries AskTool reports via transitionCount, so a legitimate option that happens to share a transition label is preserved and recommendedIndex stays valid; the timeout tests use Promise.withResolvers.
  • Review round 8 (addressed): timeout classification is restricted to the remote-cancellation ToolAbortError (unrelated provider errors still surface), and the broker winner-discovery retry preserves owner.stop() cleanup on transient read failures.
  • History: squashed into 2 logical commits (feat + changelog) on the latest dev.
  • Review round 9 (addressed): ralplan (and any approval-kind) workflow gates no longer auto-select on ask timeout — a timeout is not consent, so the approval stays pending until an explicit user response.
  • Review round 10 (addressed): form-less clients always get the ACP permission reverse channel regardless of permission mode, so selector asks stay answerable in auto/always-allow mode (the mode still gates only tool-authorization prompts); form-eliciting clients keep the ui channel.
  • Review round 11 (addressed): execution gates (ultragoal execution signoff) also disable timeout auto-selection — a timeout is not execution authorization, so approval and execution gates both require an explicit user response.
  • Independent architect review (addressed): the timeout auto-selection guard is inverted to an allow-list (workflowGate === undefined || kind === "question") so a future gate kind must opt into auto-select rather than inheriting it; the broker race fix is documented below.
  • Review round 12 (addressed): the remote selection matcher uses the same headless [x]/[ ] checkbox fallback prefixes as the selector, so a legitimate option label that starts with a marker (e.g. [x] Keep cache) matches its rendered prefixed form instead of being mis-stripped.

Broker ownership race fix (rides along per the architect's process note): two ACP processes (provider probe + agent launch) racing a cold broker state both spawned brokers; the loser exited cleanly (code 0) and ensureBrokerOnce failed instead of reusing the winner's discovery, and a transient discovery read could skip owner.stop() cleanup. The fix retries reading the winner's discovery after a clean exit (preserving cleanup on transient read failures). Verified by sdk-broker.test.ts (60 tests) and a live repro: concurrent probe + launch on a cold broker previously failed with "Detached SDK broker exited before discovery", now succeeds.

GJC verdict

Reopens the work of #3925, which was closed by the emergency maintenance freeze (the fork mirror PR snowykr#22 carried the same head during the freeze). The codex connector (chatgpt-codex-connector) approved the exact current head cdb4530 with THUMBS_UP; all threads are resolved. This records the independent review on the exact head; the author did not self-approve.

gajae.pr-review-verdict.v1 merge-approved sha256:5c5d57f52c6f9c136121e277f26976e7de33f327fcbc93c7c3aa17a15e20a25c reviewer:critic evidence:codex-connector-thumbs-up-on-cdb4530+threads-resolved; bun-test-124+tsc+biome-clean; acp-live-e2e; carries-3925-architect+12-review-rounds

Checklist

  • Target branch is dev
  • bun check (check:types on changed scope) passes
  • Tested locally
  • CHANGELOG updated (packages/coding-agent/CHANGELOG.md Unreleased) — will add with the PR number
  • Verdict above matches the exact PR head

snowykr added 2 commits August 7, 2026 01:06
Headless ACP clients without form elicitation (e.g. Paseo) could not answer
workflow-gate asks (ralplan approval, deep-interview questions): the ask had
no channel once the `ui` capability was skipped, so the gate waited
forever. Selector asks now surface as ACP `session/request_permission`
requests whose options are the answer choices; the selected optionId maps
back to the answer. The bridge serializes the typed ACP permission schema,
removes only the synthetic trailing transitions, shows selection state and
the recommended option, enforces the ask timeout by aborting the reverse
request and delegating the decision to AskTool's auto-select-on-timeout
policy (allow-listed to question-kind gates so approval/execution gates
never auto-consent), and disposes its source with the permission lease.
Headless asks no longer dereference the TUI theme, and the remote selection
matcher shares the headless checkbox fallback prefixes. Also fixes the
broker ownership race (provider probe vs agent launch on a cold broker) by
reusing the broker that wins the lock when a concurrently spawned broker
exits cleanly.
@Yeachan-Heo
Yeachan-Heo force-pushed the feat/acp-ask-permission-bridge branch from cdb4530 to f47d6c6 Compare August 7, 2026 01:06

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f47d6c606b

ℹ️ 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".

Comment on lines +289 to +290
for (let retry = 0; retry < 20; retry++) {
const winner = await readBrokerDiscovery(settings.agentDir, settings.heartbeatTtlMs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor the broker discovery deadline after a clean race loss

When two processes race to start a cold broker, the losing child can exit immediately while the winner is still initializing. This recovery loop waits for only 20 × 50 ms, even though the normal discovery budget is 10 seconds (30 seconds for fixtures), so a valid winner that publishes after the first second still causes ensureBroker to report “exited before discovery.” Continue polling until the existing discovery deadline rather than imposing this shorter fixed window.

Useful? React with 👍 / 👎.

@Yeachan-Heo
Yeachan-Heo merged commit 9079f1b into Yeachan-Heo:dev Aug 7, 2026
25 checks passed
Yeachan-Heo pushed a commit that referenced this pull request Aug 7, 2026
session/set_mode no longer routes default through mode.plan.set. default is a
safe no-op success; plan returns typed unsupported without a host plan-mode
lifecycle. Includes coordinator type-boundary repairs required for CI
root-check and rebased onto post-#3952 origin/dev.

Fixes #3905.
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