feat: add channels - #14

internal/api/udsapi/udsapi_integration_test.go (1)

316-338: Add context-wrapped errors in lifecycle helper methods.

Line 316 through Line 338 return raw errors from UpdateInstanceState, which makes failures harder to diagnose in integration runs.

♻️ Suggested change

 func (s *integrationChannelService) StartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	return s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
 		ID:      id,
 		Enabled: true,
 		Status:  channelspkg.ChannelStatusStarting,
 	})
+	if err != nil {
+		return nil, fmt.Errorf("start channel instance %q: %w", id, err)
+	}
+	return instance, nil
 }
 
 func (s *integrationChannelService) StopInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	return s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
 		ID:      id,
 		Enabled: false,
 		Status:  channelspkg.ChannelStatusDisabled,
 	})
+	if err != nil {
+		return nil, fmt.Errorf("stop channel instance %q: %w", id, err)
+	}
+	return instance, nil
 }
 
 func (s *integrationChannelService) RestartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	return s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
 		ID:      id,
 		Enabled: true,
 		Status:  channelspkg.ChannelStatusStarting,
 	})
+	if err != nil {
+		return nil, fmt.Errorf("restart channel instance %q: %w", id, err)
+	}
+	return instance, nil
 }

As per coding guidelines, "Use explicit error returns with wrapped context: fmt.Errorf("context: %w", err)".

🤖 Prompt for AI Agents

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

In `@internal/api/udsapi/udsapi_integration_test.go` around lines 316 - 338, The
StartInstance, StopInstance, and RestartInstance helpers currently return raw
errors from UpdateInstanceState; update each method (StartInstance,
StopInstance, RestartInstance) to check the error returned by
UpdateInstanceState and return a wrapped error using fmt.Errorf with contextual
text (e.g., "start instance %s: %w", "stop instance %s: %w", "restart instance
%s: %w") so failures include operation and instance id context while preserving
the original error.

internal/api/httpapi/channels_test.go (1)

13-131: Consolidate these handler tests into table-driven t.Run("Should...") cases.

Line 13 through Line 131 repeats the same setup in three standalone tests; moving to table-driven subtests will reduce duplication and align with the repository test conventions.

As per coding guidelines, "Use table-driven tests with subtests (t.Run) as default" and "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/channels_test.go` around lines 13 - 131, Combine the
three standalone tests TestCreateChannelHandlerCreatesChannelInstance,
TestListChannelRoutesHandlerReturnsRequestedRouteSet, and
TestChannelTestDeliveryHandlerResolvesTypedTarget into a single table-driven
test (e.g., TestChannelsHandlers) that uses t.Run("Should ...") subtests; create
a slice of cases each with a name and per-case stubChannelService behavior
(CreateInstanceFn, ListRoutesFn, ResolveDeliveryTargetFn) and expected
request/response checks, reuse common setup like homePaths and engine created
with newTestHandlersWithChannels, and for each case call performRequest and
decodeJSONResponse then assert expectations — this consolidates duplicated setup
and follows the repository t.Run pattern.

internal/api/udsapi/channels_integration_test.go (1)

77-88: Consider logging or handling the Close error.

Per coding guidelines, errors should not be ignored with _. While ignoring body.Close() errors is common in test cleanup (where recovery isn't meaningful), you could log it for visibility:

♻️ Suggested alternative

 func mustReadAll(t *testing.T, body io.ReadCloser) string {
 	t.Helper()
-	defer func() {
-		_ = body.Close()
-	}()
+	defer body.Close()

 	data, err := io.ReadAll(body)

Or if you want to surface close errors in tests:

+	t.Cleanup(func() {
+		if err := body.Close(); err != nil {
+			t.Logf("body.Close() error = %v", err)
+		}
+	})

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/udsapi/channels_integration_test.go` around lines 77 - 88, The
helper mustReadAll currently ignores the error from body.Close(); change the
defer to capture and handle the Close error (do not assign to `_`): in the defer
in mustReadAll, call err := body.Close() and, if non-nil, report it via the
testing.T (e.g., t.Logf or t.Errorf) so the test surfaces cleanup failures;
ensure the defer still runs t.Helper() context and returns the read string as
before.

internal/extension/channel_delivery_integration_test.go (1)

305-305: Minor: Redundant string cast.

The cast string("channel.adapter") is unnecessary since the literal is already a string.

♻️ Suggested fix

-		capabilities: []string{string("channel.adapter")},
+		capabilities: []string{"channel.adapter"},

🤖 Prompt for AI Agents

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

In `@internal/extension/channel_delivery_integration_test.go` at line 305, The
capabilities slice contains a redundant cast string("channel.adapter"); remove
the unnecessary conversion and use the string literal "channel.adapter" directly
in the capabilities field (the slice assigned to capabilities) to simplify the
code.

internal/channels/target_integration_test.go (1)

107-112: Redundant assertion can be removed.

The assertion at lines 110-112 is logically redundant. You already assert at lines 107-109 that workspaceTarget.GroupID is empty, and at lines 95-97 that globalTarget.GroupID is "global-group". If both assertions pass, they can never be equal, making the final comparison unnecessary.

♻️ Suggested simplification

 	if workspaceTarget.GroupID != "" {
 		t.Fatalf("workspaceTarget.GroupID = %q, want empty", workspaceTarget.GroupID)
 	}
-	if workspaceTarget.GroupID == globalTarget.GroupID {
-		t.Fatalf("workspaceTarget.GroupID = %q, want different value than global target", workspaceTarget.GroupID)
-	}
 	if workspaceTarget.Mode != channelspkg.DeliveryModeReply {

🤖 Prompt for AI Agents

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

In `@internal/channels/target_integration_test.go` around lines 107 - 112, The
second check comparing workspaceTarget.GroupID to globalTarget.GroupID is
redundant because workspaceTarget.GroupID is already asserted to be empty and
globalTarget.GroupID is asserted earlier to be "global-group"; remove the entire
if block that checks "if workspaceTarget.GroupID == globalTarget.GroupID {
t.Fatalf(...)}" so only the emptiness assertion for workspaceTarget.GroupID
remains (referencing workspaceTarget.GroupID and globalTarget.GroupID to locate
the code).

internal/daemon/daemon.go (1)

108-114: Type assertion silently returns nil for non-implementing services.

The pattern is safe since callers handle nil, but consider adding a comment explaining this intentional behavior for maintainability:

 func channelObserveSource(service core.ChannelService) observe.ChannelSource {
 	if service == nil {
 		return nil
 	}
+	// Returns nil if service doesn't implement ChannelSource - callers must handle nil.
 	source, _ := service.(observe.ChannelSource)
 	return source
 }

🤖 Prompt for AI Agents

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

In `@internal/daemon/daemon.go` around lines 108 - 114, The type assertion in
channelObserveSource intentionally ignores the boolean ok and returns nil when
the provided core.ChannelService does not implement observe.ChannelSource; add a
clear comment above the channelObserveSource function explaining that this
silent-nil behavior is deliberate (callers expect and handle nil) and why we
avoid returning an error or panicking, referencing the types core.ChannelService
and observe.ChannelSource so future maintainers understand the rationale.

internal/api/core/channels.go (3)

156-156: Missing whitespace normalization on path parameter.

Same inconsistency—c.Param("id") should be trimmed.

♻️ Suggested fix

-	targetReq, err := req.ToResolveDeliveryTargetRequest(c.Param("id"))
+	targetReq, err := req.ToResolveDeliveryTargetRequest(strings.TrimSpace(c.Param("id")))

🤖 Prompt for AI Agents

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

In `@internal/api/core/channels.go` at line 156, The call passing the raw path
param into ToResolveDeliveryTargetRequest should normalize whitespace first;
instead of passing c.Param("id") directly to req.ToResolveDeliveryTargetRequest,
trim the parameter (e.g., via strings.TrimSpace) and pass the trimmed value so
targetReq, err := req.ToResolveDeliveryTargetRequest(...) receives a normalized
id; update the code around the targetReq creation to use the trimmed id and
import strings if needed.

---

96-96: Missing whitespace normalization on path parameter.

Same inconsistency as line 74—c.Param("id") should be trimmed for consistency with other handlers.

♻️ Suggested fix

-	updateReq, err := req.ToUpdateInstanceRequest(c.Param("id"))
+	updateReq, err := req.ToUpdateInstanceRequest(strings.TrimSpace(c.Param("id")))

🤖 Prompt for AI Agents

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

In `@internal/api/core/channels.go` at line 96, Trim whitespace from the path
parameter before passing it to the request builder: replace the direct
c.Param("id") usage with strings.TrimSpace(c.Param("id")) when calling
req.ToUpdateInstanceRequest so updateReq is created from a normalized id (ensure
strings is imported if not present); this mirrors the same normalization applied
elsewhere (e.g., the earlier handler using c.Param("id") at line 74) and
prevents inconsistent ID handling.

---

74-74: Inconsistent whitespace handling for path parameter.

c.Param("id") is not trimmed here, but lines 133, 196, 208, and 220 use strings.TrimSpace(c.Param("id")). This inconsistency could cause lookup mismatches if IDs contain surrounding whitespace.

♻️ Suggested fix

-	instance, err := channels.GetInstance(c.Request.Context(), strings.TrimSpace(c.Param("id")))
+	instance, err := channels.GetInstance(c.Request.Context(), strings.TrimSpace(c.Param("id")))

Apply the same pattern to lines 96 and 156 as well.

🤖 Prompt for AI Agents

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

In `@internal/api/core/channels.go` at line 74, The code calls
channels.GetInstance with c.Param("id") without trimming; make it consistent by
wrapping the path param with strings.TrimSpace(c.Param("id")) wherever you pass
the ID to lookups (e.g., replace the call to
channels.GetInstance(c.Request.Context(), c.Param("id")) with
channels.GetInstance(c.Request.Context(), strings.TrimSpace(c.Param("id"))) and
apply the same change for the other occurrences flagged (the calls around the
code handling the same ID at the other two locations), ensuring all lookups use
strings.TrimSpace on c.Param("id").

internal/api/core/channels_test.go (3)

108-110: Remove dead code assertion.

The handlers variable is always non-nil since newChannelHandlerFixture returns a valid *core.BaseHandlers. This check provides no value.

🧹 Suggested removal

-	if handlers == nil {
-		t.Fatal("expected handlers")
-	}

🤖 Prompt for AI Agents

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

In `@internal/api/core/channels_test.go` around lines 108 - 110, Remove the
redundant nil-check assertion for handlers in channels_test.go: delete the if
block that tests "if handlers == nil { t.Fatal(\"expected handlers\") }" because
newChannelHandlerFixture always returns a valid *core.BaseHandlers; update or
re-run the test to ensure no other code depends on that assertion and rely on
direct use of handlers (symbol: handlers, function: newChannelHandlerFixture,
type: *core.BaseHandlers).

---

128-145: Wrap table cases in t.Run for better isolation and debugging.

The table-driven test iterates directly without using t.Run. This makes it harder to identify which case failed and prevents running subtests independently.

♻️ Suggested improvement

 	for _, tc := range []struct {
 		path   string
 		status channelspkg.ChannelStatus
 	}{
 		{path: "/channels/chan-core/enable", status: channelspkg.ChannelStatusStarting},
 		{path: "/channels/chan-core/disable", status: channelspkg.ChannelStatusDisabled},
 		{path: "/channels/chan-core/restart", status: channelspkg.ChannelStatusStarting},
 	} {
+		t.Run("Should transition status via "+tc.path, func(t *testing.T) {
 		resp := performRequest(t, engine, http.MethodPost, tc.path, nil)
 		if resp.Code != http.StatusOK {
 			t.Fatalf("%s status = %d body=%s", tc.path, resp.Code, resp.Body.String())
 		}
 		var payload contract.ChannelResponse
 		testutil.DecodeJSONResponse(t, resp, &payload)
 		if payload.Channel.Status != tc.status {
 			t.Fatalf("%s status payload = %q, want %q", tc.path, payload.Channel.Status, tc.status)
 		}
+		})
 	}

As per coding guidelines: "Use table-driven tests with subtests (t.Run) as default" and "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/core/channels_test.go` around lines 128 - 145, The table-driven
loop in channels_test.go should wrap each case in t.Run to isolate failures and
allow subtest execution: inside the for loop, call t.Run with a descriptive name
like "Should <action> <path>" (e.g., using tc.path or the expected status) and
move the test body into that closure; also shadow the loop variable (add tc :=
tc at the start of the closure) to avoid capture issues, then call
performRequest(t, engine, http.MethodPost, tc.path, nil), decode into
contract.ChannelResponse with testutil.DecodeJSONResponse, and assert
payload.Channel.Status == tc.status as before. Ensure each subtest uses the same
assertions but is contained within the t.Run closure so failures report the
specific case.

---

147-149: Remove dead code assertion.

Same issue as above — handlers is always non-nil from the fixture.

🧹 Suggested removal

-	if handlers == nil {
-		t.Fatal("expected handlers")
-	}

🤖 Prompt for AI Agents

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

In `@internal/api/core/channels_test.go` around lines 147 - 149, The assertion
that checks `if handlers == nil { t.Fatal("expected handlers") }` is dead code
because the test fixture always returns a non-nil `handlers`; remove that `if`
block from the test in channels_test.go (the check referencing the `handlers`
variable) so the test no longer contains the redundant nil-guard and relies on
the fixture directly.

internal/extension/host_api_channels.go (1)

223-248: Consider logging cleanup failures or returning early on non-critical path.

The maybeCleanupChannelIngestDedup function returns an error on cleanup failure, which would fail the entire ingest operation. Since this is a periodic maintenance task, consider whether cleanup failures should be logged but not block message ingestion.

♻️ Consider logging instead of failing

 	if err := retrySQLiteBusy(ctx, hostAPIBusyRetryAttempts, func() error {
 		_, err := h.dedupStore.DeleteExpiredChannelIngestDedup(ctx, now)
 		return err
 	}); err != nil {
-		return fmt.Errorf("extension: cleanup expired channel ingest dedup: %w", err)
+		// Log but don't fail ingestion for cleanup errors
+		if h.logger != nil {
+			h.logger.Warn("cleanup expired channel ingest dedup failed", "error", err)
+		}
 	}
 	return nil

🤖 Prompt for AI Agents

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

In `@internal/extension/host_api_channels.go` around lines 223 - 248, The cleanup
in maybeCleanupChannelIngestDedup currently returns an error when
retrySQLiteBusy(...) fails, which can bubble up and block ingestion; change the
behavior to treat cleanup as best-effort: when retrySQLiteBusy (and the
underlying h.dedupStore.DeleteExpiredChannelIngestDedup) returns an error, log
the failure via the appropriate logger (e.g., processLogger or h.logger) with
context (including the error and now value) and return nil so ingestion is not
blocked; keep the existing locking and shouldRun logic and only change the error
handling around the retrySQLiteBusy call and its fmt.Errorf wrapping.

internal/extension/channel_delivery_notifier_test.go (1)

362-372: Consider using a channel-based wait instead of polling.

The waitForExtensionDeliveryCalls uses time.Sleep polling. While acceptable in tests, a channel-based notification would be more responsive and eliminate arbitrary sleep durations.

This is a minor optimization since the current implementation works correctly for test purposes.

🤖 Prompt for AI Agents

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

In `@internal/extension/channel_delivery_notifier_test.go` around lines 362 - 372,
Replace the polling loop in waitForExtensionDeliveryCalls with a channel-based
wait: add a notification channel field (e.g., deliveryCh) to
recordingDeliveryTransport and have the transport send a signal on that channel
whenever its call list grows (the code path that currently updates
snapshotCalls). Then change waitForExtensionDeliveryCalls to wait on that
channel with a timeout (using select and time.After) and check
transport.snapshotCalls() when woken; return when count >= want or fail on
timeout. Update recordingDeliveryTransport constructors/teardown to
initialize/close the channel as needed.

internal/daemon/channels_test.go (1)

31-60: Consider adding t.Parallel() for independent tests.

None of the tests in this file use t.Parallel(). Since each test appears to create its own isolated test fixtures (homePaths, config, DB), they could potentially run in parallel to speed up test execution.

As per coding guidelines: "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/daemon/channels_test.go` around lines 31 - 60, Add t.Parallel() as
the first statement inside
TestComposeChannelRuntimeBuildsOnlyWhenRegistrySupportsChannelPersistence to
allow it to run concurrently; ensure the test setup functions you call
(testHomePaths, testConfig, newTestDaemon, openDaemonTestGlobalDB) already
produce isolated fixtures or use t.TempDir/t.Cleanup so there is no shared state
when running in parallel, and keep the rest of the test (composeChannelRuntime
checks and bootState/registry manipulation) unchanged.

internal/extension/host_api.go (2)

768-768: Use h.now() instead of time.Now() for testability.

The handler has an injectable now function but this line uses time.Now() directly, making deadline behavior harder to test.

-	deadline := time.Now().Add(seedPollWindow)
+	deadline := h.now().Add(seedPollWindow)

🤖 Prompt for AI Agents

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

In `@internal/extension/host_api.go` at line 768, Replace the direct call to
time.Now() when computing deadline with the handler's injectable clock by using
h.now(): change the line that sets deadline := time.Now().Add(seedPollWindow) to
use h.now() so deadline := h.now().Add(seedPollWindow) (keep seedPollWindow and
the deadline variable intact) to make timing behavior testable via the handler's
now() function.

---

763-766: Consider making seed poll configuration injectable.

The hardcoded 5ms poll interval and 30ms window are quite aggressive. While functional, exposing these as options (similar to channelIngestDedupTTL) would improve testability and allow tuning in production.

🤖 Prompt for AI Agents

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

In `@internal/extension/host_api.go` around lines 763 - 766, The hardcoded
constants seedPollInterval and seedPollWindow are too aggressive and should be
made configurable like channelIngestDedupTTL: add configurable fields (e.g.,
SeedPollInterval, SeedPollWindow) to the same config/options struct used for
channelIngestDedupTTL, wire those fields into the code paths that currently
reference seedPollInterval and seedPollWindow (replace uses of the constants
with the configured values, falling back to 5ms/30ms defaults), and update any
constructors/new functions that create the host API to accept or populate these
options so tests and production can override them.

internal/api/testutil/apitest.go (1)

185-298: Add a compile-time assertion for StubChannelService.

This file already pins the other stubs to their interfaces, but the new channel stub is missing the same guard. Adding var _ core.ChannelService = (*StubChannelService)(nil) near the existing assertions will make interface drift fail fast.

As per coding guidelines, "Use compile-time interface verification: var _ Interface = (*Type)(nil)".

🤖 Prompt for AI Agents

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

In `@internal/api/testutil/apitest.go` around lines 185 - 298, Add a compile-time
interface assertion to ensure StubChannelService implements the
core.ChannelService interface: declare a var assignment using
core.ChannelService and (*StubChannelService)(nil) near the other stub
assertions (e.g., alongside existing var _ ... = (*...)(nil) checks) so any
future interface drift fails at compile time.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/extension/channel_delivery_notifier.go (1)

79-89: ⚠️ Potential issue | 🟠 Major

Handle or justify fingerprint marshal failures explicitly.

transcript.MarshalAgentEvent(event) failures are currently swallowed. Please either log the error (best-effort path) or add a short inline justification comment for why this failure is safe to ignore.

🔧 Suggested fix

-func projectionEventFromAgentEvent(event acp.AgentEvent) channelspkg.DeliveryProjectionEvent {
+func projectionEventFromAgentEvent(ctx context.Context, event acp.AgentEvent) channelspkg.DeliveryProjectionEvent {
 	projected := channelspkg.DeliveryProjectionEvent{
 		Type:      event.Type,
 		TurnID:    event.TurnID,
 		Timestamp: event.Timestamp,
 		Text:      event.Text,
 		Error:     event.Error,
 	}
-	if fingerprint, err := transcript.MarshalAgentEvent(event); err == nil {
-		projected.Fingerprint = fingerprint
-	}
+	fingerprint, err := transcript.MarshalAgentEvent(event)
+	if err != nil {
+		slog.WarnContext(ctx, "extension: failed to fingerprint agent event",
+			"event_type", event.Type,
+			"turn_id", event.TurnID,
+			"error", err,
+		)
+		return projected
+	}
+	projected.Fingerprint = fingerprint
 	return projected
 }

-			if err := n.broker.ProjectEvent(ctx, sessionID, projectionEventFromAgentEvent(event)); err != nil {
+			if err := n.broker.ProjectEvent(ctx, sessionID, projectionEventFromAgentEvent(ctx, event)); err != nil {

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/extension/channel_delivery_notifier.go` around lines 79 - 89, In
projectionEventFromAgentEvent, do not silently ignore errors from
transcript.MarshalAgentEvent(event); instead handle the error by logging it via
the appropriate logger (or return/propagate if applicable) or add a concise
inline comment justifying why a marshal failure is safe to ignore; update the
block around transcript.MarshalAgentEvent to capture the error variable and
either call the logger (e.g., processLogger/errorLogger) with context including
event.Type/TurnID or add a one-line comment referencing why ignoring the error
is acceptable for DeliveryProjectionEvent fingerprinting.

internal/api/udsapi/udsapi_integration_test.go (2)

316-364: Deduplicate StartInstance and RestartInstance transition flow.

Both methods run the same Starting → Ready sequence. A shared helper would reduce maintenance drift.

Refactor sketch

+func (s *integrationChannelService) transitionToReady(ctx context.Context, id, action string) (*channelspkg.ChannelInstance, error) {
+	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+		ID:      id,
+		Enabled: true,
+		Status:  channelspkg.ChannelStatusStarting,
+	}); err != nil {
+		return nil, fmt.Errorf("%s channel instance %q: %w", action, id, err)
+	}
+	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+		ID:      id,
+		Enabled: true,
+		Status:  channelspkg.ChannelStatusReady,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("mark %s channel instance %q ready: %w", action, id, err)
+	}
+	return instance, nil
+}
+
 func (s *integrationChannelService) StartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusStarting,
-	}); err != nil {
-		return nil, fmt.Errorf("start channel instance %q: %w", id, err)
-	}
-	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusReady,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("mark channel instance %q ready: %w", id, err)
-	}
-	return instance, nil
+	return s.transitionToReady(ctx, id, "start")
 }
 
 func (s *integrationChannelService) RestartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusStarting,
-	}); err != nil {
-		return nil, fmt.Errorf("restart channel instance %q: %w", id, err)
-	}
-	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusReady,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("mark restarted channel instance %q ready: %w", id, err)
-	}
-	return instance, nil
+	return s.transitionToReady(ctx, id, "restart")
 }

🤖 Prompt for AI Agents

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

In `@internal/api/udsapi/udsapi_integration_test.go` around lines 316 - 364,
Extract the duplicate Starting→Ready sequence from StartInstance and
RestartInstance into a small helper (e.g., ensureInstanceStartsAndReady or
transitionInstanceToReady) that takes ctx and id, calls UpdateInstanceState
twice (first with Status: channelspkg.ChannelStatusStarting, Enabled: true and
then with Status: channelspkg.ChannelStatusReady, Enabled: true), and returns
the final *channelspkg.ChannelInstance and error; then have StartInstance and
RestartInstance call that helper and preserve the existing wrapped error
messages (use the helper's returned error to wrap with the same contextual
strings you currently use in StartInstance and RestartInstance), leaving
StopInstance unchanged.

---

308-314: Add an explicit compile-time interface assertion for the channel wrapper.

integrationChannelService is now a contract boundary for WithChannelService; adding var _ <interface> = (*integrationChannelService)(nil) here makes drift fail at compile time.

As per coding guidelines, "Use compile-time interface verification: var _ Interface = (*Type)(nil)".

🤖 Prompt for AI Agents

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

In `@internal/api/udsapi/udsapi_integration_test.go` around lines 308 - 314, Add a
compile-time interface assertion so the compiler will catch drift: in the same
file declare a verification line asserting integrationChannelService implements
the interface expected by WithChannelService (i.e., add a statement of the form
var _ <InterfaceName> = (*integrationChannelService)(nil) using the interface
type that WithChannelService requires from channelspkg). This ensures
integrationChannelService and the WithChannelService contract stay in sync.

internal/daemon/daemon_test.go (2)

3239-3252: Silent error handling in test utility.

markerLineCount returns 0 on os.ReadFile error without logging or indication. While acceptable for test helper code (treating missing/unreadable files as empty), consider adding a brief comment explaining this intentional behavior for future maintainers.

🤖 Prompt for AI Agents

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

In `@internal/daemon/daemon_test.go` around lines 3239 - 3252, The helper function
markerLineCount silently returns 0 on os.ReadFile errors without explanation;
add a brief comment above markerLineCount stating that returning 0 on read
errors is intentional for tests (treating missing/unreadable files as empty) so
future maintainers understand the silence, and mention any cases where this
behavior would be unacceptable; keep the function logic unchanged but document
the rationale.

---

3126-3140: Fire-and-forget goroutine may cause race with shutdown.

The goroutines spawned by sendDelayedDeliveryResult are not tracked. When releaseSlowDeliveries is called during shutdown, pending goroutines unblock but there's no wait for them to complete sending results before the shutdown response is sent and the process potentially exits.

This could cause test flakiness if tests expect all delivery acks to arrive before the shutdown ack.

♻️ Consider tracking pending deliveries with WaitGroup

 type daemonExtensionHelperServer struct {
 	scenario                string
 	marker                  string
 	scanner                 *bufio.Scanner
 	encoder                 *json.Encoder
 	slowDeliveryRelease     chan struct{}
 	slowDeliveryReleaseOnce sync.Once
+	pendingDeliveries       sync.WaitGroup
 	mu                      sync.Mutex
 }

 func (h *daemonExtensionHelperServer) sendDelayedDeliveryResult(id any, ack channelspkg.DeliveryAck) {
+	h.pendingDeliveries.Add(1)
 	go func() {
+		defer h.pendingDeliveries.Done()
 		<-h.slowDeliveryRelease
 		if err := h.sendResult(id, ack); err != nil {
 			fmt.Fprintln(os.Stderr, err)
 			os.Exit(1)
 		}
 	}()
 }

 func (h *daemonExtensionHelperServer) releaseSlowDeliveries() {
 	h.slowDeliveryReleaseOnce.Do(func() {
 		close(h.slowDeliveryRelease)
 	})
+	h.pendingDeliveries.Wait()
 }

As per coding guidelines, "**/*.go: No fire-and-forget goroutines — track with sync.WaitGroup or equivalent`".

🤖 Prompt for AI Agents

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

In `@internal/daemon/daemon_test.go` around lines 3126 - 3140,
sendDelayedDeliveryResult spawns untracked goroutines which can race with
shutdown; add a sync.WaitGroup field (e.g., pendingDeliveries) to the helper,
call pendingDeliveries.Add(1) before starting the goroutine in
sendDelayedDeliveryResult, ensure the goroutine calls pendingDeliveries.Done()
after sendResult returns (including on error), and change releaseSlowDeliveries
to close(slowDeliveryRelease) and then wait for pendingDeliveries.Wait()
(keeping slowDeliveryReleaseOnce). This ensures all delayed sendResult calls
complete before shutdown proceeds.

internal/extension/host_api.go (1)

722-722: Consider tracking the drain goroutine or documenting the justification.

go drainAgentEvents(eventsCh) is a fire-and-forget goroutine. While this pattern is common for preventing channel blocking, it technically violates the guideline requiring goroutine tracking.

The goroutine will terminate when the channel closes (after session completion), so it's bounded. However, if many prompts are submitted rapidly before completions, multiple drain goroutines could accumulate.

Consider either:

1. Adding a brief comment justifying why tracking isn't needed here
2. Using a bounded worker pool or tracking mechanism if this is a hot path

As per coding guidelines: "No fire-and-forget goroutines — track with sync.WaitGroup or equivalent".

🤖 Prompt for AI Agents

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

In `@internal/extension/host_api.go` at line 722, The call to start
drainAgentEvents as a fire-and-forget goroutine (go drainAgentEvents(eventsCh))
can create untracked goroutines; either document why this is safe (the goroutine
is bounded and exits on eventsCh close) or convert to tracked work: pass a
context or sync.WaitGroup into drainAgentEvents and register the goroutine
(wg.Add(1)/wg.Done()) or run it from a small fixed worker pool to avoid
accumulation; update the invocation site where go drainAgentEvents(eventsCh) is
called and the drainAgentEvents signature to accept the tracking primitive
(context or *sync.WaitGroup) and ensure the goroutine exits cleanly when the
session ends.

internal/channels/registry.go (1)

392-407: ListRoutes returns routes without cloning, unlike ListInstances.

ListInstances deep-clones each returned instance (lines 196-200), but ListRoutes returns the store's slice directly. If ChannelRoute contains mutable fields (e.g., JSON data, metadata maps), callers could mutate shared state.

♻️ Suggested fix for consistency

 func (s *Service) ListRoutes(ctx context.Context, channelInstanceID string) ([]ChannelRoute, error) {
 	if err := s.checkReady(ctx, "list channel routes"); err != nil {
 		return nil, err
 	}

 	trimmedInstanceID := strings.TrimSpace(channelInstanceID)
 	if _, err := s.store.GetChannelInstance(ctx, trimmedInstanceID); err != nil {
 		return nil, fmt.Errorf("channels: list channel routes for %q: load channel instance: %w", trimmedInstanceID, err)
 	}
 	routes, err := s.store.ListChannelRoutes(ctx, trimmedInstanceID)
 	if err != nil {
 		return nil, fmt.Errorf("channels: list channel routes for %q: %w", trimmedInstanceID, err)
 	}
-	return routes, nil
+	if len(routes) == 0 {
+		return routes, nil
+	}
+	cloned := make([]ChannelRoute, 0, len(routes))
+	for _, route := range routes {
+		cloned = append(cloned, *cloneChannelRoute(route))
+	}
+	return cloned, nil
 }

Additionally, verify whether cloneChannelRoute needs to deep-copy any mutable fields (similar to how cloneChannelInstance copies DeliveryDefaults).

🤖 Prompt for AI Agents

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

In `@internal/channels/registry.go` around lines 392 - 407, ListRoutes currently
returns the store slice directly, risking shared-mutable state unlike
ListInstances which deep-clones each instance; update Service.ListRoutes to
iterate over routes returned by s.store.ListChannelRoutes and append cloned
copies (use or add a cloneChannelRoute function) before returning, mirroring the
pattern used in ListInstances (lines around cloneChannelInstance usage). Ensure
cloneChannelRoute performs deep copies of any mutable fields (maps, slices, and
JSON/encoding/json.RawMessage or similar fields, and DeliveryDefaults if
present) so callers cannot mutate underlying store data.

internal/channels/delivery_projection_test.go (1)

366-460: Split this validation matrix into table-driven subtests.

This single test mixes snapshot validation, request validation, normalization, and metadata helpers, so one failure hides the rest and makes the broken path harder to spot. A small table with t.Run("Should...") subtests would fit the rest of this suite much better. As per coding guidelines, "Use table-driven tests with subtests (t.Run) as default" and "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/channels/delivery_projection_test.go` around lines 366 - 460, Split
TestDeliveryValidationAndMetadataHelpers into table-driven subtests using
t.Run("Should ...") so each validation path is isolated: create separate
subtests for snapshot validation (exercising DeliverySnapshot.Validate and cases
like lastAcked>lastSent and mismatched DeliveryID), request validation
(DeliveryRequest.Validate covering resume-with-snapshot,
resume-without-snapshot, and mismatched-snapshot cases), normalization
(DeliveryProjectionEvent.normalize trimming fields), and metadata helpers
(deliveryMetadataJSON and deliveryErrorText behaviors). For each case either
build a test-case struct slice and iterate with t.Run or create individual t.Run
blocks that assert the same expectations currently in the test, keeping the
existing helper values (snapshot, resumeEvent, badSnapshot) and using
descriptive "Should ..." names per coding guidelines.

internal/channels/delivery_broker_test.go (1)

556-579: Replace sleep-based polling with explicit synchronization.

These helpers spin on time.Sleep, which makes the async broker tests timing-sensitive under CI load. Prefer signaling from the fake transport or another synchronization primitive instead of fixed 10ms polling loops. As per coding guidelines, "Never use time.Sleep() in orchestration — use proper synchronization primitives".

🤖 Prompt for AI Agents

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

In `@internal/channels/delivery_broker_test.go` around lines 556 - 579, The
helpers waitForCalls and waitForCondition use time.Sleep polling which makes
tests flaky; update them to use explicit synchronization: add a notification
mechanism on fakeDeliveryTransport (e.g., a callsChanged channel or cond var
inside fakeDeliveryTransport referenced by snapshotCalls) and have waitForCalls
block waiting on that channel/cond with a timeout instead of sleeping, waking
and rechecking when the fake transport signals a new call; similarly change
waitForCondition to accept a context or a notify channel and wait on that
channel (or the context deadline) to detect the condition becoming true; ensure
fakeDeliveryTransport signals after mutating its internal call slice so tests
unblock deterministically.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/api/udsapi/udsapi_integration_test.go (1)

319-367: Optional DRY refactor: consolidate duplicated start/restart transition flow.

StartInstance and RestartInstance share the same two-step transition logic. Extracting a helper would reduce duplication and keep error wording consistent.

♻️ Suggested refactor

+func (s *integrationChannelService) transitionToReady(ctx context.Context, id, op string) (*channelspkg.ChannelInstance, error) {
+	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+		ID:      id,
+		Enabled: true,
+		Status:  channelspkg.ChannelStatusStarting,
+	}); err != nil {
+		return nil, fmt.Errorf("%s channel instance %q: %w", op, id, err)
+	}
+	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
+		ID:      id,
+		Enabled: true,
+		Status:  channelspkg.ChannelStatusReady,
+	})
+	if err != nil {
+		return nil, fmt.Errorf("mark %s channel instance %q ready: %w", op, id, err)
+	}
+	return instance, nil
+}
+
 func (s *integrationChannelService) StartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusStarting,
-	}); err != nil {
-		return nil, fmt.Errorf("start channel instance %q: %w", id, err)
-	}
-	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusReady,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("mark channel instance %q ready: %w", id, err)
-	}
-	return instance, nil
+	return s.transitionToReady(ctx, id, "start")
 }
 
 func (s *integrationChannelService) RestartInstance(ctx context.Context, id string) (*channelspkg.ChannelInstance, error) {
-	if _, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusStarting,
-	}); err != nil {
-		return nil, fmt.Errorf("restart channel instance %q: %w", id, err)
-	}
-	instance, err := s.UpdateInstanceState(ctx, channelspkg.UpdateInstanceStateRequest{
-		ID:      id,
-		Enabled: true,
-		Status:  channelspkg.ChannelStatusReady,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("mark restarted channel instance %q ready: %w", id, err)
-	}
-	return instance, nil
+	return s.transitionToReady(ctx, id, "restart")
 }

🤖 Prompt for AI Agents

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

In `@internal/api/udsapi/udsapi_integration_test.go` around lines 319 - 367,
StartInstance and RestartInstance duplicate the same two-step state transitions
via UpdateInstanceState; extract a small helper (e.g., performStartTransition or
setInstanceStartingThenReady) that accepts (ctx context.Context, id string) and
runs the two UpdateInstanceState calls (Status: ChannelStatusStarting then
ChannelStatusReady, Enabled: true), returns (*channelspkg.ChannelInstance,
error), and use that helper from both StartInstance and RestartInstance so the
flow and error messages are consolidated and consistent.

internal/channels/delivery_projection_test.go (1)

675-681: Replace sleep-based polling in waitForSnapshot.

This helper busy-waits with time.Sleep(10 * time.Millisecond), which tends to make parallel test runs slower and more flaky in CI. A notification-driven wait, or an existing eventual helper from internal/testutil, would be more reliable here.

As per coding guidelines, "Never use time.Sleep() in orchestration — use proper synchronization primitives" and "Use shared test helpers from internal/testutil and internal/api/testutil".

🤖 Prompt for AI Agents

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

In `@internal/channels/delivery_projection_test.go` around lines 675 - 681, The
busy-wait loop in waitForSnapshot that polls broker.Snapshot(...) with
time.Sleep should be replaced with the shared eventual helper from
internal/testutil (e.g., testutil.Eventually) to avoid sleep-based polling;
implement testutil.Eventually(t, timeout, interval, func() bool { snapshot, err
:= broker.Snapshot(testutil.Context(t), deliveryID); if err != nil { return
false }; return match(snapshot) }) and capture/return the snapshot via an outer
variable so waitForSnapshot still returns the found snapshot, or alternatively
use a notification-based wait from the broker if available; update references in
waitForSnapshot to use testutil.Context(t), deliveryID, match and ensure the
helper drives the timeout instead of time.Sleep.

internal/daemon/channels_test.go (1)

31-407: Convert this suite to t.Run("Should...") table-driven subtests.

The coverage is solid, but the file currently encodes each scenario as a separate top-level test. That diverges from the repo’s default test structure and makes the repeated setup harder to extend consistently.

As per coding guidelines, **/*_test.go: Use table-driven tests with subtests (t.Run) as default and 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/daemon/channels_test.go` around lines 31 - 407, The file contains
many top-level tests (e.g.,
TestComposeChannelRuntimeBuildsOnlyWhenRegistrySupportsChannelPersistence,
TestWithChannelSecretResolverAppliesOption,
TestBootExtensionsInjectsChannelRuntimeDeps,
TestChannelRuntimeStartInstanceTransitionsDisabledInstance,
TestChannelRuntimeCreateEnabledInstanceReloadsExtensions,
TestChannelRuntimeResolveChannelRuntimeResolvesBoundSecrets,
TestChannelRuntimeResolveChannelRuntimeRequiresSecretResolverWhenBindingsExist,
TestChannelRuntimeStopInstanceBlocksIngressPreservesRoutes,
TestChannelRuntimeRestartPreservesRoutesAndReloadsExtensions) that must be
converted into a single table-driven suite using t.Run("Should...") subtests:
extract the common setup
(homePaths/testConfig/newTestDaemon/openDaemonTestGlobalDB/newChannelRuntime/recording
resolvers/etc.) into shared setup code and create a slice of cases each with a
Name (starting with "Should...") and a function that executes the original test
body; in the parent Test function loop over cases and call t.Run(case.Name,
func(t *testing.T){ t.Parallel(); case.Run(t) }); for tests that previously used
independent top-level t.Parallel, move that into each subtest; preserve all
unique calls and assertions to functions/methods like composeChannelRuntime,
WithChannelSecretResolver, bootExtensions, StartInstance, CreateInstance,
ResolveChannelRuntime, PutChannelSecretBinding, GetInstance, StopInstance,
ListRoutes, ResolveRoute, RestartInstance and ensure extension mocks
(fakeExtensionRuntime), resolvers (recordingChannelSecretResolver), and DB
helpers (openDaemonTestGlobalDB, mustCreateDaemonChannelInstance,
mustUpsertDaemonChannelRoute) are initialized per-case or reused safely to avoid
test interference.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/daemon/channels_test.go (1)

30-586: Reshape this new suite into t.Run("Should...") cases.

The coverage is useful, but this file mostly uses one top-level test per scenario instead of the repo's default subtest style. Grouping related cases into table-driven t.Run("Should...") blocks will reduce setup duplication and align the suite with the project's test conventions.

As per coding guidelines, **/*_test.go: Use table-driven tests with subtests (t.Run) as default and 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/daemon/channels_test.go` around lines 30 - 586, Consolidate the many
top-level tests in this file into table-driven subtests using t.Run("Should...")
— for example, replace individual tests like
TestComposeChannelRuntimeBuildsOnlyWhenRegistrySupportsChannelPersistence,
TestWithChannelSecretResolverAppliesOption,
TestBootExtensionsInjectsChannelRuntimeDeps,
TestChannelRuntimeStartInstanceTransitionsDisabledInstance,
TestChannelRuntimeCreateEnabledInstanceReloadsExtensions,
TestChannelRuntimeCreateEnabledInstanceRollsBackToDisabledOnReloadFailure,
TestChannelRuntimeResolveChannelRuntimeResolvesBoundSecrets,
TestChannelRuntimeResolveChannelRuntimeRequiresSecretResolverWhenBindingsExist,
TestChannelRuntimeResolveChannelRuntimeDoesNotPersistStartingWhenSecretResolutionFails,
TestChannelRuntimeStopInstanceBlocksIngressPreservesRoutes, and
TestChannelRuntimeRestartPreservesRoutesAndReloadsExtensions into a single
TestChannelRuntimeSuite (or similar) that iterates over a table of cases and
calls t.Run("Should <short description>", func(t *testing.T) { ... }) for each
case; preserve existing setup code (homePaths, cfg, newTestDaemon,
openDaemonTestGlobalDB, newChannelRuntime, runtime methods like
StartInstance/CreateInstance/ResolveChannelRuntime/StopInstance/RestartInstance,
and helpers mustCreateDaemonChannelInstance/mustUpsertDaemonChannelRoute), move
per-case differences into the table (initial instance request,
resolver/extension behavior, expected outcomes), run each subtest in parallel
where appropriate, and keep assertions unchanged so behavior is identical but
organized as subtests.

internal/daemon/daemon_test.go (2)

2864-2875: Preserve explicit empty capability/action/security lists in fixtures.

Using len(...) == 0 here makes nil and []string{} behave the same, so callers cannot build a manifest that intentionally declares no provides, requires, or security.capabilities. That weakens negative handshake/authorization coverage because the helper silently injects "memory.backend", "sessions/list", and "session.read".

Possible fix

  capabilities := append([]string(nil), opts.capabilities...)
- if len(capabilities) == 0 {
+ if opts.capabilities == nil {
    capabilities = []string{"memory.backend"}
  }
  actions := append([]string(nil), opts.actions...)
- if len(actions) == 0 {
+ if opts.actions == nil {
    actions = []string{"sessions/list"}
  }
  security := append([]string(nil), opts.security...)
- if len(security) == 0 {
+ if opts.security == nil {
    security = []string{"session.read"}
  }

---

3195-3225: Wrap marker-recording failures with operation context.

These helpers currently bubble raw marshal / append errors. When the helper process exits, that leaves stderr without enough context to tell whether initialize encoding, delivery encoding, or file append failed.

Possible fix

 	payload, err := json.Marshal(daemonInitializeMarker{
 		Request:  request,
 		Response: response,
 	})
 	if err != nil {
-		return err
+		return fmt.Errorf("marshal initialize marker: %w", err)
 	}
-	return appendMarkerLine(h.marker, string(payload))
+	if err := appendMarkerLine(h.marker, string(payload)); err != nil {
+		return fmt.Errorf("append initialize marker: %w", err)
+	}
+	return nil
 }
 
 func (h *daemonExtensionHelperServer) recordDelivery(request channelspkg.DeliveryRequest) error {
@@
 	payload, err := json.Marshal(daemonDeliveryMarker{
 		PID:     os.Getpid(),
 		Request: request,
 	})
 	if err != nil {
-		return err
+		return fmt.Errorf("marshal delivery marker: %w", err)
 	}
-	return appendMarkerLine(h.marker, string(payload))
+	if err := appendMarkerLine(h.marker, string(payload)); err != nil {
+		return fmt.Errorf("append delivery marker: %w", err)
+	}
+	return nil
 }

As per coding guidelines, "Use explicit error returns with wrapped context: fmt.Errorf("context: %w", err)".

🤖 Prompt for AI Agents

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

In `@internal/daemon/daemon_test.go` around lines 3195 - 3225, Both
recordInitialize and recordDelivery currently return raw errors from
json.Marshal and appendMarkerLine; update them to wrap errors with contextual
messages using fmt.Errorf so stderr shows whether the failure occurred during
marshaling or file append. Specifically, in
daemonExtensionHelperServer.recordInitialize wrap the error returned from
json.Marshal with a message like "recordInitialize: marshal failed" and wrap the
error from appendMarkerLine with "recordInitialize: appendMarkerLine failed"; do
the analogous wrapping in daemonExtensionHelperServer.recordDelivery for the
json.Marshal and appendMarkerLine errors, referencing those function names
(recordInitialize, recordDelivery) and the called helper appendMarkerLine to
locate the fixes.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes