-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(ci): salvage superseded review runs and hold the loop's report-time base refresh #10123
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
Open
wenshao
wants to merge
9
commits into
main
Choose a base branch
from
review-hold-and-salvage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
4a4afb9
fix(ci): salvage superseded review runs and hold the loop's report-ti…
wenshao 728227c
test(shepherd): classify the deferred stale-base headline as transient
wenshao f62091e
fix(ci): scope supersede-salvage per attempt and reap the watcher safely
wenshao 29d6fd4
fix(ci): verify the live head before ceding and validate salvage signals
wenshao 93cdb44
Merge remote-tracking branch 'origin/main' into review-hold-and-salvage
wenshao bde0965
fix(ci): make salvage signal IO FIFO-safe and cede decisions outcome-…
wenshao ef4ec86
fix(ci): cede genuinely superseded runs and restart the retry watcher
wenshao 82c2ff1
Merge remote-tracking branch 'origin/main' into review-hold-and-salvage
qwen-code-dev-bot b16578f
fix(ci): close the salvage cede races and hostile-state aborts
qwen-code-dev-bot 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
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
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.
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] The deferred headline this diff adds says the avoided merge "would cancel it or invalidate its posting (#10110)" — the pre-#10110 framing, and false for exactly the case this PR exists for: a past-threshold lifecycle review salvages and posts its historical-head review, neither cancelled nor invalidated, and the claim directly contradicts the "⏳ Historical-head review" note the salvage path posts into the same comment stream. Three unchanged strings carry the same newly-stale wording: the hold log at :2652 ("holding this round so the push cannot cancel it (#8888)"), the user-facing ack-on-defer comment at :2683 ("a push now would cancel it and discard its work" / "现在推送会取消该 review 并丢弃其工作"), and
qwen-autofix.md#af-028's present-tense claim that head mutations cancel "via qwen-code-pr-review.yml's cancel-in-progress" — which reads as making the #8888 gate pointless now that the cancel is gone. Post-merge, a push supersedes in-run: only a parked/pre-threshold run yields. The deferral behavior itself is correct; only the wording is wrong.Suggested fix: reword all four to the new semantics, e.g. "holding this round so the push cannot supersede it (#8888/#10110)" and "merging main now would supersede it — an early run loses its work, a late run is forced to salvage mid-flight (#10110)", matching the updated af-099 text.
中文说明
[Suggestion] 本 diff 新增的延迟标题称被避免的合并"会取消它或使其发布失效(#10110)"——这是 #10110 之前的表述,而且对本 PR 要解决的核心场景恰好是错的:超阈值的生命周期评审会 salvage 并发布其历史 head 评审,既未被取消也未失效,该说法与 salvage 路径发布到同一评论流的"⏳ Historical-head review"说明直接矛盾。另有三处未改动的字符串带有同样新近过时的表述::2652 的 hold 日志("holding this round so the push cannot cancel it (#8888)")、:2683 面向用户的延迟确认评论("a push now would cancel it and discard its work" / "现在推送会取消该 review 并丢弃其工作"),以及
qwen-autofix.md#af-028中"head 变更会通过 qwen-code-pr-review.yml 的 cancel-in-progress 取消在飞评审"的现在时表述——在取消已移除后读起来像是在说 #8888 门禁毫无意义。合并后,推送触发 run 内的取代决策:只有停放/未达阈值的 run 会让位。延迟行为本身是正确的;只是措辞错了。建议修复:把四处全部改为新语义,例如"holding this round so the push cannot supersede it (#8888/#10110)"与"merging main now would supersede it — an early run loses its work, a late run is forced to salvage mid-flight (#10110)",与更新后的 af-099 文本一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
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.
Deferred to the next round (this round's batch cap was ~8 findings; the Critical plus the salvage-logic correctness fixes and their witnesses went first). The wording finding is accepted — all four spots (the new deferred headline, the hold log at ~:2652, the ack-on-defer comment at ~:2683, and
qwen-autofix.md#af-028) will be reworded to the post-#10110 supersede semantics together next round.中文说明
推迟到下一轮(本轮批量上限约 8 项发现;Critical 加 salvage 逻辑的正确性修复及其见证优先)。措辞发现已接受 —— 四处(新增的延迟标题、约 :2652 的 hold 日志、约 :2683 的延迟确认评论、
qwen-autofix.md#af-028)将在下一轮一并改为 #10110 之后的取代语义。