refactor: tui redesign - #201

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/handlers_test.go`:
- Around line 429-525: Refactor the test function
TestRunJobControlHandlersForwardPauseAndMessageRequests to use the t.Run subtest
pattern as required by project guidelines. Extract the pause endpoint testing
logic into a separate t.Run("Should pause run job", func(t *testing.T) {...})
subtest and the message endpoint testing logic into another t.Run("Should send
message to run job", func(t *testing.T) {...}) subtest. The shared setup
(handlers, engine, and middleware registration) can remain outside the subtests
before the individual t.Run calls. This will isolate failures to specific
functionality and align with the project's mandatory testing patterns.

In `@internal/cli/form_test.go`:
- Around line 495-556: The test functions
TestTaskRunFormInputsApplyMultipleWorkflowSelection and the other test case
mentioned in the consolidated sites are written as top-level test functions but
should follow the repository's testing convention by wrapping their test logic
in t.Run("Should...") subtests. Refactor each test function to use the t.Run
pattern where the subtest name describes the specific scenario being tested
(e.g., "Should apply multiple workflow selection correctly"), and move all the
test assertions and setup into the subtest body.

In `@internal/cli/root_command_execution_test.go`:
- Around line 1264-1342: The test function
TestTasksRunInteractiveFormCanStartMultipleWorkflows must be refactored to use
the required t.Run("Should...") pattern. Wrap all the existing test logic (from
the current function body) inside a t.Run call with a descriptive "Should..."
name that explains what the test validates, such as "Should start multiple
workflows from interactive form". The outer function should only contain the
t.Run call, and all other setup and assertions should be moved into the callback
function passed to t.Run.

In `@internal/cli/task_runtime_form.go`:
- Around line 53-75: The scopeWorkflow variable at line 53 is set to false for
single-workflow runs (when len(slugs) <= 1), which prevents proper lookup and
preseeding of workflow-qualified runtime rules at lines 133-143 and 170-174,
causing existing workflow-scoped overrides to be lost. Change the logic to
always use the actual workflow name for rule matching and key generation,
regardless of the number of workflows. Introduce a separate display-only
workflow variable (which can be empty for single-workflow mode) while keeping
the workflow parameter used in the populate call and rule matching logic
consistently scoped to the real workflow name. Update the form.populate call and
all rule lookup/matching logic at the cited lines to use the real workflow name
for keys and matching, with display labels using only the display variable.

In `@internal/cli/tasks_run_wizard_test.go`:
- Around line 15-369: Refactor the wizard tests to follow repository standards
by consolidating the multiple standalone top-level test functions
(TestTaskRunWizardModelSelectsMultipleWorkflowsAndSubmits,
TestTaskRunWizardModelPreservesAndReordersWorkflowSelection,
TestTaskRunWizardModelBuildsWorkflowScopedOverrides,
TestTaskRunWizardModelPreservesOverridesAcrossBackNavigation,
TestTaskRunWizardTextInputAcceptsGlobalShortcutCharacters,
TestTaskRunWizardRuntimeTextInputAcceptsNavigationLetters,
TestTaskRunWizardOverrideTextInputAcceptsNavigationLetters,
TestTaskRunWizardModelFiltersWorkflowSelection,
TestTaskRunWizardModelAcceptsManualWorkflowFallback, and
TestTaskRunWizardViewFitsTerminalBounds) into fewer parent test functions that
use t.Run("Should...") subtests for each test case. Implement table-driven test
patterns where applicable, such as for TestTaskRunWizardViewFitsTerminalBounds
which already has multiple dimensions to test. Each subtest should follow the
pattern t.Run("Should<describe_behavior>", func(t *testing.T) { ... }) and tests
should call t.Parallel() within each subtest as appropriate.

In `@internal/core/model/job_control.go`:
- Around line 157-168: In the completeJobControlResponse function, the RunID and
JobID fields are conditionally assigned only when empty, which allows mismatched
values to propagate. Remove the conditional checks for empty values and
unconditionally assign resp.RunID = req.RunID and resp.JobID = req.JobID to
ensure the response always contains the normalized request identities, enforcing
the API contract regardless of what the controller initially returned.

In `@internal/core/model/task_runtime.go`:
- Around line 81-83: The condition in the TaskRuntime matching logic (line
81-83) incorrectly treats whitespace-only workflow qualifiers as set to empty
string instead of unset. Currently, it returns false whenever Workflow is not
nil, even if it trims to empty string. Fix this by adding an additional check:
only enforce the workflow equality comparison if the trimmed Workflow value is
non-empty. Modify the condition to check that after trimming, the Workflow is
not empty before comparing it with the target's workflow. This ensures
blank/whitespace-only qualifiers are treated as unscoped rules that match any
workflow, rather than rules that match only empty workflows.

In `@internal/core/plan/prepare_test.go`:
- Around line 288-290: The comparison using wantTaskNumbers[job.CodeFiles[0]]
relies on implicit zero-value semantics for missing keys, which can mask bugs
when job.TaskNumber is also 0. Instead, use the two-value form of map lookup to
explicitly verify that the key exists in the wantTaskNumbers map before
asserting the task number. Check that job.CodeFiles[0] is present as a key in
wantTaskNumbers, and only compare values when the key exists, otherwise fail the
test with a clear message indicating the missing key.

In `@internal/core/run/internal/acpshared/session_exec.go`:
- Around line 315-317: The Pause() and SendMessage() methods in session_exec.go
should not silently fall back to context.Background() when receiving a nil
context, as this breaks cancellation and deadline propagation across runtime
boundaries. Replace the conditional check that assigns context.Background() to
ctx with code that returns an explicit error when ctx is nil. This ensures
callers are notified of invalid input rather than having their cancellation
signals ignored when these methods call external services like CancelSession().

In `@internal/core/run/transcript/model_test.go`:
- Around line 74-109: The test function
TestSessionViewModelAppendsUserMessagesAsDistinctEntries is structured as a
top-level test body instead of following the required subtest wrapper pattern.
Wrap the entire test logic inside a t.Run() call using a descriptive "Should..."
naming convention (for example, "Should append user messages as distinct
entries"), and move the test body into the callback function passed to t.Run().
This ensures consistency with the project's coding guidelines for all test cases
in _test.go files.

In `@internal/core/run/transcript/model.go`:
- Around line 345-356: The findEntryID method searches for entries by ID without
restricting to user-message entries only. This allows user message IDs to
collide with non-user entry IDs, causing applyUserMessage to incorrectly merge
user content into wrong entries. Modify the findEntryID method to add an
additional condition in the entry lookup loop that checks whether each entry is
a user-message type before matching on ID, ensuring only user-message entries
are returned from the search.

In `@internal/core/run/ui/model_test.go`:
- Around line 93-103: The test loop iterating over jobState values (jobPending,
jobRunning, jobRetrying, jobSuccess, jobFailed) needs to be refactored to use
the t.Run pattern with named subtests. For each state in the loop, wrap the
existing assertions for jobStateIcon, jobStateColor, and jobBorderColor inside a
t.Run call with a descriptive name following the "Should..." convention (for
example, naming the subtest based on the current state being tested). This
isolates each state's test results and complies with the repository's test
policy requiring t.Run subtests for all test cases.

In `@internal/core/run/ui/update_test.go`:
- Around line 136-238: The test functions
TestPauseSelectedJobEnablesComposerAndMessageResumes (lines 136-238), and the
other affected test functions at lines 341-372, 461-491, and 870-932 in file
internal/core/run/ui/update_test.go do not follow the required
t.Run("Should...") subtest pattern. For each of these test functions, wrap the
main test logic inside a t.Run call with a descriptive subtest name that
explains what the test should do (following the pattern t.Run("Should...",
func(t *testing.T) { ... })), rather than having the test body directly in the
top-level function. This ensures consistency with the coding guidelines that
require t.Run subtests for all test cases.

In `@internal/core/run/ui/update.go`:
- Around line 224-225: The code creates timeout contexts using
context.Background() which detaches them from the UI session's lifecycle,
allowing operations to continue for the full 30-second timeout even when the UI
shuts down. Add a context field to the uiModel struct, pass the cancellation
context from uiController to uiModel during initialization, and then replace
context.Background() with this passed context as the base for the timeout
contexts in the context.WithTimeout calls at lines 224-225 and 251-252. This
ensures job-control requests respect the UI/session lifecycle.

In `@internal/core/run/ui/view_test.go`:
- Around line 1487-1497: The assertNoForcedBackground function only checks for
truecolor (48;2;) and 256-color (48;5;) background SGR codes, but misses
16-color background codes (40–47 for standard colors and 100–107 for bright
colors) and reverse video (7). Extend the conditional check to include these
additional SGR patterns alongside the existing checks for 48;2; and 48;5;,
ensuring the function detects all possible ways a forced background could be
applied to the content string.
- Around line 38-60: The test function
TestJobsBodyFitsWindowWidthAcrossBreakpoints and the other test functions
referenced in the consolidated sites (at lines 358-494, 584-742, 1077-1157,
1425-1480, and 1544-1707) are not following the required "Should..." naming
pattern for subtests. Rename all t.Run calls across these test functions to use
descriptive "Should..." names instead of format-based names like "80x24". For
example, replace the format-string-based subtest name with a descriptive name
like "Should fit jobs body width at breakpoint" or similar that clearly explains
what the test case validates. This should be applied consistently across all the
mentioned test functions in the file to comply with the table-driven test
pattern with descriptive subtest naming requirement.

In `@internal/daemon/run_snapshot.go`:
- Around line 175-191: The jobQueuedTaskNumber function returns 0 prematurely
when both TaskTitle and TaskType are empty, preventing it from attempting to
extract task identity numbers from CodeFile and CodeFiles. Remove the early
return gate that checks if both TaskTitle and TaskType are empty strings (the
condition that returns 0 before the ExtractTaskIdentityNumber calls). This
allows the function to proceed and extract valid task numbers from CodeFile and
CodeFiles even when the metadata fields are absent.

In `@internal/store/rundb/run_db.go`:
- Around line 1591-1613: The three functions projectJobPausingState,
projectJobPausedState, and projectJobResumedState all call
jobIDFromIndex(payload.Index, "") with an empty string as the second argument,
which results in an empty job_id when Index equals 0, breaking state continuity.
Fix each function by providing a non-empty fallback value to the second
parameter of jobIDFromIndex instead of an empty string, ensuring these
pause/resume events are correctly keyed under the proper job ID regardless of
the Index value. Apply the same fix to any other similar functions in the
indicated line range that have the same pattern.

---

Outside diff comments:
In `@internal/core/run/ui/multi_remote.go`:
- Around line 499-501: The case statement for multiRunChildBootstrapMsg calls
handleChildBootstrap but returns nil, which prevents the spinner loop from
restarting after a child snapshot is installed. This leaves the queue frozen if
no spinner was previously scheduled. After calling handleChildBootstrap, ensure
the spinner loop is restarted by invoking the appropriate function to resume
spinner queue processing instead of directly returning m, nil.
- Around line 815-817: The placeholder child model created by
newPlaceholderChildModel() in the tab.child initialization block is missing
job-control callbacks, which prevents pause/message shortcuts from functioning.
After creating the placeholder child model (tab.child =
newPlaceholderChildModel(...)), wire the job-control callbacks from the
multi-run model (m) to the newly created child model. This ensures that when
child events arrive before a bootstrap snapshot, the placeholder still has the
necessary callbacks to call the daemon for pause and message operations.

---

Nitpick comments:
In `@internal/api/client/client_contract_test.go`:
- Around line 152-214: The tests across three files do not follow the required
subtest pattern. In internal/api/client/client_contract_test.go (lines 152-214),
wrap the entire body of the TestClientRunJobControlPostsPauseAndMessageRequests
function inside a t.Run("Should pause and message run jobs", ...) call, keeping
all assertions and logic inside the subtest closure. In
internal/store/rundb/run_db_test.go (lines 244-301), similarly wrap the test
function body inside a t.Run("Should ...", ...) subtest with an appropriate
descriptive name. In internal/core/run/ui/sidebar_screenshot_test.go (lines
21-45), wrap the screenshot assertion flow inside a t.Run("Should ...", ...)
subtest to align with the required test structure pattern. All three changes
follow the same pattern: move the test logic into a t.Run call while preserving
the test behavior.

In `@internal/core/model/job_control_test.go`:
- Around line 10-62: Refactor both test functions to use subtests with
t.Run("Should...") naming pattern. For
TestJobControlRegistryRoutesByJobIDAndIndexAlias, break the current inline
scenarios (Pause operation, SendMessage operation with index alias, and
unregister behavior) into separate named subtests. For
TestValidateJobControlMessageRejectsEmptyAndOversizedMessages, convert the
validation checks (empty message, oversized message, and valid trimmed message)
into a table-driven approach with subtests for each test case. Each t.Run
subtest should have a clear descriptive name following the "Should..." pattern
that explains what behavior is being tested.

In `@internal/core/run/ui/review_watch_remote_test.go`:
- Around line 56-68: The test function
TestReviewWatchModelAppliesWorkspaceRootToConfig needs to be wrapped with a
t.Run() subtest to follow the required test conventions. Move the test body
(after the t.Parallel() call) into a t.Run call with a descriptive name
following the "Should..." pattern to ensure compliance with the test structure
guidelines for *_test.go files.

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/run/ui/view_test.go`:
- Around line 1604-1614: The sgrParamsHaveForcedBackground function incorrectly
flags extended-color foreground sequences as having a background because it
checks for isolated tokens matching background values without considering
context. The issue is that in a sequence like "38;5;48", the value 48 is a color
index for foreground (indicated by the leading 38), not a background color
selector. Fix the function by tracking when you encounter a 38 (foreground) or
48 (background) token followed by 5 or 2, then properly skip over or interpret
the subsequent color parameter tokens based on that context. Return true only
when you encounter 48 with a mode (5 or 2), or basic background color codes
(40-47, 100-107) that stand alone. Additionally, add a test case in
TestHasForcedBackgroundSGR that verifies the string "\x1b[38;5;48mtext\x1b[0m"
returns false.

---

Outside diff comments:
In `@internal/core/run/ui/view.go`:
- Around line 126-136: The runChipStatus method in the switch statement has a
logic issue where the case m.isRunComplete() condition (line 133) can render
DONE status for non-success terminal outcomes like canceled or crashed runs when
failed equals zero, misreporting the actual final run state. Add explicit case
conditions to check for canceled and crashed terminal states before the generic
m.isRunComplete() case, ensuring these states return appropriate status
indicators and icons rather than falling through to the DONE status.

In `@internal/daemon/run_snapshot_test.go`:
- Around line 257-342: The test function
TestRunSnapshotBuilderTracksJobPauseAndResumeStates uses non-compliant naming in
its t.Run calls with generic names like "pausing", "paused", and "resumed"
instead of descriptive "Should..." names. Update the t.Run calls to use
descriptive "Should..." prefixed names that clearly describe the expected
behavior (for example, "Should track job pausing state", "Should track job
paused state", "Should track job resumed state"). Apply this change to all
subtest invocations in the test table cases shown in the diff, and also ensure
the same "Should..." pattern is consistently applied to any related test cases
in the line range 345-372 that the comment references.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/core/run/ui/update.go`:
- Around line 60-88: The dispatchSingleUIMsg function does not include a case
for runStatusMsg, so these messages fall through to the default case and are
dropped instead of being processed by applyUIMsg. Add a case statement for
runStatusMsg in the switch statement within dispatchSingleUIMsg that calls
m.applyUIMsg(v) and returns true, following the same pattern as the other
message type cases like jobQueuedMsg, jobStartedMsg, etc.