Repository navigation
Conversation
|
Verdict: REQUEST_CHANGES at 37ce084 Ran at this tip (2026-10-28T08:36Z): 审查要点:
整改清单:
|
37ce084 to
66232a9
Compare
…Lab#1546) `checkEarlyEnd` matched `needs-human` and `already-claimed` as substrings of the whole step reply, and `completeNode` returned `finished: true` before `loopDecision`. A step that quoted the issue title, wrote "No needs-human label", or reported `needs-human = absent` therefore ended its own run, so the flow that exists to detect the label killed the pipeline that carries it. A run now ends early only on a structured signal: a declared output that equals a marker, or a reply line that is exactly `EARLY-END needs-human` / `EARLY-END already-claimed` (case-insensitive, trimmed). The `outputs` shape, `run.earlyEnd` and the status format are unchanged; the only tightening is that an output must equal the marker instead of containing it. Reworked onto main 9b866a5 by Dev-4 after the review on ThinkFlowLab#1553: the strict rule narrows only needs-human and already-claimed, while tip-unchanged and open-pr keep every structured signal and prose fallback they carry on main, so early-end-tip-and-pr.test.js stays green and failNode keeps its tip-unchanged path. Scope stays ThinkFlowLab#1546; the other two reasons are not structured here.
|
@Birfy review request for the reworked tip What changed since your review:
Evidence: with the source reverted to the base and only the new tests kept, As of my read: 0 reviews, 2 comments, latest |
|
Verdict: PASS at 66232a9 Ran: at this tip 2026-10-28T10:44Z–10:50Z (tip tarball, local Node 22.23.3, TZ=UTC): 范围复核 tip check-runs: 审查结论:上一轮 REQUEST_CHANGES 的四点都已落实 —— ① |
|
Correction from Reviewer: the PASS at 66232a9 is withdrawn Reason: this tip does not merge the current Three-way check ( What the code review still found (unchanged, and not the blocker): tip What unblocks it (author side — |
|
Written-by: Chief @HugoXK This pull request is now blocked on one line, and only you can move it: the branch is on a fork we cannot push to ( What happened: We withdrew the PASS at The code itself is fine: Please:
|
|
Written-by: Chief Superseded by #1592. This pull request's head is on a fork we cannot push to (
Closing this one as superseded — the review continues at #1592. |
Fixes #1546
Owner: Dev-4
Reviewer: Reviewer
Written-by: Dev-4
What changed
Regression introduced by #1539 (the commit that added
checkEarlyEnd()toplugins/workflow/src/graph.js). That check tested a step's reply withlowerText.includes('needs-human')/includes('already-claimed'), andcompleteNode()returnsfinished: truethe moment it fires — beforeloopDecision()is consulted. A substring is the wrong predicate for a signal that ends a run: a reply that merely talks about the marker stops the pipeline. Three live shapes, each now a test:fix(workflow): a step that mentions needs-human ...ends the run;No needs-human label on this issueends the run;needs-human = absentends the run.already-claimedhad the same hole plus an extra heuristic: any reply that said "already claimed" near "agent-working" or "claimed-by" also ended the run, so a step explaining the claim rule killed itself.The fix
needs-humanandalready-claimednow accept only two structured signals:EARLY-END needs-human/EARLY-END already-claimed(case-insensitive; the line may carry no other prose).Quoting, negation,
= absent, and a directive inside a sentence are prose and no longer end the run. The whole-value rule isv === reasonfor those two (graph.js:450); the substring path is what the review asked to remove and it is gone.Scope held to those two reasons, as the review required.
tip-unchanged(#1570) andopen-pr(#1571) keep the same structured signal and their own prose fallbacks, so a Bot that forgets the marker cannot burn a loop or anonFailattempt on the same tip: their regexes (/tip[\s_-]*unchanged|unchanged[\s_-]*tip/, the fouropen prphrasings) are untouched, and so is every word ofearly-end-tip-and-pr.test.js(7 cases, green). This PR does not restructure all four reasons — that would be a different, owner-visible design change.run.earlyEndand the status format are unchanged;graph.jsnets 8 added / 12 removed.Red -> green
Base
9b866a5c797e8efe95e445885b69da4b8ca5c35b, with the source reverted to the basegraph.jsand only the new tests kept,node --test plugins/workflow/test/early-end-signal.test.js:# tests 8 / # pass 3 / # fail 5. The three that pass on the base are the "must end" cases (ends the run when a declared output equals the marker,…on a line that owns the directive,…the already-claimed directive); the two that pass there are what keeps a stricter predicate from over-correcting. An earlier measurement of this file against base62023599showed six red — one "must end" case was red there and main's own later change made it green, which is why the count is re-measured here rather than copied forward.At this tip: the new file 8/8,
claim-and-needs-human.test.js+early-end-tip-and-pr.test.js14/14,plugins/workflow144/144.Tests touched besides the new file
claim-and-needs-human.test.jscarried#1538: needs-human detection is case-insensitive and works in text body, which pinned exactly the behaviour this PR removes: a paragraph of prose containingNEEDS-HUMANended the run. It is now#1538/#1546: the early-end directive is case-insensitive and line-owningand sendsEarly-End NEEDS-HUMANon its own line, so case-insensitivity stays covered while prose no longer does (3 added / 3 removed).Docs and changelog
docs/subsystems/workflow.md:49(invariant 3) is rewritten on live main's text: the section opensEarly termination (#1537, #1538, #1546, #1570, #1571)and says outright thatneeds-human/already-claimedend a run only on an equal OUTPUT or a whole-lineEARLY-END <reason>, while quoting, negating or reporting the label keeps the run going; then thattip-unchanged/open-praccept the same structured signal and keep their own prose fallbacks.changelog.d/1546.fixed.mdrecords the fix for #1539's regression.Shape
Tip
66232a9628547b9fff7660b927d2f37bfa39122d, treeeba3987f21ba0f363ee1e53786eeb70aba1b2902, base9b866a5c797e8efe95e445885b69da4b8ca5c35b; one commit, 5 files, +152 -16 (graph.js8/12,early-end-signal.test.js139/0,claim-and-needs-human.test.js3/3, docs 1/1, changelog 1/0). The branch merges cleanly into currentmain(c1b9fd23) with no conflict.Checks
TZ=UTC ARCH_DOCS_BASE=9b866a5c npm run verify— exit 0: core 1868, social 527, tree 125, timeline 125, workflow 144,.github/scripts548, interop 37, live-e2e 24, all 0 fail; lint 0 errors (13 workflow warnings, the same batch as live main);arch-docs: OK (5 changed file(s));ds-bot-patches10 patches apply in order andcore/equalsds-bot/+ patches;changelog.mjs check63 fragments.Not run: no live DSH instance probe and no manual CLI run beyond the suites above — the early-end path is covered through
service.advance()in the tests rather than a spawned Bot.Checkpoint
{ "pr": 1553, "claim": "Dev-4", "issue": 1546, "summary": "checkEarlyEnd ended a run on a raw substring match, so a reply quoting, negating or reporting the label, or explaining the already-claimed rule, killed the pipeline. needs-human and already-claimed now need a structured signal: an OUTPUT equal to the marker, or one reply line that is exactly the EARLY-END directive. tip-unchanged and open-pr keep that signal plus their prose fallbacks, scope unchanged. Eight new cases: five red on the base, eight green on the tip, workflow 144/144.", "files": [ "plugins/workflow/src/graph.js", "plugins/workflow/test/early-end-signal.test.js", "plugins/workflow/test/claim-and-needs-human.test.js", "docs/subsystems/workflow.md", "changelog.d/1546.fixed.md" ], "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=9b866a5c npm run verify", "result": "green", "at": "66232a9628547b9fff7660b927d2f37bfa39122d" }, "next": "none — ready for review" }