[kyle-sexton]

Refs: #10848

Summary

The security review on #10848 (#10848 (comment)) found two gaps where address-feedback on an adopted owner PR could act against a person's hold:

• Gap 1: a Hold: body line added after the dispatcher adopted an owner PR was not re-checked when the round started. select() (address-feedback.mjs) now re-checks the Hold: line on owner PRs, as apply-patch.mjs:256-264 already does before the push.
• Gap 2: owner-PR address-feedback ignored lane escalations (a lane-escalated human-gated label). It now reuses fix-ci's prHold rule (gate-source.mjs), so an owner PR is worked only as that rule lets fix-ci work it.

Fix

• address-feedback.mjs select(): on an owner PR, a body line starting Hold: (the HOLD_LINE regex, now defined here and re-exported by feedback-dispatch.mjs for fix-ci-dispatch.mjs) skips the round; on an owner PR it gates with prHold instead of passing lane escalations through.
• feedback-dispatch.mjs: the dispatcher gates owner PRs with the same prHold call and its review reader, so dispatch and select agree.
• docs/conventions/pr-pipeline/README.md states both rules.

Verification

• node --test run inside .github/scripts/pr-unblock/ (the package's own test script) after merging origin/main: 980 tests, 980 pass, 0 fail.
• New cases in address-feedback.test.mjs and feedback-dispatch.test.mjs cover a late Hold: line and a lane-escalated human-gated owner PR.
• Diff checked for secrets and machine-specific paths before the push: none.
• Left out: the literal node --test .github/scripts/pr-unblock/ from the repository root, which Node v24.21.0 refuses with MODULE_NOT_FOUND (it treats the directory as a module); the same tests ran from inside the directory instead.

Related

• Security review that found both gaps: feat(pr-pipeline): answer lane review threads on owner-login PRs from the lanes App #10848 (comment)

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q3ucKDVyHvpcU5cfU8

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-29T14:01:12.154789Z	c779ccb	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[github-actions]

claude-security-review reviewed c779ccbcca344ebccd3b341948323f6010a0227b: 0 finding(s), 0 as inline comments.

No security findings. critical-findings: 0

I read the full diff, the PR context, and the unchanged prHold, laneEscalationAllows and reviewReader code in gate-source.mjs. I did not run the tests.

This change only narrows what address-feedback may do on an adopted owner PR. It touches address-feedback.mjs, feedback-dispatch.mjs and docs/conventions/pr-pipeline/README.md, which trigger the trusted-path and merge-hold scrutiny.

• Late Hold: line. select() now returns skip with awaiting-human when an adopted owner PR's body has a Hold: line, before any thread is read or kept (address-feedback.mjs:620-623). This closes the window between dispatch and round start. The regex is moved unchanged to address-feedback.mjs:1139. feedback-dispatch.mjs re-exports it, and HOLD_LINE is only used inside functions, so the circular import is safe.
• Lane escalations on owner PRs. An owner PR carrying the human-gated label is now gated by prHold in both the dispatcher and select(), the same rule fix-ci uses.
  • A lane escalation lets the round work only when address-feedback did not escalate and no person has spoken since the label went on.
  • select() passes the full review comments and reviews rather than a since-filtered read. Extra entries can only add holds, so this is the stricter direction.
  • prHold holds when the lanes App id is invalid, the gate label is unreadable, or the review read fails.
  • When held, requesters is set to the write-access fixers, so only threads a trusted person with write access asked the lane to fix are worked.
• Lanes App PRs. These still go through gateSource only, as before. Hold: a human merges this on an App PR does not skip the round.
• New REST helper. pages() and the two review listings (feedback-dispatch.mjs:815-872) are read-only GETs on pulls/<n>/comments and pulls/<n>/reviews. The since value is encodeURIComponent-encoded, the helper stops at 50 pages and fails closed by throwing, and prHold treats a throw as a hold. The 50-page cap and the extra reads are an API-budget matter, not a security one.
• Comment text. prHold reads the review-comment and PR body text only through regex tests and structured fields, and nothing is interpolated into a command.
• Dispatcher gate. In dispatchFeedback, owner is pull.user?.id !== appSenderId, and a non-adopted owner is skipped earlier (feedback-dispatch.mjs:353). So the prHold branch is reached only for adopted owner PRs, matching ownerAuthored in select().

Nothing here widens who is trusted, a token's scope, the kill switch, or a merge hold. The behaviour change only adds holds.

No instruction-surface deletions and no plugin manifest changes are in the diff, so there are no manifest <field> lines.

Spend: none of this changes models, turn limits or schedules.

[github-actions]

claude-review reviewed c779ccbcca344ebccd3b341948323f6010a0227b: 0 finding(s), 0 as inline comments.

No blocking or advisory findings. I read the diff and the PR context, and checked the changed select() and dispatcher code against gate-source.mjs and escalation-routes.json. I did not run the tests. The 980-test pass in the PR description is author-claimed and unverified by me.

What I checked

• Hold line. select() now skips with awaiting-human when an owner PR's body has a Hold: line. That mirrors ownerSkip at dispatch. It is limited to owner PRs, so a lanes App PR's own Hold: a human merges this still doesn't block the round (the new test covers this).
• Owner-PR gate. Both select() and feedback-dispatch.mjs gate an owner PR through prHold with holdLabels: []. Both pass the same activity string, "address-feedback". That matches the activity values in escalation-routes.json, so an address-feedback escalation such as feedback-round-cap holds the PR. The two sides agree.
• Non-owner path. select() computes gateSource(...).source !== "lane-escalation", which equals the old !escalated. The dispatcher's non-owner branch is the same logic with why kept for the skip message. Lanes App PR behavior is unchanged.
• Failure handling.
  • prHold treats a throwing review reader as a hold.
  • The new pages() helper in feedback-dispatch.mjs throws past 50 pages, which also ends in a hold.
  • select() passes the review comments and reviews it already fetched, so it makes no extra API calls.
• HOLD_LINE move. HOLD_LINE now lives in address-feedback.mjs and feedback-dispatch.mjs re-exports it. fix-ci-dispatch.mjs still imports it from feedback-dispatch.mjs. feedback-dispatch.mjs already imported from address-feedback.mjs, so no new module is loaded.
• Tests, changelog and docs.
  • Both test files add cases for a late Hold: line and a lane-escalated owner PR. The cases cover another lane's escalation, address-feedback's own escalation, a person speaking since the label, a write-access fix request, and a lanes App PR.
  • The .changes fragment's patch bump fits a fix.
  • The README text matches the code.

Security-sensitive paths in the diff: the touched files address-feedback.mjs and feedback-dispatch.mjs match the trusted-path list. The change only tightens when a round may run on an owner PR, and I found no weakening of a gate.

This change spends no paid plan, billing or other charge-bearing setting, so I set no hold.

[melodic-automation-lanes]

The unattended merge path enqueued this pull request at head c779ccbcca344ebccd3b341948323f6010a0227b.