-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(integration): scale interactive read-then-write waits with env timeout (#7942) #7943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+4
−7
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Removing the explicit
30000timeout makes this call fall through togetDefaultTimeout(), which returns 15 000 ms locally — halving the previous 30 s budget for a model-dependent tool-call wait. The same applies towaitForAnyToolCallat line 70. — Failure scenario: a developer running the test locally against a slow or rate-limited model endpoint sees a false timeout at 15 s where the test previously allowed 30 s.Note: every other
waitForToolCall/waitForAnyToolCallcall site in the suite already uses the default (15 s locally), so the old 30 s was an outlier. If 15 s is intentionally sufficient, this is fine; if not, considerMath.max(30000, rig.getDefaultTimeout())to preserve the floor.中文说明
[Suggestion] 移除显式的
30000超时后,此调用会回退到getDefaultTimeout(),在本地环境下返回 15 000 ms——将之前模型相关的工具调用等待的 30 秒预算减半。第 70 行的waitForAnyToolCall也存在同样的情况。—— 失败场景:开发者在本地对慢速或限速的模型端点运行测试时,会在 15 秒时看到误报超时,而此前测试允许 30 秒。注意:套件中所有其他
waitForToolCall/waitForAnyToolCall调用点已在使用默认值(本地 15 秒),因此原来的 30 秒是一个异常值。如果 15 秒已足够,这没有问题;如果不够,可以考虑使用Math.max(30000, rig.getDefaultTimeout())来保留下限。— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declined — the PR's purpose is to normalize hardcoded timeouts to the env-aware default. The old 30 s was an outlier: every other
waitForToolCall/waitForAnyToolCallcall site in the suite already uses the default (15 s locally, 60 s in CI). AddingMath.max(30000, …)would re-introduce the hardcoded floor this PR removes and make this file inconsistent with the rest of the suite again.中文说明
已拒绝——本 PR 的目的是将硬编码超时统一为环境感知的默认值。原来的 30 秒是一个异常值:套件中所有其他
waitForToolCall/waitForAnyToolCall调用点已在使用默认值(本地 15 秒、CI 60 秒)。添加Math.max(30000, …)会重新引入本 PR 要移除的硬编码下限,并使此文件再次与套件其余部分不一致。