Repository navigation
feat: add Loop lifecycle controls and surfaces (stack 2/3) - #301
Closed
pedronauck wants to merge 5 commits into
Closed
pedronauck wants to merge 5 commits into
pedronauck wants to merge 5 commits into
Conversation
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughChangesLoop lifecycle platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant WebOrCLI
participant LoopAPI
participant LoopService
participant GlobalDB
participant Coordinator
Operator->>WebOrCLI: Select cancel, kill, pause, resume, or node action
WebOrCLI->>LoopAPI: Send lifecycle request
LoopAPI->>LoopService: Pass validated identity and actor context
LoopService->>GlobalDB: Persist mutation and update lifecycle state
GlobalDB->>Coordinator: Reserve or wake coordinator work
Coordinator-->>LoopService: Return committed lifecycle state
LoopService-->>LoopAPI: Return structured mutation response
LoopAPI-->>WebOrCLI: Return response
WebOrCLI-->>Operator: Display lifecycle result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 3, 2026
Contributor
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/api/core/loops_test.go (1)
1996-2007: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen
assertLoopOKMutationto verify the echoedrun_id/node_id.
assertLoopOKMutationonly decodes and checks theokfield. The stub handlers for cancel/kill/pause/resume/requeue deliberately echoRunIDandNodeIDback inLoopMutationResponse(seehappyLoopServiceat Lines 1569-1595), but no call site (Lines 255-281) checks these values. A routing bug that swaps or dropsrun_id/node_idon the way to the service would still reportok: trueand pass this test.Decode and assert the expected
run_id(andnode_id, when applicable) inassertLoopOKMutation, or add a variant that accepts expected IDs.As per path instructions, "Ensure tests verify behavior outcomes, not just function calls."
🧪 Proposed strengthening
-func assertLoopOKMutation(t *testing.T, engine http.Handler, method string, path string, body []byte) { +func assertLoopOKMutation(t *testing.T, engine http.Handler, method string, path string, body []byte, wantRunID string, wantNodeID string) { t.Helper() resp := performRequest(t, engine, method, path, body) assertLoopStatus(t, resp.Code, http.StatusOK, resp.Body.String()) var payload struct { - OK bool `json:"ok"` + OK bool `json:"ok"` + RunID string `json:"run_id"` + NodeID string `json:"node_id,omitempty"` } testutil.DecodeJSONResponse(t, resp, &payload) - if !payload.OK { + if !payload.OK || payload.RunID != wantRunID || (wantNodeID != "" && payload.NodeID != wantNodeID) { t.Fatalf("%s %s payload = %#v, want ok=true", method, path, payload) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/core/loops_test.go` around lines 1996 - 2007, Strengthen assertLoopOKMutation to decode the echoed run_id and node_id from LoopMutationResponse and compare them with the IDs expected for each mutation request. Update its call sites for cancel, kill, pause, resume, and requeue to supply the expected identifiers, allowing node_id to be omitted only where the operation does not include one, while retaining the existing ok=true assertion.Source: Path instructions
internal/loop/compiler.go (1)
337-369: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd test coverage for wait-control event filter compilation.
This new branch is not covered by
internal/looptests. Add tests forcompileWatchEventsFiltersin the wait-control path, including whenparams.Eventisniland whennode.Params.Decode(¶ms)returns a decode error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/compiler.go` around lines 337 - 369, Add internal/loop tests covering compileWatchEventsFilters for control-wait nodes: verify a nil params.Event returns successfully without compiling a filter, and verify a node.Params.Decode failure returns the expected wrapped error. Reuse the existing test helpers and fixtures for node parameters, lintContext, and ResolvedDefinition.internal/automation/dispatch_loop.go (1)
110-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExpose loop-run suppression in automation dispatch results.
LoopStartResult.SuppressedandSuppressedCountare available from the loop starter, butdispatchLoopBackedAttemptonly propagatesRunID. A suppressed duplicate loop fire should be recorded inscheduledRun.Metadataor emitted through the post-fire hook, so operators can distinguish it from a new loop fire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/automation/dispatch_loop.go` around lines 110 - 134, The dispatch path in dispatchLoopBackedAttempt currently ignores LoopStartResult.Suppressed and SuppressedCount after StartLoop succeeds. Propagate these fields into scheduledRun.Metadata or the existing post-fire hook, preserving RunID handling, so suppressed duplicate fires and their counts are distinguishable from newly started loops.internal/daemon/loop_action_liveness.go (1)
230-257: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not treat a stalled session as fresh liveness evidence.
Status()preserves stalled liveness metadata withStallState: "stalled"even whenLiveness.Activity == nil, so a stopped-but-still-registered session still returnsevidence = trueinrefreshActionProgress. This resets the silence timer and can preventSilenceAfter/death-streak detection from firing. Treat present-but-inactive stall metadata asevidence = false, or only updateprogressAtfrom realLiveness.Activity.LastActivityAtmovement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/loop_action_liveness.go` around lines 230 - 257, Update refreshActionProgress to return evidence=false when session liveness metadata indicates a stalled state without active Liveness.Activity. Ensure only real Activity.LastActivityAt movement updates usage.progressAt and counts as fresh evidence, preserving the existing behavior for active activity and missing session data.internal/daemon/native_loop_tools_support.go (1)
230-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the duplicate TerminalRun mapping.
ReasonCodeTerminalRunandReasonCodeRunTerminalboth use theterminal_loop_runreason code, and the existing terminal-kill case already mapsReasonCodeTerminalRuntotoolspkg.ReasonLoopTerminalRun. MapReasonCodeRunTerminalto the same tool reason as terminal run rejections rather than adding a separateReasonLoopRunTerminalentry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/native_loop_tools_support.go` around lines 230 - 245, Update the reason mapping switch to map ReasonCodeRunTerminal to toolspkg.ReasonLoopTerminalRun, matching the existing ReasonCodeTerminalRun mapping; remove the separate ReasonLoopRunTerminal mapping while preserving all other cases.
🟡 Minor comments (16)
internal/loop/coordinator_action_metadata.go-68-71 (1)
68-71: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeparate the unknown-kind error from the incomplete-provenance error.
The condition mixes two distinct failures. An unrecognized
continuation_kindproduces the message "death-resume continuation provenance is incomplete", which points the operator at missing fields instead of the invalid kind.🐛 Proposed fix
- if meta.ContinuationKind != deathResumeContinuationKind || meta.ResumeFromTaskRunID == "" || - meta.ResumeFromSessionID == "" { + if meta.ContinuationKind != deathResumeContinuationKind { + return fmt.Errorf( + "%w: action continuation kind is invalid: %q", + ErrValidation, + meta.ContinuationKind, + ) + } + if meta.ResumeFromTaskRunID == "" || meta.ResumeFromSessionID == "" { return fmt.Errorf("%w: death-resume continuation provenance is incomplete", ErrValidation) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/coordinator_action_metadata.go` around lines 68 - 71, Update the validation around meta.ContinuationKind to report an unknown continuation kind separately from incomplete death-resume provenance. First reject unrecognized kinds with an error identifying the invalid kind, then validate ResumeFromTaskRunID and ResumeFromSessionID only for death-resume continuations while preserving the existing ErrValidation wrapping.internal/api/contract/loop_runs.go-86-91 (1)
86-91: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize
node_controlsandwaitsbefore buildingLoopRunResponse.
LoopRunResponsesends both fields on every response, but they should use empty arrays instead ofnull. This affects API consumers when the store has no controls or waits for a run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/contract/loop_runs.go` around lines 86 - 91, Normalize the node controls and waits collections to non-nil empty slices before constructing LoopRunResponse, so its node_controls and waits JSON fields serialize as [] rather than null when the store has no entries. Preserve existing populated values and apply this in the response-building flow.internal/store/globaldb/global_db_loop_wait_escalation_apply.go-214-224 (1)
214-224: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the single rendered intent before indexing.
RenderEffectIntensrenders each spec into anintent; an invalid node-scoped identity or empty JSON entry returns an error, but a malformed rendered entry can still fail validation silently if the contract changes. Use a length check beforeintents[0].EntryIndex = cursorto avoid a panic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_wait_escalation_apply.go` around lines 214 - 224, Before indexing the result from RenderEffectIntents in the wait-escalation flow, validate that intents contains at least one rendered intent and return an appropriate error when it is empty. Preserve the existing error return from RenderEffectIntents and only assign EntryIndex after the length check.internal/daemon/loop_hook_observer.go-335-352 (1)
335-352: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap failed terminal runs to an explicit disposition.
TaskRunStatusFailedis a valid task status. When a failed terminal run reachesOnTaskRunTerminal,FailureClasscan be populated, butloopNodeHookDispositionreturns""because only completed and canceled statuses are mapped. Map failed terminal status to an explicit disposition, or avoid settingFailureClassunlessDispositionis also populated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/loop_hook_observer.go` around lines 335 - 352, Update loopNodeHookDisposition to map TaskRunStatusFailed to an explicit failure disposition consistent with the existing looppkg attempt constants. Ensure failed terminal runs reaching OnTaskRunTerminal do not produce a populated FailureClass alongside an empty Disposition, while preserving the completed and canceled mappings.internal/store/globaldb/global_db_loop_death_resume.go-60-62 (1)
60-62: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe
AttentionRequiredsignal has no end-to-end path. The store never setsAttentionRequiredon a committed continuation, and the daemon never reads the field, so a node that reaches the resume limit produces no operator-visible signal at the moment it happens.
internal/store/globaldb/global_db_loop_death_resume.go#L60-L62: setAttentionRequired: prepared.streak >= request.DeathStreakLimiton the successful continuation result, so the flag matches theflagResumeExhaustedcall at lines 274-277.internal/daemon/loop_action_death_resume.go#L50-L64: readresult.AttentionRequiredand record it in the existing log entry, so the exhausted resume streak is observable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_death_resume.go` around lines 60 - 62, The committed continuation result in internal/store/globaldb/global_db_loop_death_resume.go lines 60-62 must set AttentionRequired when prepared.streak reaches request.DeathStreakLimit, matching flagResumeExhausted. In internal/daemon/loop_action_death_resume.go lines 50-64, read result.AttentionRequired and include it in the existing log entry so exhausted resume streaks are operator-visible.internal/cli/loop.go-64-69 (1)
64-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDescribe the
compozy loop stopreplacement in release notes.The CLI and web run page already expose
cancel/killand no longer renderstop, but the release notes do not describe the removedcompozy loop stopaction. Existing callers may otherwise break without notice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/loop.go` around lines 64 - 69, Update the release notes to document that the deprecated compozy loop stop action was removed and replaced by the existing cancel/kill actions exposed by the CLI and web run page. Reference the loop command behavior clearly so existing callers understand the migration.internal/store/globaldb/global_db_loop_wait_resume.go-203-216 (1)
203-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify the missing wait cell as a conflict.
If the generation output row is absent,
Scanreturnssql.ErrNoRowsand this code wraps it as a generic store error. Callers cannot distinguish it from an infrastructure failure, so the API maps it to a server error instead of a conflict. Mapsql.ErrNoRowstolooppkg.ErrTransitionConflict, asloadWaitForResumealready does for a missing wait.🐛 Proposed fix
mutation.ItemIndex).Scan(&status, &epoch, &paused, &quarantined, &cancelState) + if errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("%w: wait cell is missing", looppkg.ErrTransitionConflict) + } if err != nil { return fmt.Errorf("store: load Loop wait cell: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_wait_resume.go` around lines 203 - 216, Update the query error handling in the wait-cell loading flow to map sql.ErrNoRows to looppkg.ErrTransitionConflict, matching the existing behavior in loadWaitForResume. Preserve the current wrapped store error for all other query failures and keep the subsequent status validation unchanged.internal/store/globaldb/global_db_loop_cancel_wake.go-40-52 (1)
40-52: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the scope explicit in the idempotency key.
cancellationWakeIdempotencyKeywrites the literal"run"for run scope and the raw node ID for node scope into the same key segment. A node whose ID isrunthen produces the same key as run-scope cancellation for the same run, kind, and timestamp. Prefix the segment with its scope so run keys and node keys can never collide.🐛 Proposed fix
func cancellationWakeIdempotencyKey(mutation looppkg.CancellationMutation) string { - target := cancellationTargetRun + target := "run:" + cancellationTargetRun if mutation.NodeID != "" { - target = string(mutation.NodeID) + target = "node:" + string(mutation.NodeID) } return fmt.Sprintf( "loop.cancel.%s.%s.%s.%d",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_cancel_wake.go` around lines 40 - 52, Update cancellationWakeIdempotencyKey so the target segment encodes its scope explicitly: use a run-scoped prefix for cancellationTargetRun and a distinct node-scoped prefix before the node ID. Preserve the existing run ID, mutation kind, and timestamp components while ensuring a node ID of "run" cannot collide with a run-scope key.internal/store/globaldb/global_db_loop_wait_expiry_route.go-66-84 (1)
66-84: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEmit the same event payload shape for timer and approval expiry.
The approval path adds
eventSummaryContentPayloadKey: json.RawMessage(payload)(line 125). This timer path omits it, althoughpayloadholds the sameexpiredandroutecontent and is already persisted toahead_payload_json. Consumers ofloopRunEventNodeWaitResumedthen see two different payload shapes for one event kind. Add the summary content key here, or drop it from both paths.The two blocks also repeat the identical payload map and the identical coordinator idempotency key format. Extract one helper for the event payload and one for the reservation key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_wait_expiry_route.go` around lines 66 - 84, Unify timer and approval expiry handling so loopRunEventNodeWaitResumed emits the same payload shape, including eventSummaryContentPayloadKey with the expired/route payload, in both paths. Extract shared helpers for constructing the event payload and coordinator reservation idempotency key, then use them from this expiry flow and the approval flow while preserving existing values and behavior.sdk/go/watch_source.go-152-171 (1)
152-171: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the new
event_keycontract explicit in docs.The Go and TypeScript SDKs enforce
event_keyas valid UTF-8 NFC, trimmed, non-empty, and ≤256 bytes, but extension documentation or a migration note does not communicate this new required field. Add SDK/user-facing documentation for this breakingWatchPollResponsechange.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/go/watch_source.go` around lines 152 - 171, The WatchPollResponse contract needs user-facing documentation for the required event_key field. Update the Go and TypeScript SDK or extension documentation, and add a migration note if that is the established location, describing that event_key must be valid UTF-8 NFC, trimmed, non-empty, and no longer than 256 bytes.internal/daemon/native_loop_tools_support.go-241-242 (1)
241-242: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMap
ReasonCodeAlreadyDecidedtoErrorCodeConflict.
ReasonCodeAlreadyDecidedwrapsErrTransitionConflict, andStatusForLoopErrorclassifiesErrTransitionConflictas409 Conflict. ThisReasonErrorpath returns before the HTTP status switch, so it still emitsErrorCodeInvalidInput; usetoolspkg.ErrorCodeConflictfor already-decided lifecycle compare-and-swap rejections to keep tool errors consistent with state-conflict rejections.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/native_loop_tools_support.go` around lines 241 - 242, Update the ReasonCodeAlreadyDecided branch in the reason mapping to assign toolspkg.ErrorCodeConflict instead of toolspkg.ReasonLoopAlreadyDecided, preserving conflict semantics for lifecycle compare-and-swap rejections.internal/loop/coordinator_test.go-749-751 (1)
749-751: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis parent-mutation assertion cannot fail.
parentis aRunvalue.refreshAwaitingChildOutputreceives a copy, so it can never change the localparent. The check always passes and does not protect the stated invariant. Assert against the persisted parent run instead, for example by reading the run back from the runner store fixture after the call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/coordinator_test.go` around lines 749 - 751, Replace the ineffective local parent assertion in the refreshAwaitingChildOutput test with an assertion against the persisted parent run, rereading it from the runner store fixture after the call. Verify the stored run still has StatusRunning and CancelRequested unset, while preserving the existing child-terminal scenario.internal/loop/service.go-344-365 (1)
344-365: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGate
StatusCanceledfor live states outside cancellation paths.
allowedTransitionallowsStatusRunningandStatusWatchingto becomeStatusCanceledwith any cause, whileStatusQueued,StatusNeedsApproval, andStatusPausedrequireTransitionCauseOperatorCancelorTransitionCauseOperatorKill. If non-operator causes must reachcanceledfrom these live states, add the specific cause tocancelTransitionor use another terminal status for that path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/service.go` around lines 344 - 365, The allowedTransition cases for StatusRunning and StatusWatching currently permit StatusCanceled regardless of cause. Align these live-state transitions with the cause-gated behavior used by StatusNeedsApproval and StatusPaused: remove unconditional StatusCanceled acceptance and route it through cancelTransition, or explicitly allow only the required operator cancellation causes while preserving other transitions.internal/store/globaldb/global_db_loop_node_inventory.go-142-167 (1)
142-167: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDrop
item_indexfrom quarantined and attention cursors.
ListQuarantinedLoopNodeInventoryandListAttentionLoopNodeInventoryreturn0 AS item_indexand order/keyset only onstate_at/at,run.id,generation, andnode_id, while waiting/retrying pages includeitem_indexin both the query and cursor. Since these queries do not useitem_indexfor ordering or pagination, do not passCursorItemIndexforNodeInventoryQuarantinedandNodeInventoryAttentionso the cursor contract matches each query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_node_inventory.go` around lines 142 - 167, Update the NodeInventoryQuarantined and NodeInventoryAttention branches to omit CursorItemIndex from their query parameter structs, matching the queries’ ordering and pagination keys. Leave the waiting/retrying cursor handling unchanged.internal/store/globaldb/global_db_loop_node_pause_effects.go-52-52 (1)
52-52: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize
mutation.RequestedAtto UTC before persistence.The sibling cancellation path in
internal/store/globaldb/global_db_loop_cancel_finalize.gopassesmutation.RequestedAt.UTC()intoinsertLoopEffectIntentsWithExecutor. This call passes the raw value. If a caller supplies a non-UTC time, the stored effect intent timestamps use a different zone from the surrounding rows, which affects comparisons and ordering.🐛 Proposed fix
- return insertLoopEffectIntentsWithExecutor(ctx, exec, run, sourceID, intents, mutation.RequestedAt) + return insertLoopEffectIntentsWithExecutor(ctx, exec, run, sourceID, intents, mutation.RequestedAt.UTC())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/globaldb/global_db_loop_node_pause_effects.go` at line 52, Update the pause-effect persistence call in insertLoopEffectIntentsWithExecutor’s caller to pass mutation.RequestedAt normalized with UTC, matching the sibling cancellation path and ensuring stored intent timestamps use UTC.internal/loop/linter_references.go-151-158 (1)
151-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude wait event filters in unknown-output reference linting.
Control-wait event subscriptions can live under
params.event, butlintWaitReferences()only passesparams.untilthroughcompileTemplate()andlintWatchEventsFilterReferences()readsnode.Events, which is empty for control-wait nodes. Add a dedicated check fromparams.event.Filter, or include the remaining wait param fields so template unknown-output references in the filter are still reported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/linter_references.go` around lines 151 - 158, Update nodeParamStringFields and the lintWaitReferences flow to decode control-wait dsl.WaitParams and inspect params.event.Filter in addition to params.until, so template references in wait event filters are passed through the existing unknown-output linting path even when node.Events is empty. Reuse the existing compileTemplate/linting helpers and preserve the current behavior for empty or invalid wait parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 36afcc2d-861a-4ddf-95df-1d88302f091a
⛔ Files ignored due to path filters (50)
docs/qa/bugs/BUG-20260802-initial-wait-fails-run.mdis excluded by!**/*.mddocs/qa/bugs/BUG-20260802-node-kill-leaves-run-live.mdis excluded by!**/*.mddocs/qa/bugs/BUG-20260802-parked-node-cancel-stalls.mdis excluded by!**/*.mddocs/qa/charters/CH-003.mdis excluded by!**/*.mddocs/qa/charters/CH-agent-loop-lifecycle-native.mdis excluded by!**/*.mddocs/qa/charters/CH-loop-death-resume.mdis excluded by!**/*.mddocs/qa/charters/CH-loop-run-cancel-control.mdis excluded by!**/*.mddocs/qa/charters/CH-runaway-work-bounded.mdis excluded by!**/*.mddocs/qa/journeys/J-04-operator-pause-resume.mdis excluded by!**/*.mddocs/qa/journeys/J-07-agent-operated-run.mdis excluded by!**/*.mddocs/qa/journeys/J-bound-runaway-work.mdis excluded by!**/*.mddocs/qa/journeys/J-resume-dead-loop-node.mdis excluded by!**/*.mddocs/qa/reports/2026-08-02-loop-node-lifecycle-task07.mdis excluded by!**/*.mddocs/qa/scenarios/LP-016.mdis excluded by!**/*.mddocs/qa/scenarios/LP-agent-operates-lifecycle-via-native-tools.mdis excluded by!**/*.mddocs/qa/scenarios/LP-approval-link-journey.mdis excluded by!**/*.mddocs/qa/scenarios/LP-cancel-vs-kill.mdis excluded by!**/*.mddocs/qa/scenarios/LP-crash-death-resume.mdis excluded by!**/*.mddocs/qa/scenarios/LP-days-long-node-no-clock.mdis excluded by!**/*.mddocs/qa/scenarios/LP-duplicate-event-suppressed.mdis excluded by!**/*.mddocs/qa/scenarios/LP-durable-wait-restart.mdis excluded by!**/*.mddocs/qa/scenarios/LP-live-pause-repair-resume.mdis excluded by!**/*.mddocs/qa/scenarios/LP-waiting-inventory-escalation.mdis excluded by!**/*.mddocs/qa/scenarios/TA-076.mdis excluded by!**/*.mddocs/qa/scenarios/TA-084.mdis excluded by!**/*.mddocs/qa/scenarios/TA-action-run-liveness.mdis excluded by!**/*.mdinternal/store/globaldb/schema/migrations/atlas.sumis excluded by!**/*.sum,!**/*.suminternal/tools/builtin/testdata/native-tool-catalog.jsonis excluded by!**/*.jsonopenapi/compozy.jsonis excluded by!**/*.jsonpackages/site/content/docs/cli/loop/cancel.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/index.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/kill.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/meta.jsonis excluded by!**/*.jsonpackages/site/content/docs/cli/loop/node/cancel.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/node/index.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/node/kill.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/node/meta.jsonis excluded by!**/*.jsonpackages/site/content/docs/cli/loop/node/pause.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/node/requeue.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/node/resume.mdxis excluded by!**/*.mdxpackages/site/content/docs/cli/loop/nodes.mdxis excluded by!**/*.mdxpackages/site/content/docs/loops/running.mdxis excluded by!**/*.mdxsdk/typescript/src/generated/contracts.tsis excluded by!**/generated/**,!**/generated/**skills/compozy/references/extension-authoring.mdis excluded by!**/*.mdskills/compozy/references/loops.mdis excluded by!**/*.mdskills/compozy/references/tasks-and-orchestration.mdis excluded by!**/*.mdweb/src/generated/compozy-openapi.d.tsis excluded by!**/generated/**,!**/generated/**,!**/*.d.tsweb/src/generated/loop-enums.tsis excluded by!**/generated/**,!**/generated/**web/src/systems/loops/components/stories/loop-run-page.stories.tsxis excluded by!**/*.stories.tsxweb/src/systems/loops/components/stories/loop-status-pill.stories.tsxis excluded by!**/*.stories.tsx
📒 Files selected for processing (250)
internal/api/contract/generic_error_payload.gointernal/api/contract/loop_enums.gointernal/api/contract/loop_nodes.gointernal/api/contract/loop_runs.gointernal/api/contract/loops.gointernal/api/core/errors_diagnostic_payload.gointernal/api/core/errors_test.gointernal/api/core/loop_interfaces.gointernal/api/core/loops.gointernal/api/core/loops_nodes.gointernal/api/core/loops_test.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/loops_routes.gointernal/api/spec/loops.gointernal/api/spec/loops_nodes.gointernal/api/spec/loops_test.gointernal/api/spec/schema_enum_registry.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/loops_routes.gointernal/automation/dispatch_loop.gointernal/cli/client_loops.gointernal/cli/helpers_test.gointernal/cli/loop.gointernal/cli/loop_client.gointernal/cli/loop_inspect_output.gointernal/cli/loop_node.gointernal/cli/loop_nodes.gointernal/cli/loop_runs.gointernal/cli/loop_support.gointernal/cli/loop_test.gointernal/cli/marketplace.gointernal/config/merge_task.gointernal/config/task_orchestration.gointernal/config/task_orchestration_test.gointernal/config/task_orchestration_validation.gointernal/config/tool_surface_task.gointernal/config/tool_surface_test.gointernal/daemon/automation_loop_starter.gointernal/daemon/loop_action_checkpoint.gointernal/daemon/loop_action_death_resume.gointernal/daemon/loop_action_liveness.gointernal/daemon/loop_action_liveness_integration_helpers_test.gointernal/daemon/loop_action_runtime.gointernal/daemon/loop_api_contract.gointernal/daemon/loop_api_nodes.gointernal/daemon/loop_api_payloads_test.gointernal/daemon/loop_api_run_history.gointernal/daemon/loop_api_runs.gointernal/daemon/loop_api_runs_test.gointernal/daemon/loop_api_service.gointernal/daemon/loop_goal_managed_runtime_integration_test.gointernal/daemon/loop_hook_observer.gointernal/daemon/loop_hook_observer_test.gointernal/daemon/loop_managed_binding.gointernal/daemon/loop_parent_close_postcommit.gointernal/daemon/loop_watch_events_doorbell.gointernal/daemon/loop_watch_events_observer.gointernal/daemon/loop_watch_events_observer_index.gointernal/daemon/loop_watch_events_observer_test.gointernal/daemon/loop_watch_events_payloads.gointernal/daemon/native_hook_helpers.gointernal/daemon/native_loop_lifecycle_support.gointernal/daemon/native_loop_lifecycle_tools.gointernal/daemon/native_loop_tools.gointernal/daemon/native_loop_tools_support.gointernal/daemon/native_loop_tools_test.gointernal/daemon/scheduler_loop_coordinator.gointernal/daemon/scheduler_loop_due_scan.gointernal/daemon/scheduler_runtime.gointernal/daemon/scheduler_task_source_types.gointernal/daemon/task_runtime_boot.gointernal/daemon/task_runtime_test.gointernal/extension/scaffold_templates/loop-watch-source-go/main.gointernal/extension/watch_source.gointernal/extension/watch_source_test.gointernal/hooks/payloads_task_loop.gointernal/loop/action_failure.gointernal/loop/action_retry_feedback.gointernal/loop/action_runagent.gointernal/loop/action_schema.gointernal/loop/action_test.gointernal/loop/action_types.gointernal/loop/action_usage.gointernal/loop/admission_claim.gointernal/loop/cancel_control.gointernal/loop/compiler.gointernal/loop/control_eval_context.gointernal/loop/control_plan.gointernal/loop/control_plan_branch.gointernal/loop/control_plan_finish.gointernal/loop/control_topology.gointernal/loop/coordinator.gointernal/loop/coordinator_action.gointernal/loop/coordinator_action_liveness.gointernal/loop/coordinator_action_metadata.gointernal/loop/coordinator_autopause.gointernal/loop/coordinator_cancellation.gointernal/loop/coordinator_control_test.gointernal/loop/coordinator_effects.gointernal/loop/coordinator_error_routes.gointernal/loop/coordinator_gate_wait.gointernal/loop/coordinator_generation.gointernal/loop/coordinator_lifecycle.gointernal/loop/coordinator_lifecycle_test.gointernal/loop/coordinator_metadata.gointernal/loop/coordinator_outputs.gointernal/loop/coordinator_pause.gointernal/loop/coordinator_quarantine.gointernal/loop/coordinator_test.gointernal/loop/coordinator_wait.gointernal/loop/coordinator_wait_route.gointernal/loop/coordinator_watch_test.gointernal/loop/death_resume.gointernal/loop/dsl/graph.gointernal/loop/effects_dispatch.gointernal/loop/effects_dispatch_test.gointernal/loop/errors.gointernal/loop/executed_definition_validation.gointernal/loop/executed_definition_watch.gointernal/loop/generation_snapshot.gointernal/loop/generation_snapshot_controls.gointernal/loop/generation_snapshot_intents.gointernal/loop/goal_cancellation.gointernal/loop/linter.gointernal/loop/linter_references.gointernal/loop/linter_test.gointernal/loop/linter_wait.gointernal/loop/node_control_mutation.gointernal/loop/node_inventory.gointernal/loop/node_liveness.gointernal/loop/node_pause.gointernal/loop/node_wait.gointernal/loop/node_waits.gointernal/loop/parent_close.gointernal/loop/run_network_state.gointernal/loop/service.gointernal/loop/service_cancellation.gointernal/loop/service_control.gointernal/loop/service_node_pause.gointernal/loop/service_node_requeue.gointernal/loop/service_options.gointernal/loop/service_start.gointernal/loop/service_test.gointernal/loop/service_types.gointernal/loop/wait_due.gointernal/loop/watch/adapter.gointernal/loop/watch/adapter_test.gointernal/loop/watch/event_key.gointernal/loop/watch/output.gointernal/loop/watch/types.gointernal/loop/watch_events_constraints.gointernal/loop/watch_events_contract.gointernal/loop/watch_events_ledger.gointernal/session/manager_prompt_submit.gointernal/session/manager_test.gointernal/session/session_supervision.gointernal/store/globaldb/global_db_goal_reentry.gointernal/store/globaldb/global_db_goal_reentry_helpers.gointernal/store/globaldb/global_db_goal_reentry_test.gointernal/store/globaldb/global_db_goal_stop.gointernal/store/globaldb/global_db_goal_stop_checkpoints.gointernal/store/globaldb/global_db_loop.gointernal/store/globaldb/global_db_loop_admission_claims.gointernal/store/globaldb/global_db_loop_approval_wait.gointernal/store/globaldb/global_db_loop_cancel.gointernal/store/globaldb/global_db_loop_cancel_finalize.gointernal/store/globaldb/global_db_loop_cancel_live.gointernal/store/globaldb/global_db_loop_cancel_sessions.gointernal/store/globaldb/global_db_loop_cancel_wake.gointernal/store/globaldb/global_db_loop_control.gointernal/store/globaldb/global_db_loop_coordinator_seed.gointernal/store/globaldb/global_db_loop_death_resume.gointernal/store/globaldb/global_db_loop_effect_outbox.gointernal/store/globaldb/global_db_loop_events.gointernal/store/globaldb/global_db_loop_inline.gointernal/store/globaldb/global_db_loop_lifecycle_errors.gointernal/store/globaldb/global_db_loop_node_inventory.gointernal/store/globaldb/global_db_loop_node_inventory_details.gointernal/store/globaldb/global_db_loop_node_liveness.gointernal/store/globaldb/global_db_loop_node_pause.gointernal/store/globaldb/global_db_loop_node_pause_effects.gointernal/store/globaldb/global_db_loop_node_requeue.gointernal/store/globaldb/global_db_loop_node_terminal.gointernal/store/globaldb/global_db_loop_normalize.gointernal/store/globaldb/global_db_loop_parked_clock.gointernal/store/globaldb/global_db_loop_pinning.gointernal/store/globaldb/global_db_loop_schema_integration_test.gointernal/store/globaldb/global_db_loop_test.gointernal/store/globaldb/global_db_loop_transition.gointernal/store/globaldb/global_db_loop_wait_due.gointernal/store/globaldb/global_db_loop_wait_escalation.gointernal/store/globaldb/global_db_loop_wait_escalation_apply.gointernal/store/globaldb/global_db_loop_wait_events.gointernal/store/globaldb/global_db_loop_wait_expiry_route.gointernal/store/globaldb/global_db_loop_wait_resume.gointernal/store/globaldb/global_db_network_work_mutation.gointernal/store/globaldb/global_db_task_claim.gointernal/store/globaldb/global_db_task_claim_helpers.gointernal/store/globaldb/global_db_task_claim_test.gointernal/store/globaldb/global_db_task_coordinator.gointernal/store/globaldb/global_db_task_coordinator_budget.gointernal/store/globaldb/global_db_task_coordinator_cancellation.gointernal/store/globaldb/global_db_task_coordinator_effects.gointernal/store/globaldb/global_db_task_coordinator_intents.gointernal/store/globaldb/global_db_task_coordinator_mutations.gointernal/store/globaldb/global_db_task_coordinator_parent_close.gointernal/store/globaldb/global_db_task_coordinator_runtime.gointernal/store/globaldb/global_db_watch_events_loop.gointernal/store/globaldb/queries/loop_lifecycle.sqlinternal/store/globaldb/schema/definitions/50_loops.sqlinternal/store/globaldb/schema/migrations/00045_schema.sqlinternal/store/globaldb/sqlcgen/loop_lifecycle.sql.gointernal/store/globaldb/sqlcgen/models.gointernal/task/coordinator.gointernal/task/coordinator_boundary_specs.gointernal/task/manager.gointernal/task/manager_constructor.gointernal/task/manager_coordinator_run.gointernal/task/manager_test.gointernal/toolmeta/native_entries.gointernal/toolmeta/registry_test.gointernal/tools/builtin/builtin_test.gointernal/tools/builtin/descriptors.gointernal/tools/builtin/loops.gointernal/tools/builtin/loops_lifecycle_schemas.gointernal/tools/builtin/loops_output_schemas.gointernal/tools/builtin/tasks.gointernal/tools/builtin_ids.gointernal/tools/reason.gosdk/go/contracts/types_011_gen.gosdk/go/extension_test.gosdk/go/types.gosdk/go/watch_source.gosdk/typescript/src/__tests__/extension.test.tssdk/typescript/src/watch-source.tsweb/src/systems/loops/adapters/__tests__/loops-api.test.tsweb/src/systems/loops/adapters/loops-api.tsweb/src/systems/loops/adapters/loops-runs-api.tsweb/src/systems/loops/components/__tests__/loop-run-page.test.tsxweb/src/systems/loops/components/run-page/loop-run-controls.tsxweb/src/systems/loops/hooks/use-loop-actions.tsweb/src/systems/loops/index.tsweb/src/systems/loops/lib/__tests__/loop-formatters.test.tsweb/src/systems/loops/lib/loop-formatters.tsweb/src/systems/loops/lib/loop-generation-presentation.tsweb/src/systems/loops/mocks/fixture-run-details.tsweb/src/systems/loops/mocks/handlers.tsweb/src/systems/loops/types.tsweb/src/systems/os/apps/loops/loop-run-detail-location.tsxweb/src/systems/os/apps/loops/use-loop-run-page.ts
💤 Files with no reviewable changes (9)
- internal/config/task_orchestration.go
- internal/config/merge_task.go
- internal/api/core/loops.go
- internal/config/tool_surface_test.go
- internal/config/tool_surface_task.go
- internal/config/task_orchestration_validation.go
- internal/loop/service_control.go
- internal/config/task_orchestration_test.go
- internal/store/globaldb/global_db_goal_stop.go
Comment on lines
+134
to
+150
| func (h *BaseHandlers) ListLoopNodes(c *gin.Context) { | ||
| service, ok := h.requireLoopService(c) | ||
| if !ok { | ||
| return | ||
| } | ||
| query, err := ParseLoopNodeListQuery(c) | ||
| if err != nil { | ||
| h.respondLoopError(c, fmt.Errorf("%w: loop node inventory query: %v", looppkg.ErrValidation, err)) | ||
| return | ||
| } | ||
| response, err := service.ListLoopNodes(c.Request.Context(), c.Param("workspace_id"), query) | ||
| if err != nil { | ||
| h.respondLoopError(c, err) | ||
| return | ||
| } | ||
| c.JSON(http.StatusOK, response) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare workspace scoping between loop read handlers and the new inventory handler.
set -euo pipefail
fd -t f 'loops.*\.go' internal/api | xargs rg -n -C4 'workspace_id"\)' | rg -n -C4 'ListLoop|GetLoopRun|ListLoopNodes'
rg -n -C6 'func \(h \*BaseHandlers\) (ListLoopRuns|GetLoopRun)\(' internal/api/core
rg -n -C4 'taskActorContextForWorkspace|requireWorkspaceScope|authorizeWorkspace' internal/api/core internal/api/httpapi internal/api/udsapiRepository: compozy/compozy
Length of output: 20037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate workspace-scope definitions/handlers and compare read endpoints.
rg -n -C8 'taskActorContextForWorkspace|requireWorkspaceScope|authorizeWorkspace|workspace_scope|WorkspaceID' internal/api/core internal/api/httpapi internal/api/udsapi internal/api
printf '\n--- loops.go read handlers ---\n'
sed -n '220,320p' internal/api/core/loops.go
printf '\n--- loops_nodes.go handlers ---\n'
sed -n '1,170p' internal/api/core/loops_nodes.goRepository: compozy/compozy
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused workspace scope definitions ---'
rg -n -C6 'func \(h \*BaseHandlers\) taskActorContextForWorkspace|func \(h \*BaseHandlers\) requireWorkspaceScope|func .*authorizeWorkspace|WorkspaceID\(\)' internal/api/core/tasks.go internal/api/core/tasks_internal_test.go internal/api/core/router.go internal/api/core/workspaces.go | sed -n '1,240p'
printf '%s\n' '--- BaseHandlers auth session workspace extraction context ---'
rg -n -C4 'Pending\.WorkspaceID|caller.*Workspace|Session.*WorkspaceID|WorkspaceID\(\)|context\.Value\(.*Workspace|caller\(\)' internal/api/core/tasks.go internal/api/core/transport auth internal || true
printf '%s\n' '--- relevant read handlers context ---'
sed -n '220,320p' internal/api/core/loops.go
sed -n '1,170p' internal/api/core/loops_nodes.goRepository: compozy/compozy
Length of output: 1072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- taskActorContextForWorkspace implementation ---'
sed -n '100,210p' internal/api/core/tasks.go
printf '%s\n' '--- BaseHandlers config fields ---'
sed -n '1,100p' internal/api/core/base.go
printf '%s\n' '--- workspace scope extraction references ---'
rg -n -C4 'TaskActorContextResolver|Pending|WorkspaceID\(\)|Caller|caller\(' internal/api/core --glob '*.go' | sed -n '1,260p'Repository: compozy/compozy
Length of output: 1677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate BaseHandlers config/constructor references ---'
rg -n -C4 'type BaseHandlers|type BaseHandlerConfig|NewBaseHandlers|TaskActorContextResolver' internal/api --glob '*.go' | sed -n '1,260p'
printf '%s\n' '--- relevant BaseHandlers/workspace resolver snippet ---'
rg -n -C3 'resolveAgentCallerForWorkspace|HasAccess|Workspace.*Access|Pending\.Workspace|Session\.WorkspaceID|WorkspaceID' internal/api/core internal/api/httpapi internal/api/udsapi --glob '*.go' | sed -n '1,260p'Repository: compozy/compozy
Length of output: 37254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- coreHandlerConfig around resolver ---'
rg -n -C12 'coreHandlerConfig|TaskActorContextResolver.*func|func resolver|func.*resolver|AgentCaller|resolveSessionForWorkspace|Pending' internal/api/httpapi --glob '*.go' | sed -n '1,260p'
printf '%s\n' '--- udsCoreHandlerConfig around resolver ---'
rg -n -C12 'udsCoreHandlerConfig|TaskActorContextResolver.*func|func resolver|func.*resolver|AgentCaller|resolveSessionForWorkspace|Pending' internal/api/udsapi --glob '*.go' | sed -n '1,240p'
printf '%s\n' '--- full snippets if available ---'
fd -t f -i 'handlers.go|core_handlers.go' internal/api/httpapi internal/api/udsapi internal/api/core | while read -r f; do
echo "### $f"
rg -n -C8 'TaskActorContextResolver|resolveSessionForWorkspace|resolve.*Workspace|AgentCaller|agentCallerCredentialsFromRequest|WorkspaceID\(\)' "$f" | sed -n '1,180p'
doneRepository: compozy/compozy
Length of output: 26012
Match the other loop read endpoints.
ListLoopNodes uses c.Param("workspace_id") directly like ListLoopRuns and GetLoopRun, while loop mutations enforce scope with taskActorContextForWorkspace. Return inventory only in the caller’s permitted workspace before using the path parameter or add a read-scoped workspace check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/core/loops_nodes.go` around lines 134 - 150, Update
ListLoopNodes to enforce the caller’s workspace scope before invoking
service.ListLoopNodes, matching the authorization behavior of loop mutations and
other loop read endpoints. Reuse taskActorContextForWorkspace or the established
read-scoped workspace check, and only use c.Param("workspace_id") after
permission validation.
Comment on lines
+165
to
+169
| func automationLoopAdmission(req automationpkg.LoopStartRequest) *looppkg.AdmissionIdentity { | ||
| if strings.TrimSpace(req.SourceKey) == "" || strings.TrimSpace(req.EventKey) == "" { | ||
| return nil | ||
| } | ||
| return &looppkg.AdmissionIdentity{SourceKey: req.SourceKey, EventKey: req.EventKey} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Trim the keys that you store in AdmissionIdentity.
The guard trims req.SourceKey and req.EventKey, but the returned identity carries the untrimmed values. The admission identity is a deduplication key, so leading or trailing whitespace produces a different key for the same logical event. The dispatcher path currently trims event_key, but no other caller is required to.
🐛 Proposed fix
func automationLoopAdmission(req automationpkg.LoopStartRequest) *looppkg.AdmissionIdentity {
- if strings.TrimSpace(req.SourceKey) == "" || strings.TrimSpace(req.EventKey) == "" {
+ sourceKey := strings.TrimSpace(req.SourceKey)
+ eventKey := strings.TrimSpace(req.EventKey)
+ if sourceKey == "" || eventKey == "" {
return nil
}
- return &looppkg.AdmissionIdentity{SourceKey: req.SourceKey, EventKey: req.EventKey}
+ return &looppkg.AdmissionIdentity{SourceKey: sourceKey, EventKey: eventKey}
}📝 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 automationLoopAdmission(req automationpkg.LoopStartRequest) *looppkg.AdmissionIdentity { | |
| if strings.TrimSpace(req.SourceKey) == "" || strings.TrimSpace(req.EventKey) == "" { | |
| return nil | |
| } | |
| return &looppkg.AdmissionIdentity{SourceKey: req.SourceKey, EventKey: req.EventKey} | |
| func automationLoopAdmission(req automationpkg.LoopStartRequest) *looppkg.AdmissionIdentity { | |
| sourceKey := strings.TrimSpace(req.SourceKey) | |
| eventKey := strings.TrimSpace(req.EventKey) | |
| if sourceKey == "" || eventKey == "" { | |
| return nil | |
| } | |
| return &looppkg.AdmissionIdentity{SourceKey: sourceKey, EventKey: eventKey} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/automation_loop_starter.go` around lines 165 - 169, Update
automationLoopAdmission to store trimmed SourceKey and EventKey in
looppkg.AdmissionIdentity, reusing the validated trimmed values for both the
blank checks and returned deduplication key so leading or trailing whitespace
cannot create distinct identities.
Comment on lines
+40
to
+48
| for _, event := range events { | ||
| partial, found, err := deathResumePartialFromEvent(event) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if found { | ||
| checkpoint.Partials = append(checkpoint.Partials, partial) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One malformed event blocks the whole recovery.
deathResumePartialFromEvent returns an error when transcript.UnmarshalAgentEvent fails, and this loop propagates it. The caller then abandons the confirmed-death continuation. A checkpoint is best-effort context for the prompt, so a single undecodable event should not stop recovery. Skip the event and log it instead.
♻️ Proposed direction
for _, event := range events {
partial, found, err := deathResumePartialFromEvent(event)
if err != nil {
- return nil, err
+ // Best-effort context: a single undecodable event must not block recovery.
+ continue
}
if found {
checkpoint.Partials = append(checkpoint.Partials, partial)
}
}Pass a logger into the helper if you want the skipped sequence recorded.
📝 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
| for _, event := range events { | |
| partial, found, err := deathResumePartialFromEvent(event) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if found { | |
| checkpoint.Partials = append(checkpoint.Partials, partial) | |
| } | |
| } | |
| for _, event := range events { | |
| partial, found, err := deathResumePartialFromEvent(event) | |
| if err != nil { | |
| // Best-effort context: a single undecodable event must not block recovery. | |
| continue | |
| } | |
| if found { | |
| checkpoint.Partials = append(checkpoint.Partials, partial) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/loop_action_checkpoint.go` around lines 40 - 48, Update the
event-processing loop around deathResumePartialFromEvent so an unmarshalling
error is logged and the malformed event is skipped rather than returned to the
caller. Preserve appending successfully decoded partials and allow recovery to
continue processing subsequent events; pass a logger into the helper or use the
surrounding logger so the skipped event’s sequence is recorded.
Comment on lines
+25
to
+28
| info, err := r.sessions.Status(ctx, strings.TrimSpace(sessionID)) | ||
| if err != nil || !confirmedLoopActionSessionDeath(info) { | ||
| return false, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not swallow the Status error silently.
Line 26 collapses a store failure and a live session into the same result. A transient Status error is then reported as "not dead", and the confirmed-death path is skipped with no record. Separate the two cases: log or return the error, and keep the fail-open behavior only as a documented decision.
The coding guidelines require that every error is handled or carries a written justification.
🐛 Proposed fix
info, err := r.sessions.Status(ctx, strings.TrimSpace(sessionID))
- if err != nil || !confirmedLoopActionSessionDeath(info) {
+ if err != nil {
+ // Fail open: an unreadable session status must not block normal failure handling.
+ r.logger.Warn("daemon: read Loop action session status", "session_id", sessionID, "error", err)
+ return false, nil
+ }
+ if !confirmedLoopActionSessionDeath(info) {
return false, nil
}📝 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
| info, err := r.sessions.Status(ctx, strings.TrimSpace(sessionID)) | |
| if err != nil || !confirmedLoopActionSessionDeath(info) { | |
| return false, nil | |
| } | |
| info, err := r.sessions.Status(ctx, strings.TrimSpace(sessionID)) | |
| if err != nil { | |
| // Fail open: an unreadable session status must not block normal failure handling. | |
| r.logger.Warn("daemon: read Loop action session status", "session_id", sessionID, "error", err) | |
| return false, nil | |
| } | |
| if !confirmedLoopActionSessionDeath(info) { | |
| return false, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/loop_action_death_resume.go` around lines 25 - 28, Update the
Status call in the session-death resume flow to handle its error separately from
confirmedLoopActionSessionDeath: log or return the Status error with appropriate
context, and retain the existing false result only for a successful status check
showing the session is not dead. Document the intentional fail-open behavior if
the function continues suppressing the error.
Source: Coding guidelines
Comment on lines
+226
to
+239
| result, controlled, err := r.executeClaimedRun( | ||
| ctx, | ||
| claim, | ||
| actor, | ||
| leaseDuration, | ||
| actionTimeout, | ||
| authoredTimeout, | ||
| timeoutReason, | ||
| looppkg.WorkspaceID(taskRecord.WorkspaceID), | ||
| silenceWindow, | ||
| deathStreakLimit, | ||
| ) | ||
| if controlled { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect executeClaimedRun to confirm lease completion/failure in the controlled branch.
set -euo pipefail
ast-grep run --pattern 'func (r *loopActionRuntime) executeClaimedRun($$$) {
$$$
}' --lang go internal/daemon/loop_action_runtime.go
# Fallback broader search if the method lives in another file
rg -n -A 60 'func \(r \*loopActionRuntime\) executeClaimedRun' internal/daemonRepository: compozy/compozy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== matched files =="
git ls-files | rg '(^|/)loop_action_runtime\.go$|loop_action|action_runtime' || true
echo "== executeClaimedRun references =="
rg -n "executeClaimedRun|CompleteRunLease|failClaimedRun|controlled" internal/daemon || true
echo "== loop_action_runtime outline =="
ast-grep outline internal/daemon/loop_action_runtime.go --view compact 2>/dev/null || true
echo "== relevant loop_action_runtime lines =="
if [ -f internal/daemon/loop_action_runtime.go ]; then
wc -l internal/daemon/loop_action_runtime.go
sed -n '200,255p' internal/daemon/loop_action_runtime.go
fiRepository: compozy/compozy
Length of output: 4288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== loop_action_liveness executeClaimedRun section =="
wc -l internal/daemon/loop_action_liveness.go
sed -n '1,220p' internal/daemon/loop_action_liveness.go
echo "== behavioral verifier: controlled return lease handling =="
python3 - <<'PY'
from pathlib import Path
src = Path("internal/daemon/loop_action_liveness.go").read_text()
for pat in ["err = r.failClaimedRun", "CompleteRunLease"]:
print(f"{pat}: {src.count(pat)}")
if "err := r.failClaimedRun" not in src and "CompleteRunLease" not in src:
print("executeClaimedRun controlled branch does not show local lease completion/failure")
PYRepository: compozy/compozy
Length of output: 6051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== heartbeatClaim completion/failure references =="
sed -n '220,426p' internal/daemon/loop_action_liveness.go
echo "== all heartbeatClaim definitions/usages =="
rg -n "func \(.*heartbeatClaim|heartbeatClaim\\(|CompleteRunLease\\(|failClaimedRun\\(" internal/daemon -g '*.go' || true
echo "== failClaimedRun signature =="
sed -n '250,310p' internal/daemon/loop_action_runtime.go
echo "== exact controlled call context =="
python3 - <<'PY'
from pathlib import Path
p = Path("internal/daemon/loop_action_runtime.go")
s = p.read_text().splitlines()
for i, line in enumerate(s, 1):
if "result, controlled, err := r.executeClaimedRun(" in line:
start = max(1, i-8); end = min(len(s), i+22)
print(f"\n--- loop_action_runtime.go:{start}-{end} ---")
for j in range(start, end+1):
print(f"{j:4}: {s[j-1]}")
PYRepository: compozy/compozy
Length of output: 9898
Handle the lease when executeClaimedRun returns controlled = true.
When controlled is true, executeQueuedRun returns err immediately and skips failClaimedRun and CompleteRunLease. executeClaimedRun only starts heartbeatClaim and returns true for a handled dead action, but heartbeatClaim only heartbeats/extends the lease. Finalize the claimed lease in this branch, and document the controlled path clearly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/loop_action_runtime.go` around lines 226 - 239, Update the
controlled branch in executeQueuedRun after executeClaimedRun returns
controlled=true to finalize the claimed lease before returning, using the
existing failClaimedRun and CompleteRunLease flow as appropriate. Document that
this handled dead-action path is responsible for lease finalization, while
preserving the returned error behavior.
Comment on lines
+29
to
+56
| cells, next, err := g.listDueLoopWaits(ctx, now, cursor, limit) | ||
| if err != nil { | ||
| return nil, looppkg.WaitDueCursor{}, err | ||
| } | ||
| runs := make([]taskpkg.Run, 0, len(cells)) | ||
| for _, cell := range cells { | ||
| admissionAttempts, configErr := g.waitDueAdmissionAttempts(ctx, cell) | ||
| if configErr != nil { | ||
| return runs, next, configErr | ||
| } | ||
| result, resumeErr := g.ResumeWait(ctx, looppkg.WaitResumeMutation{ | ||
| WorkspaceID: cell.workspaceID, RunID: cell.runID, Generation: cell.generation, | ||
| NodeID: cell.nodeID, ItemIndex: cell.itemIndex, Payload: []byte(`{}`), | ||
| ClaimedByKind: loopWaitClaimedByTimer, ClaimedByID: loopWaitClaimedByScheduler, | ||
| AdmissionAttempts: admissionAttempts, RequestedAt: now.UTC(), | ||
| }) | ||
| if errors.Is(resumeErr, looppkg.ErrTransitionConflict) || errors.Is(resumeErr, looppkg.ErrInvalidTransition) { | ||
| continue | ||
| } | ||
| if resumeErr != nil { | ||
| return runs, next, resumeErr | ||
| } | ||
| if result.Won && result.Coordinator != nil { | ||
| runs = append(runs, *result.Coordinator) | ||
| } | ||
| } | ||
| return runs, next, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A mid-page error advances the scan cursor past unprocessed waits. The page functions return the end-of-page cursor together with a hard error, and the scheduler stores that cursor before it checks the error. Every wait between the failure point and the end of the page is then skipped for that pass, so due timers and escalations can stay unprocessed until the cursor resets on a short page.
internal/store/globaldb/global_db_loop_wait_due.go#L29-L56: onconfigErrand onresumeErr, return a cursor that points at the last successfully resumed cell, or returnlooppkg.WaitDueCursor{}instead ofnext.internal/store/globaldb/global_db_loop_wait_escalation.go#L29-L42: apply the same change to the hard-error return, which currently returnsnext, err.internal/daemon/scheduler_loop_due_scan.go#L112-L127: assigns.loopWaitDueScan.cursor = nextonly whenerris nil.internal/daemon/scheduler_loop_due_scan.go#L129-L144: assigns.loopWaitEscalationScan.cursor = nextonly whenerris nil.
📍 Affects 3 files
internal/store/globaldb/global_db_loop_wait_due.go#L29-L56(this comment)internal/store/globaldb/global_db_loop_wait_escalation.go#L29-L42internal/daemon/scheduler_loop_due_scan.go#L112-L127internal/daemon/scheduler_loop_due_scan.go#L129-L144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/globaldb/global_db_loop_wait_due.go` around lines 29 - 56,
Prevent failed mid-page scans from advancing past unprocessed waits: in
internal/store/globaldb/global_db_loop_wait_due.go lines 29-56, update the
configErr and resumeErr paths to return a cursor at the last successfully
resumed cell, or an empty looppkg.WaitDueCursor{} when none succeeded, instead
of next; in internal/store/globaldb/global_db_loop_wait_escalation.go lines
29-42, apply the same cursor handling to its hard-error return. In
internal/daemon/scheduler_loop_due_scan.go lines 112-127 and 129-144, update the
due and escalation scan flows to assign their cursors only when err is nil.
Comment on lines
+58
to
+80
| func (g *LoopRepo) waitDueAdmissionAttempts(ctx context.Context, cell waitDueCell) (int, error) { | ||
| run, err := getLoopRunByIDWithExecutor(ctx, g.db, cell.runID) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| resolved, err := loadWaitExecutedDefinition(ctx, g.db, run) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| node, found := loopWaitNodeByRuntimeID(resolved.Definition.Graph, "", string(cell.nodeID)) | ||
| if !found { | ||
| return 0, fmt.Errorf( | ||
| "%w: due Loop wait node %q is absent from the executed definition", | ||
| looppkg.ErrValidation, | ||
| cell.nodeID, | ||
| ) | ||
| } | ||
| lifecycle, err := looppkg.ResolveNodeLifecycleConfig(node, nil, resolved.EffectiveConfig.Lifecycle) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("store: resolve due Loop wait lifecycle: %w", err) | ||
| } | ||
| return lifecycle.WaitAdmissionAttempts, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Cache the executed definition per page to avoid repeated hydration.
waitDueAdmissionAttempts runs for every cell in the page. Each call loads the loop run, reads the definition snapshot JSON, and hydrates it through LoadExecutedDefinitionSnapshot. A page holds up to 1000 cells, and cells of one run share one definition_digest. The scan therefore repeats the same query and the same JSON decode many times on a scheduler tick.
Cache the resolved definition by definition_digest for the duration of one page, or resolve the admission attempts once per run.
♻️ Sketch of a per-page cache
runs := make([]taskpkg.Run, 0, len(cells))
+ resolvedCache := make(map[string]*looppkg.ResolvedDefinition, len(cells))
for _, cell := range cells {
- admissionAttempts, configErr := g.waitDueAdmissionAttempts(ctx, cell)
+ admissionAttempts, configErr := g.waitDueAdmissionAttempts(ctx, cell, resolvedCache)Inside waitDueAdmissionAttempts, look the digest up in the cache before calling loadWaitExecutedDefinition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/globaldb/global_db_loop_wait_due.go` around lines 58 - 80,
Cache the resolved executed definition for each page so waitDueAdmissionAttempts
does not repeatedly load and hydrate the same snapshot for cells sharing a
definition_digest. Add a page-scoped cache keyed by definition_digest, reuse
cached results before calling loadWaitExecutedDefinition, and preserve the
existing node lookup and lifecycle resolution behavior for each cell.
Comment on lines
+160
to
+180
| err := exec.QueryRowContext(ctx, `SELECT output.status, output.epoch, | ||
| COALESCE(control.paused, 0), COALESCE(control.quarantined, 0), | ||
| COALESCE(control.cancel_state, '') FROM loop_generation_outputs AS output | ||
| LEFT JOIN loop_node_controls AS control ON control.loop_run_id = output.loop_run_id | ||
| AND control.node_id = output.node_id WHERE output.loop_run_id = ? AND output.generation = ? | ||
| AND output.node_id = ? AND output.item_index = ?`, cell.runID, cell.generation, | ||
| cell.nodeID, cell.itemIndex).Scan(&status, &epoch, &paused, &quarantined, &cancelState) | ||
| if err != nil { | ||
| return fmt.Errorf("store: load due wait escalation cell: %w", err) | ||
| } | ||
| wantStatus := loopNodeOutputWaiting | ||
| if wait.Kind == looppkg.NodeWaitKindApprovalEscalation { | ||
| wantStatus = loopNodeOutputSucceeded | ||
| if cell.nodeID != wait.NodeID { | ||
| return fmt.Errorf("%w: approval wait identity changed", looppkg.ErrTransitionConflict) | ||
| } | ||
| } | ||
| if status != wantStatus || epoch != wait.IssuedEpoch || paused != 0 || quarantined != 0 || | ||
| strings.TrimSpace(cancelState) != "" { | ||
| return fmt.Errorf("%w: wait escalation cell changed", looppkg.ErrTransitionConflict) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Map a missing output row to a transition conflict.
validateDueWaitEscalationCell treats sql.ErrNoRows like any other database failure. The outer scan in listDueLoopWaitEscalations selects the cell through a join on loop_generation_outputs, so the row can disappear between the scan and this transaction. EscalateDueLoopWaitsPage only skips ErrTransitionConflict and ErrInvalidTransition. A single racing deletion therefore aborts the whole escalation page instead of skipping one cell.
Return looppkg.ErrTransitionConflict when the row is absent.
🐛 Proposed fix
cell.nodeID, cell.itemIndex).Scan(&status, &epoch, &paused, &quarantined, &cancelState)
+ if errors.Is(err, sql.ErrNoRows) {
+ return fmt.Errorf("%w: wait escalation cell is absent", looppkg.ErrTransitionConflict)
+ }
if err != nil {
return fmt.Errorf("store: load due wait escalation cell: %w", 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
| err := exec.QueryRowContext(ctx, `SELECT output.status, output.epoch, | |
| COALESCE(control.paused, 0), COALESCE(control.quarantined, 0), | |
| COALESCE(control.cancel_state, '') FROM loop_generation_outputs AS output | |
| LEFT JOIN loop_node_controls AS control ON control.loop_run_id = output.loop_run_id | |
| AND control.node_id = output.node_id WHERE output.loop_run_id = ? AND output.generation = ? | |
| AND output.node_id = ? AND output.item_index = ?`, cell.runID, cell.generation, | |
| cell.nodeID, cell.itemIndex).Scan(&status, &epoch, &paused, &quarantined, &cancelState) | |
| if err != nil { | |
| return fmt.Errorf("store: load due wait escalation cell: %w", err) | |
| } | |
| wantStatus := loopNodeOutputWaiting | |
| if wait.Kind == looppkg.NodeWaitKindApprovalEscalation { | |
| wantStatus = loopNodeOutputSucceeded | |
| if cell.nodeID != wait.NodeID { | |
| return fmt.Errorf("%w: approval wait identity changed", looppkg.ErrTransitionConflict) | |
| } | |
| } | |
| if status != wantStatus || epoch != wait.IssuedEpoch || paused != 0 || quarantined != 0 || | |
| strings.TrimSpace(cancelState) != "" { | |
| return fmt.Errorf("%w: wait escalation cell changed", looppkg.ErrTransitionConflict) | |
| } | |
| err := exec.QueryRowContext(ctx, `SELECT output.status, output.epoch, | |
| COALESCE(control.paused, 0), COALESCE(control.quarantined, 0), | |
| COALESCE(control.cancel_state, '') FROM loop_generation_outputs AS output | |
| LEFT JOIN loop_node_controls AS control ON control.loop_run_id = output.loop_run_id | |
| AND control.node_id = output.node_id WHERE output.loop_run_id = ? AND output.generation = ? | |
| AND output.node_id = ? AND output.item_index = ?`, cell.runID, cell.generation, | |
| cell.nodeID, cell.itemIndex).Scan(&status, &epoch, &paused, &quarantined, &cancelState) | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return fmt.Errorf("%w: wait escalation cell is absent", looppkg.ErrTransitionConflict) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("store: load due wait escalation cell: %w", err) | |
| } | |
| wantStatus := loopNodeOutputWaiting | |
| if wait.Kind == looppkg.NodeWaitKindApprovalEscalation { | |
| wantStatus = loopNodeOutputSucceeded | |
| if cell.nodeID != wait.NodeID { | |
| return fmt.Errorf("%w: approval wait identity changed", looppkg.ErrTransitionConflict) | |
| } | |
| } | |
| if status != wantStatus || epoch != wait.IssuedEpoch || paused != 0 || quarantined != 0 || | |
| strings.TrimSpace(cancelState) != "" { | |
| return fmt.Errorf("%w: wait escalation cell changed", looppkg.ErrTransitionConflict) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/globaldb/global_db_loop_wait_escalation_apply.go` around lines
160 - 180, Update validateDueWaitEscalationCell’s QueryRowContext scan handling
so sql.ErrNoRows is wrapped or returned as looppkg.ErrTransitionConflict, while
preserving the existing store error for other database failures. This allows
EscalateDueLoopWaitsPage to skip cells whose output row was deleted
concurrently.
Comment on lines
+12
to
+49
| func protectCanceledLoopFromStaleCoordinator( | ||
| ctx context.Context, | ||
| exec taskSQLExecutor, | ||
| completion *taskpkg.CoordinatorCompletion, | ||
| loopRunID string, | ||
| ) (bool, error) { | ||
| if completion == nil { | ||
| return false, fmt.Errorf("%w: coordinator completion is required", taskpkg.ErrValidation) | ||
| } | ||
| run, err := getLoopRunByIDWithExecutor(ctx, exec, looppkg.RunID(strings.TrimSpace(loopRunID))) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if !run.CancelRequested || completion.Plan.CancellationDrain { | ||
| return false, nil | ||
| } | ||
| plan := taskpkg.CoordinatorCompletionPlan{ | ||
| Snapshot: taskpkg.GenerationSnapshot{ | ||
| LoopRunID: string(run.ID), | ||
| Generation: max(1, run.Generation), | ||
| }, | ||
| Yield: true, | ||
| CancellationDrain: true, | ||
| } | ||
| if !run.Status.Terminal() { | ||
| plan.GenerationInFlight = true | ||
| plan.PostCommitWakes = []taskpkg.CoordinatorWakeSpec{{ | ||
| LoopRunID: string(run.ID), | ||
| IdempotencyKey: fmt.Sprintf( | ||
| "loop.coordinator.cancel.%s.%s", | ||
| run.ID, | ||
| strings.TrimSpace(completion.RunID), | ||
| ), | ||
| }} | ||
| } | ||
| completion.Plan = plan | ||
| return true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A 40 'func .*withTaskImmediateTransaction' internal/store/globaldbRepository: compozy/compozy
Length of output: 2670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== task transaction implementation =="
sed -n '1,80p' internal/store/globaldb/global_db_task_transaction.go
echo
echo "== task coordinator completion files outline/search relevant symbols =="
rg -n "CompleteCoordinatorAndEnqueueNext|completeCoordinatorAndEnqueueNextWithExecutor|protectCanceledLoopFromStaleCoordinator|finishCoordinatorRun|PlanSuperseded|withTaskImmediateTransaction|StoreWrite" internal/store/globaldb internal/task/manager_coordinator_run.go internal/store 2>/dev/null | head -200
echo
echo "== coordinator.go relevant sections =="
sed -n '1,150p' internal/store/globaldb/global_db_task_coordinator.go
sed -n '1,70p' internal/store/globaldb/global_db_task_coordinator_cancellation.go
echo
echo "== manager_coordinator_run.go relevant sections =="
sed -n '1,220p' internal/task/manager_coordinator_run.go
echo
echo "== global WriteTx transaction semantics =="
grep -R "func ExecuteWrite|type WriteTx|retry|transient|Backoff|time\.After|sleep" internal/store -n | head -200Repository: compozy/compozy
Length of output: 42083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate store/write transaction implementation =="
rg -n -A 80 '^func ExecuteWrite|type WriteTx|ExecuteImmediate|BeginImmediate|retry|Backoff|time\.After|sleep' internal/store -g '*.go' | head -300
echo
echo "== transaction helper implementations =="
sed -n '113,145p' internal/store/globaldb/tx_helpers.go
echo
echo "== CompleteCoordinatorAndEnqueueNext executor body relevant section =="
sed -n '62,230p' internal/store/globaldb/global_db_task_coordinator.go
echo
echo "== CoordinatorCompletion definition and result fields =="
rg -n "type CoordinatorCompletion|type CoordinatorCompletionResult|PlanSuperseded|CancellationDrain|PostCommitTimers|ParentCloses" internal/task -g '*.go' internal/store/globaldb -g '*.go' | head -220Repository: compozy/compozy
Length of output: 26617
Reset the mutated coordinator-completion pointer before retrying the transaction closure.
CompleteCoordinatorAndEnqueueNext reuses the same &normalized pointer in the transaction closure. protectCanceledLoopFromStaleCoordinator sets completion.Plan.CancellationDrain in place. Since executeWrite retries the callback for SQLITE_BUSY/SQLITE_LOCKED, the committed retry can see the stale mutation and set PlanSuperseded=false; finishCoordinatorRun then applies the original plan’s parent closes/timers on the canceled loop. Clone the completion/plan before passing it into the executor, or make the supervisor decision idempotent from run.CancelRequested without relying on the mutated Plan.
📍 Affects 2 files
internal/store/globaldb/global_db_task_coordinator_cancellation.go#L12-L49(this comment)internal/store/globaldb/global_db_task_coordinator.go#L18-L61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/globaldb/global_db_task_coordinator_cancellation.go` around
lines 12 - 49, Make the coordinator completion decision retry-safe across
protectCanceledLoopFromStaleCoordinator and CompleteCoordinatorAndEnqueueNext:
avoid reusing the same mutable completion/Plan pointer across executeWrite
retries by cloning the normalized completion (including its Plan) for each
transaction attempt, or otherwise derive the cancellation-supervisor decision
idempotently from run.CancelRequested. Update
internal/store/globaldb/global_db_task_coordinator_cancellation.go lines 12-49
and internal/store/globaldb/global_db_task_coordinator.go lines 18-61 so
finishCoordinatorRun cannot apply the original parent closes or timers after
cancellation.
Comment on lines
+24
to
+46
| for _, raw := range plan.ParentCloses { | ||
| spec := raw.Normalize() | ||
| child, err := getLoopRunByIDWithExecutor(ctx, exec, looppkg.RunID(spec.ChildLoopRunID)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if child.ParentLoopRunID != parentRunID || spec.ParentLoopRunID != string(parentRunID) { | ||
| return fmt.Errorf( | ||
| "%w: child Loop run %q is not owned by parent %q", | ||
| looppkg.ErrTransitionConflict, | ||
| child.ID, | ||
| parentRunID, | ||
| ) | ||
| } | ||
| if spec.ParentStatus != strings.TrimSpace(plan.Terminal.Status) { | ||
| return fmt.Errorf( | ||
| "%w: parent-close status %q does not match terminal %q", | ||
| taskpkg.ErrValidation, | ||
| spec.ParentStatus, | ||
| plan.Terminal.Status, | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate spec.Policy against the closed policy set.
This function is the store-side guard for parent-close actions. It checks ownership and parent status but never checks the policy value. An empty or unknown policy passes validation and reaches the mutation path, where the effect on the child run is undefined. Reject any policy outside the authored set. Decide explicitly whether abandon is accepted here; the coordinator filters it out before building intents, so accepting it at the store level would create two different meanings for the same field.
🛡️ Proposed guard
for _, raw := range plan.ParentCloses {
spec := raw.Normalize()
+ switch spec.Policy {
+ case string(dsl.ParentCloseTerminate), string(dsl.ParentCloseCancel):
+ default:
+ return fmt.Errorf(
+ "%w: parent-close policy %q is not supported",
+ taskpkg.ErrValidation,
+ spec.Policy,
+ )
+ }
child, err := getLoopRunByIDWithExecutor(ctx, exec, looppkg.RunID(spec.ChildLoopRunID))Add the dsl import, or compare against the policy constants that Normalize uses.
📝 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
| for _, raw := range plan.ParentCloses { | |
| spec := raw.Normalize() | |
| child, err := getLoopRunByIDWithExecutor(ctx, exec, looppkg.RunID(spec.ChildLoopRunID)) | |
| if err != nil { | |
| return err | |
| } | |
| if child.ParentLoopRunID != parentRunID || spec.ParentLoopRunID != string(parentRunID) { | |
| return fmt.Errorf( | |
| "%w: child Loop run %q is not owned by parent %q", | |
| looppkg.ErrTransitionConflict, | |
| child.ID, | |
| parentRunID, | |
| ) | |
| } | |
| if spec.ParentStatus != strings.TrimSpace(plan.Terminal.Status) { | |
| return fmt.Errorf( | |
| "%w: parent-close status %q does not match terminal %q", | |
| taskpkg.ErrValidation, | |
| spec.ParentStatus, | |
| plan.Terminal.Status, | |
| ) | |
| } | |
| } | |
| for _, raw := range plan.ParentCloses { | |
| spec := raw.Normalize() | |
| switch spec.Policy { | |
| case string(dsl.ParentCloseTerminate), string(dsl.ParentCloseCancel): | |
| default: | |
| return fmt.Errorf( | |
| "%w: parent-close policy %q is not supported", | |
| taskpkg.ErrValidation, | |
| spec.Policy, | |
| ) | |
| } | |
| child, err := getLoopRunByIDWithExecutor(ctx, exec, looppkg.RunID(spec.ChildLoopRunID)) | |
| if err != nil { | |
| return err | |
| } | |
| if child.ParentLoopRunID != parentRunID || spec.ParentLoopRunID != string(parentRunID) { | |
| return fmt.Errorf( | |
| "%w: child Loop run %q is not owned by parent %q", | |
| looppkg.ErrTransitionConflict, | |
| child.ID, | |
| parentRunID, | |
| ) | |
| } | |
| if spec.ParentStatus != strings.TrimSpace(plan.Terminal.Status) { | |
| return fmt.Errorf( | |
| "%w: parent-close status %q does not match terminal %q", | |
| taskpkg.ErrValidation, | |
| spec.ParentStatus, | |
| plan.Terminal.Status, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/globaldb/global_db_task_coordinator_parent_close.go` around
lines 24 - 46, In the parent-close validation loop, add validation for
spec.Policy against the authored closed-policy set using the existing DSL policy
constants or Normalize-related definitions. Reject empty and unknown policies
before mutation, and explicitly exclude abandon to match the coordinator’s
filtering behavior. Return the established validation error type consistently
with the existing parent-status check.
* feat: Schema, classification, DSL grammar and config contracts #1 Checkpoint via cy-loop-tasks (iteration 2, phase B mode=tasks). Co-Authored-By: Codex <[email protected]> * feat: implement loop node failure precedence * feat: Effects: outbox, relay and delivery identity #3 Checkpoint via cy-loop-tasks (iteration 4, phase B mode=tasks). Co-Authored-By: Codex <[email protected]> * feat: Target health breaker, quarantine and requeue #4 Checkpoint via cy-loop-tasks (iteration 5, phase B mode=tasks). Co-Authored-By: Codex <[email protected]> * fix: resolve CodeRabbit lifecycle review * fix: address incremental CodeRabbit feedback --------- Co-authored-by: Codex <[email protected]>
Checkpoint via cy-loop-tasks (iteration 7, phase B mode=tasks). Co-Authored-By: Codex <[email protected]>
Checkpoint via cy-loop-tasks (iteration 9, phase B mode=tasks). Co-Authored-By: Codex <[email protected]>
Checkpoint via cy-loop-tasks (iteration 10, phase B mode=tasks). Co-Authored-By: Codex <[email protected]>
pedronauck
force-pushed
the
stack/loop-lifecycle-v2-02-controls-surfaces
branch
from
August 3, 2026 23:58
8b9b224 to
05fd58d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/loop/coordinator_lifecycle_test.go (1)
917-1030: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSplit this test into
t.Run("Should …")subtests.
TestCoordinatorQuarantineShouldPreserveDiagnosableSanitizedHistoryputs its assertions directly in the test body. It also covers two independent scenarios: sanitized appended history at lines 922-991, and bounded encoding of an oversized entry at lines 993-1029. If the first scenario fails, the second never runs.Wrap each scenario in its own
t.Run("Should …", ...)subtest witht.Parallel(), as every other test in this file does.As per path instructions: "MUST use t.Run("Should...") pattern for ALL test cases". As per coding guidelines: "use
t.Run("Should …"), defaultt.Parallel".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/coordinator_lifecycle_test.go` around lines 917 - 1030, Split TestCoordinatorQuarantineShouldPreserveDiagnosableSanitizedHistory into two t.Run("Should …", ...) subtests: one for sanitized appended history and one for bounded oversized encoding. Move each scenario’s setup and assertions into its subtest and call t.Parallel() within both subtests, while preserving the existing test behavior and assertions.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (2)
internal/cli/client_loops.go (1)
318-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider folding
loopRunActionintoloopRunLifecycleAction.
loopRunActionat lines 306-316 builds the same path and issues the same request. Only the decoded response differs. Keeping both lets the path construction diverge later.loopRunActioncan callloopRunLifecycleActionand discard the response.♻️ Proposed refactor
func (c *unixSocketClient) loopRunAction( ctx context.Context, workspaceID string, runID string, action string, request any, credentials agentidentity.Credentials, ) error { - path := loopRunPath(workspaceID, runID) + "/" + strings.TrimSpace(action) - return c.doAgentJSON(ctx, http.MethodPost, path, nil, request, credentials, nil) + _, err := c.loopRunLifecycleAction(ctx, workspaceID, runID, action, request, credentials) + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/client_loops.go` around lines 318 - 332, Refactor loopRunAction to delegate to loopRunLifecycleAction, reusing its path construction and POST request logic while discarding the returned LoopMutationResponse. Preserve loopRunAction’s existing error propagation and public behavior, and leave loopRunLifecycleAction as the single implementation for this request flow.internal/loop/service_test.go (1)
2169-2182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that a terminal run performs no cancellation delivery.
This subtest confirms the status stays
done. It does not confirm that the service skipped delivery and state advancement. A regression that still delivers cancellation to a finished run would pass. Add an assertion onstore.cancellationStates.♻️ Proposed additional assertion
if got := store.mustRun(t, run.ID).Status; got != loop.StatusDone { t.Fatalf("stored status = %q, want done", got) } + if len(store.cancellationStates) != 0 { + t.Fatalf("cancellation states = %#v, want no delivery on a terminal run", store.cancellationStates) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/loop/service_test.go` around lines 2169 - 2182, Extend the “Should preserve a terminal completion that wins the race” subtest to assert that no cancellation delivery or state advancement occurred by checking store.cancellationStates. Keep the existing StatusDone assertion and verify the cancellation state remains absent or unchanged for the completed run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/loop/coordinator_lifecycle_test.go`:
- Around line 671-675: Fix the NodeAttempt value in the lifecycle test by
replacing the invalid new(now) assignment for EndedAt with an addressable
pointer to the existing now time.Time variable, while leaving the other attempt
fields unchanged.
---
Outside diff comments:
In `@internal/loop/coordinator_lifecycle_test.go`:
- Around line 917-1030: Split
TestCoordinatorQuarantineShouldPreserveDiagnosableSanitizedHistory into two
t.Run("Should …", ...) subtests: one for sanitized appended history and one for
bounded oversized encoding. Move each scenario’s setup and assertions into its
subtest and call t.Parallel() within both subtests, while preserving the
existing test behavior and assertions.
---
Nitpick comments:
In `@internal/cli/client_loops.go`:
- Around line 318-332: Refactor loopRunAction to delegate to
loopRunLifecycleAction, reusing its path construction and POST request logic
while discarding the returned LoopMutationResponse. Preserve loopRunAction’s
existing error propagation and public behavior, and leave loopRunLifecycleAction
as the single implementation for this request flow.
In `@internal/loop/service_test.go`:
- Around line 2169-2182: Extend the “Should preserve a terminal completion that
wins the race” subtest to assert that no cancellation delivery or state
advancement occurred by checking store.cancellationStates. Keep the existing
StatusDone assertion and verify the cancellation state remains absent or
unchanged for the completed run.
🪄 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 Plus
Run ID: ac172400-a546-4362-901a-ded129c250bd
⛔ Files ignored due to path filters (26)
docs/qa/bugs/BUG-20260802-initial-wait-fails-run.mdis excluded by!**/*.mddocs/qa/bugs/BUG-20260802-node-kill-leaves-run-live.mdis excluded by!**/*.mddocs/qa/bugs/BUG-20260802-parked-node-cancel-stalls.mdis excluded by!**/*.mddocs/qa/charters/CH-003.mdis excluded by!**/*.mddocs/qa/charters/CH-agent-loop-lifecycle-native.mdis excluded by!**/*.mddocs/qa/charters/CH-loop-death-resume.mdis excluded by!**/*.mddocs/qa/charters/CH-loop-run-cancel-control.mdis excluded by!**/*.mddocs/qa/charters/CH-runaway-work-bounded.mdis excluded by!**/*.mddocs/qa/journeys/J-04-operator-pause-resume.mdis excluded by!**/*.mddocs/qa/journeys/J-07-agent-operated-run.mdis excluded by!**/*.mddocs/qa/journeys/J-bound-runaway-work.mdis excluded by!**/*.mddocs/qa/journeys/J-resume-dead-loop-node.mdis excluded by!**/*.mddocs/qa/reports/2026-08-02-loop-node-lifecycle-task07.mdis excluded by!**/*.mddocs/qa/scenarios/LP-016.mdis excluded by!**/*.mddocs/qa/scenarios/LP-agent-operates-lifecycle-via-native-tools.mdis excluded by!**/*.mddocs/qa/scenarios/LP-approval-link-journey.mdis excluded by!**/*.mddocs/qa/scenarios/LP-cancel-vs-kill.mdis excluded by!**/*.mddocs/qa/scenarios/LP-crash-death-resume.mdis excluded by!**/*.mddocs/qa/scenarios/LP-days-long-node-no-clock.mdis excluded by!**/*.mddocs/qa/scenarios/LP-duplicate-event-suppressed.mdis excluded by!**/*.mddocs/qa/scenarios/LP-durable-wait-restart.mdis excluded by!**/*.mddocs/qa/scenarios/LP-live-pause-repair-resume.mdis excluded by!**/*.mddocs/qa/scenarios/LP-waiting-inventory-escalation.mdis excluded by!**/*.mddocs/qa/scenarios/TA-076.mdis excluded by!**/*.mddocs/qa/scenarios/TA-084.mdis excluded by!**/*.mddocs/qa/scenarios/TA-action-run-liveness.mdis excluded by!**/*.md
📒 Files selected for processing (155)
internal/api/contract/generic_error_payload.gointernal/api/contract/loop_enums.gointernal/api/contract/loop_nodes.gointernal/api/contract/loop_runs.gointernal/api/contract/loops.gointernal/api/core/errors_diagnostic_payload.gointernal/api/core/errors_test.gointernal/api/core/loop_interfaces.gointernal/api/core/loops.gointernal/api/core/loops_nodes.gointernal/api/core/loops_test.gointernal/api/httpapi/handlers_test.gointernal/api/httpapi/loops_routes.gointernal/api/spec/loops.gointernal/api/spec/loops_nodes.gointernal/api/spec/loops_test.gointernal/api/spec/schema_enum_registry.gointernal/api/udsapi/handlers_test.gointernal/api/udsapi/loops_routes.gointernal/automation/dispatch_loop.gointernal/cli/client_loops.gointernal/cli/helpers_test.gointernal/cli/loop.gointernal/cli/loop_client.gointernal/cli/loop_inspect_output.gointernal/cli/loop_node.gointernal/cli/loop_nodes.gointernal/cli/loop_runs.gointernal/cli/loop_support.gointernal/cli/loop_test.gointernal/cli/marketplace.gointernal/config/merge_task.gointernal/config/task_orchestration.gointernal/config/task_orchestration_test.gointernal/config/task_orchestration_validation.gointernal/config/tool_surface_task.gointernal/config/tool_surface_test.gointernal/daemon/automation_loop_starter.gointernal/daemon/loop_action_checkpoint.gointernal/daemon/loop_action_death_resume.gointernal/daemon/loop_action_liveness.gointernal/daemon/loop_action_liveness_integration_helpers_test.gointernal/daemon/loop_action_runtime.gointernal/daemon/loop_api_contract.gointernal/daemon/loop_api_nodes.gointernal/daemon/loop_api_payloads_test.gointernal/daemon/loop_api_run_history.gointernal/daemon/loop_api_runs.gointernal/daemon/loop_api_runs_test.gointernal/daemon/loop_api_service.gointernal/daemon/loop_goal_managed_runtime_integration_test.gointernal/daemon/loop_hook_observer.gointernal/daemon/loop_hook_observer_test.gointernal/daemon/loop_managed_binding.gointernal/daemon/loop_parent_close_postcommit.gointernal/daemon/loop_watch_events_doorbell.gointernal/daemon/loop_watch_events_observer.gointernal/daemon/loop_watch_events_observer_index.gointernal/daemon/loop_watch_events_observer_test.gointernal/daemon/loop_watch_events_payloads.gointernal/daemon/native_hook_helpers.gointernal/daemon/native_loop_lifecycle_support.gointernal/daemon/native_loop_lifecycle_tools.gointernal/daemon/native_loop_tools.gointernal/daemon/native_loop_tools_support.gointernal/daemon/native_loop_tools_test.gointernal/daemon/scheduler_loop_coordinator.gointernal/daemon/scheduler_loop_due_scan.gointernal/daemon/scheduler_runtime.gointernal/daemon/scheduler_task_source_types.gointernal/daemon/task_runtime_boot.gointernal/daemon/task_runtime_test.gointernal/extension/scaffold_templates/loop-watch-source-go/main.gointernal/extension/watch_source.gointernal/extension/watch_source_test.gointernal/hooks/payloads_task_loop.gointernal/loop/action_failure.gointernal/loop/action_retry_feedback.gointernal/loop/action_runagent.gointernal/loop/action_schema.gointernal/loop/action_test.gointernal/loop/action_types.gointernal/loop/action_usage.gointernal/loop/admission_claim.gointernal/loop/cancel_control.gointernal/loop/compiler.gointernal/loop/control_eval_context.gointernal/loop/control_plan.gointernal/loop/control_plan_branch.gointernal/loop/control_plan_finish.gointernal/loop/control_topology.gointernal/loop/coordinator.gointernal/loop/coordinator_action.gointernal/loop/coordinator_action_liveness.gointernal/loop/coordinator_action_metadata.gointernal/loop/coordinator_autopause.gointernal/loop/coordinator_cancellation.gointernal/loop/coordinator_control_test.gointernal/loop/coordinator_effects.gointernal/loop/coordinator_error_routes.gointernal/loop/coordinator_gate_wait.gointernal/loop/coordinator_generation.gointernal/loop/coordinator_lifecycle.gointernal/loop/coordinator_lifecycle_test.gointernal/loop/coordinator_metadata.gointernal/loop/coordinator_outputs.gointernal/loop/coordinator_pause.gointernal/loop/coordinator_quarantine.gointernal/loop/coordinator_test.gointernal/loop/coordinator_wait.gointernal/loop/coordinator_wait_route.gointernal/loop/coordinator_watch_test.gointernal/loop/death_resume.gointernal/loop/dsl/graph.gointernal/loop/effects_dispatch.gointernal/loop/effects_dispatch_test.gointernal/loop/errors.gointernal/loop/executed_definition_validation.gointernal/loop/executed_definition_watch.gointernal/loop/generation_snapshot.gointernal/loop/generation_snapshot_controls.gointernal/loop/generation_snapshot_intents.gointernal/loop/goal_cancellation.gointernal/loop/linter.gointernal/loop/linter_references.gointernal/loop/linter_test.gointernal/loop/linter_wait.gointernal/loop/node_control_mutation.gointernal/loop/node_inventory.gointernal/loop/node_liveness.gointernal/loop/node_pause.gointernal/loop/node_wait.gointernal/loop/node_waits.gointernal/loop/parent_close.gointernal/loop/run_network_state.gointernal/loop/service.gointernal/loop/service_cancellation.gointernal/loop/service_control.gointernal/loop/service_node_pause.gointernal/loop/service_node_requeue.gointernal/loop/service_options.gointernal/loop/service_start.gointernal/loop/service_test.gointernal/loop/service_types.gointernal/loop/wait_due.gointernal/loop/watch/adapter.gointernal/loop/watch/adapter_test.gointernal/loop/watch/event_key.gointernal/loop/watch/output.gointernal/loop/watch/types.gointernal/loop/watch_events_constraints.gointernal/loop/watch_events_contract.gointernal/loop/watch_events_ledger.gointernal/session/manager_prompt_submit.gointernal/session/manager_test.go
💤 Files with no reviewable changes (7)
- internal/config/tool_surface_task.go
- internal/config/task_orchestration_test.go
- internal/config/task_orchestration_validation.go
- internal/api/core/loops.go
- internal/config/merge_task.go
- internal/loop/service_control.go
- internal/config/task_orchestration.go
🚧 Files skipped from review as they are similar to previous changes (143)
- internal/loop/coordinator_quarantine.go
- internal/daemon/loop_watch_events_doorbell.go
- internal/session/manager_test.go
- internal/session/manager_prompt_submit.go
- internal/daemon/native_hook_helpers.go
- internal/loop/watch/types.go
- internal/cli/marketplace.go
- internal/loop/linter.go
- internal/daemon/loop_api_runs_test.go
- internal/loop/watch/event_key.go
- internal/loop/linter_references.go
- internal/loop/action_retry_feedback.go
- internal/loop/control_topology.go
- internal/config/tool_surface_test.go
- internal/loop/node_wait.go
- internal/daemon/loop_managed_binding.go
- internal/hooks/payloads_task_loop.go
- internal/loop/coordinator_error_routes.go
- internal/api/core/errors_test.go
- internal/daemon/loop_hook_observer.go
- internal/loop/executed_definition_validation.go
- internal/loop/effects_dispatch.go
- internal/daemon/scheduler_task_source_types.go
- internal/daemon/loop_action_checkpoint.go
- internal/loop/action_types.go
- internal/loop/coordinator_cancellation.go
- internal/api/spec/loops_nodes.go
- internal/loop/watch_events_contract.go
- internal/loop/control_plan_finish.go
- internal/loop/coordinator_effects.go
- internal/daemon/loop_watch_events_payloads.go
- internal/extension/scaffold_templates/loop-watch-source-go/main.go
- internal/cli/loop_nodes.go
- internal/loop/effects_dispatch_test.go
- internal/loop/watch/output.go
- internal/loop/watch_events_constraints.go
- internal/loop/coordinator_action_metadata.go
- internal/loop/action_usage.go
- internal/loop/action_test.go
- internal/daemon/loop_watch_events_observer_index.go
- internal/cli/loop_support.go
- internal/loop/watch/adapter.go
- internal/cli/loop_test.go
- internal/daemon/loop_action_death_resume.go
- internal/loop/coordinator_pause.go
- internal/loop/service_options.go
- internal/cli/loop.go
- internal/loop/action_runagent.go
- internal/loop/errors.go
- internal/daemon/scheduler_runtime.go
- internal/loop/linter_test.go
- internal/loop/service_start.go
- internal/api/core/errors_diagnostic_payload.go
- internal/loop/linter_wait.go
- internal/loop/coordinator_autopause.go
- internal/api/contract/generic_error_payload.go
- internal/extension/watch_source.go
- internal/api/httpapi/handlers_test.go
- internal/daemon/loop_api_run_history.go
- internal/loop/coordinator_gate_wait.go
- internal/api/spec/loops_test.go
- internal/loop/generation_snapshot.go
- internal/daemon/loop_api_payloads_test.go
- internal/loop/control_eval_context.go
- internal/api/udsapi/loops_routes.go
- internal/loop/run_network_state.go
- internal/daemon/loop_goal_managed_runtime_integration_test.go
- internal/cli/loop_node.go
- internal/daemon/loop_api_runs.go
- internal/api/contract/loop_nodes.go
- internal/daemon/loop_hook_observer_test.go
- internal/daemon/native_loop_lifecycle_support.go
- internal/loop/coordinator_metadata.go
- internal/api/udsapi/handlers_test.go
- internal/cli/loop_inspect_output.go
- internal/loop/control_plan.go
- internal/loop/generation_snapshot_intents.go
- internal/loop/watch/adapter_test.go
- internal/loop/service_node_requeue.go
- internal/api/core/loops_test.go
- internal/cli/loop_client.go
- internal/daemon/loop_action_runtime.go
- internal/daemon/loop_api_service.go
- internal/extension/watch_source_test.go
- internal/loop/coordinator.go
- internal/loop/watch_events_ledger.go
- internal/daemon/loop_action_liveness_integration_helpers_test.go
- internal/daemon/loop_watch_events_observer.go
- internal/loop/cancel_control.go
- internal/loop/service_node_pause.go
- internal/daemon/task_runtime_boot.go
- internal/daemon/scheduler_loop_coordinator.go
- internal/loop/dsl/graph.go
- internal/loop/wait_due.go
- internal/loop/node_liveness.go
- internal/loop/node_control_mutation.go
- internal/loop/control_plan_branch.go
- internal/loop/node_pause.go
- internal/loop/coordinator_outputs.go
- internal/api/httpapi/loops_routes.go
- internal/daemon/loop_watch_events_observer_test.go
- internal/loop/action_schema.go
- internal/loop/coordinator_wait.go
- internal/loop/coordinator_test.go
- internal/daemon/native_loop_tools.go
- internal/loop/coordinator_wait_route.go
- internal/api/spec/schema_enum_registry.go
- internal/api/core/loops_nodes.go
- internal/daemon/native_loop_lifecycle_tools.go
- internal/loop/compiler.go
- internal/cli/helpers_test.go
- internal/loop/node_inventory.go
- internal/daemon/native_loop_tools_support.go
- internal/loop/coordinator_watch_test.go
- internal/loop/coordinator_action_liveness.go
- internal/loop/service_cancellation.go
- internal/loop/coordinator_control_test.go
- internal/loop/admission_claim.go
- internal/loop/coordinator_action.go
- internal/daemon/loop_api_contract.go
- internal/daemon/native_loop_tools_test.go
- internal/api/contract/loop_runs.go
- internal/daemon/loop_api_nodes.go
- internal/loop/death_resume.go
- internal/loop/generation_snapshot_controls.go
- internal/loop/goal_cancellation.go
- internal/loop/service.go
- internal/loop/parent_close.go
- internal/loop/coordinator_generation.go
- internal/daemon/loop_parent_close_postcommit.go
- internal/loop/executed_definition_watch.go
- internal/api/spec/loops.go
- internal/cli/loop_runs.go
- internal/daemon/automation_loop_starter.go
- internal/api/contract/loop_enums.go
- internal/loop/coordinator_lifecycle.go
- internal/loop/node_waits.go
- internal/daemon/loop_action_liveness.go
- internal/loop/action_failure.go
- internal/api/core/loop_interfaces.go
- internal/automation/dispatch_loop.go
- internal/daemon/scheduler_loop_due_scan.go
- internal/loop/service_types.go
Comment on lines
+671
to
+675
| attempts: []NodeAttempt{{ | ||
| LoopRunID: loopRun.ID, Generation: 1, NodeID: "work", Attempt: 1, | ||
| FailureClass: &failureClass, Disposition: AttemptRetried, | ||
| StartedAt: now.Add(-time.Minute), EndedAt: new(now), NextAttemptAt: &nextAttemptAt, | ||
| }}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the loop package shadows the `new` builtin, and inspect the exact line.
set -euo pipefail
echo "== package-level identifiers named new =="
ast-grep run --pattern 'func new($$$) $_ { $$$ }' --lang go internal/loop || true
rg -n --type=go '^(var|func|type)\s+new\b' internal/loop || echo "no package-level 'new' declaration found"
echo "== the reported line =="
rg -n 'new\(now\)' internal/loopRepository: compozy/compozy
Length of output: 396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scoped `new` binding searches in internal/loop =="
rg -n --type=go '\bnew\s*\(' internal/loop || true
rg -n --type=go '^\s*(const|var|func|type)\s+new\b' internal/loop || true
echo "== imports and package declarations around coordinator_lifecycle_test.go =="
sed -n '1,80p' internal/loop/coordinator_lifecycle_test.go
echo "== nearby local shadow candidates in current function scope =="
sed -n '640,690p' internal/loop/coordinator_lifecycle_test.goRepository: compozy/compozy
Length of output: 14341
Use an addressable time.Time value before assigning EndedAt.
new(now) passes the time.Time variable now as if it were a type, so the compiler rejects this call. EndedAt is a pointer field; use EndedAt: &now in this value literal now that the value does not need to remain mutable after the call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/loop/coordinator_lifecycle_test.go` around lines 671 - 675, Fix the
NodeAttempt value in the lifecycle test by replacing the invalid new(now)
assignment for EndedAt with an addressable pointer to the existing now time.Time
variable, while leaving the other attempt fields unchanged.
Member
Author
|
Superseded by the single final Loop node lifecycle PR now being assembled from the complete three-stack implementation. The remediation commit and review work from this PR are already included in the final branch. |
This was referenced Aug 6, 2026
Merged
4 of 5 tasks
2 of 3 tasks
This was referenced Aug 15, 2026
This branch was successfully deployed
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.
Stack context
This is 2/3 of native GitHub stack #303 and depends on #300.
mainReview and merge from the bottom upward. This stack replaces the oversized #294 review surface.
Summary
This layer turns the runtime foundations from #300 into a complete operator and agent control plane. It adds long-running supervision, cooperative cancel versus immediate kill, pause/resume and durable waits, admission deduplication, then co-ships the public contract through HTTP, UDS, CLI, native tools, OpenAPI, and generated TypeScript types.
The key outcome is parity: no lifecycle capability is trapped behind an internal Go call or Web-only control.
What changes
Long-running supervision
canceledand livewatchingrun states without coercing them tofailedorrunning.Cancel is not kill
cancelis cooperative: the request becomes durable, delivery is observable, and the node/run drains tocanceled(operator_cancel).killimmediately stops the bound session and ends atcanceled(operator_kill).stopverb, routes, schemas, and transition cause are removed as a hard cut.Pause, resume, waits, and admission
Public and agent-manageable surfaces
compozy loopCLI commands with structured output and the same validation/error model.Public contract notes
queued,running,watching,needs-approval,paused,done,no-op,blocked,failed,exhausted,stalled, andcanceled.stopoperations fail rather than silently aliasing to cancel or kill.Verification
internal/tools/builtinpassed its race-enabled canonical catalog suite: 270 tests, including new tool IDs, output schemas, risk, digest, and capability assertions.make gate-fullis intentionally deferred until after CodeRabbit remediation. No final green-gate claim is made here.Compozy Impact Audit
Reviewer guide
Suggested order: liveness state machine → cancel/kill race rules → pause/wait persistence → admission dedupe → API contract → CLI/native parity. Please focus on whether every public surface reaches the same validator/writer and whether conflict responses remain deterministic after a fresh read.
Stack created with the official GitHub Stacks CLI.
Summary by CodeRabbit