feat: agent details#297
Merged
Merged
Conversation
Checkpoint via cy-loop-tasks (iteration 2, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 3, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 4, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 5, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 6, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 8, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 9, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 10, phase B mode=tasks). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Checkpoint via cy-loop-tasks (iteration 13, phase D review round 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (549)
📒 Files selected for processing (260)
Disabled knowledge base sources:
WalkthroughThis PR adds workspace-aware agent contracts, catalog pagination and session facets, create/update/delete/duplicate APIs, CLI lifecycle commands, synchronization and history purging, and web fleet, detail, settings, authored-file, and runtime UI. ChangesAgent platform and user interfaces
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
React Doctor found no issues. 🎉
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/bridge.go (1)
677-747: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this production file below the 500-line limit.
internal/cli/bridge.goreaches Line 747. Extract cohesive output/rendering helpers before extending it further. As per coding guidelines, “Production source files must have one responsibility and stay at or below 500 lines; split contracts, registries, implementations, and helpers into separate files.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/bridge.go` around lines 677 - 747, Split the output/rendering responsibility from bridge.go into a separate production file, moving bridgeBundle and its human/toon rendering helpers together while preserving their existing behavior and symbols. Keep bridge.go at or below 500 lines and update any references or package-level organization needed for the extracted helpers to compile.Source: Coding guidelines
🟡 Minor comments (29)
web/src/systems/agent/components/__tests__/token-list-field.test.tsx-26-28 (1)
26-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRerender the controlled value before testing deduplication.
After the first add, the parent should provide
["agh__skill_view", "mcp__github__*"]. Keeping the initial props makes this assertion incorrectly expect the existing GitHub token to disappear. Rerender with the first callback value, then expect both tokens after the duplicate add.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/__tests__/token-list-field.test.tsx` around lines 26 - 28, Update the test around the token-list field’s first onChange assertion to rerender the controlled component value with the callback result, including both "agh__skill_view" and "mcp__github__*". Then clear the mock, submit the duplicate token, and assert the controlled value retains both tokens rather than replacing the existing GitHub token.web/src/systems/agent/adapters/agent-api.ts-68-79 (1)
68-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the type guards with their actual runtime check.
isAgentDigestConflictandisAgentTargetExistsaccept anyAgentApiErrorwith matchingkind/status, but their predicates claim the concrete subclasses. Narrow only withinstanceof, or change the return type to the broaderAgentApiErrorshape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/adapters/agent-api.ts` around lines 68 - 79, Update the return type predicates of isAgentDigestConflict and isAgentTargetExists to match their runtime checks: either restrict matching to the corresponding concrete error subclasses using instanceof, or broaden each predicate to AgentApiError. Keep the existing kind/status checks only when the declared predicate type accurately represents every accepted error.internal/heartbeat/authoring_status_test.go-41-48 (1)
41-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the expected validation error.
These checks accept any failure, including unrelated storage or I/O errors, so target-validation regressions can still pass. Assert the expected typed/code-specific error for each invalid case. As per path instructions, Go tests “MUST have specific error assertions (ErrorContains, ErrorAs).”
Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/heartbeat/authoring_status_test.go` around lines 41 - 48, Strengthen the invalid-input assertions in the DeleteHeartbeatAgentHistory tests, including the additional case around lines 128-136, by verifying the returned error is the expected validation error using a specific ErrorContains or ErrorAs assertion. Keep the existing t.Fatal behavior for nil errors, but reject unrelated storage or I/O failures.Source: Path instructions
internal/config/agent_test.go-167-168 (1)
167-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake invalid-input assertions specific.
These cases pass for any operational error, not only the intended validation rejection. Assert the expected error type or message for each scenario. As per path instructions, Go tests “MUST have specific error assertions (ErrorContains, ErrorAs).”
Also applies to: 194-195, 213-214, 225-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/agent_test.go` around lines 167 - 168, Update the invalid-input tests around DeleteAgentDefinition and the additional cases to assert the intended validation error specifically, using errors.As or errors.ErrorContains rather than only checking err == nil. Preserve each test’s existing scenario and failure message while verifying the expected error type or identifying message.Source: Path instructions
web/src/systems/agent/components/agent-overview-tab.tsx-204-212 (1)
204-212: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid rounding into the next duration unit.
Math.round(59.5)produces60, so a session under one minute displays as1m; positive sub-second values can display as0s. Floor the value before unit selection (with a deliberate sub-second fallback).Proposed fix
- const total = Math.round(totalSeconds); + const total = Math.max(1, Math.floor(totalSeconds));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-overview-tab.tsx` around lines 204 - 212, Update formatElapsed to floor totalSeconds before selecting duration units, preventing values below a unit boundary from rolling into the next unit. Preserve empty output for non-finite or non-positive values, and add a deliberate fallback so positive sub-second durations display appropriately instead of “0s”.packages/ui/src/index.ts-637-638 (1)
637-638: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required colocated story for the new public exports.
The stack includes the Filters test change, but no corresponding story change for these newly public exports. As per coding guidelines, “Every new export must have a colocated story and a test in the same change.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/index.ts` around lines 637 - 638, Add a colocated story for the newly exported Filters and createFilter symbols, following the existing story conventions for the filters component. Ensure the story demonstrates the public API and is located alongside the corresponding filters implementation, while leaving the existing exports unchanged.Source: Coding guidelines
internal/store/globaldb/global_db_session_agent_counts.go-24-25 (1)
24-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the expired-lock sweep error.
Returning this error directly loses the failing operation in higher-level logs.
Proposed fix
if _, err := g.SweepExpiredSessionAttachLocks(ctx, g.now()); err != nil { - return nil, err + return nil, fmt.Errorf("store: sweep expired session attach locks: %w", err) }As per coding guidelines, production Go code must “wrap errors with
%w”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_session_agent_counts.go` around lines 24 - 25, Update the error return in the SweepExpiredSessionAttachLocks call to wrap the original error with %w and add context identifying the expired-lock sweep operation, while preserving the existing early-return behavior.Source: Coding guidelines
packages/ui/src/components/custom/__tests__/topbar.test.tsx-303-330 (1)
303-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the active owner, not just slot presence.
SlotInspectorrenders onlytitle:yes, so this test also passes if the older slot remains active after unmount. Expose or assert the actual title (Active) before and after rerender.As per coding guidelines, web tests must verify meaningful behavior outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/custom/__tests__/topbar.test.tsx` around lines 303 - 330, The test around Harness and SlotInspector only verifies that a title exists, not which slot owns it. Expose the rendered slot title through SlotInspector or use an existing title assertion, then assert “Active” before and after rerendering with showOlder={false} so the test verifies the active owner remains unchanged.Source: Coding guidelines
packages/ui/src/components/custom/hooks/use-topbar-slot.ts-42-55 (1)
42-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the
backcallback in the slot signature.
slotKeyignores function-valued fields, so a changedbackhandler can stay published until some other slot field changes. That leaves the topbar wired to a stale callback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/custom/hooks/use-topbar-slot.ts` around lines 42 - 55, Update slotKey to preserve the back callback when generating the slot signature, while continuing to omit other function-valued fields. Ensure changes to back produce a different key so the topbar republishes the current callback.Source: Coding guidelines
web/src/systems/agent/hooks/__tests__/use-agent-delete-flow.test.tsx-77-80 (1)
77-80: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse a contract-valid delete response in the mock.
DeleteAgentResponserequiresnameandoriginand does not definedeleted. BecausemockMutateis untyped, this bypasses compile-time contract checks. As per coding guidelines, TypeScript should provide type safety instead of relying on runtime checks.Proposed fix
- opts.onSuccess({ deleted: true, unshadowed_origin: "global" }); + opts.onSuccess({ + name: primaryAgentFixture.name, + origin: "workspace", + unshadowed_origin: "global", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-agent-delete-flow.test.tsx` around lines 77 - 80, Update the success payload in the delete-flow test’s mockMutate implementation to match DeleteAgentResponse: provide the required name and origin fields and remove the unsupported deleted field. Type mockMutate or its onSuccess callback so future invalid response shapes are caught by TypeScript.Source: Coding guidelines
web/src/systems/agent/hooks/use-agent-instructions-tab.ts-60-70 (1)
60-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear wake selections when their session leaves the active set.
The derived
wakeSessionIdbecomesnull, butrequestedWakeSessionIdis retained. If that session later becomes active again, it is silently reselected without user input. Reset or version the selection when the active-session set changes so the UI does not revive stale state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/use-agent-instructions-tab.ts` around lines 60 - 70, Update the wake-session selection state around requestedWakeSessionId and wakeSessionId so requestedWakeSessionId is cleared when its selected session leaves activeSessions. Ensure a session that later becomes active again is not silently reselected, while preserving the existing single-active-session behavior.web/src/systems/agent/components/agent-configuration-tab.tsx-28-37 (1)
28-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same trimmed predicate for absent-value styling.
formatAbsentOverridetreats whitespace-only values as absent, butmutedandmonocheck raw truthiness. A value such as" "therefore renders asDefaultwith foreground/monospace styling instead of the absent-value styling.Suggested fix
<ConfigField label="Model" value={formatAbsentOverride(agent.model)} - muted={!agent.model} + muted={!agent.model?.trim()} /> <ConfigField label="Command" value={formatAbsentOverride(agent.command)} - muted={!agent.command} - mono={Boolean(agent.command)} + muted={!agent.command?.trim()} + mono={Boolean(agent.command?.trim())} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-configuration-tab.tsx` around lines 28 - 37, Update the Model and Command ConfigField styling predicates to use the same trimmed-value presence check as formatAbsentOverride. Ensure whitespace-only agent.model and agent.command values are treated as absent for muted and mono styling, while non-whitespace values retain the existing styling.web/src/systems/agent/hooks/use-agent-heartbeat.ts-112-123 (1)
112-123: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winForward
workspace_idon heartbeat wake.wakeAgentHeartbeatalready supportsworkspace_id, butuseWakeAgentHeartbeatdrops the hook’sworkspaceargument and sends onlysession_id/source. That can mis-scope a workspace wake or trip a 403 while the cache key remains workspace-aware. Passworkspace_idthrough the mutation payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/use-agent-heartbeat.ts` around lines 112 - 123, The useWakeAgentHeartbeat mutation currently ignores its workspace argument when calling wakeAgentHeartbeat. Update the mutationFn to include workspace_id derived from the hook’s workspace parameter in the payload, while preserving the existing params and workspace-aware invalidation behavior.web/src/systems/agent/hooks/use-agent-authored-file-editor.ts-172-184 (1)
172-184: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPersist the validation status returned by validation.
result.validation_statusis ignored, so an invalid response updates diagnostics but leaveseditor.statusderived from the previous payload status. Update the local effective payload/status without changing the CAS baseline.const result = await onValidate(draft); setDiagnostics(result.diagnostics ?? []); + if (result.validation_status !== undefined) { + setEffectivePayload(current => + current ? { ...current, validation_status: result.validation_status } : current + ); + }Also applies to: 255-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/use-agent-authored-file-editor.ts` around lines 172 - 184, Update handleValidate to persist result.validation_status in the local effective payload/status alongside result.diagnostics, so the editor reflects the latest validation outcome. Keep the existing CAS baseline unchanged while applying this local status update, and preserve the current error and validating-state handling.web/src/systems/agent/components/agent-settings-panels.tsx-93-98 (1)
93-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable deletion in read-only and mutation-denied states.
fieldsReadOnlyprotects the editable sections, but the destructive action remains enabled whenreadOnlyormutationDeniedis true. This is not a backend authorization bypass, but it still invokes an invalid destructive mutation from a UI that says editing is unavailable. Pass the effective disabled state toAgentSettingsDangerSectionand cover it with a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-settings-panels.tsx` around lines 93 - 98, Update the danger-section rendering in the agent settings component to pass the effective disabled state, combining readOnly, mutationDenied, and any existing deletion state, to AgentSettingsDangerSection. Ensure the delete action is unavailable in both read-only and mutation-denied modes, and add a regression test covering those states.web/src/systems/agent/components/agent-fleet-list.tsx-31-31 (1)
31-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the load-more handler required.
When
hasMoreis true andonLoadMoreis omitted, this renders an enabled button whose click handler isundefined, so “Load more agents” silently does nothing. Make the handler required, or model the props sohasMorerequiresonLoadMore.Proposed contract fix
- onLoadMore?: () => void; + onLoadMore: () => void;Also applies to: 192-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-fleet-list.tsx` at line 31, Update the agent fleet list props contract around onLoadMore so the load-more handler is required whenever hasMore is true, preventing an enabled button from lacking a click callback. Make onLoadMore non-optional or use a discriminated prop shape tying hasMore=true to a required handler, and update affected call sites accordingly.web/src/routes/_app/__tests__/-agents.test.tsx-317-322 (1)
317-322: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFlush the mount before asserting that no request was made.
The request count is checked immediately after
render, before effect/query scheduling has settled. A regression that starts the catalog request asynchronously could still pass while the count is temporarily zero. Await the no-workspace state and flush a tick before asserting the counter remains unchanged.Proposed test adjustment
mockActiveWorkspaceId = null; render(<AgentsPage />); - expect(screen.getByTestId("agents-no-workspace")).toHaveTextContent("No workspace selected"); + expect(await screen.findByTestId("agents-no-workspace")).toHaveTextContent( + "No workspace selected" + ); + await act(async () => { + await new Promise(resolve => setTimeout(resolve, 0)); + }); expect(agentsRequestCount).toBe(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/routes/_app/__tests__/-agents.test.tsx` around lines 317 - 322, Update the “Should ask for a workspace before querying the fleet” test to await the rendered no-workspace state and flush pending effect/query scheduling before checking agentsRequestCount. Keep the expected “No workspace selected” content and assert the request count remains zero after asynchronous work settles.web/src/systems/agent/lib/agent-fleet-projection.ts-38-40 (1)
38-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep deep-category output within the ellipsis limit.
For paths with more than two segments, this returns the complete first and last segments. If either segment is long, the result can exceed
CATEGORY_ELLIPSIS_LIMIT, causing metadata to overflow despite the helper’s truncation contract. Allocate the limit across truncated first and last segments.Proposed bounded truncation
const first = path[0] ?? ""; const last = path[path.length - 1] ?? ""; - return `${first}${AGENT_CATEGORY_LABEL_SEPARATOR}…${AGENT_CATEGORY_LABEL_SEPARATOR}${last}`; + const available = + CATEGORY_ELLIPSIS_LIMIT - AGENT_CATEGORY_LABEL_SEPARATOR.length * 2 - 1; + const headLength = Math.ceil(available / 2); + const tailLength = Math.floor(available / 2); + return `${first.slice(0, headLength)}${AGENT_CATEGORY_LABEL_SEPARATOR}…${AGENT_CATEGORY_LABEL_SEPARATOR}${last.slice(-tailLength)}`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/lib/agent-fleet-projection.ts` around lines 38 - 40, Update the deep-category formatting helper around the first/last path segment handling to ensure its returned string never exceeds CATEGORY_ELLIPSIS_LIMIT. For paths with more than two segments, truncate or allocate the available limit across the first and last segments while preserving the separators and ellipsis; keep existing behavior for shorter paths.internal/api/testutil/session_stub.go-98-111 (1)
98-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMatch production agent-name normalization.
This fallback counts blank names and treats
" coder "separately from"coder", unlikesession.Manager.CountSessionsByAgent. Normalize and skip empty names here so core/API tests do not validate behavior that production never returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/testutil/session_stub.go` around lines 98 - 111, Update the counting logic in the fallback that builds counts from infos to normalize each agent name using the same behavior as session.Manager.CountSessionsByAgent, trim surrounding whitespace, and skip entries whose normalized name is empty. Use the normalized name as the counts map key while preserving the existing workspace, type, lineage, and state filters.internal/daemon/daemon_agent_definition_e2e_integration_test.go-24-292 (1)
24-292: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the lifecycle scenario in a named subtest.
Create, update, duplicate, delete, and restart assertions execute directly in the top-level test. Put the ordered flow in a
t.Run("Should …")block so every test case follows the required convention.As per path instructions, all test cases must use the
t.Run("Should...")pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/daemon_agent_definition_e2e_integration_test.go` around lines 24 - 292, The ordered lifecycle flow in TestDaemonE2EAgentDefinitionLifecycleParity currently runs at the top level instead of under a named subtest. Wrap the create, update, duplicate, conflict, delete, stop, restart, and not-found assertions in a single t.Run block whose name starts exactly with "Should", while preserving their existing order and shared test context.Source: Path instructions
internal/store/globaldb/global_db_session_index_test.go-200-275 (1)
200-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the dream-session exclusion.
Add a persisted
dreamsession to this fixture. The assertion currently verifies ID, workspace, and spawn-role filtering, butExcludeSessionTypes: []string{"dream"}could regress without failing.As per path instructions, tests must verify meaningful behavior and fail when the business logic changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_session_index_test.go` around lines 200 - 275, Extend the “Should group exact visible counts while excluding the live overlay” fixture with a persisted session whose SessionType is “dream” and whose workspace and agent otherwise qualify for counting. Register it alongside the existing sessions, then keep the expected coder total and active count unchanged so CountSessionsByAgent verifies ExcludeSessionTypes removes that session.Source: Path instructions
internal/daemon/native_agent_catalog.go-31-35 (1)
31-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the catalog error with operation context.
ListAgentsfailures are returned bare, losing the operation context required for daemon diagnostics. As per coding guidelines, production Go code must “wrap errors with%w”.Proposed fix
+ "fmt" + if n.deps.AgentCatalog != nil { catalogAgents, err := n.deps.AgentCatalog.ListAgents(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("list agent catalog: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/native_agent_catalog.go` around lines 31 - 35, Update the ListAgents error handling in the native agent catalog flow to wrap the returned error with descriptive operation context using Go’s %w semantics before returning it. Preserve the existing nil result behavior and avoid changing successful catalog processing.Source: Coding guidelines
internal/soul/authoring_test.go-88-90 (1)
88-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the expected validation errors.
These branches accept any failure, including an unrelated database or filesystem error. Assert the validation contract with
ErrorContainsorErrorAsfor each invalid target. As per path instructions, tests “MUST have specific error assertions (ErrorContains, ErrorAs).”Also applies to: 174-176
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/soul/authoring_test.go` around lines 88 - 90, Strengthen the invalid-target assertions in the DeleteSoulAgentHistory tests, including the additional branch at lines 174-176, by checking the returned error with ErrorContains or ErrorAs for the expected validation error. Keep the existing nil-error failure check, but ensure unrelated database or filesystem failures cannot satisfy these tests.Source: Path instructions
internal/cli/agent_commands_test.go-291-292 (1)
291-292: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConnect the filesystem assertion to the command fixture.
workspaceRootis never passed todepsand is unrelated to--workspace ws-alpha, so this always checks a fresh unrelated directory. Configure the fixture’s resolved workspace path, then assert the intended daemon-only authoring outcome there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/agent_commands_test.go` around lines 291 - 292, Update the command fixture setup so the resolved workspace path for --workspace ws-alpha is assigned to the dependency configuration used by the CLI, rather than relying on the unrelated workspaceRoot variable. Then perform the filesystem assertion against that configured workspace path, preserving the expectation that the daemon-only command does not create the authoring directory.Source: Path instructions
internal/cli/client_test.go-215-230 (1)
215-230: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert lifecycle request bodies.
The transport assertion only checks method, path, and query. Dropping or misserializing
scope,workspace,expected_digest, or duplicate overrides would still pass. Decodereq.Bodyper case and assert the relevant request payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/client_test.go` around lines 215 - 230, Extend the roundTripperFunc assertion in the unixSocketClient test to decode req.Body for each test case and validate the expected payload fields, including scope, workspace, expected_digest, and duplicate overrides. Keep the existing method, path, and query assertions, and compare the decoded request body against each case’s expected request data.Source: Path instructions
internal/api/httpapi/handlers_test.go-250-250 (1)
250-250: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap this route contract in a named subtest.
The expanded assertions remain in a top-level test body; use
t.Run("Should …")for this route-contract case.Proposed fix
func TestRegisterRoutesCoversTechSpecEndpoints(t *testing.T) { + t.Run("Should register every expected HTTP API route", func(t *testing.T) { // existing setup and assertions + }) }As per coding guidelines and path instructions, “Go tests must use
t.Run("Should …")” for all test cases.Also applies to: 369-369, 383-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/httpapi/handlers_test.go` at line 250, Wrap the “POST /api/agents/:name/duplicate” route-contract assertions in a named subtest using t.Run("Should …"), and apply the same structure to the related cases at the other indicated locations. Keep each case’s existing assertions and behavior unchanged.Sources: Coding guidelines, Path instructions
internal/cli/client_agent.go-23-104 (1)
23-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap client-operation errors with context.
Each new API method returns
doJSONerrors unchanged, so CLI failures lose the lifecycle operation that failed. Wrap them with%wwhile preservingerrors.Is/As.Proposed fix
import ( "context" + "fmt" "net/http" "net/url" "strings" ) - return nil, err + return nil, fmt.Errorf("list agents: %w", err) - return AgentRecord{}, err + return AgentRecord{}, fmt.Errorf("get agent %q: %w", name, err) - return AgentRecord{}, err + return AgentRecord{}, fmt.Errorf("create agent: %w", err) - return AgentRecord{}, err + return AgentRecord{}, fmt.Errorf("update agent %q: %w", name, err) - return contract.DeleteAgentResponse{}, err + return contract.DeleteAgentResponse{}, fmt.Errorf("delete agent %q: %w", name, err) - return AgentRecord{}, err + return AgentRecord{}, fmt.Errorf("duplicate agent %q: %w", name, err)As per coding guidelines, production Go code must “wrap errors with
%w.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/client_agent.go` around lines 23 - 104, Wrap each doJSON error in ListAgents, GetAgent, CreateAgent, UpdateAgent, DeleteAgent, and DuplicateAgent with an operation-specific context message using %w, while preserving the existing zero-value return behavior and errors.Is/As compatibility.Source: Coding guidelines
web/src/systems/agent/hooks/use-agents.ts-62-64 (1)
62-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not discard settled errors with
_error.These callbacks intentionally invalidate caches on settlement, but the
_errorparameters still discard mutation failures. Handle or explicitly acknowledge the error while preserving invalidation; the repository guideline forbids silently discarding errors with_.Also applies to: 86-89, 113-116, 139-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/use-agents.ts` around lines 62 - 64, Update the onSettled callbacks in the agent mutation hooks to handle the settled error parameter instead of naming it _error, while preserving both agentKeys.lists() and agentKeys.catalogs() invalidations. Apply the same explicit error handling or acknowledgment to all corresponding callbacks at the referenced mutation locations.Source: Coding guidelines
web/src/hooks/routes/use-agents-fleet-page.ts-67-77 (1)
67-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not capture
/when the fleet search is not rendered.Agent detail/settings routes render through this parent, so this handler prevents the browser default while
searchInputRefhas no input to focus. Re-register it when child-route state changes.Proposed fix
useEffect(() => { + if (hasChildMatch || workspaceId === "") return; + const onKeyDown = (event: KeyboardEvent) => { if (event.key !== "/" || event.metaKey || event.ctrlKey || event.altKey) return; if (isEditableTarget(event.target)) return; if (event.target instanceof Element && event.target.closest('[role="dialog"]')) return; event.preventDefault(); searchInputRef.current?.focus(); }; document.addEventListener("keydown", onKeyDown); return () => document.removeEventListener("keydown", onKeyDown); - }, []); + }, [hasChildMatch, workspaceId]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/hooks/routes/use-agents-fleet-page.ts` around lines 67 - 77, Update the useEffect keyboard handler to only intercept "/" while the fleet search input is rendered, using the relevant child-route/search visibility state. Include that state in the effect dependencies so the listener is removed or re-registered when navigating between fleet list and detail/settings routes; preserve the existing target and modifier checks.
🧹 Nitpick comments (11)
web/src/systems/agent/hooks/__tests__/use-unsaved-guard.test.tsx (1)
55-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the returned confirmation dialog.
The test verifies
proceed/resetdirectly but never rendersresult.current.confirmDialog, leaving theopen, copy, confirm, and cancel behavior untested. Add a test that mounts the dialog and verifies confirm callsproceedwhile cancel/close callsreset.As per coding guidelines, web tests should have high code coverage for critical paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-unsaved-guard.test.tsx` around lines 55 - 82, The useUnsavedGuard test should render result.current.confirmDialog and verify the confirmation dialog’s open state and copy, then trigger its confirm action to call mockProceed and its cancel/close action to call mockReset. Extend the existing blocked-state test or add a focused test while preserving the current proceed/reset assertions.Source: Coding guidelines
web/src/systems/agent/components/__tests__/agent-heartbeat-ops.test.tsx (1)
72-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for stale heartbeat state.
The suite only tests
statusErrorwhenstatusis undefined. Add cases for cached status plusstatusError, and for a selected ID absent fromactiveSessions; assert the wake button is disabled, retry is available, andonWakeis not called.As per coding guidelines, web tests should maintain high coverage for critical paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/__tests__/agent-heartbeat-ops.test.tsx` around lines 72 - 119, Add regression tests in the AgentHeartbeatOps suite for stale heartbeat state: render with cached status plus statusError and with selectedSessionId absent from activeSessions, then assert the wake button is disabled, the Retry control is available and invokes onRetryStatus, and onWake is never called. Reuse the existing direct AgentHeartbeatOps setup and mocks while preserving current healthy and ineligible-session coverage.Source: Coding guidelines
web/src/systems/agent/hooks/__tests__/use-agent-instructions-tab.test.tsx (1)
162-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the heartbeat validation, restore, and retry calls.
The test invokes
onValidate,onRestore, and both retry handlers, but only verifies the save path and a subset of refetches. The test can therefore pass while heartbeat validation, rollback, or retry wiring is broken.As per coding guidelines, critical web paths should have comprehensive behavioral coverage.
Suggested assertions
+ expect(mocks.validateHeartbeat).toHaveBeenCalledWith({ + body: "heartbeat-body", + workspace_id: "ws-test", + }); + expect(mocks.rollbackHeartbeat).toHaveBeenCalledWith({ + revision_id: "heartbeat-rev", + expected_digest: "heartbeat-digest", + workspace_id: "ws-test", + }); + expect(mocks.soulHistoryRefetch).toHaveBeenCalled(); + expect(mocks.heartbeatRefetch).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-agent-instructions-tab.test.tsx` around lines 162 - 196, Extend the behavioral assertions in the test around the soul and heartbeat handlers to verify heartbeat validation via mocks.validateHeartbeat with its body and workspace_id, heartbeat restore via mocks.rollbackHeartbeat with revision_id, expected_digest, and workspace_id, and both onRetry handlers invoking their expected retry behavior. Preserve the existing save, wake, and refetch assertions while covering each invoked callback.Source: Coding guidelines
web/src/systems/agent/components/__tests__/agent-authored-file-editor.test.tsx (1)
68-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the validation status as well as diagnostics.
The mock returns
validation_status: "invalid", but this test only checks the diagnostic text. Add an assertion that the editor displaysinvalid; otherwise the test will not catch stale status-pill behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/__tests__/agent-authored-file-editor.test.tsx` around lines 68 - 99, Extend the test around the agent-soul validation flow after clicking agent-soul-validate to assert that the editor displays the returned validation status “invalid”, alongside the existing “Role is required” diagnostic assertion. Use the rendered status text or the relevant status-pill selector so stale validation-status rendering is detected.web/src/systems/agent/lib/__tests__/agent-authored-file-source.test.ts (1)
25-48: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for lossless body serialization.
This exact-output test uses ordinary Markdown only, so it would pass despite
finishSourceremoving trailing Markdown spaces/newlines. Include a body ending in something like"line \n\n"and assert that the serialized source preserves it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/lib/__tests__/agent-authored-file-source.test.ts` around lines 25 - 48, Add a regression case to the serializeAgentSoulSource test using a body that ends with meaningful trailing Markdown spaces and newlines, such as “line \n\n”. Update the exact expected serialized output to retain those characters, ensuring finishSource does not trim the body during serialization.web/src/systems/agent/components/agent-instructions-tab.tsx (1)
30-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the prescribed eyebrow treatment for uppercase file labels.
AGENT.md,SOUL.md, andHEARTBEAT.mdare rendered as plain labels. Apply<Eyebrow>or theeyebrowutility class to the filename text, while keeping the “missing” badge separate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-instructions-tab.tsx` around lines 30 - 60, Update the file labels in the fileItems useMemo to apply the prescribed eyebrow treatment to the filename text for AGENT.md, SOUL.md, and HEARTBEAT.md, using the existing Eyebrow component or eyebrow utility class. Keep each “missing” badge outside the eyebrow-styled filename text.Source: Coding guidelines
web/src/systems/agent/components/agent-fleet-list.tsx (1)
55-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse design-system breakpoints and sizing tokens.
The new
min-[720px],min-[1100px], andsize-[26px]values are ad hoc values. Replace them with existing exported design values, or add the values to the token source before using them here.As per coding guidelines, web styling must use design values from
packages/ui/src/tokens.cssandDESIGN.md.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/components/agent-fleet-list.tsx` around lines 55 - 73, Update the loading grid and action skeleton in the agent-fleet-list component to use existing exported design-system breakpoint and sizing tokens instead of the arbitrary 720px, 1100px, and 26px values. If suitable tokens do not exist, add them to packages/ui/src/tokens.css and document them in DESIGN.md before referencing them in the component.Source: Coding guidelines
web/src/systems/agent/hooks/__tests__/use-agent-heartbeat.test.tsx (2)
130-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the put assertion prove a cache update.
The query has already cached
heartbeat, and the put mock returns the same value, so Line 158 passes even if the mutation’s cache update is removed. Return a distinct post-put value and assert the adapter arguments plus the updated cache entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-agent-heartbeat.test.tsx` around lines 130 - 159, Update the “Should load heartbeat/status and cache put results” test so the put mutation returns a value distinct from the initially cached heartbeat, then assert the mutation adapter receives the expected arguments and that queryClient.getQueryData for agentKeys.heartbeat reflects the distinct post-put value. Keep the existing pre-put setup and hook coverage unchanged.
161-193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every mutation branch in this test.
Lines 168-184 only prove that validate, delete, and rollback resolve; only wake is verified at Line 192. A disconnected validate/delete/rollback hook could still pass. Assert each adapter call and its expected cache/state transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-agent-heartbeat.test.tsx` around lines 161 - 193, Strengthen the “Should validate, delete, rollback, and wake” test by asserting each mutation’s adapter invocation and expected cache/state transition, not just that mutateAsync resolves. Add assertions after the validate, delete, and rollback calls, alongside the existing mockWake assertion, using the corresponding hook and mock symbols already defined in the test.web/src/systems/agent/hooks/__tests__/use-agent-soul.test.tsx (1)
97-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake put and rollback cache checks non-tautological.
The initial query already stores
soul, and both mutations return the same value. Therefore, Lines 106 and 138 pass even if the mutation success handlers do nothing. Use distinct returned payloads and assert exact adapter arguments and cache updates.Also applies to: 129-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/systems/agent/hooks/__tests__/use-agent-soul.test.tsx` around lines 97 - 106, Update the usePutAgentSoul and rollback mutation tests so their mocked mutation responses differ from the initial cached soul, then assert the adapter receives the expected arguments and each success handler updates the corresponding agentKeys.soul cache to the distinct returned payload. Ensure the assertions would fail if the mutation handlers did nothing.internal/daemon/agent_resource_catalog.go (1)
62-65: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the redundant pre-sort.
recordsis rebuilt into a map and sorted by name later, so this only adds extra work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/agent_resource_catalog.go` around lines 62 - 65, Remove the slices.SortFunc call and its agentRecordSortKey comparator from the records snapshot in the catalog flow, leaving records unsorted before it is rebuilt into the map and sorted by name later.
Comment on lines
+125
to
+145
| if err := aghconfig.DeleteAgentDefinition(agentsRoot, resolved.Entry.Def.SourcePath); err != nil { | ||
| h.respondError(c, statusForAgentDefinitionError(err), err) | ||
| return | ||
| } | ||
| mutationErr := h.purgeAgentDefinitionHistory(c.Request.Context(), resolved) | ||
| syncStartedAt := time.Now() | ||
| if syncErr := h.AgentDefinitionSync.Sync(c.Request.Context()); syncErr != nil { | ||
| mutationErr = errors.Join(mutationErr, fmt.Errorf("api: sync deleted agent definition: %w", syncErr)) | ||
| } | ||
| syncDuration := time.Since(syncStartedAt) | ||
| if mutationErr != nil { | ||
| h.logAgentMutationFailure( | ||
| "delete", | ||
| resolved.Entry.Def.SourcePath, | ||
| startedAt, | ||
| syncDuration, | ||
| mutationErr, | ||
| ) | ||
| h.respondError(c, http.StatusInternalServerError, mutationErr) | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files internal/api/core/agent_definitions.go internal/api/core | sed 's#^`#FILE` #'
echo '--- outline ---'
ast-grep outline internal/api/core/agent_definitions.go --view expanded || true
echo '--- relevant ranges ---'
nl -ba internal/api/core/agent_definitions.go | sed -n '1,260p'
echo '--- purge helper search ---'
rg -n "purgeAgentDefinitionHistory|DeleteAgentDefinition|resolveAgentDefinition|soul|heartbeat|revision history" internal/api/core -nRepository: compozy/agh
Length of output: 8172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' internal/api/core/agent_definitions.go | nl -ba | sed -n '1,260p'Repository: compozy/agh
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba internal/api/core/agent_definitions.go | sed -n '1,220p'Repository: compozy/agh
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- delete handler ---'
sed -n '98,155p' internal/api/core/agent_definitions.go
echo '--- purge helper ---'
sed -n '293,340p' internal/api/core/agent_definitions.go
echo '--- resolve/delete error handling references ---'
rg -n "resolveAgentDefinition|DeleteAgentDefinition|purgeAgentDefinitionHistory|statusForAgentDefinitionError|NotFound" internal/api/core/agent_definitions.go internal/api/core/agent_definition_resolution.goRepository: compozy/agh
Length of output: 5869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- resolver ---'
sed -n '1,140p' internal/api/core/agent_definition_resolution.go
echo '--- purger implementations/references ---'
rg -n "type .*HistoryPurger|PurgeAgentHistory\\(|SoulHistoryPurger|HeartbeatHistoryPurger|agent definition history purgers" internal -g '*.go'
echo '--- delete agent definition semantics ---'
rg -n "func DeleteAgentDefinition|DeleteAgentDefinition\\(" -g '*.go' .Repository: compozy/agh
Length of output: 12298
Delete history before removing the definition.
For workspace-scoped agents, deleting the definition first makes a purge failure unretryable: a later request resolves as 404, so the soul/heartbeat history can be left behind permanently. Purge first here (it only needs workspaceID, name, and sourcePath), or add a compensating cleanup path if delete must stay first.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/core/agent_definitions.go` around lines 125 - 145, Update the
agent deletion flow around aghconfig.DeleteAgentDefinition and
purgeAgentDefinitionHistory to purge workspace-scoped soul/heartbeat history
before removing the definition, using the resolved workspaceID, name, and
sourcePath. Preserve the existing sync, mutation-error logging, and response
behavior, ensuring a purge failure leaves the definition available for retry.
Comment on lines
+122
to
+132
| agent, err := aghconfig.CreateAgentDefFile(path, draft, false) | ||
| if err != nil { | ||
| h.respondError(c, statusForCreateAgentError(err), err) | ||
| return | ||
| } | ||
| syncStartedAt := time.Now() | ||
| if err := h.AgentDefinitionSync.Sync(c.Request.Context()); err != nil { | ||
| syncErr := fmt.Errorf("api: sync created agent definition: %w", err) | ||
| h.logAgentMutationFailure("create", agent.SourcePath, startedAt, time.Since(syncStartedAt), syncErr) | ||
| h.respondError(c, http.StatusInternalServerError, syncErr) | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not report creation as failed after persisting the definition.
Line 122 writes the agent file before sync; Lines 128-132 return a failure when sync fails, leaving a durable agent behind. Clients receive a 500 and may retry into a conflict while the catalog remains stale. Make this a compensating operation (including reconciliation) or return an explicit persisted-but-unsynced result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/core/agent_handlers.go` around lines 122 - 132, Update the
create-agent flow around CreateAgentDefFile and AgentDefinitionSync.Sync so a
sync failure after persistence is handled as a compensating operation, including
catalog reconciliation, or returned as an explicit persisted-but-unsynced
outcome. Do not report the operation as a generic creation failure or return a
retry-inducing 500 while the agent definition remains durable; preserve failure
logging through logAgentMutationFailure as appropriate.
Comment on lines
+31
to
+41
| target, err := h.resolveAuthoredAgentTarget(ctx, health.WorkspaceID, health.AgentName) | ||
| if err != nil { | ||
| return contract.HeartbeatStatusResponse{}, err | ||
| } | ||
| result, err := h.HeartbeatStatus.Status(ctx, heartbeat.StatusRequest{ | ||
| Target: target.heartbeatAuthoringTarget(), | ||
| SessionID: health.SessionID, | ||
| IncludeSessionHealth: includeHealth, | ||
| }) | ||
| if err != nil { | ||
| return contract.HeartbeatStatusResponse{}, err |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add context when propagating resolver and status errors.
Preserve the sentinel chain with %w, but identify whether target resolution or the heartbeat status call failed.
Proposed fix
import (
"context"
"errors"
+ "fmt"
"github.com/compozy/agh/internal/api/contract"
"github.com/compozy/agh/internal/heartbeat"
)
target, err := h.resolveAuthoredAgentTarget(ctx, health.WorkspaceID, health.AgentName)
if err != nil {
- return contract.HeartbeatStatusResponse{}, err
+ return contract.HeartbeatStatusResponse{}, fmt.Errorf("resolve authored agent target: %w", err)
}
...
if err != nil {
- return contract.HeartbeatStatusResponse{}, err
+ return contract.HeartbeatStatusResponse{}, fmt.Errorf("read heartbeat status: %w", err)
}As per coding guidelines, production Go code must follow agh-code-guidelines: wrap errors with %w.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| target, err := h.resolveAuthoredAgentTarget(ctx, health.WorkspaceID, health.AgentName) | |
| if err != nil { | |
| return contract.HeartbeatStatusResponse{}, err | |
| } | |
| result, err := h.HeartbeatStatus.Status(ctx, heartbeat.StatusRequest{ | |
| Target: target.heartbeatAuthoringTarget(), | |
| SessionID: health.SessionID, | |
| IncludeSessionHealth: includeHealth, | |
| }) | |
| if err != nil { | |
| return contract.HeartbeatStatusResponse{}, err | |
| target, err := h.resolveAuthoredAgentTarget(ctx, health.WorkspaceID, health.AgentName) | |
| if err != nil { | |
| return contract.HeartbeatStatusResponse{}, fmt.Errorf("resolve authored agent target: %w", err) | |
| } | |
| result, err := h.HeartbeatStatus.Status(ctx, heartbeat.StatusRequest{ | |
| Target: target.heartbeatAuthoringTarget(), | |
| SessionID: health.SessionID, | |
| IncludeSessionHealth: includeHealth, | |
| }) | |
| if err != nil { | |
| return contract.HeartbeatStatusResponse{}, fmt.Errorf("read heartbeat status: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/core/session_heartbeat_status.go` around lines 31 - 41, Wrap
both errors in the heartbeat status flow with contextual messages using %w:
identify the failure from resolveAuthoredAgentTarget as target resolution and
the failure from HeartbeatStatus.Status as the heartbeat status call. Preserve
the existing error returns and sentinel-error chain.
Source: Coding guidelines
Comment on lines
+30
to
+43
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| client, err := clientFromDeps(deps) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| request, err := createAgentRequestFromFlags(cmd, args[0], flags) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| agent, err := client.CreateAgent(cmd.Context(), request) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return writeCommandOutput(cmd, agentBundle(agent)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Wrap command-boundary errors with operation context.
The raw return err paths make CLI failures ambiguous. Wrap each underlying error with the command step while preserving errors.Is/As via %w.
Proposed fix
client, err := clientFromDeps(deps)
if err != nil {
- return err
+ return fmt.Errorf("cli: initialize agent client: %w", err)
}
request, err := createAgentRequestFromFlags(cmd, args[0], flags)
if err != nil {
- return err
+ return fmt.Errorf("cli: build create-agent request: %w", err)
}
agent, err := client.CreateAgent(cmd.Context(), request)
if err != nil {
- return err
+ return fmt.Errorf("cli: create agent: %w", err)
}As per coding guidelines, production Go code must follow agh-code-guidelines: wrap errors with %w.
Also applies to: 58-82, 97-113, 128-141, 159-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/agent_mutate.go` around lines 30 - 43, Update the RunE handlers
in the affected command sections to wrap each error from clientFromDeps,
createAgentRequestFromFlags, client.CreateAgent, and writeCommandOutput with
operation-specific context using %w, preserving errors.Is/As behavior. Apply the
same treatment to all indicated ranges while keeping successful command flow
unchanged.
Source: Coding guidelines
Comment on lines
+564
to
+568
| if agentChanged { | ||
| if err := s.projectAgents(ctx); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Retry a failed agent projection on later syncs.
Agent writes are already durable when projectAgents fails. The next Sync then gets agentChanged == false and permanently skips projection, leaving the agent catalog stale until a later config mutation. Project every sync, or retain pending projection state.
Proposed fix
- if agentChanged {
- if err := s.projectAgents(ctx); err != nil {
- return err
- }
+ if err := s.projectAgents(ctx); err != nil {
+ return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if agentChanged { | |
| if err := s.projectAgents(ctx); err != nil { | |
| return err | |
| } | |
| } | |
| if err := s.projectAgents(ctx); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/agent_skill_resources.go` around lines 564 - 568, Update the
Sync flow around projectAgents so agent projection is attempted on every sync,
rather than only when agentChanged is true. Preserve the existing error
propagation from projectAgents and ensure failed projections are retried during
subsequent syncs until they succeed.
Comment on lines
+29
to
+31
| function finishSource(lines: string[], body: string): string { | ||
| lines.push("---", body.trimEnd()); | ||
| return `${lines.join("\n")}\n`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve trailing body whitespace during serialization.
trimEnd() removes meaningful Markdown trailing spaces (for example, hard line breaks) and blank lines on every save, contradicting the “lossless” contract and potentially changing authored content.
Proposed fix
function finishSource(lines: string[], body: string): string {
- lines.push("---", body.trimEnd());
+ const normalizedBody = body.endsWith("\n") ? body : `${body}\n`;
+ lines.push("---", normalizedBody);
return `${lines.join("\n")}\n`;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/systems/agent/lib/agent-authored-file-source.ts` around lines 29 -
31, Update finishSource to preserve the body exactly during serialization:
remove the body.trimEnd() call while retaining the existing separator and final
newline behavior. Ensure authored trailing spaces and blank lines remain
unchanged across saves.
Comment on lines
+22
to
+31
| export function agentCatalogRequest( | ||
| workspace: string, | ||
| filters: AgentCatalogStableFilter, | ||
| cursor?: string | ||
| ) { | ||
| return { | ||
| workspace: workspace.trim(), | ||
| ...filters, | ||
| ...(cursor ? { cursor } : {}), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize filters before building the request.
agentKeys.catalog uses normalizeAgentCatalogFilter, but this request builder spreads raw filters. For example, { q: " " } hashes like an omitted query while still being sent to the API, allowing responses to be reused under the wrong cache key.
export function agentCatalogRequest(
workspace: string,
filters: AgentCatalogStableFilter,
cursor?: string
) {
return {
workspace: workspace.trim(),
- ...filters,
+ ...normalizeAgentCatalogFilter(filters),
...(cursor ? { cursor } : {}),
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function agentCatalogRequest( | |
| workspace: string, | |
| filters: AgentCatalogStableFilter, | |
| cursor?: string | |
| ) { | |
| return { | |
| workspace: workspace.trim(), | |
| ...filters, | |
| ...(cursor ? { cursor } : {}), | |
| }; | |
| export function agentCatalogRequest( | |
| workspace: string, | |
| filters: AgentCatalogStableFilter, | |
| cursor?: string | |
| ) { | |
| return { | |
| workspace: workspace.trim(), | |
| ...normalizeAgentCatalogFilter(filters), | |
| ...(cursor ? { cursor } : {}), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/systems/agent/lib/agent-catalog-query.ts` around lines 22 - 31,
Update agentCatalogRequest to normalize filters with normalizeAgentCatalogFilter
before spreading them into the returned request. Ensure the normalized filters
are used consistently for both the API payload and the cache-key behavior, while
preserving workspace trimming and conditional cursor handling.
This was referenced Jul 12, 2026
This was referenced Jul 13, 2026
This was referenced Jul 20, 2026
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