[mashkovd]

Closes the last volumeClaimTemplates in the agent fleet (#1047). With it, no mctl-agents workflow creates a Hetzner volume any more.

Why now

Hetzner's volume action history for the last week (/v1/volumes/actions, cross-referenced with kube_persistentvolumeclaim_info) attributes every volume created since 2026-09-08 to this one template:

day	volumes created	templates
09-04	18	run, incidents, investigate, shepherd, approve
09-05	31	implement + the others, mid-port
09-08	3	implement only
09-09	16	implement only
09-10	4	implement only
09-11	20	implement only

Six steps were six pods spread across nodes, so the RWO volume moved between them on every step — six attach/detach pairs per run, Multi-Attach error in the events on each. At ~20 DevLoop-driven runs a day that is ~120 volume operations daily on one template. The cost is trivial (€0.0572/GB/month); the risk is not: a wedged detach leaves a pod Pending, and a Pending pod holding mctl-gitops-main-writes is the 2026-07-30 cascade.

The port

Same recipe as #1042 / #1046 / #1048, carried over in full:

• Per-pod clone. The standalone clone-gitops step is gone; each pod that needs the checkout takes its own in an initContainer.
• Artifact handoff at a fixed workflow-scoped S3 key, read literally by commit-and-push. Both primary and fallback write the same key, so no reference has to resolve through a skipped step. optional: true, and the output directory is created only once there is something to put in it, so an attempt that produced nothing cannot clobber the previous one's object.
• Collector owned by the script and run after the agent exits, paths split on NUL in Python, and a path that is not a .status.yaml under agents-state is fatal rather than skipped.
• commit-and-push takes its OWN fresh clone and validates the handoff before any of it reaches the checkout — the .git/hooks boundary git status structurally cannot see. This closes commit-and-push runs git as root inside a worktree the agent can write #983 P1 for this template and lets safe.directory and git reset -q origin/main go with the shared worktree.
• activity reaches onExit as an optional artifact, not a file on the PVC and not a workflow.outputs.parameters entry (fix(agents): skip Telegram notify on no-op Succeeded runs #530/fix(agents): pass activity via workdir PVC, not workflow.outputs #531, and shepherd: post-deploy-verify reports "unknown (verification did not complete)" on runs where it completed cleanly #1186 for why the one remaining slot did not work either).
• ephemeral-storage requests and limits on every step, because an emptyDir counts against the pod rather than being invisible to the node.
• retryStrategy on commit-and-push, matching cwft-mctl-agents-run.

The Go cache is deleted, not ported

restore-go-cache / save-go-cache ran on minio/mc and bridged the shared volume to MinIO. The investigation recorded on #1047 found they have never worked:

• The agent overrides GOPATH to $HOME/go on every run — 178 export GOPATH=$HOME/go statements in one run's audit log — so the container's GOPATH=/workdir/gocache/gopath is set correctly and then discarded.
• The save-side emptiness check find /workdir/gocache/gopath -maxdepth 3 -type f cannot see a Go module cache even when it is full; the first file sits deeper.

Both steps end in an unconditional exit 0 and are documented as non-fatal, so six months of "starting cold" went unnoticed, and platform-cache in MinIO has never held a mctl-agents/gocache/ object. Removing them is removing dead code, not a regression. Their ExternalSecret in argo-workflows had no other consumer and goes with them; the per-tenant openclaw minio-cache-creds secrets are a separate declaration and are untouched.

A cache that actually works is a different design — a per-service/per-toolchain partitioned artifact at the path the agent really uses (/home/app/go), plus per-language images so the Go/Zig/musl-cross/gcc bootstrap stops being paid at all, which is the larger half of the cold start. Tracked separately from this port.

Validation

• argo lint --offline clean on the whole cluster-templates/ directory, with the CLI pinned to the cluster's v3.7.11. (The v4 CLI rejects synchronization.mutex on every template in the repo, including untouched ones — a schema change, not a finding.)
• scripts/validate-local-workdir.py, scripts/validate-shell-param-interpolation.py, scripts/validate-agent-platform.py --selftest all pass.

What a merge does NOT prove

#676/#677 were green and were reverted on a misattribution, so a green run is not the acceptance criterion. Before this is called done:

• A live implement run creates no volume: kubectl get pvc -n argo-workflows empty during the run, no SuccessfulAttachVolume in pod events, no create_volume in Hetzner's action history — and the PR it was implementing still lands.
• onExit reports a real activity value (yes / none), asserted on the value and not on the exit code.
• The scope guard proven by mutation: a handoff carrying a path outside agents-state fails the step and pushes nothing, while the pre-change template accepts the same payload.

Refs #1047, #856, #983

🤖 Generated with Claude Code

https://claude.ai/code/session_011zuKeZdjt1ZfF4SaK3rEkf

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @mashkovd's task in 6m 6s —— View job

---

Review complete for PR #1194

• Fetch pinned commits and view diff
• Review cwft-mctl-agents-implement.yaml changes
• Review deletion of mctl-agents-minio-cache.yaml
• Post findings / verdict

Verdict: APPROVED — 0 P1/P2 findings, 1 P3 (non-blocking).

This is a faithful port of the already-validated recipe from #1042/#1046/#1048: per-pod initContainer clone (no more shared workdir volume), artifact-based .status.yaml handoff at a fixed workflow-scoped S3 key, commit-and-push taking its own fresh clone (closing #983 P1 for this template), activity traveling to onExit as an artifact instead of a PVC file or workflow.outputs.parameters, and ephemeral-storage requests/limits added everywhere the workdir moved to an emptyDir.

Specifically verified:

• No leftover references to the removed top-level clone-gitops template, restore-go-cache/save-go-cache, or GOPATH — clean removal.
• minio-cache-creds ExternalSecret deletion has no other consumer in the argo-workflows namespace; the per-tenant openclaw secrets of the same name are separate declarations, untouched.
• The Python handoff collector is NUL-safe and correctly rejects paths outside agents-state/**/.status.yaml, symlinks, .., and control characters before anything reaches the checkout.
• Mutex + new retryStrategy on commit-and-push mirrors the existing pattern on cwft-mctl-agents-run.

One P3 posted inline: commit-and-push's second-layer NON_STATUS_CHANGES/STATUS_CHANGES guard parses git status --porcelain -z with a blind cut -c4-, which is the same rename/copy mis-parse class the file's own Python-collector comment calls out (chops 3 real chars off a rename's old-path continuation record). Not exploitable here — the checkout only ever contains paths the Python side already validated, so a detected rename could only occur between two already-in-scope .status.yaml paths — and the identical pattern is already merged in cwft-mctl-agents-shepherd.yaml, so it isn't novel to this PR. Noted for awareness, not blocking.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high · commit aea8563.

Severity: P2
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-implement.yaml
Line: ~422
Anchor: + python - "$RC" <<'COLLECT'

Scenario: The workflow replaces the shared PVC with an Argo artifact created by a Python script running at the very end of the run-implementer shell script. The template relies on continueOn: error: true to rescue partial .status.yaml writes if the pod suffers an Error (e.g., OOMKilled or hitting activeDeadlineSeconds). However, if the container is OOMKilled, the Linux kernel sends SIGKILL, abruptly terminating the shell script. The Python collector will never execute, /tmp/state-out is never populated, and Argo uploads an empty artifact. commit-and-push then receives this empty handoff and exits cleanly, pushing nothing. Any valid .status.yaml updates written to disk prior to the crash are silently lost, breaking the durability guarantee of the old PVC design and causing duplicate PR attempts on the next run.

Proposed Fix: Configure Argo to directly archive the /workdir/mctl-gitops/platform-gitops/agents-state directory natively via the changes artifact path, rather than relying on a post-run Python script. The commit-and-push step already enforces strict path validation and rejects symlinks, securely protecting against malformed handoffs. (Note: capturing file deletions natively via artifacts is non-trivial, which is why the script was introduced; this highlights that emptyDir + artifacts fundamentally lacks the true cross-step durability of a shared PVC against SIGKILL).

---

Severity: P3
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-implement.yaml
Line: ~9
Anchor: + 1. run-implementer — orchestrator/run_implementer.py inside

Scenario: The PR introduces complex new shell and Python logic to compute git diffs, handle renames/deletions, and securely overlay artifacts onto a pristine Git checkout. There is no test visible in this diff to verify that these new handoff boundaries correctly process edge cases or reject malicious paths as intended.

Proposed Fix: Add unit or E2E tests validating the Python artifact collection script and the strict commit-and-push guard logic.

[claude]

P3: This second-layer guard parses git status --porcelain -z with tr '\0' '\n' | cut -c4-, blindly stripping 4 chars off every NUL-separated record. The Python collector earlier in this same file (and its comment at the top of the COLLECT heredoc) explicitly calls out why that's wrong for a detected rename/copy: XY new\0old\0 puts the status prefix only on the first field, so this cut chops 3 real characters off the old path continuation line (the exact class of bug fixed as claude P2 on #1046). Not exploitable today — the handoff onto this checkout is purely additive from paths the Python side already validated (no control chars), so a same-repo rename between two already-allowed .status.yaml paths is the only way to reach this code, and both endpoints would already be in-scope. Flagging because it's a latent inconsistency: if this guard is ever relied on more broadly (e.g. pathspec widened), it silently reintroduces the bug the NUL-safe Python parsing was written to close. Same pattern already exists in cwft-mctl-agents-shepherd.yaml, so this isn't new to this PR — just noting it since it's not blocking here.

[claude]

No P1/P2 findings (1 P3). Good to merge.

Reviewed against the pinned diff (dcaf93b...aea8563, two files: cwft-mctl-agents-implement.yaml and the deleted mctl-agents-minio-cache.yaml ExternalSecret). This is a faithful port of the already-validated recipe from #1042/#1046/#1048 (per-pod initContainer clone, artifact-based handoff at a fixed workflow-scoped S3 key, commit-and-push on its own fresh clone, activity via onExit artifact, ephemeral-storage accounting for the new emptyDir).

Checked specifically:

• No leftover references to the removed clone-gitops top-level template, restore-go-cache/save-go-cache, or GOPATH — clean removal.
• minio-cache-creds ExternalSecret deletion: confirmed no other consumer in argo-workflows namespace; the per-tenant openclaw secrets of the same name are separate declarations and untouched.
• Handoff path validation (Python collector, NUL-safe) correctly blocks paths outside agents-state/**/.status.yaml, symlinks, .., and control characters before anything reaches the checkout.
• commit-and-push takes its own fresh clone, never touches the agent's worktree (closes #983 P1 for this template), and the mutex/retryStrategy pairing matches cwft-mctl-agents-run.
• Resource requests/limits (cpu/memory/ephemeral-storage) added consistently across the new initContainers and the emptyDir-backed main containers.

One P3 posted inline: the second-layer NON_STATUS_CHANGES/STATUS_CHANGES guard in commit-and-push parses git status --porcelain -z with a blind cut -c4-, which is the same rename/copy mis-parse class the Python collector's own comment calls out. Not exploitable in this flow (upstream validation already excludes control chars, so a rename can only occur between two already-in-scope paths) and it's an existing pattern already merged in cwft-mctl-agents-shepherd.yaml, so not novel to this PR and not blocking.

[mashkovd]

Answering agy's two findings, since the P2 describes a real mechanism even though its consequence does not follow.

P2 — SIGKILL loses the handoff. Mechanism confirmed, consequence does not hold.

Every link up to the last one is true, and it is worth stating plainly: an OOMKilled pod loses whatever .status.yaml it had written. The kernel sends SIGKILL, the shell dies, the Python collector never runs, /tmp/state-out is absent, optional: true turns that into no artifact, and commit-and-push writes none and exits clean. Under the PVC that same crash left the files on a volume the next pod mounted, so this is a genuine difference, not a wash.

Where the finding breaks is "causing duplicate PR attempts on the next run". implement_one resolves prior state from GitHub, not from .status.yaml, before it does anything else — orchestrator/run_implementer.py:1259 is literally commented "Mark in-progress only after GitHub proves there is no prior result", and the preflight above it adopts an existing deterministic result or routes it to needs-triage. That is the 1.20.0 change this template's header names. So the only window that loses anything is: agent pushes a branch, opens a PR, writes implemented, and is then killed before the collector. The next run reads that PR off GitHub and reconciles the status rather than opening a second one.

The residual cost is one wasted attempt in the narrower case where a needs-triage verdict is lost and the proposal is retried instead of quarantined. Bounded by max_proposals: 1 and the operator triage loop, and it does not reach main as a duplicate PR.

The proposed fix is self-defeating and says so in its own parenthetical: archiving agents-state/ natively cannot express deletions, which is exactly why the collector exists (claude P1 on #1046), and it would move the security validation off the producer side. Trading a bounded crash-durability gap for a correctness gap on every successful run is the wrong direction.

Worth being explicit about scope: this property is identical in cwft-mctl-agents-run, -investigate and -shepherd, all merged. It is a property of the emptyDir + artifact design the fleet already adopted (#856), not something this PR introduces. If the durability gap is judged unacceptable, the fix is fleet-wide — an incremental handoff written by run_implementer as it goes, in mctl-agents — not a fifth variant of the collector here.

P3 — no tests for the handoff logic

Fair, and it is the same gap for the three templates already carrying this code. There is no harness in this repo for CWFT shell or for a heredoc'd Python collector; scripts/validate-*.py check manifest shape, not step behaviour. That is why the PR lists mutation of the scope guard as an acceptance criterion rather than claiming a green run proves anything — the guard has to be shown to reject a path outside agents-state while the pre-change template accepts the same payload.

claude[bot]'s P3

Accurate and correctly scoped as non-blocking: the second-layer guard's tr '\0' '\n' | cut -c4- is the rename mis-parse class the Python collector's own comment calls out. Unreachable here because the only paths that reach the checkout came through a validator that already refuses control characters, and both endpoints of any rename would be in scope. It is copied verbatim from cwft-mctl-agents-shepherd.yaml, so fixing it belongs in one pass across both files rather than diverging them in this PR.

---

Gate: claude[bot] APPROVED, 0 P1/P2. Codex hit its usage limit and never read the diff. agy is the non-blocking pilot and both of its findings are answered above.

[mashkovd]

Correcting the gate line at the end of my previous comment, and adding the part of the P2 rebuttal I had not yet verified in code.

The gate on this PR is agy, not claude[bot]

I wrote "agy is the non-blocking pilot". That is the general rule and it is the wrong rule for this class of PR. On gitops#982 — shell inside a ClusterWorkflowTemplate, exactly what this PR is — claude[bot] returned APPROVED, "No P1/P2 findings", five rounds running, while agy found 8 real issues across those same rounds including three P1, two of which were defects introduced by the previous round's fix. claude is reliably blind to the trust boundaries between Argo steps: what reaches git commit, what git status can see, who else can write to /workdir, what survives a cut. It checks pathspecs and syntax and then calls the logic sound without asking who else holds the tree.

So APPROVED, 0 P1/P2 here is not the merge gate. agy's VERDICT: FAIL:P2 is the thing to answer, which is what the previous comment does — and the answer has to stand on reproduction, because agy also produces false findings (two on #982) and neither bot's verdict is taken on trust.

The P2 rebuttal, now verified against the code rather than a comment

Previously I cited the comment at orchestrator/run_implementer.py:1259. Reading the actual path: implement_one calls _preflight_existing_result(ref) before anything else and branches on its verdict — open writes implemented with the PR URL, merged writes merged, closed writes rejected, needs-triage quarantines. Every one of those is derived from GitHub, and GitHubPreflightError fails closed rather than proceeding. Only after all of that does it write in-progress and spend model quota.

And there is a point in the emptyDir design's favour that I missed: on SIGKILL the in-progress claim is lost too. Under the PVC, a crash mid-run still left in-progress on the shared volume for commit-and-push to commit, with an attempt carrying expires_at at +130 minutes — so a killed run could park the proposal in a state that blocks its own retry until that expiry. With the artifact handoff nothing lands at all, main keeps accepted, and the next run re-preflights against GitHub and adopts whatever really happened. For the OOM case specifically that is a better failure mode, not a worse one.

What genuinely regresses is narrower than the finding states: a needs-triage verdict lost to SIGKILL costs one extra attempt instead of quarantining immediately, bounded by max_proposals: 1.

Merge sequencing

Recording this on the PR because it is part of the change, not of the review: the last time this exact port was merged — #676/#677, take-reconcile-off-the-PVC, clean CI and a clean claude review — the first live cron tick after ArgoCD synced failed two production crons and both had to be reverted (#679). CI here validates schema, not behaviour; argo lint does not prove that the executor collects the artifact or that an onExit template resolves a literal S3 key at run time.

So this does not get merged and left alone. After merge: wait for ArgoCD to sync (Argo expands workflowTemplateRef at submission, so a trigger fired too early silently runs the OLD spec), confirm the live ClusterWorkflowTemplate carries emptyDir, then submit one controlled Workflow against it and read that run before anything else is allowed to use the template. mctl-agents-implement's cron is suspended, so the risk is not a tick — it is the DevLoop firing implement on the next approval and becoming the first execution by accident.