Strix run-name is an unenforced parse contract: any suffix makes the scheduler preserve every stale run and stop dispatching

[seonghobae]

While triaging PR #1009 I found a coupling that would disable Strix dispatch for every open PR in the organization if that branch merged as written. Filing separately because the defect is a latent fragility in main, independent of whether #1009 ever lands.

The coupling

.github/workflows/strix.yml sets a run-name, and scripts/ci/pr_review_merge_scheduler_core.py parses that name back out to recover the reviewed head SHA:

# pr_review_merge_scheduler_core.py:3487-3493
prefixes = tuple(f"{title} {repo}#{number}@" for title in sorted(titles, key=len, reverse=True))
prefix = next((c for c in prefixes if display_title.startswith(c)), None)
if prefix is None:
    raise ValueError("repository_dispatch run has no trusted target identity")
return validate_git_sha(display_title.removeprefix(prefix)).lower()

The prefix ends at @, so everything after @ must be a bare 40-hex SHA and nothing else. That contract lives entirely in the string format — nothing enforces it, and nothing in strix.yml mentions it.

Why it matters

PR #1009 appends a merge-state suffix to that same run-name:

… #<n>@${{ …pr_head_sha… }}:${{ github.event.client_payload.merge_state || 'open' }}

Executed against origin/main's real function:

Strix Security Scan org/repo#7@aaaa…aaaa        → OK
Strix Security Scan org/repo#7@aaaa…aaaa:open   → ValueError: invalid git sha: 'aaaa…aaaa:open'

_review_run_still_superseded (core:3506-3517) catches ValueError, prints ::warning::Preserving review run … failed closed, and returns False — meaning not superseded. Failing closed is right in isolation; the consequence is not. A stale Strix run can then never be proven stale, so it is preserved, folded into the current run refs, and dispatch_strix_evidence answers already_running rather than dispatching. Every open PR in every target repository, from one string edit in a different file.

The class of defect

This is the shape already catalogued as silently-inactive required checks: a guard that looks fully configured while a narrower condition quietly never matches. The novelty is the trust boundary being a display string — the producer (strix.yml) and the consumer (the scheduler core) have no shared constant, no test spanning both, and no comment on either side naming the other.

Two cheap fixes, not exclusive:

1. Make the parse tolerant. validate_git_sha(display_title.removeprefix(prefix).split(":", 1)[0]) — a suffix stops being fatal, and the reviewed head is still recovered exactly.
2. Pin the contract from both ends. One test asserting strix.yml's run-name ends with the head-SHA expression and that _review_run_target_head accepts what that template produces. Today each side is tested only against its own idea of the format.

A third, worth considering separately: the except (…ValueError…) in _review_run_still_superseded cannot distinguish "the API told us something inconsistent" (preserve — correct) from "we cannot parse our own run-name" (a bug in us). The second deserves a distinct, louder signal than a ::warning:: that scrolls past.

Status

Not fixed here — the parse is in the scheduler core, which several sessions are actively changing, and the run-name is in a required workflow. Recorded so the fix is deliberate rather than a side effect of someone else's merge. No repository state was changed while finding this; PR #1009 was left byte-identical.

🤖 Generated with Claude Code

[seonghobae]

Independently verified on main at f2f91b80 (a second reading, not a relay):

• Producer: .github/workflows/strix.yml:2-5 — run-name: Strix Security Scan <target_repository>#<pr_number>@<…>.
• Consumer: scripts/ci/pr_review_merge_scheduler_core.py:3486-3493 — builds f"{title} {repo}#{number}@" prefixes, strips the matching one from display_title, and passes the remainder to validate_git_sha; anything after @ that is not a bare SHA raises ValueError.
• Catch: :3511-3516 — except (KeyError, RuntimeError, TypeError, ValueError) → ::warning::Preserving review run … live stale-run revalidation failed closed → return False.
• Coupling test: none found — the only run_name occurrences under tests/ are runpy.run_path(..., run_name="__main__").

So the contract lives only in the string format, and a producer-side change (as #1009's branch makes, appending :open) fails closed into "preserve", which is the state that blocks dispatch_strix_evidence with already_running. Fix shape I would back: one shared constant for the separator/format used by both sides, one round-trip test that renders the run-name template and parses it, and a split of that except so a ValueError from our own format is loud rather than preserved. Not touching the core while other sessions have it open.