Skip to content

feat(pr-pipeline): add a limits leaf for the limit-stop restart values - #7918

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/7764-pr-pipeline-limits
Oct 18, 2026
Merged

kyle-sexton merged 3 commits into
mainfrom
fix/7764-pr-pipeline-limits

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

Closes #7764

Summary

The limit-stop restart values from #7737 were hard-coded in three scripts. They are now an optional limits leaf of pr-pipeline.yaml. Its schema defaults equal today's values, so behavior does not change. This repository's pr-pipeline.yaml sets no limits block.

Keys follow the defaulted decision on the issue: fallback-reset (5), reset-clamp-min (1) and reset-clamp-max (168), all in whole hours, plus restarts-per-run (5), limit-stops-per-head (3) and limit-stops-per-issue (3).

Fix

  • Schema (docs/conventions/pr-pipeline/pr-pipeline.schema.json): new limits object, additionalProperties: false, integer leaves, each with minimum: 1 and a default. Each leaf also has a maximum: 720 for the hour leaves, 10 for both stop caps, 50 for restarts-per-run. The stop caps bound how often a forged limit stop can restart a lane, so a typo cannot lift that bound.
  • resolve-config:
    • resolve.mjs emits a resolved limits object next to loop. The defaults are inlined the way loop does it, and resolve.mjs gains no import.
    • resolveLimits(files, { configPath }) runs the same file checks as resolve() and returns only the limits.
    • resolve-config.mjs exports limitsFromBase(env), which reads the config from the base checkout.
  • .github/scripts/work-resolve-issue/limits.mjs (new, no package imports):
    • Exports LIMIT_DEFAULTS, readLimits(path) and clampReset(resets, from, limits). readLimits reads resolve-config's output file and falls back to the defaults.
    • The file sits beside report-outcome.mjs because that script runs from a copy of its own directory alone (report-outcome.test.mjs:1404-1406).
  • report-outcome reads limits from RESOLVED_CONFIG (default $RUNNER_TEMP/resolved-config.json), the file it already reads for denied paths. It re-exports clampReset, so label-unblocked.mjs keeps its import.
  • work-unblock sweep (label-unblocked.mjs) reads the same file, which work-unblock.yml:119 already writes. restarts-per-run caps restartLimited. restartRefusal uses limit-stops-per-issue and the resolved clamp. fix(pr-pipeline): don't leave issues parked on agent-ready after their lane PR ends #7889's refs hold, hand-off and restartRefusal guard are merged in, and the hand-off count also runs against restarts-per-run.
  • pr-unblock limit stop: limitResetsAt(run, limits) and anyHandles(runs, { now, limits }).
  • pr-unblock-dispatch:
    • pr-unblock-dispatch.yml gains an "Install resolve-config's dependencies" step. It is the same command as pr-feedback-dispatch.yml:79, under the dispatch step's own kill-switch and trusted-trigger condition.
    • dispatch.mjs resolves the base config's limits through limitsFromBase before any dispatch and passes them to anyHandles. A config the resolver refuses fails the run red before anything is dispatched, as feedback-dispatch already does. That includes the review-dependency dispatches in the same run.
  • feedback-dispatch.mjs: the existing check-run test now runs after the config resolves, so it can pass resolved.limits. The skip logic is the same, but a head that already has a check run now costs one resolve-config read before it is skipped.
  • Cleanup:
    • FALLBACK_RESET_MS, RESTARTS_PER_RUN and MAX_LIMIT_STOPS are gone, along with every source comment that pointed at this issue.
    • The MAX_LIMIT_STOPS comment in pr-run-activity-write.yml now names the two limits keys and reset-clamp-min.
    • The convention README has a Limits section, the CHANGELOG has a 2.8.0 entry, and the resolve-config README's Output paragraph is updated.

Verification

Tests were written first and run red before each change:

  • Consumer tests: limit-stop (2), report-outcome (1) and label-unblocked (1).
  • Resolve tests: the resolve.test default and override tests.
  • Dispatch tests: dispatch.test limits (1) and resolve-config/dispatch-limits.test.mjs (2). The second file runs dispatch.mjs's main against a real base checkout: limit-stops-per-head: 1 stops a re-dispatch that the defaults allow, and limit-stops-per-head: 0 fails the run with no dispatch.
  • The schema maximums were added test first: the reject test with values over each maximum failed, then passed.
  • The reject test (unknown key, 0, -1, 1.5 or a string under limits) passed before the change only because the root schema refused limits as an unknown key. It now exercises the leaf schema.

Results:

  • node --test .github/scripts/pr-unblock/*.test.mjs .github/scripts/work-unblock/*.test.mjs .github/scripts/work-resolve-issue/*.test.mjs .github/actions/resolve-config/*.test.mjs .github/actions/report-check-run/*.test.mjs: 924 pass, 0 fail. Existing expectations are unchanged; the dispatch.test main helper now injects the default limits.
  • actionlint on pr-unblock-dispatch.yml and pr-run-activity-write.yml: exit 0.
  • zizmor --offline on both: no findings (14 ignored, 6 suppressed, unchanged).
  • Schema check with Ajv 2020 and resolve-config's validator settings: docs/conventions/pr-pipeline.yaml and examples/claude-code-plugins.yaml are valid, and so is the live config with all six defaults set.
  • markdownlint-cli2 on the changed docs: 0 issues.
  • Security review (review:security-reviewer, opus) of the full diff found nothing at critical, high or medium. Points it confirmed:
    • Limits, the config path and the npm install come only from the refs/heads/main checkout in .base.
    • The new step matches pr-feedback-dispatch.yml:79: lockfile with exact pins, --ignore-scripts, no token in its env, and the same kill-switch and gate condition.
    • A forged or missing resolved-config.json cannot drop the restart floor below one hour.
    • report-outcome.mjs still loads from a copy of its own directory.
  • The review raised two low notes:
    • P4: the schema had no maximum on any leaf. Fixed in this PR with the maximums above.
    • P5: the dispatch step now runs the pinned ajv and yaml packages beside its GITHUB_TOKEN. That adds no power pr-feedback-dispatch.yml does not already grant, so it is accepted as is.

Basis:

  • pr-unblock-dispatch.yml:18-21: the dispatcher triggers on push and workflow_dispatch only. Its checkout is refs/heads/main into .base with persist-credentials: false, so the installed lockfile and the config are the default branch's.
  • pr-feedback-dispatch.yml:79: the same install command, already in use.
  • resolve.mjs loop block: the inline-defaults pattern copied here.
  • report-outcome.test.mjs:1404-1406: the own-directory copy constraint.
  • work-unblock.yml:119: writes resolved-config.json.
  • The five-hour fallback rationale is the existing comment's (https://support.claude.com/en/articles/11145838). I did not re-fetch it this session, so it is UNVERIFIED here.
  • The schema does not enforce reset-clamp-min ≤ reset-clamp-max; that hardening is deferred. Basis: judgment.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits October 17, 2026 22:41
pr-pipeline.yaml gains an optional `limits` block: fallback-reset,
reset-clamp-min and reset-clamp-max (hours), restarts-per-run,
limit-stops-per-head and limit-stops-per-issue, each defaulting to the
value the lanes hard-coded. resolve-config emits all six. report-outcome
and the work-unblock sweep read them from resolve-config's output file,
and pr-feedback-dispatch from the config it resolves; pr-unblock-dispatch
resolves no config and keeps the defaults. The shared defaults and reset
clamp sit beside report-outcome.mjs, which runs from a copy of its own
directory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-unblock-dispatch.yml installs resolve-config's dependencies the way
pr-feedback-dispatch.yml does, and dispatch.mjs resolves the base
config's limits before any dispatch, so limits.limit-stops-per-head and
the reset clamp reach both dispatchers. A config the resolver refuses
fails the run before anything is dispatched. The schema caps each leaf
(hours at 720, stop caps at 10, restarts per run at 50) so a typo
cannot let a forged limit stop restart a lane without bound, and the
pr-run-activity-write.yml comment names the limits keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #7889 and #7912. label-unblocked.mjs keeps #7889's
refs-hold import, hand-off and restartRefusal guard and threads the
resolved limits through them; the hand-off comment now names
limits.restarts-per-run. The limits CHANGELOG entry moves to 2.8.0
above main's 2.7.0.

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

chatgpt-codex-connector Bot commented Oct 18, 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-18T03:15:14.866070Z 713e81e 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 713e81e5901f0bcc13b36df2f0bccdb8a84b9a55: 0 finding(s), 0 as inline comments.

Security review: PR #7918 (head 713e81e)

No security findings. I read all 20 changed files, including tests and docs.

What I checked:

  • Where the limits come from. pr-unblock-dispatch.yml runs only from refs/heads/main. It checks out main into .base and runs npm ci --ignore-scripts --omit=dev, so the install runs no lifecycle scripts. The job has permissions: {} at the top level and a read-plus-actions: write job token. The new limits come from the base config, not from the PR head, so a PR cannot set them for itself.
  • Bounds on the values. The schema caps every value: durations at 720 hours, stop caps at 10, restarts-per-run at 50. resolveLimits and readLimits fall back to the defaults for anything that is not a positive integer, and the clamp still limits any restart to a bounded delay. A forged limit-stop record therefore stays bounded, as the old hard-coded values did.
  • The resolved-config file. report-outcome.mjs and label-unblocked.mjs now read limits from resolved-config.json. report-outcome.mjs already read that file for the hold paths, which are more sensitive, so the trust boundary is unchanged.
  • feedback-dispatch.mjs. The check-run lookup now runs after the resolve step. The existing check-run gate still runs before the dispatch call, and the order of the other gates is unchanged.
  • Instruction surfaces. The diff deletes no guardrail. It replaces the hard-coded FALLBACK_RESET_MS, MAX_LIMIT_STOPS, RESTARTS_PER_RUN and the clamp constants with the same defaults, now configurable and schema-bounded.

I did not read the instruction exception register, because nothing in this diff deletes or softens a standing instruction.

@github-actions

Copy link
Copy Markdown
Contributor

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

No blocking findings on head 713e81e.

Closing issue #7764. I checked each acceptance criterion against the diff:

  • The schema accepts a config with or without limits. It rejects unknown keys, non-integers and values below 1, and it also rejects values above each maximum (resolve.test.mjs).
  • With no limits leaf, resolve.mjs fills in all six default values, and a test asserts them.
  • Each consumer has a test that a non-default value changes its behavior:
    • limit-stop.test.mjs covers pr-unblock.
    • report-outcome.test.mjs covers report-outcome.
    • label-unblocked.test.mjs covers the sweep.
  • The old constants and the issues/7764 links no longer appear under .github, plugins or docs (grep over .mjs, .yml, .md and .json).
  • This repository's pr-pipeline.yaml is untouched, so it sets no limits block.

Checks I ran.

  • In pr-unblock-dispatch.yml, dispatch.mjs runs from .base. import.meta.url therefore resolves resolve-config under .base, which is where the new npm ci --prefix .base/... step installs its dependencies.
  • The install step has the same if condition as the dispatch step.
  • A bad base config now fails dispatch.mjs before any dispatch, including the dependency-review dispatches. The diff text and the PR body both say so.
  • anyHandles and limitResetsAt default limits to LIMIT_DEFAULTS when a caller omits it, so callers that pass no limits keep today's behavior.
  • If reset-clamp-min is set above reset-clamp-max, clampReset returns the max. The PR body lists this as deferred.

Not run. I did not run the test suites. The 924-pass result is the author's claim, and I did not verify it.

Residual risk, below the blocking bar. The six defaults now live in three places: the schema, resolve.mjs limitsOf, and limits.mjs LIMIT_DEFAULTS. The diff has no test that checks LIMIT_DEFAULTS equals the resolved defaults. A later edit to one copy would let the lane scripts and the resolver disagree without any test failing. The resolver-side test (resolve.test.mjs) asserts the schema-side values, and dispatch.test.mjs uses LIMIT_DEFAULTS as its expected values. A one-line equality test in resolve-config would close that gap.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 18, 2026
Merged via the queue into main with commit 3064b62 Oct 18, 2026
78 of 79 checks passed
@kyle-sexton
kyle-sexton deleted the fix/7764-pr-pipeline-limits branch October 18, 2026 03:26
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.

feat(pr-pipeline): read lane limit settings from a limits leaf in pr-pipeline.yaml

1 participant