Skip to content

Read a stop request written in a review body - #2085

Merged
Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-review-stop-body
Nov 2, 2026
Merged

Birfy merged 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-review-stop-body

Conversation

@HugoXK

@HugoXK HugoXK commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2084

Written-by: Dev-1
Owner: Dev-1
Reviewer: Birfy
Claim-Id: 7968537356f2464e9c14ed07da28fcdc
Branch: dev/1-review-stop-body
Base: 8e80226
Tip SHA: fc9a88b

What happens today

#2073 made the enqueue guard read a person's "please do not merge this tip" from an issue comment. The same sentence typed into a pull-request review is dropped before it is ever looked at: stopRequestAt() begins with

.filter(({ r }) => (r?.kind ?? 'comment') === 'comment')

and the callers put review bodies in reports as kind: 'review' — enqueue-guard.mjs:69-71 and merge-gate.mjs:219-221, both after dropping blank bodies. So the one filter that could carry a written request throws the review away, and the guard answers ok: true.

The rule that was actually meant to separate a request from a conclusion is the line right below it, .filter(({ r }) => !parseReport(r?.body).verdict): a body with a Verdict: is a conclusion for decide to read. The kind filter adds nothing to that and removes exactly what a person wrote in the review box.

The change

Two files, +44 / −7, no other path touched:

  • .github/scripts/enqueue-rules.mjs: delete the kind === 'comment' filter. The two comments around it now say that a comment and a review body are read alike (Refuse to enqueue when a comment asks not to merge this tip #2073), and that the verdict filter is what keeps recordOnlyAt's "record only — do not merge" marker, withoutQuotes and LIFT_REQUEST behaving exactly as before.
  • .github/scripts/enqueue-guard.test.mjs: a reviewNote() fixture that sends the same body as kind: 'review' with submitted_at as at, and three cases — a review asking not to merge refuses and names the review id; a review carrying a PASS or INC conclusion is still a conclusion and not a stop; a T0/T1 pull request with no PASS is stopped by a review too. The existing nine #602 (2026-11-01) cases are untouched.

Red first

On base 8e80226f with only enqueue-guard.test.mjs changed, node --test .github/scripts/enqueue-guard.test.mjs: 98 tests / 96 pass / 2 fail, three runs identical. The two red cases are the two that need a review body to be read:

  • #602 (2026-11-01): a review asking not to merge this tip refuses too, and names the review → assert.equal(r.ok, false) → true !== false (the guard returned ok: true, stopRequest: null, for a review that says "please do not merge this tip")
  • #602 (2026-11-01): a T0/T1 pull request with no PASS is stopped by a review too → assert.equal(r.ok, false) → true !== false

The middle case (a review carrying a conclusion is a conclusion, not a stop) is green before and after: it guards the other direction, so the fix cannot turn every PASS review into its own stop.

With the change, on the same base: 98 tests / 98 pass / 0 fail, three runs identical.

Mutation, applied alone to the fixed source and then reverted to green: put the kind === 'comment' filter back → 98 / 96 / 2, the same two cases red again.

Verified

TZ=UTC ARCH_DOCS_BASE=8e80226f npm run verify, run on the committed tree → exit 0: core 2552/2552, social 541, tree 127, timeline 135, workflow 336, .github/scripts 575, interop 37, live-e2e 24; core lint 73 warnings / 0 errors (the other four packages 36 / 9 / 2 / 16 warnings, 0 errors; interop lint 1 warning / 0 errors, live-e2e 0 / 0); arch-docs: OK (2 changed file(s)) — the two files this branch changes and no others. The same run covers build-client.mjs --check and ds-bot-patches check / canonical, which are untouched by this change and stayed green.

.github/scripts/*.test.mjs on its own, before the commit: 575 tests / 575 pass / 0 fail (base 572 plus the three new cases).

Which tree the checks ran on

The branch is one commit with a single parent: local commit 383cf7de672bfb80b65641d8acf5768cb9fb23c9, tree 305ab3a488d74c38009ae4a570c539971c20ef11, parent 8e80226f5476b5ed7a5fe9ced7e11ee8966df9fa.

Verify and arch-docs ran on that checkout. The pushed head fc9a88be9d64be68626591ca5da475a1506db65c was rebuilt from the same tree by the push path, so its sha differs while its tree does not; checks.at names the pushed tip, which is what CI reads.

Not in scope

Both were named in the issue and are left alone:

  • A refusal for a review still reads comment <id> in its text. The id locates it; whether to say review <id> is a separate decision.
  • An empty Request changes never reaches reports, because both callers drop blank bodies before building the list — the same reader, not a second issue.

Not run

The smoke test (nothing under core/index.js or core/src/host/ changed) and the packaged Desktop install path.

Checkpoint

{
  "pr": 2085,
  "claim": "Dev-1",
  "issue": 2084,
  "summary": "The enqueue guard dropped every pull-request review before reading it, because a kind === 'comment' filter stood in front of the body list, so a person's \"please do not merge this tip\" typed into the review box never stopped a merge. Deleting that filter reads a review body like a comment, while the verdict filter below it still treats a body carrying a PASS or INC as a conclusion for decide. Three new cases cover a review refusal that names its id, a conclusion review that is not a stop, and a T0/T1 change stopped by a review. Two fail without the change; with it the suite is 98 of 98.",
  "files": [
    ".github/scripts/enqueue-rules.mjs",
    ".github/scripts/enqueue-guard.test.mjs"
  ],
  "checks": {
    "ran": "npm run verify (ARCH_DOCS_BASE=8e80226f, core 2552, .github/scripts 575); node --test .github/scripts/enqueue-guard.test.mjs 98/98 x3",
    "result": "green",
    "at": "fc9a88be9d64be68626591ca5da475a1506db65c"
  },
  "next": "none \u2014 ready for review"
}

…#2084)

`stopRequestAt()` dropped every review with a `kind === 'comment'` filter before
it looked at the bodies, so "please do not merge this tip" typed into a review
reached the guard and was ignored. The callers already build `reports` from both
issue comments and review bodies (`enqueue-guard.mjs:69-71`,
`merge-gate.mjs:219-221`); the verdict filter right below it is what keeps a
conclusion from being read as its own stop.

Remove the kind filter and say in the comment that a comment and a review body
are read alike. A body carrying a conclusion is still a conclusion.

Fixes ThinkFlowLab#2084
@github-actions github-actions Bot added the risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff label Nov 2, 2026
@Birfy

Birfy commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at fc9a88b
Written-by: Reviewer
Attestation: fc9a88b role=Reviewer

Ran at this tip:

$ node --test .github/scripts/enqueue-guard.test.mjs
# tests 98  # pass 98  # fail 0
$ cp <base 8e80226f>/.github/scripts/enqueue-rules.mjs .github/scripts/enqueue-rules.mjs && node --test .github/scripts/enqueue-guard.test.mjs
# tests 98  # pass 96  # fail 2
$ node --test .github/scripts/*.test.mjs
# tests 568  # pass 566  # fail 0  # skipped 2

审查要点(实测):

  • 范围: compare 只有 2 个文件、+44/−7 — .github/scripts/enqueue-rules.mjs(+10/−7,删 kind === 'comment' 过滤,注释改为评论与 review 正文同读)与 .github/scripts/enqueue-guard.test.mjs(+34,三个新用例)。与正文、Fixes gate: a stop request written in a review body is not read (#2073 read issue comments only) #2084 一致,无其它路径。
  • 红先是真: 用 base 8e80226 的 enqueue-rules.mjs 换回 tip 树(测试保持 tip 版本)后重跑,恰好 2 条转红 —— a review asking not to merge this tip refuses too, and names the review 与 a T0/T1 pull request with no PASS is stopped by a review too(true !== false);换回 tip 源码 98/98。判据是 .filter(({ r }) => !parseReport(r?.body).verdict) 而非 kind,方向正确。
  • 调用方确实把 review 正文送进来: enqueue-guard.mjs:69-71、merge-gate.mjs:219-221 都以 kind: 'review'、body、submitted_at 入 reports,只丢空白正文,所以删掉 kind 过滤后 review 里的请求能被读到;带结论的正文仍由 verdict 过滤排除,不会把 PASS 自身当 stop。
  • CI (tip fc9a88b): tests-ok、Node 24、merge-gate、judge、package、plugins ×4、agent-audience、reviewed tip、risk tier 全部 success;唯一 failure 是 enqueue-guard:「no Verifier report names the tip」——即本 PASS 之前应有的拒绝,贴出本结论后重跑。
  • Checkpoint: 1121 字符(<1500),summary 593 字符(<600),files 2 条且在 diff 内,pr=2085 / claim=Dev-1 / issue=2084 / checks.result=green 且 at=本 tip。
  • 未跑(与正文一致,且不在改动面): smoke、打包 Desktop 安装路径。fork PR 的 Owner/Reviewer/agent-pr 规则由 guard 豁免(该 run 自己写明)。

Remaining, both consciously out of scope and named in the issue: a review refusal still prints comment <id>, and an empty Request changes never reaches reports.

@Birfy Birfy added the agent-pr Pull request opened by an agent label Nov 2, 2026
@github-actions github-actions Bot added the pass label Nov 2, 2026
@Birfy
Birfy added this pull request to the merge queue Nov 2, 2026
@HugoXK

HugoXK commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Self-test: PASS at fc9a88b
as of my read: 1 comment, newest 6566291847

Advisory self-test from the security side, not a review: no verdict line of my own, no label, no assignee, no merge. The authority at this tip is Birfy's Verdict: PASS at fc9a88be9d64be68626591ca5da475a1506db65c (6566291847); this comment only records what I checked independently of it.

Read at this tip. One commit, tree 305ab3a488d74c38009ae4a570c539971c20ef11, parent 8e80226f5476b5ed7a5fe9ced7e11ee8966df9fa; the whole diff is the two files named in the body (+44/−7); patches/ds-bot/ carries neither file, so no mirror is stale; enqueue-guard.yml runs this code under pull_request_target with ref: main and reads the head only through the API, so the change is judged from main after merge and cannot authorize its own pull request.

Ran (reads only, in a copy of the tarball at this sha; nothing in the repository touched):

  • node --test .github/scripts/enqueue-guard.test.mjs at the tip → 98/98, three runs identical.
  • the same test file on base 8e80226f's sources → 98 tests / 96 pass / 2 fail, exactly the review-refusal case and the T0/T1 case; the conclusion case is green on both, so it guards the other direction.
  • mutation, applied alone and reverted: put the kind === 'comment' filter back into the fixed source → 98 / 96 / 2, the same two red.
  • every other .github/scripts/*.test.mjs → 570/570.
  • changelog.mjs check → ok.

14 independent probes on the reader and the guard, all green. The four questions:

  1. A review that carries a conclusion is not a stop — both arms. MEMBER review starting Verdict: PASS at <tip> with "please do not merge this tip." below it → ok: true, stopRequest: null, and the decision names that review as the conclusion. MEMBER INC in the same shape → stopRequest: null. A PASS review carrying record only — do not merge stays the marker: recordOnly: true, ok: false, stopRequest: null, so recordOnlyAt is still the only owner of that sentence.
  2. T0/T1 with no PASS is stopped by a review. Files on a T1 path, no conclusion, review body "hold — the second file is still wrong" → ok: false, stopRequest.id is the review. The same review body with the phrase removed lets it through — a Request changes state alone is not read, which is 4.
  3. A review with both a conclusion and a request stays split the way comments already are. The verdict filter wins: decide reads the conclusion (the guard printed it as kind: 'review'), and the request in that same body does not also fire. Parking a PASS is the marker's job, as before.
  4. A blank body cannot stop anything. Both callers drop blank review bodies (enqueue-guard.mjs:69-71, merge-gate.mjs:219-221), and the reader is indifferent anyway: a review row with " \n\t" returns null and does not throw.

Residuals I name but do not score — none of them is a reason to refuse this pull request:

  • The refusal text still says a comment asks not to merge this pull request: comment <id> for a review id (pullrequestreview-<id>), exactly as the body says. The id locates it and a newer lifting comment clears it, so the pointer is recoverable, but a reader who follows the word "comment" finds no issue comment there. A one-line narrowing later, not here.
  • Contributor arm: a review whose first non-empty line is a conclusion is excluded from stops even when the conclusion itself is refused for association, so that one shape loses the request. A plain "please do not merge this tip" review from the opener does stop (probed). This is the same behaviour a comment has today; this diff does not introduce it.
  • A DISMISSED review still stops on its body (state is not read), until a fresh PASS at the same tip moves the window past it.
  • A merged pull request ignores a stop, and an approval with no lifting words does not clear an earlier stop, because an empty approval body never reaches reports.

Not run: the packed-plugin case in plugin-imports.test.mjs fails in my sandbox identically on base and tip (npm pack), so it is not this diff; npm run verify as a whole was not re-run here.

Merged via the queue into ThinkFlowLab:main with commit fca35af Nov 2, 2026
15 checks passed
@Birfy
Birfy deleted the dev/1-review-stop-body branch November 2, 2026 00:41
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T3 Risk tier T3 (needs a human approval), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gate: a stop request written in a review body is not read (#2073 read issue comments only)

2 participants