Repository navigation
feat(dev-loop): abandon a DevLoopWorkflow over REST and MCP - #450
Conversation
A DevLoop whose proposal is deliberately never approved stays Running at
the approval park, and the only way to end it was the mctl-agents
Temporal CLI, which needs cluster access. Two controlled-test loops and
seerrsense-73 are parked that way now.
POST /api/v1/agents/dev-loop/{workflow_id}/abandon and the
mctl_abandon_dev_loop tool send the workflow's existing `abandon` signal
(mctl-agents#420) rather than a Temporal terminate, so the workflow
still releases its lifecycle-ownership row. Authorization matches
approve: admin-only, and the caller recorded as abandoned_by comes from
authentication, never the body.
The execution is described before signalling so every answer is an
observation: 404 for no execution, the final status for one already
finished (a retry is harmless), and 502 for any Temporal error or a
status that could not be determined. A signal that races the execution
closing is answered from a second describe, not assumed.
Closes #392
Claude-Session: https://claude.ai/code/session_01QfTaaTnKZbCFJbu841N6j7
|
Claude finished @mashkovd's task in 3m 58s —— View job Review complete — changes requested
0 P1 · 2 P2 · 2 P3 — review submitted, bound to P2 findings1. The describe → signal pair is not pinned to a run id —
Narrow, but it is the one race the handler cannot detect afterwards; every other one here is answered by a second observation. Threading the observed run id through 2. The endpoint is missing from the served OpenAPI spec —
P3 nits (non-blocking)
What looks right
Not verifiedTests were not executed — |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| r.Post("/roadmap/waves/execute", h.ExecuteRoadmapWave) | ||
| r.Post("/agents/dev-loop/{workflow_id}/approve", h.ApproveDevLoopWorkflow) | ||
| // Ends a DevLoop gracefully (mctl-api#392). | ||
| r.Post("/agents/dev-loop/{workflow_id}/abandon", h.AbandonDevLoopWorkflow) |
There was a problem hiding this comment.
P2 — the new route is not in internal/openapi/openapi.yaml, which is embedded and served at /openapi.yaml as the published contract. Every sibling dev-loop route is documented there: /api/v1/agents/dev-loop/start (line 2323), /api/v1/agents/dev-loop/{workflow_id} (2492) and /api/v1/agents/dev-loop/{workflow_id}/approve (2549). internal/openapi/embed_test.go only pins a hardcoded work-items subset, so nothing fails — the gap is silent.
Worth adding a /api/v1/agents/dev-loop/{workflow_id}/abandon entry with the required reason body (additionalProperties: false, matching DisallowUnknownFields) and the 200 / 400 / 401 / 403 / 404 / 502 / 503 answers this handler actually produces. The outcome enum (signalled | already_finished) is the part a client most needs from the spec.
There was a problem hiding this comment.
Fixed in 238cdad. POST /api/v1/agents/dev-loop/{workflow_id}/abandon is now in internal/openapi/openapi.yaml, directly after /approve. The request body requires reason (minLength: 1) and sets additionalProperties: false. The 200 response lists workflow_id, run_id, status, outcome (enum signalled | already_finished), signalled (present only when signalled) and message. The other responses are 400, 401, 403, 404, 409 (the run was replaced before the signal; see the run-pinning thread), 502 and 503. TestSpecParses in embed_test.go now pins both the /abandon and /approve dev-loop routes.
| } | ||
|
|
||
| payload := map[string]string{"reason": reason, "abandoned_by": user.ID} | ||
| if err := h.opts.TemporalClient.SignalAbandon(r.Context(), workflowID, payload); err != nil { |
There was a problem hiding this comment.
P2 — the describe and the signal are not pinned to the same run, so the one gap the fail-closed design does not close is the one where it matters.
DescribeDevLoopExecution resolves the latest run (DescribeWorkflowExecution(ctx, workflowID, "")), and SignalAbandon likewise passes "" for the run id. Between line 513 and line 523 the observed run can close and StartDevLoopWorkflow can open a new one under the same id — that is exactly what USE_EXISTING/REJECT_DUPLICATE permit once the previous run is closed. The signal then lands on the new loop, and the 200 at line 548 reports run_id: exec.RunID from the stale describe, so the audit trail and the response both name a run that was never signalled.
Narrow, but it is the one case the handler cannot detect afterwards: every other race here is answered by a second observation, and this one silently succeeds against the wrong execution. Threading the observed run id through closes it exactly:
func (c *Client) SignalAbandon(ctx context.Context, workflowID, runID string, payload map[string]string) error {
if err := c.temporal.SignalWorkflow(ctx, workflowID, runID, AbandonSignalName, payload); err != nil {called as SignalAbandon(r.Context(), workflowID, exec.RunID, payload). Temporal then answers NotFound when that specific run is no longer running, which routes into the second-describe branch already written below — no new branch needed, and the existing NotFound tests keep their meaning.
There was a problem hiding this comment.
Fixed in 238cdad. SignalAbandon now takes a run id, and the handler passes exec.RunID from the describe, so the call is SignalWorkflow(ctx, workflowID, runID, ...). A run that closed after the describe now answers NotFound, even if a new run has already taken the id, and goes into the second-describe branch. That branch now also checks the run id. If the second describe finds a different run still Running, the handler returns 409 and does not abandon that run; if it finds the same run still Running, it returns 502. Tests: TestSignalAbandon_SignalsAbandonOnTheGivenRun checks that the run id reaches SignalWorkflow. In TestAbandonDevLoopWorkflow_SignalNotFoundIsAnsweredFromASecondDescribe, every case asserts that the signal carried run-1 and that a NotFound is never reported as signalled; a new replaced by a new run case expects the 409. I also mutation-checked it: passing "" as the run id fails both handler tests.
| if exec == nil { | ||
| return | ||
| } | ||
| if exec.Status != "Running" { |
There was a problem hiding this comment.
P3 — "Running" is now a bare literal in four places in this file (lines 126, 517, 534, 620) while its sibling value has a named constant (devLoopStatusUnknown, handlers_lifecycle_conflict.go:201). Since this handler branches on the literal twice and treats everything else as terminal, a devLoopStatusRunning = "Running" next to devLoopStatusUnknown would make the pair read as one vocabulary. Pre-existing pattern, not introduced here.
There was a problem hiding this comment.
Fixed in 238cdad. I added devLoopStatusRunning = "Running" next to devLoopStatusUnknown in handlers_lifecycle_conflict.go. Every bare "Running" in handlers_dev_loop.go now uses it, including the pre-existing ones in classifyDevLoop and GetDevLoopWorkflow.
| } | ||
| // Temporal answers NotFound for an execution that completed after | ||
| // the describe above. Look again instead of guessing. | ||
| after := describe() |
There was a problem hiding this comment.
P3 — this second describe inherits the IsNotFound branch, so a NotFound here answers 404 with the audit message "workflow not found". That reads as "no such loop", but the handler just described this execution successfully one call earlier; the only way to reach it is retention expiry mid-request. Not wrong as an HTTP code, but the audit line will mislead whoever reads it back. A distinct message for the post-signal look (e.g. "execution vanished between describe and signal") would cost one argument and keep the audit trail honest about what was observed.
There was a problem hiding this comment.
Fixed in 238cdad. describe now takes the audit message to use for NotFound. The first look still logs workflow not found. The look after a NotFound signal logs execution vanished between describe and signal, and the HTTP answer stays 404. The gone entirely case asserts that exact audit message, and I mutation-checked it.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 2 P3.
P2 — the describe/signal pair is not pinned to a run id: both resolve the latest run, so a run that closes and is restarted in the window between them gets signalled while the response and audit entry report the stale run_id. It is the one race the fail-closed design leaves undetectable; threading exec.RunID into SignalAbandon closes it and routes through the existing NotFound branch.
P2 — the new endpoint is absent from internal/openapi/openapi.yaml, where all three sibling dev-loop routes are documented. embed_test.go pins only a work-items subset, so the gap is silent.
The authorization, identity-refusal and fail-closed outcome logic are sound, and the test file covers the state machine well, including all four NotFound-race variants. Tests were not executed here (sandbox denied go test); the review is static.
P3 nits are non-blocking. The portal-allowlist enabled: true is explicitly flagged as an owner decision in the PR body and is left to the owner.
The abandon handler described the latest run and then signalled the latest run with an empty run id. If the described run closed and a new run started under the same workflow id in between, the signal landed on a loop nobody looked at and the response still said "signalled". The signal now carries the described run id, so Temporal answers NotFound for a closed run. The second describe that follows reports a replacing run as 409 instead of abandoning it. A NotFound on that second describe is audited as "execution vanished between describe and signal": the run was observed moments earlier, and "workflow not found" misread it as a loop that never existed. The route is now in the published OpenAPI contract with its closed request body and every response it answers, and embed_test pins both the abandon and approve dev-loop routes so a future gap fails a test. "Running" becomes devLoopStatusRunning, next to devLoopStatusUnknown. Claude-Session: https://claude.ai/code/session_01QfTaaTnKZbCFJbu841N6j7
|
Claude finished @mashkovd's task in 3m 17s —— View job Review complete — approved
0 P1 · 0 P2 · 3 P3 — review submitted, bound to Both prior P2s are closedRun-id pinning — OpenAPI — P3 nits (non-blocking)
What looks right
Not verifiedTests were not executed — |
Antigravity (agy) review — informationalModel: gemini-3.8-flash-medium · commit No significant issues found. |
| Parameters: auditParams, | ||
| WorkflowName: workflowID, | ||
| Status: "succeeded", | ||
| RiskLevel: string(operations.RiskMedium), |
There was a problem hiding this comment.
P3 — the one audit entry that records an actual signal is the only one that does not name the run. finished() writes "not signalled: already finished (run <id>, status <status>)" and the 409 names both runs, but this succeeded entry has no Message and auditParams carries only abandoned_by / reason, so the audit log says "tester abandoned dev-loop-x" without saying which run was signalled. Since pinning the signal to the described run is exactly what this commit added, carrying that run id into the record it produces would finish the job — either Message: "signalled abandon on run " + exec.RunID or a run_id key in auditParams for the signalled path.
| // could not determine, so a read that determined nothing would otherwise | ||
| // answer known=true about a false it derived from nothing. The old | ||
| // `status == "Running"` covered that case by accident; this covers it on | ||
| // `status == devLoopStatusRunning` covered that case by accident; this covers it on |
There was a problem hiding this comment.
P3 — the constant substitution went one line too far. This sentence quotes the code as it used to read ("The old status == "Running" covered that case by accident"), so naming the constant inside the quotation describes a version of the old code that never existed, and the line now runs past the wrap width the rest of the block keeps. Reverting just this occurrence to the literal keeps the history accurate; the live comparisons (lines 522, 540, 634) are the ones that benefit from the constant.
Related: line 633 still compares against a bare "Unknown" while devLoopStatusUnknown exists and is used by the new handler. Constifying one half of the pair and leaving the other is the inconsistency the new constant was meant to remove.
| } | ||
|
|
||
| payload := map[string]string{"reason": reason, "abandoned_by": user.ID} | ||
| if err := h.opts.TemporalClient.SignalAbandon(r.Context(), workflowID, exec.RunID, payload); err != nil { |
There was a problem hiding this comment.
P3 (note, not a defect) — pinning the run also makes continue-as-new look like a replaced run. If DevLoopWorkflow ever continues-as-new (a long approval park is a plausible reason to trim history), Temporal answers NotFound for the pinned, now-closed run, the second describe finds a Running execution with a different run id, and the caller gets a 409 telling it to re-issue. The retry then abandons the continued run, so the outcome is still correct and self-healing — but an operator reading "a new run took the id" will look for a restart that never happened. One sentence in this comment block saying that a continue-as-new also lands in the 409 branch would save that investigation.
Also minor: the signal call uses r.Context() with no deadline of its own, while both describes get devLoopDescribeTimeout. Same as ApproveDevLoopWorkflow, so this is consistency rather than a change in behaviour.
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
Both P2s from the review of 7ddfd4b are closed by 238cdad: the signal is now pinned to the described run (SignalAbandon takes runID, and a NotFound answer routes into the second describe, where a Running execution under a different run id is a 409 rather than a silent abandon of a run nobody looked at), and the endpoint is documented in internal/openapi/openapi.yaml with additionalProperties: false, the outcome enum, and the 409 — with both dev-loop POST paths now pinned in embed_test.go, so the next gap fails. The race table covers all five second-describe outcomes and asserts the pinned run id on every one.
Authorization, identity refusal and the fail-closed outcome set are unchanged and sound. P3s are non-blocking: the succeeded audit entry for the signalled path does not record the run id it pinned, the comment at line 631 quotes old code with the new constant name while "Unknown" nearby stays a literal, and a continue-as-new would surface as a 409 requiring one retry (correct, worth a sentence).
Tests were not executed — go build, go vet and go test were denied by the sandbox in this run, so this review is static. CI results stand on their own.
Closes #392
Why
A DevLoopWorkflow whose proposal is deliberately never approved stays
Runningat the approval park. The only way to end it waspython -m orchestrator.temporal.cli abandon, which needs Temporal access from inside the cluster.dev-loop-mctlhq-seerrsense-73,dev-loop-mctlhq-mctl-telegram-737anddev-loop-mctlhq-mctl-docs-124are parked that way right now.What
POST /api/v1/agents/dev-loop/{workflow_id}/abandon, body{"reason": "<required, non-empty>"}. It sits in the same write rate-limit group as approve.abandonsignal (mctl-agents#420,DevLoopWorkflow.abandon) with{"reason", "abandoned_by"}. A signal, not a Temporal terminate, so_watch_pr'sfinallystill releases the lifecycle-ownership row.requireTemporalAdmin, so 503 when Temporal is not configured, 401 when unauthenticated, 403 for non-admins.refuseIdentityFieldsruns as well.abandoned_byis the authenticated caller. The body is decoded withDisallowUnknownFields, so a request cannot name the actor itself.outcome: already_finishedwith its final status. Nothing is signalled, so a retry is harmless.outcome: signalledUnknown): 502Running, the result is 502, not a success.mctl_abandon_dev_loop(workflow_id, reason), registered next tomctl_approve_dev_loop. It is annotated destructive and idempotent, and added toannotations_test.go's recorded hints.docs/portal-allowlist.json: enabled, with amutatingOnPortalvouch. This followsmctl_approve_dev_loop, and the issue's acceptance step is running this tool from the MCP surface. This is an owner decision. If you would rather keep it off the shared portal, flip it toenabled: falseand drop the vouch line; the tests accept either.Verified against mctl-agents (origin/main)
abandon(orchestrator/temporal/workflows/dev_loop.py,@workflow.signal def abandon). It readsreasonthrough_first_string, so the extraabandoned_bykey does no harm. The workflow does not recordabandoned_byyet; for now it lives in mctl-api's audit log (dev-loop-abandon).approval-watchpoll loop and the legacy branch wait onself._approved or self._abandoned, and the result isended="abandoned: <reason>". The re-approval park does the same.Not covered here
.status.yaml. The workflow's abandon path does not flip the proposal out ofproposed, and this endpoint does not touch gitops either. That is the issue's third "Verify" point, and it needs a mctl-agents change (or a separate gitops step). It is not addressed by this PR.mctl_approve_dev_loop/mctl_get_dev_loopare not operations-registry entries either: that registry describes Argo CWFT operations. The new tool follows approve's pattern, and the registry is unchanged.Tests
go build ./...,go vet ./internal/...go test -p 1 ./...: all pass locally. The Postgres-backed tests skip withoutTEST_DATABASE_URL.golangci-lint run --new-from-rev origin/main ./...: 0 issues. A full run reports one pre-existing gosec G710 atinternal/api/router.go:274with local v2.14.0; that line is outside this diff, and CI pins v2.11.4.internal/api/handlers_dev_loop_abandon_test.go): 503, 401/403, reason required, unknown, identity andabandoned_byfields rejected, the running path with the caller as actor plus an audit entry, already-finished, 404, 502 on an unreadable execution, 502 on a signal failure, and the NotFound race in four variantsDisallowUnknownFields, accepting a still-Runningsecond describe, or signalling a finished execution each fails a test.🤖 Generated with Claude Code
https://claude.ai/code/session_01QfTaaTnKZbCFJbu841N6j7