feat(acp): bridge workflow-gate asks to the ACP permission channel - #3925
feat(acp): bridge workflow-gate asks to the ACP permission channel#3925snowykr wants to merge 2 commits into
Conversation
bf43b79 to
c5bad72
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5e28ede50
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3caada4e3c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2131cc0fed
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4296be060
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0019187310
ℹ️ 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".
|
@codex review |
e20769f to
852ab6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 852ab6df5b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95052b9f24
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd1f7b84ba
ℹ️ 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".
fd1f7b8 to
71b92cf
Compare
71b92cf to
8739eb8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8739eb84c3
ℹ️ 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".
8739eb8 to
092dfc5
Compare
092dfc5 to
900be24
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 900be24742
ℹ️ 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".
900be24 to
80934ab
Compare
80934ab to
d84491a
Compare
d84491a to
4e08740
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e08740c8d
ℹ️ 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".
4e08740 to
9767adf
Compare
9767adf to
544f4c5
Compare
yazzang-homelab
left a comment
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review — first human review here; the eleven prior entries are bot comments.
The authorization property is right, and it is the part that mattered
Bridging workflow gates onto a permission channel means a timeout now has to be prevented from becoming consent. Get that wrong and a headless client silently approves a ralplan plan or authorizes execution by doing nothing. This PR gets it right and pins it:
autoSelectOnTimeout:
!intentContract(q.deepInterview) &&
!intentReview(q.deepInterview) &&
q.workflowGate?.kind !== "approval" &&
q.workflowGate?.kind !== "execution",and the consumer side actually honours it (ask.ts:1142-1156): when timedOut && !autoSelectOnTimeout, it returns selectedOptions: [] with no custom input rather than falling through to getAutoSelectionOnTimeout.
The two tests are the ones I would have asked for, and their comments state the invariant rather than the mechanics:
does not auto-select a ralplan approval gate on ask timeout— "A timeout is not consent: the plan approval must stay unselected."does not auto-select an execution gate on ask timeout— "A timeout is not execution authorization."
Keeping the default at autoSelectOnTimeout = true and carving out only the gates is also correct: ordinary selector asks keep their existing convenience behaviour, so this is a narrow authorization fix and not a UX regression.
One change I would make: invert the carve-out
The guard is a deny-list on an enum. Today it is exhaustive —
kind: z.enum(["question", "approval", "execution"]) // ask.ts:194— so excluding approval and execution leaves only question auto-selecting, which is correct. But the failure mode when that enum grows is silent and lands on an authorization boundary: add a fourth kind ("destructive", "consent", whatever) and it defaults to auto-select on timeout, with no test failing and no type error.
The allow-list form is identical today and safe by construction tomorrow:
autoSelectOnTimeout:
!intentContract(q.deepInterview) &&
!intentReview(q.deepInterview) &&
(q.workflowGate === undefined || q.workflowGate.kind === "question"),Same behaviour for all three current values; a new gate kind must be explicitly opted into auto-select rather than inheriting it. On a "did the user consent" decision, that is the direction the default should point.
Rest of the bridge
- Aborting the reverse request and delegating the decision to AskTool's policy is the right division: one component owns the transport deadline, one owns what a timeout means. Having the ACP layer synthesize an answer on timeout would have duplicated the policy in a place with no view of
workflowGate. - Disposing the source with the permission lease ties the ask's lifetime to the thing that can actually answer it; a lease outliving its source is how these bridges leak pending requests.
- Removing only the synthetic trailing transitions rather than filtering broadly keeps user-authored options intact — worth the precision.
- Headless asks no longer dereference the TUI theme. That is the actual crash for a client without
ui, and it is easy to miss because it only fires on the path the capability check was supposed to skip. - CHANGELOG intact at 312,593 bytes (grew by this entry). Merges cleanly into current
dev.
One process note: the description says this "also fixes the broker ownership race". A concurrency fix riding along with a protocol bridge is hard to review and hard to revert independently — if that race has its own reproduction, it would be better as its own PR. Not blocking, but please make sure the PR body states what the race was and how the fix is verified, since the tests listed here are all about the permission bridge.
gajae.pr-review-verdict.v1 merge-approved sha256:9767adf64e3678b51b06286367f979d17716135d reviewer:architect evidence:read of ask.ts:192-195,806-818,1138-1162,1650-1668 and the two timeout gate tests at this head; merge-tree vs origin/dev clean
544f4c5 to
7685412
Compare
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 (toolCallId/title/toolName, option optionId/name, nested outcome), 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, and disposes its source with the permission lease. Headless asks no longer dereference the TUI theme. 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7685412fcb
ℹ️ 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".
7685412 to
cf744a7
Compare
cf744a7 to
26f52ee
Compare
|
Closing during the emergency maintenance freeze. This PR is not in the retained critical or maintainer-owned set. Do not open a replacement PR unless a maintainer explicitly directs it. — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26f52eebc7
ℹ️ 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".
| const choices = new Map<string, AskRemoteInteraction>(); | ||
| const options: Array<Record<string, unknown>> = bridgedOptions.map((label, index) => { | ||
| const optionId = `option:${index}`; | ||
| choices.set(optionId, { kind: "value", value: label }); |
There was a problem hiding this comment.
When an authoritative option is literally Other (type your own) or Ask about these choices (including its numbered deep-interview form), selecting it returns the raw label here, which AskTool still classifies by value as an editor transition; the permission source then cannot answer the resulting editor request, so the ask is cancelled instead of selecting that option. Fresh evidence after the trailing-filter fix is that the new preservation test only inspects the rendered option names and never selects the retained option; carry explicit transition identity rather than relying on the label text.
Useful? React with 👍 / 👎.
3 participants
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_permissionrequest whose options are the answer choices; the client's selectedoptionIdmaps 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.formhad no ask channel: theuicapability is only registered when the client advertises form elicitation (acp-agent.tsacpProviderRegistrations), so the gate waited forever. The SDK already exposesworkflow.gate_answer/workflow.plan_approvewith an ACP disposition ofgeneric_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: newcreateSdkPermissionAskAnswerSource+ wiring in the existingpermissioncapability 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 nouisource is installed (form-eliciting clients keep the richeruichannel).custom_editor/clarification_editor) have no permission-option representation and stay unanswered (unchanged from today).prompt.Testing
Fresh upstream
devcheckout, 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)sdk-ask-answer-source,tools/ask,acp-*,sdk-acp-*,sdk-q29,sdk-host-wiring) → 226 pass, 0 failtsc --noEmitclean,biome checkclean,git diff --checkcleaninitialize → session/new → session/promptreturned the expected "OK" answer (run path incl. provider registration unaffected)session/request_permissionwith 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 requiredtoolCallId/title).toolCallId/title/toolName, optionoptionId/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.selectedOptionsstate in option names, and restore the permission ask source when the ui lease is removed while the permission lease stays live.[x]/[ ]checkbox fallbacks) and matches raw remote choices directly against option labels, so deep-interview / multi-select gates surface assession/request_permissioninstead of crashing (verified live: multi-select ask emitted the permission request with the question and options).AskAnswerRequest.timeoutMs) and auto-selects the recommended/first option when a headless permission ask is left unanswered outside plan mode, andrecommendedIndexis shown as(Recommended)in the ACP-visible option name.Promise.withResolvers) and returns without an answer, delegating the decision toAskTool's own auto-select-on-timeout policy and timed-out settlement (multi-select toggling and deep-interviewautoSelectOnTimeout: falsepreserved); 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.AskToolreports viatransitionCount, so a legitimate option that happens to share a transition label is preserved andrecommendedIndexstays valid; the timeout tests usePromise.withResolvers.ToolAbortError(unrelated provider errors still surface), and the broker winner-discovery retry preservesowner.stop()cleanup on transient read failures.feat+ changelog) on the latestdev.uichannel.approvalandexecutiongates both require an explicit user response.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.[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) andensureBrokerOncefailed instead of reusing the winner's discovery, and a transient discovery read could skipowner.stop()cleanup. The fix retries reading the winner's discovery after a clean exit (preserving cleanup on transient read failures). Verified bysdk-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
The codex connector (
chatgpt-codex-connector) approved the exact current head71b92cfwith THUMBS_UP; all 16 review threads across 8 rounds are resolved. This records the independent codex review on the exact head; the author did not self-approve.Checklist
devbun check(check:types on changed scope) passes