feat: add network implementation#15
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a complete AGH network subsystem (protocol, validation, transport, router, peer registry, delivery coordinator, manager, audit, and CLI/API surfaces), adds session turn provenance and Space propagation, updates DB schema/migrations, and wires tests and runtime integration across daemon, API, CLI, session manager, ACP, and store layers. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(120,120,255,0.5)
participant CLI as CLI
participant API as HTTP API
participant UDS as UDS API
end
rect rgba(120,200,120,0.5)
participant Manager as Network Manager
participant Router as Router
participant Transport as Transport (NATS)
end
rect rgba(255,160,80,0.5)
participant Delivery as DeliveryCoordinator
participant Session as Session / ACP
participant Audit as AuditWriter
end
CLI->>API: POST /api/network/send (payload)
API->>Manager: Manager.Send(request)
Manager->>Router: Router.Send(validatedEnvelope)
Router->>Transport: Publish(subject, envelopeJSON)
Transport->>Transport: NATS broker delivers to subject
Transport->>Manager: inbound callback -> Manager.handleInboundMessage(payload)
Manager->>Audit: RecordReceived(sessionID, envelope)
Manager->>Delivery: accept(sessionID, envelope)
Delivery->>Session: PromptNetwork(sessionID, preview)
Session->>Manager: OnTurnEnd(sessionID) -> triggers queued deliveries
sequenceDiagram
participant Transport as Transport (NATS)
participant Router as Router
participant Manager as Network Manager
participant Delivery as DeliveryCoordinator
participant Session as Session / ACP
participant Store as GlobalDB
Transport->>Router: inbound message
Router->>Manager: Receive => RouteResult (deliveries/generated/rejected)
Manager->>Store: WriteNetworkAudit(received)
Manager->>Delivery: accept(envelope)
Delivery->>Session: PromptNetwork(sessionID, preview)
Session->>Manager: OnTurnEnd(sessionID)
Manager->>Router: (possible generated receipts) Router.Send(...)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/store/globaldb/global_db_session.go (1)
192-223: 🛠️ Refactor suggestion | 🟠 MajorWrap DB execution errors in
registerSession.This function currently returns the raw DB error, which drops operation/session context.
Proposed fix
func (g *GlobalDB) registerSession(ctx context.Context, exec sqlExecutor, session store.SessionInfo) error { - _, err := exec.ExecContext( + _, err := exec.ExecContext( ctx, `INSERT INTO sessions ( id, name, agent_name, workspace_id, session_type, space, state, acp_session_id, stop_reason, stop_detail, created_at, updated_at ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) @@ store.FormatTimestamp(session.CreatedAt), store.FormatTimestamp(session.UpdatedAt), ) - return err + if err != nil { + return fmt.Errorf("store: upsert session %q: %w", session.ID, 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/store/globaldb/global_db_session.go` around lines 192 - 223, registerSession currently returns the raw DB error from exec.ExecContext; wrap that error with contextual information before returning. In the registerSession function (and around the exec.ExecContext call using the sqlExecutor), capture the returned err and if non-nil return a wrapped error using fmt.Errorf with context such as "registerSession id=%s: %w" (use session.ID) so callers get operation and session context rather than the raw DB error.internal/acp/handlers.go (1)
327-351:⚠️ Potential issue | 🟠 MajorRemove shell-splitting from the default terminal execution path—only use it for the network-turn allowlist check.
The ACP schema defines
CreateTerminalRequest.Commandas the executable path andArgsas separate arguments. The current implementation callsterminalArgv()unconditionally interminalManager.create(), shell-splittingCommandfor every terminal request. This breaks the ACP contract and fails for any executable path containing spaces (e.g.,/Program Files/app).Use
request.Commandandrequest.Argsdirectly for execution; shell-splitting is useful only for the network-turn allowlist validation.Also applies to: 414-420, 662-675
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/handlers.go` around lines 327 - 351, The code currently shell-splits the command via terminalArgv() in the normal terminal creation path; change it so terminalArgv() is only used for the network-turn allowlist check (where isAllowedNetworkTerminalArgv(argv) is called) and do not call terminalArgv() when building the execution parameters for p.terminals.create; instead pass request.Command and request.Args directly (preserve terminalOwnership logic with networkOwned/ownerTurnID), and update terminalManager.create (and any other creation entry points that currently call terminalArgv(), e.g., the similar places around the other noted spots) to accept and use the verbatim Command and Args without shell-splitting.
🟡 Minor comments (11)
internal/api/udsapi/server_test.go-30-31 (1)
30-31:⚠️ Potential issue | 🟡 MinorStrengthen this option test to assert the injected instance, not just non-nil.
The current check can pass with a default fallback implementation, so it does not prove
WithExtensionService(...)wiring.💡 Proposed fix
- extensions := stubExtensionService{} + extensions := &stubExtensionService{} ... - WithExtensionService(extensions), + WithExtensionService(extensions), ... - if server.extensions == nil || server.handlers.Extensions == nil { - t.Fatal("expected extension service option to be installed") + if server.extensions != extensions || server.handlers.Extensions != extensions { + t.Fatal("expected provided extension service instance to be installed") }As per coding guidelines,
Anti-Patterns to REJECT: Constructor tests that only check != nil.Also applies to: 49-50, 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/udsapi/server_test.go` around lines 30 - 31, The test currently only asserts the extension service is non-nil, which can pass with a default; update the test that uses stubExtensionService and the server/options that accept WithExtensionService(...) to assert that the server's extension service is the exact injected instance (pointer equality or interface identity) rather than != nil; locate where cfg := aghconfig.DefaultWithHome(homePaths) and the option WithExtensionService(...) are applied (and the other similar tests using stubExtensionService) and change the assertions to compare the returned/installed extension service to the original stubExtensionService value to prove wiring.internal/session/manager_integration_test.go-143-145 (1)
143-145:⚠️ Potential issue | 🟡 MinorGuard
startCallslength before indexed assertions.These direct indexes can panic and hide the real regression. Add explicit length checks for clearer failures.
🛠️ Suggested patch
if got := strings.Count(h.driver.startCalls[0].SystemPrompt, networkSkill); got != 1 { t.Fatalf("create prompt network skill occurrences = %d, want 1", got) } @@ + if len(h.driver.startCalls) < 2 { + t.Fatalf("start calls = %d, want at least 2 (create + resume)", len(h.driver.startCalls)) + } if got := h.driver.startCalls[1].SystemPrompt; !strings.Contains(got, networkSkill) { t.Fatalf("resume system prompt = %q, want bundled network skill content", got) }Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_integration_test.go` around lines 143 - 145, The test indexes into h.driver.startCalls without verifying its length, which can panic; before any asserts that use h.driver.startCalls[0] (and the similar block at the later lines around the second assertion), add explicit checks like if len(h.driver.startCalls) == 0 { t.Fatalf("expected at least 1 startCall, got 0") } (and adjust message/count for the second case if expecting >1) so the test fails with a clear error instead of a panic when startCalls is shorter than expected; update both occurrences referencing startCalls/SystemPrompt/networkSkill to perform these guards before accessing indexed elements.internal/acp/client_test.go-1010-1037 (1)
1010-1037:⚠️ Potential issue | 🟡 MinorDon't collapse every failure into
*_blocked.Both branches currently treat any error as a successful guardrail hit, so the integration can still pass on unrelated failures like bad paths, transport issues, or terminal setup problems. Please surface the actual error class here, or at least distinguish the expected network-turn rejection from generic request failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client_test.go` around lines 1010 - 1037, The tests currently collapse any error from a.conn.WriteTextFile and a.conn.CreateTerminal into "write_blocked"/"shell_blocked"; change each error branch to distinguish expected guardrail rejections from other failures by inspecting the error (e.g., errors.Is / type assertion against the SDK sentinel or rejection error returned by WriteTextFile/CreateTerminal) and only set writeResult/shellResult to "write_blocked"/"shell_blocked" for that specific rejection; for any other error, include the actual error class/text in the SessionUpdate (e.g., "write_error: <err>" / "shell_error: <err>") or return the error instead of masking it so callers/tests can see transport/setup failures, using the existing symbols WriteTextFile, CreateTerminal, writeResult, shellResult, SessionUpdate, UpdateAgentMessageText, and conn to locate the code.internal/cli/network_test.go-177-183 (1)
177-183:⚠️ Potential issue | 🟡 MinorWeak assertion: "3" could match unrelated content.
The check
strings.Contains(inboxOut, "3")is fragile—it could pass if any "3" appears in the output (e.g., peer counts, timestamps). Consider a more specific assertion like checking for the exact formatted handoff version label and value.💡 Suggested improvement
- if !strings.Contains(inboxOut, "wf-1") || !strings.Contains(inboxOut, "3") { + if !strings.Contains(inboxOut, "wf-1") || !strings.Contains(inboxOut, "Handoff") { t.Fatalf("network inbox output = %q, want workflow and handoff metadata", inboxOut) }Or check for a more specific pattern like
"handoff_version": 3or similar depending on the output format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/network_test.go` around lines 177 - 183, The assertion is too weak because strings.Contains(inboxOut, "3") can match unrelated "3"s; update the test that calls executeRootCommand (the network inbox case with session "sess-a" producing inboxOut) to assert a more specific handoff/version field for workflow "wf-1" — e.g. check for the exact formatted label and value (like `"handoff_version": 3` or `handoff_version: 3`) or use a regexp that matches the handoff key and the digit 3 next to it so the test only passes when the handoff version is actually 3.internal/session/manager.go-307-315 (1)
307-315:⚠️ Potential issue | 🟡 MinorAdd the same nil-receiver guard here.
The surrounding late-bound accessors are nil-safe, but
IsPromptingcallsGetand will panic if the manager is absent. Returningfalsekeeps this new surface consistent.Suggested fix
func (m *Manager) IsPrompting(id string) bool { + if m == nil { + return false + } session, ok := m.Get(id) if !ok { return false } return session.IsPrompting() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 307 - 315, The IsPrompting method must be nil-receiver safe: add a guard at the start of Manager.IsPrompting to return false if the receiver m is nil, then proceed to call m.Get(id) as before; this mirrors other late-bound accessors and prevents a panic when Manager is absent, preserving the intended false return semantics and leaving the subsequent session.IsPrompting() call unchanged.internal/daemon/daemon_integration_test.go-127-140 (1)
127-140:⚠️ Potential issue | 🟡 MinorComplete the delivery in the happy-path test.
This stub only closes the event stream when
ctxis canceled, soTestBootNetworkEnabledDeliversInboundAndShutsDownCleanlynever exercises a successfully completed inbound delivery—it only proves the prompt started before shutdown. Emitacp.EventTypeDonehere (and ideally assertMessagesDeliveredbeforeShutdown) so the test actually covers the clean-delivery path.As per coding guidelines, "MUST test meaningful business logic, not trivial operations" and "Ensure tests verify behavior outcomes, not just function calls".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_integration_test.go` around lines 127 - 140, The promptNetworkFn stub in the happy-path test (bindableSessions.promptNetworkFn) only closes the events channel on ctx cancellation and never emits a successful delivery; modify the goroutine that drives the returned events channel to send an acp.AgentEvent with Type set to acp.EventTypeDone (and appropriate Message/Metadata if required) before closing the channel so the test path exercises a completed inbound delivery, and update TestBootNetworkEnabledDeliversInboundAndShutsDownCleanly to assert that MessagesDelivered (from the agent/session metrics) is incremented prior to Shutdown to verify the clean-delivery behavior.internal/network/transport_integration_test.go-66-129 (1)
66-129:⚠️ Potential issue | 🟡 MinorThread the broker token through the audited path if you want this leak test to fail correctly.
Right now
transport.tokenis only used in the finalstrings.Containschecks; none of the data passed toNewAuditWriter,RecordSent, orRecordRejectedcontains it. That means this test stays green even if a real transport/audit integration later starts serializing the token elsewhere. Assert against the actual transport/daemon output that previously leaked, or inject the token into the code path the audit writer is supposed to sanitize.As per coding guidelines, "MUST test meaningful business logic, not trivial operations" and "Ensure tests verify behavior outcomes, not just function calls".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/transport_integration_test.go` around lines 66 - 129, The test currently never injects the broker token into the data sent to the audit writer, so the leak assertions are ineffective; update the test to thread transport.token through the audited path by mutating the envelope before calling NewAuditWriter/RecordSent/RecordRejected (e.g., set a field on the envelope returned by testAuditEnvelope(t) such as MessageID, PeerFrom, PeerTo, or a metadata field to transport.token), then call writer.RecordSent and writer.RecordRejected and keep the existing checks that os.ReadFile(auditPath) and the DB entries (from db.ListNetworkAudit) do not contain transport.token; reference symbols: transport.token, testAuditEnvelope, NewAuditWriter, RecordSent, RecordRejected, db.ListNetworkAudit.internal/store/globaldb/global_db_network_audit.go-76-78 (1)
76-78:⚠️ Potential issue | 🟡 MinorDon't discard
rows.Close()errors.
QueryContextdrivers can surface late failures onClose()._ = rows.Close()drops that signal and makes this path harder to diagnose if the read fails after iteration.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/store/globaldb/global_db_network_audit.go` around lines 76 - 78, The deferred call currently swallows errors from rows.Close() (defer func(){ _ = rows.Close() }()), lose late failure signals; change the defer to capture and handle the error from rows.Close() instead—e.g. in the defer func() check the returned error and either log it or wrap/return it so callers see the failure. Locate the defer around rows.Close() (the rows variable and its defer in global_db_network_audit.go) and replace the blank-assignment with explicit error handling that records the error via the existing logger or returns it from the enclosing function.internal/network/router_integration_test.go-22-24 (1)
22-24:⚠️ Potential issue | 🟡 MinorDon't swallow teardown failures in the integration harness.
These cleanups currently discard
Shutdown/Unsubscribeerrors, which makes leaked NATS resources show up later as flaky failures instead of failing the test at the source.🧹 Suggested change
t.Cleanup(func() { - _ = transport.Shutdown(context.Background()) + shutdownCtx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + if err := transport.Shutdown(shutdownCtx); err != nil { + t.Errorf("transport.Shutdown() error = %v", err) + } }) ... t.Cleanup(func() { - _ = subscription.Unsubscribe() + if err := subscription.Unsubscribe(); err != nil { + t.Errorf("subscription.Unsubscribe() error = %v", err) + } })As per coding guidelines, "Never ignore errors with
_— every error must be handled or have a written justification".Also applies to: 62-64, 125-127, 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/router_integration_test.go` around lines 22 - 24, The test teardown currently discards errors from transport.Shutdown (and similar calls like transport.Unsubscribe) inside t.Cleanup, which hides teardown failures; update each t.Cleanup block (the ones calling transport.Shutdown and transport.Unsubscribe) to capture the returned error, and call t.Fatalf or t.Errorf (prefer t.Fatalf in cleanup to fail the test immediately) when an error is non-nil so teardown failures surface as test failures instead of being swallowed.internal/cli/network.go-162-165 (1)
162-165:⚠️ Potential issue | 🟡 MinorHandle
MarkFlagRequiredfailures instead of discarding them.If one of these flag names drifts during a refactor, the command will silently stop enforcing required input.
🛠️ Suggested change
+ mustMarkFlagRequired := func(name string) { + if err := cmd.MarkFlagRequired(name); err != nil { + panic(fmt.Errorf("cli: mark %q as required: %w", name, err)) + } + } - _ = cmd.MarkFlagRequired("session") - _ = cmd.MarkFlagRequired("space") - _ = cmd.MarkFlagRequired("kind") - _ = cmd.MarkFlagRequired("body") + mustMarkFlagRequired("session") + mustMarkFlagRequired("space") + mustMarkFlagRequired("kind") + mustMarkFlagRequired("body")As per coding guidelines, "Never ignore errors with
_— every error must be handled or have a written justification".Also applies to: 189-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/network.go` around lines 162 - 165, The calls to cmd.MarkFlagRequired("session"), "space", "kind", and "body" (and the similar calls at the later locations around the other flags) currently ignore errors using `_`; instead check and handle each error return from cmd.MarkFlagRequired, e.g., capture the error variable and either return it from the enclosing function or log/exit with a clear message; update the code paths that call MarkFlagRequired (the command setup function around cmd.MarkFlagRequired) so that any non-nil error results in immediate handling rather than being discarded.internal/network/audit.go-152-154 (1)
152-154:⚠️ Potential issue | 🟡 MinorHandle or justify the ignored
file.Close()error.The error from
file.Close()is suppressed with_. Per coding guidelines, every error must be handled or have a written justification. While close errors on read-only files are often ignorable, for writable files this can indicate data loss (e.g., buffered data not flushed).Consider logging the error or joining it with the return value:
Proposed fix
- defer func() { - _ = file.Close() - }() + defer func() { + if closeErr := file.Close(); closeErr != nil { + // Close error after successful sync is unlikely but logged for diagnostics + } + }()Alternatively, join with sync error if sync succeeds but close fails.
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/network/audit.go` around lines 152 - 154, The deferred anonymous function currently ignores the error from file.Close() (defer func() { _ = file.Close() }()), which violates the rule to never ignore errors; update the defer to capture and handle the error from file.Close() on the variable file (e.g., log it via the existing logger, or if the surrounding function has a named error return combine/append the close error to the function error so callers see it), or add an explicit comment justifying why it can be safely ignored; reference the defer func and the file variable in internal/network/audit.go to make the change.
🧹 Nitpick comments (18)
internal/api/udsapi/extensions_additional_test.go (1)
119-153: Split unrelated approve-session check into a dedicated test.
extensionStatusCodemapping and/api/sessions/sess-1/approvebehavior are separate concerns. Splitting them improves failure localization and keeps each test scoped to one business behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/udsapi/extensions_additional_test.go` around lines 119 - 153, The current TestExtensionStatusCodeMappingsAndApproveSession mixes two concerns; split it by keeping the table-driven assertions that call extensionStatusCode(err) in the existing test and move the approve-session request into a new test (e.g., TestApproveSessionReturnsNotImplemented). In the new test, set up homePaths via newTestHomePaths(t), create the router with newTestRouter(t, newTestHandlers(t, stubSessionManager{}, stubObserver{}, homePaths)), call performRequest(t, engine, http.MethodPost, "/api/sessions/sess-1/approve", nil) and assert recorder.Code == http.StatusNotImplemented; remove the approve-session block from the original TestExtensionStatusCodeMappingsAndApproveSession so each test focuses on one behavior.internal/cli/helpers_test.go (1)
19-25: Add compile-time interface verification forstubClient.Given this stub now tracks more
DaemonClientmethods, add an explicit compile-time assertion so future interface changes fail fast in tests.♻️ Suggested patch
type stubClient struct { @@ } + +var _ DaemonClient = stubClient{}As per coding guidelines, "Use compile-time interface verification:
var _ Interface = (*Type)(nil)".Also applies to: 67-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/helpers_test.go` around lines 19 - 25, Add a compile-time interface assertion so the test stub implements DaemonClient and future method additions fail fast: in internal/cli/helpers_test.go add a var _ DaemonClient = (*stubClient)(nil) near the stubClient type definition (and repeat for any other stub types around lines ~67-100 if present) to cause a compile-time error when the stub no longer satisfies DaemonClient.internal/session/manager_hooks_test.go (1)
237-275: Wrap this case int.Run("Should...")to match test conventions.The assertions are solid; just align structure with the suite’s required subtest pattern so future case expansion stays uniform.
As per coding guidelines, "MUST use
t.Run(\"Should...\")pattern for ALL test cases" and "Use table-driven tests with subtests (t.Run) as default".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_hooks_test.go` around lines 237 - 275, TestPromptNetworkUsesNetworkInputClass is a top-level test that should be converted into a subtest using t.Run("Should use network input class") to follow the project's t.Run subtest convention; wrap the existing body (setup of spyHookDispatcher, newHarness, createSession, PromptNetwork call, collectEvents and the three assertions checking inputPayload.InputClass, turnStartPayload.InputClass, and turnStartPayload.UserMessage) inside a t.Run call so the test becomes TestPromptNetworkUsesNetworkInputClass -> t.Run("Should use network input class", func(t *testing.T) { ... }), keeping all existing setup and assertions and preserving t.Parallel() behavior by calling t.Parallel() inside the subtest if parallelism is still desired.internal/config/merge_test.go (1)
92-139: Consider converting this to at.Run("Should...")(or table-driven) case.Behavior coverage is good; this is mainly to keep consistency with the required test-case structure.
As per coding guidelines, "MUST use
t.Run(\"Should...\")pattern for ALL test cases" and "Use table-driven tests with subtests (t.Run) as default in Go tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/merge_test.go` around lines 92 - 139, The test TestApplyConfigOverlayFileAppliesNetworkOverlay violates the repository pattern requiring t.Run subtests; update this test to use a subtest (e.g., t.Run("Should apply network overlay", func(t *testing.T) { ... })) or convert it into a table-driven test with subtests, moving the existing setup/assertions (ResolveHomePathsFrom, DefaultWithHome, writing overlay file, ApplyConfigOverlayFile and the subsequent cfg.Network.* checks) into the subtest body so it follows the required t.Run("Should...") structure while keeping the same assertions and helper calls.internal/network/transport_test.go (1)
159-161: Consider simplifyingnilTransportContexthelper.This helper function adds indirection for a simple
nilvalue. It could be inlined at call sites for clarity, or at minimum deserves a comment explaining why it exists (perhaps to avoid linter warnings about passingnildirectly).♻️ Option: inline or add comment
-func nilTransportContext() context.Context { - return nil -} +// nilTransportContext returns nil, factored out to suppress linter warnings +// about passing untyped nil to context.Context parameters. +func nilTransportContext() context.Context { return nil }Or simply use
(context.Context)(nil)at call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/transport_test.go` around lines 159 - 161, The helper function nilTransportContext adds unnecessary indirection; update callers to pass a nil context directly (use (context.Context)(nil) at call sites) or keep the function but add a brief comment on why it exists (e.g., to avoid linter warnings about passing nil contexts). Locate the helper nilTransportContext in internal/network/transport_test.go and either remove it and replace uses with (context.Context)(nil) or annotate the function with a one-line comment explaining its purpose before leaving it.internal/api/core/errors.go (1)
88-94: Consider preserving the full error chain with%w.Using
%vfor the inner error loses its chain, soerrors.Is(err, someInnerSentinel)will fail on the wrapped result. If callers need to unwrap both sentinels, useerrors.Joinor a multi-error pattern.♻️ Optional: Preserve both error chains
func NewNetworkValidationError(err error) error { if err == nil { return nil } - return fmt.Errorf("%w: %v", ErrNetworkValidation, err) + return fmt.Errorf("%w: %w", ErrNetworkValidation, err) }Note: Go 1.20+ supports multiple
%wverbs infmt.Errorf.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/errors.go` around lines 88 - 94, The wrapper NewNetworkValidationError currently drops the wrapped error's chain by formatting the inner error with %v; update it to preserve unwrapping semantics by returning a wrapped error that keeps both sentinels (e.g., use fmt.Errorf("%w: %w", ErrNetworkValidation, err) or errors.Join(ErrNetworkValidation, err)) while retaining the nil check; locate the NewNetworkValidationError function and replace the format so callers can errors.Is/errors.Unwrap the inner error reliably.internal/cli/network_test.go (1)
186-225: Consider usingt.Runsubtests for validation cases.This would provide better test isolation and clearer failure reporting. As per coding guidelines, table-driven tests with subtests (
t.Run) are the default pattern.♻️ Suggested refactor with t.Run
func TestNetworkSendParsersRejectInvalidFlags(t *testing.T) { t.Parallel() deps := newTestDeps(t, stubClient{ networkSendFn: func(context.Context, NetworkSendRequest) (NetworkSendRecord, error) { return NetworkSendRecord{}, nil }, }) - if _, _, err := executeRootCommand(t, deps, + t.Run("Should reject non-JSON body", func(t *testing.T) { + t.Parallel() + _, _, err := executeRootCommand(t, deps, "network", "send", "--session", "sess-a", "--space", "builders", "--kind", "say", "--body", `not-json`, - ); err == nil || !strings.Contains(err.Error(), "--body must be valid JSON") { - t.Fatalf("invalid --body error = %v, want JSON validation", err) - } + ) + if err == nil || !strings.Contains(err.Error(), "--body must be valid JSON") { + t.Fatalf("invalid --body error = %v, want JSON validation", err) + } + }) // ... similar for other cases🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/network_test.go` around lines 186 - 225, The test TestNetworkSendParsersRejectInvalidFlags should be refactored to use t.Run subtests (or a table-driven loop that calls t.Run) so each validation case is isolated and reported separately; update the three executeRootCommand assertions into distinct subtests (e.g., "invalid body JSON", "invalid ext object", "invalid expires-at format") that call executeRootCommand with the same deps and assert the error contains the expected message, keeping the top-level t.Parallel() and using t.Fatalf or t.Error inside each subtest as before; reference TestNetworkSendParsersRejectInvalidFlags and executeRootCommand when locating and converting the three existing inline checks into t.Run subtests.internal/config/config.go (1)
378-380: Wrap network validation errors with context.This is the only nested config validation here that returns the raw error. Wrapping it keeps failures actionable once more network checks land.
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/config/config.go` around lines 378 - 380, The call to c.Network.Validate() returns raw errors; update the error return in the function containing that code to wrap the error with contextual text (e.g., return fmt.Errorf("network validation: %w", err)) so failures are actionable; also add/import "fmt" in internal/config/config.go if not already present.internal/network/delivery_test.go (1)
499-509: Avoid polling withtime.Sleepin the concurrency helper.
waitForCallsis coordinating goroutine progress, so the fixed 10ms sleep loop makes this suite timing-sensitive under load. HavePromptNetworksignal a channel orsync.Condand wait on that here instead.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/network/delivery_test.go` around lines 499 - 509, The helper waitForCalls in fakeDeliveryPrompter is polling with time.Sleep and should instead block on a synchronization primitive; add a notify mechanism to fakeDeliveryPrompter (e.g., a channel or sync.Cond) that PromptNetwork (or whichever method increments callCount) signals when it increments the counter, then replace the loop in waitForCalls to wait on that notification with a timeout (select on the notify channel and a time.After deadline or use cond.Wait with a timed loop) so the test unblocks deterministically when callCount reaches want; update references to the call increment site and waitForCalls accordingly.internal/network/delivery_integration_test.go (1)
337-359: These polling helpers are still wall-clock dependent.The
time.Sleeploops make the integration suite slower and prone to flakes on loaded runners. Prefer signaling fromintegrationPromptDriver/deliveryCoordinatorwith channels or async.Condinstead of retrying on a timer.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/network/delivery_integration_test.go` around lines 337 - 359, The two helpers assertPromptCountEventually and waitForDeliveryCondition are using time.Sleep polling which causes flakiness; update them to block on explicit synchronization from integrationPromptDriver/deliveryCoordinator instead: add a notification mechanism (e.g., a channel or sync.Cond) to integrationPromptDriver and/or deliveryCoordinator that is signaled whenever promptCount or delivery state changes, then rewrite assertPromptCountEventually to select on that notification channel with a time.After timeout (or use cond.Wait with timeout) and check driver.promptCount() only when signaled, and rewrite waitForDeliveryCondition to wait on the same notifier (or a dedicated done channel) and evaluate fn() when signaled; ensure the notifier is closed or signaled on final states so tests never rely on fixed sleeps.internal/store/globaldb/global_db_network_audit.go (1)
125-127: Wrap timestamp parse failures with audit-row context.Returning the raw
store.ParseTimestamperror makes it harder to tell which scan path failed when audit reads break. Please wrap it here the same way the other database errors are wrapped.As per coding guidelines, "Use explicit error returns with wrapped context: `fmt.Errorf(\"context: %w\", err)`".Proposed change
timestamp, err := store.ParseTimestamp(timestampRaw) if err != nil { - return store.NetworkAuditEntry{}, err + return store.NetworkAuditEntry{}, fmt.Errorf("store: parse network audit timestamp: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/globaldb/global_db_network_audit.go` around lines 125 - 127, The ParseTimestamp failure should be wrapped with audit-row context before returning; replace the bare return of err after calling store.ParseTimestamp(timestampRaw) so the function still returns store.NetworkAuditEntry{} but the error is wrapped using fmt.Errorf to include identifying context (e.g., the audit row/scan path and timestampRaw) following the project pattern used for other DB errors, referencing store.ParseTimestamp, timestampRaw, and the NetworkAuditEntry return.internal/network/manager_test.go (1)
166-177: Replace this sleep loop with a deterministic wait.Polling
Inbox()withtime.Sleepmakes this test timing-sensitive and more likely to flake under load. A signal from the fake prompter/delivery path would make the queue assertion deterministic.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/network/manager_test.go` around lines 166 - 177, Replace the polling loop that calls manager.Inbox() with a deterministic synchronization: add a signal channel or waitgroup in the fake prompter/delivery test harness and have the fake delivery path send on that channel when it enqueues the message; in the test wait for that signal with a select and a timeout, then call manager.Inbox(ctx, "sess-busy") once and assert len(inbox)==1. Update the fake prompter/delivery implementation to expose the notify channel and use it in the test instead of the time.Sleep loop so the test no longer relies on polling.internal/daemon/daemon_test.go (1)
281-283: Avoid matching the boot failure by substring.Line 282 couples this test to error wording rather than the actual contract. Please prefer a sentinel/typed boot error and assert it with
errors.Is/errors.As; otherwise harmless message rewording will break the test.As per coding guidelines, "Use
errors.Is()anderrors.As()for error matching — never compare error strings".🤖 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 281 - 283, The test currently matches d.boot() failure by substring; change it to assert a sentinel or typed error using errors.Is/errors.As instead: introduce or use a sentinel error like ErrMissingNetworkBindingSurface (or the typed error type returned by boot) and replace the string containment check with errors.Is(err, ErrMissingNetworkBindingSurface) (or use errors.As to inspect the typed error). If the boot implementation doesn't already wrap/return that sentinel, update boot() to return or wrap the sentinel using fmt.Errorf("%w: ...", ErrMissingNetworkBindingSurface) so the test can reliably use errors.Is; keep the assertion as t.Fatalf("boot() error = %v, want ErrMissingNetworkBindingSurface", err) when the check fails.internal/api/core/network_test.go (1)
120-282: Split the endpoint coverage into subtests.This one test currently exercises five handlers plus response-shape assertions. Reusing the shared fixture inside
t.Run(...)blocks would keep setup cheap while making failures much easier to localize.As per coding guidelines, "Use table-driven tests with subtests (
t.Run) as default".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/network_test.go` around lines 120 - 282, The test TestBaseHandlersNetworkEndpoints is exercising five endpoints in one function; split it into subtests using t.Run to make failures localizable while reusing the shared fixture. Keep the existing fixture creation (fixture := newHandlerFixture(...)) and Config.Network.Enabled flag at the top of TestBaseHandlersNetworkEndpoints, then create t.Run blocks named e.g. "status", "peers", "spaces", "send", "inbox" that each contain the request/response/assert logic for the corresponding endpoint (/network/status, /network/peers, /network/spaces, /network/send, /network/inbox) and reference the same fixture variable; retain any fixed test data (fixedNow, deadline, stubbed fixture.Handlers functions like StatusFn, ListPeersFn, SendFn, InboxFn) but move only the per-endpoint performRequest and assertions into the subtest bodies so setup remains cheap and failures are isolated.internal/network/helpers_test.go (2)
12-20: Consider usingt.Runfor validation loop iterations.The loop validating each
Kindwould benefit from subtests for clearer failure reporting when a specific kind fails.Proposed fix
validKinds := []Kind{KindGreet, KindWhois, KindSay, KindDirect, KindRecipe, KindReceipt, KindTrace} for _, kind := range validKinds { - if err := kind.Validate(); err != nil { - t.Fatalf("Kind(%q).Validate() error = %v", kind, err) - } + t.Run(fmt.Sprintf("Should validate kind %s", kind), func(t *testing.T) { + if err := kind.Validate(); err != nil { + t.Fatalf("Kind(%q).Validate() error = %v", kind, err) + } + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/helpers_test.go` around lines 12 - 20, The test should use subtests so failures show which Kind failed: replace the for loop over validKinds in helpers_test.go with a table-driven subtest using t.Run for each entry (e.g., t.Run(string(kind), func(t *testing.T){ if err := kind.Validate(); err != nil { t.Fatalf(...)} }), and likewise change the invalid case to a t.Run("invalid", ...) that asserts errors.Is(err, ErrInvalidKind). Update references to Kind, Validate, and ErrInvalidKind accordingly.
201-219: Uset.Runsubtests for the trace transition matrix.The table-driven test for
canApplyTraceshould uset.Runfor each case to improve test isolation and failure reporting. This aligns with testing guidelines.Proposed fix
for _, tc := range matrix { - if got := canApplyTrace(tc.current, tc.next); got != tc.want { - t.Fatalf("canApplyTrace(%q, %q) = %v, want %v", tc.current, tc.next, got, tc.want) - } + t.Run(fmt.Sprintf("Should transition %s to %s = %v", tc.current, tc.next, tc.want), func(t *testing.T) { + if got := canApplyTrace(tc.current, tc.next); got != tc.want { + t.Fatalf("canApplyTrace(%q, %q) = %v, want %v", tc.current, tc.next, got, tc.want) + } + }) }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/network/helpers_test.go` around lines 201 - 219, Convert the table-driven loop over matrix into subtests by calling t.Run for each entry: inside the for _, tc := range matrix loop create a descriptive name like fmt.Sprintf("Should transition %s->%s", tc.current, tc.next), capture the loop variable with tc := tc at top of the loop body, and call t.Run(name, func(t *testing.T) { if got := canApplyTrace(tc.current, tc.next); got != tc.want { t.Fatalf("canApplyTrace(%q, %q) = %v, want %v", tc.current, tc.next, got, tc.want) } }). This keeps the same matrix data and uses canApplyTrace and matrix identifiers from the diff and follows the "Should..." naming guideline.internal/network/audit.go (1)
140-169: Consider keeping the file handle open for high-frequency audit logging.The current implementation opens and closes the file for every audit entry. While this ensures durability (each write is synced), it introduces significant I/O overhead if audit events are frequent.
If audit volume is expected to be high, consider:
- Keeping the file handle open and syncing periodically
- Using buffered writes with periodic flushes
- Documenting the durability-over-performance trade-off
This is acceptable if audit frequency is low or if maximum durability is required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/audit.go` around lines 140 - 169, The appendFile method opens, writes, syncs, and closes the file on every call (FileAuditWriter.appendFile), which causes high I/O overhead for frequent logs; change FileAuditWriter to hold a persistent file handle and buffered writer (e.g., add fields like file *os.File and buf *bufio.Writer on the FileAuditWriter struct), initialize/open them once (create a new Open/Start method or lazy-open inside appendFile when file==nil), replace per-call os.OpenFile/Close with writes to buf.Write and periodically call buf.Flush and file.Sync (or implement a background goroutine that flushes/syncs at intervals and on Close/Stop), guard access with w.mu and ensure Close/Stop flushes, syncs, and closes the file to preserve durability while reducing per-entry I/O.internal/network/envelope.go (1)
202-300: Add compile-time assertions for theBodyimplementations.This file introduces the
Bodyinterface plus seven concrete body types, but there are novar _ Body = ...checks. Adding them makes receiver/signature drift fail at compile time instead of surfacing later through call sites.♻️ Suggested assertions
+var ( + _ Body = (*GreetBody)(nil) + _ Body = (*WhoisBody)(nil) + _ Body = (*SayBody)(nil) + _ Body = (*DirectBody)(nil) + _ Body = (*RecipeBody)(nil) + _ Body = (*ReceiptBody)(nil) + _ Body = (*TraceBody)(nil) +)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/network/envelope.go` around lines 202 - 300, Add compile-time interface assertions to ensure the concrete types implement Body: insert lines like var _ Body = (*GreetBody)(nil), var _ Body = (*WhoisBody)(nil), var _ Body = (*SayBody)(nil), var _ Body = (*DirectBody)(nil), var _ Body = (*RecipeBody)(nil), var _ Body = (*ReceiptBody)(nil), and var _ Body = (*TraceBody)(nil) (one per concrete body type) near the type declarations (e.g., after the corresponding type blocks or at the bottom of the file) so any receiver/signature drift fails at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c52ad3f4-b1cd-4a62-a013-1663b08fc32a
⛔ Files ignored due to path filters (28)
.compozy/tasks/agh-network/_meta.mdis excluded by!**/*.md.compozy/tasks/agh-network/_tasks.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/agh-network/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_01.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_02.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_03.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_04.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_05.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_06.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_07.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_08.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_09.mdis excluded by!**/*.md.compozy/tasks/agh-network/task_10.mdis excluded by!**/*.mdbun.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.suminternal/skills/bundled/skills/agh-network/SKILL.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (109)
go.modinternal/acp/client_integration_test.gointernal/acp/client_test.gointernal/acp/handlers.gointernal/acp/handlers_test.gointernal/acp/permission.gointernal/acp/types.gointernal/api/contract/contract.gointernal/api/contract/responses.gointernal/api/core/conversions.gointernal/api/core/conversions_parsers_test.gointernal/api/core/errors.gointernal/api/core/handlers.gointernal/api/core/handlers_test.gointernal/api/core/hooks_test.gointernal/api/core/interfaces.gointernal/api/core/network.gointernal/api/core/network_test.gointernal/api/core/test_helpers_test.gointernal/api/httpapi/handlers.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/routes.gointernal/api/httpapi/server.gointernal/api/testutil/apitest.gointernal/api/udsapi/extensions_additional_test.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/helpers_test.gointernal/api/udsapi/network_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/server.gointernal/api/udsapi/server_test.gointernal/api/udsapi/udsapi_integration_test.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/command_paths_test.gointernal/cli/daemon.gointernal/cli/helpers_test.gointernal/cli/network.gointernal/cli/network_client_test.gointernal/cli/network_test.gointernal/cli/root.gointernal/cli/session.gointernal/cli/session_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/home.gointernal/config/home_test.gointernal/config/merge.gointernal/config/merge_test.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/info.gointernal/network/audit.gointernal/network/audit_test.gointernal/network/delivery.gointernal/network/delivery_integration_test.gointernal/network/delivery_test.gointernal/network/envelope.gointernal/network/envelope_integration_test.gointernal/network/helpers_test.gointernal/network/lifecycle.gointernal/network/lifecycle_test.gointernal/network/manager.gointernal/network/manager_test.gointernal/network/peer.gointernal/network/peer_test.gointernal/network/router.gointernal/network/router_integration_test.gointernal/network/router_test.gointernal/network/stats.gointernal/network/transport.gointernal/network/transport_integration_test.gointernal/network/transport_test.gointernal/network/validate.gointernal/network/validate_test.gointernal/observe/observer.gointernal/observe/reconcile.gointernal/session/interfaces.gointernal/session/manager.gointernal/session/manager_helpers.gointernal/session/manager_hooks.gointernal/session/manager_hooks_test.gointernal/session/manager_integration_test.gointernal/session/manager_lifecycle.gointernal/session/manager_network_skill.gointernal/session/manager_prompt.gointernal/session/manager_start.gointernal/session/manager_stop_integration_test.gointernal/session/manager_test.gointernal/session/query.gointernal/session/query_test.gointernal/session/session.gointernal/session/session_test.gointernal/session/stop_reason.gointernal/skills/bundled/bundled_test.gointernal/skills/bundled/content.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_network_audit.gointernal/store/globaldb/global_db_network_audit_test.gointernal/store/globaldb/global_db_session.gointernal/store/globaldb/global_db_session_test.gointernal/store/globaldb/global_db_test.gointernal/store/globaldb/migrate_workspace.gointernal/store/meta_test.gointernal/store/store.gointernal/store/types.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
internal/session/manager_helpers.go (1)
128-128: Wrap errors with session context for consistency.Per coding guidelines, errors should be wrapped with context. While
joinNetworkPeererror is wrapped at the call site (line 91), wrapping in the helpers ensures all callers receive consistent context.♻️ Suggested wrapping
For
joinNetworkPeer:- return lifecycle.JoinSpace(ctx, info.ID, networkPeerID(info.AgentName, info.ID), info.Space) + if err := lifecycle.JoinSpace(ctx, info.ID, networkPeerID(info.AgentName, info.ID), info.Space); err != nil { + return fmt.Errorf("session: join network peer for %q: %w", info.ID, err) + } + return nilFor
leaveNetworkPeer:- return lifecycle.LeaveSpace(ctx, info.ID) + if err := lifecycle.LeaveSpace(ctx, info.ID); err != nil { + return fmt.Errorf("session: leave network peer for %q: %w", info.ID, err) + } + return nilAlso applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_helpers.go` at line 128, The helpers that call joinNetworkPeer/leaveNetworkPeer (the functions in manager_helpers.go that return lifecycle.JoinSpace(...) and corresponding leave call) must wrap any returned error with session context; update joinNetworkPeer and leaveNetworkPeer call sites to capture the error and return fmt.Errorf("joinNetworkPeer %s/%s: %w", info.AgentName, info.ID, err) and fmt.Errorf("leaveNetworkPeer %s/%s: %w", info.AgentName, info.ID, err) respectively (or use errors.Wrap) so callers always get consistent session-aware error messages referencing info.AgentName and info.ID.internal/network/delivery.go (1)
647-656: Consider reusing thestrings.Replacerinstance.Creating a new
strings.Replaceron every call toxmlEscapeis inefficient for a hot path. Consider making it a package-level variable.♻️ Suggested refactor
+var xmlReplacer = strings.NewReplacer( + "&", "&", + "<", "<", + ">", ">", + `"`, """, + "'", "'", +) + func xmlEscape(value string) string { - replacer := strings.NewReplacer( - "&", "&", - "<", "<", - ">", ">", - `"`, """, - "'", "'", - ) - return replacer.Replace(strings.TrimSpace(value)) + return xmlReplacer.Replace(strings.TrimSpace(value)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/delivery.go` around lines 647 - 656, The xmlEscape function currently allocates a new strings.Replacer on every call; hoist the replacer to a package-level variable (e.g., xmlReplacer) initialized once with the same replacement pairs and then have xmlEscape call xmlReplacer.Replace(strings.TrimSpace(value)); update references to the replacer variable and remove the local NewReplacer creation inside xmlEscape to avoid per-call allocations.internal/network/manager.go (2)
934-934: Minor: Redundantstrings.TrimSpacecall.Line 934 calls
strings.TrimSpace(compactJSON(raw))butcompactJSONalready trims the result at line 953-955.♻️ Suggested fix
- return compactJSON(raw), strings.TrimSpace(compactJSON(raw)) != "" + compact := compactJSON(raw) + return compact, compact != "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/manager.go` at line 934, The return currently calls compactJSON(raw) twice and wraps the second call in strings.TrimSpace redundantly since compactJSON already trims; modify the return to call compactJSON(raw) once (e.g., assign to a local variable like compact) and return that value along with compact != "" so you don't call compactJSON twice and remove the unnecessary strings.TrimSpace; reference the compactJSON(raw) invocation and the existing return expression to locate the change.
643-680: Potential TOCTOU race inacquireBroadcastSubscription.There's a time-of-check to time-of-use (TOCTOU) race between lines 647-652 and 665-673. After releasing the lock to subscribe (line 652), another goroutine could add the space to
m.spaces, causing duplicate subscription handling. While this is handled bycleanupDuplicateBroadcastSubscription, the pattern could be simplified by holding the lock during the entire operation or using a sync.Map.However, holding the lock during a potentially blocking network call (subscribe) is not ideal. The current approach with duplicate cleanup is a reasonable trade-off.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/manager.go` around lines 643 - 680, acquireBroadcastSubscription has a TOCTOU between the initial existence check and the Subscribe call; fix by installing a placeholder managedSpace under m.spaces before doing the network Subscribe so concurrent callers see the placeholder and can increment refCount instead of duplicating work: inside acquireBroadcastSubscription, after trimming targetSpace and finding no entry, create and store &managedSpace{space: targetSpace, broadcastSub: nil, refCount: 1} while holding m.mu, then unlock and call m.transport.Subscribe; after Subscribe returns, lock m.mu again and set the stored managedSpace.broadcastSub = subscription (or, on Subscribe error, remove/decrement the placeholder and return the error); update co-operating callers to treat a nil broadcastSub as “subscription in progress” so they only increment refCount and do not Subscribe themselves (this eliminates reliance on cleanupDuplicateBroadcastSubscription and removes the TOCTOU window).internal/network/delivery_integration_test.go (2)
88-156: Consider addingt.Parallel()for parallel test execution.Same recommendation as the first test - this test uses isolated resources and should be safe for parallel execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/delivery_integration_test.go` around lines 88 - 156, The test TestDeliveryCoordinatorIntegrationMultipleSessionsDoNotBlockEachOther should call t.Parallel() at the start of the test function to enable parallel execution; add t.Parallel() as the first statement in the test body (immediately inside TestDeliveryCoordinatorIntegrationMultipleSessionsDoNotBlockEachOther) to allow the Go test runner to run it concurrently with other parallel tests, and verify that the local setup functions used here (newDeliveryIntegrationHarness, createIntegrationSession, newDeliveryCoordinator, manager, driver) remain local to the test so no shared global state is introduced.
24-86: Consider addingt.Parallel()to independent integration tests.Per coding guidelines, independent tests should use
t.Parallel()for parallel execution. Since these integration tests use isolated resources (separate managers, temp directories), they should be safe to run in parallel.func TestDeliveryCoordinatorIntegrationDrainsOneQueuedPromptPerTurn(t *testing.T) { + t.Parallel() manager, driver := newDeliveryIntegrationHarness(t)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/delivery_integration_test.go` around lines 24 - 86, Add t.Parallel() at the start of the TestDeliveryCoordinatorIntegrationDrainsOneQueuedPromptPerTurn test to allow it to run concurrently (call t.Parallel() as the first statement inside the function after the *testing.T parameter is available). Update the TestDeliveryCoordinatorIntegrationDrainsOneQueuedPromptPerTurn function to invoke t.Parallel() so the per-test manager/driver setup remains isolated while enabling parallel execution.internal/network/router.go (1)
515-529: The seen map is bounded by the maxReplayAge window, not truly unbounded.The cleanup loop executes on every
markSeencall (once per inbound message), not periodically. Map size is naturally bounded to the number of unique message IDs that can arrive within themaxReplayAgewindow (default 5 minutes per RFC). Each entry is deleted as soon as it expires, making this a standard bounded-cache pattern.If you want to be explicit about the upper bound or add monitoring, consider tracking the map size in metrics or documenting the expected bounds in a comment based on your typical message rate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/router.go` around lines 515 - 529, Add a short clarifying comment above the markSeen method explaining that r.seen is a sliding-window/bounded cache (bounded by r.maxReplayAge and cleaned on each markSeen call) so it is not unbounded; if you want explicit visibility add optional metrics updates when inserting/deleting (e.g., increment/decrement a seen-size gauge inside markSeen around r.seen[envelope.ID] assignment and the delete in the cleanup loop) and reference existing symbols markSeen, r.seen, r.maxReplayAge and replayDeadline when adding the comment/metrics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/core/network_test.go`:
- Around line 18-119: The test TestNetworkConversionHelpersPreserveMetadata
bundles three independent checks; split it into t.Run subtests named like
t.Run("Should map status metadata"), t.Run("Should convert NetworkSendRequest
preserving metadata"), and t.Run("Should convert Envelope preserving metadata")
so failures are isolated. For each subtest, move only the relevant
setup/assertions into that t.Run body (you can keep shared fixtures like
deadline/status/payload/envelope definitions at the top of the test or re-create
minimal needed values inside each subtest), and call the existing helper
functions NetworkStatusPayloadFromStatus, NetworkSendRequestFromPayload, and
NetworkEnvelopePayloadFromEnvelope inside their respective subtests, preserving
the same assertions and error handling.
- Around line 127-128: The test contains a no-op branch that checks if deadline
== 0 which never occurs because deadline is hardcoded; remove that branch and
replace it with a concrete assertion that deadline equals the expected value
(either the hardcoded constant 1775823000 or compute expectedDeadline :=
fixedNow.Unix() and assert equality). Update both occurrences where variables
fixedNow and deadline are used so the test asserts the correct non-zero deadline
rather than skipping verification.
- Around line 299-379: The TestBaseHandlersNetworkErrorsAndDisabledMode
currently bundles many scenarios in one flow; split it into t.Run subtests with
descriptive "Should..." names for each scenario (e.g.,
"ShouldReturnDisabledStatus", "ShouldReturnServiceUnavailableWhenPeersDisabled",
"ShouldMapNetworkStatusErrorTo500", "ShouldMapListSpacesErrorTo400",
"ShouldReturnBadRequestOnSendDecode", "ShouldMapSendTargetNotFoundTo404",
"ShouldReturnBadRequestWhenInboxMissing", "ShouldMapInboxInvalidFieldTo400"),
moving the relevant setup (fixture, toggling
fixture.Handlers.Config.Network.Enabled, and swapping fixture.Handlers.Network
stub implementations) into each subtest; within each subtest replace bare
status-code checks with stronger assertions that decode the JSON error payload
(using testutil.DecodeJSONResponse) and assert contents via ErrorContains or
errors.As where appropriate, and keep the final assertions on
core.StatusForNetworkError(validationErr) intact as their own subtest too—locate
and modify the TestBaseHandlersNetworkErrorsAndDisabledMode function and the
calls to performRequest, fixture.Handlers.Network, and
core.StatusForNetworkError to implement these changes.
---
Nitpick comments:
In `@internal/network/delivery_integration_test.go`:
- Around line 88-156: The test
TestDeliveryCoordinatorIntegrationMultipleSessionsDoNotBlockEachOther should
call t.Parallel() at the start of the test function to enable parallel
execution; add t.Parallel() as the first statement in the test body (immediately
inside TestDeliveryCoordinatorIntegrationMultipleSessionsDoNotBlockEachOther) to
allow the Go test runner to run it concurrently with other parallel tests, and
verify that the local setup functions used here (newDeliveryIntegrationHarness,
createIntegrationSession, newDeliveryCoordinator, manager, driver) remain local
to the test so no shared global state is introduced.
- Around line 24-86: Add t.Parallel() at the start of the
TestDeliveryCoordinatorIntegrationDrainsOneQueuedPromptPerTurn test to allow it
to run concurrently (call t.Parallel() as the first statement inside the
function after the *testing.T parameter is available). Update the
TestDeliveryCoordinatorIntegrationDrainsOneQueuedPromptPerTurn function to
invoke t.Parallel() so the per-test manager/driver setup remains isolated while
enabling parallel execution.
In `@internal/network/delivery.go`:
- Around line 647-656: The xmlEscape function currently allocates a new
strings.Replacer on every call; hoist the replacer to a package-level variable
(e.g., xmlReplacer) initialized once with the same replacement pairs and then
have xmlEscape call xmlReplacer.Replace(strings.TrimSpace(value)); update
references to the replacer variable and remove the local NewReplacer creation
inside xmlEscape to avoid per-call allocations.
In `@internal/network/manager.go`:
- Line 934: The return currently calls compactJSON(raw) twice and wraps the
second call in strings.TrimSpace redundantly since compactJSON already trims;
modify the return to call compactJSON(raw) once (e.g., assign to a local
variable like compact) and return that value along with compact != "" so you
don't call compactJSON twice and remove the unnecessary strings.TrimSpace;
reference the compactJSON(raw) invocation and the existing return expression to
locate the change.
- Around line 643-680: acquireBroadcastSubscription has a TOCTOU between the
initial existence check and the Subscribe call; fix by installing a placeholder
managedSpace under m.spaces before doing the network Subscribe so concurrent
callers see the placeholder and can increment refCount instead of duplicating
work: inside acquireBroadcastSubscription, after trimming targetSpace and
finding no entry, create and store &managedSpace{space: targetSpace,
broadcastSub: nil, refCount: 1} while holding m.mu, then unlock and call
m.transport.Subscribe; after Subscribe returns, lock m.mu again and set the
stored managedSpace.broadcastSub = subscription (or, on Subscribe error,
remove/decrement the placeholder and return the error); update co-operating
callers to treat a nil broadcastSub as “subscription in progress” so they only
increment refCount and do not Subscribe themselves (this eliminates reliance on
cleanupDuplicateBroadcastSubscription and removes the TOCTOU window).
In `@internal/network/router.go`:
- Around line 515-529: Add a short clarifying comment above the markSeen method
explaining that r.seen is a sliding-window/bounded cache (bounded by
r.maxReplayAge and cleaned on each markSeen call) so it is not unbounded; if you
want explicit visibility add optional metrics updates when inserting/deleting
(e.g., increment/decrement a seen-size gauge inside markSeen around
r.seen[envelope.ID] assignment and the delete in the cleanup loop) and reference
existing symbols markSeen, r.seen, r.maxReplayAge and replayDeadline when adding
the comment/metrics.
In `@internal/session/manager_helpers.go`:
- Line 128: The helpers that call joinNetworkPeer/leaveNetworkPeer (the
functions in manager_helpers.go that return lifecycle.JoinSpace(...) and
corresponding leave call) must wrap any returned error with session context;
update joinNetworkPeer and leaveNetworkPeer call sites to capture the error and
return fmt.Errorf("joinNetworkPeer %s/%s: %w", info.AgentName, info.ID, err) and
fmt.Errorf("leaveNetworkPeer %s/%s: %w", info.AgentName, info.ID, err)
respectively (or use errors.Wrap) so callers always get consistent session-aware
error messages referencing info.AgentName and info.ID.
🪄 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: 28a2fc33-a466-4e65-a1b8-e2a61921d95f
⛔ Files ignored due to path filters (63)
.compozy/tasks/network/_meta.mdis excluded by!**/*.md.compozy/tasks/network/_tasks.mdis excluded by!**/*.md.compozy/tasks/network/_techspec.mdis excluded by!**/*.md.compozy/tasks/network/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/network/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/network/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/network/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/network/adrs/adr-005.mdis excluded by!**/*.md.compozy/tasks/network/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/network/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_031.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_032.mdis excluded by!**/*.md.compozy/tasks/network/reviews-001/issue_033.mdis excluded by!**/*.md.compozy/tasks/network/task_01.mdis excluded by!**/*.md.compozy/tasks/network/task_02.mdis excluded by!**/*.md.compozy/tasks/network/task_03.mdis excluded by!**/*.md.compozy/tasks/network/task_04.mdis excluded by!**/*.md.compozy/tasks/network/task_05.mdis excluded by!**/*.md.compozy/tasks/network/task_06.mdis excluded by!**/*.md.compozy/tasks/network/task_07.mdis excluded by!**/*.md.compozy/tasks/network/task_08.mdis excluded by!**/*.md.compozy/tasks/network/task_09.mdis excluded by!**/*.md.compozy/tasks/network/task_10.mdis excluded by!**/*.md
📒 Files selected for processing (28)
internal/api/core/errors.gointernal/api/core/network_test.gointernal/api/udsapi/extensions_additional_test.gointernal/cli/helpers_test.gointernal/cli/network_test.gointernal/config/config.gointernal/config/merge_test.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/network/delivery.gointernal/network/delivery_integration_test.gointernal/network/delivery_test.gointernal/network/envelope.gointernal/network/helpers_test.gointernal/network/manager.gointernal/network/manager_test.gointernal/network/router.gointernal/network/router_test.gointernal/session/manager_helpers.gointernal/session/manager_hooks_test.gointernal/session/manager_lifecycle.gointernal/session/stop_reason.gointernal/skills/bundled/bundled_test.gointernal/skills/bundled/content.gointernal/store/globaldb/global_db_network_audit.gointernal/store/globaldb/global_db_network_audit_test.gointernal/store/types.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/cli/helpers_test.go
- internal/session/manager_lifecycle.go
- internal/config/merge_test.go
- internal/daemon/daemon.go
- internal/api/udsapi/extensions_additional_test.go
- internal/network/manager_test.go
- internal/session/manager_hooks_test.go
- internal/daemon/daemon_test.go
- internal/network/delivery_test.go
- internal/cli/network_test.go
- internal/config/config.go
- internal/api/core/errors.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/cli/daemon_wait_test.go (1)
136-142: Assert stop-state explicitly, not onlyNetwork == nil.At Line 140, this test only checks network clearing. Add a
status.Status == "stopped"assertion (optionally PID too) so state-transition regressions are caught.💡 Suggested assertion update
status, err := waitForDaemonStop(testutil.Context(t), deps, runtime, info) if err != nil { t.Fatalf("waitForDaemonStop() error = %v", err) } + if status.Status != "stopped" || status.PID != 42 { + t.Fatalf("waitForDaemonStop() status = %#v, want stopped pid 42", status) + } if status.Network != nil { t.Fatalf("waitForDaemonStop() network = %#v, want nil after stop", status.Network) }As per coding guidelines, "
**/*_test.go: Focus on critical paths: workflow execution, state management, error handling" and "**/*_test.go: MUST test meaningful business logic, not trivial operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/daemon_wait_test.go` around lines 136 - 142, The test only verifies that the network field was cleared after stop; update the assertions in the waitForDaemonStop() test to also assert the daemon reached the stopped state by checking status.Status == "stopped" (and optionally that PID is cleared/zero, e.g., status.PID == 0) after the call to waitForDaemonStop(testutil.Context(t), deps, runtime, info) so state-transition regressions are caught; locate the assertions around variables status and err in the test and add the new equality checks for status.Status (and optional PID) immediately after the Network nil check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/acp/client.go`:
- Around line 570-590: The env handling currently reads/updates the first
matching key but Go uses the last occurrence at process start; update envValue
to iterate from the end of the env slice and return the last matching key
(search backwards), and update setEnvValue to remove all existing entries with
the same prefix then append a single "KEY=VALUE" entry at the end so the new
value is the last occurrence; refer to the envValue and setEnvValue functions to
implement these changes.
In `@internal/cli/daemon_wait_test.go`:
- Around line 92-143: Wrap the existing test body of
TestWaitForDaemonStopClearsStaleNetworkSnapshot in a t.Run subtest (e.g.
t.Run("Should clear stale network snapshot when daemon stops", func(t
*testing.T) { ... })) and move the t.Parallel() call into that subtest function
so the subtest is run in parallel; keep all existing setup, assertions and
references to waitForDaemonStop, loadRuntimeContext, deps, info, and status
unchanged inside the subtest.
---
Nitpick comments:
In `@internal/cli/daemon_wait_test.go`:
- Around line 136-142: The test only verifies that the network field was cleared
after stop; update the assertions in the waitForDaemonStop() test to also assert
the daemon reached the stopped state by checking status.Status == "stopped" (and
optionally that PID is cleared/zero, e.g., status.PID == 0) after the call to
waitForDaemonStop(testutil.Context(t), deps, runtime, info) so state-transition
regressions are caught; locate the assertions around variables status and err in
the test and add the new equality checks for status.Status (and optional PID)
immediately after the Network nil check.
🪄 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: 61f43218-6277-445b-84d6-3960ed3769fc
⛔ Files ignored due to path filters (2)
.compozy/tasks/network/qa/review_001.mdis excluded by!**/*.mdinternal/skills/bundled/skills/agh-network/SKILL.mdis excluded by!**/*.md
📒 Files selected for processing (9)
internal/acp/client.gointernal/acp/client_test.gointernal/cli/command_paths_test.gointernal/cli/daemon.gointernal/cli/daemon_wait_test.gointernal/network/delivery.gointernal/network/delivery_test.gointernal/session/manager_start.gointernal/session/manager_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/network/delivery_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/acp/client_test.go
- internal/cli/command_paths_test.go
- internal/cli/daemon.go
- internal/network/delivery.go
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
internal/acp/client.go (1)
640-660:⚠️ Potential issue | 🟠 MajorFix duplicate environment variable handling to respect last occurrence.
The issue flagged in a prior review remains unaddressed. Go's
os/exechonors the last occurrence of duplicate environment variable keys. The currentenvValuescans forward (returning the first match), andsetEnvValueupdates the first match. Ifbasealready contains duplicates, the pinned values may be shadowed by later entries.🐛 Proposed fix
func envValue(env []string, key string) (string, bool) { prefix := key + "=" - for _, variable := range env { + for i := len(env) - 1; i >= 0; i-- { + variable := env[i] if strings.HasPrefix(variable, prefix) { return variable[len(prefix):], true } } return "", false } func setEnvValue(env []string, key string, value string) []string { prefix := key + "=" entry := prefix + value - for idx, variable := range env { + filtered := env[:0] + for _, variable := range env { if strings.HasPrefix(variable, prefix) { - env[idx] = entry - return env + continue } + filtered = append(filtered, variable) } - return append(env, entry) + return append(filtered, entry) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client.go` around lines 640 - 660, envValue and setEnvValue currently scan forwards and therefore return/update the first occurrence, but exec respects the last duplicate key; change both functions (envValue and setEnvValue) to iterate the env slice from end to start so they find and act on the last occurrence: envValue should return the value from the last matching "key=" entry, and setEnvValue should replace the last matching entry (or append if none) to ensure updates respect exec's last-wins behavior.
🧹 Nitpick comments (3)
internal/acp/client.go (2)
279-294: Consider adding debug logging for observability.
applySessionModesilently returnsnilwhen no preferred mode is found or when preconditions fail. While not incorrect, adding debug-level logging would aid troubleshooting when session mode negotiation doesn't behave as expected.💡 Suggested improvement
func (d *Driver) applySessionMode(ctx context.Context, process *AgentProcess, permissions aghconfig.PermissionMode) error { if ctx == nil || process == nil || process.conn == nil { + d.logger.Debug("skipping session mode: missing context or process") return nil } modeID := preferredSessionMode(process.Caps.SupportedModes, permissions) if modeID == "" { + d.logger.Debug("no matching session mode found", slog.String("permissions", string(permissions)), slog.Any("supported", process.Caps.SupportedModes)) return nil } + d.logger.Debug("applying session mode", slog.String("mode", modeID)) _, err := acpsdk.SendRequest[acpsdk.SetSessionModeResponse](process.conn, ctx, acpsdk.AgentMethodSessionSetMode, acpsdk.SetSessionModeRequest{ SessionId: acpsdk.SessionId(process.SessionID), ModeId: acpsdk.SessionModeId(modeID), }) return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client.go` around lines 279 - 294, The applySessionMode function currently returns silently in precondition-failure and "no preferred mode" cases, which reduces observability; add debug-level logs inside applySessionMode: log when ctx==nil or process==nil or process.conn==nil (including process.SessionID or process.Caps where available), log when preferredSessionMode(process.Caps.SupportedModes, permissions) returns empty (include permissions and SupportedModes), and log the response/error from acpsdk.SendRequest (include ModeId, SessionId and err) so callers can trace session mode negotiation; keep logs at debug/trace level and use the same logger used elsewhere for AgentProcess or the package logger.
591-619: Add debug logging when executable resolution fails.When
os.Executable()orfilepath.EvalSymlinks()fails, the function silently returns without settingAGH_BINor modifyingPATH. While the fallback behavior (returning unmodified env) is safe, the silent failure makes it difficult to diagnose issues where child processes cannot locateagh.💡 Suggested improvement
-func daemonMatchedEnv(base []string) []string { +func daemonMatchedEnv(logger *slog.Logger, base []string) []string { env := append([]string(nil), base...) if len(env) == 0 { env = os.Environ() } executable, err := os.Executable() if err != nil { + if logger != nil { + logger.Debug("unable to resolve executable path", slog.Any("error", err)) + } return env } if resolved, resolveErr := filepath.EvalSymlinks(executable); resolveErr == nil && strings.TrimSpace(resolved) != "" { executable = resolved + } else if resolveErr != nil && logger != nil { + logger.Debug("unable to resolve executable symlinks", slog.String("path", executable), slog.Any("error", resolveErr)) }Then update the call site in
spawnProcessto passd.logger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client.go` around lines 591 - 619, The function daemonMatchedEnv currently returns silently on failures from os.Executable() or filepath.EvalSymlinks(), so add debug logging there: inside daemonMatchedEnv log the error when os.Executable() returns an error and when filepath.EvalSymlinks() returns resolveErr (including the error text and the original executable path) before returning; keep behavior unchanged otherwise. Use the daemon's logger (pass it into daemonMatchedEnv or accept a logger param) so logs can be emitted, and update the spawnProcess call site to pass d.logger into daemonMatchedEnv (or into the new logger-accepting variant) so these failures are recorded; keep usages of setEnvValue, envValue, and prependPathEntry unchanged.internal/network/lifecycle.go (1)
186-194: Harden participant invariants for direct/recipe envelopes.For existing interactions, direct/recipe handling validates
env.Frombut does not enforce thatenv.Tois present and belongs to the same participant pair. Adding this check prevents out-of-pair routing from mutating lifecycle state.Proposed patch
switch env.Kind { case KindDirect, KindRecipe: + if env.To == nil { + return LifecycleResult{}, fmt.Errorf("%w: direct/recipe to is required", ErrMissingField) + } + if !interaction.IsParticipant(*env.To) { + return LifecycleResult{}, fmt.Errorf("%w: to=%q", ErrInteractionActorNotAllowed, *env.To) + } if interaction.State == StateNeedsInput { interaction.State = StateWorking interaction.UpdatedAt = at return LifecycleResult{Interaction: interaction, Action: LifecycleActionAdvanced}, nil }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/network/lifecycle.go`:
- Around line 42-44: Return values that directly propagate errors (e.g., the
ValidateSpace(i.Space) check) must be wrapped with operation context using
fmt.Errorf("...: %w", err); update every direct "return err" in
internal/network/lifecycle.go (including the six occurrences reported around
lines 42-44, 57-58, 122-124, 151-153, 195-198, 205-208) to wrap the underlying
error with a short descriptive message of the failing operation (for example:
fmt.Errorf("validating space %q: %w", i.Space, err) or similar for each
operation), so callers get explicit context while preserving the original error
with %w.
In `@internal/network/manager_test.go`:
- Around line 44-59: Update TestNewManagerRequiresEnabledConfigAndPrompter to
assert specific validation failures instead of any non-nil error: call
NewManager for each case (disabled config via testManagerConfig() with
cfg.Enabled=false, nil prompter via passing nil for newFakeDeliveryPrompter(),
and nil context via nilTestContext()) and replace the generic err==nil checks
with assertions that inspect the error content/type (use testing helpers like
ErrorContains or ErrorAs) to verify the correct validation reason is returned
for each invocation of NewManager.
- Around line 859-891: Add a compile-time interface assertion to ensure
recordingAuditWriter implements AuditWriter: after the recordingAuditWriter type
definition, add the statement that assigns (*recordingAuditWriter)(nil) to a
variable of type AuditWriter (var _ AuditWriter = (*recordingAuditWriter)(nil))
so the compiler will catch any interface drift for the recordingAuditWriter
methods (RecordSent, RecordReceived, RecordRejected).
- Around line 44-252: Tests in TestNewManagerRequiresEnabledConfigAndPrompter,
TestNewManagerReportsRollbackShutdownFailures,
TestManagerJoinSendStatusAndLeave, and
TestManagerQueuesBusyDeliveriesTracksDisconnectsAndShutsDownIdempotently must be
converted from long top-level test bodies into subtests using t.Run("Should...")
for each independent case; for each case create a t.Run with the descriptive
"Should..." name, call t.Parallel() inside the subtest, move the relevant
assertions/setup into that subtest closure, and preserve existing helpers (e.g.
newFakeDeliveryPrompter, testManagerConfig, mustRawJSON, manager methods) and
defer/cleanup semantics; ensure repeated/shorthand cases (like repeated
LeaveSpace and repeated Shutdown) become their own t.Run cases or are clearly
named subcases so failures localize.
In `@internal/network/manager.go`:
- Around line 928-949: The recordAuditSent, recordAuditReceived (and the similar
recordAuditRejected) helpers call
m.stats.recordSent/recordReceived/recordRejected without checking m.stats and
can panic when m.stats is nil; update these functions (recordAuditSent,
recordAuditReceived, and recordAuditRejected) to guard m.stats == nil before
calling any m.stats.* methods (skip calling
recordSent/recordReceived/recordRejected if m.stats is nil) so the functions no
longer panic when Manager is partially constructed but still perform
logging/auditing as before.
---
Duplicate comments:
In `@internal/acp/client.go`:
- Around line 640-660: envValue and setEnvValue currently scan forwards and
therefore return/update the first occurrence, but exec respects the last
duplicate key; change both functions (envValue and setEnvValue) to iterate the
env slice from end to start so they find and act on the last occurrence:
envValue should return the value from the last matching "key=" entry, and
setEnvValue should replace the last matching entry (or append if none) to ensure
updates respect exec's last-wins behavior.
---
Nitpick comments:
In `@internal/acp/client.go`:
- Around line 279-294: The applySessionMode function currently returns silently
in precondition-failure and "no preferred mode" cases, which reduces
observability; add debug-level logs inside applySessionMode: log when ctx==nil
or process==nil or process.conn==nil (including process.SessionID or
process.Caps where available), log when
preferredSessionMode(process.Caps.SupportedModes, permissions) returns empty
(include permissions and SupportedModes), and log the response/error from
acpsdk.SendRequest (include ModeId, SessionId and err) so callers can trace
session mode negotiation; keep logs at debug/trace level and use the same logger
used elsewhere for AgentProcess or the package logger.
- Around line 591-619: The function daemonMatchedEnv currently returns silently
on failures from os.Executable() or filepath.EvalSymlinks(), so add debug
logging there: inside daemonMatchedEnv log the error when os.Executable()
returns an error and when filepath.EvalSymlinks() returns resolveErr (including
the error text and the original executable path) before returning; keep behavior
unchanged otherwise. Use the daemon's logger (pass it into daemonMatchedEnv or
accept a logger param) so logs can be emitted, and update the spawnProcess call
site to pass d.logger into daemonMatchedEnv (or into the new logger-accepting
variant) so these failures are recorded; keep usages of setEnvValue, envValue,
and prependPathEntry unchanged.
🪄 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: 236792ea-6265-4a0b-a561-480851222c0b
⛔ Files ignored due to path filters (7)
.agh/memory/MEMORY.mdis excluded by!**/*.md.agh/memory/agh-network-foundation.mdis excluded by!**/*.md.agh/memory/agh-network-safety.mdis excluded by!**/*.md.claude/settings.local.jsonis excluded by!**/*.json,!.claude/**.compozy/tasks/network/qa/review_002.mdis excluded by!**/*.md.compozy/tasks/network/qa/review_003.mdis excluded by!**/*.mdinternal/skills/bundled/skills/agh-network/SKILL.mdis excluded by!**/*.md
📒 Files selected for processing (20)
internal/acp/client.gointernal/acp/client_test.gointernal/network/delivery.gointernal/network/delivery_test.gointernal/network/helpers_test.gointernal/network/lifecycle.gointernal/network/lifecycle_test.gointernal/network/manager.gointernal/network/manager_test.gointernal/network/router.gointernal/network/router_integration_test.gointernal/network/router_test.gointernal/network/validate.gointernal/network/validate_test.gointernal/skills/bundled/bundled_test.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_network_audit_test.gointernal/store/globaldb/migrate_workspace.gointernal/store/sqlite.gointernal/store/store_extra_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/network/helpers_test.go
- internal/network/router_test.go
- internal/network/delivery.go
- internal/network/router.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/network/lifecycle_test.go
- internal/network/router_integration_test.go
- internal/acp/client_test.go
- internal/network/delivery_test.go
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/network/lifecycle.go (1)
148-153:⚠️ Potential issue | 🟠 MajorWrap the forwarded
OpenInteractionerror with call-site context.At Line 153,
erris returned directly, which drops theApplyInteractionEnvelopeoperation context and breaks the file’s otherwise consistent wrapping style.Proposed fix
opened, err := OpenInteraction(env, at) if err != nil { if env.Kind != KindDirect && env.Kind != KindRecipe { return LifecycleResult{}, fmt.Errorf("%w: kind=%q", ErrInteractionNotFound, env.Kind) } - return LifecycleResult{}, err + return LifecycleResult{}, fmt.Errorf("open interaction: %w", err) }#!/bin/bash # Verify direct error propagation sites in internal/network/lifecycle.go rg -nP 'return\s+(LifecycleResult\{\}|Interaction\{\}),\s*err\b|return\s+err\b' internal/network/lifecycle.go -C2As 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/network/lifecycle.go` around lines 148 - 153, The forwarded error from OpenInteraction is returned raw, losing the ApplyInteractionEnvelope call-site context; replace the direct return of err with a wrapped error (e.g. return LifecycleResult{}, fmt.Errorf("ApplyInteractionEnvelope: %w", err)) in the error branch where OpenInteraction(env, at) fails (refer to OpenInteraction and the ApplyInteractionEnvelope call site) so the error preserves the operation context.
🧹 Nitpick comments (3)
internal/network/delivery_integration_test.go (3)
114-115: Same fire-and-forget pattern as in the first test.Both goroutines lack tracking. Apply the same WaitGroup pattern here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/delivery_integration_test.go` around lines 114 - 115, The two goroutines calling drainAgentEvents(userEventsA) and drainAgentEvents(userEventsB) are launched fire-and-forget and need synchronization; update the test to use a sync.WaitGroup: add a wg variable, wg.Add(2) before starting goroutines, pass a reference (or call wg.Done() inside drainAgentEvents) so each invocation signals completion, and call wg.Wait() before the test exits. Ensure drainAgentEvents either accepts a *sync.WaitGroup parameter and calls wg.Done() or that you wrap its calls in anonymous goroutines that defer wg.Done(), referencing the function name drainAgentEvents and the channels userEventsA/userEventsB.
43-43: Fire-and-forget goroutine violates coding guidelines.This goroutine is not tracked with
sync.WaitGroupor equivalent. While in this test context the risk is lower (the goroutine will exit whenuserEventschannel is closed), it violates the "No fire-and-forget goroutines" guideline.Consider adding a WaitGroup or ensuring the drain completes before test exits:
♻️ Proposed fix using sync.WaitGroup
+ var drainWg sync.WaitGroup + drainWg.Add(1) - go drainAgentEvents(userEvents) + go func() { + defer drainWg.Done() + drainAgentEvents(userEvents) + }()Then at the end of the test:
drainWg.Wait()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/network/delivery_integration_test.go` at line 43, The test launches a fire-and-forget goroutine calling drainAgentEvents(userEvents) without tracking it; change to use a sync.WaitGroup (e.g., drainWg): increment before starting the goroutine, have the goroutine call defer drainWg.Done() (either by wrapping drainAgentEvents in a closure or by adding a wg parameter to drainAgentEvents), and then call drainWg.Wait() before the test exits to ensure the drain completes; update references to drainAgentEvents/userEvents accordingly so the goroutine is properly awaited.
368-385: Function name is misleading — consider renaming for clarity.The name
assertPromptCountEventuallysuggests it waits for a count to reach a value (likeEventuallyin assertion libraries). However, this function actually asserts that the count remains stable atwantfor 150ms, failing immediately if it changes.A clearer name would be
assertPromptCountStableorassertPromptCountRemainsAt.♻️ Proposed rename
-func assertPromptCountEventually(t *testing.T, driver *integrationPromptDriver, want int) { +func assertPromptCountStable(t *testing.T, driver *integrationPromptDriver, want int) { t.Helper() timer := time.NewTimer(150 * time.Millisecond)Also update the call site at line 70:
- assertPromptCountEventually(t, driver, 2) + assertPromptCountStable(t, driver, 2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/network/delivery_integration_test.go` around lines 368 - 385, Rename the misleading test helper assertPromptCountEventually to a name that reflects its behavior (e.g., assertPromptCountStable or assertPromptCountRemainsAt) and update all call sites to use the new name; specifically change the function signature for assertPromptCountEventually to assertPromptCountStable (keeping the same parameters: t *testing.T, driver *integrationPromptDriver, want int) and leave the implementation (checking driver.promptCount() stays equal to want and waiting on driver.promptNotify or a 150ms timer) intact, then replace any calls that reference assertPromptCountEventually (including the call near the earlier test root) to call assertPromptCountStable so the name matches the semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/acp/client.go`:
- Around line 154-160: The terminal-launch path still uses
mergeCommandEnv(os.Environ(), request.Env) so it doesn’t inherit the pinned
AGH_BIN/PATH produced by daemonMatchedEnv(normalized.Env); update the terminal
execution to merge the daemon-adjusted environment instead of the raw process
env — e.g., call mergeCommandEnv(daemonMatchedEnv(normalized.Env), request.Env)
(or explicitly inject AGH_BIN/PATH from daemonMatchedEnv) in the handler that
launches terminal commands (the code that currently calls mergeCommandEnv) and
apply the same change to the other analogous block around the 591-619 region so
terminal commands resolve the same binary path as subprocess.Launch.
- Around line 154-160: The spawnProcess function currently drops the caller's
ctx and calls subprocess.Launch with context.Background(); change spawnProcess
to accept a context.Context as its first parameter and pass that ctx through
into subprocess.Launch instead of context.Background(). Update the Start
function (and any other callers) to forward its caller-owned ctx into
spawnProcess, and adjust the spawnProcess signature and any variable names
accordingly so cancellation and timeouts propagate to subprocess.Launch.
- Around line 621-638: The helper currently drops empty PATH segments by
trimming and skipping blank segments; change it to preserve original segments
except when they duplicate the injected entry: compute cleanEntry :=
strings.TrimSpace(entry) as before, then iterate the original segments (without
removing or trimming them) and skip only those whose strings.TrimSpace(segment)
== cleanEntry; append all other segments (including empty "" segments)
unchanged, and finally join with the os.PathListSeparator with cleanEntry
prepended; update the function prependPathEntry accordingly so only duplicates
of cleanEntry are de-duplicated and empty segments are preserved.
In `@internal/network/lifecycle.go`:
- Around line 48-50: Change the error-wrapping order to put the context string
first for the ValidatePeerID calls: replace occurrences that use fmt.Errorf("%w:
interaction initiator", err) (and the similar "%w: ..." at the other occurrence)
with the context-first pattern e.g. fmt.Errorf("interaction initiator: %w", err)
so that both ValidatePeerID(i.Initiator) and the other ValidatePeerID call
follow the "context: %w" guideline.
---
Duplicate comments:
In `@internal/network/lifecycle.go`:
- Around line 148-153: The forwarded error from OpenInteraction is returned raw,
losing the ApplyInteractionEnvelope call-site context; replace the direct return
of err with a wrapped error (e.g. return LifecycleResult{},
fmt.Errorf("ApplyInteractionEnvelope: %w", err)) in the error branch where
OpenInteraction(env, at) fails (refer to OpenInteraction and the
ApplyInteractionEnvelope call site) so the error preserves the operation
context.
---
Nitpick comments:
In `@internal/network/delivery_integration_test.go`:
- Around line 114-115: The two goroutines calling drainAgentEvents(userEventsA)
and drainAgentEvents(userEventsB) are launched fire-and-forget and need
synchronization; update the test to use a sync.WaitGroup: add a wg variable,
wg.Add(2) before starting goroutines, pass a reference (or call wg.Done() inside
drainAgentEvents) so each invocation signals completion, and call wg.Wait()
before the test exits. Ensure drainAgentEvents either accepts a *sync.WaitGroup
parameter and calls wg.Done() or that you wrap its calls in anonymous goroutines
that defer wg.Done(), referencing the function name drainAgentEvents and the
channels userEventsA/userEventsB.
- Line 43: The test launches a fire-and-forget goroutine calling
drainAgentEvents(userEvents) without tracking it; change to use a sync.WaitGroup
(e.g., drainWg): increment before starting the goroutine, have the goroutine
call defer drainWg.Done() (either by wrapping drainAgentEvents in a closure or
by adding a wg parameter to drainAgentEvents), and then call drainWg.Wait()
before the test exits to ensure the drain completes; update references to
drainAgentEvents/userEvents accordingly so the goroutine is properly awaited.
- Around line 368-385: Rename the misleading test helper
assertPromptCountEventually to a name that reflects its behavior (e.g.,
assertPromptCountStable or assertPromptCountRemainsAt) and update all call sites
to use the new name; specifically change the function signature for
assertPromptCountEventually to assertPromptCountStable (keeping the same
parameters: t *testing.T, driver *integrationPromptDriver, want int) and leave
the implementation (checking driver.promptCount() stays equal to want and
waiting on driver.promptNotify or a 150ms timer) intact, then replace any calls
that reference assertPromptCountEventually (including the call near the earlier
test root) to call assertPromptCountStable so the name matches the semantics.
🪄 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: e1656493-0757-482f-b950-561557e25873
⛔ Files ignored due to path filters (22)
.compozy/tasks/network/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_014.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_015.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_016.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_017.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_018.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_019.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_020.mdis excluded by!**/*.md.compozy/tasks/network/reviews-002/issue_021.mdis excluded by!**/*.md
📒 Files selected for processing (9)
internal/acp/client.gointernal/acp/client_test.gointernal/api/core/network_test.gointernal/cli/daemon_wait_test.gointernal/network/delivery_integration_test.gointernal/network/lifecycle.gointernal/network/lifecycle_test.gointernal/network/manager.gointernal/network/manager_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/api/core/network_test.go
- internal/network/manager_test.go
- internal/network/manager.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/daemon_wait_test.go
- internal/network/lifecycle_test.go
- internal/acp/client_test.go
Comment on lines
+154
to
+160
| processEnv := daemonMatchedEnv(normalized.Env) | ||
|
|
||
| managed, err := subprocess.Launch(context.Background(), subprocess.LaunchConfig{ | ||
| Command: command, | ||
| Args: args, | ||
| Dir: normalized.Cwd, | ||
| Env: normalized.Env, | ||
| Env: processEnv, |
There was a problem hiding this comment.
Propagate the pinned AGH_BIN/PATH to terminal execution too.
This only reshapes the ACP agent subprocess environment. Terminal commands are still launched from internal/acp/handlers.go:418-424 with mergeCommandEnv(os.Environ(), request.Env), so agh network ... can still resolve against the daemon's ambient PATH instead of the binary pinned here. That leaves the new network path inconsistent between agent startup and terminal execution.
Also applies to: 591-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/acp/client.go` around lines 154 - 160, The terminal-launch path
still uses mergeCommandEnv(os.Environ(), request.Env) so it doesn’t inherit the
pinned AGH_BIN/PATH produced by daemonMatchedEnv(normalized.Env); update the
terminal execution to merge the daemon-adjusted environment instead of the raw
process env — e.g., call mergeCommandEnv(daemonMatchedEnv(normalized.Env),
request.Env) (or explicitly inject AGH_BIN/PATH from daemonMatchedEnv) in the
handler that launches terminal commands (the code that currently calls
mergeCommandEnv) and apply the same change to the other analogous block around
the 591-619 region so terminal commands resolve the same binary path as
subprocess.Launch.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that Driver.Start receives a request context but spawnProcess discards it.
rg -n -C2 'func \(d \*Driver\) Start|func \(d \*Driver\) spawnProcess|subprocess\.Launch\(context\.Background' internal/acp/client.goRepository: compozy/agh
Length of output: 717
Pass the request context through to subprocess launch.
Start receives a caller-owned ctx at line 119, but spawnProcess (line 143) discards it and calls subprocess.Launch(context.Background(), ...) at line 156. This breaks cancellation propagation—if the caller cancels the request context, the subprocess launch cannot be interrupted.
Refactor spawnProcess to accept context.Context as its first parameter and use it in the subprocess launch call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/acp/client.go` around lines 154 - 160, The spawnProcess function
currently drops the caller's ctx and calls subprocess.Launch with
context.Background(); change spawnProcess to accept a context.Context as its
first parameter and pass that ctx through into subprocess.Launch instead of
context.Background(). Update the Start function (and any other callers) to
forward its caller-owned ctx into spawnProcess, and adjust the spawnProcess
signature and any variable names accordingly so cancellation and timeouts
propagate to subprocess.Launch.
Comment on lines
+621
to
+638
| func prependPathEntry(pathValue string, entry string) string { | ||
| cleanEntry := strings.TrimSpace(entry) | ||
| if cleanEntry == "" { | ||
| return pathValue | ||
| } | ||
|
|
||
| separator := string(os.PathListSeparator) | ||
| segments := strings.Split(pathValue, separator) | ||
| filtered := make([]string, 0, len(segments)) | ||
| for _, segment := range segments { | ||
| trimmed := strings.TrimSpace(segment) | ||
| if trimmed == "" || trimmed == cleanEntry { | ||
| continue | ||
| } | ||
| filtered = append(filtered, trimmed) | ||
| } | ||
| return strings.Join(append([]string{cleanEntry}, filtered...), separator) | ||
| } |
There was a problem hiding this comment.
Preserve empty PATH segments when prepending.
Dropping "" entries changes existing search semantics: an empty PATH segment represents the current working directory on Unix. This helper should only de-duplicate the injected entry, not remove other path entries.
💡 Suggested fix
func prependPathEntry(pathValue string, entry string) string {
cleanEntry := strings.TrimSpace(entry)
if cleanEntry == "" {
return pathValue
}
separator := string(os.PathListSeparator)
segments := strings.Split(pathValue, separator)
filtered := make([]string, 0, len(segments))
for _, segment := range segments {
- trimmed := strings.TrimSpace(segment)
- if trimmed == "" || trimmed == cleanEntry {
+ if segment == "" {
+ filtered = append(filtered, segment)
+ continue
+ }
+ if strings.TrimSpace(segment) == cleanEntry {
continue
}
- filtered = append(filtered, trimmed)
+ filtered = append(filtered, segment)
}
return strings.Join(append([]string{cleanEntry}, filtered...), separator)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/acp/client.go` around lines 621 - 638, The helper currently drops
empty PATH segments by trimming and skipping blank segments; change it to
preserve original segments except when they duplicate the injected entry:
compute cleanEntry := strings.TrimSpace(entry) as before, then iterate the
original segments (without removing or trimming them) and skip only those whose
strings.TrimSpace(segment) == cleanEntry; append all other segments (including
empty "" segments) unchanged, and finally join with the os.PathListSeparator
with cleanEntry prepended; update the function prependPathEntry accordingly so
only duplicates of cleanEntry are de-duplicated and empty segments are
preserved.
Comment on lines
+48
to
+50
| if err := ValidatePeerID(i.Initiator); err != nil { | ||
| return fmt.Errorf("%w: interaction initiator", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify non-standard wrapping order patterns in internal/network/lifecycle.go
rg -nP 'fmt\.Errorf\("%w:[^"]*",\s*err\)' internal/network/lifecycle.go -C2Repository: compozy/agh
Length of output: 372
Use context-first wrapping format for error messages.
Lines 49 and 55 use "%w: ..." format. Change to "context: %w" for consistency with the coding guidelines.
Proposed fix
if err := ValidatePeerID(i.Initiator); err != nil {
- return fmt.Errorf("%w: interaction initiator", err)
+ return fmt.Errorf("validate interaction initiator: %w", err)
}
@@
if err := ValidatePeerID(i.Target); err != nil {
- return fmt.Errorf("%w: interaction target", err)
+ return fmt.Errorf("validate interaction target: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/network/lifecycle.go` around lines 48 - 50, Change the
error-wrapping order to put the context string first for the ValidatePeerID
calls: replace occurrences that use fmt.Errorf("%w: interaction initiator", err)
(and the similar "%w: ..." at the other occurrence) with the context-first
pattern e.g. fmt.Errorf("interaction initiator: %w", err) so that both
ValidatePeerID(i.Initiator) and the other ValidatePeerID call follow the
"context: %w" guideline.
This was referenced May 23, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.1 This PR prepares the release of version v0.0.1. ### Changelog ## 0.0.1 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Merged
pedronauck
added a commit
that referenced
this pull request
May 26, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced May 26, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-26 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated web assets dependency to a newer version for improved stability and performance. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/211?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This was referenced May 27, 2026
Closed
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
May 27, 2026
## Release v0.0.2 This PR prepares the release of version v0.0.2. ### Changelog ## 0.0.2 - 2026-05-27 ### Other Changes - Lessons learned ### ♻️ Refactoring - Project structure (#7) - Kb improvements (#12) - Rename spaces to channels (#17) - Add extensions gaps (#21) - Improve tool calls ui (#22) - Remove web app header - Module improvements (#29) - Memory improvements (#35) - Storybook for web and ui (#38) - Enable AGH network by default for new installs (#57) - Hermes adjustments (#69) - Badges design (#84) - Storybook scenario and logos gallery - Migrate typescript tests (#114) - Internal go packages (#120) - Ui patterns (#127) - Improve e2e tests (#130) - Ui redesign - Workspace isolation across runtime surfaces (#145) - Prod ready applies (#162) - Tool card ui (#164) - Alpha on logo - Prod ready features (#167) - Thread sheet (#202) ### 🎉 Features - Implement config foundation packages - Implement sqlite store package - Add ACP client package - Add session lifecycle manager - Implement observe package - Add daemon composition root - Add uds api server - Implement cli package - Add http api server - Add system design - Add foundation types, schemas, and layout shell for web client - Add daemon health polling and agent sidebar systems for web client - Add session system CRUD, streaming core, and session store for web client - Add chat view, messages, and composer tests for web client - Add tool cards and renderers for web client - Add file-backed memory store core - Scaffold memory session seams - Add memory dream consolidation service - Wire memory assembler into daemon - Add memory api and cli - New skills system (#1) - Add workspace entity (#5) - Add new skill capabilities (#8) - Web ui v2 (#9) - Improve hooks system (#10) - Session resilience (#11) - Add extensability (#13) - Add automation (#16) - Add channels (#14) - Add network implementation (#15) - Add network, bridges and automations web pages (#18) - Ext registry (#20) - Add core tasks (#19) - Bridge adapters (#23) - Add site (#26) - Add ext refac and sandbox (#25) - Settings ui (#37) - Tasks ui (#36) - Harness improvements (#44) - Agent capabilities (#49) - Redesign ui (#48) - Unify capability (#53) - Redesign network workspace (#59) - Add task deletion and split session delete from stop (#58) - Session provider selection (#60) - Production grade adjustments (#66) - Autonomous system (#75) - Add agent session route (#80) - Tools registry (#85) - Agents soul (#88) - Add network threads (#105) - Orchestration improvements (#106) - Memory v2 (#108) - Agent categories (#113) - Providers model (#118) - Add canonical AGH bundled skill (#143) - Onboarding and improvements (#198) - Onboarding and improvements (#201) ### 🐛 Bug Fixes - Review round - Review rounds - Resolve memory extensibility review batch - Embed web into daemon - Defaults agents - Acp integration (#4) - Lint errors - Prd folder - Remove orphan web actions and dead surfaces (#55) - Qa testing and fixes (#73) - New review rounds (#82) - Security audit (#90) - Release qa round (#95) - Add missing tools (#141) - New qa round (#147) - Advanced qa round (#149) - Homebrew tap - Final review round (#151) - Daemon healthy - Reasoning models (#158) - Lint errors (#160) - Review round (#168) - Release adjustments (#171) - Stabilize release ci fixtures - Stabilize release integration gate - Stabilize release verify gates - Stabilize release integration flows - Stabilize release verify gates - Stabilize main verify shutdown - Ignore stale acpmock cancel - Marketplace search focus and filtering (#193) - Website video - Workspace command select ### 📚 Documentation - Update agents.md - Update prd - Update skills - Update compozy tasks - Update compozy - Update compozy - Add new skills - Archive prd - Update prds - Update rfc - Update prds - Update prds - Add automation prd - Channels prd - Update prd - Update prd - New prds - Archive prds - Bridges adapters prd - Sandbox prd - Update - Archive prd - Update - Add new prd - New design - Update prd - Archive prds - Update prds - Tasks-ui prd tasks - Update prd - Update design docs - Agent capabilities prd - Improve site docs - Remove old design references - Udpate - Autonomous prd - Update skills - Blog design - Agent sould prd - Final qa plan - Update - Remove codex ledgers from gitignore - Remove not needed files - Udpate ledger - Update cy-codex-loop skill - Orchestration improves prd - Update prds - Orch improvs prd - Memv2 prd - Providers model prd - Update refacs prd - New design proposal - Update rules - Update skills - New blog posts (#173) - Format docs - Remove old design files - Remove old - Skeeper update ### 📦 Build System - Initial structure - Commitlint - Frontend base structure - Update vscode settings - Add subagents - Coderabbit - Prd and tooling - Bun lock - Lint tooling - Copy.md and tooling adjusts - Add repoclone rc - Upgrade skeeper to v0.2.0 - Update go.mod - Adopt task artifacts into skeeper - Sync codex plans with skeeper - Skeeper lock - Skeeper lock - New skills - Skeeper lock - Skeeper lock - Skeeper lock - Update deps and go - Regenerate daytona sidecar assets for go 1.26.3 - Fix cliff - Ignore docs on fmt - Build web assets before goreleaser - Extend release dry-run timeout - Fix release dry-run token contract ### 🔧 CI/CD - Lint errors - Fint release pr - Fix goreleaser - Fix release - Fix release process - Fix release sync - Decouple release dry-run npm auth - Persist web assets git auth - Require npm auth before release merge ### 🧪 Testing - Add e2e tests (#27) - Qa rounds (#78) - Improve test suite (#138) - Harden daemon-served restart reloads - Harden daemon-served readiness waits - Stabilize dashboard focus assertion - Stabilize release integration gates - Stabilize release e2e markers - Stabilize release e2e flows - Improve suite speed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated dependencies to latest versions. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
](https://app.coderabbit.ai/change-stack/compozy/agh/pull/214?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
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