Repository navigation
feat(argo): record an approver on an already-accepted proposal, and stop reading exit 45 as quota - #1209
feat(argo): record an approver on an already-accepted proposal, and stop reading exit 45 as quota#1209mashkovd wants to merge 10 commits into
Conversation
…top reading exit 45 as quota Closes gitops#1206. Both halves are the CWFT side of mctl-agents#349, which could only fix the mctl-agents side: it made the refusal visible (a `blocked` outcome, a durable marker and EXIT_BLOCKED_ONLY=45) but could not make the proposal approvable, because the flip lives here. (a) cwft-mctl-agents-approve.yaml — flip-status `status: accepted` was an unconditional no-op. That left one shape permanently stuck: a proposal committed straight to `accepted` while carrying `control.requires_human_approval` and no `approval` block can never become runnable, because this step is the only writer of `approval.approved_by` and it declined to run, while the implementer refuses without that record. Both sides exited 0 and reported success. The branch now distinguishes three cases and the step's result grows a third value, `recorded`, alongside `flipped` and `noop`; the step-list `when` compares against `noop` rather than `== flipped` so both writing outcomes reach commit. Deliberately narrow. It only ever ADDS an approval block where one is demanded and absent: it never edits an existing approval, never touches `status`, and treats a missing `control` block as "never asked" so the incident-responder shape keeps no-op'ing. The falsey spellings mirror mctl-agents' _FALSEY, and anything unrecognised requires approval rather than waiving it. An anonymous approver is refused rather than written. `unknown`/`none`/`null` are in mctl-agents' _ANONYMOUS_APPROVERS, so recording one would commit a record that looks like an approval and leaves the proposal exactly as blocked. (b) cwft-mctl-agents-implement.yaml — fallback gate and assert-attempt Neither gate looked at an exit code; both compared Argo step status strings. So exit 45 made `implement` Failed like any other non-zero exit, which fired the account-2 fallback to reach the identical refusal a second time, and then had assert-attempt blame the Claude usage limit for a proposal nobody had approved. `implement-fallback` now also requires the exit code not to be 45, compared as a string so a pod-level Error carrying no exit code still evaluates instead of failing to resolve. assert-attempt takes both exit codes and reports the blocked condition with its real remedy, keeping the quota text for every other failure. Verified, not assumed flip-status was extracted and run in alpine/git:2.43.0, the image the step actually uses (macOS sed is not a stand-in -- BSD `sed -i` needs an argument): proposed + gated, no approval -> flipped accepted + gated, no approval -> recorded accepted + gated: "true" (quoted) -> recorded accepted + gated, approval present -> noop accepted, no control block -> noop accepted + requires_...: false -> noop accepted + gated, approver=unknown -> exit 1 implemented -> exit 1 The recorded file parses as YAML, `approved_by` satisfies the gate, `status` is untouched and the handoff artifact lands at the right repo-relative path. Mutation-proven: deleting the existing-approval guard turns the noop case into `recorded`, which is the overwrite this must never do. assert-attempt was exercised the same way: success exits 0; exit 45 on either attempt gives the blocked message; a real failure keeps the quota message; a pod-level Error with no exit code falls through to the generic branch. Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 4m 34s —— View job Review of PR #1209 complete
Verdict: Changes requested (1 P2, 1 P3) — review P2 — P3 — Everything else checks out: the |
…he baseline CI caught this, correctly: the first version interpolated the two new parameters straight into the script text, which is the gitops#992 hazard -- Argo substitutes a parameter BEFORE the interpreter parses the line, so a value carrying a quote and a semicolon executes. Rather than bind only the two new ones, all four in cwft-mctl-agents-implement.yaml's assert-attempt now come through env. That retires primary/fallback's two BASELINE entries instead of adding to them, so the ratchet moves down: 32 known sites -> 30. The BASELINE comprehension drops this file and says why, since a baseline that outlives its sites is itself a failure the detector reports. These four are step statuses and exit codes rather than user input, which is the reasoning that put primary/fallback in the "origin not traced" half of the baseline to begin with. "Looks internal" is exactly the assumption that lets one of them turn out not to be. Re-verified after the change, same table as before plus one new row: an exit-45 run reports blocked, a real failure keeps the quota text, a pod-level Error carrying no exit code still falls through cleanly, and a primary status of `Failed'; touch /tmp/PWNED; echo '` no longer executes -- which it would have in the interpolated version. Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 5m 45s —— View job Reviewing PR #1209
Findings(a)
(b) (c) I couldn't execute Verdict: ✅ Approved — no P1/P2 findings, 2 P3 nits (non-blocking). |
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Severity: P2 Severity: P3 Severity: P3 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - name: primaryExit | ||
| value: "{{steps.implement.exitCode}}" | ||
| - name: fallbackExit | ||
| value: "{{steps.implement-fallback.exitCode}}" |
There was a problem hiding this comment.
P2: assert-produced unconditionally references {{steps.implement-fallback.exitCode}}, even on the common happy path where implement succeeds and implement-fallback is skipped by its when. Unlike .status (a top-level NodeStatus field, proven safe on skipped steps by the identical pattern already live in cwft-mctl-agents-run/-investigate/-shepherd), .exitCode lives under Outputs, which is typically nil/unset on a Skipped node — a known class of Argo resolution gotcha for outputs.*-nested fields (as opposed to top-level fields like status) that this repo has no prior working example of. If it fails to resolve, assert-produced breaks on every green run, not just the exit-45 path. The PR description's own 'Risk, stated plainly' section flags this as unverifiable locally — worth confirming with a live tick (per this repo's own CWFT-change convention) before treating it as settled, given it can regress the common case rather than only the new one.
There was a problem hiding this comment.
Both fixed in ad6ee314, and the P2 is now settled by measurement rather than left for a live tick.
P2 — confirmed, and worse than "unproven"
You were right, and it was checkable without touching the template. Reading the archived workflows in the cluster:
implement Succeeded outputs.exitCode: 0 (present)
implement-fallback Skipped outputs: null (absent entirely)
Checked the other direction across every workflow still held, to establish the rule rather than one data point: a node that actually ran carries outputs.exitCode whether it Succeeded or Failed (mctl-agents-investigate-c107fab1 has three Failed pod nodes, all with exitCode: 1 present). So it is "ran, therefore present" — and Skipped is exactly the case with nothing under Outputs.
Since implement-fallback is Skipped on every green run, the reference would have risked the common path to describe a rare one. Dropped entirely — assert-produced now passes only {{steps.implement.exitCode}}, and implement always runs.
What that gives up, stated so it is not rediscovered later: implement fails for some other reason and the fallback is the one that hits 45. The primary's real failure is then the more important signal, and the generic message is the right one anyway.
The when on implement-fallback still reads {{steps.implement.exitCode}} — same rule, implement always runs.
P3 — accepted, and it was sharper than "nice-to-have"
The part that makes it more than pre-existing behaviour is the one you named: this PR adds never edit an existing approval, which turns a written approved_by: 'unknown' from "wrong but fixable by approving again" into permanently unrecoverable — every later approve noops on it forever. Closing one door while making what comes through the other one permanent is worse than leaving both open.
NEEDS_APPROVAL is now computed before the status branch, and the refusal guards both modes.
Re-verified, eleven rows, in alpine/git:2.43.0
| on disk | approver | result |
|---|---|---|
| proposed + gated | named | flipped |
| proposed + ungated | named | flipped |
| proposed + gated | unknown | exit 1 — new |
| proposed + ungated | unknown | flipped — must not break, and does not |
| accepted + gated, no approval | named | recorded |
accepted + gated "true" |
named | recorded |
| accepted + gated, approval present | named | noop |
| accepted, no control block | named | noop |
accepted + requires_...: false |
named | noop |
| accepted + gated | unknown | exit 1 |
| implemented | named | exit 1 |
The fourth row is the one that matters for your P3: an ungated proposal may still be approved anonymously exactly as before, so the DevLoop's default approver is only refused where it would actually create a stuck state.
assert-attempt re-checked after the parameter was dropped: success exits 0, exit 45 gives the blocked message, a real failure keeps the quota text, and a pod-level Error carrying no exit code still falls through cleanly.
validate-shell-param-interpolation.py stays green at 30 known sites.
| echo "❌ ${WORKFLOW_SLUG} is already accepted and needs a named approver, but the caller supplied '${APPROVER_SAFE}'." >&2 | ||
| echo " An anonymous approver does not satisfy the gate — the proposal would stay blocked while looking approved." >&2 | ||
| exit 1 ;; | ||
| esac |
There was a problem hiding this comment.
P3: the new anonymous-approver refusal (unknown|none|null) only guards MODE=record. A first-time flip of a gated proposal (CURRENT=proposed) where the caller leaves approver at its default "unknown" still writes approval.approved_by: 'unknown' unchecked at line 318, landing in the same 'looks approved but the gate stays unsatisfied' state — except now it's unrecoverable, because the sibling invariant this PR adds ('never edit an existing approval') means a later approve call will just noop on it forever. Pre-existing behavior, not introduced here, but since this PR is specifically closing that class of permanently-stuck state, it may be worth applying the same anonymous check to the flip path too (at least when gated) rather than leaving one door open while closing the other.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 1 P3. The P2 is the unproven risk the PR itself calls out: assert-produced (cwft-mctl-agents-implement.yaml) unconditionally reads {{steps.implement-fallback.exitCode}}, which is skipped (and thus lacks a populated Outputs) on every plain successful run — the common case, not just the exit-45 one. .status is proven safe on a skipped step elsewhere in this repo; .exitCode under Outputs is not. Worth a live-tick confirmation (per this repo's own convention for CWFT changes) before merging, given a resolution failure here would break the happy path. The P3 (anonymous-approver check only covers the new record mode, not the pre-existing flip mode) is a nice-to-have, not a regression. Both files parse correctly (yamllint/Validate Manifests already green) and the rest of the logic — the noop/flipped/recorded three-way split, the falsey/anonymous handling, the string-quoted exit-code comparisons — checks out against the PR's own verification table.
…ous approver on both paths
Two review findings, both confirmed against the cluster rather than argued.
P2 -- exitCode on a Skipped node
assert-produced passed {{steps.implement-fallback.exitCode}} unconditionally,
and implement-fallback is Skipped on every green run. Measured in-cluster:
implement Succeeded outputs.exitCode: 0 (present)
implement-fallback Skipped outputs: null (absent entirely)
Checked the other direction too, across every workflow still held: a node that
actually RAN carries outputs.exitCode whether it Succeeded or Failed. So the
rule is "ran, therefore present" -- and the reviewer is right that referencing
a skipped step's exitCode risks the common path in order to describe a rare
one. `implement` always runs, so only its exit code is read now.
What that gives up: implement fails for some other reason and the FALLBACK is
the one that hits 45. The primary's real failure is then the more important
signal and the generic message is the right one anyway.
P3 -- an anonymous approver could still be written on the flip path
The refusal only guarded MODE=record. A first-time approve of a gated proposal
with the default approver still wrote approval.approved_by: 'unknown', which
does not satisfy the gate -- and the invariant this PR adds, never edit an
existing approval, makes that state UNRECOVERABLE: every later approve noops on
it forever. This PR would have closed one door while leaving the other open,
and made what came through it permanent.
NEEDS_APPROVAL is now computed before the status branch and the refusal guards
both modes. An ungated proposal is unaffected and may still be approved
anonymously, which the table below checks explicitly rather than assuming.
Re-verified in alpine/git:2.43.0, eleven rows:
proposed + gated, approver named -> flipped
proposed + ungated, approver named -> flipped
proposed + gated, approver unknown -> exit 1 (new)
proposed + ungated, approver unknown -> flipped (must NOT break)
accepted + gated, no approval -> recorded
accepted + gated: "true" quoted -> recorded
accepted + gated, approval present -> noop
accepted, no control block -> noop
accepted + requires_...: false -> noop
accepted + gated, approver unknown -> exit 1
implemented -> exit 1
assert-attempt re-checked after dropping the parameter: success exits 0, exit
45 gives the blocked message, a real failure keeps the quota text, and a
pod-level Error with no exit code still falls through cleanly.
Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 4m 31s —— View job Review of PR #1209 complete
Verdict: Approved (0 P1/P2, 2 P3) — review This commit (
P3 nits (inline, non-blocking):
Everything else checks out: the |
![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: a310806ad4
ℹ️ 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".
| - name: fallbackExit | ||
| value: "{{steps.implement-fallback.exitCode}}" |
There was a problem hiding this comment.
Whenever the primary attempt succeeds, or exits 45 on the new blocked path, implement-fallback is skipped and therefore has no exit code to interpolate here. The same template explicitly notes at lines 260-263 that references to skipped steps do not resolve, so assert-produced cannot start; this turns normal successful runs and the intended blocked-only report into workflow errors. Supply a literal/default value when the fallback was skipped instead of referencing its exit code unconditionally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All five addressed in 2325ae77. Four applied; the P1 was already fixed before this review ran.
P1 — reading the skipped fallback's exitCode: already gone
These comments are anchored to a310806a, the branch's first commit. The current head is ad6ee314 (now 2325ae77), and that reference was removed there:
$ git show origin/feat/cwft-approve-record-and-exit45:...cwft-mctl-agents-implement.yaml \
| grep -n 'implement-fallback.exitCode\|fallbackExit'
(no output)
The reasoning was the same as yours, and it was settled by measurement rather than argument — from the archived workflows in the cluster:
implement Succeeded outputs.exitCode: 0 (present)
implement-fallback Skipped outputs: null (absent entirely)
assert-produced takes only implement's exit code now.
P2 — the when could fail to resolve on a pod-level Error: applied, and it was the sharpest of the five
This one I had not closed. implement always runs, and the same archive shows a node that ran carries outputs.exitCode whether it Succeeded or Failed (mctl-agents-investigate-c107fab1 has three Failed pod nodes, all with exitCode: 1). The pod-level Error case is the one nothing in the cluster could be measured against — and you are right that quoting a plain tag does not manufacture a value, it still has to resolve.
Both tags are now expression tags with an explicit default, which is what you asked for:
when: "{{=steps['implement'].status != 'Succeeded' && (steps['implement'].exitCode ?? '0') != '45'}}"
value: "{{=steps['implement'].exitCode ?? ''}}"Argo in this cluster is v3.7.11, so expr tags are available. ?? '0' on the gate means the fallback still fires on the error path continueOn.error covers.
P2 — explicit YAML null: applied
Correct, and a genuine parity break. This is a text parser standing in for a YAML one: Python's control.get("requires_human_approval") returns None for :, : null and : ~ alike, and None means "never asked" → satisfied. The shell saw the literal null / ~, fell to the default branch, and would have written an approval record onto a proposal that never wanted one.
Both spellings join the falsey case. Verified in alpine/git:2.43.0:
| fixture | result |
|---|---|
requires_human_approval: null |
noop |
requires_human_approval: ~ |
noop |
requires_human_approval: (empty) |
noop |
P2 — concurrent approves overwriting each other: applied
Correct, and it broke this PR's own stated invariant. flip-status evaluates "no approval here" outside any mutex and hands over a whole-file artifact; the second run to take the mutex would replace the first's approver and timestamp.
The check is now repeated in commit-and-push, under the mutex, against the freshly cloned tree — the only place it is serialized:
| checkout | handoff | result |
|---|---|---|
| approved by alice | approved by bob | keeps alice, exit 0 — the concurrent run did this run's job |
| no approval | approved by bob | proceeds |
| approved by alice | no approval | refuses, exit 1 — a handoff must never remove an approval |
| approved by alice | approved by alice | proceeds (idempotent) |
P2 — the blocked remedy pointed away from this PR's own fix: applied
Fair, and it was self-inflicted: the message still said "re-publish as proposed" when this same change makes mctl-agents-approve record an approver on an already-accepted proposal. The diagnostic now names that as the direct fix and keeps re-publishing as the fallback.
All fourteen flip-status rows re-checked after the changes, with every prior row unchanged.
| - - name: implement-fallback | ||
| template: run-implementer | ||
| when: "{{steps.implement.status}} != Succeeded" | ||
| when: "{{steps.implement.status}} != Succeeded && '{{steps.implement.exitCode}}' != '45'" |
There was a problem hiding this comment.
When implement ends in an Argo Error without a container exit code, such as a deadline or controller error, Argo must resolve the inner tag before evaluating this when; surrounding the tag with quotes does not manufacture an empty value. The condition therefore fails to resolve and the account-2 fallback is not run, regressing the pod-error path explicitly covered by continueOn.error. Default the missing exit code or condition on status without unconditionally interpolating exitCode.
Useful? React with 👍 / 👎.
| *) NEEDS_APPROVAL=yes ;; | ||
| esac | ||
| fi | ||
| if [ "$NEEDS_APPROVAL" = "no" ] || grep -q '^approval:' "$STATUS_FILE"; then |
There was a problem hiding this comment.
If two approval workflows start from the same accepted/no-approval revision, both pass this guard and produce full-file artifacts with different approvers. Only commit-and-push is mutex-protected (lines 418-420), and it replaces its fresh checkout with that stale artifact at lines 505-506, so the second workflow silently overwrites the first approval identity and timestamp despite the new path's stated invariant that it never edits an existing approval. Perform the check and mutation under the mutex, or re-check the fresh checkout before applying the artifact.
Useful? React with 👍 / 👎.
| echo "❌ Every proposal this run could act on is permanently blocked on a missing approval." >&2 | ||
| echo " This is NOT a quota or credential problem and retrying will not help." >&2 | ||
| echo " Read the blocked block in the proposal's .status.yaml for the reason and remedy;" >&2 | ||
| echo " the usual fix is to re-publish the proposal in 'proposed' so the approve flip records an approver." >&2 |
There was a problem hiding this comment.
For the exit-45 condition this diagnostic still tells the operator to republish the proposal as proposed, even though this same change adds a supported path that records a named approver while leaving an already-accepted proposal accepted. Republishing is now an unnecessary lifecycle rewrite and obscures the direct remedy the change introduces; the message should instruct the operator to invoke mctl-agents-approve with a named approver, with republishing reserved only as a fallback.
Useful? React with 👍 / 👎.
| case "$RHA" in | ||
| ''|false|no|0|off) ;; | ||
| *) NEEDS_APPROVAL=yes ;; |
There was a problem hiding this comment.
When a valid status file uses requires_human_approval: null or requires_human_approval: ~, the YAML reader used by human_approval_satisfied() sees None and considers the approval requirement absent, as the comment above this case also describes. This text parser instead obtains null or ~, falls into the default branch, and writes an approval record onto a proposal that does not require approval. Parse the YAML value or otherwise distinguish YAML null values consistently with the authoritative gate.
Useful? React with 👍 / 👎.
| when: "{{steps.flip.outputs.result}} != noop" | ||
|
|
||
| # ── 2. Flip proposed → accepted ─────────────────────────────────────── | ||
| # stdout is EXACTLY "flipped" or "noop" — Argo captures the script's |
There was a problem hiding this comment.
P3: This comment still says flip-status's stdout is EXACTLY "flipped" or "noop", but the script below now also emits recorded as a third outputs.result value. Worth updating so a future reader relying on this comment (e.g. when adding a fourth outcome) doesn't get a stale picture of the contract.
|
|
||
| echo "✅ ${WORKFLOW_SERVICE}/${WORKFLOW_SLUG}: proposed → accepted (by ${APPROVER_SAFE})" >&2 | ||
| echo -n "flipped" | ||
| if [ "$MODE" = "flip" ]; then |
There was a problem hiding this comment.
P3: The post-edit check above (around line 320, grep -q "^status: accepted$") only re-verifies status, which in record mode was already accepted before this script ran (status is deliberately never touched on this path). There is no equivalent verification that the approval: block was actually appended before the step emits recorded here and hands the file to commit-and-push. Low risk in practice (an append failure would trip set -e earlier and abort the step), but a grep for the approval line, mirroring the status check, would give the record path the same on-disk guarantee the flip path already has.
| when: "{{steps.flip.outputs.result}} != noop" | ||
|
|
||
| # ── 2. Flip proposed → accepted ─────────────────────────────────────── | ||
| # stdout is EXACTLY "flipped" or "noop" — Argo captures the script's |
There was a problem hiding this comment.
P3: this comment ("stdout is EXACTLY flipped or noop") is now stale — the script three lines below this can also emit recorded (the new MODE=record path). Doesn't affect correctness since the step-list when at line 112 checks != noop rather than enumerating outcomes, but worth updating the comment so a future reader isn't misled about the possible values.
| # Bound through env rather than interpolated into the script text. | ||
| # Argo substitutes a parameter BEFORE the interpreter parses the line, | ||
| # so a value carrying a quote and a semicolon would execute | ||
| # (gitops#992). These four are step statuses and exit codes rather than |
There was a problem hiding this comment.
P3: "These four are step statuses and exit codes" — the env: block below only defines three vars (PARAM_PRIMARY, PARAM_FALLBACK, PARAM_PRIMARY_EXIT), matching the three inputs.parameters (primary, fallback, primaryExit). Should say "these three".
There was a problem hiding this comment.
No P1/P2 findings (2 P3). Both prior review findings are resolved in this commit: the anonymous-approver guard now checks NEEDS_APPROVAL unconditionally (covers both flip and record MODE), and assert-attempt no longer references implement-fallback's exitCode at all — it only reads implement's (a step that always runs), eliminating the Skipped-node/Outputs-nil risk on the happy path. The env-binding move for assert-attempt's script params is correct and shrinks BASELINE as claimed; validator's check_env_bindings would pass (all three PARAM_* used are defined). Two trivial stale-comment nits left inline, neither blocking.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Here are the findings for this pull request: Severity: P1 Severity: P1 Severity: P3 Severity: P3 |
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Finding 1Severity: P1 Concrete failure scenario: Proposed fix: Finding 2Severity: P1 Concrete failure scenario: Proposed fix: |
… the approval check Four Codex findings on the current head. A fifth (P1, reading the SKIPPED fallback's exitCode) was already fixed in ad6ee31 before that review ran -- Codex reviewed a310806 -- and is answered on the PR rather than re-applied. P2 -- the when could fail to resolve on a pod-level Error `implement` always runs, and a node that RAN carries outputs.exitCode whether it Succeeded or Failed. A pod-level Error (deadline, OOM, controller error) is the case nothing in the cluster could be measured against, and quoting a plain tag does not manufacture a value: it still has to resolve first. A failure there would stop the fallback running on exactly the path continueOn.error exists to cover. Both tags are now expression tags with an explicit default -- `?? '0'` on the gate, `?? ''` on the parameter -- which is what the finding asked for. Argo in this cluster is v3.7.11, so expr tags are available. P2 -- the blocked remedy pointed away from the path this PR adds The exit-45 diagnostic told the operator to re-publish as `proposed`. This same change makes `mctl-agents-approve` record an approver on an already-accepted proposal, so the direct fix is now to run it with a named approver and leave the status alone. Re-publishing still works and is kept as the fallback. P2 -- explicit YAML null read as "gated" This is a TEXT parser standing in for a YAML one. Python's `control.get("requires_human_approval")` returns None for `:`, `: null` and `: ~` alike, and None means "never asked" -> satisfied. The shell saw the literal strings `null` and `~`, fell through to the default branch, and would have written an approval record onto a proposal that never wanted one. Both spellings join the falsey case. P2 -- two concurrent approves could overwrite each other's approver flip-status evaluates "this proposal has no approval" OUTSIDE any mutex and hands over a whole-file artifact. Two runs from the same revision both pass it; the second to take the mutex would replace the first's approver and timestamp, breaking this PR's own invariant that an existing approval is never edited. The check is repeated in commit-and-push, under the mutex, against the freshly cloned tree -- the only place it is serialized. A different approver already on disk is a no-op (the concurrent run did this run's job); a handoff that would REMOVE an existing approval is refused outright. Verified in alpine/git:2.43.0 -- fourteen rows for flip-status, including the three null spellings now reading as noop and every prior row unchanged, and four for the new guard: checkout=alice, handoff=bob -> keeps alice, exit 0 checkout=none, handoff=bob -> proceeds checkout=alice, handoff=none -> refuses, exit 1 checkout=alice, handoff=alice -> proceeds Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 6m 1s —— View job Review of PR #1209 complete (commit
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2325ae77c2
ℹ️ 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".
| RHA=$(sed -n 's/^[[:space:]][[:space:]]*requires_human_approval:[[:space:]]*//p' "$STATUS_FILE" \ | ||
| | head -1 | tr -d '"\047' | tr 'A-Z' 'a-z' | tr -d '[:space:]') |
There was a problem hiding this comment.
When requires_human_approval is the valid YAML string "null", 'null', "~", or '~', the authoritative YAML reader receives a string and therefore fails closed by requiring approval, but this pipeline removes the quotes before the null|~ case classifies the value as absent. An already-accepted proposal with such a value consequently no-ops even when a named approver is supplied and remains blocked; on the proposed path, the default anonymous approver can also be committed before the implementer rejects it. Fresh evidence beyond the earlier YAML-null comment is that the follow-up now strips quote characters before distinguishing YAML null from quoted strings.
Useful? React with 👍 / 👎.
| HANDOFF_APPROVER=$(sed -n 's/^[[:space:]][[:space:]]*approved_by:[[:space:]]*//p' "/artifact/${STATUS_PATH}" | head -1) | ||
| CURRENT_APPROVER=$(sed -n 's/^[[:space:]][[:space:]]*approved_by:[[:space:]]*//p' "$STATUS_PATH" | head -1) | ||
| if [ "$HANDOFF_APPROVER" != "$CURRENT_APPROVER" ]; then |
There was a problem hiding this comment.
When two approval workflows for the same accepted/no-approval revision carry the same approver, the new under-mutex guard sees equal approved_by values and falls through rather than treating the second run as a no-op. It then copies the second workflow's stale whole-file artifact over the first approval, replacing at least approved_at and updated_at whenever the runs generated different timestamps, despite the stated invariant that an existing approval is never edited. The fresh evidence relative to the earlier concurrency finding is that the added serialized check compares only the approver name rather than whether the approval record already exists.
Useful? React with 👍 / 👎.
| # cannot be told apart here — mctl-agents' human_approval_satisfied() | ||
| # fails closed on that, this cannot, and gitops#1207's validator | ||
| # rejects that shape at commit time instead. | ||
| RHA=$(sed -n 's/^[[:space:]][[:space:]]*requires_human_approval:[[:space:]]*//p' "$STATUS_FILE" \ |
There was a problem hiding this comment.
P2: this RHA extraction still scans the entire $STATUS_FILE for any line matching ^[[:space:]]+requires_human_approval:, not just the one inside the already-confirmed control: block — the exact injection Antigravity flagged P1 on the previous commit (ad6ee31) and that this commit does not appear to address (it only added null/~ to the falsey list). A multi-line block-scalar value on an earlier top-level key (e.g. an LLM-authored description:/plan: field containing example YAML text like ' requires_human_approval: false') stays valid, indented string content under the real YAML parser, but this sed pattern has no document-structure awareness and head -1 will pick it up first if it precedes the real control: block.
Impact is narrower than a full approval bypass — mctl-agents' own human_approval_satisfied() presumably still parses real YAML and fails closed downstream, so this can't itself launch an unapproved implementer run. But it CAN cause this script to compute the wrong NEEDS_APPROVAL, and specifically for a false-negative case (real value true, extracted value falsey) it puts the proposal right back into the permanently-stuck, no-op'ing forever state this PR exists to close (line 266: NEEDS_APPROVAL=no → noop, no approval ever recorded) — just reached through corrupted input instead of the original bug.
Suggest scoping the extraction to only the control: block, e.g. awk '/^control:/{f=1} f \&\& /requires_human_approval:/{print; exit}' "$STATUS_FILE" (or an equivalent sed range restricted to /^control:/,/^[^ ]/), rather than a whole-file line match.
There was a problem hiding this comment.
All three fixed in 50e9982b. One of them is me over-correcting the previous round, which is worth saying plainly.
Scope the read — claude P2 / agy P1
Correct, and I had only patched the value handling last round while leaving the lookup wrong. RHA came from a whole-file scan with head -1, so an earlier top-level key carrying a multi-line block scalar — an LLM-authored description: or plan: quoting example YAML — could win. To a real parser that text is string content belonging to another field; to a line scan it is indistinguishable.
awk walks the document now: enter at ^control:, leave at the next unindented line, match only inside.
Mutation-proven. Restoring the whole-file scan against this fixture:
status: accepted
description: |
The proposal explains the schema like so:
requires_human_approval: false
...and that is only prose.
control:
requires_human_approval: trueflips the result from recorded to noop — the prose wins and a gated proposal is treated as ungated. That is the bypass, reproduced.
Keep quoting — Codex P2
This one is mine, and it is a good illustration of a fix creating the next bug. Last round added null/~ to the falsey list and stripped quotes before testing, which collapsed two genuinely different YAML values:
| value | YAML sees | mctl-agents | my previous code |
|---|---|---|---|
null / ~ (bare) |
None |
not gated | not gated ✓ |
"null" / '~' (quoted) |
string | gated — not in _FALSEY |
not gated ✗ |
So a proposal gated with a quoted null-like string would have no-op'd forever — the exact permanently-stuck state this PR exists to close, reintroduced by the fix for the previous finding.
Quoting is detected before the value is normalised, and the two cases take different falsey sets. Verified: "null" → recorded, '~' → recorded, bare null/~/empty → noop.
Never edit an approval, whoever wrote it — Codex P2
Correct. Comparing approvers meant two runs with the same approver fell through and copied the second artifact over the first, rewriting approved_at and updated_at. That is an edit to an existing approval, which is the invariant — the approver's identity was never what made it one.
The comparison is gone: an approval already on main means there is nothing to add. Mutation-proven — restoring the comparison makes the same-approver case PROCEED and overwrite approved_at: T1 with T2; without it the run no-ops. The ordinary path (fresh clone with no approval: line) still proceeds.
Seventeen rows re-checked in alpine/git:2.43.0 with every prior row unchanged, and validate-shell-param-interpolation.py still green at 30 known sites.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2. This commit correctly fixes the two issues raised on the prior round — the exit-code when/parameter tags now use Argo's expr syntax ({{=steps['implement'].exitCode ?? '0'}}) so a pod-level Error with no populated Outputs still resolves instead of breaking the workflow (Antigravity P1 on ad6ee31), and requires_human_approval: null/~ are now treated as 'never asked' rather than gated (Codex P2). The new TOCTOU guard in commit-and-push (approve.yaml:538-553) correctly re-checks the freshly-cloned tree under the mctl-gitops-main-writes mutex before overwriting, refusing to regress an existing approval and no-op'ing (not erroring) when a concurrent run already recorded one with a different approver — verified the ordinary single-run path is unaffected since the fresh clone won't yet have an approval: line.
One P2 left open: the RHA extraction (approve.yaml:238, inline comment) still does a whole-file line scan for requires_human_approval: rather than scoping to the confirmed control: block — the injection Antigravity flagged P1 on the previous commit is still present. Impact is narrower than full approval bypass (the downstream Python human_approval_satisfied() parses real YAML and fails closed), but it can misclassify NEEDS_APPROVAL and reopen the exact permanently-stuck/no-op state this PR exists to close, via an LLM-authored description/plan field that happens to contain matching indented text ahead of the real control: block.
Everything else in this commit (and the earlier commits in the PR) checks out against prior review rounds: the noop/flipped/recorded three-way split, the anonymous-approver refusal now covering both flip and record modes, assert-attempt only ever reading implement's exit code (never the Skipped implement-fallback node's), and the env-bound parameter passing in assert-attempt closing the injection class from gitops#992.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Severity: P3 |
|
Claude finished @mashkovd's task in 5m 42s —— View job Reviewing PR #1209 (commit
|
| # Carried over with a real parser rather than a line splice: the same | ||
| # class of defect cost five review rounds on the approve template. | ||
| if [ -d /artifact/tree/platform-gitops/agents-state ]; then | ||
| apk add --no-cache python3 py3-yaml >/dev/null 2>&1 |
There was a problem hiding this comment.
P3: same issue as cwft-mctl-agents-approve.yaml:231 — apk add --no-cache python3 py3-yaml >/dev/null 2>&1 is a bare statement under set -e. A package-fetch failure here aborts commit-and-push (the mutex-holding, retry-wrapped step) with no log output at all, right before the new approval-carry-forward logic that this same commit adds specifically to avoid a silent data-loss race. Worth keeping the diagnostics for this one, e.g. drop the 2>&1 or check its exit status explicitly, so a real failure here doesn't look identical to "nothing to report."
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 2 P3. This commit replaces the shell/awk gate-decision logic in cwft-mctl-agents-approve.yaml with a real YAML parser (python3 + PyYAML), which correctly resolves the whole class of text-parser edge cases five prior review rounds found (block-scoping, quoted vs bare null, inline comments) — mirrors mctl-agents' human_approval_satisfied() directly instead of re-deriving it heuristically. It also adds a carry-forward fix in cwft-mctl-agents-implement.yaml's commit-and-push so an approval written concurrently by the approve workflow isn't dropped by a stale handoff overlay (the last open Codex P2).
P2 — approve.yaml:290: the new HAS_APPROVAL (derived from the python gate's gated-approved/gated-unapproved) means "gate satisfied by a named approver," not "an approval: block exists on disk" — diverging from the write guard at line 374, which still checks raw block presence. For an accepted+gated proposal whose approval: block already has an anonymous approved_by (e.g. unknown), the noop branch is skipped, MODE=record proceeds, the anonymous-approver check passes (it checks the new approver, not the stale one on disk), but the write guard then finds the existing approval: line and skips writing — so nothing is actually fixed, updated_at/updated_by still get bumped, and the step reports "approver recorded" and commits that message to git. The proposal stays exactly as blocked as before, now hidden behind a false success. Confirmed this shape exists in this repo's real data today (mctl-academy/issue-212, currently status: rejected so not currently exploitable, but it shows the shape occurs).
P3 x2 — both new apk add --no-cache python3 py3-yaml >/dev/null 2>&1 lines (approve.yaml:231, implement.yaml:881) run as bare statements under set -e with both stdout and stderr discarded; a package-fetch failure aborts the step with zero log output, unlike every other apk add in these templates. Not blocking, just a debuggability regression against this repo's stated fail-loudly convention.
Everything else checks out: the python gate script's FALSEY/ANONYMOUS handling matches mctl-agents' semantics from prior rounds, assert-attempt still only reads implement's exit code (never the Skipped implement-fallback node's), the approval-carry-forward script only rewrites the one handoff file it actually needs to (bounded blast radius), and it runs after the existing handoff-shape validation and under the same mctl-gitops-main-writes mutex as the rest of commit-and-push.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Finding 1 Concrete failure scenario: Consequently, when Proposed fix: Finding 2 Concrete failure scenario: Proposed fix: |
…rd that never happened
claude P2, and it survived the previous commit: splitting HAS_APPROVAL from
HAS_BLOCK fixed the READ side, but the write guard was still a raw
`grep -q '^approval:'`.
The gap: an accepted+gated proposal whose approval block names nobody real
(`approved_by: unknown` / `none` / `null` / absent). The parser reports
gated + hasblock, so HAS_APPROVAL=no and the noop branch is skipped; MODE=record
proceeds; the anonymous-approver check passes because it tests the NEW approver,
not the stale one on disk; and then the append is skipped because the key
exists. The run reports `recorded` having recorded nothing, and the proposal
stays exactly as blocked as before -- the failure mode this whole PR exists to
remove, reached by a different door.
Such a block is a placeholder, not an approval: the gate rejects it, which is
precisely why HAS_APPROVAL is no. Replacing it is what this path is for, and
does not violate "never edit an existing approval" -- there is no approval
there to edit.
Stripped with awk bounded to the top-level key (enter at `^approval:`, leave at
the next unindented line), which is safe here because the YAML parser already
established that the key exists and is top-level. The append then writes the
real approver.
Verified in alpine/git:2.43.0:
accepted + gated + approved_by: unknown -> recorded, approved_by is the
caller, exactly one top-level `approval:`, unrelated keys (`pr:`) intact
accepted + gated + approved_by: null -> recorded
accepted + gated + named approver -> noop, untouched
proposed + stale approval -> exit 1, as added last commit
plus the flip/record/noop rows unchanged
Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 2m 43s —— View job Review of PR #1209 complete (commit
|
| if [ "$HAS_BLOCK" = "yes" ] && [ "$HAS_APPROVAL" = "no" ]; then | ||
| awk ' | ||
| /^approval:/ { drop = 1; next } | ||
| drop && /^[^[:space:]#]/ { drop = 0 } |
There was a problem hiding this comment.
P3: the awk exit condition drop && /^[^[:space:]#]/ only ends the drop on a top-level line whose first char is non-space AND non-#. A blank line or a top-level # comment immediately following the stripped approval: block (before the next real key) doesn't match, so it stays in drop state and gets silently deleted along with the placeholder block. Purely a formatting/comment-loss nit — no functional impact, since the next actual key still terminates the block correctly — but worth noting if a future status file relies on a trailing comment surviving this rewrite.
…t success
Codex P2. The conflict handler treated `status: accepted` on origin/main as
proof that a concurrent run had done this run's job. For a flip that is true --
the flip IS the job. For a record it is not: the proposal was already accepted
before the workflow started, so the test was true from the outset. A conflict
whose winning side merely added some other annotation would be read as an
idempotent no-op, this run's approval commit would be discarded, and the run
would exit 0 leaving the proposal exactly as blocked as before.
That is this PR's own failure mode -- a green run over a still-unrunnable
proposal -- reached through the error path instead of the happy one.
Derived from the HANDOFF rather than from a MODE variable, because
commit-and-push is a different pod and has none: if what we pushed carries an
approval block, then main must end up with a satisfying named approver, ours or
the concurrent run's. If it does not (an ungated flip), the status test remains
correct and is unchanged.
The check reads main's copy with a YAML parser, same as the gate.
Verified against fixtures:
handoff has approval, main names someone else -> no-op, exit 0
handoff has approval, main has no approver -> error, exit 1 (was 0)
handoff has approval, main has approved_by:
unknown -> error, exit 1 (was 0)
handoff has none, main accepted -> no-op, exit 0 (unchanged)
handoff has none, main not accepted -> error, exit 1 (unchanged)
The sibling finding in the same round (`approval: null` / a non-mapping
approval value reported as `recorded` without writing) needed nothing: the
placeholder replacement added in 421a0e6 already strips any approval block
without a named approver before the append. Confirmed on both shapes --
`approval: null` and `approval: "oops"` -- each ends with approved_by set to
the caller, one top-level key, and unrelated keys intact.
Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 3m 7s —— View job Review of PR #1209 complete (commit
|
| echo "Rebase conflict and ${STATUS_PATH} is absent on main - needs a human look" >&2 | ||
| exit 1 | ||
| } | ||
| if grep -q '^approval:' "/artifact/${STATUS_PATH}"; then |
There was a problem hiding this comment.
P2: this branch is entered on every flip, not only on the gated ones it is reasoning about — which turns a benign race into a hard failure on the ungated path.
The comment just above says the status test is kept for the case where the handoff carries no approval, "(an ungated flip)". But a flip handoff always carries one. flip-status appends approval: unconditionally on the flip path (lines 424–429); the only way to reach that append with a block already present is HAS_BLOCK=yes, and line 382 already refused that. So grep -q '^approval:' "/artifact/${STATUS_PATH}" is true for gated and ungated flips alike, and the status: accepted fallback at line 715 is now only reachable for a handoff shape this template never produces.
Why that matters concretely: approver defaults to unknown (line 61), and the anonymous-approver refusal at line 336 fires only when NEEDS_APPROVAL=yes — so an ungated flip legitimately writes approved_by: 'unknown' (the author's own verification table has this row: proposed + ungated, clean, approver unknown → flipped). If such a run loses the push race to a concurrent approve that also wrote an anonymous approver, PYCHECK exits 1 and this step exits 1 with "main still has no named approver - this run's approval was discarded" — even though origin/main reads status: accepted, the concurrent run did exactly this run's job, and the pre-c63ede6f code correctly exited 0. That regresses the contract stated in this template's own header (lines 18–19: "Idempotent: already-accepted exits 0 so Temporal retries and rebase races never fail the loop"), on the DevLoop's default-approver path, in the one branch that exists specifically to keep races from failing the loop.
Suggested fix: derive the strictness from our own handoff rather than from the mere presence of the key — run the same PYCHECK against /artifact/${STATUS_PATH} first, and only demand a named approver on main when our handoff itself carries one; otherwise fall through to the status: accepted test. That keeps the Codex P2 closed (a record-path run always has a named approver, so it still cannot green-exit on a discarded approval) without catching the ungated flip in the same net.
| echo "Rebase conflict and main still has no named approver - this run's approval was discarded" >&2 | ||
| exit 1 | ||
| fi | ||
| if grep -q '^status: accepted$' /tmp/main-status.yaml; then |
There was a problem hiding this comment.
P3: the two facts are now treated as alternatives when they are independent. For an approval-carrying handoff (i.e. every flip, per the P2 above) the status: accepted test on main is skipped entirely, so a gated flip that hits a conflict is accepted as "done" on the strength of a named approver alone — it no longer verifies that the status flip, which is the other half of what that run was doing, actually landed.
Contrived but reachable: a commit lands in the conflict window that re-publishes the proposal (status: proposed) while leaving the stale approval block behind — exactly the malformed shape line 382 refuses at flip time, but nothing prevents it appearing on main between this pod's clone and its push. Main then has a named approver and status: proposed, this run's flip is discarded, and the step exits 0.
Requiring both — a named approver and status: accepted — on the approval-carrying branch keeps the guarantee the record path needs and the one the flip path needs, rather than trading one for the other.
| exit 1 | ||
| } | ||
| if grep -q '^approval:' "/artifact/${STATUS_PATH}"; then | ||
| apk add --no-cache python3 py3-yaml >/dev/null 2>&1 |
There was a problem hiding this comment.
P3: third instance of the silent apk add (after approve.yaml:231 and implement.yaml:881, both raised last round), and this is the worst placement of the three. It is a bare statement under set -e with stdout and stderr discarded, inside the push-retry loop, after a rebase conflict has already occurred — so a transient mirror/DNS failure here aborts the step with no log line at all, at precisely the moment an operator is trying to work out whether their approval landed. The diagnostics this whole block exists to produce are then never printed. Dropping the 2>&1 (or wrapping it in the || { echo ...; exit 1; } pattern used for the PYGATE invocation) restores that for free.
Unrelated nit in the same hunk: the new/rewritten messages here drop the ✅/❌/ℹ️ prefixes every other message in both templates uses, including the one this hunk replaced. If that was deliberate (ASCII-only), fine; if not, it makes the conflict path's log lines visually distinct from the rest of the step for no reason.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P2, 2 P3.
This commit closes the Codex P2 it targets — the rebase-conflict handler no longer accepts status: accepted on main as proof that a record-path run's job was done, which was true from the outset on that path and would have swallowed a conflict that discarded the approval. The PYCHECK mirrors the gate's ANONYMOUS semantics, the heredoc indentation is consistent with the PYGATE block scalar, and git show ... || { ... exit 1; } handles an absent file explicitly.
P2 — approve.yaml:691: the new branch keys on grep -q '^approval:' /artifact/..., and the comment above it assumes that distinguishes gated from ungated flips. It does not — flip-status appends an approval: block unconditionally on the flip path (line 424, reachable only with HAS_BLOCK=no after the line-382 guard), so every flip handoff carries the key and the status: accepted fallback at line 715 is now unreachable for any handoff this template actually produces. Consequence: an ungated flip legitimately writes approved_by: 'unknown' (the approver param defaults to unknown; the anonymous refusal at line 336 only fires when gated), so if it loses the push race to a concurrent approve that also wrote an anonymous approver, this exits 1 with "this run's approval was discarded" where the pre-c63ede6f code correctly exited 0 — a false red in the exact branch that exists to keep races from failing the loop, on the DevLoop's default-approver path. Fix: run PYCHECK against the handoff too and only demand a named approver on main when our own handoff carries one.
P3 x2 — approve.yaml:715: the approval-carrying branch drops the status: accepted check entirely, so a gated flip that conflicts is accepted as done on a named approver alone without confirming the flip landed; requiring both is strictly better. approve.yaml:692: third silent apk add ... >/dev/null 2>&1 under set -e, here inside the retry loop after a conflict — a package-fetch failure aborts with zero output exactly when the operator needs the diagnosis.
cwft-mctl-agents-implement.yaml and scripts/validate-shell-param-interpolation.py are unchanged since 421a0e6c (reviewed clean apart from pre-existing P3 nits).
Note: only git commands are permitted in this sandbox, so the shell/awk/python logic was traced by hand rather than executed.
…ng apk failures
Three findings, and the P2 is a regression I introduced one commit earlier.
P2 -- the new branch fired on every flip, not the gated ones it reasoned about
The comment claimed a handoff without an `approval:` key meant an ungated flip.
It never does: the flip path appends that key unconditionally, writing
`approved_by: unknown` for an ungated proposal. So the branch was entered on
EVERY flip and demanded a named approver on main -- turning a benign concurrent
ungated approve, which the old code handled as a no-op, into a hard failure.
The key's presence was never the right test. A NAMED approver is.
P3 -- the two facts were treated as alternatives
They are independent, and a conflict is only benign when BOTH hold:
1. did the status flip land? ALWAYS checked -- both paths end accepted.
2. did an approval land? checked only when OUR handoff carried a named
one, since that is what a conflict can discard.
Previously an approval-carrying handoff skipped the status test entirely, so a
gated flip that conflicted was accepted on the strength of an approver alone,
without confirming the flip itself survived.
P3 -- silent apk add
Three of them, this one worst placed: a bare statement under `set -e` with both
streams discarded, inside the push-retry loop after a conflict -- so a transient
mirror failure aborted the step with no log line at all, exactly when an
operator is trying to find out whether their approval landed. All three now
report the failure and the apk error before exiting.
Verified against fixtures:
ungated flip, concurrent ungated approve -> no-op, exit 0 (was exit 1)
gated, main names someone else -> no-op, exit 0
gated, main anonymous, ours named -> error, exit 1
main not accepted, ours named -> error, exit 1 (was accepted)
main not accepted, ours anonymous -> error, exit 1
Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 5m 14s —— View job Review of PR #1209 complete (commit
|
| # mctl-agents decides this with yaml.safe_load; so does this now, and | ||
| # the whole class goes with it (gitops#1209, Codex: "parse the YAML | ||
| # scalar rather than normalizing the raw line"). | ||
| if ! apk add --no-cache python3 py3-yaml 2>/tmp/apk.err; then |
There was a problem hiding this comment.
P1: this step's stdout is outputs.result, and dropping the >/dev/null puts apk's progress output into it.
The comment at line 115 states the contract: "Argo captures the script's whole stdout as outputs.result, and the step-list when compares it verbatim, so every log line here goes to stderr instead." Every log line in flip-status obeys that — >&2 throughout, with echo -n "noop"/"flipped"/"recorded" the only writes to fd 1. The previous form, apk add --no-cache python3 py3-yaml >/dev/null 2>&1, obeyed it too.
The new form redirects only fd 2. apk's informational output (fetch https://dl-cdn.alpinelinux.org/..., (1/6) Installing python3 ..., OK: 62 MiB in 40 packages) is written to stdout — errors go to stderr, which is why apk add ... >/dev/null is the usual silencing idiom and why the original line needed the >/dev/null at all. So outputs.result becomes those lines followed by the verdict:
fetch https://dl-cdn.alpinelinux.org/alpine/v3.19/main/x86_64/APKINDEX.tar.gz
(1/6) Installing ...
OK: 62 MiB in 40 packages
noop
That breaks the step-list gate at line 112, when: "{{steps.flip.outputs.result}} != noop", on every run of this template, not just the new paths:
- The substitution is a multi-line string spliced unquoted into the expression, so the likely outcome is that the
whenno longer parses at all (fetch https://... != noop) and the workflow errors beforecommit-and-pushis scheduled. - If it does parse, the
noopverdict no longer compares equal tonoop, so the mutex-holding commit step runs on every no-op. Survivable —commit-and-pushexits 0 on an absent handoff (line 604) — but it takesmctl-gitops-main-writesfor nothing on the idempotent path thiswhenexists to skip.
This is the one instance of the three where the redirect actually carried weight. commit-and-push here and in cwft-mctl-agents-implement.yaml have no consumed outputs.result (nothing references either), so the same change is free in those two.
Fix keeps the diagnostic and restores the contract: if ! apk add --no-cache python3 py3-yaml >/dev/null 2>/tmp/apk.err; then.
Note: I could not execute apk in this sandbox (docker run needs an approval that was not granted), so the stream attribution above is from apk-tools' behaviour rather than a run I made. Worth one docker run --rm alpine/git:2.43.0 sh -c 'apk add --no-cache python3 py3-yaml 1>/dev/null' — if that prints nothing, stdout was carrying it.
| exit 1 | ||
| fi | ||
| # 2. the approval, only if ours carried a real one | ||
| if named_approver "/artifact/${STATUS_PATH}" && ! named_approver /tmp/main-status.yaml; then |
There was a problem hiding this comment.
P3: the false red is much narrower than it was, but it is not gone — it now fires on an ungated flip that supplies a real name.
The comment above keys the strictness on approved_by: unknown being what an ungated flip writes. That is the default, not the rule: approver is a free parameter, and nothing refuses a named approver on an ungated proposal (the anonymous guard at line 336 runs only when NEEDS_APPROVAL=yes). So an ungated flip approved by alice produces a handoff with a named approver.
If that run loses the push race to a concurrent ungated approve that took the default, main ends up status: accepted with approved_by: 'unknown' — named_approver /tmp/main-status.yaml returns 1, and this exits 1 with "this run's approval was discarded" for a proposal where no approval was ever required and the concurrent run did the whole job. Pre-c63ede6f that exited 0.
Two orders of magnitude rarer than the shape the last round flagged (it needs a mixed named/anonymous race on the same ungated proposal, not just any ungated race), and it is a false red rather than a false green, so not blocking. But the fact the check is trying to establish is "was an approval required of us", and NEEDS_APPROVAL is that fact — the handoff's approver name is a proxy for it that is exact on the record path and approximate on the flip path. If it is worth closing, the direct route is to carry NEEDS_APPROVAL into the handoff (or re-derive it from the handoff with the same PYGATE logic) rather than inferring intent from whether the name happens to be anonymous.
| with open(sys.argv[1]) as fh: | ||
| data = yaml.safe_load(fh) | ||
| except yaml.YAMLError: | ||
| sys.exit(1) |
There was a problem hiding this comment.
P3: named_approver collapses three different answers into exit 1 — "unparseable", "not a mapping", and "no named approver" — and the two call sites want opposite defaults from that.
On /tmp/main-status.yaml (line 728, right operand) collapsing is fail-closed and correct: an unreadable main is treated as "no approval landed" and the step goes red.
On /artifact/${STATUS_PATH} (left operand) it is fail-open. If our own handoff does not parse, the guard reads it as "we carried no approval", skips check 2 entirely, and exits 0 — reporting a clean concurrent no-op for a run whose approval may well have been discarded, which is the exact failure c63ede6f was written to close.
Reachable because nothing between the append and the handoff parses the file: flip-status writes approval: with printf and then verifies only grep -q '^status: accepted$' (line 443), and commit-and-push validates the artifact's path set, not its content. A .status.yaml that was already borderline-invalid upstream survives all of that.
Cheap fix, since the parser is already there: distinguish the codes — sys.exit(2) for unparseable/not-a-mapping, as PYGATE at line 244 already does, and have the handoff-side call treat 2 as "stop, do not no-op" rather than as "no approver".
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 1 P1, 2 P3.
This commit does what it says on the conflict handler. Splitting the two questions is right, and it fixes both findings from the last round: status: accepted on main is now always required (so a gated flip that conflicts can no longer be accepted as done on a named approver alone, and a concurrent rejection carrying a stale named approver no longer green-exits), and the approval demand is keyed on the handoff's approver rather than on the approval: key existing — which was the thing that fired on every flip. The named_approver helper's heredoc terminator lands at column 0 after the block scalar is stripped, matching the PYGATE pattern, and the handoff file is guaranteed present by the guard at line 604, so the shell shape is sound.
P1 — cwft-mctl-agents-approve.yaml:231: the apk visibility fix is right in commit-and-push (both templates), and wrong in flip-status. That step's stdout is outputs.result — the comment at line 115 says so, and every log line in the script obeys it with >&2. Replacing >/dev/null 2>&1 with 2>/tmp/apk.err leaves fd 1 open, so apk's fetch/Installing/OK: lines are captured as the step result and prepended to noop/flipped/recorded. The step-list gate at line 112, when: "{{steps.flip.outputs.result}} != noop", then either fails to parse (a multi-line value spliced unquoted into the expression) or stops matching noop, so the mutex-holding commit step runs on every idempotent no-op. This is the common path, on every run, not just the new ones. >/dev/null 2>/tmp/apk.err keeps the diagnostic and the contract. Caveat: docker run was not approved in this sandbox, so I could not run apk to confirm which stream carries its progress output — one apk add --no-cache python3 py3-yaml 1>/dev/null in alpine/git:2.43.0 settles it.
P3 x2 — approve.yaml:728: the false red is narrowed, not closed. approver is a free parameter and nothing refuses a named approver on an ungated proposal, so an ungated flip by alice losing the race to a concurrent default-approver run still exits 1 with "this run's approval was discarded" where pre-c63ede6f it exited 0. The fact wanted is NEEDS_APPROVAL; the approver's name is a proxy that is exact on the record path and approximate on the flip path. approve.yaml:713: named_approver collapses "unparseable", "not a mapping" and "no named approver" into exit 1, which is fail-closed against main and fail-open against our own handoff — an unparseable handoff skips check 2 and green-exits. PYGATE already distinguishes these with sys.exit(2).
Two pre-existing P3s are still open and unaddressed across five rounds now: approve.yaml:115 still says stdout is "EXACTLY flipped or noop" (recorded has existed since the first commit, and that stale contract is the same one the P1 above breaks), and implement.yaml's "These four are step statuses and exit codes" still counts four env bindings where three remain.
scripts/validate-shell-param-interpolation.py is unchanged since 62bd121d. The cwft-mctl-agents-implement.yaml change in this commit is the apk diagnostic only, and is correct there — nothing consumes that template's outputs.result.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8e5fb57cc
ℹ️ 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 grep -q '^approval:' "$STATUS_PATH" 2>/dev/null \ | ||
| && grep -q '^status: accepted$' "$STATUS_PATH" 2>/dev/null; then |
There was a problem hiding this comment.
When an accepted, gated file already contains a placeholder such as approval: null or approved_by: unknown, flip-status deliberately replaces it with the named approver in the artifact, but this check sees the original top-level key in the fresh checkout and exits before copying that artifact. The workflow therefore reports success while the placeholder remains and the implementation gate stays blocked. Fresh evidence relative to the earlier null-value comment is that this newly added under-mutex guard treats any approval key as an existing approval; it needs the same named-approver validation used by flip-status.
Useful? React with 👍 / 👎.
| if [ -f "$STATUS_PATH" ] \ | ||
| && grep -q '^status: accepted$' "$STATUS_PATH" \ | ||
| && grep -q '^approval:' "$STATUS_PATH" \ | ||
| && ! grep -q '^approval:' "/artifact/${STATUS_PATH}"; then |
There was a problem hiding this comment.
When an implement run and the new record-only approval path start from the same accepted, unapproved revision and the implement commit acquires the shared mutex first, the approval pod clones the newly written blocked/attempt fields but then falls through these approval-only guards and replaces that file with its older artifact. This silently removes the implementer's durable diagnostic and any other fields written while approval was in flight; the preservation added to cwft-mctl-agents-implement.yaml only handles the opposite commit order. The record path should add its approval to the fresh checkout rather than applying the stale whole-file snapshot.
Useful? React with 👍 / 👎.
| hand = load(h_path) | ||
| if hand is None or "approval" in hand: | ||
| continue |
There was a problem hiding this comment.
If an in-flight implement artifact already contains a placeholder approval (null, unknown, or a mapping without a named approver) and a valid named approval reaches the live checkout before this commit step, this condition skips preservation merely because the stale artifact has an approval key. The subsequent overlay then replaces the valid live approval with the placeholder and leaves the proposal blocked again. Fresh evidence beyond the earlier stale-handoff comment is this new key-presence shortcut; preservation should compare approval validity and prefer a live named record over an absent or anonymous handoff value.
Useful? React with 👍 / 👎.
| if ! apk add --no-cache python3 py3-yaml 2>/tmp/apk.err; then | ||
| echo "Could not install python3/py3-yaml to read the approval gate:" >&2 | ||
| cat /tmp/apk.err >&2 | ||
| exit 1 |
There was a problem hiding this comment.
On every successful invocation, apk add writes its fetch/install progress to stdout, while this redirection captures only stderr. Argo appends that stdout to flip-status's outputs.result, so an actual no-op produces package logs followed by noop rather than exactly noop; the when: "{{steps.flip.outputs.result}} != noop" condition consequently runs the mutex-holding commit step for every already-approved or ungated accepted proposal, and may attempt to resolve the deliberately absent optional artifact. Redirect the package-manager output away from stdout so the documented result contract remains exact.
Useful? React with 👍 / 👎.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit P1: Fallback runs unconditionally due to incorrect
|
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Severity: P2 Severity: P3 |
|
Closing this unmerged. The work is not lost — it moves to #1206 as findings, and the branch stays for reference — but this PR should not be the vehicle. WhyNine review rounds, a real defect in every one. That alone is a signal, but the shape matters more than the count:
The finding that decided itagy's P1 on So the fallback fired on every failure and That is precisely the defect this PR exists to remove: a thing that reports success while doing nothing. Shipping it would have added one more. It also invalidates the split I proposed twice in this thread, on the argument that the Worth noting how it got there: the move to expr was itself a fix, for a Codex P2 asking for a default in case a pod-level What carries forwardVerified, and worth not re-deriving:
The branch What the rework needsNot another review round — a live check. The exit-45 path has to be exercised against a real workflow before it can be believed, and the approval-record path needs a test that lives in the repository rather than a table that lives in one session (agy's P3, raised twice here and still fair). #1206 stays open and now carries these findings. |
Closes #1206. The CWFT side of mctlhq/mctl-agents#349 — the half that repo could not fix, because the flip lives here.
mctl-agents#358 (merged) made the refusal visible: a
blockedoutcome, a durable marker, andEXIT_BLOCKED_ONLY = 45. It could not make the proposal approvable.(a)
cwft-mctl-agents-approve.yaml— an already-accepted proposal can now get an approverstatus: acceptedwas an unconditional no-op:That left one shape permanently stuck. A proposal committed straight to
acceptedwhile carryingcontrol.requires_human_approvaland noapprovalblock can never become runnable: this step is the only writer ofapproval.approved_byand it declined to run, while the implementer refuses without that record. Both sides exit 0 and report success.The branch now distinguishes three cases, and the step's
outputs.resultgrows a third value —recorded, alongsideflippedandnoop. The step-listwhencompares againstnoopinstead of== flipped, so both writing outcomes reach the commit step.Deliberately narrow. It only ever adds an approval block where one is demanded and absent:
proposed, gatedflipped(unchanged behaviour)accepted, gated, noapprovalrecorded— newaccepted, gated,approvalpresentnoop— never edits an existing approvalaccepted, nocontrolblocknoop— incident-responder shape, "never asked"accepted,requires_human_approval: falsenoopstatusis never touched on the new path. Falsey spellings mirror mctl-agents'_FALSEY, and anything unrecognised requires approval rather than waiving it — the same fail-closed direction two reviews hardenedhuman_approval_satisfied()into.An anonymous approver is refused, not written.
unknown/none/nullare in_ANONYMOUS_APPROVERS, so recording one would commit a record that looks like an approval and leave the proposal exactly as blocked. Sinceapproverdefaults to"unknown", that case is real.(b)
cwft-mctl-agents-implement.yaml— exit 45 is not a quota failureNeither gate looked at an exit code; both compared Argo step status strings. Exit 45 therefore made
implementFailedlike any other non-zero exit, which:implement-fallbackon the account-2 token — reaching the identical refusal a second time, since the approval gate is evaluated before any model callassert-attemptfail the workflow with "Most often the Claude five_hour/seven_day usage limit / HTTP 429 on both accounts" — for a proposal nobody had approvedRed is correct; that is what exit 45 is for. Blaming quota is not.
implement-fallbacknow also requires the exit code not to be45, compared as a string so a pod-levelError— which carries no exit code — still evaluates rather than failing to resolve.assert-attempttakes both exit codes and reports the blocked condition with its real remedy, keeping the quota text for every other failure.Verified in the real image, not assumed
flip-statuswas extracted and run underalpine/git:2.43.0, the image the step actually uses. macOS is not a stand-in here — BSDsed -irequires an argument and silently breaks the script, which is exactly how a first local pass produced false failures.All eight rows of the table above behave as listed. The
recordedfile parses as YAML,approved_bysatisfies the gate,statusis untouched, and the handoff artifact lands at the right repo-relative path.Mutation-proven: deleting the existing-approval guard turns the
noopcase intorecorded— the overwrite this must never do. Restored, it returns tonoop.assert-attemptwas exercised the same way:Risk, stated plainly
{{steps.implement.exitCode}}in awhenis the one thing a local harness cannot prove — expression resolution happens in the Argo controller. If it fails to resolve for a skipped or errored step, the fallback gate breaks rather than the blocked path. Quoting it as a string is the mitigation. Per this repo's own rule, this CWFT should get one live tick watched before the next change to it — and the implement template has had four clean live runs since #1194, so this change starts from a known-good baseline.argo lintlocally reportsunknown field spec.templates[N].synchronization.mutexon both files; that reproduces on unmodifiedorigin/mainand is a CLI-version mismatch (localargo v4.1.3expectsmutexes). CI does not runargo lint.🤖 Generated with Claude Code
https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U