fix(acp): own deep-interview skill lifecycle end to end - #3797
Conversation
e9fed46 to
9aadeb8
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
GJC adversarial exact-head review
Verdict: REQUEST_CHANGES
HIGH — the regression test does not exercise the ACP/notifications lifecycle that regressed
The shipped claim in packages/coding-agent/CHANGELOG.md is specifically that a headless client advertising elicitation.form continues receiving elicitation/create after notifications activate. The production fix depends on two separate lifecycle callsites in packages/coding-agent/src/sdk/bus/index.ts: the SDK UI provider is classified as "protocol" at lines 3653–3658, while notification activation installs registerInteractiveAnswerSource(...) at lines 5164–5168, which classifies its source as "interactive".
The new sdk-ask-answer-source-priority.test.ts bypasses both paths. It manually registers synthetic objects through registerAskAnswerSource and asserts only getAskAnswerSource. It would remain green if either production callsite used the wrong kind or if notification activation replaced/bypassed the selected protocol source. Existing sdk-host-wiring.test.ts covers the opposite real ordering — a prior source before the UI provider — but not the reported failure where notifications register after the protocol provider.
Add an observable regression test that starts the SDK/ACP UI provider, activates notifications afterward, then proves an ask still emits the protocol ui.elicit/form reverse request. The same test should prove interactive fallback after protocol removal.
MEDIUM — omitted-argument compatibility is not covered under contention
registerAskAnswerSource(sessionId, source) now relies on the default third argument being "interactive", but every new priority test passes an explicit kind. Cover a legacy two-argument registration performed after a protocol source and assert that it does not steal priority. This is the compatibility boundary introduced by the API change.
Exact-head state
At review time GitHub reports this head as draft and DIRTY, with no check runs attached; the two workflow runs for the SHA concluded action_required, not green. No CI action was taken.
Signature: GJC adversarial review | PR #3797 | exact-head 9aadeb828fe5d4240bb9b9065c8dff1ee6efa478 | model layofflabs/codex-auto-review | REQUEST_CHANGES
VERDICT: REQUEST_CHANGES
|
@Loxia106 — ownership stays with you on the existing Signed REQUEST_CHANGES handoffReviewed head: Required same-PR scope
The prior formal review remains the exact-head gate: #3797 (review) Signature: GJC ACP backlog lane | owner |
9aadeb8 to
649e71e
Compare
|
Both requested tests added and independently verified:
Both tests independently mutation-proven: reverted the selector to the old plain-LIFO Branch rebased onto the current Note:
|
649e71e to
3063cf2
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
GJC GPT-heavy adversarial exact-head review
Terminal verdict: REQUEST_CHANGES
Reviewed head: 3063cf26508c612e032c865017e58f5a1833dda1
Current base tip: dev at f9dffed426a433bf4948bdeb8fb2fe76f25ef5ca
Head merge-base: a8fa63a599e9fa3d0189e48bfce778a36353ccdd
GitHub state: open draft, mergeable/rebaseable, unstable
Prior requested changes: substantively resolved
The exact head now exercises both production classification callsites: notifications register an "interactive" source at packages/coding-agent/src/sdk/bus/index.ts:1779-1847, and a live SDK UI provider registers a "protocol" source at 3651-3659. The registry at packages/coding-agent/src/tools/ask-answer-registry.ts:28-50 uses entry identity for safe stale disposers, selects the newest protocol source, and preserves the two-argument default as interactive. The new priority test at test/sdk-ask-answer-source-priority.test.ts:93-117 covers the prior legacy compatibility blocker.
HIGH — exact-head integration and CI evidence are absent
The contributor head is not descended from the current base tip: dev has advanced nine commits beyond merge-base a8fa63a5, while the PR carries four commits from that merge-base. GitHub currently reports the PR mergeable, and merge-tree inspection found no textual conflict marker, but checks on 3063cf2 do not validate integration with current dev.
The PR is also still draft. Exact-head check runs: 0; legacy statuses: 0 (pending). The only exact-head workflow records are:
Dev CIrun30905911706:action_required.Public site syncrun30905911713:action_required.
Required: update this same contributor branch onto current dev, keep the existing PR/owner lane, mark it ready after the fixes below, and obtain green checks on the resulting exact head or an explicitly recorded maintainer CI waiver. No CI action was taken here.
MEDIUM — disconnect fallback regression has no teardown/readiness barrier
At packages/coding-agent/test/sdk-host-wiring.test.ts:3070-3082, closeSocket(socket) is followed immediately by opening another socket and calling getAskAnswerSource(sessionId)!.awaitAnswer(...). The test does not wait for the server-side disconnect callback to run removeProviderDefinitions and dispose the protocol source. A delayed close callback can therefore select the stale protocol source and send the fallback ask toward the dead provider. It also invokes the ask after only the client open event, without waiting for the fallback connection's application-level hello.
The neighboring existing lifecycle test correctly waits for source restoration at line 2999. Capture the protocol source, wait until the selected source changes to the interactive source, wait for fallback hello, then issue and answer the fallback ask.
MEDIUM — the new live test does not shut down its runtime
The test discards the handler map returned by start() at line 3014, turns notifications on, and ends without invoking session_shutdown. File-level cleanup closes sockets and broker owners but does not execute stopSession, so the interactive registry source and native notification runtime can survive the case. Retain the handlers and invoke session_shutdown in finally before shared cleanup.
LOW — synthetic priority registrations leak across cases
test/sdk-ask-answer-source-priority.test.ts:17-21 only removes temp directories. Several tests register process-wide sources without invoking their disposers; AgentSession.dispose() does not remove manual registry entries. Track and dispose every registration.
Verification
Exact-head focused test attempts in an isolated worktree were blocked before test execution by the environment's installed native binary lacking the @gajae-code/natives@0.12.11 sentinel after frozen dependency installation hit the local node-pty/node-gyp failure. This is not counted as a source test failure, but it provides no independent green evidence.
Contributor ledger / issue disposition
- Admission: admitted. Prior exact-source review confirmed the registration-order bug, and this exact head resolves the prior real-lifecycle and omitted-argument blockers in substance.
- Duplicate: no competing implementation lane was found. PR #3241's B2 elicitation timeout/frame-tail finding is a distinct pre-existing issue, not a duplicate of answer-source authority.
- Current owner path:
Loxia106:fix/acp-answer-source-priority→ PR #3797 →Yeachan-Heo:dev. The prior owner handoff remains authoritative: keep fixes and the base update on this same branch/PR; no replacement PR or maintainer mutation branch.
Signature: GJC hostile review | exact-head 3063cf26508c612e032c865017e58f5a1833dda1 | base f9dffed426a433bf4948bdeb8fb2fe76f25ef5ca | merge-base a8fa63a599e9fa3d0189e48bfce778a36353ccdd | three independent layofflabs/gpt-5.6-terra architect lanes plus maintainer pass | GitHub actor Yeachan-Heo | REQUEST_CHANGES
VERDICT: REQUEST_CHANGES
Yeachan-Heo
left a comment
There was a problem hiding this comment.
GJC exact-head adversarial review
Terminal verdict: REQUEST_CHANGES
Reviewed head: 3063cf26508c612e032c865017e58f5a1833dda1 (unchanged since the prior exact-head review of 2026-08-05T01:42:56Z)
Current dev: f359a9d7ed54aab392a310a3047f276a884f46a1 (#3702, 2026-08-05T06:31:38Z)
PR merge-base: a8fa63a599e9fa3d0189e48bfce778a36353ccdd — dev has advanced ~10 commits beyond the PR base; the head is not descended from current dev.
GitHub state: open draft, mergeable, mergeable_state: unstable.
Production change (reviewed independently, read-only)
ask-answer-registry.ts: kind-tagged entries (protocol/interactive), protocol-over-interactive selection viafindLast, newest-wins within the same kind, entry-identity disposers (stricter than the old object-identitylastIndexOf), and a backward-compatible"interactive"default for the legacy two-argument form. The selection semantics match the documented contract; the ES2024libcoversfindLast.- Both production registration callsites in
sdk/bus/index.tsare correctly classified: the SDK UI provider registers"protocol"(3653–3659) andregisterInteractiveAnswerSourceregisters"interactive"(1779–1847). No other production registration sites exist, so the kind split is complete. - No security/privacy surface change: the same
ui.elicitreverse request is emitted; only the selection authority changed. - No generated artifacts are touched (schemas/models/plugins/docs-index/defaults unchanged); the CHANGELOG entry sits inside
[Unreleased]and the duplicate### Fixedheader there is pre-existing. - The new lifecycle test at
sdk-host-wiring.test.ts:3003-3112does exercise the real wiring (liveregister_providerover WebSocket, real/notify on, protocolui.elicitframe asserted, interactiveaction_neededfallback after disconnect), so the prior review's HIGH on synthetic-only coverage is substantively addressed.
BLOCKER — exact-head CI is not clean and the head is not validated against current dev
- GitHub reports zero check runs and zero commit statuses on
3063cf2(combined statuspending,total_count: 0). - The only two Actions workflow runs for this exact head both concluded
action_required, not success: Dev CI run30905911706and Public site sync run30905911713(created 2026-08-04T11:42:18Z, fork-approval gate). No exact-head CI result is green; the contributor'scheck:tools/focused-test claims have no independent CI evidence. - The head is still not descended from current
dev: the base tip moved fromf9dffed4(prior review) tof359a9d7today while the PR merge-base remainsa8fa63a5.mergeable_stateisunstable. - The PR is still a draft.
- The prior exact-head REQUEST_CHANGES (2026-08-05T01:42:56Z) is therefore unaddressed in its blocker dimension: no green exact-head checks, no base sync, not marked ready.
Required: rebase/merge this same contributor branch onto current dev, obtain green exact-head checks (Dev CI + Public site sync) or an explicitly recorded maintainer CI waiver, mark the PR ready for review, and push the result as the new exact head. No CI action was taken here.
Secondary (non-blocking) test-hygiene findings, carried from the prior exact-head review
- MEDIUM — disconnect fallback has no teardown/readiness barrier (
sdk-host-wiring.test.ts:3070-3082): aftercloseSocket(socket), the test opens the fallback socket and immediately callsgetAskAnswerSource(sessionId)!.awaitAnswer(...)without waiting for the server-side disconnect callback to dispose the protocol source. The neighboring lifecycle test correctly waits for source restoration (waitFor(() => getAskAnswerSource(sessionId) === priorAnswerSource)at line 2999); this one does not, so the fallback ask can race onto the dead protocol provider. - MEDIUM — the new live test does not shut down its runtime (line 3014):
start(...)returns the handler map and the test discards it, never invokingsession_shutdown; the interactive source and notification runtime registered bynotify oncan outlive the case. - LOW — synthetic priority registrations leak (
sdk-ask-answer-source-priority.test.ts): several cases register process-wide sources without invoking their disposers;AgentSession.dispose()does not clear manual registry entries.
These do not change the verdict; they should be folded into the same branch before or with the CI pass.
Signature: GJC adversarial exact-head review | PR #3797 | exact-head 3063cf26508c612e032c865017e58f5a1833dda1 | base f359a9d7ed54aab392a310a3047f276a884f46a1 | merge-base a8fa63a599e9fa3d0189e48bfce778a36353ccdd | REQUEST_CHANGES
VERDICT: REQUEST_CHANGES
3063cf2 to
e340de5
Compare
|
Addressed the latest maintainer review on the same branch:
Local verification on exact head
|
|
OpenCode-driven full ACP validation found an additional blocker in the concrete user path, so I returned this PR to draft. Environment: OpenCode Observed on exact PR head
Controlled diagnostic only (temporary worktrees, not committed): adding The same full startup flow also received the form with the old LIFO selector because the notification runtime, when enabled from process startup, registers before the ACP provider; the ACP provider is therefore newest. Sending Verdict: the registry priority bug and its focused lifecycle test are valid in the forced post-registration activation order, but this PR does not yet make the concrete OpenCode → GJC ACP → deep-interview choice path work. The headless session-host theme initialization blocker needs a production fix and a real subprocess ACP deep-interview regression before this should be ready again. |
Exact-head ACP/OpenCode completion receiptHead: The concrete user path is now covered rather than inferred from registry-only tests:
The latest durability blockers are also closed:
Startup and answer-source fixes remain production-bound:
Verification on this exact head
No workflow was manually rerun or cancelled. Fork workflow approval remains a maintainer-side gate if GitHub leaves the runs as |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
GJC exact-head adversarial review
Terminal verdict: REQUEST_CHANGES
Reviewed head: 7b9fac9012c98525fdc73cd10d0d50145294a6d4 (current exact head, updated 2026-08-05T16:29:47Z)
Current dev: 11e48d5bc — 1 commit beyond the PR base 6c1c8a47 (MiniMax M3 catalog, unrelated); merge-tree vs current dev is clean (0 conflict markers); GitHub reports mergeable.
GitHub state: open, non-draft, unstable.
Resolved since the prior REQUEST_CHANGES on 3063cf2
- BLOCKER — head not descended from current dev / still draft — addressed. The branch was rebased onto
6c1c8a47; the merge-tree against currentdevis clean; the PR is marked ready (non-draft). - MEDIUM — disconnect-fallback teardown/readiness barrier — addressed. The "…falls back on provider disconnect" test now waits for interactive answer-source restoration (
waitForselected source !== protocol source) and for the fallback connection'shellobefore issuing the fallback ask, uses the production SDK host, and stops it infinally. - MEDIUM — live test does not shut down its runtime — addressed. The ACP wire test registers its subprocess for teardown; the host-wiring cases use
host.stop()/session_shutdown. - LOW — synthetic priority registrations leak — addressed via
trackAnswerSource/disposers insdk-ask-answer-source-priority.test.ts. - Original HIGH — synthetic-only lifecycle coverage — substantively addressed.
test/acp-deep-interview-wire.test.tsexercises the real ACP path end to end (livegjc --mode acpsubprocess,/skill:*routing, form elicitation, cancellation fence).
New scope at this head (commit 7b9fac90)
The final commit adds the canonical skill-prompt lifecycle: exact single-text /skill:* ACP prompts route to skill.invoke; preflight cancellation is fenced through a new preflightSignal seam with pending→accepting→accepted phases; skill reconciliation records carry durable pending outcomes; theme init runs before the MCP readiness budget. Wire tests cover routing, cancellation-before-start, late-acceptance fencing, and restart settlement. Coherent, tested extension; no blocking code finding.
Blocking findings (still present at the exact head)
- No green CI on the exact head. Check runs on
7b9fac9:0; legacy statuses:0(pending). Workflow runs for the exact head: Dev CI8592and Public site sync5748both concludedaction_requiredwith zero jobs executed (fork-approval gate). This is the same blocker carried from the two prior reviews; it requires a maintainer to approve the fork runs or record an explicit CI waiver — the contributor cannot self-approve. - Changelog entry in a released section. The entry for this PR sits at line 40 of
packages/coding-agent/CHANGELOG.md, inside the released## [0.12.12] - 2026-08-05section (lines 9–42) rather than under## [Unreleased]. The rebase onto post-cutdevslid it into released history; entries for unreleased work must live in[Unreleased]and released sections are immutable.
Verdict
REQUEST_CHANGES — move the changelog entry to ## [Unreleased], and obtain green exact-head checks (maintainer approval of the fork CI runs) or an explicitly recorded maintainer CI waiver, then re-request review.
Signature: GJC exact-head review | PR #3797 | exact-head 7b9fac9012c98525fdc73cd10d0d50145294a6d4 | base 6c1c8a479 | merge-base 6c1c8a479 | REQUEST_CHANGES
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
|
CI가 안 도는 이유를 확인했다 — 네 잘못이 아니다. 이 PR의 워크플로 런은 푸시를 더 해도 달라지지 않는다. 승인 없이는 새 런도 같은 상태로 들어간다. 같은 사유로 막힌 PR이 6건이라 #3940 으로 정리해 올렸다. 리뷰는 CI와 무관하게 진행하고 있으니 코드 피드백은 그대로 받으면 된다. 다만 이 저장소는 머지에 exact-head CI 증거를 요구하므로, 승인이 떨어지기 전까지는 머지가 불가능하다는 점만 알아두면 된다. |
… regardless of registration order
The ask-answer registry in ask-answer-registry.ts selected the active answer
source by plain LIFO order (`.at(-1)`), so whichever producer registered last
won regardless of role. The notifications extension's interactive answer
source registers via enableNotifications() -> activate(), while an ACP
client's protocol-level UI answer source registers via
installProviderDefinitions("ui", ...) when the client advertises the
elicitation.form capability. Depending on registration order, the
notifications extension's interactive source could register after the ACP
source and permanently shadow it, so an ACP or other headless client that
advertised elicitation.form and expected elicitation/create requests never
received them.
This adds an explicit kind ("protocol" | "interactive") to each registered
source and changes selection to always prefer the highest-priority protocol
source present, falling back to the most recent interactive source only when
no protocol source is registered. This fixes the bug for both registration
orders, not only the one this defect happened to be observed in, and
preserves the existing "most recent wins" behavior within a single kind.
Lore-id: a43d62f9
Constraint: must not change the AskAnswerSource interface
Constraint: must preserve existing "most recent wins" behavior within a single kind
Rejected: fix scoped only to the observed registration order | still reproduces the bug under the reverse order
Tested: bun test packages/coding-agent/test/sdk-ask-answer-source.test.ts packages/coding-agent/test/sdk-ask-answer-source-priority.test.ts (0 failures, 0 skips)
Tested: bun run check:tools (clean)
Not-tested: real ACP client end-to-end reconnect/elicitation flow
Confidence: high
Scope-risk: narrow
Reversibility: clean-revert
Tested: bash /tmp/changelog-boundary-check.sh packages/coding-agent/CHANGELOG.md upstream/dev..HEAD (BOUNDARY_OK) Confidence: high Scope-risk: narrow Reversibility: easy
Keep two-argument registrations compatible with protocol source precedence. Lore-id: a43d62f9 Constraint: preserve protocol priority over default interactive registrations Rejected: explicit kind-only tests | omitted-argument callers remain a supported contract Tested: priority suite; LIFO mutation fails this case Not-tested: full SDK host suite blocked by unrelated durable workflow gate timeout Confidence: high Scope-risk: narrow Reversibility: test-only
Prove live ACP form elicitation survives later notification activation and falls back after provider disconnect. Lore-id: a43d62f9 Constraint: use the production WebSocket provider and /notify on paths Rejected: synthetic registry registration | it bypasses lifecycle kind classification Tested: focused lifecycle test; LIFO mutation fails while waiting for ui.elicit Not-tested: full SDK host suite blocked by unrelated durable workflow gate timeout Confidence: high Scope-risk: narrow Reversibility: test-only
7b9fac9 to
515ebff
Compare
yazzang-homelab
left a comment
There was a problem hiding this comment.
Independent architect review of head 515ebff22. Both remaining blockers from @Yeachan-Heo's last review are still present — and both trace to repo-level problems rather than to anything you did, which changes what you should do about them.
Blocker 1 — the changelog entry is still in a released section
28: ## [0.12.12] - 2026-08-05
...
61: - <this PR's entry>
Line 61 sits under ## [0.12.12], which shipped on 2026-08-05. Same finding as before, at a new head.
This is almost certainly not you moving it. .gitattributes carried packages/*/CHANGELOG.md merge=union until 11:25:32Z today. Union never conflicts — it concatenates both sides of an overlapping hunk, and release commits insert ## [X.Y.Z] directly beneath the surviving ## [Unreleased] heading, so entries added under Unreleased get silently relocated below the new version heading on rebase. I audited dev and found 35 pre-existing entries in that state (#3929). Your PR is one more instance.
The driver is gone now (#3932), so once you move the entry it will stay put. Move it to ## [Unreleased]:
# cut the line at 61, re-insert under ## [Unreleased]Do not rebase-and-hope — the relocation is already committed in your branch and a rebase will not undo it.
While you are in that file: verify it is intact. Twelve open PRs lost their entire changelog to a bad conflict resolution right after the driver was removed (#3942). Yours is currently fine at 312,799 bytes — keep it that way:
git cat-file -s HEAD:packages/coding-agent/CHANGELOG.md # expect ~312 KBBlocker 2 — no green CI, and you cannot fix it
$ gh api "repos/Yeachan-Heo/gajae-code/actions/runs?head_sha=515ebff22" \
-q '.workflow_runs[]|[.name,.conclusion]|@tsv'
Dev CI action_required
Public site sync action_requiredaction_required means the runs are blocked pending maintainer approval of fork workflows — they have never executed. That is why check runs are 0 and why pushing again will not help: every new head enters the same state. Six PRs are stuck this way; I filed #3940 with the list and the setting to change.
So the "obtain green exact-head checks" requirement is currently unobtainable from your side. Push the changelog fix, then it needs @Yeachan-Heo to click "Approve and run workflows".
What is resolved
I re-checked the five previously-resolved items against this head and they hold: the branch descends from a recent dev and merges cleanly, the disconnect-fallback test waits on a readiness barrier rather than a sleep, the ACP wire test registers its subprocess for teardown, synthetic priority registrations are disposed via trackAnswerSource, and acp-deep-interview-wire.test.ts exercises the real lifecycle rather than only synthetic fixtures. The skill-prompt routing addition (exact single-text /skill:* → skill.invoke, with preflight cancellation) is a coherent extension of that lifecycle rather than a separate concern bolted on.
Nothing in the code needs to change. One line moves, and one maintainer click.
gajae.pr-review-verdict.v1 merge-blocked sha256:515ebff2287ed4f227612d53625a0ab4b573e45f reviewer:architect evidence:CHANGELOG entry at line 61 resolves under "## [0.12.12] - 2026-08-05" (heading at line 28) on this head; both workflow runs for this head report conclusion action_required
|
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. — |
Maintainer-review fix published; same-PR reopen requiredThe requested changelog correction is complete on the existing contributor branch.
Exact-tree verification:
PR #3797 was closed by the repository owner at |
What
This PR fixes the real OpenCode → GJC ACP →
deep-interviewchoice flow end to end:/skill:*ACP prompts route through canonicalskill.invoke;Why
OpenCode can control GJC over ACP and invoke
deep-interview, but the concrete path previously failed before form elicitation because the headless lifecycle host had no initialized theme. The original registry fix also depended on registration order, and canonical skill invocation lacked exact requester-owned completion/cancellation semantics. Together those defects could prevent the structured choice from reaching OpenCode or leave the ACP request with ambiguous terminal ownership.Production proof
packages/coding-agent/test/acp-deep-interview-wire.test.tsuses the official ACP SDK against a real GJC subprocess. It advertiseselicitation.form, invokesdeep-interview, selectsoption:0, verifies the selected choice reaches the continuation, and covers normal completion plus active cancellation.The production-path and host tests additionally cover:
/notify onactivation after protocol-provider registration and interactive fallback after provider removal;Exact-head verification
Head:
7b9fac9012c98525fdc73cd10d0d50145294a6d4Base:
dev@6c1c8a4798917be42071af02ab3c005ca71817c3bun run check:tools: passed; Biome checked 3338 filessdk-host-wiring.test.ts: 80 pass, 0 fail, 434 assertionsCLEAR/CLEAR/CLEAR,APPROVEOKAY, no blockersGitHub Actions for fork commits may remain
action_requireduntil a maintainer approves them; no workflow was manually rerun or cancelled.Scope note
PR #3241's separate form-elicitation
frameTailtimeout finding is unchanged and remains out of scope. This PR fixes invocation routing, answer-source authority, startup, cancellation, and durable terminal ownership without claiming to solve that independent timeout policy.