feat: orchestration improvements#106
Merged
Merged
Conversation
|
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 (53)
📒 Files selected for processing (57)
WalkthroughThis PR introduces comprehensive task execution orchestration by adding per-task execution profiles for worker/coordinator/sandbox configuration, task run review workflows for human validation and continuation, and bridge notification subscriptions for terminal task event delivery. Changes span API contracts, HTTP handlers, database schema and migrations, daemon services, configuration system, and CLI tooling. ChangesTask Execution Orchestration
Sequence DiagramThe PR's scope and heterogeneous nature (spanning database, API, daemon, CLI, and config domains without a single clearly delineated sequential flow among distinct external components) do not meet the conditions for a sequence diagram. The changes are primarily internal service integrations and data persistence layers rather than interactions between separable client/server/database actors following a clear temporal sequence. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical 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 (6)
internal/extension/host_api_test.go (1)
4923-4942:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRefresh the session list before issuing cleanup stops.
activeSessionsis captured before the settle/drain wait, so any session started in that window is never stopped here. These tests exercise async bridge and automation flows, so this can leave a live session behind and make later parallel tests flaky.Suggested fix
- activeSessions := manager.List() - if waitForHostAPIPromptsToSettle(t, manager, activeSessions) { + activeSessions := manager.List() + if waitForHostAPIPromptsToSettle(t, manager, activeSessions) { if err := manager.WaitForPromptDrains(ctx); err != nil { t.Errorf("sessions.WaitForPromptDrains() cleanup error = %v", err) } } - for _, info := range activeSessions { + for _, info := range manager.List() { if info == nil { continue } if err := manager.Stop(ctx, info.ID); err != nil && !errors.Is(err, session.ErrSessionNotFound) { t.Errorf("sessions.Stop(%q) cleanup error = %v", info.ID, 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/extension/host_api_test.go` around lines 4923 - 4942, The cleanup uses activeSessions captured before waiting for prompts to settle, so sessions created during wait can be missed; after waitForHostAPIPromptsToSettle and manager.WaitForPromptDrains calls, re-fetch the session list by calling manager.List() again (replace iterating over the old activeSessions with the fresh list), then perform the same nil-check and manager.Stop(ctx, info.ID) with the existing errors.Is(err, session.ErrSessionNotFound) guard, and keep the subsequent manager.WaitForFinalizations and final manager.WaitForPromptDrains calls as-is.internal/session/manager_start.go (2)
130-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the persisted model override on resume.
prepareSessionStartRuntimenow resolves withspec.model, but the resume path never restoresmeta.ModelintosessionStartSpec. A session created with an explicit runtime model can therefore resume against the workspace/default model instead of the stored one.Suggested fix
return sessionStartSpec{ sessionID: meta.ID, sandboxID: sessionSandboxID(meta.Sandbox), sandbox: cloneSessionSandboxMeta(meta.Sandbox), sandboxDisabled: meta.Sandbox == nil, sessionName: meta.Name, agentName: meta.AgentName, provider: strings.TrimSpace(meta.Provider), + model: strings.TrimSpace(meta.Model), workspace: resolvedWorkspace, channel: strings.TrimSpace(meta.Channel), sessionType: normalizeSessionType(Type(meta.SessionType)),Also applies to: 363-365
🤖 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/session/manager_start.go` around lines 130 - 154, The resume path is not restoring the persisted runtime model (meta.Model) into the returned sessionStartSpec, so resumed sessions can pick up workspace/default model instead of the stored model; update the resume construction (the sessionStartSpec return in manager_start.go and the similar block at the other resume location around lines 363-365) to set spec.model (or the sessionStartSpec field that holds runtime/model information) from meta.Model (or deref/meta equivalent) before returning, and ensure prepareSessionStartRuntime receives that restored model when called so the explicit persisted model is preserved on resume (reference prepareSessionStartRuntime, sessionStartSpec, and meta.Model).
130-154:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't recompute sandbox cleanup from the current workspace on resume.
On create,
applyCreateSandboxOverridecan change the effective sandbox selection, but resume only preservesmeta.Sandbox == nil.newStartingSessionthen derivessandboxDestroyOnStopfrom the current workspace config, so a resumed session can destroy an overridden sandbox after a workspace config change or keep one that should have been cleaned up. Persist the effective destroy-on-stop choice with the session sandbox metadata and restore it here instead of recalculating it fromspec.workspace.Also applies to: 463-464
🤖 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/session/manager_start.go` around lines 130 - 154, When resuming a session, don't recompute sandbox destruction behavior from the current workspace; instead restore the destroy-on-stop flag that was saved with the session sandbox metadata. In the code that builds the sessionStartSpec (the return block in newStartingSession/resume path), set the sandboxDestroyOnStop field from the persisted meta.Sandbox (or the stored sandbox metadata) rather than deriving it from spec.workspace or applyCreateSandboxOverride logic; likewise update the other resume code path referenced around lines 463-464 to read and copy the saved destroy-on-stop value from meta.Sandbox into the sessionStartSpec so resumed sessions honor the original create-time override.internal/api/core/tasks_surface_test.go (1)
85-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the new dependency assertions with a length check.
If
TaskSummaryPayloadFromSummaryregresses toDependencyCount == 1but returns an emptyDependenciesslice, this test will panic before it prints the payload. Addinglen(summaryPayload.Dependencies) != 1before indexing keeps the failure diagnostic.🤖 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/tasks_surface_test.go` around lines 85 - 93, The test currently indexes summaryPayload.Dependencies without checking its length, which can panic if Dependencies is empty; update the failing condition in the test to also assert len(summaryPayload.Dependencies) == 1 (e.g. include len(summaryPayload.Dependencies) != 1 in the combined if) before accessing summaryPayload.Dependencies[0], so the guard aligns with DependencyCount and prevents a panic while preserving the existing assertions (referencing summaryPayload, Dependencies, DependencyCount, and the TaskSummaryPayloadFromSummary usage).internal/daemon/bridges.go (1)
31-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire cursor persistence in
bridgeRuntimeStore.These methods silently degrade to
ErrCursorNotFoundwhen the injected store does not implementnotifications.CursorStore, so a wiring bug looks identical to missing data and the feature fails only at runtime.Suggested fix
type bridgeRuntimeStore interface { bridgepkg.RegistryStore bridgepkg.ResourceProjectionStore bridgepkg.BridgeTaskSubscriptionStore + notifications.CursorStore bridgeDedupStore PutBridgeSecretBinding(ctx context.Context, binding bridgepkg.BridgeSecretBinding) error ListBridgeSecretBindings(ctx context.Context, bridgeInstanceID string) ([]bridgepkg.BridgeSecretBinding, error) DeleteBridgeSecretBinding(ctx context.Context, bridgeInstanceID string, bindingName string) error }func (r *bridgeRuntime) GetCursor( ctx context.Context, key notifications.CursorKey, ) (notifications.Cursor, error) { if r == nil || r.store == nil { return notifications.Cursor{}, notifications.ErrCursorNotFound } - cursorStore, ok := r.store.(interface { - GetCursor(context.Context, notifications.CursorKey) (notifications.Cursor, error) - }) - if !ok { - return notifications.Cursor{}, notifications.ErrCursorNotFound - } - return cursorStore.GetCursor(ctx, key) + return r.store.GetCursor(ctx, key) } func (r *bridgeRuntime) ListCursors( ctx context.Context, query notifications.CursorQuery, ) ([]notifications.Cursor, error) { if r == nil || r.store == nil { return nil, notifications.ErrCursorNotFound } - cursorStore, ok := r.store.(notifications.CursorStore) - if !ok { - return nil, notifications.ErrCursorNotFound - } - return cursorStore.ListCursors(ctx, query) + return r.store.ListCursors(ctx, query) } func (r *bridgeRuntime) AdvanceCursor( ctx context.Context, update notifications.AdvanceCursor, ) (notifications.Cursor, error) { if r == nil || r.store == nil { return notifications.Cursor{}, notifications.ErrCursorNotFound } - cursorStore, ok := r.store.(notifications.CursorStore) - if !ok { - return notifications.Cursor{}, notifications.ErrCursorNotFound - } - return cursorStore.AdvanceCursor(ctx, update) + return r.store.AdvanceCursor(ctx, update) } func (r *bridgeRuntime) ResetCursor( ctx context.Context, reset notifications.ResetCursor, ) (notifications.Cursor, error) { if r == nil || r.store == nil { return notifications.Cursor{}, notifications.ErrCursorNotFound } - cursorStore, ok := r.store.(notifications.CursorStore) - if !ok { - return notifications.Cursor{}, notifications.ErrCursorNotFound - } - return cursorStore.ResetCursor(ctx, reset) + return r.store.ResetCursor(ctx, reset) } func (r *bridgeRuntime) RecordCursorError( ctx context.Context, report notifications.CursorError, ) (notifications.Cursor, error) { if r == nil || r.store == nil { return notifications.Cursor{}, notifications.ErrCursorNotFound } - cursorStore, ok := r.store.(notifications.CursorStore) - if !ok { - return notifications.Cursor{}, notifications.ErrCursorNotFound - } - return cursorStore.RecordCursorError(ctx, report) + return r.store.RecordCursorError(ctx, report) }Also applies to: 171-241
🤖 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/bridges.go` around lines 31 - 39, The bridgeRuntimeStore interface currently omits cursor persistence so non‑CursorStore implementations silently degrade; update bridgeRuntimeStore to require notifications.CursorStore (embed notifications.CursorStore) so the compiler enforces cursor methods are present, then re-run and fix any implementors; also apply the same change to the other runtime store interface(s) in this file that mirror bridgeRuntimeStore (the ones combined with bridgeDedupStore/Bridge* methods) so all stores must implement notifications.CursorStore.internal/situation/service.go (1)
525-529:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPopulate
latest_event_seqon these task references.
contract.TaskReferencePayloadnow includesLatestEventSeq, but both of these paths still build their task context throughtaskReferencePayload(taskRecord), which never sets it. That means the active-run and review fallback payloads will always emit0here even when the task has newer events.Also applies to: 584-587
🤖 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/situation/service.go` around lines 525 - 529, taskReferencePayload currently never sets TaskReferencePayload.LatestEventSeq, so AgentTaskContextPayload constructions (e.g., the local taskContext variable built with contract.AgentTaskContextPayload and the similar review-fallback path) always emit 0; fix by populating LatestEventSeq when building the task reference: either update taskReferencePayload(taskRecord) to set LatestEventSeq from the taskRecord's latest event sequence (e.g., taskRecord.LatestEventSeq or by deriving it from taskRecord.Events) or, after calling taskReferencePayload(taskRecord), assign ref.LatestEventSeq = <taskRecord's latest seq> before putting it into contract.AgentTaskContextPayload/Bundle; ensure the field name LatestEventSeq on contract.TaskReferencePayload is filled in both places that call taskReferencePayload.
🟠 Major comments (19)
internal/daemon/task_event_bridge_notifier.go-14-14 (1)
14-14: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove the bridge notification timeout into config.
This bakes an operator-facing runtime policy into code, so bridge delivery latency/backpressure cannot be tuned without recompiling. Please thread it through config or env and inject it here.
As per coding guidelines, "Never hardcode configuration values in Go code; always read from
config.tomlor environment variables".🤖 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/task_event_bridge_notifier.go` at line 14, Replace the hardcoded const defaultBridgeTerminalNotificationTimeout with a configurable value: add a BridgeTerminalNotificationTimeout (time.Duration) field to your app config (or read BRIDGE_TERMINAL_NOTIFICATION_TIMEOUT from env), parse it as a duration with a sensible fallback of 10s, and inject that value into the component that used defaultBridgeTerminalNotificationTimeout in task_event_bridge_notifier.go (reference the symbol defaultBridgeTerminalNotificationTimeout and the notifier that consumes it). Ensure the config loading code validates/handles missing or malformed durations and that the notifier reads the timeout from the injected config instead of using the hardcoded constant.internal/bridges/task_notifier.go-183-243 (1)
183-243:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPage past ignored records or subscriptions can get stuck forever.
listTaskNotificationRecordsis capped byeventLimit, but the cursor only advances after a delivery. If the first page contains only non-terminal events, or only superseded mismatches before the real terminal event, every sweep re-reads the same prefix and never reaches the later final record. This needs paging/progress semantics that can move past records which are no longer candidates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bridges/task_notifier.go` around lines 183 - 243, The loop can repeatedly re-read the same limited page from listTaskNotificationRecords (capped by eventLimit) when all returned records are non-terminal or mismatched, so change the paging/progress semantics: inside the processing loop over records (after calling listTaskNotificationRecords and while using resolveTerminalTaskNotification and checking terminalTaskNotificationDecisionMismatch/Defer), track the highest record.Sequence seen and, if you exit the loop without delivering (no terminalTaskNotificationDelivered), call n.cursors.Advance with Key=cursorKey and LastSequence=highestSequence (and Now=n.now()) to move the cursor past inspected records; keep existing error handling via recordCursorError and only avoid advancing past records you actually need to reprocess (e.g., on transient errors), ensuring you still call deliverNotification and advance to the delivery’s DeliveryID when delivering.internal/daemon/task_event_bridge_notifier.go-144-168 (1)
144-168:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid doing bridge delivery inline on the task event callback path.
DeliverDueperforms store reads and outbound bridge delivery, andtaskEventObserverFanoutcalls observers serially. A slow or unavailable bridge can therefore hold the task event publisher for up to the timeout on every wake event. This should be offloaded to a bounded worker/queue instead of running inline.🤖 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/task_event_bridge_notifier.go` around lines 144 - 168, The DeliverDue call is running inline on the task event callback path (from taskEventObserverFanout) and can block the publisher; change this to enqueue the record (e.g., the record.Event or its TaskID) onto a bounded channel/worker queue instead of calling o.notifier.DeliverDue directly in the callback; introduce a fixed-size worker pool (goroutines) that read from the queue and invoke o.notifier.DeliverDue with their own context/timeout handling (using context.WithTimeout inside workers) and perform logging on failures, and ensure the enqueue path applies backpressure (blocking or rejecting with metrics/log) when the channel is full so the publisher is not tied to bridge latency. Ensure you remove the inline DeliverDue invocation and related notifyCtx/cancel usage in the callback and wire error handling to the worker-side call.internal/bridges/task_notifier.go-349-358 (1)
349-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact task errors before building the bridge notification.
run.Erroris copied verbatim intoTerminalTaskNotification.Error, and that field is then sent in bothProviderMetadataand the rendered message text. Any raw claim token in the task error can leak over the bridge transport from this path.🔒 Proposed fix
- Error: strings.TrimSpace(run.Error), + Error: taskpkg.RedactClaimTokens(strings.TrimSpace(run.Error)),As per coding guidelines, "Raw
claim_token(agh_claim_*) ... MUST NEVER appear in logs, status APIs, settings views, error payloads, channel messages, SSE, web UI, or memory — use hash forms (claim_token_hash) over the wire".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bridges/task_notifier.go` around lines 349 - 358, The TerminalTaskNotification currently copies run.Error verbatim into TerminalTaskNotification.Error (and that value flows into ProviderMetadata and rendered messages), risking leaking raw claim tokens; before building the notification, run the error through a redaction function (e.g., redactClaimToken or redactSensitiveError) and assign the redacted string to TerminalTaskNotification.Error and any ProviderMetadata/message text fields instead of run.Error; update the code that builds the notification (symbols: TerminalTaskNotification, run.Error, ProviderMetadata, any message rendering code) to use strings.TrimSpace(redactedError) (or equivalent) so non-sensitive parts remain but agh_claim_* tokens are hashed/removed per guidelines.internal/daemon/native_profile_tools.go-66-67 (1)
66-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject conflicting
task_idvalues before persisting the profile.
taskExecutionProfileSet()keys the write by the top-leveltask_id, butprofile()will preferprofile.task_idwhen it is set. A caller can submit two different IDs here and produce a stored payload that no longer matches the record being updated.Suggested fix
func (i *taskExecutionProfileInput) profile(taskID string) taskpkg.ExecutionProfile { - profileTaskID := strings.TrimSpace(i.TaskID) - if profileTaskID == "" { - profileTaskID = taskID - } return taskpkg.ExecutionProfile{ - TaskID: profileTaskID, + TaskID: strings.TrimSpace(taskID), Coordinator: i.Coordinator, Worker: i.Worker, Review: i.Review, Participants: i.Participants, Sandbox: i.Sandbox, } }Also applies to: 111-123
🤖 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_profile_tools.go` around lines 66 - 67, Reject and return an error when the submitted profile contains a different task_id than the top-level taskID before calling SetExecutionProfile: inside taskExecutionProfileSet, after constructing profile via input.Profile.profile(taskID) (or before calling input.Profile.profile if that logic prefers profile.task_id), compare profile.task_id (or profile.TaskID) to the provided taskID and if profile.task_id is non-empty and not equal to taskID, abort and return a clear validation error instead of persisting; keep the existing call to n.deps.Tasks.SetExecutionProfile only when they match.internal/daemon/native_review_tools.go-348-397 (1)
348-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap unbound-review failures to denied, and keep the fallback redacted.
LookupRunReviewForSession()misses satisfyErrRunReviewNotFound, so this switch returns 404 beforeisReviewBindingError()can translate them toReasonSessionDenied. Thedefaultbranch then returns the original error unchanged, which bypasses the redaction already computed inmessage. Missing reviewer bindings should be denied, and unmapped task errors should still surface through a redacted tool error.As per coding guidelines, "Raw
claim_token(agh_claim_*) ... MUST NEVER appear in ... error payloads".🤖 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_review_tools.go` around lines 348 - 397, nativeReviewToolError currently matches taskpkg.ErrRunReviewNotFound in a NotFound branch so isReviewBindingError() never gets to map unbound-review failures to denied, and the default branch returns the raw err instead of a redacted tool error; update nativeReviewToolError by removing ErrRunReviewNotFound from the NotFound/ErrRunReviewNotFound case so that isReviewBindingError (which should keep matching ErrRunReviewNotFound and ErrPermissionDenied) can trigger the Denied mapping, and change the default branch to return a redacted tool error (use toolspkg.NewToolError with the redacted message variable, wrap the original err, and an appropriate ErrorCode like ErrorCodeInternal and a generic reason) so no raw claim tokens are leaked.internal/store/globaldb/global_db_task_claim.go-545-576 (1)
545-576:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlank agent names currently bypass execution-profile routing.
When
criteria.AgentNameis empty, this helper returns before applying the explicitworker_agent_namecheck and the allowed-agent filters. That makes runs with agent-pinned execution profiles claimable by any matching session that has the required capabilities.Suggested fix
func appendProfileClaimFilters( where []string, args []any, criteria taskpkg.ClaimCriteria, ) ([]string, []any) { where, args = appendProfileRequiredCapabilityFilter( where, args, profileRoleWorker, criteria.RequiredCapabilities, ) where, args = appendProfileRequiredCapabilityFilter( where, args, profileRoleParticipant, criteria.RequiredCapabilities, ) agentName := strings.TrimSpace(criteria.AgentName) if agentName == "" { - return where, args + where = append(where, `NOT EXISTS ( + SELECT 1 + FROM task_execution_profiles tep + WHERE tep.task_id = t.id + AND COALESCE(tep.worker_agent_name, '') <> '' + )`) + where = append(where, `NOT EXISTS ( + SELECT 1 + FROM task_profile_agents pa + WHERE pa.task_id = t.id + AND pa.role IN (?, ?) + AND pa.preference = ? + )`) + args = append(args, profileRoleWorker, profileRoleParticipant, profilePreferenceAllowed) + return where, args } where = append(where, `NOT EXISTS ( SELECT 1 FROM task_execution_profiles tep WHERE tep.task_id = t.id🤖 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_task_claim.go` around lines 545 - 576, In appendProfileClaimFilters, remove the early return on empty criteria.AgentName and always apply the execution-profile routing checks: if agentName is empty, append a NOT EXISTS(...) clause that only filters out any tep rows with a non-empty worker_agent_name (i.e. AND COALESCE(tep.worker_agent_name,'') <> '') without binding a ? arg; if agentName is non-empty keep the existing NOT EXISTS(... AND tep.worker_agent_name <> ?) behavior and pass agentName as an arg. After adding the NOT EXISTS clause in both cases, call appendProfileAllowedAgentFilter(...) for profileRoleWorker and profileRoleParticipant (they can handle an empty agentName as needed); reference appendProfileClaimFilters, appendProfileRequiredCapabilityFilter, and appendProfileAllowedAgentFilter to locate the code to modify.internal/daemon/task_runtime.go-320-333 (1)
320-333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t make missing reentry support a daemon-wide boot failure.
bootTasksalready treats a missingtaskStoreas a feature downgrade, but this new path makes the weakerharnessReentryStore/harnessReentrySessionManagerchecks fatal. That means a registry or session manager that can still execute tasks will now stop the whole daemon at startup just because synthetic reentry is unavailable. Consider treating “reentry unsupported” as an opt-out and only returning an error for real construction failures.Also applies to: 374-392
🤖 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/task_runtime.go` around lines 320 - 333, bootHarnessReentryBridge's unsupported/missing-reentry case should not abort daemon boot: change the error handling around bootHarnessReentryBridge(ctx, state) so that you only return a fatal error for real failures and treat an "unsupported" (or sentinel ErrReentryUnsupported / nil-reentry) response as non-fatal — set reentry to nil and continue creating the task manager (taskpkg.NewManager via taskManagerOptions and d.composeTaskEventObserver) so tasks can run without synthetic reentry; apply the same non-fatal handling to the other occurrence of bootHarnessReentryBridge referenced in the later block (lines ~374-392).internal/api/spec/spec.go-4469-4475 (1)
4469-4475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the default review-policy enum value too.
The schema migration in this PR defaults
tasks.review_policyto"none", but this enum only advertises the routed policies. Clients generated from the OpenAPI document will reject the persisted default, and the spec no longer round-trips existing task data correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/spec/spec.go` around lines 4469 - 4475, The OpenAPI enum returned by taskReviewPolicyValues() omits the default "none" value; update the function to include the persisted default by adding string(taskpkg.ReviewPolicyNone) alongside the existing values (string(taskpkg.ReviewPolicyAlways), string(taskpkg.ReviewPolicyOnSuccess), string(taskpkg.ReviewPolicyOnFailure)) so generated clients accept the database default and existing task records round-trip correctly.internal/daemon/review_router.go-473-475 (1)
473-475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't exclude every reviewer that shares the worker's agent name.
Self-review is already prevented by session ID / peer ID. The extra agent-name exclusion here rejects distinct sessions running the same agent definition and even prevents spawning a second reviewer session for that agent. In a workspace that only has one reviewer-capable agent type, reviews become unroutable even when separate worker instances exist.
Suggested fix
- if strings.TrimSpace(candidate) == strings.TrimSpace(original.agentName) { - continue - } - ok, err := r.agentHasCapabilities(ctx, resolved, candidate, review.RequiredCapabilities) if err != nil { if errors.Is(err, workspacepkg.ErrAgentNotAvailable) { continue @@ - if strings.TrimSpace(original.agentName) != "" && resolved != nil { + if resolved != nil { for _, agent := range sortedResolvedAgents(resolved) { - if strings.TrimSpace(agent.Name) == "" || strings.TrimSpace(agent.Name) == original.agentName { + if strings.TrimSpace(agent.Name) == "" { continue } return strings.TrimSpace(agent.Name), "", nil } - return "", "original worker is the only resolvable reviewer agent", nil } return "", "", nil } @@ if strings.TrimSpace(original.sessionID) != "" && strings.TrimSpace(info.ID) == original.sessionID { return true } - if strings.TrimSpace(original.peerID) != "" && reviewRouterPeerID(info) == original.peerID { - return true - } - return strings.TrimSpace(original.agentName) != "" && - strings.TrimSpace(info.AgentName) == strings.TrimSpace(original.agentName) + return strings.TrimSpace(original.peerID) != "" && reviewRouterPeerID(info) == original.peerID }Also applies to: 493-500, 600-611
🤖 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/review_router.go` around lines 473 - 475, The code currently excludes any reviewer whose candidate string equals original.agentName (strings.TrimSpace(candidate) == strings.TrimSpace(original.agentName)), which incorrectly blocks distinct sessions running the same agent; remove that agent-name exclusion and instead ensure self-review is prevented using the existing session/peer identity checks (e.g., compare candidateSessionID or candidatePeerID against original.sessionID/original.peerID). Update the reviewer-filtering logic in the functions handling candidate selection (references: candidate, original.agentName, original.sessionID/original.peerID) so same agentName is allowed when the session/peer IDs differ, and drop the agentName equality check from the filter.internal/daemon/review_router.go-232-252 (1)
232-252:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid orphaning reviewer sessions when bind fails.
Createhappens beforeBindRunReviewSession. If the bind step conflicts or errors after the session is created, the new reviewer session stays alive with no review attached. Duplicate notifications and transient store errors will leak system reviewer sessions over time.🤖 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/review_router.go` around lines 232 - 252, Create must not orphan sessions if binding fails: after calling r.sessions.Create (created / info), ensure you clean up the newly created session when r.tasks.BindRunReviewSession returns an error. Concretely, capture created.Info().ID (or info.ID) and on any bind error call the session teardown API (e.g. r.sessions.Delete(ctx, info.ID) or the project's equivalent) before returning; implement this as a deferred cleanup that only runs on error if created was newly created so existing sessions are not deleted.internal/daemon/review_router.go-165-199 (1)
165-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetach review routing from request cancellation.
This callback performs daemon-owned work, but it keeps the caller's context all the way through reviewer selection, session creation, and diagnostic recording. A client disconnect or request timeout can cancel the review before it is bound, leaving it stuck in the requested state.
Suggested fix
func (r *reviewRouter) OnRunReviewRequested( ctx context.Context, notification *taskpkg.RunReviewRequestedNotification, ) { if r == nil || notification == nil { return } if ctx == nil { ctx = context.Background() + } else { + detached := context.WithoutCancel(ctx) + if deadline, ok := ctx.Deadline(); ok { + var cancel context.CancelFunc + detached, cancel = context.WithDeadline(detached, deadline) + defer cancel() + } + ctx = detached } if strings.TrimSpace(notification.Review.ReviewID) == "" { return }As per coding guidelines, "Detached execution lifetime — work that outlives an HTTP/UDS request must detach via
context.WithoutCancel(ctx), never tie execution lifetime to request lifetime" and "context.WithoutCanceldoes NOT preserve deadlines — re-attach a deadline if needed".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/review_router.go` around lines 165 - 199, The OnRunReviewRequested handler currently passes the incoming request context through daemon work (routeRunReview, recordNoRouteDiagnostic) so a client cancel can abort background processing; detach the execution by calling context.WithoutCancel(ctx) at the start of the handler (e.g., replace uses of the original ctx with a detached context variable) and if the incoming ctx had a deadline re-attach it to the detached context using context.WithDeadline/WithTimeout before calling routeRunReview or recordNoRouteDiagnostic; ensure subsequent calls (routeRunReview, recordNoRouteDiagnostic and any session creation helpers) use the detached-and-deadline-reattached context so the work outlives the request while still honoring any original deadline.internal/situation/task_context.go-13-17 (1)
13-17: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftBreak the
internal/situation→internal/apidependency.
renderTaskBundlePromptnow pullscontract.AgentContextPayloadintointernal/situation, which inverts the layering this repo explicitly forbids. Please move the render DTO/helper belowinternal/api, or make the API layer adapttaskpkg.ContextBundlebefore callingRenderPrompt.As per coding guidelines,
Packages under internal/ must not import from daemon/, api/, or cli/ — dependencies flow downward only toward the composition root.Also applies to: 443-450
🤖 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/situation/task_context.go` around lines 13 - 17, The file internal/situation/task_context.go introduces a forbidden upward dependency by using contract.AgentContextPayload inside renderTaskBundlePrompt; refactor so internal/situation no longer imports internal/api: either move the render DTO/helper (the code that constructs/uses AgentContextPayload) down into internal/api and keep renderTaskBundlePrompt working there, or change the caller in the API layer to convert taskpkg.ContextBundle into the required DTO and then call RenderPrompt (i.e., have RenderPrompt accept taskpkg.ContextBundle or a neutral interface). Locate usages around renderTaskBundlePrompt, contract.AgentContextPayload, taskpkg.ContextBundle and RenderPrompt and apply the conversion at the API boundary (or relocate the helper) so internal/situation stops importing internal/api.internal/situation/task_context.go-413-439 (1)
413-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t return an over-budget bundle as success.
The
defaultbranch exits even whentaskContextOverBudgetis still true. If the remaining fields alone exceedmaxBytes, callers still get an oversized bundle and the configured context cap is no longer enforced.Suggested fix
default: - return bundle, nil + return taskpkg.ContextBundle{}, fmt.Errorf( + "situation: task context bundle exceeds %d bytes after trimming", + maxBytes, + ) } } }🤖 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/situation/task_context.go` around lines 413 - 439, The loop in the reducer incorrectly returns the current (still-overbudget) bundle in the default branch; instead, when no more fields can be trimmed (default) and taskContextOverBudget(bundle, maxBytes) is still true, return an explicit error rather than bundle success. Update the loop in task_context.go (the for loop that calls taskContextOverBudget) so that the default branch does not return bundle, nil; it should check (or assume) over-budget and return an error (e.g., fmt.Errorf or a package error) indicating the bundle cannot be reduced under maxBytes; keep existing trimming logic for RecentEvents, PriorAttempts, ReviewHistory, HandoffSummary, and ReviewContinuation/NextRoundGuidance and use truncateUTF8Bytes as before.internal/situation/task_context.go-122-130 (1)
122-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify that
runIDbelongs totaskID.
GetTaskandGetTaskRunare loaded independently here and then combined without checkingrun.TaskID == taskRecord.ID. A mismatched pair will render task A with run B data and can leak the wrong run context.Suggested fix
taskRecord, err := store.GetTask(ctx, strings.TrimSpace(taskID)) if err != nil { return "", err } run, err := store.GetTaskRun(ctx, strings.TrimSpace(runID)) if err != nil { return "", err } +if strings.TrimSpace(run.TaskID) != strings.TrimSpace(taskRecord.ID) { + return "", fmt.Errorf( + "%w: run %q does not belong to task %q", + taskpkg.ErrValidation, + strings.TrimSpace(run.ID), + strings.TrimSpace(taskRecord.ID), + ) +} return s.TaskRunPromptOverlay(ctx, taskRecord, run, nil)🤖 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/situation/task_context.go` around lines 122 - 130, The code fetches taskRecord via GetTask and run via GetTaskRun and then calls TaskRunPromptOverlay without ensuring the run belongs to the task; add a validation after loading run (in the function containing the current snippet) to check that run.TaskID == taskRecord.ID and return a clear error if it does not, so TaskRunPromptOverlay is only invoked for matched task/run pairs (referencing taskRecord, run, GetTask, GetTaskRun, and TaskRunPromptOverlay).internal/store/globaldb/global_db_task_profile.go-72-85 (1)
72-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways bump
updated_aton a successful upsert.A normal read-modify-write flow reuses the previously loaded timestamp here, so profile changes can be persisted without changing
updated_at, which breaks change detection and ordering for consumers.Suggested fix
if found { normalized.CreatedAt = existing.CreatedAt } if normalized.CreatedAt.IsZero() { normalized.CreatedAt = now } - if normalized.UpdatedAt.IsZero() { - normalized.UpdatedAt = now - } + normalized.UpdatedAt = now🤖 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_task_profile.go` around lines 72 - 85, The current upsert flow preserves the previously loaded UpdatedAt (via existing.CreatedAt reuse logic), allowing updates to go through without bumping updated_at; change the logic in the upsert path (around loadExecutionProfile, the normalized.CreatedAt/normalized.UpdatedAt handling and g.now()) so that: if found keep existing.CreatedAt, otherwise set normalized.CreatedAt = now, and always set normalized.UpdatedAt = now unconditionally before persisting; this ensures UpdatedAt is bumped on every successful upsert while preserving CreatedAt when present.internal/api/core/tasks.go-337-359 (1)
337-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip server-managed fields from execution-profile writes.
This endpoint binds directly into
taskpkg.ExecutionProfile, and the helper only normalizesTaskID. The manager later preserves a non-zeroCreatedAt, so callers can backdate or otherwise forge immutable profile metadata through the public API. Please switch to a dedicated request DTO, or at minimum clear the server-owned fields here before handing the object off.💡 Minimal mitigation
func taskExecutionProfileFromRequest( taskID string, req *contract.SetTaskExecutionProfileRequest, ) (*taskpkg.ExecutionProfile, error) { trimmedID := strings.TrimSpace(taskID) if trimmedID == "" { return nil, NewTaskValidationError(errors.New("task id is required")) } if req == nil { return nil, NewTaskValidationError(errors.New("task execution profile request is required")) } if strings.TrimSpace(req.TaskID) != "" && strings.TrimSpace(req.TaskID) != trimmedID { return nil, NewTaskValidationError(fmt.Errorf( "task_execution_profile.task_id must match task id %q", trimmedID, )) } req.TaskID = trimmedID + req.CreatedAt = time.Time{} + req.UpdatedAt = time.Time{} return req, nil }Also applies to: 1537-1555
🤖 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/tasks.go` around lines 337 - 359, The request currently binds into taskpkg.ExecutionProfile via taskExecutionProfileFromRequest and then passes the result to manager.SetExecutionProfile, allowing clients to supply server-managed fields (e.g., CreatedAt) that get preserved; update the handler that processes contract.SetTaskExecutionProfileRequest (the req variable and the call-site around taskExecutionProfileFromRequest and manager.SetExecutionProfile) to either use a dedicated request DTO or explicitly zero/clear all server-owned fields (CreatedAt, CreatedBy, IDs, immutable metadata) on the produced profile before calling manager.SetExecutionProfile so the manager cannot be tricked into persisting client-controlled server-managed values.internal/situation/service.go-870-873 (1)
870-873:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
run.CoordinationChannelIDauthoritative.Leading with
metadata["coordination_channel_id"]lets duplicated JSON override the structured field on the run. If those ever diverge, the session context can point at the wrong coordination channel and filter the wrong inbox/peer set.💡 Suggested change
- channelID := firstTrimmed(metadata["coordination_channel_id"], run.CoordinationChannelID, run.NetworkChannel) + channelID := firstTrimmed(run.CoordinationChannelID, metadata["coordination_channel_id"], run.NetworkChannel)As per coding guidelines, "Authoritative primitives are exclusive — when a primitive owns a state transition (
task.Service.ClaimNextRun,Spawn,EnsureMigration), no peer package may replicate it".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/situation/service.go` around lines 870 - 873, The code in coordinationChannelPayload is using metadata["coordination_channel_id"] before the structured run.CoordinationChannelID, which allows replicated JSON to override the authoritative run value; change the selection order so run.CoordinationChannelID is chosen first (i.e., compute channelID using run.CoordinationChannelID then metadata["coordination_channel_id"] then run.NetworkChannel) while leaving channelName logic (firstTrimmed(run.NetworkChannel, channelID)) intact; update the assignment in coordinationChannelPayload to reflect this authoritative ordering so the session context always uses run.CoordinationChannelID when present.internal/cli/client.go-2630-2638 (1)
2630-2638:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd nil checks for pointer request parameters before marshaling.
Lines 2633, 2654, 2711, and 2751 accept
*...Requestpointer parameters and pass them throughdoJSON, which marshals usinginterface{}. In Go, a typed nil pointer stored in an interface{} compares non-nil, sojson.Marshalwill encode it as JSONnullinstead of rejecting it at the client. Add a nil check at the entry of each method to fail fast.Suggested fix
func (c *unixSocketClient) SetTaskExecutionProfile( ctx context.Context, id string, request *TaskExecutionProfileRequest, ) (TaskExecutionProfileRecord, error) { + if request == nil { + return TaskExecutionProfileRecord{}, errors.New("cli: task execution profile request is required") + } var response contract.TaskExecutionProfileResponse path := taskExecutionProfilePath(id) if err := c.doJSON(ctx, http.MethodPut, path, nil, request, &response); err != nil { return TaskExecutionProfileRecord{}, err } @@ func (c *unixSocketClient) CreateTaskBridgeNotificationSubscription( ctx context.Context, taskID string, request *TaskBridgeNotificationSubscriptionRequest, ) (TaskBridgeNotificationSubscriptionRecord, error) { + if request == nil { + return TaskBridgeNotificationSubscriptionRecord{}, errors.New("cli: task bridge notification subscription request is required") + } var response contract.TaskBridgeNotificationSubscriptionResponse path := taskBridgeNotificationSubscriptionsPath(taskID) if err := c.doJSON(ctx, http.MethodPost, path, nil, request, &response); err != nil { return TaskBridgeNotificationSubscriptionRecord{}, err } @@ func (c *unixSocketClient) RequestTaskRunReview( ctx context.Context, runID string, request *TaskRunReviewRequest, ) (TaskRunReviewRequestRecord, error) { + if request == nil { + return TaskRunReviewRequestRecord{}, errors.New("cli: task run review request is required") + } var response contract.TaskRunReviewRequestResponse path := "/api/task-runs/" + url.PathEscape(strings.TrimSpace(runID)) + "/reviews" if err := c.doJSON(ctx, http.MethodPost, path, nil, request, &response); err != nil { return TaskRunReviewRequestRecord{}, err } @@ func (c *unixSocketClient) SubmitTaskRunReviewVerdict( ctx context.Context, reviewID string, request *TaskRunReviewVerdictRequest, ) (TaskRunReviewVerdictRecord, error) { + if request == nil { + return TaskRunReviewVerdictRecord{}, errors.New("cli: task run review verdict request is required") + } var response contract.TaskRunReviewVerdictResponse path := "/api/task-reviews/" + url.PathEscape(strings.TrimSpace(reviewID)) + "/verdict" if err := c.doJSON(ctx, http.MethodPost, path, nil, request, &response); err != nil { return TaskRunReviewVerdictRecord{}, 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/cli/client.go` around lines 2630 - 2638, Add a nil-check at the start of SetTaskExecutionProfile and the other unixSocketClient methods that accept pointer request parameters (the ones flagged at lines 2654, 2711 and 2751) to fail fast if the caller passed a nil typed pointer; detect if request == nil and return a clear non-nil error immediately instead of letting doJSON marshal a typed nil into JSON null (doJSON takes interface{} and json.Marshal will encode typed nil as null).
🟡 Minor comments (7)
internal/extension/host_api_test.go-4958-4974 (1)
4958-4974:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid a hard 2s wall-clock timeout in this cleanup poll.
A fixed 2-second budget is easy to trip under
-raceor a busy CI runner, even when the prompts would settle shortly after. Reuse the cleanup context deadline here instead of a separate short constant.Suggested fix
-func waitForHostAPIPromptsToSettle( - t testing.TB, - manager *session.Manager, - sessions []*session.Info, -) bool { +func waitForHostAPIPromptsToSettle( + ctx context.Context, + t testing.TB, + manager *session.Manager, + sessions []*session.Info, +) bool { t.Helper() if manager == nil || len(sessions) == 0 { return true } - deadline := time.Now().Add(2 * time.Second) for { allSettled := true for _, info := range sessions { if info != nil && manager.IsPrompting(info.ID) { allSettled = false break } } if allSettled { return true } - if time.Now().After(deadline) { - t.Errorf("timed out waiting for host API prompt cleanup") + if err := ctx.Err(); err != nil { + t.Errorf("timed out waiting for host API prompt cleanup: %v", err) return false } time.Sleep(5 * time.Millisecond) } }As per coding guidelines,
Run tests with -race flag and ensure CGO_ENABLED=1 for race-sensitive packages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/extension/host_api_test.go` around lines 4958 - 4974, The polling loop uses a hard 2s deadline; change it to reuse the existing cleanup context deadline instead: get the deadline via cleanupCtx.Deadline() (or ctx.Deadline()) and use that deadline in the loop that checks manager.IsPrompting(info.ID) over sessions; if the context has no deadline, fall back to a sensible longer timeout (or fail fast) and preserve the same sleep interval and error message ("timed out waiting for host API prompt cleanup"). This ensures the loop respects the test's cleanup context instead of a fixed 2s wall-clock timer.internal/api/core/tasks_test.go-247-269 (1)
247-269:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert response bodies on the new negative and 204 cases.
These additions introduce several status-only checks. That leaves the response contract untested on the paths most likely to drift — especially the 400/404 JSON payloads and the 204 empty-body cases. Decode
contract.ErrorPayloadon the error branches and assert an empty body for the204responses.As per coding guidelines "Always assert both HTTP status code AND response body in tests; status-code-only assertions are insufficient".
Also applies to: 447-512, 763-780
🤖 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/tasks_test.go` around lines 247 - 269, The test currently only checks status codes for several requests (e.g., the DELETE and the PUT to "/tasks/task-1/execution-profile" around the performRequest calls that set/delete execution profiles and the deletedTaskID assertion) but omits validating the response bodies; update these assertions to decode and assert response bodies: for error branches (HTTP 400/404) unmarshal resp.Body into contract.ErrorPayload and assert expected error fields/messages, and for HTTP 204 responses assert that resp.Body is empty (zero length) or exactly an empty string. Apply the same pattern to the other similar blocks referenced (near lines 447-512 and 763-780) so every test asserts both status code and response body content.internal/session/query_test.go-595-616 (1)
595-616:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the finalization wait assertion deterministic.
The
time.After(50 * time.Millisecond)window can pass even ifopenQueryRecordernever waits; the goroutine may simply not be scheduled before the timeout. The final assertion also only compares event counts. Add a handshake that confirms the goroutine has entered the call before unblocking finalization, then compare event sequences/IDs againstactiveEventsso this test fails on premature returns instead of scheduler variance.Also applies to: 631-636
🤖 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/session/query_test.go` around lines 595 - 616, The test's timing is flaky because it relies on a short sleep; modify the goroutine that calls h.manager.openQueryRecorder so it uses a handshake channel (e.g., startCh) to signal when it has entered the Query call before the test attempts to finalize/cleanup, and have the test wait on that handshake instead of time.After; then, instead of only asserting len(result.events), compare the returned events' sequence/IDs from got.Query against the expected activeEvents slice (or their IDs) to ensure the recorder truly waited and returned the correct sequence; update both the block using resultCh/queryResult and the similar block at lines 631-636 to use the same handshake and sequence comparison with openQueryRecorder, Query, cleanup, resultCh, and activeEvents.internal/cli/task.go-824-845 (1)
824-845:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative review rounds and attempts in the CLI.
buildTaskRunReviewRequestcurrently passes negative--roundand--attemptvalues through unchanged. That turns obvious caller mistakes into server-side validation failures instead of immediate CLI feedback.Suggested fix
func buildTaskRunReviewRequest( runID string, reasonRaw string, policyRaw string, round int, attempt int, parentID string, ) (*TaskRunReviewRequest, error) { + if round < 0 { + return nil, errors.New("cli: --round must be zero or positive") + } + if attempt < 0 { + return nil, errors.New("cli: --attempt must be zero or positive") + } policy, err := parseOptionalReviewPolicy(policyRaw) if err != nil { return nil, 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/cli/task.go` around lines 824 - 845, Validate that the incoming round and attempt values are non-negative in buildTaskRunReviewRequest; if round < 0 or attempt < 0 return an error immediately (rather than building a request), e.g. check the round and attempt parameters at the top of buildTaskRunReviewRequest and return a descriptive error when negative; keep the rest of the function unchanged (references: buildTaskRunReviewRequest, TaskRunReviewRequest, round, attempt).internal/cli/task.go-658-676 (1)
658-676:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
--lastfor notification listing.This builder forwards negative limits to the API, while the other list commands in this file reject them locally.
agh task notification list --last -1should fail fast in the CLI instead of producing downstream behavior that depends on the server implementation.Suggested fix
func buildTaskBridgeNotificationSubscriptionListQuery( bridgeInstanceID string, scopeRaw string, workspaceID string, last int, ) (TaskBridgeNotificationSubscriptionQuery, error) { + if err := validateTaskLast(last); err != nil { + return TaskBridgeNotificationSubscriptionQuery{}, err + } query := TaskBridgeNotificationSubscriptionQuery{ BridgeInstanceID: strings.TrimSpace(bridgeInstanceID), WorkspaceID: strings.TrimSpace(workspaceID), Limit: last, }🤖 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/task.go` around lines 658 - 676, The buildTaskBridgeNotificationSubscriptionListQuery function currently accepts a negative last and forwards it; add validation at the start of this function to reject negative limits by checking the last parameter (e.g., if last < 0) and return a descriptive error (e.g., fmt.Errorf("cli: invalid --last value: must be >= 0")). Keep the existing setting of query.Limit when valid and reference the function name buildTaskBridgeNotificationSubscriptionListQuery and the last parameter when making the change so the CLI fails fast like the other list commands.internal/situation/service_test.go-1214-1236 (1)
1214-1236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSort the stubbed reviews before applying
Limit.Go map iteration is randomized, so slicing the last
Limitentries froms.reviewsmakes any multi-review assertion non-deterministic. Sort the collected slice by a stable field before trimming it to keep these context-bundle tests reliable.🤖 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/situation/service_test.go` around lines 1214 - 1236, The ListRunReviews stub collects reviews from s.reviews in map-random order then trims the last query.Limit items which makes tests nondeterministic; after building the reviews slice and before applying the Limit branch, sort reviews deterministically (use sort.SliceStable) with a stable comparator over review fields such as TaskID, RunID, ReviewerSessionID and Status (compare those strings in that order) so that trimming (reviews = reviews[len(reviews)-query.Limit:]) yields stable, predictable results.internal/store/globaldb/global_db_notification_cursor.go-124-137 (1)
124-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIdempotent advances should still clear stale diagnostics.
If
RecordCursorErrorsetslast_erroron the current delivery, replaying that same confirmed delivery hits this fast path and returns without clearing the error or refreshingupdated_at. The cursor can stay stuck in an error state until a newer sequence arrives.Suggested fix
if err := validateNotificationCursorAdvance(current, normalized); err != nil { return notifications.Cursor{}, err } if current.LastSequence == normalized.LastSequence && - current.LastDeliveryID == normalized.DeliveryID { + current.LastDeliveryID == normalized.DeliveryID && + current.LastError == "" { if _, err = conn.ExecContext(ctx, "COMMIT"); err != nil { return notifications.Cursor{}, fmt.Errorf( "store: commit idempotent notification cursor advance: %w", err, ) } finished = true return current, nil } cursor, err = updateNotificationCursor(ctx, conn, normalized)🤖 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_notification_cursor.go` around lines 124 - 137, The idempotent-advance fast path returns early without clearing any stale delivery diagnostics set by RecordCursorError; update the code in the block where current.LastSequence == normalized.LastSequence && current.LastDeliveryID == normalized.DeliveryID to clear current.LastError (set last_error = NULL) and refresh current.UpdatedAt (set to now) before committing so replay clears stale errors; implement this by executing an UPDATE via conn.ExecContext within the same transaction (use the same ctx/conn and then call the existing conn.ExecContext(ctx, "COMMIT") as before) and then return the updated current cursor. Ensure you reference the existing variables/current state (current, normalized) and keep the change inside the same transaction that uses conn.ExecContext.
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
internal/daemon/task_event_bridge_notifier_test.go (1)
461-469: 💤 Low valueConsider thread-safe counter if fanout becomes concurrent.
The
countfield is incremented without synchronization. While the current fanout implementation appears to call observers synchronously (making this safe), consider usingatomic.Int32for future-proofing if the fanout ever becomes concurrent.♻️ Optional: Use atomic counter
+import "sync/atomic" + type recordingTaskEventObserver struct { - count int + count atomic.Int32 } func (o *recordingTaskEventObserver) OnTaskEvent(context.Context, taskpkg.EventRecord) { - o.count++ + o.count.Add(1) }🤖 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/task_event_bridge_notifier_test.go` around lines 461 - 469, The recordingTaskEventObserver uses an unsynchronized int field `count` incremented in `OnTaskEvent`, which is unsafe if observers are ever invoked concurrently; replace `count int` with an atomic counter (e.g., atomic.Int32 or an int32 accessed via sync/atomic) and update `OnTaskEvent` to increment the atomic value and read it using the atomic load/store APIs so increments and reads are safe across goroutines.internal/bridges/task_notifier.go (1)
237-262: ⚡ Quick winConfusing function signature with two separate error return values.
The return signature
(terminalTaskNotificationDecision, error, int64, error)with two distinct error semantics (diagnostic at position 2, processing error at position 4) is non-idiomatic and error-prone for callers. Consider wrapping both into a single result struct:type processRecordResult struct { decision terminalTaskNotificationDecision diagnostic error sequence int64 }Then return
(processRecordResult, error)where the error is the processing failure.♻️ Suggested refactor
+type processRecordResult struct { + decision terminalTaskNotificationDecision + diagnostic error + sequence int64 +} + func (n *TerminalTaskNotifier) processTaskNotificationRecord( ctx context.Context, cursorKey notifications.CursorKey, subscription BridgeTaskSubscription, record taskpkg.EventRecord, -) (terminalTaskNotificationDecision, error, int64, error) { +) (processRecordResult, error) { if !isTerminalTaskNotificationCandidate(record.Event.EventType) { - return terminalTaskNotificationDecisionIgnore, nil, record.Sequence, nil + return processRecordResult{decision: terminalTaskNotificationDecisionIgnore, sequence: record.Sequence}, nil } // ... update remaining return sites similarly🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bridges/task_notifier.go` around lines 237 - 262, The function processTaskNotificationRecord has a confusing non-idiomatic signature returning two separate error values (terminalTaskNotificationDecision, error, int64, error); refactor by introducing a single result struct (e.g. processRecordResult { decision terminalTaskNotificationDecision; diagnostic error; sequence int64 }) and change the function signature to return (processRecordResult, error) where the single error is the processing failure and diagnostic lives in the result; update processTaskNotificationRecord to populate and return the struct in all branches (including mapping record.Sequence into sequence, placing resolveTerminalTaskNotification diagnostic into diagnostic, and folding recordCursorError into the returned error), and update all callers to use the new struct fields instead of positional errors.internal/situation/service_test.go (1)
96-104: ⚡ Quick winMake the channel-source precedence explicit in this fixture.
These changes now feed three different channel identifiers into the same scenario (
CoordinationChannelID,NetworkChannel, and metadatacoordination_channel_id), but the test never pins which oneContextForSessionis supposed to surface. That means a regression can read the wrong source and still pass. Either collapse these to one canonical value here or add an explicit assertion onpayload.CoordinationChannel.Channel.ID.As per coding guidelines, "Ensure tests verify behavior outcomes, not just function calls."
Also applies to: 143-145, 150-168
🤖 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/situation/service_test.go` around lines 96 - 104, The fixture feeds three different channel identifiers (CoordinationChannelID, NetworkChannel, and metadata coordination_channel_id) but the test doesn't assert which source ContextForSession should prefer; update the test to make precedence explicit by either collapsing the three fields to the same canonical ID or adding an assertion on payload.CoordinationChannel.Channel.ID to expect the chosen source; locate the session construction that sets CoordinationChannelID, NetworkChannel, and Metadata (jsonRaw) and then modify the fixture or add an assertion after calling ContextForSession to verify payload.CoordinationChannel.Channel.ID equals the intended canonical value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/bridges/task_notifier.go`:
- Around line 591-597: The current truncateTerminalTaskCursorError truncates by
byte index and can split multi-byte UTF-8 runes; update
truncateTerminalTaskCursorError to perform rune-safe truncation by iterating the
string with a for-range to get rune start byte indices and cut at the last rune
boundary that does not exceed maxTerminalTaskCursorErrorBytes (use the range
index as the slice boundary). Ensure you still TrimSpace at the start and return
the full trimmed string if its byte length is within the limit, otherwise return
trimmed[:safeCutIndex].
In `@internal/cli/task.go`:
- Around line 1830-1840: In missingWorkFromFlags, after
parseJSONFlag("missing-work-json", raw) returns payload, validate that payload
is a JSON array (not an object or scalar) before returning; e.g. unmarshal
payload into []interface{} (or check trimmed first rune is '[' and json.Valid)
and return a clear error like "cli: --missing-work-json must be a JSON array" if
it fails, so that missingWorkFromFlags enforces the flag help promise.
- Around line 893-920: parseTaskRunReviewListFilters currently rejects requests
when both taskID and runID are empty, which prevents reviewer/status-only
queries; update the validation so it still errors if both taskID and runID are
provided, but allows both to be empty when the caller supplied a status or a
reviewerSessionID (i.e. permit global queries like "my pending reviews"). To
implement: in parseTaskRunReviewListFilters, call parseOptionalReviewStatus and
trim reviewerSessionID early, then change the check that enforces a non-empty
task or run to only trigger when status and reviewerSessionID are both empty;
keep validateTaskLast and the existing return structure (TaskID, RunID, Status,
ReviewerSessionID, Limit) unchanged and ensure you still trim inputs when
building the TaskRunReviewListQuery.
In `@internal/daemon/review_router.go`:
- Around line 296-329: The current originalWorkerIdentity fills agentName but
callers ignore it, so update isOriginalWorker and selectCreateAgent to consider
original.agentName (and peerID) when determining exclusions: have
isOriginalWorker check both sessionID and agentName/peerID matches (use
originalWorkerIdentity returned fields) and change selectCreateAgent to preserve
and pass through the original identity instead of discarding it so selection
logic can exclude any reviewer sessions or newly-created self-review sessions
whose agentName or peerID equals the original; adjust comparisons to use
strings.TrimSpace on agentName and use reviewRouterPeerID/peerID comparisons
consistently.
- Around line 173-177: After calling detachDaemonOwnedContext(ctx) re-attach a
deadline (e.g., use context.WithTimeout or context.WithDeadline) before passing
the context into routeRunReview and recordNoRouteDiagnostic so those operations
aren't unbounded; update the call site that currently does ctx =
detachDaemonOwnedContext(ctx) to create a new timedCtx from that detached
context and use timedCtx for routed, diagnostic, err := r.routeRunReview(...)
and any subsequent store/task calls. For cleanupCreatedReviewerSession do not
wrap a timeout context with WithoutCancel (i.e., create the 5s timeout directly
from the detached context or the original request context and call StopWithCause
with that timed context) so the StopWithCause call retains its deadline;
reference detachDaemonOwnedContext, routeRunReview, recordNoRouteDiagnostic,
cleanupCreatedReviewerSession, and StopWithCause when applying these changes.
In `@internal/session/query_test.go`:
- Line 557: Rename the t.Run subtest whose current title starts with "finalizing
active session waits before reading a closed recorder handle" to follow the
repository convention using the "Should ..." pattern (for example: t.Run("Should
wait for finalization before reopening stored events", func(t *testing.T) { ...
})), updating the test title string in the t.Run call in
internal/session/query_test.go so it matches other subtests and the required
naming convention.
- Around line 595-623: The current test only closes started before launching the
goroutine so the immediate select can pass even if openQueryRecorder never began
waiting; modify the goroutine around h.manager.openQueryRecorder(session.ID) to
signal a new synchronization channel (e.g., enteredWait or waitingStarted) after
openQueryRecorder has entered its wait path (or right after calling
openQueryRecorder but before blocking) and have the main test block on that
channel before performing the non-blocking select; alternatively give the main
test a short bounded window (using a time.After deadline) to allow the goroutine
to return before asserting it is blocked—ensure you reference the existing
started/resultCh/openQueryRecorder variables and still send the final result on
resultCh and call cleanup as before.
In `@internal/situation/service_test.go`:
- Around line 377-379: The current test only checks that
bundle.ReviewContinuation.Reason is not "review_rejected"; update the assertion
to verify the redacted reviewer reason positively by asserting the exact
expected redacted string or at minimum assert that
bundle.ReviewContinuation.Reason contains the redaction marker (e.g.,
"[REDACTED]") and does not contain the raw reviewer token; locate the assertion
around the ReviewContinuation.Reason check in service_test.go and replace the
negative equality check with a positive containment/exclusion check against the
redaction marker and the raw token.
- Around line 477-480: The test for TaskRunPromptOverlayByID only asserts the
sentinel error (taskpkg.ErrValidation) and can mask which validation branch
failed; update the assertions to pin the failing reason by keeping the errors.Is
check and additionally assert the error message contains a stable identifying
substring (e.g., "mismatched run" for the mismatched run path and "oversized
bundle" for the oversized-bundle path) so the test verifies both the sentinel
and the specific failure; apply this to the TaskRunPromptOverlayByID case using
mismatchedRun and taskRecord.ID and similarly update the other failing assertion
block at the referenced lines to use ErrorContains (or errors.As plus message
substring) to ensure the exact validation branch is tested.
In `@internal/situation/service.go`:
- Around line 507-510: The bundle assembly via s.bundleForRun is currently
treated as fatal and returns an error from
ContextForSession/reviewBindingTaskAndChannelContext; change it to optional
enrichment: if s.bundleForRun returns an error, log the error (using the
existing logger on the service, e.g., s.logger or similar) and continue building
the session context with an empty/nil bundle instead of returning the error, so
task/channel context remains available; apply this fallback pattern to the other
occurrences called out (the calls inside reviewBindingTaskAndChannelContext and
the other spots at the indicated contexts) so bundle failures don't abort the
overall session-context build.
In `@internal/situation/task_context.go`:
- Around line 654-675: redactTaskContextJSONValue currently only drops the
literal "claim_token" map key; update it to strip any structured secret-bearing
keys before recursing (e.g. keys that equal "claim_token" case‑insensitive, that
have prefix "agh_claim_", "mcp_auth_token", or that match names like
"oauth_code", "pkce_verifier", "secret_binding" or common secret suffixes like
"_secret") so their values are omitted entirely and not traversed; keep using
taskpkg.RedactClaimTokens for free-form strings, but for map keys detect these
patterns with strings.EqualFold/strings.HasPrefix/strings.HasSuffix and skip
adding those entries to cleaned so they never appear in the returned structure.
In `@internal/store/globaldb/global_db_notification_cursor.go`:
- Around line 205-235: RecordCursorError currently does an ExecContext to write
the error and then calls loadNotificationCursor in a separate statement,
allowing AdvanceCursor/ResetCursor to interleave; make the write-and-read atomic
by performing both operations on the same DB transaction/connection. Modify
RecordCursorError to begin a transaction or acquire a single connection (using
BeginTx/Conn) and run the INSERT ... ON CONFLICT and the subsequent
loadNotificationCursor logic within that transaction (or use INSERT ...
RETURNING to read and return the row directly), ensuring you call
loadNotificationCursor against the same tx/conn and commit before returning;
reference RecordCursorError, loadNotificationCursor, AdvanceCursor, and
ResetCursor in the change.
- Around line 108-116: The rollbackCtx created with context.WithoutCancel(ctx)
strips the deadline so rollbackImmediate/conn.ExecContext can block
indefinitely; change the code that sets rollbackCtx in the notification cursor
advance path (currently using rollbackCtx, joinCleanupError and calling
rollbackImmediate for "notification cursor advance") to reattach a deadline by
wrapping WithoutCancel with a timeout (e.g.
context.WithTimeout(context.WithoutCancel(ctx), timeout)) and pass that bounded
context into rollbackImmediate/conn.ExecContext; apply the same fix to the other
occurrence around lines 170-178 so all rollbackImmediate calls use a
deadline-bound context.
---
Nitpick comments:
In `@internal/bridges/task_notifier.go`:
- Around line 237-262: The function processTaskNotificationRecord has a
confusing non-idiomatic signature returning two separate error values
(terminalTaskNotificationDecision, error, int64, error); refactor by introducing
a single result struct (e.g. processRecordResult { decision
terminalTaskNotificationDecision; diagnostic error; sequence int64 }) and change
the function signature to return (processRecordResult, error) where the single
error is the processing failure and diagnostic lives in the result; update
processTaskNotificationRecord to populate and return the struct in all branches
(including mapping record.Sequence into sequence, placing
resolveTerminalTaskNotification diagnostic into diagnostic, and folding
recordCursorError into the returned error), and update all callers to use the
new struct fields instead of positional errors.
In `@internal/daemon/task_event_bridge_notifier_test.go`:
- Around line 461-469: The recordingTaskEventObserver uses an unsynchronized int
field `count` incremented in `OnTaskEvent`, which is unsafe if observers are
ever invoked concurrently; replace `count int` with an atomic counter (e.g.,
atomic.Int32 or an int32 accessed via sync/atomic) and update `OnTaskEvent` to
increment the atomic value and read it using the atomic load/store APIs so
increments and reads are safe across goroutines.
In `@internal/situation/service_test.go`:
- Around line 96-104: The fixture feeds three different channel identifiers
(CoordinationChannelID, NetworkChannel, and metadata coordination_channel_id)
but the test doesn't assert which source ContextForSession should prefer; update
the test to make precedence explicit by either collapsing the three fields to
the same canonical ID or adding an assertion on
payload.CoordinationChannel.Channel.ID to expect the chosen source; locate the
session construction that sets CoordinationChannelID, NetworkChannel, and
Metadata (jsonRaw) and then modify the fixture or add an assertion after calling
ContextForSession to verify payload.CoordinationChannel.Channel.ID equals the
intended canonical value.
🪄 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: e46a9913-d158-4483-a625-8e94602e2b48
⛔ Files ignored due to path filters (31)
.compozy/tasks/orch-improvs/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-001/issue_029.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (35)
internal/api/core/bridges.gointernal/api/core/tasks.gointernal/api/core/tasks_test.gointernal/api/spec/spec.gointernal/bridges/task_notifier.gointernal/bridges/task_notifier_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/task.gointernal/cli/task_test.gointernal/config/merge.gointernal/config/task_orchestration.gointernal/config/task_orchestration_test.gointernal/daemon/coordinator_runtime_test.gointernal/daemon/native_profile_tools.gointernal/daemon/native_review_tools.gointernal/daemon/native_tools_test.gointernal/daemon/review_router.gointernal/daemon/review_router_test.gointernal/daemon/task_event_bridge_notifier.gointernal/daemon/task_event_bridge_notifier_test.gointernal/daemon/task_runtime.gointernal/daemon/task_runtime_test.gointernal/extension/host_api_test.gointernal/session/query_test.gointernal/situation/service.gointernal/situation/service_test.gointernal/situation/task_context.gointernal/store/globaldb/global_db_notification_cursor.gointernal/store/globaldb/global_db_notification_cursor_test.gointernal/store/globaldb/global_db_task_claim.gointernal/store/globaldb/global_db_task_claim_test.gointernal/store/globaldb/global_db_task_profile.gointernal/store/globaldb/global_db_task_profile_test.gointernal/store/globaldb/global_db_task_projection.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/core/tasks_test.go
- internal/daemon/native_review_tools.go
- internal/daemon/task_event_bridge_notifier.go
- internal/cli/client.go
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/session/query_test.go (1)
557-558: ⚡ Quick winAdd
t.Parallel()to the new subtest.This new case does not opt into parallel execution, which diverges from the repository’s default test convention.
As per coding guidelines, "Default to `t.Parallel` in Go tests unless there is a specific reason to disable it (opt-out with `t.Setenv`)".♻️ Suggested change
t.Run("Should wait for finalization before reading a closed recorder handle", func(t *testing.T) { + t.Parallel() h := newHarness(t) session := createSession(t, h)🤖 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/session/query_test.go` around lines 557 - 558, The new subtest "Should wait for finalization before reading a closed recorder handle" is missing t.Parallel(); add a call to t.Parallel() as the first statement inside the subtest's func(t *testing.T) { ... } (i.e. immediately after the start of the anonymous function that calls newHarness(t)) so the subtest opts into parallel execution consistent with other tests; reference the subtest name and the use of newHarness(t) to locate the block to modify.internal/situation/service_test.go (1)
417-450: ⚡ Quick winWrap these new scenarios in
t.Run("Should ...")blocks.These added cases are the only new tests in this chunk that skip the repo’s required subtest structure, which makes later expansion and table conversion harder.
As per coding guidelines, "Use
t.Run('Should ...')pattern for Go test subtests instead of flat test structures".Also applies to: 452-539
🤖 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/situation/service_test.go` around lines 417 - 450, The new flat test cases in TestTaskStoreStubListRunReviewsSortsBeforeApplyingLimit (and the other new tests in the same chunk) must be converted to t.Run subtests using the "Should ..." pattern; locate the test function TestTaskStoreStubListRunReviewsSortsBeforeApplyingLimit and wrap the scenario body (setup, call to store.ListRunReviews, and assertions) inside t.Run("Should <describe behavior>", func(t *testing.T) { ... }), preserve t.Parallel() placement as appropriate (either inside each subtest or at the top-level per file style), and do the same wrapping for the other added cases referenced in the same diff so all new scenarios follow the repo's subtest structure.internal/bridges/task_notifier.go (1)
23-24: 🏗️ Heavy liftMove the default sweep limit out of code.
defaultTerminalTaskNotifierLimithardcodes an operational tuning knob into the binary. This notifier is new infrastructure, so the default should come from config/env andEventLimitshould only override it when explicitly supplied.As per coding guidelines, Never hardcode configuration values in Go code; always read from
config.tomlor environment variablesAlso applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/bridges/task_notifier.go` around lines 23 - 24, defaultTerminalTaskNotifierLimit is a hardcoded operational constant; change initialization so the notifier reads its default sweep limit from configuration/environment (e.g., a config key like TerminalTaskNotifier.DefaultLimit or env var TERMINAL_TASK_NOTIFIER_LIMIT) and only use EventLimit to override that configured default when EventLimit is explicitly set; update places referencing defaultTerminalTaskNotifierLimit (and the related constants at lines ~80-83) to pull from the config/env provider during NewTerminalTaskNotifier (or the constructor) and fall back to a sensible configured default instead of a compile-time constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/bridges/task_notifier.go`:
- Around line 422-435: The code assigns record.Event.Payload directly into
TerminalTaskNotification.Payload (in the terminalTaskNotificationResolution
returned by terminalTaskNotificationDeliveryID / the TerminalTaskNotification
construction), which allows secret fields to cross the bridge; instead sanitize
the payload before assignment by applying the same redaction logic used for
Error (e.g., call the existing taskpkg redaction helper or a new JSON-redaction
utility) to cloneRawJSON(record.Event.Payload) and/or whitelist allowed keys,
then set Notification.Payload to that redacted/allow-listed JSON; update both
occurrences (the block returning terminalTaskNotificationResolution and the
similar block around lines ~501-515) to use the redacted payload variable.
- Around line 539-548: TerminalTaskNotifier.recordCursorError currently persists
cause.Error() verbatim into CursorError.LastError; update this to redact
sensitive tokens before storing by passing a sanitized string instead of
cause.Error() to truncateTerminalTaskCursorError. Implement or call a redaction
helper (e.g., redactSensitiveTokens or similar) that detects and replaces raw
claim tokens (agh_claim_*), MCP auth tokens, OAuth codes, PKCE verifiers, and
secret bindings with hashed/placeholder forms (claim_token_hash or similar) and
then feed the result into truncateTerminalTaskCursorError when constructing
notifications.CursorError. Ensure the new redaction helper is used wherever
CursorError.LastError is set so no raw secrets are persisted.
In `@internal/daemon/review_router.go`:
- Around line 181-205: routeRunReview conflates transient operational errors
with true "no-route" diagnostics and the current code logs the error but still
calls recordNoRouteDiagnostic, which can permanently mark reviews blocked;
change the control flow so that when r.routeRunReview returns a non-nil err you
log and return immediately (i.e., do not proceed to recordNoRouteDiagnostic),
and only call recordNoRouteDiagnostic when err == nil and routed is false and
diagnostic is non-empty; update the branch around r.routeRunReview,
detachedDaemonOperationContext/reviewRouterRouteTimeout usage and the call to
recordNoRouteDiagnostic accordingly.
In `@internal/session/query_test.go`:
- Around line 816-834: The helper compareQueriedSessionEvents currently only
checks ID and Sequence which lets business-field regressions slip; update
compareQueriedSessionEvents to assert the full business payload for each event
(e.g., Type, Content/Payload, Metadata/Attributes, timestamps or any stable
business fields that openQueryRecorder should preserve) instead of only
ID/Sequence — either by doing explicit field comparisons for those stable fields
or by using a deep-equality check (reflect.DeepEqual or cmp.Diff) between
want[index] and got[index] after normalizing any non-deterministic fields, and
return a descriptive error showing the mismatch when they differ.
In `@internal/situation/service.go`:
- Around line 558-597: The code fetches taskRecord via store.GetTask and run via
store.GetTaskRun but never verifies they belong together; add a guard after
retrieving run to compare run.TaskID and taskRecord.ID and if they differ return
the same zero-value triple used elsewhere (contract.AgentTaskContextPayload{},
contract.AgentCoordinationChannelContextPayload{}, "", nil) to avoid mixing task
metadata and run coordination state before calling s.sessionContextBundle or
coordinationChannelPayload; place this check between the GetTaskRun call and the
s.sessionContextBundle(...) invocation and use the existing return pattern used
for non-context errors.
---
Nitpick comments:
In `@internal/bridges/task_notifier.go`:
- Around line 23-24: defaultTerminalTaskNotifierLimit is a hardcoded operational
constant; change initialization so the notifier reads its default sweep limit
from configuration/environment (e.g., a config key like
TerminalTaskNotifier.DefaultLimit or env var TERMINAL_TASK_NOTIFIER_LIMIT) and
only use EventLimit to override that configured default when EventLimit is
explicitly set; update places referencing defaultTerminalTaskNotifierLimit (and
the related constants at lines ~80-83) to pull from the config/env provider
during NewTerminalTaskNotifier (or the constructor) and fall back to a sensible
configured default instead of a compile-time constant.
In `@internal/session/query_test.go`:
- Around line 557-558: The new subtest "Should wait for finalization before
reading a closed recorder handle" is missing t.Parallel(); add a call to
t.Parallel() as the first statement inside the subtest's func(t *testing.T) {
... } (i.e. immediately after the start of the anonymous function that calls
newHarness(t)) so the subtest opts into parallel execution consistent with other
tests; reference the subtest name and the use of newHarness(t) to locate the
block to modify.
In `@internal/situation/service_test.go`:
- Around line 417-450: The new flat test cases in
TestTaskStoreStubListRunReviewsSortsBeforeApplyingLimit (and the other new tests
in the same chunk) must be converted to t.Run subtests using the "Should ..."
pattern; locate the test function
TestTaskStoreStubListRunReviewsSortsBeforeApplyingLimit and wrap the scenario
body (setup, call to store.ListRunReviews, and assertions) inside t.Run("Should
<describe behavior>", func(t *testing.T) { ... }), preserve t.Parallel()
placement as appropriate (either inside each subtest or at the top-level per
file style), and do the same wrapping for the other added cases referenced in
the same diff so all new scenarios follow the repo's subtest structure.
🪄 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: f14f5bc3-eb50-4501-bb17-6e77cf345bb4
⛔ Files ignored due to path filters (16)
.compozy/tasks/orch-improvs/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_014.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_015.mdis excluded by!**/*.md.compozy/tasks/orch-improvs/reviews-002/issue_016.mdis excluded by!**/*.md
📒 Files selected for processing (12)
internal/bridges/task_notifier.gointernal/bridges/task_notifier_test.gointernal/cli/task.gointernal/cli/task_test.gointernal/daemon/review_router.gointernal/daemon/review_router_test.gointernal/session/query_test.gointernal/situation/service.gointernal/situation/service_test.gointernal/situation/task_context.gointernal/store/globaldb/global_db_notification_cursor.gointernal/store/globaldb/global_db_notification_cursor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/situation/task_context.go
Closed
This was referenced May 6, 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