Repository navigation
feat(temporal): DevLoopWorkflow — issue -> investigate -> approve -> implement - #105
Conversation
…implement Phase 4 of the dev-workflow control plane plan: the first Temporal worker and workflow, layered on top of the Argo pipeline rather than replacing it. - orchestrator/temporal/activities/registry.py: resolve_agent_release, reading the phase-2 agent registry in mctl-api (falls back to None, not an exception, when nothing has ever been promoted for an agent/env pair). - orchestrator/temporal/activities/argo.py: submit_and_wait, submitting a CWFT via mctl-api's existing operations-execute path and polling to a terminal Argo phase. Registered with maximum_attempts=1 by the workflow — the CWFTs already retry within a run on a second OAuth account, so a Temporal-level retry on top would multiply real SDK spend. - orchestrator/temporal/activities/state.py: record_execution, writing the phase-4 execution audit trail via POST /api/v1/agents/executions (mctl-api companion change, separate PR) — Argo workflow objects expire after ttlStrategy.secondsAfterCompletion, so this is the durable record. - orchestrator/temporal/workflows/dev_loop.py: DevLoopWorkflow, covering exactly the plan's phase-4 acceptance slice (investigate -> durable wait for a signalled approval -> implement). Review-loop and deploy/monitor stages are phase 5/6, layered on top later. - orchestrator/temporal/worker.py + cli.py: worker entrypoint (task queue mctl-dev-loop) and a manual start/approve/status CLI, standing in for the mctl_trigger_issue use_temporal wiring (separate mctl-api change) until that lands. Tests: activities against a mocked httpx transport (real request/response shapes asserted, not just "was called"); DevLoopWorkflow against temporalio's time-skipping WorkflowEnvironment with fake activities.
|
Claude finished @mashkovd's task in 3m 19s —— View job Reviewing PR #105
SummarySolid, well-documented phase-4 slice (investigate → durable approval wait → implement) with good test coverage (activities against mocked httpx, workflow against Temporal's time-skipping test env). CI ("PR validation": ruff/mypy/pytest) passed. Findings: 0 P1, 2 P2, 2 P3
Everything else — retry-ownership reasoning (no Temporal retry stacked on SDK-backed CWFT runs), the Submitted as request changes given the two P2s (both are real risk-of-duplicate-spend / stuck-workflow scenarios, not style nits). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async def _resolve(agent: str) -> ResolvedRelease | None: | ||
| return await workflow.execute_activity( | ||
| resolve_agent_release, | ||
| args=[agent, ENVIRONMENT], | ||
| start_to_close_timeout=FAST_ACTIVITY_TIMEOUT, | ||
| ) | ||
|
|
||
|
|
||
| async def _run_cwft(operation: str, params: dict[str, str]) -> WorkflowResult: | ||
| return await workflow.execute_activity( | ||
| submit_and_wait, | ||
| SubmitAndWaitInput(operation=operation, params=params), | ||
| start_to_close_timeout=SDK_STEP_TIMEOUT, | ||
| heartbeat_timeout=SDK_STEP_HEARTBEAT_TIMEOUT, | ||
| retry_policy=SDK_STEP_RETRY_POLICY, | ||
| ) | ||
|
|
||
|
|
||
| async def _record(agent: str, release: ResolvedRelease | None, result: WorkflowResult) -> None: | ||
| await workflow.execute_activity( |
There was a problem hiding this comment.
P2 — _resolve and _record execute their activities with no explicit retry_policy, so they inherit Temporal's default of unlimited attempts with capped backoff. Per the PR description, POST /api/v1/agents/executions doesn't exist in mctl-api yet (separate PR). If mctl-agents-worker is deployed before that companion endpoint ships, every real run will succeed at investigate and then retry _record forever, permanently blocking wait_condition for approval — even though the actual SDK work already completed successfully.
Consider a bounded retry_policy (e.g. maximum_attempts=N) on record_execution (and resolve_agent_release), or making the audit-trail write best-effort (log + continue) so a state-recording failure can't wedge the workflow's critical path indefinitely.
| while True: | ||
| # Heartbeat before every poll, not just on change: a stuck | ||
| # mctl-api / cluster makes this loop spin on the `continue` | ||
| # below without ever reaching a terminal phase, and the | ||
| # heartbeat is what lets Temporal notice a genuinely wedged | ||
| # activity (worker crash, network partition) instead of the | ||
| # activity looking alive forever because polling itself is | ||
| # still succeeding. | ||
| activity.heartbeat(workflow_name) | ||
|
|
||
| status_resp = await client.get(f"/api/v1/workflows/{workflow_name}", headers=headers) |
There was a problem hiding this comment.
P2 — The poll loop has no resilience to transient HTTP errors on the GET /api/v1/workflows/{name} call: status_resp.raise_for_status() propagates any transient 5xx/network blip straight out of the activity. Since DevLoopWorkflow registers this activity with maximum_attempts=1 (deliberately, to avoid duplicate Argo/SDK submissions), a single mctl-api hiccup hours into an already-submitted run kills the whole activity permanently — and the workflow_name of the in-flight Argo run isn't persisted anywhere. Recovering means either losing track of that run or an operator manually retrying, which resubmits a new CWFT and duplicates real SDK spend — exactly the failure mode maximum_attempts=1 was chosen to prevent, just triggered by a polling-side blip instead of a Temporal auto-retry.
Consider catching transient errors in the polling loop itself (e.g. retry the GET a few times before giving up) rather than letting any single poll failure fail the whole activity.
| resolve_resp.raise_for_status() | ||
| version = resolve_resp.json()["version"] | ||
|
|
||
| versions_resp = await client.get(f"/api/v1/agents/{agent}/versions", headers=headers) |
There was a problem hiding this comment.
P3 — versions_resp.json().get("items", []) assumes the /versions response is unpaginated. If mctl-api ever paginates this list, a version could silently fail to be found here, and resolve_agent_release would return image_ref="" (falling back to the CWFT default) rather than surfacing an error. Worth a comment noting the assumption, or handling pagination if the endpoint supports it.
| @@ -0,0 +1,99 @@ | |||
| """Manual DevLoopWorkflow control: `python -m orchestrator.temporal.cli ...`. | |||
There was a problem hiding this comment.
P3 — No tests for this module (start/approve/status). It's described as a stand-in CLI for manual verification, so this is low priority, but _workflow_id_for's URL-parsing logic (lines 25-31) would be cheap to unit test directly.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: (0 P1, 2 P2, 2 P3) — record_execution/resolve_agent_release have no bounded retry_policy and can wedge the workflow forever if mctl-api's companion endpoint is unavailable; the Argo poll loop has no resilience to transient HTTP errors, which combined with maximum_attempts=1 risks duplicate CWFT/SDK submissions on recovery.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24b586a8fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SubmitAndWaitInput(operation=operation, params=params), | ||
| start_to_close_timeout=SDK_STEP_TIMEOUT, | ||
| heartbeat_timeout=SDK_STEP_HEARTBEAT_TIMEOUT, | ||
| retry_policy=SDK_STEP_RETRY_POLICY, |
There was a problem hiding this comment.
When the worker restarts or loses heartbeats after submit_and_wait has already POSTed the Argo workflow, the missed heartbeat times out this same activity attempt; with maximum_attempts=1, Temporal then fails the whole DevLoopWorkflow instead of resuming the poll, even though the Argo run may still finish and need to be recorded/approved. For long SDK-backed runs during a worker rollout or crash, this loses the durable per-issue state the new control plane is meant to provide; split submit from polling or resume polling from a persisted workflow name/idempotency key while keeping duplicate submits disabled.
Useful? React with 👍 / 👎.
| version: str # "" when no registry release existed yet (CWFT default image used) | ||
| image_ref: str # "" when no registry release existed yet | ||
| argo_workflow_name: str | ||
| phase: str # Succeeded | Failed | Error |
There was a problem hiding this comment.
For issue-investigator and implementer runs, the repository inventory states that runtime inputs outside this repo are pinned by the target repo git SHA recorded on the execution record (docs/agent-inventory.yaml:55-63), because the same agent version against different target SHAs is not reproducible. This new execution record only captures the agent version/image and Argo result, so mctl_list_recent_agent_runs cannot reconstruct or compare the actual prompt surface when the target repo changes between runs; include the target repo/ref SHA in the recorded payload.
Useful? React with 👍 / 👎.
| implement_params["agent_image"] = implementer_release.image_ref | ||
| implement_params["agent_version"] = f"implementer@{implementer_release.version}" | ||
|
|
||
| implement_result = await _run_cwft("mctl-agents-implement", implement_params) |
There was a problem hiding this comment.
In the Temporal-only approval path, the investigator still writes a proposed proposal and this signal only flips the workflow's in-memory _approved flag; nothing updates the gitops .status.yaml to accepted before invoking the legacy mctl-agents-implement template. Since run_implementer.py only selects proposals whose status is accepted, an approved DevLoopWorkflow will either do nothing for this issue or pick some unrelated already-accepted proposal because implement_params is empty.
Useful? React with 👍 / 👎.
| temporal_workflow_id=workflow.info().workflow_id, | ||
| agent=agent, | ||
| environment=ENVIRONMENT, | ||
| version=release.version if release else "", |
There was a problem hiding this comment.
When resolve_agent_release finds a release version but cannot build an image_ref from the versions response, the workflow deliberately omits agent_image/agent_version and lets Argo run the CWFT's baked-in default image. This record still stores release.version, so the audit trail can claim version 1.2.0 produced a PR even though that version was never passed to the run; only record the version when the image was actually pinned, or explicitly mark that the default image ran.
Useful? React with 👍 / 👎.
- dev_loop.py: _resolve and _record now use a bounded RetryPolicy (maximum_attempts=5) instead of Temporal's unlimited default. _record's eventual failure is caught and logged rather than propagated — it's an audit-trail write for a step that already succeeded, so it must never block wait_condition/approval indefinitely (P2, claude[bot]). - argo.py: submit_and_wait's poll loop now rides out up to 20 consecutive transient errors (mctl-api 5xx, network blip) before giving up, instead of letting any single poll failure kill the activity outright — this activity runs with maximum_attempts=1 on purpose, so a bare raise_for_status() on every poll meant one blip permanently lost track of an already-submitted Argo run (P2, claude[bot]). - registry.py: note the unpaginated-response assumption on GET .../versions (P3). - New tests/test_temporal_cli.py for _workflow_id_for (P3) — must stay byte-for-byte identical to mctl-api's Go-side WorkflowIDForIssueURL. - New workflow test for the best-effort record_execution behavior, and two new activity tests for the poll-resilience fix (rides out N errors then succeeds; gives up after too many).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 3m 55s —— View job Reviewing PR #105 (new commit
|
| while True: | ||
| # Heartbeat before every poll, not just on change: a stuck | ||
| # mctl-api / cluster makes this loop spin on the `continue` | ||
| # below without ever reaching a terminal phase, and the |
There was a problem hiding this comment.
P2 — _run_cwft still registers submit_and_wait with maximum_attempts=1 and, per this activity's own docstring, the submitted Argo workflow_name is never persisted anywhere Temporal can use to resume (no query handler, no memo/search attribute — only activity.logger.info at line 79 and the per-poll activity.heartbeat(workflow_name), whose details are never read back on retry since there is no retry to read them on). If the worker restarts or a single heartbeat is missed mid-poll (SDK_STEP_HEARTBEAT_TIMEOUT = 2 min) after the Argo submission already succeeded, the whole DevLoopWorkflow run fails permanently and the in-flight Argo run is orphaned — it may still finish, but nothing will ever record or approve based on it.
This is the same tradeoff already called out in the module docstring (lines 5-12) as intentional — avoiding a Temporal retry stacked on the CWFT's own OAuth-account fallback — but the resumability gap itself (as opposed to the retry-multiplication one) wasn't in scope of that reasoning. Worth tracking before this worker is deployed for real (per the PR's own "Live" test-plan item, still unchecked): e.g. persist workflow_name via workflow.upsert_memo/a query handler so an operator can at least recover/re-poll it manually, even with maximum_attempts=1 staying at 1.
| implement_params["agent_image"] = implementer_release.image_ref | ||
| implement_params["agent_version"] = f"implementer@{implementer_release.version}" | ||
|
|
||
| implement_result = await _run_cwft("mctl-agents-implement", implement_params) |
There was a problem hiding this comment.
P2 — Confirmed against orchestrator/run_implementer.py: find_accepted_proposals filters strictly on status == "accepted" and implement_one skips anything else. Nothing in this repo (this PR or pre-existing) writes .status.yaml to accepted automatically — run_issue_investigator.py only ever writes proposed and tells a human to flip it manually. approve() here (lines 125-127) has no side effect beyond self._approved = True.
So today, calling approve() on a real deployment and reaching this _run_cwft("mctl-agents-implement", implement_params) call does one of two things, neither of which is "implement this issue's proposal": with no accepted proposals it's a no-op, or — since implement_params (lines 152-156) carries no service/slug filter — it picks up whatever unrelated proposal already happens to be accepted (max_proposals=1 per the CWFT default). The latter is the concerning case: it's not just "nothing happens", it's "the wrong issue's proposal gets implemented."
This is called out as a known simplification in the module docstring (lines 12-17, 19-23), so not a surprise to the author, but it means the PR's own acceptance slice ("approval signal -> implement -> PR") doesn't actually close the loop end-to-end yet — worth confirming this is truly gated behind the still-unchecked "Live" test-plan item and isn't reachable from mctl_trigger_issue before phase 5's cutover lands.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 0 P3.
Both P2s from my prior review (unbounded retries on _resolve/_record, no resilience in the Argo poll loop) are well fixed in 82cf0ae — bounded RetryPolicy(maximum_attempts=5) plus a best-effort try/except around record_execution, and the poll loop now rides out up to 20 consecutive transient errors (~5 min) before giving up. The registry.py P3 got a documenting comment and cli.py picked up tests for _workflow_id_for. Nice work.
Two new P2s, independently verified against the current code (both already surfaced by Codex's automated review, confirming here with direct evidence):
activities/argo.py—submit_and_wait's Argoworkflow_namestill isn't persisted anywhere Temporal can use to resume; combined withmaximum_attempts=1, a worker crash or missed heartbeat mid-poll after a successful Argo submission permanently fails the workflow and orphans the in-flight Argo run.workflows/dev_loop.py— confirmed againstrun_implementer.py:approve()only flips an in-memory flag, nothing writes.status.yamltoaccepted, andimplement_paramshas no service/slug filter — so an approved run either no-ops or can implement an unrelated already-accepted proposal instead of this issue's.
Both are explicitly called out as known simplifications in the module docstrings, so not a surprise to the author — flagging as should-fix given they affect the PR's core "approve -> implement" acceptance claim and the still-unchecked "Live" test-plan item, not as a rejection of the scoping decision itself.
| while True: | ||
| # Heartbeat before every poll, not just on change: a stuck | ||
| # mctl-api / cluster makes this loop spin on the `continue` | ||
| # below without ever reaching a terminal phase, and the |
There was a problem hiding this comment.
P2 — _run_cwft still registers submit_and_wait with maximum_attempts=1 and, per this activity's own docstring, the submitted Argo workflow_name is never persisted anywhere Temporal can use to resume (no query handler, no memo/search attribute — only activity.logger.info at line 79 and the per-poll activity.heartbeat(workflow_name), whose details are never read back on retry since there is no retry to read them on). If the worker restarts or a single heartbeat is missed mid-poll (SDK_STEP_HEARTBEAT_TIMEOUT = 2 min) after the Argo submission already succeeded, the whole DevLoopWorkflow run fails permanently and the in-flight Argo run is orphaned — it may still finish, but nothing will ever record or approve based on it.
This is the same tradeoff already called out in the module docstring (lines 5-12) as intentional — avoiding a Temporal retry stacked on the CWFT's own OAuth-account fallback — but the resumability gap itself (as opposed to the retry-multiplication one) wasn't in scope of that reasoning. Worth tracking before this worker is deployed for real (per the PR's own "Live" test-plan item, still unchecked): e.g. persist workflow_name via workflow.upsert_memo/a query handler so an operator can at least recover/re-poll it manually, even with maximum_attempts=1 staying at 1.
…it trail (PR #105 review) Addresses the 4 P1/P2 findings from claude[bot] + chatgpt-codex-connector[bot] on the latest commit, all independently confirmed against the code: - submit_and_wait now distinguishes a retry before submission (must re-submit) from a retry after submission (must not, only resume polling) via activity.info().heartbeat_details, so a worker crash or missed heartbeat mid-poll no longer orphans an already-submitted Argo run or fails the whole DevLoopWorkflow. SDK_STEP_RETRY_POLICY can now allow a few attempts without ever causing a second real SDK run. - The implement step is now scoped to the issue's own repo (the CWFT's existing `service` parameter, derived from issue_url via the new shared orchestrator/temporal/issue_ref.py). approve() still doesn't automate the proposed -> accepted gitops flip (that needs a new gitops-committing CWFT step, deferred to phase 5 per the mctl-gitops-main-writes mutex constraint), but it can no longer cause a DIFFERENT repo's already-accepted proposal to be implemented. - Execution records no longer claim a registry version/image_ref was used when resolve_agent_release found a version but no pullable image_ref (the CWFT's own baked-in default ran instead). - Execution records now carry target_repo, the first half of the reproducibility-pinning story in docs/agent-inventory.yaml (the exact target SHA still isn't captured -- no CWFT exposes one as an output yet).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 6m 4s —— View job Reviewing PR #105 (new commit
|
| submit_resp.raise_for_status() | ||
| workflow_name = submit_resp.json()["workflow"]["workflowName"] | ||
| activity.logger.info("submitted %s -> %s", input.operation, workflow_name) | ||
| # Record the submission immediately, before the first poll, so | ||
| # even a crash on the very next line leaves a retry able to find | ||
| # workflow_name via heartbeat_details above instead of resubmitting. | ||
| activity.heartbeat(workflow_name) |
There was a problem hiding this comment.
P2 — The resumability fix narrows but doesn't fully close the duplicate-submission window it's meant to eliminate. Between submit_resp.raise_for_status() succeeding (Argo has already created the run server-side) and activity.heartbeat(workflow_name) actually completing, a crash, KeyError/malformed-JSON on submit_resp.json()["workflow"]["workflowName"], or any other exception in that gap leaves heartbeat_details empty on the next attempt — so SDK_STEP_RETRY_POLICY's maximum_attempts=3 will resubmit and duplicate the real SDK run, exactly the failure mode this design is built to prevent.
This is a much narrower window than before (previously the entire activity duration was unprotected, but maximum_attempts=1 meant no retry ever happened, so no duplicate risk either) — raising maximum_attempts to 3 is what reintroduces this specific gap. The module docstring's claim ("without ever causing a second real SDK run for the same step") slightly overstates the actual guarantee.
Given submit_resp.json()["workflow"]["workflowName"] is the most likely failure point in that window (an unexpected response shape), consider either wrapping that parse in a try/except that still heartbeats a "submitted, name unknown" sentinel (so retries at least know not to resubmit even if they can't resume polling), or — the more complete fix — having mctl-api's /execute accept a caller-supplied idempotency key so a duplicate POST after a crash returns the existing workflow instead of creating a new one.
| # implement CWFT only considers proposals under agents-state/<service>/, | ||
| # so an approve() here can at worst implement a different | ||
| # already-accepted proposal in the SAME repo, never a different one. | ||
| implement_params: dict[str, str] = {"service": target_repo} |
There was a problem hiding this comment.
P2 — This fix's correctness depends entirely on the sibling mctl-gitops repo's cwft-mctl-agents-implement.yaml CWFT already declaring a service input parameter and threading it through to run_implementer.py --service <value> — nothing in this repo can verify that link (orchestrator/validate_manifest.py itself notes the real CWFT YAML lives in mctl-gitops, which isn't checked out here). run_implementer.py's find_accepted_proposals(service_filter=...) confirms the filter logic is correct if it receives a service_filter, and an absent one is exactly today's unscoped "pick up anything accepted" behavior — so if the CWFT param isn't wired yet, this fix silently regresses to the pre-fix behavior with no error and no test able to catch it from this repo.
Given this exact scoping gap was the P1 flagged by Codex's automated review last round, worth explicitly confirming the mctl-gitops companion change has landed (or at minimum tracking it) before this is exercised for real — the PR's own "Live" test-plan item is still unchecked.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 0 P1, 2 P2, 0 P3. All 4 P1/P2s from the prior two rounds are well fixed in ecda152 (resumable Argo polling via heartbeat_details distinguishing pre/post-submission retries, implement-step scoped to the issue's own repo via the service param, unpinned releases no longer recorded as used, target_repo added to the audit trail). Two new P2s found on this round: (1) activities/argo.py — the resumability fix narrows but doesn't fully close the duplicate-submission window: a crash/malformed-response exception between the Argo POST succeeding and activity.heartbeat() completing still causes a resubmit on retry now that maximum_attempts=3 (previously =1 meant no retry, so no duplicate risk existed at all). (2) workflows/dev_loop.py — the service-param scoping fix's correctness depends on an unverified assumption about the sibling mctl-gitops repo's CWFT already wiring service through to run_implementer.py --service; if that companion change hasn't landed, the fix silently no-ops with no test able to catch it from this repo.
…ndency (PR #105 review round 3) Addresses claude[bot]'s 2 new P2s on ecda152: - submit_and_wait now wraps the workflow-name parse in try/except: if mctl-api returns 2xx (the Argo run genuinely exists) but the body can't be parsed for a name, a sentinel is heartbeated so a retry detects "already submitted, name unknown" and fails loudly instead of resubmitting (which would duplicate the real SDK run). Narrows the remaining unprotected window down to a crash strictly between the POST succeeding and that first heartbeat landing — no I/O in between, so vanishingly unlikely, and documented as such rather than overstated as fully closed. - The implement-step `service` scoping depends on mctl-gitops's cwft-mctl-agents-implement.yaml already wiring a `service` parameter through to run_implementer.py --service. Verified directly against that file (not assumed) and documented with the exact evidence inline, since mctl-agents' CI has no way to check this cross-repo dependency itself. Also mirrors StartDevLoopWorkflow's new WorkflowIDReusePolicy/ WorkflowIDConflictPolicy (mctl-api PR #129) in cli.py's start(), so a repeated start against the same issue is a true no-op on both sides instead of the SDK's ALLOW_DUPLICATE/UNSPECIFIED defaults.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @mashkovd's task in 2m 27s —— View job Reviewing PR #105 (new commit
|
| # as an empty phase right after submission. | ||
| TERMINAL_PHASES = frozenset({"Succeeded", "Failed", "Error"}) | ||
| # How many consecutive poll failures (mctl-api 5xx, network blip) to ride | ||
| # out before giving up. Because this activity runs with maximum_attempts=1 |
There was a problem hiding this comment.
P3 — pre-existing, not introduced by this commit, but worth a quick fix while touching this file: this comment still says "this activity runs with maximum_attempts=1", but SDK_STEP_RETRY_POLICY in workflows/dev_loop.py was bumped to maximum_attempts=3 back in ecda152 (the resumable-polling fix, two rounds ago) and the module docstring above was updated accordingly — this one spot wasn't. Doesn't affect correctness (MAX_CONSECUTIVE_POLL_ERRORS's reasoning holds either way), just stale/misleading for a future reader trying to reconstruct the actual retry-attempt count from this file alone.
There was a problem hiding this comment.
No P1/P2 findings (1 P3, pre-existing stale comment). Good to merge.
Both P2s from the prior round are well resolved in 9575ae3:
activities/argo.py— the remaining duplicate-submission window is closed for the identified gap: a submit response that returns 2xx but fails to parse a workflow name now heartbeats a sentinel (_SUBMITTED_UNKNOWN_NAME) before raising, and a retry that sees that sentinel refuses to resubmit and fails loudly instead, with direct test coverage for both the sentinel-heartbeat and refuse-to-resubmit paths. The one gap left (a crash strictly between the Argo POST succeeding and the heartbeat call landing at the Temporal server, no I/O in between) is now explicitly documented as an accepted, vanishingly-small residual risk rather than an overstated guarantee — reasonable given closing it fully would require an idempotency key on mctl-api's/executeendpoint, which is out of scope for this repo.workflows/dev_loop.py— theservice-param scoping dependency on the siblingmctl-gitopsCWFT is now documented as directly verified (with the actual YAML snippet referenced) rather than assumed, with an explicit note that a future unannounced change to that CWFT would silently regress scoping with no error on this side. Not a code-level fix (no test can catch drift from this repo), but a reasonable resolution for a cross-repo dependency this repo can't check out.
The cli.py change (REJECT_DUPLICATE + USE_EXISTING id policies, mirroring mctl-api's planned Go client) is a reasonable idempotency hardening, not directly tied to a prior finding.
CI (PR validation: ruff/mypy/pytest) passed on 9575ae3.
Summary
.claude/plans/deployment-purrfect-umbrella.md): first Temporal worker + workflow, layered on top of the existing Argo pipeline (does not replace or duplicate any agent's actual SDK run).orchestrator/temporal/activities/registry.py—resolve_agent_release, reading the phase-2 agent registry in mctl-api. ReturnsNone(not an exception) when nothing has ever been promoted for an (agent, environment) pair — the expected steady state before a firstmctl_promote_agent, not a failure.orchestrator/temporal/activities/argo.py—submit_and_wait, submitting a CWFT via mctl-api's existing/api/v1/operations/{name}/executepath (same pathmctl_trigger_issuealready uses) and polling/api/v1/workflows/{name}to a terminal Argo phase.DevLoopWorkflowregisters this activity withmaximum_attempts=1— the CWFTs already retry within a run (second-OAuth-account fallback on a 429), so a Temporal-level retry on top would multiply real SDK spend exactly when quota is already tight.orchestrator/temporal/activities/state.py—record_execution, POSTing to/api/v1/agents/executions(mctl-api companion change — separate PR, mctl-api#TBD). Needed because Argo workflow objects expire afterttlStrategy.secondsAfterCompletion— this activity captures the outcome at completion time instead of relying on being able to re-read Argo later.orchestrator/temporal/workflows/dev_loop.py—DevLoopWorkflow, covering exactly the plan's phase-4 acceptance slice: investigate → durablewait_conditionfor a signalledapprove()→ implement. The review-fix loop (shepherd) and deploy/monitor stages are phase 5/6, meant to layer on top of this workflow once the slice is proven, not part of this PR.orchestrator/temporal/worker.py+cli.py— worker entrypoint (task queuemctl-dev-loop) and a manualstart/approve/statusCLI. Stands in for themctl_trigger_issueuse_temporalflag (a separate, larger mctl-api Go-client change) until that lands — lets a real workflow be started/approved/inspected for verification in the meantime.pyproject.toml— addedtemporalio==1.19.0as a new pinned direct dependency, and promotedhttpx(already transitive viaclaude-agent-sdk/mcp) to a direct, deliberately-pinned one since the new activities import it directly.Test plan
ruff check .— cleanmypy— clean (29 source files)pytest -q— 306 passed (was 294; +12 new: 9 activity tests against a mockedhttpxtransport asserting real mctl-api request/response shapes, 3DevLoopWorkflowtests againsttemporalio.testing.WorkflowEnvironment.start_time_skipping()with fake activities)mctl-agents-workeris deployed (mctl-gitops, separate PR) andPOST /api/v1/agents/executionsexists (mctl-api, separate PR), start a real workflow viaorchestrator.temporal.cli start <issue_url>, confirm it shows up in the Temporal UI, signalapprove, confirm the implement step runs