fix(argo): take shepherd off its workdir PVC, delete the dead issue-poll by mashkovd · Pull Request #1048 · mctlhq/mctl-gitops

mashkovd · 2026-09-05T07:22:22Z

Completes the port started in #677. After this, implement is the only agent template still declaring a PVC — and it is blocked on something else entirely (#1047).

shepherd — blocker 2 did not need solving on its own terms

This was the one #856 called the genuine blocker: three values the onExit handler reads off the shared volume — activity, shepherd_status, degraded_unfiltered — written by three different steps, with the single usable workflow.outputs.parameters slot already spent on degraded_apps. A second entry sourced from a different step is exactly the bug that broke all three agent CronWorkflows on 2026-07-06 (#530/#531).

The way through is not another output parameter. An onExit template can consume an artifact addressed by a literal S3 key — the same mechanism that solved blocker 1 — so the three values travel as artifacts and the output-parameter limit never comes into play.

Verified on a canary before writing any of the port:

case	onExit saw
producer succeeded, fallback skipped	the producer's value
producer failed, fallback ran	the fallback's value
no producer at all	`unknown` (optional artifact absent, existing shell fallback)

Everything else is the recipe from #1042/#1046, unchanged: one fixed workflow-scoped key written by both attempts, optional: true with the directory built only when there is something to hand off, the reset owned by the collector and run after the agent exits, NUL-split parsing in Python, deletions carried explicitly and applied with :(literal), and a commit step that clones for itself and validates the handoff before any of it touches the checkout. That last part closes #983 P1 for shepherd as well, and lets safe.directory and git reset -q origin/main go with the shared volume.

Two pre-existing defects found while porting

Both fixed in place, because they are in the code this PR rewrites.

set -e made the failure path unreachable. set -e is active from line 1 of run-shepherd, so a bare "$@" that fails aborts the script immediately — RC=$? and the shepherd_status write never run. The write on the failure path has therefore never executed: onExit reported unknown, not failed. The comment directly above it has claimed since PR #613 that capturing the exit code is what lets notify-telegram tell a masked run-shepherd failure from a genuine clean run. It could not.

Confirmed with a one-line probe:

$ printf 'set -e\nfalse\nRC=$?\necho "reached, RC=$RC"\n' | sh ; echo "exit: $?"
exit: 1                      # "reached" never printed

Now wrapped in set +e / set -e, the shape run-orchestrator and run-investigator already use.

commit-and-push declared no resources at all — unbounded on cpu and memory as well as disk.

Verification

Canary with the push redirected to a scratch branch, so main was never touched:

no PVC created;
exactly one .status.yaml committed and pushed;
and the assertion that actually matters, because blocker 2 fails silently: onExit received activity=yes status=succeeded degraded=[app-a app-b] — real values from three different steps, not three unknowns. A naive port would have been green with all three blank.

A first canary run also caught a real mistake before it landed: the emptyDir edit left two volumes: keys in the spec, so YAML's last-wins dropped workdir and commit failed with volume 'workdir' not found. Worth noting that kubectl apply --dry-run=server accepted that happily — a duplicate mapping key is valid YAML. All seven agent templates are now scanned for duplicate keys; none remain.

issue-poll — deleted rather than ported

It has not run once in the archive window, its CronWorkflow has been suspended since Temporal took over intake, and nothing outside docs and old proposals references the template. Porting it would have been work on a template nobody executes.

scripts/validate-local-workdir.py names the agent templates explicitly and runs as a CI gate, so its list is updated in the same commit — otherwise the deletion breaks validate-manifests.

State after this

template	workdir
reconcile, investigate, approve, run, shepherd	emptyDir
implement	PVC 8Gi — #1047
~~issue-poll~~	deleted

🤖 Generated with Claude Code

https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

…ue-poll Completes the port: no agent template except implement declares a PVC now. ## shepherd The hardest of the set, because of what #856 calls blocker 2 — three values the onExit handler reads off the shared volume (`activity`, `shepherd_status`, `degraded_unfiltered`), written by three different steps, with the one usable `workflow.outputs.parameters` slot already spent on `degraded_apps`. A second entry sourced from a different step is the bug that broke all three agent CronWorkflows on 2026-07-06 (#530/#531). Blocker 2 turns out not to need solving on its own terms. An **onExit template can consume an artifact addressed by literal S3 key** — the same mechanism that solved blocker 1 — so the three values travel as artifacts and no additional output parameter is needed at all. Verified on a canary first: resolved with the producing step skipped, with a fallback producing instead, and with no producer at all (falls through to "unknown"). Everything else is the recipe from #1042/#1046: fixed workflow-scoped key written by both attempts, `optional: true` with the directory built only when there is something to hand off, collector-owned reset AFTER the agent exits, NUL-split parsing in Python, deletions carried explicitly and applied with `:(literal)`, and a commit step that clones for itself and validates the handoff before any of it reaches the checkout — which closes #983 P1 here too. `safe.directory` and `git reset -q origin/main` go with the shared volume. Two pre-existing defects found while porting and fixed in place: - `set -e` is active from line 1 of run-shepherd, so the bare `"$@"` aborted the script on failure and `RC=$?` never ran. The shepherd_status write on the failure path has therefore never executed, and onExit reported "unknown" instead of "failed" — while the comment above it has claimed since PR #613 that capturing the exit code is what tells a masked failure from a clean run. Verified with a one-line probe. Now wrapped in `set +e`/`set -e`, the same shape run-orchestrator and run-investigator already use. - commit-and-push declared no resources at all — unbounded on cpu and memory as well as disk. Verified on a canary with the push redirected to a scratch branch: no PVC created, exactly one `.status.yaml` committed, and — the assertion that matters, because blocker 2 fails silently — onExit received `activity=yes status=succeeded degraded=[app-a app-b]`, real values from three different steps rather than three "unknown"s. ## issue-poll, deleted rather than ported It has not run once in the archive window, its CronWorkflow has been suspended since Temporal took over intake, and nothing references the template outside docs and old proposals. Porting it would have been work on a template nobody executes; deleting it removes the last PVC declaration that was not doing anything. `scripts/validate-local-workdir.py` names the agent templates explicitly and is a CI gate, so its list is updated in the same commit — otherwise the deletion breaks validate-manifests. Refs #856, #983 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

chatgpt-codex-connector · 2026-09-05T07:22:27Z

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 · 2026-09-05T07:22:42Z

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

Review complete

Gather context (PR body, changed files)
Review diff 71e5b4e3c6...193dbe5c41e for correctness/safety issues
Check Argo Workflows template/parameter consistency
Check scripts/validate-local-workdir.py update matches deletions
Post inline findings
Submit verdict

Verdict: Changes requested (1 P1, 3 P3)

The overall design — dropping the PVC for emptyDir + per-pod clones, and moving the three onExit-consumed values to S3-keyed artifacts instead of a second workflow.outputs.parameters entry — is sound and matches the reasoning in the PR description, including the verified canary behavior for the artifact fallback cases.

P1 — cwft-mctl-agents-shepherd.yaml:411: in run-shepherd's early fallback-token guard, echo -n "failed" > /tmp/onexit/shepherd_status runs before /tmp/onexit exists — mkdir -p /tmp/onexit only happens later, at line 460. Under set -e (still active at that point), the write fails and the script aborts without ever recording the failure. The shepherd-status artifact is then absent, so notify-telegram falls back to "unknown" instead of "failed" — reproducing, in this one branch, exactly the masked-failure bug this PR's own set -e/set +e fix elsewhere is meant to eliminate. Fix: hoist mkdir -p /tmp/onexit above this guard (e.g. right after set -e on line 402).

P3 nits:

volumeMounts: left as a dangling empty key (no list items) at lines 1015 and 1234, after the workdir mounts were removed from post-deploy-verify and notify-telegram. Harmless (null ≡ omitted) but dead YAML.
Line 1241: stale comment still says "Read off the shared workdir PVC" even though the code below it now reads /onexit/commit/activity from an artifact.
Line 748: the "Pre-create a PVC-backed scratch dir for the implementer subprocess" comment (and the mkdir -p /workdir/clones under it) was copy-pasted into commit-and-push's initContainer, but that pod never runs the implementer subprocess.

scripts/validate-local-workdir.py change (dropping the issue-poll entry) is correct and matches the file deletions.

claude · 2026-09-05T07:26:36Z

              if [ -z "$CLAUDE_CODE_OAUTH_TOKEN" ]; then
                echo "❌ Primary attempt failed and no fallback token (claude-code-oauth-token-2) is set — cannot fall back." >&2
-                echo -n "failed" > /workdir/shepherd_status
+                echo -n "failed" > /tmp/onexit/shepherd_status


P1: this write happens before /tmp/onexit exists. mkdir -p /tmp/onexit is only called later, at line 460 (right before the set +e / "$@" block) — it does not run before this early fallback-token guard. With set -e still active here, echo -n "failed" > /tmp/onexit/shepherd_status fails ("No such file or directory") and aborts the script before shepherd_status is ever written. The optional shepherd-status artifact is then absent, so notify-telegram's || echo "unknown" fallback reports unknown instead of failed" — exactly the class of bug this PR's own commit message (the set -e/set +efix) is trying to eliminate, just relocated to this earlier branch. Fix: move (or duplicate)mkdir -p /tmp/onexitabove this guard, e.g. right afterset -e` on line 402.

claude · 2026-09-05T07:26:44Z

        # global output parameter sourced from a different step broke Argo's static
        # validator workflow-wide (2026-07-06 incident). This step and notify-telegram
        # are different steps, so the same landmine applies here.
        volumeMounts:


P3: volumeMounts: is left as an empty/dangling key (no list items) after the workdir mount was removed. Harmless (K8s treats a null volumeMounts the same as an omitted one) but dead YAML — worth deleting the key entirely rather than leaving a null marker. Same thing at line 1234 in notify-telegram.

claude · 2026-09-05T07:26:51Z

@@ -954,7 +1240,7 @@ spec:
          # hourly tick with no proposal transitions). Read off the shared
          # workdir PVC rather than a workflow output parameter — see the


P3: stale comment — it still says "Read off the shared workdir PVC", but the PVC is gone and the line below now reads /onexit/commit/activity from the artifact-mounted path. Worth updating to match the new artifact-based channel (as was done for the equivalent comments elsewhere in this same diff, e.g. around degraded_apps/shepherd_status).

claude · 2026-09-05T07:27:01Z

+              chmod 600 ~/.ssh/config
+              git clone --depth=1 git@github.com:mctlhq/mctl-gitops.git /workdir/mctl-gitops
+              mkdir -p /workdir/mctl-gitops/platform-gitops/agents-state
+              # Pre-create a PVC-backed scratch dir for the implementer


P3: this comment (and the mkdir -p /workdir/clones below it) is copy-pasted verbatim from run-shepherd's initContainer, but doesn't apply here — commit-and-push never invokes the implementer subprocess, so there's no gh repo clone target that needs this scratch dir. Harmless (just an unused empty directory), but worth trimming so the comment doesn't mislead a future reader about what this pod does.

github-actions · 2026-09-05T07:27:02Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

An analysis of the diff reveals a critical JSON injection vulnerability that can lead to alert suppression and data exfiltration, along with a minor parser desynchronization bug.

Finding 1
Severity: P1
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 1256 (inside the notify-telegram step)

Concrete Failure Scenario:
The notify-telegram step interpolates untrusted workflow parameters (WORKFLOW_SLUG and WORKFLOW_SERVICE) into the TEXT shell variable, which is then unsafely string-interpolated directly into the Telegram API JSON payload (-d "{\"chat_id\":\"${TG_CHAT_ID}\",\"text\":\"${TEXT}\"...").

An attacker or a buggy caller can invoke the workflow with a slug containing double quotes and JSON keys (e.g., slug: 'foo", "chat_id": "ATTACKER_CHAT_ID", "dummy": "'). This payload successfully escapes the "text" field and injects a secondary "chat_id" field. Because standard JSON parsers (including Telegram's) typically honor the last occurrence of a duplicate key, the injected chat ID overrides the intended destination. The API will silently redirect the alert—which contains the internal workflow URL and potentially sensitive degraded app names—to the attacker's chat, while simultaneously suppressing the failure alert for the real operators.

Proposed Fix:
Use jq to safely JSON-encode the payload, mirroring how the incident-reporting leg correctly handles it earlier in the script:

TG_BODY=$(jq -nc --arg chat "$TG_CHAT_ID" --arg text "$TEXT" '{chat_id: $chat, text: $text, disable_web_page_preview: true}')
curl -s -o /dev/null -w 'telegram http=%{http_code}\n' \
  -X POST "https://api.telegram.org/bot${TG_BOT_TOKEN}/sendMessage" \
  -H "Content-Type: application/json" \
  -d "$TG_BODY"

Finding 2
Severity: P3
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 423 (inside the COLLECT Python script)

Concrete Failure Scenario:
The Python COLLECT script parses git status -z output but only checks status[0] (index status) to determine if it should consume the next NUL-separated field as the original path for a rename or copy (if status[0] in ("R", "C"):).

According to Git's porcelain v1 format, if a file is renamed or copied in the working tree but not staged (status[1] is R or C), git status -z still outputs the original path as the very next field. The script will fail to consume this field, desynchronizing the NUL parser. This causes the next loop iteration to treat the original path as a status record, leading to a crash (malformed rename entry) or skipped files. While the agent typically edits .status.yaml files in-place, manual local testing or unexpected agent behavior could trigger this edge case.

Proposed Fix:
Check both the index and working tree status for renames and copies:

if "R" in status or "C" in status:
    if i >= len(fields):
        sys.stderr.write("malformed rename entry: missing original path\n")
        sys.exit(1)
    original = _text(fields[i])
    i += 1
    # A rename removes the old path; a copy leaves it in place.
    if "R" in status:
        deletions.append(original)

claude

Has P1/P2 findings, changes requested: 1 P1 — in run-shepherd's early fallback-token guard (line 411), the shepherd_status write targets /tmp/onexit before that directory is created (mkdir -p /tmp/onexit only runs later, at line 460), so under set -e the write fails and the failure is never recorded — onExit falls back to "unknown" instead of "failed" for exactly this case, undermining the PR's own set -e/set +e fix. Also 3 P3 nits (dead volumeMounts: keys, a stale PVC-era comment, a copy-pasted comment that doesn't apply to its new location).

…to it claude[bot]'s P1 on #1048, and it is the same bug this PR set out to remove, one branch earlier. `mkdir -p /tmp/onexit` sat just above the `set +e` block, but the fallback-token guard runs BEFORE that and writes `failed` into the same directory. With `set -e` active and the directory absent, that write fails, the script aborts, the optional artifact is never produced, and onExit falls through to "unknown" — exactly the outcome the `set +e` fix was added to prevent, for exactly the case that guard exists to report. The mkdir now runs immediately after `set -e`, before any branch that writes. Verified on the cluster by driving the guard directly (fallback attempt with an empty token): with the early mkdir: onExit saw status=failed without it (mutation): onExit saw status=unknown Three P3s from the same review, all correct: - two dangling `volumeMounts:` keys left behind when the workdir mount was removed from post-deploy-verify and notify-telegram; - a comment in notify-telegram still describing the shared PVC where the line below it now reads the artifact mount; - a comment plus `mkdir -p /workdir/clones` copy-pasted into commit-and-push's initContainer from run-shepherd's, describing an implementer subprocess that this pod never invokes. Refs #856 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

mashkovd · 2026-09-05T07:31:05Z

P1 fixed in the latest commit, and it is a fair sting: the same bug this PR set out to remove, relocated one branch earlier.

mkdir -p /tmp/onexit sat just above the set +e block — but the fallback-token guard runs before that and writes failed into the same directory. With set -e active and the directory absent, the write fails, the script aborts, the optional artifact is never produced, and onExit falls through to unknown. Precisely the outcome the set +e fix exists to prevent, for precisely the case that guard exists to report.

The mkdir now runs immediately after set -e, before any branch that writes.

Verified on the cluster, by driving the guard directly (fallback attempt with an empty token):

	onExit saw
with the early mkdir	`status=failed`
without it (mutation)	`status=unknown`

The second row is the point — the fix is what changes the behaviour, not the canary.

All three P3s were also correct and are fixed:

two dangling volumeMounts: keys left behind when the workdir mount was removed from post-deploy-verify and notify-telegram;
a comment in notify-telegram still describing the shared PVC where the line below it now reads the artifact mount;
a comment plus mkdir -p /workdir/clones copy-pasted into commit-and-push's initContainer from run-shepherd's, describing an implementer subprocess this pod never invokes.

claude · 2026-09-05T07:31:28Z

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

Review complete

Gather context (PR body, prior review comments)
Review diff 71e5b4e3c6...68098098 for the fixes applied in the latest commit
Verify the P1 fix (mkdir ordering) and 3 P3 nits are actually resolved
Check for any new issues introduced by the fix commit
Submit verdict

Verdict: Approved (0 P1/P2/P3)

Commit 6809809 correctly addresses everything raised in the prior review round:

P1 (mkdir ordering) — mkdir -p /tmp/onexit now runs immediately after set -e, before the fallback-token guard that writes failed into that directory. Verified the guard can no longer abort on a missing dir under set -e.
P3 — the two dangling volumeMounts: keys (in post-deploy-verify and notify-telegram) are removed entirely rather than left as null markers.
P3 — the stale "shared workdir PVC" comment now correctly describes the artifact-mounted path (/onexit/commit/activity).
P3 — the copy-pasted implementer-subprocess comment + mkdir -p /workdir/clones is removed from commit-and-push's initContainer. Confirmed this didn't regress anything: /workdir/clones is still created in run-shepherd's initContainer, which is the one that actually consumes it via the TMPDIR env var.

No new issues introduced by this commit. scripts/validate-local-workdir.py's issue-poll removal (from the earlier commit) still matches the deleted files.

Formal review submitted via the GitHub Reviews API (APPROVE), bound to commit 680980989d28df28c05c741ada41a60a7de65087.

claude

No P1/P2 findings (0 P3). The mkdir fix is correctly ordered (now runs immediately after set -e, before the fallback-token guard that writes to /tmp/onexit), and all three P3 nits (dangling volumeMounts: keys, stale PVC-era comment, copy-pasted implementer-subprocess comment) are cleanly resolved with no regressions — confirmed /workdir/clones is still created where it's actually needed (run-shepherd's initContainer, used via TMPDIR). Good to merge.

github-actions · 2026-09-05T07:36:34Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Here are the findings from the review of the diff:

Finding 1

Severity: P2
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 880 (post-deploy-verify step)
Concrete failure scenario: When the post-deploy-verify step detects degraded apps after a merge, it waits 120s for a rolling-update grace period before querying the Kubernetes API a second time to confirm. If this second kubectl get applications call fails due to a transient API server error (e.g., network timeout), RAW2 simply defaults to an empty string. Because RAW2 is empty, the script bypasses the BAD2 verification check, incorrectly logs "Apps recovered within grace period", and exits with a success status. This masks real outages, allowing a merged PR that genuinely broke an application to bypass the safeguard without triggering an alert.
Proposed fix: Wrap the post-grace-period kubectl get applications call in the same 3-attempt retry loop that was implemented for the initial check. If it persistently fails after the retries, the step should fail (exit 1) rather than silently defaulting to an empty response that assumes recovery.

Finding 2

Severity: P2
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Approximate Line: 1030 (notify-telegram step)
Concrete failure scenario: The Telegram notification script constructs its JSON payload using raw shell string interpolation: -d "{\"chat_id\":\"${TG_CHAT_ID}\",\"text\":\"${TEXT}\"...}". The TEXT variable includes workflow arguments like WORKFLOW_SLUG and WORKFLOW_SERVICE, as well as the names of any degraded applications. If any of these variables contain double quotes or unescaped backslashes, the resulting JSON becomes malformed. The Telegram API will reject the payload with an HTTP 400 error, and because the curl command is silenced (-s -o /dev/null), the alert will be silently dropped without failing the step or notifying the operator.
Proposed fix: Safely encode the payload using jq -nc --arg text "$TEXT" before passing it to curl, mirroring the secure implementation that existed in the removed issue-poll template.

agy P2 on #1048. Pre-existing, but a two-line fix matching a pattern already present three lines further down the same script. The Telegram leg built its body by shell interpolation: -d "{\"chat_id\":\"${TG_CHAT_ID}\",\"text\":\"${TEXT}\",...}" `$TEXT` carries the proposal slug, the service name and the degraded-app list. A double quote or a backslash in any of them produces malformed JSON, Telegram answers 400, and the alert is dropped — visible only as an `http=400` line in a log nobody reads, with the step still green. The incident leg immediately below has always used `jq -nc --arg`, and so does cwft-mctl-agents-investigate's equivalent, with a comment naming this exact hazard. This leg was the odd one out. agy's other P2 from the same review — a failed post-grace `kubectl` call being reported as "apps recovered" — is real and confirmed, but is filed as #1049 rather than fixed here: the fix changes what a shepherd tick does when the Kubernetes API is briefly unreachable (go green, or fail and raise an incident), which is a decision rather than an encoding bug. Refs #856, #1049 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

mashkovd · 2026-09-05T07:38:43Z

Both agy P2s verified against the code. One fixed here, one filed.

Fixed: Telegram payload built by shell interpolation

Confirmed. The Telegram leg did:

-d "{\"chat_id\":\"${TG_CHAT_ID}\",\"text\":\"${TEXT}\",\"disable_web_page_preview\":true}"

while the incident leg three lines below in the same script uses jq -nc --arg, and cwft-mctl-agents-investigate does too, with a comment naming this exact hazard. This leg was the odd one out. $TEXT carries the slug, the service name and the degraded-app list, so a quote or backslash yields malformed JSON, a 400, and a dropped alert with the step still green.

Now built with jq -nc --arg. Checked the encoding directly:

$ TEXT='has "quotes" and \back and — dash'
$ jq -nc --arg chat 123 --arg text "$TEXT" '{chat_id:$chat,text:$text,disable_web_page_preview:true}'
{"chat_id":"123","text":"has \"quotes\" and \\back and — dash","disable_web_page_preview":true}

Filed, not fixed: the post-grace `kubectl` masking → #1049

Also real, and I confirmed the path line by line:

RAW2=$(kubectl get applications.argoproj.io -n argocd -o json 2>/dev/null) || RAW2=""
BAD2=""
if [ -n "$RAW2" ]; then BAD2=$(… jq …); fi
if [ -n "$BAD2" ]; then …still degraded… fi
echo "✓ Apps recovered within grace period …"

A failed second query leaves RAW2 empty, so BAD2 is never computed and control falls through to the recovery message. Nothing distinguishes "we looked and they are healthy" from "we could not look".

Filed as #1049 rather than fixed inline because the fix is a decision, not an encoding bug: retrying and then exit 1 makes an unverified deploy fail the tick and — through this same onExit path — raise an incident, on a template that runs 8×/day. The issue lays out that trade and flags the neighbouring FRESH="${RAW2:-$RAW}" fallback, which has the same shape and should be decided in the same pass.

Both are pre-existing; this PR only changes where post-deploy-verify writes degraded_unfiltered and how notify-telegram reads its three values.

claude · 2026-09-05T07:39:05Z

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

Review complete

Gather context (PR body, prior review comments, diff)
Review full diff 71e5b4e3c6...13b58a24c6 for correctness/safety issues
Check the new commit's jq-encoding fix for the Telegram payload
Post inline findings
Submit verdict

Verdict: Changes requested (1 P2, 1 P3)

Reviewed the full PR diff (base 71e5b4e3 → head 13b58a24), with focus on what changed in the latest commit — the jq-encoding fix for the shepherd Telegram payload that addresses the JSON-injection finding from the earlier Antigravity review.

P2 — cwft-mctl-agents-shepherd.yaml:1313: the jq fix correctly closes the injection hole, but introduces a formatting regression. TEXT is still built with plain double-quote interpolation (lines 1289, 1294), where \n is two literal characters, not a newline — the old raw-interpolation code relied on that literal \n landing directly in the JSON source text, where it happened to be interpreted as the JSON newline escape. jq --arg treats the input as an opaque string and escapes the literal backslash instead, so Telegram will now render the status line and workflow URL glued together with a visible \n rather than on separate lines. cwft-mctl-agents-investigate.yaml:946 (referenced in this commit's own comment as the pattern being mirrored) avoids exactly this by building TEXT via printf '...\n...' specifically so jq gets a real newline character — this commit's TEXT construction wasn't updated to match.

P3 — cwft-mctl-agents-shepherd.yaml:533: the git status -z rename/copy parser only checks status[0] (the staged/index column). An unstaged rename or copy (worktree column R/C, on by default since Git 2.18 via status.renames) also emits the extra original-path field, and this check won't consume it — desyncing the NUL-split parser for the rest of the output. Low likelihood given the agent mostly edits .status.yaml in place, but fails silently rather than loudly if it happens.

Everything raised in the prior review rounds (mkdir ordering, dangling volumeMounts:, stale comments) was verified as correctly resolved — no regressions found there.

Formal review submitted via the GitHub Reviews API (REQUEST_CHANGES), bound to commit 13b58a24c6fd1952b58dbe81b699888360ea9fa9.

claude · 2026-09-05T07:42:46Z

+            # (agy P2 on gitops#1048). The incident leg below has always built
+            # its body this way, and so does cwft-mctl-agents-investigate; this
+            # leg was the odd one out.
+            TG_BODY=$(jq -nc --arg chat "$TG_CHAT_ID" --arg text "$TEXT" \


P2: this jq fix correctly closes the JSON-injection hole, but it introduces a formatting regression. TEXT is still built with plain double-quote interpolation (TEXT="${ICON} ... (${WORKFLOW_DURATION}s)\n${UI_URL}" at line 1289, and again at line 1294) — inside "..." in bash, \n is two literal characters (backslash + n), not a newline. The old raw-interpolation code relied on that literal \n landing directly in the JSON source text, where it was interpreted as the JSON escape for newline. jq --arg treats the input as an opaque string and properly escapes the literal backslash, producing \\n in the JSON — which Telegram will render as the literal text \n, not a line break. So the status line and the workflow URL (and the post-deploy-verify addendum) will now show glued together with a visible \n instead of on separate lines.

Compare to cwft-mctl-agents-investigate.yaml:946, which builds TEXT via printf '%s ... \n%s' ... specifically so jq gets a real newline character to encode — with the comment "Build with a real newline (printf) so jq encodes it as a proper JSON \n below." This shepherd commit's jq fix should follow the same pattern (build TEXT with printf or $'\n' so the newline is a real control character before it reaches jq).

github-actions · 2026-09-05T07:42:51Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

No significant issues found.

claude · 2026-09-05T07:43:10Z

+                i += 1
+                status = _text(entry[:2])
+                path = _text(entry[3:])
+                if status[0] in ("R", "C"):


P3: the rename/copy check only looks at status[0] (the index/staged column, X). Per git status --porcelain=v1 -z, an unstaged rename or copy (worktree column Y = R/C, X = space) also emits the extra original-path field, and since Git 2.18 unstaged rename detection is on by default (status.renames). if status[0] in ("R", "C") won't consume that extra field in that case, desynchronizing the NUL-split parser for every subsequent entry in this git status output — either a crash further down or silently mis-parsed paths. Should check status[1] (or both) for R/C, not just status[0]. Low likelihood given the agent mostly edits .status.yaml in place, but worth hardening since a desync here fails silently rather than loudly.

claude

Has P1/P2 findings, changes requested: 1 P2. The jq fix (commit 13b58a2) correctly closes the JSON-injection hole in notify-telegram, but $TEXT is still built with plain shell interpolation, so its literal \n markers (meant to be JSON newline escapes, as the old raw-interpolation code relied on) get double-escaped by jq --arg into literal backslash-n text in the Telegram message instead of a line break — cwft-mctl-agents-investigate.yaml's equivalent leg avoids this by building TEXT with printf to get a real newline character before jq encodes it. Also 1 P3 (git status -z rename/copy check only inspects the index column, not the worktree column, for R/C).

…names on both columns Two findings from the review of the previous commit. The first is a regression that commit introduced. P2 — the jq fix broke the line breaks it was meant to protect. `$TEXT` was built as `"… (${WORKFLOW_DURATION}s)\n${UI_URL}"`, a LITERAL backslash-n. That worked only because the payload was assembled by shell interpolation directly into a JSON string literal, where `\n` IS the escape. Encoding the body with jq escapes the backslash, so the message would have shown `\n` as text instead of a line break: TEXT='line1\nline2' -> {"text":"line1\\nline2"} TEXT=$(printf 'line1\nline2') -> {"text":"line1\nline2"} Built with printf now, which is why cwft-mctl-agents-investigate's equivalent leg has always done it that way. P3 — the rename check looked only at the index column. cwft-mctl-agents-run and -investigate were hardened to match both columns earlier in this series; shepherd's collector was written from the pre-hardening source and kept `status[0]`. A plain `mv` produces no rename entry at all (measured: `" D old"` plus `"?? new"`), so the Y column is not reachable in practice — but a missed pair desyncs the NUL walk for every entry after it, and that fails silently rather than loudly. Also adds a check that was missing throughout: the collector is Python inside a heredoc inside a YAML block scalar, so neither `yaml.safe_load` nor `sh -n` nor `kubectl --dry-run=server` can see a syntax error in it. Re-indenting the P3 comment produced exactly such an error, and only extracting and running the collector caught it. All three templates carrying a collector now compile clean, verified explicitly. Collector regression-checked against a scratch worktree after the change: `.status.yaml` edit hands off one file; a stray non-status file is ignored; a deletion is recorded; a `git mv` of a proposal directory yields the new path in `tree/` and the old one in `deleted.lst`. Refs #856 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

mashkovd · 2026-09-05T07:46:41Z

Both fixed in 7a5e06e. The P2 is a regression the previous commit introduced, so thanks for catching it in the same pass.

P2 — the jq fix broke the line breaks it was meant to protect

Exactly right, and I reproduced it:

TEXT='line1\nline2'            ->  {"text":"line1\\nline2"}     # literal \n in the message
TEXT=$(printf 'line1\nline2')  ->  {"text":"line1\nline2"}      # real line break

$TEXT was built as "… (${WORKFLOW_DURATION}s)\n${UI_URL}" — a literal backslash-n. That worked only because the payload used to be interpolated straight into a JSON string literal, where \n is the escape. Encoding with jq escapes the backslash, so the alert would have rendered \n as text.

Built with printf now. Which, as you note, is precisely why cwft-mctl-agents-investigate's equivalent leg has always done it that way — I copied the jq half of that template and not the printf half.

P3 — rename check on one column

Correct and fixed. run and investigate were hardened to match both columns earlier in this series; shepherd's collector was written from the pre-hardening source and kept status[0]. A plain mv produces no rename entry at all (measured: " D old" + "?? new"), so Y is not reachable in practice here — but as you say, a missed pair desyncs the NUL walk for everything after it, and that fails silently.

A gap this exposed in my own checking

The collector is Python, inside a heredoc, inside a YAML block scalar. yaml.safe_load, sh -n and kubectl apply --dry-run=server all pass it happily — none of them can see a Python syntax error in there. Re-indenting the P3 comment produced exactly such an error, and the only thing that caught it was extracting the collector and running it.

All three templates carrying a collector are now compile()-checked explicitly, and shepherd's is regression-tested against a scratch worktree after the change:

case	result
`.status.yaml` edit	one file in `tree/`
stray non-status file alongside	ignored
`.status.yaml` deleted	recorded in `deleted.lst`
`git mv` of a proposal directory	new path in `tree/`, old path in `deleted.lst`

claude · 2026-09-05T07:46:50Z

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

Review complete

Gather context (diff 71e5b4e3c6...7a5e06e0)
Verify the printf/newline fix for the Telegram TEXT build
Verify the rename/copy check now inspects both status columns
Check for regressions vs. prior review rounds
Post findings and submit verdict

Verdict: Approved (0 P1/P2/P3)

Commit 7a5e06e0 addresses both outstanding findings from the previous review round, and only touches cwft-mctl-agents-shepherd.yaml — no other file changed.

P2 (Telegram \n regression) — fixed. TEXT is now built with printf '%s ... (%ss)\n%s' ... (and the degraded-apps addendum with a second printf), so the separator reaching jq --arg is a real newline character rather than the literal two-character \n. jq will now encode it as the proper JSON \n escape, and Telegram will render the status line and URL on separate lines again — matching cwft-mctl-agents-investigate's pattern. Confirmed both %s argument positions are passed as printf arguments (not interpolated into the format string), so there's no injection/format-string hazard from $ICON/$SUBJECT/$WORKFLOW_DEGRADED_APPS containing %.
P3 (rename/copy parser) — fixed. The check is now if "R" in status or "C" in status:, inspecting both the index and worktree columns, with the deletion append correspondingly checking "R" in status. This matches the pattern already used in cwft-mctl-agents-run/-investigate and no longer desyncs the NUL-split walk on an unstaged rename/copy.

No new issues introduced by this commit. All prior rounds' findings (mkdir ordering, dangling volumeMounts:, stale comments, JSON-injection in the Telegram payload) remain correctly resolved.

Formal review submitted via the GitHub Reviews API (APPROVE), bound to commit 7a5e06e07393375fb0cefe093510edc825a13905.

claude

No P1/P2 findings (0 P3). The printf-based TEXT build correctly restores real newlines before jq encoding (previous P2 fixed), and the git-status rename/copy check now inspects both status columns (previous P3 fixed). Good to merge.

github-actions · 2026-09-05T07:50:49Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Severity: P1
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml (line ~335)
Scenario: The COLLECT Python script inside the run-shepherd template introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition. It iterates through all tracked files in a first loop to verify they are not symlinks (os.path.islink) and then uses a second loop to copy them with shutil.copy2. A compromised agent (e.g., via malicious PR content) can stage thousands of .status.yaml files to widen the race window, and use a background script to swap an already-checked .status.yaml file with a symlink pointing to the sensitive /var/run/gh/github-token file. Because shutil.copy2 follows symlinks by default, it will read the token and copy its contents into the artifact. The commit-and-push step will then push this plain-text token to the git repository, resulting in a critical secret leak that bypasses network egress restrictions.
Proposed Fix: Modify the copy command to prevent following symlinks by adding follow_symlinks=False: shutil.copy2(os.path.join(ROOT, rel), dst, follow_symlinks=False). With this fix, if an attacker attempts the race condition, the symlink itself will be copied into the artifact, and the strict find ... ! -type d ! -type f check in the commit-and-push step will safely catch it and fail the workflow.

agy P1 on #1048, and the chain checks out end to end. The collector classifies paths in one pass (`os.path.islink` → skip) and copies them in a second. The SDK agent can leave a background process alive past its own exit, so a path checked as a regular file can be a symlink by the time it is copied — and `shutil.copy2` DEREFERENCES by default. What that costs is not a broken file. It is a secret leak with no downstream guard: the copy reads the target — the GitHub token is mounted at /var/run/gh in the same pod — and writes its CONTENTS into the handoff as an ordinary file. commit-and-push's `find ! -type d ! -type f` check sees a perfectly ordinary file at a perfectly valid `.status.yaml` path, so it passes, and the token is committed and pushed to main. Measured, both halves: copy2(link, dst) -> dst is a regular file containing "SECRET-TOKEN" copy2(link, dst, follow_symlinks=False) -> dst is a symlink and then, with the fix, driving the swap through to the consumer guard: the artifact holds a symlink, and `find /artifact ! -type d ! -type f` finds it, so the step fails and nothing is pushed. Applied to all three collectors — shepherd, run and investigate carry the same code. Refs #856, #983 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KofQCrsXaJ67mJuQqiFHN1

mashkovd · 2026-09-05T07:53:25Z

agy's P1 is correct, the chain checks out end to end, and it is the most serious finding on this PR series. Fixed in 3c0d16b, in all three collectors.

Why it is a leak and not a broken file

The collector classifies paths in one pass (os.path.islink → skip) and copies them in a second. The SDK agent can leave a background process alive past its own exit, so a path checked as a regular file can be a symlink by the time it is copied — and shutil.copy2 dereferences by default.

The part that makes it P1 is what happens downstream. The copy reads the target — the GitHub token is mounted at /var/run/gh in that same pod — and writes its contents into the handoff as an ordinary file. commit-and-push's find /artifact ! -type d ! -type f then sees a perfectly ordinary file at a perfectly valid .status.yaml path. It passes. The token is committed and pushed to main.

So the guard I added specifically to catch smuggled non-regular files is blind to this by construction, because by the time it runs there is nothing non-regular left to see.

Measured

copy2(link, dst)                        -> dst is a regular file containing "SECRET-TOKEN"
copy2(link, dst, follow_symlinks=False) -> dst is a symlink

And driving the swap through to the consumer guard with the fix in place:

in the artifact:  symlink -> /tmp/fake-token
consumer guard:   find /artifact ! -type d ! -type f  ->  hit, step fails, nothing pushed

Applied to shepherd, run and investigate — they carry the same collector.

Worth stating plainly, since it is the sixth finding of the same shape across this series: every guard I wrote in these templates has been correct about the thing it names and blind to the adjacent path into the same outcome. Newline in a pathspec, a deletion that never travels, a glob in git rm, a mkdir after the first write — and now a check that runs one pass too early. The pattern is that I verify the mechanism I am thinking about and not the one next to it.

claude · 2026-09-05T07:53:34Z

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

Reviewing PR #1048 (commit `3c0d16bf`)

Gather context (PR body, prior review rounds, custom instructions)
git diff 71e5b4e3c693478554c861daa4f079e422932cbf...3c0d16bfe4812e299aedf0c1d3ab98ef724efd9e
Focus on the latest commit (symlink-dereference fix) and re-check nothing prior regressed
Post inline findings (none — see below)
Submit verdict

Review complete

This round covers commit 3c0d16bf, which fixes the agy P1 (TOCTOU symlink → secret leak) from the prior round: the collector's os.path.islink() check runs in a classification pass, then shutil.copy2() runs in a separate copy pass — a symlink swapped in between (e.g. by a leftover background process from the SDK agent) would previously be dereferenced, copying the contents of whatever it points to (e.g. the GitHub token at /var/run/gh) into the handoff as an ordinary file.

Verified the fix end-to-end:

shutil.copy2(..., follow_symlinks=False) is now applied identically in all three collectors (shepherd, run, investigate — cwft-mctl-agents-shepherd.yaml:586, cwft-mctl-agents-run.yaml, cwft-mctl-agents-investigate.yaml). With follow_symlinks=False, copy2 recreates the symlink itself via os.symlink rather than reading through it — this closes the race regardless of when the swap happens, not just at the moment of the original islink() check.
Confirmed the downstream consumer guard actually catches a symlink that slips through: commit-and-push's find /artifact -mindepth 1 ! -type d ! -type f (shepherd.yaml:865, and the equivalent in run/investigate) uses find's default lstat semantics, so a symlink has type l, fails ! -type f, and the step aborts before anything is pushed — matching the author's on-cluster verification in the PR thread.
No regression risk from the flag: for non-symlink sources follow_symlinks=False is a no-op, and shutil.copystat's permission step silently skips chmod on symlinks when os.lchmod isn't available (Linux), so no new exception path.

Also re-confirmed nothing from earlier rounds regressed: commit-and-push still has explicit resources.requests/limits (cpu/memory/ephemeral-storage) at both call sites, the mkdir -p /tmp/onexit ordering fix is intact, the Telegram payload is still built via printf + jq -nc --arg, and the rename/copy parser still checks both status columns.

No P1/P2/P3 findings on this commit.

gh api repos/mctlhq/mctl-gitops/pulls/1048/reviews --method POST -f commit_id="3c0d16bfe4812e299aedf0c1d3ab98ef724efd9e" -f event="APPROVE" -f body="No P1/P2 findings (0 P3). The symlink-dereference fix (follow_symlinks=False) correctly closes the prior P1 across all three collectors, and I verified the downstream find-based guard catches a symlink that slips through. Good to merge."

claude

No P1/P2 findings (0 P3). The symlink-dereference fix (follow_symlinks=False) correctly closes the prior P1 across all three collectors, and I verified the downstream find-based guard catches a symlink that slips through. Good to merge.

github-actions · 2026-09-05T07:58:26Z

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

A review of the diff reveals a correctness/error-handling bug in the new post-deploy-verify step that can mask genuine production breakages.

Finding 1

Severity: P2
File: platform-gitops/argo-workflows/cluster-templates/cwft-mctl-agents-shepherd.yaml
Line: ~1323 (inside the post-deploy-verify script)
Scenario: The script checks for Degraded apps and, if found, waits 120 seconds before performing a second check (RAW2) to confirm they haven't recovered. However, the second fetch lacks the 3× retry loop and error handling that the first fetch has. If kubectl get applications.argoproj.io fails due to a transient API server error or network blip, RAW2 is set to "". The script then bypasses the jq parsing (if [ -n "$RAW2" ]), leaves BAD2 empty, and incorrectly echoes ✓ Apps recovered within grace period. The workflow exits 0 (Success), completely dropping the alert and masking genuine downstream breakages caused by the merged PR.
Fix: Implement the same retry-and-backoff loop for the RAW2 fetch as is used for the initial RAW fetch. At a minimum, fail the step explicitly if RAW2 evaluates to empty rather than absorbing it as a successful recovery.

mashkovd · 2026-09-05T07:59:14Z

agy's latest P2 is a repeat of the RAW2 masking finding from its earlier round. It is real — I verified the path line by line — and it is tracked as #1049, not dismissed.

It is deliberately not fixed here for two reasons stated in that issue:

It is pre-existing. This PR changes only where post-deploy-verify writes degraded_unfiltered (a file on the PVC, now an artifact). The RAW2 logic is untouched and predates the change.
The fix is a decision, not an encoding bug. Retrying and then exit 1 makes an unverified deploy fail the tick and — through this same onExit path — raise an incident, on a template that runs 8×/day. shepherd: a failed post-grace kubectl call is reported as "apps recovered" #1049 lays out that trade and flags the neighbouring FRESH="${RAW2:-$RAW}" fallback, which has the same shape and should be decided in the same pass.

Merge state: claude[bot] APPROVED on the current head (3c0d16b), all checks green, codex never reviewed (usage limit). Zero unaddressed P1/P2 — the one open P2 is filed with its reasoning.

claude Bot reviewed Sep 5, 2026

View reviewed changes

claude Bot requested changes Sep 5, 2026

View reviewed changes

claude Bot approved these changes Sep 5, 2026

View reviewed changes

mashkovd mentioned this pull request Sep 5, 2026

shepherd: a failed post-grace kubectl call is reported as "apps recovered" #1049

Open

claude Bot reviewed Sep 5, 2026

View reviewed changes

claude Bot requested changes Sep 5, 2026

View reviewed changes

claude Bot approved these changes Sep 5, 2026

View reviewed changes

mashkovd merged commit a37d603 into main Sep 5, 2026
15 checks passed

mashkovd deleted the fix/gitops-856-shepherd-emptydir branch September 5, 2026 07:59

		@@ -954,7 +1240,7 @@ spec:
		# hourly tick with no proposal transitions). Read off the shared
		# workdir PVC rather than a workflow output parameter — see the

Conversation

mashkovd commented Sep 5, 2026

shepherd — blocker 2 did not need solving on its own terms

Two pre-existing defects found while porting

Verification

issue-poll — deleted rather than ported

State after this

Uh oh!

chatgpt-codex-connector Bot commented Sep 5, 2026

Uh oh!

claude Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 5, 2026

Antigravity (agy) review — informational

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mashkovd commented Sep 5, 2026

Uh oh!

claude Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 5, 2026

Antigravity (agy) review — informational

Uh oh!

mashkovd commented Sep 5, 2026

Fixed: Telegram payload built by shell interpolation

Filed, not fixed: the post-grace kubectl masking → #1049

Uh oh!

claude Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 5, 2026

Antigravity (agy) review — informational

Uh oh!

claude Bot Sep 5, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

mashkovd commented Sep 5, 2026

P2 — the jq fix broke the line breaks it was meant to protect

P3 — rename check on one column

A gap this exposed in my own checking

Uh oh!

claude Bot commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 5, 2026

Antigravity (agy) review — informational

Uh oh!

mashkovd commented Sep 5, 2026

claude Bot commented Sep 5, 2026 •

edited

Loading

claude Bot commented Sep 5, 2026 •

edited

Loading

Filed, not fixed: the post-grace `kubectl` masking → #1049

claude Bot commented Sep 5, 2026 •

edited

Loading

claude Bot commented Sep 5, 2026 •

edited

Loading

claude Bot commented Sep 5, 2026 •

edited

Loading

Reviewing PR #1048 (commit `3c0d16bf`)