Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .compozy/tasks/qa-rounds/reviews-001/_meta.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
---
provider: coderabbit
pr: "78"
pr: "80"
round: 1
created_at: 2026-04-26T20:29:02.311014Z
created_at: 2026-04-27T00:34:22.802061Z
---

## Summary
- Total: 33
- Total: 13
- Resolved: 0
- Unresolved: 33
- Unresolved: 13
25 changes: 14 additions & 11 deletions .compozy/tasks/qa-rounds/reviews-001/issue_001.md
Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
---
status: resolved
file: internal/api/core/agent_identity.go
line: 148
file: web/src/components/app-sidebar.test.tsx
line: 263
severity: nitpick
author: coderabbitai[bot]
provider_ref: review:4177411700,nitpick_hash:ee846dd868b1
review_hash: ee846dd868b1
source_review_id: "4177411700"
source_review_submitted_at: "2026-04-26T20:24:27Z"
provider_ref: review:4177569108,nitpick_hash:ad1c523077c7
review_hash: ad1c523077c7
source_review_id: "4177569108"
source_review_submitted_at: "2026-04-26T22:35:58Z"
---

# Issue 001: Consider wrapping the error with context.
# Issue 001: Make this route mock param-aware.
## Review Comment

Line 158 returns the error from `ResolveCoordinatorConfig` without wrapping. Adding context would help trace failures.

As per coding guidelines: "Use explicit error returns with wrapped context: `fmt.Errorf("context: %w", err)` in Go".
The current stub keys only on `to`, so setting `matchedRouteFuzzy["/agents/$name"] = true` would mark every agent row active. That makes this assertion non-discriminating for the `name` param handling you want to cover.

## Triage

- Decision: `VALID`
- Notes: `agentCoordinatorConfigPayload` returns the raw `ResolveCoordinatorConfig` error. This violates the local wrapped-error convention and loses call-site context. Fix by wrapping the resolver failure with `fmt.Errorf("resolve coordinator config: %w", err)`.
- Notes:
- The `useMatchRoute` test stub only keys route activity by `to`, while `AppSidebar` calls `matchRoute({ to: "/agents/$name", params: { name: agent.name }, fuzzy: true })`.
- Setting a fuzzy match for `"/agents/$name"` would therefore mark every agent row active and fail to prove that the `name` param is respected.
- Fix by making the test mock key on `to + params` and updating the active-row assertion to render multiple agents, with only the matching param active.
- Resolution: implemented param-aware route match keys and asserted that only the matching agent row is active.
- Verification: targeted Vitest passed; `make verify` passed.
58 changes: 39 additions & 19 deletions .compozy/tasks/qa-rounds/reviews-001/issue_002.md
Original file line number Diff line number Diff line change
@@ -1,39 +1,54 @@
---
status: resolved
file: internal/api/udsapi/agent_channels_test.go
line: 116
file: web/src/routes/_app/agents.$name.sessions.$id.tsx
line: 136
author: coderabbitai[bot]
provider_ref: thread:PRRT_kwDOR5y4QM59r7vG,comment:PRRC_kwDOR5y4QM67Z0NA
provider_ref: thread:PRRT_kwDOR5y4QM59sdE9,comment:PRRC_kwDOR5y4QM67ae48
---

# Issue 002: _⚠️ Potential issue_ | _🟠 Major_
# Issue 002: _⚠️ Potential issue_ | _🟡 Minor_
## Review Comment

_⚠️ Potential issue_ | _🟠 Major_
_⚠️ Potential issue_ | _🟡 Minor_

**Wrap this test case in `t.Run("Should ...")` to match required test structure.**
**Use canonical agent name for post-delete navigation.**

This new test is currently a direct top-level body without the required subtest naming pattern.
`onDeleteSuccess` currently routes with the URL param `name`, which can be stale if the path is manually edited or mismatched. Use the resolved session agent name for consistent redirect behavior.

<details>
<summary>🐛 Suggested fix</summary>

As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
```diff
const workspaceName = workspaces?.find(workspace => workspace.id === session.workspace_id)?.name;
+ const resolvedAgentName = session.agent_name ?? name;
@@
<SessionPageContent
- agentName={session.agent_name ?? name}
+ agentName={resolvedAgentName}
sessionId={id}
session={session}
workspaceName={workspaceName}
onDeleteSuccess={() => {
- void navigate({ to: "/agents/$name", params: { name } });
+ void navigate({ to: "/agents/$name", params: { name: resolvedAgentName } });
}}
/>
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In `@internal/api/udsapi/agent_channels_test.go` around lines 72 - 116, Wrap the
existing TestAgentCoordinatorConfigRouteReturnsResolvedPayload body in a subtest
using t.Run("Should return resolved workspace coordinator payload", func(t
*testing.T) { ... }) so the test follows the required t.Run("Should ...")
pattern; locate the TestAgentCoordinatorConfigRouteReturnsResolvedPayload
function and move its current contents into a t.Run call while keeping all setup
(manager := activeAgentSessionManager, handlers := newTestHandlers,
handlers.CoordinatorConfig = agentCoordinatorConfigResolverFunc, engine :=
newTestRouter, performAgentKernelRequest, decodeJSONResponse and assertions)
unchanged inside the subtest body.
In `@web/src/routes/_app/agents`.$name.sessions.$id.tsx around lines 125 - 136,
The onDeleteSuccess handler uses the possibly-stale route param name when
navigating after deletion; change it to use the resolved agent name from the
session (e.g., session.agent_name ?? name) so the redirect is canonical. Update
the onDeleteSuccess in SessionPageContent's props to call navigate({ to:
"/agents/$name", params: { name: session.agent_name ?? name } }) (or compute a
resolvedAgentName variable) to ensure consistent post-delete routing.
```

</details>
Expand All @@ -45,4 +60,9 @@ unchanged inside the subtest body.
## Triage

- Decision: `VALID`
- Notes: `TestAgentCoordinatorConfigRouteReturnsResolvedPayload` contains one direct top-level test body while this repo requires each case to run under a `t.Run("Should ...")` subtest. Fix by moving the existing setup, request, and assertions into a named `Should return resolved workspace coordinator payload` subtest.
- Notes:
- The route passes `session.agent_name ?? name` to the page content but still navigates after delete with the raw URL param `name`.
- If the URL param is stale or manually edited, delete success redirects to a non-canonical agent route even though the resolved session payload contains the correct agent name.
- Fix by computing a single resolved agent name from the session and using it for both rendering and post-delete navigation. Add a route test that exercises delete success with a mismatched URL param.
- Resolution: reused `resolvedAgentName` for both content and delete-success navigation, with a regression test covering a mismatched route param.
- Verification: targeted Vitest passed; `make verify` passed.
97 changes: 15 additions & 82 deletions .compozy/tasks/qa-rounds/reviews-001/issue_003.md
Original file line number Diff line number Diff line change
@@ -1,93 +1,26 @@
---
status: resolved
file: internal/automation/dispatch.go
line: 60
file: web/src/routes/_app/agents.$name.tsx
line: 18
severity: nitpick
author: coderabbitai[bot]
provider_ref: thread:PRRT_kwDOR5y4QM59r7vH,comment:PRRC_kwDOR5y4QM67Z0NB
provider_ref: review:4177569108,nitpick_hash:803039727d80
review_hash: 803039727d80
source_review_id: "4177569108"
source_review_submitted_at: "2026-04-26T22:35:58Z"
---

# Issue 003: _🛠️ Refactor suggestion_ | _🟠 Major_
# Issue 003: This still fetches agent detail data on nested session routes.
## Review Comment

_🛠️ Refactor suggestion_ | _🟠 Major_

**Make session stop timeout configurable, not hardcoded.**

The new 10s value is operational policy in a core runtime path; it should be injected via dispatcher options (or TOML-backed config), not fixed in code.


<details>
<summary>Proposed refactor</summary>

```diff
-const dispatcherSessionStopTimeout = 10 * time.Second
+const defaultDispatcherSessionStopTimeout = 10 * time.Second

type Dispatcher struct {
sessions SessionCreator
runs RunStore
tasks TaskService
+ sessionStopTimeout time.Duration
...
}

func NewDispatcher(sessions SessionCreator, runs RunStore, opts ...DispatcherOption) (*Dispatcher, error) {
...
dispatcher := &Dispatcher{
sessions: sessions,
runs: runs,
logger: slog.Default(),
now: func() time.Time { return time.Now().UTC() },
sleep: sleepWithContext,
maxConcurrent: DefaultMaxConcurrentJobs,
+ sessionStopTimeout: defaultDispatcherSessionStopTimeout,
}
...
}

+func WithDispatcherSessionStopTimeout(timeout time.Duration) DispatcherOption {
+ return func(dispatcher *Dispatcher) {
+ if timeout > 0 {
+ dispatcher.sessionStopTimeout = timeout
+ }
+ }
+}

func (d *Dispatcher) stopAutomationSession(ctx context.Context, sessionID string, status RunStatus, runErr error) error {
...
- stopCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), dispatcherSessionStopTimeout)
+ stopCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), d.sessionStopTimeout)
defer cancel()
...
}
```
</details>
As per coding guidelines, "Never hardcode configuration in Go — use TOML config or functional options".

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In `@internal/automation/dispatch.go` around lines 56 - 60, The hardcoded constant
dispatcherSessionStopTimeout should be made configurable via dispatcher options
rather than fixed in code: remove or replace the package-level const
dispatcherSessionStopTimeout and add a field (e.g., SessionStopTimeout
time.Duration) to the dispatcher options/config struct used by NewDispatcher (or
Dispatcher) and its option helpers; wire that value into the shutdown logic that
currently references dispatcherSessionStopTimeout and provide a sensible default
(10*time.Second) when the option is not set so existing behavior remains
unchanged.
```

</details>

<!-- fingerprinting:phantom:poseidon:hawk -->

<!-- This is an auto-generated comment by CodeRabbit -->
`useAgentDetailPage(name)` runs before the child-route early return, so `/agents/$name/sessions/$id` still pays for the agent + sessions queries even though this component renders only `<Outlet />`. The clean fix is to move the detail shell into an index child route and let this parent route stay as a layout route.

## Triage

- Decision: `VALID`
- Notes: `dispatcherSessionStopTimeout` is a package-level operational timeout fixed at 10 seconds. The dispatcher already uses functional options for policy injection, so this should be configurable with a default. Fix by adding a `sessionStopTimeout` field, a `defaultDispatcherSessionStopTimeout`, and `WithDispatcherSessionStopTimeout`, then using the field in `stopAutomationSession`. A focused constructor test in `internal/automation/dispatch_test.go` is needed even though that file is outside the batch list because it validates the new option.
- Notes:
- `AgentDetailPage` currently calls `useAgentDetailPage(name)` before checking `useChildMatches()`, so nested session routes still start agent detail and session list queries even though the parent renders only `<Outlet />`.
- The root cause is that the detail data hook lives in the layout component instead of in the detail-only rendering branch.
- Fix by splitting the detail shell into a child component that is rendered only when there is no child match, preserving hook rules while preventing nested routes from paying for unused data. Add a route test proving child routes render the outlet without invoking `useAgentDetailPage`.
- Resolution: split the detail shell into `AgentDetailContent`, leaving nested child routes to render `<Outlet />` without invoking the detail hook.
- Verification: targeted Vitest passed; `make verify` passed.
Loading
Loading