refactor: task artifacts changes - #52

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

Inline comments:
In `@internal/cli/exit.go`:
- Around line 8-10: The Error() method on commandExitError can panic if its
e.err field is nil; modify commandExitError.Error to defensively handle a nil
e.err (e.g., return an empty string or a default message) instead of directly
calling e.err.Error(), and/or update any factory like withExitCode to ensure
e.err is never nil; reference the commandExitError type and its Error() method
and adjust callers such as withExitCode to either pass a non-nil error or rely
on the new nil-safe behavior.

In `@internal/cli/root.go`:
- Around line 485-498: migrationFixPrompt currently builds the Issue message
with an empty type string, producing `type "" must be one of: ...`; update
migrationFixPrompt to include the actual unmapped type value instead of the
empty string by reading it from the migration result or file metadata (e.g.,
look up the type for the path in core.MigrationResult — e.g., a map or struct
like result.MigratedFiles[path].Type or a result.FileType map — or, if not
available, parse the file at the given path to extract the type) and use that
variable in the fmt.Sprintf call that sets tasks.Issue.Message (keep the rest of
the loop and the Report creation the same, only replace the literal "" with the
actual type value).

In `@internal/cli/validate_tasks.go`:
- Around line 110-121: The validator trims whitespace when checking s.format in
validateFormat() but doesn't persist that normalized value, causing run() to see
the original untrimmed string; update validateFormat() to normalize and store
the trimmed value (e.g., assign strings.TrimSpace(s.format) back to s.format)
before the switch so subsequent logic in run() and branches like
validateTasksFormatText / validateTasksFormatJSON operate on the normalized
format.
- Around line 211-224: resolveTaskWorkflowDir currently calls filepath.Abs on
resolvedTasksDir which resolves relative paths against the process CWD; instead,
if resolvedTasksDir is not absolute (use filepath.IsAbs), join it with
workspaceRoot (e.g. filepath.Join(workspaceRoot, resolvedTasksDir)) and then
call filepath.Abs on that combined path so relative --tasks-dir values are
resolved from the workspaceRoot; update references in resolveTaskWorkflowDir
(resolvedTasksDir, workspaceRoot, model.TaskDirectoryForWorkspace) and preserve
the existing error wrapping from fmt.Errorf("resolve tasks dir: %w", err).
- Around line 66-68: The run method on validateTasksCommandState currently uses
context.Background() when creating the cancelable context with
signal.NotifyContext; change it to start from cmd.Context() (with a fallback to
context.Background() if cmd.Context() is nil) so parent cancellations/deadlines
propagate, i.e., obtain the base context via cmd.Context(), then call
signal.NotifyContext(baseCtx, os.Interrupt, syscall.SIGTERM) and keep the
existing defer stop() logic.

In `@internal/core/plan/prepare.go`:
- Around line 143-145: The call to memory.Prepare currently returns the raw
error which loses task context; update the error return to wrap the underlying
error with task/workflow information using fmt.Errorf, e.g. wrap the err from
memory.Prepare(cfg.TasksDir, batchIssues[0].Name) with a message that includes
batchIssues[0].Name (or other identifying fields) before returning so callers
see "memory.Prepare for <task>: %w".

In `@internal/core/run/preflight_test.go`:
- Around line 183-203: The test TestRunValidationFormUsesInjectedInputAndOutput
mutates package-level variables validationFormInput and validationFormOutput
while calling runValidationForm, which can race with other parallel tests;
remove t.Parallel() from this test (delete the t.Parallel() call) so the test
runs serially, keep the t.Cleanup restore logic intact, and rerun tests to
ensure no races; alternatively, if you prefer concurrency safety instead, guard
access to validationFormInput/validationFormOutput with a sync.Mutex or refactor
runValidationForm to accept io.Reader/io.Writer via its config so tests can
inject per-test I/O without touching package-level state.

In `@internal/core/run/preflight.go`:
- Around line 16-19: The package-level mutable vars validationFormInput and
validationFormOutput are causing potential test races; add io.Reader and
io.Writer fields (e.g., ValidationInput io.Reader, ValidationOutput io.Writer)
to PreflightConfig, update runValidationForm to use cfg.ValidationInput and
cfg.ValidationOutput when non-nil (falling back to os.Stdin/os.Stdout), remove
or stop using the package-level vars, and update
TestRunValidationFormUsesInjectedInputAndOutput to inject the test buffers via
PreflightConfig instead of mutating the globals.

In `@internal/core/run/validation_form.go`:
- Around line 48-69: The Update method currently only handles quit keys and
doesn't support scrolling, so long reports run off-screen; add state to
validationFormModel (e.g., issueOffset int and issueViewportHeight int) and in
the WindowSizeMsg branch compute issueViewportHeight = max(0, m.height -
reservedLines) where reservedLines accounts for header/footer/help lines, then
clamp issueOffset; extend the KeyPressMsg handling to support scroll keys
("up"/"k" -> issueOffset--, "down"/"j" -> issueOffset++, "pgup" -> issueOffset
-= issueViewportHeight, "pgdown" -> issueOffset += issueViewportHeight,
optionally "home"/"end") and always clamp issueOffset between 0 and
len(m.issues)-issueViewportHeight; update the rendering code that lists issues
to slice by issueOffset..issueOffset+issueViewportHeight so the visible region
is bounded; keep existing behaviors around writeFixPrompt(), setting m.decision
(PreflightContinued/PreflightAborted) and quitting unchanged.

In `@internal/core/tasks/type_remap.go`:
- Around line 43-47: The loop over registry.Values() can panic if registry is
nil; add a nil check for registry before iterating in the fallback branch (the
code using registry.Values() and comparing to normalized), or short-circuit to
the legacyTypeRemap/return path when registry == nil; update the function that
performs the remap so the IsAllowed check and the subsequent for _, candidate :=
range registry.Values() are protected by the same registry != nil guard
(reference symbols: registry, Values(), normalized, legacyTypeRemap, IsAllowed).

---

Outside diff comments:
In `@internal/core/run/ui_view.go`:
- Around line 474-493: The transcript viewport height isn't reduced when the
optional spacer row for a non-empty taskTitle is added in renderTimelinePanel,
causing the panel to grow by one line and potentially clip the footer; update
renderTimelinePanel so that before calling m.transcriptViewport.SetHeight(...)
you compute the height budget as base := max(m.contentHeight-4,
logViewportMinHeight) and if job != nil && strings.TrimSpace(job.taskTitle) !=
"" then subtract 1 (e.g., base = max(base-1, logViewportMinHeight)); then call
m.transcriptViewport.SetHeight(base) and proceed with setting content and
restoring offsets so the extra spacer is accounted for without overflowing the
footer.

---

Nitpick comments:
In `@internal/core/migrate_test.go`:
- Around line 537-545: The helper mustMigrationRegistry duplicates setup logic
used by preflight tests; extract a shared test helper (e.g., NewTestRegistry or
testValidationRegistry) into a test utilities package and have tests call that
instead of duplicating code. Concretely, move the registry creation logic that
calls taskscore.NewRegistry(nil) and the t.Fatalf error handling into a new
helper function (name it NewTestRegistry or testValidationRegistry), update
callers (tests referencing mustMigrationRegistry and the preflight tests) to
import and call the shared helper, and adjust imports to use the new test
utilities package so duplication is removed while preserving the same behavior.
- Around line 48-57: Remove the redundant loop-variable capture lines (`tt :=
tt`) inside the test loops; update the table-driven tests that call
t.Run(tt.name, ...) and use tt.body / tt.want (and the second occurrence at the
other test block around line 111) to rely on Go 1.22+ loop semantics, simply
iterating `for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... })
}` while keeping calls to taskscore.ExtractTaskBodyTitle(tt.body) and assertions
unchanged.

In `@internal/core/model/model_test.go`:
- Around line 212-252: Replace the runtime reflection checks in
TestTaskMetadataUsesV2Fields with compile-time field accesses: remove the
reflect.Type loops and instead directly reference the required fields on
model.TaskEntry and model.TaskFileMeta (e.g., access .Title, .Status, .TaskType,
.Complexity, .Dependencies) so compilation will fail if fields are removed; also
ensure the forbidden fields are not referenced and rename each t.Run subtest
name to follow the "Should..." pattern (e.g., "Should include Title and drop
Domain/Scope") to comply with test naming guidelines.

In `@internal/core/plan/input.go`:
- Around line 308-313: Duplicate wrapTaskParseError implementations should be
consolidated into a single shared helper: create a new helper (e.g., package
internal/core/helpers) and move the logic into an exported function
WrapTaskParseError(path string, err error) error that preserves the same
behavior (checking errors.Is against prompt.ErrLegacyTaskMetadata and
prompt.ErrV1TaskMetadata and returning the same formatted messages). Replace the
wrapTaskParseError definitions in both places with calls to
helpers.WrapTaskParseError and update imports accordingly (remove the local
function and import the helper package in files that used it, e.g., the previous
internal/core/plan and internal/core/tasks usages).

In `@internal/core/prompt/prompt_test.go`:
- Around line 505-535: TestTaskMetadataHelpers mixes a table-driven subtest for
IsTaskCompleted with two inline assertions for ExtractTaskNumber and
SafeFileName, which reduces isolation; move those inline checks into their own
subtests or separate test functions (e.g., add t.Run("ExtractTaskNumber", ...)
and t.Run("SafeFileName", ...) inside TestTaskMetadataHelpers or create
TestExtractTaskNumber and TestSafeFileName) and call t.Parallel() inside each
new subtest/test so failures are isolated and parallelism is preserved;
reference IsTaskCompleted, ExtractTaskNumber, SafeFileName and the existing
TestTaskMetadataHelpers when making the change.

In `@internal/core/tasks/fix_prompt_test.go`:
- Around line 52-54: The helper filepathJoin currently joins parts with a
hardcoded "/" which is non-idiomatic; update the filepathJoin function to
delegate to path/filepath's filepath.Join (i.e., return filepath.Join(parts...))
and add the import for "path/filepath" (remove or keep "strings" if still used
elsewhere) so tests use the OS-native separator via the filepath.Join function.

In `@internal/core/tasks/type_remap_test.go`:
- Around line 5-50: Add a new test function
TestRemapLegacyTaskTypeWithNilRegistry that calls RemapLegacyTaskType with a nil
registry and asserts it returns the empty string; implement it as a parallel
table-driven test (use t.Parallel, table of cases with rawType "Refactor" and
"backend" and want ""), run each case via t.Run and compare got :=
RemapLegacyTaskType(tt.rawType, nil) to tt.want and fail with a clear message on
mismatch so the nil-registry behavior is covered and prevents the nil pointer
regression.

In `@internal/core/tasks/types.go`:
- Line 22: The regexp variable taskTypeSlugPattern enforces a 2–32 character
slug (one leading lowercase letter plus 1–31 of [a-z0-9_-]) which rejects
single-character types; add a brief inline comment above taskTypeSlugPattern
describing this minimum 2-character requirement and the intent (e.g., "must
start with a lowercase letter, total length 2–32, single-character slugs like
'a' are invalid") so readers understand the design choice and the regex
components.

In `@internal/core/tasks/validate.go`:
- Around line 46-48: The current pre-check in validate tasks uses
context.Cause(ctx) which is intended for extracting cancellation causes; replace
that call with the idiomatic ctx.Err() check in the same location (the block
that currently calls context.Cause(ctx) and returns fmt.Errorf("validate tasks:
%w", err)) so the code simply checks if ctx.Err() != nil and returns the wrapped
error; update the conditional around context.Cause(ctx) to use ctx.Err() while
keeping the existing error wrapping and return behavior.

In `@internal/core/workspace/config_test.go`:
- Around line 465-475: The file defines a custom helper equalStrings that
duplicates standard library functionality; replace all uses of equalStrings with
slices.Equal from the standard library and remove the equalStrings function
definition; also add the "slices" import to this test file (or reuse the
existing slices import if present in related tests like types_test.go) so
comparisons use slices.Equal(got, want) instead of the custom loop.