fix: codex acp integration - #151

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/core/agent/terminal.go`:
- Around line 332-337: newTerminalOutputBuffer currently treats a nil or
non-positive limit as "unlimited", which can let a single command exhaust RAM;
change newTerminalOutputBuffer to apply a server-side default cap when limit is
nil or <= 0 (e.g., a constant like defaultOutputByteLimit such as 10*1024*1024
bytes), so resolvedLimit is set to that default instead of 0; update/introduce
the default constant (name: defaultOutputByteLimit) and ensure
terminalOutputBuffer{limit: resolvedLimit} uses that capped value so
OutputByteLimit unset no longer means unlimited.
- Around line 123-129: The code currently calls c.removeTerminal(...) before
terminal.waitFor(ctx), which removes the terminal from c.terminals and can
orphan the subprocess on context deadline; instead, look up the terminal without
removing it (or fetch it from c.terminals directly), call terminal.kill(), wait
for it with terminal.waitFor(ctx), and only after waitFor returns (successful
cleanup or final error) call c.removeTerminal(...) to remove tracking; update
code paths that used removeTerminal(...) to use the non-destructive lookup
first, then call removeTerminal by name/ID after waitFor completes.

In `@internal/core/provider/coderabbit/coderabbit_test.go`:
- Around line 147-175: The test table entries use names that will be passed to
t.Run but do not follow the required "Should ..." pattern; update the name
fields for the affected cases in the test table in coderabbit_test.go (the
entries with headSHA "head-without-review", "head-new" with stale latest
provider review, and "head-new" with stale review + pending status) so each name
begins with "Should ..." (e.g., "Should mark current settled when CodeRabbit
completed current head without a provider review", "Should mark current settled
when CodeRabbit completed current head but latest provider review is stale",
"Should be pending when CodeRabbit is still processing current head with stale
latest review") to comply with the t.Run naming convention.
- Around line 203-218: Wrap the test body of
TestWatchStatusFailsWhenCodeRabbitStatusFailsWithoutProviderReview in a t.Run
subtest whose name starts with "Should" (e.g., t.Run("Should return coderabbit
status failure", func(t *testing.T) { ... })), move the assertions and the call
to New(...).WatchStatus(...) into that subtest, and call t.Parallel()
appropriately (either keep the existing top-level t.Parallel() or call
t.Parallel() inside the subtest) so the test follows the required
t.Run("Should...") pattern while preserving the existing logic and referenced
symbols (TestWatchStatusFailsWhenCodeRabbitStatusFailsWithoutProviderReview,
New, WithCommandRunner, testWatchStatusRunnerWithStatuses, WatchStatus).

In `@internal/daemon/review_watch_test.go`:
- Around line 117-190: Both subtests titled "Should fetch and fix unresolved
reviews when provider settled without a current review object" and "Should
declare clean after provider settled current head and no unresolved reviews
remain" are independent and should be marked parallel; add t.Parallel() as the
first line inside each subtest's anonymous func(t *testing.T) to enable
concurrent execution and follow the test guideline. Locate the two t.Run(...)
blocks in internal/daemon/review_watch_test.go and insert t.Parallel() at the
top of each subtest function body (the funcs passed to t.Run).

In `@internal/daemon/run_manager.go`:
- Around line 1164-1176: The resumeExistingExecRun success path currently
returns createdRun=false which loses the fact the persisted row was touched;
change the return to propagate the `ok` flag returned by `resumeExistingExecRun`
(i.e., return row, ok, nil) so callers like `openRunScopeForStart`/`startRun`
know the row was resumed and can run rollback/terminal-state mirroring; ensure
the code path that calls `resumeExistingExecRun` uses the `ok` boolean instead
of a hardcoded false when deciding cleanup or failure-marking.

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 `@internal/daemon/run_manager_test.go`:
- Around line 1127-1283: Wrap each standalone test scenario into a t.Run("Should
...") subtest: for TestRunManagerStartExecRunRetriesImplicitRunIDCollision,
TestRunManagerStartExecRunExplicitDuplicateRunIDStillFails, and
TestRunManagerStartExecRunAllocatesDistinctImplicitRunIDsInParallel move the
current test body into a t.Run call (e.g. t.Run("Should retry implicit run-id
collision", func(t *testing.T) { ... })) so the existing setup, channels, env :=
newRunManagerTestEnv(...), assertions and cleanup remain inside the closure;
keep the top-level Test... function as a thin wrapper that calls t.Run so the
repo-wide t.Run("Should...") pattern is followed and test semantics are
unchanged.