Repository navigation
feat(ci): refuse a proposal that is unrunnable the moment it is committed - #1210
Conversation
…tted
Closes gitops#1207.
A `.status.yaml` that reads `status: accepted` while demanding a human
approval it does not carry can never be implemented by any supported path.
mctl-agents' human_approval_satisfied() correctly refuses it, and the approve
operation used to short-circuit on anything already accepted without recording
an approver. Both sides exit 0 and report success, so once merged the condition
is invisible -- mctl-design/issue-21 sat that way for five weeks before anyone
looked (mctl-agents#349).
mctl-agents#358 added a write-time guard, but it binds only writers inside that
repository. It cannot stop a tool committing YAML straight into this one, which
is exactly how the stuck proposal was created. This is the gate that can.
The predicate mirrors human_approval_satisfied() rather than inventing its own,
and keeps its fail-closed direction: only a recognisably falsey value waives the
gate, an absent `control` block means "never asked" -- the incident-responder
writes `accepted` with no control block and must keep passing -- and a `control`
block present but not a mapping is corrupt rather than absent. Those semantics
were hardened twice by review over there; the docstring says to change both
together if they move.
Shipped strict rather than warn-only, because origin/main is clean: all 309
proposals pass. The one historical violation was repaired by gitops#1208 before
this landed, so no green build turns red for a reason unrelated to the PR that
trips it.
Proven in both directions, not just observed to pass:
- --selftest drives the real check_dir() over ten throwaway fixtures and
asserts on each: four unrunnable shapes rejected (bare gated, quoted
"true", anonymous approver, corrupt control), six legitimate ones accepted
(named approver, no control block, false, "no", and the proposed /
implemented states this validator has no business judging).
- Restoring issue-21's historical shape in the real tree makes the real pass
exit 1 and name the file.
- Breaking the predicate so an absent `control` block requires approval makes
--selftest exit 1 with "wrongly flagged accepted_no_control_block", which
is the incident-responder regression it exists to prevent.
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 14s —— View job Review of PR #1210 — complete
Verdict: APPROVED (1 P3, no P1/P2) Reviewed What I checked:
Finding posted inline (P3, non-blocking): the module docstring at |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| gate, an absent `control` block means "never asked", and a `control` block | ||
| that is present but not a mapping is corrupt rather than absent. Those | ||
| semantics were hardened twice by review; if they change there, change them | ||
| here -- `test_parity_with_mctl_agents` in the self-test names the contract. |
There was a problem hiding this comment.
P3: The module docstring says test_parity_with_mctl_agents in the self-test names the contract to keep this predicate in sync with orchestrator/proposal_state.py, but no such test/case exists anywhere in this file — SELFTEST_CASES uses names like accepted_gated_no_approval etc. Either add a case with that name (or a comment referencing the real one) or fix the docstring so it doesn't point future maintainers at something that isn't there.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Predicate logic correctly mirrors the fail-closed semantics described (absent control = never asked, non-mapping control = corrupt/fails closed, string/int falsey values recognized, anonymous approvers rejected). Glob pattern */proposals/*/.status.yaml matches all 309 existing .status.yaml files under platform-gitops/agents-state (verified via find), so the real pass covers the full tree as claimed. Self-test drives the shipping check_dir() over throwaway fixtures, consistent with the repo's other --selftest detectors. Good to merge.
![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: 88cfb1a665
ℹ️ 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".
| control = data.get("control") | ||
| if control is None: | ||
| # No control block at all means the proposal never asked. The |
There was a problem hiding this comment.
When an accepted status file contains control: null, yaml.safe_load() produces None, so this branch treats the explicitly present non-mapping block as if it were absent and the validator passes it. The downstream approval predicate will reject that corrupt proposal, recreating the silently unrunnable state this gate is intended to prevent; check whether the control key is absent before granting this exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Three of the four findings are fixed in 1c151f56. The fourth is answered rather than applied, because measuring it showed the premise does not hold.
Applied
Codex P1 — shape before approver. Correct, and it was the serious one. unrunnable() returned early on has_named_approver() before checking the shape of control, but human_approval_satisfied() fails closed on a non-mapping control before it looks at approval. So a named approver does not rescue that file over there — and the validator was calling an unrunnable proposal clean. Measured against orchestrator/proposal_state.py at mctl-agents c13df87:
{"status": "accepted", "control": "yes please",
"approval": {"approved_by": "mashkovd"}} -> satisfied=False
Shape is checked first now.
Codex P2 — or {}. Correct. [], false, 0 and "" all became an empty mapping and sailed through the isinstance check. Only a genuinely empty document (None) is defaulted now; anything else non-mapping is a finding that names the type it got.
claude P3 — the docstring named a test that does not exist. Correct, and mine. It promised test_parity_with_mctl_agents; nothing of that name was ever written, which is worse than making no claim — it tells the next reader the parity contract is enforced when it is not. Replaced with a PARITY block of cases --selftest actually asserts.
Self-test goes from 10 cases to 14 (7 rejected, 7 accepted), and both new branches are mutation-proven:
| mutation | selftest says |
|---|---|
| restore approver-before-shape | did not flag accepted_control_not_mapping_with_approver |
restore or {} |
did not flag root_is_empty_list, did not flag root_is_false |
Not applied — Codex P1 on control: null
When an accepted status file contains
control: null… The downstream approval predicate will reject that corrupt proposal, recreating the silently unrunnable state this gate is intended to prevent.
It does not reject it. human_approval_satisfied() opens with:
control = data.get("control")
if control is None:
return Truedata.get("control") returns None for an absent key and for an explicit control: null — the two are indistinguishable at that line, so both are "never asked" and both are satisfied. Executed against the real function at c13df87:
{"status": "accepted", "control": None} -> satisfied=True (runs)
{"status": "accepted"} -> satisfied=True (runs)
A control: null proposal is therefore runnable, not stuck, and flagging it would make this gate red on files the pipeline handles fine. The finding's own stated consequence — "recreating the silently unrunnable state" — is what would not happen.
Distinguishing absent from null here while mctl-agents cannot distinguish them there would also break the parity this validator exists to hold, which is the drift #1207 specifically warned about.
control: null is now pinned as an accepted case in the PARITY block with the measured value beside it, so the next reader does not have to re-derive this.
Happy to revisit if human_approval_satisfied() changes to separate the two — but then the fix belongs there first, and this follows.
| findings: list[tuple[Path, str]] = [] | ||
| for path in sorted(agents_state.glob("*/proposals/*/.status.yaml")): | ||
| try: | ||
| data = yaml.safe_load(path.read_text()) or {} |
There was a problem hiding this comment.
When a status file parses as a falsey non-mapping value such as [], false, 0, or "", the or {} replaces that value with an empty mapping, so the following type check never reports the corruption and CI passes. Such a status file is unreadable as proposal state and can make the proposal disappear from the agent pipeline without the promised failure signal; default only a genuinely empty YAML document if that case is intended to be accepted.
Useful? React with 👍 / 👎.
| if has_named_approver(data): | ||
| return None | ||
| control = data.get("control") | ||
| if not isinstance(control, dict): |
There was a problem hiding this comment.
When control is any non-null non-mapping value and approval.approved_by contains a name, requires_human_approval() correctly treats the control block as corrupt, but this early return then accepts the file before the non-mapping check below runs. The downstream predicate fails closed on that control shape regardless of the approval field, so this combination still creates an unrunnable proposal while CI remains green; reject malformed control blocks before considering the approver.
Useful? React with 👍 / 👎.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
…wing falsey roots
Three review findings, two from Codex and one from claude. All three real; a
fourth from Codex is answered on the PR rather than applied, because measuring
it showed the premise does not hold.
Codex P1 -- a corrupt control block plus a valid approver passed
unrunnable() returned early on has_named_approver() before it ever checked the
shape of `control`. But human_approval_satisfied() fails closed on a non-mapping
`control` BEFORE it looks at `approval`, so over there a named approver does not
rescue that file -- it stays blocked. The validator therefore called an
unrunnable proposal clean. Measured against orchestrator/proposal_state.py at
mctl-agents c13df87:
{"status": "accepted", "control": "yes please",
"approval": {"approved_by": "mashkovd"}} -> satisfied=False
Shape is now checked first, with the reason spelled out where the reader needs
it rather than in a commit nobody re-reads.
Codex P2 -- `or {}` hid every falsey non-mapping root
`yaml.safe_load(...) or {}` turned `[]`, `false`, `0` and `""` into an empty
mapping, which then sailed through the isinstance check and was reported clean.
Only a genuinely empty document (None) is defaulted now; anything else
non-mapping is a finding that names the type it got.
claude P3 -- the docstring named a test that does not exist
It promised `test_parity_with_mctl_agents`. Nothing of that name was ever
written, which is worse than no claim at all: it tells the next reader the
parity contract is enforced when it is not. Replaced with a PARITY block of
cases that --selftest actually asserts, each measured by calling
human_approval_satisfied() rather than read off it.
Self-test grows from 10 cases to 14 -- 7 rejected, 7 accepted -- and both new
branches are mutation-proven:
restore approver-before-shape -> "did not flag
accepted_control_not_mapping_with_approver"
restore `or {}` -> "did not flag root_is_empty_list",
"did not flag root_is_false"
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. |
There was a problem hiding this comment.
No P1/P2 findings (0 P3). All three prior Codex/claude findings on 88cfb1a (control-shape-before-approver ordering, falsey-root swallowed by or {}, and the docstring referencing a nonexistent test) are fixed in 1c151f5 and covered by the grown self-test (10->14 cases, mutation-proven per commit message). Traced requires_human_approval/has_named_approver/unrunnable/check_dir by hand: shape check now runs before the approver check, only a genuinely-empty YAML document is defaulted to {}, and the null-control parity claim (control: null -> not gated) matches the measured orchestrator behavior cited in the commit message. Good to merge.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit Here is the review of the pull request diff. FindingsSeverity: P2 reason_str = str(reason).replace('%', '%25').replace('\r', '%0D').replace('\n', '%0A')
rel_str = str(rel).replace('%', '%25').replace('\r', '%0D').replace('\n', '%0A')
print(f"::error file={path.relative_to(ROOT)}::{rel_str}: {reason_str}", file=sys.stderr)Severity: P3 |
… branch Two agy findings on 1c151f5, both real. P2 -- the ::error annotation interpolated unescaped values A GitHub Actions workflow command is terminated by a newline, so an unescaped one truncates the message there. That lands hardest on the case that most needs detail: a yaml.YAMLError renders across several lines carrying the line and column, and all of it after the first line was being dropped. The same interpolation is a log-injection vector. A proposal directory whose name contains a newline followed by `::warning::...` would have that command executed by the runner, from a path this validator itself prints. Percent, CR and LF are now escaped -- percent first, or it would double-escape the other two. P3 -- the empty-document branch had no test `if data is None: continue` was added to keep a genuinely empty file out of the falsey-root finding. Nothing asserted it, so a refactor could drop it and start flagging empty files as unrunnable with nothing to catch the regression. Self-test goes to 15 cases (7 rejected, 8 accepted) and the new one is mutation-proven: deleting the branch fails with "wrongly flagged empty_document". Claude-Session: https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U
|
Both agy findings fixed in P2 — unescaped values in the
|
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude finished @mashkovd's task in 1m 44s —— View job Review of PR #1210 (commit
|
| for path, reason in findings: | ||
| rel = path.relative_to(agents_state) | ||
| print( | ||
| f"::error file={_wc_escape(path.relative_to(ROOT))}" |
There was a problem hiding this comment.
P3: _wc_escape fixes the injection-critical part (%, \r, \n), but the file= argument is an annotation property, not free-form data. GitHub's own workflow-command escaping distinguishes the two: data only needs %/\r/\n escaped, but properties (like file=) additionally need : → %3A and , → %2C, since the runner splits properties on comma and parses key:value pairs. A proposal directory name containing a comma or colon would still render as a malformed/truncated file= property (not a security issue since newlines are already handled, just a correctness gap in the annotation). Consider a separate _wc_escape_property used only for the file= value, mirroring escapeProperty vs escapeData in actions/toolkit.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). This commit correctly fixes the two prior agy findings: _wc_escape (percent-then-CR-then-LF order) neutralizes the workflow-command injection/truncation on the ::error annotation, and the empty_document self-test case pins the data-is-None branch. Verified check_dir/unrunnable/requires_human_approval/has_named_approver ordering (shape-before-approver) and the PARITY cases still hold. One P3 posted inline: file= is an annotation property and technically also needs ':' and ',' escaped per actions/toolkit's escapeProperty, not just escapeData's %/\r/\n — non-blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86f0744246
ℹ️ 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 data.get("status") != "accepted": | ||
| return None |
There was a problem hiding this comment.
When a hand-written proposed record has a non-mapping control value, this return lets it pass validation even though the normal approval workflow will turn it into the exact deadlocked state this gate is meant to prevent. In cwft-mctl-agents-approve.yaml, the approve step only changes status and appends an approval (lines 239-259), then commits directly to main (lines 451-455); because the implementer rejects malformed control even with a named approver, validation only turns red after the unrunnable commit has already landed. Reject malformed control blocks while they are still proposed, or validate them before the approve workflow pushes.
Useful? React with 👍 / 👎.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
…escape the file= property Codex P2 -- the gate went red one commit too late A `proposed` record with a non-mapping `control` passed, because the check sat behind `status != accepted`. That is not harmless: the approve workflow only flips `status` and appends an approval, then commits straight to main. So approving such a record MANUFACTURES the deadlocked accepted state through an entirely supported path, and this gate would only fail on the NEXT run -- after the unrunnable record has already landed. The shape check now runs at any status. The implementer fails closed on a malformed control block regardless of any approver, so such a proposal can never run whatever its current status says. claude P3 -- file= is a property, not data GitHub's runner splits annotation properties on commas and parses each as key:value, so a property needs `:` and `,` escaped on top of what data needs -- the same split actions/toolkit makes between escapeProperty and escapeData. A proposal directory carrying either character rendered a malformed or truncated `file=`. `_wc_escape_property` wraps `_wc_escape` for that one argument; the message text keeps data escaping, where a colon is ordinary. Self-test goes to 17 cases (8 rejected, 9 accepted). Mutation-proven: moving the shape check back behind the accepted guard fails with "did not flag proposed_control_not_mapping". `proposed` with `control:` null still passes, since null is indistinguishable from absent to the authoritative parser too. 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 1m 50s —— View job Review of PR #1210 (commit
|
| for path, reason in findings: | ||
| rel = path.relative_to(agents_state) | ||
| print( | ||
| f"::error file={_wc_escape_property(path.relative_to(ROOT))}" |
There was a problem hiding this comment.
P3: --selftest exercises check_dir()/the predicate logic thoroughly but never calls report(), so the new _wc_escape/_wc_escape_property behavior (the fixes for the agy P2 log-injection/truncation finding and the claude P3 property-escaping finding on this PR) has no automated regression test — only the PR-description manual traces. A future refactor that drops _wc_escape_property's :/, handling, or reintroduces unescaped newlines, would ship silently green. Consider a small non-selftest unit check (e.g. asserting _wc_escape("a\n::warning::x") == "a%0A::warning::x" and _wc_escape_property("a:b,c") == "a%3Ab%2Cc") so the escaping itself is pinned, not just the shape predicate.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Verified the two fixes in this commit: the control-shape check now runs before the status check (so a malformed control on a proposed record is caught before the approve workflow can turn it into a deadlocked accepted state, per Codex's finding), and _wc_escape_property correctly adds :/, escaping on top of _wc_escape's %/\r/\n for the file= annotation property, matching actions/toolkit's escapeProperty/escapeData split. Traced the self-test's new parity cases (accepted_control_not_mapping_with_approver, proposed_control_not_mapping, proposed_control_null) by hand against the updated `unrunnable()" and they're consistent. Good to merge.
Antigravity (agy) review — informationalModel: gemini-3.1-pro-high · commit No significant issues found. |
Closes #1207.
The hole
A
.status.yamlthat readsstatus: acceptedwhile demanding a human approval it does not carry can never be implemented by any supported path.human_approval_satisfied()correctly refuses it, and the approve operation used to short-circuit on anything already accepted without recording an approver.Nothing fails. Both sides exit 0 and report success, so once merged the condition is invisible.
mctl-design/issue-21-docs-license-file-reference-in-readmesat that way from 2026-08-05 until someone went looking on 2026-09-12 — five weeks, written byclaude-code-e2e-verificationdirectly as:mctlhq/mctl-agents#358 added a write-time guard there, but it binds only writers inside that repository. It cannot stop a tool committing YAML straight into this one — which is exactly how that proposal was created. This is the gate that can.
The predicate is borrowed, not invented
requires_human_approval()/has_named_approver()mirrorhuman_approval_satisfied()inorchestrator/proposal_state.pyand keep its fail-closed direction:"true"is truthy, and anything unrecognised requires approval rather than being read as consent (both were review findings over there)controlblock means "never asked" — the incident-responder writesacceptedwith no control block and must keep passingcontrolblock present but not a mapping is corrupt, not absent, so it fails closedThe docstring says plainly that these move together. Reimplementing the rule freehand is how it would drift, which is what #1207 warned about.
Strict from day one
origin/mainis clean — all 309 proposals pass. The single historical violation was repaired by #1208 before this landed, so no unrelated PR inherits a red build. No warn-only mode needed.Proven in both directions
A guard that has only ever been seen to pass is not known to work, so all three were run:
--selftestcheck_dir()over ten throwaway fixtures and asserts each: 4 rejected (bare gated, quoted"true", anonymousapproved_by: unknown, corruptcontrol), 6 accepted (named approver, no control block,false,"no", plusproposedandimplemented— states this validator has no business judging)controlblock require approval makes--selftestexit 1 withwrongly flagged accepted_no_control_block— the incident-responder regression it exists to preventThe selftest drives the shipping code path rather than a copy of it, same as
check-vm-rules.sh --selftestandvalidate-origin-mtls.py --selftest, and it is wired to run before the real pass invalidate-manifests.yml.Error output
The remedy names the supported recovery and explicitly refuses the forgery, because hand-writing
approved_byis the tempting wrong answer.Related
🤖 Generated with Claude Code
https://claude.ai/code/session_01Dix3wU7BjCgs85hYe6z85U