test(workflow): classify active state by exclusion, not allowlist - #16
Open
echoVic wants to merge 1 commit into
Open
test(workflow): classify active state by exclusion, not allowlist#16echoVic wants to merge 1 commit into
echoVic wants to merge 1 commit into
Conversation
wait_until_active gated on an allowlist of `queued`/`running` while WorkflowRunStatus has nine variants. The remaining variants fell into a gap where the loop neither returned nor failed: it polled for the whole run and only tripped once the run went terminal. That made every failure report `completed`, which reads as "the run finished too fast" and sent the previous fixes after timing that was never the cause. Invert the check so any non-terminal status counts as observably active, and report the observed status sequence on failure so a future gap names itself instead of looking like a flake. Also assert the contract this test is named for and never checked: that `workflow run` returns while the 6s model call is still streaming. Local runs pass on macOS, but so did the previous code -- the Windows status sequence is what will confirm or refute this.
📝 WalkthroughWalkthroughThe workflow CLI contract tests now verify that ChangesWorkflow CLI contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/workflow_cli_contract.rs`:
- Around line 615-617: Update the timeout assertion in the workflow polling
helper around the `seen` and `show` diagnostics to include the complete observed
status sequence in the failure message. Preserve the existing task ID, deadline,
and last-status details while adding `seen` so polling transitions are retained
when the deadline is reached.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17b1a475-676b-4fe5-b653-f4b55b50f2c9
📒 Files selected for processing (1)
tests/workflow_cli_contract.rs
Comment on lines
615
to
+617
| assert!( | ||
| Instant::now() < deadline, | ||
| "workflow task {task_id} never became active within 20s (last: {last})" | ||
| "workflow task {task_id} never reported a status within 20s (last: {show})" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the observed status sequence in timeout failures.
Line [617] reports only the last response. If polling reaches the deadline, the seen sequence is lost even though the helper records it. Add seen to this diagnostic so every polling failure includes the observed transitions.
Proposed fix
- "workflow task {task_id} never reported a status within 20s (last: {show})"
+ "workflow task {task_id} never reported a status within 20s \
+ (status sequence: {seen:?}; last: {show})"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert!( | |
| Instant::now() < deadline, | |
| "workflow task {task_id} never became active within 20s (last: {last})" | |
| "workflow task {task_id} never reported a status within 20s (last: {show})" | |
| assert!( | |
| Instant::now() < deadline, | |
| "workflow task {task_id} never reported a status within 20s \ | |
| (status sequence: {seen:?}; last: {show})" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/workflow_cli_contract.rs` around lines 615 - 617, Update the timeout
assertion in the workflow polling helper around the `seen` and `show`
diagnostics to include the complete observed status sequence in the failure
message. Preserve the existing task ID, deadline, and last-status details while
adding `seen` so polling transitions are retained when the deadline is reached.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
为什么之前一直修不好
失败长得像 timing flake，于是过去几个 commit 全在调时序、调 gate、调等待方式。但真正的问题在判定逻辑，调时间碰不到它。
三个失败的测试恰好是唯一调用
wait_until_active()的三个，x64 / arm64 稳定 7 passed / 3 failed，两个架构一致。稳定复现 + 精确对应一个 helper —— 不是竞态。wait_until_active用白名单判定活跃：只认queued/running（2 个），只防 4 个 terminal 态。但WorkflowRunStatus有 9 个变体，剩下的掉进缝里：不返回、不报错、继续空转，直到 run 结束变 terminal 才失败 —— 失败现场因此永远显示completed，看着就像"跑太快了"，于是下一轮又去加延迟。改了什么
tests/workflow_cli_contract.rs：workflow_run_returns_before_slow_workflow_completes名字承诺、却从没断言过的契约：workflow run必须在 6s 模型调用结束之前返回。待验证
本地 macOS 10/10 通过，但旧代码在 macOS 上本来也过 —— 这证明不了 Windows 修好了。这次改动的定性是：消除一整类静默空转 + 装上能点名的诊断。Windows 跑出来的状态序列才会确认或推翻判断。
Summary by CodeRabbit