fix(acp): settle prompts correctly and make content, limits, and cancellation spec-conformant - #3950
Conversation
8220561 to
c14b447
Compare
5be0900 to
0fa81fb
Compare
|
I reproduced the #3949 correlation loss through a Hermes -> GJC ralplan approval gate and validated the correlation commit at this PR head. Evidence:
Would you be open to folding this small assertion into the existing continuation test? The local test-only diff is: + const emitter = new BrokerWorkflowGateEmitter(sessionId, new FileGateStore(path.join(cwd, ".gjc", "state", "workflow-gates.json")));
- const sessionContext = context(cwd, sessionId);
+ const sessionContext = context(cwd, sessionId, "main", {}, emitter);
// ... accept prompt, first agent_start, second agent_start
+ const advance = emitter.emitGate({
+ stage: "ralplan",
+ kind: "approval",
+ schema: { type: "string", enum: ["approve"] },
+ });
+ const gate = emitter.listWorkflowGateQueryRecords!()[0];
+ expect(gate).toMatchObject({ tag: "pending", runtime_turn_id: turnId });
+ await emitter.resolveGate!({
+ gate_id: gate!.gate_id,
+ answer: "approve",
+ idempotency_key: "continuation-gate-runtime-turn",
+ });
+ expect(await advance).toBe("approve");This is complementary to #3951: this PR preserves the SDK prompt turn ID through continuation starts; #3951 addresses coordinator state-file/current-turn propagation. |
|
Correction to my previous comment: an exact isolated Hermes -> GJC ralplan smoke against this PR head still reproduced the coordinator failure.
The current regression proves the duplicate agent_start case, but the live nested-subagent lifecycle has at least one additional boundary that clears the parent correlation. Please treat my earlier sufficiency claim as withdrawn and do not fold the proposed assertion as proof of a complete fix yet. I am tracing the remaining event sequence and will follow up with a discriminating regression and minimal patch. |
Resolves the CHANGELOG.md unreleased-section conflict that made Yeachan-Heo#3950 unmergeable. Both dev's entries and this branch's ACP entries are kept; no code changes. Lore-id: 3950merge Confidence: high Scope-risk: narrow Reversibility: easy Tested: bun test (ACP + SDK host + todo suites, 168 pass), bun --cwd=packages/coding-agent run check, live Paseo ACP smoke
Local verification against Paseo (CLI/daemon 0.2.5)Tested this branch in isolation —
#3949 — prompt never settles on continuationReproduced the exact path and it now settles. Transcript shows the two
Follow-up turn on the same session (write a file, then confirm): #3962 — conformance
Gates
Merge conflictMerged |
…cal resume Managed legacy-local migration calls captureTree on a session sibling `local/` tree that foreign/legacy writers often leave group/other-readable. Snapshot failed closed with mode_mismatch and aborted switchSession, including the task fallback authority restore path. Re-secure owner-only once and retry, matching managed-scope prepare self-heal, without relaxing the owner-only contract. Reproduces on plain origin/dev (baseline-wide); landed on Yeachan-Heo#3950 so ACP CI is unblocked after rebase onto current dev. Lore-id: 3950mode1 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: no-session-output-refs (17), task-managed-descendants (incl. new heal unit), coding-agent check Not-tested: full CI matrix
171b538 to
e6293cf
Compare
…3976) * fix(session): resecure managed legacy-local mode_mismatch on resume switchSession migration of artifacts/<id>/local aborted with bare mode_mismatch when the legacy tree was written with group/other-readable modes (default umask / non-managed writers). Scope prepare already self-heals that class of drift; legacy-local capture did not. On recoverable mode_mismatch, re-apply owner-only security once under the absolute legacy local path and retry captureTree before failing closed. Regression fixture plants 0o755/0o644 modes so the path stays covered (no assertion relaxation). Baseline-wide session contract — separate from ACP #3950 branch ownership. Lore-id: legacy-local-mode-self-heal-1 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: not offline (natives missing); exact-head CI verifies the failing test Not-tested: full managed-scope matrix offline * style(session): biome organize-imports for legacy-local self-heal Mechanical CI gate: coding-agent biome rejected import ordering of the new managed-session-scope helpers in session-manager.ts. No behavior change. * fix(session): actually resecure owner-only modes on managed-scope self-heal After #3951 root-store identity binding, prepareManagedSessionScopeForWriteSync no longer walked the managed tree on construct, so mode_mismatch self-heal never ran and prepare returned resolved while descendants stayed group/other-readable (e.g. 0o036 / 0o644). - After store open, snapshotManagedTree("") when retained authority is bound so drifted descendants still surface mode_mismatch - reapplyOwnerOnlyManagedTree now apply+verify and throws on failure instead of best-effort swallowing ignored native results Proved: managed-scope-owner-only-self-heal (3 pass) + no-session-output-refs legacy-local fallback test (1 pass); coding-agent check clean. Lore-id: scope-mode-self-heal-2 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: managed-scope-owner-only-self-heal; no-session-output-refs legacy-local; bun --cwd packages/coding-agent run check Not-tested: full CI matrix offline * fix(session): mode-only drift detect without snapshotManagedTree race snapshotManagedTree(\"\") after store open reintroduced concurrent-writer identity_mismatch races (#3906) that break SessionManager.moveTo (/move). Detect group/other-readable descendants with a mode-only lstat walk instead, then resecure+verify and retry. Keeps owner-only self-heal for drifted modes (0o755/0o644 → group/other 000) without altering root-identity construct. Proved: managed-scope-owner-only-self-heal + sdk-move-cwd + move-to + legacy-local fallback; coding-agent check clean. Telegram generation guard on prior head: pass (unrelated to this authority path). Lore-id: scope-mode-self-heal-3 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: managed-scope-owner-only-self-heal; sdk-move-cwd; session-manager/move-to; no-session-output-refs legacy-local; coding-agent check Not-tested: full CI matrix offline --------- Co-authored-by: gaebal-gajae <gaebal-gajae@users.noreply.github.com>
A continuation (todo reminder, TTSR resume, auto-continue) re-enters the agent loop inside one prompt and emits a second `agent_start`. That handler shifted the already-empty pending queue and overwrote the live correlation with `undefined`, so the prompt's `agent_end` carried no identity, terminalizePrompt never ran, and ACP clients hung until the 30-minute prompt deadline even though the answer had already streamed to them. Lore-id: 7c1a4e92 Constraint: agent_start must stay the binding point for a fresh prompt -- only skip the claim when this session already owns one Constraint: a continuation must not publish a duplicate agent_start lifecycle frame to SDK clients Rejected: reset activePromptCorrelation on agent_end only | the clobber happens before agent_end, so the terminal is already orphaned Rejected: give ACP its own prompt watchdog | masks the lost correlation instead of fixing it, and still reports a wrong stop reason Confidence: high Scope-risk: medium Reversibility: easy Tested: regression test times out on the old code and passes on the fixed code; live ACP wire probe returns stopReason end_turn in 30s; paseo run --provider gjc reports status completed Not-tested: ACP cancel racing a continuation agent_start
Smoke-testing GJC against the Paseo ACP client surfaced a cluster of
conformance defects: an attached image reached the model but never appeared in
the client transcript, a 244 KiB image killed the session with an opaque
connection_closed, a user's own cancel surfaced as a spurious error, and
todo_write hard-failed mid-turn whenever the model spelled the operation
"complete".
Lore-id: 3fb27c14
Constraint: prompt size must be measured inside the real control_request envelope -- SdkClient adds {type,operation,input,id} before the socket sees it
Constraint: involuntary teardown must keep rejecting; only client-initiated close/cancel may resolve as cancelled
Constraint: locally configured `sse` MCP entries must keep working even though the capability is no longer advertised
Rejected: implement legacy MCP HTTP+SSE | the transport is deprecated by the MCP spec and unused here; advertising a transport we cannot connect to was the actual bug
Rejected: give ACP its own prompt watchdog | masks the transport ceiling instead of reporting it
Rejected: relax todo_write raw-key validation | the strictness is correct; only the done/complete vocabulary mismatch needed fixing
Confidence: high
Scope-risk: medium
Reversibility: easy
Tested: live Paseo runs for image rendering, oversize rejection (-32602), and answer correctness; reverse-frame and adapter boundary tests verified failing before the fix
Not-tested: legacy MCP SSE servers (deliberately unimplemented); session/load image replay
todo_write hard-failed mid-turn whenever the model wrote `op: "complete"` or `content:` instead of `task:`. Both spellings come straight from the tool's own vocabulary -- the operation sets a status named `completed`, and a task is stored and rendered as `content` -- so the schema invited the mistake and the strict raw validator then rejected it before coercion could repair anything. Also moves the ACP changelog entries into Unreleased; the rebase had landed them inside the already-released 0.12.13 section. Lore-id: 5a9e33c7 Constraint: the accepted key set must not widen -- `content` is normalized to `task`, never stored Constraint: a done/drop op must still name a task or phase, including through an alias Rejected: relax the raw-key rejection wholesale | the strictness is correct and catches real malformed payloads Rejected: rename the ops to match the stored field | breaks every existing caller and the documented vocabulary Confidence: high Scope-risk: low Reversibility: easy Tested: alias acceptance, canonical spellings, unknown ops and unknown keys still rejected, target still required; verified failing before the fix Not-tested: string-encoded `ops` payloads, which already worked
…hat names the session Skills never appeared in Paseo. `session/new` scheduled its bootstrap `session/update` notifications on a 50ms timer taken before the session-state queries ran, so whenever those queries took longer the notifications overtook the response carrying the sessionId. Measured live: `available_commands_update` arrived 27ms before the `session/new` response, naming a session the client had never seen, so it was dropped. Lore-id: 9c41e0f2 Constraint: only `session/load` may stream updates before its response -- new/resume/fork must not Constraint: a macrotask is required, not a microtask, so it lands after the response is written Rejected: raise the timer | any fixed delay is still a guess about how slow the host is Rejected: buffer updates in the client bridge | the ordering bug belongs to the agent, not the client Confidence: high Scope-risk: low Reversibility: easy Tested: ordering assertion with an injected session-state stall, verified failing before the fix; live wire capture shows response before notifications Not-tested: Paseo desktop GUI rendering, which I could not capture -- the window stays on another Space/display
…o target
Observed on the wire: the model sends `{"op":"complete","id":"1"}`. String-encoded
`ops` skip the raw key validator entirely, Zod then strips the unknown `id`, and
what reaches execute is a bare `{op:"done"}` whose error said only "Missing task
or phase" -- never what to use instead. The prompt already forbids positional
ids, so the lever that remains is the error the model actually reads.
Lore-id: 7d8e21ab
Constraint: do not alias `id` to `task` -- `id: "1"` is a position, not content, and would silently match nothing
Rejected: accept an index-based target | task order is not stable across ops within one call
Rejected: reject string-encoded `ops` | they already work and models emit them constantly
Confidence: medium
Scope-risk: low
Reversibility: easy
Tested: targetless done returns the actionable message; existing alias and rejection cases unchanged
Not-tested: whether models recover on the first retry more often -- needs live sampling
…cal resume Managed legacy-local migration calls captureTree on a session sibling `local/` tree that foreign/legacy writers often leave group/other-readable. Snapshot failed closed with mode_mismatch and aborted switchSession, including the task fallback authority restore path. Re-secure owner-only once and retry, matching managed-scope prepare self-heal, without relaxing the owner-only contract. Reproduces on plain origin/dev (baseline-wide); landed on Yeachan-Heo#3950 so ACP CI is unblocked after rebase onto current dev. Lore-id: 3950mode1 Confidence: high Scope-risk: narrow Reversibility: trivial Tested: no-session-output-refs (17), task-managed-descendants (incl. new heal unit), coding-agent check Not-tested: full CI matrix
e6293cf to
f0176d8
Compare
Yeachan-Heo#3976 is the sole owner of managed legacy-local mode_mismatch self-heal. Restore managed-session-storage and task-managed-descendants to origin/dev and remove the parallel captureTree changelog line so this PR keeps only ACP/prompt/todo contracts. Lore-id: 3950strip1 Confidence: high Scope-risk: narrow Reversibility: trivial
Ownership transfer: managed legacy-local → #3976Dropped the parallel Sole owner: #3976 (merged #3950 scope retained: ACP prompt correlation, content/limits/cancellation, todo synonym acceptance, bootstrap timing only. Waiting fresh exact-head CI on — |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Red-team — MERGE_READY / CLEAR
Exact head: 044ffc58051e6856db504679339cc1d11b3ecc8b | Dev CI: run 31145275075 success (24 success / 6 skipped / 0 failure)
Scope (after strip of managed-legacy-local)
ACP prompt settlement, content/limits/cancellation conformance, mid-prompt correlation, todo-write synonym/addressing, reverse-leases/host wiring tests. Explicitly dropped managed-session-storage ownership (belongs to #3976, already merged).
Review
- Large but coherent ACP/SDK contract surface with proportional tests.
- Non-overlapping with #3979 CI matrix.
mergeable_state: cleanon currentdev.
Verdict
MERGE_READY / CLEAR
— census terminal red-team
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Terminal red-team (exact head 044ffc5)
Verdict: MERGE_READY / APPROVE
- Contract: ACP prompt settlement, content/limits/cancellation conformance; todo-write continue correlation.
- Scope (post-cleanup): ACP adapter, agent-session continue path, todo-write, tests — no managed-session-storage / baseline mode_mismatch ownership (that lives in merged #3976).
- Exact-head CI: green (24 success / 6 skipped); mergeable=clean; contains current origin/dev.
- Independent of #3846/#3764 perf red chains and notifications baseline.
Merge authorized.
|
Re-verified the reduced-scope head Compiled binary from
Transcript confirms the regression path is exercised — the same Tests on One note on |
Red-team verdict: MERGE_READY → mergingHead: Scope (ACP/prompt only after ownership strip)
Adversarial checks
Residual risk
Verdict: MERGE_READY — |
Fixes #3949. Fixes #3962.
Two rounds of work from smoke-testing GJC's ACP mode against the Paseo client (CLI/daemon 0.2.5), then auditing the implementation against the ACP v1 spec.
1.
session/promptnever settled when the agent continued mid-prompt (#3949)api.on("agent_start")unconditionally claimed the next queued correlation. A continuation (todo reminder, TTSR resume, auto-continue) re-enters the agent loop within the same prompt and emits a secondagent_start, soshift()on the already-empty queue returnedundefinedand clobbered the live correlation:agent_endthen took the uncorrelated branch, so no terminal was published and ACP's#handleSdkFramedropped it for lack of a complete correlation — the client hung until the 30-minute deadline despite the answer having already streamed.agent_startnow claims a pending correlation only when the session has no active one, and#checkTodoCompletionholds the predecessoragent_endlike every other#scheduleAgentContinuecall site.2. ACP conformance defects (#3962)
user_message_chunkat all (it existed only on thesession/loadreplay path), so an attached image reached the model but never appeared in the client transcript.session/promptnow echoes the prompt's text and image content blocks before dispatching the turn.CloseCode::Size, which reached clients as an opaque-32603 connection_closed. The prompt is now measured inside its realcontrol_requestenvelope (the{type,operation,input,id}wrapperSdkClientadds) and rejected with-32602naming the actual and permitted size.session/cancelandsession/closesettle the prompt ascancelled. A cancel landing during preflight rejected withbusy, andsession/closerejected withconnection_closed; both showed the user a spurious error for their own action. Involuntary teardown still rejects.mcpCapabilities.sseis no longer advertised. Legacy MCP HTTP+SSE (deprecated in the MCP spec) is not implemented —sseconfigs route to the Streamable HTTP transport, which never performs theendpoint-event handshake. Locally configuredsseentries still resolve throughcreateTransportand keep working; only the false advertisement to ACP clients is removed.result, so a near-limitfs/read_text_fileresult raises the typedpayload_too_largeinstead of tripping the total-frame ceiling and closing the session.todo_writeacceptscomplete/completedas aliases fordone. The status this op sets is spelledcompleted, so models repeatedly emittedop:"complete"and hit a hard mid-turn tool failure. The vocabulary is otherwise unchanged and a completion still requires a task or phase target.Verification
Regression tests, each confirmed failing before its fix and passing after:
sdk-host-wiring.test.ts— continuationagent_startkeeps the correlation (times out on the old code, reproducing the hang)sdk-reverse-rpc.test.ts+sdk-acp-adapter.test.ts— inner result under the cap whose full frame exceeds it is rejectedtools/todo-write.test.ts—complete/completedaliases, canonicaldonestill works, unknown ops still rejected, and an aliased completion without a target is still rejectedacp-prompt-conformance.test.ts— image blocks survive payload conversion; frame-size boundary arithmeticruntime-mcp-sse-transport.test.ts— pins that ansseconfig really is served by Streamable HTTPSuites: 134 pass / 0 fail across the ACP, SDK, reverse-RPC, todo, and host-wiring files.
bun --cwd=packages/coding-agent run check(biome + tsc) clean.Live, after
bun run build:native && bun run install:dev:bin:paseo run --provider gjc --image <out-of-workspace png>→status: completed, answeredmagenta, black, yellow(unguessable, tools disabled)user_message_chunkcarryingmime image/png b64len 372(previously zero image-bearing updates)-32602 Invalid params: ACP prompt is 261 KiB, over the 256 KiB transport limitwith the session still alive (previously-32603 connection_closed)Not covered
Two findings from the same audit are left open and documented in #3962:
session/loadreplay drops user images (transcriptReplayContentalways reportsimages:{available:false}), andpromptCapabilities.embeddedContextis advertised while non-image blob resources are rejected. ACP cancel racing a continuationagent_startis also untested.