diff --git a/.github/workflows/strix.yml b/.github/workflows/strix.yml index 58ed3dab8d..f15b29f564 100644 --- a/.github/workflows/strix.yml +++ b/.github/workflows/strix.yml @@ -44,12 +44,15 @@ on: # them, so the same doc/image-only decision is enforced by the # changed-scope job below. The run-name # includes the PR number and head SHA for status grouping, while the - # concurrency group is scoped per repository and event class to prevent - # shared-provider key rate-limit storms. Strix runs intentionally do not - # cancel in progress because a pre-job cancellation leaves no scanner log to - # review. GitHub keeps one active and one pending run per group; the merge - # scheduler re-dispatches exact-head evidence when a pending run is - # superseded. For PRs the merge scheduler manages, same-head Strix evidence + # concurrency group is scoped per repository AND pull request (native and + # dispatch PR runs share one group), or per protected branch for push + # events, to prevent shared-provider key rate-limit storms. That group + # runs with cancel-in-progress: true, so a newer head of the same PR or + # branch retires the older run: cancellation is a supersede signal, never + # passing evidence, and the merge scheduler still requires exact-head + # evidence before it will act. schedule and PR-less repository_dispatch + # keep a unique run id and so are never cancelled by a sibling. + # For PRs the merge scheduler manages, same-head Strix evidence # is still forced at merge time via repository_dispatch (which paths-ignore # does not affect), so merged code never loses evidence. paths-ignore: @@ -77,12 +80,36 @@ on: concurrency: # Workflow-level admission is required: job-level groups are never evaluated # while the whole run is queued behind the organization job ceiling. + # Push scans coalesce per protected branch: a newer head of the same branch + # supersedes the older scan exactly as a newer PR head does. A push scan + # covers the whole tree (STRIX_TARGET_PATH is './' outside PR scope) and + # publishes no 'strix' commit status, so the newest head's scan is a + # complete scan OF THE CURRENT TREE -- not a record of every earlier + # commit's findings: code that entered and left main between two heads, and + # findings a retired run never uploaded, are absent from it. With the run-id + # fallback every main push was its own group and nothing ever retired a + # superseded main scan: on 2026-09-05 nine push/main runs were outstanding + # at once in this repository against a 10-30 minute normal scan -- five + # holding runner slots under the shared 60-job ceiling (running for up to + # two hours) and four still queued, which occupy no slot until a runner is + # assigned. schedule and repository_dispatch without a PR number keep a + # unique run id. + # Coverage trade, measured the same day: main moved 50 times in 24 hours + # (median gap 8.4 min, mean 26.5 min, bursty), so with cancel-in-progress + # only the final head of each merge burst completes a scan -- one completed + # main scan per quiet window, not one per push. That is the intended + # exchange: a cancelled push scan gives up its own report, and the scan that + # replaces it re-covers the current tree rather than that commit's history; + # a per-commit evidence-retention guarantee would need its own preservation + # contract. The weekly schedule scan is the floor. group: >- strix-security-scan-${{ github.event.pull_request.base.repo.full_name || github.event.client_payload.target_repository || github.repository }}-${{ github.event.pull_request.number || - github.event.client_payload.pr_number || github.run_id }} + github.event.client_payload.pr_number || + (github.event_name == 'push' && format('push-{0}', github.ref_name)) || + github.run_id }} cancel-in-progress: true # Scorecard Token-Permissions (alert #43): keep the workflow-level token diff --git a/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md b/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md index e710eb5d1f..5ba354947a 100644 --- a/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md +++ b/docs/doctoring/startup-failure-and-strix-concurrency-20260904.md @@ -43,6 +43,35 @@ another. Workflow-level concurrency was deliberately not used because GitHub applies it before any live-head admission job can run and does not guarantee concurrency ordering. +**Amendment (2026-09-05).** "nor one another" no longer holds for `push` +events on the same branch. Measured at 14:27Z in `.github`: nine `push`/`main` +Strix runs were outstanding at once — five holding runner slots under the +shared 60-job ceiling (jobs started 12:31-14:25Z, one already past two hours) +and four more waiting in the queue behind them, which occupy no slot until a +runner is assigned — against a 10-30 minute normal scan. The run-id fallback +in the workflow-level group made every main push its own group, so no newer +main head ever retired an older scan. The workflow-level group now scopes +`push` events as `push-`: a newer head of the same protected branch +supersedes the older scan exactly as a newer PR head does. What a retired scan +gives up is its own report, not the gate's inputs: a push scan covers the whole +tree (`STRIX_TARGET_PATH` is `./` outside PR scope) and publishes no `strix` +commit status, so the newest head's scan is a complete scan *of the current +tree*. It is not a record of every earlier commit: code that entered and left +`main` between two heads, and findings a retired run never uploaded, are absent +from the newest report, and report collection preserves only runs that reach +it. A per-commit evidence-retention guarantee would need a separate, +verifiable preservation contract; this change does not provide one. `schedule` +and PR-less `repository_dispatch` runs still receive a unique run id. The +`pr_number=${GITHUB_RUN_ID}` admission output is unchanged. + +Tradeoff, stated so a later reader of the security dashboard is not +surprised: with `main` moving roughly every 30 minutes against a 10-30 minute +scan, "main is scanned after every merge" becomes "the latest `main` is +scanned once merging pauses for at least one scan duration". During a merge +burst each new head cancels the previous scan; the burst's final head is +scanned, and the weekly full-tree `schedule` scan (unique run id, never +cancelled) is the floor under a sustained burst. + ## Verification - `python -m pytest -q tests/test_pr_review_merge_scheduler.py -k 'startup_failures or startup_failure'` diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index b9b1c43de3..6ea00c099f 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -203,6 +203,7 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$workflow_file" "needs: [changed-scope, admit-current-head]" "strix provider queue waits for live-head admission" assert_file_contains "$workflow_file" 'strix-security-scan-${{' "strix workflow coalesces by repository and PR before job admission" assert_file_not_contains "$workflow_file" 'strix-security-scan-${{ needs.admit-current-head.outputs.target_repository }}-${{' "strix concurrency is not delayed until job admission" + assert_file_contains "$workflow_file" "format('push-{0}', github.ref_name)" "strix push scans coalesce per protected branch instead of one group per run id" assert_file_contains "$workflow_file" "cancel-superseded-pr-runs:" "strix workflow runs superseded-head cleanup outside the provider scan queue" assert_file_not_contains "$workflow_file" "format('closed-pr-{0}-{1}'" "strix cleanup does not need a second concurrency queue" assert_file_contains "$workflow_file" 'echo "pr_number=${GITHUB_RUN_ID}"' "strix workflow preserves independent push and schedule evidence" diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 512cb6bef5..9b8b66313b 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -33,6 +33,31 @@ def workflow_text(name: str) -> str: ) +def _strip_yaml_inline_comment(text: str) -> str: + """Drop a YAML inline comment from one scalar line. + + YAML opens a comment at ``#`` only when it starts the line or follows + whitespace, and never inside a quoted scalar, so a bare ``split("#")`` + would truncate a legitimate value that merely contains the character. + """ + index = 0 + quote = "" + while index < len(text): + char = text[index] + if quote: + if quote == '"' and char == "\\": + index += 2 + continue + if char == quote: + quote = "" + elif char in "\"'": + quote = char + elif char == "#" and (index == 0 or text[index - 1] in " \t"): + return text[:index] + index += 1 + return text + + def workflow_level_concurrency_group(workflow: str) -> str: """Return only the workflow-level ``concurrency.group`` value, comments removed. @@ -56,7 +81,7 @@ def workflow_level_concurrency_group(workflow: str) -> str: if not collecting: if re.match(r"^\s*group:", line): collecting = True - value.append(line.split("group:", 1)[1]) + value.append(_strip_yaml_inline_comment(line.split("group:", 1)[1])) continue if re.match(r"^\s*[A-Za-z][\w-]*:", line): break @@ -437,6 +462,66 @@ def test_concurrency_group_slice_ignores_the_comment_that_documents_it() -> None assert "opencode-review-dispatch-${{ github.repository }}" in group_value +def test_concurrency_group_slice_ignores_an_inline_comment_on_the_key() -> None: + """An inline comment beside a plain-scalar key must not satisfy the contract. + + The full-line negative control above does not cover this shape. YAML allows a + comment on the key's own line, so a change collapsing the group to the + repository alone could keep the documented expressions one space away and + leave every substring assertion green. + """ + inline = textwrap.dedent( + """\ + concurrency: + group: opencode-review-dispatch-${{ github.repository }} # ${{ github.event.client_payload.pr_number || github.run_id }} + cancel-in-progress: true + permissions: + contents: read + """ + ) + group_value = workflow_level_concurrency_group(inline) + + assert "opencode-review-dispatch-${{ github.repository }}" in group_value + assert "github.event.client_payload.pr_number" not in group_value + assert "github.run_id" not in group_value + + +def test_concurrency_group_slice_keeps_a_hash_that_is_not_a_comment() -> None: + """Stripping must follow YAML's rules rather than cutting at every ``#``. + + Two shapes would be corrupted by a naive ``split("#")``: a quoted scalar + containing the character, and a folded block body, where ``#`` is literal + content and never opens a comment. Only the key's own line is stripped. + """ + quoted = textwrap.dedent( + """\ + concurrency: + group: "release-#42-${{ github.repository }}" + cancel-in-progress: true + permissions: + contents: read + """ + ) + assert "release-#42-${{ github.repository }}" in workflow_level_concurrency_group( + quoted + ) + + folded = textwrap.dedent( + """\ + concurrency: + group: >- + release-${{ github.repository }}-#${{ + github.run_id }} + cancel-in-progress: true + permissions: + contents: read + """ + ) + folded_value = workflow_level_concurrency_group(folded) + assert "#${{" in folded_value + assert "github.run_id" in folded_value + + def test_concurrency_group_slice_reads_a_folded_multi_line_key() -> None: """The real key is a folded block, so the slice must join its continuation lines.""" folded = textwrap.dedent( @@ -701,6 +786,18 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: superseded runs before runner admission, including runs still blocked by the organization-wide job ceiling. Native and dispatched evidence share one group; non-PR events use a unique run id. + + 2026-09-05: push events are scoped per protected branch (``push-``) + instead of a unique run id. Measured that morning in this repository: + nine ``push``/``main`` Strix runs were outstanding at once (five running + for up to two hours, four queued) against a 10-30 minute normal scan, + because the run-id fallback made every main push its own group and + nothing ever retired a superseded main scan. A push scan covers the whole + tree (``STRIX_TARGET_PATH`` is ``./`` outside PR scope) and publishes no + ``strix`` commit status, so the newest head's scan is a complete scan of + the current tree (not a record of every earlier commit's findings). + ``schedule`` and ``repository_dispatch`` without a PR number keep a + unique run id. """ workflow = workflow_text("strix.yml") concurrency_contract = workflow.split("concurrency:", 1)[1].split( @@ -719,6 +816,14 @@ def test_strix_serializes_provider_evidence_per_repository_and_pr() -> None: assert "github.event.pull_request.number" in group_value assert "github.event.client_payload.pr_number" in group_value assert "github.run_id" in group_value + # Asserted against group_value, not the whole concurrency block: #1970 made + # these keys immune to comment leakage, and this file's own prose now + # discusses the push clause at length, so the comment text would otherwise + # satisfy the assertion whether or not the expression survived. + assert ( + "(github.event_name == 'push' && format('push-{0}', github.ref_name)) ||" + in group_value + ) assert "github.event.pull_request.head.sha" not in concurrency_contract assert "github.event.client_payload.pr_head_sha" not in concurrency_contract assert "cancel-in-progress: true" in concurrency_contract