Skip to content

fix(pr-pipeline): post stop-resolved so only a cleared stop leaves the stalled-stop signal - #11552

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/10434
Oct 31, 2026
Merged

kyle-sexton merged 1 commit into
mainfrom
fix/10434

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #10434

Summary

Problem: a stop a person clears by hand stays listed as a stalled lane stop until the head moves, because the run posts the same Skipped: not-applicable check as unrelated skips.

The address-feedback and fix-ci select jobs now post a distinct stop-resolved skip where they find the stop gone, and the maintenance monitor counts only that skip as ending a stop.

Fix

  • stop-resolved joins the skip-reason enum in pr-pipeline.schema.json.
  • address-feedback.mjs and fix-ci-context.mjs return it where they set resolved: true. Every other skip keeps its reason, including a fix-ci skip before checks settle.
  • collect.mjs treats a Skipped: stop-resolved check started after the stop as ending it.

Three changes the issue does not name, for the review lanes to judge:

  • fix-ci-dispatch.mjs: retryable matched only the old title. fix-ci now posts stop-resolved on every settled-green head, so without this a base move would stop re-dispatching that head.
  • collect.mjs drops the whole-listing re-read and the draft guard, since the reason alone now settles it. This saves up to 3 reads per stop.
  • stop-resolved posts on any settled head, not only after a stop. Other readers of skip titles do not key on it: dependency-dispatch.mjs:51 reads review-dependency's check, and feedback-dispatch.mjs:204 and limit-stop.mjs match other titles.

Verification

  • Tests changed with code unchanged fail on main: report-check-run 1, address-feedback 3, fix-ci-context 2, fix-ci-dispatch 1, collect 1. With the fix all pass: report-check-run 40, decide 60, address-feedback 169, fix-ci-context 42, fix-ci-dispatch 74, collect 32.
  • collect.test.mjs covers both activities dropping off on stop-resolved, both staying listed on not-applicable, an older stop-resolved staying listed, and a listing read failing the test.
  • Schema readers pass: resolve-config 74, resolve 147, live-config 47, check-convention-yaml PASS=51, publish-feedback 136. No file under .github/workflows/ changes; pr-unblock.yml:874 passes the reason through.
  • Known gaps: a not-applicable skip posted before this merges does not end a stop, so that stop stays listed until a new dispatch or head. From reading the code, a pass-only address-feedback round with no stale notes posts no check, so a stop cleared only that way stays listed; this predates the change.

Left out: callers.test.mjs, which loads the schema but whose workflow inputs are unchanged; CI runs it.

Related

🤖 Generated with Claude Code

…e stalled-stop signal

The address-feedback and fix-ci selects posted not-applicable for a stop
they found gone, the same reason as an owner's untaken threads and a fix-ci
skip before checks settled, so the monitor could not tell them apart.

Add stop-resolved to the skip-reason enum and return it where the selects
set resolved. The monitor counts only that skip as ending a stop and no
longer re-reads the head's check listing. fix-ci-dispatch keeps reading the
skip as a run with nothing to fix, so a new base still dispatches the head.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 31, 2026 14:30
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

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-31T14:32:03.542642Z 56ff61c 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

Copy link
Copy Markdown
Contributor

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

No security findings in this change, so there are no comments and no hold. I read the diff and the PR context, and also grepped .github for other readers of the not-applicable and stop-resolved strings.

  • Trusted paths touched: address-feedback.mjs and fix-ci-context.mjs. Both changes only rename the skip reason to stop-resolved where resolved: true was already set. The conditions that set resolved are unchanged, and neither file touches a merge hold, token scope, trusted-actor check or kill switch. Two edits widen nothing:
    • The address-feedback.mjs edit changes only the reason string.
    • The fix-ci-context.mjs edit keeps the settled condition and returns not-applicable otherwise.
  • pr-pipeline.schema.json: the one change adds stop-resolved to the skip-reason enum. report-check-run validates reasons against that enum and posts neutral, so this adds a label and grants nothing.
  • fix-ci-dispatch.mjs: retryable now also accepts Skipped: stop-resolved as a nothing-to-fix skip. The per-head cap (maxRunsPerHead) and the other retry limits are unchanged, so a base move can't dispatch a head more often than before. I checked the dispatch path and found no new route to a token, a leak or a charge.
  • collect.mjs: the stalled-stop signal now counts only a stop-resolved skip as ending a stop, and no longer reads the head's full check listing. This is monitoring only and gates no merge or token.
    • The check_name query doesn't filter by posting app, so any app that can post a check named pr-unblock / <activity> titled Skipped: stop-resolved could hide a stalled stop from the signal. The old not-applicable and attempted tests had the same gap. A hidden monitoring signal is not a leak, compromise or charge, so it isn't a finding.
  • Other readers of skip titles:
    • dependency-dispatch.mjs:51 reads review-dependency's checks, not these.
    • The pr-unblock.yml clear-stop gating already keys on the stop-resolved output.
    • The other not-applicable hits (report-outcome.mjs, predicate.mjs, review-dependency.mjs) are unrelated to the two changed activities.
    • Nothing found treats not-applicable from these two activities as a merge or trust signal.
  • Manifests: no plugin.json or marketplace.json changes.
  • PR text: I found no instructions embedded in the PR description or the issue comment.
  • Spend: none. No workflow files, models, limits or schedules change.

critical-findings: 0

@github-actions

Copy link
Copy Markdown
Contributor

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

No findings. I read the full diff and the PR context, and grepped for other readers of the skip titles. I ran no tests, so the PR's test counts are author-claimed and unverified.

Closing issue #10434

  • The skip-reason enum gains stop-resolved. pr-pipeline.schema.json adds it, and the new report-check-run test covers it. Met.
  • address-feedback.mjs and fix-ci-context.mjs return stop-resolved where they set resolved: true. Every other skip keeps its reason. The fix-ci tests assert not-applicable for the in-progress and cut-short-listing cases. Met.
  • collect.mjs counts a Skipped: stop-resolved check started after the stop as ending it. The test covers both activities dropping off, not-applicable staying listed, and an older stop-resolved staying listed. Met.
  • No file under .github/workflows/ changes. Met.
  • "Other not-applicable skips still do not count" is covered by the 8602 and 8604 cases in collect.test.mjs.

The three changes the PR asks the lanes to judge

  • fix-ci-dispatch.mjs: adding STOP_RESOLVED to the retryable set is needed. Without it, a settled-green head that now posts stop-resolved would drop out of base-move re-dispatch. The new test covers it.
  • collect.mjs: the draft guard and the whole-listing re-read are no longer needed, because the reason alone settles it. A grep of collect.mjs found no leftover use of failingChecks, CHECK_PAGES, RESOLVES, NOT_APPLICABLE or draft. The stalledChecks budget comment is updated to match.
  • Posting stop-resolved on any settled head: in the non-test scripts under .github/scripts, the only other reader of Skipped: not-applicable is dependency-dispatch.mjs. It reads review-dependency's check, not these activities. I did not read the workflow files for a branch on this reason.

Known gap, stated in the PR: a not-applicable skip posted before this merges does not end a stop. That stop stays listed until a new dispatch or head. This is a transition cost, not a defect.

Spend: none. The change adds no billed resource and no extra lane runs.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 31, 2026
Merged via the queue into main with commit ce85e61 Oct 31, 2026
73 checks passed
@kyle-sexton
kyle-sexton deleted the fix/10434 branch October 31, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(pr-pipeline): let a hand-cleared pr-unblock stop drop off the stalled-stop signal

1 participant