Skip to content

fix(ci): clear a cost-only review hold the owner already decided #9408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kyle-sexton merged 9 commits into from
Oct 23, 2026
Merged

fix(ci): clear a cost-only review hold the owner already decided #9408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a7e0c30
fix(ci): clear a cost-only review hold the owner already decided
kyle-sexton Oct 23, 2026
5a2ee83
test(ci): build the decision fixture with jq
kyle-sexton Oct 23, 2026
bbaec80
fix(ci): satisfy actionlint's shellcheck on the cost-hold step
kyle-sexton Oct 23, 2026
64d0fa5
Merge branch 'main' into fix/9403-cost-hold-recorded-decision
kyle-sexton Oct 23, 2026
f3eb71b
fix(ci): read cost holds from the lanes' check-run notices
kyle-sexton Oct 23, 2026
eb5c79e
perf(ci): stop the cost-hold step at the cheapest read that decides it
kyle-sexton Oct 23, 2026
5601c09
fix(ci): accept a cost decision only on the PR or its one closing issue
kyle-sexton Oct 23, 2026
c77199e
Merge remote-tracking branch 'origin/main' into fix/9403-cost-hold-re…
kyle-sexton Oct 23, 2026
2587923
fix(ci): keep a cost hold whose lane run could have forged its notice
kyle-sexton Oct 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 198 additions & 5 deletions .github/workflows/pr-require-checks.yml
View file
Open in desktop

@github-actions github-actions Bot Oct 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pr-verify verifier failed head 258792331a8d5fd1991eb08e9493acf214ad4fe3. What it wrote, as data:

Summary: #9403 asks that a review lane's `cost` hold not stick when the PR's closing or linked issue has an owner-authored `**Decision recorded.** Cost:` comment newer than the PR's createdAt. The new ci-status step only clears the hold when the PR body links that comment, and it ignores linked (`Refs:`) issues. A decision recorded on the closing issue, which is the #9224 rule the issue cites, leaves the PR held unless someone also edits the body to cite it. The PR body itself says the motivating case, #9332, is still not fixed. The rest of the step is careful and well tested: it fails closed, keeps trust-gate and critical-security holds, reads holds from notices rather than comments, and vets lane workflows. But the done-when condition is only met in a narrower case than the issue asks for.
- The review comments on the PR are the owner's (author_id 153232337) and explain design choices. They are context, not spec. I saw no text trying to steer the reviewer.
- .github/workflows/pr-require-checks.yml:124: the step finds a decision only through a `#issuecomment-` link in the PR body (`[ -n "$links" ] || keep "the body links no comment"`). The spec counts a decision when the closing issue carries the comment, with no body citation. A decision must also be newer than the PR's createdAt, so a decision on the closing issue never clears the hold unless the body is edited afterwards. Fix: read the comments of the one closing issue (and of the PR) directly, and apply the same author, unedited, first-line and newer-than-createdAt checks, instead of requiring a body link.
- .github/workflows/pr-require-checks.yml:133: only GitHub's `closingIssuesReferences` (totalCount == 1) counts, and a `Refs:`-linked issue never does. The spec says "closing or linked issue", and its motivating case #9332 links #9306 only through `Refs:`. The PR body admits #9332 stays held. Either support a linked issue in a way that resists the unrelated-issue exploit (for example, require the issue to be referenced in the PR body outside HTML comments and the decision to be newer than createdAt, as the spec states), or get #9403's done-when amended to closing-issue-only before merging this narrowing.
- .github/workflows/pr-require-checks.yml:146: owner identity is any `human` on the base trusted-actor list, not the single owner account the spec names. The PR says this matches babysit's cost_decision rule, which the spec accepts. I could not verify that against babysit_merge_policy.py.
- .github/workflows/pr-require-checks.yml:3117: ci-status gains `checks: read` and its timeout goes from 7 to 9 minutes. Both serve the new step and look proportionate.
- .github/workflows/pr-require-checks.yml:236: the hold-lift step now skips when cleared=true. That also skips it when an untrusted account removed a cost-only hold that the owner's decision covers. This seems to be intended, but a human should confirm it.

Original file line number Diff line number Diff line change
Expand Up @@ -3110,18 +3110,25 @@ jobs:
# of own-gate retries, 7 s of status-write retries and the 90 s
# `rerun-wait-seconds` wait, about four and a half minutes, plus up to
# 15 s of retry sleeps for each of the hold-lift step's three reads and
# the review-lane step's four, under two minutes.
timeout-minutes: 7
# the review-lane step's four, under two minutes, plus the cost-hold step's
# reads: three for most pull requests, which stop at the label events; for a
# lane-labeled one whose body cites a decision, about ten, and two more per
# review run since the window opened.
timeout-minutes: 9
permissions:
# `yield-to-full-run` lists this workflow's runs on the head SHA, and
# `rerun-contract-only-siblings` re-runs the red contract-only ones once
# the full run records `success`. Under an explicit permissions block every
# unlisted scope defaults to none; write covers the read.
actions: write
# The cost-hold step reads the review lanes' hold notices from their
# check-run annotations.
checks: read
contents: read
# check-contract upserts the advisory linkage comment and moves the linkage
# label; ci-status records the `ci-lanes` commit status the carry-forward
# branch reads back on the next contract-only event.
# branch reads back on the next contract-only event. The cost-hold step
# comments and removes the human-gated label.
pull-requests: write
statuses: write
steps:
Expand Down Expand Up @@ -3249,6 +3256,191 @@ jobs:
msg="draft: lanes not run; the ready_for_review run lints and tests this SHA"
echo "::notice::$msg"
echo "$msg" >>"$GITHUB_STEP_SUMMARY"
# A COST HOLD THE OWNER ALREADY DECIDED COMES OFF. A review lane re-holds
# every new head, so its `cost` hold went back on after the owner had
# recorded the cost decision. This step clears a hold only when all
# of these hold, else it changes nothing and the two steps below judge
# the label as before:
# - The window opens at the newest removal of the human-gated label by a
# `human` of the base commit's trusted-actor list (ever, when none).
# - The label was added in it, and only by github-actions[bot] or
# claude[bot], each time followed within ten minutes by a held review.
# - Every hold a review lane applied in it has criteria `cost` alone. The
# holds are read from the `claude-lane-hold ... criteria=` notice the
# lane's status job records (any attempt, on this branch), never from
# the hold comment, which anyone with write access can edit or delete.
# A pull_request run takes its workflow from the pull request, so a
# run whose head changes either review-lane workflow from the base (or
# whose comparison lists 300 files, the API's cap) could forge that
# notice, and keeps the hold.
# - No lanes App comment was written and no comment was deleted in it: a
# lanes App escalation adds no label event to a label already on.
# - The pull request body, outside HTML comments, links a decision comment
# as babysit_merge_policy.py's cost_decision reads one: by a User on the
# trusted-actor list, not through an App, never edited, first line
# `**Decision recorded.** Cost:`, on the pull request itself or (written
# after the pull request) on the one issue in this repository that
# GitHub records it as closing. A body `Refs:` line or a second closing
# issue counts for nothing, so an owner decision on an unrelated issue
# cannot clear this hold.
# It then outputs cleared=true and the decision link, and, with the label
# on, posts one comment citing the decision and removes the label. A
# GITHUB_TOKEN removal starts no run, so the steps below read the label
# off in this run, and the hold-lift step skips on cleared=true.
# Unreadable input clears nothing; the step itself never fails the job.
# Pinned by scripts/ci-hold-cost-decision.test.sh, which runs this body.
- name: Clear a cost-only review hold the owner decided
id: cost_hold
if: ${{ !cancelled() && github.event_name == 'pull_request' }}
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
LANES_APP_ID: ${{ vars.AUTOMATION_LANES_APP_SENDER_ID }}
run: |
keep() {
echo "The cost hold stays for a person: $1."
exit 0
}
read_api() {
local attempt out
for attempt in 1 2 3; do
if out=$(gh api "$@" 2>"$RUNNER_TEMP/cost-hold.err"); then
printf '%s\n' "$out"
return 0
fi
! grep -q 'HTTP 404' "$RUNNER_TEMP/cost-hold.err" || return 2
[ "$attempt" -lt 3 ] || return 1
sleep $((attempt * 5))
done
}
[[ $BASE_SHA =~ ^[0-9a-f]{40}$ && $HEAD_SHA =~ ^[0-9a-f]{40}$ && $PR_NUMBER =~ ^[1-9][0-9]*$ ]] || keep "the event names no commit or pull request"
[[ $LANES_APP_ID =~ ^[1-9][0-9]*$ ]] || keep "AUTOMATION_LANES_APP_SENDER_ID is not set"
repo=$GITHUB_REPOSITORY
tracker=$(read_api -H 'Accept: application/vnd.github.raw+json' "repos/$repo/contents/.work-item-tracker.json?ref=$BASE_SHA") && rc=0 || rc=$?
[ "$rc" -ne 1 ] || keep "the tracker config is unreadable"
label=needs-human
if [ "$rc" -eq 0 ] && [ -n "$tracker" ]; then
label=$(jq -er '[objects | .config | objects | .role_labels | objects
| .["human-gated"] | strings | gsub("^\\s+|\\s+$"; "")][0] // ""
| if . == "" then "needs-human" else . end' <<<"$tracker") || keep "the tracker config is invalid"
fi
actors=$(read_api -H 'Accept: application/vnd.github.raw+json' "repos/$repo/contents/.github/standards/trusted-actors/trusted-actors.json?ref=$BASE_SHA") || keep "the trusted-actor list is unreadable"
humans=$(jq -ec '[.actors[] | select(.kind == "human") | .id | numbers] | select(length > 0)' <<<"$actors") || keep "the trusted-actor list names no person"
events=$(read_api "repos/$repo/issues/$PR_NUMBER/events?per_page=100" --paginate) || keep "the events are unreadable"
# The window opens at the newest removal by a trusted person.
since=$(jq -n --arg label "$label" --argjson humans "$humans" '[inputs[] | select(.label.name == $label
and .event == "unlabeled" and (.actor.id as $id | $humans | index($id))) | .created_at | fromdateiso8601] | max // -1' \
<<<"$events") || keep "the events do not parse"
# Cheap reads first: most pull requests stop here.
added=$(jq -nr --arg label "$label" --argjson since "$since" '[inputs[] | select(.label.name == $label
and .event == "labeled" and (.created_at | fromdateiso8601) > $since)]
| if length == 0 then "no label was added since a trusted person last removed it"
elif any(.[]; .actor.login != "github-actions[bot]" and .actor.login != "claude[bot]") then "someone other than a review lane added the label"
else "lane" end' <<<"$events") || keep "the events do not parse"
[ "$added" = lane ] || keep "$added"
pr=$(read_api "repos/$repo/pulls/$PR_NUMBER") || keep "the pull request is unreadable"
ref=$(jq -r '.head.ref | @uri' <<<"$pr") || keep "the pull request has no head branch"
links=$(jq -r '.body // "" | gsub("<!--[\\s\\S]*?(?:-->|\\z)"; "")
| match("https://github\\.com/([A-Za-z0-9-]+)/([A-Za-z0-9._-]+)/(?:issues|pull)/([0-9]+)#issuecomment-([0-9]+)"; "g")
| [.string, (.captures[] | .string)] | @tsv' <<<"$pr") || links=
[ -n "$links" ] || keep "the body links no comment"
# shellcheck disable=SC2016 # GraphQL variables, not shell
closing=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){closingIssuesReferences(first:2){totalCount nodes{number repository{nameWithOwner}}}}}}' \
-F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the closing issues are unreadable"
# The one issue in this repository the pull request closes, as babysit's
# cost_decision reads it: none when it closes more than one.
issues=" $(jq -r --arg repo "$repo" '.data.repository.pullRequest.closingIssuesReferences
| select(.totalCount == 1) | .nodes[]
| select((.repository.nameWithOwner | ascii_downcase) == ($repo | ascii_downcase)) | .number | tostring' <<<"$closing" 2>/dev/null) "
opened=$(jq -r '.created_at | fromdateiso8601' <<<"$pr") || keep "the pull request has no creation time"
decision='' decision_id=''
while IFS=$'\t' read -r link owner name number id; do
[ -n "$link" ] || continue
[ "$(tr '[:upper:]' '[:lower:]' <<<"$owner/$name")" = "$(tr '[:upper:]' '[:lower:]' <<<"$repo")" ] || continue
[ "$number" = "$PR_NUMBER" ] || [[ $issues == *" $number "* ]] || continue
comment=$(read_api "repos/$repo/issues/comments/$id") || continue
jq -e --arg number "$number" --arg pr "$PR_NUMBER" --argjson humans "$humans" --argjson opened "$opened" '
(.issue_url // "" | endswith("/issues/\($number)"))
and .user.type == "User" and .performed_via_github_app == null
and (.user.id as $id | $humans | index($id) != null)
and (.body // "" | sub("\\A\\s+"; "") | test("\\A\\*\\*Decision recorded\\.\\*\\*[ \\t]+Cost:"))
and ($number == $pr or (.created_at | fromdateiso8601) > $opened)' <<<"$comment" >/dev/null 2>&1 || continue
node=$(jq -r .node_id <<<"$comment")
# shellcheck disable=SC2016 # GraphQL variables, not shell
edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$node") || continue
jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$edited" >/dev/null 2>&1 || continue
decision=$link decision_id=$id
break
done <<<"$links"
[ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering it"
# Every hold a review lane recorded in the window, as {at, criteria}.
holds='[]' vetted=''
while read -r file check; do
runs=$(read_api "repos/$repo/actions/workflows/$file/runs?event=pull_request&branch=$ref&per_page=100" --paginate) || keep "the $file runs are unreadable"
while read -r id sha; do
[ -n "$id" ] || continue
[[ $sha =~ ^[0-9a-f]{40}$ ]] || keep "run $id names no head commit"
if [[ " $vetted " != *" $sha "* ]]; then
diff=$(read_api "repos/$repo/compare/$BASE_SHA...$sha") || keep "the changes of $sha are unreadable"
changed=$(jq -er '[.files[].filename] as $f
| if ($f | length) >= 300 then "too many files"
elif any($f[]; . == ".github/workflows/pr-review-hosted.yml" or . == ".github/workflows/pr-review-security-hosted.yml") then "a review-lane workflow"
else "" end' <<<"$diff") || keep "the changes of $sha do not parse"
[ -z "$changed" ] || keep "lane run $id ran on $sha, which changes $changed"
vetted+=" $sha"
fi
jobs=$(read_api "repos/$repo/actions/runs/$id/jobs?filter=all&per_page=100" --paginate) || keep "the jobs of run $id are unreadable"
for job in $(jq -nr --arg check "$check" --argjson since "$since" '[inputs.jobs[] | select(.name == $check
Comment thread
kyle-sexton marked this conversation as resolved.
and .completed_at != null and (.completed_at | fromdateiso8601) > $since) | "\(.id)/\(.completed_at)"] | unique[]' <<<"$jobs"); do
notes=$(read_api "repos/$repo/check-runs/${job%%/*}/annotations?per_page=100" --paginate) || keep "the annotations of job ${job%%/*} are unreadable"
holds=$(jq -nc --argjson holds "$holds" --arg at "${job#*/}" '$holds + [inputs[] | select(.annotation_level == "notice")
| .message | select(startswith("claude-lane-hold "))
| {at: ($at | fromdateiso8601), criteria: (capture("^claude-lane-hold lane=[a-z-]+ sha=[0-9a-f]{40} criteria=(?<c>[a-z,-]+) label=").c // "unparsed")}]' \
<<<"$notes") || keep "the annotations of job ${job%%/*} do not parse"
done
done < <(jq -nr --argjson since "$since" '[inputs.workflow_runs[] | select((.updated_at | fromdateiso8601) > $since) | "\(.id) \(.head_sha)"] | unique[]' <<<"$runs")
done <<<'pr-review-hosted.yml pr-review / claude-review-status
pr-review-security-hosted.yml pr-review-security / security-review'
comments=$(read_api "repos/$repo/issues/$PR_NUMBER/comments?per_page=100" --paginate) || keep "the comments are unreadable"
# shellcheck disable=SC2016 # GraphQL variables, not shell
deleted=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){timelineItems(itemTypes:[COMMENT_DELETED_EVENT],last:100){filteredCount nodes{... on CommentDeletedEvent{createdAt}}}}}}' \
-F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the deleted comments are unreadable"
verdict=$(jq -nr --arg label "$label" --argjson since "$since" --argjson holds "$holds" --argjson app "$LANES_APP_ID" \
--slurpfile events <(printf '%s\n' "$events") --slurpfile comments <(printf '%s\n' "$comments") \
--slurpfile deleted <(printf '%s\n' "$deleted") '
def t: fromdateiso8601;
([$events[][] | select(.label.name == $label and .event == "labeled" and (.created_at | t) > $since)]) as $added
| ($deleted[0].data.repository.pullRequest.timelineItems) as $gone
| if ($holds | length) == 0 then "no review lane recorded a hold since a trusted person last removed the label"
elif any($holds[]; .criteria != "cost") then "a review-lane hold since then is not cost-only"
elif any($added[]; (.created_at | t) as $at | any($holds[]; .at >= $at and .at - $at <= 600) | not) then "a label was added with no held review after it"
elif any($comments[][]; .user.id == $app and ([.created_at, .updated_at] | map(t) | max) > $since) then "the lanes App commented since then"
elif ($gone | type) != "object" or $gone.filteredCount > ($gone.nodes | length)
or any($gone.nodes[]; (.createdAt | t) > $since) then "a comment was deleted since then"
else "cost" end' 2>/dev/null) || keep "the events, holds or comments do not parse"
[ "$verdict" = cost ] || keep "$verdict"
{
echo "cleared=true"
echo "decision=$decision"
} >>"$GITHUB_OUTPUT"
echo "::notice::every review-lane hold on #$PR_NUMBER since a trusted person last removed '$label' is cost-only, and $decision records the owner's cost decision."
labels=$(read_api "repos/$repo/issues/$PR_NUMBER/labels?per_page=100" --paginate --jq '.[].name') || {
echo "::warning::could not read the labels of #$PR_NUMBER, so '$label' was not removed."
exit 0
}
grep -qxF -- "$label" <<<"$labels" || exit 0
marker="<!-- ci-status cost-hold-cleared decision=$decision_id -->"
if ! jq -se --arg marker "$marker" 'any(.[][]; .user.login == "github-actions[bot]" and (.body | startswith($marker)))' \
<<<"$comments" >/dev/null; then
gh api -X POST "repos/$repo/issues/$PR_NUMBER/comments" -f body="$marker
ci-status removed \`$label\`: every review-lane hold since a trusted person last removed it is cost-only, and the owner recorded the cost decision in $decision. A person adding \`$label\` back holds it again." >/dev/null ||
echo "::warning::could not post the removal comment on #$PR_NUMBER."
fi
# shellcheck disable=SC2016 # $l is a jq variable
gh api -X DELETE "repos/$repo/issues/$PR_NUMBER/labels/$(jq -rn --arg l "$label" '$l | @uri')" >/dev/null ||
echo "::warning::could not remove '$label' from #$PR_NUMBER; the step below holds it."
# A PULL REQUEST CARRYING THE HUMAN-GATED LABEL IS HELD. A review lane or
# an escalation puts the label on so a person decides; removing it is the
# approval, and the `unlabeled` event re-runs this job. The label's name
Expand Down Expand Up @@ -3335,10 +3527,11 @@ jobs:
# later: an event cannot be deleted or edited, so removing the comment
# does not remove the hold, and a review re-run that labels the same head
# again starts a new hold. Unreadable comments, events or actor list fail
# closed.
# closed. It skips when the cost_hold step cleared a cost-only hold, whose
# label that step (or anyone) may have removed.
# Pinned by scripts/ci-hold-lifted-by.test.sh, which runs this body.
- name: Fail a review-lane hold an untrusted account lifted
if: ${{ !cancelled() && github.event_name == 'pull_request' && steps.human_gated.outputs.held == 'false' }}
if: ${{ !cancelled() && github.event_name == 'pull_request' && steps.human_gated.outputs.held == 'false' && steps.cost_hold.outputs.cleared != 'true' }}
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
Expand Down
Loading
Loading