feat: add core tasks#19
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new task subsystem and integrates it across API contracts, HTTP/UDS routes, Host API, network ingress/auditing, persistence (DB schema + stores), daemon task runtime/session bridge, automation delegation into tasks, CLI/client surfaces, observer metrics/health, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant Dispatcher as Dispatcher
participant TaskSvc as TaskService
participant SessionMgr as SessionMgr
participant TaskBridge as TaskSessionBridge
Scheduler->>Dispatcher: TriggerJob (job with Task config)
activate Dispatcher
Dispatcher->>TaskSvc: CreateTask(spec with automation origin)
activate TaskSvc
TaskSvc-->>Dispatcher: Task{id}
deactivate TaskSvc
Dispatcher->>TaskSvc: EnqueueRun(TaskID, idempotency_key)
activate TaskSvc
TaskSvc-->>Dispatcher: TaskRun{id, status=queued}
deactivate TaskSvc
Dispatcher->>Dispatcher: delegateRun (set TaskID/TaskRunID, status=delegated)
Dispatcher-->>Scheduler: Run{status=delegated, task_id, task_run_id}
deactivate Dispatcher
Note over TaskSvc,TaskBridge: Later when TaskRun executes
TaskSvc->>TaskBridge: StartTaskSession(for run)
activate TaskBridge
TaskBridge->>SessionMgr: CreateSession(system, channel, workspace)
activate SessionMgr
SessionMgr-->>TaskBridge: Session{id}
deactivate SessionMgr
TaskBridge->>TaskSvc: AttachRunSession(runID, sessionID)
TaskSvc-->>TaskBridge: TaskRun{session_id set}
deactivate TaskBridge
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/api/contract/automation.go (1)
100-123:⚠️ Potential issue | 🟠 Major
UpdateJobRequestcannot clear an existing task binding.
Taskis optional here, but the patch shape has no way to distinguish "leave unchanged" from "remove the current task config". Once a job is created withtask, clients cannot switch it back to a prompt/agent-backed job through PATCH becausenilis treated as omission and{}still means "set task".💡 Suggested API shape
type UpdateJobRequest struct { Name *string `json:"name,omitempty"` AgentName *string `json:"agent_name,omitempty"` WorkspaceID *string `json:"workspace_id,omitempty"` Prompt *string `json:"prompt,omitempty"` Schedule *automationpkg.ScheduleSpec `json:"schedule,omitempty"` Task *automationpkg.JobTaskConfig `json:"task,omitempty"` + ClearTask bool `json:"clear_task,omitempty"` Enabled *bool `json:"enabled,omitempty"` Retry *automationpkg.RetryConfig `json:"retry,omitempty"` FireLimit *automationpkg.FireLimitConfig `json:"fire_limit,omitempty"` } func (r UpdateJobRequest) HasChanges() bool { return r.Name != nil || r.AgentName != nil || r.WorkspaceID != nil || r.Prompt != nil || r.Schedule != nil || r.Task != nil || + r.ClearTask || r.Enabled != nil || r.Retry != nil || r.FireLimit != nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/contract/automation.go` around lines 100 - 123, UpdateJobRequest cannot distinguish "omit" from "explicitly remove task"; change the API shape to allow explicit removal by adding a sentinel field (e.g., TaskRemoved *bool or TaskAction *string) or implement an explicit Task patch wrapper (e.g., TaskPatch { Set *JobTaskConfig; Remove *bool }) so JSON null/omission are distinguishable from "remove". Update the UpdateJobRequest struct (referencing UpdateJobRequest, Task, JobTaskConfig) and adjust HasChanges to treat TaskRemoved/TaskPatch.Remove as a change, and ensure request handling code checks the new sentinel to clear the job's task when present.internal/store/globaldb/global_db.go (1)
145-159:⚠️ Potential issue | 🟠 MajorAdd DB constraints for delegated task links on
automation_runs.
task_idandtask_run_idare free-text columns here. That means a bad write can persist nonexistent task references, and task deletion can leave automation history pointing at rows that no longer exist. Since these IDs are now first-class relations, the schema should enforce them the same way it already does forjob_idandtrigger_id.🛠️ Suggested constraint update
`CREATE TABLE IF NOT EXISTS automation_runs ( id TEXT PRIMARY KEY, job_id TEXT, trigger_id TEXT, session_id TEXT, task_id TEXT, task_run_id TEXT, status TEXT NOT NULL, attempt INTEGER NOT NULL DEFAULT 1, started_at TEXT, ended_at TEXT, error TEXT, + CHECK (task_run_id IS NULL OR task_id IS NOT NULL), FOREIGN KEY(job_id) REFERENCES automation_jobs(id) ON DELETE SET NULL, - FOREIGN KEY(trigger_id) REFERENCES automation_triggers(id) ON DELETE SET NULL + FOREIGN KEY(trigger_id) REFERENCES automation_triggers(id) ON DELETE SET NULL, + FOREIGN KEY(task_id) REFERENCES tasks(id) ON DELETE SET NULL, + FOREIGN KEY(task_run_id) REFERENCES task_runs(id) ON DELETE SET NULL );`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/globaldb/global_db.go` around lines 145 - 159, The automation_runs table currently defines task_id and task_run_id as plain TEXT allowing dangling references; update the table schema to add foreign key constraints for these columns similar to job_id/trigger_id so they reference the proper tables (e.g., FOREIGN KEY(task_id) REFERENCES tasks(id) ON DELETE SET NULL and FOREIGN KEY(task_run_id) REFERENCES task_runs(id) ON DELETE SET NULL), ensuring deletion of tasks or task runs nulls the links and prevents persisting nonexistent IDs.internal/extension/host_api_integration_test.go (1)
488-532:⚠️ Potential issue | 🟡 Minor
UnauthorizedExtensionIsDeniedForEveryMethodskips the new task endpoints.This PR adds
tasks/*andtasks/runs/*, but the denial matrix never exercises them. A capability regression on the new Host API surface would pass unnoticed unless those methods are added here or the test is renamed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_integration_test.go` around lines 488 - 532, The test UnauthorizedExtensionIsDeniedForEveryMethod's request matrix (the tests slice in host_api_integration_test.go) omits the new Host API endpoints under tasks/* and tasks/runs/*; add representative calls for the new surface (e.g. "tasks/list", "tasks/create", "tasks/get" with appropriate params like workspace or task_id, and "tasks/runs/list", "tasks/runs/create", "tasks/runs/get" with params like workspace, task_id or run_id) to the tests slice so the denial matrix exercises the new endpoints.
🧹 Nitpick comments (12)
internal/config/automation.go (1)
258-261: Avoid shallow-copyingJobTaskConfig.
taskCfg := *j.Taskonly copies the top-level struct, so nested pointers remain shared between the parsed config and the runtime job. A smallcloneJobTaskConfighelper would prevent runtime mutations from leaking back into config state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/automation.go` around lines 258 - 261, The code currently does a shallow copy with taskCfg := *j.Task and assigns job.Task = &taskCfg, which leaves nested pointers shared; implement a helper cloneJobTaskConfig(original *JobTaskConfig) *JobTaskConfig that deep-copies all nested pointer fields/slices/maps and use it in the j.Task != nil branch (replace the shallow copy with job.Task = cloneJobTaskConfig(j.Task)) so runtime mutations on job.Task do not mutate the parsed config.internal/api/contract/contract_test.go (1)
452-556: Uset.Run("Should...")consistently for the new task contract tests.This block introduces bare task JSON-shape tests and short case names like
"empty"/"title", which drifts from the repo’s required subtest convention.As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/contract/contract_test.go` around lines 452 - 556, Rename the standalone and subtests to follow the "Should..." t.Run pattern: wrap the assertions in TestTaskPayloadJSONShape and TestTaskRunPayloadJSONShape inside t.Run("Should marshal task payload JSON shape", ...) and t.Run("Should marshal task run payload JSON shape", ...) respectively (keeping t.Parallel where appropriate), and change the table-driven subtest invocation in TestUpdateTaskRequestHasChanges from t.Run(tc.name, ...) to t.Run("Should "+tc.name, ...) so every test/subtest uses the required "Should..." naming convention (locate functions TestTaskPayloadJSONShape, TestTaskRunPayloadJSONShape, and the t.Run call inside TestUpdateTaskRequestHasChanges that references tc.name).internal/extension/host_api_tasks.go (1)
162-191: Consider adding server-side filtering for task runs.
handleTasksRunsfetches the entireTaskView(including all runs, events, dependencies, children) and then filters runs in-memory. For tasks with many runs, this could be inefficient.Consider adding a dedicated
ListTaskRuns(ctx, taskID, query, actor)method to the manager that applies filters at the database level, rather than fetching everything and filtering client-side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_tasks.go` around lines 162 - 191, handleTasksRuns currently loads the full TaskView via manager.GetTask and filters runs in-memory which is inefficient for tasks with many runs; add a new manager method ListTaskRuns(ctx context.Context, taskID string, query TaskRunListQuery, actor Actor) ([]TaskRun, error) (and update the manager interface/implementations) that applies query filtering at the DB layer (use the existing taskRunQueryFromParams to build the query), then change handleTasksRuns to call manager.ListTaskRuns(ctx, taskID, query, actor), handle errors with mapTaskRPCError("task", taskID, err) as before, and return taskRunPayloadsFromRuns on the returned runs instead of fetching the full TaskView and calling filterTaskRuns.internal/extension/host_api_test.go (1)
1006-1023: Avoid wall-clock polling in this test.The
time.Sleeploop makes this test timing-sensitive and prone to CI flakiness. Prefer a completion signal from the fake driver/session recorder, or another deterministic synchronization primitive, instead of polling fordone.As per coding guidelines "Never use
time.Sleep()in orchestration — use proper synchronization primitives".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_test.go` around lines 1006 - 1023, The test currently polls with a time.Sleep loop to wait for a "done" event from promptEvents (loop using deadline, env.sessions.Events(testutil.Context(t), sess.ID, store.EventQuery{TurnID: prompt.TurnID}), checking storedEvent.Type == acp.EventTypeDone), which is flaky; replace this wall-clock polling with a deterministic synchronization primitive exposed by the fake driver/session recorder (e.g., a Done channel or a WaitForEvent/WaitForTurn method) so the test blocks until the recorder signals completion for prompt.TurnID, then call env.sessions.Events once (or immediately after the signal) and assert the acp.EventTypeDone is present instead of sleeping in a loop.internal/cli/task_test.go (2)
484-505: Make the sample run fixture match the requested status.
sampleTaskRunRecordpopulates end-state fields even for queued/claimed/starting runs. That makes it harder for these tests to catch status-dependent rendering or JSON-shaping bugs.As per coding guidelines, `**/*_test.go`: Ensure tests verify behavior outcomes, not just function calls.Suggested direction
func sampleTaskRunRecord(status taskpkg.TaskRunStatus) TaskRunRecord { - claimedAt := fixedTestNow.Add(time.Minute) - startedAt := fixedTestNow.Add(2 * time.Minute) - endedAt := fixedTestNow.Add(3 * time.Minute) - return TaskRunRecord{ + record := TaskRunRecord{ ID: "run-1", TaskID: "task-1", Status: status, Attempt: 1, - ClaimedBy: &taskpkg.ActorIdentity{Kind: taskpkg.ActorKindHuman, Ref: "local-user"}, - SessionID: "sess-1", Origin: taskpkg.Origin{Kind: taskpkg.OriginKindCLI, Ref: "tasks.run.start"}, IdempotencyKey: "idem-run", NetworkChannel: "builders", QueuedAt: fixedTestNow, - ClaimedAt: claimedAt, - StartedAt: startedAt, - EndedAt: endedAt, - Error: "boom", - Result: json.RawMessage(`{"ok":true}`), } + if status >= taskpkg.TaskRunStatusClaimed { + record.ClaimedBy = &taskpkg.ActorIdentity{Kind: taskpkg.ActorKindHuman, Ref: "local-user"} + record.ClaimedAt = fixedTestNow.Add(time.Minute) + } + if status == taskpkg.TaskRunStatusStarting || status == taskpkg.TaskRunStatusRunning || status == taskpkg.TaskRunStatusCompleted || status == taskpkg.TaskRunStatusFailed { + record.SessionID = "sess-1" + } + if status == taskpkg.TaskRunStatusRunning || status == taskpkg.TaskRunStatusCompleted || status == taskpkg.TaskRunStatusFailed { + record.StartedAt = fixedTestNow.Add(2 * time.Minute) + } + if status == taskpkg.TaskRunStatusCompleted || status == taskpkg.TaskRunStatusFailed || status == taskpkg.TaskRunStatusCancelled { + record.EndedAt = fixedTestNow.Add(3 * time.Minute) + } + if status == taskpkg.TaskRunStatusCompleted { + record.Result = json.RawMessage(`{"ok":true}`) + } + if status == taskpkg.TaskRunStatusFailed { + record.Error = "boom" + } + return record - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/task_test.go` around lines 484 - 505, The sampleTaskRunRecord test fixture currently fills end-state fields unconditionally which hides status-specific behavior; update sampleTaskRunRecord to set ClaimedAt, StartedAt, EndedAt, Error and Result only when the provided status indicates those lifecycle phases (e.g., set ClaimedAt for TaskRunStatusClaimed/Running/Completed, StartedAt for Running/Completed, EndedAt/Error/Result for Completed/Failed), leaving those fields nil/empty for queued/claimed/starting statuses so tests can observe correct rendering/JSON shaping for TaskRunRecord and TaskRunStatus.
151-255: Break the command-mapping coverage into table-driven subtests.Each of these tests covers many unrelated commands in one flow, so the first failure hides the rest of the parsing surface.
As per coding guidelines,
**/*_test.go: Use table-driven tests with subtests (t.Run) as default in Go tests.Also applies to: 257-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/task_test.go` around lines 151 - 255, The TestTaskRunCommandsMapLifecycleRequests function bundles many independent command-parsing checks into one linear test; refactor it into a table-driven test with t.Run subtests so each command case runs and fails independently. Create a []struct testCases with fields name, args []string, setup stubClient handlers (or reset captured vars like runListQuery, enqueueRequest, claimRequest, startRequest, attachRequest, completeRequest, failRequest, cancelRequest) and expected assertions; for each case call executeRootCommand with the case args and assert only the relevant captured request (e.g., runListQuery for "task run list", enqueueRequest for "task run enqueue", claimRequest for "task run claim", startRequest for "task run start", attachRequest for "task run attach-session", completeRequest for "task run complete", failRequest for "task run fail", cancelRequest for "task run cancel"). Ensure captured variables are reinitialized between subtests and reuse the stubClient functions (listTaskRunsFn, enqueueTaskRunFn, claimTaskRunFn, startTaskRunFn, attachTaskRunSessionFn, completeTaskRunFn, failTaskRunFn, cancelTaskRunFn) to populate those captures; apply the same table-driven refactor to the other similar test block mentioned in the comment.internal/api/httpapi/httpapi_integration_test.go (1)
1033-1167: Split the run-lifecycle matrix into subtests.This single test covers three independent flows (
complete,attach-session/fail,cancel). When one step breaks, the rest of the route coverage disappears behind the first failure.As per coding guidelines,
**/*_test.go: Use table-driven tests with subtests (t.Run) as default in Go tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/httpapi_integration_test.go` around lines 1033 - 1167, The TestHTTPTaskRunLifecycleRoutesRoundTrip is exercising three independent flows in one test; split it into subtests using t.Run for each scenario ("complete", "attach-session/fail", "cancel") while keeping the common setup (runtime := newIntegrationRuntime(t) and created := createIntegrationTask(...)) outside or in a shared setup helper, then inside each t.Run create its own run with enqueueIntegrationTaskRun and perform only that flow's HTTP calls (claimIntegrationTaskRun / start / complete for the "complete" case; claimIntegrationTaskRun / attach-session / fail for the "attach-session/fail" case; and just enqueueIntegrationTaskRun / cancel for the "cancel" case), assert only that flow's expected terminal status (use decodeHTTPJSON and check TaskRunStatusCompleted/Failed/Cancelled), and avoid relying on cross-flow finalRuns aggregation so the test outcomes are isolated and failures in one subtest do not prevent others from running.internal/observe/tasks.go (1)
198-220: Consider adding explicit context nil check for consistency.While
loadTaskSnapshotperforms the nil context check, adding it at the start ofQueryTaskSummary(likeQueryTaskMetricsdoes) would provide consistent API behavior and clearer error messages.💡 Proposed fix for consistency
func (o *Observer) QueryTaskSummary(ctx context.Context, query TaskSummaryQuery) (TaskSummary, error) { + if ctx == nil { + return TaskSummary{}, errors.New("observe: task summary context is required") + } snapshot, err := o.loadTaskSnapshot(ctx, query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/observe/tasks.go` around lines 198 - 220, Add an explicit nil context check at the start of QueryTaskSummary (like QueryTaskMetrics does) to return an immediate error when ctx == nil before calling loadTaskSnapshot; update the function to return an empty TaskSummary and the same error message/type used by QueryTaskMetrics so behavior and error messaging are consistent across the API (locate the check inside the QueryTaskSummary function before the call to loadTaskSnapshot).internal/api/core/tasks.go (2)
1122-1128: Consider nil slice handling for cloned raw message.The
cloneRawMessagefunction (called on line 1126) should handle the case where the sourcejson.RawMessageis an empty slice vs nil. Currently, if*sourceis[]byte{}, it will be passed tocloneRawMessagewhich returnsnilfor empty slices. Verify this is the intended behavior for JSON semantics (empty vs absent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/tasks.go` around lines 1122 - 1128, The cloneRawMessagePtr helper currently calls cloneRawMessage(*source) which treats an empty json.RawMessage (zero-length []byte{}) as nil; update behavior so empty vs absent are preserved: either modify cloneRawMessage to return a distinct empty slice instead of nil for zero-length input, or change cloneRawMessagePtr to detect len(*source)==0 and return a pointer to an explicitly allocated empty json.RawMessage (not nil). Ensure you update the logic around cloneRawMessage and cloneRawMessagePtr so callers see nil only for absent values and a non-nil pointer for an empty JSON payload.
1077-1092: In-memory filtering may not scale well for large task run sets.The
filterTaskRunsfunction loads all runs from the view and filters in-memory. For tasks with many runs, this could impact performance. Consider whether theListTaskRunsendpoint should delegate filtering to the store layer with proper SQL clauses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/tasks.go` around lines 1077 - 1092, The current filterTaskRuns function performs in-memory filtering of all task runs which won't scale; update the ListTaskRuns flow to push filtering into the store layer (e.g., add or extend a store method such as Store.ListTaskRuns or TaskRunStore.QueryTaskRuns) so SQL-level predicates handle Status (normalized), SessionID (trimmed), and Limit (and optionally Offset) instead of loading all runs; keep filterTaskRuns only as a defensive fallback if needed, but change callers (the ListTaskRuns handler/function) to call the new store method and remove the unbounded retrieval that precedes filterTaskRuns.internal/store/globaldb/global_db_task.go (2)
183-185: Ignored error on rows.Close() is acceptable but consider logging.While ignoring the
rows.Close()error in a defer is a common Go pattern (since the error often isn't actionable at this point), the coding guidelines state "Never ignore errors with_— every error must be handled or have a written justification." Consider adding a brief comment explaining why the error is ignored here.💡 Proposed documentation fix
defer func() { - _ = rows.Close() + _ = rows.Close() // Close error is not actionable after successful iteration }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/globaldb/global_db_task.go` around lines 183 - 185, The deferred call currently swallows the error from rows.Close() using "_ = rows.Close()" which violates the guideline to never ignore errors; update the defer in global_db_task.go (the defer wrapping rows.Close()) to either log the returned error using the existing logger or add a concise comment justifying why the error is intentionally ignored (e.g., non-actionable after result processing), for example replacing the blank assignment with a small if err := rows.Close(); err != nil { /* log or justify */ } or adding a one-line justification comment above the defer referencing rows.Close().
567-581: Consider returning more specific error when taskID is empty.The
ensureTaskExistsfunction returnstaskpkg.ErrTaskNotFoundwhen thetaskIDis empty (line 570). This conflates "missing input" with "record not found". Consider returning a validation error for empty input to aid debugging.💡 Proposed fix for clearer error semantics
func (g *GlobalDB) ensureTaskExists(ctx context.Context, taskID string) error { trimmedID := strings.TrimSpace(taskID) if trimmedID == "" { - return taskpkg.ErrTaskNotFound + return fmt.Errorf("store: task id is required: %w", taskpkg.ErrValidation) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/store/globaldb/global_db_task.go` around lines 567 - 581, The function ensureTaskExists conflates empty input with "not found"; change the initial validation so that when strings.TrimSpace(taskID) yields an empty string it returns a distinct validation error (e.g. taskpkg.ErrInvalidTaskID or a new exported error in taskpkg like ErrInvalidTaskID/ErrEmptyTaskID with a clear message) instead of taskpkg.ErrTaskNotFound, update the error construction to use that error, and ensure any callers expecting taskpkg.ErrTaskNotFound for non-existent records remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/contract/tasks.go`:
- Line 25: Task timestamp fields are currently time.Time so omitempty won't
work; change TaskSummaryPayload.ClosedAt and TaskPayload.ClosedAt, and
TaskRunPayload.ClaimedAt, StartedAt, EndedAt to use *time.Time (or implement
custom JSON marshaling) so unset values serialize as omitted. Update any
constructors/assignment sites that populate these fields to take pointers (use
&t for existing time values and nil for absent values) and ensure JSON
consumers/validators expect nullable timestamps.
In `@internal/api/core/errors_test.go`:
- Around line 195-196: The test uses an invalid status code 0 which causes
httptest.ResponseRecorder.WriteHeader to panic when RespondError(ctx, 0, ...) is
called; update the test case (the entry with name "unknown error fallback") to
use a valid status code such as 599 instead of 0 so the test exercises the
unknown-error fallback path in RespondError without panicking and still yields
an empty http.StatusText() for assertion.
In `@internal/api/core/tasks_internal_test.go`:
- Around line 168-205: The tests currently only assert err != nil; change each
assertion to verify the specific expected error type or message (using
ErrorContains/ErrorAs or errors.Is) for the functions
attachTaskRunSessionIDFromRequest, failTaskRunFromRequest, validateTaskChannel,
enqueueTaskRunFromRequest, requiredPathID, handlers.parseTaskListQuery,
parseTaskRunListQuery, and decodeOptionalJSON so they fail if a different error
is returned; replace generic nil checks with targeted assertions that match the
exact validation/decoding error text or exported error value for each case.
In `@internal/api/httpapi/httpapi_integration_test.go`:
- Around line 1185-1192: The StartTaskSession method on
integrationTaskSessionExecutor increments the started counter without
synchronization which can race under concurrent HTTP tests; fix by making
started a thread-safe counter (e.g., change started to an int32/int64 and use
atomic.AddInt32/AddInt64 in StartTaskSession, or add a sync.Mutex on
integrationTaskSessionExecutor and lock/unlock while incrementing/reading
started) so that StartTaskSession and the SessionID generation are safe under
concurrent calls.
In `@internal/api/udsapi/routes.go`:
- Around line 91-113: RegisterRoutes now exposes /api/tasks and /api/task-runs
but the OpenAPI registry returned by internal/api/spec/spec.go::Operations() is
missing those paths; update Operations() to add path objects and operation
entries for all task-related endpoints (map HTTP methods and operationIds to
CreateTask, ListTasks, GetTask, UpdateTask, CancelTask, CreateChildTask,
AddTaskDependency, RemoveTaskDependency, EnqueueTaskRun, ListTaskRuns and for
task-runs: ClaimTaskRun, StartTaskRun, AttachTaskRunSession, CompleteTaskRun,
FailTaskRun, CancelTaskRun) so the canonical spec matches the handlers and
methods defined in RegisterRoutes.
In `@internal/api/udsapi/server_test.go`:
- Around line 103-113: The test cases calling New(...) in server_test.go
currently omit multiple required options so failures are ambiguous; update each
case to include all other required dependencies except the one under test (use
WithHomePaths, WithSessionManager(stubSessionManager{}),
WithTaskService(stubTaskManager{}), WithObserver(stubObserver{}),
WithWorkspaceResolver(stubWorkspaceResolver{}) as appropriate) so only the
intended missing dependency triggers the error, and replace generic nil-checks
with specific assertions (t.ErrorContains or errors.As) that verify the exact
error value or message returned by New for the missing dependency (reference
New, WithSessionManager, WithTaskService, WithObserver, WithWorkspaceResolver
and the stub* types to locate and fix each case).
In `@internal/automation/dispatch_test.go`:
- Around line 261-267: The test currently only checks that dispatcher.Dispatch
returned a non-nil error; change it to assert the specific "missing task
service" failure by using errors.Is or ErrorContains instead of a nil check:
after calling dispatcher.Dispatch(...) replace the generic err != nil check with
an assertion that errors.Is(err, <expected sentinel error for missing task
service>) or testutil.ErrorContains(err, "<expected message substring>"),
referencing the Dispatch function call and
DispatchRequest/DispatchKindManual/job variables so the test fails if Dispatch
errors for any other reason.
In `@internal/automation/dispatch.go`:
- Around line 452-456: The pre-fire hook's rewritten prompt returned by
dispatchPreFireHook is being ignored; update the call sites (where preFirePrompt
is set and where directTaskSpec is constructed) to assign the returned patched
prompt (the first return value) back into preFirePrompt and use that variable
when building directTaskSpec instead of falling back to req.Job.Prompt or
req.Prompt; specifically, capture the patchedPrompt from dispatchPreFireHook and
pass that into the delegated task payload so hook-based rewrites/sanitization
are preserved (adjust the occurrences around dispatchPreFireHook and where
directTaskSpec is populated).
- Around line 469-489: The current flow calls tasks.CreateTask (directTaskSpec)
before the idempotent tasks.EnqueueRun, so retries can create duplicate/orphaned
tasks; change to an idempotent creation path by deriving a stable task identity
from scheduledRun.ID (e.g., pass a TaskID or idempotency key computed from
scheduledRun.ID into tasks.CreateTask or use a new
tasks.CreateOrGetTask/CreateTaskWithInitialRun API that atomically creates the
task and first run), or update the call sites (directTaskSpec,
automationTaskRunIdempotencyKey, tasks.CreateTask, tasks.EnqueueRun,
delegateRun) to use a single atomic operation on the tasks service that returns
both task and run, ensuring retries are safe and no orphaned tasks are produced.
In `@internal/automation/manager_test.go`:
- Around line 591-622: The new test TestManagerSessionTaskActorLifecycle
violates the repo test convention by being a flat test; wrap its assertions
inside a t.Run("Should ...") subtest. Modify
TestManagerSessionTaskActorLifecycle to call t.Run with a descriptive "Should
..." name and move the existing setup and assertions (including calls to
newManager(...), taskpkg.DeriveAutomationLinkedAgentSessionActorContext,
manager.RecordAutomationSessionTaskActor, manager.TaskActorContextForSession and
manager.DeleteAutomationSessionTaskActor) into the subtest function body,
preserving the existing error checks and t.Fatalf calls.
In `@internal/automation/model/validate.go`:
- Around line 337-340: The current Run.Validate only enforces that when TaskID
is set and Status == RunDelegated, TaskRunID must be present; add the symmetric
check so that when TaskRunID is set and Status == RunDelegated, TaskID must not
be empty. In the Run.Validate function (referencing r.TaskID, r.TaskRunID,
r.Status and the RunDelegated constant), add a second conditional mirroring the
existing one that returns fmt.Errorf(...) using nestedPath(path, "task_id") and
nestedPath(path, "task_run_id") to produce the appropriate error message when
TaskRunID is provided but TaskID is missing.
In `@internal/daemon/daemon_test.go`:
- Around line 686-755: Test only covers the healthy survivor path; add a second
extension fixture and make the fakeExtensionRuntime return an
unhealthy/unregistered result for it so the partial-start recovery excludes it.
Specifically, in
TestBootExtensionsKeepsHealthyRegisteredExtensionsAfterPartialStartFailure add
another installDaemonTestExtension call (e.g., "ext-bad") and update the
fakeExtensionRuntime.getExt / get behavior to return a Status with
Registered=false or Enabled=false (or nil/missing) for "ext-bad"; after
d.bootExtensions assert that the bad extension is not present (e.g.,
state.currentExtensionRuntime() != runtime for that name or check
registry/cleanup entries do not include "ext-bad"), keeping the existing
assertions for the healthy "ext-healthy".
In `@internal/daemon/daemon.go`:
- Line 228: The daemon struct's tasks field (type *taskRuntime) is not being
cleared in Shutdown(); update the Shutdown() method to set d.tasks = nil
alongside the other component fields so the tasks pointer is defensively
released after shutdown; locate the Shutdown() function on the daemon type and
add a line clearing the tasks field (d.tasks = nil) after any stop/cleanup calls
handling taskRuntime to match the existing pattern used for other fields.
In `@internal/extension/manager_test.go`:
- Around line 753-788: TestManagerReloadValidatesAndRestarts currently only
verifies Reload() returns nil and can be satisfied by a no-op; update the test
to seed the registry with at least one extension (use newRegistryTestEnv /
env.registry or helper that registers an extension) before starting the manager
(NewManager / startedManager), then call startedManager.Reload and assert an
observable side effect such as the extension's Start/Reload handler being
invoked, a changed Manager state, or a registry reload counter; also split the
existing checks into separate t.Run("Should ...") subtests for the nil manager
case, canceled context case, missing registry case, and the successful reload
case so each scenario is isolated and verifies behavior, not just return values.
In `@internal/network/audit_test.go`:
- Around line 114-159: The new TestAuditWriterRecordTaskIngress must be
converted to use a subtest via t.Run to follow the repository's required subtest
pattern: wrap the existing test body (setup of storeSink, NewAuditWriter,
writer.now, the call to writer.RecordTaskIngress, and all assertions on
storeSink.entries[0] including SessionID, Kind, Direction, Reason, and Size)
inside a t.Run("Should record task ingress", func(t *testing.T) { ... }) closure
(and keep/relocate t.Parallel inside the subtest if desired); ensure the same
unique symbols (TestAuditWriterRecordTaskIngress, writer.RecordTaskIngress,
storeSink.entries) are used unchanged, and keep all assertions intact inside the
subtest.
In `@internal/network/tasks_integration_test.go`:
- Around line 302-315: The helper findNetworkAuditByMessageID currently returns
entries[0] for any non-empty result; change it to assert exactly one matching
audit row to catch duplicate writes: after calling db.ListNetworkAudit in
findNetworkAuditByMessageID, fail the test if len(entries) == 0 or len(entries)
> 1 (use t.Fatalf with context including messageID and len(entries)), and only
return entries[0] when len(entries) == 1 so tests detect duplicate audit rows;
reference ListNetworkAudit, NetworkAuditQuery, and NetworkAuditEntry when making
the check.
In `@internal/observe/health.go`:
- Around line 49-52: The call to collectTaskHealth in observe.health.go returns
an unwrapped error which loses context; update the error return in the
observe.collectTaskHealth call inside function Get/Collect/whatever the
surrounding function is (the one that currently assigns taskHealth, err :=
o.collectTaskHealth(ctx)) to wrap the error with contextual text using
fmt.Errorf, e.g. return Health{}, fmt.Errorf("collecting task health: %w", err),
so callers see the operation that failed; ensure you import fmt if not already
present.
In `@internal/observe/tasks_test.go`:
- Around line 388-396: The tests currently only assert that Validate() returns
non-nil, which is too weak; update the three assertions to check the error
shape/content for the specific invalid field. For the TaskSummaryQuery cases
(TaskSummaryQuery{Scope: taskpkg.Scope("bogus")} and TaskSummaryQuery{OwnerKind:
taskpkg.OwnerKind("bogus")}) and the TaskMetricsQuery case
(TaskMetricsQuery{OriginKind: taskpkg.OriginKind("bogus")}), replace the plain
nil checks with assertions that the returned error either matches the expected
validation error type via errors.As or contains an expected substring (e.g.,
"scope", "owner kind", "origin kind") using ErrorContains / strings.Contains so
the test fails if Validate() rejects a different field.
In `@internal/session/stop_reason.go`:
- Around line 54-81: RequestStopWithCause (and the sibling method around lines
84-116) dereference the receiver m before checking for a typed nil, causing a
panic for a nil *Manager; add an initial guard like "if m == nil { return
errors.New(\"session: manager is required\") }" at the top of
RequestStopWithCause and the other public stop method. Keep the rest of the flow
(calls to m.prepareStopWithCause, m.finalizeStopped, m.driver.Cancel and
isProcessDone) unchanged so the nil-receive case returns a proper error instead
of panicking.
---
Outside diff comments:
In `@internal/api/contract/automation.go`:
- Around line 100-123: UpdateJobRequest cannot distinguish "omit" from
"explicitly remove task"; change the API shape to allow explicit removal by
adding a sentinel field (e.g., TaskRemoved *bool or TaskAction *string) or
implement an explicit Task patch wrapper (e.g., TaskPatch { Set *JobTaskConfig;
Remove *bool }) so JSON null/omission are distinguishable from "remove". Update
the UpdateJobRequest struct (referencing UpdateJobRequest, Task, JobTaskConfig)
and adjust HasChanges to treat TaskRemoved/TaskPatch.Remove as a change, and
ensure request handling code checks the new sentinel to clear the job's task
when present.
In `@internal/extension/host_api_integration_test.go`:
- Around line 488-532: The test UnauthorizedExtensionIsDeniedForEveryMethod's
request matrix (the tests slice in host_api_integration_test.go) omits the new
Host API endpoints under tasks/* and tasks/runs/*; add representative calls for
the new surface (e.g. "tasks/list", "tasks/create", "tasks/get" with appropriate
params like workspace or task_id, and "tasks/runs/list", "tasks/runs/create",
"tasks/runs/get" with params like workspace, task_id or run_id) to the tests
slice so the denial matrix exercises the new endpoints.
In `@internal/store/globaldb/global_db.go`:
- Around line 145-159: The automation_runs table currently defines task_id and
task_run_id as plain TEXT allowing dangling references; update the table schema
to add foreign key constraints for these columns similar to job_id/trigger_id so
they reference the proper tables (e.g., FOREIGN KEY(task_id) REFERENCES
tasks(id) ON DELETE SET NULL and FOREIGN KEY(task_run_id) REFERENCES
task_runs(id) ON DELETE SET NULL), ensuring deletion of tasks or task runs nulls
the links and prevents persisting nonexistent IDs.
---
Nitpick comments:
In `@internal/api/contract/contract_test.go`:
- Around line 452-556: Rename the standalone and subtests to follow the
"Should..." t.Run pattern: wrap the assertions in TestTaskPayloadJSONShape and
TestTaskRunPayloadJSONShape inside t.Run("Should marshal task payload JSON
shape", ...) and t.Run("Should marshal task run payload JSON shape", ...)
respectively (keeping t.Parallel where appropriate), and change the table-driven
subtest invocation in TestUpdateTaskRequestHasChanges from t.Run(tc.name, ...)
to t.Run("Should "+tc.name, ...) so every test/subtest uses the required
"Should..." naming convention (locate functions TestTaskPayloadJSONShape,
TestTaskRunPayloadJSONShape, and the t.Run call inside
TestUpdateTaskRequestHasChanges that references tc.name).
In `@internal/api/core/tasks.go`:
- Around line 1122-1128: The cloneRawMessagePtr helper currently calls
cloneRawMessage(*source) which treats an empty json.RawMessage (zero-length
[]byte{}) as nil; update behavior so empty vs absent are preserved: either
modify cloneRawMessage to return a distinct empty slice instead of nil for
zero-length input, or change cloneRawMessagePtr to detect len(*source)==0 and
return a pointer to an explicitly allocated empty json.RawMessage (not nil).
Ensure you update the logic around cloneRawMessage and cloneRawMessagePtr so
callers see nil only for absent values and a non-nil pointer for an empty JSON
payload.
- Around line 1077-1092: The current filterTaskRuns function performs in-memory
filtering of all task runs which won't scale; update the ListTaskRuns flow to
push filtering into the store layer (e.g., add or extend a store method such as
Store.ListTaskRuns or TaskRunStore.QueryTaskRuns) so SQL-level predicates handle
Status (normalized), SessionID (trimmed), and Limit (and optionally Offset)
instead of loading all runs; keep filterTaskRuns only as a defensive fallback if
needed, but change callers (the ListTaskRuns handler/function) to call the new
store method and remove the unbounded retrieval that precedes filterTaskRuns.
In `@internal/api/httpapi/httpapi_integration_test.go`:
- Around line 1033-1167: The TestHTTPTaskRunLifecycleRoutesRoundTrip is
exercising three independent flows in one test; split it into subtests using
t.Run for each scenario ("complete", "attach-session/fail", "cancel") while
keeping the common setup (runtime := newIntegrationRuntime(t) and created :=
createIntegrationTask(...)) outside or in a shared setup helper, then inside
each t.Run create its own run with enqueueIntegrationTaskRun and perform only
that flow's HTTP calls (claimIntegrationTaskRun / start / complete for the
"complete" case; claimIntegrationTaskRun / attach-session / fail for the
"attach-session/fail" case; and just enqueueIntegrationTaskRun / cancel for the
"cancel" case), assert only that flow's expected terminal status (use
decodeHTTPJSON and check TaskRunStatusCompleted/Failed/Cancelled), and avoid
relying on cross-flow finalRuns aggregation so the test outcomes are isolated
and failures in one subtest do not prevent others from running.
In `@internal/cli/task_test.go`:
- Around line 484-505: The sampleTaskRunRecord test fixture currently fills
end-state fields unconditionally which hides status-specific behavior; update
sampleTaskRunRecord to set ClaimedAt, StartedAt, EndedAt, Error and Result only
when the provided status indicates those lifecycle phases (e.g., set ClaimedAt
for TaskRunStatusClaimed/Running/Completed, StartedAt for Running/Completed,
EndedAt/Error/Result for Completed/Failed), leaving those fields nil/empty for
queued/claimed/starting statuses so tests can observe correct rendering/JSON
shaping for TaskRunRecord and TaskRunStatus.
- Around line 151-255: The TestTaskRunCommandsMapLifecycleRequests function
bundles many independent command-parsing checks into one linear test; refactor
it into a table-driven test with t.Run subtests so each command case runs and
fails independently. Create a []struct testCases with fields name, args
[]string, setup stubClient handlers (or reset captured vars like runListQuery,
enqueueRequest, claimRequest, startRequest, attachRequest, completeRequest,
failRequest, cancelRequest) and expected assertions; for each case call
executeRootCommand with the case args and assert only the relevant captured
request (e.g., runListQuery for "task run list", enqueueRequest for "task run
enqueue", claimRequest for "task run claim", startRequest for "task run start",
attachRequest for "task run attach-session", completeRequest for "task run
complete", failRequest for "task run fail", cancelRequest for "task run
cancel"). Ensure captured variables are reinitialized between subtests and reuse
the stubClient functions (listTaskRunsFn, enqueueTaskRunFn, claimTaskRunFn,
startTaskRunFn, attachTaskRunSessionFn, completeTaskRunFn, failTaskRunFn,
cancelTaskRunFn) to populate those captures; apply the same table-driven
refactor to the other similar test block mentioned in the comment.
In `@internal/config/automation.go`:
- Around line 258-261: The code currently does a shallow copy with taskCfg :=
*j.Task and assigns job.Task = &taskCfg, which leaves nested pointers shared;
implement a helper cloneJobTaskConfig(original *JobTaskConfig) *JobTaskConfig
that deep-copies all nested pointer fields/slices/maps and use it in the j.Task
!= nil branch (replace the shallow copy with job.Task =
cloneJobTaskConfig(j.Task)) so runtime mutations on job.Task do not mutate the
parsed config.
In `@internal/extension/host_api_tasks.go`:
- Around line 162-191: handleTasksRuns currently loads the full TaskView via
manager.GetTask and filters runs in-memory which is inefficient for tasks with
many runs; add a new manager method ListTaskRuns(ctx context.Context, taskID
string, query TaskRunListQuery, actor Actor) ([]TaskRun, error) (and update the
manager interface/implementations) that applies query filtering at the DB layer
(use the existing taskRunQueryFromParams to build the query), then change
handleTasksRuns to call manager.ListTaskRuns(ctx, taskID, query, actor), handle
errors with mapTaskRPCError("task", taskID, err) as before, and return
taskRunPayloadsFromRuns on the returned runs instead of fetching the full
TaskView and calling filterTaskRuns.
In `@internal/extension/host_api_test.go`:
- Around line 1006-1023: The test currently polls with a time.Sleep loop to wait
for a "done" event from promptEvents (loop using deadline,
env.sessions.Events(testutil.Context(t), sess.ID, store.EventQuery{TurnID:
prompt.TurnID}), checking storedEvent.Type == acp.EventTypeDone), which is
flaky; replace this wall-clock polling with a deterministic synchronization
primitive exposed by the fake driver/session recorder (e.g., a Done channel or a
WaitForEvent/WaitForTurn method) so the test blocks until the recorder signals
completion for prompt.TurnID, then call env.sessions.Events once (or immediately
after the signal) and assert the acp.EventTypeDone is present instead of
sleeping in a loop.
In `@internal/observe/tasks.go`:
- Around line 198-220: Add an explicit nil context check at the start of
QueryTaskSummary (like QueryTaskMetrics does) to return an immediate error when
ctx == nil before calling loadTaskSnapshot; update the function to return an
empty TaskSummary and the same error message/type used by QueryTaskMetrics so
behavior and error messaging are consistent across the API (locate the check
inside the QueryTaskSummary function before the call to loadTaskSnapshot).
In `@internal/store/globaldb/global_db_task.go`:
- Around line 183-185: The deferred call currently swallows the error from
rows.Close() using "_ = rows.Close()" which violates the guideline to never
ignore errors; update the defer in global_db_task.go (the defer wrapping
rows.Close()) to either log the returned error using the existing logger or add
a concise comment justifying why the error is intentionally ignored (e.g.,
non-actionable after result processing), for example replacing the blank
assignment with a small if err := rows.Close(); err != nil { /* log or justify
*/ } or adding a one-line justification comment above the defer referencing
rows.Close().
- Around line 567-581: The function ensureTaskExists conflates empty input with
"not found"; change the initial validation so that when
strings.TrimSpace(taskID) yields an empty string it returns a distinct
validation error (e.g. taskpkg.ErrInvalidTaskID or a new exported error in
taskpkg like ErrInvalidTaskID/ErrEmptyTaskID with a clear message) instead of
taskpkg.ErrTaskNotFound, update the error construction to use that error, and
ensure any callers expecting taskpkg.ErrTaskNotFound for non-existent records
remain unchanged.
🪄 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: fb3213a7-e659-40b7-96fd-6ce8a8bf61a7
⛔ Files ignored due to path filters (33)
.compozy/tasks/core-tasks/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/_tasks.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_11.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_12.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_13.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_01.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_02.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_03.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_04.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_05.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_06.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_07.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_08.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_09.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_10.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_11.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_12.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_13.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonsdk/examples/telegram-reference/extension.tomlis excluded by!**/*.tomlsdk/typescript/src/generated/contracts.tsis excluded by!**/generated/**web/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (98)
internal/api/contract/automation.gointernal/api/contract/contract_test.gointernal/api/contract/responses.gointernal/api/contract/tasks.gointernal/api/core/automation.gointernal/api/core/automation_additional_test.gointernal/api/core/automation_test.gointernal/api/core/conversions.gointernal/api/core/errors.gointernal/api/core/errors_test.gointernal/api/core/handlers.gointernal/api/core/handlers_internal_test.gointernal/api/core/interfaces.gointernal/api/core/tasks.gointernal/api/core/tasks_integration_test.gointernal/api/core/tasks_internal_test.gointernal/api/core/tasks_test.gointernal/api/core/test_helpers_test.gointernal/api/httpapi/handlers.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/helpers_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/routes.gointernal/api/httpapi/server.gointernal/api/httpapi/server_test.gointernal/api/spec/spec.gointernal/api/spec/spec_test.gointernal/api/testutil/apitest.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/helpers_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/server.gointernal/api/udsapi/server_test.gointernal/api/udsapi/udsapi_integration_test.gointernal/automation/dispatch.gointernal/automation/dispatch_test.gointernal/automation/manager.gointernal/automation/manager_integration_test.gointernal/automation/manager_test.gointernal/automation/model/types.gointernal/automation/model/validate.gointernal/automation/types.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/helpers_test.gointernal/cli/root.gointernal/cli/task.gointernal/cli/task_test.gointernal/config/automation.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/task_runtime.gointernal/daemon/task_runtime_test.gointernal/extension/capability.gointernal/extension/contract/host_api.gointernal/extension/host_api.gointernal/extension/host_api_integration_test.gointernal/extension/host_api_tasks.gointernal/extension/host_api_test.gointernal/extension/manager_test.gointernal/extension/protocol/host_api.gointernal/extension/protocol/host_api_test.gointernal/network/audit.gointernal/network/audit_test.gointernal/network/manager.gointernal/network/tasks.gointernal/network/tasks_integration_test.gointernal/network/tasks_test.gointernal/observe/health.gointernal/observe/observer.gointernal/observe/tasks.gointernal/observe/tasks_integration_test.gointernal/observe/tasks_test.gointernal/session/stop_reason.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_automation.gointernal/store/globaldb/global_db_automation_test.gointernal/store/globaldb/global_db_task.gointernal/store/globaldb/global_db_task_aux.gointernal/store/globaldb/global_db_task_graph_audit_integration_test.gointernal/store/globaldb/global_db_task_graph_audit_test.gointernal/store/globaldb/global_db_task_integration_test.gointernal/store/globaldb/global_db_task_test.gointernal/task/actors.gointernal/task/doc.gointernal/task/errors.gointernal/task/interfaces.gointernal/task/interfaces_integration_test.gointernal/task/limits.gointernal/task/manager.gointernal/task/manager_integration_test.gointernal/task/manager_test.gointernal/task/types.gointernal/task/validate.gointernal/task/validate_test.go
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
internal/automation/dispatch.go (1)
469-483:⚠️ Potential issue | 🟠 Major
CreateTask+EnqueueRunis still not retry-safe.If
CreateTasksucceeds andEnqueueRunfails, the retry path will create a second task for the same automation run and orphan the first one. This is the same duplicate/orphan failure mode raised earlier; it still needs either a stable task identity derived fromscheduledRun.IDor a single atomic task+run creation API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/automation/dispatch.go` around lines 469 - 483, CreateTask followed by EnqueueRun is not retry-safe: if CreateTask succeeds but EnqueueRun fails you can end up with duplicated/orphaned tasks; make task creation idempotent by deriving a stable task identity from scheduledRun.ID and using that when creating/enqueuing so retries use the same task instead of creating another. Concretely, change the flow around tasks.CreateTask and tasks.EnqueueRun (or the directTaskSpec builder) so you compute a deterministic task ID (e.g., automationTaskID := "automation:"+scheduledRun.ID) and pass that into CreateTask (or call a CreateTaskWithID API / set the task.ID field in directTaskSpec) and then call EnqueueRun using that same TaskID (and keep using automationTaskRunIdempotencyKey for the run); alternatively, replace the two-step call with an atomic CreateTask+Run API if available.
🧹 Nitpick comments (4)
internal/observe/health.go (1)
49-52: Consider graceful degradation instead of failing the full health snapshot.
collectTaskHealthnow gates the entireHealth(ctx)response. Since task health aggregation depends on multiple queries, transient task-store failures can make the whole health endpoint fail. Consider returning core health with a degraded task status (plus telemetry) rather than hard-failing the full snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/observe/health.go` around lines 49 - 52, The current Health(ctx) flow hard-fails when collectTaskHealth returns an error; change this to graceful degradation by catching the error from collectTaskHealth, emitting telemetry/logging with the error, and constructing a task-health object that marks tasks as degraded (include the error message or code) instead of returning early. Update the Health(ctx) return path to merge core health with this degraded task section so the endpoint still returns a snapshot, and ensure you reference collectTaskHealth and the Health(ctx) method when making the change.internal/automation/validate_test.go (1)
832-841: Cover the mirrored delegated validation case too.This only locks down the missing
task_idbranch. A regression in thetask_run_idrequirement for delegated runs would still slip through untested.Suggested follow-up test
delegatedMissingTaskID := Run{ Status: RunDelegated, Attempt: 1, TaskRunID: "task-run-1", } if err := delegatedMissingTaskID.Validate("run"); err == nil { t.Fatal("Run.Validate(delegated missing task id) error = nil, want non-nil") } else if got := err.Error(); !strings.Contains(got, "run.task_id is required when run.status is \"delegated\"") { t.Fatalf("Run.Validate(delegated missing task id) error = %q, want delegated task_id failure", got) } + + delegatedMissingTaskRunID := Run{ + Status: RunDelegated, + Attempt: 1, + TaskID: "task-1", + } + if err := delegatedMissingTaskRunID.Validate("run"); err == nil { + t.Fatal("Run.Validate(delegated missing task run id) error = nil, want non-nil") + } else if got := err.Error(); !strings.Contains(got, "run.task_run_id is required when run.status is \"delegated\"") { + t.Fatalf("Run.Validate(delegated missing task run id) error = %q, want delegated task_run_id failure", got) + }As per coding guidelines,
**/*_test.go: Focus on critical paths: workflow execution, state management, error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/automation/validate_test.go` around lines 832 - 841, Add a mirrored test for the delegated-run validation that ensures TaskRunID is required: create a Run with Status set to RunDelegated and Attempt=1 that has TaskID set (e.g., "task-1") but an empty TaskRunID, call Validate("run") and assert it returns non-nil and that err.Error() contains the message about run.task_run_id being required when run.status is "delegated"; reference the existing delegatedMissingTaskID pattern and the Run.Validate, RunDelegated, TaskID and TaskRunID symbols when adding this test.internal/api/httpapi/httpapi_integration_test.go (1)
1034-1171: Mark these new subtests as parallel.Each subtest spins up its own isolated runtime, so they look independent. Adding
t.Parallel()here keeps the integration suite aligned with the repo test rules and makes race coverage more useful.Suggested change
t.Run("Should enqueue claim start and complete a task run", func(t *testing.T) { + t.Parallel() runtime := newIntegrationRuntime(t) created := createIntegrationTask(t, runtime, []byte(`{"scope":"global","title":"Run task routes"}`)) // ... }) t.Run("Should attach a claimed run session and then fail it", func(t *testing.T) { + t.Parallel() runtime := newIntegrationRuntime(t) created := createIntegrationTask(t, runtime, []byte(`{"scope":"global","title":"Run task routes"}`)) // ... }) t.Run("Should cancel one queued task run", func(t *testing.T) { + t.Parallel() runtime := newIntegrationRuntime(t) created := createIntegrationTask(t, runtime, []byte(`{"scope":"global","title":"Run task routes"}`)) // ... })As per coding guidelines,
**/*_test.go: Uset.Parallel()for independent subtests in Go tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/httpapi/httpapi_integration_test.go` around lines 1034 - 1171, The three subtests ("Should enqueue claim start and complete a task run", "Should attach a claimed run session and then fail it", "Should cancel one queued task run") are not marked parallel; add t.Parallel() as the first statement inside each t.Run closure so they run concurrently. Locate the t.Run blocks with those exact names in httpapi_integration_test.go and insert t.Parallel() at the top of each anonymous test function (before calling newIntegrationRuntime / createIntegrationTask / enqueueIntegrationTaskRun) to make each subtest parallel-safe.internal/api/core/tasks.go (1)
395-401: Push run filtering and limiting into the task service.
ListTaskRunscurrently loads the full task view and then filters runs in memory. For tasks with many runs, that turns a paged run listing into a full detail read and bypasses storage-side filtering/limit enforcement. A dedicatedListTaskRuns/GetTaskRunsservice call would scale much better here.Also applies to: 1078-1092
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/tasks.go` around lines 395 - 401, The handler currently calls manager.GetTask to load the entire Task view and then filters/limits runs in-memory using filterTaskRuns(view.Runs, query), which forces full reads for tasks with many runs; add a new service method (e.g., manager.ListTaskRuns or manager.GetTaskRuns) that accepts (ctx, taskID, query, actor) and performs storage-side filtering and limit enforcement, then replace the GetTask+filterTaskRuns usage in this handler (and the similar occurrence around lines 1078-1092) with a call to the new method and convert its result to contract.TaskRunsResponse via TaskRunPayloadsFromRuns; ensure the new service method is implemented in the task manager and storage layers to apply paging/filters and respect authorization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/core/tasks.go`:
- Around line 699-705: The handler currently resolves workspaceRef via
h.lookupWorkspaceID whenever a workspace query param is supplied, which can leak
workspace existence; change the logic to first inspect the normalized scope (the
request's scope parameter or the code path that sets query.Scope) and only call
h.lookupWorkspaceID and set query.WorkspaceID when the request is actually
workspace-scoped (i.e., scope != "global" and matches the workspace-scoped
enum/value); apply the same conditional change to the other occurrences noted
(the similar blocks around the other occurrences for lines 738-741 and 764-767)
so workspace resolution only happens for workspace-scoped requests.
In `@internal/automation/dispatch.go`:
- Around line 139-144: The interface AutomationSessionTaskActorRecorder must
accept context propagation: change RecordAutomationSessionTaskActor(sessionID
string, actor taskpkg.ActorContext) error to
RecordAutomationSessionTaskActor(ctx context.Context, sessionID string, actor
taskpkg.ActorContext) error and change
DeleteAutomationSessionTaskActor(sessionID string) to
DeleteAutomationSessionTaskActor(ctx context.Context) error (make it return
error so callers can handle failures); then update all implementations and call
sites to pass the inbound ctx through (do not create background contexts) and
handle/propagate returned errors accordingly.
- Around line 469-483: The task-backed path currently always records
CreateTask/EnqueueRun errors as RunFailed; change it to classify the error like
the session-backed path by calling classifyDispatchError(err) and passing that
status to d.finishRun instead of hard-coding RunFailed so cancellations/timeouts
become RunCancelled; update both error returns after d.tasks.CreateTask(ctx,
...) and d.tasks.EnqueueRun(ctx, ...) to compute status :=
classifyDispatchError(err) (or equivalent) and call d.finishRun(ctx,
scheduledRun, status, err) so lifecycle hooks and retry logic behave correctly.
In `@internal/automation/model/validate.go`:
- Around line 349-355: JobTaskConfig.Validate currently only validates c.Owner
and ignores c.NetworkChannel (job.task.network_channel), so invalid
network_channel values slip through; update the JobTaskConfig.Validate(path
string) error method to validate c.NetworkChannel (e.g., call its Validate
method or perform the same checks used elsewhere) using nestedPath(path,
"network_channel") and return any error found; reference the JobTaskConfig type,
the Validate method, c.NetworkChannel (or network_channel field) and nestedPath
to locate where to add the check.
In `@internal/cli/task_test.go`:
- Around line 580-604: The assertions are out of sync with the current
renderers: update the tests to match what taskDetailBundle(...).toon() actually
emits (check for task, runs and dependencies sections rather than child/event
sections), remove expectations for "Idempotency Key" and "Result" from
taskRunBundle(...).human() (keep a generic check like "Task Run" and at least
one run field such as "Status" or "Started At"), and tighten the
taskRunListBundle(...).toon() assertion to the smaller schema it emits (e.g.,
assert for a compact task_runs array like "task_runs[1]{id,status,attempt}"
instead of the long field list). Ensure you modify the three assertions that
reference taskDetailBundle().toon(), taskRunBundle().human(), and
taskRunListBundle().toon() accordingly.
- Around line 607-611: The test is inverted: parseOptionalTaskDependencyKind
accepts "relates" so the negative assertion should be changed to expect success;
update the second assertion in the test to call
parseOptionalTaskDependencyKind("relates") and assert err == nil and kind ==
taskpkg.DependencyKindRelates (or the appropriate constant), instead of checking
for an "unsupported value" error, so both supported values ("blocks" and
"relates") are validated as successful parses.
In `@internal/cli/task.go`:
- Around line 751-780: parseTaskListFilters currently allows a half-specified
owner filter (only owner-kind or only owner-ref) which results in invalid API
requests; update parseTaskListFilters to validate that ownerKindRaw and ownerRef
are provided together: trim ownerRef (strings.TrimSpace(ownerRef)) and if one is
non-empty while the other is empty return a usage error (e.g., fmt.Errorf("both
--owner-kind and --owner-ref must be specified together")) before calling
parseOptionalTaskOwnerKind and before building TaskListQuery; ensure the error
is returned early so TaskListQuery never contains a mismatched owner filter.
In `@internal/daemon/daemon_test.go`:
- Around line 690-783: Wrap the entire test body of
TestBootExtensionsKeepsHealthyRegisteredExtensionsAfterPartialStartFailure in a
t.Run subtest using the "Should..." naming convention (e.g. t.Run("Should keep
healthy registered extensions after partial start failure", func(t *testing.T) {
... })); move t.Parallel() into that subtest and keep all setup/assertions (db
:= openDaemonTestGlobalDB, installDaemonTestExtension, runtime :=
&fakeExtensionRuntime{...}, d.newExtensionManager, state := &bootState{...},
cleanup := &bootCleanup{}, call to d.bootExtensions, and all checks) inside the
subtest closure so the top-level Test... function only invokes t.Run with the
described name.
In `@internal/extension/host_api_test.go`:
- Around line 1740-1770: The subtest names in the table (the slice named tests
and its entries used in t.Run(tt.name, ...)) don't follow the required
"Should..." pattern; update each test case's name field (e.g., "CreateDenied",
"UpdateDenied", "RunStartDenied" and all similar names in the other blocks) to
be prefixed with "Should" and a descriptive action (e.g., "Should deny create",
"Should deny update", "Should deny run start"), and keep the test body unchanged
(the call to env.call and the assertCapabilityDenied assertions). Ensure you
update the string used in t.Run(...) to the new "Should..." value so all
subtests comply with the naming convention.
- Around line 1012-1019: The test's select that waits on the turnEnded notifier
is using a 500ms timeout which is too short for CI; update the time.After(500 *
time.Millisecond) to a larger duration (e.g. 2s or make it configurable) so the
case reading from the turnEnded channel (notifiedSessionID compared to sess.ID)
has more leeway and reduces flakiness in CI.
- Around line 2745-2785: The executor currently returns raw errors from session
operations; wrap each returned error with contextual text using fmt.Errorf so
failures are identifiable: when creating sessions wrap the error from
e.sessions.Create (in the Create flow that assigns created, err), when attaching
wrap the error from e.sessions.Status in AttachTaskSession, and when stopping
wrap the errors returned by e.sessions.RequestStopWithCause in RequestTaskStop
and e.sessions.StopWithCause in ForceTaskStop — use messages like "create
session: %w", "attach session <sessionID>: %w", "request stop <sessionID>: %w"
and "force stop <sessionID>: %w" (trim sessionID where appropriate) to preserve
the original error while adding operation context.
In `@internal/extension/manager_test.go`:
- Around line 760-762: Tests currently assert errors by matching substrings of
err.Error() (e.g., in manager_test.go around Reload/Start/Stop checks); define
package-level sentinel errors in manager.go (e.g., ErrContextRequired,
ErrManagerRequired, ErrRegistryRequired, etc.) and replace the string-based
errors.New(...) returns in the manager methods with those sentinel variables,
then update the tests (checks around Reload, Start, Stop and other mentioned
lines) to use errors.Is(err, ErrManagerRequired) (or the appropriate sentinel)
instead of strings.Contains, ensuring all relevant error returns and test
assertions reference the new sentinel symbols like ErrManagerRequired,
ErrContextRequired, ErrRegistryRequired and the methods Reload/Start/Stop.
In `@internal/network/tasks_integration_test.go`:
- Around line 243-247: The test cleanup currently calls db.Close(ctx) using the
test context obtained via testutil.Context(t), which may be cancelled when the
test finishes; change the cleanup to call db.Close(context.Background()) so the
close runs with a non-cancelled context—update the t.Cleanup anonymous function
to call db.Close(context.Background()) instead of db.Close(ctx) (referencing the
existing t.Cleanup(func() { ... }) and db.Close(...) call).
---
Duplicate comments:
In `@internal/automation/dispatch.go`:
- Around line 469-483: CreateTask followed by EnqueueRun is not retry-safe: if
CreateTask succeeds but EnqueueRun fails you can end up with duplicated/orphaned
tasks; make task creation idempotent by deriving a stable task identity from
scheduledRun.ID and using that when creating/enqueuing so retries use the same
task instead of creating another. Concretely, change the flow around
tasks.CreateTask and tasks.EnqueueRun (or the directTaskSpec builder) so you
compute a deterministic task ID (e.g., automationTaskID :=
"automation:"+scheduledRun.ID) and pass that into CreateTask (or call a
CreateTaskWithID API / set the task.ID field in directTaskSpec) and then call
EnqueueRun using that same TaskID (and keep using
automationTaskRunIdempotencyKey for the run); alternatively, replace the
two-step call with an atomic CreateTask+Run API if available.
---
Nitpick comments:
In `@internal/api/core/tasks.go`:
- Around line 395-401: The handler currently calls manager.GetTask to load the
entire Task view and then filters/limits runs in-memory using
filterTaskRuns(view.Runs, query), which forces full reads for tasks with many
runs; add a new service method (e.g., manager.ListTaskRuns or
manager.GetTaskRuns) that accepts (ctx, taskID, query, actor) and performs
storage-side filtering and limit enforcement, then replace the
GetTask+filterTaskRuns usage in this handler (and the similar occurrence around
lines 1078-1092) with a call to the new method and convert its result to
contract.TaskRunsResponse via TaskRunPayloadsFromRuns; ensure the new service
method is implemented in the task manager and storage layers to apply
paging/filters and respect authorization.
In `@internal/api/httpapi/httpapi_integration_test.go`:
- Around line 1034-1171: The three subtests ("Should enqueue claim start and
complete a task run", "Should attach a claimed run session and then fail it",
"Should cancel one queued task run") are not marked parallel; add t.Parallel()
as the first statement inside each t.Run closure so they run concurrently.
Locate the t.Run blocks with those exact names in httpapi_integration_test.go
and insert t.Parallel() at the top of each anonymous test function (before
calling newIntegrationRuntime / createIntegrationTask /
enqueueIntegrationTaskRun) to make each subtest parallel-safe.
In `@internal/automation/validate_test.go`:
- Around line 832-841: Add a mirrored test for the delegated-run validation that
ensures TaskRunID is required: create a Run with Status set to RunDelegated and
Attempt=1 that has TaskID set (e.g., "task-1") but an empty TaskRunID, call
Validate("run") and assert it returns non-nil and that err.Error() contains the
message about run.task_run_id being required when run.status is "delegated";
reference the existing delegatedMissingTaskID pattern and the Run.Validate,
RunDelegated, TaskID and TaskRunID symbols when adding this test.
In `@internal/observe/health.go`:
- Around line 49-52: The current Health(ctx) flow hard-fails when
collectTaskHealth returns an error; change this to graceful degradation by
catching the error from collectTaskHealth, emitting telemetry/logging with the
error, and constructing a task-health object that marks tasks as degraded
(include the error message or code) instead of returning early. Update the
Health(ctx) return path to merge core health with this degraded task section so
the endpoint still returns a snapshot, and ensure you reference
collectTaskHealth and the Health(ctx) method when making the change.
🪄 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: 17f611f9-9409-4d08-aed8-838087420c74
⛔ Files ignored due to path filters (34)
.compozy/tasks/core-tasks/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_031.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonweb/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
internal/api/contract/contract_test.gointernal/api/contract/tasks.gointernal/api/core/tasks.gointernal/api/core/tasks_internal_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/spec/spec.gointernal/api/spec/spec_test.gointernal/api/udsapi/server_test.gointernal/automation/dispatch.gointernal/automation/dispatch_test.gointernal/automation/manager_test.gointernal/automation/model/validate.gointernal/automation/validate_test.gointernal/cli/task.gointernal/cli/task_test.gointernal/config/automation.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/extension/host_api_tasks.gointernal/extension/host_api_test.gointernal/extension/manager_test.gointernal/network/audit_test.gointernal/network/tasks_integration_test.gointernal/observe/health.gointernal/observe/tasks_test.gointernal/session/stop_reason.gointernal/session/stop_reason_test.gointernal/store/globaldb/global_db_task.go
✅ Files skipped from review due to trivial changes (2)
- internal/network/audit_test.go
- internal/api/core/tasks_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/automation/manager_test.go
- internal/extension/host_api_tasks.go
- internal/automation/dispatch_test.go
- internal/api/spec/spec_test.go
- internal/api/udsapi/server_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (4)
internal/extension/host_api_test.go (1)
2288-2288:⚠️ Potential issue | 🟠 MajorRename nested subtest groups to the required
Should...format.The new subtest group names (
"MissingManager"and"InvalidInputs") break the required naming pattern already used elsewhere in this file.💡 Suggested rename
- t.Run("MissingManager", func(t *testing.T) { + t.Run("ShouldRejectWhenTaskManagerIsMissing", func(t *testing.T) { @@ - t.Run("InvalidInputs", func(t *testing.T) { + t.Run("ShouldRejectInvalidInputs", func(t *testing.T) {As per coding guidelines, "MUST use t.Run("Should...") pattern for ALL test cases".
Also applies to: 2334-2334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_test.go` at line 2288, The subtest declarations use non-conforming names t.Run("MissingManager", ...) and t.Run("InvalidInputs", ...) and must follow the required pattern; rename these nested t.Run calls to start with "Should..." (e.g., t.Run("Should report missing manager"...) or similar descriptive "Should..." phrases) so they match the rest of the file's naming convention; update the names in the test function where t.Run is invoked (the nested subtest groups currently labeled "MissingManager" and "InvalidInputs") and ensure any references/assertion messages expecting the old names are adjusted accordingly.internal/automation/dispatch.go (2)
141-144: 🛠️ Refactor suggestion | 🟠 MajorPass
context.Contextthrough the recorder boundary.This interface crosses from dispatch into persistence/provenance code, but callers cannot propagate cancellation or observe cleanup failures because
DeleteAutomationSessionTaskActorreturns nothing. Make both methods...(ctx context.Context, ...) errorand thread the dispatch context through the helpers and call sites.#!/bin/bash set -euo pipefail rg -n -C2 'type AutomationSessionTaskActorRecorder interface|RecordAutomationSessionTaskActor|DeleteAutomationSessionTaskActor' --type goAs per coding guidelines,
Use context.Context as first argument to functions crossing runtime boundaries — avoid context.Background() outside main and focused tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/automation/dispatch.go` around lines 141 - 144, Update the AutomationSessionTaskActorRecorder API to accept and propagate context.Context: change both RecordAutomationSessionTaskActor and DeleteAutomationSessionTaskActor to have signatures like RecordAutomationSessionTaskActor(ctx context.Context, sessionID string, actor taskpkg.ActorContext) error and DeleteAutomationSessionTaskActor(ctx context.Context, sessionID string) error; then modify all implementations of AutomationSessionTaskActorRecorder and every caller in dispatch.go and related helpers to pass the dispatch context as the first argument and handle returned errors (no context.Background() usage). Ensure context is threaded through helper functions that interact with the recorder and that DeleteAutomationSessionTaskActor now returns errors to be observed and logged/handled.
469-483:⚠️ Potential issue | 🟠 Major
CreateTaskandEnqueueRunstill leave orphaned tasks on partial failure.If
CreateTasksucceeds andEnqueueRunfails, the automation run is finished as failed/cancelled beforedelegateRun()ever recordsTaskID. That leaves a durable task with no linked automation run and no recovery path in this flow. Please make creation+enqueue atomic, or at least persist the createdTaskIDbefore enqueue so reconciliation/cleanup can find the orphan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/automation/dispatch.go` around lines 469 - 483, CreateTask followed by EnqueueRun can leave orphan tasks if EnqueueRun fails; fix by making creation+enqueue atomic or persisting the created TaskID before attempting EnqueueRun. Specifically, in the dispatch logic around d.tasks.CreateTask and d.tasks.EnqueueRun, after CreateTask returns a non-empty taskRecord.ID, immediately persist that TaskID onto the automation run record (e.g., call the existing delegateRun/update run path that records TaskID or update scheduledRun with TaskID) so reconciliation/cleanup can find it; alternatively, if you cannot persist first, ensure you clean up the created task on EnqueueRun failure (delete the task in the error branch) and/or perform both actions inside a transactional operation so delegateRun()/finishRun() sees a recorded TaskID in all failure cases.internal/api/core/tasks.go (1)
699-707:⚠️ Potential issue | 🟠 MajorOnly resolve
workspacefor explicit workspace scope.
scope=omitted or invalid still falls through tolookupWorkspaceID(). That means/api/tasks?workspace=...can still return404when the workspace is missing and400when it exists, which leaks workspace existence and makes the status depend on unrelated data. Gate the lookup onquery.Scope == taskpkg.ScopeWorkspaceand validate every other scope/binding combination before resolving the ref.Suggested fix
if workspaceRef := strings.TrimSpace(c.Query("workspace")); workspaceRef != "" { - if query.Scope.Normalize() == taskpkg.ScopeGlobal { + if query.Scope.Normalize() != taskpkg.ScopeWorkspace { return taskpkg.TaskQuery{}, taskpkg.ValidateScopeBinding(query.Scope, workspaceRef, "task_query", "workspace") } workspaceID, err := h.lookupWorkspaceID(ctx, workspaceRef) if err != nil { return taskpkg.TaskQuery{}, err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/tasks.go` around lines 699 - 707, The code currently calls lookupWorkspaceID for any non-global scope when a workspace query param is present, leaking workspace existence; change the logic in the handler that builds taskpkg.TaskQuery so that you only call h.lookupWorkspaceID(ctx, workspaceRef) when query.Scope.Normalize() == taskpkg.ScopeWorkspace; for all other cases first run taskpkg.ValidateScopeBinding(query.Scope, workspaceRef, "task_query", "workspace") (and return its error if any) and do not resolve the workspace ref or set query.WorkspaceID, ensuring lookup happens only for explicit workspace scope and other scope/binding combinations are validated without performing the lookup.
🧹 Nitpick comments (1)
internal/automation/model/validate.go (1)
351-357: Reuse the shared channel validator here.
JobTaskConfignow hardcodes its own regex, but task transport validation goes throughinternal/network.ValidateChannelininternal/api/core/tasks.go. If those rules drift, automation config can pass model validation and still fail when the delegated task is materialized. Prefer calling the shared validator instead of maintaining a second pattern.Suggested fix
import ( "errors" "fmt" - "regexp" "strings" "time" + "github.com/pedronauck/agh/internal/network" cron "github.com/robfig/cron/v3" ) @@ -var jobTaskNetworkChannelPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_-]{0,63}$`) - @@ func (c JobTaskConfig) Validate(path string) error { if channel := strings.TrimSpace(c.NetworkChannel); channel != "" { - if !jobTaskNetworkChannelPattern.MatchString(channel) { - return fmt.Errorf("%s is invalid: channel=%q", nestedPath(path, "network_channel"), channel) + if err := network.ValidateChannel(channel); err != nil { + return fmt.Errorf("%s is invalid: %w", nestedPath(path, "network_channel"), err) } } if c.Owner != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/automation/model/validate.go` around lines 351 - 357, Replace the inline regex check in JobTaskConfig.Validate for NetworkChannel with the shared validator: call internal/network.ValidateChannel(channel) (or its exported name) instead of using jobTaskNetworkChannelPattern so validation rules are centralized; on error return fmt.Errorf("%s is invalid: %v", nestedPath(path, "network_channel"), err) to preserve the existing nestedPath context and remove the duplicated jobTaskNetworkChannelPattern usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/api/core/tasks.go`:
- Around line 699-707: The code currently calls lookupWorkspaceID for any
non-global scope when a workspace query param is present, leaking workspace
existence; change the logic in the handler that builds taskpkg.TaskQuery so that
you only call h.lookupWorkspaceID(ctx, workspaceRef) when
query.Scope.Normalize() == taskpkg.ScopeWorkspace; for all other cases first run
taskpkg.ValidateScopeBinding(query.Scope, workspaceRef, "task_query",
"workspace") (and return its error if any) and do not resolve the workspace ref
or set query.WorkspaceID, ensuring lookup happens only for explicit workspace
scope and other scope/binding combinations are validated without performing the
lookup.
In `@internal/automation/dispatch.go`:
- Around line 141-144: Update the AutomationSessionTaskActorRecorder API to
accept and propagate context.Context: change both
RecordAutomationSessionTaskActor and DeleteAutomationSessionTaskActor to have
signatures like RecordAutomationSessionTaskActor(ctx context.Context, sessionID
string, actor taskpkg.ActorContext) error and
DeleteAutomationSessionTaskActor(ctx context.Context, sessionID string) error;
then modify all implementations of AutomationSessionTaskActorRecorder and every
caller in dispatch.go and related helpers to pass the dispatch context as the
first argument and handle returned errors (no context.Background() usage).
Ensure context is threaded through helper functions that interact with the
recorder and that DeleteAutomationSessionTaskActor now returns errors to be
observed and logged/handled.
- Around line 469-483: CreateTask followed by EnqueueRun can leave orphan tasks
if EnqueueRun fails; fix by making creation+enqueue atomic or persisting the
created TaskID before attempting EnqueueRun. Specifically, in the dispatch logic
around d.tasks.CreateTask and d.tasks.EnqueueRun, after CreateTask returns a
non-empty taskRecord.ID, immediately persist that TaskID onto the automation run
record (e.g., call the existing delegateRun/update run path that records TaskID
or update scheduledRun with TaskID) so reconciliation/cleanup can find it;
alternatively, if you cannot persist first, ensure you clean up the created task
on EnqueueRun failure (delete the task in the error branch) and/or perform both
actions inside a transactional operation so delegateRun()/finishRun() sees a
recorded TaskID in all failure cases.
In `@internal/extension/host_api_test.go`:
- Line 2288: The subtest declarations use non-conforming names
t.Run("MissingManager", ...) and t.Run("InvalidInputs", ...) and must follow the
required pattern; rename these nested t.Run calls to start with "Should..."
(e.g., t.Run("Should report missing manager"...) or similar descriptive
"Should..." phrases) so they match the rest of the file's naming convention;
update the names in the test function where t.Run is invoked (the nested subtest
groups currently labeled "MissingManager" and "InvalidInputs") and ensure any
references/assertion messages expecting the old names are adjusted accordingly.
---
Nitpick comments:
In `@internal/automation/model/validate.go`:
- Around line 351-357: Replace the inline regex check in JobTaskConfig.Validate
for NetworkChannel with the shared validator: call
internal/network.ValidateChannel(channel) (or its exported name) instead of
using jobTaskNetworkChannelPattern so validation rules are centralized; on error
return fmt.Errorf("%s is invalid: %v", nestedPath(path, "network_channel"), err)
to preserve the existing nestedPath context and remove the duplicated
jobTaskNetworkChannelPattern usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bb18d51-b36c-45b8-a182-985f63890424
⛔ Files ignored due to path filters (21)
.agents/skills/compozy/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/cli-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/core-tasks/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_016.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_017.mdis excluded by!**/*.md
📒 Files selected for processing (18)
internal/api/core/interfaces.gointernal/api/core/tasks.gointernal/api/core/tasks_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/testutil/apitest.gointernal/automation/dispatch.gointernal/automation/dispatch_test.gointernal/automation/model/validate.gointernal/automation/validate_test.gointernal/cli/task.gointernal/cli/task_test.gointernal/daemon/daemon_test.gointernal/extension/host_api_test.gointernal/extension/manager.gointernal/extension/manager_test.gointernal/network/tasks_integration_test.gointernal/task/interfaces.gointernal/task/manager.go
✅ Files skipped from review due to trivial changes (4)
- internal/api/core/tasks_test.go
- internal/cli/task_test.go
- internal/daemon/daemon_test.go
- internal/cli/task.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/api/testutil/apitest.go
- internal/api/core/interfaces.go
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/core/test_helpers_test.go (1)
48-80:⚠️ Potential issue | 🟡 MinorMark the wrapper fixtures as test helpers.
newHandlerFixture,newHandlerFixtureWithAutomation, andnewHandlerFixtureWithTasksare test helpers too, but they do not callt.Helper(). That makes failures point at the wrapper instead of the calling test.Proposed fix
func newHandlerFixture( t *testing.T, manager testutil.StubSessionManager, observer testutil.StubObserver, workspaces testutil.StubWorkspaceService, store *memory.Store, dream core.DreamTrigger, ) handlerFixture { + t.Helper() return newHandlerFixtureWithAutomationAndTasks(t, manager, observer, testutil.StubAutomationManager{}, testutil.StubTaskManager{}, workspaces, store, dream) } func newHandlerFixtureWithAutomation( t *testing.T, manager testutil.StubSessionManager, observer testutil.StubObserver, automation testutil.StubAutomationManager, workspaces testutil.StubWorkspaceService, store *memory.Store, dream core.DreamTrigger, ) handlerFixture { + t.Helper() return newHandlerFixtureWithAutomationAndTasks(t, manager, observer, automation, testutil.StubTaskManager{}, workspaces, store, dream) } func newHandlerFixtureWithTasks( t *testing.T, manager testutil.StubSessionManager, observer testutil.StubObserver, tasks testutil.StubTaskManager, workspaces testutil.StubWorkspaceService, store *memory.Store, dream core.DreamTrigger, ) handlerFixture { + t.Helper() return newHandlerFixtureWithAutomationAndTasks(t, manager, observer, testutil.StubAutomationManager{}, tasks, workspaces, store, dream) }As per coding guidelines,
**/*_test.go: Uset.Helper()on test helper functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/test_helpers_test.go` around lines 48 - 80, The three wrapper test helper functions newHandlerFixture, newHandlerFixtureWithAutomation, and newHandlerFixtureWithTasks don't call t.Helper(), causing failures to be attributed to the wrapper instead of the caller; add t.Helper() as the first statement in each of these functions so the testing framework reports the correct call site (locate the functions and insert t.Helper() at the top of each function body before any other logic).
♻️ Duplicate comments (2)
internal/api/core/tasks.go (1)
699-707:⚠️ Potential issue | 🟠 MajorOnly resolve
workspacefor workspace-scoped queries.This still resolves the workspace whenever the normalized scope is anything other than
global, so empty or unsupported scopes can return lookup errors before query validation and still leak workspace existence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/tasks.go` around lines 699 - 707, Only resolve the workspace when the query scope is explicitly the workspace scope: change the branch around c.Query("workspace") so that if workspaceRef is non-empty and query.Scope.Normalize() == taskpkg.ScopeWorkspace you call h.lookupWorkspaceID(...) and set query.WorkspaceID; otherwise (workspaceRef provided but scope is not workspace) call taskpkg.ValidateScopeBinding(query.Scope, workspaceRef, "task_query", "workspace") and return its error. Update references: c.Query("workspace"), query.Scope.Normalize(), taskpkg.ScopeWorkspace, h.lookupWorkspaceID, taskpkg.ValidateScopeBinding, and query.WorkspaceID.internal/api/udsapi/udsapi_integration_test.go (1)
746-753:⚠️ Potential issue | 🟠 MajorProtect
startedfrom concurrent access.
StartTaskSessionincrements shared state without synchronization. These integration tests are expected to pass under-race, and concurrent task starts can race here and reuse session IDs.Proposed fix
type integrationTaskSessionExecutor struct { + mu sync.Mutex started int } func (e *integrationTaskSessionExecutor) StartTaskSession(_ context.Context, _ taskpkg.StartTaskSession) (*taskpkg.SessionRef, error) { + e.mu.Lock() + defer e.mu.Unlock() e.started++ return &taskpkg.SessionRef{SessionID: fmt.Sprintf("task-sess-%d", e.started)}, nil }As per coding guidelines,
**/*_test.go: Run tests with -race flag before committing — zero tolerance for race conditions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/udsapi/udsapi_integration_test.go` around lines 746 - 753, The field started in struct integrationTaskSessionExecutor is accessed concurrently in StartTaskSession causing races; fix by protecting increments and reads with synchronization: either add a sync.Mutex (e.g., mu sync.Mutex on integrationTaskSessionExecutor and guard the e.started++ and fmt.Sprintf call with mu.Lock()/mu.Unlock()) or change started to an int32 and use atomic.AddInt32(&e.started, 1) and atomic.LoadInt32 when building the SessionRef ID; update the struct and StartTaskSession implementation accordingly to eliminate the data race.
🧹 Nitpick comments (3)
internal/extension/manager_test.go (1)
31-37: Pin the new sink stub to its interface.Since
noopBridgeTelemetrySinkis now a reusable test double, add a compile-time interface assertion next to it so interface drift fails immediately instead of only where the stub is wired in.As per coding guidelines,
**/*.go: "Use compile-time interface verification:var _ Interface = (*Type)(nil)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/manager_test.go` around lines 31 - 37, Add a compile-time assertion that pins the noopBridgeTelemetrySink to the expected interface by adding: var _ interface{ RecordBridgeAuthFailure(string); RecordBridgeRuntimeIssue(string, bridgepkg.BridgeStatus, string); ClearBridgeRuntimeIssue(string) } = (*noopBridgeTelemetrySink)(nil) so any method-signature drift on noopBridgeTelemetrySink (methods RecordBridgeAuthFailure, RecordBridgeRuntimeIssue, ClearBridgeRuntimeIssue) fails at compile time.internal/daemon/task_runtime_test.go (1)
18-43: Test case names should use "Should" prefix pattern.Per coding guidelines, test cases should use
t.Run("Should...")pattern. Current names like"workspace task uses workspace id"and"claimed without session requeues"are descriptive but don't follow the prescribed format.As per coding guidelines, "MUST use t.Run('Should...') pattern for ALL test cases".
♻️ Example name adjustments
- name: "workspace task uses workspace id", + name: "Should use workspace id for workspace-scoped task", ... - name: "claimed without session requeues", + name: "Should requeue claimed run without session",Also applies to: 167-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/daemon/task_runtime_test.go` around lines 18 - 43, Rename the test case "name" values inside the testCases slice in internal/daemon/task_runtime_test.go to follow the t.Run "Should..." pattern (e.g., change "workspace task uses workspace id" to "Should use workspace id for workspace-scoped task" and "global task uses global workspace path" to "Should use global workspace path for global-scoped task"), and apply the same "Should..." naming convention to the other test case names mentioned (around the other occurrence referencing claimed/requeue tests); update the testCases variable entries and any t.Run invocations that use these names so all tests use the required "Should..." prefix.internal/extension/host_api_tasks.go (1)
187-191: UseListTaskRunsfor the runs endpoint instead of filteringGetTask().Runs.There is already a dedicated service query for this path. Pulling the full task view here means limit/status/session filtering happens after the fetch, so behavior and cost can drift from the transport API that already uses
TaskService.ListTaskRuns.Suggested change
- view, err := manager.GetTask(ctx, taskID, actor) + runs, err := manager.ListTaskRuns(ctx, taskID, query, actor) if err != nil { return nil, mapTaskRPCError("task", taskID, err) } - return taskRunPayloadsFromRuns(filterTaskRuns(view.Runs, query)), nil + return taskRunPayloadsFromRuns(runs), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/extension/host_api_tasks.go` around lines 187 - 191, Replace the full task fetch with the dedicated ListTaskRuns call: instead of calling manager.GetTask(ctx, taskID, actor) and filtering view.Runs, call manager.ListTaskRuns(ctx, taskID, actor, query) (or the TaskService.ListTaskRuns equivalent) to get the runs directly, handle the returned error with mapTaskRPCError (adjusting the resource name if needed), and pass the returned runs into taskRunPayloadsFromRuns rather than using view.Runs + filterTaskRuns; this ensures limit/status/session filtering occurs in the service query rather than post-fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/core/automation_additional_test.go`:
- Around line 87-114: The table-driven subtests (using performRequest,
fixture.Engine and t.Run) only assert HTTP status codes, which is too shallow
and the t.Run names don't follow the "Should..." pattern; update each subtest to
use t.Run("Should ...") and add at least one meaningful assertion per route: for
GETs assert a specific JSON field/value or structure in resp.Body (e.g., job
id/name for "/automation/jobs/job-1", trigger fields for
"/automation/triggers/trigger-1", run entries for runs endpoints) and for
DELETEs assert side-effects such as a subsequent GET to the same resource
returns 404 or that a list endpoint no longer contains the deleted id
(performRequest + resp.Body checks); keep using the existing table but include
an expected-check function or switch on request.path to validate payloads/side
effects rather than only status.
In `@internal/api/udsapi/server_test.go`:
- Around line 157-158: Replace string-based error matching with sentinel errors:
define exported package-level sentinel errors (ErrSessionManagerRequired,
ErrTaskServiceRequired, ErrObserverRequired, ErrWorkspaceResolverRequired) in
server.go and have New(...) return these sentinel errors for the respective
validation failures instead of constructing new errors each call; then update
the test in server_test.go to assert using errors.Is(err,
ErrSessionManagerRequired) (and the other sentinels) rather than
strings.Contains(err.Error(), ...), locating the logic around the New() call and
the validation branches that currently return errors.New.
In `@internal/cli/task_test.go`:
- Around line 16-51: The table-driven tests in the tests variable use
non-conforming names passed through t.Run(tt.name); rename each tt.name value to
the "Should ..." form (e.g. "Should require workspace for workspace scope",
"Should forbid workspace for global scope", "Should require change flags on
update", "Should reject clear owner with owner mutation") so t.Run receives
names that follow the enforced pattern; no behavioral code changes needed beyond
updating the string literals in the tests slice.
In `@internal/extension/host_api_tasks.go`:
- Around line 434-456: taskQueryFromParams currently resolves the workspace
(resolveTaskWorkspaceID) before validating the requested scope, which can
convert invalid-scope/empty-scope errors into "workspace not found"; instead,
normalize and validate the scope binding from params.Scope (call
Scope.Normalize() and any scope-binding validation you have) first and only call
resolveTaskWorkspaceID when the scope indicates a workspace-bound query. Update
taskQueryFromParams to: normalize/validate params.Scope, then conditionally call
resolveTaskWorkspaceID and set WorkspaceID; keep the existing
validateTaskChannel and query.Validate flow. Apply the same change to the other
function in this file that also calls resolveTaskWorkspaceID before scope
validation (search for other usages of resolveTaskWorkspaceID and fix them
similarly).
In `@internal/extension/host_api_test.go`:
- Around line 2288-2398: Rename the two outer test suite labels that violate the
t.Run("Should...") convention: change the t.Run call using the literal
"MissingManager" to a Should-prefixed label (e.g.,
"ShouldRejectWhenManagerMissing") and change the t.Run call using
"InvalidInputs" to a Should-prefixed label (e.g., "ShouldRejectInvalidInputs");
locate the offending calls by finding the t.Run invocations that wrap the
MissingManager and InvalidInputs blocks in internal/extension/host_api_test.go
and update only the string names to follow the "Should..." pattern.
In `@internal/network/audit.go`:
- Around line 128-149: The RecordTaskIngress method calls w.now() before
verifying the writer's sinks and before guarding against a nil now func, which
can panic on a zero-value FileAuditWriter and also silently return when no sinks
are configured; update RecordTaskIngress (in FileAuditWriter) to first ensure at
least one sink is set (w.path != "" || w.store != nil) and return a descriptive
error if none are configured, then compute the timestamp using a safe now: use
w.now() only if w.now != nil otherwise fall back to time.Now(), then pass that
timestamp to normalizeTaskIngressAuditEntry; keep conditional calls to
appendFile and store.WriteNetworkAudit unchanged but ensure they run after these
guards to avoid panics and silent drops.
- Around line 136-147: The errors returned from normalizeTaskIngressAuditEntry,
w.appendFile, w.store.WriteNetworkAudit, and entry.Validate need to be wrapped
with operation context using fmt.Errorf and %w before being returned or joined;
specifically, replace the raw return of err from normalizeTaskIngressAuditEntry
with a wrapped error like fmt.Errorf("network: normalize task ingress audit
entry: %w", err), wrap the results of w.appendFile(entry) and
w.store.WriteNetworkAudit(ctx, entry) similarly before they are combined into
recordErr (so recordErr = errors.Join(recordErr, fmt.Errorf("network: append
file audit entry: %w", err))) and wrap the error returned from entry.Validate()
with context (e.g., fmt.Errorf("network: validate audit entry: %w", err)) so all
propagated errors include clear operation-specific context.
---
Outside diff comments:
In `@internal/api/core/test_helpers_test.go`:
- Around line 48-80: The three wrapper test helper functions newHandlerFixture,
newHandlerFixtureWithAutomation, and newHandlerFixtureWithTasks don't call
t.Helper(), causing failures to be attributed to the wrapper instead of the
caller; add t.Helper() as the first statement in each of these functions so the
testing framework reports the correct call site (locate the functions and insert
t.Helper() at the top of each function body before any other logic).
---
Duplicate comments:
In `@internal/api/core/tasks.go`:
- Around line 699-707: Only resolve the workspace when the query scope is
explicitly the workspace scope: change the branch around c.Query("workspace") so
that if workspaceRef is non-empty and query.Scope.Normalize() ==
taskpkg.ScopeWorkspace you call h.lookupWorkspaceID(...) and set
query.WorkspaceID; otherwise (workspaceRef provided but scope is not workspace)
call taskpkg.ValidateScopeBinding(query.Scope, workspaceRef, "task_query",
"workspace") and return its error. Update references: c.Query("workspace"),
query.Scope.Normalize(), taskpkg.ScopeWorkspace, h.lookupWorkspaceID,
taskpkg.ValidateScopeBinding, and query.WorkspaceID.
In `@internal/api/udsapi/udsapi_integration_test.go`:
- Around line 746-753: The field started in struct
integrationTaskSessionExecutor is accessed concurrently in StartTaskSession
causing races; fix by protecting increments and reads with synchronization:
either add a sync.Mutex (e.g., mu sync.Mutex on integrationTaskSessionExecutor
and guard the e.started++ and fmt.Sprintf call with mu.Lock()/mu.Unlock()) or
change started to an int32 and use atomic.AddInt32(&e.started, 1) and
atomic.LoadInt32 when building the SessionRef ID; update the struct and
StartTaskSession implementation accordingly to eliminate the data race.
---
Nitpick comments:
In `@internal/daemon/task_runtime_test.go`:
- Around line 18-43: Rename the test case "name" values inside the testCases
slice in internal/daemon/task_runtime_test.go to follow the t.Run "Should..."
pattern (e.g., change "workspace task uses workspace id" to "Should use
workspace id for workspace-scoped task" and "global task uses global workspace
path" to "Should use global workspace path for global-scoped task"), and apply
the same "Should..." naming convention to the other test case names mentioned
(around the other occurrence referencing claimed/requeue tests); update the
testCases variable entries and any t.Run invocations that use these names so all
tests use the required "Should..." prefix.
In `@internal/extension/host_api_tasks.go`:
- Around line 187-191: Replace the full task fetch with the dedicated
ListTaskRuns call: instead of calling manager.GetTask(ctx, taskID, actor) and
filtering view.Runs, call manager.ListTaskRuns(ctx, taskID, actor, query) (or
the TaskService.ListTaskRuns equivalent) to get the runs directly, handle the
returned error with mapTaskRPCError (adjusting the resource name if needed), and
pass the returned runs into taskRunPayloadsFromRuns rather than using view.Runs
+ filterTaskRuns; this ensures limit/status/session filtering occurs in the
service query rather than post-fetch.
In `@internal/extension/manager_test.go`:
- Around line 31-37: Add a compile-time assertion that pins the
noopBridgeTelemetrySink to the expected interface by adding: var _ interface{
RecordBridgeAuthFailure(string); RecordBridgeRuntimeIssue(string,
bridgepkg.BridgeStatus, string); ClearBridgeRuntimeIssue(string) } =
(*noopBridgeTelemetrySink)(nil) so any method-signature drift on
noopBridgeTelemetrySink (methods RecordBridgeAuthFailure,
RecordBridgeRuntimeIssue, ClearBridgeRuntimeIssue) fails at compile time.
🪄 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: 83c3c247-76c6-4288-b9de-b1c663d7f9b5
⛔ Files ignored due to path filters (86)
.agents/skills/compozy/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/cli-reference.mdis excluded by!**/*.md,!.agents/**.agents/skills/compozy/references/config-reference.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/core-tasks/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/_tasks.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/MEMORY.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_01.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_02.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_03.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_04.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_05.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_06.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_07.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_08.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_09.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_10.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_11.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_12.mdis excluded by!**/*.md.compozy/tasks/core-tasks/memory/task_13.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_016.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_017.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_018.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_019.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_020.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_021.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_022.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_023.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_024.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_025.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_026.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_027.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_028.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_029.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_030.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-001/issue_031.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/_meta.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_016.mdis excluded by!**/*.md.compozy/tasks/core-tasks/reviews-002/issue_017.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_01.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_02.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_03.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_04.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_05.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_06.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_07.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_08.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_09.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_10.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_11.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_12.mdis excluded by!**/*.md.compozy/tasks/core-tasks/task_13.mdis excluded by!**/*.mdopenapi/agh.jsonis excluded by!**/*.jsonsdk/examples/telegram-reference/extension.tomlis excluded by!**/*.tomlsdk/typescript/src/generated/contracts.tsis excluded by!**/generated/**web/src/generated/agh-openapi.d.tsis excluded by!**/generated/**
📒 Files selected for processing (101)
internal/api/contract/automation.gointernal/api/contract/contract_test.gointernal/api/contract/responses.gointernal/api/contract/tasks.gointernal/api/core/automation.gointernal/api/core/automation_additional_test.gointernal/api/core/automation_test.gointernal/api/core/conversions.gointernal/api/core/errors.gointernal/api/core/errors_test.gointernal/api/core/handlers.gointernal/api/core/handlers_internal_test.gointernal/api/core/interfaces.gointernal/api/core/tasks.gointernal/api/core/tasks_integration_test.gointernal/api/core/tasks_internal_test.gointernal/api/core/tasks_test.gointernal/api/core/test_helpers_test.gointernal/api/httpapi/handlers.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/helpers_test.gointernal/api/httpapi/httpapi_integration_test.gointernal/api/httpapi/routes.gointernal/api/httpapi/server.gointernal/api/httpapi/server_test.gointernal/api/spec/spec.gointernal/api/spec/spec_test.gointernal/api/testutil/apitest.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/helpers_test.gointernal/api/udsapi/routes.gointernal/api/udsapi/server.gointernal/api/udsapi/server_test.gointernal/api/udsapi/udsapi_integration_test.gointernal/automation/dispatch.gointernal/automation/dispatch_test.gointernal/automation/manager.gointernal/automation/manager_integration_test.gointernal/automation/manager_test.gointernal/automation/model/types.gointernal/automation/model/validate.gointernal/automation/types.gointernal/automation/validate_test.gointernal/cli/cli_integration_test.gointernal/cli/client.gointernal/cli/client_test.gointernal/cli/helpers_test.gointernal/cli/root.gointernal/cli/task.gointernal/cli/task_test.gointernal/config/automation.gointernal/daemon/boot.gointernal/daemon/daemon.gointernal/daemon/daemon_integration_test.gointernal/daemon/daemon_test.gointernal/daemon/task_runtime.gointernal/daemon/task_runtime_test.gointernal/extension/capability.gointernal/extension/contract/host_api.gointernal/extension/host_api.gointernal/extension/host_api_integration_test.gointernal/extension/host_api_tasks.gointernal/extension/host_api_test.gointernal/extension/manager.gointernal/extension/manager_test.gointernal/extension/protocol/host_api.gointernal/extension/protocol/host_api_test.gointernal/network/audit.gointernal/network/audit_test.gointernal/network/manager.gointernal/network/tasks.gointernal/network/tasks_integration_test.gointernal/network/tasks_test.gointernal/observe/health.gointernal/observe/observer.gointernal/observe/tasks.gointernal/observe/tasks_integration_test.gointernal/observe/tasks_test.gointernal/session/stop_reason.gointernal/session/stop_reason_test.gointernal/store/globaldb/global_db.gointernal/store/globaldb/global_db_automation.gointernal/store/globaldb/global_db_automation_test.gointernal/store/globaldb/global_db_task.gointernal/store/globaldb/global_db_task_aux.gointernal/store/globaldb/global_db_task_graph_audit_integration_test.gointernal/store/globaldb/global_db_task_graph_audit_test.gointernal/store/globaldb/global_db_task_integration_test.gointernal/store/globaldb/global_db_task_test.gointernal/task/actors.gointernal/task/doc.gointernal/task/errors.gointernal/task/interfaces.gointernal/task/interfaces_integration_test.gointernal/task/limits.gointernal/task/manager.gointernal/task/manager_integration_test.gointernal/task/manager_test.gointernal/task/types.gointernal/task/validate.gointernal/task/validate_test.go
✅ Files skipped from review due to trivial changes (11)
- internal/api/httpapi/server_test.go
- internal/automation/validate_test.go
- internal/api/udsapi/handlers_test.go
- internal/api/core/automation.go
- internal/daemon/daemon_integration_test.go
- internal/extension/host_api_integration_test.go
- internal/api/core/handlers_internal_test.go
- internal/extension/manager.go
- internal/api/core/tasks_internal_test.go
- internal/api/contract/responses.go
- internal/cli/client.go
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/cli/root.go
- internal/api/udsapi/helpers_test.go
- internal/api/httpapi/handlers.go
- internal/api/udsapi/routes.go
- internal/extension/protocol/host_api_test.go
- internal/api/core/conversions.go
- internal/api/udsapi/server.go
- internal/automation/manager_test.go
- internal/api/httpapi/handlers_test.go
- internal/api/testutil/apitest.go
- internal/extension/capability.go
- internal/api/httpapi/server.go
- internal/api/core/errors_test.go
- internal/extension/host_api.go
- internal/automation/dispatch_test.go
- internal/extension/protocol/host_api.go
- internal/cli/task.go
- internal/api/contract/tasks.go
- internal/api/core/tasks_test.go
- internal/cli/client_test.go
- internal/automation/manager_integration_test.go
- internal/automation/manager.go
- internal/daemon/daemon_test.go
Comment on lines
+87
to
+114
| for _, request := range []struct { | ||
| method string | ||
| path string | ||
| }{ | ||
| {method: http.MethodGet, path: "/automation/jobs/job-1"}, | ||
| {method: http.MethodDelete, path: "/automation/jobs/job-1"}, | ||
| {method: http.MethodGet, path: "/automation/triggers"}, | ||
| {method: http.MethodGet, path: "/automation/triggers/trigger-1"}, | ||
| {method: http.MethodDelete, path: "/automation/triggers/trigger-1"}, | ||
| {method: http.MethodGet, path: "/automation/jobs/job-1/runs"}, | ||
| {method: http.MethodGet, path: "/automation/triggers/trigger-1/runs"}, | ||
| {method: http.MethodGet, path: "/automation/runs?limit=1"}, | ||
| {method: http.MethodGet, path: "/automation/runs/run-1"}, | ||
| } { | ||
| request := request | ||
| t.Run(request.method+" "+request.path, func(t *testing.T) { | ||
| resp := performRequest(t, fixture.Engine, request.method, request.path, nil) | ||
| if request.method == http.MethodDelete { | ||
| if resp.Code != http.StatusNoContent { | ||
| t.Fatalf("%s %s status = %d, want %d; body=%s", request.method, request.path, resp.Code, http.StatusNoContent, resp.Body.String()) | ||
| } | ||
| return | ||
| } | ||
| if resp.Code != http.StatusOK { | ||
| t.Fatalf("%s %s status = %d, want %d; body=%s", request.method, request.path, resp.Code, http.StatusOK, resp.Body.String()) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Status-only subtests make this coverage too shallow.
Every case here only checks 200/204, so a handler can return the wrong payload or miss the endpoint-specific behavior and this table still passes. Please assert at least one response field or one stub side effect per route; if you keep the table, the subtest names should also follow the required Should... pattern.
As per coding guidelines, **/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases; MUST test meaningful business logic, not trivial operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/core/automation_additional_test.go` around lines 87 - 114, The
table-driven subtests (using performRequest, fixture.Engine and t.Run) only
assert HTTP status codes, which is too shallow and the t.Run names don't follow
the "Should..." pattern; update each subtest to use t.Run("Should ...") and add
at least one meaningful assertion per route: for GETs assert a specific JSON
field/value or structure in resp.Body (e.g., job id/name for
"/automation/jobs/job-1", trigger fields for "/automation/triggers/trigger-1",
run entries for runs endpoints) and for DELETEs assert side-effects such as a
subsequent GET to the same resource returns 404 or that a list endpoint no
longer contains the deleted id (performRequest + resp.Body checks); keep using
the existing table but include an expected-check function or switch on
request.path to validate payloads/side effects rather than only status.
Comment on lines
+157
to
+158
| if _, err := New(tc.opts...); err == nil || !strings.Contains(err.Error(), tc.wantErr) { | ||
| t.Fatalf("New() error = %v, want %q", err, tc.wantErr) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check if udsapi defines sentinel/typed errors for constructor validation.
rg -n --type=go -C3 'var\s+Err\w+\s*=\s*errors\.New|type\s+\w+Error\s+struct' internal/api/udsapi
# 2) Inspect New(...) validation returns to see if errors are wrapped for errors.Is().
rg -n --type=go -C3 'session manager is required|task service is required|observer is required|workspace resolver is required|fmt\.Errorf\(' internal/api/udsapi/server.go
# 3) Find current error assertion style in this test file.
rg -n --type=go -C2 'err\.Error\(\)|strings\.Contains|errors\.Is|errors\.As' internal/api/udsapi/server_test.goRepository: compozy/agh
Length of output: 37
🏁 Script executed:
# First, verify the file exists and get basic info
find . -name "server_test.go" -path "*/udsapi/*" 2>/dev/null | head -20Repository: compozy/agh
Length of output: 91
🏁 Script executed:
# If the above finds the file, examine lines around 157-158
if [ -f "internal/api/udsapi/server_test.go" ]; then
echo "=== Lines 150-170 of server_test.go ==="
sed -n '150,170p' internal/api/udsapi/server_test.go
fiRepository: compozy/agh
Length of output: 611
🏁 Script executed:
# Check the New() function implementation in server.go
if [ -f "internal/api/udsapi/server.go" ]; then
echo "=== Searching for New() constructor ==="
rg -n "^func New" internal/api/udsapi/server.go -A 20
fiRepository: compozy/agh
Length of output: 685
🏁 Script executed:
# Search for error definitions in the udsapi package
if [ -d "internal/api/udsapi" ]; then
echo "=== Looking for error definitions ==="
rg -n "var Err|type.*Error" internal/api/udsapi/ -t go
fiRepository: compozy/agh
Length of output: 92
🏁 Script executed:
# Get the full New() function
sed -n '253,310p' internal/api/udsapi/server.goRepository: compozy/agh
Length of output: 1615
🏁 Script executed:
# Get the test cases to see what wantErr values are
sed -n '100,160p' internal/api/udsapi/server_test.goRepository: compozy/agh
Length of output: 1679
🏁 Script executed:
# Check the entire udsapi directory for any error type definitions
ls -la internal/api/udsapi/Repository: compozy/agh
Length of output: 1440
🏁 Script executed:
# Search more broadly for error types in udsapi
find internal/api/udsapi -name "*.go" -type f -exec basename {} \;Repository: compozy/agh
Length of output: 375
🏁 Script executed:
# Check if errors are wrapped with fmt.Errorf anywhere in the file
rg -n "fmt\.Errorf.*session manager is required|fmt\.Errorf.*task service is required" internal/api/udsapi/server.goRepository: compozy/agh
Length of output: 37
Use sentinel errors instead of string matching for New() validation errors.
The test currently matches error text with strings.Contains(err.Error(), ...), which violates the coding guideline: "Use errors.Is() and errors.As() for error matching — never compare error strings."
The New() function in server.go returns plain errors.New() for validation failures. To fix this:
-
Define sentinel errors in server.go:
var ( ErrSessionManagerRequired = errors.New("udsapi: session manager is required") ErrTaskServiceRequired = errors.New("udsapi: task service is required") ErrObserverRequired = errors.New("udsapi: observer is required") ErrWorkspaceResolverRequired = errors.New("udsapi: workspace resolver is required") )
-
Return these sentinel errors from New() instead of creating new errors each time.
-
Update test assertions to use
errors.Is(err, ErrSessionManagerRequired)instead of string matching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/udsapi/server_test.go` around lines 157 - 158, Replace
string-based error matching with sentinel errors: define exported package-level
sentinel errors (ErrSessionManagerRequired, ErrTaskServiceRequired,
ErrObserverRequired, ErrWorkspaceResolverRequired) in server.go and have
New(...) return these sentinel errors for the respective validation failures
instead of constructing new errors each call; then update the test in
server_test.go to assert using errors.Is(err, ErrSessionManagerRequired) (and
the other sentinels) rather than strings.Contains(err.Error(), ...), locating
the logic around the New() call and the validation branches that currently
return errors.New.
Comment on lines
+16
to
+51
| tests := []struct { | ||
| name string | ||
| args []string | ||
| wantErr string | ||
| }{ | ||
| { | ||
| name: "create requires workspace for workspace scope", | ||
| args: []string{"task", "create", "--scope", "workspace", "--title", "Investigate"}, | ||
| wantErr: "--workspace is required when --scope is workspace", | ||
| }, | ||
| { | ||
| name: "create forbids workspace for global scope", | ||
| args: []string{"task", "create", "--scope", "global", "--workspace", "alpha", "--title", "Investigate"}, | ||
| wantErr: "--workspace must be empty when --scope is global", | ||
| }, | ||
| { | ||
| name: "update requires change flags", | ||
| args: []string{"task", "update", "task-1"}, | ||
| wantErr: "task update requires at least one change flag", | ||
| }, | ||
| { | ||
| name: "update rejects clear owner with owner mutation", | ||
| args: []string{"task", "update", "task-1", "--clear-owner", "--owner-kind", "pool", "--owner-ref", "triage"}, | ||
| wantErr: "--clear-owner cannot be combined with --owner-kind or --owner-ref", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, _, err := executeRootCommand(t, newTestDeps(t, stubClient{}), tt.args...) | ||
| if err == nil || !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("executeRootCommand(%v) error = %v, want %q", tt.args, err, tt.wantErr) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Rename these subtests to the required Should... form.
This table still feeds t.Run(...) with names that do not follow the enforced Go test naming convention. As per coding guidelines, **/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/task_test.go` around lines 16 - 51, The table-driven tests in
the tests variable use non-conforming names passed through t.Run(tt.name);
rename each tt.name value to the "Should ..." form (e.g. "Should require
workspace for workspace scope", "Should forbid workspace for global scope",
"Should require change flags on update", "Should reject clear owner with owner
mutation") so t.Run receives names that follow the enforced pattern; no
behavioral code changes needed beyond updating the string literals in the tests
slice.
Comment on lines
+434
to
+456
| func (h *HostAPIHandler) taskQueryFromParams(ctx context.Context, params hostAPITasksParams) (taskpkg.TaskQuery, error) { | ||
| workspaceID, err := h.resolveTaskWorkspaceID(ctx, strings.TrimSpace(params.Workspace)) | ||
| if err != nil { | ||
| return taskpkg.TaskQuery{}, err | ||
| } | ||
| if err := validateTaskChannel("task_query.network_channel", params.NetworkChannel); err != nil { | ||
| return taskpkg.TaskQuery{}, err | ||
| } | ||
|
|
||
| query := taskpkg.TaskQuery{ | ||
| Scope: params.Scope.Normalize(), | ||
| WorkspaceID: workspaceID, | ||
| Status: params.Status.Normalize(), | ||
| OwnerKind: params.OwnerKind.Normalize(), | ||
| OwnerRef: strings.TrimSpace(params.OwnerRef), | ||
| ParentTaskID: strings.TrimSpace(params.ParentTaskID), | ||
| NetworkChannel: strings.TrimSpace(params.NetworkChannel), | ||
| Limit: params.Limit, | ||
| } | ||
| if err := query.Validate("task_query"); err != nil { | ||
| return taskpkg.TaskQuery{}, invalidParamsRPCError(err) | ||
| } | ||
| return query, nil |
There was a problem hiding this comment.
Validate scope binding before resolving workspace.
Both helpers resolve the workspace reference first, so scope=global or an empty/invalid scope can turn an invalid-params request into workspace not found, and that makes the response depend on whether the workspace exists.
Also applies to: 471-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/extension/host_api_tasks.go` around lines 434 - 456,
taskQueryFromParams currently resolves the workspace (resolveTaskWorkspaceID)
before validating the requested scope, which can convert
invalid-scope/empty-scope errors into "workspace not found"; instead, normalize
and validate the scope binding from params.Scope (call Scope.Normalize() and any
scope-binding validation you have) first and only call resolveTaskWorkspaceID
when the scope indicates a workspace-bound query. Update taskQueryFromParams to:
normalize/validate params.Scope, then conditionally call resolveTaskWorkspaceID
and set WorkspaceID; keep the existing validateTaskChannel and query.Validate
flow. Apply the same change to the other function in this file that also calls
resolveTaskWorkspaceID before scope validation (search for other usages of
resolveTaskWorkspaceID and fix them similarly).
Comment on lines
+2288
to
+2398
| t.Run("MissingManager", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| checker := &CapabilityChecker{} | ||
| checker.Register("ext-tasks", SourceUser, &Manifest{ | ||
| Actions: ActionsConfig{Requires: []string{"tasks", "tasks/get", "tasks/runs"}}, | ||
| Security: SecurityConfig{ | ||
| Capabilities: []string{"task.read"}, | ||
| }, | ||
| }) | ||
|
|
||
| handler := NewHostAPIHandler( | ||
| nil, | ||
| nil, | ||
| nil, | ||
| nil, | ||
| WithHostAPICapabilityChecker(checker), | ||
| WithHostAPIRateLimit(1000, 1000), | ||
| ) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| method string | ||
| params map[string]any | ||
| }{ | ||
| {name: "ShouldRejectList", method: "tasks", params: map[string]any{}}, | ||
| {name: "ShouldRejectGet", method: "tasks/get", params: map[string]any{"id": "task-1"}}, | ||
| {name: "ShouldRejectRuns", method: "tasks/runs", params: map[string]any{"id": "task-1"}}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| params, err := marshalParams(tt.params) | ||
| if err != nil { | ||
| t.Fatalf("marshalParams() error = %v", err) | ||
| } | ||
|
|
||
| _, err = handler.Handle(testutil.Context(t), "ext-tasks", tt.method, params) | ||
| assertErrorContains(t, err, "task manager is not configured") | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("InvalidInputs", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| env := newHostAPITestEnv(t) | ||
| env.grant( | ||
| "ext-tasks", | ||
| []string{"tasks", "tasks/create", "tasks/update", "tasks/runs/attach_session"}, | ||
| []string{"task.read", "task.write"}, | ||
| ) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| method string | ||
| params map[string]any | ||
| wantCode int | ||
| wantText string | ||
| }{ | ||
| { | ||
| name: "ShouldRejectUnknownWorkspace", | ||
| method: "tasks/create", | ||
| params: map[string]any{ | ||
| "scope": taskpkg.ScopeWorkspace, | ||
| "workspace": "ws-missing", | ||
| "title": "Missing workspace task", | ||
| }, | ||
| wantCode: HostAPINotFoundCode, | ||
| wantText: "workspace", | ||
| }, | ||
| { | ||
| name: "ShouldRejectInvalidListChannel", | ||
| method: "tasks", | ||
| params: map[string]any{ | ||
| "network_channel": "not valid", | ||
| }, | ||
| wantCode: HostAPIInvalidParamsCode, | ||
| wantText: "task_query.network_channel", | ||
| }, | ||
| { | ||
| name: "ShouldRequireUpdateChanges", | ||
| method: "tasks/update", | ||
| params: map[string]any{"id": "task-1"}, | ||
| wantCode: HostAPIInvalidParamsCode, | ||
| wantText: "at least one mutable field", | ||
| }, | ||
| { | ||
| name: "ShouldRequireAttachSessionID", | ||
| method: "tasks/runs/attach_session", | ||
| params: map[string]any{"id": "run-1"}, | ||
| wantCode: HostAPIInvalidParamsCode, | ||
| wantText: "session_id is required", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| _, err := env.call(t, "ext-tasks", tt.method, tt.params) | ||
| assertRPCErrorCode(t, err, tt.wantCode) | ||
| assertErrorContains(t, err, tt.wantText) | ||
| }) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Rename the nested suite labels to Should....
MissingManager and InvalidInputs still break the required t.Run("Should...") convention for Go tests. As per coding guidelines, **/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/extension/host_api_test.go` around lines 2288 - 2398, Rename the two
outer test suite labels that violate the t.Run("Should...") convention: change
the t.Run call using the literal "MissingManager" to a Should-prefixed label
(e.g., "ShouldRejectWhenManagerMissing") and change the t.Run call using
"InvalidInputs" to a Should-prefixed label (e.g., "ShouldRejectInvalidInputs");
locate the offending calls by finding the t.Run invocations that wrap the
MissingManager and InvalidInputs blocks in internal/extension/host_api_test.go
and update only the string names to follow the "Should..." pattern.
Comment on lines
+128
to
+149
| func (w *FileAuditWriter) RecordTaskIngress(ctx context.Context, audit TaskIngressAudit) error { | ||
| if ctx == nil { | ||
| return errors.New("network: audit context is required") | ||
| } | ||
| if w == nil { | ||
| return errors.New("network: audit writer is required") | ||
| } | ||
|
|
||
| entry, err := normalizeTaskIngressAuditEntry(audit, w.now()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var recordErr error | ||
| if w.path != "" { | ||
| recordErr = errors.Join(recordErr, w.appendFile(entry)) | ||
| } | ||
| if w.store != nil { | ||
| recordErr = errors.Join(recordErr, w.store.WriteNetworkAudit(ctx, entry)) | ||
| } | ||
|
|
||
| return recordErr |
There was a problem hiding this comment.
Guard zero-value writer paths to avoid panic and silent audit drops.
At Line 136, w.now() can panic if FileAuditWriter is zero-initialized, and when both sinks are unset (w.path == "" && w.store == nil) this method currently returns nil without recording anything.
🔧 Proposed hardening
func (w *FileAuditWriter) RecordTaskIngress(ctx context.Context, audit TaskIngressAudit) error {
- if ctx == nil {
- return errors.New("network: audit context is required")
- }
if w == nil {
return errors.New("network: audit writer is required")
}
+ if ctx == nil {
+ return errors.New("network: audit context is required")
+ }
+ if w.path == "" && w.store == nil {
+ return errors.New("network: audit sink is required")
+ }
+
+ now := w.now
+ if now == nil {
+ now = func() time.Time { return time.Now().UTC() }
+ }
- entry, err := normalizeTaskIngressAuditEntry(audit, w.now())
+ entry, err := normalizeTaskIngressAuditEntry(audit, now())
if err != nil {
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (w *FileAuditWriter) RecordTaskIngress(ctx context.Context, audit TaskIngressAudit) error { | |
| if ctx == nil { | |
| return errors.New("network: audit context is required") | |
| } | |
| if w == nil { | |
| return errors.New("network: audit writer is required") | |
| } | |
| entry, err := normalizeTaskIngressAuditEntry(audit, w.now()) | |
| if err != nil { | |
| return err | |
| } | |
| var recordErr error | |
| if w.path != "" { | |
| recordErr = errors.Join(recordErr, w.appendFile(entry)) | |
| } | |
| if w.store != nil { | |
| recordErr = errors.Join(recordErr, w.store.WriteNetworkAudit(ctx, entry)) | |
| } | |
| return recordErr | |
| func (w *FileAuditWriter) RecordTaskIngress(ctx context.Context, audit TaskIngressAudit) error { | |
| if w == nil { | |
| return errors.New("network: audit writer is required") | |
| } | |
| if ctx == nil { | |
| return errors.New("network: audit context is required") | |
| } | |
| if w.path == "" && w.store == nil { | |
| return errors.New("network: audit sink is required") | |
| } | |
| now := w.now | |
| if now == nil { | |
| now = func() time.Time { return time.Now().UTC() } | |
| } | |
| entry, err := normalizeTaskIngressAuditEntry(audit, now()) | |
| if err != nil { | |
| return err | |
| } | |
| var recordErr error | |
| if w.path != "" { | |
| recordErr = errors.Join(recordErr, w.appendFile(entry)) | |
| } | |
| if w.store != nil { | |
| recordErr = errors.Join(recordErr, w.store.WriteNetworkAudit(ctx, entry)) | |
| } | |
| return recordErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/network/audit.go` around lines 128 - 149, The RecordTaskIngress
method calls w.now() before verifying the writer's sinks and before guarding
against a nil now func, which can panic on a zero-value FileAuditWriter and also
silently return when no sinks are configured; update RecordTaskIngress (in
FileAuditWriter) to first ensure at least one sink is set (w.path != "" ||
w.store != nil) and return a descriptive error if none are configured, then
compute the timestamp using a safe now: use w.now() only if w.now != nil
otherwise fall back to time.Now(), then pass that timestamp to
normalizeTaskIngressAuditEntry; keep conditional calls to appendFile and
store.WriteNetworkAudit unchanged but ensure they run after these guards to
avoid panics and silent drops.
Comment on lines
+136
to
+147
| entry, err := normalizeTaskIngressAuditEntry(audit, w.now()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var recordErr error | ||
| if w.path != "" { | ||
| recordErr = errors.Join(recordErr, w.appendFile(entry)) | ||
| } | ||
| if w.store != nil { | ||
| recordErr = errors.Join(recordErr, w.store.WriteNetworkAudit(ctx, entry)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the audit.go file
find . -name "audit.go" -type f | head -20Repository: compozy/agh
Length of output: 82
🏁 Script executed:
# Read the relevant lines from internal/network/audit.go
if [ -f "internal/network/audit.go" ]; then
echo "=== Lines 130-155 ==="
sed -n '130,155p' internal/network/audit.go | cat -n
echo ""
echo "=== Lines 280-295 ==="
sed -n '280,295p' internal/network/audit.go | cat -n
else
echo "File not found"
fiRepository: compozy/agh
Length of output: 1530
🏁 Script executed:
# Get the full file size to understand context better
wc -l internal/network/audit.goRepository: compozy/agh
Length of output: 84
🏁 Script executed:
# Let me get more context around these lines to understand the function names
sed -n '120,160p' internal/network/audit.go | cat -nRepository: compozy/agh
Length of output: 1663
🏁 Script executed:
# Also check around lines 287-289 for more context
sed -n '275,300p' internal/network/audit.go | cat -nRepository: compozy/agh
Length of output: 1132
Wrap propagated errors with operation context.
Two locations in this function return or join raw errors without local operation context:
- Line 139 (
return err): The error fromnormalizeTaskIngressAuditEntry()should be wrapped with operation context before returning. - Lines 144 and 147: Errors from
w.appendFile(entry)andw.store.WriteNetworkAudit(ctx, entry)should be wrapped before joining. - Line 289: The error from
entry.Validate()should be wrapped before returning.
Wrap each error with fmt.Errorf() to add operation-specific context (e.g., fmt.Errorf("network: normalize task ingress audit entry: %w", err)), making root-cause tracing easier in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/network/audit.go` around lines 136 - 147, The errors returned from
normalizeTaskIngressAuditEntry, w.appendFile, w.store.WriteNetworkAudit, and
entry.Validate need to be wrapped with operation context using fmt.Errorf and %w
before being returned or joined; specifically, replace the raw return of err
from normalizeTaskIngressAuditEntry with a wrapped error like
fmt.Errorf("network: normalize task ingress audit entry: %w", err), wrap the
results of w.appendFile(entry) and w.store.WriteNetworkAudit(ctx, entry)
similarly before they are combined into recordErr (so recordErr =
errors.Join(recordErr, fmt.Errorf("network: append file audit entry: %w", err)))
and wrap the error returned from entry.Validate() with context (e.g.,
fmt.Errorf("network: validate audit entry: %w", err)) so all propagated errors
include clear operation-specific context.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/core/automation_test.go (2)
625-682: Split the new helper assertions intot.Run("Should ...")subtests.These added task-related checks are currently embedded in one large test, which reduces failure isolation and diverges from the repo’s test-case structure policy.
As per coding guidelines,
**/*_test.go:MUST use t.Run("Should...") pattern for ALL test cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/automation_test.go` around lines 625 - 682, The new assertions around jobFromCreateRequest and applyJobPatch are lumped into one big test; split them into focused t.Run subtests to follow the repository policy. Create separate t.Run blocks that each assert a single behavior or small related group: e.g., t.Run("Should normalize create request fields") to validate jobFromCreateRequest (Scope, Name trimming, AgentName, WorkspaceID, Prompt, Schedule.Interval, Task fields including Title, NetworkChannel, Owner.Ref), and t.Run("Should apply update patch correctly") to validate applyJobPatch (Name/AgentName/WorkspaceID/Prompt trimming, Enabled flag, Schedule.Expr, Task.Title/NetworkChannel, Retry.MaxRetries, FireLimit.Max). Keep existing setup values (jobName, jobAgent, etc.), call the same helpers (jobFromCreateRequest, applyJobPatch) inside each t.Run, and replace the single large t.Fatalf check with focused t.Fatalf/assertions per subtest for clearer failure isolation.
654-680: Add an anti-aliasing assertion for patchedTask.The new patch assertions verify values, but not clone semantics. A direct pointer assignment regression would still pass.
Proposed anti-aliasing assertion
if updatedJob.Name != "renamed" || updatedJob.AgentName != "reviewer" || updatedJob.WorkspaceID != "ws-beta" || updatedJob.Prompt != "next prompt" || updatedJob.Enabled || updatedJob.Schedule == nil || updatedJob.Schedule.Expr != "0 * * * *" || updatedJob.Task == nil || updatedJob.Task.Title != "Delegate review" || updatedJob.Task.NetworkChannel != "ops-queue" || updatedJob.Retry.MaxRetries != 3 || updatedJob.FireLimit.Max != 4 { t.Fatalf("applyJobPatch() = %#v", updatedJob) } + jobTask.Title = "mutated" + if updatedJob.Task.Title != "Delegate review" { + t.Fatalf("applyJobPatch() task clone = %#v", updatedJob.Task) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/core/automation_test.go` around lines 654 - 680, The patched Job's Task must be a cloned object, not a direct pointer alias; update the test around applyJobPatch so after creating updatedJob you assert updatedJob.Task != original.Job.Task (the Job passed into applyJobPatch) and also verify deep-copy semantics (e.g., change updatedJob.Task.Title and assert the original Job.Task.Title and the input patch jobTask remain unchanged) to catch pointer-assignment regressions for automationpkg.JobTaskConfig and the Task field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/core/automation_test.go`:
- Around line 635-645: The test assertion after creating the job (checking
createdJob from jobFromCreateRequest) currently verifies Task.Owner.Ref but
omits verifying Task.Owner.Kind; update the assertion to also check
createdJob.Task.Owner.Kind == taskpkg.OwnerKindAutomation so ownership-kind
mapping is validated alongside Owner.Ref and other fields (adjust the combined
conditional that compares createdJob.Scope, Name, AgentName, WorkspaceID,
Prompt, Schedule, Task.Title, Task.NetworkChannel, Task.Owner.Ref to include
Task.Owner.Kind).
---
Nitpick comments:
In `@internal/api/core/automation_test.go`:
- Around line 625-682: The new assertions around jobFromCreateRequest and
applyJobPatch are lumped into one big test; split them into focused t.Run
subtests to follow the repository policy. Create separate t.Run blocks that each
assert a single behavior or small related group: e.g., t.Run("Should normalize
create request fields") to validate jobFromCreateRequest (Scope, Name trimming,
AgentName, WorkspaceID, Prompt, Schedule.Interval, Task fields including Title,
NetworkChannel, Owner.Ref), and t.Run("Should apply update patch correctly") to
validate applyJobPatch (Name/AgentName/WorkspaceID/Prompt trimming, Enabled
flag, Schedule.Expr, Task.Title/NetworkChannel, Retry.MaxRetries,
FireLimit.Max). Keep existing setup values (jobName, jobAgent, etc.), call the
same helpers (jobFromCreateRequest, applyJobPatch) inside each t.Run, and
replace the single large t.Fatalf check with focused t.Fatalf/assertions per
subtest for clearer failure isolation.
- Around line 654-680: The patched Job's Task must be a cloned object, not a
direct pointer alias; update the test around applyJobPatch so after creating
updatedJob you assert updatedJob.Task != original.Job.Task (the Job passed into
applyJobPatch) and also verify deep-copy semantics (e.g., change
updatedJob.Task.Title and assert the original Job.Task.Title and the input patch
jobTask remain unchanged) to catch pointer-assignment regressions for
automationpkg.JobTaskConfig and the Task field.
🪄 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: 5fa13c4e-d6df-45f2-863b-02d3f34686b9
⛔ Files ignored due to path filters (102)
.agents/skills/qa-execution/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/assets/verification-report-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/checklist.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/project-signals.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/references/web-ui-qa.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-execution/scripts/discover-project-contract.pyis excluded by!.agents/**.agents/skills/qa-report/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/bug_report_templates.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/figma_validation.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/regression_testing.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/references/test_case_templates.mdis excluded by!**/*.md,!.agents/**.agents/skills/qa-report/scripts/create_bug_report.shis excluded by!.agents/**.agents/skills/qa-report/scripts/generate_test_cases.shis excluded by!.agents/**.agents/skills/systematic-qa/SKILL.mdis excluded by!**/*.md,!.agents/**.agents/skills/systematic-qa/assets/issue-template.mdis excluded by!**/*.md,!.agents/**.agents/skills/systematic-qa/assets/verification-report-template.mdis excluded by!**/*.md,!.agents/**.compozy/tasks/core-tasks/qa/case-execution-matrix.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/issues/BUG-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/review-round-1.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/screenshots/ui-404.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-automation-job-created.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-automation-post-verify-final.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-automation-post-verify.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-network-disabled.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-network-post-verify-final.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-network-post-verify.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/screenshots/ui-root.pngis excluded by!**/*.png.compozy/tasks/core-tasks/qa/test-cases/SMOKE-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/SMOKE-010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-016.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-017.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-018.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-019.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-020.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-021.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-022.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-023.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-024.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-025.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-026.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-027.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-028.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-029.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-FUNC-030.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-009.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-010.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-011.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-012.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-013.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-014.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-INT-015.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-PERF-006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-001.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-002.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-003.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-004.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-005.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-006.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-007.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-cases/TC-SEC-008.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-plans/core-tasks-regression.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/test-plans/core-tasks-test-plan.mdis excluded by!**/*.md.compozy/tasks/core-tasks/qa/verification-report.mdis excluded by!**/*.mdskills-lock.jsonis excluded by!**/*.json
📒 Files selected for processing (12)
internal/api/contract/contract_test.gointernal/api/core/automation_test.gointernal/automation/manager_integration_test.gointernal/daemon/daemon.gointernal/daemon/daemon_test.gointernal/observe/tasks.gointernal/observe/tasks_health_optimization_test.gointernal/session/manager.gointernal/session/manager_hooks_test.gointernal/session/manager_lifecycle.gointernal/store/globaldb/global_db_automation_test.gointernal/task/manager_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/api/contract/contract_test.go
Comment on lines
635
to
645
| Task: &automationpkg.JobTaskConfig{ | ||
| Title: " Review repo ", | ||
| NetworkChannel: " ops-automation ", | ||
| Owner: &taskpkg.Ownership{ | ||
| Kind: taskpkg.OwnerKindAutomation, | ||
| Ref: " rule:build-review ", | ||
| }, | ||
| }, | ||
| }) | ||
| if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" { | ||
| if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" || createdJob.Task == nil || createdJob.Task.Title != "Review repo" || createdJob.Task.NetworkChannel != "ops-automation" || createdJob.Task.Owner == nil || createdJob.Task.Owner.Ref != "rule:build-review" { | ||
| t.Fatalf("jobFromCreateRequest() = %#v", createdJob) |
There was a problem hiding this comment.
Assert Owner.Kind in the new task mapping check.
Line 644 validates Owner.Ref but skips Owner.Kind, so a regression in ownership-kind mapping could pass unnoticed.
Proposed test assertion update
- if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" || createdJob.Task == nil || createdJob.Task.Title != "Review repo" || createdJob.Task.NetworkChannel != "ops-automation" || createdJob.Task.Owner == nil || createdJob.Task.Owner.Ref != "rule:build-review" {
+ if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" || createdJob.Task == nil || createdJob.Task.Title != "Review repo" || createdJob.Task.NetworkChannel != "ops-automation" || createdJob.Task.Owner == nil || createdJob.Task.Owner.Kind != taskpkg.OwnerKindAutomation || createdJob.Task.Owner.Ref != "rule:build-review" {
t.Fatalf("jobFromCreateRequest() = %#v", createdJob)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Task: &automationpkg.JobTaskConfig{ | |
| Title: " Review repo ", | |
| NetworkChannel: " ops-automation ", | |
| Owner: &taskpkg.Ownership{ | |
| Kind: taskpkg.OwnerKindAutomation, | |
| Ref: " rule:build-review ", | |
| }, | |
| }, | |
| }) | |
| if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" { | |
| if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" || createdJob.Task == nil || createdJob.Task.Title != "Review repo" || createdJob.Task.NetworkChannel != "ops-automation" || createdJob.Task.Owner == nil || createdJob.Task.Owner.Ref != "rule:build-review" { | |
| t.Fatalf("jobFromCreateRequest() = %#v", createdJob) | |
| Task: &automationpkg.JobTaskConfig{ | |
| Title: " Review repo ", | |
| NetworkChannel: " ops-automation ", | |
| Owner: &taskpkg.Ownership{ | |
| Kind: taskpkg.OwnerKindAutomation, | |
| Ref: " rule:build-review ", | |
| }, | |
| }, | |
| }) | |
| if createdJob.Scope != automationpkg.AutomationScopeWorkspace || createdJob.Name != "build review" || createdJob.AgentName != "coder" || createdJob.WorkspaceID != "ws-alpha" || createdJob.Prompt != "inspect repo" || createdJob.Schedule == nil || createdJob.Schedule.Interval != "2h" || createdJob.Task == nil || createdJob.Task.Title != "Review repo" || createdJob.Task.NetworkChannel != "ops-automation" || createdJob.Task.Owner == nil || createdJob.Task.Owner.Kind != taskpkg.OwnerKindAutomation || createdJob.Task.Owner.Ref != "rule:build-review" { | |
| t.Fatalf("jobFromCreateRequest() = %#v", createdJob) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/core/automation_test.go` around lines 635 - 645, The test
assertion after creating the job (checking createdJob from jobFromCreateRequest)
currently verifies Task.Owner.Ref but omits verifying Task.Owner.Kind; update
the assertion to also check createdJob.Task.Owner.Kind ==
taskpkg.OwnerKindAutomation so ownership-kind mapping is validated alongside
Owner.Ref and other fields (adjust the combined conditional that compares
createdJob.Scope, Name, AgentName, WorkspaceID, Prompt, Schedule, Task.Title,
Task.NetworkChannel, Task.Owner.Ref to include Task.Owner.Kind).
This was referenced May 24, 2026
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 was referenced May 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests