feat: use acp instead of stream raw json - #34

internal/setup/bundle.go (1)

27-29: Consider dropping the unused error return from bundledSkillsRoot.

The function only has one call site (in bundle_test.go) and never actually returns an error, so the (fs.FS, error) signature is unnecessary. Simplifying to (fs.FS) would eliminate redundant error handling at the call site.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/setup/bundle.go` around lines 27 - 29, The function
bundledSkillsRoot currently returns (fs.FS, error) but never returns an error;
change its signature to return only fs.FS (func bundledSkillsRoot() fs.FS) and
update the body to return skills.FS directly; then update all call sites
(notably bundle_test.go) to stop expecting an error return and remove the
redundant error handling/unwrapping code. Ensure any references to the old
two-value return are adjusted to a single-value use.

internal/core/run/execution_ui_test.go (1)

60-117: Please fold these shutdown checks into t.Run("Should...") cases.

The three added tests all build the same fake controller/UI harness and only vary the trigger plus expected shutdown effects. A table-driven subtest suite will match the repo test pattern and remove most of the duplicated setup here.

As per coding guidelines, "Use table-driven tests with subtests (t.Run) as the default pattern" and "MUST use t.Run("Should...") pattern for ALL test cases."

Also applies to: 119-183, 185-240

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/execution_ui_test.go` around lines 60 - 117, The test
TestExecutorControllerUIQuitEntersDrainingPath duplicates setup across multiple
similar cases; refactor it into a table-driven subtest using t.Run("Should...")
where each entry varies the trigger (calls to executorController.requestShutdown
with uiQuitRequestDrain or others) and the expected effects (cancelCalls,
ui.shutdownCalls, observed shutdownPhase via ui.eventsCh); keep the shared
harness creation (fakeLifecycleUISession, jobExecutionContext,
executorController, done channel, resultCh and goroutine invoking
controller.awaitCompletion) and move per-case actions/assertions into each
subtest, using identifiers like TestExecutorControllerUIQuitEntersDrainingPath,
executorController.requestShutdown, uiQuitRequestDrain, fakeLifecycleUISession,
awaitCompletion, shutdownStatusMsg and shutdownPhaseDraining to locate the code
to change. Ensure each subtest asserts its specific expectations and preserves
timeouts used on ui.eventsCh and resultCh.

internal/core/run/ui_model_test.go (2)

76-97: Table-driven test loop should use t.Run subtests.

The loop iterates over job states but doesn't wrap each case in t.Run. This prevents individual test case identification in output and skips t.Parallel() benefits for subtests.

♻️ Suggested refactor

 	for _, tc := range []struct {
 		state jobState
 		label string
 	}{
 		{jobPending, "PENDING"},
 		{jobRunning, "RUNNING"},
 		{jobSuccess, "SUCCESS"},
 		{jobFailed, "FAILED"},
 	} {
+		t.Run(fmt.Sprintf("Should return correct helpers for %s state", tc.label), func(t *testing.T) {
+			t.Parallel()
 			if got := m.getStateLabel(tc.state); got != tc.label {
 				t.Fatalf("unexpected state label for %v: %q", tc.state, got)
 			}
 			if got := m.jobStateIcon(tc.state); got == "" {
 				t.Fatalf("expected icon for state %v", tc.state)
 			}
 			if m.jobStateColor(tc.state) == nil {
 				t.Fatalf("expected color for state %v", tc.state)
 			}
 			if m.jobBorderColor(&uiJob{state: tc.state}) == nil {
 				t.Fatalf("expected border color for state %v", tc.state)
 			}
+		})
 	}

As per coding guidelines: "MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_model_test.go` around lines 76 - 97, The table-driven
loop in ui_model_test.go should be converted to t.Run subtests so each case is
identifiable and can run in parallel; for each test case (enumerated states
jobPending, jobRunning, jobSuccess, jobFailed) wrap the assertions in t.Run with
a descriptive name (e.g. the label) and call t.Parallel() at the start of the
subtest, then perform the same checks against m.getStateLabel, m.jobStateIcon,
m.jobStateColor, and m.jobBorderColor(&uiJob{state: tc.state}); ensure failures
use t.Fatalf (or t.Errorf) inside the subtest so failures are reported per-case.

---

61-114: Consider splitting this test into focused subtests.

This test covers multiple unrelated concerns: number formatting, duration formatting, state labels, icons, colors, elapsed strings, and view rendering. Splitting into separate t.Run blocks would improve readability and make failures easier to diagnose.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_model_test.go` around lines 61 - 114, The test
TestFormattingAndStateHelpersCoverBranches is exercising many concerns; split it
into focused subtests using t.Run for each area: one subtest for formatNumber
and formatDuration (call formatNumber and formatDuration and assert results),
one for state helpers (create newUIModel(1) and separately assert getStateLabel,
jobStateIcon, jobStateColor, jobBorderColor for each jobState), one for elapsed
rendering (use uiJob instances and assert elapsedStr renders), and one for view
rendering (set m.jobs, m.total, m.currentView and assert m.View().Content). Keep
the same assertions and helper values but move them into these named t.Run
blocks so failures point to the specific function under test (referencing
functions: formatNumber, formatDuration, newUIModel, getStateLabel,
jobStateIcon, jobStateColor, jobBorderColor, elapsedStr, View).

internal/core/agent/session.go (1)

80-95: Unlock before channel operations to avoid holding lock during potential blocking.

The mutex is held while performing channel operations (send and close). If the channel send in the select were to block (it won't here due to default), or if close panics on an already-closed channel, the lock would be held. Consider unlocking before channel operations.

♻️ Suggested refactor

 func (s *sessionImpl) finish(status model.SessionStatus, err error) {
 	s.mu.Lock()
 	if s.finished {
 		s.mu.Unlock()
 		return
 	}
 	s.finished = true
 	s.err = err
+	s.mu.Unlock()
+
 	select {
 	case s.updates <- model.SessionUpdate{Status: status}:
 	default:
 	}
 	close(s.updates)
 	close(s.done)
-	s.mu.Unlock()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/session.go` around lines 80 - 95, In sessionImpl.finish,
avoid holding s.mu while doing channel operations: perform the mutex-protected
checks and state updates (check s.finished, set s.finished = true, assign s.err)
while locked, then unlock before doing the non-atomic actions (sending to
s.updates and closing s.updates and s.done). Concretely, move the select and the
close(s.updates)/close(s.done) calls to after s.mu.Unlock() so the lock is not
held during potential blocking or panic-prone channel operations.

internal/core/run/acp_transcript_test.go (1)

200-208: Consider consolidating duplicate test helpers.

This helper is nearly identical to mustContentBlockLoggingTest in logging_test.go. Consider extracting a shared test helper to reduce duplication.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/acp_transcript_test.go` around lines 200 - 208, The two
test helpers mustContentBlockTranscriptTest (in acp_transcript_test.go) and
mustContentBlockLoggingTest (in logging_test.go) are duplicate; extract a single
shared helper (e.g., mustContentBlock or mustContentBlockTest) into a common
test helper file in the same package (e.g., test_helpers_test.go) that calls
model.NewContentBlock, handles the error with t.Fatalf, and returns
model.ContentBlock, then update call sites to use the new helper and remove the
duplicated functions.

internal/core/run/ui_update.go (1)

329-344: Magic number 3 should be a named constant.

The value 3 appears to represent the height of a sidebar item. Extract this to a named constant for clarity and maintainability.

♻️ Suggested improvement

+const sidebarItemHeight = 3
+
 func (m *uiModel) refreshSidebarContent() {
 	var items []string
 	for i := range m.jobs {
 		items = append(items, m.renderSidebarItem(&m.jobs[i], i == m.selectedJob))
 	}
 	m.sidebarViewport.SetContent(strings.Join(items, "\n"))

-	lineOffset := m.selectedJob * 3
+	lineOffset := m.selectedJob * sidebarItemHeight
 	sidebarOffset := m.sidebarViewport.YOffset()
 	sidebarHeight := m.sidebarViewport.Height()
-	if lineOffset > sidebarOffset+sidebarHeight-3 {
-		m.sidebarViewport.SetYOffset(lineOffset - sidebarHeight + 3)
+	if lineOffset > sidebarOffset+sidebarHeight-sidebarItemHeight {
+		m.sidebarViewport.SetYOffset(lineOffset - sidebarHeight + sidebarItemHeight)
 	} else if lineOffset < sidebarOffset {
 		m.sidebarViewport.SetYOffset(lineOffset)
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_update.go` around lines 329 - 344, Replace the magic
number 3 in refreshSidebarContent with a named constant representing the sidebar
item height: declare a package-level constant (e.g. sidebarItemHeight) and use
it instead of the literal in the calculations for lineOffset and the YOffset
adjustments inside uiModel.refreshSidebarContent; update both occurrences
(lineOffset calculation and SetYOffset adjustments) so the intent is clear and
maintainable while leaving renderSidebarItem, m.selectedJob, m.sidebarViewport
and related logic unchanged.

internal/core/agent/registry_test.go (1)

87-105: Remove unnecessary loop variable capture pattern—Go 1.26.1 supports improved loop semantics.

The tc := tc pattern at lines 88 and 257 is no longer required. Go 1.22+ fixed loop variable closure issues, making this redundant for this project.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/registry_test.go` around lines 87 - 105, The test
contains an unnecessary inner loop-variable capture "tc := tc" before t.Run;
remove that redundant reassignment so the subtest uses the loop variable
directly. Update the table-driven loop where tc is used with lookupAgentSpec,
spec.launchCommand(resolveModel(...)), and spec.probeCommand() by deleting the
"tc := tc" line(s) to rely on Go's corrected loop variable semantics.

internal/core/run/ui_view.go (1)

193-206: Cache metric labels could be more descriptive.

The labels "CACHER" and "CACHEW" are abbreviated. Consider using "CACHE_R" / "CACHE_W" or full words for clarity in the UI.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_view.go` around lines 193 - 206, Update the cache metric
labels to be more descriptive: replace the abbreviated strings "CACHER " and
"CACHEW " passed into renderStyledOnBackground with clearer labels such as
"CACHE_R " and "CACHE_W " or full words like "CACHE READ " and "CACHE WRITE "
where the calls occur inside the two renderOwnedLine invocations that render
u.CacheReads and u.CacheWrites (these use innerW, bg, renderStyledOnBackground,
renderGap, value, label, and formatNumber). Ensure spacing/formatting remains
consistent with the surrounding UI text.

internal/core/agent/client_test.go (3)

401-444: Test modifies global PATH environment - ensure isolation.

This test uses t.Setenv to modify PATH which is properly scoped to the test. However, the test doesn't call t.Parallel() which is correct given the environment modification. Consider adding a comment explaining why parallelism is disabled.

📝 Add clarifying comment

 func TestClientCreateSessionSurfacesStartupCommandAndStderr(t *testing.T) {
+	// Not parallel: modifies PATH environment variable
 	tmpDir := t.TempDir()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/client_test.go` around lines 401 - 444, The test
TestClientCreateSessionSurfacesStartupCommandAndStderr intentionally calls
t.Setenv to modify PATH and therefore must not run in parallel; add a short
clarifying comment above the test (or directly above the t.Setenv call) stating
that t.Parallel is omitted because the test mutates global environment (PATH)
and relies on isolation, so running in parallel could interfere with other
tests; reference the test name and the t.Setenv usage in the comment so future
maintainers understand why parallelism is disabled.

---

184-192: Time-based waits may cause flaky tests in slow CI environments.

The 2-second timeouts at lines 186 and 248 could cause intermittent failures on slow CI runners. Consider using a longer timeout or making it configurable via environment variable for CI.

♻️ Consider longer timeout

+const testSessionTimeout = 5 * time.Second
+
 	select {
 	case <-session.Done():
-	case <-time.After(2 * time.Second):
+	case <-time.After(testSessionTimeout):
 		t.Fatal("timed out waiting for session cancellation")
 	}

Also applies to: 246-254

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/client_test.go` around lines 184 - 192, The test uses a
hardcoded 2-second time.After window when waiting on session.Done() which can
flake on slow CI; replace the literal timeout with a configurable duration (e.g.
read from an env var like TEST_TIMEOUT or use a package-level default constant)
and apply it to both uses of time.After(...) in this test so the waits are
longer or adjustable in CI; keep the logic that checks <-session.Done() and
errors.Is(session.Err(), context.Canceled) unchanged, only swap the
2*time.Second literal for the configurable duration used by the test.

---

675-678: Use direct field access instead of reflection to check outcome variant.

The outcomeHasVariant function uses reflection to check for a field named "Cancelled", but direct field access (outcome.Cancelled != nil) is already used elsewhere in the same test file for outcome.Selected. The string concatenation "Cancel"+"led" is also unnecessarily obfuscated.

Replace with direct field access:

-func outcomeHasVariant(outcome acp.RequestPermissionOutcome, variant string) bool {
-	field := reflect.ValueOf(outcome).FieldByName(variant)
-	return field.IsValid() && field.Kind() == reflect.Pointer && !field.IsNil()
+func outcomeCancelled(outcome acp.RequestPermissionOutcome) bool {
+	return outcome.Cancelled != nil
 }

Then update the call site:

-	if !outcomeHasVariant(cancelledResp.Outcome, "Cancel"+"led") {
+	if !outcomeCancelled(cancelledResp.Outcome) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/client_test.go` around lines 675 - 678, The current
outcomeHasVariant uses reflection to detect variants; replace it with direct
field access to use concrete checks (e.g., outcome.Cancelled != nil) to match
the rest of the test file. Modify the outcomeHasVariant function to switch on
the known variant names (such as "Cancelled" and "Selected") and return the
corresponding direct nil-checks (outcome.Cancelled != nil, outcome.Selected !=
nil) instead of reflect.ValueOf; then update any call sites that rely on
outcomeHasVariant to pass those explicit variant names or, better, replace their
calls with the direct nil-checks directly where used.

internal/core/agent/session_helpers_test.go (2)

170-185: Wrap loop iterations in t.Run subtests for proper test isolation and reporting.

The loop iterates over test cases without using t.Run, which means failures won't identify which specific block type failed. As per coding guidelines, use table-driven tests with subtests (t.Run) as the default pattern.

♻️ Proposed refactor

 func TestConvertACPContentBlockFallbacks(t *testing.T) {
 	t.Parallel()

-	cases := []acp.ContentBlock{
-		acp.AudioBlock("U29tZUF1ZGlv", "audio/mpeg"),
-		acp.ResourceLinkBlock("docs", "https://example.com"),
+	cases := []struct {
+		name  string
+		block acp.ContentBlock
+	}{
+		{"audio block", acp.AudioBlock("U29tZUF1ZGlv", "audio/mpeg")},
+		{"resource link block", acp.ResourceLinkBlock("docs", "https://example.com")},
 	}

-	for _, block := range cases {
-		converted, err := convertACPContentBlock(block)
-		if err != nil {
-			t.Fatalf("convert content block fallback: %v", err)
-		}
-		assertBlockTypes(t, converted, model.BlockText)
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			converted, err := convertACPContentBlock(tc.block)
+			if err != nil {
+				t.Fatalf("convert content block fallback: %v", err)
+			}
+			assertBlockTypes(t, converted, model.BlockText)
+		})
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/session_helpers_test.go` around lines 170 - 185, Wrap
each iteration in TestConvertACPContentBlockFallbacks with a t.Run subtest to
isolate failures: inside the for loop create a local copy (e.g., tc := block)
and call t.Run with a descriptive name (like fmt.Sprintf("%T", tc) or a string
identifying the block), then run the test body in the closure (optionally
calling t.Parallel() inside the subtest), invoking convertACPContentBlock(tc)
and using assertBlockTypes(t, converted, model.BlockText); this ensures the loop
variable is captured correctly and failures report which specific block type
failed.

---

187-203: Consider splitting into focused subtests for distinct helper functions.

This test validates multiple unrelated helpers (marshalRawJSON, stringifyValue, renderDiffText) in a single test function. Using t.Run subtests would improve failure isolation and test clarity.

♻️ Suggested structure

 func TestSessionConversionHelpers(t *testing.T) {
 	t.Parallel()

-	raw := marshalRawJSON(map[string]string{"path": "main.go"})
-	if string(raw) != `{"path":"main.go"}` {
-		t.Fatalf("unexpected raw json: %s", string(raw))
-	}
-	if got := stringifyValue(map[string]string{"status": "ok"}); !strings.Contains(got, `"status":"ok"`) {
-		t.Fatalf("unexpected stringified value: %s", got)
-	}
-	if got := stringifyValue("plain"); got != "plain" {
-		t.Fatalf("unexpected plain string: %q", got)
-	}
-	if got := renderDiffText("main.go", "new", nil); !strings.Contains(got, "+++ main.go") {
-		t.Fatalf("unexpected rendered diff: %q", got)
-	}
+	t.Run("marshalRawJSON produces valid JSON", func(t *testing.T) {
+		t.Parallel()
+		raw := marshalRawJSON(map[string]string{"path": "main.go"})
+		if string(raw) != `{"path":"main.go"}` {
+			t.Fatalf("unexpected raw json: %s", string(raw))
+		}
+	})
+
+	t.Run("stringifyValue handles maps and strings", func(t *testing.T) {
+		t.Parallel()
+		if got := stringifyValue(map[string]string{"status": "ok"}); !strings.Contains(got, `"status":"ok"`) {
+			t.Fatalf("unexpected stringified value: %s", got)
+		}
+		if got := stringifyValue("plain"); got != "plain" {
+			t.Fatalf("unexpected plain string: %q", got)
+		}
+	})
+
+	t.Run("renderDiffText includes file header", func(t *testing.T) {
+		t.Parallel()
+		if got := renderDiffText("main.go", "new", nil); !strings.Contains(got, "+++ main.go") {
+			t.Fatalf("unexpected rendered diff: %q", got)
+		}
+	})
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/session_helpers_test.go` around lines 187 - 203, Split
TestSessionConversionHelpers into focused t.Run subtests so failures isolate to
the specific helper: create subtests for marshalRawJSON (assert exact JSON
output), stringifyValue (one subtest for map value containing `"status":"ok"`
and one for plain string returning "plain"), and renderDiffText (assert it
contains "+++ main.go"); keep t.Parallel() either at the top-level test and/or
inside each t.Run closure, and move the existing assertions into their
corresponding subtest bodies referencing the same helper functions
marshalRawJSON, stringifyValue, and renderDiffText.

internal/core/run/execution.go (1)

633-640: Goroutine in waitChannel lacks explicit ownership tracking.

Per coding guidelines, every goroutine must have explicit ownership and shutdown. This monitoring goroutine is acceptable as it naturally terminates when wg.Wait() completes, but consider adding a brief comment clarifying its lifecycle.

📝 Add clarifying comment

 func (j *jobExecutionContext) waitChannel() <-chan struct{} {
 	done := make(chan struct{})
+	// Monitoring goroutine - terminates when all jobs complete (wg.Wait returns)
 	go func() {
 		j.wg.Wait()
 		close(done)
 	}()
 	return done
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/execution.go` around lines 633 - 640, The goroutine started
in jobExecutionContext.waitChannel currently has no explicit ownership note; add
a brief comment above the goroutine in waitChannel explaining that the goroutine
is owned by the jobExecutionContext, its sole purpose is to close the returned
done channel after j.wg.Wait() completes, and that it will always exit when the
wait group finishes (so no explicit shutdown is required). Locate the
waitChannel method and insert the explanatory comment referencing the goroutine,
j.wg.Wait(), and the done channel to satisfy the ownership/shutdown guideline.

internal/core/run/session_view_model.go (1)

289-302: Use constants for plan entry status strings instead of magic strings.

The status matching uses hardcoded strings "completed" and "in_progress". These should reference constants (likely from acp package or define local ones) to prevent typos and improve maintainability.

♻️ Suggested improvement

+const (
+	planStatusCompleted  = "completed"
+	planStatusInProgress = "in_progress"
+)
+
 func buildPlanState(entries []model.SessionPlanEntry) SessionPlanState {
 	state := SessionPlanState{Entries: clonePlanEntries(entries)}
 	for _, entry := range entries {
 		switch entry.Status {
-		case "completed":
+		case planStatusCompleted:
 			state.DoneCount++
-		case "in_progress":
+		case planStatusInProgress:
 			state.RunningCount++
 		default:
 			state.PendingCount++
 		}
 	}
 	return state
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/session_view_model.go` around lines 289 - 302, The
buildPlanState function currently compares entry.Status to magic strings
("completed", "in_progress"); replace those literals with named constants to
avoid typos and improve maintainability. Update the switch in buildPlanState
(and any related uses of SessionPlanEntry/SessionPlanState) to compare against
the canonical constants from the acp package (e.g., acp.PlanEntryStatusCompleted
and acp.PlanEntryStatusInProgress) and treat all other values as pending; if
those acp constants don’t exist, declare local consts (e.g.,
planEntryStatusCompleted, planEntryStatusInProgress) at the top of the file and
use them instead.

internal/core/agent/registry.go (1)

574-593: Joining multiple availability errors loses structured information.

When multiple launchers fail, joinAvailabilityErrors concatenates error strings with |, losing the structured *AvailabilityError type. Consider returning an errors.Join or a custom aggregate error that preserves the structure.

♻️ Preserve error structure

 func joinAvailabilityErrors(spec Spec, errs []error) error {
 	if len(errs) == 0 {
 		return &AvailabilityError{
 			IDE:         spec.ID,
 			DisplayName: spec.DisplayName,
 			DocsURL:     spec.DocsURL,
 			InstallHint: spec.InstallHint,
 			Cause:       errors.New("no ACP launch candidates configured"),
 		}
 	}
 	if len(errs) == 1 {
 		return errs[0]
 	}

-	parts := make([]string, 0, len(errs))
-	for _, err := range errs {
-		parts = append(parts, err.Error())
-	}
-	return errors.New(strings.Join(parts, " | "))
+	// Return the first AvailabilityError with all attempts as cause chain
+	return &AvailabilityError{
+		IDE:         spec.ID,
+		DisplayName: spec.DisplayName,
+		DocsURL:     spec.DocsURL,
+		InstallHint: spec.InstallHint,
+		Cause:       errors.Join(errs...),
+	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/registry.go` around lines 574 - 593, The
joinAvailabilityErrors function currently flattens multiple errors into a joined
string, losing the structured AvailabilityError information; modify
joinAvailabilityErrors to preserve error types by returning either
errors.Join(errs...) or constructing a custom aggregate error that embeds the
original []error (and any AvailabilityError instances) so callers can
unwrap/type-assert individual AvailabilityError values; update the function to
collect the original error values (not just Error() strings) and return
errors.Join(errs...) (or a new AggregateAvailabilityError type that implements
Error() and Unwrap()/Errors() to expose the slice) while keeping the existing
single-error and zero-error behaviors (return the AvailabilityError for
zero-candidate case and the single error for len==1).

internal/core/run/command_io.go (2)

27-42: File close errors are silently ignored; consider logging.

The close() method ignores errors from outFile.Close() and errFile.Close() (lines 32, 35). While these rarely fail in practice, logging them would help diagnose file system issues.

♻️ Log file close errors

 func (s *sessionExecution) close() {
 	if s.releaseClient != nil {
 		defer s.releaseClient()
 	}
 	if s.outFile != nil {
-		_ = s.outFile.Close()
+		if err := s.outFile.Close(); err != nil {
+			s.logger.Warn("failed to close stdout log file", "error", err)
+		}
 	}
 	if s.errFile != nil {
-		_ = s.errFile.Close()
+		if err := s.errFile.Close(); err != nil {
+			s.logger.Warn("failed to close stderr log file", "error", err)
+		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/command_io.go` around lines 27 - 42, The close() method on
sessionExecution currently ignores errors returned by s.outFile.Close() and
s.errFile.Close(); update sessionExecution.close to capture those errors and log
them (e.g., using s.logger.Warn or s.logger.Error) similarly to how
s.client.Close() is handled, producing descriptive messages like "failed to
close outFile" / "failed to close errFile" and include the error value so file
close failures are visible; keep the existing defer for s.releaseClient and the
existing s.client close behavior.

---

201-207: Environment variables for color support are hardcoded.

Consider documenting why these specific environment variables are set, or making them configurable for environments where color output should be disabled.

📝 Add documentation comment

+// buildSessionEnvironment returns environment variables that enable color output
+// in terminal-aware tools running within the ACP session. These ensure consistent
+// rendering of styled output from the underlying agent process.
 func buildSessionEnvironment() map[string]string {
 	return map[string]string{
 		"FORCE_COLOR":    "1",
 		"CLICOLOR_FORCE": "1",
 		"TERM":           "xterm-256color",
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/command_io.go` around lines 201 - 207, The
buildSessionEnvironment function hardcodes color-related env vars which may be
inappropriate in some runtimes; update buildSessionEnvironment to either read
and respect existing user environment (e.g., NO_COLOR or TERM), or accept a
configuration flag to enable/disable forcing color, and add a clear doc comment
above buildSessionEnvironment explaining why FORCE_COLOR, CLICOLOR_FORCE and
TERM are set and when they can be overridden; reference the
buildSessionEnvironment function name so reviewers can find and change the
behavior and add the documentation.

internal/core/run/ui_layout.go (2)

10-33: Consider extracting magic numbers as named constants for maintainability.

The minimum terminal dimensions (80x24) are hardcoded. Extracting these as named constants would improve readability and make future adjustments easier.

♻️ Suggested constants

+const (
+	minTerminalWidth  = 80
+	minTerminalHeight = 24
+)
+
 func (m *uiModel) computeLayout(totalWidth, totalHeight int) uiLayoutState {
-	if totalWidth < 80 || totalHeight < 24 {
+	if totalWidth < minTerminalWidth || totalHeight < minTerminalHeight {
 		return uiLayoutState{
 			mode:          uiLayoutResizeBlocked,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_layout.go` around lines 10 - 33, Extract the hardcoded
minimum terminal dimensions and any other inline "magic" numbers in
computeLayout into named constants to improve readability and maintainability:
replace the literal 80 and 24 with e.g. minTerminalWidth and minTerminalHeight,
and consider naming the 0.28 sidebar ratio (e.g. defaultSidebarRatio) and
chromeHeight/minContentHeight if not already constants; update the
uiModel.computeLayout method and any references to use these new constants so
uiLayoutState construction still behaves identically.

---

45-53: Consider using built-in min/max functions available in Go 1.21+.

The clamp helper can be simplified using the built-in min and max functions if the project targets Go 1.21 or later.

♻️ Simplified clamp

 func clamp(value, minValue, maxValue int) int {
-	if value < minValue {
-		return minValue
-	}
-	if value > maxValue {
-		return maxValue
-	}
-	return value
+	return max(minValue, min(value, maxValue))
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/run/ui_layout.go` around lines 45 - 53, Replace the manual
comparisons in the clamp function with the Go 1.21 built-in min/max helpers: in
function clamp(value, minValue, maxValue int) return min(max(value, minValue),
maxValue) so the logic is expressed using the built-in min and max; ensure the
codebase is targeting Go 1.21+ before making this change and update any tests if
necessary.

internal/core/agent/client.go (1)

479-511: Consider documenting the magic idle wait duration.

Line 509 uses a hardcoded 15*time.Millisecond for waitForIdle. This appears to be a grace period for final updates to arrive before marking the session complete. Consider adding a comment explaining the rationale or making it configurable.

Add explanatory comment

-	session.waitForIdle(ctx, 15*time.Millisecond)
+	// Allow a brief grace period for any final streamed updates to arrive
+	// before marking the session complete.
+	session.waitForIdle(ctx, 15*time.Millisecond)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/agent/client.go` around lines 479 - 511, The hardcoded
15*time.Millisecond grace period in runPrompt when calling session.waitForIdle
is unclear; update runPrompt to either add a concise comment explaining why 15ms
was chosen (what it guards against and any tradeoffs) or make the wait duration
configurable (e.g., plumb a configurable idleWait duration into clientImpl or
sessionImpl and use that instead) and replace the literal with the named
variable; reference runPrompt, session.waitForIdle, and the 15*time.Millisecond
literal when applying the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes