refactor: daemon improvs - #121

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

Inline comments:
In @.compozy/tasks/daemon-improvs/analysis/qa/logs/live-daemon-run-id.txt:
- Line 1: Remove the committed one-off QA run ID string
"tasks-node-health-8904e3-20260421-054130-000000000" from the repository and
stop committing this generated state: either delete the file content and add the
filename to .gitignore (or move it under an ignored runtime/ci artifacts
folder), or change the workflow that writes it so the run ID is emitted as a
CI/test artifact instead of being checked into the tree; update any references
to the file so code expecting it reads from the new ignored/CI artifact
location.

In `@internal/api/client/client_contract_test.go`:
- Around line 249-253: This test mutates the package-level variable
streamHeartbeatGapTolerance while also using t.Parallel(), risking races; remove
t.Parallel() from the test (or alternatively change the test to inject a
per-test configuration instead of modifying streamHeartbeatGapTolerance) and
keep the existing t.Cleanup that restores the previousGap to ensure isolation;
locate the mutation of streamHeartbeatGapTolerance and the t.Parallel() call in
the test and delete the parallelization (or refactor to accept a test-specific
timeout/config parameter rather than touching the package variable).

In `@internal/api/client/client_transport_test.go`:
- Around line 201-213: The test stub for the stop endpoint is too permissive and
doesn't assert the StopDaemon(force bool) encoding; update the Transport
roundTripFunc handler for the case handling "POST /api/daemon/stop" to validate
the request shape (either check req.URL.Query() for a force=true param or decode
the JSON body and assert {"force":true} as appropriate for StopDaemon), and only
return jsonResponse(http.StatusAccepted, `{"accepted":true}`) when that shape
matches; ensure the test fails if StopDaemon(true) omits or mis-encodes the
force flag so the transport contract for StopDaemon is actually enforced.

In `@internal/api/contract/contract_integration_test.go`:
- Around line 207-230: In the loop handling framesCh, avoid closing sendOverflow
on every heartbeat; change the heartbeat branch (where you currently call
close(sendOverflow)) to only close it once by checking if sendOverflow != nil
before closing and then set sendOverflow = nil (or use a boolean flag) so
subsequent heartbeat events won't attempt to close an already-closed channel;
update the code around the heartbeat handling in the test (where heartbeat,
overflow, framesCh, and sendOverflow are referenced) accordingly.

In `@internal/api/contract/types.go`:
- Around line 500-513: In RunEventPageResponse.Decode, add a fast-fail when the
server declares HasMore but doesn't provide a NextCursor: if r.HasMore &&
r.NextCursor == "" return an error (e.g. fmt.Errorf("decode events:
has_more=true but next_cursor missing")). Keep the ParseCursor call and its
error handling for non-empty cursors, and only set page.NextCursor when
ParseCursor succeeds and nextCursor.Sequence > 0; reference
RunEventPageResponse.Decode, RunEventPage, r.HasMore, r.NextCursor, and
ParseCursor.
- Around line 245-273: The nested session snapshot structs (SessionEntry,
SessionPlanState, SessionMetaState, SessionViewSnapshot) are missing JSON tags
so the marshalled fields use PascalCase instead of the expected snake_case;
update each struct's fields (e.g., SessionEntry.ID, SessionEntry.Kind,
SessionPlanState.Entries, SessionPlanState.PendingCount,
SessionMetaState.CurrentModeID,
SessionViewSnapshot.Revision/Entries/Plan/Session, etc.) to include explicit
`json:"..."` tags using the expected snake_case names (e.g., `id`, `kind`,
`title`, `preview`, `tool_call_id`, `tool_call_state`, `blocks`, `entries`,
`pending_count`, `running_count`, `done_count`, `current_mode_id`,
`available_commands`, `status`, `revision`, `plan`, `session`) so JSON
encoding/decoding matches the wire contract.

In `@internal/api/httpapi/transport_integration_test.go`:
- Around line 1789-1851: readSSEFramesUntil currently takes an io.Reader so when
we return early (stop(frames) == true) the scanner goroutine keeps blocking on
body and leaks; change the function to accept an io.ReadCloser (or add a
separate io.Closer param), and ensure you Close() the body when exiting early so
the scanner unblocks; update the goroutine/cleanup to still send scanner.Err()
to errCh and close linesCh as before, and call body.Close() (or defer a close on
normal return) immediately before returning from readSSEFramesUntil to ensure
the scanner goroutine terminates.
- Around line 1889-1893: The test helper currently ignores the error returned by
io.ReadAll when building the failure message for a non-OK response; update the
error path in the block that checks response.StatusCode (the code using
response, io.ReadAll and t.Fatalf) to properly handle the ReadAll error by
checking and including it in the failure output (e.g., if io.ReadAll returns an
error, include that error string in the t.Fatalf message alongside or instead of
the body content), ensure response.Body is deferred closed before reading, and
avoid using the blank identifier for the read error.

In `@internal/daemon/boot_test.go`:
- Around line 550-560: Replace the manual temp-dir creation and cleanup with
t.TempDir(): stop using os.TempDir()/os.MkdirTemp and the homeRoot variable plus
t.Cleanup + os.RemoveAll; instead call homeRoot := t.TempDir() where the current
MkdirTemp result is used and remove the explicit t.Cleanup/_ = os.RemoveAll
block so the test harness manages cleanup and no errors are ignored.

In `@internal/daemon/watchers.go`:
- Around line 253-261: The pre-sync reconcile clears state.pending and
state.refreshWatches before calling w.reconcileWatchState(watcher, state, ...)
so a failed reconcile drops queued changes; change the flow to preserve those
flags until reconcileWatchState succeeds — either move the clearing of
state.pending and state.refreshWatches to after a successful return from
reconcileWatchState, or if you must clear earlier, restore/re-queue them when
reconcileWatchState returns false; update the logic around reconcileWatchState,
state.pending and state.refreshWatches to ensure pending artifact changes are
not lost on transient reconcile failures.

In `@internal/store/globaldb/close_test.go`:
- Around line 11-42: Wrap the existing
TestGlobalDBCloseContextDelegatesToSQLiteCloser logic inside a named subtest
using t.Run with a "Should..." description (e.g., t.Run("Should propagate caller
context and close underlying sqlite DB", func(t *testing.T) { ... })), keeping
the existing setup that overrides closeGlobalSQLiteDatabase and assertions
against gotCtx, gotDB and global.closed; if you anticipate more scenarios,
convert to a table-driven loop and call t.Run for each case, but at minimum
enclose the current test body in a single t.Run("Should...") subtest to match
repository test conventions.

In `@internal/store/globaldb/global_db.go`:
- Around line 80-84: The code currently flips g.closed to true before calling
closeGlobalSQLiteDatabase, preventing retries on failure; change the logic so
g.closed is only set to true after closeGlobalSQLiteDatabase succeeds.
Specifically, acquire a single-attempt lock (either add a new atomic g.closing
used with CompareAndSwap(false,true) to guard concurrent closes, or keep the
existing CompareAndSwap but if you set g.closed to true only after err == nil),
call closeGlobalSQLiteDatabase(ctx, g.db), and if that call returns an error
ensure the in-progress flag is cleared (or do not set g.closed) so subsequent
calls can retry; finally set g.closed to true only on successful close.

In `@internal/store/rundb/run_db.go`:
- Around line 759-810: The read-modify-write race happens because GetIntegrity()
and the subsequent upsert (r.db.ExecContext with ON CONFLICT) are separate
operations; wrap the read + merge + upsert in a single database transaction to
make the merge atomic. Specifically, start a transaction (e.g., tx, err :=
r.db.BeginTx(ctx, nil)), call a transactional variant of GetIntegrity that
selects the singleton row inside the tx (using tx.QueryRowContext or SELECT ...
FOR UPDATE if supported), perform the same merge into RunIntegrityState (using
r.now() and encodeIntegrityReasons), then execute the INSERT ... ON CONFLICT
upsert using tx.ExecContext and commit the tx; ensure errors roll back the tx.
This keeps the existing logic (mergeIntegrityReasons, encodeIntegrityReasons,
r.now, store.FormatTimestamp) but performs all operations using the tx instead
of r.db to prevent lost updates.
- Around line 147-156: The CloseContext method currently sets r.db = nil before
calling closeRunSQLiteDatabase, which hides failures; change it so you capture
the db handle into a local variable (db := r.db), call
closeRunSQLiteDatabase(ctx, db) first, check its error, and only on success set
r.db = nil and return nil; on error leave r.db intact and return the error.
Update the CloseContext implementation (referencing CloseContext, r.db, and
closeRunSQLiteDatabase) to nil out the field only after a successful close.

---

Nitpick comments:
In `@internal/api/contract/compatibility.go`:
- Around line 3-8: Add a documentation comment for the exported
CompatibilityNote type that explains its purpose and what each field represents;
update the comment immediately above the type declaration for CompatibilityNote
and describe Surface, StableJSONFields, AdapterRequiredFor, and Notes so
external users understand when and how to use this struct.

In `@internal/api/contract/sse.go`:
- Around line 78-80: The parse-and-validate for the cursor sequence currently
collapses the ParseUint error into a generic "invalid cursor sequence" return;
change the logic in sse.go so you first call strconv.ParseUint and if err != nil
return a wrapped error like fmt.Errorf("parsing cursor sequence %q: %w",
parts[1], err), then separately validate sequence==0 and return the existing
fmt.Errorf("invalid cursor sequence %q", parts[1]) (returning StreamCursor{} on
each error) so the original ParseUint error is preserved and wrapped per
guidelines.

In `@internal/api/core/handlers_contract_test.go`:
- Around line 398-492: The SSE frame type and parser (canonicalSSEFrame,
readSSEFramesUntil, hasSSEFrame, firstSSEFrame and the helper
newCanonicalHandlersEngine) are duplicated across tests; extract them into a
single shared test helper (e.g., internal/api/testhelpers/sse_helpers_test.go)
and replace the copies in all test files to import/use that helper; move the
type and functions into the shared file, keep them unexported if intended for
package-level use or export them if cross-package, and update any tests
referencing the old local definitions to call the shared functions so there is a
single canonical implementation to maintain.

In `@internal/cli/daemon_commands_test.go`:
- Around line 1087-1088: The test function
TestDaemonStartCommandInternalChildUsesDetachedRunMode is missing t.Parallel()
so it can't run concurrently; open that function and add a call to t.Parallel()
immediately after the test begins (after acquireCLITestGlobalOverride(t) if that
setup must run first, or at the top of the function if safe) to mark it as
parallelizable, ensuring it follows the project's parallel test conventions.
- Around line 1132-1133: Add t.Parallel() at the start of
TestLaunchCLIDaemonProcessFailsWhenDaemonLogFileCannotBeOpened to allow it to
run concurrently; locate the test function declaration in
internal/cli/daemon_commands_test.go (function name:
TestLaunchCLIDaemonProcessFailsWhenDaemonLogFileCannotBeOpened) and insert a
call to t.Parallel() immediately after the first line in the function body so
the test uses its t.TempDir() isolation safely in parallel.

In `@internal/cli/daemon_exec_test_helpers_test.go`:
- Around line 68-72: The test currently ignores the error returned by
manager.Shutdown inside the t.Cleanup closure; either handle the error or
document why it's safe to ignore it. Update the t.Cleanup block around
manager.Shutdown(shutdownCtx, true) to capture the returned error and call
t.Logf or t.Fatalf on non-nil error (or add a short comment explaining why
suppressing the error is acceptable for test cleanup), referencing the existing
symbols shutdownCtx, shutdownCancel, manager.Shutdown and t.Cleanup so reviewers
can locate and verify the change.

In `@internal/cli/reviews_exec_daemon_additional_test.go`:
- Around line 1389-1424: Rename the subtest title to follow the "Should..."
pattern (e.g., t.Run("Should wait for durable terminal snapshot after terminal
event", ...) instead of the current text) and make the subtest run in parallel
by calling t.Parallel() at the start of the subtest body; update the t.Run
invocation and insert t.Parallel() inside the anonymous func used with t.Run
that exercises defaultWatchCLIRun (the test that uses newStaticClientRunStream,
stubDaemonCommandClient and snapshotFunc).

In `@internal/core/run/journal/journal.go`:
- Around line 404-414: The function isTerminalJournalEvent duplicates the
terminal-kind check already implemented by isTerminalEvent; update
recordDroppedSubmit (and any other callers) to call isTerminalEvent instead of
isTerminalJournalEvent, then remove or deprecate isTerminalJournalEvent to
eliminate duplicated logic; ensure all references (e.g., in recordDroppedSubmit)
are updated to use isTerminalEvent so terminal-kind changes remain centralized.

In `@internal/daemon/boot_integration_test.go`:
- Around line 514-523: The polling loop that checks QueryStatus (using
ProbeOptions{}) currently uses time.Sleep; replace it with a
time.NewTicker-based loop with defer ticker.Stop() to match the pattern used by
waitForHealthyDaemon/waitForManagedDaemonReady: create a ticker (e.g., 50ms),
check status, break/return when status.State == want, and also check a deadline
via time.Now().After(deadline) to t.Fatalf on timeout; reference QueryStatus,
ProbeOptions, and the local want variable when updating the loop.

In `@internal/daemon/run_integrity.go`:
- Around line 31-42: Wrap the raw errors returned from acquireRunDB and the DB
integrity operations with contextual run information including runID and phase
names; e.g., when calling acquireRunDB(ctx, runID) return fmt.Errorf("acquire
run DB for run %s: %w", runID, err) on error, and when
lease.DB().UpsertIntegrity(ctx, update) (and the corresponding get/upsert calls
on lines 70-89) return fmt.Errorf("upsert integrity for run %s: %w", runID, err)
(or use "get integrity" for get calls) so every error includes the run ID and
operation phase.

In `@internal/daemon/run_manager.go`:
- Around line 524-538: The call to m.persistRuntimeIntegrity currently only logs
a warning and proceeds; decide whether to fail fast or surface the persistence
error in the snapshot: either (A) propagate the error by returning it after the
call to m.persistRuntimeIntegrity (e.g., wrap and return the error from the
enclosing function) so the snapshot creation stops on persistence failure, or
(B) attach the persistence error to the returned RunSnapshot (add a
RuntimeIntegrityError/RuntimeIntegrityStatus field) so callers can observe the
failure while still returning the snapshot; implement the chosen approach at the
m.persistRuntimeIntegrity call site (just before loadRunIntegrity) and ensure
the log remains informative.

In `@internal/daemon/shutdown.go`:
- Around line 165-183: Add a short comment above the joinRunManagerMetricKey and
splitRunManagerMetricKey functions documenting that they use a null-byte
("\x00") as an internal separator, why that choice is safe (mode/status values
will never contain null bytes), and that the functions trim inputs and treat
empty parts as invalid; this clarifies the convention for future maintainers and
prevents confusion about the unconventional delimiter.

In `@internal/logger/logger_test.go`:
- Line 12: Add t.Parallel() as the first statement in each independent test
function in this file to enable concurrent execution; specifically insert it at
the top of TestInstallDaemonLoggerForegroundMirrorsToStderrAndFile and the other
three peer tests in the same file so each test (which already uses t.TempDir()
for isolation) runs in parallel. Ensure t.Parallel() is the immediate first
action inside the test function block for
TestInstallDaemonLoggerForegroundMirrorsToStderrAndFile and the other test
functions to follow the coding guideline.

In `@internal/logger/logger.go`:
- Around line 283-289: normalizeFilePath currently only trims whitespace; update
it to use filepath.Clean to normalize path components (collapse "..", ".",
redundant separators) before returning. Specifically, after trimming in
normalizeFilePath, call filepath.Clean(cleanPath) and return that cleaned value
(still return the existing error when the trimmed string is empty); import
"path/filepath" if not present. This ensures consistent, normalized paths for
log rotation and reduces path traversal issues.

In `@internal/store/rundb/close_test.go`:
- Around line 11-42: Wrap the existing
TestRunDBCloseContextDelegatesToSQLiteCloser body in a t.Run subtest named with
the "Should ..." pattern (e.g. t.Run("Should delegate CloseContext to sqlite
closer", func(t *testing.T) { ... })), moving all setup, the t.Cleanup restore
of closeRunSQLiteDatabase, the stub assignment to closeRunSQLiteDatabase,
creation of db and runDB, the ctx creation using runDBCloseContextKey("scope"),
the call to runDB.CloseContext(ctx), and all assertions into that subtest
closure so the test follows the repository's subtest pattern while preserving
the original behavior and cleanup semantics.

In `@internal/store/sqlite_test.go`:
- Around line 11-71: Combine the two separate tests
TestCloseSQLiteDatabaseCheckpointsBeforeClose and
TestCloseSQLiteDatabaseStillClosesWhenCheckpointFails into a single table-driven
test named TestCloseSQLiteDatabase that uses t.Run("Should ...") subtests;
create a slice of test cases describing the scenario (e.g., success checkpoint
vs failing checkpoint) and for each case set up the mocked checkpointSQLiteWAL
and closeSQLiteHandle behaviors, run CloseSQLiteDatabase(context.Background(),
&sql.DB{}), and assert the expected error and whether closeSQLiteHandle was
invoked and the order of steps; ensure you restore original checkpointSQLiteWAL
and closeSQLiteHandle via t.Cleanup and keep references to those symbols to
locate the existing mocks.

In `@pkg/compozy/runs/run_test.go`:
- Around line 62-82: Wrap the existing assertions in
TestAdaptRemoteRunSnapshotPreservesIncompleteReasons inside a subtest using
t.Run with a "Should..." description (e.g., t.Run("Should preserve incomplete
reasons and next cursor", func(t *testing.T) { ... })), and call t.Parallel() at
the start of that subtest; keep the call to
adaptRemoteRunSnapshot(apicore.RunSnapshot{...}) and the current assertions
unchanged but moved into the subtest body to satisfy the test naming and
parallelization guidelines.

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

Inline comments:
In `@internal/daemon/boot_integration_test.go`:
- Around line 283-353: Rename the subtest names to follow the repository's
"Should..." convention: in
TestManagedDaemonStopEndpointShutsDownAndRemovesSocket change
t.Run("force="+strconv.FormatBool(force), ...) to something like t.Run("Should
stop and remove socket when force="+strconv.FormatBool(force), ...) (referencing
the force loop and StopDaemon/QueryStatus/paths.SocketPath logic), and in
TestManagedDaemonRunModesControlLogging change t.Run("foreground mirrors to
stderr", ...) to t.Run("Should mirror logs to stderr in foreground mode", ...)
and t.Run("detached writes only file", ...) to t.Run("Should write logs only to
file in detached mode", ...) (these refer to startManagedDaemonHelperProcess,
RunModeForeground/RunModeDetached, waitForLogContains and
waitForStderrContains). Ensure no other behavior changes beyond renaming the
t.Run strings.

In `@internal/daemon/watchers_test.go`:
- Around line 218-276: Wrap the existing test body inside a t.Run subtest (e.g.
t.Run("Should preserve state when pre-sync reconcile fails", func(t *testing.T)
{ ... })) and replace the weak nil check on watcher.stopError() with a specific
error assertion that the recorded error is non-nil and its message contains a
reconcile-related substring; for example, call err := watcher.stopError(); if
err == nil { t.Fatal(...) } else if !strings.Contains(err.Error(), "reconcile")
{ t.Fatalf("stopError() = %v, want message containing %q", err, "reconcile") }
so the test uses a subtest pattern and validates the error message.

In `@internal/logger/logger.go`:
- Around line 221-236: The rotation code closes r.file before calling
rotateLogFiles/openLogFile which can leave r.file pointing at a closed
descriptor if rotation or reopening fails; change the logic to preserve the
active handle until a new file is successfully opened: keep the existing r.file
(and its descriptor) open, call rotateLogFiles using r.path, attempt to open a
new file via openLogFile into local variables (e.g., newFile, newSize), and only
if openLogFile succeeds assign r.file=newFile and r.size=newSize and then close
the previous file; if rotateLogFiles or openLogFile fail, leave r.file intact
and return the error so logging continues uninterrupted (referencing r.file,
rotateLogFiles, openLogFile, r.path, r.maxRetainedFiles, r.filePerm).

In `@internal/store/rundb/run_db_test.go`:
- Around line 457-485: The test is ineffective because db.now is called after
UpsertIntegrity has already taken integrityMu, so only one goroutine ever
signals nowEntered; fix this by moving the timestamp acquisition out of the
integrityMu critical section: in UpsertIntegrity, call db.now() (the now hook)
before acquiring integrityMu so multiple goroutines can enter and block on the
test's releaseNow/nowEntered signals; ensure any state that depends on the
timestamp is used after the lock is acquired and that you keep integrityMu
protection for shared state updates.

In `@internal/store/rundb/run_db.go`:
- Around line 163-167: closeRunSQLiteDatabase(ctx, db) can return an error even
though the underlying *sql.DB has been closed; to avoid leaving r.db pointing at
a closed/unusable handle, call closeRunSQLiteDatabase(ctx, db), capture its
error, then unconditionally set r.db = nil before returning (preserve and return
the error). Update the block surrounding r.db and closeRunSQLiteDatabase to
always clear the cached r.db regardless of the error path so retries don't hit
"database is closed" permanently.

---

Duplicate comments:
In `@internal/store/globaldb/global_db.go`:
- Around line 88-90: The return of closeGlobalSQLiteDatabase(ctx, g.db) should
be wrapped with context using fmt.Errorf so callers can attribute failures to
the GlobalDB layer; replace `if err := closeGlobalSQLiteDatabase(ctx, g.db); err
!= nil { return err }` with a wrapped return like `if err :=
closeGlobalSQLiteDatabase(ctx, g.db); err != nil { return fmt.Errorf("GlobalDB:
closeGlobalSQLiteDatabase failed: %w", err) }`, using fmt.Errorf and %w to
preserve the original error.

---

Nitpick comments:
In `@internal/api/core/handlers_contract_test.go`:
- Around line 59-61: The redundant loop-variable capture "tc := tc" inside the
test loops (around the t.Run calls that iterate over testCases in
handlers_contract_test.go) should be removed because Go 1.22+ makes loop
variables per-iteration; delete the duplicate assignments found in the first
t.Run loop and the second occurrence later in the file (the other t.Run block)
so the subtests use the loop variable directly (keep the t.Run signature and
test case usage unchanged).

In `@internal/daemon/boot_integration_test.go`:
- Around line 533-562: Both waitForLogContains and waitForStderrContains
currently busy-poll with time.Sleep; replace those polling loops with a
ticker/timer select loop like the other helpers: create a
time.NewTicker(50*time.Millisecond) and time.NewTimer(10*time.Second), defer
Stop on the ticker and Timer's drain/Stop, and in a select handle ticker.C to
re-check the condition (os.ReadFile + strings.Contains for waitForLogContains,
stderr != nil && strings.Contains(stderr.String(), ...) for
waitForStderrContains) and timer.C to fail the test with the same diagnostics;
keep references to the existing functions and the synchronizedBuffer type to
locate the code.

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

Inline comments:
In `@internal/daemon/boot_integration_test.go`:
- Around line 388-391: The cleanup and helper calls currently ignore errors
(e.g., the calls to cmd.Process.Kill(), cmd.Process.Wait(), ReadInfo, ReadFile)
using `_`, which hides failures; update each occurrence so you capture the error
(err := cmd.Process.Kill(), err := cmd.Process.Wait(), etc.) and handle it
explicitly — for unexpected failures call t.Fatalf or t.Errorf to surface the
root cause in CI, and for any genuinely expected/benign errors use an explicit
errors.Is(...) check with a short comment justifying why that specific error can
be ignored; apply this change to the cleanup block around
cmd.Process.Kill()/Wait and to the ReadInfo/ReadFile helper usages mentioned so
no error is discarded without handling or justification.

In `@internal/logger/logger_test.go`:
- Around line 12-177: Several top-level tests
(TestInstallDaemonLoggerForegroundMirrorsToStderrAndFile,
TestInstallDaemonLoggerDetachedWritesOnlyFile,
TestOpenRotatingFileRotatesAtConfiguredSize,
TestOpenRotatingFileKeepsWritingAfterRotationFailure,
TestValidateDaemonFilePathRejectsEmptyPath,
TestNormalizeFilePathCleansRelativeSegments) must be refactored into a
table-driven pattern using t.Run("Should...") subtests; convert these cases into
a single Test... function that iterates a slice of test cases and invokes t.Run
with descriptive "Should..." names for each scenario, keeping existing
assertions and reusing helpers like InstallDaemonLogger, openRotatingFile,
ValidateDaemonFilePath, and normalizeFilePath to locate logic. Also fix the
ignored error in TestOpenRotatingFileKeepsWritingAfterRotationFailure by
checking the return value from writer.Close() (the writer variable from
openRotatingFile) and handling/failing the test on error instead of using `_ =
writer.Close()`, or add an explicit comment justifying why it can be ignored if
you genuinely choose that route.
- Around line 118-120: In TestOpenRotatingFileKeepsWritingAfterRotationFailure
remove the redundant defer that calls _ = writer.Close() and do not ignore the
close error; delete that defer block and rely on the existing explicit
writer.Close() at the end of the test (or convert it to t.Cleanup) and
assert/check its error (e.g., require.NoError/if err != nil t.Fatalf) so the
writer variable is closed exactly once with proper error handling.
- Around line 134-136: The test currently only checks for a non-nil error from
writer.Write during a rotation failure; change that assertion to use errors.As
to confirm the returned (possibly wrapped) error is a file operation error such
as *os.PathError (i.e., replace the nil-check with an errors.As check against
os.PathError when calling writer.Write in the rotation test). Also make the
package export a sentinel error (e.g., var ErrInvalidPath = errors.New("invalid
daemon path")) used by ValidateDaemonFilePath, then update the test to assert
errors.Is(err, logger.ErrInvalidPath) (or alternatively compare the error
string) so the ValidateDaemonFilePath failure can be detected via errors.Is
rather than an unwrapped errors.New.

In `@internal/store/rundb/run_db_test.go`:
- Around line 365-447: The test
TestRunDBUpsertIntegrityIsStickyAndSurvivesReopen violates the repo policy
requiring subtests; wrap the test body in a t.Run("Should ...") subtest (e.g.
t.Run("Should persist integrity and sticky counts across reopen", func(t
*testing.T) { ... })) and similarly update the other new test referenced (lines
449-508) to use t.Run subtests (or table-driven t.Run cases if there are
multiple scenarios); keep the existing assertions and helper calls
(openTestRunDB, UpsertIntegrity, GetIntegrity, Open, Close) unchanged but move
them inside the t.Run closures and call t.Parallel() inside the subtest where
appropriate.

---

Duplicate comments:
In `@internal/store/rundb/run_db_test.go`:
- Around line 449-490: The test
TestRunDBUpsertIntegritySerializesConcurrentMerges can still release the gate
before both goroutines are blocked in db.now; to fix this ensure both
UpsertIntegrity callers have entered the db.now hook before closing releaseNow:
increment the nowEntered channel capacity (or reuse it) and block until you
receive two signals from nowEntered (one per goroutine) prior to closing
releaseNow, and remove or adjust the secondStarted timing hack so the test waits
on the two nowEntered receipts before releasing; reference
TestRunDBUpsertIntegritySerializesConcurrentMerges, db.now, nowEntered,
releaseNow and UpsertIntegrity to locate where to add the additional wait.

---

Nitpick comments:
In `@internal/api/core/handlers_contract_test.go`:
- Around line 59-61: Rename subtest name strings to follow the "Should..."
convention: when calling t.Run(tc.name, ...) update the test case names in the
testCases slice (referenced by variable testCases and used in t.Run) so each
tc.name is prefixed with "Should " (e.g., change "degraded" to "Should return
degraded status") ensuring all entries follow the consistent "Should..." pattern
used across tests.
- Around line 232-246: The test uses http.NewRequestWithContext to open an SSE
connection but then calls http.DefaultClient.Do which has no timeout; replace
the use of http.DefaultClient with a dedicated http.Client that has a reasonable
Timeout (e.g., a few seconds) and call client.Do(request) so the initial
connection cannot hang indefinitely; you can alternatively wrap request creation
in context.WithTimeout, but ensure the code around the SSE endpoint (the request
to server.URL+"/api/runs/"+runID+"/stream") uses that timeout and continues to
work with ReadSSEFramesUntil.

In `@internal/daemon/boot_integration_test.go`:
- Around line 327-353: Refactor TestManagedDaemonRunModesControlLogging into a
table-driven test: replace the two explicit subtests with a slice of test cases
(including fields like name, mode (use RunModeForeground / RunModeDetached),
expectStderrMirror bool, and expectedModeString) and iterate with t.Run for each
case; inside the loop call startManagedDaemonHelperProcess,
waitForManagedDaemonReady, waitForLogContains and stopDaemonHelperProcess
exactly as before, and assert stderr behavior using waitForStderrContains when
expectStderrMirror is true or assert absence of the message in output.stderr
when false, keeping references to the existing helpers
(startManagedDaemonHelperProcess, waitForManagedDaemonReady, waitForLogContains,
waitForStderrContains, stopDaemonHelperProcess) and preserving the existing log
assertions for `"msg":"daemon started"` and the
`"mode":"foreground"/"mode":"detached"` checks via the expectedModeString field.

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

Inline comments:
In `@internal/api/core/handlers_contract_test.go`:
- Around line 200-214: The goroutine in the test blocks on `<-sendOverflow` and
can leak if the test fails before a heartbeat; update the goroutine to respect a
cancellation context or test “done” signal: accept a ctx (or use t.Cleanup to
create one) and replace the blocking `<-sendOverflow` with a select that listens
for either `case <-sendOverflow:` or `case <-ctx.Done():` so the goroutine can
exit on test cancellation; ensure you still send the overflow event to
`stream.events` (RunStreamItem with Overflow) and close
`stream.events`/`stream.errors` on shutdown.

In `@internal/store/globaldb/registry_test.go`:
- Around line 88-132: Wrap the test body of
TestGetByPathPrefersResolvedCanonicalWorkspaceRow in a subtest using t.Run with
a "Should..." description (e.g. t.Run("Should prefer resolved canonical
workspace row", func(t *testing.T) { ... })) and move the existing t.Parallel()
and all setup/assertions inside that subtest; do the same for the other added
test case referenced at lines 235-267 so both follow the repository's
t.Run("Should...") subtest pattern while keeping function name and helper calls
(db.Register, db.Get, db.now, etc.) unchanged.
- Around line 238-266: The test fixture uses root := string(filepath.Separator)
which yields "\" on Windows and produces non-absolute paths; update the fixture
so the constructed paths are absolute on Windows by prefixing a drive/volume
(use the same drive as the test input). Specifically, when building
root/usersDir/homeDir etc. for canonicalizeExistingPathCaseWith, set root to the
volume + separator (e.g., filepath.VolumeName(input) +
string(filepath.Separator)) or otherwise ensure input is absolute via
filepath.IsAbs/input->filepath.Abs before calling
canonicalizeExistingPathCaseWith so the function treats the path as absolute and
the test exercises the case-normalization logic.

In `@internal/store/globaldb/registry.go`:
- Around line 907-911: The current call to canonicalizeExistingPathCase in
registry.go should be made best-effort rather than a hard failure: if
canonicalizeExistingPathCase(resolvedPath) returns an error, do not return that
error — instead fall back to returning filepath.Clean(resolvedPath) (optionally
logging/debugging the canonicalization error) so lack of read permission on
parent directories doesn't block normalization; apply the same change for the
other instance referenced (the similar block around lines 946-949) so both uses
of canonicalizeExistingPathCase degrade gracefully.
- Around line 922-925: The validation currently runs filepath.Clean before
checking for empty input, letting whitespace-only strings become "."; change the
order to trim whitespace first and validate that result is non-empty (use
strings.TrimSpace on path and check trimmed == ""), then call filepath.Clean on
the trimmed value and assign to cleanPath; update the error path that returns
errors.New("globaldb: workspace path is required") to trigger on the trimmed
empty string.

---

Nitpick comments:
In `@internal/api/core/handlers_contract_test.go`:
- Around line 191-192: The test function
TestStreamRunEmitsCanonicalEventHeartbeatAndOverflowPayloads is missing a
t.Parallel() call; open the
TestStreamRunEmitsCanonicalEventHeartbeatAndOverflowPayloads function and add a
t.Parallel() immediately after the function begins (right after
gin.SetMode(gin.TestMode)) so the test runs in parallel like the others.