[kyle-sexton]

Refs: #7349

Summary

Some agent-ready items are human-merged: an owner comment on the issue says the PR carries Hold: a human merges this. /work-items:work and /work-items:ship run never put that line in the PR body, and since #7347 they ready the PR right after create, so the babysit merge gate (BODY_MERGE_HOLD_RE, #7349) saw no hold.

Fix

• New plugins/work-items/scripts/merge-hold.sh --repo <owner/name> --issue <n>: reads the item's comments, and for each one containing Hold: a human merges this reads the author's repository permission. Only admin or write (maintain maps to write, triage to read) holds, the same trust rule as the work-loop's label-provenance. Exit 0 prints the line; 1 no hold; 11 a read failed, so a hold was not ruled out.
• work/SKILL.md open-the-PR step: after create --pushed and before ready, run the script; on 0 add the line to the body by read-modify-write; on any exit but 0 or 1 (a failed read, a usage error, missing gh) skip ready and leave the PR a draft. ship/actions/run.md points at the same step.
• Changelog fragment for work-items (patch).

The line is added right after create rather than inside create --pushed, which has no input for extra body lines; changing that would touch source-control. The PR is still a draft at that point, so nothing can merge before the line lands.

Verification

• merge-hold.test.sh (gh stub): admin and maintain commenters hold; triage and read do not; an untrusted line beside a trusted one holds; a missing line, no comments and a different-case line do not; failed comment or permission reads exit 11; a failed permission read beside a writer still holds; missing gh exits 8; usage errors exit 2. Widening the trust rule to read fails 2 cases.
• Live: merge-hold.sh on fix(guardrails): treat the heredoc body of git commit -F - and git tag -F - as data in block-root-delete-target #7379 (owner comment) prints the line; on fix(source-control): hold the babysit merge on a body line starting Hold: #7349 exits 1.
• scripts/check-affected.sh --tests --base origin/main: 8 passed, 1 skipped. check-skill-portability.sh: no unexcused tokens. shellcheck clean.
• Left out: an end-to-end /work-items:work run on a held item.

Related

• feat(work-items): ready the PR after the pre-PR gate in work #7347, fix(source-control): hold the babysit merge on a body line starting Hold: #7349

🤖 Generated with Claude Code

https://claude.ai/code/session_01W6SjPvY7DmXBGzboeCq8nj

[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-13T22:29:49.881591Z	33b3969	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.

[claude]

Claude finished @kyle-sexton's task in 2m 59s —— View job

---

Security review

• Gather context (diff, changed files)
• Review merge-hold.sh for injection / trust-boundary issues
• Review merge-hold.test.sh
• Review SKILL.md / run.md changes
• Review changelog fragment
• Post findings

No security issues found.

What I checked:

• plugins/work-items/scripts/merge-hold.sh is the only new logic with an external trust boundary (GitHub issue comments). Inputs are tightly validated: --repo and --issue are regex-anchored before use, and each candidate comment author's login is regex-validated (^[A-Za-z0-9][A-Za-z0-9-]*(\[bot\])?$) before being interpolated into the gh api repos/$REPO/collaborators/$login/permission call — this blocks path-traversal/argument-injection via a crafted login even though gh api already treats it as a single argument, not shell text.
• The trust decision itself does not trust comment content for authorization — it only trusts the comment's substring match for intent, then independently re-checks the author's current collaborator permission via the GitHub API (admin/write only; write required, matching the existing label-provenance trust rule). An unprivileged commenter quoting the hold phrase cannot force a hold, and a privileged commenter's historical comment is still gated by their live permission at read time.
• Failure handling is fail-closed in the safe direction: a failed comment read or a failed permission read (exit 11) is treated as "hold not ruled out," and work/SKILL.md/ship/actions/run.md wire any exit other than 0/1 to skip ready and leave the PR a draft — so a transient API error or gh outage can't accidentally let a held item slip through to merge-eligible.
• The PR-body line is only ever added before ready is invoked (the PR is still a draft at that point), so there's no window where the babysit merge gate could observe a body lacking the hold.
• merge-hold.test.sh is an offline fixture-driven stub (no eval, no untrusted interpreter invocation) and doesn't execute attacker-controlled content.

No command/argument injection, no authorization bypass, and no unsafe handling of tokens/secrets found in this diff.

claude-lane-summary: lane=claude-security-review sha=33b3969120cec2234c1a286481414b9090982178 run=38477891117

[claude]

Claude finished @kyle-sexton's task in 3m 50s —— View job

---

Review checklist

• Gather context (PR description, diff, changed files)
• Read plugins/work-items/scripts/merge-hold.sh and its test
• Read the work/SKILL.md and ship/actions/run.md doc edits
• Cross-check against REVIEW.md criteria and the babysit merge gate (BODY_MERGE_HOLD_RE, check-ready-holds.sh)
• Cross-check the trust model against the existing label-provenance.sh convention it mirrors
• Post findings

Findings

None at the high-signal bar for this lane (correctness, design, error handling, tests, docs — security is scoped to the dedicated security lane, which runs in this repo).

Notes from the review:

• merge-hold.sh's trust rule (admin/write hold, maintain→write, triage→read, same login_re) matches the existing pattern in plugins/work-items/tools/work-item-tracker/adapters/github/label-provenance.sh, so it's consistent with precedent rather than inventing a new trust mechanism.
• The generated Hold: a human merges this line satisfies BODY_MERGE_HOLD_RE's line-start hold: match in babysit_merge.py, and check-ready-holds.sh explicitly excludes Hold: lines from blocking the ready flip itself — so the documented flow (add the line, still call ready, let the babysit merge gate hold the actual merge) is internally consistent with both existing scripts.
• Exit-code handling in the script (0/1/2/8/11) and the orchestrator's "any exit but 0 or 1 skips ready" rule in work/SKILL.md fail safe (draft) on every ambiguous case I traced, including a bad login on an otherwise-only hold comment and a failed permission read sitting next to a trusted one.
• Author-claimed, not independently verified here: the test run (merge-hold.test.sh), check-affected.sh, check-skill-portability.sh, shellcheck, and the live run against fix(guardrails): treat the heredoc body of git commit -F - and git tag -F - as data in block-root-delete-target #7379/fix(source-control): hold the babysit merge on a body line starting Hold: #7349. This lane's sandbox denies Bash execution, so I read the script and test logic statically rather than re-running them.

claude-lane-summary: lane=claude-review sha=33b3969120cec2234c1a286481414b9090982178 run=38477891163

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 33b3969120

ℹ️ 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".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

claude-security-review has reviewed this pull request through a4480d1; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through a4480d1; a later push is reviewed from there.

[claude]

Claude finished @kyle-sexton's task in 1m 15s —— View job

---

Security review

• Gather context (incremental diff, full file contents)
• Review merge-hold.sh changes (login_re widening, unread-login handling)
• Review merge-hold.test.sh changes (new/changed cases)
• Post findings

No security issues found.

Scope: per the review header, this lane last reviewed 33b3969; this pass covers only the delta since then in plugins/work-items/scripts/merge-hold.sh and plugins/work-items/scripts/merge-hold.test.sh (.claude-lane/incremental.diff), addressing the Codex P1 finding on EMU (managed-user) logins.

What changed:

• merge-hold.sh#L83: login_re now accepts _ (^[A-Za-z0-9][A-Za-z0-9_-]*(\[bot\])?$), matching GitHub's managed-user login form (name_shortcode).
• merge-hold.sh#L87-L90: a login that still fails the (now wider) regex is treated as a failed read (read_failed=1) rather than silently skipped — this is the actual fix for the P1: previously an unmatched writer's login caused the script to fall through to exit 1 ("not held"), which let work/ship run flip the PR to ready and let the babysit lane merge despite an explicit human-merge request. Now any login the regex can't validate forces exit 11 (unread → stays a draft), which is fail-safe in the correct direction.

Trust-boundary check: the widened character class (A-Za-z0-9_- plus the literal [bot] suffix) still excludes /, ., whitespace, and shell metacharacters, so the value interpolated into gh api "repos/$REPO/collaborators/$login/permission" can't path-traverse or break out of the collaborators-lookup endpoint, and gh api takes it as a single argument regardless. The permission decision still comes from a live gh api call keyed on the real .user.login from GitHub's own comments response, not from attacker-controlled comment text — unchanged by this diff. Both directions of the regex widening were fail-safe before (unmatched → not held, a correctness bug but not an authz bypass) and are fail-safe now (unmatched → unread/draft).

Test coverage: the two new/changed cases (merge-hold.test.sh:89-90) directly exercise both edges — a still-unmatchable login (a;b) now expects rc 11 instead of rc 1, and a valid EMU-style login (mona-cat_octo, admin permission) now expects rc 0 — consistent with the script's new logic.

No command/argument injection, no authorization bypass, and no unsafe handling of tokens/secrets in this delta.

claude-lane-summary: lane=claude-security-review sha=a4480d1acfd4365504258d3b0e146c6b819c3e12 run=38478558554

[claude]

Claude finished @kyle-sexton's task in 1m 19s —— View job

---

Review complete

• Gather context (PR description, diff, custom instructions)
• Read incremental diff (.claude-lane/incremental.diff) and current merge-hold.sh / merge-hold.test.sh
• Resolve project standards / paved paths (no standards index with a paved-path row found; nothing applies)
• Review changed lines for correctness, error handling, test coverage, maintainability
• Post findings

Scope

Per this lane's custom instructions, this is an incremental review covering only what changed since commit 33b3969 in:

• plugins/work-items/scripts/merge-hold.sh
• plugins/work-items/scripts/merge-hold.test.sh

This is the direct fix for the P1 Codex finding and the matching author finding on the prior commit: EMU/managed-user logins (name_shortcode) were rejected by login_re, silently dropping a legitimate writer's hold and readying the PR.

What changed and verification

• login_re widened from ^[A-Za-z0-9][A-Za-z0-9-]*(\[bot\])?$ to ^[A-Za-z0-9][A-Za-z0-9_-]*(\[bot\])?$ (merge-hold.sh:83), which now accepts the _shortcode suffix GitHub documents for Enterprise Managed Users, while still excluding / and other shell/path-unsafe characters — no widening of the trust boundary for the collaborator-permission path segment.
• A login that still fails the (now broader) regex is treated as unread (read_failed=1, falls through to exit 11 → draft stays) rather than silently ignored (previously fell through to exit 1 only if no other hold was found) — merge-hold.sh:87-91. This is the fail-safe direction both review comments on the prior commit asked for.
• Test coverage matches the fix exactly: a new mona-cat_octo fixture/case proves an underscore-bearing admin login now holds (exit 0), and the old "malformed login is ignored" (rc 1) case is correctly re-targeted to "unusable login refuses" (rc 11) for the a;b login.

Traced the loop/exit logic by hand against both new cases (success path exits immediately on a matching writer regardless of loop order; the read_failed flag is only consulted after the loop drains) and found it consistent with the stated exit-code contract. Did not execute merge-hold.test.sh myself (no Bash execution in this lane's sandbox) — the PR's own test-run claim is author-asserted, not verified here.

No correctness, error-handling, or test-coverage issues found in this incremental diff. No security findings (scoped to the security lane, which already reviewed this trust path on the prior commit and reached the same conclusion).

Findings: 0

claude-lane-summary: lane=claude-review sha=a4480d1acfd4365504258d3b0e146c6b819c3e12 run=38478558512