fix(actions): give both stale-run cleanup jobs an identity they can match - #1993
fix(actions): give both stale-run cleanup jobs an identity they can match#1993seonghobae wants to merge 6 commits into
Conversation
…atch `strix.yml` and `opencode-review.yml` each run a cleanup job whose only purpose is retiring superseded runs of their own workflow, and both selected those runs with `select(.name == "<declared workflow name>")`. Both workflows declare `run-name:`, and GitHub reports the *rendered* run-name in a run's `name` field -- so neither equality ever matched and both jobs were silent no-ops. Measured 2026-09-07 against the live API: of the 100 most recent runs of each workflow, 0 carried the bare declared name and 100 carried the rendered `"<declared name> <repo>#<pr>@<sha>"` form. The workflow-level `concurrency` group hides this on `synchronize`, where it cancels the previous head's run itself. What was actually lost is every case no successor run supersedes: a `closed` or `converted_to_draft` pull request left its in-flight scans running to completion, holding admission slots under the shared 60-job ceiling for a pull request nobody is waiting on. Identity now comes from `.path`, the signal `noema-review.yml` already adopted for this same defect. The same measurement confirmed it stable in both contexts: `.github/workflows/strix.yml` on native runs in `.github` and on the nine ruleset-injected runs in `bandscope`, which carries no local copy. Deleting the filter instead would have been wrong -- the surviving `$metadata_matches` branch accepts any run whose `pull_requests[]` names the pull request, so an unfiltered job would cancel other workflows' runs. A negative control per selector pins that. The same equality appears twice in the scheduler and is repaired with one shared predicate, `run_name_identifies_workflow`: - `dispatch_strix_evidence`'s `busy_refs` was always empty, so the `repository_busy` guard that serializes Strix dispatch per target repository never fired. Since 2026-09-04 that let 20 dispatch runs reach 5 simultaneous for one target repository, against the 1 the guard specifies. - `stale_pr_run_ids`'s `workflow=` filter had the same defect. Its only production caller passes no workflow, so it never ran; it stayed a trap that silently returned nothing for any caller that did supply one. Two fixtures asserted the dead behaviour and are corrected rather than deleted. `test_dispatch_strix_waits_for_active_target_repository_run` paired a bare `name` with a rendered `display_title` inside one run record -- a combination GitHub cannot produce -- which is how a passing test coexisted with a predicate matching 0 of 100 live runs. `run_name_identifies_workflow` accepts a candidate followed by a space, so a future workflow named as an extension of an existing one would start answering for it. Nothing else in the repository would notice, so the premise that makes the prefix accept safe -- no central workflow name prefixes another, 0 of 35 -- is pinned as its own test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
added
bug
ChatGPT Codex Connector
`run_name_identifies_workflow` accepts a candidate followed by a space, which is safe only while no central workflow name prefixes another. The test pinning that premise globbed `*.yml` alone, so a future `.yaml` workflow would drop out of the check silently -- and the `>= 30` vacuity guard would not catch it either, since 35 files minus one still clears the floor. A test whose only purpose is announcing a change nobody would otherwise see must not be bypassable by a file extension. Reported by a peer session's read review of #1993. Both extensions are globbed now, and a second assertion requires every workflow file to contribute a name, so a file dropping out by losing its top-level `name:` fails as loudly as one dropping out by extension. Neither change alters today's result: 35 `.yml`, 0 `.yaml`, 35 distinct names. This closes a future hole rather than fixing a present miss, and is stated that way because nothing in the tree can demonstrate it yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review for 18a04550dd0325b10ecfafa41b06d3f9c16ff3fa.
Blocking scope finding — split the Strix repository-wide serialization policy from the stale-run identity repair.
The .path repairs in strix.yml and opencode-review.yml, plus the rendered-name predicate used by stale_pr_run_ids, are direct repairs for selectors that matched 0/100 live runs. Their production selectors have positive and foreign-workflow negative controls, and current-head Agent Review Runtime Quality run 34066418775 verifies the exact checkout with 2,984 passed / 1 skipped / 21 subtests and 100% measured coverage.
However, changing dispatch_strix_evidence().busy_refs from exact equality to run_name_identifies_workflow is a separate throughput-policy activation. It is not required to retire predecessor-head runs. It changes distinct current PRs in one repository from observed concurrency up to five to repository-wide serialization. The PR's own live measurement says an active Strix lifetime is median 177 minutes, p90 637 minutes, max 742 minutes; therefore the second unrelated PR can lose current-head evidence for hours. That is a buyer-visible admission/fairness decision, not a fixture correction.
Please keep the three stale/superseded-run identity repairs here and move the busy_refs activation into its own owner lane with RED→GREEN evidence for: (1) bounded wait/SLO for a second current PR, (2) starvation-free retry/fairness across PRs, (3) provider/rate-limit benefit versus the measured latency cost, and (4) exact telemetry for repository_busy duration and target run identity. If the policy remains here, equivalent acceptance evidence is required before merge.
This branch also overlaps scripts/ci/pr_review_merge_scheduler_core.py with #1986 and #1999. Preserve every valid delta by ordinary merge order plus non-force restack onto the first integrated sibling; do not treat a sibling's checks or review as evidence for this head.
Current required security runs are queued, not passing, so this review does not confer merge authority.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Revalidation on current exact head 5c3e6b272838dca0a1496123976d5f6fcc26349d.
The only delta from reviewed head 18a04550… is in tests/test_stale_run_cleanup_workflow_identity.py: it adds *.yaml discovery and non-vacuity/name-count assertions. That strengthens the workflow-name collision premise, but it does not alter the production dispatch_strix_evidence().busy_refs activation.
Therefore the blocking scope finding in review 5127083392 remains current: split repository-wide cross-PR Strix serialization into its own policy lane (with bounded-wait/fairness/rate-limit/telemetry evidence), while preserving the three stale-run identity repairs here. The new head's required workflow runs are queued; the cancelled predecessor-head runs are obsolete evidence and are not promoted.
seonghobae
removed
the
status: needs-review
seonghobae
added
the
status: draft
ChatGPT Codex Connector
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head verification for da7636a7a5c01a24de4008195409ed00387bef9c.
The scope finding from reviews 5127083392 / 5127085606 is resolved on this head:
- the child delta removes the repository-wide
busy_refs/repository_busypolicy path and its synthetic policy test; dispatch_strix_evidencenow performs only same-PR current/stale run handling before exact-head dispatch;- the direct
.pathrepairs for Strix/OpenCode cleanup and the rendered-name repair forstale_pr_run_ids(workflow=...)remain; - PR lifecycle is correctly Draft while current-head evidence is incomplete.
Fresh hosted run 34068657349 checked out this exact SHA and completed Agent Review Runtime Quality successfully: Strix quick gate PASS; 2,983 passed, 1 skipped, 21 subtests; 100% measured coverage; interrogate PASS. I found no new substantive defect in the scope-correction delta.
This does not grant merge authority: CodeQL, Security Scan, Python Security, and SAST are still queued, and there is no qualifying independent current-head approval. Preserve #1986/#1999 overlap through ordinary merge order and non-force restack after the first sibling integrates.
…t inverted
Splitting the `repository_busy` policy out of this branch also rewrapped the
sentence explaining why a prefix accept is safe, and four words fell out at the
line boundary. It read:
... so this predicate narrows a candidate identity alone.
instead of:
... so this predicate narrows a candidate set rather than deciding
identity alone.
The shortened form asserts exactly what the sentence exists to deny, and it is
the sentence a future reader relies on to decide whether the prefix accept can
stand. No test reads prose, so the full suite stayed green over it; the split's
own addition -- pull-request metadata as a third identity pin -- is correct and
is kept.
`cancelled_refs` also became assigned-but-never-read once `busy_refs` was
removed, since that comprehension was its only consumer. Renamed to
`_cancelled_refs`, matching the sibling call thirty lines above which already
discards the same value as `_cancelled_run_refs`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head revalidation for c35acda41249b2492fa31d51d4398b15ecdb6617: the single commit since da7636a7a5c01a24de4008195409ed00387bef9c is non-behavioral.
- It restores the intended sentence that
run_name_identifies_workflow()only narrows a candidate set; the call sites still require an independent identity pin. - It renames the now-unused
cancelled_refsbinding after the repository-widebusy_refs/repository_busypolicy was removed. - It does not reintroduce cross-PR Strix serialization, and the prior merge-admission finding remains resolved.
This is not an approval or a transfer of predecessor-head CI evidence. The PR remains Draft and the exact-head CodeQL, Security Scan, Python Security, SAST, and Runtime Quality runs must reach terminal success on this unchanged SHA before ordinary merge admission.
Pull request was converted to draft
1 participant
Causal defect
strix.ymlandopencode-review.ymlselected cleanup candidates with an exact comparison against the declared workflow name. Both workflows declarerun-name:, and the live API reports the rendered run name in.name; the former equality matched 0 of 100 sampled runs for each workflow on 2026-09-07. Closed or converted-to-draft pull requests could therefore leave obsolete scans consuming shared admission capacity.Bounded repair
.pathplus the expected rendered-name prefix;stale_pr_run_ids(workflow=...)and existing current/superseded review-run discovery;synchronizeconcurrency and late live-head revalidation;*.ymland*.yaml.Scope correction
The earlier head also activated
dispatch_strix_evidence().busy_refsfor rendered names. That was not required for stale-run retirement: it would serialize unrelated current PRs in one repository while the measured Strix active lifetime was median 177 minutes, p90 637 minutes, and max 742 minutes. Exact headda7636a7a5c01a24de4008195409ed00387bef9cremoves that dormant repository-wide guard and its synthetic policy test. Any future repository-wide admission policy needs its own bounded-wait, fairness, rate-limit-benefit, and telemetry evidence.Exact authority
main@c9052e607e5f3cc76e73207e7786b21500721b79da7636a7a5c01a24de4008195409ed00387bef9cThe predecessor head reported
2984 passed, 1 skipped, 21 subtests, 100% coverage, and 100% interrogate, but that evidence does not transfer. Fresh CI, security, current-head review, and destructive-selector regressions must be terminal-clean on the unchanged exact head before Ready or ordinary merge.