fix(subagents): harden lifecycle review findings
- commit
- 8f2fcb4601ab4d747c3e2df40440372445c0d5d7
- author
- brice · · 2026-08-07 14:00 UTC
- parent
- 4206363
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
10 files · +130 -43
M packages/agent/build.go +3 -0
| @@ -1340,6 +1340,9 @@ func autoSubagentsToolAllowedFor(args Args, toolName string) bool { | |||
| 1340 | 1340 | if args.NoTools || args.PermissionSet != nil { | |
| 1341 | 1341 | return false | |
| 1342 | 1342 | } | |
| 1343 | + | if args.ToolsSet && len(args.Tools) == 0 { | |
| 1344 | + | return false | |
| 1345 | + | } | |
| 1343 | 1346 | if len(args.Tools) == 0 { | |
| 1344 | 1347 | return true | |
| 1345 | 1348 | } | |
| @@ -1340,6 +1340,9 @@ func autoSubagentsToolAllowedFor(args Args, toolName string) bool { | |||||
| 1340 | if args.NoTools || args.PermissionSet != nil { | 1340 | if args.NoTools || args.PermissionSet != nil { | ||
| 1341 | return false | 1341 | return false | ||
| 1342 | } | 1342 | } | ||
| 1343 | + | if args.ToolsSet && len(args.Tools) == 0 { | |||
| 1344 | + | return false | |||
| 1345 | + | } | |||
| 1343 | if len(args.Tools) == 0 { | 1346 | if len(args.Tools) == 0 { | ||
| 1344 | return true | 1347 | return true | ||
| 1345 | } | 1348 | } | ||
M packages/agent/build_test.go +3 -1
| @@ -233,6 +233,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||
| 233 | 233 | for _, tc := range []struct { | |
| 234 | 234 | name string | |
| 235 | 235 | tools []string | |
| 236 | + | toolsSet bool | |
| 236 | 237 | wantSpawn bool | |
| 237 | 238 | wantStatus bool | |
| 238 | 239 | wantStop bool | |
| @@ -240,6 +241,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||
| 240 | 241 | wantAny bool | |
| 241 | 242 | }{ | |
| 242 | 243 | {name: "default", wantSpawn: true, wantStatus: true, wantStop: true, wantResume: true, wantAny: true}, | |
| 244 | + | {name: "explicit empty", toolsSet: true}, | |
| 243 | 245 | {name: "spawn", tools: []string{"subagent_spawn"}, wantSpawn: true, wantAny: true}, | |
| 244 | 246 | {name: "status", tools: []string{"subagent_status"}, wantStatus: true, wantAny: true}, | |
| 245 | 247 | {name: "stop", tools: []string{"subagent_stop"}, wantStop: true, wantAny: true}, | |
| @@ -248,7 +250,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||
| 248 | 250 | {name: "no tools", wantAny: false, tools: nil}, | |
| 249 | 251 | } { | |
| 250 | 252 | t.Run(tc.name, func(t *testing.T) { | |
| 251 | - | args := Args{Tools: tc.tools} | |
| 253 | + | args := Args{Tools: tc.tools, ToolsSet: tc.toolsSet} | |
| 252 | 254 | if tc.name == "no tools" { | |
| 253 | 255 | args.NoTools = true | |
| 254 | 256 | } | |
| @@ -233,6 +233,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||||
| 233 | for _, tc := range []struct { | 233 | for _, tc := range []struct { | ||
| 234 | name string | 234 | name string | ||
| 235 | tools []string | 235 | tools []string | ||
| 236 | + | toolsSet bool | |||
| 236 | wantSpawn bool | 237 | wantSpawn bool | ||
| 237 | wantStatus bool | 238 | wantStatus bool | ||
| 238 | wantStop bool | 239 | wantStop bool | ||
| @@ -240,6 +241,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||||
| 240 | wantAny bool | 241 | wantAny bool | ||
| 241 | }{ | 242 | }{ | ||
| 242 | {name: "default", wantSpawn: true, wantStatus: true, wantStop: true, wantResume: true, wantAny: true}, | 243 | {name: "default", wantSpawn: true, wantStatus: true, wantStop: true, wantResume: true, wantAny: true}, | ||
| 244 | + | {name: "explicit empty", toolsSet: true}, | |||
| 243 | {name: "spawn", tools: []string{"subagent_spawn"}, wantSpawn: true, wantAny: true}, | 245 | {name: "spawn", tools: []string{"subagent_spawn"}, wantSpawn: true, wantAny: true}, | ||
| 244 | {name: "status", tools: []string{"subagent_status"}, wantStatus: true, wantAny: true}, | 246 | {name: "status", tools: []string{"subagent_status"}, wantStatus: true, wantAny: true}, | ||
| 245 | {name: "stop", tools: []string{"subagent_stop"}, wantStop: true, wantAny: true}, | 247 | {name: "stop", tools: []string{"subagent_stop"}, wantStop: true, wantAny: true}, | ||
| @@ -248,7 +250,7 @@ func TestAutoSubagentsToolPoliciesTrackEachTool(t *testing.T) { | |||||
| 248 | {name: "no tools", wantAny: false, tools: nil}, | 250 | {name: "no tools", wantAny: false, tools: nil}, | ||
| 249 | } { | 251 | } { | ||
| 250 | t.Run(tc.name, func(t *testing.T) { | 252 | t.Run(tc.name, func(t *testing.T) { | ||
| 251 | - | args := Args{Tools: tc.tools} | 253 | + | args := Args{Tools: tc.tools, ToolsSet: tc.toolsSet} |
| 252 | if tc.name == "no tools" { | 254 | if tc.name == "no tools" { | ||
| 253 | args.NoTools = true | 255 | args.NoTools = true | ||
| 254 | } | 256 | } | ||
M packages/agent/subagents/agent.go +17 -14
| @@ -80,11 +80,12 @@ type Agent struct { | |||
| 80 | 80 | // Not persisted — every Resume sets it explicitly. | |
| 81 | 81 | Resuming bool | |
| 82 | 82 | ||
| 83 | - | // ResumePrompt is an optional manager follow-up passed to a resumed child as | |
| 84 | - | // its initial prompt. It remains in durable metadata until the child reports | |
| 85 | - | // turn.started, so a host exit while the worker is queued cannot lose it. | |
| 86 | - | ResumePrompt string | |
| 87 | - | ResumePromptAt time.Time | |
| 83 | + | // resumePromptText is an optional manager follow-up passed to a resumed | |
| 84 | + | // child as its initial prompt. It remains in durable metadata until the | |
| 85 | + | // child reports turn.started, so a host exit while the worker is queued | |
| 86 | + | // cannot lose it. Access it through ResumePromptInfo. | |
| 87 | + | resumePromptText string | |
| 88 | + | resumePromptAt time.Time | |
| 88 | 89 | ||
| 89 | 90 | // InboxPath is the unix socket the child agent listens on for | |
| 90 | 91 | // follow-up prompts and control messages. The supervisor opens | |
| @@ -387,33 +388,35 @@ func (a *Agent) stateDirectory(defaultRoot string) string { | |||
| 387 | 388 | // turn_end event with a step field. Safe to call from any goroutine: | |
| 388 | 389 | // the runner reads the callback under the same mutex. | |
| 389 | 390 | func (a *Agent) resumePrompt() string { | |
| 390 | - | prompt, _ := a.resumePromptInfo() | |
| 391 | + | prompt, _ := a.ResumePromptInfo() | |
| 391 | 392 | return prompt | |
| 392 | 393 | } | |
| 393 | 394 | ||
| 394 | - | func (a *Agent) resumePromptInfo() (string, time.Time) { | |
| 395 | + | // ResumePromptInfo returns the queued manager follow-up and its acceptance | |
| 396 | + | // time. Both values are read as one synchronized snapshot. | |
| 397 | + | func (a *Agent) ResumePromptInfo() (string, time.Time) { | |
| 395 | 398 | a.mu.Lock() | |
| 396 | 399 | defer a.mu.Unlock() | |
| 397 | - | return a.ResumePrompt, a.ResumePromptAt | |
| 400 | + | return a.resumePromptText, a.resumePromptAt | |
| 398 | 401 | } | |
| 399 | 402 | ||
| 400 | 403 | func (a *Agent) setResumePrompt(prompt string, acceptedAt time.Time) (string, time.Time) { | |
| 401 | 404 | a.mu.Lock() | |
| 402 | 405 | defer a.mu.Unlock() | |
| 403 | - | previousPrompt, previousAcceptedAt := a.ResumePrompt, a.ResumePromptAt | |
| 404 | - | a.ResumePrompt = prompt | |
| 405 | - | a.ResumePromptAt = acceptedAt | |
| 406 | + | previousPrompt, previousAcceptedAt := a.resumePromptText, a.resumePromptAt | |
| 407 | + | a.resumePromptText = prompt | |
| 408 | + | a.resumePromptAt = acceptedAt | |
| 406 | 409 | return previousPrompt, previousAcceptedAt | |
| 407 | 410 | } | |
| 408 | 411 | ||
| 409 | 412 | func (a *Agent) clearResumePrompt() bool { | |
| 410 | 413 | a.mu.Lock() | |
| 411 | 414 | defer a.mu.Unlock() | |
| 412 | - | if a.ResumePrompt == "" { | |
| 415 | + | if a.resumePromptText == "" { | |
| 413 | 416 | return false | |
| 414 | 417 | } | |
| 415 | - | a.ResumePrompt = "" | |
| 416 | - | a.ResumePromptAt = time.Time{} | |
| 418 | + | a.resumePromptText = "" | |
| 419 | + | a.resumePromptAt = time.Time{} | |
| 417 | 420 | return true | |
| 418 | 421 | } | |
| 419 | 422 | ||
| @@ -80,11 +80,12 @@ type Agent struct { | |||||
| 80 | // Not persisted — every Resume sets it explicitly. | 80 | // Not persisted — every Resume sets it explicitly. | ||
| 81 | Resuming bool | 81 | Resuming bool | ||
| 82 | 82 | ||||
| 83 | - | // ResumePrompt is an optional manager follow-up passed to a resumed child as | 83 | + | // resumePromptText is an optional manager follow-up passed to a resumed |
| 84 | - | // its initial prompt. It remains in durable metadata until the child reports | 84 | + | // child as its initial prompt. It remains in durable metadata until the |
| 85 | - | // turn.started, so a host exit while the worker is queued cannot lose it. | 85 | + | // child reports turn.started, so a host exit while the worker is queued |
| 86 | - | ResumePrompt string | 86 | + | // cannot lose it. Access it through ResumePromptInfo. |
| 87 | - | ResumePromptAt time.Time | 87 | + | resumePromptText string |
| 88 | + | resumePromptAt time.Time | |||
| 88 | 89 | ||||
| 89 | // InboxPath is the unix socket the child agent listens on for | 90 | // InboxPath is the unix socket the child agent listens on for | ||
| 90 | // follow-up prompts and control messages. The supervisor opens | 91 | // follow-up prompts and control messages. The supervisor opens | ||
| @@ -387,33 +388,35 @@ func (a *Agent) stateDirectory(defaultRoot string) string { | |||||
| 387 | // turn_end event with a step field. Safe to call from any goroutine: | 388 | // turn_end event with a step field. Safe to call from any goroutine: | ||
| 388 | // the runner reads the callback under the same mutex. | 389 | // the runner reads the callback under the same mutex. | ||
| 389 | func (a *Agent) resumePrompt() string { | 390 | func (a *Agent) resumePrompt() string { | ||
| 390 | - | prompt, _ := a.resumePromptInfo() | 391 | + | prompt, _ := a.ResumePromptInfo() |
| 391 | return prompt | 392 | return prompt | ||
| 392 | } | 393 | } | ||
| 393 | 394 | ||||
| 394 | - | func (a *Agent) resumePromptInfo() (string, time.Time) { | 395 | + | // ResumePromptInfo returns the queued manager follow-up and its acceptance |
| 396 | + | // time. Both values are read as one synchronized snapshot. | |||
| 397 | + | func (a *Agent) ResumePromptInfo() (string, time.Time) { | |||
| 395 | a.mu.Lock() | 398 | a.mu.Lock() | ||
| 396 | defer a.mu.Unlock() | 399 | defer a.mu.Unlock() | ||
| 397 | - | return a.ResumePrompt, a.ResumePromptAt | 400 | + | return a.resumePromptText, a.resumePromptAt |
| 398 | } | 401 | } | ||
| 399 | 402 | ||||
| 400 | func (a *Agent) setResumePrompt(prompt string, acceptedAt time.Time) (string, time.Time) { | 403 | func (a *Agent) setResumePrompt(prompt string, acceptedAt time.Time) (string, time.Time) { | ||
| 401 | a.mu.Lock() | 404 | a.mu.Lock() | ||
| 402 | defer a.mu.Unlock() | 405 | defer a.mu.Unlock() | ||
| 403 | - | previousPrompt, previousAcceptedAt := a.ResumePrompt, a.ResumePromptAt | 406 | + | previousPrompt, previousAcceptedAt := a.resumePromptText, a.resumePromptAt |
| 404 | - | a.ResumePrompt = prompt | 407 | + | a.resumePromptText = prompt |
| 405 | - | a.ResumePromptAt = acceptedAt | 408 | + | a.resumePromptAt = acceptedAt |
| 406 | return previousPrompt, previousAcceptedAt | 409 | return previousPrompt, previousAcceptedAt | ||
| 407 | } | 410 | } | ||
| 408 | 411 | ||||
| 409 | func (a *Agent) clearResumePrompt() bool { | 412 | func (a *Agent) clearResumePrompt() bool { | ||
| 410 | a.mu.Lock() | 413 | a.mu.Lock() | ||
| 411 | defer a.mu.Unlock() | 414 | defer a.mu.Unlock() | ||
| 412 | - | if a.ResumePrompt == "" { | 415 | + | if a.resumePromptText == "" { |
| 413 | return false | 416 | return false | ||
| 414 | } | 417 | } | ||
| 415 | - | a.ResumePrompt = "" | 418 | + | a.resumePromptText = "" |
| 416 | - | a.ResumePromptAt = time.Time{} | 419 | + | a.resumePromptAt = time.Time{} |
| 417 | return true | 420 | return true | ||
| 418 | } | 421 | } | ||
| 419 | 422 | ||||
M packages/agent/subagents/inbox.go +3 -0
| @@ -219,6 +219,9 @@ func dialUnixContext(ctx context.Context, path string, timeout time.Duration) (n | |||
| 219 | 219 | case <-wait.C: | |
| 220 | 220 | } | |
| 221 | 221 | } | |
| 222 | + | if lastErr == nil { | |
| 223 | + | return nil, ErrNotReady | |
| 224 | + | } | |
| 222 | 225 | if _, statErr := os.Stat(path); os.IsNotExist(statErr) { | |
| 223 | 226 | return nil, ErrNotReady | |
| 224 | 227 | } | |
| @@ -219,6 +219,9 @@ func dialUnixContext(ctx context.Context, path string, timeout time.Duration) (n | |||||
| 219 | case <-wait.C: | 219 | case <-wait.C: | ||
| 220 | } | 220 | } | ||
| 221 | } | 221 | } | ||
| 222 | + | if lastErr == nil { | |||
| 223 | + | return nil, ErrNotReady | |||
| 224 | + | } | |||
| 222 | if _, statErr := os.Stat(path); os.IsNotExist(statErr) { | 225 | if _, statErr := os.Stat(path); os.IsNotExist(statErr) { | ||
| 223 | return nil, ErrNotReady | 226 | return nil, ErrNotReady | ||
| 224 | } | 227 | } | ||
M packages/agent/subagents/inbox_test.go +13 -0
| @@ -95,6 +95,19 @@ func TestInboxNotReady(t *testing.T) { | |||
| 95 | 95 | } | |
| 96 | 96 | } | |
| 97 | 97 | ||
| 98 | + | func TestDialUnixContextNonPositiveTimeoutIsNotReady(t *testing.T) { | |
| 99 | + | path := filepath.Join(t.TempDir(), "existing") | |
| 100 | + | if err := os.WriteFile(path, nil, 0o600); err != nil { | |
| 101 | + | t.Fatal(err) | |
| 102 | + | } | |
| 103 | + | for _, timeout := range []time.Duration{0, -time.Millisecond} { | |
| 104 | + | _, err := dialUnixContext(context.Background(), path, timeout) | |
| 105 | + | if !errors.Is(err, ErrNotReady) { | |
| 106 | + | t.Fatalf("dial timeout %s error = %v, want ErrNotReady", timeout, err) | |
| 107 | + | } | |
| 108 | + | } | |
| 109 | + | } | |
| 110 | + | ||
| 98 | 111 | type blockingWriteConn struct { | |
| 99 | 112 | net.Conn | |
| 100 | 113 | started chan struct{} | |
| @@ -95,6 +95,19 @@ func TestInboxNotReady(t *testing.T) { | |||||
| 95 | } | 95 | } | ||
| 96 | } | 96 | } | ||
| 97 | 97 | ||||
| 98 | + | func TestDialUnixContextNonPositiveTimeoutIsNotReady(t *testing.T) { | |||
| 99 | + | path := filepath.Join(t.TempDir(), "existing") | |||
| 100 | + | if err := os.WriteFile(path, nil, 0o600); err != nil { | |||
| 101 | + | t.Fatal(err) | |||
| 102 | + | } | |||
| 103 | + | for _, timeout := range []time.Duration{0, -time.Millisecond} { | |||
| 104 | + | _, err := dialUnixContext(context.Background(), path, timeout) | |||
| 105 | + | if !errors.Is(err, ErrNotReady) { | |||
| 106 | + | t.Fatalf("dial timeout %s error = %v, want ErrNotReady", timeout, err) | |||
| 107 | + | } | |||
| 108 | + | } | |||
| 109 | + | } | |||
| 110 | + | ||||
| 98 | type blockingWriteConn struct { | 111 | type blockingWriteConn struct { | ||
| 99 | net.Conn | 112 | net.Conn | ||
| 100 | started chan struct{} | 113 | started chan struct{} | ||
M packages/agent/subagents/persist.go +11 -16
| @@ -102,8 +102,8 @@ func writeAgentMeta(stateDir string, a *Agent) error { | |||
| 102 | 102 | status := a.status | |
| 103 | 103 | activity := a.activity | |
| 104 | 104 | finished := a.finished | |
| 105 | - | resumePrompt := a.ResumePrompt | |
| 106 | - | resumePromptAt := a.ResumePromptAt | |
| 105 | + | resumePrompt := a.resumePromptText | |
| 106 | + | resumePromptAt := a.resumePromptAt | |
| 107 | 107 | errText := "" | |
| 108 | 108 | if a.lastErr != nil { | |
| 109 | 109 | errText = a.lastErr.Error() | |
| @@ -449,8 +449,8 @@ func (f *Supervisor) buildDetachedAgent(m agentMeta) *Agent { | |||
| 449 | 449 | InboxPath: m.InboxPath, | |
| 450 | 450 | EventLogPath: m.EventLogPath, | |
| 451 | 451 | SessionPath: m.SessionPath, | |
| 452 | - | ResumePrompt: m.ResumePrompt, | |
| 453 | - | ResumePromptAt: m.ResumePromptAt, | |
| 452 | + | resumePromptText: m.ResumePrompt, | |
| 453 | + | resumePromptAt: m.ResumePromptAt, | |
| 454 | 454 | SessionID: m.SessionID, | |
| 455 | 455 | inbox: NewInbox(m.InboxPath), | |
| 456 | 456 | status: status, | |
| @@ -542,7 +542,7 @@ func (a *Agent) loadTranscript() error { | |||
| 542 | 542 | if err != nil { | |
| 543 | 543 | return fmt.Errorf("read subagent transcript: %w", err) | |
| 544 | 544 | } | |
| 545 | - | if prompt, acceptedAt := a.resumePromptInfo(); prompt != "" && resumePromptAcknowledged(events, acceptedAt) { | |
| 545 | + | if prompt, acceptedAt := a.ResumePromptInfo(); prompt != "" && resumePromptAcknowledged(events, acceptedAt) { | |
| 546 | 546 | a.clearResumePrompt() | |
| 547 | 547 | } | |
| 548 | 548 | a.mu.Lock() | |
| @@ -852,15 +852,10 @@ func (f *Supervisor) resume(ctx context.Context, id string, resuming bool, resum | |||
| 852 | 852 | } | |
| 853 | 853 | now := f.cfg.Now() | |
| 854 | 854 | fastMode := existing.FastMode | |
| 855 | - | pendingResumePrompt := "" | |
| 856 | - | pendingResumePromptAt := time.Time{} | |
| 857 | - | if resuming { | |
| 858 | - | pendingResumePrompt = firstNonEmpty(resumePrompt, existing.resumePrompt()) | |
| 859 | - | if resumePrompt != "" { | |
| 860 | - | pendingResumePromptAt = now | |
| 861 | - | } else if pendingResumePrompt != "" { | |
| 862 | - | _, pendingResumePromptAt = existing.resumePromptInfo() | |
| 863 | - | } | |
| 855 | + | pendingResumePrompt, pendingResumePromptAt := existing.ResumePromptInfo() | |
| 856 | + | if resumePrompt != "" { | |
| 857 | + | pendingResumePrompt = resumePrompt | |
| 858 | + | pendingResumePromptAt = now | |
| 864 | 859 | } | |
| 865 | 860 | m := agentMeta{ | |
| 866 | 861 | ID: existing.ID, Task: existing.Task, | |
| @@ -968,8 +963,8 @@ func (f *Supervisor) resume(ctx context.Context, id string, resuming bool, resum | |||
| 968 | 963 | EventLogPath: m.EventLogPath, | |
| 969 | 964 | SessionPath: m.SessionPath, | |
| 970 | 965 | Resuming: resuming, | |
| 971 | - | ResumePrompt: pendingResumePrompt, | |
| 972 | - | ResumePromptAt: pendingResumePromptAt, | |
| 966 | + | resumePromptText: pendingResumePrompt, | |
| 967 | + | resumePromptAt: pendingResumePromptAt, | |
| 973 | 968 | inbox: NewInbox(m.InboxPath), | |
| 974 | 969 | status: StatusPending, | |
| 975 | 970 | activity: "resuming", | |
| @@ -102,8 +102,8 @@ func writeAgentMeta(stateDir string, a *Agent) error { | |||||
| 102 | status := a.status | 102 | status := a.status | ||
| 103 | activity := a.activity | 103 | activity := a.activity | ||
| 104 | finished := a.finished | 104 | finished := a.finished | ||
| 105 | - | resumePrompt := a.ResumePrompt | 105 | + | resumePrompt := a.resumePromptText |
| 106 | - | resumePromptAt := a.ResumePromptAt | 106 | + | resumePromptAt := a.resumePromptAt |
| 107 | errText := "" | 107 | errText := "" | ||
| 108 | if a.lastErr != nil { | 108 | if a.lastErr != nil { | ||
| 109 | errText = a.lastErr.Error() | 109 | errText = a.lastErr.Error() | ||
| @@ -449,8 +449,8 @@ func (f *Supervisor) buildDetachedAgent(m agentMeta) *Agent { | |||||
| 449 | InboxPath: m.InboxPath, | 449 | InboxPath: m.InboxPath, | ||
| 450 | EventLogPath: m.EventLogPath, | 450 | EventLogPath: m.EventLogPath, | ||
| 451 | SessionPath: m.SessionPath, | 451 | SessionPath: m.SessionPath, | ||
| 452 | - | ResumePrompt: m.ResumePrompt, | 452 | + | resumePromptText: m.ResumePrompt, |
| 453 | - | ResumePromptAt: m.ResumePromptAt, | 453 | + | resumePromptAt: m.ResumePromptAt, |
| 454 | SessionID: m.SessionID, | 454 | SessionID: m.SessionID, | ||
| 455 | inbox: NewInbox(m.InboxPath), | 455 | inbox: NewInbox(m.InboxPath), | ||
| 456 | status: status, | 456 | status: status, | ||
| @@ -542,7 +542,7 @@ func (a *Agent) loadTranscript() error { | |||||
| 542 | if err != nil { | 542 | if err != nil { | ||
| 543 | return fmt.Errorf("read subagent transcript: %w", err) | 543 | return fmt.Errorf("read subagent transcript: %w", err) | ||
| 544 | } | 544 | } | ||
| 545 | - | if prompt, acceptedAt := a.resumePromptInfo(); prompt != "" && resumePromptAcknowledged(events, acceptedAt) { | 545 | + | if prompt, acceptedAt := a.ResumePromptInfo(); prompt != "" && resumePromptAcknowledged(events, acceptedAt) { |
| 546 | a.clearResumePrompt() | 546 | a.clearResumePrompt() | ||
| 547 | } | 547 | } | ||
| 548 | a.mu.Lock() | 548 | a.mu.Lock() | ||
| @@ -852,15 +852,10 @@ func (f *Supervisor) resume(ctx context.Context, id string, resuming bool, resum | |||||
| 852 | } | 852 | } | ||
| 853 | now := f.cfg.Now() | 853 | now := f.cfg.Now() | ||
| 854 | fastMode := existing.FastMode | 854 | fastMode := existing.FastMode | ||
| 855 | - | pendingResumePrompt := "" | 855 | + | pendingResumePrompt, pendingResumePromptAt := existing.ResumePromptInfo() |
| 856 | - | pendingResumePromptAt := time.Time{} | 856 | + | if resumePrompt != "" { |
| 857 | - | if resuming { | 857 | + | pendingResumePrompt = resumePrompt |
| 858 | - | pendingResumePrompt = firstNonEmpty(resumePrompt, existing.resumePrompt()) | 858 | + | pendingResumePromptAt = now |
| 859 | - | if resumePrompt != "" { | |||
| 860 | - | pendingResumePromptAt = now | |||
| 861 | - | } else if pendingResumePrompt != "" { | |||
| 862 | - | _, pendingResumePromptAt = existing.resumePromptInfo() | |||
| 863 | - | } | |||
| 864 | } | 859 | } | ||
| 865 | m := agentMeta{ | 860 | m := agentMeta{ | ||
| 866 | ID: existing.ID, Task: existing.Task, | 861 | ID: existing.ID, Task: existing.Task, | ||
| @@ -968,8 +963,8 @@ func (f *Supervisor) resume(ctx context.Context, id string, resuming bool, resum | |||||
| 968 | EventLogPath: m.EventLogPath, | 963 | EventLogPath: m.EventLogPath, | ||
| 969 | SessionPath: m.SessionPath, | 964 | SessionPath: m.SessionPath, | ||
| 970 | Resuming: resuming, | 965 | Resuming: resuming, | ||
| 971 | - | ResumePrompt: pendingResumePrompt, | 966 | + | resumePromptText: pendingResumePrompt, |
| 972 | - | ResumePromptAt: pendingResumePromptAt, | 967 | + | resumePromptAt: pendingResumePromptAt, |
| 973 | inbox: NewInbox(m.InboxPath), | 968 | inbox: NewInbox(m.InboxPath), | ||
| 974 | status: StatusPending, | 969 | status: StatusPending, | ||
| 975 | activity: "resuming", | 970 | activity: "resuming", | ||
M packages/agent/subagents/persist_test.go +69 -4
| @@ -513,6 +513,69 @@ func TestResumeRestartsRunnerOnSameSession(t *testing.T) { | |||
| 513 | 513 | } | |
| 514 | 514 | } | |
| 515 | 515 | ||
| 516 | + | func TestRestartTaskRetainsQueuedResumePrompt(t *testing.T) { | |
| 517 | + | root := t.TempDir() | |
| 518 | + | started := make(chan *Agent, 2) | |
| 519 | + | f := New(Config{ | |
| 520 | + | Root: root, RepoRoot: root, | |
| 521 | + | NewRunner: func(a *Agent) Runner { | |
| 522 | + | return RunnerFunc(func(ctx context.Context, _ Sink) error { | |
| 523 | + | started <- a | |
| 524 | + | <-ctx.Done() | |
| 525 | + | return ctx.Err() | |
| 526 | + | }) | |
| 527 | + | }, | |
| 528 | + | }) | |
| 529 | + | t.Cleanup(f.StopAll) | |
| 530 | + | ||
| 531 | + | existing, err := f.Spawn(context.Background(), "review the implementation") | |
| 532 | + | if err != nil { | |
| 533 | + | t.Fatal(err) | |
| 534 | + | } | |
| 535 | + | select { | |
| 536 | + | case got := <-started: | |
| 537 | + | if got != existing { | |
| 538 | + | t.Fatalf("initial runner agent = %p, want %p", got, existing) | |
| 539 | + | } | |
| 540 | + | case <-time.After(time.Second): | |
| 541 | + | t.Fatal("initial runner did not start") | |
| 542 | + | } | |
| 543 | + | ||
| 544 | + | const followUp = "I applied your review. What do you think now?" | |
| 545 | + | acceptedAt := time.Now().Add(-time.Minute) | |
| 546 | + | existing.setResumePrompt(followUp, acceptedAt) | |
| 547 | + | f.persistAgent(existing) | |
| 548 | + | if err := f.Stop(existing.ID); err != nil { | |
| 549 | + | t.Fatal(err) | |
| 550 | + | } | |
| 551 | + | existing.Wait() | |
| 552 | + | ||
| 553 | + | restarted, err := f.RestartTask(context.Background(), existing.ID) | |
| 554 | + | if err != nil { | |
| 555 | + | t.Fatal(err) | |
| 556 | + | } | |
| 557 | + | select { | |
| 558 | + | case got := <-started: | |
| 559 | + | if got != restarted { | |
| 560 | + | t.Fatalf("restarted runner agent = %p, want %p", got, restarted) | |
| 561 | + | } | |
| 562 | + | resumePrompt, resumePromptAt := got.ResumePromptInfo() | |
| 563 | + | if got.Resuming || resumePrompt != followUp || !resumePromptAt.Equal(acceptedAt) { | |
| 564 | + | t.Fatalf("restarted lifecycle = resuming %t prompt %q accepted %s, want false, prompt %q, and accepted %s", got.Resuming, resumePrompt, resumePromptAt, followUp, acceptedAt) | |
| 565 | + | } | |
| 566 | + | case <-time.After(time.Second): | |
| 567 | + | t.Fatal("restarted runner did not start") | |
| 568 | + | } | |
| 569 | + | ||
| 570 | + | persisted, err := readAgentMeta(filepath.Join(root, "agents", existing.ID)) | |
| 571 | + | if err != nil { | |
| 572 | + | t.Fatal(err) | |
| 573 | + | } | |
| 574 | + | if persisted.ResumePrompt != followUp || !persisted.ResumePromptAt.Equal(acceptedAt) { | |
| 575 | + | t.Fatalf("persisted restart prompt = %q accepted %s, want prompt %q and accepted %s", persisted.ResumePrompt, persisted.ResumePromptAt, followUp, acceptedAt) | |
| 576 | + | } | |
| 577 | + | } | |
| 578 | + | ||
| 516 | 579 | func TestResumeRecalculatesInvalidStateInboxPath(t *testing.T) { | |
| 517 | 580 | if runtime.GOOS == "windows" { | |
| 518 | 581 | t.Skip("subagent inbox runtime relocation applies to Unix sockets") | |
| @@ -771,8 +834,9 @@ func TestResumeWithPromptRetainsSessionAndStartsFollowUp(t *testing.T) { | |||
| 771 | 834 | if resumed.ID != first.ID || resumed.SessionPath != first.SessionPath { | |
| 772 | 835 | t.Fatalf("resumed agent = id %q session %q, want id %q session %q", resumed.ID, resumed.SessionPath, first.ID, first.SessionPath) | |
| 773 | 836 | } | |
| 774 | - | if !resumed.Resuming || resumed.ResumePrompt != followUp || resumed.ResumePromptAt.IsZero() { | |
| 775 | - | t.Fatalf("resumed lifecycle = resuming %t prompt %q accepted %s, want true, prompt %q, and timestamp", resumed.Resuming, resumed.ResumePrompt, resumed.ResumePromptAt, followUp) | |
| 837 | + | resumePrompt, resumePromptAt := resumed.ResumePromptInfo() | |
| 838 | + | if !resumed.Resuming || resumePrompt != followUp || resumePromptAt.IsZero() { | |
| 839 | + | t.Fatalf("resumed lifecycle = resuming %t prompt %q accepted %s, want true, prompt %q, and timestamp", resumed.Resuming, resumePrompt, resumePromptAt, followUp) | |
| 776 | 840 | } | |
| 777 | 841 | persisted, err := readAgentMeta(filepath.Join(root, "agents", first.ID)) | |
| 778 | 842 | if err != nil { | |
| @@ -786,8 +850,9 @@ func TestResumeWithPromptRetainsSessionAndStartsFollowUp(t *testing.T) { | |||
| 786 | 850 | if got != resumed { | |
| 787 | 851 | t.Fatalf("follow-up runner agent = %p, want %p", got, resumed) | |
| 788 | 852 | } | |
| 789 | - | if got.ResumePrompt != followUp { | |
| 790 | - | t.Fatalf("runner follow-up = %q, want %q", got.ResumePrompt, followUp) | |
| 853 | + | resumePrompt, _ := got.ResumePromptInfo() | |
| 854 | + | if resumePrompt != followUp { | |
| 855 | + | t.Fatalf("runner follow-up = %q, want %q", resumePrompt, followUp) | |
| 791 | 856 | } | |
| 792 | 857 | case <-time.After(time.Second): | |
| 793 | 858 | t.Fatal("resumed runner did not start") | |
| @@ -513,6 +513,69 @@ func TestResumeRestartsRunnerOnSameSession(t *testing.T) { | |||||
| 513 | } | 513 | } | ||
| 514 | } | 514 | } | ||
| 515 | 515 | ||||
| 516 | + | func TestRestartTaskRetainsQueuedResumePrompt(t *testing.T) { | |||
| 517 | + | root := t.TempDir() | |||
| 518 | + | started := make(chan *Agent, 2) | |||
| 519 | + | f := New(Config{ | |||
| 520 | + | Root: root, RepoRoot: root, | |||
| 521 | + | NewRunner: func(a *Agent) Runner { | |||
| 522 | + | return RunnerFunc(func(ctx context.Context, _ Sink) error { | |||
| 523 | + | started <- a | |||
| 524 | + | <-ctx.Done() | |||
| 525 | + | return ctx.Err() | |||
| 526 | + | }) | |||
| 527 | + | }, | |||
| 528 | + | }) | |||
| 529 | + | t.Cleanup(f.StopAll) | |||
| 530 | + | ||||
| 531 | + | existing, err := f.Spawn(context.Background(), "review the implementation") | |||
| 532 | + | if err != nil { | |||
| 533 | + | t.Fatal(err) | |||
| 534 | + | } | |||
| 535 | + | select { | |||
| 536 | + | case got := <-started: | |||
| 537 | + | if got != existing { | |||
| 538 | + | t.Fatalf("initial runner agent = %p, want %p", got, existing) | |||
| 539 | + | } | |||
| 540 | + | case <-time.After(time.Second): | |||
| 541 | + | t.Fatal("initial runner did not start") | |||
| 542 | + | } | |||
| 543 | + | ||||
| 544 | + | const followUp = "I applied your review. What do you think now?" | |||
| 545 | + | acceptedAt := time.Now().Add(-time.Minute) | |||
| 546 | + | existing.setResumePrompt(followUp, acceptedAt) | |||
| 547 | + | f.persistAgent(existing) | |||
| 548 | + | if err := f.Stop(existing.ID); err != nil { | |||
| 549 | + | t.Fatal(err) | |||
| 550 | + | } | |||
| 551 | + | existing.Wait() | |||
| 552 | + | ||||
| 553 | + | restarted, err := f.RestartTask(context.Background(), existing.ID) | |||
| 554 | + | if err != nil { | |||
| 555 | + | t.Fatal(err) | |||
| 556 | + | } | |||
| 557 | + | select { | |||
| 558 | + | case got := <-started: | |||
| 559 | + | if got != restarted { | |||
| 560 | + | t.Fatalf("restarted runner agent = %p, want %p", got, restarted) | |||
| 561 | + | } | |||
| 562 | + | resumePrompt, resumePromptAt := got.ResumePromptInfo() | |||
| 563 | + | if got.Resuming || resumePrompt != followUp || !resumePromptAt.Equal(acceptedAt) { | |||
| 564 | + | t.Fatalf("restarted lifecycle = resuming %t prompt %q accepted %s, want false, prompt %q, and accepted %s", got.Resuming, resumePrompt, resumePromptAt, followUp, acceptedAt) | |||
| 565 | + | } | |||
| 566 | + | case <-time.After(time.Second): | |||
| 567 | + | t.Fatal("restarted runner did not start") | |||
| 568 | + | } | |||
| 569 | + | ||||
| 570 | + | persisted, err := readAgentMeta(filepath.Join(root, "agents", existing.ID)) | |||
| 571 | + | if err != nil { | |||
| 572 | + | t.Fatal(err) | |||
| 573 | + | } | |||
| 574 | + | if persisted.ResumePrompt != followUp || !persisted.ResumePromptAt.Equal(acceptedAt) { | |||
| 575 | + | t.Fatalf("persisted restart prompt = %q accepted %s, want prompt %q and accepted %s", persisted.ResumePrompt, persisted.ResumePromptAt, followUp, acceptedAt) | |||
| 576 | + | } | |||
| 577 | + | } | |||
| 578 | + | ||||
| 516 | func TestResumeRecalculatesInvalidStateInboxPath(t *testing.T) { | 579 | func TestResumeRecalculatesInvalidStateInboxPath(t *testing.T) { | ||
| 517 | if runtime.GOOS == "windows" { | 580 | if runtime.GOOS == "windows" { | ||
| 518 | t.Skip("subagent inbox runtime relocation applies to Unix sockets") | 581 | t.Skip("subagent inbox runtime relocation applies to Unix sockets") | ||
| @@ -771,8 +834,9 @@ func TestResumeWithPromptRetainsSessionAndStartsFollowUp(t *testing.T) { | |||||
| 771 | if resumed.ID != first.ID || resumed.SessionPath != first.SessionPath { | 834 | if resumed.ID != first.ID || resumed.SessionPath != first.SessionPath { | ||
| 772 | t.Fatalf("resumed agent = id %q session %q, want id %q session %q", resumed.ID, resumed.SessionPath, first.ID, first.SessionPath) | 835 | t.Fatalf("resumed agent = id %q session %q, want id %q session %q", resumed.ID, resumed.SessionPath, first.ID, first.SessionPath) | ||
| 773 | } | 836 | } | ||
| 774 | - | if !resumed.Resuming || resumed.ResumePrompt != followUp || resumed.ResumePromptAt.IsZero() { | 837 | + | resumePrompt, resumePromptAt := resumed.ResumePromptInfo() |
| 775 | - | t.Fatalf("resumed lifecycle = resuming %t prompt %q accepted %s, want true, prompt %q, and timestamp", resumed.Resuming, resumed.ResumePrompt, resumed.ResumePromptAt, followUp) | 838 | + | if !resumed.Resuming || resumePrompt != followUp || resumePromptAt.IsZero() { |
| 839 | + | t.Fatalf("resumed lifecycle = resuming %t prompt %q accepted %s, want true, prompt %q, and timestamp", resumed.Resuming, resumePrompt, resumePromptAt, followUp) | |||
| 776 | } | 840 | } | ||
| 777 | persisted, err := readAgentMeta(filepath.Join(root, "agents", first.ID)) | 841 | persisted, err := readAgentMeta(filepath.Join(root, "agents", first.ID)) | ||
| 778 | if err != nil { | 842 | if err != nil { | ||
| @@ -786,8 +850,9 @@ func TestResumeWithPromptRetainsSessionAndStartsFollowUp(t *testing.T) { | |||||
| 786 | if got != resumed { | 850 | if got != resumed { | ||
| 787 | t.Fatalf("follow-up runner agent = %p, want %p", got, resumed) | 851 | t.Fatalf("follow-up runner agent = %p, want %p", got, resumed) | ||
| 788 | } | 852 | } | ||
| 789 | - | if got.ResumePrompt != followUp { | 853 | + | resumePrompt, _ := got.ResumePromptInfo() |
| 790 | - | t.Fatalf("runner follow-up = %q, want %q", got.ResumePrompt, followUp) | 854 | + | if resumePrompt != followUp { |
| 855 | + | t.Fatalf("runner follow-up = %q, want %q", resumePrompt, followUp) | |||
| 791 | } | 856 | } | ||
| 792 | case <-time.After(time.Second): | 857 | case <-time.After(time.Second): | ||
| 793 | t.Fatal("resumed runner did not start") | 858 | t.Fatal("resumed runner did not start") | ||
M packages/agent/subagents/runner_test.go +7 -6
| @@ -366,14 +366,15 @@ func TestDefaultChildArgsResumeOmitsTask(t *testing.T) { | |||
| 366 | 366 | ||
| 367 | 367 | func TestDefaultChildArgsResumeUsesFollowUpPrompt(t *testing.T) { | |
| 368 | 368 | a := &Agent{ | |
| 369 | - | Dir: "/wt", | |
| 370 | - | Task: "review the change", | |
| 371 | - | Resuming: true, | |
| 372 | - | ResumePrompt: "I applied your feedback. Please review it again.", | |
| 369 | + | Dir: "/wt", | |
| 370 | + | Task: "review the change", | |
| 371 | + | Resuming: true, | |
| 373 | 372 | } | |
| 373 | + | const followUp = "I applied your feedback. Please review it again." | |
| 374 | + | a.setResumePrompt(followUp, time.Now()) | |
| 374 | 375 | args := defaultChildArgs("/zut", a, "/s.json", "/in.sock") | |
| 375 | - | if got := args[len(args)-1]; got != a.ResumePrompt { | |
| 376 | - | t.Fatalf("resume argv last = %q, want follow-up prompt %q\n%v", got, a.ResumePrompt, args) | |
| 376 | + | if got := args[len(args)-1]; got != followUp { | |
| 377 | + | t.Fatalf("resume argv last = %q, want follow-up prompt %q\n%v", got, followUp, args) | |
| 377 | 378 | } | |
| 378 | 379 | for _, v := range args[:len(args)-1] { | |
| 379 | 380 | if v == a.Task { | |
| @@ -366,14 +366,15 @@ func TestDefaultChildArgsResumeOmitsTask(t *testing.T) { | |||||
| 366 | 366 | ||||
| 367 | func TestDefaultChildArgsResumeUsesFollowUpPrompt(t *testing.T) { | 367 | func TestDefaultChildArgsResumeUsesFollowUpPrompt(t *testing.T) { | ||
| 368 | a := &Agent{ | 368 | a := &Agent{ | ||
| 369 | - | Dir: "/wt", | 369 | + | Dir: "/wt", |
| 370 | - | Task: "review the change", | 370 | + | Task: "review the change", |
| 371 | - | Resuming: true, | 371 | + | Resuming: true, |
| 372 | - | ResumePrompt: "I applied your feedback. Please review it again.", | |||
| 373 | } | 372 | } | ||
| 373 | + | const followUp = "I applied your feedback. Please review it again." | |||
| 374 | + | a.setResumePrompt(followUp, time.Now()) | |||
| 374 | args := defaultChildArgs("/zut", a, "/s.json", "/in.sock") | 375 | args := defaultChildArgs("/zut", a, "/s.json", "/in.sock") | ||
| 375 | - | if got := args[len(args)-1]; got != a.ResumePrompt { | 376 | + | if got := args[len(args)-1]; got != followUp { |
| 376 | - | t.Fatalf("resume argv last = %q, want follow-up prompt %q\n%v", got, a.ResumePrompt, args) | 377 | + | t.Fatalf("resume argv last = %q, want follow-up prompt %q\n%v", got, followUp, args) |
| 377 | } | 378 | } | ||
| 378 | for _, v := range args[:len(args)-1] { | 379 | for _, v := range args[:len(args)-1] { | ||
| 379 | if v == a.Task { | 380 | if v == a.Task { | ||
M packages/agent/subagents/supervisor_test.go +1 -0
| @@ -297,6 +297,7 @@ func TestStopDetachedWorkerClosesWaiters(t *testing.T) { | |||
| 297 | 297 | if err != nil { | |
| 298 | 298 | t.Fatal(err) | |
| 299 | 299 | } | |
| 300 | + | t.Cleanup(func() { _ = listener.Close() }) | |
| 300 | 301 | ||
| 301 | 302 | root := t.TempDir() | |
| 302 | 303 | f := New(Config{Root: root, RepoRoot: root}) | |
| @@ -297,6 +297,7 @@ func TestStopDetachedWorkerClosesWaiters(t *testing.T) { | |||||
| 297 | if err != nil { | 297 | if err != nil { | ||
| 298 | t.Fatal(err) | 298 | t.Fatal(err) | ||
| 299 | } | 299 | } | ||
| 300 | + | t.Cleanup(func() { _ = listener.Close() }) | |||
| 300 | 301 | ||||
| 301 | root := t.TempDir() | 302 | root := t.TempDir() | ||
| 302 | f := New(Config{Root: root, RepoRoot: root}) | 303 | f := New(Config{Root: root, RepoRoot: root}) | ||
M packages/agent/tools/subagent_lifecycle_test.go +3 -2
| @@ -149,8 +149,9 @@ func TestSubagentResumeRestartsSessionWithFollowUp(t *testing.T) { | |||
| 149 | 149 | if resumed.SessionPath != first.SessionPath { | |
| 150 | 150 | t.Fatalf("resumed session = %q, want %q", resumed.SessionPath, first.SessionPath) | |
| 151 | 151 | } | |
| 152 | - | if !resumed.Resuming || resumed.ResumePrompt != prompt { | |
| 153 | - | t.Fatalf("resumed lifecycle = resuming %t prompt %q, want true and %q", resumed.Resuming, resumed.ResumePrompt, prompt) | |
| 152 | + | resumePrompt, _ := resumed.ResumePromptInfo() | |
| 153 | + | if !resumed.Resuming || resumePrompt != prompt { | |
| 154 | + | t.Fatalf("resumed lifecycle = resuming %t prompt %q, want true and %q", resumed.Resuming, resumePrompt, prompt) | |
| 154 | 155 | } | |
| 155 | 156 | case <-time.After(time.Second): | |
| 156 | 157 | t.Fatal("resumed subagent did not start") | |
| @@ -149,8 +149,9 @@ func TestSubagentResumeRestartsSessionWithFollowUp(t *testing.T) { | |||||
| 149 | if resumed.SessionPath != first.SessionPath { | 149 | if resumed.SessionPath != first.SessionPath { | ||
| 150 | t.Fatalf("resumed session = %q, want %q", resumed.SessionPath, first.SessionPath) | 150 | t.Fatalf("resumed session = %q, want %q", resumed.SessionPath, first.SessionPath) | ||
| 151 | } | 151 | } | ||
| 152 | - | if !resumed.Resuming || resumed.ResumePrompt != prompt { | 152 | + | resumePrompt, _ := resumed.ResumePromptInfo() |
| 153 | - | t.Fatalf("resumed lifecycle = resuming %t prompt %q, want true and %q", resumed.Resuming, resumed.ResumePrompt, prompt) | 153 | + | if !resumed.Resuming || resumePrompt != prompt { |
| 154 | + | t.Fatalf("resumed lifecycle = resuming %t prompt %q, want true and %q", resumed.Resuming, resumePrompt, prompt) | |||
| 154 | } | 155 | } | ||
| 155 | case <-time.After(time.Second): | 156 | case <-time.After(time.Second): | ||
| 156 | t.Fatal("resumed subagent did not start") | 157 | t.Fatal("resumed subagent did not start") | ||