Skip to content

Commit c3d5329

Browse files
julianknutsenclaude
andcommitted
fix: address review findings for session submit intents
- Guard cmdSessionSuspend and cmdSessionWait with cityUsesManagedReconciler to prevent silently succeeding on unmanaged ad-hoc cities - Add SubmissionCapabilities to createAgentSession response to match createProviderSession - Restore StopTurn (POST /stop) to always use SIGINT; soft Escape is reserved for the submit interrupt_now path via stopTurnLocked - Fix emitSessionSubmitResult to distinguish queued follow-ups from immediately delivered ones on suspended sessions - Skip redundant stopTurnLocked before hard restart in interruptAndSubmitLocked since Stop() is a superset - Add justifying comment on PID file usage in nudge poller Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent eccb580 commit c3d5329

8 files changed

Lines changed: 91 additions & 35 deletions

File tree

cmd/gc/cmd_session.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,9 @@ func cmdSessionSuspend(args []string, stdout, stderr io.Writer) int {
730730
}
731731

732732
// Try reconciler-first path: set held_until metadata, poke controller.
733-
if cityErr == nil {
733+
// Only use this path when the city is managed by a standalone controller
734+
// or the machine-wide supervisor — not for unmanaged ad-hoc cities.
735+
if cityErr == nil && cityUsesManagedReconciler(cityPath) {
734736
if pokeErr := pokeController(cityPath); pokeErr == nil {
735737
// Controller is running — metadata-only suspend.
736738
// Set held_until far in the future so the reconciler drains/stops the session.
@@ -1173,8 +1175,10 @@ func cmdSessionSubmit(args []string, intent session.SubmitIntent, stdout, stderr
11731175

11741176
func emitSessionSubmitResult(stdout io.Writer, target string, intent session.SubmitIntent, queued bool) {
11751177
switch {
1176-
case queued || intent == session.SubmitIntentFollowUp:
1178+
case queued:
11771179
fmt.Fprintf(stdout, "Queued follow-up for %s\n", target) //nolint:errcheck // best-effort stdout
1180+
case intent == session.SubmitIntentFollowUp:
1181+
fmt.Fprintf(stdout, "Submitted follow-up to %s\n", target) //nolint:errcheck // best-effort stdout
11781182
case intent == session.SubmitIntentInterruptNow:
11791183
fmt.Fprintf(stdout, "Interrupted and submitted to %s\n", target) //nolint:errcheck // best-effort stdout
11801184
default:

cmd/gc/cmd_session_submit_test.go

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,26 @@ import (
88
"github.com/gastownhall/gascity/internal/session"
99
)
1010

11-
func TestEmitSessionSubmitResultFollowUp(t *testing.T) {
11+
func TestEmitSessionSubmitResultFollowUpQueued(t *testing.T) {
1212
var stdout bytes.Buffer
1313
emitSessionSubmitResult(&stdout, "mayor", session.SubmitIntentFollowUp, true)
1414
if got := stdout.String(); !strings.Contains(got, "Queued follow-up for mayor") {
1515
t.Fatalf("stdout = %q, want queued confirmation", got)
1616
}
1717
}
1818

19+
func TestEmitSessionSubmitResultFollowUpImmediate(t *testing.T) {
20+
var stdout bytes.Buffer
21+
emitSessionSubmitResult(&stdout, "mayor", session.SubmitIntentFollowUp, false)
22+
got := stdout.String()
23+
if strings.Contains(got, "Queued") {
24+
t.Fatalf("stdout = %q, should not say Queued when message was delivered immediately", got)
25+
}
26+
if !strings.Contains(got, "Submitted follow-up to mayor") {
27+
t.Fatalf("stdout = %q, want submitted follow-up confirmation", got)
28+
}
29+
}
30+
1931
func TestParseSessionSubmitIntentAcceptsLegacySpellings(t *testing.T) {
2032
cases := []struct {
2133
raw string

cmd/gc/cmd_wait.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,8 @@ func cmdSessionWait(args, depIDs []string, matchAny bool, note string, sleep boo
159159
}
160160
if sleep {
161161
cityPath, err := resolveCity()
162-
if err != nil || pokeController(cityPath) != nil {
163-
fmt.Fprintln(stderr, "gc session wait: controller must be running when --sleep is used") //nolint:errcheck
162+
if err != nil || !cityUsesManagedReconciler(cityPath) {
163+
fmt.Fprintln(stderr, "gc session wait: a managed controller must be running when --sleep is used") //nolint:errcheck
164164
return 1
165165
}
166166
}

internal/api/handler_session_chat.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,9 @@ func (s *Server) handleSessionCreate(w http.ResponseWriter, r *http.Request) {
403403

404404
resp := sessionToResponse(info, s.state.Config())
405405
resp.Kind = "agent"
406+
if caps, capErr := s.sessionManager(store).SubmissionCapabilities(info.ID); capErr == nil {
407+
resp.SubmissionCapabilities = caps
408+
}
406409
s.enrichSessionResponse(&resp, info, s.state.Config(), s.state.SessionProvider(), false)
407410
statusCode := http.StatusAccepted // always async for agent sessions
408411
s.idem.storeResponse(idemKey, bodyHash, statusCode, resp)

internal/api/handler_session_submit_test.go

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func TestHandleSessionGetIncludesSubmissionCapabilities(t *testing.T) {
148148
}
149149
}
150150

151-
func TestHandleSessionStopUsesSoftEscapeForCodex(t *testing.T) {
151+
func TestHandleSessionStopUsesInterruptForCodex(t *testing.T) {
152152
fs := newSessionFakeState(t)
153153
srv := New(fs)
154154

@@ -165,19 +165,15 @@ func TestHandleSessionStopUsesSoftEscapeForCodex(t *testing.T) {
165165
if rec.Code != http.StatusOK {
166166
t.Fatalf("stop status = %d, want %d; body: %s", rec.Code, http.StatusOK, rec.Body.String())
167167
}
168-
var sawEscape, sawInterrupt bool
168+
// StopTurn (backing /stop) always uses SIGINT regardless of provider.
169+
// Soft Escape is reserved for the submit interrupt_now path.
170+
var sawInterrupt bool
169171
for _, call := range fs.sp.Calls {
170-
if call.Method == "SendKeys" && call.Name == info.SessionName && call.Message == "Escape" {
171-
sawEscape = true
172-
}
173172
if call.Method == "Interrupt" && call.Name == info.SessionName {
174173
sawInterrupt = true
175174
}
176175
}
177-
if !sawEscape {
178-
t.Fatalf("calls = %#v, want SendKeys(Escape)", fs.sp.Calls)
179-
}
180-
if sawInterrupt {
181-
t.Fatalf("calls = %#v, did not want Interrupt for codex stop", fs.sp.Calls)
176+
if !sawInterrupt {
177+
t.Fatalf("calls = %#v, want Interrupt for StopTurn", fs.sp.Calls)
182178
}
183179
}

internal/session/chat.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,9 @@ func (m *Manager) SendImmediate(ctx context.Context, id, message, resumeCommand
371371
return m.send(ctx, id, message, resumeCommand, hints, true)
372372
}
373373

374-
// StopTurn issues a soft interrupt for the currently running turn.
374+
// StopTurn issues a hard interrupt (SIGINT) for the currently running turn.
375+
// This backs the POST /v0/session/{id}/stop API endpoint and always uses
376+
// SIGINT for reliable interruption across all provider states.
375377
// Pool-managed sessions are rejected: they have no human user, so
376378
// Claude Code's interactive "What should Claude do instead?" prompt
377379
// would hang them forever.
@@ -381,7 +383,16 @@ func (m *Manager) StopTurn(id string) error {
381383
if err != nil {
382384
return err
383385
}
384-
return m.stopTurnLocked(b, sessName)
386+
if State(b.Metadata["state"]) == StateSuspended || !m.sp.IsRunning(sessName) {
387+
return nil
388+
}
389+
if b.Metadata["pool_managed"] == "true" || strings.TrimSpace(b.Metadata["pool_slot"]) != "" {
390+
return fmt.Errorf("%w: %s", ErrPoolManaged, sessName)
391+
}
392+
if err := m.sp.Interrupt(sessName); err != nil {
393+
return fmt.Errorf("interrupting session: %w", err)
394+
}
395+
return nil
385396
})
386397
}
387398

internal/session/submit.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,17 @@ func (m *Manager) interruptAndSubmitLocked(ctx context.Context, id string, b bea
122122
if !running {
123123
return m.sendLocked(ctx, id, b, sessName, message, resumeCommand, hints, true)
124124
}
125-
if err := m.stopTurnLocked(b, sessName); err != nil {
126-
return err
127-
}
128125
if usesHardRestartSubmit(b) {
126+
// Hard-restart providers (Claude over tmux) are killed and restarted.
127+
// Stop() is a superset of stopTurnLocked(), so skip the intermediate
128+
// interrupt to avoid wasted latency.
129129
if err := m.sp.Stop(sessName); err != nil {
130130
return fmt.Errorf("stopping session before submit: %w", err)
131131
}
132+
} else {
133+
if err := m.stopTurnLocked(b, sessName); err != nil {
134+
return err
135+
}
132136
}
133137
return m.sendLocked(ctx, id, b, sessName, message, resumeCommand, hints, true)
134138
}
@@ -230,6 +234,11 @@ func deferredSubmitAgentKey(b beads.Bead) string {
230234

231235
var startSessionSubmitPoller = ensureSessionSubmitPoller
232236

237+
// ensureSessionSubmitPoller starts a background nudge poller if one is not
238+
// already running. PID files are used here for orphan bounding rather than
239+
// state tracking: the poller validates PID liveness via kill(pid, 0) and the
240+
// 15-second grace period in shouldKeepNudgePollerAlive caps orphan lifetime
241+
// on parent crash.
233242
func ensureSessionSubmitPoller(cityPath, agentName, sessionName string) error {
234243
pidPath := sessionSubmitPollerPIDPath(cityPath, sessionName)
235244
return withSessionSubmitPollerPIDLock(pidPath, func() error {

internal/session/submit_test.go

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -245,11 +245,8 @@ func TestSubmitInterruptNowFallsBackToHardRestartForClaude(t *testing.T) {
245245
t.Fatal("Submit(interrupt_now) unexpectedly queued")
246246
}
247247

248-
var sawInterrupt, sawStop, sawRestart, sawNudge bool
248+
var sawStop, sawRestart, sawNudge bool
249249
for _, call := range sp.Calls {
250-
if call.Method == "Interrupt" && call.Name == info.SessionName {
251-
sawInterrupt = true
252-
}
253250
if call.Method == "Stop" && call.Name == info.SessionName {
254251
sawStop = true
255252
}
@@ -260,12 +257,12 @@ func TestSubmitInterruptNowFallsBackToHardRestartForClaude(t *testing.T) {
260257
sawNudge = true
261258
}
262259
}
263-
if !sawInterrupt || !sawStop || !sawRestart || !sawNudge {
264-
t.Fatalf("calls = %#v, want interrupt + stop + restart + nudge", sp.Calls)
260+
if !sawStop || !sawRestart || !sawNudge {
261+
t.Fatalf("calls = %#v, want stop + restart + nudge (no intermediate interrupt)", sp.Calls)
265262
}
266263
}
267264

268-
func TestStopTurnUsesSoftEscapeForCodex(t *testing.T) {
265+
func TestStopTurnUsesInterruptForCodex(t *testing.T) {
269266
store := beads.NewMemStore()
270267
sp := runtime.NewFake()
271268
mgr := NewManager(store, sp)
@@ -279,19 +276,43 @@ func TestStopTurnUsesSoftEscapeForCodex(t *testing.T) {
279276
t.Fatalf("StopTurn: %v", err)
280277
}
281278

282-
var sawEscape, sawInterrupt bool
279+
// StopTurn always uses SIGINT (Interrupt) regardless of provider.
280+
// Soft Escape is only used by the submit interrupt_now path via stopTurnLocked.
281+
var sawInterrupt bool
283282
for _, call := range sp.Calls {
284-
if call.Method == "SendKeys" && call.Name == info.SessionName && call.Message == "Escape" {
285-
sawEscape = true
286-
}
287283
if call.Method == "Interrupt" && call.Name == info.SessionName {
288284
sawInterrupt = true
289285
}
290286
}
291-
if !sawEscape {
292-
t.Fatalf("calls = %#v, want SendKeys(Escape)", sp.Calls)
287+
if !sawInterrupt {
288+
t.Fatalf("calls = %#v, want Interrupt for StopTurn", sp.Calls)
293289
}
294-
if sawInterrupt {
295-
t.Fatalf("calls = %#v, did not want Interrupt for codex stop", sp.Calls)
290+
}
291+
292+
func TestSubmitInterruptNowUsesSoftEscapeForCodex(t *testing.T) {
293+
store := beads.NewMemStore()
294+
sp := runtime.NewFake()
295+
mgr := NewManager(store, sp)
296+
297+
info, err := mgr.Create(context.Background(), "helper", "", "codex", t.TempDir(), "codex", nil, ProviderResume{}, runtime.Config{})
298+
if err != nil {
299+
t.Fatalf("Create: %v", err)
300+
}
301+
302+
_, err = mgr.Submit(context.Background(), info.ID, "replace", BuildResumeCommand(info), runtime.Config{WorkDir: info.WorkDir}, SubmitIntentInterruptNow)
303+
if err != nil {
304+
t.Fatalf("Submit(interrupt_now): %v", err)
305+
}
306+
307+
// The submit interrupt_now path uses stopTurnLocked which sends
308+
// soft Escape for codex instead of SIGINT.
309+
var sawEscape bool
310+
for _, call := range sp.Calls {
311+
if call.Method == "SendKeys" && call.Name == info.SessionName && call.Message == "Escape" {
312+
sawEscape = true
313+
}
314+
}
315+
if !sawEscape {
316+
t.Fatalf("calls = %#v, want SendKeys(Escape) via submit interrupt_now", sp.Calls)
296317
}
297318
}

0 commit comments

Comments
 (0)