Skip to content

fix(acp): own deep-interview skill lifecycle end to end - #3797

Closed
Loxia106 wants to merge 6 commits into
Yeachan-Heo:devfrom
Loxia106:fix/acp-answer-source-priority
Closed

fix(acp): own deep-interview skill lifecycle end to end#3797
Loxia106 wants to merge 6 commits into
Yeachan-Heo:devfrom
Loxia106:fix/acp-answer-source-priority

Conversation

@Loxia106

@Loxia106 Loxia106 commented Aug 3, 2026

Copy link
Copy Markdown

What

This PR fixes the real OpenCode → GJC ACP → deep-interview choice flow end to end:

  • protocol answer providers outrank later interactive notification providers while preserving the legacy two-argument registration API and fallback disposal behavior;
  • exact single-text /skill:* ACP prompts route through canonical skill.invoke;
  • requester-owned skill invocations use correlated prompt completion as their sole durable terminal authority;
  • active and preflight cancellation are fenced so late acceptance cannot start unowned work;
  • skill pending outcomes survive delayed store writes and process restart without quarantine or outcome loss;
  • headless lifecycle hosts initialize the shared theme before calculating the remaining MCP readiness budget.

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.ts uses the official ACP SDK against a real GJC subprocess. It advertises elicitation.form, invokes deep-interview, selects option:0, verifies the selected choice reaches the continuation, and covers normal completion plus active cancellation.

The production-path and host tests additionally cover:

  • exact canonical command parsing and malformed/multi-block fallback to normal prompts;
  • correlated completion and preflight cancellation with late-release fencing;
  • legitimate local /notify on activation after protocol-provider registration and interactive fallback after provider removal;
  • delayed durable normal/cancel outcomes, restart settlement, and no corrupt quarantine;
  • lifecycle option forwarding, theme startup ordering, and MCP budget accounting.

Exact-head verification

Head: 7b9fac9012c98525fdc73cd10d0d50145294a6d4
Base: dev@6c1c8a4798917be42071af02ab3c005ca71817c3

  • coding-agent TypeScript check: passed
  • bun run check:tools: passed; Biome checked 3338 files
  • reconciliation suites: 19 pass, 0 fail, 69 assertions
  • ACP/provider suites: 59 pass, 0 fail, 237 assertions
  • full sdk-host-wiring.test.ts: 80 pass, 0 fail, 434 assertions
  • OpenCode 1.18.13 dogfood of the real wire + production path: 4 pass, 0 fail, 106 assertions
  • independent frozen-head architect review: CLEAR/CLEAR/CLEAR, APPROVE
  • independent frozen-head QA/red-team: passed, no blockers
  • terminal critic: OKAY, no blockers

GitHub Actions for fork commits may remain action_required until a maintainer approves them; no workflow was manually rerun or cancelled.

Scope note

PR #3241's separate form-elicitation frameTail timeout 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.

@Loxia106 Loxia106 closed this Aug 3, 2026
@Loxia106
Loxia106 deleted the fix/acp-answer-source-priority branch August 3, 2026 15:08
@Loxia106
Loxia106 restored the fix/acp-answer-source-priority branch August 3, 2026 15:12
@Loxia106 Loxia106 reopened this Aug 3, 2026
@Loxia106
Loxia106 force-pushed the fix/acp-answer-source-priority branch 3 times, most recently from e9fed46 to 9aadeb8 Compare August 3, 2026 15:22
@Loxia106
Loxia106 marked this pull request as draft August 3, 2026 15:26

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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

@Yeachan-Heo

Copy link
Copy Markdown
Owner

@Loxia106 — ownership stays with you on the existing fix/acp-answer-source-priority branch and PR #3797. No parallel mutation branch or replacement PR has been opened.

Signed REQUEST_CHANGES handoff

Reviewed head: 9aadeb828fe5d4240bb9b9065c8dff1ee6efa478
Current dev: 29b9d1b17f836911bed89f8154d303cb6886c31c

Required same-PR scope

  1. Real ACP/SDK lifecycle regression: exercise the production host/provider path rather than registering synthetic registry entries. Install an SDK UI provider that advertises form elicitation, then activate notifications afterward. Issue an ask and prove the selected source still emits the protocol reverse request for ui.elicit with mode: "form". Remove/disconnect the protocol provider and prove the already-registered interactive notification source becomes the fallback.
  2. Legacy compatibility: register an explicit protocol source, then call the existing two-argument registerAskAnswerSource(sessionId, source) form. Assert the legacy registration does not steal priority; after disposing the protocol source, assert the legacy source is restored.
  3. Branch state: update this same branch onto current dev and keep the fix limited to the reviewed ACP answer-source behavior, its tests, and directly required changelog fallout. Local three-way merge-tree inspection found no textual conflict markers, so the GitHub DIRTY state should be refreshed by updating the branch rather than opening another PR.
  4. Draft/workflow state: leave the PR draft while addressing the requested changes, then mark this same PR ready. The current workflow records are action_required; do not manually rerun or cancel them. If fork-workflow approval remains required after the owner update, that approval is a maintainer-side gate, not a reason to duplicate the branch.

The prior formal review remains the exact-head gate: #3797 (review)

Signature: GJC ACP backlog lane | owner Loxia106 | PR #3797@9aadeb828fe5d4240bb9b9065c8dff1ee6efa478 | dev 29b9d1b17f836911bed89f8154d303cb6886c31c | OWNER_FIX_HANDOFF

@Loxia106
Loxia106 force-pushed the fix/acp-answer-source-priority branch from 9aadeb8 to 649e71e Compare August 4, 2026 02:17
@Loxia106

Loxia106 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Both requested tests added and independently verified:

  • Real ACP/SDK lifecycle regression test (sdk-host-wiring.test.ts): registers the UI provider via a live WebSocket register_provider frame, then activates notifications via the real /notify on command, then proves the ask still routes through the protocol ui.elicit (mode: form) request, then closes the provider socket and proves fallback to the interactive path via action_needed/ask.answer.
  • Legacy two-argument compatibility test (sdk-ask-answer-source-priority.test.ts): a protocol source registered first, then a two-argument (implicit-interactive) registration, confirms priority is not stolen, then confirms fallback after the protocol source is disposed.

Both tests independently mutation-proven: reverted the selector to the old plain-LIFO .at(-1) form and confirmed both new tests fail, then restored and confirmed both pass again.

Branch rebased onto the current upstream/dev tip (a8fa63a599e9fa3d0189e48bfce778a36353ccdd) with no conflicts.

Note: bun test packages/coding-agent/test/sdk-host-wiring.test.ts (full file) currently has one unrelated pre-existing failure, "SDK host discovers, answers, and advances a durable workflow gate", which times out identically on a clean upstream/dev checkout with none of this PR's changes present — confirmed via a separate worktree at the same commit. Not something this PR introduced or can fix within its scope.

bun run check:tools: clean.

@Loxia106
Loxia106 marked this pull request as ready for review August 4, 2026 02:24
@Loxia106
Loxia106 marked this pull request as draft August 4, 2026 09:35
@Loxia106
Loxia106 force-pushed the fix/acp-answer-source-priority branch from 649e71e to 3063cf2 Compare August 4, 2026 11:42

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 CI run 30905911706: action_required.
  • Public site sync run 30905911713: 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 #3797Yeachan-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 Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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: a8fa63a599e9fa3d0189e48bfce778a36353ccdddev 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 via findLast, newest-wins within the same kind, entry-identity disposers (stricter than the old object-identity lastIndexOf), and a backward-compatible "interactive" default for the legacy two-argument form. The selection semantics match the documented contract; the ES2024 lib covers findLast.
  • Both production registration callsites in sdk/bus/index.ts are correctly classified: the SDK UI provider registers "protocol" (3653–3659) and registerInteractiveAnswerSource registers "interactive" (1779–1847). No other production registration sites exist, so the kind split is complete.
  • No security/privacy surface change: the same ui.elicit reverse 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 ### Fixed header there is pre-existing.
  • The new lifecycle test at sdk-host-wiring.test.ts:3003-3112 does exercise the real wiring (live register_provider over WebSocket, real /notify on, protocol ui.elicit frame asserted, interactive action_needed fallback 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 status pending, total_count: 0).
  • The only two Actions workflow runs for this exact head both concluded action_required, not success: Dev CI run 30905911706 and Public site sync run 30905911713 (created 2026-08-04T11:42:18Z, fork-approval gate). No exact-head CI result is green; the contributor's check:tools/focused-test claims have no independent CI evidence.
  • The head is still not descended from current dev: the base tip moved from f9dffed4 (prior review) to f359a9d7 today while the PR merge-base remains a8fa63a5. mergeable_state is unstable.
  • 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

  1. MEDIUM — disconnect fallback has no teardown/readiness barrier (sdk-host-wiring.test.ts:3070-3082): after closeSocket(socket), the test opens the fallback socket and immediately calls getAskAnswerSource(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.
  2. MEDIUM — the new live test does not shut down its runtime (line 3014): start(...) returns the handler map and the test discards it, never invoking session_shutdown; the interactive source and notification runtime registered by notify on can outlive the case.
  3. 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

@Loxia106
Loxia106 force-pushed the fix/acp-answer-source-priority branch from 3063cf2 to e340de5 Compare August 5, 2026 13:00
@Loxia106
Loxia106 marked this pull request as ready for review August 5, 2026 13:01
@Loxia106

Loxia106 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Addressed the latest maintainer review on the same branch:

  • rebased onto current upstream/dev
  • captured the protocol answer source and wait for server-side disconnect restoration before fallback
  • wait for the fallback connection hello before issuing the interactive ask
  • retain the lifecycle handlers and invoke session_shutdown in finally
  • track and dispose every synthetic registry registration after each priority test

Local verification on exact head e340de562605e0d1d9d90299a99f0fc7b56843a5:

  • bun test packages/coding-agent/test/sdk-ask-answer-source-priority.test.ts — 6 pass
  • focused sdk-host-wiring.test.ts ACP notification priority test — 1 pass
  • coding-agent TypeScript check — clean
  • bun run check:tools — clean
  • Biome check on both changed tests — clean

@Loxia106
Loxia106 marked this pull request as draft August 5, 2026 14:14
@Loxia106

Loxia106 commented Aug 5, 2026

Copy link
Copy Markdown
Author

OpenCode-driven full ACP validation found an additional blocker in the concrete user path, so I returned this PR to draft.

Environment: OpenCode 1.18.13 acted as the external controller; its generated harness used the official @agentclientprotocol/sdk ClientSideConnection/ndJsonStream, advertised elicitation.form, spawned this branch via bun packages/coding-agent/src/cli.ts --mode acp, created a real session, and invoked /skill:deep-interview ....

Observed on exact PR head e340de562605e0d1d9d90299a99f0fc7b56843a5:

  • deep-interview reached the real ask tool, but every ask failed before source selection with undefined is not an object (evaluating 'theme.status')
  • ACP received 0 elicitation/create requests; the turn timed out
  • the SDK lifecycle session host does not initialize the shared theme, while askSingleQuestion() calls getDoneOptionLabel() and dereferences theme.status before it invokes the selected remote answer source

Controlled diagnostic only (temporary worktrees, not committed): adding await initTheme(false) before createLifecycleAgentSession() unblocked the full path. The ACP client then received and answered the real Deep Interview Round 0 form (option:0, “Looks right”) with no timeout.

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 /notify on as an ACP prompt did not execute the extension command—it was handled as an ordinary model prompt—so it did not reproduce the reviewed “notifications activate after protocol provider” ordering.

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.

@Loxia106 Loxia106 changed the title fix(acp): give protocol answer sources priority over interactive ones regardless of registration order fix(acp): own deep-interview skill lifecycle end to end Aug 5, 2026
@Loxia106

Loxia106 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Exact-head ACP/OpenCode completion receipt

Head: 7b9fac9012c98525fdc73cd10d0d50145294a6d4
Base: dev@6c1c8a4798917be42071af02ab3c005ca71817c3
OpenCode: 1.18.13

The concrete user path is now covered rather than inferred from registry-only tests:

  1. OpenCode acts as the external controller.
  2. The official ACP SDK starts a real gjc --mode acp subprocess.
  3. An exact single-text /skill:deep-interview request routes through canonical skill.invoke.
  4. GJC emits a real structured elicitation.form; option:0 is selected and reaches the continuation.
  5. Normal completion, active cancellation, and cancellation before agent start all terminate exactly once.

The latest durability blockers are also closed:

  • skill pendingOutcome records are schema-valid, reload without quarantine, and preserve exact normal/cancel outcomes after restart;
  • requester-owned skills terminalize only through correlated prompt authority; legacy skill transitions remain only for unowned invocations;
  • delayed-store tests prove claim/finalize serialization and retain interleaved prompt/skill records.

Startup and answer-source fixes remain production-bound:

  • theme initialization runs under structured lifecycle failure handling;
  • remaining MCP readiness budget is calculated after theme initialization;
  • a real local /notify on activation after ACP provider registration cannot steal form authority;
  • provider removal restores the already-registered interactive fallback;
  • legacy two-argument registration and teardown hygiene remain covered.

Verification on this exact head

  • bun run check:types in packages/coding-agent — passed
  • reconciliation suites — 19 pass, 0 fail, 69 assertions
  • ACP/provider/startup/extension suites — 59 pass, 0 fail, 237 assertions
  • full sdk-host-wiring.test.ts80 pass, 0 fail, 434 assertions
  • bun run check:tools — passed; 3338 files checked
  • OpenCode dogfood of acp-deep-interview-wire.test.ts + sdk-acp-production-path.test.ts4 pass, 0 fail, 106 assertions
  • frozen-source architect review — CLEAR / CLEAR / CLEAR, APPROVE
  • frozen-source executor QA/red-team — passed, no blockers
  • terminal critic — OKAY, no blockers

No workflow was manually rerun or cancelled. Fork workflow approval remains a maintainer-side gate if GitHub leaves the runs as action_required.

@Loxia106
Loxia106 marked this pull request as ready for review August 5, 2026 16:29

@Yeachan-Heo Yeachan-Heo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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

  1. BLOCKER — head not descended from current dev / still draft — addressed. The branch was rebased onto 6c1c8a47; the merge-tree against current dev is clean; the PR is marked ready (non-draft).
  2. MEDIUM — disconnect-fallback teardown/readiness barrier — addressed. The "…falls back on provider disconnect" test now waits for interactive answer-source restoration (waitFor selected source !== protocol source) and for the fallback connection's hello before issuing the fallback ask, uses the production SDK host, and stops it in finally.
  3. 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.
  4. LOW — synthetic priority registrations leak — addressed via trackAnswerSource/disposers in sdk-ask-answer-source-priority.test.ts.
  5. Original HIGH — synthetic-only lifecycle coverage — substantively addressed. test/acp-deep-interview-wire.test.ts exercises the real ACP path end to end (live gjc --mode acp subprocess, /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)

  1. No green CI on the exact head. Check runs on 7b9fac9: 0; legacy statuses: 0 (pending). Workflow runs for the exact head: Dev CI 8592 and Public site sync 5748 both concluded action_required with 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.
  2. 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-05 section (lines 9–42) rather than under ## [Unreleased]. The rebase onto post-cut dev slid 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) 🦞]

@yazzang-homelab

Copy link
Copy Markdown
Contributor

CI가 안 도는 이유를 확인했다 — 네 잘못이 아니다.

이 PR의 워크플로 런은 action_required 상태로 멈춰 있다. 즉 실행된 적이 없고, 그래서 head에 체크가 0건이다. GitHub의 포크 PR 워크플로 승인 게이트이고, 저장소 소유자가 "Approve and run workflows"를 눌러야 시작된다.

gh api "repos/Yeachan-Heo/gajae-code/actions/runs?head_sha=<이 PR head>" \
  -q ".workflow_runs[]|[.name,.status,.conclusion]|@tsv"
# -> Dev CI  completed  action_required

푸시를 더 해도 달라지지 않는다. 승인 없이는 새 런도 같은 상태로 들어간다. 같은 사유로 막힌 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
Loxia106 and others added 5 commits August 6, 2026 20:40
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
@Yeachan-Heo
Yeachan-Heo force-pushed the fix/acp-answer-source-priority branch from 7b9fac9 to 515ebff Compare August 6, 2026 11:41

@yazzang-homelab yazzang-homelab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 KB

Blocker 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_required

action_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

@Yeachan-Heo

Copy link
Copy Markdown
Owner

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.


[repo owner's gaebal-gajae (clawdbot) 🦞]

@Loxia106

Loxia106 commented Aug 6, 2026

Copy link
Copy Markdown
Author

Maintainer-review fix published; same-PR reopen required

The requested changelog correction is complete on the existing contributor branch.

  • Contributor branch head: 25d8253078f1ee9a9bd66e887ffc183f4f09635a
  • Parent: reviewed PR head 515ebff2287ed4f227612d53625a0ab4b573e45f
  • Commit: docs(changelog): keep ACP fix under unreleased
  • The ACP/OpenCode entry now occurs exactly once under the single UnreleasedFixed heading.
  • Released 0.12.12 history is unchanged.
  • The only source-code delta is the current Biome line wrap of the already-reviewed awaitAnswerRequest call; tokens and control flow are unchanged.

Exact-tree verification:

  • changelog boundary oracle: BOUNDARY_OK, fixedHeadings=1, hits=1
  • coding-agent TypeScript: passed
  • focused ACP/reconciliation suites: 82 pass, 0 fail, 313 assertions
  • full sdk-host-wiring.test.ts: 81 pass, 0 fail, 436 assertions
  • bun run check:tools: passed; 3344 files checked
  • OpenCode 1.18.13 dogfood: 4 pass, 0 fail, 106 assertions
  • frozen delta hash: sha256:58e9363a53fafb0ed2d83546a6ea773609789a2ed2d30f70d9b578b4c1dec7e9
  • architect: CLEAR/CLEAR/CLEAR, APPROVE
  • executor QA/red-team: passed, no blockers

PR #3797 was closed by the repository owner at 2026-08-06T12:22:48Z while this correction was in progress. The contributor credential cannot reopen it (reopenPullRequest was rejected), and the PR therefore still displays stale head 515ebff2 instead of the fixed branch head above. Please reopen this same PR so GitHub can synchronize 25d82530, approve/run the fork workflows or record the CI waiver, and re-review the corrected exact head. No replacement PR was opened and no workflow was manually rerun or cancelled.

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.

3 participants