From 6b5650af943f1fd463595b22cfbcb4308f0548da Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 12:52:43 +0530 Subject: [PATCH 1/2] fix(acp): publish sub-run sessions loaded over ACP as view-only session/load registered a fully promptable session for any session kind, including the child, side and spec sub-runs that every other resume path refuses through sessions.IsResumableKind: the TUI, exec sessions, the exec CLI, and ACP's own session/resume. So a sub-run could be opened and continued as a standalone conversation through ACP and nowhere else, with whatever its parent run relied on about that sub-run's lifecycle no longer true. Load has a real render-only use, a client rebuilding a transcript it can scroll, so it still loads those sessions and still replays their history. What changes is that the session it publishes for a non-resumable kind is marked read-only, and session/prompt refuses it with an invalid-params error that says why. runTurn is reached only from session/prompt, so that one gate covers every way a turn can run; set_mode and the config options only change settings. The flag is decided at first registration and never changes. Resume refuses a non-resumable kind before it registers anything, and session/new always creates a standalone session, so the only path that can publish one of these ids is load, which now publishes it read-only. TestACPResumeAppliesStandaloneSessionKindPolicy claimed "session/load should retain render-only access", but its prompt check ran before the load, so it only proved the refused resume never registered the session. It now prompts after the load as well. TestACPLoadPublishesSubRunsReadOnly checks every kind in both directions, so a gate that refused everything would fail on the resumable kinds, with a setup guard that the table agrees with IsResumableKind. Fixes #1045 --- internal/acp/agent.go | 22 +++++++++--- internal/acp/agent_test.go | 70 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/internal/acp/agent.go b/internal/acp/agent.go index 7c301044c..003a2c1cb 100644 --- a/internal/acp/agent.go +++ b/internal/acp/agent.go @@ -106,6 +106,14 @@ type acpSession struct { restrictModels bool cancel context.CancelFunc history []turnRecord + + // readOnly marks a session published by session/load for a kind that is + // not resumable (a child, side or spec sub-run). Load still replays its + // transcript, which is the render-only use a client needs, but a prompt + // would continue the sub-run as a standalone conversation, which every + // other resume path refuses (sessions.IsResumableKind). Set once when the + // session is first registered and never changed, so it is read without mu. + readOnly bool } // NewAgent builds the ACP server and registers its method handlers on conn. @@ -204,7 +212,7 @@ func (a *Agent) handleSessionNew(ctx context.Context, params json.RawMessage) (a if err != nil { return nil, RPCError(codeInternalError, "create session: "+err.Error()) } - sess, _ := a.registerSession(meta.SessionID, root, nil, model, models, restrictModels) + sess, _ := a.registerSession(meta.SessionID, root, nil, model, models, restrictModels, false) return NewSessionResult{ SessionID: sess.id, ConfigOptions: a.configOptions(sess), @@ -311,7 +319,10 @@ func (a *Agent) activatePersistedSession(ctx context.Context, p LoadSessionParam models = append(models, SessionConfigOptionValue{Value: persistedModel, Name: persistedModel}) } } - sess, existed := a.registerSession(meta.SessionID, root, history, model, models, restrictModels) + // Resume has already refused a non-resumable kind above, so only load can + // reach here with one, and it publishes that session for viewing only. + readOnly := !sessions.IsResumableKind(meta.SessionKind) + sess, existed := a.registerSession(meta.SessionID, root, history, model, models, restrictModels, readOnly) if existed { _, messages, historyWarning, historyErr = a.refreshSessionHistory(sess, requireHistoryLog) if operation == persistedSessionResume && historyErr != nil { @@ -433,6 +444,9 @@ func (a *Agent) handleSessionPrompt(ctx context.Context, params json.RawMessage) if sess == nil { return nil, RPCError(codeInvalidParams, "unknown session: "+p.SessionID) } + if sess.readOnly { + return nil, RPCError(codeInvalidParams, "session is not resumable, so it was loaded for viewing only and cannot be prompted: "+p.SessionID) + } // Serialize turns for this session so two prompts can't interleave history or // fight over the single cancel slot. session/cancel still works concurrently @@ -1154,13 +1168,13 @@ func promptImages(blocks []ContentBlock) []zeroruntime.ImageBlock { // The bool reports that case so persisted activation can re-read and apply disk // history under turnMu. history is set BEFORE first publication so no concurrent // prompt can read a half-initialized session. -func (a *Agent) registerSession(id, cwd string, history []turnRecord, model string, models []SessionConfigOptionValue, restrictModels bool) (*acpSession, bool) { +func (a *Agent) registerSession(id, cwd string, history []turnRecord, model string, models []SessionConfigOptionValue, restrictModels bool, readOnly bool) (*acpSession, bool) { a.mu.Lock() defer a.mu.Unlock() if existing := a.sessions[id]; existing != nil { return existing, true } - sess := &acpSession{id: id, cwd: cwd, mode: agent.PermissionModeAuto, model: model, models: models, restrictModels: restrictModels, history: history} + sess := &acpSession{id: id, cwd: cwd, mode: agent.PermissionModeAuto, model: model, models: models, restrictModels: restrictModels, history: history, readOnly: readOnly} a.sessions[id] = sess return sess, false } diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 41457549e..90535cf77 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1251,6 +1251,76 @@ func TestACPResumeAppliesStandaloneSessionKindPolicy(t *testing.T) { }, &LoadSessionResult{}); err != nil { t.Fatalf("session/load should retain render-only access: %v", err) } + // The check above only showed the refused resume never registered the + // session. Load does register it, so the prompt that matters is the one + // AFTER the load: render-only means the transcript can be read and the + // sub-run cannot be continued. + err = h.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: created.SessionID, Prompt: []ContentBlock{TextBlock("must stay closed after load")}, + }, &PromptResult{}) + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { + t.Fatalf("session/prompt after load of a %s session = %v, want invalid params", tc.name, err) + } + }) + } +} + +// LOAD PUBLISHES A SUB-RUN FOR VIEWING, NOT FOR CONTINUING. Every other resume +// path (the TUI, exec sessions, the exec CLI, and ACP's own session/resume) +// refuses a kind sessions.IsResumableKind rejects. session/load has a real +// render-only use, a client rebuilding a transcript it can scroll, so it keeps +// loading those sessions; what it must not do is publish one that accepts a +// prompt, because the prompt would carry on a child, side or spec sub-run as a +// standalone conversation. Checked in both directions, so a gate that refused +// everything would fail on the resumable kinds. +func TestACPLoadPublishesSubRunsReadOnly(t *testing.T) { + for _, tc := range []struct { + name string + kind sessions.SessionKind + readOnly bool + }{ + {name: "regular", kind: ""}, + {name: "fork", kind: sessions.SessionKindFork}, + {name: "child", kind: sessions.SessionKindChild, readOnly: true}, + {name: "side", kind: sessions.SessionKindSide, readOnly: true}, + {name: "spec draft", kind: sessions.SessionKindSpecDraft, readOnly: true}, + {name: "spec impl", kind: sessions.SessionKindSpecImpl, readOnly: true}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := !sessions.IsResumableKind(tc.kind); got != tc.readOnly { + t.Fatalf("SETUP INVALID: IsResumableKind(%q) disagrees with this table (readOnly=%v), so the cases no longer mean what they say", tc.kind, tc.readOnly) + } + deps := testDeps(t) + workspace := t.TempDir() + created, err := deps.Store.Create(sessions.CreateInput{ + SessionID: "load-" + strings.ReplaceAll(tc.name, " ", "-"), Cwd: workspace, SessionKind: tc.kind, + }) + if err != nil { + t.Fatal(err) + } + h := newHarness(t, deps) + defer h.stop() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + if err := h.client.Call(ctx, MethodSessionLoad, LoadSessionParams{ + SessionID: created.SessionID, Cwd: workspace, + }, &LoadSessionResult{}); err != nil { + t.Fatalf("session/load of a %s session: %v", tc.name, err) + } + err = h.client.Call(ctx, MethodSessionPrompt, PromptParams{ + SessionID: created.SessionID, Prompt: []ContentBlock{TextBlock("continue")}, + }, &PromptResult{}) + refused := err != nil && strings.Contains(err.Error(), "loaded for viewing only") + if refused != tc.readOnly { + t.Fatalf("session/prompt after load of a %s session: refused as view-only = %v, want %v (err = %v)", tc.name, refused, tc.readOnly, err) + } + if tc.readOnly { + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { + t.Fatalf("view-only refusal = %v, want invalid params", err) + } + } }) } } From 71b1896778942ba15e71ecce38addbbc3b7be517 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Thu, 24 Sep 2026 13:37:54 +0530 Subject: [PATCH 2/2] test(acp): require resumable sessions to be promptable after load, not merely unrefused TestACPLoadPublishesSubRunsReadOnly decided the outcome from the error text: "refused as view-only" was false for any error other than the view-only one, so a regular or fork session whose prompt failed for some other reason still passed. With every writable prompt made to fail with an internal error, the old test stayed green. Raised by CodeRabbit on the PR. Resumable sessions now have to prompt successfully. For the sub-run kinds the decision is the RPC code, and the message is checked only to tell the view-only refusal apart from other invalid-params errors such as an unknown session id. --- internal/acp/agent_test.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/internal/acp/agent_test.go b/internal/acp/agent_test.go index 90535cf77..88635d99e 100644 --- a/internal/acp/agent_test.go +++ b/internal/acp/agent_test.go @@ -1311,15 +1311,24 @@ func TestACPLoadPublishesSubRunsReadOnly(t *testing.T) { err = h.client.Call(ctx, MethodSessionPrompt, PromptParams{ SessionID: created.SessionID, Prompt: []ContentBlock{TextBlock("continue")}, }, &PromptResult{}) - refused := err != nil && strings.Contains(err.Error(), "loaded for viewing only") - if refused != tc.readOnly { - t.Fatalf("session/prompt after load of a %s session: refused as view-only = %v, want %v (err = %v)", tc.name, refused, tc.readOnly, err) - } - if tc.readOnly { - var rpcErr *rpcError - if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { - t.Fatalf("view-only refusal = %v, want invalid params", err) + // A resumable session has to be genuinely promptable, not merely "not + // refused as view-only": any other failure would hide a broken + // writable path behind a passing test. + if !tc.readOnly { + if err != nil { + t.Fatalf("session/prompt after load of a %s session = %v, want success", tc.name, err) } + return + } + // The decision is the RPC code; the message only distinguishes the + // view-only refusal from other invalid-params errors such as an + // unknown session id. + var rpcErr *rpcError + if !errors.As(err, &rpcErr) || rpcErr.Code != codeInvalidParams { + t.Fatalf("session/prompt after load of a %s session = %v, want invalid params", tc.name, err) + } + if !strings.Contains(rpcErr.Message, "loaded for viewing only") { + t.Fatalf("session/prompt after load of a %s session was refused for another reason: %v", tc.name, err) } }) }