[kyle-sexton]

Refs: melodic-software/claude-code-plugins#9436

Summary

The hold step carries a recorded cost decision onto a later head from the
decided head: the sha= of this lane's newest cost hold marker posted before
the decision. If that newest marker was deleted, an older marker became the
decided head, so a decision could carry to a diff the owner never saw. This
applies the rule merge-holds.mjs deletedSince applies in
melodic-software/claude-code-plugins#9569 (owner decision Q18 d on #9436), so
both readers refuse the same carries.

Fix

• carryDecision() reads the pull request timeline's COMMENT_DELETED_EVENT
  items (GraphQL, last: 100, with filteredCount) after the marker's edit
  check and before the head check, so deleting a newer marker cannot promote an
  older head.
• It carries nothing when any comment was deleted at or after the marker's
  created_at, logged as "a comment was deleted since the decided-head marker
  was posted".
• It fails closed, logged as "could not confirm no comment was deleted since the
  decided-head marker was posted", when the read fails, the answer has no node
  list, filteredCount is not an integer, filteredCount exceeds the listed
  nodes, or a deletion time does not parse.
• Every existing no-carry rule stays: no marker, an edited or unreadable marker
  (no older one stands in), an unusable base name, 300 or more compared files, a
  missing patch, a duplicate filename, a failed compare.
• The pr-review-cli.yml hold-step contract comment states the rule.

Verification

• node --test .github/scripts/apply-review-hold.test.cjs: 42 pass (38 before).
  New cases: a deletion after the newest marker, with an older marker at the
  head's sha left, holds with no compare; a deletion at the marker's own time
  holds and one before it carries; eight unreadable deletion lists (failed read,
  null timeline, no node list, no count, a string count, 101 counted with 100
  listed, an unparsable time, an event with no time) hold; an edited marker reads
  no deletion list. Three call-sequence tests now expect the deletion read.
  Against main's hold.cjs, 6 fail: the 3 new behavior cases and the 3 updated
  call-sequence cases.
• node --test .github/scripts/pr-review-cli-api-calls.test.cjs: 10 pass.
  node --test .github/scripts/pr-review-cli.test.cjs: 24 pass.
• biome ci --error-on-warnings on the test file with biome 2.5.15 locally (CI
  pins 2.5.11): clean. The workflow change is comment-only; CI's actionlint job
  covers it.
• Left out: a live run against a PR with a deleted marker.

Related

• fix(pr-merge): bind a cost decision to the pull request it names claude-code-plugins#9569, the merge-holds reader with the
  same rule.
• fix(pr-pipeline): carry an owner-lifted claude-review cost hold forward across base-only merges claude-code-plugins#9436, where the cost-decision carry was
  designed.

🤖 Generated with Claude Code

[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	🔄 Running since 2026-10-24T14:56:45.629143Z	0f171b3	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 6s —— View job

---

Security review complete

• Gather context (diff, PR description)
• Skip-gate check — not eligible: this is an authorization/trust-logic fix (carryDecision), so it needed full review
• Review hold.cjs changes for security issues
• Review test file and workflow comment changes
• Report findings

Scope: .github/actions/apply-review-hold/hold.cjs, .github/scripts/apply-review-hold.test.cjs, .github/workflows/pr-review-cli.yml (comment-only).

Analysis: The fix adds deletedSince(), a new GraphQL read (DELETED_QUERY, parameterized — no injection surface) over the PR timeline's COMMENT_DELETED_EVENT items, consulted in carryDecision() before the head-sha/diff comparison, exactly as the PR description states. I traced the exploit path it closes: decided/held is picked as the newest cost-hold marker still present in the live prComments list (L282-L291). If the true newest marker was deleted, an older one is silently promoted to "decided head," and the owner's cost decision could then carry onto a diff they never reviewed. Since any such promoted marker is necessarily older than the deleted one, and a deletion event's createdAt can never precede the deleted item's creation time, deletedSince(held.created_at) is guaranteed to observe the deletion and return "deleted", blocking the carry. This reasoning matches the new test "a comment deleted after the decided-head marker keeps the cost hold, with no older marker standing in".

Failure modes are handled fail-closed, not fail-open: a failed GraphQL read, a missing/non-array node list, a non-integer filteredCount, filteredCount exceeding the listed nodes (truncation from the last: 100 window), or an unparsable createdAt all return "unknown", which carryDecision treats the same as a confirmed deletion (hold stays). This is the safe direction — the worst outcome is an unnecessary hold, never an unwarranted carry. The markerLine and DELETED_QUERY use parameterized GraphQL variables (no string-built query injection), and LANE_ID/HEAD_SHA are validated by regex earlier in run() before being interpolated into markerLine's regex (pre-existing, unchanged by this diff). The workflow file change is a contract-comment update only — no trigger, permission, or secrets handling changed.

No security-relevant gaps found in this diff. No CRITICAL/IMPORTANT/SUGGESTION findings to report.

critical-findings: 0
· branch

claude-lane-summary: lane=claude-security-review sha=0f171b3d338c5cc53317f175c319990d322a8b91 run=40104734217

[claude]

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

---

Code review

• Gather context (diff, CLAUDE.md, REVIEW.md, path-scoped rules, PR context)
• Review hold.cjs changes
• Review test file changes
• Review workflow comment change
• Check pr-body-contract.md rule applicability
• Post findings

No blocking or advisory findings.

What I checked:

• deletedSince() (hold.cjs#L107-L127): the fail-closed branches (items null, no node list, non-integer filteredCount, filteredCount exceeding the listed nodes, an unparsable deletion time) all short-circuit via || before any property access on a possibly-null items, so there's no null-deref risk on the "no timeline" / "no node list" paths. The boundary (t >= when, "at or after") matches the two new boundary tests (deletion at the marker's own timestamp holds; one second earlier carries).
• carryDecision()'s new deletedSince call (hold.cjs#L299-L306) runs after the edit check and before the decided/head-sha comparison, which is what prevents a deleted newer marker from promoting an older one — matches the stated intent and the call-sequence tests.
• The rule is intentionally broad (any comment deleted on the PR at or after the marker's time blocks the carry, not just deletion of the marker itself). That could in principle make the carry permanently unavailable on a PR with routine bot delete-and-repost comment traffic, but this repo's own actions never call a comment-delete API (deleteComment/issues.deleteComment — no matches under .github/), and the broad rule mirrors the already-decided design in merge-holds.mjs's deletedSince (owner decision Q18 d on claude-code-plugins#9436) cited in the PR body, so I'm not flagging it as a defect here.
• Test additions line up with the new branches (deleted-after-marker, deleted-at vs. just-before the marker, each unreadable/ambiguous deletion-list shape, and the edited-marker short-circuit that skips the deletion read). The updated call-sequence assertions correctly insert the new graphql call.
• The pr-review-cli.yml comment-only update states the new rule accurately against the code.
• .claude/rules/pr-body-contract.md carries no paths: frontmatter, so it isn't a path-scoped rule for this diff's files; not applied.

Reviewed as the code-review lane; security-relevant concerns (none found) would be left to the separate security lane per REVIEW.md.

Findings reported: 0

claude-lane-summary: lane=claude-review sha=0f171b3d338c5cc53317f175c319990d322a8b91 run=40104734217

[github-actions]

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

[github-actions]

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

[claude]

Claude finished @kyle-sexton's task in 22s —— View job

---

Code review (incremental)

• Gather context (incremental diff, PR context, path-scoped rules)
• Review the change since 0f171b3
• Apply skip gate
• Report findings

Scope: .github/scripts/apply-review-hold.test.cjs, the only file changed since this lane's last review at 0f171b3.

Skip gate applied: the incremental change is a comment-only reword — it drops the melodic-software/claude-code-plugins#9436 tracker reference from a block comment above a test, leaving the described behavior (and the test itself) unchanged. No code, assertion, or test logic changed. This has no meaningful review surface, so per the skip gate this lane reports nothing further.

No blocking or advisory findings.

claude-lane-summary: lane=claude-review sha=1a570ecf75dff314dcddd642089e5144eaaf64cd run=40105205946

[claude]

Claude finished @kyle-sexton's task in 24s —— View job

---

Security review complete

• Gather context (incremental diff, PR context)
• Skip-gate check
• Report findings

Scope: incremental review since 0f171b3, limited to .github/scripts/apply-review-hold.test.cjs per the lane's REVIEW SCOPE header. The change (commit 1a570ec) is:

-// one the decided head (owner decision Q18 d on
-// melodic-software/claude-code-plugins#9436, as merge-holds.mjs deletedSince
-// reads it): any comment deleted at or after the decided-head marker was
-// posted carries nothing. The pull request timeline's COMMENT_DELETED_EVENT
+// one the decided head (as merge-holds.mjs deletedSince reads it): any
+// comment deleted at or after the decided-head marker was posted carries
+// nothing. The pull request timeline's COMMENT_DELETED_EVENT

This drops a cross-repo issue-tracker reference from a code comment above a test case. No test logic, assertion, fixture, or behavior changed — only prose in a comment. It touches no instruction surface (CLAUDE.md/AGENTS.md/rules/skill/agent/hook list), no plugin manifest, and no runtime or workflow code.

Skip gate: triggered — this diff has no security-relevant surface (gate item 2). No findings to report for this increment.

manifest: no manifest files changed.

critical-findings: 0
· branch

claude-lane-summary: lane=claude-security-review sha=1a570ecf75dff314dcddd642089e5144eaaf64cd run=40105205946