test: add e2e tests#27
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds comprehensive e2e infrastructure, a runtime test harness and artifact system, ACP mock fixture/driver tooling, many integration and Playwright tests, SSE finish payload typing, and threads typed prompt metadata through ACP, session, and network delivery layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client / Web UI
participant Runtime as RuntimeHarness / Daemon (HTTP/UDS)
participant Driver as ACP Driver (acpmock)
participant Agent as Fixture Agent
participant Store as DB / Projection
Client->>Runtime: POST /api/sessions/{id}/prompt (message + optional PromptMeta)
Runtime->>Runtime: normalize & validate PromptMeta
Runtime->>Driver: acpsdk PromptRequest (SessionId, Prompt, Meta)
Driver->>Agent: execute fixture steps (tool calls, permissions, env ops)
Agent-->>Driver: streamed AgentEvents (text-delta, permission, error, done)
Driver->>Runtime: stream AgentEvents (SSE)
Runtime->>Client: forward SSE events (typed "done" finish payload)
Runtime->>Store: persist session/events/transcript/run projections
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/daemon/automation_resources_test.go (2)
75-87:⚠️ Potential issue | 🟠 MajorConvert all tests to table-driven format with
t.Run("Should...")subtests.All four test functions in this file inline multiple scenarios without subtest isolation. Restructure each test using table-driven subtests following the pattern below:
♻️ Refactor pattern
func TestAutomationResourceTargetReturnsProjectorTargetOnlyWhenSupported(t *testing.T) { t.Parallel() - - if got := automationResourceTarget(nil); got != nil { - t.Fatalf("automationResourceTarget(nil) = %#v, want nil", got) - } - if got := automationResourceTarget(automationRuntimeOnlyStub{}); got != nil { - t.Fatalf("automationResourceTarget(runtime without projector target) = %#v, want nil", got) - } - if got := automationResourceTarget(automationRuntimeTargetStub{}); got == nil { - t.Fatal("automationResourceTarget(runtime with projector target) = nil, want target") - } + tests := []struct { + name string + input automationRuntime + wantNil bool + }{ + {name: "Should return nil when runtime is nil", input: nil, wantNil: true}, + {name: "Should return nil when projector target is unsupported", input: automationRuntimeOnlyStub{}, wantNil: true}, + {name: "Should return target when projector target is supported", input: automationRuntimeTargetStub{}, wantNil: false}, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := automationResourceTarget(tt.input) + if (got == nil) != tt.wantNil { + t.Fatalf("automationResourceTarget(%T) nil=%v, want nil=%v", tt.input, got == nil, tt.wantNil) + } + }) + } }This applies to all test functions: lines 75–87, 89–112, 114–185, and 187–223.
Additionally, replace string-based error assertions (
strings.Contains(err.Error(), ...)at lines 103 and 109) witherrors.Is()or a custom error type check per the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/automation_resources_test.go` around lines 75 - 87, Convert this test (and the other tests in the same file) to table-driven subtests using t.Run("Should...") so each scenario is its own subtest; specifically refactor TestAutomationResourceTargetReturnsProjectorTargetOnlyWhenSupported to iterate a slice of cases (name, input runtime, wantNil bool) and call automationResourceTarget with automationRuntimeOnlyStub and automationRuntimeTargetStub inside t.Run subtests, asserting nil vs non-nil accordingly. Do the same pattern for the other tests in this file. Also replace any string-based error assertions (e.g. checks that currently use strings.Contains(err.Error(), "...")) with errors.Is(err, SomeExpectedErr) or a type/assertion-based check using the concrete error value so failures are compared robustly rather than by substring.
101-110:⚠️ Potential issue | 🟠 MajorAvoid string-based error checks; use
errors.Is()orerrors.As()for error validation.Lines 103 and 109 use
strings.Contains(err.Error(), ...)to validate error messages, which violates the coding guideline requiringerrors.Is()orerrors.As()for error matching. Additionally, lines 171–184 only check for nil errors without validating error type or identity.To fix: Define sentinel errors (e.g.,
var ErrRawStoreRequired = errors.New(...)) in the source code, then update the test assertions to useerrors.Is():-} else if !strings.Contains(err.Error(), "raw store is required") { - t.Fatalf("automationResourceStores(nil, codecs) error = %v, want raw store context", err) +} else if !errors.Is(err, ErrRawStoreRequired) { + t.Fatalf("automationResourceStores(nil, codecs) error = %v, want ErrRawStoreRequired", err)Apply the same pattern to lines 109 and strengthen the nil-only checks at 171–184 with explicit error type assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/automation_resources_test.go` around lines 101 - 110, Define sentinel errors (e.g., var ErrRawStoreRequired = errors.New("raw store is required") and var ErrCodecRegistryRequired = errors.New("codec registry is required")) in the package where automationResourceStores is implemented, return those sentinel errors from automationResourceStores when the corresponding argument is missing, and update the test assertions in automation_resources_test.go to use errors.Is(err, ErrRawStoreRequired) and errors.Is(err, ErrCodecRegistryRequired) instead of strings.Contains(err.Error(), ...); also replace the nil-only checks around the later block (lines 171–184) with errors.Is / errors.As checks that assert the expected sentinel error type or value.
🧹 Nitpick comments (25)
internal/daemon/tool_mcp_resources_test.go (3)
15-16: Consider adoptingt.Run("Should...")pattern throughout.The coding guidelines specify: "MUST use
t.Run("Should...")pattern for ALL test cases." While the current tests verify meaningful behavior, restructuring with subtests would improve:
- Test output clarity (each subtest named descriptively)
- Ability to run specific scenarios (
go test -run TestX/Should_handle_Y)- Parallel execution of independent scenarios within a test
This applies to all test functions in this file.
As per coding guidelines: "MUST use t.Run('Should...') pattern for ALL test cases"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/tool_mcp_resources_test.go` around lines 15 - 16, The test TestResourceCatalogProjectorBuildAndApply (and all other test functions in this file) must be restructured to use t.Run("Should...") subtests: replace the direct test body with one or more t.Run calls with descriptive "Should ..." names (e.g., t.Run("Should build and apply resources"), ...) and move t.Parallel() into each subtest (or call it at top of the parent t.Run when appropriate) so subtests can run in parallel; update the test control flow inside TestResourceCatalogProjectorBuildAndApply and any other tests in this file to locate and wrap the existing assertions and setup into these subtests while preserving helper function calls and names like TestResourceCatalogProjectorBuildAndApply.
75-223: Monolithic test function covers too many unrelated concerns.This 150-line test function verifies at least 10 distinct behaviors (nil projectors, nil catalogs, nil plans, codec comparison helpers, nil publisher, syncer creation) without subtests. Per coding guidelines, tests should use
t.Run("Should...")pattern for organization.Splitting into focused subtests improves:
- Failure diagnostics (know exactly which scenario failed)
- Parallel execution within the test
- Readability and maintenance
♻️ Suggested structure using subtests
func TestToolMCPComparisonAndNilHelpers(t *testing.T) { t.Parallel() - if got := newToolProjector(nil); got != nil { - t.Fatalf("newToolProjector(nil) = %#v, want nil", got) - } - if got := newMCPServerProjector(nil); got != nil { - t.Fatalf("newMCPServerProjector(nil) = %#v, want nil", got) - } + t.Run("Should return nil projector when catalog is nil", func(t *testing.T) { + t.Parallel() + if got := newToolProjector(nil); got != nil { + t.Fatalf("newToolProjector(nil) = %#v, want nil", got) + } + if got := newMCPServerProjector(nil); got != nil { + t.Fatalf("newMCPServerProjector(nil) = %#v, want nil", got) + } + }) + + t.Run("Should handle nil catalog operations safely", func(t *testing.T) { + t.Parallel() + // ... nil catalog tests + }) + + t.Run("Should match tools with same scope and encoding", func(t *testing.T) { + t.Parallel() + // ... sameTool tests + }) // ... continue pattern for remaining scenarios🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/tool_mcp_resources_test.go` around lines 75 - 223, The test TestToolMCPComparisonAndNilHelpers is monolithic and should be split into focused subtests using t.Run (and t.Parallel where appropriate); create separate subtests for: "nil projectors" (validate newToolProjector/newMCPServerProjector return nil), "nil catalog helpers" (nilCatalog.Replace/Snapshot/Revision), "nil plan helpers" (resourceCatalogProjectionPlan.Kind/Revision/OperationCount), "nil projector methods" (resourceCatalogProjector.Kind/DependsOn/Build/Apply error behavior), "codec comparison helpers" (sameTool and sameMCPServer checks using toolCodec/mcpCodec and sample records), "nil publisher behavior" (nilPublisher.Sync and wrapped publisher invocation), and "syncer creation" (newToolMCPSourceSyncer(nil deps) returns nil and newToolMCPSourceSyncer with nil logger returns concrete *toolMCPSourceSyncer and its Sync runs); keep the same assertions but move each block into its own t.Run with descriptive names and call t.Parallel() in subtests where safe.
566-574: Consider usingerrors.Newfor sentinel errors.The custom
errorStringtype works but adds complexity. Standard sentinel errors usingerrors.Newwork better witherrors.Is():♻️ Alternative using standard errors
-func assertErr(message string) error { - return errorString(message) -} - -type errorString string - -func (e errorString) Error() string { - return string(e) -} +var ( + errTriggerFailure = errors.New("trigger failure") + errProviderFailure = errors.New("provider failure") +)Then use these sentinels directly in tests and match with
errors.Is().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/tool_mcp_resources_test.go` around lines 566 - 574, Replace the custom error type utilities (the assertErr function, errorString type and its Error() method) with a standard sentinel error created via errors.New and update tests to compare using errors.Is; specifically remove or stop using assertErr and errorString, declare a package-level sentinel via errors.New("..."), import the standard "errors" package, and change test comparisons to errors.Is(err, sentinel) so tests use the standard error sentinel pattern.web/playwright.config.ts (1)
25-27: Consider enabling trace/screenshot on failure for CI debugging.With all artifacts disabled, diagnosing flaky or failing E2E tests in CI becomes difficult. Playwright supports conditional artifact capture.
♻️ Suggested configuration for better CI debugging
use: { ...devices["Desktop Chrome"], headless: process.env.PLAYWRIGHT_HEADFUL !== "1", - trace: "off", - screenshot: "off", - video: "off", + trace: "retain-on-failure", + screenshot: "only-on-failure", + video: "off", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/playwright.config.ts` around lines 25 - 27, Enable conditional artifact capture in the Playwright config so CI failures can be diagnosed: update the playwrigh.config.ts settings for trace, screenshot, and video (the trace, screenshot, and video fields) from "off" to CI-friendly modes such as "on-first-retry" or "on" for trace and "only-on-failure"/"retain-on-failure" or "on-first-retry" for screenshot and video, so artifacts are collected only when tests fail or on retries.web/src/routes/_app/-automation.integration.test.tsx (1)
59-63: Use aninterfacefor mockedLinkprops shape.This works, but the inline object type for
paramsshould be promoted to aninterfaceto match repo TS shape conventions.♻️ Suggested refactor
+interface MockLinkProps extends AnchorHTMLAttributes<HTMLAnchorElement> { + params?: { id?: string }; +} + vi.mock("@tanstack/react-router", () => ({ createFileRoute: () => (opts: { component: () => React.ReactNode }) => ({ component: opts.component, }), Link: ({ children, params, ...props - }: AnchorHTMLAttributes<HTMLAnchorElement> & { params?: { id?: string } }) => ( + }: MockLinkProps) => ( <a href={`/session/${params?.id ?? ""}`} {...props}> {children} </a> ), }));As per coding guidelines,
web/**/*.ts?(x): Useinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/routes/_app/-automation.integration.test.tsx` around lines 59 - 63, The inline object type for the mocked Link component's params should be extracted to an interface: create an interface (e.g., LinkProps or MockLinkProps) that extends AnchorHTMLAttributes<HTMLAnchorElement> and declares params?: { id?: string }, then update the Link component signature to use that interface instead of the inline type; update any related references in the test to use the new interface name for consistency with the repo TS conventions.web/src/systems/automation/components/automation-detail-panel.test.tsx (1)
6-10: Promote mockedLinkprops shape to aninterface.The inline object shape works, but it violates the repo TypeScript shape convention.
As per coding guidelines,
web/**/*.ts?(x): Useinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/systems/automation/components/automation-detail-panel.test.tsx` around lines 6 - 10, Replace the inline prop type for the mocked Link component with a named interface (e.g., declare interface LinkProps) and use it in the component signature instead of the inline AnchorHTMLAttributes<...> & { params?: { id?: string } } type; update the function signature (Link: (props: LinkProps) => ...) and export or keep the interface local to the test file so the prop shape follows the repo TypeScript convention and matches other tests/components.internal/api/httpapi/prompt.go (1)
407-417: Prefer a typed finish payload overmap[string]any.
typeandfinishReasonare known fields; a typed struct improves safety and keeps payload contracts explicit.♻️ Suggested change
+type aiSDKFinishPayload struct { + Type string `json:"type"` + FinishReason string `json:"finishReason,omitempty"` +} @@ - finishPayload := map[string]any{ - "type": "finish", - } + finishPayload := aiSDKFinishPayload{Type: "finish"} if finishReason := aiSDKFinishReason(event.StopReason); finishReason != "" { - finishPayload["finishReason"] = finishReason + finishPayload.FinishReason = finishReason }As per coding guidelines:
Never use interface{}/any when a concrete type is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/prompt.go` around lines 407 - 417, Replace the untyped finishPayload map with a concrete struct type (e.g., FinishPayload with fields Type string `json:"type"` and FinishReason string `json:"finishReason,omitempty"`) and populate that struct using aiSDKFinishReason(event.StopReason) instead of assigning to map keys; then pass the typed struct as Data to core.WriteSSE (SSEMessage{Name: "done", Data: finishPayload}) to enforce a concrete payload contract and remove use of map[string]any.internal/api/httpapi/handlers_test.go (1)
715-771: Use a typed finish payload in this test instead ofmap[string]any.The parsed shape is known (
finishReason, optionalstopReason), so a typed struct is safer and clearer.♻️ Suggested change
- var finishPart map[string]any + var finishPart struct { + FinishReason string `json:"finishReason"` + StopReason *string `json:"stopReason,omitempty"` + } @@ - if got := finishPart["finishReason"]; got != "stop" { + if got := finishPart.FinishReason; got != "stop" { t.Fatalf("finishReason = %#v, want %q", got, "stop") } - if _, ok := finishPart["stopReason"]; ok { + if finishPart.StopReason != nil { t.Fatalf("finish part unexpectedly includes stopReason: %#v", finishPart) }As per coding guidelines:
Never use interface{}/any when a concrete type is known.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/handlers_test.go` around lines 715 - 771, Replace the untyped maps with concrete structs: define a finish payload struct (e.g., finishPayload { FinishReason string `json:"finishReason"` StopReason *string `json:"stopReason,omitempty"` }) and unmarshal the final record into that instead of finishPart, then assert finishPayload.FinishReason == "stop" and finishPayload.StopReason == nil; likewise replace the generic prompt part parsing with a small typed struct (e.g., partType { Type string `json:"type"` }) and unmarshal each prompt record into it to collect the Type values; update references to finishPart and promptParts to use these typed variables in the existing checks.internal/extensiontest/bridge_adapter_harness_test.go (1)
212-303: Wrap this case in at.Run("Should...")subtest and mark it parallel.This test is isolated and fits the repo’s required subtest pattern.
♻️ Suggested structure
func TestMarkerHelpersReadStandaloneMarkerSet(t *testing.T) { - markers := NewMarkerPaths(t.TempDir()) - // ... + t.Run("ShouldReadStandaloneMarkerSet", func(t *testing.T) { + t.Parallel() + markers := NewMarkerPaths(t.TempDir()) + // existing assertions... + }) }As per coding guidelines:
MUST use t.Run("Should...") pattern for ALL test casesandUse t.Parallel() for independent subtests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extensiontest/bridge_adapter_harness_test.go` around lines 212 - 303, Wrap the existing TestMarkerHelpersReadStandaloneMarkerSet body in a t.Run subtest named starting with "Should..." and make that subtest run in parallel by calling t.Parallel() as the first statement inside the subtest; specifically, in the TestMarkerHelpersReadStandaloneMarkerSet function move the existing setup/assertions into t.Run("Should read standalone marker set", func(t *testing.T) { t.Parallel(); ... }) so the test uses the repo's subtest pattern while keeping all uses of NewMarkerPaths, WaitForHandshakeMarker, WaitForStateMarkers, WaitForDeliveryMarkers, WaitForIngestMarkers and ReportFromMarkers unchanged inside the subtest.web/e2e/session-onboarding.spec.ts (1)
76-80: Consider simplifying URL assertion to avoid regex construction.Same pattern flagged by static analysis as in
network.spec.ts. The escaped regex from variable input has minimal ReDoS risk here but can be simplified.Proposed simplification
await appPage.reload({ waitUntil: "domcontentloaded" }); - await expect(appPage).toHaveURL( - new RegExp(`${sessionPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`) - ); + expect(new URL(appPage.url()).pathname).toBe(sessionPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/e2e/session-onboarding.spec.ts` around lines 76 - 80, Replace the regex-based URL assertion with a direct string assertion to avoid constructing an escaped regex: use the existing appPage and sessionPath values and call toHaveURL with sessionPath (or the fully-qualified URL if needed) instead of building new RegExp(...). Update the assertion around appPage.reload and the following expect(appPage).toHaveURL(...) so it compares against the plain sessionPath string.web/e2e/network.spec.ts (1)
124-129: Consider simplifying URL assertion to avoid regex construction.Static analysis flagged potential ReDoS risk. While the input is controlled (from
appPage.url()), you could simplify this by using Playwright's built-in string URL matching or extracting the path comparison.Proposed simplification
const networkPath = new URL(appPage.url()).pathname; await appPage.reload({ waitUntil: "domcontentloaded" }); - await expect(appPage).toHaveURL( - new RegExp(`${networkPath.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}$`) - ); + expect(new URL(appPage.url()).pathname).toBe(networkPath);This avoids regex construction entirely while validating the same behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/e2e/network.spec.ts` around lines 124 - 129, Replace the regex-based URL assertion with a simpler string or path comparison to avoid building a RegExp from appPage.url(); specifically, keep networkPath and the reload call, then use await expect(appPage).toHaveURL(networkPath) or alternatively compare new URL(await appPage.url()).pathname === networkPath to assert the path; update the assertion around expect(appPage).toHaveURL(...) and remove the RegExp construction.internal/e2elane/lanes_test.go (1)
133-139: Consider adding specific error message validation.The test verifies that an error is returned for unknown lanes, but doesn't validate the error message content. This could mask regressions where the error type or message changes unexpectedly.
Proposed enhancement
func TestPlanForLaneRejectsUnknownLane(t *testing.T) { t.Parallel() - if _, err := PlanForLane("mystery"); err == nil { + _, err := PlanForLane("mystery") + if err == nil { t.Fatal("PlanForLane() error = nil, want non-nil") } + if !strings.Contains(err.Error(), "mystery") { + t.Fatalf("PlanForLane() error = %v, want error mentioning unknown lane", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/e2elane/lanes_test.go` around lines 133 - 139, Test PlanForLane for unknown lanes currently only asserts a non-nil error; update TestPlanForLaneRejectsUnknownLane to also validate the error content/type by checking the returned error from PlanForLane("mystery") contains or matches the expected message or sentinel (e.g., "unknown lane" or the package's exported error constant) using errors.Is or a string assertion so the test fails if the error message/type regresses; reference PlanForLane and TestPlanForLaneRejectsUnknownLane when making this change.internal/e2elane/command_wiring_test.go (1)
32-42: Consider addingt.Parallel()to subtests.The subtests test independent make targets and could run concurrently to reduce test execution time.
Proposed change
for _, tt := range tests { t.Run(tt.target, func(t *testing.T) { + t.Parallel() output := runCommand(t, repoRoot, "make", "-n", tt.target)As per coding guidelines: "Use t.Parallel() for independent subtests in Go tests"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/e2elane/command_wiring_test.go` around lines 32 - 42, Subtests are independent and should run in parallel: inside the t.Run closure for the loop over tests (the subtest that calls runCommand with repoRoot and tt.target), capture the loop variable (e.g. add tt := tt) and call t.Parallel() as the first statement in the closure so each subtest can execute concurrently; keep the rest of the checks (strings.Contains, t.Fatalf) unchanged.internal/testutil/e2e/runtime_harness_helpers_test.go (1)
591-624: Assert the error cause, not just “non-nil”.Every branch here passes on any failure path, including unrelated request-shape bugs. This helper is supposed to prove transport/decode errors are surfaced, so the assertions should check the distinguishing status/decoding context instead of only
err != nil. As per coding guidelines, "MUST have specific error assertions (ErrorContains, ErrorAs)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/runtime_harness_helpers_test.go` around lines 591 - 624, The tests currently only assert non-nil errors; change each assertion to verify the specific error cause: for harness.GetExtension, harness.CreateBridge, harness.PutBridgeSecretBinding, harness.CaptureBridgeRoutes and harness.CaptureBridgeSecretBindings assert the error is the transport-layer error (use require.ErrorIs/require.ErrorContains or errors.As to check for the transport error type/marker—e.g. errors.Is(err, <transport error sentinel>) or ErrorAs into a *TransportError) so we prove transport failures surfaced; for harness.CaptureBridgeHealth assert a decoding error (use require.ErrorAs to match json.SyntaxError/json.UnmarshalTypeError or ErrorContains with "decode"/"unmarshal") instead of just err != nil. Ensure each t.Fatal(...) is replaced with the appropriate specific assertion that checks error type/message for the given function name listed above.internal/api/httpapi/transport_parity_integration_test.go (1)
26-111: Addt.Parallel()to integration test functions.The test functions are missing
t.Parallel()calls. For integration tests that use isolated harnesses, parallel execution should be safe and improve test suite performance.♻️ Proposed fix
func TestHTTPTransportApprovalFlowUsesSharedRuntimeHarness(t *testing.T) { + t.Parallel() + skipWithoutNode(t)Apply similar change to
TestHTTPTransportWebhookIngressUsesSharedRuntimeHarnessat line 113.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/transport_parity_integration_test.go` around lines 26 - 111, Test functions TestHTTPTransportApprovalFlowUsesSharedRuntimeHarness (and similarly TestHTTPTransportWebhookIngressUsesSharedRuntimeHarness) do not call t.Parallel() to enable parallel execution; add a t.Parallel() call as the first statement inside each test body (immediately after skipWithoutNode(t)) so the integration tests using isolated e2etest.StartRuntimeHarness can run concurrently and speed up the suite.internal/daemon/daemon_mock_agents_integration_test.go (1)
21-78: Addt.Parallel()to integration test functions.The daemon E2E test functions are missing
t.Parallel()calls. Each test uses an isolated runtime harness, so parallel execution should be safe.♻️ Proposed fix
func TestDaemonE2EFixtureBackedMockAgentLaunchesThroughNormalAgentDefinition(t *testing.T) { + t.Parallel() + skipWithoutNode(t)Apply similar changes to
TestDaemonE2EMockAgentsRemainIsolated(line 80) andTestDaemonE2EToolPermissionFixtureEventsSurface(line 153).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_mock_agents_integration_test.go` around lines 21 - 78, Add t.Parallel() to the start of the integration test functions so they can run concurrently: insert a call to t.Parallel() in the body of TestDaemonE2EFixtureBackedMockAgentLaunchesThroughNormalAgentDefinition (ideally immediately after skipWithoutNode(t) or at the top of the test), and apply the same change to TestDaemonE2EMockAgentsRemainIsolated and TestDaemonE2EToolPermissionFixtureEventsSurface to enable parallel execution while preserving the existing harness setup and teardown.internal/testutil/e2e/runtime_harness_integration_test.go (2)
24-34: Clarify helper process behavior with a comment.The
TestE2EACPHelperProcessfunction is a subprocess entry point, not a regular test. The early return when the env var isn't set is correct, but the pattern could benefit from a brief comment explaining its purpose.📝 Proposed documentation
+// TestE2EACPHelperProcess is a subprocess entry point invoked by tests that +// spawn the test binary with -test.run=TestE2EACPHelperProcess. It implements +// an in-process ACP agent for E2E validation. func TestE2EACPHelperProcess(t *testing.T) { if os.Getenv(e2eACPHelperEnvKey) != "1" { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/runtime_harness_integration_test.go` around lines 24 - 34, Add a short explanatory comment above TestE2EACPHelperProcess describing that this function is an entry point for a helper subprocess (triggered when the e2eACPHelperEnvKey environment variable is set) rather than a normal test; specifically note that the early return when the env var is not "1" prevents the function from running in the parent test process and that the function spins up e2eACPAgent with acpsdk.NewAgentSideConnection and exits when the connection completes. Reference the existing function name TestE2EACPHelperProcess and the env var symbol e2eACPHelperEnvKey so reviewers can locate and understand the subprocess pattern.
36-166: Addt.Parallel()to integration test functions.The main test functions (
TestStartRuntimeHarnessBootsRealDaemonAndExposesClients,TestStartRuntimeHarnessResolvesSeededWorkspaceThroughPublicSurface,TestStartRuntimeHarnessCapturesTranscriptAndEventsArtifacts) are missingt.Parallel()calls.♻️ Proposed fix
func TestStartRuntimeHarnessBootsRealDaemonAndExposesClients(t *testing.T) { + t.Parallel() + harness := StartRuntimeHarness(t, RuntimeHarnessOptions{})Apply similar changes to the other test functions at lines 66 and 93.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/runtime_harness_integration_test.go` around lines 36 - 166, Add t.Parallel() as the first statement in each integration test function to enable running tests in parallel: TestStartRuntimeHarnessBootsRealDaemonAndExposesClients, TestStartRuntimeHarnessResolvesSeededWorkspaceThroughPublicSurface, and TestStartRuntimeHarnessCapturesTranscriptAndEventsArtifacts; ensure you insert t.Parallel() immediately after each func's opening line (before any harness setup or context creation) so parallel execution doesn't interfere with shared state.internal/testutil/acpmock/fixture_test.go (1)
274-352: Consider addingt.Parallel()to the parent test function.
TestRegistrationHelperOverridesAndDiagnosticsErrorsis missingt.Parallel()on the parent function. Note that the subtest usingt.Setenv(line 280) correctly omitst.Parallel()sincet.Setenvis incompatible with parallel execution.♻️ Proposed fix
func TestRegistrationHelperOverridesAndDiagnosticsErrors(t *testing.T) { + t.Parallel() + t.Run("resolve node override and env override", func(t *testing.T) { + // Note: No t.Parallel() here due to t.Setenv usage if got, err := resolveNodePath("/custom/node"); err != nil || got != "/custom/node" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/acpmock/fixture_test.go` around lines 274 - 352, Add t.Parallel() as the first statement inside TestRegistrationHelperOverridesAndDiagnosticsErrors to allow the whole test to run in parallel, but leave the "resolve node override and env override" subtest (the one that calls t.Setenv) without t.Parallel() so it remains serial; ensure t.Parallel() is added in the parent function (TestRegistrationHelperOverridesAndDiagnosticsErrors) before creating any t.Run subtests.internal/testutil/e2e/runtime_harness_lifecycle_test.go (1)
25-34: Consider handling the JSON encoding error.The error from
json.NewEncoder(w).Encode()is ignored. While this is unlikely to fail in test scenarios, handling it would align with the coding guideline to never ignore errors.♻️ Proposed fix
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _ = json.NewEncoder(w).Encode(aghcontract.DaemonStatusResponse{ + if err := json.NewEncoder(w).Encode(aghcontract.DaemonStatusResponse{ Daemon: aghcontract.DaemonStatusPayload{ Status: "running", Socket: "/tmp/agh.sock", HTTPHost: "127.0.0.1", HTTPPort: 2123, }, - }) + }); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + } }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/runtime_harness_lifecycle_test.go` around lines 25 - 34, The JSON encoding error from json.NewEncoder(w).Encode(...) inside the httptest.NewServer handler is currently ignored; capture its return value and handle it (e.g., call t.Fatalf or t.Error with the error) so the test fails on encoding problems. Update the handler in the server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { ... })) block to check err := json.NewEncoder(w).Encode(aghcontract.DaemonStatusResponse{...}); if err != nil { t.Fatalf("encoding response: %v", err) } (or use your preferred test assertion) so failures are not silently ignored.internal/testutil/acpmock/driver_test.go (1)
17-84: Addt.Parallel()to enable parallel test execution.The test functions are missing
t.Parallel()calls. Per coding guidelines, independent tests should run in parallel.♻️ Proposed fix
func TestDriverStreamsStablePermissionAndToolSequence(t *testing.T) { + t.Parallel() + nodePath, err := ResolveNodePath()Apply similar changes to
TestDriverSupportsNetworkOriginEnvironmentExpectations(line 86) andTestDriverAdvertisesAndSupportsLoadSession(line 160).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/acpmock/driver_test.go` around lines 17 - 84, Add t.Parallel() to each independent test so they can run concurrently: in TestDriverStreamsStablePermissionAndToolSequence, TestDriverSupportsNetworkOriginEnvironmentExpectations, and TestDriverAdvertisesAndSupportsLoadSession insert a call to t.Parallel() near the start of each test function body (after any initial ResolveNodePath() skip/early checks) so the tests run in parallel without changing existing setup or teardown logic.internal/api/udsapi/transport_parity_integration_test.go (1)
144-163: Replace the sleep-based polling helpers.These wait loops rely on fixed
time.Sleepintervals, which makes the transport lane slower and flakier under CI load. Switch them to the shared wait helper or a context-aware ticker/select so the wait can stop immediately on success or cancellation. As per coding guidelines, Never usetime.Sleep()in orchestration — use proper synchronization primitives.Also applies to: 165-185, 222-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/udsapi/transport_parity_integration_test.go` around lines 144 - 163, The helper waitForUDSAutomationRun uses a time.Sleep-based polling loop; replace it with a context-aware wait (either the project’s shared wait helper or a ticker + select on ctx.Done()) so the loop exits immediately on success or cancellation and doesn’t block CI. Locate waitForUDSAutomationRun and the other similar helpers in the file (the blocks around lines ~165-185 and ~222-242) and change the for loop to use a context-aware select that checks the ticker.C and ctx.Done() (or call the existing harness/shared WaitFor helper) and remove time.Sleep; preserve the same deadline/timeout behavior by honoring ctx or a time.After timeout. Ensure error handling and t.Fatalf behavior remain the same when the timeout/context cancellation occurs.internal/daemon/daemon_network_collaboration_integration_test.go (1)
856-872: Avoid fixed sleeps inwaitForRuntimeCondition.Every waiter in this file now depends on a polling loop with
time.Sleep, which adds CI flake and delays cancellation. Prefer a context-aware ticker/select or an event-driven wait primitive from the harness. As per coding guidelines, Never usetime.Sleep()in orchestration — use proper synchronization primitives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_network_collaboration_integration_test.go` around lines 856 - 872, The polling loop in waitForRuntimeCondition uses fixed time.Sleep which can delay cancellation and cause CI flake; change it to a context-aware wait using a time.Ticker and select (watching ticker.C and a ctx.Done channel) or use the test harness's event/wait primitive if available; update the waitForRuntimeCondition signature to accept a context.Context (or derive one from t) and replace the for/sleep with a select over the ticker and context deadline/cancellation, ensuring you still return on fn() true and call t.Fatalf on timeout/cancellation with the label.internal/testutil/e2e/artifacts.go (1)
384-406: Consider usingtarget.Sync()before closing for durability.The
copyFilefunction callstarget.Close()twice (once in defer, once explicitly). The explicit close is intentional to return any write errors, but consider addingtarget.Sync()before closing if durability across power loss is important for artifact preservation.♻️ Optional: Add sync for durability
if _, err := io.Copy(target, source); err != nil { return err } + if err := target.Sync(); err != nil { + return err + } + return target.Close() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/artifacts.go` around lines 384 - 406, The copyFile function should call target.Sync() before the explicit target.Close() to ensure data is flushed to disk; update the copyFile implementation (function copyFile, variables target/source) to call target.Sync(), check and return any error from Sync, then proceed to the explicit Close() to capture final write/close errors (keeping the deferred Close() as a safety net).internal/testutil/e2e/bridges_extensions.go (1)
13-15: Consider validating empty bridgeID before path construction.The
bridgePathhelper trims whitespace but doesn't return an error for empty IDs. While callers likeGetBridgemay rely on the server to reject empty paths, explicit validation would fail faster and provide clearer error messages.♻️ Optional: Add empty ID validation
func bridgePath(bridgeID string) string { + trimmed := strings.TrimSpace(bridgeID) + if trimmed == "" { + return "/api/bridges/" + } - return "/api/bridges/" + url.PathEscape(strings.TrimSpace(bridgeID)) + return "/api/bridges/" + url.PathEscape(trimmed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/bridges_extensions.go` around lines 13 - 15, The helper bridgePath currently trims but doesn't reject empty IDs; change bridgePath to validate the trimmed bridgeID and return an error when it's empty (e.g., signature bridgePath(bridgeID string) (string, error)), construct and return the escaped path only on success, and update callers such as GetBridge to handle the error and propagate or fail fast with a clear message; ensure callers no longer assume a valid path string without checking the returned error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c575b271-c966-4cab-bcd2-b4a3ab6720ce
⛔ Files ignored due to path filters (24)
bun.lockis excluded by!**/*.lockinternal/testutil/acpmock/testdata/automation_task_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/bridge_ingress_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/browser_session_lifecycle_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/multi_agent_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!**/*.jsoninternal/testutil/acpmock/testdata/tool_permission_golden.jsonis excluded by!**/*.jsonpackage.jsonis excluded by!**/*.jsonweb/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-helpers.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/runtime.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/runtime.tsis excluded by!**/fixtures/**web/e2e/fixtures/selectors.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/selectors.tsis excluded by!**/fixtures/**web/e2e/fixtures/test.tsis excluded by!**/fixtures/**web/package.jsonis excluded by!**/*.jsonweb/tsconfig.jsonis excluded by!**/*.json
📒 Files selected for processing (65)
Makefileinternal/api/httpapi/handlers_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/prompt.gointernal/api/httpapi/transport_parity_integration_test.gointernal/api/udsapi/transport_parity_integration_test.gointernal/daemon/automation_resources_test.gointernal/daemon/automation_task_e2e_assertions_test.gointernal/daemon/bridge_extension_e2e_assertions_test.gointernal/daemon/daemon_automation_task_integration_test.gointernal/daemon/daemon_bridge_extension_integration_test.gointernal/daemon/daemon_environment_sandbox_integration_test.gointernal/daemon/daemon_mock_agents_integration_test.gointernal/daemon/daemon_network_collaboration_integration_test.gointernal/daemon/daemon_nightly_combined_integration_test.gointernal/daemon/network_e2e_assertions_test.gointernal/daemon/tool_mcp_resources_test.gointernal/e2elane/command_wiring_test.gointernal/e2elane/lanes.gointernal/e2elane/lanes_test.gointernal/extensiontest/bridge_adapter_harness.gointernal/extensiontest/bridge_adapter_harness_test.gointernal/testutil/acpmock/diagnostics.gointernal/testutil/acpmock/driver_test.gointernal/testutil/acpmock/fixture.gointernal/testutil/acpmock/fixture_test.gointernal/testutil/acpmock/registration.gointernal/testutil/e2e/artifacts.gointernal/testutil/e2e/artifacts_test.gointernal/testutil/e2e/automation_tasks.gointernal/testutil/e2e/automation_tasks_test.gointernal/testutil/e2e/bridges_extensions.gointernal/testutil/e2e/config_seed.gointernal/testutil/e2e/config_seed_test.gointernal/testutil/e2e/mock_agents.gointernal/testutil/e2e/mock_agents_test.gointernal/testutil/e2e/runtime_harness.gointernal/testutil/e2e/runtime_harness_helpers_test.gointernal/testutil/e2e/runtime_harness_integration_test.gointernal/testutil/e2e/runtime_harness_lifecycle_test.gointernal/testutil/e2e/runtime_harness_test.gointernal/testutil/e2e/transport_parity.gointernal/testutil/e2e/transport_parity_test.gomagefile.goweb/e2e/automation.spec.tsweb/e2e/bridges.spec.tsweb/e2e/combined-flows.spec.tsweb/e2e/harness-smoke.spec.tsweb/e2e/network.spec.tsweb/e2e/session-onboarding.spec.tsweb/playwright.config.tsweb/src/hooks/routes/use-network-page.tsweb/src/routes/_app/-automation.integration.test.tsxweb/src/routes/_app/-network.test.tsxweb/src/routes/_app/network.tsxweb/src/styles.cssweb/src/styles.test.tsweb/src/systems/automation/components/automation-detail-panel.test.tsxweb/src/systems/automation/components/automation-run-history.tsxweb/src/systems/bridges/hooks/use-bridge-health-stream.test.tsxweb/src/systems/bridges/hooks/use-bridge-health-stream.tsweb/src/systems/network/components/network-peer-detail-panel.tsxweb/src/systems/workspace/components/workspace-setup.tsxweb/src/test-setup.tsweb/vitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
internal/testutil/e2e/config_seed.go (1)
307-316:⚠️ Potential issue | 🟠 MajorUse
internal/frontmatterinstead of direct YAML marshal for AGENT.md metadata.This still bypasses the repository frontmatter helper and keeps custom framing logic in this file. Please switch metadata serialization/parsing to
internal/frontmatterfor consistency with repo conventions.As per coding guidelines, "Use YAML frontmatter parsing from
internal/frontmatterfor document metadata".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/e2e/config_seed.go` around lines 307 - 316, Replace the direct YAML marshal and manual framing with the repository frontmatter helper: instead of calling yaml.Marshal(metadata) and building the document with strings.Builder, import internal/frontmatter and use its frontmatter serialization API to produce the frontmatter block and full document; update the code that currently references frontmatterBytes, yaml.Marshal, builder, defaultString(seed.Prompt, "You are "+name+".") to call the frontmatter package (e.g., frontmatter.Marshal/Encode or the package's provided helper to render frontmatter + body) so the metadata is serialized and framed consistently with the rest of the repo.
🧹 Nitpick comments (6)
internal/api/httpapi/prompt.go (1)
64-68: Harden the finish-event schema by removingstopReasonfrom the outbound payload type.
StopReasonis intentionally no longer emitted for"done"events, but keeping it inpromptFinishPayloadleaves room for accidental reintroduction later. Prefer a strict DTO for the wire contract.Suggested diff
type promptFinishPayload struct { Type string `json:"type"` FinishReason string `json:"finishReason,omitempty"` - StopReason *string `json:"stopReason,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/prompt.go` around lines 64 - 68, The outbound finish-event DTO promptFinishPayload still includes the StopReason field which allows accidental reintroduction of stopReason into "done" events; remove the StopReason field from the promptFinishPayload struct so the wire contract only contains Type and FinishReason, update any code that marshals or constructs promptFinishPayload (references to promptFinishPayload, StopReason, Type, FinishReason) to stop setting or relying on StopReason, and if other event variants need stopReason keep them in a separate explicit DTO rather than this struct.internal/daemon/daemon_bridge_extension_integration_test.go (3)
87-109: Minor redundancy:Statusfield may be unnecessary whenEnabled: false.Setting both
Enabled: falseandStatus: bridgepkg.BridgeStatusDisabledappears redundant. Consider whether the API derives status from the enabled flag, making the explicit status field unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_bridge_extension_integration_test.go` around lines 87 - 109, The test redundantly sets both Enabled: false and Status: bridgepkg.BridgeStatusDisabled in the aghcontract.CreateBridgeRequest; update the CreateBridge call in the test (the request passed to harness.CreateBridge) to only set the definitive source of truth (either remove the Status field and assert createdBridge.Bridge.Status.Normalize() equals bridgepkg.BridgeStatusDisabled derived from Enabled, or keep Status and remove/ignore Enabled) and then adjust the subsequent assertions around createdBridge.Bridge.Enabled and createdBridge.Bridge.Status.Normalize() to match that chosen approach (refer to CreateBridge request construction and the createdBridge.Bridge.Status.Normalize() / createdBridge.Bridge.Enabled checks).
334-336: Consider extracting complex condition for readability.The compound condition checking both first and last ingest session IDs against route session ID is correct but dense. Consider extracting to a local variable or adding a brief comment.
♻️ Suggested improvement
- if report.Ingests[0].Result.SessionID != route.SessionID || report.Ingests[len(report.Ingests)-1].Result.SessionID != route.SessionID { + firstIngestMatchesRoute := report.Ingests[0].Result.SessionID == route.SessionID + lastIngestMatchesRoute := report.Ingests[len(report.Ingests)-1].Result.SessionID == route.SessionID + if !firstIngestMatchesRoute || !lastIngestMatchesRoute { t.Fatalf("ingest session IDs = %#v, want route session %q", report.Ingests, route.SessionID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_bridge_extension_integration_test.go` around lines 334 - 336, The compound condition in the test comparing report.Ingests[0].Result.SessionID and report.Ingests[len(report.Ingests)-1].Result.SessionID to route.SessionID is correct but hard to read; extract the boolean into a clearly named local variable (e.g., sessionIDsMatchRoute) or split into two descriptive booleans (firstMatch, lastMatch) and use that variable in the t.Fatalf check, optionally adding a one-line comment explaining intent so the assertion reads self-documenting and easier to understand when locating report.Ingests and route.SessionID.
28-64: Consider usingt.Runsubtests for distinct test phases.The test covers multiple distinct phases (extension installation, bridge creation, first ingress, second ingress, conformance validation) that would benefit from
t.Run("Should...")grouping. This improves failure diagnostics by clearly indicating which phase failed.As per coding guidelines: "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_bridge_extension_integration_test.go` around lines 28 - 64, The test TestDaemonE2EBridgeIngressCreatesAndReusesRouteThroughTelegramExtension exercises multiple phases and should be split into t.Run subtests to improve failure diagnostics; wrap each distinct phase (extension installation/prepareDaemonTelegramReferenceExtension + registerBridgeExtensionArtifacts, bridge creation, first ingress, second ingress, and conformance validation) in descriptive t.Run("Should ...") blocks and move phase-specific setup/assertions into those closures so failures report which phase failed; use the existing helper calls (prepareDaemonTelegramReferenceExtension, registerBridgeExtensionArtifacts, e2etest.StartRuntimeHarness, harness.MockAgentRegistration) inside the appropriate t.Run bodies and keep shared setup (repoRoot, markers, env, harness start/ctx) either in an outer scope or separate t.Run setup to avoid duplication.internal/api/httpapi/transport_parity_integration_test.go (1)
305-312: Consider parsing JSON instead of string matching.The string-based type detection could produce false positives if the
wantvalue appears in other JSON fields:strings.Contains(string(event.Content), `"type":"`+want+`"`)For test reliability, consider unmarshaling the JSON and checking the
typefield directly.♻️ Proposed fix
func httpSessionEventsContainType(events []aghcontract.SessionEventPayload, want string) bool { for _, event := range events { - if strings.Contains(string(event.Content), `"type":"`+want+`"`) { + var parsed struct { + Type string `json:"type"` + } + if err := json.Unmarshal(event.Content, &parsed); err == nil && parsed.Type == want { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/transport_parity_integration_test.go` around lines 305 - 312, The helper httpSessionEventsContainType currently does string matching on event.Content which can yield false positives; change it to unmarshal each event.Content (in httpSessionEventsContainType) into a small struct or map and check the "type" field value equals want, handling/unskipping events that fail to unmarshal (log/continue) and return true only when the parsed type matches want exactly; reference event.Content and the function httpSessionEventsContainType to locate where to replace strings.Contains with JSON parsing and equality check.internal/testutil/acpmock/registration.go (1)
47-50: Wrap pass-through errors inRegisterwith local operation context.A few returns pass errors through directly, which makes root-cause tracing harder from this call site.
♻️ Proposed fix
fixture, err := LoadFixture(fixturePath) if err != nil { - return Registration{}, err + return Registration{}, fmt.Errorf("acpmock: load fixture %q: %w", fixturePath, err) } @@ agent, err := fixture.Agent(fixtureAgentName) if err != nil { - return Registration{}, err + return Registration{}, fmt.Errorf("acpmock: resolve fixture agent %q: %w", fixtureAgentName, err) } @@ driverPath, err := resolveDriverPath(opts.DriverPath) if err != nil { - return Registration{}, err + return Registration{}, fmt.Errorf("acpmock: resolve driver path: %w", err) } @@ diagnosticsPath, err := resolveDiagnosticsPath(homePaths, runtimeAgentName, opts.DiagnosticsPath) if err != nil { - return Registration{}, err + return Registration{}, fmt.Errorf("acpmock: resolve diagnostics path: %w", err) }As per coding guidelines, "Use explicit error returns with wrapped context:
fmt.Errorf(\"context: %w\", err)".Also applies to: 59-62, 69-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/acpmock/registration.go` around lines 47 - 50, The Register function returns raw errors from calls like LoadFixture and others; update each direct error return (e.g., the LoadFixture call and the returns at the other noted sites) to wrap the underlying error with local context using fmt.Errorf, e.g. replace "return Registration{}, err" with "return Registration{}, fmt.Errorf(\"Register: LoadFixture failed: %w\", err)" (and similarly name the failing operation for the other returns referenced at 59-62 and 69-77) so callers get contextualized errors; ensure fmt is imported if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/daemon/tool_mcp_resources_test.go`:
- Around line 20-79: TestResourceCatalogProjectorBuildAndApply must be
refactored to use a t.Run("Should...") subtest; wrap the existing test logic
inside a subtest named like "Should build and apply resource catalog projector"
and move t.Parallel() into that subtest (or call it both places if needed),
keeping all assertions and usage of newResourceCatalog, newToolProjector,
projector.Build, projector.Apply, catalog.Snapshot, etc. unchanged except for
indentation and scope so the test body executes inside the t.Run callback and
the top-level TestResourceCatalogProjectorBuildAndApply only defines the
subtest.
- Around line 523-538: Replace the use of map[string]any in the test by decoding
toolEncoded into a concrete struct instead of toolPayload: define a local struct
type with fields Name, Description, Source, ReadOnly and json tags "name",
"description", "source", "read_only", then call json.Unmarshal(toolEncoded,
&thatStruct) and assert against its fields (replace checks against
toolPayload["..."] with struct field comparisons). Do the same for the MCP
payload: replace mcpPayload map usage by decoding into a typed struct with
fields Name and Command and ensure the struct's json tags use the correct
lowercase field names (e.g., "name", "command") so the test reads struct.Name
and struct.Command rather than mcpPayload["Name"]/["Command"].
In `@internal/testutil/acpmock/fixture.go`:
- Around line 167-174: Fixture.Validate and Fixture.Agent disagree on name
normalization: ensure both use the same normalized key by trimming whitespace
before validation, storage, and lookup. In Fixture.Validate (where you build
seen), normalize agent.Name with strings.TrimSpace before checking
empty/duplicate and store the trimmed name in seen; in Fixture.Agent, compare
using strings.TrimSpace(agent.Name) (or trim the lookup key) so lookups for
names like " demo " succeed; apply the same trimming normalization to the other
similar block referenced around the Agent lookup (the code at the other
duplicate-check/lookup sites).
In `@internal/testutil/acpmock/registration.go`:
- Around line 64-67: The agent name from opts.AgentName is used directly as
runtimeAgentName and then as a filesystem path segment (used in path
construction around registration.go); sanitize it before use by trimming
whitespace, rejecting or normalizing dangerous segments (remove any path
separators like '/' and '\\', strip any "." or ".." segments, and allow only a
safe charset such as letters, digits, hyphen and underscore), and if the
sanitized result is empty fall back to fixtureAgentName; replace the existing
runtimeAgentName assignment and any uses (e.g., where runtimeAgentName is
interpolated into file paths) to use this sanitized value so malicious names
cannot escape intended directories.
- Around line 163-167: The current code silently falls back to creating a
relative "acpmock" directory when opts.DiagnosticsPath is empty and
homePaths.LogsDir is blank; change the logic in the diagnostics path resolver
(the code that uses opts.DiagnosticsPath and homePaths.LogsDir and builds dir
with filepath.Join) to detect an empty homePaths.LogsDir when
opts.DiagnosticsPath is not provided and return an explicit error instead of
creating a relative directory; specifically, before calling os.MkdirAll on dir,
add a guard that if opts.DiagnosticsPath == "" &&
strings.TrimSpace(homePaths.LogsDir) == "" then return a clear error indicating
diagnostics root is unavailable so callers fail fast.
In `@internal/testutil/e2e/config_seed.go`:
- Line 157: Replace the ignored file.Close() calls (e.g., "_ = file.Close()")
with explicit error handling: capture the return value (if err := file.Close();
err != nil { ... }) and either fail the test (t.Fatalf/t.Fatalf with context) or
log the error (t.Logf/processLogger.Error) with a clear justification in cleanup
paths; apply the same change to the other occurrences around lines 340-342 so no
filesystem error is silently dropped and the handling matches the surrounding
function's error reporting style.
---
Duplicate comments:
In `@internal/testutil/e2e/config_seed.go`:
- Around line 307-316: Replace the direct YAML marshal and manual framing with
the repository frontmatter helper: instead of calling yaml.Marshal(metadata) and
building the document with strings.Builder, import internal/frontmatter and use
its frontmatter serialization API to produce the frontmatter block and full
document; update the code that currently references frontmatterBytes,
yaml.Marshal, builder, defaultString(seed.Prompt, "You are "+name+".") to call
the frontmatter package (e.g., frontmatter.Marshal/Encode or the package's
provided helper to render frontmatter + body) so the metadata is serialized and
framed consistently with the rest of the repo.
---
Nitpick comments:
In `@internal/api/httpapi/prompt.go`:
- Around line 64-68: The outbound finish-event DTO promptFinishPayload still
includes the StopReason field which allows accidental reintroduction of
stopReason into "done" events; remove the StopReason field from the
promptFinishPayload struct so the wire contract only contains Type and
FinishReason, update any code that marshals or constructs promptFinishPayload
(references to promptFinishPayload, StopReason, Type, FinishReason) to stop
setting or relying on StopReason, and if other event variants need stopReason
keep them in a separate explicit DTO rather than this struct.
In `@internal/api/httpapi/transport_parity_integration_test.go`:
- Around line 305-312: The helper httpSessionEventsContainType currently does
string matching on event.Content which can yield false positives; change it to
unmarshal each event.Content (in httpSessionEventsContainType) into a small
struct or map and check the "type" field value equals want, handling/unskipping
events that fail to unmarshal (log/continue) and return true only when the
parsed type matches want exactly; reference event.Content and the function
httpSessionEventsContainType to locate where to replace strings.Contains with
JSON parsing and equality check.
In `@internal/daemon/daemon_bridge_extension_integration_test.go`:
- Around line 87-109: The test redundantly sets both Enabled: false and Status:
bridgepkg.BridgeStatusDisabled in the aghcontract.CreateBridgeRequest; update
the CreateBridge call in the test (the request passed to harness.CreateBridge)
to only set the definitive source of truth (either remove the Status field and
assert createdBridge.Bridge.Status.Normalize() equals
bridgepkg.BridgeStatusDisabled derived from Enabled, or keep Status and
remove/ignore Enabled) and then adjust the subsequent assertions around
createdBridge.Bridge.Enabled and createdBridge.Bridge.Status.Normalize() to
match that chosen approach (refer to CreateBridge request construction and the
createdBridge.Bridge.Status.Normalize() / createdBridge.Bridge.Enabled checks).
- Around line 334-336: The compound condition in the test comparing
report.Ingests[0].Result.SessionID and
report.Ingests[len(report.Ingests)-1].Result.SessionID to route.SessionID is
correct but hard to read; extract the boolean into a clearly named local
variable (e.g., sessionIDsMatchRoute) or split into two descriptive booleans
(firstMatch, lastMatch) and use that variable in the t.Fatalf check, optionally
adding a one-line comment explaining intent so the assertion reads
self-documenting and easier to understand when locating report.Ingests and
route.SessionID.
- Around line 28-64: The test
TestDaemonE2EBridgeIngressCreatesAndReusesRouteThroughTelegramExtension
exercises multiple phases and should be split into t.Run subtests to improve
failure diagnostics; wrap each distinct phase (extension
installation/prepareDaemonTelegramReferenceExtension +
registerBridgeExtensionArtifacts, bridge creation, first ingress, second
ingress, and conformance validation) in descriptive t.Run("Should ...") blocks
and move phase-specific setup/assertions into those closures so failures report
which phase failed; use the existing helper calls
(prepareDaemonTelegramReferenceExtension, registerBridgeExtensionArtifacts,
e2etest.StartRuntimeHarness, harness.MockAgentRegistration) inside the
appropriate t.Run bodies and keep shared setup (repoRoot, markers, env, harness
start/ctx) either in an outer scope or separate t.Run setup to avoid
duplication.
In `@internal/testutil/acpmock/registration.go`:
- Around line 47-50: The Register function returns raw errors from calls like
LoadFixture and others; update each direct error return (e.g., the LoadFixture
call and the returns at the other noted sites) to wrap the underlying error with
local context using fmt.Errorf, e.g. replace "return Registration{}, err" with
"return Registration{}, fmt.Errorf(\"Register: LoadFixture failed: %w\", err)"
(and similarly name the failing operation for the other returns referenced at
59-62 and 69-77) so callers get contextualized errors; ensure fmt is imported if
not already.
🪄 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: 84902364-f80f-471e-8488-fdeb5cebb392
⛔ Files ignored due to path filters (93)
.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.codex/plans/2026-04-17-e2e-confidence-hardening.mdis excluded by!**/*.md.compozy/tasks/e2e/_techspec.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-006.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-007.mdis excluded by!**/*.md.compozy/tasks/e2e/cc_analysis.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_031.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_032.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_033.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_034.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_035.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_036.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_037.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_038.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_039.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_040.mdis excluded by!**/*.md.compozy/tasks/e2e/task_02.mdis excluded by!**/*.md.compozy/tasks/settings-ui/_tasks.mdis excluded by!**/*.md.compozy/tasks/settings-ui/_techspec.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_01.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_02.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_03.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_04.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_05.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_06.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_07.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_08.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_09.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_10.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_11.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_12.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_13.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_14.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/_techspec.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-004.mdis excluded by!**/*.md.github/actions/setup-bun/action.ymlis excluded by!**/*.yml.github/actions/setup-git-cliff/action.ymlis excluded by!**/*.yml.github/actions/setup-go/action.ymlis excluded by!**/*.yml.github/actions/setup-release/action.ymlis excluded by!**/*.yml.github/workflows/ci.ymlis excluded by!**/*.yml.github/workflows/release.ymlis excluded by!**/*.yml.goreleaser.ymlis excluded by!**/*.ymlcliff.tomlis excluded by!**/*.tomlinternal/testutil/acpmock/testdata/automation_task_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/bridge_ingress_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/browser_session_lifecycle_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/driver_fault_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/multi_agent_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!**/*.jsonweb/e2e/fixtures/runtime-seed.test.tsis excluded by!**/fixtures/**web/e2e/fixtures/runtime-seed.tsis excluded by!**/fixtures/**web/e2e/fixtures/runtime.tsis excluded by!**/fixtures/**
📒 Files selected for processing (59)
.bun-version.goreleaser.release-footer.md.tmpl.goreleaser.release-header.md.tmpl.release-notes/.gitkeepinternal/acp/client.gointernal/acp/client_test.gointernal/acp/types.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/prompt.gointernal/api/httpapi/transport_parity_integration_test.gointernal/api/udsapi/transport_parity_integration_test.gointernal/daemon/daemon.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/daemon_acpmock_helpers_integration_test.gointernal/daemon/daemon_automation_task_integration_test.gointernal/daemon/daemon_bridge_extension_integration_test.gointernal/daemon/daemon_environment_sandbox_integration_test.gointernal/daemon/daemon_mock_agents_integration_test.gointernal/daemon/daemon_network_collaboration_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/network_e2e_assertions_test.gointernal/daemon/tool_mcp_resources_test.gointernal/e2elane/command_wiring_test.gointernal/e2elane/lanes.gointernal/e2elane/lanes_test.gointernal/network/delivery.gointernal/network/delivery_test.gointernal/session/interfaces.gointernal/session/manager_lifecycle.gointernal/session/manager_prompt.gointernal/session/manager_test.gointernal/session/session.gointernal/session/stop_reason.gointernal/session/stop_reason_test.gointernal/testutil/acpmock/cmd/acpmock-driver/main.gointernal/testutil/acpmock/diagnostics.gointernal/testutil/acpmock/driver_binary.gointernal/testutil/acpmock/driver_test.gointernal/testutil/acpmock/driver_testutil.gointernal/testutil/acpmock/fixture.gointernal/testutil/acpmock/fixture_test.gointernal/testutil/acpmock/registration.gointernal/testutil/e2e/bridges_extensions.gointernal/testutil/e2e/config_seed.gointernal/testutil/e2e/config_seed_test.gointernal/testutil/e2e/mock_agents.gointernal/testutil/e2e/mock_agents_test.gointernal/testutil/e2e/runtime_harness.gointernal/testutil/e2e/runtime_harness_helpers_test.gointernal/testutil/e2e/runtime_harness_integration_test.gointernal/testutil/e2e/runtime_harness_lifecycle_test.gomagefile.gomagefile_test.goweb/e2e/automation.spec.tsweb/e2e/bridges.spec.tsweb/e2e/network.spec.tsweb/e2e/session-onboarding.spec.tsweb/src/routes/_app/-automation.integration.test.tsxweb/src/systems/automation/components/automation-detail-panel.test.tsx
✅ Files skipped from review due to trivial changes (6)
- .bun-version
- .goreleaser.release-header.md.tmpl
- .goreleaser.release-footer.md.tmpl
- internal/api/httpapi/handlers_test.go
- internal/daemon/daemon_acpmock_helpers_integration_test.go
- internal/daemon/daemon_mock_agents_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/daemon/network_e2e_assertions_test.go
- internal/e2elane/lanes_test.go
- internal/e2elane/command_wiring_test.go
- internal/api/udsapi/transport_parity_integration_test.go
- internal/testutil/e2e/bridges_extensions.go
- internal/daemon/daemon_network_collaboration_integration_test.go
- internal/e2elane/lanes.go
- internal/daemon/daemon_automation_task_integration_test.go
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
internal/daemon/daemon_environment_sandbox_integration_test.go (1)
132-141: Diagnostics capture records expected values, not observed runtime data.The
diagnosticsstruct is populated with expected values for artifact capture purposes. While the test correctly verifies side effects (file content at line 107), it doesn't read back runtime-emitted diagnostics to confirm the daemon actually recorded the tool-host operation. This is acceptable since the file content verification proves the behavior, but for fuller observability validation, consider reading fromharness.SessionEnvironmentArtifactor a dedicated diagnostics endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_environment_sandbox_integration_test.go` around lines 132 - 141, The test currently constructs an expected diagnostics object (diagnostics / e2etest.ToolHostDiagnosticsArtifact with Operation "write_text_file") but does not assert that the daemon actually recorded this artifact; update the test to read and assert the runtime-emitted diagnostics (e.g., via harness.SessionEnvironmentArtifact or the diagnostics endpoint) after the operation and validate that a ToolHostOperationDiagnostic with Operation "write_text_file", Path "toolhost/allowed.txt", Outcome e2etest.ToolHostOutcomeAllowed and SideEffectExists true is present, rather than only populating the expected diagnostics struct.internal/daemon/daemon_nightly_combined_integration_test.go (1)
233-242: Same pattern: diagnostics populated with expected values for capture.Similar to the environment tests, the
diagnosticsstruct records expected values rather than observed runtime data. This is consistent across the E2E test suite and acceptable given the direct side-effect verification.Also applies to: 429-438
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_nightly_combined_integration_test.go` around lines 233 - 242, The diagnostics block is being populated with expected values rather than observed runtime data; update the creation of diagnostics (the diagnostics variable of type e2etest.ToolHostDiagnosticsArtifact and its Operations slice of e2etest.ToolHostOperationDiagnostic) to capture actual runtime results: read the real outcome, path, side-effect path and existence from the runtime/side-effect check instead of hardcoding Operation, Path, Outcome, SideEffectPath and SideEffectExists values, and assign those observed values into diagnostics so the test records measured diagnostics rather than expected constants.internal/api/udsapi/transport_parity_integration_test.go (1)
327-334: String containment for JSON type matching is fragile but acceptable for tests.The
udsSessionEventsContainTypehelper usesstrings.Contains(string(event.Content),"type":"+ want +")which could false-positive on embedded strings. Consider JSON unmarshaling for robustness (similar to the httpapi implementation), though the current approach is acceptable for test code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/udsapi/transport_parity_integration_test.go` around lines 327 - 334, The helper udsSessionEventsContainType currently checks for `"type":"<want>"` via string containment which can false-positive; update it to unmarshal each event.Content as JSON (e.g., into map[string]interface{} or a small struct with Type string) and then compare the parsed "type" field to want, returning true on match and continuing on parse errors rather than relying on substring checks; keep the function name udsSessionEventsContainType and behavior (returns bool) but replace the strings.Contains logic with JSON parsing and explicit field comparison.internal/e2elane/command_wiring_test.go (1)
1-143: Consider adding integration build tag for tests that execute external commands.These tests invoke
makeviaexec.CommandContext, which requires the repository structure and build tools to be present. While the filename doesn't follow the*_integration_test.gopattern, tests that depend on external tooling and repo layout are typically gated behind build tags to avoid CI failures in constrained environments.If these tests should run with regular unit tests, the current approach is acceptable. Otherwise, consider:
- Renaming to
command_wiring_integration_test.gowith//go:build integration- Or adding a lighter build tag like
//go:build e2elane_wiring🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/e2elane/command_wiring_test.go` around lines 1 - 143, The tests (TestMakefileE2ETargetsDelegateToLaneSpecificMageTargets, TestMakeHelpListsTheE2ELaneTargets, TestRootPackageScriptsExposeTheRepoLevelE2ELaneEntryPoints, TestWebPackageScriptsPreserveDaemonServedModeAndNightlySplit) run external commands and should be gated by a build tag to avoid running in environments without repo/tooling; either rename the file to command_wiring_integration_test.go and add a top-line build constraint like //go:build integration (and matching // +build integration for older toolchains) or keep the filename and add a specific tag such as //go:build e2elane_wiring, then update CI or docs to run that tag for integration runs so runCommand/exec.CommandContext-based tests are excluded from regular unit test runs.internal/daemon/daemon_bridge_extension_integration_test.go (1)
28-353: Consider addingt.Parallel()if the test can run concurrently.This comprehensive bridge ingress E2E test validates the full Telegram extension lifecycle but lacks
t.Parallel(). Given it builds the extension binary and runs a daemon, this may be intentional due to resource constraints or test isolation requirements.If concurrent execution is feasible, adding
t.Parallel()would improve test suite throughput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/daemon_bridge_extension_integration_test.go` around lines 28 - 353, The test TestDaemonE2EBridgeIngressCreatesAndReusesRouteThroughTelegramExtension lacks t.Parallel(); make it parallel by inserting a call to t.Parallel() at the start of the test (immediately after the test function begins, e.g. as the first executable statement or right after acpmock.RequireDriver(t)), and verify any shared/global resources used by this test (harness setup, marker paths, env vars, or any driver mocks) are isolated per-test so parallel execution is safe.internal/acp/client_test.go (1)
297-344: Wrap this case in at.Run("Should...")subtest.This is the only newly added test here that skips the repo’s required test-case pattern, which makes the file inconsistent with the test contract.
As per coding guidelines,
**/*_test.go: "MUST use t.Run("Should...") pattern for ALL test cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/acp/client_test.go` around lines 297 - 344, The test function TestPromptTransmitsStructuredMetadata breaks the repo's required pattern by not using a t.Run subtest; wrap the existing assertions and setup inside a t.Run("Should transmit structured metadata") (or similar "Should..." description) subtest block so the function uses the t.Run("Should...") pattern; keep the same setup (New, startHelperProcess, driver.Prompt call), event collection, and assertions (json.Unmarshal checks, payload.TurnSource and payload.Network.MessageID) but move them into the closure passed to t.Run and ensure deferred stopProcess remains valid for the subtest.internal/session/manager_prompt.go (1)
31-45: Avoid silently dropping extra metadata values.
PromptNetworkaccepts...acp.PromptNetworkMeta, but onlymeta[0]is ever used. If a caller passes more than one value, the extras compile cleanly and disappear. Prefer a single metadata parameter, or rejectlen(meta) > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_prompt.go` around lines 31 - 45, PromptNetwork currently accepts a variadic ...acp.PromptNetworkMeta but silently ignores any extras (only meta[0] is used); update Manager.PromptNetwork to detect multiple metadata values and return an error when len(meta) > 1 (or change the signature to accept a single acp.PromptNetworkMeta if you prefer), then continue using meta[0] to populate promptMeta.Network and call PromptWithOpts with PromptOpts (Message, TurnSource: TurnSourceNetwork, PromptMeta). Ensure the error message clearly states that multiple PromptNetworkMeta values are not supported and reference Manager.PromptNetwork, PromptWithOpts, PromptOpts, PromptMeta, and acp.PromptNetworkMeta in the fix.internal/testutil/acpmock/fixture_test.go (1)
18-19: Use the repository’sShould...subtest naming convention.The coverage looks good, but these new
t.Run(...)cases should follow the requiredShould...pattern for consistency with the rest of the suite.As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/acpmock/fixture_test.go` around lines 18 - 19, The test subcase name in fixture_test.go uses "maps named agents and bridge responses" which violates the repository's "Should..." naming convention; rename the t.Run invocation for the subtest (the one currently labelled "maps named agents and bridge responses") to a "Should..." form such as "Should map named agents and bridge responses" (or similar grammatically consistent "Should..." phrasing) so it follows the suite convention used across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/daemon/tool_mcp_resources_test.go`:
- Around line 545-551: The test currently only checks that
validateAndEncodeTool(...) returned a non-nil error, which is too broad; update
the assertions to check for the specific validation sentinel/type or stable
message fragment (using errors.Is/errors.As or an "ErrorContains" style check)
so the failure is asserted for the intended validation reason. Concretely,
change the assertion around validateAndEncodeTool(...) for the invalid Name case
(and the similar case at lines 591-594) to verify the returned error matches the
expected validation error value or contains the expected message fragment rather
than simply being non-nil, referencing validateAndEncodeTool and the test cases
that pass toolspkg.Tool with an invalid Name/Source.
- Around line 248-290: The test function
TestToolMCPSourceSyncerHandlesNilReceiverAndTriggerFailures must be converted to
use a named subtest; wrap the existing test body inside t.Run("Should handle nil
receiver and trigger failures", func(t *testing.T) { ... }) so the scenario is
isolated and follows the required t.Run("Should...") pattern; do the same
pattern for the other top-level tests noted (the functions covering lines
292-316, 318-384, 386-412, 414-455, 457-500, 502-595) by wrapping each distinct
scenario in a t.Run call with a "Should..." descriptive name, keeping the
existing setup (e.g., nilSyncer, newToolMCPSourceSyncer, the trigger failure
closure, and assertions on syncer.Sync) inside their respective subtest
closures.
In `@internal/testutil/acpmock/cmd/acpmock-driver/main.go`:
- Around line 448-457: The async branch detaches driver-control goroutines from
the agent lifetime; change it to register each async worker with the agent's
lifecycle (e.g., a shared sync.WaitGroup or a per-agent worker tracker) and use
a context derived from the agent/connection context so cancellation cancels
pending delays; specifically, when control.Async is true, create a child context
tied to the agent lifetime (not context.WithoutCancel), increment the agent's
WaitGroup/worker tracker before spawning the goroutine, run
waitDriverControlDelay and performDriverControl inside that goroutine, and
ensure you call Done()/unregister on exit so the agent can cancel and Wait for
all async driver-control goroutines during teardown. Ensure references to
waitDriverControlDelay, performDriverControl, and DriverControlStep are used to
locate and update the code.
- Around line 882-891: The pauseForDelivery function currently uses a fixed 5ms
timer which makes tests timing-dependent; replace this with a real
synchronization point by waiting for a delivery-complete signal instead of
sleeping. Modify pauseForDelivery to accept or access a delivery notification
(e.g., a chan struct{} or sync.Cond) from the driver harness and select on
ctx.Done() and that notification channel (or use a condition variable with a
mutex), and ensure the code that completes a delivery closes/signals that
channel (or broadcasts the cond) so pauseForDelivery reliably returns when
delivery state is reached; update all call sites to provide the notification
channel if needed.
In `@internal/testutil/acpmock/driver_binary.go`:
- Around line 23-64: The cache check and build in DefaultDriverPath must be
serialized to avoid concurrent go build runs: move the driverBinaryMu.Lock() to
cover the entire check-and-build sequence (lock before reading driverBinaryPath
and hold the lock until after driverBinaryPath is set) so only one goroutine
builds while others block and then read the cached path; if the build fails,
ensure you unlock and leave driverBinaryPath empty so subsequent callers can
retry (do not use sync.Once because it prevents retries on failure). Use the
existing symbols DefaultDriverPath, driverBinaryMu, and driverBinaryPath to
locate and implement the change.
- Around line 42-52: Replace the unbounded context.Background() passed to
exec.CommandContext with a timeout-bound context using context.WithTimeout
(e.g., 30–60s) so the go build subprocess cannot hang indefinitely; create ctx,
cancel := context.WithTimeout(context.Background(), 30*time.Second) and defer
cancel(), then call exec.CommandContext(ctx, "go", "build", "-o", outputPath,
"./internal/testutil/acpmock/cmd/acpmock-driver"); update imports if necessary
to include time and context and keep using the existing cmd variable and
repoRoot handling unchanged.
In `@internal/testutil/acpmock/driver_test.go`:
- Around line 316-362: Update the test
TestDriverControlBlockUntilCancelReturnsCanceledStopReason to assert the stop
reason on the emitted error event explicitly: after collecting events with
collectPromptEvents/normalizeEvents, iterate the normalized events to find the
event with "type" == acp.EventTypeError and assert its "stop_reason" field
equals the canceled stop reason constant (e.g. acp.StopReasonCanceled); fail the
test with a clear message if no error event has that stop_reason. This ensures
the test checks the specific stop_reason behavior rather than only the presence
of an error event.
In `@internal/testutil/acpmock/fixture_test.go`:
- Around line 360-369: The subtest
TestRegistrationHelperOverridesAndDiagnosticsErrors mutates the process env
(driverBinaryEnvVar) which races with TestValidationAndDriverHelpers when tests
run in parallel; fix by isolating the env mutation: extract the env-dependent
checks into their own top-level test (e.g.,
TestResolveDriverPathHonorsOverrides) that calls t.Setenv(driverBinaryEnvVar,
...) and does NOT call t.Parallel(), or alternatively ensure both tests use
t.Setenv in their own top-level tests and avoid t.Parallel() for any test that
relies on resolveDriverPath("") reading the default/override; reference
resolveDriverPath and driverBinaryEnvVar when making the change.
---
Nitpick comments:
In `@internal/acp/client_test.go`:
- Around line 297-344: The test function TestPromptTransmitsStructuredMetadata
breaks the repo's required pattern by not using a t.Run subtest; wrap the
existing assertions and setup inside a t.Run("Should transmit structured
metadata") (or similar "Should..." description) subtest block so the function
uses the t.Run("Should...") pattern; keep the same setup (New,
startHelperProcess, driver.Prompt call), event collection, and assertions
(json.Unmarshal checks, payload.TurnSource and payload.Network.MessageID) but
move them into the closure passed to t.Run and ensure deferred stopProcess
remains valid for the subtest.
In `@internal/api/udsapi/transport_parity_integration_test.go`:
- Around line 327-334: The helper udsSessionEventsContainType currently checks
for `"type":"<want>"` via string containment which can false-positive; update it
to unmarshal each event.Content as JSON (e.g., into map[string]interface{} or a
small struct with Type string) and then compare the parsed "type" field to want,
returning true on match and continuing on parse errors rather than relying on
substring checks; keep the function name udsSessionEventsContainType and
behavior (returns bool) but replace the strings.Contains logic with JSON parsing
and explicit field comparison.
In `@internal/daemon/daemon_bridge_extension_integration_test.go`:
- Around line 28-353: The test
TestDaemonE2EBridgeIngressCreatesAndReusesRouteThroughTelegramExtension lacks
t.Parallel(); make it parallel by inserting a call to t.Parallel() at the start
of the test (immediately after the test function begins, e.g. as the first
executable statement or right after acpmock.RequireDriver(t)), and verify any
shared/global resources used by this test (harness setup, marker paths, env
vars, or any driver mocks) are isolated per-test so parallel execution is safe.
In `@internal/daemon/daemon_environment_sandbox_integration_test.go`:
- Around line 132-141: The test currently constructs an expected diagnostics
object (diagnostics / e2etest.ToolHostDiagnosticsArtifact with Operation
"write_text_file") but does not assert that the daemon actually recorded this
artifact; update the test to read and assert the runtime-emitted diagnostics
(e.g., via harness.SessionEnvironmentArtifact or the diagnostics endpoint) after
the operation and validate that a ToolHostOperationDiagnostic with Operation
"write_text_file", Path "toolhost/allowed.txt", Outcome
e2etest.ToolHostOutcomeAllowed and SideEffectExists true is present, rather than
only populating the expected diagnostics struct.
In `@internal/daemon/daemon_nightly_combined_integration_test.go`:
- Around line 233-242: The diagnostics block is being populated with expected
values rather than observed runtime data; update the creation of diagnostics
(the diagnostics variable of type e2etest.ToolHostDiagnosticsArtifact and its
Operations slice of e2etest.ToolHostOperationDiagnostic) to capture actual
runtime results: read the real outcome, path, side-effect path and existence
from the runtime/side-effect check instead of hardcoding Operation, Path,
Outcome, SideEffectPath and SideEffectExists values, and assign those observed
values into diagnostics so the test records measured diagnostics rather than
expected constants.
In `@internal/e2elane/command_wiring_test.go`:
- Around line 1-143: The tests
(TestMakefileE2ETargetsDelegateToLaneSpecificMageTargets,
TestMakeHelpListsTheE2ELaneTargets,
TestRootPackageScriptsExposeTheRepoLevelE2ELaneEntryPoints,
TestWebPackageScriptsPreserveDaemonServedModeAndNightlySplit) run external
commands and should be gated by a build tag to avoid running in environments
without repo/tooling; either rename the file to
command_wiring_integration_test.go and add a top-line build constraint like
//go:build integration (and matching // +build integration for older toolchains)
or keep the filename and add a specific tag such as //go:build e2elane_wiring,
then update CI or docs to run that tag for integration runs so
runCommand/exec.CommandContext-based tests are excluded from regular unit test
runs.
In `@internal/session/manager_prompt.go`:
- Around line 31-45: PromptNetwork currently accepts a variadic
...acp.PromptNetworkMeta but silently ignores any extras (only meta[0] is used);
update Manager.PromptNetwork to detect multiple metadata values and return an
error when len(meta) > 1 (or change the signature to accept a single
acp.PromptNetworkMeta if you prefer), then continue using meta[0] to populate
promptMeta.Network and call PromptWithOpts with PromptOpts (Message, TurnSource:
TurnSourceNetwork, PromptMeta). Ensure the error message clearly states that
multiple PromptNetworkMeta values are not supported and reference
Manager.PromptNetwork, PromptWithOpts, PromptOpts, PromptMeta, and
acp.PromptNetworkMeta in the fix.
In `@internal/testutil/acpmock/fixture_test.go`:
- Around line 18-19: The test subcase name in fixture_test.go uses "maps named
agents and bridge responses" which violates the repository's "Should..." naming
convention; rename the t.Run invocation for the subtest (the one currently
labelled "maps named agents and bridge responses") to a "Should..." form such as
"Should map named agents and bridge responses" (or similar grammatically
consistent "Should..." phrasing) so it follows the suite convention used across
tests.
🪄 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: 70b5656c-22ff-4dc3-a038-581d09f99a08
⛔ Files ignored due to path filters (105)
.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.codex/plans/2026-04-17-e2e-confidence-hardening.mdis excluded by!**/*.md.compozy/tasks/e2e/_techspec.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-006.mdis excluded by!**/*.md.compozy/tasks/e2e/adrs/adr-007.mdis excluded by!**/*.md.compozy/tasks/e2e/cc_analysis.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_031.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_032.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_033.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_034.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_035.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_036.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_037.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_038.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_039.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-001/issue_040.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/e2e/task_02.mdis excluded by!**/*.md.compozy/tasks/settings-ui/_tasks.mdis excluded by!**/*.md.compozy/tasks/settings-ui/_techspec.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/settings-ui/adrs/adr-004.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_01.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_02.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_03.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_04.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_05.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_06.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_07.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_08.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_09.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_10.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_11.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_12.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_13.mdis excluded by!**/*.md.compozy/tasks/settings-ui/task_14.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/_techspec.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-001.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-002.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-003.mdis excluded by!**/*.md.compozy/tasks/tasks-ui/adrs/adr-004.mdis excluded by!**/*.md.github/actions/setup-bun/action.ymlis excluded by!**/*.yml.github/actions/setup-git-cliff/action.ymlis excluded by!**/*.yml.github/actions/setup-go/action.ymlis excluded by!**/*.yml.github/actions/setup-release/action.ymlis excluded by!**/*.yml.github/workflows/ci.ymlis excluded by!**/*.yml.github/workflows/release.ymlis excluded by!**/*.yml.goreleaser.ymlis excluded by!**/*.ymlbun.lockis excluded by!**/*.lockcliff.tomlis excluded by!**/*.tomlinternal/testutil/acpmock/testdata/automation_task_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/bridge_ingress_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/browser_session_lifecycle_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/driver_fault_fixture.jsonis excluded by!**/*.jsoninternal/testutil/acpmock/testdata/multi_agent_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!**/*.jsoninternal/testutil/acpmock/testdata/tool_permission_golden.jsonis excluded by!**/*.json
📒 Files selected for processing (61)
.bun-version.goreleaser.release-footer.md.tmpl.goreleaser.release-header.md.tmpl.release-notes/.gitkeepMakefileinternal/acp/client.gointernal/acp/client_test.gointernal/acp/types.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/prompt.gointernal/api/httpapi/transport_parity_integration_test.gointernal/api/udsapi/transport_parity_integration_test.gointernal/daemon/automation_resources_test.gointernal/daemon/automation_task_e2e_assertions_test.gointernal/daemon/bridge_extension_e2e_assertions_test.gointernal/daemon/daemon.gointernal/daemon/daemon_acpmock_faults_integration_test.gointernal/daemon/daemon_acpmock_helpers_integration_test.gointernal/daemon/daemon_automation_task_integration_test.gointernal/daemon/daemon_bridge_extension_integration_test.gointernal/daemon/daemon_environment_sandbox_integration_test.gointernal/daemon/daemon_mock_agents_integration_test.gointernal/daemon/daemon_network_collaboration_integration_test.gointernal/daemon/daemon_nightly_combined_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/network_e2e_assertions_test.gointernal/daemon/tool_mcp_resources_test.gointernal/e2elane/command_wiring_test.gointernal/e2elane/lanes.gointernal/e2elane/lanes_test.gointernal/extensiontest/bridge_adapter_harness.gointernal/extensiontest/bridge_adapter_harness_test.gointernal/network/delivery.gointernal/network/delivery_test.gointernal/session/interfaces.gointernal/session/manager_lifecycle.gointernal/session/manager_prompt.gointernal/session/manager_test.gointernal/session/session.gointernal/session/stop_reason.gointernal/session/stop_reason_test.gointernal/testutil/acpmock/cmd/acpmock-driver/main.gointernal/testutil/acpmock/diagnostics.gointernal/testutil/acpmock/driver_binary.gointernal/testutil/acpmock/driver_test.gointernal/testutil/acpmock/driver_testutil.gointernal/testutil/acpmock/fixture.gointernal/testutil/acpmock/fixture_test.gointernal/testutil/acpmock/registration.gointernal/testutil/e2e/artifacts.gointernal/testutil/e2e/artifacts_test.gointernal/testutil/e2e/automation_tasks.gointernal/testutil/e2e/automation_tasks_test.gointernal/testutil/e2e/bridges_extensions.gointernal/testutil/e2e/config_seed.gointernal/testutil/e2e/config_seed_test.gointernal/testutil/e2e/mock_agents.gointernal/testutil/e2e/mock_agents_test.gointernal/testutil/e2e/runtime_harness.gointernal/testutil/e2e/runtime_harness_helpers_test.go
✅ Files skipped from review due to trivial changes (8)
- .bun-version
- internal/testutil/acpmock/driver_testutil.go
- .goreleaser.release-header.md.tmpl
- .goreleaser.release-footer.md.tmpl
- internal/daemon/daemon_test.go
- Makefile
- internal/api/httpapi/handlers_test.go
- internal/testutil/acpmock/diagnostics.go
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/api/httpapi/httpapi_integration_test.go
- internal/daemon/daemon_acpmock_helpers_integration_test.go
- internal/session/stop_reason_test.go
- internal/session/manager_test.go
- internal/network/delivery.go
- internal/daemon/automation_task_e2e_assertions_test.go
- internal/daemon/network_e2e_assertions_test.go
- internal/network/delivery_test.go
- internal/testutil/e2e/artifacts_test.go
- internal/acp/types.go
- internal/daemon/daemon_mock_agents_integration_test.go
- internal/session/stop_reason.go
- internal/e2elane/lanes.go
- internal/testutil/acpmock/registration.go
- internal/daemon/daemon_automation_task_integration_test.go
- internal/testutil/acpmock/fixture.go
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/testutil/acpmock/driver_test.go (1)
356-362:⚠️ Potential issue | 🟡 MinorAssert the
stop_reasonfield explicitly to match test intent.The test is named
ReturnsCanceledStopReasonbut only verifies that an error event exists, not that it has the expectedstop_reason. This could miss regressions in stop reason emission.🔧 Proposed fix
time.AfterFunc(100*time.Millisecond, cancel) events := collectPromptEvents(t, eventsCh, nil) - if !containsNormalizedEvent(normalizeEvents(events), map[string]string{ - "type": acp.EventTypeError, - }) { - t.Fatalf("events = %#v, want error event after prompt cancellation", events) + normalized := normalizeEvents(events) + if !containsNormalizedEvent(normalized, map[string]string{ + "type": acp.EventTypeError, + "stop_reason": acp.StopReasonCanceled, + }) { + t.Fatalf("events = %#v, want error event with canceled stop_reason", normalized) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/testutil/acpmock/driver_test.go` around lines 356 - 362, The test currently only checks for an error event but must also assert the stop_reason equals the canceled stop reason; after collecting events via collectPromptEvents(t, eventsCh, nil) and normalizing with normalizeEvents(events), add/replace the containsNormalizedEvent check to look for a map including "type": acp.EventTypeError and "stop_reason": acp.StopReasonCanceled (or the exact constant/string used for canceled in the acp package) so the ReturnsCanceledStopReason test explicitly verifies the stop reason emitted.
🧹 Nitpick comments (2)
internal/session/manager_prompt.go (2)
147-166: Validation logic is well-structured with proper layering.The function correctly:
- Normalizes metadata and defaults
TurnSourcewhen empty- Validates consistency between request turn source and metadata turn source
- Provides session-specific error messages before delegating to
acp.Validate()One minor note: Line 163 returns the
Validate()error without wrapping. Per coding guidelines, errors should be wrapped with context. The error already has an "acp:" prefix, but wrapping would clarify the session-layer origin.♻️ Optional: wrap error with session context
if err := normalized.Validate(); err != nil { - return acp.PromptMeta{}, err + return acp.PromptMeta{}, fmt.Errorf("session: normalize prompt meta: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_prompt.go` around lines 147 - 166, normalizePromptMeta currently returns the error from normalized.Validate() directly; wrap that error with session context to indicate its origin in this layer. Update the normalized.Validate() error return in normalizePromptMeta to wrap the error (e.g., using fmt.Errorf or errors.Wrap) with a prefixed message like "session: prompt meta validation failed" so callers can distinguish session-layer failures from acp errors while preserving the original error via %w.
31-50: Consider using a pointer parameter instead of variadic for optional metadata.The variadic approach for an optional single value requires defensive length checking and is non-idiomatic Go. Refactoring to
meta *acp.PromptNetworkMetawould simplify the API, though this requires updating the interface definitions indeliveryPrompterandnetworkBindableSessionManager, all implementations, test fakes, and removing the multiple-metadata validation test case (lines 943–955 in manager_test.go).♻️ Proposed signature change
func (m *Manager) PromptNetwork( ctx context.Context, id string, msg string, - meta ...acp.PromptNetworkMeta, + meta *acp.PromptNetworkMeta, ) (<-chan acp.AgentEvent, error) { - if len(meta) > 1 { - return nil, errors.New("session: network prompt accepts at most one metadata value") - } - var promptMeta acp.PromptMeta - if len(meta) > 0 { - promptMeta.Network = &meta[0] + if meta != nil { + promptMeta.Network = meta } return m.PromptWithOpts(ctx, id, PromptOpts{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_prompt.go` around lines 31 - 50, The PromptNetwork method uses a variadic meta parameter for a single optional value which is non-idiomatic and forces defensive length checks; change PromptNetwork(ctx context.Context, id string, msg string, meta ...acp.PromptNetworkMeta) to accept a pointer meta *acp.PromptNetworkMeta, remove the len(meta) checks and the creation of promptMeta.Network from the slice, and instead set promptMeta.Network = meta directly; update all related interface signatures (deliveryPrompter, networkBindableSessionManager), all implementations and test fakes to accept *acp.PromptNetworkMeta, update calls to pass a pointer or nil, and remove the test case that asserts multiple-metadata validation in manager_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/udsapi/transport_parity_integration_test.go`:
- Around line 82-84: The test currently discards the error returned by
resp.Body.Close(); change the code that calls resp.Body.Close() (near the
variables body, readErr and resp) to capture and handle the error instead of
using `_ = resp.Body.Close()`: call err := resp.Body.Close() and if err != nil
report the failure (e.g., t.Fatalf or t.Errorf with a clear message including
the error) so any transport/IO close failures are surfaced in the test.
- Around line 34-199: Replace the three standalone tests
(TestUDSTransportApprovalRouteDocumentsNotImplementedGap,
TestUDSTransportProjectionParityMatchesHTTPAndCLI,
TestUDSTransportPromptFailureProjectionUsesSharedRuntimeHarness) with a single
test that uses t.Run subtests (or a table-driven loop calling t.Run) named like
t.Run("Should ...") for each scenario; move each test body into its own subtest,
keep unique setup calls (e.g., acpmock.RequireDriver,
e2etest.StartRuntimeHarness, runtimeHarness.CreateSession,
seedTransportWebhookTrigger) inside the corresponding subtest, and call
t.Parallel() inside each subtest if parallelism is desired, ensuring you
preserve the original function/variable names (transportUDSApprovalAgent,
transportUDSAutomationAgent, transportUDSFaultyAgent, mustUnixRequest,
waitForHTTPAutomationRun, waitForUDSAutomationRun, waitForCLIAutomationRun,
e2etest.ValidateWebhookRunProjection, etc.) so the test logic and assertions
remain identical but conform to the repo's t.Run("Should...") pattern.
In `@internal/daemon/tool_mcp_resources_test.go`:
- Around line 546-558: Replace string-based error assertions with sentinel
checks: in the validateAndEncodeTool test remove the
strings.Contains(err.Error(), "tool.name is required") assertion and keep/ensure
the errors.Is(err, resources.ErrValidation) check; in the MCP test add an
errors.Is(err, resources.ErrValidation) assertion before any contextual checks
and remove any strings.Contains comparisons that check for specific message text
(e.g., "mcp_server.command is required"); if you need to assert context use
errors.As to extract the wrapped error type instead of comparing error strings,
referencing validateAndEncodeTool, validateAndEncodeMCPServer, and
resources.ErrValidation to locate the assertions to change.
---
Duplicate comments:
In `@internal/testutil/acpmock/driver_test.go`:
- Around line 356-362: The test currently only checks for an error event but
must also assert the stop_reason equals the canceled stop reason; after
collecting events via collectPromptEvents(t, eventsCh, nil) and normalizing with
normalizeEvents(events), add/replace the containsNormalizedEvent check to look
for a map including "type": acp.EventTypeError and "stop_reason":
acp.StopReasonCanceled (or the exact constant/string used for canceled in the
acp package) so the ReturnsCanceledStopReason test explicitly verifies the stop
reason emitted.
---
Nitpick comments:
In `@internal/session/manager_prompt.go`:
- Around line 147-166: normalizePromptMeta currently returns the error from
normalized.Validate() directly; wrap that error with session context to indicate
its origin in this layer. Update the normalized.Validate() error return in
normalizePromptMeta to wrap the error (e.g., using fmt.Errorf or errors.Wrap)
with a prefixed message like "session: prompt meta validation failed" so callers
can distinguish session-layer failures from acp errors while preserving the
original error via %w.
- Around line 31-50: The PromptNetwork method uses a variadic meta parameter for
a single optional value which is non-idiomatic and forces defensive length
checks; change PromptNetwork(ctx context.Context, id string, msg string, meta
...acp.PromptNetworkMeta) to accept a pointer meta *acp.PromptNetworkMeta,
remove the len(meta) checks and the creation of promptMeta.Network from the
slice, and instead set promptMeta.Network = meta directly; update all related
interface signatures (deliveryPrompter, networkBindableSessionManager), all
implementations and test fakes to accept *acp.PromptNetworkMeta, update calls to
pass a pointer or nil, and remove the test case that asserts multiple-metadata
validation in manager_test.go.
🪄 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: 1585ac8a-372c-4139-a712-215cce517759
⛔ Files ignored due to path filters (17)
.compozy/tasks/e2e/reviews-003/_meta.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_001.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_002.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_003.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_004.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_005.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_006.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_007.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_008.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_009.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_010.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_011.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_012.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_013.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_014.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_015.mdis excluded by!**/*.md.compozy/tasks/e2e/reviews-003/issue_016.mdis excluded by!**/*.md
📒 Files selected for processing (8)
internal/api/udsapi/transport_parity_integration_test.gointernal/daemon/tool_mcp_resources_test.gointernal/session/manager_prompt.gointernal/session/manager_test.gointernal/testutil/acpmock/cmd/acpmock-driver/main.gointernal/testutil/acpmock/driver_binary.gointernal/testutil/acpmock/driver_test.gointernal/testutil/acpmock/fixture_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/testutil/acpmock/cmd/acpmock-driver/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/session/manager_test.go
This was referenced Apr 17, 2026
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
Tests
Bug Fixes