refactor: improve packages - #70

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

Inline comments:
In `@internal/cli/run.go`:
- Around line 95-103: Replace the ignored return values from the fmt.Fprintf
call that writes the summary (the call using cmd.OutOrStdout() with
result.Total/result.Provider/result.PR/result.ReviewsDir/result.Round) with
proper error handling: capture the returned n, err := fmt.Fprintf(...), check if
err != nil and return or wrap that error (or log and return) so the function
does not silently succeed when stdout is closed or the write fails.

In `@internal/cli/state.go`:
- Around line 379-423: The logic currently omits non-empty stdin from
sourceCount, allowing two prompt sources; fix by calling
readPromptFromCommandInput(cmd.InOrStdin()) before computing/validating
sourceCount, store stdinPrompt and hasStdinPrompt, increment sourceCount if
hasStdinPrompt, then perform the existing exclusivity check and use the stored
stdinPrompt/hasStdinPrompt in the switch; ensure you don't re-read stdin later
(use the saved stdinPrompt and set s.readPromptStdin = true /
s.resolvedPromptText = stdinPrompt when selecting the stdin case).

In `@internal/contentblock/engine.go`:
- Around line 51-60: MarshalEnvelopeJSON currently returns any non-empty data
verbatim without validating it's well-formed JSON or that an embedded "type"
matches blockType; update MarshalEnvelopeJSON(blockType, data) to first ensure
json.Valid(data) (return an error if not), then unmarshal data into a small
container (e.g. map[string]json.RawMessage or struct with a Type field) and, if
a "type" field is present, verify it equals blockType (return an error if it
disagrees); if validation passes, return the original data unchanged and include
clear contextual error messages mentioning blockType.

In `@internal/core/agent/registry_launch.go`:
- Around line 73-79: EnsureAvailable currently calls resolveLaunchCommand with
spec.DefaultModel, ignoring any user-supplied cfg.Model; change it to use the
effective model selection logic (use cfg.Model when set, otherwise fall back to
spec.DefaultModel) and pass that effective model into resolveLaunchCommand so
availability probing uses the same model choice as runtime. Update the call in
EnsureAvailable to compute effectiveModel := cfg.Model; if effectiveModel == ""
then effectiveModel = spec.DefaultModel, then call resolveLaunchCommand(spec,
effectiveModel, cfg.ReasoningEffort, cfg.AddDirs, cfg.AccessMode, true).
- Around line 306-324: joinAvailabilityErrors currently flattens multiple errors
into a single string losing typed AvailabilityError details; update the function
(joinAvailabilityErrors) to preserve error types by returning a chained/joined
error instead of strings.Join — e.g., when len(errs) > 1 return
errors.Join(errs...) (or wrap with fmt.Errorf("%w", err) per project style) so
callers can use errors.As to recover AvailabilityError fields; keep the existing
zero-case that returns an AvailabilityError constructed from Spec and the
single-error case returning errs[0].
- Around line 180-186: formatShellArg currently uses strconv.Quote, which
produces Go string literals but allows shell expansions; replace that with
shell-safe quoting: implement a shell quoting routine (used by formatShellArg)
that wraps the arg in single quotes and escapes any internal single quotes by
replacing ' with '\'' (i.e., close single quote, insert escaped single quote,
reopen single quote) so the resulting string is safe to paste into a POSIX shell
and will not undergo $ or ` expansion; update formatShellArg to call this
shell-safe function whenever strings.ContainsAny detects characters requiring
quoting.

In `@internal/core/agent/registry_specs.go`:
- Around line 55-60: Update the RuntimeRegistry interface so EnsureAvailable
accepts context.Context as its first parameter (change signature to
EnsureAvailable(ctx context.Context, cfg *model.RuntimeConfig) error), then
update all concrete implementations of RuntimeRegistry to match that signature
and propagate the incoming ctx through their external process calls; finally,
replace any uses that call EnsureAvailable (notably verifyLauncher) to pass the
caller's ctx instead of using context.Background(), ensuring context is threaded
through to allow cancellation/timeouts.

In `@internal/core/agent/registry_validate.go`:
- Around line 145-156: The runtimePromptSourceCount function currently counts
PromptText, PromptFile, and ReadPromptStdin but omits ResolvedPromptText,
causing valid requests normalized into ResolvedPromptText to fail validation;
update runtimePromptSourceCount(cfg *model.RuntimeConfig) to also check
strings.TrimSpace(cfg.ResolvedPromptText) != "" and increment sources when
present (treat it as a fallback prompt source), so the function aligns with
resolveExecPromptText which prefers ResolvedPromptText.
- Around line 12-18: ValidateRuntimeConfig currently dereferences cfg (cfg.Mode)
and will panic when called with nil; add an explicit nil check at the start of
ValidateRuntimeConfig to return a descriptive error (e.g., "runtime config is
nil") instead of panicking, then proceed to call validateRuntimeMode and
lookupAgentSpec as before; reference the ValidateRuntimeConfig function and
ensure any callers still get the same semantic errors from
validateRuntimeMode/lookupAgentSpec when cfg is non-nil.

In `@internal/core/agent/tool_call_name.go`:
- Around line 161-171: The condition inside the ref_id branch redundantly tests
refID != "" again (refID was already checked), causing the OpenURL branch to
always fire when pattern is empty; in the block starting with refID :=
extractString(input, "ref_id", "refId") check, change the final if to only test
presence of "url" (i.e., if extractString(input, "url") != "" ) before returning
"OpenURL", leaving the Click and toolNameFind checks intact so OpenURL is only
returned when a url key exists.

In `@internal/core/migration/migrate.go`:
- Around line 166-194: The three functions appendTaskMigration,
appendReviewMigration, and recordRoundMeta currently swallow all errors from
inspectTaskArtifact/inspectReviewArtifact/inspectRoundMeta by calling
s.recordInvalid(path) and returning nil; instead, preserve and return the
original errors wrapped with context (e.g., fmt.Errorf("inspectTaskArtifact %s:
%w", path, err)) so I/O/permission/parse failures remain visible to callers.
Concretely: in appendTaskMigration and appendReviewMigration, on error do not
just call s.recordInvalid(path) and return nil — return a wrapped error; if you
have a specific sentinel error for “invalid artifact” (e.g.,
ErrInvalidArtifact), you may still call s.recordInvalid(path) for that case but
still return the wrapped sentinel error. Apply the same change to
recordRoundMeta (wrap and return inspectRoundMeta errors instead of silently
recording invalid).
- Around line 81-88: The write loop over pending artifacts currently always
writes every file; change it to honor the migration context by checking
ctx.Err() or using a select on ctx.Done() before each os.WriteFile call and
aborting early (returning ctx.Err() or a wrapped context.Canceled error) so no
further files are mutated after cancellation; update the loop that iterates over
pending (the slice sorted with sort.Slice) to perform this context check before
attempting each write and avoid proceeding to os.WriteFile when the context is
cancelled.

In `@internal/core/migration/workflow_target.go`:
- Around line 30-31: Reject workflow names that can escape the workflow root by
validating cfg.name before joining it to rootDir: ensure cfg.name is non-empty
and does not contain path separators ('/' or '\\'), dot-segments ('.' or '..'),
or an absolute path prefix; if any of these are present return an error (fail
fast). Update the check around strings.TrimSpace(cfg.name) != "" (and the
similar check at the other occurrence around lines 61-63) to perform this
validation and use the same validation helper wherever cfg.name is used to form
a path to prevent directory traversal/escape.

In `@internal/core/model/artifacts.go`:
- Around line 74-79: The JobArtifacts helper trusts the caller's safeName and
can be tricked into creating paths outside the intended jobs directory; update
RunArtifacts.JobArtifacts to sanitize/normalize safeName (same approach used in
NewRunArtifacts) before joining: reject empty or names containing path
separators or "..", or else replace with filepath.Base / strings.ReplaceAll to
strip directory components and disallow traversal, and then use that sanitized
name to build PromptPath, OutLogPath and ErrLogPath so artifact writes remain
confined to the flat jobs namespace.

In `@internal/core/model/content_test.go`:
- Around line 217-248: Wrap the existing test body of
TestContentBlockMarshalUsesCamelCaseJSONTags into a t.Run subtest with a
"Should..." name (e.g., t.Run("Should use camelCase JSON tags", func(t
*testing.T) { ... })), moving t.Parallel() into the subtest if parallelism is
desired, and keep all existing setup and assertions (NewContentBlock,
json.Marshal, checks against encoded for required/forbidden fields) unchanged
inside that subtest so the test follows the repository's t.Run naming pattern.

In `@internal/core/model/preparation.go`:
- Around line 35-53: SetJournal currently overwrites an existing JournalHandle
without releasing it and CloseJournal clears JournalHandle before Close(ctx)
completes; update SetJournal and CloseJournal on SolvePreparation to protect
ownership: in SetJournal, if p.JournalHandle != nil then refuse to replace
(no-op) or return without dropping the old owner (i.e., do not call
journal.NewOwner when an owner already exists) to avoid orphaning the previous
handle; in CloseJournal, capture the current handle, call handle.Close(ctx)
first and only set p.JournalHandle = nil after Close returns successfully
(propagate/return the error if Close fails) so the previous owner remains
reachable on failure; reference methods: SetJournal, CloseJournal,
JournalHandle, journal.NewOwner, and handle.Close.

In `@internal/core/prompt/prompt_test.go`:
- Around line 288-294: Update TestSafeFileName to a table-driven subtest using
t.Run and verify both the sanitized prefix and the hash suffix behavior of
SafeFileName: for each case assert the returned string starts with the expected
sanitized prefix and then either (a) verify the suffix matches the expected
6-character alphanumeric fingerprint shape (e.g., regex [A-Za-z0-9]{6} after a
hyphen) or (b) include a pair of inputs that sanitize to the same base and
assert SafeFileName yields different full names for them (ensuring a
collision-resistant suffix); reference the SafeFileName function and the
TestSafeFileName test name when updating the assertions.

In `@internal/core/reviews/parser_test.go`:
- Around line 9-96: The tests in TestReviewParsingHelpers,
TestExtractIssueNumber, and TestWrapParseErrorProvidesMigrationGuidance must be
refactored into table-driven subtests using t.Run("Should ...") so each scenario
is isolated; create a slice of test cases (with name, input, expected) for the
ParseReviewContext/IsReviewResolved/legacy parse/extract flows and for
ExtractIssueNumber and WrapParseError, iterate over cases and call t.Run("Should
<behavior>", func(t *testing.T){ t.Parallel(); ... assertions ... }), moving
each assertion into its matching subtest and invoking the existing helper
functions (ParseReviewContext, IsReviewResolved, ParseLegacyReviewContext,
ExtractLegacyReviewBody, LooksLikeLegacyReviewFile, ExtractIssueNumber,
WrapParseError) inside the subtests; ensure error expectations use errors.Is
where appropriate and keep test behaviour identical while adopting the required
"Should..." subtest naming and table-driven pattern.

In `@internal/core/reviews/parser.go`:
- Around line 144-156: The extractXMLTag function currently searches the whole
file and must be limited to the <review_context> block: first locate the
"<review_context>" open and "</review_context>" close (return "" if either is
missing), slice content to just that inner block, then perform the existing tag
extraction logic on that slice; update references to extractXMLTag to continue
accepting the full file string (the function itself will do the scoping), and
keep the function name extractXMLTag and tag parameter unchanged.

---

Outside diff comments:
In `@internal/core/plan/journal.go`:
- Around line 19-21: Replace the use of context.Background() fallback with
context.TODO() in the journal close path: where the variable closeCtx is being
defaulted (the block containing "if closeCtx == nil { closeCtx =
context.Background() }"), change it to use context.TODO() so the code signals an
incomplete context setup; update the assignment referencing closeCtx in the
journal.go function/method that performs the close logic accordingly.

---

Nitpick comments:
In `@cmd/compozy/main.go`:
- Around line 61-66: waitForUpdateResult currently blocks on <-result and can
hang the CLI if CheckForUpdate is slow; change waitForUpdateResult to use a
select that waits on the result channel and a timeout (e.g., time.After with a
short duration) and return nil on timeout so the CLI can exit promptly;
reference the result channel returned by startUpdateCheck/CheckForUpdate and
ensure you import time and handle the nil result case as before.

In `@internal/cli/kernel_dispatch_test.go`:
- Around line 248-249: Remove the redundant per-iteration capture assignment by
deleting the inner "tt := tt" in the test loop that iterates over "tests" (the
loop variable "tt" is already correctly scoped in Go 1.26.1), so simply iterate
with "for _, tt := range tests { ... }" and update any closures to reference
"tt" directly without the extra reassignment.

In `@internal/cli/workspace_config_test.go`:
- Around line 234-308: The two monolithic tests
TestApplyConfigHandlesSupportedTypes and TestApplyInputHandlesSupportedTypes
should be refactored into table-driven subtests using t.Run("Should ...") so
each coercion/parsing branch is exercised and reported independently; for each
test create a slice of cases (each with a name like "Should apply string
config", target flag/key, input value, helper function reference such as
applyConfig/applyInput, and expected result) and iterate calling
t.Run(case.name, func(t *testing.T){ t.Parallel(); ... assert expectations }),
keeping use of helpers passThroughInput, parseIntInput, parseStringSliceInput
and the clone function for slices, and preserve current assertions but split
them into per-case checks so failures identify the exact contract that
regressed.

In `@internal/core/agent/acp_convert.go`:
- Around line 436-449: marshalRawJSON currently swallows json.Marshal errors and
returns nil; change its signature to return (json.RawMessage, error) and
propagate the marshal error instead of returning nil, updating callers to handle
the error (or alternatively log the error where appropriate); specifically
modify function marshalRawJSON to return both payload and error and update any
call sites that use marshalRawJSON to check and handle the returned error.

In `@internal/core/agent/tool_call_name.go`:
- Around line 134-143: Remove the duplicate trimming logic by deleting the
acp.ToolKindOther case block inside the switch in the GetToolCallName (or
equivalent) function so that the post-switch fallback that trims and returns
title handles that kind; ensure you leave the final post-switch check that uses
strings.TrimSpace(title) and returns trimmed or "" so other cases still
short-circuit as intended and no duplicate trimmed variable declarations remain.
- Around line 91-93: The conditional combining token and input checks is correct
but unclear; update the if in tool_call_name.go to use explicit parentheses
around the alternatives—e.g., parenthesize (token == "update_todo") and (input
!= nil && input["todos"] != nil) when returning toolNameTodoWrite—so the intent
is immediately obvious when reading the token, input, and toolNameTodoWrite
logic.
- Around line 227-236: The cloneMap function currently does a shallow copy and
will share nested map/slice references; either document this behavior or change
cloneMap to perform a deep copy by recursively handling nested types: when
iterating in cloneMap, if value is map[string]any call cloneMap(value)
recursively, if value is []any make a new slice and deep-clone each element
(recursing for nested maps/slices), and for other types copy as-is; update
function name/signature or add helper deepCloneValue/deepCloneMap helpers as
needed to keep code clear.
- Around line 28-60: The map commonToolTitleAliases mixes string literals and
existing constants (e.g., toolNameFind); define constants for the repeated tool
title strings (e.g., Edit, Write, Glob, Grep, Read, Task, etc.), replace the
string literals in commonToolTitleAliases with those new constants, and update
any other places that use the same literal values to use the constants for
consistency; keep existing constant names like toolNameFind unchanged and ensure
the new constants are declared near other toolName* constants for
discoverability.

In `@internal/core/kernel/core_adapters_test.go`:
- Around line 98-221: The four near-duplicate tests
(TestDispatchFetchReviewsAdapterBuildsReviewsFetchCommand,
TestDispatchMigrateAdapterBuildsWorkspaceMigrateCommand,
TestDispatchSyncAdapterBuildsWorkflowSyncCommand,
TestDispatchArchiveAdapterBuildsWorkflowArchiveCommand) should be merged into a
single table-driven test that iterates cases and uses t.Run("Should ...")
subtests; create a cases slice where each entry specifies the adapter function
to call (dispatchFetchReviewsAdapter, dispatchMigrateAdapter,
dispatchSyncAdapter, dispatchArchiveAdapter), the input config, the expected
result assertions, and the capture handler type/result to register, then inside
the loop set up NewDispatcher(), Register(dispatcher, handler), stub
coreAdapterDispatcherFn to return the dispatcher, call the adapter function, and
assert result shape and handler.got fields; keep using t.Cleanup to restore
coreAdapterDispatcherFn and preserve the same assertions per-case but eliminated
boilerplate by centralizing common setup and teardown.

In `@internal/core/kernel/deps_test.go`:
- Around line 580-581: Remove the redundant loop-variable shadowing line (`tt :=
tt`) inside the for loop over `tests`; simply iterate `for _, tt := range tests
{` and use `tt` directly (or pass `tt` into the subtest closure via
`t.Run(tt.name, func(t *testing.T) { tc := tt; ... })` if you prefer explicit
capture), so eliminate the unnecessary `tt := tt` statement in the test loop in
deps_test.go.

In `@internal/core/model/constants.go`:
- Around line 15-22: The constants DefaultCodexModel, DefaultClaudeModel,
DefaultCursorModel, DefaultOpenCodeModel, DefaultPiModel, DefaultGeminiModel,
DefaultCopilotModel and DefaultActivityTimeout are hardcoded runtime
configuration and should be moved out of package constants; instead remove these
values from internal/core/model/constants.go and load them from a TOML config or
provide them via functional options/constructor parameters (e.g., pass defaults
into the model/provider constructors or a Config struct) so provider
renames/deprecations and timeouts can be changed without code edits; update any
usages of these symbols to read from the injected Config or options and add
sensible fallback logic where the config is missing.

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

Inline comments:
In `@internal/contentblock/engine.go`:
- Around line 86-87: The code calls validate(envelope.Type, data) without
checking if validate is nil and returns the validator error without context; add
a nil-check for the validate function pointer and return a clear wrapped error
if it's missing (e.g., return Envelope[T]{}, fmt.Errorf("no validator for type
%s", envelope.Type)), and when validate returns an error wrap it with context
including envelope.Type using fmt.Errorf("validate %s: %w", envelope.Type, err);
ensure fmt is imported and update the call site around validate(...) and the
Envelope[T] return accordingly.
- Around line 100-111: The code calls the blockType callback without checking
for nil (unlike normalize); add a nil guard before invoking blockType in the
decode function: verify blockType != nil and return a descriptive error (e.g.,
"decode <expected> block: missing blockType callback") if it's nil, then proceed
to call blockType(block) and compare to expected; reference the blockType and
normalize parameters and the existing error-return pattern used after
json.Unmarshal to keep behavior consistent.

In `@internal/core/agent/registry_launch.go`:
- Around line 63-87: EnsureAvailable may dereference a nil cfg (cfg.DryRun)
before the ctx nil-check; add an explicit nil check for cfg at the start of
EnsureAvailable (e.g., if cfg == nil { return fmt.Errorf("nil RuntimeConfig") })
and return a clear error, preserving the existing ctx nil-default logic and
subsequent calls to lookupAgentSpec and resolveLaunchCommand; reference
EnsureAvailable and cfg.DryRun when making the change.

In `@internal/core/migration/workflow_target_test.go`:
- Around line 39-40: Replace string-based error checks by introducing a sentinel
error (e.g., ErrInvalidWorkflowName) in the package, have normalizeWorkflowName
wrap/return that sentinel (use fmt.Errorf("%w: %s", ErrInvalidWorkflowName,
details) or similar) so callers can detect the failure, and update the test in
workflow_target_test.go to assert with errors.Is(err, ErrInvalidWorkflowName)
instead of strings.Contains(err.Error(), ...); apply the same
replace-for-string-match change in the other tests noted in migrate_test.go that
check the same validation.

In `@internal/core/model/preparation_test.go`:
- Around line 13-62: Split the two top-level tests
TestSolvePreparationSetJournalPreservesExistingOwnership and
TestSolvePreparationCloseJournalPreservesHandleOnFailure into t.Run subtests
(preferably table-driven): create a table of cases describing the scenario and
expected outcome and for each case call t.Run("Should <description>", func(t
*testing.T){ ... }); move the existing setup/assert logic into the subtest body
and preserve use of SolvePreparation.SetJournal, SolvePreparation.Journal(),
CloseJournal, and the stubJournalHandle (err) to drive the failure case; ensure
each subtest calls t.Parallel() where appropriate and that the original
assertions remain unchanged.
- Around line 55-58: The test currently only checks err != nil for
prep.CloseJournal; instead define or reuse a sentinel error (e.g.,
ErrCloseJournalFailed) that the CloseJournal implementation returns on the
failure path, then update the test to assert the exact error using
errors.Is(err, ErrCloseJournalFailed) (or testing helper
require.ErrorIs/Assert.ErrorIs) against prep.CloseJournal; ensure the sentinel
is exported/visible to the test or placed in a shared test-only package and that
CloseJournal returns that sentinel in the failure scenario.

---

Nitpick comments:
In `@internal/cli/root_test.go`:
- Around line 1619-1628: The failOnWriteWriter.Write fallback returns
io.ErrClosedPipe when w.err is nil, which can mask real underlying errors;
modify the Write method on the failOnWriteWriter type to return a more
informative default by returning the underlying w.err when present and otherwise
return a distinct sentinel (e.g., nil or a new explicit error) that clearly
signals "no underlying error set" so callers/tests won't be misled—update the
Write method on failOnWriteWriter (referenced by type name failOnWriteWriter and
its Write method) to return w.err when non-nil and a clearer fallback error when
nil.

In `@internal/cli/run.go`:
- Around line 13-67: Extract the shared setup flow from run and exec into a
helper (e.g., prepareAndRun) that performs signalCommandContext,
applyWorkspaceDefaults, buildConfig, applyPersistedExecConfig, cfg.Validate and
runPrepared, and accept a small setupFn parameter so callers can inject the
differing step (call maybeCollectInteractiveParams for run and
resolveExecPromptSource for exec); ensure the helper uses the same
error-wrapping/handling via handleExecError and preserves the special
applyWorkspaceDefaults error message formatting used in exec, then replace the
bodies of run and exec to call this new prepareAndRun helper.

In `@internal/cli/state.go`:
- Around line 107-134: The method withFallbacks currently mutates its value
receiver (commandStateDefaults) and returns it, which is confusing; either
convert it to an in-place pointer receiver or make it purely functional. To fix:
choose one approach — (A) change the receiver to *commandStateDefaults
(withFallbacks) and remove the return value so the method mutates the struct in
place, updating all call sites to rely on mutation; or (B) keep a value receiver
but avoid mutating the parameter by creating a new local variable (e.g., result
:= defaults), set fallbacks on result using defaultCommandStateDefaults(), and
return result; then keep existing call sites that expect a returned value.
Ensure you update all references to withFallbacks accordingly.
- Around line 225-249: The nil checks for isInteractive and collectForm in
maybeCollectInteractiveParams are redundant because newCommandStateWithDefaults
calls withFallbacks() which guarantees those callbacks are set; remove the
conditional assignments and directly call s.isInteractive() and
s.collectForm(cmd, s) (or, if you prefer to keep the defensive pattern, add a
short comment above the checks referencing newCommandStateWithDefaults and
withFallbacks to explain they are only defensive for non-standard construction).
Update maybeCollectInteractiveParams to use the guaranteed callbacks
(isInteractive and collectForm) and simplify the error/flow accordingly,
referencing the methods/fields: maybeCollectInteractiveParams, isInteractive,
collectForm, newCommandStateWithDefaults, and withFallbacks.

In `@internal/core/agent/client.go`:
- Around line 537-545: The call to resolveLaunchCommand uses
context.Background() which prevents caller cancellation; update ensureStarted to
pass its incoming ctx through to resolveLaunchCommand (replace
context.Background() with the ensureStarted ctx) so resolution honors
timeouts/cancellation, referencing resolveLaunchCommand and ensureStarted and
the existing parameters (c.spec, startModel, c.cfg.ReasoningEffort,
c.cfg.AddDirs, c.cfg.AccessMode); if the behavior of decoupling is intentional,
instead add a brief comment above the call explaining why a detached context is
required and the decision rationale.

In `@internal/core/model/preparation.go`:
- Around line 53-54: The current return of the raw error from handle.Close(ctx)
should be wrapped with operation context; change the error return in the
function that calls handle.Close(ctx) to return fmt.Errorf("closing handle: %w",
err) (or another clear contextual string) and add a fmt import if missing so the
caller receives a wrapped error with call-site context.
- Around line 9-12: Add a compile-time interface conformance check to ensure the
concrete type produced by journal.NewOwner implements the JournalHandle
interface used by SetJournal: add a package-level var declaration asserting
JournalHandle is satisfied by the concrete owner type returned from
journal.NewOwner (use the concrete type name from the journal package, e.g. the
Owner type) so future interface drift causes a compile error.

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

Inline comments:
In `@internal/core/agent/registry_test.go`:
- Around line 587-695: These tests violate the project's t.Run("Should...")
subtest pattern—refactor each standalone test
(TestValidateRuntimeConfigRejectsNilConfig, TestEnsureAvailableRejectsNilConfig,
TestValidateRuntimeConfigAcceptsResolvedPromptTextAsExecPromptSource,
TestBuildShellCommandStringShellEscapesInterpolatedArguments,
TestEnsureAvailableHonorsCallerContext,
TestJoinAvailabilityErrorsPreservesAvailabilityErrorTypes) to use the required
subtest pattern: either convert them into a single table-driven test with
multiple t.Run("Should ...", func(t *testing.T){ ... }) entries or wrap the
existing body inside a t.Run("Should <behavior>", func(t *testing.T){ ... })
call; keep the existing assertions and setup (e.g., calls to
ValidateRuntimeConfig, EnsureAvailable, BuildShellCommandString,
joinAvailabilityErrors, registerTestSpec) unchanged but move them into the
corresponding subtest functions and preserve t.Parallel where appropriate inside
each subtest.
- Around line 590-603: Add a package-level sentinel error (e.g., var
ErrRuntimeConfigNil = errors.New("runtime config is nil")) and replace the
string-based error checks and string-returned errors with that sentinel: change
the error returns in the functions that currently return the "runtime config is
nil" message (refer to the error returns in registry_validate.go and
registry_launch.go) to return ErrRuntimeConfigNil, and update the tests that
call ValidateRuntimeConfig and EnsureAvailable to use errors.Is(err,
ErrRuntimeConfigNil) (or errors.As where appropriate) instead of
strings.Contains(err.Error(), ...); ensure imports include "errors" where added.

---

Nitpick comments:
In @.gitignore:
- Line 16: Change the .gitignore entry "ai-docs" to a directory-specific pattern
"ai-docs/" so the rule only ignores the ai-docs directory (not a file named
ai-docs); update the entry referenced as "ai-docs" in the .gitignore
accordingly.

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

Inline comments:
In `@cmd/compozy/main.go`:
- Around line 31-33: The code currently discards the error from fmt.Fprintln
when writing the update notification; change it to capture and handle the error
from fmt.Fprintln(cmd.ErrOrStderr(), renderUpdateNotification(version.Version,
release)) (called after waitForUpdateResult and renderUpdateNotification) —
assign the return error to a variable, check if err != nil, and then handle it
(for example, write a descriptive error message to stderr via cmd.ErrOrStderr()
or propagate/return the error from main), optionally ignoring benign
broken-pipe/EPIPE errors if appropriate.

In `@internal/cli/state.go`:
- Around line 255-261: The code currently accepts zero or negative durations
from s.timeout because time.ParseDuration allows them; update the parsing logic
in the function that builds the core.Config (where s.timeout is parsed into
timeoutDuration) to validate parsed > 0 and return an error when parsed <= 0,
e.g., after calling time.ParseDuration(s.timeout) check the parsed value and
return fmt.Errorf("invalid timeout: must be > 0") (or similar) rather than using
a non-positive timeoutDuration; keep the variable names timeoutDuration,
s.timeout and the time.ParseDuration call so the change is localized.
- Around line 226-252: The interactivity gate in maybeCollectInteractiveParams
blocks the exec command when a positional prompt exists because
cmd.Flags().NFlag() only counts flags; update prepareAndRun/exec flow so
resolveExecPromptSource is called (or positional prompt presence is checked)
before calling maybeCollectInteractiveParams, or alter
maybeCollectInteractiveParams to skip the interactive-terminal check when
resolveExecPromptSource (or a positional prompt) indicates a provided prompt;
specifically locate prepareAndRun and the exec path that calls
resolveExecPromptSource and reorder or add a conditional that if
resolveExecPromptSource returns a valid prompt (or if positional args exist)
then do not enforce the isInteractive() check that uses cmd.Flags().NFlag().
- Around line 371-423: The method resolveExecPromptSource leaves s.promptFile
from a prior run which can make the exclusivity check or behavior incorrect;
ensure s.promptFile is cleared unless the current chosen source is the
prompt-file. Specifically, after computing local promptFile :=
strings.TrimSpace(s.promptFile) (and before the sourceCount logic) or
immediately when taking the positionalPrompt or stdin branch, set s.promptFile =
"" so stale state is removed; when the prompt-file branch wins keep the existing
assignment s.promptFile = promptFile as currently done. This guarantees
s.promptFile only reflects the active source and prevents false positives in the
exclusivity check.

---

Duplicate comments:
In `@internal/core/agent/tool_call_name.go`:
- Around line 169-179: The branch in the tool name resolver incorrectly uses
"extractString(input, \"url\") != \"\" || refID != \"\"" which is redundant
because refID is already known non-empty (set by the outer if), causing
toolNameOpenURL to be returned unconditionally; update the final conditional in
the block that references refID (the one after extractInt and
extractString("pattern")) to check only for a non-empty URL (i.e., use
extractString(input, "url") != "") so OpenURL is returned only when a url key is
present; keep the existing extractInt, extractString("pattern") checks and the
outer refID check intact and ensure you reference extractString, extractInt,
refID, toolNameClick, toolNameFind, and toolNameOpenURL when making the change.

---

Nitpick comments:
In `@cmd/compozy/main_test.go`:
- Around line 10-56: Refactor TestWaitForUpdateResult into a table-driven test
that iterates over cases describing the channel setup and expected result; keep
using subtests via t.Run for each case, and exercise waitForUpdateResult with
inputs such as (1) a buffered result channel pre-filled with
&update.ReleaseInfo{Version:"v1.2.3"} expecting that pointer, (2) a closed
unbuffered channel expecting nil, and (3) a never-sent channel with a goroutine
reading waitForUpdateResult and a timeout using updateResultWaitTimeout
expecting nil; preserve parallelization (t.Parallel()) appropriately and
reference TestWaitForUpdateResult, waitForUpdateResult, update.ReleaseInfo and
updateResultWaitTimeout when locating the code to change.

In `@cmd/compozy/main.go`:
- Around line 41-61: startUpdateCheck currently creates its own background
context and spawns a fire-and-forget goroutine; change its signature to accept a
caller-owned context (e.g., startUpdateCheck(ctx context.Context, currentVersion
string) ...) and create a derived ctx via context.WithCancel(ctx), pass that ctx
into update.CheckForUpdate, and return the cancel function. Also add an explicit
completion signal (e.g., return a done channel or WaitGroup notification) that
the goroutine closes/signals when it finishes so callers can wait for shutdown;
keep usage of update.StateFilePath and update.CheckForUpdate unchanged except
for using the provided ctx and ensure the goroutine closes the result channel
and the done signal on all exit paths.

In `@internal/cli/workspace_config_test.go`:
- Around line 426-432: The subtest names in the cases table (variable cases
containing entries with name and base referencing
migrateState.simpleCommandBase, syncState.simpleCommandBase,
archiveState.simpleCommandBase) must be renamed to the "Should..." form; update
each name string (e.g., change "migrate", "sync", "archive") to descriptive
"Should ..." phrases (e.g., "Should migrate workspace", "Should sync workspace",
"Should archive workspace") so that t.Run uses the required t.Run("Should...")
pattern for all entries using the cases slice and simpleCommandBase references.

In `@internal/core/agent/acp_convert.go`:
- Around line 466-471: The renderDiffText function can produce malformed diffs
when path contains newlines or when newText/oldText don't end with a newline;
update renderDiffText to sanitize the path (e.g., replace or escape newline
characters and other control characters in the path parameter) and normalize
content by ensuring newText and *oldText end with a trailing newline (append
"\n" if missing) before formatting the diff; reference the renderDiffText(path
string, newText string, oldText *string) function and apply these
transformations to path, newText, and oldText prior to the fmt.Sprintf calls.

In `@internal/core/agent/tool_call_name.go`:
- Around line 219-229: looksLikeWebSearchInput currently treats any input with a
"url" key as a web search which causes false positives for fetch-like tools;
update the logic so a lone "url" does not trigger web-search detection — either
remove "url" from the top-level key list in looksLikeWebSearchInput or change
the check to require "url" plus a query-like key (e.g., "query", "queries",
"search_query", "action_query") before returning true; also ensure
normalizeToolNameByKind (used when kind == ToolKindSearch) continues to prefer
WebFetch/OpenURL for url-only inputs if you choose the list change, so
inferToolNameFromInputShape, normalizeToolNameByKind, and
looksLikeWebSearchInput remain consistent.

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

Inline comments:
In `@internal/cli/root_test.go`:
- Around line 836-848: Replace the string-based error check with an errors.Is
assertion against a sentinel error: introduce a package-level sentinel variable
(e.g., var brokenStdout = errors.New("broken stdout")), use that sentinel when
constructing the failOnWriteWriter in the test (failOnWriteWriter{err:
brokenStdout}), call state.fetchReviews(cmd, nil) as before, then assert
errors.Is(err, brokenStdout) instead of checking err.Error() for "write fetch
summary"; keep the existing check for the wrapper message if you still want to
validate the outer text but ensure the underlying writer error is asserted via
errors.Is.

In `@internal/core/agent/registry_launch.go`:
- Around line 186-192: The shell-escaping logic in formatShellArg incorrectly
substitutes embedded single quotes with '\"'\"' which is not valid POSIX syntax;
update the replacement so embedded "'" are escaped using the correct pattern
'"'\'"'" (i.e. replace occurrences of "'" with `'"'"'`) so the function returns
a valid single-quoted string like "'" + strings.ReplaceAll(arg, "'", `'"'"'`) +
"'" and ensures inputs like O'Connor produce a well-formed quoted argument.

---

Duplicate comments:
In `@internal/core/agent/registry_launch.go`:
- Around line 74-85: Ensure the availability probe uses the caller-specified
effective model instead of always passing spec.DefaultModel: inside the
EnsureAvailable / registry launch flow replace the hardcoded spec.DefaultModel
argument to resolveLaunchCommand with the effective model from the config (e.g.,
cfg.Model or a computed effectiveModel derived from cfg and spec) so that
resolveLaunchCommand (called in EnsureAvailable) validates the same model the
real launch will use.

---

Nitpick comments:
In `@internal/cli/state.go`:
- Around line 311-314: Wrap the error returned from coreRun.LoadPersistedExecRun
with contextual information about which run was being resumed: replace the bare
return err after calling coreRun.LoadPersistedExecRun(s.workspaceRoot, s.runID)
with a fmt.Errorf that includes s.runID (and optionally s.workspaceRoot) and
uses %w to wrap the original err so callers retain the original error while
gaining context.

In `@internal/core/agent/acp_convert.go`:
- Around line 15-20: The helper errors from convertACPMessageUpdate and
convertACPToolLifecycleUpdate must be returned with contextual wrapping instead
of passed through; change the early-return logic to first check ok and return
the converted value when ok is true, and if err != nil return nil,
fmt.Errorf("message conversion failed: %w", err) for convertACPMessageUpdate
(and similarly fmt.Errorf("tool-lifecycle conversion failed: %w", err) for
convertACPToolLifecycleUpdate), ensuring you import fmt if needed and preserve
the original converted return when ok is true.

In `@internal/core/agent/registry_launch.go`:
- Around line 141-149: The current LookPath error is being replaced with a
hardcoded message which loses the original error context; update the code in
registry_launch.go (the exec.LookPath check) to wrap the original err when
constructing the AvailabilityError Cause (use fmt.Errorf("command %q: %w",
command[0], err) or equivalent) so callers can use errors.Is/errors.As and see
the actual underlying failure while preserving the descriptive "command X"
context in the AvailabilityError returned by the function that constructs
AvailabilityError.