feat: review watch - #133

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

Inline comments:
In `@internal/api/core/handlers_service_errors_test.go`:
- Around line 303-347: The test case names used as tc.name for t.Run are not
following the required "Should..." pattern; update the three table entries whose
name strings are "review watch active conflict", "review watch invalid request",
and "review watch unavailable" to use descriptive "Should..." phrases (e.g.,
"Should return conflict when review watch already active", "Should return
unprocessable entity for invalid watch request", "Should return service
unavailable when review watch is unavailable") so t.Run(tc.name, ...) uses the
mandated naming convention.

In `@internal/core/agent/client_test.go`:
- Around line 55-88: Wrap the entire body of
TestClientCreateSessionStartsAgentProcessInWorkingDirectory in a t.Run subtest
using a descriptive "Should..." name (e.g., t.Run("Should start agent process in
provided working directory", func(t *testing.T) { ... })) so the test follows
the mandated subtest pattern; keep all existing setup and assertions
(newTestClient, SessionRequest with WorkingDir, client.CreateSession,
collectSessionUpdates, session.Err(), client.Close()) inside that t.Run callback
and preserve t.Parallel() placement per project convention.

In `@internal/core/archive_test.go`:
- Around line 228-264: Refactor the two tests
TestArchiveTaskWorkflowAllowsResolvedReviewOnlyWorkflow and
TestArchiveTaskWorkflowRejectsUnresolvedReviewOnlyWorkflow into a single
table-driven test using t.Run("Should ...") subtests: create a slice of cases
describing name ("Should archive resolved review-only", "Should reject
unresolved review-only"), inputs (use writeArchiveReviewRound to write rounds
and mustSyncArchiveWorkflow), expected error (nil vs
globaldb.ErrWorkflowNotArchivable) and expected result assertions
(WorkflowsScanned/Archived/Skipped and os.Stat checks), then iterate cases and
call t.Run(case.name, func(t *testing.T) { t.Parallel(); /* call
Archive(context.Background(), ArchiveConfig{TasksDir: workflowDir}) and assert
*/ }); ensure each subtest uses t.Parallel() where safe and reference Archive,
ArchiveConfig, writeArchiveReviewRound, and mustSyncArchiveWorkflow to locate
logic.

In `@internal/core/provider/overlay_test.go`:
- Around line 169-192: Wrap the existing test logic inside a t.Run subtest
(e.g., t.Run("Should delegate watch status to target", func(t *testing.T) { ...
})) and move the setup/teardown (ActivateOverlay and its defer restore) and the
rest of the assertions into that inner function so the outer
TestAliasedProviderWatchStatusDelegatesToTarget only contains the t.Run call;
keep the same calls to ActivateOverlay, NewRegistry,
base.Register(&overlayTestProvider{name: "base"}), ResolveRegistry,
registry.Get("ext-review") and FetchWatchStatus to preserve behavior and
assertions.

In `@internal/core/provider/provider_test.go`:
- Around line 23-36: Wrap the existing test body of
TestFetchWatchStatusReturnsUnsupportedError in a t.Run subtest with a
descriptive name (e.g., "Should return unsupported error") and move t.Parallel()
into that subtest; specifically, inside
TestFetchWatchStatusReturnsUnsupportedError call t.Run("Should ...", func(t
*testing.T) { t.Parallel(); /* existing assertions using FetchWatchStatus,
unsupportedWatchProvider{}, WatchStatusRequest{PR: "259"} and checks against
ErrWatchStatusUnsupported and WatchStatusUnsupported */ }) so the test follows
the enforced t.Run("Should...") pattern while still running in parallel.

In `@internal/core/providerdefaults/defaults_test.go`:
- Around line 13-65: Wrap the body of
TestDefaultRegistryForWorkspaceRunsCodeRabbitCommandsFromWorkspaceRoot inside a
t.Run subtest (e.g., t.Run("Should fetch workspace-scoped CodeRabbit comments
from workspace root", func(t *testing.T) { ... })) so the test follows the
required t.Run("Should...") pattern; keep the existing setup (workspaceRoot,
fake gh script, env changes, registry := DefaultRegistryForWorkspace,
registry.Get, FetchReviews and assertions) but move them into the anonymous
subtest function and use that t parameter for assertions and t.Setenv/t.Chdir
calls.

In `@internal/core/workspace/config_validate.go`:
- Around line 189-194: The validation currently only rejects explicit false for
defaults.AutoCommit but lets nil pass; update the conditional that guards the
error in Validate (where cfg.AutoPush and defaults.AutoCommit are checked) to
use the isEnabled helper consistently: replace the check "defaults.AutoCommit !=
nil && !*defaults.AutoCommit" with "!isEnabled(defaults.AutoCommit, false)" so
nil and false are treated the same; keep the existing fmt.Errorf call that uses
configFieldName(scope, "watch_reviews.auto_push") and configFieldName(scope,
"defaults.auto_commit") unchanged.

In `@internal/daemon/extension_bridge.go`:
- Around line 91-93: The code currently replaces a nil context with
context.Background(), which breaks cancellation propagation for daemon-run
paths; instead, in the function that calls
resolveExtensionBridgeWorkspaceRoot(ctx) (in extension_bridge.go, part of the
DaemonHostBridge implementation), detect if ctx == nil and immediately return an
error (e.g., fmt.Errorf or wrapped error) indicating a missing context rather
than substituting context.Background(); keep
resolveExtensionBridgeWorkspaceRoot(ctx) and all downstream calls unchanged so
callers must pass a valid context.

In `@internal/daemon/query_service.go`:
- Around line 159-165: The embedded Workflow summary is built with
transportWorkflowSummary(workflow), which omits derived task_counts and
start-block fields; instead call the counted summary builder (buildWorkflowCard
or the function that accepts both workflow and counts) to construct the summary
using the existing counts variable, then set summary.ArchiveEligible =
&archiveEligible and summary.ArchiveReason = archiveReason before returning the
WorkflowOverviewPayload so the embedded Workflow matches the top-level
TaskCounts payload.

In `@internal/daemon/review_exec_transport_service_test.go`:
- Around line 126-195: These two test functions
(TestTransportReviewServiceFetchSyncsNoMetaRoundAfterLegacyReview and
TestTransportReviewServiceStartWatchUsesDaemonRunManager) must be restructured
into t.Run("Should ...") subtests (or table-driven subtests) to follow the
mandated pattern: wrap each scenario in t.Run with a descriptive "Should ..."
name, move their setup and assertions into the subtest closure, ensure proper
variable capture (use local copies if looping), and call t.Parallel() inside
subtests where safe; keep the existing calls to newTransportReviewService,
service.Fetch/GetRound/ListIssues and service.StartWatch unchanged but invoked
from within the t.Run closures.

In `@internal/daemon/review_watch_test.go`:
- Around line 961-967: In the execute helper function, move the nil-check for
cfg (model.RuntimeConfig) to before calling resolveIssues so you don't
dereference a nil pointer; i.e., first return errors.New("runtime config is
required") when cfg == nil, then call resolveIssues(ctx, preparation, cfg) and
handle its error, referencing the anonymous execute func and the resolveIssues
call to locate the change.
- Around line 726-743: The tests currently only assert that
resolveReviewWatchOptions and resolveReviewWatchDuration return a non-nil error;
change them to assert specific error content/types so they validate the intended
validation branch: for resolveReviewWatchOptions calls (with
apicore.ReviewWatchRequest{PRRef: "123"} and {Provider: "coderabbit"}) assert
the returned error is the expected validation error (use errors.Is or test
helper ErrorAs/ErrorContains with the known validation error sentinel/message),
and for resolveReviewWatchDuration calls assert the error contains the expected
message for zero duration and for invalid parse (use ErrorContains or
strings.Contains on the error string or errors.Is if there’s a sentinel).
Reference the functions resolveReviewWatchOptions and resolveReviewWatchDuration
and the input shapes apicore.ReviewWatchRequest to locate tests to update.
- Around line 26-810: Wrap each top-level test body in this file (e.g.
TestRunManagerReviewWatchCompletesCleanWithoutEmptyRound,
TestRunManagerReviewWatchPersistsRoundAndStartsOneChildRun,
TestRunManagerReviewWatchRejectsDuplicateActiveWatch, etc.) with a t.Run("Should
...", func(t *testing.T) { ... }) subtest using a human-readable "Should ..."
description derived from the test name, moving the current contents inside that
closure; preserve any existing inner t.Run calls (like the table-driven cases in
TestRunManagerReviewWatchFailureStates) and keep all setup/teardown, helper
calls, and assertions unchanged, only adding the t.Run wrapper to satisfy the
required subtest pattern.

In `@internal/daemon/task_transport_service.go`:
- Around line 67-77: The current loop in task_transport_service.go causes N+1 DB
calls by calling s.globalDB.ListTaskItems and attachWorkflowArchiveEligibility
for each workflow row; fix this by fetching task rows and archive-eligibility in
bulk before the loop (e.g., add or use a DB method like
ListTaskItemsForWorkflowIDs(ctx, ids) to get all task rows keyed by workflow ID
and a bulk attachWorkflowArchiveEligibilityBulk(ctx, db, rows) or a single query
that returns archive eligibility per workflow), then iterate rows and call
transportWorkflowSummaryWithTaskCounts(row,
summarizeTaskRows(bulkTaskMap[row.ID])) and set the precomputed eligibility from
the bulk result instead of per-row DB calls.
- Around line 99-103: GetWorkflow builds the summary using
transportWorkflowSummary which omits the derived task-count/start-block fields;
replace that call with transportWorkflowSummaryWithTaskCounts (and keep the
existing attachWorkflowArchiveEligibility call) so the single-workflow path
populates the same derived fields as ListWorkflows; update the code in
task_transport_service.go to create summary via
transportWorkflowSummaryWithTaskCounts(ctx, row, s.globalDB or same params used
by the list path) and then call attachWorkflowArchiveEligibility(ctx,
s.globalDB, row, &summary) before returning.

In `@internal/store/globaldb/archive_test.go`:
- Around line 100-196: The two new tests violate the project's
subtest/table-driven convention—convert
TestGetWorkflowArchiveEligibilityAllowsResolvedReviewOnlyWorkflow and
TestGetWorkflowArchiveEligibilityBlocksUnresolvedReviewOnlyWorkflow into
subtests under a single TestGetWorkflowArchiveEligibility function that uses
t.Run("Should ...") for each case; preserve t.Parallel(), drive cases via a
table/slice (each case including name, workflowID, review rounds, and expected
counts/archivable/skipReason), and inside each subtest call
mustReconcileArchiveWorkflow, db.GetWorkflowArchiveEligibility, and assert
TaskTotal/ReviewIssueTotal/UnresolvedReviewIssues/Archivable()/SkipReason()
accordingly so the unique helpers (mustReconcileArchiveWorkflow, mustWorkspace,
openTestGlobalDB, GetWorkflowArchiveEligibility) remain referenced and tests
follow the required pattern.

In `@packages/ui/src/components/surface-card.tsx`:
- Line 26: The header/footer class forces shrink-0 on the last child which, when
there's only one child (it is both first and last), prevents shrinking; update
the selector so shrink-0 is only applied when the last child is not the only
child—replace the [&>*:last-child]:shrink-0 usage with a non-only-child selector
such as [&>*:last-child:not(:only-child)]:shrink-0 (and apply the same change
for the other occurrence referenced in the file) while keeping the existing
[&>*:first-child]:min-w-0 rule.

In `@pkg/compozy/events/event_test.go`:
- Line 397: Rename the table-driven test case's name field to follow the
required t.Run("Should...") pattern: change the case currently named "review
watch lifecycle" to a name starting with "Should", e.g. "Should review watch
lifecycle", so the test invocation t.Run(...) conforms to the enforced naming
convention; update the entry in the test cases slice in event_test.go where the
name key is defined (the test case that currently reads "review watch
lifecycle").

In `@pkg/compozy/events/kinds/payload_compat_test.go`:
- Around line 91-139: Wrap the existing assertions in
TestReviewWatchPayloadJSONCompatibility inside a t.Run subtest with a
descriptive "Should ..." name (e.g., t.Run("Should serialize ReviewWatchPayload
to stable JSON map", func(t *testing.T) { ... })), move the t.Parallel() call
into the subtest body (call t.Parallel() as the first line inside the func), and
keep all existing setup, the payload variable, mustMarshalMap(t, payload) call,
and the reflect.DeepEqual check unchanged inside that subtest so the test logic
and assertions remain identical.

In `@web/src/systems/reviews/hooks/use-reviews.ts`:
- Around line 46-52: The enabled check currently allows round == 0 which causes
invalid API calls; update the guard to require round > 0 and add a runtime
validation before calling getReviewRound to throw on non-positive rounds.
Specifically, change the react-query enabled expression to use round != null &&
round > 0 (instead of round >= 0) and adjust the early validation (the if (round
== null) block) to also reject round <= 0 (e.g., if (round == null || round <=
0) throw ...) so getReviewRound is never invoked with round 0.

---

Nitpick comments:
In `@internal/core/reviews/store.go`:
- Around line 211-214: The call to roundMetaFromIssueFrontMatter returns raw err
which is returned directly; wrap this error with call-site context using
fmt.Errorf to preserve `%w` semantics so upstream matching still works. Update
the error return in the block that calls roundMetaFromIssueFrontMatter (the
variables meta, err) to return a wrapped error like fmt.Errorf("extracting round
meta from issue front matter: %w", err), ensuring you import fmt if not already
imported.

In `@internal/core/sync_test.go`:
- Around line 573-600: Rename the subtest label to follow the "Should..."
convention: in the t.Run call inside the Test (the subtest that writes files and
calls collectReviewRounds), change the name string from "consistent provider and
pr" to something like "Should project metadata when provider and pr are
consistent" so the test description follows the project's naming guideline while
leaving the logic (writeSyncWorkflowFile calls, collectReviewRounds, and
assertions on rounds[0].Provider, rounds[0].PRRef, ResolvedCount,
UnresolvedCount) unchanged.

In `@internal/daemon/review_watch_git_test.go`:
- Around line 192-217: Add t.Parallel() inside the subtest body so each case
runs concurrently; inside the loop capture the loop variable to avoid
closure/race (e.g., assign local := failingCall or change the t.Run callback to
accept the name) and then call t.Parallel() as the first statement in that
subtest. Update the subtest that invokes t.Run with failingCall (which
constructs execReviewWatchGit and calls git.State) to use the captured local
variable when comparing the constructed command and when naming the subtest.

In `@internal/daemon/review_watch_hooks.go`:
- Around line 239-261: In dispatchReviewWatchPostRoundHook (on RunManager) add
structured debug logging before the early returns so nil-guarded cases are
observable: detect when active is nil, when active.scope is nil, or when runtime
is nil and call slog.Logger (or the project's standard logger) with contextual
fields (e.g., runID, workflowSlug, provider, and which variable was nil) using
active.ctx when available, then return nil as before; ensure you instrument the
same function names seen here (dispatchReviewWatchPostRoundHook, activeRun,
reviewWatchCoordinatorRuntime) and keep the original behavior of calling
model.DispatchObserverHook and waitReviewWatchObserverHooks when not nil.

In `@internal/daemon/review_watch_test.go`:
- Around line 629-648: Add t.Parallel() at the start of each subtest body so the
table-driven cases run concurrently: inside the t.Run callback (where
newReviewWatchTestEnv, startReviewWatch, waitForRun, and requireRunEvent are
invoked for each tc) call t.Parallel() immediately before creating the env and
running the test logic to mark the subtest as parallelizable while preserving
isolation per tc.

In `@internal/daemon/review_watch.go`:
- Around line 1048-1071: The check in reserveReviewWatch is confusing because it
tests both runID != "" and keyReserved(m.activeReviewWatches, key); simplify by
using keyReserved to detect an existing reservation or active entry and
separately check runID to detect an active run: retrieve runID :=
strings.TrimSpace(m.activeReviewWatches[key]) after normalizing key, then if
keyReserved(m.activeReviewWatches, key) return the conflict with run_id set to
runID, else if runID != "" return the conflict for an active run; update
reserveReviewWatch to rely on keyReserved and activeReviewWatches consistently
and keep the allocation line m.activeReviewWatches[key] = "" for the reservation
case.

In `@internal/daemon/run_manager.go`:
- Around line 764-798: When SnapshotTaskMeta (taskscore.SnapshotTaskMeta) fails
and you fall back to m.globalDB.ListTaskItems, don't mask the original metadata
error with a generic message; instead, after calling ListTaskItems and
summarizeTaskRows, if len(taskRows) == 0 return nil, if counts.Total>0 &&
counts.Pending==0 return workflowNoPendingTasksProblem(workflowSlug, counts),
and otherwise return a clearer wrapped error that includes the original
SnapshotTaskMeta error plus DB findings (e.g. include counts.Total and
counts.Pending) so callers see both the metadata inspection failure and what the
DB check returned; update the final return to combine err with DB info rather
than returning the original err alone.

In `@internal/store/globaldb/sync.go`:
- Around line 228-299: PruneMissingActiveWorkflows may mis-report skip reasons
because after calling deleteActiveWorkflowIfNoActiveRuns (which performs an
atomic DELETE guarded by NOT EXISTS active runs and archived_at IS NULL) we
re-count active runs and always set WorkflowPruneSkipped.Reason to
archiveReasonActiveRuns; instead, update the logic in
PruneMissingActiveWorkflows to distinguish why delete failed by (a) checking the
workflow's ArchivedAt/archived flag first (using the workflow record from
ListWorkflows or refetching if needed) and set the skip reason to an "already
archived" reason when archived_at != nil, (b) only call
countActiveRunsForWorkflow to set archiveReasonActiveRuns if the workflow is not
archived, and (c) preserve the existing branch where deleted==true appends to
PrunedSlugs; use the identifiers PruneMissingActiveWorkflows,
deleteActiveWorkflowIfNoActiveRuns, countActiveRunsForWorkflow,
WorkflowPruneSkipped, and archiveReasonActiveRuns to locate and implement this
change.

In `@sdk/extension/internal_test.go`:
- Around line 241-242: Rename the subtest name used in the t.Run call from the
raw hook value to the required "Should..." pattern: replace
t.Run(string(tc.hook), ...) with a name that prefixes the hook with "Should "
(e.g., "Should "+string(tc.hook) or fmt.Sprintf("Should %s", tc.hook)); update
the t.Run invocation that currently wraps t.Parallel() so all subtests follow
the "Should..." naming convention (adjust imports if you choose fmt.Sprintf).

In `@web/src/systems/reviews/components/review-detail-view.tsx`:
- Around line 286-294: The displayReviewTitle function currently removes all
underscores via the .replace(/_/g, "") call which can mangle snake_case
identifiers; update displayReviewTitle to replace underscores with spaces (e.g.,
replace /_/g with " " or a regex that collapses consecutive underscores to a
single space), then collapse multiple whitespace (.replace(/\s+/g, " ")) and
trim as already done so titles like "Fix my_variable_name" become "Fix my
variable name" while preserving spacing and readability.

In `@web/src/systems/reviews/components/review-round-detail-view.test.tsx`:
- Around line 99-108: The test currently calls renderRoundDetail multiple times
causing cumulative DOM; update the test to reuse a single render tree by either
calling cleanup() between renderRoundDetail invocations or by using the rerender
function returned from the initial renderRoundDetail call to update props/state;
modify the "Should render loading, error, empty, and refreshing states" test to
call renderRoundDetail once, then use rerender(...) to switch to { issues: [],
issuesError: "boom" } and then to { issues: [] }, or call cleanup() before each
subsequent renderRoundDetail, ensuring assertions (screen.getByTestId(...)) run
against a single, non-leaking render tree.

In `@web/src/systems/runs/components/runs-list-view.test.tsx`:
- Around line 141-144: Add an assertion to the test that verifies the status
badge does not shrink: locate the status element retrieved by
getByTestId(`runs-list-status-${longRunId}`) in the runs-list-view test and add
an expectation that it includes the "shrink-0" class (e.g.,
expect(screen.getByTestId(`runs-list-status-${longRunId}`)).toHaveClass("shrink-0")).
This will complement the existing truncation and text assertions and guard the
non-shrinking layout behavior.

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

Inline comments:
In `@internal/cli/reviews_exec_daemon_additional_test.go`:
- Around line 498-584: Rename and wrap the standalone test function
TestReviewsWatchCommandBuildsDaemonRequest and all new watch subtests into the
t.Run("Should ...") pattern required by our guidelines: for
TestReviewsWatchCommandBuildsDaemonRequest, wrap its body in t.Run("Should build
daemon request for watch command", func(t *testing.T) { ... }) and similarly
rename and wrap each new subtest (those currently added after the
TestReviewsWatchCommandBuildsDaemonRequest and any subtests referenced between
the old ranges) to use descriptive "Should ..." titles; ensure you keep the
existing assertions and variables (e.g., client.startWatchReq, req, overrides,
batching) unchanged, only move them inside the new t.Run closures and update
subtest names to the "Should..." prefix consistently across all watch-related
tests.

In `@internal/core/reviews/store_test.go`:
- Around line 263-269: Update the two table-driven test case "name" values that
currently read "empty pr field" and "missing pr field" to follow the required
t.Run("Should...") naming convention (e.g., "ShouldAllowEmptyPRField" and
"ShouldAllowMissingPRField") so that the test cases in the slice (the entries
with name: "empty pr field" and name: "missing pr field" in
internal/core/reviews/store_test.go) use the Must-use t.Run("Should...")
pattern.
- Around line 385-406: Convert the test helper reviewIssueContentWithRound to
accept a *testing.T parameter and call t.Helper() at the top of the function so
failures point to the caller; update any test callers that invoke
reviewIssueContentWithRound to pass their *testing.T. Specifically, change the
signature of reviewIssueContentWithRound to include t *testing.T, add t.Helper()
as the first statement inside the function, and update all usages in the tests
to pass the test instance.

In `@internal/core/sync_test.go`:
- Around line 684-686: Replace the broad nil-check on the error returned by
collectReviewRounds with a specific assertion that the error contains a stable
fragment describing the failure (e.g., "invalid review issue"); update the test
so after calling collectReviewRounds(workflowDir) you assert err != nil and that
strings.Contains(err.Error(), "invalid review issue") (or use your test helper
ErrorContains/require.ErrorContains helper) so the test validates the failure
reason rather than just presence of an error.

In `@internal/daemon/review_watch_git_test.go`:
- Around line 86-88: Replace the generic non-nil assertion for the git.Push
failure with a specific error assertion that verifies the expected failure
reason (e.g., use require.ErrorContains or require.ErrorAs / errors.Is to assert
the "missing target" validation message or the concrete error type returned by
git.Push); update the assertion around the git.Push(context.Background(),
"/repo", "", "feature") call to check the specific error text/type and apply the
same tightening to the other similar assertions in this test file (the blocks
that currently only assert err != nil) so tests fail if a different error path
is taken.
- Around line 109-119: The test calls the system git binary via
newExecReviewWatchGit() and git.run(...) which can fail in minimal CI; add a
guard at the start of the test to detect whether "git" exists on PATH (e.g.
using exec.LookPath or equivalent) and call t.Skipf(...) if not found, then
proceed with the existing assertions; ensure the check runs before creating
newExecReviewWatchGit() and invoking git.run so the test is skipped rather than
failing when git is unavailable.
- Around line 11-239: The tests in this file (e.g.,
TestReviewWatchGitStateReadsOnlyRepositoryState,
TestReviewWatchGitPushUsesOnlyAllowedCommandShape,
TestReviewWatchGitPushRejectsMissingTarget,
TestReviewWatchGitPushWrapsCommandFailure,
TestReviewWatchGitCommandRunnerAndParsers,
TestReviewWatchGitStateWithoutUpstreamStillReportsHead,
TestReviewWatchGitStateValidatesRunnerWorkspaceAndRequiredReads,
TestReviewWatchGitPushValidatesRunnerAndWorkspace) must follow the repository
pattern of using t.Run("Should...") for each case; wrap each test body inside a
top-level t.Run call with a descriptive "Should ..." name (keeping t.Parallel()
inside the subtest where needed) or convert the test to call t.Run(...)
immediately after t.Parallel(), ensuring existing assertions and helper setup
(like the execReviewWatchGit run closures and subtests in
TestReviewWatchGitStateValidatesRunnerWorkspaceAndRequiredReads) remain
unchanged except for being enclosed in the t.Run subtest closure so the suite
consistently uses the t.Run("Should...") pattern.

In `@internal/store/globaldb/read_queries_test.go`:
- Around line 235-240: The test is asserting zero counts for
resolved.Workflow.ID but doesn't verify the map actually contains that key, so a
missing entry would falsely pass; update the assertion in read_queries_test.go
to first check presence (e.g., using the comma-ok pattern: _, ok :=
counts[resolved.Workflow.ID]) and fail if !ok, then proceed to validate
counts[resolved.Workflow.ID].Total/Completed/Pending are all zero to ensure the
key exists and values are correct.
- Around line 181-265: Wrap the TestReadQueriesBulkWorkflowSummariesByID test
body in a t.Run subtest using the "Should..." naming pattern (e.g. t.Run("Should
return correct workflow summary eligibility and counts", func(t *testing.T) {
... })); move the existing t.Parallel() and all setup/assertions into that
subtest closure so the test follows the repo's subtest convention while
preserving behavior of functions like openTestGlobalDB, mustWorkspace,
mustReconcileArchiveWorkflow, TaskCountsByWorkflowIDs, and
WorkflowArchiveEligibilityByIDs.

In `@internal/store/globaldb/sync.go`:
- Around line 251-254: Wrap the error returned by g.ListWorkflows with pruning
context before returning so callers retain operation information; replace the
current bare return of err in the WorkflowPruneResult path (call to
g.ListWorkflows with ListWorkflowsOptions{WorkspaceID: trimmedWorkspaceID}) with
a wrapped error using fmt.Errorf, e.g. include text like "failed to list
workflows for prune (workspace=<trimmedWorkspaceID>): %w" to preserve the
original error.

---

Nitpick comments:
In `@internal/core/agent/client_test.go`:
- Around line 67-88: The test currently creates the client with newTestClient(t,
scenario) but calls client.Close() only at the end, so if an assertion fails
earlier the client (and helper process) can leak; immediately register teardown
by calling t.Cleanup(func(){ _ = client.Close() }) right after creating the
client to ensure client.Close() runs on test exit regardless of failures
(reference newTestClient and client.Close in the session test).

In `@internal/core/reviews/store_test.go`:
- Around line 256-271: Add a test case to the existing cases slice in the test
(the table-driven test that defines name, prLine, wantPR, createdAt) that covers
a populated PR value: e.g. add an entry with name "populated pr field", prLine
`pr: "259"`, wantPR `"259"`, and the same createdAt value used by the other
cases (time.Date(2026, 3, 28, 10, 0, 0, 0, time.UTC)). Ensure this new case is
included in the loop that exercises the refresh/persist logic so the assertion
compares the persisted/read PR against wantPR.

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

Inline comments:
In `@internal/core/sync_test.go`:
- Around line 115-182: The test function
TestSyncTaskMetadataRootScanPrunesDeletedWorkflowRows and the other newly added
single-scenario tests must be converted into subtests that follow the repo
pattern by wrapping their assertions in t.Run("Should <describe behavior>",
func(t *testing.T) { ... }), so edit the body of
TestSyncTaskMetadataRootScanPrunesDeletedWorkflowRows (and the other affected
tests at the noted ranges) to move setup/assertion code into a single t.Run call
with a "Should..." description, preserving the existing calls to Sync, sqlDB
queries, and helper funcs (writeSyncWorkflowFile, openSyncSQLite, queryCount)
and returning the same failures using t.Fatalf inside the subtest. Ensure the
top-level Test* function remains and only contains one t.Run subtest per
scenario with the original test logic.

---

Duplicate comments:
In `@internal/core/reviews/store_test.go`:
- Around line 25-42: Rename the test case names in the table entries so they
follow the required t.Run("Should...") pattern: change the `name` values
currently set to "empty pr field" and "missing pr field" to descriptive
Should... phrases (for example `Should handle empty pr field` and `Should handle
missing pr field`) so that t.Run uses the mandated naming convention; update the
`name` fields in the table where these entries appear to the new Should...
strings (the test table entries with `name:` keys).
- Around line 391-412: Change the helper function reviewIssueContentWithRound to
accept t *testing.T and call t.Helper() at the start so failures point to the
caller; update the signature reviewIssueContentWithRound(t *testing.T, status
string, prLine string, createdAt time.Time) and add t.Helper() as the first
statement inside the function, then update all call sites of
reviewIssueContentWithRound(...) to pass the test value (t) as the first
argument.

---

Nitpick comments:
In `@internal/core/sync_test.go`:
- Around line 165-181: Add an assertion that verifies review_issues rows tied to
the pruned beta workflow are deleted: call queryCount(t, sqlDB, "SELECT COUNT(1)
FROM review_issues WHERE workflow_id = ?", betaWorkflowID) and fail the test if
the result is not 0 (similar to the existing checks for task_items and
review_rounds). Place this check alongside the other post-prune assertions so it
uses the same sqlDB, t, and betaWorkflowID variables.