feat(acp): bridge workflow-gate asks to the ACP permission channel - #22
Closed
snowykr wants to merge 33 commits into
Closed
feat(acp): bridge workflow-gate asks to the ACP permission channel#22snowykr wants to merge 33 commits into
snowykr wants to merge 33 commits into
Conversation
snowykr
force-pushed
the
feat/acp-ask-permission-bridge
branch
from
August 6, 2026 12:45
aed5dfb to
81dc56e
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: 81dc56ed84
ℹ️ 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".
snowykr
force-pushed
the
feat/acp-ask-permission-bridge
branch
from
August 6, 2026 12:58
81dc56e to
5c77c13
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c77c139df
ℹ️ 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".
snowykr
force-pushed
the
feat/acp-ask-permission-bridge
branch
3 times, most recently
from
August 6, 2026 13:39
278607b to
e36d11b
Compare
Dev CI run 31102063678 (dev @473eab944) failed because coding-agent shard 7 timed out in "Smithery env trust boundary > uses the built-in endpoints and no key by default" after 5004ms (Bun's default per-test timeout). Root cause: each test spawns a child Bun process (smithery-env-probe.ts) that eagerly parses 9 env/dotfiles (5 shell rc files + 4 .env paths) at module load via $credentialEnv → env.ts:39-50. Locally each test runs ~800ms, but under CI's 7-shard parallel load the child-process spawn + TS compilation + synchronous file I/O can exceed the 5s default. This is a CI flake (resource contention), not a deterministic regression — reproduced 5 pass / 0 fail consistently at dev 473eab9 across 3 local runs (~4s total for all 5 tests). The fix adds a 30s per-test timeout matching the established codebase pattern (agent-session-resilient-retry .test.ts:589,1283 use 30_000 for child-process tests). No production code or test logic changed — only the timeout budget. Lore-id: 7d2e8c1f Confidence: high Scope-risk: narrow Reversibility: trivial Tested: 3x reproduction at dev 473eab9 (5 pass / 0 fail each, ~4s total); post-fix verification (5 pass / 0 fail, ~6s total) Not-tested: CI shard-7 parallel contention (requires CI runner) Supersedes: dev CI run 31102063678 failure evidence
snowykr
force-pushed
the
feat/acp-ask-permission-bridge
branch
from
August 6, 2026 13:48
e36d11b to
cdb4530
Compare
This was referenced Aug 6, 2026
The issue fixture treated one configured model's limits as provider-wide defaults for unrelated discovered ids. Cover same-id YAML precedence, exact-id model overrides, and unknown fallback independently. Lore-id: issue-3954-yaml-discovery-contract Constraint: model configuration remains identity-scoped Rejected: provider-wide cross-model limit inheritance | not supported by schema or runtime merge semantics Confidence: high Scope-risk: narrow Reversibility: easy Tested: focused issue regression and three nearby model-registry discovery checks
…eachan-Heo#3948) Red-team CLEAR at f9c0fe0. All CI checks success/skipped. Test-only timeout stabilization for Dev CI flake 31102063678.
…Heo#3961) Red-team MERGE_READY at 460285d. Test-only contract correction for post-Yeachan-Heo#3927 discovery limits (same-id YAML, modelOverrides, UNK fallbacks). No runtime change. Local public surfaces failed pre-checkout on GitHub Actions 5xx only.
…ntract Dev CI at 4f6e860 fails provider-onboarding-wizard-redteam on a stale assertion: it expected "At least one model id is required" while addApiCompatibleProvider throws the post-Yeachan-Heo#3927 public message "At least one model id or model discovery is required." Update the empty-models red-team expectation to the exact intended substring of that contract. No runtime change. Lore-id: 9c4e1a02 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test packages/coding-agent/test/provider-onboarding-wizard-redteam.test.ts (7 pass / 0 fail) Not-tested: full Dev CI shard matrix
…ntract (Yeachan-Heo#3966) Red-team MERGE_READY. Test-only: assert post-Yeachan-Heo#3927 public error 'At least one model id or model discovery is required'. Dev CI 31127515306 success. No runtime change.
Dev CI run 31128319216 (Yeachan-Heo#3855 exact head) failed only when "uses the built-in endpoints and no key by default" hit 30004ms — exactly the 30s per-test cap from Yeachan-Heo#3948. Assertions and provider behavior were green; this is CI contention at the previous budget. Raise each child-process it() timeout from 30_000 to 60_000, matching existing 60s child-process patterns (e.g. acp-session-delete-wire). No assertion or production change. Lore-id: a3f8c201 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: sequential 3x (5 pass/~4s); 6-way parallel contention (6x 5 pass/~4s) Not-tested: full 8-shard Dev CI load shape
…chan-Heo#3967) Red-team MERGE_READY at accba29. Test-only 30s→60s timeout for smithery-env-trust child-process CI contention flake (run 31128319216 @30004ms). Unblocks Yeachan-Heo#3855.
The builtin notify handler always returns {prompt: command.text} for on/off so
it cannot shadow the extension-owned per-session control — whether or not a
lazy/native command is installed. A stale fixture on the startup-cost PR
expected {consumed:true} + "unavailable" for the no-command path and failed
exact-head shard-5.
Add the contract on dev as a minimal baseline so the performance PR does not
absorb unrelated expectation rewrites into its diff.
Lore-id: ntfy-pass-1
Confidence: high
Scope-risk: narrow
Reversibility: reversible
Tested: bun test packages/coding-agent/src/slash-commands/notify-command.test.ts
Not-tested: full coding-agent shard matrix
…nly-pass-through test(coding-agent): lock /notify on|off always-pass-through contract
… decision contract
ACP clients answer `session/request_permission` with the spec-shaped
`RequestPermissionResponse` (`{ outcome: { outcome, optionId } }`), but the SDK
permission-provider contract is the flat decision (`{ outcome, optionId }`).
The ACP reverse bridge returned the raw nested payload, so every
permission-gated tool call (bash, edit, write) under a standards-compliant
client such as Paseo failed with "permission provider returned an invalid
response". Unwrap the outer outcome at the adapter boundary; the flat legacy
shape passes through unchanged.
Lore-id: 2b18a5e0
Constraint: permission decisions must stay fail-closed -- malformed responses still surface the existing invalid-response error
Rejected: accepting both shapes in the SDK bus | duplicates ACP wire knowledge in the generic SDK contract
Confidence: high
Scope-risk: narrow
Reversibility: revert-commit
Tested: nested selected/cancelled and flat shapes via createAcpReverseConnection unit test; live ACP probe against cli.ts acp with a Paseo-shaped nested response
Not-tested: other ACP client implementations beyond Paseo-shaped responses
The response-shape fix was only proven at the reverse helper boundary, leaving the registered provider path, alias, and typed bridge contracts exposed to regression. Add a coupled fail-closed fixture and correct the release note to match the actual guarded tools. Lore-id: pr3953-followup Constraint: keep ACP envelope parsing at createAcpReverseConnection Constraint: SDK permission providers remain flat and fail closed Tested: ACP startup, client bridge, agent-session permission, SDK host permission, coding-agent package check Not-tested: full repository test suite
… contention Root cause: the first Bun probe child absorbs cold compile of the probe + env module graph. Under coding-agent shard contention that cold start can exceed the 60s per-test budget (observed 60001ms on Yeachan-Heo#3969 exact-head) while the four sibling cases complete in ~300ms once the module graph is warm. Fix (harness isolation, assertions unchanged): - beforeAll warmup spawn (120s suite budget) moves cold-start outside it() - resolveIn kills stalled children at 45s with a diagnostic instead of leaking pipes until the outer it() timeout Not a third blind per-test timeout bump; the 60s it() budgets stay. Lore-id: smithery-spawn-isolation-1 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: not fully offline (local natives missing); exact-head CI verifies Not-tested: full parallel shard matrix offline
…-trust-spawn-isolation fix(test): isolate smithery-env-trust first-spawn cold-start under CI contention
…vailable Port the midrun fail-closed contract onto current dev's prune path: use ensureArtifactManager (so ephemeral install failures are visible) and return undefined from #pruneToolOutputs when tool-output eviction is planned but no artifact store can be established. Prevents reporting a successful prune that skipped durable eviction. Regression: force ephemeral install failure via SessionManagerTestHooks and assert mid-run maintenance outcomes "failed" with original tool text retained. Lore-id: midrun-fc1-devbase Confidence: high Scope-risk: narrow Reversibility: reversible Tested: not run locally (node-pty unavailable); exact-head CI will verify Not-tested: full coding-agent shard matrix offline
Drop the extra blank line so check:@gajae-code/coding-agent passes.
…eady Ownership was surrendered by exactly one path: releaseDaemonOwnership, which runs only after a fully quiesced and fully persisted shutdown. Every other ending -- an uncaught error, a failed final topic-registry persist, a signal -- left ownershipPhase:"ready" and a matching lock on disk for a process that no longer existed, and later readers attached to it. Found live on a developer machine: the daemon wrote one heartbeat 559 ms after readiness, died on an uncaught "shared topic authority unavailable", and was still recorded as ready eight hours and seventeen crashes later. No notification was delivered in that window and nothing said so, because the only component that would have noticed -- the next ensure -- had already run. markDaemonOwnerStopped writes stoppedAt and nothing else. It is fenced on full owner identity so a successor's state is never marked stopped, and it leaves the ownership lock alone: a process on its way out is the least qualified party to decide who owns the daemon next, and the reclaim path already adjudicates a dead owner. isFreshLiveOwner treats stoppedAt as disqualifying, so recovery stops depending on a pid liveness check that a recycled pid can defeat. Lore-id: 7c4a1e88 Constraint: a fatal handler must not throw -- every failure here returns false Constraint: the marker must never unlink the ownership lock it did not win Rejected: release ownership on crash | unlinks the lock and implies a clean handoff the corpse cannot guarantee Rejected: make the topic-registry persist failure non-fatal | fixes one crash source and leaves the lie behind for the next one Rejected: rely on pidAlive alone | a recycled pid resurrects a dead owner Confidence: high Scope-risk: narrow Reversibility: safe Tested: fenced marker accepts its own owner and refuses successor, rebound pid, foreign incarnation, wrong generation, mismatched lock, already-stopped and malformed state Tested: 610 existing notification daemon cases still pass Not-tested: end-to-end recovery after a real uncaught daemon crash
…act-fail-closed fix(session): fail closed when tool-output eviction artifacts are unavailable
Protected lifecycle change to runDaemonInternal (markDaemonOwnerStopped wiring via finally + postmortem) requires a strictly higher DAEMON_GENERATION. Bump 53 → 54 and regenerate the generation manifest digests so the Telegram daemon generation guard accepts the Yeachan-Heo#3965 change.
…tinuations Auto-compaction and queued-follow-up continuations racing a still-busy agent rescheduled agent.continue() on a fixed 100ms timer with no attempt cap, spinning indefinitely while the busy state persisted — observed in production logs as 10,742 reschedules over 21 minutes in a single session. Reschedule with capped exponential backoff (100ms doubling to 5s) and give up after 50 attempts (~4 minutes), logging an explicit warn and routing through the standard failure handlers. The same bound is applied to the auto-continue-prompt reschedule loop.
Generation 54 covers owner stoppedAt postmortem for unclean daemon death (Yeachan-Heo#3965).
…sy-reschedule-cap fix(coding-agent): bound busy-reschedule loop for fire-and-forget continuations
…eady (Yeachan-Heo#3965) Red-team MERGE_READY at 322c233. markDaemonOwnerStopped + postmortem/finally wiring; DAEMON_GENERATION 54; generation manifest updated. Dev CI 31132445631 success.
…shape fix(acp): normalize session/request_permission responses into the SDK decision contract
Dev CI run 31133356543 at f1eb7b5 failed two coupled contracts: 1) notifications-topic-registry.test.ts still pinned DAEMON_GENERATION to 53 after Yeachan-Heo#3965 shipped generation 54 — pure stale assertion. 2) Smithery "still honors inherited Smithery configuration" hit exactly 60001.59ms. Prior harness set a 45s kill timer but still awaited Promise.all(stdout, stderr, exited). When kill did not complete the pipe reads, the outer it(..., 60_000) failed while sibling cases before and after completed in ~370ms. This is a child-process lifecycle hang, not an assertion bug and not fixed by another blind timeout bump. Repair: - Pin topic-registry generation contract to 54 / serving epoch 5. - Probe harness: minimal child env (no full process.env copy), stdin ignore, per-attempt 15s deadline via settled Promise race + SIGKILL, up to 2 timeout-only retries. Assertions unchanged. Lore-id: d4f2a801 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: topic-registry generation pin; smithery 3× sequential + 6× parallel contention (all 5 pass / ~3s) Not-tested: full 8-shard Dev CI load shape
probepark was invited as a maintainer (write access), but the repo had no maintainer record and the README contributor list omitted them. Add MAINTAINERS.md, cross-link it from CONTRIBUTING.md, and add probepark to the README contributors list. Access is recorded as "write" because this is a personal-account repo where GitHub exposes no org-only "maintain" role. Confidence: high Scope-risk: low Reversibility: trivial Tested: n/a (docs only)
…ation-pin-and-smithery-hang fix(test): unstick Smithery probe lifecycle and pin DAEMON_GENERATION 54
…nvalid
A managed scope whose tree exceeds the native snapshot budget fails with
`content_too_large` from `snapshotManagedTree`. The resolver collapsed any
message outside a hand-written allowlist into `binding_invalid`, so launch
aborted with:
Could not prepare managed session scope (binding_invalid: prepare:store)
The binding file in that state is byte-for-byte canonical. The diagnostic
names the one artifact that is provably healthy, so the obvious next step —
delete the binding and let it regenerate — destroys correct state without
addressing the real cause.
The allowlist had also drifted. `managedScopeFailureCodes` lists five codes,
but the three classification arms were copied by hand and only one of them
kept `migration_busy`; the other two downgraded it to `binding_invalid`.
The same failure therefore produced different codes depending on which arm
observed it.
Route all three through one helper backed by the existing set, and add
`capacity_exceeded` so an over-budget tree is reported as what it is.
Unrecognized messages still fall back to `binding_invalid`.
Tests: packages/coding-agent/test/managed-scope-capacity-classification.test.ts
(3 new; each fails on dev before this change)
`ResolveManagedSessionScopeResult` restated the managed scope error codes by hand, so adding a code to the canonical union broke assignability instead of propagating. This is the same hand-copied-union drift the parent commit fixes in the resolver; point the SDK type at the source of truth so the two cannot diverge again. Tests: bun test packages/coding-agent/test/sdk-session-directory.test.ts (10 pass)
…e-capacity-classification fix(session): report managed-scope capacity failures as capacity_exceeded, not binding_invalid
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
force-pushed
the
feat/acp-ask-permission-bridge
branch
from
August 7, 2026 01:06
cdb4530 to
f47d6c6
Compare
There was a problem hiding this comment.
💡 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
+3515
to
+3517
| const lock = await readOwnershipLock(fsImpl, paths.lock); | ||
| if (!ownershipLockMatchesState(lock, state)) return false; | ||
| await writeJsonAtomic(fsImpl, paths.state, { ...state, stoppedAt: (input.now ?? Date.now)() }); |
There was a problem hiding this comment.
When fatal cleanup overlaps an in-flight renewDaemonHeartbeat, the renewal can read the live state before this check, this function can write stoppedAt, and the renewal can then publish its previously read state without stoppedAt, leaving the dead daemon advertised as ready again. markDaemonOwnerStopped must acquire and validate the same transition lock used by heartbeat renewal and ownership takeover across the state/lock checks and write; merely checking that the ownership lock matches is subject to this TOCTOU race.
Useful? React with 👍 / 👎.
Owner
Author
|
Closing the freeze mirror: the upstream PR Yeachan-Heo#3952 (same head) was merged by Yeachan-Heo on 2026-08-07. The ask-bridge work is now in upstream dev; the fork dev lag made this PR's diff stale (23 upstream files beyond the ask-bridge scope). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
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
uicapability was skipped, so the gate waited forever. Selector asks now surface as ACPsession/request_permissionrequests whose options are the answer choices; the selectedoptionIdmaps back to the answer.Why
Fork-internal PR mirroring upstream #3925, which was closed by the emergency maintenance freeze. This change is complete and review-passed on the upstream head (independent architect review + 12 codex bot review rounds, 20 threads resolved); this fork PR preserves the work until the freeze lifts.
Scope (reviewed surface)
toolCallId/title/toolName, optionoptionId/name, nested outcome)transitionCount), selection-state and recommended markersAskTool's auto-select-on-timeout policy, allow-listed to question-kind gates (approval/execution gates never auto-consent)Testing
bun test: ask tool (92) + ask-permission source (11) + ACP registration (26) + broker (60) — 189 passtsc --noEmitclean,biome checkclean,git diff --checkcleansession/request_permission, answers mapped back correctly; ralplan/execution gates do not auto-select on timeoutGJC verdict
GJC_ASK_TIMEOUT_CODE) instead of relying on elapsed-time + error-class heuristics, so a genuine cancellation past the deadline still propagates instead of auto-selecting an answer the user never picked.Checklist
dev(fork)bun check(check:types on changed scope) passes