feat: exec command - #60

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

Inline comments:
In `@internal/cli/workspace_config_test.go`:
- Around line 243-264: The two tests in root_test.go
(TestStartCommandWrapsWorkspaceDefaultErrors and
TestSyncCommandWrapsWorkspaceRootErrors) call os.Chdir directly causing races
with workspace_config_test.go; change them to use the existing helper
chdirCLITest (which acquires cliWorkingDirMu) or wrap their os.Chdir calls with
cliWorkingDirMu.Lock()/Unlock() to serialize directory changes; update both
tests to call chdirCLITest(t, targetDir) (or add explicit mutex locking around
the Chdir and restore) so directory changes are protected by the cliWorkingDirMu
mutex.

In `@internal/core/run/result.go`:
- Around line 74-80: shutdownErr (which can include
awaitUIAfterCompletion()/shutdownUI() errors) is currently used to set
result.Error and mark runs as canceled, which hides actual job failures; change
the logic in result.go so teardown errors do not overwrite job outcomes: first
inspect failures (the failures slice) and set result.Error from failures[0] when
present, derive cancellation only from job-level cancellation flags or explicit
cancellation causes (not from shutdownErr), and store teardown/UI errors
separately (e.g., result.TeardownError or append to a result.Errors list)
instead of assigning shutdownErr.Error() to result.Error; apply the same fix to
the other similar block referenced (lines ~84-96) so teardown errors are
surfaced but don't mark successful job runs as canceled or hide earlier
failures.

---

Outside diff comments:
In `@internal/core/run/execution.go`:
- Around line 918-959: The code currently sets emitHuman :=
cfg.humanOutputEnabled() unconditionally which allows stderr fallback logs while
the UI is active; change emitHuman to respect the UI by setting it to
cfg.humanOutputEnabled() && !useUI (or equivalent logic) so that when useUI is
true, emitHuman is false; update the local variable used in the call to
handleSessionTimeout/recordFailureWithContext and the eventual
executeSessionAndResolve invocation so all failure/cancel paths
(handleNilExecution, handleSessionCancellation, handleSessionTimeout,
buildFailureResult) will not emit raw stderr fallback lines while the TUI is
active.

---

Nitpick comments:
In @.gitignore:
- Around line 17-18: The .gitignore contains redundant patterns for
node_modules; remove the duplicate entries and keep a single concise pattern
(e.g., "**/node_modules" or "node_modules/") and delete "**/**/node_modules" and
the extra "node_modules/" so only one node_modules ignore pattern remains.

In `@internal/cli/root_command_execution_test.go`:
- Around line 267-320: The helper executeRootCommandCapturingProcessIO
introduces a race by reassigning global os.Stdout/os.Stderr while commands may
spawn goroutines; instead stop mutating os.Stdout/os.Stderr and rely on
cmd.SetOut and cmd.SetErr (as used on lines with cmd.SetOut/cmd.SetErr) to
capture output, or refactor code paths that write directly to the globals to
accept io.Writer injection; also consolidate this helper with
captureExecuteStreams into a shared test utility so the same safe capture
strategy is used everywhere.

In `@internal/cli/root_test.go`:
- Around line 280-399: Convert the standalone tests
TestExecHelpShowsExecFlagsOnly, TestExecHelpMatchesGolden,
TestREADMEExecDocumentationMatchesCurrentContract, and
TestActiveDocsAndHelpFixturesOmitLegacyArtifactRoot into a single table-driven
test that uses t.Run("Should ...") subtests for each scenario (e.g.,
t.Run("Should show exec flags only", func(t *testing.T){...})), or wrap each
existing test body in a t.Run call with a "Should ..." name; ensure all subtest
names follow the "Should ..." pattern and keep the original assertions and
helper calls (findCommand, executeRootCommand, mustCLITestDataPath,
mustCLIRepoRootPath) unchanged, and apply the same conversion for the other
exec-related tests mentioned (lines ~491-605) so every case uses t.Run("Should
...").

In `@internal/cli/workspace_config_test.go`:
- Around line 104-106: The test
TestApplyWorkspaceDefaultsUsesExecOverridesOverDefaults calls chdirCLITest which
serializes via the cliWorkingDirMu mutex, so leaving t.Parallel() is misleading;
either remove the t.Parallel() call from this test (and any other tests that
call chdirCLITest) or refactor chdirCLITest to avoid global working-directory
state (e.g., make it return an isolated temp dir and not use cliWorkingDirMu) so
true parallelism is possible—refer to
TestApplyWorkspaceDefaultsUsesExecOverridesOverDefaults, chdirCLITest, and
cliWorkingDirMu to locate the code to change.

In `@internal/core/api_test.go`:
- Around line 32-43: Update the test
TestConfigValidateRejectsExecModeWithoutPromptSource to assert the validation
error message content rather than only checking nil: call Config{Mode: ModeExec,
IDE: IDECodex, OutputFormat: OutputFormatText}.Validate(), verify err is non-nil
and use an error assertion (e.g., ErrorContains) to check for the expected
substring produced by Validate (for example mentioning missing prompt source or
exec mode requirement), referencing Config.Validate and ModeExec to locate the
validation logic.

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

Inline comments:
In `@internal/cli/root_test.go`:
- Around line 1332-1335: The wrapper executeRootCommand currently returns stdout
+ stderr which can reorder interleaved output; change it to preserve emission
order by capturing both streams into a single unified writer when calling
executeRootCommandWithIO (e.g., pass a single io.Writer that is used for both
stdout and stderr) so the returned string reflects actual emission order; apply
the same unified-stream change to the other wrapper variants that call
executeRootCommandWithIO in this file (the functions around the same area).
- Around line 559-563: The subtests in the table-driven tests call
t.Run(tc.name, ...) with names like "positional prompt" that don't follow the
required "Should..." convention; update the test case names in the cases table
(the elements used by the loop over cases) so each tc.name begins with "Should
..." (e.g., "Should handle positional prompt"), or replace the t.Run call to
format a "Should ..." prefix (t.Run(fmt.Sprintf("Should %s", tc.name), ...)) so
all subtest names conform to the t.Run("Should...") pattern referenced by the
test harness; locate the table of test cases and the t.Run invocation inside the
loop to apply the change (cases, tc.name, t.Run).

In `@internal/core/run/execution.go`:
- Around line 166-172: The shutdown fallback writes currently only check
humanOutputEnabled() and therefore still print to stderr during interactive UI
runs; update the drain/force/completion fallback branches in execution.go to
first check c.execCtx.cfg.humanOutputEnabled() before writing to os.Stderr
(apply same guard used around the existing UI path), remove or neutralize
wording that implies a graceful-only shutdown (change the completion message in
the final branch to a neutral statement like "Controller shutdown complete" so
it remains accurate after beginForce()), and ensure that publishShutdownStatus()
remains the primary mechanism for UI state updates; apply these same guarded
changes to the other fallback write sites corresponding to the ranges around the
existing branches (the ones referenced near lines 208-215 and 239-241).

In `@internal/core/run/result_test.go`:
- Around line 11-158: Add a new test that exercises
deriveRunStatus()/buildExecutionResult()'s canceled-precedence branch: create a
runArtifacts/config like the other tests, build a jobs slice with a job having
status runStatusCanceled (use the job struct and runArtifacts.JobsDir for
paths), pass non-nil []failInfo (e.g. failInfo{{err: errors.New("job failed")}})
and a teardown error to buildExecutionResult, then assert that result.Status ==
runStatusCanceled (not failed) while preserving result.Error and
result.TeardownError as appropriate; place the test alongside the existing
TestBuildExecutionResult... tests to cover the highest-priority canceled branch.

---

Nitpick comments:
In `@internal/core/run/execution_acp_test.go`:
- Around line 691-705: The test TestHandleNilExecutionReturnsSetupFailure
currently only calls handleNilExecution with emitHuman hard-coded true, so add a
table-driven subtest matrix using t.Run to exercise both emitHuman=true and
emitHuman=false; for each subtest call handleNilExecution(&job{...}, 0,
emitHuman) and capture/inspect stderr (or the logger output) to assert the new
suppression behavior in addition to the existing checks on result.status and
result.failure.err; update assertions inside each subtest to validate both the
setup-failure payload and the presence or absence of the human-readable output
depending on the emitHuman value so both branches of the function are covered.

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

Inline comments:
In `@internal/cli/root.go`:
- Around line 811-814: Avoid calling readPromptFromCommandInput(cmd.InOrStdin())
eagerly; only probe stdin after explicit prompt sources (the [prompt] value and
--prompt-file) have been ruled out. Change the logic in root command setup so
you first check whether a prompt value or prompt-file flag is set, and if
neither is present then detect whether stdin is a non-interactive pipe/tty (use
os.Stdin.Stat to check for a pipe) and perform a non-blocking readiness check
before calling readPromptFromCommandInput; if stdin is not ready or is a
terminal, skip calling readPromptFromCommandInput to prevent blocking. Ensure
the same change is applied to the other block that currently calls
readPromptFromCommandInput (the code around the 863–885 region) so both places
only read stdin when explicitly needed.

In `@internal/core/agent/registry.go`:
- Around line 341-356: The validation in validateRuntimeOutputFormat incorrectly
rejects the zero-value output format; treat the empty string as equivalent to
model.OutputFormatText by accepting "" in the allowed cases (or normalize
cfg.OutputFormat = model.OutputFormatText before validating). Update
validateRuntimeOutputFormat (referencing cfg.OutputFormat and cfg.Mode) so "" is
considered valid and the existing exec-mode check continues to compare against
model.OutputFormatText (or the normalized value).

In `@internal/core/run/exec_flow_integration_test.go`:
- Around line 298-333: Rename the subtest names in the table-driven test to
follow the "Should..." convention: update the t.Run labels for each case
(currently values like "keeps agent message chunks", "drops plan updates", etc.)
to "Should keep agent message chunks", "Should drop plan updates", "Should keep
unknown completed updates", and "Should drop unknown running updates" so they
match the repo's enforced t.Run("Should...") pattern; ensure these updated
labels are used in the loop that calls t.Run for the tests exercising
shouldEmitLeanSessionUpdate.

In `@internal/core/run/exec_flow.go`:
- Around line 1054-1056: The run ID generator buildExecRunID currently uses
time.Format("20060102-150405-000000000") so the trailing zeros are literal and
IDs collide within the same second; update buildExecRunID to include fractional
seconds by using the correct Go layout (e.g., "20060102-150405.000000000") or
alternatively append time.Now().UTC().UnixNano() (or a short hex/base36 of it)
to the ID so multiple IDs generated in the same second are unique; modify the
buildExecRunID function and its fmt.Sprintf call accordingly.
- Around line 405-437: The goroutine in streamExecSession can leak because it
never observes context cancellation; change streamExecSession to accept the
attempt context (e.g., streamExecSession(ctx context.Context, execution
*sessionExecution, state *execRunState)), and inside the goroutine replace the
range over execution.session.Updates() with an explicit loop that selects on
ctx.Done() and a receive from execution.session.Updates(); when ctx is done
return (and send ctx.Err() on streamErrCh), otherwise handle the update as
before (calling execution.handler.HandleUpdate and state.emitSessionUpdate).
Update the call site to pass attemptCtx into streamExecSession so the goroutine
stops promptly when attemptCtx is canceled.

In `@internal/core/run/execution_acp_integration_test.go`:
- Around line 1137-1182: The captureExecuteStreams helper blocks if fn() writes
more than the OS pipe buffer because stdout/stderr are only read after fn
returns; fix by starting background goroutines that continuously read from
stdoutRead and stderrRead into byte buffers (or io.ReadAll) before calling fn(),
use channels to return the read results (bytes and any read error), then call
fn(), close stdoutWrite/stderrWrite, and wait for the reader goroutines to
finish and send their results; reference the symbols captureExecuteStreams,
stdoutRead, stdoutWrite, stderrRead, stderrWrite and runErr to locate where to
spawn the readers, pass their outputs back, and return the collected
stdout/stderr strings along with runErr.

---

Outside diff comments:
In `@internal/core/workspace/config.go`:
- Around line 169-188: ProjectConfig.Validate currently only validates
validateExec(cfg.Exec) and misses cases where output_format is set in Defaults
but Exec.output_format is unset (e.g., defaults.output_format="json" with
exec.tui=true), causing invalid effective exec config at runtime; update
ProjectConfig.Validate to compute the effective exec config by merging
cfg.Defaults (specifically Defaults.OutputFormat) into cfg.Exec when
Exec.OutputFormat is empty and then pass that merged exec struct into
validateExec (also apply the same merge logic in the other validation block
referenced around validateExec usage at the later location mentioned).

---

Nitpick comments:
In `@internal/cli/root.go`:
- Around line 777-779: Replace the reflect.DeepEqual call with slices.Equal for
the normalized []string comparison: change
reflect.DeepEqual(core.NormalizeAddDirs(cfg.AddDirs),
core.NormalizeAddDirs(record.AddDirs)) to slices.Equal(...) and ensure the
"slices" package is imported; keep the existing cmd.Flags().Changed("add-dir")
check and use core.NormalizeAddDirs on both cfg.AddDirs and record.AddDirs as
before to perform the equality check.

In `@internal/core/run/exec_flow.go`:
- Around line 791-797: The early conditional in execRunState.emit has redundant
dead logic: when s == nil || s.events == nil the function always returns nil but
currently contains an unnecessary nested strings.TrimSpace(event.Output) and
event.Type check; simplify by making the top-level guard (in method emit on type
execRunState) immediately return nil when s == nil || s.events == nil and remove
the inner conditional that checks event.Output and event.Type, leaving normal
behavior for the rest of the method that uses s.events.