Repository navigation
fix: preserve ACP stream disconnect recovery - #319
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughThe change adds portable process exit diagnostics, enforces terminal events for prompt streams, propagates fatal failures through session lifecycle handling, preserves partial events, maps native tool failures, and records process metadata in version 2 crash bundles. ChangesSession failure handling
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PromptStream
participant SessionManager
participant ACPProcess
participant CrashBundle
Client->>PromptStream: submit session prompt
PromptStream->>SessionManager: forward prompt events
ACPProcess-->>SessionManager: emit partial events and failure
SessionManager->>ACPProcess: wait for process exit
ACPProcess-->>SessionManager: return exit status
SessionManager->>CrashBundle: persist failure diagnostics
SessionManager-->>Client: return partial output and failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
| Filename | Overview |
|---|---|
| internal/session/manager_prompt_failure_barrier.go | Adds the process-evidence barrier and coordinates fatal prompt failures with normal session-stop finalization. |
| internal/session/manager_prompt_runtime_loop.go | Routes fatal stream outcomes through the new cleanup and finalization path without synthesizing completion. |
| internal/subprocess/exit_status.go | Introduces synchronized subprocess exit-status capture and filters signal sentinel values from numeric exit-code output. |
| internal/session/crash_bundle.go | Hard-cuts crash evidence to schema v2 and records available exit code and signal facts. |
| internal/api/core/prompt_stream_delivery.go | Emits a terminal transport failure for incomplete upstream streams and stops after semantic terminal events. |
| internal/cli/client_session_prompt.go | Preserves streamed frames while rejecting terminal errors and EOF without an explicit completion marker. |
| internal/daemon/native_session_prompt_stream.go | Classifies drained native-tool prompt streams so terminal runtime failures cannot become successful tool results. |
| internal/sandbox/daytona/launcher.go | Exposes numeric remote-process exit status through the ACP process boundary. |
Sequence Diagram
sequenceDiagram
participant User
participant Consumer as CLI / Native Tool / HTTP
participant Manager as Session Manager
participant ACP as ACP Subprocess
participant Store as Transcript / Crash Bundle
User->>Consumer: Send prompt
Consumer->>Manager: Start prompt stream
Manager->>ACP: Submit prompt
ACP-->>Manager: Partial response chunks
Manager->>Store: Persist delivered chunks
Manager-->>Consumer: Stream partial frames
ACP--xManager: Disconnect / exit
Manager->>ACP: Wait for bounded natural exit
Manager->>Store: Record process exit diagnostics
Manager-->>Consumer: Emit explicit terminal error
Consumer-->>User: Return failure, preserving prior frames
User->>Consumer: Send new explicit prompt
Consumer->>Manager: Resume same session
Manager->>ACP: Start replacement process
ACP-->>Manager: Complete response
Manager-->>Consumer: Finish + [DONE]
Reviews (2): Last reviewed commit: "fix: harden ACP stream failure finalizat..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/session/manager_prompt_contract_test.go`:
- Around line 375-405: Wrap the changed scenario in
TestPromptFatalProcessFailureStopsSessionAndAllowsFreshResumeFallback with
t.Run("Should stop after a fatal process failure and recover with an explicit
prompt", ...) and call t.Parallel() at the start of that subtest. Include all
related setup, hooks, assertions, and cleanup for the scenario within the
subtest, including the additional ranges noted in the review.
In `@internal/subprocess/exit_status_unix.go`:
- Around line 1-15: Restrict the build constraint for processStateSignal to
supported Unix targets rather than using only !windows, so Plan 9 does not
compile this syscall.WaitStatus implementation. Update the build tag on
exit_status_unix.go while preserving the existing processStateSignal behavior
for Unix platforms.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad682d63-36d8-4635-95d5-a1e1a572a0fc
⛔ Files ignored due to path filters (8)
docs/qa/charters/CH-acp-stream-disconnect-recovery.mdis excluded by!**/*.mddocs/qa/journeys/J-15-operate-session-via-cli-api.mdis excluded by!**/*.mddocs/qa/reports/2026-08-05-acp-stream-disconnect.mdis excluded by!**/*.mddocs/qa/scenarios/RT-acp-stream-disconnect-recovery.mdis excluded by!**/*.mdinternal/testutil/acpmock/testdata/driver_fault_fixture.jsonis excluded by!**/*.jsonpackages/site/content/docs/guides/debug-a-failed-session.mdxis excluded by!**/*.mdxpackages/site/content/docs/sessions/lifecycle.mdxis excluded by!**/*.mdxskills/compozy/references/runtime-operations.mdis excluded by!**/*.md
📒 Files selected for processing (19)
internal/acp/agent_process.gointernal/acp/launcher.gointernal/cli/client_session_prompt.gointernal/cli/client_test.gointernal/cli/session_test.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/native_session_mutation.gointernal/daemon/native_session_prompt_stream.gointernal/daemon/native_tools_test.gointernal/session/crash_bundle.gointernal/session/interfaces.gointernal/session/manager_prompt_contract_test.gointernal/session/manager_prompt_runtime_loop.gointernal/subprocess/exit_status.gointernal/subprocess/exit_status_unix.gointernal/subprocess/exit_status_windows.gointernal/subprocess/process.gointernal/subprocess/process_lifecycle.gointernal/subprocess/process_test.go
pedronauck
force-pushed
the
codex/issue-315-acp-stream-disconnect
branch
from
August 6, 2026 00:59
4bc0329 to
911f158
Compare
Member
Author
|
Publication update: pushed remediation commit |
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/session/manager_prompt_runtime_loop.go (1)
302-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the returned flag self-describing.
prepareFatalPromptFailureEventreturns a barrier outcome, and this code uses it directly asattachDiagnostics. The two meanings are not the same. When the barrier fails, this code also skipsattachPromptFailureDiagnostics, which is the case where crash context is most useful. If that suppression is intentional, name the value for the decision it drives and add a short comment.♻️ Suggested clarification
- attachDiagnostics := true - if isFatalPromptFailureEvent(normalized) { - attachDiagnostics = m.prepareFatalPromptFailureEvent(ctx, session, normalized, fatal) - } + // Diagnostics require an observed process exit; the barrier guarantees it. + attachDiagnostics := true + if isFatalPromptFailureEvent(normalized) { + barrierOwned := m.prepareFatalPromptFailureEvent(ctx, session, normalized, fatal) + attachDiagnostics = barrierOwned + }🤖 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 `@internal/session/manager_prompt_runtime_loop.go` around lines 302 - 308, Update the fatal-event handling around prepareFatalPromptFailureEvent so its barrier outcome is stored in a self-describing variable that reflects the decision it controls, not attachDiagnostics. Add a brief comment documenting the intentional behavior when the barrier fails, and use that decision variable to govern attachPromptFailureDiagnostics while preserving the existing logic.internal/session/manager_synthetic_prompt_test.go (1)
166-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the released-prompt hook into a shared helper.
The same four-line pattern now appears in four hooks: buffer one event, wait for a release channel, emit
EventTypeDone, close. Add a helper next tocompletedSyntheticPromptEventsto keep the fixtures consistent.♻️ Suggested helper
func releasedPromptEvents(release <-chan struct{}, turnID string) <-chan acp.AgentEvent { events := make(chan acp.AgentEvent, 1) go func() { <-release events <- acp.AgentEvent{Type: acp.EventTypeDone, TurnID: turnID} close(events) }() return events }Also applies to: 272-280, 367-375, 457-465
🤖 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 `@internal/session/manager_synthetic_prompt_test.go` around lines 166 - 174, Add a shared releasedPromptEvents helper next to completedSyntheticPromptEvents that creates the buffered event channel, waits on the provided release channel, emits the completed event, and closes the channel. Replace the duplicated goroutine patterns in all four hooks, including the locations around the existing releaseFirstPrompt usages, with calls to this helper while preserving each hook’s release channel and request TurnID.internal/session/manager_prompt_failure_barrier.go (3)
85-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not return
(error, error).
stopProcessForFatalPromptFailurereturns twoerrorvalues. The first is a payload passed tofatal.ownFinalization, and the second is a control-flow error. Callers cannot tell them apart from the signature, and this shape breaks the Go convention that a trailingerroris the only error. Use named results or return the wait error in a small struct.♻️ Suggested signature
func (m *Manager) stopProcessForFatalPromptFailure( ctx context.Context, session *Session, proc *AgentProcess, failure *store.SessionFailure, summary string, -) (error, error) { +) (processWaitErr error, err error) {🤖 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 `@internal/session/manager_prompt_failure_barrier.go` around lines 85 - 113, Change stopProcessForFatalPromptFailure so it no longer returns two error values; introduce a named result type or small struct that clearly separates the fatal-finalization payload from the control-flow error, and update all callers including fatal.ownFinalization to use the new result while preserving both existing error semantics.
115-133: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard
fatal.failurebefore the log dereference.Line 129 reads
fatal.failure.Kind. The function only checksfatal.finalizationOwned. TodayfinishPromptPumpreaches this branch only whenfatal.failureis non-nil, so no panic occurs. That invariant lives in another file. Add a local guard so a future caller cannot introduce a nil dereference.🛡️ Suggested guard
- if m == nil || session == nil || fatal == nil || !fatal.finalizationOwned { + if m == nil || session == nil || fatal == nil || fatal.failure == nil || !fatal.finalizationOwned { return }🤖 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 `@internal/session/manager_prompt_failure_barrier.go` around lines 115 - 133, In finalizeSessionAfterFatalPromptFailure, guard fatal.failure before dereferencing its Kind in the warning log. Preserve finalization behavior, and use a nil-safe failure kind value or conditional logging so callers with a nil failure cannot panic.
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the finalization handle is discarded.
claimFinalizationreturns(bool, *sessionFinalization), not an error. Since this barrier only checks ownership and later callsfinishFinalization(session.ID, err)regardless, add a short comment explaining thatfinalizationis intentionally ignored in this path.🤖 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 `@internal/session/manager_prompt_failure_barrier.go` around lines 56 - 61, In the finalization-ownership branch around claimFinalization, add a short comment documenting that the returned finalization handle is intentionally discarded because this barrier only checks ownership and finishFinalization(session.ID, err) is called later regardless. Preserve the existing ownership check and fatal failure flow.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 `@internal/api/core/prompt_stream_test.go`:
- Around line 45-52: Extend the test around the stream collected from
writer.String() to assert that the previously emitted “partial response” remains
present before the terminal error event, and verify that the terminal sentinel
“data: [DONE]” is emitted exactly once and after the error. Keep the existing
incomplete-stream failure assertions and use the test’s existing stream/event
symbols.
In `@internal/session/interfaces.go`:
- Around line 250-257: The process-exit error assigned to event.Error in
promptProcessFailureEvent must be bounded and normalized rather than using the
raw multi-line waitErr.Error() text. Reuse the existing error-summary
normalization or truncation helper before assigning event.Error, while
preserving the current normalized handling of event.Failure.Summary.
In `@internal/session/manager_prompt_process_exit.go`:
- Around line 67-75: Update the exit-status detail construction in the
process-exit summary to use status.ExitCodeValue() instead of the raw
status.ExitCode, while preserving the existing signal-detail override and
summary formatting. Keep the change scoped to the status handling in the
surrounding process-exit logic.
---
Nitpick comments:
In `@internal/session/manager_prompt_failure_barrier.go`:
- Around line 85-113: Change stopProcessForFatalPromptFailure so it no longer
returns two error values; introduce a named result type or small struct that
clearly separates the fatal-finalization payload from the control-flow error,
and update all callers including fatal.ownFinalization to use the new result
while preserving both existing error semantics.
- Around line 115-133: In finalizeSessionAfterFatalPromptFailure, guard
fatal.failure before dereferencing its Kind in the warning log. Preserve
finalization behavior, and use a nil-safe failure kind value or conditional
logging so callers with a nil failure cannot panic.
- Around line 56-61: In the finalization-ownership branch around
claimFinalization, add a short comment documenting that the returned
finalization handle is intentionally discarded because this barrier only checks
ownership and finishFinalization(session.ID, err) is called later regardless.
Preserve the existing ownership check and fatal failure flow.
In `@internal/session/manager_prompt_runtime_loop.go`:
- Around line 302-308: Update the fatal-event handling around
prepareFatalPromptFailureEvent so its barrier outcome is stored in a
self-describing variable that reflects the decision it controls, not
attachDiagnostics. Add a brief comment documenting the intentional behavior when
the barrier fails, and use that decision variable to govern
attachPromptFailureDiagnostics while preserving the existing logic.
In `@internal/session/manager_synthetic_prompt_test.go`:
- Around line 166-174: Add a shared releasedPromptEvents helper next to
completedSyntheticPromptEvents that creates the buffered event channel, waits on
the provided release channel, emits the completed event, and closes the channel.
Replace the duplicated goroutine patterns in all four hooks, including the
locations around the existing releaseFirstPrompt usages, with calls to this
helper while preserving each hook’s release channel and request TurnID.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b296898f-78d2-4e42-88f3-5e54ceed8253
⛔ Files ignored due to path filters (9)
docs/qa/charters/CH-acp-stream-disconnect-recovery.mdis excluded by!**/*.mddocs/qa/journeys/J-15-operate-session-via-cli-api.mdis excluded by!**/*.mddocs/qa/reports/2026-08-05-acp-stream-disconnect.mdis excluded by!**/*.mddocs/qa/reports/2026-08-05-pr319-acp-stream-remediation.mdis excluded by!**/*.mddocs/qa/scenarios/RT-acp-stream-disconnect-recovery.mdis excluded by!**/*.mdinternal/testutil/acpmock/testdata/driver_fault_fixture.jsonis excluded by!**/*.jsonpackages/site/content/docs/guides/debug-a-failed-session.mdxis excluded by!**/*.mdxpackages/site/content/docs/sessions/lifecycle.mdxis excluded by!**/*.mdxskills/compozy/references/runtime-operations.mdis excluded by!**/*.md
📒 Files selected for processing (39)
internal/acp/agent_process.gointernal/acp/launcher.gointernal/acp/prompt_stream_terminal.gointernal/api/core/prompt_stream_delivery.gointernal/api/core/prompt_stream_test.gointernal/cli/client_session_prompt.gointernal/cli/client_test.gointernal/cli/session_test.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/native_session_mutation.gointernal/daemon/native_session_prompt_stream.gointernal/daemon/native_tools_test.gointernal/sandbox/daytona/launcher.gointernal/sandbox/daytona/provider_test.gointernal/session/crash_bundle.gointernal/session/crash_bundle_test.gointernal/session/interfaces.gointernal/session/manager_hooks_lifecycle_test.gointernal/session/manager_lifecycle.gointernal/session/manager_managed_input_submit.gointernal/session/manager_prompt.gointernal/session/manager_prompt_batch.gointernal/session/manager_prompt_contract_test.gointernal/session/manager_prompt_failure_barrier.gointernal/session/manager_prompt_process_exit.gointernal/session/manager_prompt_pump.gointernal/session/manager_prompt_runtime_loop.gointernal/session/manager_prompt_submit.gointernal/session/manager_synthetic_prompt_test.gointernal/session/session.gointernal/session/session_prompt_state.gointernal/session/stop_reason.gointernal/subprocess/exit_status.gointernal/subprocess/exit_status_other.gointernal/subprocess/exit_status_unix.gointernal/subprocess/exit_status_windows.gointernal/subprocess/process.gointernal/subprocess/process_lifecycle.gointernal/subprocess/process_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/acp/agent_process.go
- internal/subprocess/exit_status_windows.go
- internal/subprocess/exit_status_unix.go
- internal/acp/launcher.go
- internal/subprocess/process_lifecycle.go
- internal/daemon/native_session_mutation.go
- internal/daemon/native_tools_test.go
- internal/cli/session_test.go
- internal/cli/client_test.go
- internal/cli/client_session_prompt.go
- internal/subprocess/process.go
- internal/daemon/daemon_acpmock_faults_integration_test.go
Comment on lines
+45
to
+52
| stream := writer.String() | ||
| if !strings.Contains(stream, `"type":"error"`) || | ||
| !strings.Contains(stream, "ended before a terminal event") { | ||
| t.Fatalf("stream = %q, want explicit incomplete-stream failure", stream) | ||
| } | ||
| if !strings.Contains(stream, "data: [DONE]") { | ||
| t.Fatalf("stream = %q, want a terminal sentinel after the failure", stream) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify partial-output retention and terminal order.
This test sends "partial response" before the channel closes. It does not assert that the response retains that chunk before the terminal error. It also does not assert that delivery emits one terminal sentinel.
Proposed test assertions
stream := writer.String()
if !strings.Contains(stream, `"type":"error"`) ||
!strings.Contains(stream, "ended before a terminal event") {
t.Fatalf("stream = %q, want explicit incomplete-stream failure", stream)
}
- if !strings.Contains(stream, "data: [DONE]") {
- t.Fatalf("stream = %q, want a terminal sentinel after the failure", stream)
+ partialAt := strings.Index(stream, "partial response")
+ errorAt := strings.Index(stream, `"type":"error"`)
+ if partialAt < 0 || partialAt > errorAt {
+ t.Fatalf("stream = %q, want retained partial output before terminal failure", stream)
+ }
+ if got := strings.Count(stream, "data: [DONE]"); got != 1 {
+ t.Fatalf("terminal sentinel count = %d, want 1; stream = %q", got, stream)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| stream := writer.String() | |
| if !strings.Contains(stream, `"type":"error"`) || | |
| !strings.Contains(stream, "ended before a terminal event") { | |
| t.Fatalf("stream = %q, want explicit incomplete-stream failure", stream) | |
| } | |
| if !strings.Contains(stream, "data: [DONE]") { | |
| t.Fatalf("stream = %q, want a terminal sentinel after the failure", stream) | |
| } | |
| stream := writer.String() | |
| if !strings.Contains(stream, `"type":"error"`) || | |
| !strings.Contains(stream, "ended before a terminal event") { | |
| t.Fatalf("stream = %q, want explicit incomplete-stream failure", stream) | |
| } | |
| partialAt := strings.Index(stream, "partial response") | |
| errorAt := strings.Index(stream, `"type":"error"`) | |
| if partialAt < 0 || partialAt > errorAt { | |
| t.Fatalf("stream = %q, want retained partial output before terminal failure", stream) | |
| } | |
| if got := strings.Count(stream, "data: [DONE]"); got != 1 { | |
| t.Fatalf("terminal sentinel count = %d, want 1; stream = %q", got) | |
| } |
🤖 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 `@internal/api/core/prompt_stream_test.go` around lines 45 - 52, Extend the
test around the stream collected from writer.String() to assert that the
previously emitted “partial response” remains present before the terminal error
event, and verify that the terminal sentinel “data: [DONE]” is emitted exactly
once and after the error. Keep the existing incomplete-stream failure assertions
and use the test’s existing stream/event symbols.
Comment on lines
250
to
257
| func (p *AgentProcess) Wait() error { | ||
| <-p.Done() | ||
| waitErr := p.waitFn() | ||
| p.waitOverrideMu.RLock() | ||
| override := p.waitErrOverride | ||
| p.waitOverrideMu.RUnlock() | ||
| if override != nil { | ||
| return override | ||
| } | ||
| return p.waitFn() | ||
| return errors.Join(waitErr, override) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Trace how wait errors become failure summaries and whether multi-line text is normalized.
set -euo pipefail
rg -n -C5 'waitErrOverride|firstTrimmedNonEmpty|func failureSummary' internal/session internal/acpRepository: compozy/compozy
Length of output: 23267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- internal/session/manager_prompt_process_exit.go ---\n'
sed -n '1,120p' internal/session/manager_prompt_process_exit.go | cat -n
printf '\n--- internal/acp/failure.go relevant sections ---\n'
sed -n '1,120p' internal/acp/failure.go | cat -n
sed -n '170,245p' internal/acp/failure.go | cat -n
printf '\n--- internal/session/diagnostics references ---\n'
fd -a 'diagnostics' internal | sed 's#^\./##'
rg -n 'func RedactAndBound|RedactAndBound|maxFailureSummaryBytes|maxSessionFailureSummaryBytes' internalRepository: compozy/compozy
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- internal/session/manager_prompt_process_exit.go ---'
sed -n '1,120p' internal/session/manager_prompt_process_exit.go | cat -n
printf '%s\n' ''
printf '%s\n' '--- internal/acp/failure.go relevant sections ---'
sed -n '70,110p' internal/acp/failure.go | cat -n
sed -n '204,233p' internal/acp/failure.go | cat -n
printf '%s\n' ''
printf '%s\n' '--- diagnostics/bounds references ---'
rg -n 'func RedactAndBound|RedactAndBound|maxFailureSummaryBytes|maxSessionFailureSummaryBytes' internalRepository: compozy/compozy
Length of output: 23494
Bound the process-exit summary before assigning it to event.Error.
promptProcessFailureEvent can leave waitErr.Error() as event.Error unbounded and multi-line. event.Error is written to the session event/crash bundle, while event.Failure.Summary already receives truncation through normalization, so use a bounded summary before assignment or otherwise normalize the multi-line joined text.
🤖 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 `@internal/session/interfaces.go` around lines 250 - 257, The process-exit
error assigned to event.Error in promptProcessFailureEvent must be bounded and
normalized rather than using the raw multi-line waitErr.Error() text. Reuse the
existing error-summary normalization or truncation helper before assigning
event.Error, while preserving the current normalized handling of
event.Failure.Summary.
Comment on lines
+67
to
+75
| if proc != nil { | ||
| if status, ok := proc.ExitStatus(); ok { | ||
| detail := fmt.Sprintf("exit code %d", status.ExitCode) | ||
| if status.Signal != "" { | ||
| detail = "signal " + status.Signal | ||
| } | ||
| summary = fmt.Sprintf("%s (%s)", summary, detail) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use ExitCodeValue() for the summary detail.
status.ExitCode is used raw. On Unix a signal termination reports -1, and the fallback processStateSignal returns an empty signal on platforms outside darwin, linux, and windows. In that combination the summary reads "exit code -1", which is not an exit code. writeCrashBundleAtPath in internal/session/crash_bundle.go (Lines 161-166) already suppresses the sentinel through ExitCodeValue(), so the event summary and the crash bundle disagree for the same failure.
🐛 Proposed fix for the exit-code detail
if proc != nil {
if status, ok := proc.ExitStatus(); ok {
- detail := fmt.Sprintf("exit code %d", status.ExitCode)
- if status.Signal != "" {
- detail = "signal " + status.Signal
- }
- summary = fmt.Sprintf("%s (%s)", summary, detail)
+ detail := ""
+ switch {
+ case status.Signal != "":
+ detail = "signal " + status.Signal
+ default:
+ if code, available := status.ExitCodeValue(); available {
+ detail = fmt.Sprintf("exit code %d", code)
+ }
+ }
+ if detail != "" {
+ summary = fmt.Sprintf("%s (%s)", summary, detail)
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if proc != nil { | |
| if status, ok := proc.ExitStatus(); ok { | |
| detail := fmt.Sprintf("exit code %d", status.ExitCode) | |
| if status.Signal != "" { | |
| detail = "signal " + status.Signal | |
| } | |
| summary = fmt.Sprintf("%s (%s)", summary, detail) | |
| } | |
| } | |
| if proc != nil { | |
| if status, ok := proc.ExitStatus(); ok { | |
| detail := "" | |
| switch { | |
| case status.Signal != "": | |
| detail = "signal " + status.Signal | |
| default: | |
| if code, available := status.ExitCodeValue(); available { | |
| detail = fmt.Sprintf("exit code %d", code) | |
| } | |
| } | |
| if detail != "" { | |
| summary = fmt.Sprintf("%s (%s)", summary, detail) | |
| } | |
| } | |
| } |
🤖 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 `@internal/session/manager_prompt_process_exit.go` around lines 67 - 75, Update
the exit-status detail construction in the process-exit summary to use
status.ExitCodeValue() instead of the raw status.ExitCode, while preserving the
existing signal-detail override and summary formatting. Keep the change scoped
to the status handling in the surrounding process-exit logic.
This was referenced Aug 5, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 13, 2026
Merged
Merged
Merged
This was referenced Aug 14, 2026
Merged
Merged
Merged
This was referenced Aug 18, 2026
Merged
Merged
Merged
Merged
This was referenced Aug 27, 2026
Merged
Merged
This was referenced Sep 7, 2026
Merged
Merged
Merged
This was referenced Sep 15, 2026
Merged
Merged
This was referenced Sep 27, 2026
Merged
Merged
This was referenced Oct 6, 2026
Merged
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context and reproduction
A Codex ACP subprocess could disconnect while Compozy was still streaming the final response. The session manager persisted the chunks it had already received, but its fatal-path cleanup raced the subprocess' natural exit. That race replaced the real process result with a generic wait override, discarded useful exit diagnostics, and left stream consumers unable to distinguish a complete stream from a disconnected or crashed process.
The visible symptoms were:
compozy__session_prompttool could drain a terminal runtime error and still return success;Proven root cause
There were four connected causes:
manager_prompt_runtime_loop.goinstalled a wait override and immediately stopped the ACP process, racing the operating-system wait path.AgentProcess.Waitwrapper returned the override without waiting for the underlying process, so it lost the subprocess exit result and diagnostics.The regression fixture reproduces a subprocess that emits partial output and then exits with code 23. The integration test proves the previous ambiguity across HTTP/SSE, persisted transcript, CLI JSONL, crash evidence, native tool invocation, and same-session recovery.
Solution design
This change uses a fail-closed stream contract:
done,finish,prompt_result, or[DONE]);process_exitremain distinct failure outcomes;cmd.Waitand propagated into crash evidence;Changes by surface
Runtime and process lifecycle
compozy.session_crash_bundle.v2with structuredexit_codeandsignal.HTTP/SSE and CLI
UDS/native tools
compozy__session_promptnow classifies terminal session errors instead of treating a drained stream as success.process_exitbecomestool_backend_failedwithbackend_dead, while partial events remain available in the persisted session transcript.Documentation and operator workflow
Invariants and canonical tests
TestDaemonE2EACPmockCrashMidStreamProjectsRuntimeFailure.QA and evidence
RT-acp-stream-disconnect-recovery— pass77bfa676781096cad214652973d291c4acd1073e2026-08-05T21:18:18ZWeb/Docs impact
The web application needed no code change: it already renders persisted session events and explicit runtime error events from the daemon. The public contract changed at the runtime/consumer boundary, so session lifecycle and failure-debugging docs were updated. The QA scenario exercises the API and agent-manageable surfaces; there is no new UI control or client-side state.
Compozy Impact Audit
compozy__session_promptterminal classification changed soprocess_exitreturnstool_backend_failed/backend_dead; tool ID, descriptor, input/output schema, digest, risk flags, and capability gates are unchanged. Existing native-tool and daemon integration suites cover the new failure result.workspace_idpropagation through session manager, store, HTTP/SSE, CLI, native tools, event persistence, and transcript reads is unchanged. The integration test creates and reads the affected session through one workspace and verifies recovery by the same session ID; no global cache or cross-workspace list/read path was added.skills/compozy/references/runtime-operations.mdto document explicit terminal completion, retained partial events, process diagnostics, and safe explicit retry.Compatibility and risks
This is a greenfield hard cut: crash bundles now use schema v2, with no v1 compatibility branch. Stream consumers that previously relied on bare EOF as success will now receive the correct failure unless the daemon emitted an explicit completion event.
Automatic retry is intentionally excluded because a prompt may already have caused external side effects before the process disconnected. Recovery is explicit and reuses the same session. Signal diagnostics are present when the operating system exposes a signal; otherwise the exit code, stderr tail, runtime error, and persisted events remain available.
The deterministic ACP fault fixture exercises the exact process boundary without depending on an unreliable live-provider crash. The remaining risk is provider-specific behavior outside that boundary, mitigated by the common subprocess/ACP lifecycle and public-surface integration coverage.
Scope relative to #306
This PR does not change ACP protocol-version negotiation, initialize-handshake compatibility, provider presets, or Windows executable discovery. The only conceptual overlap is the shared ACP subprocess failure boundary; this diff is limited to stream disconnect classification, process-exit diagnostics, safe recovery, and client-visible failure semantics.
Fixes #315
Summary by CodeRabbit