Repository navigation
feat!: migrate to daemon - #112
Merged
Merged
Conversation
Fix the run.post_start observer ordering regression discovered during the daemon QA pass and record the final verification artifacts.
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
⛔ Files ignored due to path filters (6)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a localhost HTTP + UDS daemon API, a matching Go client, SSE run streaming, core transport handlers/middleware/errors, CLI daemon commands and daemon-backed task/exec workflows, conditional update-check suppression, many tests, and supporting utilities and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (tasks run)
participant Bootstrap as Daemon Bootstrap
participant Client as API Client
participant Daemon as Daemon Process
CLI->>Bootstrap: probe daemon health
alt daemon not running
Bootstrap->>Daemon: launch process
Daemon->>Daemon: initialize services
Bootstrap->>Daemon: poll health
Daemon-->>Bootstrap: ready
end
Bootstrap-->>CLI: return API client
CLI->>Client: POST /api/tasks/{slug}/runs
Client->>Daemon: HTTP/UDS request
Daemon-->>Client: run created/accepted
Client-->>CLI: apicore.Run
alt attach=UI
CLI->>Client: GET /api/runs/{id}/snapshot
Client->>Daemon: request
Daemon-->>Client: snapshot
else attach=stream
CLI->>Client: GET /api/runs/{id}/stream (SSE)
Daemon-->>Client: SSE events (heartbeat, events, overflow, completed)
Client-->>CLI: parsed frames
end
sequenceDiagram
participant Transport as HTTP/UDS
participant Middleware as Middleware (RequestID/Error)
participant Handler as Core Handler
participant Service as Backend Service
Transport->>Middleware: incoming request
Middleware->>Handler: request with context (request id)
Handler->>Service: invoke service method
alt success
Service-->>Handler: result
Handler->>Transport: JSON 2xx + X-Request-Id
else error
Service-->>Handler: error
Handler->>Handler: map to Problem / TransportError
Handler->>Transport: JSON error envelope + status
end
opt streaming
Handler->>Transport: PrepareSSE + WriteSSE loop
Service-->>Handler: stream items/errors
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
internal/cli/commands.go (1)
13-13: Remove the ignored dispatcher parameter from these private builders.Each helper now hardwires the daemon runner and never uses
*kernel.Dispatcher. Keeping it in the signature preserves dead API surface and makes the migration look partial. Drop the parameter from theWithDefaultshelpers and from the wrappers that only thread it through.Also applies to: 43-43, 143-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands.go` at line 13, The helper functions like newFetchReviewsCommandWithDefaults currently accept an unused *kernel.Dispatcher parameter; remove that parameter from their signatures (e.g., newFetchReviewsCommandWithDefaults, other private builders at the referenced locations) and update any thin wrapper functions that only threaded the dispatcher through to call the helpers directly without the dispatcher argument; then update all call sites to stop passing a *kernel.Dispatcher. Ensure compiler errors are resolved by renaming and adjusting any wrapper functions that previously forwarded the dispatcher so the daemon runner remains hardwired and no unused parameter remains.internal/api/client/reviews_exec_test.go (1)
217-247: Avoid string-matching the guard errors.These assertions are tied to the exact wording of the error, so a harmless message rewrite will fail the tests even if the behavior is still correct. Exposing sentinel or typed validation errors here would let the tests assert with
errors.Is/errors.Asinstead.As per coding guidelines, "Use
errors.Is()anderrors.As()for error matching; do not compare error strings" and "*_test.go: MUST have specific error assertions (ErrorContains, ErrorAs)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/reviews_exec_test.go` around lines 217 - 247, Replace fragile string-based error assertions with sentinel or typed error checks using errors.Is or errors.As (or test helpers that wrap them) for the calls StartExecRun, FetchReview on nilClient and GetLatestReview, GetReviewRound, ListReviewIssues, StartReviewRun on client; update the code to assert the expected exported sentinel/typed validation errors (e.g., ErrDaemonClientRequired, ErrWorkflowSlugRequired) via errors.Is/require.ErrorIs or errors.As/require.ErrorAs instead of strings.Contains on err.Error(), ensuring tests import and reference the actual sentinel/typed error values returned by the functions.internal/cli/daemon_commands_test.go (1)
414-452: Complex ref-counting lock pattern for global test overrides.This implements a ref-counting scheme to serialize tests that modify global state (
newCLIDaemonBootstrap). The pattern:
- Check if this test already has refs (reentrant case)
- If not, acquire the global mutex
cliTestGlobalOverrideMu- Track refs per test name
- Release global mutex only when last ref is released
The nested locking is correct but complex. Consider adding a brief comment explaining why ref-counting is needed (likely for subtests that install multiple overrides).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/daemon_commands_test.go` around lines 414 - 452, Add a short explanatory comment above acquireCLITestGlobalOverride and releaseCLITestGlobalOverride that states why the ref-counting and the cliTestGlobalOverrideMu global mutex are required (to serialize tests that modify global state like newCLIDaemonBootstrap and to allow reentrant calls from subtests that need multiple overrides), and describe the lifecycle: first caller acquires the global mutex, subsequent reentrant callers just increment refs, and the mutex is unlocked only when the last ref is released; reference the function names acquireCLITestGlobalOverride, releaseCLITestGlobalOverride, cliTestGlobalOverrides.refs, and cliTestGlobalOverrideMu so future readers understand the locking/ref-counting coordination.internal/api/core/internal_helpers_test.go (1)
16-138: Comprehensive but monolithic test for Problem and error helpers.This test covers many aspects of the error handling system:
- Problem.Error() formatting and fallbacks
- errors.Is() wrapping behavior
- Nil-pointer safety
- Status/code/message mapping for various error types
- Helper constructors (invalidJSONProblem, serviceUnavailableProblem)
- Schema error details extraction
- Context cancellation detection
Consider splitting into focused subtests using
t.Runto improve test failure diagnostics and align with the table-driven test pattern guideline.As per coding guidelines: "Table-driven tests with subtests (
t.Run) as the default pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/internal_helpers_test.go` around lines 16 - 138, Split the monolithic TestProblemAndHelperFunctions into focused subtests using t.Run (e.g., "ErrorFormatting", "NilSafety", "StatusCodeMapping", "ConstructorHelpers", "SchemaDetails", "ContextCancellation") and/or table-driven subtests; move each assertion group that references symbols like NewProblem, (*Problem).Error, errors.Is, statusForError, defaultCodeForStatus, messageForError, invalidJSONProblem, serviceUnavailableProblem, codeForError, detailsForError, and requestCanceled into its own t.Run so failures are isolated and use table-driven loops where multiple inputs are tested (e.g., defaultCodeForStatus and schema errors) while keeping existing assertions and expected values unchanged.internal/api/core/sse_test.go (1)
37-66: Missingt.Parallel()for test independence.This test function doesn't call
t.Parallel(), unlike the other tests in this file. Adding it would allow this test to run concurrently with others.As per coding guidelines: "Use
t.Parallel()for independent subtests".♻️ Proposed fix
func TestWriteSSEFormatsFramesWithCanonicalCursor(t *testing.T) { + t.Parallel() + timestamp := time.Date(2026, 4, 17, 12, 0, 0, 123456789, time.UTC)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/sse_test.go` around lines 37 - 66, Add t.Parallel() at the start of the TestWriteSSEFormatsFramesWithCanonicalCursor test to make it run concurrently with other independent tests; open the function TestWriteSSEFormatsFramesWithCanonicalCursor and insert a t.Parallel() call immediately after the func declaration (before using timestamp, cursor, writer or calling core.WriteSSE) so the test remains independent of other tests that exercise core.WriteSSE and flushBuffer.internal/api/httpapi/transport_integration_test.go (1)
487-539: Rename these subtests to the requiredShould...form.
"invalid"and"stale"make failures harder to scan, and they don't match the repo's required subtest pattern.Suggested fix
{ - name: "invalid", + name: "Should reject an invalid cursor", lastEvent: "bad", wantCode: http.StatusUnprocessableEntity, wantReason: "invalid_cursor", }, { - name: "stale", + name: "Should reject a stale cursor", lastEvent: staleCursor, wantCode: http.StatusUnprocessableEntity, wantReason: "stale_cursor", },As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/transport_integration_test.go` around lines 487 - 539, Rename the subtest names in the testCases slice from "invalid" and "stale" to the required t.Run "Should..." pattern so failures are easier to scan; update the name fields in the testCases declaration (used by the t.Run(...) call) to descriptive names such as "ShouldReturnUnprocessableEntityForInvalidCursor" and "ShouldReturnUnprocessableEntityForStaleCursor" (or any "Should..." phrasing that describes the expectation) while leaving the rest of the test logic (request creation, header Last-Event-ID, assertions on response.StatusCode and payload.Code) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/compozy/main.go`:
- Around line 67-77: The current loop in shouldStartUpdateCheck returns true on
the first non-flag token, which wrongly opts in for cases like "compozy tasks
help"; instead, iterate through all non-flag tokens (the args -> value loop) and
track whether any non-flag token exists, while immediately returning false if
any token matches the help/version/completion whitelist ("help", "version",
"completion", "__complete", "__completeNoDesc"); after the loop, return true
only if at least one non-flag token was seen and none were in the whitelist.
This keeps the whitelist check authoritative and delays opting in until the
entire command path is scanned.
In `@internal/api/client/client.go`:
- Around line 150-167: The slug variable is not URL-escaped before being
embedded into the request path, so pass the slug through url.PathEscape and use
that escaped value when building path (replace the concatenation that creates
path := "/api/tasks/"+slug+"/runs" with a version that uses
url.PathEscape(slug)); ensure the net/url import is present and then call
c.doJSON as before with the escaped path so requests with slashes, spaces, or
special chars route correctly.
In `@internal/api/client/runs.go`:
- Around line 452-458: sendItem currently treats a full s.items channel as a
terminal error which kills the SSE session; instead, make sendItem wait for
space and only return an error when the stream is actually canceled. Replace the
non-blocking send in sendItem with a blocking select that sends item to s.items
or returns only if the stream context/channel is closed (e.g. check s.ctx.Done()
or the stream's close channel), so normal backpressure blocks the sender rather
than terminating the stream; keep references to sendItem, s.items and the read()
consumer so the change preserves expected lifetime semantics.
In `@internal/api/core/handlers_error_paths_test.go`:
- Around line 15-17: The test TestSharedHandlersValidationAndServiceErrors calls
gin.SetMode which mutates global Gin state; add a t.Cleanup that captures the
current mode via old := gin.Mode() before calling gin.SetMode(gin.TestMode) and
restore it in cleanup with gin.SetMode(old) so subsequent tests are not affected
(reference gin.SetMode, gin.Mode, and
TestSharedHandlersValidationAndServiceErrors).
In `@internal/api/core/handlers_test.go`:
- Around line 107-115: The test currently uses time.Sleep inside the goroutine
that sends a core.RunStreamOverflow to stream.events, which races with the
heartbeat; replace the sleep with an explicit synchronization primitive: add a
channel or sync.WaitGroup (e.g., readyCh) that the main test signals once the
heartbeat has been processed, then have the goroutine block on that signal
before sending the overflow frame to stream.events and closing
stream.events/stream.errors; update the test to wait for the ready signal rather
than sleeping so newFakeRunStream, stream.events, and the overflow send are
deterministically ordered.
In `@internal/api/core/handlers.go`:
- Around line 768-775: The handlers currently accept any positive integer from
parsePositiveInt into the local variable limit; clamp limit to a safe maximum
before calling the run service (e.g., define a MAX_LIMIT constant and set limit
= min(limit, MAX_LIMIT) or return an error if limit > MAX_LIMIT), and apply the
same change in both ListRuns and ListRunEvents so oversized requests are
prevented; update the code paths that call the run service with this clamped
limit (reference symbols: parsePositiveInt, limit variable, ListRuns,
ListRunEvents, and the run service call).
- Around line 630-644: The Fetch review handler currently forwards body.Round
unchecked into h.Reviews.Fetch; add a validation after requireWorkspaceRef and
before constructing ReviewFetchRequest that rejects non-positive rounds
(body.Round <= 0) and returns an HTTP 400 Bad Request (use the existing error
helper such as h.errorBadRequest or the project's standard response helper) with
a clear message like "round must be a positive integer"; keep existing flow
using reviewFetchBody, h.bindJSON, requireWorkspaceRef, and ReviewFetchRequest
and only proceed to call h.Reviews.Fetch when the round is valid.
- Around line 883-885: The defer currently swallows the error from
stream.Close(); change it to capture and log the error with the run id instead
of assigning to `_`. Replace the defer func that calls `_ = stream.Close()` with
code like: defer func() { if err := stream.Close(); err != nil { /* use the
existing logger in scope (e.g., logger/processLogger) */
logger.Errorf("stream.Close failed for run %s: %v", runID, err) } }() so the
Close error for stream.Close() is not ignored and includes the run id for
diagnostics.
In `@internal/api/httpapi/server.go`:
- Around line 212-214: The shared mutation of core.Handlers via
s.handlers.SetStreamDone in the HTTP server Start is unsafe because
internal/api/udsapi/server.go also writes the same streamDone slot; change Start
so it does not mutate a shared handlers instance. Fix by giving each transport
its own handler instance or by adding transport-scoped stream cancellation
(e.g., create a clone/copy of core.Handlers for HTTP before calling
SetStreamDone, or add a per-transport SetStreamDone variant such as
handlers.ForTransport("http").SetStreamDone), and leave SetHTTPPort usage as-is;
update the HTTP server Start (where s.mu.Lock() and s.handlers.SetStreamDone are
called) and the UDS Start in internal/api/udsapi/server.go to use
transport-specific handler instances or APIs instead of writing the same shared
slot.
- Around line 179-184: The Start method currently checks s.started while holding
s.mu but releases the lock before setting it, allowing two concurrent Start
calls to both proceed; fix by setting s.started = true while still holding s.mu
(i.e., inside the s.mu.Lock() / Unlock() section) so the second caller
immediately sees started; also ensure that on every subsequent error path (any
failure before launching the serve goroutine that sets
s.httpServer/s.listener/s.serveDone) you perform the same rollback (clear
s.started and any partially set fields) while holding s.mu so the server state
remains consistent and Shutdown will clean up correctly; reference s.mu,
s.started, s.httpServer, s.listener, s.serveDone and the serve goroutine launch
when applying the fix.
In `@internal/api/udsapi/server.go`:
- Around line 139-144: The start routine currently checks s.started under s.mu,
unlocks, then proceeds to create listeners allowing a race where two callers
both create listeners; fix by holding s.mu, set s.started = true before
performing the listen/accept setup (so the first caller wins), then release the
lock and continue creating the listener. Update the code around the
s.mu/s.started checks in server.start (or the method containing the shown
snippet) to set s.started = true while still holding s.mu, and only then
unlock—keeping the same error return path if already started.
- Around line 67-73: The constructor New currently calls
compozyconfig.ResolveHomePaths() before applying provided options, causing
New(WithSocketPath(...)) to fail when ResolveHomePaths() errors; change New to
first instantiate Server (zero-value), apply all Option functions (e.g.,
WithSocketPath) to that server, and only call ResolveHomePaths() as a fallback
when server.socketPath is still empty; update references in New, Option
handling, and Server.socketPath accordingly so explicit options override the
home-path resolution.
In `@internal/cli/archive_command_integration_test.go`:
- Around line 12-13: The test currently only sets HOME using homeDir :=
newShortCLITestHomeDir(t) and t.Setenv("HOME", homeDir); update it to fully
isolate environment like the daemon-backed tests: create an xdgConfigHome (e.g.
via t.TempDir()), then call t.Setenv("XDG_CONFIG_HOME", xdgConfigHome) and also
set testCLIDaemonHomeEnv to homeDir and testCLIXDGHomeEnv to xdgConfigHome
(using t.Setenv) so operator commands running behind the daemon use the isolated
HOME/XDG and test-specific envs.
In `@internal/cli/commands_simple.go`:
- Around line 316-322: The current logic silently drops errors from
absoluteWorkflowPath when s.rootDir is provided, which can hide
misconfiguration; update the block that sets archiveRootBase (using
model.TasksBaseDirForWorkspace, s.workspaceRoot, s.rootDir) to surface failures
from absoluteWorkflowPath: either return the error up to the caller or log a
clear warning including the failed s.rootDir and the error message so users see
invalid path problems instead of silently falling back to the default; ensure
you modify the surrounding function signature/return path if you choose to
propagate the error.
In `@internal/cli/daemon_exec_test_helpers_test.go`:
- Around line 253-317: The bridge goroutine in newInProcessClientRunStream can
block forever on the unbuffered items channel when consumers exit early; add a
shutdown signal by adding a done channel to the inProcessClientRunStream struct,
have the goroutine include a case <-done in its select loop to break and stop
forwarding, and update Close() on inProcessClientRunStream to close or signal
the done channel before calling the upstream stream.Close so the bridge exits
immediately and does not leak; make sure items and errors channels are still
closed by the goroutine when shutting down.
In `@internal/cli/daemon.go`:
- Around line 48-54: The RunE handler currently creates a context with
signal.NotifyContext(context.Background(), ...), losing any
cancellation/deadline from Cobra; replace that logic by calling the existing
helper signalCommandContext(cmd) to derive the context (i.e. use ctx :=
signalCommandContext(cmd) and defer any stop returned by that helper if
applicable) and pass that ctx into daemon.Run in RunE so the command respects
injected contexts and tests; remove the manual signal.NotifyContext usage and
keep the daemon.Run invocation and RunOptions.Version as-is.
- Around line 100-123: The probe and daemon RPC calls in daemonStatusState.run
and daemonStopState.run use context.Background(), which prevents signal
propagation; replace each context.Background() passed to daemon.QueryStatus,
client.DaemonStatus, client.Health and client.StopDaemon with the command
context (cmd.Context()) so cancellation and Ctrl+C propagate correctly — locate
the calls in the methods daemonStatusState.run and daemonStopState.run and swap
the context argument to cmd.Context().
---
Nitpick comments:
In `@internal/api/client/reviews_exec_test.go`:
- Around line 217-247: Replace fragile string-based error assertions with
sentinel or typed error checks using errors.Is or errors.As (or test helpers
that wrap them) for the calls StartExecRun, FetchReview on nilClient and
GetLatestReview, GetReviewRound, ListReviewIssues, StartReviewRun on client;
update the code to assert the expected exported sentinel/typed validation errors
(e.g., ErrDaemonClientRequired, ErrWorkflowSlugRequired) via
errors.Is/require.ErrorIs or errors.As/require.ErrorAs instead of
strings.Contains on err.Error(), ensuring tests import and reference the actual
sentinel/typed error values returned by the functions.
In `@internal/api/core/internal_helpers_test.go`:
- Around line 16-138: Split the monolithic TestProblemAndHelperFunctions into
focused subtests using t.Run (e.g., "ErrorFormatting", "NilSafety",
"StatusCodeMapping", "ConstructorHelpers", "SchemaDetails",
"ContextCancellation") and/or table-driven subtests; move each assertion group
that references symbols like NewProblem, (*Problem).Error, errors.Is,
statusForError, defaultCodeForStatus, messageForError, invalidJSONProblem,
serviceUnavailableProblem, codeForError, detailsForError, and requestCanceled
into its own t.Run so failures are isolated and use table-driven loops where
multiple inputs are tested (e.g., defaultCodeForStatus and schema errors) while
keeping existing assertions and expected values unchanged.
In `@internal/api/core/sse_test.go`:
- Around line 37-66: Add t.Parallel() at the start of the
TestWriteSSEFormatsFramesWithCanonicalCursor test to make it run concurrently
with other independent tests; open the function
TestWriteSSEFormatsFramesWithCanonicalCursor and insert a t.Parallel() call
immediately after the func declaration (before using timestamp, cursor, writer
or calling core.WriteSSE) so the test remains independent of other tests that
exercise core.WriteSSE and flushBuffer.
In `@internal/api/httpapi/transport_integration_test.go`:
- Around line 487-539: Rename the subtest names in the testCases slice from
"invalid" and "stale" to the required t.Run "Should..." pattern so failures are
easier to scan; update the name fields in the testCases declaration (used by the
t.Run(...) call) to descriptive names such as
"ShouldReturnUnprocessableEntityForInvalidCursor" and
"ShouldReturnUnprocessableEntityForStaleCursor" (or any "Should..." phrasing
that describes the expectation) while leaving the rest of the test logic
(request creation, header Last-Event-ID, assertions on response.StatusCode and
payload.Code) unchanged.
In `@internal/cli/commands.go`:
- Line 13: The helper functions like newFetchReviewsCommandWithDefaults
currently accept an unused *kernel.Dispatcher parameter; remove that parameter
from their signatures (e.g., newFetchReviewsCommandWithDefaults, other private
builders at the referenced locations) and update any thin wrapper functions that
only threaded the dispatcher through to call the helpers directly without the
dispatcher argument; then update all call sites to stop passing a
*kernel.Dispatcher. Ensure compiler errors are resolved by renaming and
adjusting any wrapper functions that previously forwarded the dispatcher so the
daemon runner remains hardwired and no unused parameter remains.
In `@internal/cli/daemon_commands_test.go`:
- Around line 414-452: Add a short explanatory comment above
acquireCLITestGlobalOverride and releaseCLITestGlobalOverride that states why
the ref-counting and the cliTestGlobalOverrideMu global mutex are required (to
serialize tests that modify global state like newCLIDaemonBootstrap and to allow
reentrant calls from subtests that need multiple overrides), and describe the
lifecycle: first caller acquires the global mutex, subsequent reentrant callers
just increment refs, and the mutex is unlocked only when the last ref is
released; reference the function names acquireCLITestGlobalOverride,
releaseCLITestGlobalOverride, cliTestGlobalOverrides.refs, and
cliTestGlobalOverrideMu so future readers understand the locking/ref-counting
coordination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7f41a6e-1480-45ad-afdb-35c5bc944895
⛔ Files ignored due to path filters (179)
.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.codex/ledger/2026-04-17-MEMORY-active-run-watchers.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-archive-db-state.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-cli-daemon-client.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-daemon-architecture.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-daemon-qa-tasks.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-daemon-run-manager.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-extension-runtime-daemon.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-global-db-registry.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-shared-transport-core.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-sync-persistence-rewrite.mdis excluded by!**/*.md.codex/ledger/2026-04-17-MEMORY-tui-remote-attach.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-daemon-command-surface.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-daemon-memory-research.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-daemon-perf.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-daemon-qa-execution.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-daemon-qa-plan.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-regression-cleanup.mdis excluded by!**/*.md.codex/ledger/2026-04-18-MEMORY-reviews-exec-migration.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/_prompt.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/qa/issue_001.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/qa/issue_002.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/qa/issue_003.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/qa/issue_004.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/_archived/ext-imp/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/_archived/tasks-props/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/daemon/_meta.mdis excluded by!**/*.md.compozy/tasks/daemon/_tasks.mdis excluded by!**/*.md.compozy/tasks/daemon/_techspec.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_11.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_12.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_13.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_14.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_15.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_16.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_17.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_18.mdis excluded by!**/*.md.compozy/tasks/daemon/memory/task_19.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/SUMMARY.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_cli_startup.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_daemon_runtime.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_event_journal.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_run_executor.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_storage_sqlite.mdis excluded by!**/*.md.compozy/tasks/daemon/perf_analysis/analysis_tui_rendering.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/issues/BUG-001.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/logs/baseline-make-verify.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/final-make-verify-clean.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/final-make-verify.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/full-support-api-core.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/full-support-core.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/full-support-daemon.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/full-support-runs-stream.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/perf-build.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/postclean-tc-func-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/postclean-tc-int-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/postgate-tc-func-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/postgate-tc-func-006.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/postgate-tc-int-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-001.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-003.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-004.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-005.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-func-006.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-int-001.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-int-002.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-int-003.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-perf-001-bench.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tc-perf-001-hyperfine.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tui-executor.logis excluded by!**/*.log.compozy/tasks/daemon/qa/logs/tui-remote.logis excluded by!**/*.log.compozy/tasks/daemon/qa/test-cases/TC-FUNC-001.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-FUNC-002.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-FUNC-003.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-FUNC-004.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-FUNC-005.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-FUNC-006.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-INT-001.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-INT-002.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-INT-003.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-PERF-001.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-cases/TC-UI-001.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-plans/daemon-regression.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/test-plans/daemon-test-plan.mdis excluded by!**/*.md.compozy/tasks/daemon/qa/verification-report.mdis excluded by!**/*.md.compozy/tasks/daemon/task_01.mdis excluded by!**/*.md.compozy/tasks/daemon/task_02.mdis excluded by!**/*.md.compozy/tasks/daemon/task_03.mdis excluded by!**/*.md.compozy/tasks/daemon/task_04.mdis excluded by!**/*.md.compozy/tasks/daemon/task_05.mdis excluded by!**/*.md.compozy/tasks/daemon/task_06.mdis excluded by!**/*.md.compozy/tasks/daemon/task_07.mdis excluded by!**/*.md.compozy/tasks/daemon/task_08.mdis excluded by!**/*.md.compozy/tasks/daemon/task_09.mdis excluded by!**/*.md.compozy/tasks/daemon/task_10.mdis excluded by!**/*.md.compozy/tasks/daemon/task_11.mdis excluded by!**/*.md.compozy/tasks/daemon/task_12.mdis excluded by!**/*.md.compozy/tasks/daemon/task_13.mdis excluded by!**/*.md.compozy/tasks/daemon/task_14.mdis excluded by!**/*.md.compozy/tasks/daemon/task_15.mdis excluded by!**/*.md.compozy/tasks/daemon/task_16.mdis excluded by!**/*.md.compozy/tasks/daemon/task_17.mdis excluded by!**/*.md.compozy/tasks/daemon/task_18.mdis excluded by!**/*.md.compozy/tasks/daemon/task_19.mdis excluded by!**/*.mdREADME.mdis excluded by!**/*.mddocs/events.mdis excluded by!**/*.mddocs/extensibility/architecture.mdis excluded by!**/*.mddocs/extensibility/getting-started.mdis excluded by!**/*.mddocs/extensibility/hello-world-go.mdis excluded by!**/*.mddocs/extensibility/hello-world-ts.mdis excluded by!**/*.mddocs/extensibility/host-api-reference.mdis excluded by!**/*.mddocs/extensibility/trust-and-enablement.mdis excluded by!**/*.mddocs/reader-library.mdis excluded by!**/*.mdgo.sumis excluded by!**/*.sum
📒 Files selected for processing (121)
.compozy/tasks/daemon/qa/issues/.gitkeep.compozy/tasks/daemon/qa/screenshots/.gitkeepcmd/compozy/main.gocmd/compozy/main_test.gogo.modinternal/api/client/client.gointernal/api/client/operator.gointernal/api/client/reviews_exec.gointernal/api/client/reviews_exec_test.gointernal/api/client/runs.gointernal/api/core/errors.gointernal/api/core/handlers.gointernal/api/core/handlers_error_paths_test.gointernal/api/core/handlers_service_errors_test.gointernal/api/core/handlers_smoke_test.gointernal/api/core/handlers_test.gointernal/api/core/interfaces.gointernal/api/core/internal_helpers_test.gointernal/api/core/middleware.gointernal/api/core/routes.gointernal/api/core/sse.gointernal/api/core/sse_test.gointernal/api/httpapi/routes.gointernal/api/httpapi/server.gointernal/api/httpapi/transport_integration_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/server.gointernal/cli/agents_commands_test.gointernal/cli/archive_command_integration_test.gointernal/cli/commands.gointernal/cli/commands_simple.gointernal/cli/commands_test.gointernal/cli/daemon.gointernal/cli/daemon_commands.gointernal/cli/daemon_commands_test.gointernal/cli/daemon_exec_test_helpers_test.gointernal/cli/daemon_launch_other.gointernal/cli/daemon_launch_unix.gointernal/cli/dispatch_adapters.gointernal/cli/extensions_bootstrap_test.gointernal/cli/form_test.gointernal/cli/kernel_dispatch_test.gointernal/cli/operator_commands_integration_test.gointernal/cli/operator_output.gointernal/cli/reusable_agents_doc_examples_test.gointernal/cli/reviews_exec_daemon.gointernal/cli/reviews_exec_daemon_additional_test.gointernal/cli/root.gointernal/cli/root_command_execution_test.gointernal/cli/root_test.gointernal/cli/run.gointernal/cli/run_observe.gointernal/cli/runs.gointernal/cli/skills_preflight_test.gointernal/cli/state.gointernal/cli/testdata/exec_help.goldeninternal/cli/testdata/tasks_run_help.goldeninternal/cli/validate_tasks_test.gointernal/cli/workspace_commands.gointernal/cli/workspace_config.gointernal/cli/workspace_config_test.gointernal/config/home.gointernal/config/home_test.gointernal/core/archive.gointernal/core/archive_test.gointernal/core/extension/audit.gointernal/core/extension/daemon_bridge.gointernal/core/extension/dispatcher.gointernal/core/extension/dispatcher_test.gointernal/core/extension/host_api_errors.gointernal/core/extension/host_helpers.gointernal/core/extension/host_helpers_test.gointernal/core/extension/host_writes.gointernal/core/extension/host_writes_test.gointernal/core/extension/manager_shutdown.gointernal/core/extension/manager_spawn.gointernal/core/extension/manager_test.gointernal/core/extension/review_provider_runtime.gointernal/core/extension/review_provider_runtime_test.gointernal/core/extension/runtime.gointernal/core/extension/testdata/mock_extension/main.gointernal/core/fetch.gointernal/core/kernel/commands/workflow_sync.gointernal/core/kernel/handlers.gointernal/core/kernel/run_scope_integration_test.gointernal/core/migration/migrate_test.gointernal/core/model/artifacts.gointernal/core/model/model_test.gointernal/core/model/run_scope.gointernal/core/model/runtime_config.gointernal/core/model/workflow_ops.gointernal/core/model/workspace_paths.gointernal/core/plan/prepare.gointernal/core/plan/prepare_test.gointernal/core/provider/overlay.gointernal/core/reviews/store.gointernal/core/reviews/store_test.gointernal/core/run/exec/exec.gointernal/core/run/exec/exec_integration_test.gointernal/core/run/exec/exec_test.gointernal/core/run/executor/event_stream.gointernal/core/run/executor/execution.gointernal/core/run/executor/execution_acp_integration_test.gointernal/core/run/executor/execution_test.gointernal/core/run/executor/lifecycle.gointernal/core/run/executor/review_hooks.gointernal/core/run/internal/acpshared/command_io.gointernal/core/run/internal/acpshared/command_io_test.gointernal/core/run/internal/acpshared/session_handler.gointernal/core/run/internal/acpshared/session_handler_test.gointernal/core/run/internal/runshared/config.gointernal/core/run/internal/runtimeevents/events.gointernal/core/run/journal/journal.gointernal/core/run/journal/journal_test.gointernal/core/run/transcript/model.gointernal/core/run/transcript/model_test.gointernal/core/run/ui/adapter_test.gointernal/core/run/ui/model.gointernal/core/run/ui/remote.gointernal/core/run/ui/remote_test.gointernal/core/run/ui/sidebar.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/api/client/client.go (2)
246-249: Add a comment justifying the ignored error fromresponse.Body.Close().Per coding guidelines, errors should not be ignored with
_without written justification. While ignoringClose()errors on fully-read response bodies is common practice (the body is already consumed and errors are rarely actionable), the guidelines require an explicit comment.♻️ Suggested fix
defer func() { - _ = response.Body.Close() + // Body is fully consumed; Close errors are not actionable. + _ = response.Body.Close() }()As per coding guidelines, "NEVER ignore errors with
_— every error must be handled or have a written justification".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/client.go` around lines 246 - 249, The deferred anonymous function currently discards the error from response.Body.Close() (defer func() { _ = response.Body.Close() }()), which violates the "never ignore errors" guideline; update that deferred close in client.go (the defer around response.Body.Close()) to either handle/log the error or add a concise inline comment justifying why it’s safe to ignore (e.g., the response body is fully consumed and Close errors are not actionable in this context), e.g., replace the blank assignment with a brief comment explaining the rationale or call processLogger.Debug/Warning to record non-fatal close errors.
284-290: Consider using exact path match for health endpoint special-casing.
strings.HasSuffix(path, "/daemon/health")would also match unexpected paths like/api/v2/daemon/healthor/api/foo/daemon/health. An exact match is safer for this special-case handling.♻️ Suggested fix
if statusCode == http.StatusServiceUnavailable && - strings.HasSuffix(path, "/daemon/health") && + path == "/api/daemon/health" && responseBody != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client/client.go` around lines 284 - 290, The health-endpoint special-case currently uses strings.HasSuffix(path, "/daemon/health") which can match unintended routes; update the conditional in the function handling the HTTP response to require an exact path match (e.g., path == "/daemon/health") when checking statusCode == http.StatusServiceUnavailable and responseBody != nil, then proceed to json.Unmarshal(payload, responseBody) as before so only the intended /daemon/health endpoint triggers the special-case.internal/api/core/internal_helpers_test.go (2)
149-202: Refactor to uset.Run("Should...")subtests for better isolation and clarity.This test covers multiple distinct behaviors (transport name, time sourcing, stream-done channel, Clone, SetHTTPPort) in a single flat function without subtests. Per coding guidelines, all test cases must use the
t.Run("Should...")pattern for organization andt.Parallel()for independent subtests.♻️ Suggested structure
func TestHandlerInternalHelpers(t *testing.T) { - handlers := NewHandlers(&HandlerConfig{}) - if got := handlers.transportName(); got != "api" { - t.Fatalf("transportName() = %q, want api", got) - } - // ... rest of flat test ... + t.Parallel() + + t.Run("Should return default transport name when empty", func(t *testing.T) { + t.Parallel() + handlers := NewHandlers(&HandlerConfig{}) + if got := handlers.transportName(); got != "api" { + t.Fatalf("transportName() = %q, want api", got) + } + }) + + t.Run("Should return configured transport name when set", func(t *testing.T) { + t.Parallel() + handlers := NewHandlers(&HandlerConfig{}) + handlers.TransportName = "http" + if got := handlers.transportName(); got != "http" { + t.Fatalf("transportName(non-empty) = %q, want http", got) + } + }) + + // ... continue for other behaviors: now(), SetStreamDone, Clone, SetHTTPPort ... }As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases" and "Use
t.Parallel()for independent subtests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/internal_helpers_test.go` around lines 149 - 202, The TestHandlerInternalHelpers function bundles many independent checks; refactor it into t.Run subtests (use names like t.Run("Should return default transport name", ...), "Should respect TransportName override", "Should return non-zero time from now", "Should handle nil Now", "Should set and return stream done channel", "Should clone handlers and provide independent stream channel", "Should no-op on nil handlers streamDoneChannel", "Should set HTTP port and reject invalid sets") and call t.Parallel() inside each subtest where safe; keep the same assertions and use the existing symbols (NewHandlers, handlers.transportName, handlers.Now/now, handlers.SetStreamDone/streamDoneChannel, handlers.Clone, handlers.SetHTTPPort/httpPort.Load, nilHandlers.SetHTTPPort) so behavior remains identical but tests are isolated and parallelizable.
246-248: The loop variable capturett := ttis no longer needed in Go 1.22+.Go 1.22 changed loop variable semantics so each iteration gets its own copy. This pattern is now unnecessary but harmless.
♻️ Suggested cleanup
for _, tt := range testCases { - tt := tt t.Run(tt.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/internal_helpers_test.go` around lines 246 - 248, Remove the redundant loop-variable shadowing inside the test loop: delete the line "tt := tt" so the for-range variable tt is used directly in the t.Run closure (the t.Run call and testCases loop remain unchanged); this relies on Go 1.22+ per-iteration variable semantics and simplifies the test in internal_helpers_test.go by eliminating unnecessary shadowing of tt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/client/client.go`:
- Around line 246-249: The deferred anonymous function currently discards the
error from response.Body.Close() (defer func() { _ = response.Body.Close() }()),
which violates the "never ignore errors" guideline; update that deferred close
in client.go (the defer around response.Body.Close()) to either handle/log the
error or add a concise inline comment justifying why it’s safe to ignore (e.g.,
the response body is fully consumed and Close errors are not actionable in this
context), e.g., replace the blank assignment with a brief comment explaining the
rationale or call processLogger.Debug/Warning to record non-fatal close errors.
- Around line 284-290: The health-endpoint special-case currently uses
strings.HasSuffix(path, "/daemon/health") which can match unintended routes;
update the conditional in the function handling the HTTP response to require an
exact path match (e.g., path == "/daemon/health") when checking statusCode ==
http.StatusServiceUnavailable and responseBody != nil, then proceed to
json.Unmarshal(payload, responseBody) as before so only the intended
/daemon/health endpoint triggers the special-case.
In `@internal/api/core/internal_helpers_test.go`:
- Around line 149-202: The TestHandlerInternalHelpers function bundles many
independent checks; refactor it into t.Run subtests (use names like
t.Run("Should return default transport name", ...), "Should respect
TransportName override", "Should return non-zero time from now", "Should handle
nil Now", "Should set and return stream done channel", "Should clone handlers
and provide independent stream channel", "Should no-op on nil handlers
streamDoneChannel", "Should set HTTP port and reject invalid sets") and call
t.Parallel() inside each subtest where safe; keep the same assertions and use
the existing symbols (NewHandlers, handlers.transportName, handlers.Now/now,
handlers.SetStreamDone/streamDoneChannel, handlers.Clone,
handlers.SetHTTPPort/httpPort.Load, nilHandlers.SetHTTPPort) so behavior remains
identical but tests are isolated and parallelizable.
- Around line 246-248: Remove the redundant loop-variable shadowing inside the
test loop: delete the line "tt := tt" so the for-range variable tt is used
directly in the t.Run closure (the t.Run call and testCases loop remain
unchanged); this relies on Go 1.22+ per-iteration variable semantics and
simplifies the test in internal_helpers_test.go by eliminating unnecessary
shadowing of tt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b13f40c7-5687-4e43-b4b8-4fe53c4d8530
⛔ Files ignored due to path filters (10)
.compozy/tasks/daemon/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/daemon/reviews-002/issue_009.mdis excluded by!**/*.md
📒 Files selected for processing (10)
cmd/compozy/main.gocmd/compozy/main_test.gointernal/api/client/client.gointernal/api/client/reviews_exec_test.gointernal/api/core/handlers_test.gointernal/api/core/internal_helpers_test.gointernal/api/core/sse.gointernal/api/core/sse_test.gointernal/cli/daemon.gointernal/cli/daemon_commands_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/api/core/handlers_test.go
- internal/api/client/reviews_exec_test.go
- internal/api/core/sse_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/compozy/main_test.go
- cmd/compozy/main.go
This was referenced Apr 18, 2026
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 21, 2026
Closed
Closed
This was referenced Apr 21, 2026
Merged
This was referenced Apr 21, 2026
Closed
Closed
Closed
Merged
Merged
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores