feat: add network threads#105
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds prompt-stop suppression for certain ACP prompt errors; and a large network redesign introducing surfaces (thread/direct), Work-centric lifecycle (WorkID/WorkState), durable conversation persistence, new network hooks/events, expanded API/CLI/HostAPI surfaces, contract changes, and extensive tests. ChangesACP Prompt Stop Short-circuit
Network & Hooks Overhaul
Sequence Diagram(s)sequenceDiagram
participant Client
participant Manager
participant ConversationStore
participant HookDispatcher
participant AuditStore
Client->>Manager: Send/Receive envelope (with Surface/WorkID)
Manager->>ConversationStore: WriteConversationMessage(entry)
ConversationStore-->>Manager: WriteResult(message_id, work_id, duplicate)
alt not duplicate
Manager->>HookDispatcher: DispatchNetworkHooks(payload per event)
HookDispatcher-->>Manager: Ack / error
Manager->>AuditStore: WriteNetworkAudit(entry with persisted flags)
else duplicate
Manager->>AuditStore: WriteNetworkAudit(mark duplicate)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
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 (5)
internal/extension/host_api_bridges.go (1)
577-586:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't block bridge ingest on full prompt completion.
drainAgentEventsonly returns when the prompt event channel closes. Calling it inline here turns message ingest into a full prompt wait, andregisterPromptDelivery(...)does not run until after the terminal event. That effectively disables live bridge delivery and can hang this RPC on slow or long-running prompts.💡 Minimal fix
eventsCh, err := h.promptBridgeSession( context.WithoutCancel(ctx), sessionID, message, bridgePromptNetworkMeta(envelope), ) if err != nil { return hostAPIPromptSubmission{}, err } - drainAgentEvents(eventsCh) + go drainAgentEvents(eventsCh) events, err := h.sessions.Events(ctx, sessionID, store.EventQuery{ AfterSequence: lastSequence, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/host_api_bridges.go` around lines 577 - 586, The code currently calls drainAgentEvents(eventsCh) inline which blocks until the prompt channel closes and prevents registerPromptDelivery(...) from running; change this to run the drain in the background (e.g., spawn a goroutine: go drainAgentEvents(eventsCh)) so promptBridgeSession returns immediately and registerPromptDelivery can proceed without waiting for full prompt completion.internal/extension/host_api.go (1)
1774-1780:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't block Host API prompt submission on the full agent event stream.
drainAgentEvents(eventsCh)now waits for the prompt to finish beforesessions/create/sessions/promptreturn. For blocked or long-running prompts, that can hang the RPC indefinitely, andcontext.WithoutCancel(ctx)means client cancellation no longer bounds the wait. This should keep draining in the background, or stop once the turn-init event is durable and return from there.As per coding guidelines, "Detached execution lifetime — work that outlives an HTTP/UDS request must detach via
context.WithoutCancel(ctx), never tie execution lifetime to request lifetime" and "context.WithoutCanceldoes NOT preserve deadlines — re-attach a deadline if needed".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/host_api.go` around lines 1774 - 1780, After calling h.sessions.Prompt, don't block on drainAgentEvents; instead spawn it in a detached goroutine so the RPC can return immediately: create detachedCtx := context.WithoutCancel(ctx) and if the incoming ctx has a deadline re-attach that deadline to detachedCtx (use context.WithDeadline) before launching the goroutine, then run drainAgentEvents(eventsCh) inside that goroutine (using detachedCtx if drainAgentEvents needs a ctx). This keeps h.sessions.Prompt/sessionID/message from hanging while still continuing to consume events in the background and preserves any original deadline semantics.internal/api/core/network_test.go (1)
324-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the new direct-room metadata in this test.
SurfaceandDirectIDare now part of the fixture, but this case still only verifies trace/proof/ext fields.NetworkEnvelopePayloadFromEnvelopecould drop either conversation field and this test would stay green.As per coding guidelines, tests must verify behavior outcomes, not just function calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/core/network_test.go` around lines 324 - 343, The test currently builds a network.Envelope with Surface and DirectID but doesn't assert they survive conversion; update the test that calls NetworkEnvelopePayloadFromEnvelope to also assert the resulting payload includes the direct-room metadata (e.g. payload.Surface equals network.SurfaceDirect or the expected enum/string and payload.DirectID equals "direct_0123456789abcdef0123456789abcdef"); ensure the assertions reference the network.Envelope fixture fields (Surface, DirectID) and the conversion function NetworkEnvelopePayloadFromEnvelope so the test fails if those conversation fields are dropped.internal/network/router.go (1)
297-326:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
PublishPreparedcan publish an envelope that never went throughPrepareSend.
SendResultis exported and mutable, and this method does not re-run envelope validation, target-presence checks, or lifecycle checks before publishing. It also returnsprepared.Subjectverbatim even thoughpublishEnvelope()recomputes the real subject internally, so the returned subject can diverge from what was actually published.At minimum, recompute the subject here and rerun the same validation/preflight guards, or make the prepared token opaque so callers cannot forge it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/router.go` around lines 297 - 326, PublishPrepared currently accepts a mutable SendResult and skips validation/preflight, allowing callers to publish envelopes that never passed PrepareSend and return a subject that may not match publishEnvelope's computed subject; fix by re-running the same validation/guards used by PrepareSend (target presence, envelope validation, lifecycle checks) inside PublishPrepared before calling publishEnvelope, recompute the subject with subjectForEnvelope (or use the subject value returned/used by publishEnvelope) instead of trusting prepared.Subject, and treat SendResult as an opaque token for callers (or document/make fields unexported) so callers cannot forge prepared tokens; update references to PublishPrepared, SendResult, publishEnvelope, subjectForEnvelope, PrepareSend, and syncSentLifecycle accordingly.internal/network/lifecycle.go (1)
231-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the
receipt/tracerecipient against the work counterparty.This function only validates
toforsay/capability. A participant can therefore send areceiptortracefor workA↔Bto some other peer in the same thread/direct container and it will still passvalidateWorkEnvelope, because onlyfromis checked. The router will then deliver that lifecycle message to the addressed local peer even though they are not part of the work.💡 Suggested fix
func validateWorkDirection(current Work, env Envelope) error { - if env.Kind != KindSay && env.Kind != KindCapability { - return nil - } if env.To == nil { return fmt.Errorf("%w: %s to is required", ErrMissingField, env.Kind) } expectedTarget, ok := current.counterparty(env.From)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/lifecycle.go` around lines 231 - 249, The validateWorkDirection function currently only enforces env.To for KindSay and KindCapability; update it to also require and validate env.To for lifecycle messages (receipt and trace). In validateWorkDirection, include KindReceipt and KindTrace (or the appropriate constants used for receipts/traces) in the condition that enforces env.To, and keep the same error handling: return ErrMissingField if env.To is nil and return ErrWorkActorNotAllowed when *env.To != current.counterparty(env.From). Use the existing current.counterparty(...) call and the same fmt.Errorf shapes so receipts/traces are checked against the work counterparty just like say/capability messages.
🟡 Minor comments (14)
internal/hooks/dispatch_events.go-136-144 (1)
136-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn zero correlation when the network actor id is unknown.
If both
PeerFromandSessionIDare blank, this branch still emitsActorKind: "network_peer"with an emptyActorID. That creates a partial canonical correlation record and differs from the fallback path below, which returnsDispatchCorrelation{}when no identifier exists. GuardactorID == ""before building the struct.As per coding guidelines, "Every domain operation must emit a canonical event with correlation keys (
workspace_id,session_id,parent_session_id,root_session_id,agent_name,task_id,run_id,claim_token_hash,lease_until,workflow_id,coordinator_session_id,scheduler_reason,hook_event,hook_name,spawn_depth,actor_kind,actor_id,release_reason)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/dispatch_events.go` around lines 136 - 144, The NetworkPayload branch currently emits a DispatchCorrelation with ActorKind "network_peer" even when both PeerFrom and SessionID are blank; modify the case handling for NetworkPayload so that after computing actorID from strings.TrimSpace(typed.PeerFrom) and fallback to strings.TrimSpace(typed.SessionID) you check if actorID == "" and if so return an empty DispatchCorrelation{} instead of building a partial record; otherwise return DispatchCorrelation{ActorKind: "network_peer", ActorID: actorID}. Ensure you reference the NetworkPayload type, PeerFrom, SessionID, and DispatchCorrelation symbols when making the change.internal/api/spec/spec.go-1275-1283 (1)
1275-1283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
aftercursor description for thread listing.This endpoint paginates threads, but the parameter text says “message id”. That will mislead clients reading the generated OpenAPI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/spec/spec.go` around lines 1275 - 1283, The OpenAPI parameter for the "after" query in the listNetworkThreads operation is incorrectly described as a "message id"; update the parameter description used in the Path "/api/network/channels/{channel}/threads" / OperationID "listNetworkThreads" so the queryParam("after", ...) text refers to a thread cursor/id (e.g., "Return threads after the specified thread id" or "Return results after the specified thread cursor") to accurately reflect pagination for threads.internal/api/spec/spec.go-1275-1434 (1)
1275-1434:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing OpenAPI specification for
GET /api/network/peers/{peer_id}/messagesendpoint.The
NetworkPeerMessageshandler is fully implemented, tested, and registered as a public route, but missing from the OpenAPI spec. Similar message endpoints for threads and directs are documented (lines 1313 and 1396 respectively), so this should follow the same pattern ininternal/api/spec/spec.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/spec/spec.go` around lines 1275 - 1434, The OpenAPI spec is missing the entry for the GET /api/network/peers/{peer_id}/messages endpoint; add a new ParameterSpec/ResponseSpec block matching the pattern used by listNetworkThreadMessages and listNetworkDirectRoomMessages: create a route object with Method "GET", Path "/api/network/peers/{peer_id}/messages", OperationID "listNetworkPeerMessages" (or match the existing handler name NetworkPeerMessages), Summary "List messages for a network peer", Tags []string{"network"}, Transports {TransportHTTP, TransportUDS}, Parameters including pathParam("peer_id","Peer id"), query params before/after/kind/work_id and intQueryParam("limit"), and Responses mirroring the others (200 -> contract.NetworkPeerMessagesResponse{}, 400/404/503/500 -> contract.ErrorPayload{}).internal/bridges/types.go-621-625 (1)
621-625:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
work_idwith the same canonical rules as the other AGH conversation IDs.
thread_idanddirect_idare format-checked here, butwork_idonly gets generic length/control-character validation. That means malformed values like"foo"passInboundMessageEnvelope.Validate()and only fail later in downstream network code.Suggested fix
switch field { case "thread_id": if !strings.HasPrefix(trimmed, "thread_") { return fmt.Errorf("bridges: invalid network conversation thread_id %q", value) } case "direct_id": if !strings.HasPrefix(trimmed, "direct_") || len(trimmed) != len("direct_")+32 { return fmt.Errorf("bridges: invalid network conversation direct_id %q", value) } + case "work_id": + if !strings.HasPrefix(trimmed, "work_") { + return fmt.Errorf("bridges: invalid network conversation work_id %q", value) + } }Also applies to: 999-1017
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bridges/types.go` around lines 621 - 625, Replace the generic length/control-character check for normalized.WorkID in InboundMessageEnvelope.Validate() with the same canonical validation used for thread_id and direct_id by calling validateBridgeNetworkConversationID(normalized.WorkID, "work_id"); i.e., locate the block that currently only checks normalized.WorkID and swap it so it invokes validateBridgeNetworkConversationID and returns its error (same pattern as used for thread_id/direct_id) to ensure malformed work_id values are rejected early.internal/network/perf_bench_test.go-12-23 (1)
12-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis benchmark no longer constructs a direct message.
BenchmarkFormatNetworkMessageDirectnow setsKindSay, but it does not mark the envelope as a direct surface or provide aDirectID. That makes it easy for the benchmark to drift into measuring the generic say formatter instead of the direct-message path its name implies. Either add the direct routing metadata here or rename the benchmark.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/perf_bench_test.go` around lines 12 - 23, The benchmark BenchmarkFormatNetworkMessageDirect is constructing a KindSay envelope but not marking it as a direct message, so change the test message construction to include the direct routing metadata (set the envelope's DirectID to a non-empty value and/or set the Surface to the direct surface constant used in your codebase) so the formatter exercises the direct-message code path; alternatively, if you intended to measure the generic say formatter, rename BenchmarkFormatNetworkMessageDirect to reflect that intent. Ensure you modify the message creation in the benchmark (the same block that sets Protocol, ID, Kind, Channel, From, To, WorkID, ReplyTo, TraceID, CausationID, TS, Body) to add DirectID (and Surface/direct flag) when keeping the current benchmark name.internal/api/udsapi/handlers_test.go-549-562 (1)
549-562:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis doesn't actually verify the HTTP router surface.
gotis collected once from the UDS engine, then compared to the documented HTTP and UDS route sets. If HTTP registration drops a network route while the spec remains shared, this still passes because no HTTP router is ever instantiated here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/udsapi/handlers_test.go` around lines 549 - 562, The test TestRegisterNetworkRoutesMatchDocumentedHTTPAndUDSSurface only instantiates a UDS router and reuses its routes for both TransportHTTP and TransportUDS, so it never verifies the HTTP router surface; fix by instantiating an HTTP router (e.g., call newTestRouter with the same newTestHandlers but configured for HTTP), collect its routes with registeredNetworkRoutesFromEngine on that HTTP engine, then compare the HTTP-collected routes to documentedNetworkRoutesForTransport(apispec.TransportHTTP) and the UDS-collected routes to documentedNetworkRoutesForTransport(apispec.TransportUDS) using the existing comparison logic.internal/hooks/network_dispatch_test.go-23-38 (1)
23-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse event-consistent fixtures for the direct-room case.
Both the matcher and
networkDispatchTestPayloadforceSurface: "thread"with a thread ID for every event. That meansHookNetworkDirectRoomOpenednever exercises the direct-surface / direct-ID path, so a regression there would still pass this test.Also applies to: 84-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/network_dispatch_test.go` around lines 23 - 38, The test currently forces NetworkMatcher{Surface: "thread"} and networkDispatchTestPayload fixtures for all events, so HookNetworkDirectRoomOpened never hits the direct-surface path; update the test to create event-consistent fixtures: when iterating events (the events slice and HookDecl creation), detect HookNetworkDirectRoomOpened and set the matcher and the corresponding networkDispatchTestPayload for that case to use Surface: "direct" and a direct-room ID (instead of "thread" and thread ID), while leaving other events unchanged; ensure references to HookDecl, HookMatcher/NetworkMatcher, networkDispatchTestPayload, and HookNetworkDirectRoomOpened are used so the direct-room case exercises the direct-surface/direct-ID code path.internal/extension/manifest_test.go-150-153 (1)
150-153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the full matcher mapping here.
This only checks
ChannelandWorkStateafterhookConfigMatcher(...). If the conversion dropsSurface,Kind, orDirection, the new test still passes even though those are part of the network matcher path added in this PR.💡 Tighten the assertion
hookMatcher := hookConfigMatcher(matcher) - if hookMatcher.NetworkMatcher == nil || hookMatcher.Channel != "builders" || hookMatcher.WorkState != "completed" { + if hookMatcher.NetworkMatcher == nil || + hookMatcher.Channel != "builders" || + hookMatcher.Surface != "thread" || + hookMatcher.Kind != "trace" || + hookMatcher.Direction != "received" || + hookMatcher.WorkState != "completed" { t.Fatalf("hookConfigMatcher() = %#v, want network matcher fields", hookMatcher) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/manifest_test.go` around lines 150 - 153, The test only validates Channel and WorkState from hookConfigMatcher(matcher) and may miss regressions in NetworkMatcher fields; update the test to assert the complete matcher mapping by comparing hookMatcher (the full return value of hookConfigMatcher) against an expected struct/value that includes NetworkMatcher.Surface, NetworkMatcher.Kind, NetworkMatcher.Direction, Channel, and WorkState (or use reflect.DeepEqual/cmp to compare the whole hookMatcher to expected). Ensure the assertion fails if any of Surface/Kind/Direction are dropped or mistranslated.internal/hooks/dispatch_events_test.go-64-93 (1)
64-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact turn ID here.
These subtests only prove that
TurnIDFromPayload()returns some trimmed non-empty string. If the implementation starts pulling the wrong field, this still passes. Comparing against the exact trimmed turn ID would make the test regression-sensitive.As per coding guidelines, "MUST test meaningful business logic, not trivial operations" and "Check tests can fail when business logic changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/dispatch_events_test.go` around lines 64 - 93, The test currently only checks that TurnIDFromPayload returns a non-empty trimmed string; change the table-driven tests to include the exact expected trimmed turn ID for each case and assert equality (e.g., add an expected field to each case and replace the loose checks with if got != tc.expected { t.Fatalf(...)}), ensuring you still trim inputs with surrounding spaces and call TurnIDFromPayload(payload) for each payload; reference the test cases slice and the TurnIDFromPayload function to locate where to add the expected values and perform the strict equality assertion.internal/acp/client_test.go-424-437 (1)
424-437:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover the new routing fields in this metadata round-trip.
This fixture now sets
SurfaceandDirectID, but the test never asserts them. A regression that drops the new conversation-routing metadata would still pass here.As per coding guidelines, "Focus on critical paths: workflow execution, state management, error handling" and "Check tests can fail when business logic changes."
Also applies to: 462-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/acp/client_test.go` around lines 424 - 437, The test fixture populates PromptNetworkMeta.Surface and .DirectID but the assertions don't verify them; update the round-trip assertions in internal/acp/client_test.go to check that the decoded/returned PromptNetworkMeta has the same Surface and DirectID as the fixture (add assertions for PromptNetworkMeta.Surface and PromptNetworkMeta.DirectID where other meta fields are asserted), and do this for both places the fixture is used (the block around the existing metadata assertions and the second occurrence around lines 462-467) so a regression dropping routing metadata will fail.internal/network/audit_test.go-213-215 (1)
213-215:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParallelize this new top-level test.
Everything here is per-test state, so leaving the top-level case serialized just slows the package and breaks the default parallel-test convention already used elsewhere in this file.
As per coding guidelines,
**/*_test.go: "Default tot.Parallelin Go tests unless there is a specific reason to disable it (opt-out witht.Setenv)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/audit_test.go` around lines 213 - 215, The top-level test TestAuditWriterRecordsRepeatedGreetHeartbeatsOnlyAsAuditRows is not marked parallel; add t.Parallel() as the first statement in the TestAuditWriterRecordsRepeatedGreetHeartbeatsOnlyAsAuditRows function to allow the whole test to run in parallel (leaving the existing t.Run subcases as-is), ensuring per-test state remains isolated while following the file's parallel-test convention.internal/network/manager_test.go-312-313 (1)
312-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLock the conversation-order counters on read.
publishCountAtWriteandpromptCountAtWriteare written unders.mu, but these assertions read them directly. IfWriteConversationMessageever runs on a worker goroutine, this helper will start failing under-raceeven when the ordering is correct. Add tiny getter methods that read those fields under the same mutex and use those from the assertions.Example fix
type recordingConversationStore struct { store.NetworkConversationStore mu sync.Mutex entries []store.NetworkConversationMessage publishCount func() int promptCount func() int publishCountAtWrite int promptCountAtWrite int result store.NetworkConversationWriteResult err error } + +func (s *recordingConversationStore) publishCountWritten() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.publishCountAtWrite +} + +func (s *recordingConversationStore) promptCountWritten() int { + s.mu.Lock() + defer s.mu.Unlock() + return s.promptCountAtWrite +}As per coding guidelines, "Run tests with
-raceflag and ensureCGO_ENABLED=1for race-sensitive packages."Also applies to: 389-390, 1699-1743
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/manager_test.go` around lines 312 - 313, The test reads s.publishCountAtWrite and s.promptCountAtWrite without locking though they are written under s.mu; add small getter methods (e.g., func (s *<type>) publishCountAtWriteLocked() int and func (s *<type>) promptCountAtWriteLocked() int) that acquire s.mu, return the field, and update the test helper to call these getters in place of direct field access; ensure assertions in manager_test.go (around the lines referencing publishCountAtWrite/promptCountAtWrite and other occurrences noted) use those getters so reads are synchronized with WriteConversationMessage which holds s.mu.internal/api/core/network.go-302-349 (1)
302-349:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant surface check results in unreachable code.
At lines 324-327, the condition
surface == ""is checked with an innerifforthreadID != "" || directID != "", but both branches return the same error. The inner check at line 324-325 makes the standalone return at line 327 unreachable whenthreadIDordirectIDis set. The logic should be simplified.🐛 Proposed fix to simplify the surface check
if surface == "" { - if threadID != "" || directID != "" { - return NewNetworkValidationError(fmt.Errorf("%w: surface is required", network.ErrMissingField)) - } return NewNetworkValidationError(fmt.Errorf("%w: surface is required", network.ErrMissingField)) }If the intent was to provide different error messages, consider:
if surface == "" { if threadID != "" || directID != "" { - return NewNetworkValidationError(fmt.Errorf("%w: surface is required", network.ErrMissingField)) + return NewNetworkValidationError(fmt.Errorf("%w: surface is required when thread_id or direct_id is specified", network.ErrMissingField)) } return NewNetworkValidationError(fmt.Errorf("%w: surface is required", network.ErrMissingField)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/core/network.go` around lines 302 - 349, In validateNetworkSendConversation, the redundant nested surface == "" check with an inner if (threadID != "" || directID != "") returns the same NewNetworkValidationError in both branches, making the inner branch unnecessary and unreachable; simplify by replacing that nested if/return with a single check that if surface == "" returns NewNetworkValidationError(fmt.Errorf("%w: surface is required", network.ErrMissingField)); this removes the unreachable branch and keeps behavior for validateNetworkSendConversation, ref.Validate, and the subsequent work_id checks unchanged.internal/network/router_test.go-1452-1461 (1)
1452-1461:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake these validation checks fail for the reason the test names claim.
Both assertions accept any non-nil error, and
OpenWork(Envelope{Kind: KindSay}, time.Time{})is invalid in several ways before it ever proves the "non-opener" path. Use a fully valid non-opener envelope and assert the specific error so this test breaks on the intended regression.As per coding guidelines, tests MUST have specific error assertions (ErrorContains, ErrorAs).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/router_test.go` around lines 1452 - 1461, The test TestWorkValidationErrors currently accepts any non-nil error; change it to assert specific validation failures: keep the first assertion calling (Work{}).Validate() but check the exact error (e.g., using t.ErrorContains or t.ErrorAs) that Validate returns for an empty Work; for the OpenWork assertion, construct a syntactically/semantically valid Envelope that is not an opener (e.g., populate required fields like ID, Sender, Timestamp but set Kind to KindSay) and call OpenWork with that Envelope and a valid time, then assert the specific "non-opener" error returned by OpenWork using t.ErrorContains or t.ErrorAs so the test fails only when the non-opener path regresses; target symbols: TestWorkValidationErrors, Work.Validate, OpenWork, Envelope, KindSay.
🧹 Nitpick comments (6)
internal/network/helpers_test.go (1)
12-12: ⚡ Quick winRemove duplicated test scenarios for
KindSay.Line [12] repeats
KindSay, and Lines [77-79] duplicate the sameSayBody.Kind()assertion. This adds noise and can mask missing coverage when enum values change.Proposed cleanup
- validKinds := []Kind{KindGreet, KindWhois, KindSay, KindSay, KindCapability, KindReceipt, KindTrace} + validKinds := []Kind{KindGreet, KindWhois, KindSay, KindCapability, KindReceipt, KindTrace} ... - if got := (SayBody{}).Kind(); got != KindSay { - t.Fatalf("SayBody.Kind() = %q, want %q", got, KindSay) - }Also applies to: 77-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/helpers_test.go` at line 12, Remove the duplicated KindSay entries and redundant assertion: in the test's validKinds slice remove the second KindSay so each Kind value appears once, and delete the duplicate assertion block that checks SayBody.Kind() (leave a single assertion for SayBody.Kind()). Update any expected counts or table-driven test expectations that relied on the duplicated entry so the test still validates all unique Kind values exactly once.internal/api/udsapi/agent_channels_test.go (1)
365-368: ⚡ Quick winAssert the resolved direct room ID, not just that a pointer exists.
The new routing contract is surface-based, so this test should fail if reply resolution produces the wrong direct room or an empty string. Seed
source.DirectIDand check*seen.DirectIDexactly;seen.DirectID != nilalone won't catch that regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/udsapi/agent_channels_test.go` around lines 365 - 368, The test currently only asserts seen.DirectID != nil which misses wrong/empty direct-room resolution; seed the expected direct room by setting source.DirectID to the exact string you expect and replace the nil check with an equality assertion that *seen.DirectID == expectedDirectID (i.e., compare *seen.DirectID to the seeded source.DirectID value) in the same test that inspects seen (the block checking seen.Kind, seen.Surface, seen.DirectID), so the test fails if reply resolution returns the wrong or empty direct room ID.internal/hooks/hooks_test.go (1)
1571-1571: ⚡ Quick winAdd a non-matching compaction case here.
This only exercises
CompactionMatcherwith the matching"token_limit"reason, so the test still passes if the new nested matcher is ignored completely. Add a second dispatch with a differentReasonand assert the payload stays unchanged.Also applies to: 1652-1655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/hooks_test.go` at line 1571, Add a second test dispatch that uses HookMatcher with CompactionMatcher where Reason is something other than "token_limit" (e.g., "other_reason") to exercise the non-matching path; call the same dispatch/handle function used for the matching case and then assert that the returned/handled payload is unchanged from the input. Specifically, add a new HookMatcher{CompactionMatcher: &CompactionMatcher{Reason: "other_reason"}} dispatch alongside the existing matching dispatch and add an assertion that the payload equals the original payload (no modifications) to ensure the nested matcher is evaluated.internal/daemon/network_e2e_assertions_test.go (1)
168-314: ⚡ Quick winWrap these cases in
t.Run("Should ...")subtests.The added/updated cases are still flat top-level tests, so this file is drifting from the repo’s required Go test structure.
As per coding guidelines,
**/*_test.go:Use t.Run('Should ...') pattern for Go test subtests instead of flat test structures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/network_e2e_assertions_test.go` around lines 168 - 314, The top-level tests TestValidateNetworkCorrelationSurfacesUsesTargetedAttributes, TestValidateNetworkCorrelationSurfacesRejectsSplitTranscriptMatches, TestValidateNetworkAuditEntryMatchesDuplicateRejection, and TestValidateNetworkAuditEntryRejectsWrongContainer should be converted to use t.Run subtests (e.g. t.Run("Should validate ...", func(t *testing.T) { ... })) so they follow the repo test pattern; wrap each existing test body in a t.Run with a descriptive "Should ..." name, keep calling t.Parallel() inside each subtest (or at the parent if you want all subtests parallel), and leave the calls to validateNetworkCorrelationSurfaces and validateNetworkAuditEntry and their expectations unchanged.internal/api/httpapi/network_test.go (1)
18-93: ⚡ Quick winMark the subtest parallel as well.
The parent test is parallelized, but this new
t.Run("Should ...")block still runs serially. Addingt.Parallel()here keeps the new coverage aligned with the rest of the suite's default test behavior.As per coding guidelines, "Use
t.Run("Should ...")subtests witht.Parallelas default (opt-out witht.Setenv)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/httpapi/network_test.go` around lines 18 - 93, The subtest "Should create deterministic direct room" inside network_test.go is not marked parallel; add t.Parallel() as the first statement inside that t.Run closure (immediately after func(t *testing.T) {) so the subtest runs concurrently like the parent suite; update the subtest containing newTestHomePaths, newTestHandlers, handlers.Network = ..., handlers.NetworkStore = ..., engine := newTestRouter(...) etc. to include t.Parallel() at the top of the closure.internal/api/core/agent_channels.go (1)
860-877: 💤 Low valueHardcoded query key limits reusability.
The
parsePositiveIntQueryfunction now hardcodes the key as"limit", which reduces its flexibility compared to the previous signature that accepted a key parameter. If only thelimitquery parameter needs this parsing, consider renaming toparseLimitQueryfor clarity.♻️ Optional: Rename for clarity
-func parsePositiveIntQuery(c *gin.Context) (int, error) { - const key = "limit" +func parseLimitQuery(c *gin.Context) (int, error) { + const key = "limit"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/core/agent_channels.go` around lines 860 - 877, The function parsePositiveIntQuery currently hardcodes the query key "limit", reducing reuse; change its signature back to accept a key parameter (e.g., func parsePositiveIntQuery(c *gin.Context, key string) (int, error)), replace the constant key usage with the parameter, and update all call sites to pass the desired query name (or if you intentionally only want "limit", instead rename the function to parseLimitQuery and adjust callers accordingly); ensure error messages still reference the provided key and tests/build are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbd455d9-1770-4b71-904c-9ec558be8cc6
⛔ Files ignored due to path filters (64)
internal/skills/bundled/skills/agh-network/SKILL.mdis excluded by!**/*.mdinternal/testutil/acpmock/testdata/browser_session_lifecycle_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/driver_fault_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/network_collaboration_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/permission_env_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/tool_permission_fixture.jsonis excluded by!**/*.jsonopenapi/agh.jsonis excluded by!**/*.jsonpackages/site/content/blog/posts/introducing-agh-the-first-agent-network-protocol.mdxis excluded by!**/*.mdxpackages/site/content/protocol/capability-discovery.mdxis excluded by!**/*.mdxpackages/site/content/protocol/conformance.mdxis excluded by!**/*.mdxpackages/site/content/protocol/delivery.mdxis excluded by!**/*.mdxpackages/site/content/protocol/ed25519-jcs.mdxis excluded by!**/*.mdxpackages/site/content/protocol/envelope.mdxis excluded by!**/*.mdxpackages/site/content/protocol/examples.mdxis excluded by!**/*.mdxpackages/site/content/protocol/guide/minimal-sender.mdxis excluded by!**/*.mdxpackages/site/content/protocol/guide/nats-transport.mdxis excluded by!**/*.mdxpackages/site/content/protocol/guide/testing.mdxis excluded by!**/*.mdxpackages/site/content/protocol/guide/trust-verification.mdxis excluded by!**/*.mdxpackages/site/content/protocol/interactions.mdxis excluded by!**/*.mdxpackages/site/content/protocol/message-kinds.mdxis excluded by!**/*.mdxpackages/site/content/protocol/nats.mdxis excluded by!**/*.mdxpackages/site/content/protocol/overview.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/directs/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/directs/list.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/directs/messages.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/directs/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/network/directs/resolve.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/directs/show.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/network/send.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/threads/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/threads/list.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/threads/messages.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/threads/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/network/threads/show.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/work/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/work/lookup.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/network/work/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/cli-reference/network/work/status.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/observe/events.mdxis excluded by!**/*.mdxpackages/site/content/runtime/cli-reference/task/run/enqueue.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/agents/capabilities.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/channels-and-peers.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/delivery-and-safety.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/directs.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/meta.jsonis excluded by!**/*.jsonpackages/site/content/runtime/core/network/protocol.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/task-ingress.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/threads.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/network/work.mdxis excluded by!**/*.mdxpackages/site/content/runtime/guides/coordinate-agents-over-network.mdxis excluded by!**/*.mdxpackages/site/content/runtime/use-cases/handoff-between-agents.mdxis excluded by!**/*.mdxsdk/typescript/src/generated/contracts.tsis excluded by!**/generated/**web/e2e/fixtures/artifacts.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/artifacts.tsis excluded by!**/fixtures/**web/e2e/fixtures/browser-artifact-session.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/browser-artifact-session.tsis excluded by!**/fixtures/**web/e2e/fixtures/runtime-seed.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/runtime-seed.tsis excluded by!**/fixtures/**web/e2e/fixtures/selectors.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/selectors.tsis excluded by!**/fixtures/**web/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (236)
internal/acp/client.gointernal/acp/client_test.gointernal/acp/types.gointernal/api/contract/contract.gointernal/api/contract/contract_test.gointernal/api/contract/responses.gointernal/api/core/agent_channels.gointernal/api/core/agent_channels_internal_test.gointernal/api/core/authored_context.gointernal/api/core/coverage_helpers_test.gointernal/api/core/errors.gointernal/api/core/interfaces.gointernal/api/core/network.gointernal/api/core/network_conversations.gointernal/api/core/network_details.gointernal/api/core/network_test.gointernal/api/core/test_helpers_test.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/network_test.gointernal/api/httpapi/routes.gointernal/api/spec/spec.gointernal/api/testutil/apitest.gointernal/api/udsapi/agent_channels_test.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/network_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/transport_parity_integration_test.gointernal/bridges/types.gointernal/bridges/types_test.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/command_paths_test.gointernal/cli/helpers_test.gointernal/cli/hooks.gointernal/cli/hooks_test.gointernal/cli/network.gointernal/cli/network_client_test.gointernal/cli/network_test.gointernal/config/agent.gointernal/config/agent_edit.gointernal/config/hooks.gointernal/config/hooks_test.gointernal/config/merge.gointernal/config/tool_surface.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/daemon_network_collaboration_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/hooks_bridge.gointernal/daemon/native_tools.gointernal/daemon/native_tools_test.gointernal/daemon/network_e2e_assertions_test.gointernal/daemon/prompt_input_composite_integration_test.gointernal/extension/capability.gointernal/extension/capability_test.gointernal/extension/contract/host_api.gointernal/extension/contract/sdk.gointernal/extension/contract/sdk_test.gointernal/extension/host_api.gointernal/extension/host_api_bridges.gointernal/extension/host_api_network.gointernal/extension/host_api_network_test.gointernal/extension/manager.gointernal/extension/manifest.gointernal/extension/manifest_test.gointernal/extension/protocol/host_api.gointernal/extension/protocol/host_api_test.gointernal/hooks/async_clone_test.gointernal/hooks/dispatch.gointernal/hooks/dispatch_events.gointernal/hooks/dispatch_events_test.gointernal/hooks/events.gointernal/hooks/events_test.gointernal/hooks/hooks_test.gointernal/hooks/introspection.gointernal/hooks/introspection_test.gointernal/hooks/matcher.gointernal/hooks/matcher_test.gointernal/hooks/network_dispatch_test.gointernal/hooks/payloads.gointernal/hooks/payloads_test.gointernal/hooks/types.gointernal/network/audit.gointernal/network/audit_test.gointernal/network/delivery.gointernal/network/delivery_test.gointernal/network/envelope.gointernal/network/envelope_integration_test.gointernal/network/helpers_test.gointernal/network/hooks.gointernal/network/hooks_test.gointernal/network/lifecycle.gointernal/network/lifecycle_test.gointernal/network/manager.gointernal/network/manager_integration_test.gointernal/network/manager_test.gointernal/network/perf_bench_test.gointernal/network/router.gointernal/network/router_integration_test.gointernal/network/router_test.gointernal/network/stats.gointernal/network/tasks.gointernal/network/tasks_integration_test.gointernal/network/tasks_test.gointernal/network/validate.gointernal/network/validate_test.gointernal/session/query.gointernal/session/query_test.gointernal/settings/collections.gointernal/settings/service_test.gointernal/situation/service.gointernal/situation/service_test.gointernal/skills/bundled/bundled_test.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_heartbeat_test.gointernal/store/globaldb/global_db_network_audit.gointernal/store/globaldb/global_db_network_audit_test.gointernal/store/globaldb/global_db_network_conversation_repository_test.gointernal/store/globaldb/global_db_network_conversations.gointernal/store/globaldb/global_db_network_conversations_test.gointernal/store/globaldb/global_db_network_messages.gointernal/store/globaldb/global_db_network_messages_test.gointernal/store/globaldb/global_db_soul_test.gointernal/store/globaldb/global_db_test.gointernal/store/globaldb/tx_helpers.gointernal/store/network_conversation_types_test.gointernal/store/sessiondb/session_db.gointernal/store/sessiondb/session_db_extra_test.gointernal/store/store.gointernal/store/store_helpers_test.gointernal/store/types.gointernal/testutil/acpmock/fixture.gointernal/testutil/acpmock/fixture_test.gointernal/testutil/e2e/artifacts.gointernal/testutil/e2e/runtime_harness.gointernal/testutil/e2e/runtime_harness_helpers_test.gointernal/tools/builtin/builtin_test.gointernal/tools/builtin/network.gointernal/tools/builtin/toolsets.gointernal/tools/builtin_ids.gointernal/tools/native_test.gointernal/tools/schema.gopackages/site/components/blog/kind-chip.tsxpackages/site/components/landing/__tests__/landing.test.tsxpackages/site/components/landing/network-protocol-visual.tsxpackages/site/components/landing/network-section.tsxpackages/site/components/landing/primitives/network-kinds.tspackages/site/lib/landing-cli-snippets.test.tspackages/site/lib/protocol-rfc-hard-cut.test.tspackages/site/lib/runtime-manual-cli-examples.test.tssdk/go/extension_test.gosdk/go/host_api.gosdk/typescript/src/host-api.test.tssdk/typescript/src/host-api.tssdk/typescript/src/index.tsweb/e2e/bridges.spec.tsweb/e2e/network.spec.tsweb/e2e/storybook-bootstrap.spec.tsweb/src/hooks/routes/use-network-page.tsweb/src/routeTree.gen.tsweb/src/routes/_app/-network-routes.test.tsweb/src/routes/_app/-network-task14-routes.test.tsweb/src/routes/_app/-network.test.tsxweb/src/routes/_app/network.$channel.activity.tsxweb/src/routes/_app/network.$channel.directs.$directId.tsxweb/src/routes/_app/network.$channel.directs.tsxweb/src/routes/_app/network.$channel.threads.$threadId.tsxweb/src/routes/_app/network.$channel.threads.tsxweb/src/routes/_app/network.tsxweb/src/routes/_app/settings/-network.test.tsxweb/src/routes/_app/stories/-network.stories.tsxweb/src/storybook/web-storybook-stories-and-fixtures.test.tsxweb/src/systems/network/adapters/network-api.tsweb/src/systems/network/components/activity/activity-feed.test.tsxweb/src/systems/network/components/activity/activity-feed.tsxweb/src/systems/network/components/activity/index.tsweb/src/systems/network/components/composer/channel-thread-composer.test.tsxweb/src/systems/network/components/composer/channel-thread-composer.tsxweb/src/systems/network/components/composer/composer-slash-popover.tsxweb/src/systems/network/components/composer/composer-toolbar.tsxweb/src/systems/network/components/composer/composer.test.tsxweb/src/systems/network/components/composer/composer.tsxweb/src/systems/network/components/composer/detail-composer.tsxweb/src/systems/network/components/composer/index.tsweb/src/systems/network/components/composer/use-composer-state.tsweb/src/systems/network/components/directs/direct-room.test.tsxweb/src/systems/network/components/directs/direct-room.tsxweb/src/systems/network/components/directs/directs-list.test.tsxweb/src/systems/network/components/directs/directs-list.tsxweb/src/systems/network/components/directs/index.tsweb/src/systems/network/components/directs/new-direct-dialog.tsxweb/src/systems/network/components/directs/use-direct-room-view.tsweb/src/systems/network/components/empty-states/conversation-error.tsxweb/src/systems/network/components/empty-states/daemon-down.tsxweb/src/systems/network/components/empty-states/direct-empty.tsxweb/src/systems/network/components/empty-states/directs-empty.tsxweb/src/systems/network/components/empty-states/empty-states.test.tsxweb/src/systems/network/components/empty-states/index.tsweb/src/systems/network/components/empty-states/network-empty.tsxweb/src/systems/network/components/empty-states/thread-empty.tsxweb/src/systems/network/components/empty-states/threads-empty.tsxweb/src/systems/network/components/network-workspace-shell.test.tsxweb/src/systems/network/components/network-workspace-shell.tsxweb/src/systems/network/components/shell/channel-header.tsxweb/src/systems/network/components/shell/channel-rail-recents.tsxweb/src/systems/network/components/shell/channel-rail-row.tsxweb/src/systems/network/components/shell/channel-rail.test.tsxweb/src/systems/network/components/shell/channel-rail.tsxweb/src/systems/network/components/shell/channel-tabs.test.tsxweb/src/systems/network/components/shell/channel-tabs.tsxweb/src/systems/network/components/shell/index.tsweb/src/systems/network/components/shell/inspector-activity-feed.tsxweb/src/systems/network/components/shell/inspector-members-list.tsxweb/src/systems/network/components/shell/list-filter-bar.test.tsxweb/src/systems/network/components/shell/list-filter-bar.tsxweb/src/systems/network/components/shell/network-inspector.tsxweb/src/systems/network/components/shell/network-shell.tsxweb/src/systems/network/components/shell/right-rail.tsxweb/src/systems/network/components/stories/channel-header.stories.tsxweb/src/systems/network/components/stories/channel-rail.stories.tsxweb/src/systems/network/components/stories/composer.stories.tsxweb/src/systems/network/components/stories/empty-states.stories.tsxweb/src/systems/network/components/stories/message-row.stories.tsxweb/src/systems/network/components/stories/network-shell.stories.tsxweb/src/systems/network/components/stories/network-workspace-shell.stories.tsxweb/src/systems/network/components/stories/thread-overlay.stories.tsxweb/src/systems/network/components/stories/timeline.stories.tsxweb/src/systems/network/components/stories/work.stories.tsxweb/src/systems/network/components/thread-overlay/index.tsweb/src/systems/network/components/thread-overlay/thread-overlay-header.tsxweb/src/systems/network/components/thread-overlay/thread-overlay-replies.tsxweb/src/systems/network/components/thread-overlay/thread-overlay-root.tsxweb/src/systems/network/components/thread-overlay/thread-overlay.test.tsxweb/src/systems/network/components/thread-overlay/thread-overlay.tsxweb/src/systems/network/components/thread-overlay/use-thread-overlay-view.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/network/audit_test.go (1)
260-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
t.Parallel()in the subtest.This subtest is missing
t.Parallel()while all sibling subtests in the file use it, inconsistent with the project's default testing pattern.✅ Suggested patch
t.Run("Should return audit write failures", func(t *testing.T) { + t.Parallel() + storeErr := errors.New("audit store unavailable")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/audit_test.go` around lines 260 - 275, The subtest "Should return audit write failures" is missing t.Parallel(), causing inconsistency with sibling subtests; inside the t.Run block for that test (around the setup using failingAuditStore, NewAuditWriter and the RecordSent call) add a call to t.Parallel() immediately after the subtest starts so the subtest runs in parallel with its siblings.internal/config/hooks.go (1)
200-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep nested matchers nil unless that matcher family is actually configured.
toHookMatcher()now always returns non-nilNetworkMatcherandCompactionMatcher, even for hooks that never set any of those fields. That changes the meaning of pointer presence for downstream code and makes config-defined hooks look like they opted into these matcher families when they did not. Gate each allocation on at least one non-empty field.Suggested fix
func (m parsedHookMatcher) toHookMatcher(scopeAgentName string) (hookspkg.HookMatcher, error) { matcher := hookspkg.HookMatcher{ AgentName: strings.TrimSpace(m.AgentName), AgentType: strings.TrimSpace(m.AgentType), WorkspaceID: strings.TrimSpace(m.WorkspaceID), WorkspaceRoot: strings.TrimSpace(m.WorkspaceRoot), SessionType: strings.TrimSpace(m.SessionType), InputClass: strings.TrimSpace(m.InputClass), ACPEventType: strings.TrimSpace(m.ACPEventType), TurnID: strings.TrimSpace(m.TurnID), ToolID: strings.TrimSpace(m.ToolID), ToolName: strings.TrimSpace(m.ToolName), DecisionClass: strings.TrimSpace(m.DecisionClass), MessageRole: strings.TrimSpace(m.MessageRole), MessageDeltaType: strings.TrimSpace(m.MessageDeltaType), } - matcher.NetworkMatcher = &hookspkg.NetworkMatcher{ - Channel: strings.TrimSpace(m.Channel), - Surface: strings.TrimSpace(m.Surface), - Kind: strings.TrimSpace(m.Kind), - Direction: strings.TrimSpace(m.Direction), - WorkState: strings.TrimSpace(m.WorkState), - } - matcher.CompactionMatcher = &hookspkg.CompactionMatcher{ - Reason: strings.TrimSpace(m.CompactionReason), - Strategy: strings.TrimSpace(m.CompactionStrategy), - } + channel := strings.TrimSpace(m.Channel) + surface := strings.TrimSpace(m.Surface) + kind := strings.TrimSpace(m.Kind) + direction := strings.TrimSpace(m.Direction) + workState := strings.TrimSpace(m.WorkState) + if channel != "" || surface != "" || kind != "" || direction != "" || workState != "" { + matcher.NetworkMatcher = &hookspkg.NetworkMatcher{ + Channel: channel, + Surface: surface, + Kind: kind, + Direction: direction, + WorkState: workState, + } + } + reason := strings.TrimSpace(m.CompactionReason) + strategy := strings.TrimSpace(m.CompactionStrategy) + if reason != "" || strategy != "" { + matcher.CompactionMatcher = &hookspkg.CompactionMatcher{ + Reason: reason, + Strategy: strategy, + } + } if m.ToolReadOnly != nil { value := *m.ToolReadOnly matcher.ToolReadOnly = &value }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/hooks.go` around lines 200 - 239, In toHookMatcher, avoid always allocating matcher.NetworkMatcher and matcher.CompactionMatcher; instead, only set matcher.NetworkMatcher to &hookspkg.NetworkMatcher{...} if at least one of Channel, Surface, Kind, Direction, or WorkState is non-empty (after TrimSpace), and only set matcher.CompactionMatcher to &hookspkg.CompactionMatcher{...} if either CompactionReason or CompactionStrategy is non-empty; leave those pointer fields nil when no related fields are configured so downstream code can rely on nil meaning "not configured". Use the same trimmed values you already build for the matcher fields to decide the allocations in parsedHookMatcher.toHookMatcher.internal/extension/manager.go (1)
1620-1651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve nil matcher pointers when no network or compaction fields are configured.
hookConfigMatcher()now makesNetworkMatcherandCompactionMatchernon-nil for every hook, even when all fields in those groups are empty. That removes the existing “nil means this matcher family is absent” signal for downstream consumers, including the new CLI/network rendering path added in this PR. Only allocate each nested matcher when at least one field in that family is set.Suggested fix
func hookConfigMatcher(cfg HookMatcherConfig) hookspkg.HookMatcher { matcher := hookspkg.HookMatcher{ AgentName: strings.TrimSpace(cfg.AgentName), AgentType: strings.TrimSpace(cfg.AgentType), WorkspaceID: strings.TrimSpace(cfg.WorkspaceID), WorkspaceRoot: strings.TrimSpace(cfg.WorkspaceRoot), SessionType: strings.TrimSpace(cfg.SessionType), InputClass: strings.TrimSpace(cfg.InputClass), ACPEventType: strings.TrimSpace(cfg.ACPEventType), TurnID: strings.TrimSpace(cfg.TurnID), ToolID: strings.TrimSpace(cfg.ToolID), ToolName: strings.TrimSpace(cfg.ToolName), DecisionClass: strings.TrimSpace(cfg.DecisionClass), MessageRole: strings.TrimSpace(cfg.MessageRole), MessageDeltaType: strings.TrimSpace(cfg.MessageDeltaType), } - matcher.NetworkMatcher = &hookspkg.NetworkMatcher{ - Channel: strings.TrimSpace(cfg.Channel), - Surface: strings.TrimSpace(cfg.Surface), - Kind: strings.TrimSpace(cfg.Kind), - Direction: strings.TrimSpace(cfg.Direction), - WorkState: strings.TrimSpace(cfg.WorkState), - } - matcher.CompactionMatcher = &hookspkg.CompactionMatcher{ - Reason: strings.TrimSpace(cfg.CompactionReason), - Strategy: strings.TrimSpace(cfg.CompactionStrategy), - } + channel := strings.TrimSpace(cfg.Channel) + surface := strings.TrimSpace(cfg.Surface) + kind := strings.TrimSpace(cfg.Kind) + direction := strings.TrimSpace(cfg.Direction) + workState := strings.TrimSpace(cfg.WorkState) + if channel != "" || surface != "" || kind != "" || direction != "" || workState != "" { + matcher.NetworkMatcher = &hookspkg.NetworkMatcher{ + Channel: channel, + Surface: surface, + Kind: kind, + Direction: direction, + WorkState: workState, + } + } + reason := strings.TrimSpace(cfg.CompactionReason) + strategy := strings.TrimSpace(cfg.CompactionStrategy) + if reason != "" || strategy != "" { + matcher.CompactionMatcher = &hookspkg.CompactionMatcher{ + Reason: reason, + Strategy: strategy, + } + } if cfg.ToolReadOnly != nil { value := *cfg.ToolReadOnly matcher.ToolReadOnly = &value } return matcher }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/manager.go` around lines 1620 - 1651, In hookConfigMatcher, avoid always allocating matcher.NetworkMatcher and matcher.CompactionMatcher; instead check whether any of the corresponding cfg fields are non-empty and only then create & assign hookspkg.NetworkMatcher or &hookspkg.CompactionMatcher. Concretely, in hookConfigMatcher inspect cfg.Channel|Surface|Kind|Direction|WorkState and create matcher.NetworkMatcher = &hookspkg.NetworkMatcher{...} only if at least one of those trimmed values is non-empty, and likewise inspect cfg.CompactionReason|CompactionStrategy and only assign matcher.CompactionMatcher = &hookspkg.CompactionMatcher{...} when at least one is non-empty, leaving the pointer nil otherwise (keep existing ToolReadOnly handling).
🧹 Nitpick comments (6)
internal/daemon/network_e2e_assertions_test.go (2)
298-322: ⚡ Quick winWrap test body in
t.Run("Should ...")for consistency.Same issue as above—this test should use the subtest pattern for consistency with the rest of the file.
♻️ Proposed fix
func TestValidateNetworkAuditEntryRejectsWrongContainer(t *testing.T) { t.Parallel() + t.Run("Should reject mismatched direct_id", func(t *testing.T) { + t.Parallel() + entries := []store.NetworkAuditEntry{ { MessageID: "msg_direct_01", Direction: "delivered", Kind: "say", Surface: "direct", DirectID: "direct_wrong", WorkID: "work_patch_42", }, } if err := validateNetworkAuditEntry(entries, networkAuditExpectation{ MessageID: "msg_direct_01", Direction: "delivered", Kind: "say", Surface: "direct", DirectID: "direct_test_01", WorkID: "work_patch_42", }); err == nil { t.Fatal("validateNetworkAuditEntry() error = nil, want direct_id mismatch") } + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/network_e2e_assertions_test.go` around lines 298 - 322, Wrap the test body of TestValidateNetworkAuditEntryRejectsWrongContainer in a subtest using t.Run("Should reject when direct_id mismatches", func(t *testing.T) { ... }) while keeping t.Parallel() and the existing assertions intact; locate the test function TestValidateNetworkAuditEntryRejectsWrongContainer and move the current entries setup and the call to validateNetworkAuditEntry (with the networkAuditExpectation) into the t.Run closure so the test follows the same subtest pattern used elsewhere in the file.
270-296: ⚡ Quick winWrap test body in
t.Run("Should ...")for consistency.This test uses a flat structure while the other tests in this file use
t.Run("Should ...")subtests. As per coding guidelines, "MUST use t.Run('Should...') pattern for ALL test cases."♻️ Proposed fix
func TestValidateNetworkAuditEntryMatchesDuplicateRejection(t *testing.T) { t.Parallel() + t.Run("Should match duplicate rejection audit entry", func(t *testing.T) { + t.Parallel() + entries := []store.NetworkAuditEntry{ { MessageID: "msg_direct_01", Direction: "rejected", Kind: "say", Surface: "direct", DirectID: "direct_test_01", WorkID: "work_patch_42", Reason: "duplicate", }, } if err := validateNetworkAuditEntry(entries, networkAuditExpectation{ MessageID: "msg_direct_01", Direction: "rejected", Kind: "say", Surface: "direct", DirectID: "direct_test_01", WorkID: "work_patch_42", Reason: "duplicate", }); err != nil { t.Fatalf("validateNetworkAuditEntry() error = %v", err) } + }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/network_e2e_assertions_test.go` around lines 270 - 296, Wrap the test body of TestValidateNetworkAuditEntryMatchesDuplicateRejection in a t.Run subtest named starting with "Should" (e.g., t.Run("Should validate duplicate rejection"), func(t *testing.T) { ... }), keeping the existing t.Parallel() and all existing setup and assertions intact; locate the test by the function name TestValidateNetworkAuditEntryMatchesDuplicateRejection and ensure the call to validateNetworkAuditEntry (with the networkAuditExpectation) remains unchanged inside the t.Run closure.internal/network/helpers_test.go (1)
307-310: 💤 Low valueVariable name
blankDirectis misleading after KindDirect removal.This test uses
KindSaybut the variable is namedblankDirect, which is confusing givenKindDirectwas removed from the valid kinds. Consider renaming to reflect what's actually being tested.Suggested rename
- blankDirect, err := DecodeBody(KindSay, mustRawJSON(t, map[string]any{"text": "\n"})) - if !errors.Is(err, ErrInvalidBody) || blankDirect != nil { - t.Fatalf("DecodeBody(blank direct) body = %#v, error = %v, want nil + ErrInvalidBody", blankDirect, err) + blankSayNewline, err := DecodeBody(KindSay, mustRawJSON(t, map[string]any{"text": "\n"})) + if !errors.Is(err, ErrInvalidBody) || blankSayNewline != nil { + t.Fatalf("DecodeBody(blank say newline) body = %#v, error = %v, want nil + ErrInvalidBody", blankSayNewline, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/helpers_test.go` around lines 307 - 310, The variable name blankDirect is misleading because the test calls DecodeBody with KindSay; rename the variable to something like blankSay or blankBody to reflect the actual kind being tested. Update the declaration and all uses of blankDirect in the test block that calls DecodeBody(KindSay, ...) and keep the error check against ErrInvalidBody unchanged so the assertion still verifies DecodeBody, ErrInvalidBody and nil return value for the blank input.internal/hooks/hooks_test.go (1)
1666-1679: ⚡ Quick winSplit the unmatched compaction path into its own subtest.
This is a second behavior case inside
TestDispatchPermissionAndContextHooksApplyPatches; moving it intot.Run("Should leave compaction untouched when no hook matches", ...)will keep failures isolated and match the repo’s Go test conventions.As per coding guidelines,
**/*_test.go: Uset.Run('Should ...')pattern for Go test subtests instead of flat test structures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/hooks_test.go` around lines 1666 - 1679, Move the unmatched compaction verification into its own Go subtest by wrapping the existing unmatchedPayload block inside t.Run("Should leave compaction untouched when no hook matches", func(t *testing.T) { ... }), keeping the call to hooks.DispatchContextPreCompact, error check, and the three assertions (Reason/Strategy, ContextBlocks length) unchanged; place this t.Run inside TestDispatchPermissionAndContextHooksApplyPatches so failures are isolated and follow the repo’s t.Run subtest convention.internal/extension/manifest_test.go (1)
110-160: ⚡ Quick winWrap this new parser scenario in a named subtest.
This new case should follow the repo’s
t.Run("Should ...")convention for consistent failure reporting. Keep it serial here, sincewithDaemonVersionmutates process-wide test state.As per coding guidelines, "Use
t.Run('Should ...')pattern for Go test subtests instead of flat test structures".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/manifest_test.go` around lines 110 - 160, Wrap the existing TestLoadManifestParsesNetworkHookMatcher body in a named subtest using t.Run with a "Should ..." description (e.g., t.Run("Should parse network hook matcher", func(t *testing.T) { ... })), moving the call to withDaemonVersion and all test logic into that subtest so it runs serially; keep all references to LoadManifest, manifestTOMLFileName, hookConfigMatcher and the matcher assertions unchanged but scoped inside the subtest closure to preserve behavior and avoid parallel execution.internal/api/httpapi/network_test.go (1)
144-145: 💤 Low valueConsider using
newTestRouterfor consistency.This test manually creates a gin.Engine and registers only one route, while other tests in this file use
newTestRouter(t, handlers). If the goal is test isolation, this is fine, but using the shared router helper would ensure the handler is wired the same way as production.♻️ Suggested change for consistency
- engine := gin.New() - engine.GET("/api/network/peers/:peer_id/messages", handlers.NetworkPeerMessages) + engine := newTestRouter(t, handlers)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/httpapi/network_test.go` around lines 144 - 145, The test creates a gin.Engine directly and registers only the GET route for handlers.NetworkPeerMessages instead of using the existing test helper newTestRouter, leading to inconsistent wiring; replace the manual engine setup by calling newTestRouter(t, handlers) and use its returned router so the handler is registered the same way as other tests (or, if isolation is required, explicitly document why and ensure middleware and group wiring matches production by mirroring newTestRouter's setup).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/acp/client_test.go`:
- Around line 1094-1125: The TestPromptStopDoesNotEmitRuntimeError function
should be converted to use a t.Run("Should ...") subtest and register guaranteed
cleanup right after the helper process is started: after calling
startHelperProcess(t, driver, "block_prompt_until_cancel", "", StartOpts{})
immediately call t.Cleanup to stop/terminate the proc (e.g., call driver.Stop or
equivalent) to avoid leaking the helper on failures, then keep the existing
logic (Prompt, select, Stop, collectEvents) inside the t.Run body; ensure
references to New(), startHelperProcess, driver.Stop, eventsCh and collectEvents
are preserved and used in the subtest so cleanup always runs even if the test
fails early.
In `@internal/api/core/network_details.go`:
- Around line 1150-1160: summarizeNetworkMessageHistory() currently merges
presence episodes using only channel + peer pair, which collapses pings across
different routing contexts; change the episode grouping key to include routing
metadata Surface, ThreadID, DirectID and WorkID (and keep PeerFrom/PeerTo as
before) so episodes are scoped by surface/thread/direct/work; when merging
choose and preserve the routing fields from the earliest message in the episode
(not the later one) so earlier rows aren't relabeled; apply the same fix to the
other merge/episode-creation sites in this file where similar logic appears (the
nearby presence-merge blocks that handle DisplayName/SessionID/Local/WorkID).
In `@internal/hooks/dispatch_events_test.go`:
- Around line 64-130: Rename the subtest names in the table-driven cases (the
cases slice used by the Test that calls TurnIDFromPayload) from terse labels
like "input" to descriptive "Should ..." phrases (e.g., "Should return turn ID
for input payload") and ensure each subtest goroutine calls t.Parallel() at the
top of the t.Run body unless the subtest intentionally opts out; update the
cases variable entries' name fields and add t.Parallel() inside the anonymous
t.Run closures that invoke TurnIDFromPayload; apply the same change pattern to
the other table-driven subtests in this file that the review references (the
other cases blocks around the file) so all follow the "Should ..." naming and
default to t.Parallel().
In `@internal/hooks/network_dispatch_test.go`:
- Around line 9-86: The test TestDispatchNetworkHooksUseAsyncObservationPayloads
currently asserts all six events in one flat loop; split each event into its own
t.Run subtest (e.g., t.Run(fmt.Sprintf("Should dispatch %s", event.String()),
func(t *testing.T){ ... })) so failures localize. Keep the setup (decls,
executors, hooks, Rebuild) outside the loop, then inside each subtest call the
appropriate dispatch function (DispatchNetworkThreadOpened,
DispatchNetworkDirectRoomOpened, DispatchNetworkMessagePersisted,
DispatchNetworkWorkOpened, DispatchNetworkWorkTransitioned,
DispatchNetworkWorkClosed) using networkDispatchTestPayload(event), and assert
reading from the shared seen channel with the same timeout; run subtests
serially (do not call t.Parallel()) to preserve ordering.
In `@internal/network/manager.go`:
- Around line 1080-1092: The durable delivery path is double-counting and
double-logging because recordSentDelivery and recordReceivedDelivery call both
record*Observed(...) and recordAudit*(...), while
recordAuditSent/recordAuditReceived already increment the same counters and emit
the same logs; update recordSentDelivery and recordReceivedDelivery to call
recordAuditSent/recordAuditReceived only when durable is false (i.e., skip the
audit call for durable deliveries) or alternatively ensure recordAudit* no-ops
when the observed path has already incremented — adjust the logic in
recordSentDelivery, recordReceivedDelivery and mirror the same change in the
other affected block (around the 1351-1393 region) referencing the same
functions so counters/logs are not duplicated.
---
Outside diff comments:
In `@internal/config/hooks.go`:
- Around line 200-239: In toHookMatcher, avoid always allocating
matcher.NetworkMatcher and matcher.CompactionMatcher; instead, only set
matcher.NetworkMatcher to &hookspkg.NetworkMatcher{...} if at least one of
Channel, Surface, Kind, Direction, or WorkState is non-empty (after TrimSpace),
and only set matcher.CompactionMatcher to &hookspkg.CompactionMatcher{...} if
either CompactionReason or CompactionStrategy is non-empty; leave those pointer
fields nil when no related fields are configured so downstream code can rely on
nil meaning "not configured". Use the same trimmed values you already build for
the matcher fields to decide the allocations in parsedHookMatcher.toHookMatcher.
In `@internal/extension/manager.go`:
- Around line 1620-1651: In hookConfigMatcher, avoid always allocating
matcher.NetworkMatcher and matcher.CompactionMatcher; instead check whether any
of the corresponding cfg fields are non-empty and only then create & assign
hookspkg.NetworkMatcher or &hookspkg.CompactionMatcher. Concretely, in
hookConfigMatcher inspect cfg.Channel|Surface|Kind|Direction|WorkState and
create matcher.NetworkMatcher = &hookspkg.NetworkMatcher{...} only if at least
one of those trimmed values is non-empty, and likewise inspect
cfg.CompactionReason|CompactionStrategy and only assign
matcher.CompactionMatcher = &hookspkg.CompactionMatcher{...} when at least one
is non-empty, leaving the pointer nil otherwise (keep existing ToolReadOnly
handling).
In `@internal/network/audit_test.go`:
- Around line 260-275: The subtest "Should return audit write failures" is
missing t.Parallel(), causing inconsistency with sibling subtests; inside the
t.Run block for that test (around the setup using failingAuditStore,
NewAuditWriter and the RecordSent call) add a call to t.Parallel() immediately
after the subtest starts so the subtest runs in parallel with its siblings.
---
Nitpick comments:
In `@internal/api/httpapi/network_test.go`:
- Around line 144-145: The test creates a gin.Engine directly and registers only
the GET route for handlers.NetworkPeerMessages instead of using the existing
test helper newTestRouter, leading to inconsistent wiring; replace the manual
engine setup by calling newTestRouter(t, handlers) and use its returned router
so the handler is registered the same way as other tests (or, if isolation is
required, explicitly document why and ensure middleware and group wiring matches
production by mirroring newTestRouter's setup).
In `@internal/daemon/network_e2e_assertions_test.go`:
- Around line 298-322: Wrap the test body of
TestValidateNetworkAuditEntryRejectsWrongContainer in a subtest using
t.Run("Should reject when direct_id mismatches", func(t *testing.T) { ... })
while keeping t.Parallel() and the existing assertions intact; locate the test
function TestValidateNetworkAuditEntryRejectsWrongContainer and move the current
entries setup and the call to validateNetworkAuditEntry (with the
networkAuditExpectation) into the t.Run closure so the test follows the same
subtest pattern used elsewhere in the file.
- Around line 270-296: Wrap the test body of
TestValidateNetworkAuditEntryMatchesDuplicateRejection in a t.Run subtest named
starting with "Should" (e.g., t.Run("Should validate duplicate rejection"),
func(t *testing.T) { ... }), keeping the existing t.Parallel() and all existing
setup and assertions intact; locate the test by the function name
TestValidateNetworkAuditEntryMatchesDuplicateRejection and ensure the call to
validateNetworkAuditEntry (with the networkAuditExpectation) remains unchanged
inside the t.Run closure.
In `@internal/extension/manifest_test.go`:
- Around line 110-160: Wrap the existing
TestLoadManifestParsesNetworkHookMatcher body in a named subtest using t.Run
with a "Should ..." description (e.g., t.Run("Should parse network hook
matcher", func(t *testing.T) { ... })), moving the call to withDaemonVersion and
all test logic into that subtest so it runs serially; keep all references to
LoadManifest, manifestTOMLFileName, hookConfigMatcher and the matcher assertions
unchanged but scoped inside the subtest closure to preserve behavior and avoid
parallel execution.
In `@internal/hooks/hooks_test.go`:
- Around line 1666-1679: Move the unmatched compaction verification into its own
Go subtest by wrapping the existing unmatchedPayload block inside t.Run("Should
leave compaction untouched when no hook matches", func(t *testing.T) { ... }),
keeping the call to hooks.DispatchContextPreCompact, error check, and the three
assertions (Reason/Strategy, ContextBlocks length) unchanged; place this t.Run
inside TestDispatchPermissionAndContextHooksApplyPatches so failures are
isolated and follow the repo’s t.Run subtest convention.
In `@internal/network/helpers_test.go`:
- Around line 307-310: The variable name blankDirect is misleading because the
test calls DecodeBody with KindSay; rename the variable to something like
blankSay or blankBody to reflect the actual kind being tested. Update the
declaration and all uses of blankDirect in the test block that calls
DecodeBody(KindSay, ...) and keep the error check against ErrInvalidBody
unchanged so the assertion still verifies DecodeBody, ErrInvalidBody and nil
return value for the blank input.
🪄 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: adfab84c-1898-4635-ad84-d73a589b232f
⛔ Files ignored due to path filters (33)
.compozy/tasks/network-threads/reviews-004/issue_001.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_002.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_003.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_004.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_005.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_006.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_007.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_008.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_009.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_010.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_011.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_012.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_013.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_014.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_015.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_016.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_017.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_018.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_019.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_020.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_021.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_022.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_023.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_024.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_025.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_026.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_027.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_028.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_029.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_030.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-004/issue_031.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (27)
internal/acp/client.gointernal/acp/client_test.gointernal/api/core/network.gointernal/api/core/network_details.gointernal/api/httpapi/network_test.gointernal/api/spec/spec.gointernal/api/udsapi/agent_channels_test.gointernal/api/udsapi/handlers_test.gointernal/cli/hooks_test.gointernal/config/hooks.gointernal/config/hooks_test.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/network_e2e_assertions_test.gointernal/extension/manager.gointernal/extension/manifest_test.gointernal/hooks/dispatch_events.gointernal/hooks/dispatch_events_test.gointernal/hooks/hooks_test.gointernal/hooks/introspection_test.gointernal/hooks/network_dispatch_test.gointernal/network/audit_test.gointernal/network/helpers_test.gointernal/network/manager.gointernal/network/manager_test.gointernal/network/perf_bench_test.gointernal/network/router.gointernal/network/router_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/udsapi/handlers_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/hooks/hooks_test.go (1)
1652-1683: 💤 Low valueAdd
t.Parallel()to subtests for consistency.Both subtests follow the
t.Run("Should ...")naming pattern correctly, but are missingt.Parallel()calls. Other subtests in this file (e.g., lines 664, 844, 1229) includet.Parallel(). The parent test already callst.Parallel()and thehooksinstance is thread-safe.♻️ Proposed fix
t.Run("Should patch context compaction when a hook matches", func(t *testing.T) { + t.Parallel() + contextPayload, err := hooks.DispatchContextPreCompact(t.Context(), ContextPreCompactPayload{t.Run("Should leave compaction untouched when no hook matches", func(t *testing.T) { + t.Parallel() + unmatchedPayload, err := hooks.DispatchContextPreCompact(t.Context(), ContextPreCompactPayload{As per coding guidelines: "Use
t.Run('Should ...')subtests witht.Parallelas default (opt-out witht.Setenv)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/hooks_test.go` around lines 1652 - 1683, The two subtests that call hooks.DispatchContextPreCompact (the "Should patch context compaction when a hook matches" and "Should leave compaction untouched when no hook matches" t.Run blocks) need to be marked parallel; add t.Parallel() as the first statement inside each subtest function so they run concurrently with the parent and other subtests while keeping use of DispatchContextPreCompact, contextPayload and unmatchedPayload intact.internal/extension/manifest_test.go (1)
152-160: ⚡ Quick winAssert nested
NetworkMatcherfields directly for stronger regression protection.The current check validates promoted fields on
hookMatcher. Given the pointer-based matcher shape, assertinghookMatcher.NetworkMatcher.<field>directly makes this test stricter and better aligned with the migration intent.Suggested assertion tightening
- if hookMatcher.NetworkMatcher == nil || - hookMatcher.Channel != "builders" || - hookMatcher.Surface != "thread" || - hookMatcher.Kind != "trace" || - hookMatcher.Direction != "received" || - hookMatcher.WorkState != "completed" { + if hookMatcher.NetworkMatcher == nil || + hookMatcher.NetworkMatcher.Channel != "builders" || + hookMatcher.NetworkMatcher.Surface != "thread" || + hookMatcher.NetworkMatcher.Kind != "trace" || + hookMatcher.NetworkMatcher.Direction != "received" || + hookMatcher.NetworkMatcher.WorkState != "completed" { t.Fatalf("hookConfigMatcher() = %#v, want network matcher fields", hookMatcher) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/manifest_test.go` around lines 152 - 160, The test is asserting promoted fields on hookMatcher instead of the nested pointer, so update the assertions to directly check the NetworkMatcher pointer and its fields: ensure hookMatcher.NetworkMatcher != nil and then assert hookMatcher.NetworkMatcher.Channel == "builders", hookMatcher.NetworkMatcher.Surface == "thread", hookMatcher.NetworkMatcher.Kind == "trace", hookMatcher.NetworkMatcher.Direction == "received", and hookMatcher.NetworkMatcher.WorkState == "completed" (all in the hookConfigMatcher test that calls hookConfigMatcher/matcher).internal/acp/client_test.go (1)
1097-1123: ⚡ Quick winMake the subtest parallel and avoid relying on double-stop idempotency
This subtest skips
t.Parallel()and currently invokesStop()both explicitly and again in cleanup. Guarding cleanup after a successful explicit stop makes this test less brittle.Suggested patch
t.Run("Should not emit runtime error after explicit stop", func(t *testing.T) { + t.Parallel() + driver := New() proc := startHelperProcess(t, driver, "block_prompt_until_cancel", "", StartOpts{}) t.Cleanup(func() { - stopProcess(t, driver, proc) + if proc != nil { + stopProcess(t, driver, proc) + } }) @@ if err := driver.Stop(testutil.Context(t), proc); err != nil { t.Fatalf("Stop() error = %v", err) } + proc = nil for _, event := range collectEvents(t, eventsCh) { if event.Type == EventTypeError { t.Fatalf("prompt events contain %q after explicit stop: %#v", EventTypeError, event) } } })As per coding guidelines, "Default to
t.Parallelin Go tests unless there is a specific reason to disable it (opt-out witht.Setenv)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/acp/client_test.go` around lines 1097 - 1123, Add t.Parallel() at the top of the subtest, and avoid double-stopping in cleanup by tracking whether the explicit Stop succeeded; call driver.Stop(testutil.Context(t), proc) as you do now, set a local flag (e.g., stopped) on success, and change the t.Cleanup closure that currently calls stopProcess(t, driver, proc) to only call it when that flag is false. This touches the subtest around New(), startHelperProcess(...), driver.Stop(...), and the t.Cleanup closure to make the test parallel-safe and prevent a second stop.internal/hooks/dispatch_events_test.go (1)
513-574: 💤 Low valueConsider organizing into subtests for better test isolation and failure reporting.
This test covers 9 distinct validation scenarios in a flat structure. While the logic is correct, organizing into
t.Run("Should ...")subtests would:
- Provide clearer failure messages identifying which scenario failed
- Allow parallel execution of independent scenarios
- Align with the table-driven pattern used elsewhere in this file
Additionally, the error assertions (lines 523-528, 545-547, 568-573) only check
err == nilwithout validating error content. Consider usingerrors.Isor checking error messages for more specific assertions.♻️ Example subtest structure (partial)
func TestHookTypeValidationBranches(t *testing.T) { t.Parallel() + t.Run("Should unmarshal valid HookSource from text", func(t *testing.T) { + t.Parallel() + var source HookSource + if err := source.UnmarshalText([]byte(" config ")); err != nil { + t.Fatalf("UnmarshalText(config) error = %v, want nil", err) + } + if source != HookSourceConfig { + t.Fatalf("source = %v, want HookSourceConfig", source) + } + }) + + t.Run("Should reject marshal of invalid HookSource", func(t *testing.T) { + t.Parallel() + _, err := HookSource(200).MarshalText() + if err == nil { + t.Fatal("MarshalText(invalid) error = nil, want error") + } + }) + // ... additional subtests }As per coding guidelines, "Use
t.Run('Should ...')pattern for Go test subtests instead of flat test structures."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/hooks/dispatch_events_test.go` around lines 513 - 574, Split TestHookTypeValidationBranches into named t.Run subtests for each scenario (e.g., "Unmarshal valid config", "Marshal invalid source", "Unmarshal unknown source", "PriorityFromInt valid", "RegisteredHook validate sync/required", "ResolvedHook validate name mismatch", "ResolvedHook nil validate", etc.) and call t.Parallel() inside subtests where they are independent; keep the same setup code but move each assertion into its corresponding subtest. Replace bare err==nil checks with stronger assertions using errors.Is or comparing err.Error()/strings.Contains to expected error values/messages for the UnmarshalText/MarshalText, RegisteredHook.Validate and ResolvedHook.Validate failure checks, and reference the exact symbols from the diff (UnmarshalText, MarshalText, PriorityFromInt, RegisteredHook.Validate, ResolvedHook.Validate, NewTypedNativeExecutor) so reviewers can locate the code. Ensure test names are descriptive so failures show which case failed.internal/network/helpers_test.go (1)
214-248: ⚡ Quick winCover the remaining terminal work states in the transition matrix.
This matrix only pins
completedas terminal.failedandcanceledare part of the new lifecycle too, so regressions likefailed -> workingwould currently slip through.Suggested matrix additions
{name: "ShouldAllowNeedsInputToWorking", current: WorkStateNeedsInput, next: WorkStateWorking, want: true}, {name: "ShouldAllowNeedsInputToCanceled", current: WorkStateNeedsInput, next: WorkStateCanceled, want: true}, {name: "ShouldRejectCompletedToWorking", current: WorkStateCompleted, next: WorkStateWorking, want: false}, + {name: "ShouldRejectFailedToWorking", current: WorkStateFailed, next: WorkStateWorking, want: false}, + {name: "ShouldRejectCanceledToCompleted", current: WorkStateCanceled, next: WorkStateCompleted, want: false}, }As per coding guidelines,
**/*_test.go: Focus on critical paths: workflow execution, state management, error handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/network/helpers_test.go` around lines 214 - 248, The test matrix in helpers_test.go currently treats only Completed as a terminal state; add test rows covering the other terminal states (WorkStateFailed and WorkStateCanceled) to ensure transitions like failed->working and canceled->working are rejected. Specifically, add entries such as "ShouldRejectFailedToWorking", "ShouldRejectCanceledToWorking", "ShouldRejectFailedToSubmitted", and "ShouldRejectCanceledToSubmitted" with current set to WorkStateFailed or WorkStateCanceled, next set to WorkStateWorking or WorkStateSubmitted, and want=false, so the existing assertions around canApplyTrace and ValidateWorkTransition (the functions named canApplyTrace and ValidateWorkTransition) will catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/core/network_details.go`:
- Around line 1154-1164: The visibility logic currently only checks PeerTo
inside isDirectedChannelMessage(), so messages with Surface == "direct" or a
non-empty DirectID are misclassified; update isDirectedChannelMessage() (and any
similar directed-message checks used elsewhere) to also treat entries as
directed when entry.Surface equals the SurfaceDirect value (or literal "direct")
or when entry.DirectID is non-empty/trimmed, and ensure the same change is
applied to the parallel check used around the other mapping path noted (the
second occurrence that mirrors isDirectedChannelMessage()). Trim whitespace when
reading Surface/DirectID to match the struct population.
In `@internal/daemon/network_e2e_assertions_test.go`:
- Around line 152-158: optionalAuditFieldMatches currently treats an empty want
string as “don't care,” which prevents tests from asserting that routing fields
must be absent; change the function signature (optionalAuditFieldMatches) to use
a tri-state expectation—e.g. accept a *string (nil = ignore, pointer to "" =
must be empty, pointer to "X" = must equal) or add an explicit check flag
alongside the desired string—then implement logic to: return true when want is
nil (ignore), return true only if strings.TrimSpace(got) == "" when want points
to "", and otherwise compare trimmed values; update all callers to pass nil/ptr
values (or the flag) so tests can distinguish ignore vs must-be-empty vs
must-equal.
In `@internal/network/helpers_test.go`:
- Around line 210-212: The test currently asserts
OpenWork(withDirectSurface(openErrEnv), ...) returns ErrInvalidField, but this
case represents an actor/permission mismatch; update the assertion to expect the
actor-mismatch sentinel instead (use errors.Is(err, ErrActorMismatch) or the
project's defined ErrActorMismatch constant) when calling OpenWork with
withDirectSurface(openErrEnv) and time.Time{}; keep the rest of the call
(OpenWork, withDirectSurface, openErrEnv) unchanged so callers can distinguish
actor-mismatch from field-validation errors.
---
Nitpick comments:
In `@internal/acp/client_test.go`:
- Around line 1097-1123: Add t.Parallel() at the top of the subtest, and avoid
double-stopping in cleanup by tracking whether the explicit Stop succeeded; call
driver.Stop(testutil.Context(t), proc) as you do now, set a local flag (e.g.,
stopped) on success, and change the t.Cleanup closure that currently calls
stopProcess(t, driver, proc) to only call it when that flag is false. This
touches the subtest around New(), startHelperProcess(...), driver.Stop(...), and
the t.Cleanup closure to make the test parallel-safe and prevent a second stop.
In `@internal/extension/manifest_test.go`:
- Around line 152-160: The test is asserting promoted fields on hookMatcher
instead of the nested pointer, so update the assertions to directly check the
NetworkMatcher pointer and its fields: ensure hookMatcher.NetworkMatcher != nil
and then assert hookMatcher.NetworkMatcher.Channel == "builders",
hookMatcher.NetworkMatcher.Surface == "thread", hookMatcher.NetworkMatcher.Kind
== "trace", hookMatcher.NetworkMatcher.Direction == "received", and
hookMatcher.NetworkMatcher.WorkState == "completed" (all in the
hookConfigMatcher test that calls hookConfigMatcher/matcher).
In `@internal/hooks/dispatch_events_test.go`:
- Around line 513-574: Split TestHookTypeValidationBranches into named t.Run
subtests for each scenario (e.g., "Unmarshal valid config", "Marshal invalid
source", "Unmarshal unknown source", "PriorityFromInt valid", "RegisteredHook
validate sync/required", "ResolvedHook validate name mismatch", "ResolvedHook
nil validate", etc.) and call t.Parallel() inside subtests where they are
independent; keep the same setup code but move each assertion into its
corresponding subtest. Replace bare err==nil checks with stronger assertions
using errors.Is or comparing err.Error()/strings.Contains to expected error
values/messages for the UnmarshalText/MarshalText, RegisteredHook.Validate and
ResolvedHook.Validate failure checks, and reference the exact symbols from the
diff (UnmarshalText, MarshalText, PriorityFromInt, RegisteredHook.Validate,
ResolvedHook.Validate, NewTypedNativeExecutor) so reviewers can locate the code.
Ensure test names are descriptive so failures show which case failed.
In `@internal/hooks/hooks_test.go`:
- Around line 1652-1683: The two subtests that call
hooks.DispatchContextPreCompact (the "Should patch context compaction when a
hook matches" and "Should leave compaction untouched when no hook matches" t.Run
blocks) need to be marked parallel; add t.Parallel() as the first statement
inside each subtest function so they run concurrently with the parent and other
subtests while keeping use of DispatchContextPreCompact, contextPayload and
unmatchedPayload intact.
In `@internal/network/helpers_test.go`:
- Around line 214-248: The test matrix in helpers_test.go currently treats only
Completed as a terminal state; add test rows covering the other terminal states
(WorkStateFailed and WorkStateCanceled) to ensure transitions like
failed->working and canceled->working are rejected. Specifically, add entries
such as "ShouldRejectFailedToWorking", "ShouldRejectCanceledToWorking",
"ShouldRejectFailedToSubmitted", and "ShouldRejectCanceledToSubmitted" with
current set to WorkStateFailed or WorkStateCanceled, next set to
WorkStateWorking or WorkStateSubmitted, and want=false, so the existing
assertions around canApplyTrace and ValidateWorkTransition (the functions named
canApplyTrace and ValidateWorkTransition) will catch regressions.
🪄 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: 6eae0c68-2c8d-4d46-bf6a-9e6e8eb355bf
⛔ Files ignored due to path filters (11)
.compozy/tasks/network-threads/reviews-005/issue_001.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_002.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_003.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_004.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_005.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_006.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_007.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_008.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_009.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_010.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-005/issue_011.mdis excluded by!**/*.md
📒 Files selected for processing (11)
internal/acp/client_test.gointernal/api/core/network_details.gointernal/api/httpapi/network_test.gointernal/daemon/network_e2e_assertions_test.gointernal/extension/manifest_test.gointernal/hooks/dispatch_events_test.gointernal/hooks/hooks_test.gointernal/hooks/network_dispatch_test.gointernal/network/helpers_test.gointernal/network/manager.gointernal/network/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/httpapi/network_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/acp/client_test.go (1)
1094-1135: 💤 Low valueAdd
t.Parallel()inside the subtest for consistency.The subtest is missing
t.Parallel()which other subtests in this file include (e.g., lines 363-364). This should be added unless there's a specific reason to run sequentially.Suggested fix
t.Run("Should not emit runtime error after explicit stop", func(t *testing.T) { + t.Parallel() + driver := New()As per coding guidelines, "Default to
t.Parallelin Go tests unless there is a specific reason to disable it (opt-out witht.Setenv)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/acp/client_test.go` around lines 1094 - 1135, The subtest inside TestPromptStopDoesNotEmitRuntimeError (the t.Run with name "Should not emit runtime error after explicit stop") is missing t.Parallel(); add a call to t.Parallel() as the first statement inside that subtest function to match other tests' parallelization (or explicitly opt out only if you need to use t.Setenv or require sequential execution). This ensures the subtest runs in parallel like the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/network_e2e_assertions_test.go`:
- Around line 26-37: The test helper networkAuditExpectation struct omits the
ReplyTo, TraceID and CausationID fields that the audit persistence code
(functions that set ReplyTo/TraceID/CausationID) writes, so extend the
networkAuditExpectation type to include pointer/string fields for ReplyTo,
TraceID and CausationID and update the assertion logic that compares actual
persisted audit records (the helper assertions referenced around the existing
checks at 84-92 and 114-149) to compare those new fields as well (treat nil vs
empty appropriately), ensuring the tests fail if durable audit persistence stops
storing these correlation keys.
- Around line 247-283: The negative test fixture doesn't provide routing
attributes in any single message/audit row so a buggy
validateNetworkCorrelationSurfaces can pass by aggregating across messages;
update the test data (the messages slice, audit slice, and the
networkCorrelationExpectation) so at least one transcript UIMessage contains the
explicit attributes surface, direct-id, causation-id and trust (e.g., add
attributes to the first messages[0].Parts[0].Text or include them as metadata in
that message), and ensure the audit entries include the corresponding routing
fields for the same MessageID (e.g., add audit rows with
DirectID/WorkID/TraceID/CausationID or the appropriate fields matching
"msg_direct_01"), so the negative case truly verifies that
validateNetworkCorrelationSurfaces (and the networkCorrelationExpectation) does
not succeed by combining attributes from multiple messages.
---
Nitpick comments:
In `@internal/acp/client_test.go`:
- Around line 1094-1135: The subtest inside
TestPromptStopDoesNotEmitRuntimeError (the t.Run with name "Should not emit
runtime error after explicit stop") is missing t.Parallel(); add a call to
t.Parallel() as the first statement inside that subtest function to match other
tests' parallelization (or explicitly opt out only if you need to use t.Setenv
or require sequential execution). This ensures the subtest runs in parallel like
the rest of the file.
🪄 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: 8a1e5a10-db54-4095-8fa3-d54b758157fb
⛔ Files ignored due to path filters (8)
.compozy/tasks/network-threads/reviews-006/issue_001.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_002.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_003.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_004.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_005.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_006.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_007.mdis excluded by!**/*.md.compozy/tasks/network-threads/reviews-006/issue_008.mdis excluded by!**/*.md
📒 Files selected for processing (6)
internal/acp/client_test.gointernal/api/core/network_details.gointernal/daemon/network_e2e_assertions_test.gointernal/hooks/dispatch_events_test.gointernal/hooks/hooks_test.gointernal/network/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/hooks/dispatch_events_test.go
This was referenced May 6, 2026
Closed
Closed
Closed
Closed
Closed
Closed
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
New Features
Bug Fixes