Repository navigation
feat(shepherd): post-deploy ArgoCD verify after merge - #143
Conversation
After the shepherd commits .status.yaml transitions back to gitops main (merge / rejected / review-fixing), pause 300s for ArgoCD to reconcile and then list any Application that flipped to Health=Degraded since the workflow started. A non-empty result fails the workflow, which the existing notify-telegram onExit handler turns into a Telegram alert naming the affected apps. Closes the gap exposed by the external-secrets incident 2026-05-01 → 2026-05-07: the eso-cve-patch implementer-driven merge broke a downstream Application and the shepherd had no signal to halt its run or alert anyone — the breakage stayed Degraded for 6 days. See ~/.claude/plans/streamed-soaring-fox.md for full context. Threshold = workflow.creationTimestamp so that pre-existing Degraded apps (carried over from previous incidents) don't false-positive every shepherd run. argo-workflow-sa already has applications: get/list/patch in platform-gitops/argo-workflows/config/sa-and-rbac.yaml — no RBAC change needed. Auto-revert is intentionally NOT in this PR. v2 follow-up will need commit-SHA → owning-Application correlation; for now the Telegram alert lands and an operator decides.
|
@codex review |
![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: ffc09a46ae
ℹ️ 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".
| if [ "$RC" -ne 0 ] || [ -z "$RAW" ]; then | ||
| echo "⚠️ kubectl get applications failed (rc=$RC); skipping verify rather than failing run." | ||
| exit 0 |
There was a problem hiding this comment.
This branch exits 0 for any kubectl get applications.argoproj.io failure, not just transient API hiccups. In cases like missing RBAC, missing CRD, or broken cluster auth, the post-deploy safeguard is silently bypassed on every run, so merges keep succeeding without the intended degraded-app check. The workflow should fail (or at least explicitly alert) on persistent query failures and only suppress clearly transient errors.
Useful? React with 👍 / 👎.
| echo "❌ Newly Degraded ArgoCD apps after shepherd run: $BAD" | ||
| echo "Threshold (workflow.creationTimestamp): $THRESHOLD" | ||
| exit 1 |
There was a problem hiding this comment.
The degraded application list is only echoed in this step and never exported as a workflow output, so the notify-telegram onExit template cannot include the affected app names. Because onExit runs in a separate pod and currently sends only status/subject/duration/URL, operators won't receive the app list in the alert even when verify fails, which undermines the stated goal of naming impacted apps in notifications.
Useful? React with 👍 / 👎.
Codex review on PR #143 surfaced two findings, both addressed here: P1 — non-transient kubectl failures were silently bypassed. The previous `set +e` + `exit 0` fallback treated EVERY kubectl error as transient, so missing RBAC, missing CRD, or broken cluster auth would silently disable the safeguard on every shepherd run. Replace with a 3× retry loop with 5s backoff: a single transient hiccup is absorbed, but persistent failures escalate to exit 1 so the operator notices the safeguard itself is broken. P2 — affected app names weren't reaching the Telegram alert. notify-telegram runs in a separate pod (onExit) and only sees status/subject/duration/URL. The verify step's degraded list was echoed to logs but never exported, so operators had to dig through workflow logs to find which apps the shepherd had broken — defeats the stated goal of naming impacted apps in alerts. Fix: write the degraded list to /tmp/degraded.txt, declare it as a step output parameter, hoist it to a workflow-level output via the shepherd-proposals template, then read it as {{workflow.outputs.parameters.degraded_apps}} in notify-telegram and append a "⚠️ post-deploy-verify flagged: …" line when non-empty. Empty case (clean run / verify never ran) keeps the existing alert shape unchanged.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3294e0741
ℹ️ 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".
| RC=1 | ||
| for attempt in 1 2 3; do | ||
| set +e | ||
| RAW=$(kubectl get applications.argoproj.io -A -o json 2>/dev/null) |
There was a problem hiding this comment.
kubectl get applications.argoproj.io -A requests a cluster-wide list, but this workflow’s service account is only bound via a namespaced RoleBinding in argocd (platform-gitops/argo-workflows/config/sa-and-rbac.yaml, lines 76-100). With that RBAC, the call will be forbidden, the 3 retries will all fail, and post-deploy-verify exits 1 on every run even when nothing actually degraded. Use -n argocd (if all Applications live there) or grant a ClusterRole/ClusterRoleBinding for cluster-wide listing.
Useful? React with 👍 / 👎.
Codex P1 on PR #143 second pass: argo-workflow-sa is bound via a namespaced RoleBinding in the argocd namespace (platform-gitops/argo-workflows/config/sa-and-rbac.yaml lines 73-100), NOT a ClusterRoleBinding. A cluster-wide `kubectl get applications.argoproj.io -A` would 403 on every attempt — all 3 retries fail, the persistent-failure branch fires, and the step exits 1 on every shepherd run regardless of actual cluster health, exactly the silent-bypass anti-pattern the previous fix was meant to eliminate. All ArgoCD Applications live in the `argocd` namespace, so scoping the kubectl call to `-n argocd` matches the existing RBAC without needing to widen the binding.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47589a4698
ℹ️ 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".
| # plus a buffer for the per-resource health re-evaluation pass | ||
| # that runs on the next tick after a sync. | ||
| echo "Sleeping 300s for ArgoCD to reconcile post-merge…" | ||
| sleep 300 |
There was a problem hiding this comment.
This new sleep 300 runs after commit-and-push, but the workflow still holds the top-level synchronization.mutex (mctl-gitops-main-writes) until completion, so every shepherd run now blocks other workflows that share that mutex for an extra 5 minutes even though no Git write is happening. That can unnecessarily queue run/implement workflows and slow down merge/fix cycles; the verify logic should run outside the global write lock (or use a narrower lock scope around commit/push only).
Useful? React with 👍 / 👎.
| BAD=$(printf '%s' "$RAW" | jq -r --arg ts "$THRESHOLD" ' | ||
| .items[] | ||
| | select(.status.health.status == "Degraded") | ||
| | select((.status.health.lastTransitionTime // "") > $ts) |
There was a problem hiding this comment.
The degraded-app filter uses lexical string comparison for RFC3339 timestamps, which can misorder valid timestamps when one value includes fractional seconds and the other does not (for example, ...:30.5Z sorts before ...:30Z). In that case, apps that transitioned after workflow start can be missed and the safeguard silently fails; parse both values to epoch time (e.g., with fromdateiso8601) before comparing.
Useful? React with 👍 / 👎.
Codex P2 ×2 on PR #143 third pass: 1. Mutex held during 300s sleep blocked unrelated workflows. mctl-gitops-main-writes was at workflow spec-level so the entire shepherd run (clone → shepherd → commit-push → 300s verify sleep) held the global lock. cwft-mctl-agents-run and -implement share this mutex and would queue for the full 1-2 min duration of every shepherd tick, even though only commit-and-push actually writes to gitops main. Hoist the mutex to the commit-and-push template only — race-against-other-writers is still handled there by the existing 5×retry-with-rebase loop. 2. Lexical RFC3339 comparison breaks on fractional seconds. `(.status.health.lastTransitionTime // "") > $ts` is bytewise string comparison. "...:30.5Z" sorts BEFORE "...:30Z" because "." (0x2E) < "Z" (0x5A). Apps that transitioned just after workflow start with sub-second precision would slip through the filter. Replace with fromdateiso8601 epoch comparison; null / empty lastTransitionTime defaults to 1970-01-01 so unparsed apps never match the threshold.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Why
The external-secrets incident (2026-05-01 → 2026-05-07): `eso-cve-patch` implementer-driven merge broke a downstream Application; the shepherd happily flipped it to `merged` and the breakage went unnoticed for 6 days. Without this verify step the shepherd is a one-way door that can't tell when its merge wedged the cluster. See plan `~/.claude/plans/streamed-soaring-fox.md`.
Design notes
Test plan