Repository navigation
test(dev-loop): add #702 T1/T5/T6 approval-failure tests - #711
Conversation
The #702 review found the planned T1, T5, T6 and T9 tests missing. T1 now asserts the we_ advances once as Failed. T5 covers a flip that fails on a rewritten proposal: the grant is superseded, re-asked on the same we_, and the second flip reaches the implementer. T6 covers repeated stale flip failures ending typed approval_refused past MAX_GRANT_SUPERSEDES. T9: run against the pre-#702 dev_loop.py (56b9224^), T1, T2, T5 and T6 all fail; against main they pass.
|
Claude finished @mashkovd's task in 3m 38s —— View job Review complete ✅
Verdict: APPROVE (no P1/P2, 1 P3) This is a test-only change adding T1 (assertion strengthening), T5, and T6 coverage for #702's approval-supersede flow. I traced the new fixture behavior against the production code it exercises:
P3 (nit, non-blocking): No P1/P2 findings. Approved. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # post-failure freshness check sees a new digest. | ||
| digest_state["current"] = digest_state.pop("after_flip") | ||
| if isinstance(delegated_result, list): | ||
| return delegated_result.pop(0) if len(delegated_result) > 1 else delegated_result[0] |
There was a problem hiding this comment.
P3: delegated_result[0] raises IndexError if an empty list is ever passed (the len > 1 branch pops, but the else branch indexes unconditionally without checking for empty). Not reachable today since every caller passes a non-empty list, but a guard or docstring note on _canonical_activities/_fake_activities would make the fixture's contract explicit.
There was a problem hiding this comment.
No P1/P2 findings (1 P3). Verified the new fixture logic (list-valued delegated_result, after_flip digest rewrite) and the T5/T6 assertions against the production supersede/freshness logic in orchestrator/temporal/workflows/dev_loop.py (MAX_GRANT_SUPERSEDES, _supersede_grant_if_stale, _delegated_approve) — the test flow matches it precisely. Good to merge.
Antigravity (agy) reviewModel: gemini-3.8-flash-high · commit No significant issues found. |
Follow-up to #702 (PR #709): the review found the planned tests T1, T5, T6 and T9 missing. Test-only change; no production code touched.
What
_canonical_activities/_fake_activitiesaccept a list fordelegated_result(answers flips in order, repeating the last), anddigest_state["after_flip"]rewrites the proposal digest while a flip runs.test_failed_approve_flip_never_implements): also asserts thewe_advances exactly once, asFailed.test_702_t5_...): first flip endsFailedon a rewritten proposal -> grant superseded, re-asked on the samewe_test_own(attempt + 1), second flip succeeds, implementer runs, singleSucceededadvance, oneGRANT_SUPERSEDEDcomment naming both digests.test_702_t6_...): every flip fails on a fresh rewrite -> pastMAX_GRANT_SUPERSEDESthe loop endsapproval_refused("approval superseded too many times"), no implementer, singleFailedadvance.test_a_non_stale_refusal_ends_typed_approval_refused.T9 revert check
With
orchestrator/temporal/workflows/dev_loop.pyreplaced by56b9224^(pre-#702), T1, T2, T5 and T6 all fail; on main they pass.Verification
pytest tests/test_dev_loop_workflow.py tests/test_temporal_cli.py: 251 passedruff check: cleanRefs #702
🤖 Generated with Claude Code