Repository navigation
feat(agents): issue-702-fix-devloop-a-failed-approval-must-not-p - #709
mctl-agents[bot] merged 2 commits into
Conversation
A failed or refused canonical approval now ends the own we_ Failed, returns a typed DevLoopResult.outcome (approval_refused / approval_failed) with prefixed ended, re-asks a fresh grant on the same we_ when the grant is provably stale, and is restartable on its own workflow id. Gated by the approval-failure-outcome patch. Proposal: platform-gitops/agents-state/mctl-agents/proposals/issue-702-fix-devloop-a-failed-approval-must-not-p/ Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Claude finished @mctl-agents[bot]'s task in 6m 26s —— View job Review of PR #709 complete
Verdict: Changes requested (2 P2 findings, posted inline, bound to commit P2 — stale P2 — no test coverage for the PR's core behavior change. The typed Both are explained in detail in the inline comments on the diff. Note: an early permission-probing attempt accidentally posted a placeholder comment with body "test" on Nothing else stood out: the I was unable to run the test suite or a linter directly (no execution permission in this run), so this review is based on static reading of the diff and surrounding code only. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| self._approve_refusal_code = "" | ||
| return None, f"approve flip unavailable: {type(exc.cause).__name__ if exc.cause else 'unknown'}" | ||
| if delegated.refusal or delegated.result is None: | ||
| self._approve_refusal_code = delegated.code or "" |
Antigravity (agy) reviewModel: gemini-3.8-flash-high · commit FindingsFinding 1
|
| self._approve_refusal_code = "" | ||
| return None, f"approve flip unavailable: {type(exc.cause).__name__ if exc.cause else 'unknown'}" | ||
| if delegated.refusal or delegated.result is None: | ||
| self._approve_refusal_code = delegated.code or "" |
There was a problem hiding this comment.
P2: _delegated_approve's two early returns (slug mismatch at line 2295-2296, missing grant at line 2297-2298) never reset self._approve_refusal_code. The new stale-grant retry loop (around line 4008) reads that attribute right after a failed _delegated_approve call to decide whether to supersede-and-reask vs. end the loop. When the call takes one of those two early-return paths, code still holds whatever a previous attempt left (or "" on the first try), so maybe_stale is computed from stale/unrelated data instead of the current refusal reason.
Concrete failure: the clearest stale-grant signal there is -- approve flip refused: the proposal changed since the approval was requested (reachable because _request_canonical_approval re-looks-up the slug and can get a different one than the outer slug closure variable captured once before the retry loop) -- is not itself classified via code. If the previous iteration's _approve_refusal_code happened not to contain digest/drift, maybe_stale comes out False and the loop permanently ends with approval_refused instead of superseding the grant and re-asking, even though this is exactly the staleness case #701/#702 exist to recover from.
Suggest setting self._approve_refusal_code on every return path of _delegated_approve (including the two early returns), or giving those two paths an explicit code (e.g. stale-slug/missing-grant) that STALE_GRANT_REFUSAL_CODES recognizes.
| investigate_result, | ||
| approve_result, | ||
| human_input_outcome, | ||
| ) |
There was a problem hiding this comment.
P2: this PR's core behavior change -- the typed approval_refused/approval_failed outcomes, the new DevLoopResult.outcome field, _approval_ended, the APPROVAL_FAILURE_OUTCOME_PATCH gate, and this whole stale-grant retry loop (lines ~3976-4039, plus the non-canonical path at line ~4056) -- has no new or updated test in tests/test_dev_loop_workflow.py. The only tests touched in this PR are in tests/test_temporal_cli.py, which cover restart_if_restartable/RESTARTABLE_ENDED_PREFIXES (the start.py side), not the workflow logic that actually produces those outcomes.
Given the PR title is literally "a failed approval must not proceed" and this ~120-line hunk is where that happens, I'd expect at least: (1) a canonical-loop test where the delegated approve flip fails/refuses and asserts result.outcome == "approval_failed"/"approval_refused" and result.ended starts with the right prefix with implement is None; (2) a test exercising the stale-grant retry path (refusal code containing digest/drift -> supersede -> re-ask -> succeed); and (3) a non-canonical-path test for the typed and not self._canonical branch at line 4056.
There was a problem hiding this comment.
Has P1/P2 findings, changes requested: 2 P2. (1) dev_loop.py: _delegated_approve's two early-return paths (slug mismatch, missing grant) never reset self._approve_refusal_code, so the new stale-grant retry decision can read a stale/unrelated code and skip the supersede-and-reask path even for the clearest staleness signal ("the proposal changed since the approval was requested"), ending the loop prematurely. (2) No test coverage was added for this PR's core change: the typed approval outcomes, _approval_ended, and the stale-grant retry loop in dev_loop.py -- only test_temporal_cli.py (the start.py restart-eligibility side) was updated.
…failed-approval-must-not-p Reset/assign _approve_refusal_code on the two early returns of _delegated_approve (stale-slug counts as a stale-grant signal), add workflow tests for typed approval outcomes, update the capped-supersede test to the typed ended string, and fix two E501 lint errors. Proposal: platform-gitops/agents-state/mctl-agents/proposals/issue-702-fix-devloop-a-failed-approval-must-not-p/ Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@claude review |
|
Claude finished @mctl-agents[bot]'s task in 4m 11s —— View job Review of PR #709 complete (commit
|
| assert result.outcome == "approval_refused" | ||
| assert result.ended.startswith("approval refused") | ||
| assert result.implement is None | ||
| assert len(delegated) == 1 |
There was a problem hiding this comment.
P3: both prior P2s look addressed by this commit — _delegated_approve's two early-return paths now set self._approve_refusal_code ("stale-slug" / "missing-grant"), and "stale-slug" was added to STALE_GRANT_REFUSAL_CODES, so the slug-mismatch refusal (the clearest staleness signal) now correctly drives the supersede-and-reask path instead of being misread from a leftover code. Good fix.
One gap remains in the new tests here: there's still no direct regression test that exercises the success path this fix enables — i.e. _delegated_approve hits the slug != self._approval_slug early return (code "stale-slug"), the loop calls _supersede_grant_if_stale, re-asks via _await_canonical_reapproval, and a fresh grant lets the flip succeed on retry. The two new/updated tests (test_a_non_stale_refusal_ends_typed_approval_refused, and the approval_refused/outcome assertions on the supersede-cap test) cover the refusal outcomes well, but not the retry-then-succeed path that was the actual bug. Not blocking given the logic fix itself is correct and covered indirectly, but worth adding before the next change touches this area.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Both prior P2s are fixed: _delegated_approve's two early-return paths now set _approve_refusal_code (stale-slug/missing-grant), and stale-slug was added to STALE_GRANT_REFUSAL_CODES, so the slug-mismatch staleness signal is correctly recognized by the retry decision instead of reading a leftover code. Test coverage was also meaningfully extended (typed outcome/ended assertions) though the retry-then-succeed path still lacks a direct test (noted as P3, non-blocking). Good to merge.
Antigravity (agy) reviewModel: gemini-3.8-flash-high · commit No significant issues found. |
A present but non-mapping subject, binding, risk or sections block was skipped instead of rejected, so `risk: high` silently waived the required failure modes and `binding: null` passed with no binding. Malformed is not absent: emit BAD_BLOCK. run_explainer.sh recorded the latency of its own placeholder echo as a measurement; it now writes unobserved. The #702 pre-implementation commit is observable (a42c3e7, first parent of the #709 merge), so the report names it instead of calling it unobserved.
Implements accepted proposal
mctl-agents/issue-702-fix-devloop-a-failed-approval-must-not-p.Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-agents/proposals/issue-702-fix-devloop-a-failed-approval-must-not-p/
Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.
Closes #702