feat: accept input while a session's foreground turn waits on background work#1668
feat: accept input while a session's foreground turn waits on background work#1668point-source wants to merge 78 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds fine-grained foreground activity tracking for background work, updates prompt gating and session event delivery, standardizes Monitor and ACP background payload handling, enriches backend session DTOs, and updates web state, icons, and end-to-end coverage. ChangesForeground activity signaling
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Agent
participant ACP
participant Orchestrator
participant EventBus
participant WebSocket
participant WebUI
Agent->>ACP: Emit background tool call or update
ACP->>Orchestrator: Provide normalized background payload
Orchestrator->>Orchestrator: Register background activity
Orchestrator->>EventBus: Publish foreground activity
EventBus->>WebSocket: Send session.activity_changed
WebSocket->>WebUI: Update session foreground_activity
WebUI->>WebUI: Show working status and accept input
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56ada34f00
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
| Filename | Overview |
|---|---|
| apps/backend/internal/orchestrator/turn_activity.go | New file implementing the per-session foreground activity tracker. Well-structured with careful locking; the claim/release/complete lifecycle is sound, though the stale-claim edge case (turn ends between claimForegroundTurn and sendPrompt) surfaces a confusing error to the user. |
| apps/backend/internal/orchestrator/task_operations.go | PromptTask refactored to support background-idle acceptance; foreground claim/release guard paths cover ensureSessionRunning and claimSessionRunningForPrompt failures, though the known leaked promptInFlight on PromptWithDispatchCallback failure is already tracked in a previous thread. |
| apps/backend/internal/agent/runtime/lifecycle/session.go | SendPrompt refactored into sendPrompt/preparePrompt/triggerPrompt/finishAcceptedPrompt; new promptMu serializes concurrent callers per-execution. No deadlock risk since the agent-event handler that sends to promptDoneCh does not acquire promptMu. dispatchedPromptPending correctly handles the dispatch-only completion race. |
| apps/backend/internal/orchestrator/executor/executor_interaction.go | PromptWithDispatchCallback added; passthrough path calls onDispatched on success; non-passthrough path type-asserts to promptAgentWithDispatchCallback, returning ErrPromptDispatchCallbackUnsupported for agents that don't implement it — propagates as a session error without special handling. |
| apps/backend/internal/orchestrator/event_handlers_streaming.go | Background task registration/completion wired into tool_call and tool_update handlers; child tool calls correctly excluded; terminal status check consistent with isTerminalToolStatus helper. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go | Bug fix: isBackgroundExecInput now recognizes both wait:false and run_in_background:true shapes; update path sets Background only ever to true. |
| apps/backend/internal/agentctl/types/streams/monitor_view.go | MonitorPayload attestation cleanly separates adapter-attested identity from agent-shaped output; IsActiveMonitor reads only the adapter-set field, preventing impersonation by non-Claude tools. |
| apps/backend/internal/task/service/task_activity.go | Task-level MOST-ACTIVE-WINS aggregate with dedup tracking; safe-fallback when session list fails to load; forgetTaskActivity called on task delete. |
| apps/backend/internal/backendapp/boot_state.go | Boot state enrichment stamps foreground_activity onto session and task DTOs for fresh page-loads; no-op when orchestrator is not wired. |
| apps/web/hooks/domains/session/use-session-state.ts | deriveSessionFlags takes the full session; isAgentBusy gates on foreground-generating only; isWorking stays true for all RUNNING states. |
| apps/web/lib/ws/handlers/agent-session.ts | applyForegroundActivity guards against missing session, non-RUNNING state, and task-ID mismatch; foreground_activity reset wired into buildSessionUpdate for every coarse transition. |
| apps/web/lib/ws/handlers/tasks.ts | mergeTaskUpdate preserves existing foregroundActivity when payload omits the field; correctly applies explicit null; hasPayloadField distinguishes absence from explicit null. |
| apps/web/lib/ui/state-icons.tsx | Tri-state icon system with shape+motion+hue separation satisfying the not-color-alone accessibility requirement. |
| apps/backend/pkg/api/v1/task.go | ForegroundActivity type and AggregateForegroundActivity MOST-ACTIVE-WINS reduction correctly short-circuits on generating with a sawBackground sentinel for background. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Op as Operator
participant BE as promptTask (Orchestrator)
participant TA as turnActivity (in-memory)
participant Ag as lifecycle.Manager
participant WS as WebSocket (client)
Note over Op,WS: Background-idle session (turn open, foreground yielded)
Ag->>TA: registerBackgroundTask(sessionID, toolCallID)
TA-->>WS: publishForegroundActivityChanged - background
WS-->>Op: "session.activity_changed foreground_activity=background"
Op->>BE: PromptTask
BE->>TA: "checkSessionPromptable - isForegroundTurnGenerating=false - pass"
BE->>TA: "claimForegroundTurn - promptInFlight=true yielded=false"
BE->>BE: ensureSessionRunning effectivePrompt
BE->>TA: markForegroundGenerating - publish generating
WS-->>Op: "session.activity_changed foreground_activity=generating"
BE->>Ag: PromptWithDispatchCallback(onDispatched)
Ag->>Ag: sendPrompt dispatch prompt to ACP
Ag->>BE: onDispatched() - completeForegroundClaim
Op-->>WS: prompt accepted
Note over Ag,TA: Background tool_call completes
Ag->>TA: completeBackgroundTask(sessionID, toolCallID)
TA-->>WS: publishForegroundActivityChanged if last task
Ag->>BE: waitForPromptDone - turn completes
BE->>TA: clearTurnActivity(sessionID)
BE-->>WS: "state_changed WAITING_FOR_INPUT foreground_activity=generating"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Op as Operator
participant BE as promptTask (Orchestrator)
participant TA as turnActivity (in-memory)
participant Ag as lifecycle.Manager
participant WS as WebSocket (client)
Note over Op,WS: Background-idle session (turn open, foreground yielded)
Ag->>TA: registerBackgroundTask(sessionID, toolCallID)
TA-->>WS: publishForegroundActivityChanged - background
WS-->>Op: "session.activity_changed foreground_activity=background"
Op->>BE: PromptTask
BE->>TA: "checkSessionPromptable - isForegroundTurnGenerating=false - pass"
BE->>TA: "claimForegroundTurn - promptInFlight=true yielded=false"
BE->>BE: ensureSessionRunning effectivePrompt
BE->>TA: markForegroundGenerating - publish generating
WS-->>Op: "session.activity_changed foreground_activity=generating"
BE->>Ag: PromptWithDispatchCallback(onDispatched)
Ag->>Ag: sendPrompt dispatch prompt to ACP
Ag->>BE: onDispatched() - completeForegroundClaim
Op-->>WS: prompt accepted
Note over Ag,TA: Background tool_call completes
Ag->>TA: completeBackgroundTask(sessionID, toolCallID)
TA-->>WS: publishForegroundActivityChanged if last task
Ag->>BE: waitForPromptDone - turn completes
BE->>TA: clearTurnActivity(sessionID)
BE-->>WS: "state_changed WAITING_FOR_INPUT foreground_activity=generating"
Reviews (12): Last reviewed commit: "Merge pull request #6 from point-source/..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/backend/internal/orchestrator/event_handlers_streaming.go (1)
393-483: 🎯 Functional Correctness | 🔵 TrivialBackground-tracking is unconditional in
handleToolCallEventbut gated behindmessageCreatorhere.
handleToolCallEvent's new registration (lines 259-268) intentionally sits outside themessageCreator != nilblock.trackBackgroundToolUpdatehere is called at line 442, nested inside both theif s.messageCreator == nil { return }guard (404-406) and the statusswitch. IfmessageCreatoris ever nil when tool-update events are processed, a background task registered byhandleToolCallEventcan never be completed via this path, parking the session in the "background" substate for the rest of the turn. Moving the call ahead of the guard mirrors the call-side symmetry and removes the dependency.♻️ Proposed fix to decouple background tracking from messageCreator
if s.shouldDropCompletedExecutionStreamEvent(payload) { return } + // Background-work bookkeeping must run even when messageCreator is unset, + // mirroring handleToolCallEvent's unconditional registration. + s.trackBackgroundToolUpdate(ctx, payload) + if s.messageCreator == nil { return }s.setSessionRunningForExecution(ctx, payload.TaskID, payload.SessionID, payload.ExecutionID) } - - // Background-work bookkeeping for the finer-grained busy signal. - s.trackBackgroundToolUpdate(ctx, payload) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/orchestrator/event_handlers_streaming.go` around lines 393 - 483, Move the trackBackgroundToolUpdate call in handleToolUpdateEvent before the messageCreator nil guard and outside the status/message-update handling, so background completion tracking always runs even when messageCreator is unavailable. Preserve the existing payload validation and avoid changing the tool message update behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/backend/internal/orchestrator/event_handlers_streaming.go`:
- Around line 393-483: Move the trackBackgroundToolUpdate call in
handleToolUpdateEvent before the messageCreator nil guard and outside the
status/message-update handling, so background completion tracking always runs
even when messageCreator is unavailable. Preserve the existing payload
validation and avoid changing the tool message update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3aaebd2-f212-4f29-9a7d-0eb2c6ce30c6
📒 Files selected for processing (42)
apps/backend/cmd/mock-agent/background_test.goapps/backend/cmd/mock-agent/handler.goapps/backend/cmd/mock-agent/main.goapps/backend/internal/agentctl/server/adapter/transport/acp/monitor.goapps/backend/internal/agentctl/server/adapter/transport/acp/normalize.goapps/backend/internal/agentctl/server/adapter/transport/acp/normalize_background_test.goapps/backend/internal/agentctl/types/streams/monitor_view.goapps/backend/internal/agentctl/types/streams/monitor_view_test.goapps/backend/internal/backendapp/boot_state.goapps/backend/internal/events/types.goapps/backend/internal/gateway/websocket/task_notifications.goapps/backend/internal/gateway/websocket/task_notifications_test.goapps/backend/internal/orchestrator/event_handlers_streaming.goapps/backend/internal/orchestrator/foreground_activity_signal_test.goapps/backend/internal/orchestrator/foreground_busy_signal_test.goapps/backend/internal/orchestrator/prompt_background_acceptance_test.goapps/backend/internal/orchestrator/service.goapps/backend/internal/orchestrator/task_operations.goapps/backend/internal/orchestrator/turn_activity.goapps/backend/internal/task/dto/dto.goapps/backend/internal/task/dto/foreground_activity_test.goapps/backend/internal/task/handlers/foreground_activity_test.goapps/backend/internal/task/handlers/task_handlers.goapps/backend/internal/task/handlers/task_http_handlers.goapps/backend/internal/task/handlers/task_ws_handlers.goapps/backend/pkg/api/v1/task.goapps/backend/pkg/websocket/actions.goapps/web/components/task/session-reopen-menu.tsxapps/web/components/task/sessions-dropdown.tsxapps/web/e2e/tests/chat/busy-signal.spec.tsapps/web/e2e/tests/chat/mobile-busy-signal.spec.tsapps/web/hooks/domains/session/use-session-state.test.tsapps/web/hooks/domains/session/use-session-state.tsapps/web/lib/state/slices/session/session-slice.upsert.test.tsapps/web/lib/types/backend.tsapps/web/lib/types/http.tsapps/web/lib/ui/state-icons.test.tsxapps/web/lib/ui/state-icons.tsxapps/web/lib/ws/handlers/agent-session.test.tsapps/web/lib/ws/handlers/agent-session.tsdocs/decisions/0035-fine-grained-foreground-idle-busy-signal.mddocs/decisions/INDEX.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 42 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Thanks for the thorough reviews. Addressed in Fixed
Acknowledged — follow-ups, not regressions
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge with suggestions. The core state machine ( |
|
Thanks for the contribution. The direction makes sense 💪, but I think the prompt-admission path needs a little more work before merging:
The current E2E test proves that the message appears and avoids the visible queue, but it would be useful to extend the mock so the second prompt produces a unique response. The test could then assert that the response arrives after the correct prompt, that each completion wakes the correct request, and that the user message and response belong to the same turn. |
Addresses the second round of automated review on kdlbs#1668. - PromptTask now atomically claims the foreground turn instead of acting on a bare read. checkSessionPromptable only reads the substate, and the window between that read and the point the turn is marked generating spans a session reload, ensureSessionRunning, and a possibly network-bound model switch — so two prompts landing in the background-idle window together (a double-send, two tabs) both passed the gate and both reached executor.Prompt, starting overlapping turns on one ACP session. Exactly one prompt now wins; the losers are rejected with ErrAgentPromptInProgress as before. A claim whose prompt then fails before reaching the agent is released, so a failed send can't leave the session advertising a foreground it doesn't have (cubic P1). - IsActiveMonitor gains a provenance check. A Generic payload's Output is otherwise the agent's own raw tool result, so the predicate now demands the full view the adapter writes, including the task_id only a real Monitor registration produces. Note GenericPayload.Name is NOT usable as the discriminator as suggested — it carries the ACP tool kind, which is "other" for Monitor, so asserting Name == "Monitor" would have disabled Monitor recognition outright (cubic P1). - The Monitor view map keys are exported from streams and consumed by acp/monitor.go, replacing the duplicated string literals on the producer side, and a contract test drives real monitorOutputWrapper output through IsActiveMonitor (incl. across the agentctl→orchestrator JSON boundary) so the two sides can't drift apart silently (cubic P2). - Document the deliberate hard-vs-soft ForegroundActivity dependency asymmetry between MessageHandlers (admission gate — must not silently degrade) and TaskHandlers (DTO enrichment — safe to omit), and drop a comment that restated its own state guard (github-actions). Tests: concurrent-PromptTask regression proving exactly one turn opens (red without the claim: all 8 prompts were accepted), claim/release unit coverage, and the producer→consumer Monitor contract tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/orchestrator/task_operations.go`:
- Around line 2343-2348: Update the early-return branch handling trySwitchModel
in the surrounding task operation to publish the foreground activity change when
the model switch succeeds, before returning. Preserve the existing
releaseForegroundClaimOnFailure behavior for errors and avoid publishing the
success update when trySwitchModel does not switch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c9571ba-464a-4b35-8400-0ff22334d384
📒 Files selected for processing (9)
apps/backend/internal/agentctl/server/adapter/transport/acp/monitor.goapps/backend/internal/agentctl/server/adapter/transport/acp/monitor_contract_test.goapps/backend/internal/agentctl/types/streams/monitor_view.goapps/backend/internal/agentctl/types/streams/monitor_view_test.goapps/backend/internal/orchestrator/foreground_claim_test.goapps/backend/internal/orchestrator/task_operations.goapps/backend/internal/orchestrator/turn_activity.goapps/backend/internal/task/handlers/message_handlers.godocs/decisions/0035-fine-grained-foreground-idle-busy-signal.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/backend/internal/agentctl/types/streams/monitor_view.go
- apps/backend/internal/task/handlers/message_handlers.go
- apps/backend/internal/agentctl/types/streams/monitor_view_test.go
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions — both are narrow, won't block the feature. The release-claim publish gap is the more impactful of the two (UX regression in a rare error path); the constant-usage gap is a test hygiene fix. Neither affects the correctness of the happy path. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/backend/internal/agent/runtime/lifecycle/session_test.go (1)
894-915: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister test resources with
t.Cleanup.Replace the new
defer mock.Close()anddefer client.Close()calls with cleanup callbacks registered immediately after creation.As per coding guidelines, lifecycle tests must register
t.Cleanupimmediately after resource creation.Also applies to: 968-973, 1065-1087
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/agent/runtime/lifecycle/session_test.go` around lines 894 - 915, Replace the deferred mock and client shutdowns with t.Cleanup callbacks registered immediately after each resource is created. Apply this consistently in the shown setup and the corresponding resource-creation blocks around the additional locations, preserving the existing Close behavior and cleanup order.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/agent/runtime/lifecycle/session.go`:
- Around line 713-721: Update the reconnect retry handling in the session
lifecycle method around retryPromptAfterReconnect so that a failed retry
propagates its retryErr into the subsequent error-mapping flow instead of
leaving the original err unchanged. Preserve the existing success return and
warning log, while ensuring retry-side ErrCancelEscalated errors are mapped
correctly for callers.
In `@apps/backend/internal/orchestrator/executor/executor_interaction.go`:
- Around line 188-196: Update the dispatch path around
promptAgentWithDispatchCallback so dispatchOnly requests require an agentManager
implementing PromptAgentWithDispatchCallback; when that capability is absent,
fail explicitly instead of calling PromptAgent and invoking onDispatched after
completion. Preserve the existing callback-capable flow and only use the
PromptAgent fallback where the dispatch-callback contract is not required.
---
Nitpick comments:
In `@apps/backend/internal/agent/runtime/lifecycle/session_test.go`:
- Around line 894-915: Replace the deferred mock and client shutdowns with
t.Cleanup callbacks registered immediately after each resource is created. Apply
this consistently in the shown setup and the corresponding resource-creation
blocks around the additional locations, preserving the existing Close behavior
and cleanup order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a68e0b0-ee9e-4f76-b7ee-b4d3b39a03c0
📒 Files selected for processing (13)
apps/backend/internal/agent/runtime/lifecycle/manager_interaction.goapps/backend/internal/agent/runtime/lifecycle/session.goapps/backend/internal/agent/runtime/lifecycle/session_test.goapps/backend/internal/agent/runtime/lifecycle/types.goapps/backend/internal/backendapp/adapters.goapps/backend/internal/orchestrator/event_handlers_streaming.goapps/backend/internal/orchestrator/executor/executor_interaction.goapps/backend/internal/orchestrator/foreground_activity_signal_test.goapps/backend/internal/orchestrator/foreground_busy_signal_test.goapps/backend/internal/orchestrator/foreground_claim_test.goapps/backend/internal/orchestrator/task_operations.goapps/backend/internal/orchestrator/turn_activity.godocs/decisions/0035-fine-grained-foreground-idle-busy-signal.md
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/decisions/0035-fine-grained-foreground-idle-busy-signal.md
- apps/backend/internal/orchestrator/foreground_claim_test.go
- apps/backend/internal/orchestrator/turn_activity.go
- apps/backend/internal/orchestrator/event_handlers_streaming.go
- apps/backend/internal/orchestrator/task_operations.go
- apps/backend/internal/orchestrator/foreground_busy_signal_test.go
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Addresses the second round of automated review on kdlbs#1668. - PromptTask now atomically claims the foreground turn instead of acting on a bare read. checkSessionPromptable only reads the substate, and the window between that read and the point the turn is marked generating spans a session reload, ensureSessionRunning, and a possibly network-bound model switch — so two prompts landing in the background-idle window together (a double-send, two tabs) both passed the gate and both reached executor.Prompt, starting overlapping turns on one ACP session. Exactly one prompt now wins; the losers are rejected with ErrAgentPromptInProgress as before. A claim whose prompt then fails before reaching the agent is released, so a failed send can't leave the session advertising a foreground it doesn't have (cubic P1). - IsActiveMonitor gains a provenance check. A Generic payload's Output is otherwise the agent's own raw tool result, so the predicate now demands the full view the adapter writes, including the task_id only a real Monitor registration produces. Note GenericPayload.Name is NOT usable as the discriminator as suggested — it carries the ACP tool kind, which is "other" for Monitor, so asserting Name == "Monitor" would have disabled Monitor recognition outright (cubic P1). - The Monitor view map keys are exported from streams and consumed by acp/monitor.go, replacing the duplicated string literals on the producer side, and a contract test drives real monitorOutputWrapper output through IsActiveMonitor (incl. across the agentctl→orchestrator JSON boundary) so the two sides can't drift apart silently (cubic P2). - Document the deliberate hard-vs-soft ForegroundActivity dependency asymmetry between MessageHandlers (admission gate — must not silently degrade) and TaskHandlers (DTO enrichment — safe to omit), and drop a comment that restated its own state guard (github-actions). Tests: concurrent-PromptTask regression proving exactly one turn opens (red without the claim: all 8 prompts were accepted), claim/release unit coverage, and the producer→consumer Monitor contract tests.
1a923b8 to
3869db9
Compare
beb3443 to
c935b1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 170 out of 170 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
apps/web/lib/types/backend.ts:96
- There’s a stray semicolon in the comment (
//;) which reads like a typo and can confuse readers. Removing it keeps the doc comment clean.
| ActionSessionStateChanged = "session.state_changed" | ||
| ActionSessionActivityChanged = "session.activity_changed" | ||
| ActionSessionWaitingForInput = "session.waiting_for_input" | ||
| ActionSessionAgentctlStarting = "session.agentctl_starting" |
| // ForegroundActivity is the fine-grained busy substate of a RUNNING session | ||
| // (ADR-0049). It distinguishes a foreground turn that is | ||
| // actively generating from one that is idle, held open only by spawned | ||
| // background work (a subagent task, a run-in-background shell, an active | ||
| // Monitor). It is only meaningful while the session state is RUNNING; for every | ||
| // other state the coarse state already tells the whole story. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 183 out of 183 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
apps/backend/pkg/api/v1/task.go:51
- The doc comment says ForegroundActivity is only meaningful while session state is RUNNING, but this PR explicitly carries "background" beyond the foreground turn (e.g. WAITING_FOR_INPUT) and both backend/frontend code rely on that. The comment is misleading for maintainers reading the API types.
apps/web/lib/types/backend.ts:95 - There’s a stray semicolon in the comment (
//; absent/null...), which reads like a typo in the generated backend payload types.
apps/web/components/task/chat/messages/agent-status.tsx:45 - resolveAgentStatusConfig only shows the "Background work is running" label for WAITING_FOR_INPUT+isWorking. But this PR also makes RUNNING+foreground_activity="background" directly promptable, and in that state the UI still resolves to the coarse RUNNING label ("Agent is running"), which contradicts the intended tri-state indicator described in the PR (generating vs background vs done).
|
I found two cases that should be fixed:
Ideally, carry a stable workload ID through the adapter: AgentEvent{
Type: streams.EventTypeBackgroundComplete,
ToolCallID: taskID,
}If the provider cannot expose that identity, the accounting contract needs another deduplication/correlation strategy rather than leaving all registrations live.
if eventType == events.TaskDeleted {
queue.deleted = true
queue.pending = nil
}Please add deterministic regression tests for both sequences. Two smaller follow-ups: the chat footer still says “Agent is running” for |
What & why
A session whose durable state reads
RUNNINGused to reject every new operator message as "agent is already running" — even when the foreground turn was actually idle, held open only by spawned background work (a subagentTask, arun_in_backgroundshell, or an active Monitor watch). For long background jobs this locked the operator out of the conversation for the entire duration.This PR narrows the busy signal to the foreground turn: when the foreground agent has yielded to registered background work, the session accepts "are you still working?" instead of dropping it. The change is conservative and capability-gated — any agent whose in-flight frames aren't recognized as background work (Codex, OpenCode, and anything else) keeps today's exact reject-while-
RUNNINGbehavior.Explicitly out of scope: mid-turn steering while the model is actively generating (a separate future concern — needs ACP concurrent-prompt support and per-agent capability gating). This PR corrects only whether input is accepted, not delivery into a live generating turn.
Full rationale, alternatives, and consequences are recorded in ADR-0035.
Behavior delivered
checkSessionPromptableto the foreground turn via an in-memory per-session activity tracker; default stays busy.run_in_background:trueshell shape (a genuine bug fix regardless of the feature).RUNNING; a tri-state status icon distinguishes generating / working-in-background / done (the established running indicator is unchanged — this only adds a background indicator); and the substate is delivered live over asession.activity_changedWS event and surfaced on the boot payload / REST / WS session DTOs (RUNNING-only, not persisted) so a fresh page-load or second tab is immediately correct, with a backend restart resetting to the safe "generating" default.Validated against upstream idle-turn completion
A falsifiable acceptance test drives a chatty Monitor (repeated event bursts that keep the idle-completion debounce alive) and asserts a prompt sent during a burst window is accepted and forwarded — proving the gate is still needed. A non-Claude regression test asserts Codex/OpenCode-shaped frames leave the gate unchanged.
End-to-end verification for the reviewer
/background 20s(spawns a subagent and holds the turn open with the foreground idle).RUNNINGwith a working-in-background status (spinner, not the done check). Send "are you still working?" — it is accepted (composer shows send/accept, not "Queue more instructions…") and forwarded, not rejected.WAITING_FOR_INPUT(done check).CI: backend
go test -race ./..., changed-filegolangci-lint, web typecheck/lint/vitest, plus desktop + mobile Playwright specs (including a reload assertion).Note
This is the upstream-targeted version of the change (the fork carries the same code under its own governance docs). It deliberately documents the decision as an ADR rather than the fork's SPEC/REQUIREMENTS files.
Preview Environment
8310c18