diff --git a/.github/workflows/pr-review-merge-scheduler.yml b/.github/workflows/pr-review-merge-scheduler.yml index 697038d1c..a9bb54f8a 100644 --- a/.github/workflows/pr-review-merge-scheduler.yml +++ b/.github/workflows/pr-review-merge-scheduler.yml @@ -617,11 +617,17 @@ jobs: # order every tick (the org repos API response order), so the same early # repositories always exhaust the shared budget and every later repository # starves indefinitely even with zero-open-thread, all-green PRs - # (ContextualWisdomLab/.github#1219). `github.run_number` increments on - # every run of this workflow, so rotating the walk order by it spreads the - # same fixed total budget across repositories over successive ticks instead - # of raising it. - ORG_SWEEP_ROTATION_INDEX: ${{ github.run_number }} + # (ContextualWisdomLab/.github#1219). Left unset here so the sweep step + # below derives it from a persistent per-execution counter (or, as a + # fallback, wall-clock time) instead of `github.run_number`: run_number + # increments on every trigger of this workflow (push, + # pull_request_target, pull_request_review, workflow_run), not only the + # sweep schedule, so it cannot give the "bounded by repository_count + # ticks" guarantee a rotation is meant to provide. Wall-clock time alone + # is also insufficient, since this single-flight/non-cancelling job can + # run up to 60 minutes and a delayed real execution can let more than + # one 900s window elapse, occasionally repeating a modulo offset + # (ContextualWisdomLab/.github#1223 review finding). # A repository the sweep credential structurally cannot read (the OpenCode # app is not installed there / the PR_REVIEW_MERGE_TOKEN lacks it) returns # HTTP 403 "Resource not accessible by integration". That is an access-grant @@ -826,8 +832,95 @@ jobs: echo "::error::ORG_SWEEP_BRANCH_UPDATE_LIMIT must be -1 or a non-negative integer; got '${ORG_SWEEP_BRANCH_UPDATE_LIMIT}'. Fix the ORG_SWEEP_BRANCH_UPDATE_LIMIT repository variable." exit 1 fi + # Unset in production (see the env-block comment above). Primary + # source: a persistent `ORG_SWEEP_ROTATION_COUNTER` repository + # variable on this (.github) repository, incremented by exactly + # one at the start of every actual org-queue-sweep execution. A + # wall-clock tick (one per 900s) is *not* sufficient on its own: + # this job is single-flight/non-cancelling with up to a 60-minute + # timeout, so a delayed or backlogged execution can let more than + # one 900s window elapse between two real sweep runs, and if that + # gap happens to be an exact multiple of the repository count the + # modulo offset repeats -- reintroducing the exact starvation + # #1220 fixed (CodeRabbit review finding on #1223). A persistent + # per-execution counter advances by exactly one every time the + # sweep body actually runs, regardless of how much wall-clock time + # a slow prior run consumed. Falls back to the wall-clock tick, + # which still strictly improves on the pre-#1220 fixed order, only + # if the counter read/write itself is unavailable (permissions, + # transient API failure) -- a fairness mechanism must never fail + # the sweep's much more important review-dispatch/merge work. + # Tests inject ORG_SWEEP_ROTATION_INDEX directly for determinism, + # which this only fills in when absent. + # + # Two known, accepted limitations of this counter (Devin review on + # #1223), neither of which is fixed here: + # - Read-modify-write is not atomic. A schedule-triggered run and a + # manual `repository_dispatch` org_sweep run use different + # concurrency groups and can therefore execute concurrently, in + # which case both could read the same counter value and pick the + # same rotation offset for that one pair of runs. The REST + # Variables API has no compare-and-swap primitive to close this + # without a broader concurrency-group redesign shared across + # every trigger type this workflow serves; the consequence is + # bounded and self-correcting (one occasionally-repeated offset, + # not a stuck one), so it is accepted rather than redesigned. + # - Whether the PATCH/POST below ever succeeds in production + # depends on the resolved token actually holding repository + # Variables-write scope, which is not independently verifiable + # from inside this workflow. If it does not, every run silently + # but safely degrades to the wall-clock fallback below (logged + # via ::warning:: each time), which is still strictly better + # than the pre-#1220 fixed order -- never a hard failure, and + # observable in the run log for whoever holds that token. + if [ -z "${ORG_SWEEP_ROTATION_INDEX:-}" ]; then + counter_variable_name="ORG_SWEEP_ROTATION_COUNTER" + # Distinguish a *successful* read (the variable exists; its + # value, valid or not, is authoritative) from a *failed* read + # (transient error, permissions, or the variable genuinely + # doesn't exist yet -- indistinguishable from here). Only a + # successful read may PATCH: a transient failure that silently + # became "treat as 0" would let the PATCH below clobber an + # already-accumulated counter value back down to 1, restarting + # the rotation sequence instead of degrading to the wall-clock + # fallback the design intends (Devin review finding on #1223). + if counter_current="$( + gh api "repos/${GITHUB_REPOSITORY}/actions/variables/${counter_variable_name}" \ + --jq '.value' 2>/dev/null + )"; then + if ! [[ "$counter_current" =~ ^[0-9]+$ ]]; then + counter_current=0 + fi + # Force base-10: a manually-seeded value with a leading zero + # (e.g. "08") passes the digit-only check above but bash's + # unprefixed arithmetic parses a leading-zero literal as + # octal, and "08"/"09" are not valid octal digits -- errors + # under set -e. $((10#...)) is the same guard already used + # elsewhere in this file (STALE_OPENCODE_MINUTES). + counter_next=$(( 10#$counter_current + 1 )) + if gh api "repos/${GITHUB_REPOSITORY}/actions/variables/${counter_variable_name}" \ + -X PATCH -f "value=${counter_next}" >/dev/null 2>&1; then + ORG_SWEEP_ROTATION_INDEX="$counter_next" + else + echo "::warning::read ${counter_variable_name}=${counter_current} but could not PATCH it; falling back to a wall-clock rotation tick for this run only" + ORG_SWEEP_ROTATION_INDEX=$(( $(date -u +%s) / 900 )) + fi + elif gh api "repos/${GITHUB_REPOSITORY}/actions/variables" \ + -X POST -f "name=${counter_variable_name}" -f "value=1" >/dev/null 2>&1; then + # The read failed, so this is only safe as a first-run + # create: POST fails on its own if the variable actually + # already exists (a real read outage rather than a genuinely + # missing variable), which correctly falls through to the + # wall-clock branch below instead of resetting a value this + # run could not see. + ORG_SWEEP_ROTATION_INDEX=1 + else + echo "::warning::could not read/write ${counter_variable_name}; falling back to a wall-clock rotation tick for this run only" + ORG_SWEEP_ROTATION_INDEX=$(( $(date -u +%s) / 900 )) + fi + fi if ! [[ "$ORG_SWEEP_ROTATION_INDEX" =~ ^[0-9]+$ ]]; then - echo "::error::ORG_SWEEP_ROTATION_INDEX must be a non-negative integer; got '${ORG_SWEEP_ROTATION_INDEX}'. This is derived from github.run_number and should never be malformed." + echo "::error::ORG_SWEEP_ROTATION_INDEX must be a non-negative integer; got '${ORG_SWEEP_ROTATION_INDEX}'." exit 1 fi @@ -845,10 +938,12 @@ jobs: ' <<<"$repositories_json" ) sweep_target_count=${#sweep_targets[@]} - # Rotate the fixed walk order by the run number so the same - # organization-wide review-dispatch/branch-update budget lands on a - # different starting repository each tick instead of always exhausting - # on the same early repositories (#1219). Total dispatches per tick are + # Rotate the fixed walk order by ORG_SWEEP_ROTATION_INDEX (see + # above: a persistent per-execution counter, falling back to a + # wall-clock tick) so the same organization-wide review-dispatch + # /branch-update budget lands on a different starting repository + # each execution instead of always exhausting on the same early + # repositories (#1219). Total dispatches per execution are # unchanged; only which repositories receive them rotates over time. rotation_offset=0 if [ "$sweep_target_count" -gt 0 ]; then @@ -860,7 +955,7 @@ jobs: ) fi fi - echo "Sweeping ${sweep_target_count} repositories starting at rotation offset ${rotation_offset} (run number ${ORG_SWEEP_ROTATION_INDEX})." + echo "Sweeping ${sweep_target_count} repositories starting at rotation offset ${rotation_offset} (rotation tick ${ORG_SWEEP_ROTATION_INDEX})." failures=0 unavailable=0 diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bc40394c..d6bfe8c8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Semantic Versioning where the repository publishes a release. ### Fixed +- Derived `org-queue-sweep`'s rotation index (added in `ContextualWisdomLab/.github#1220` to stop the walk-order starvation from `ContextualWisdomLab/.github#1219`) from a persistent `ORG_SWEEP_ROTATION_COUNTER` repository variable incremented by exactly one at the start of every actual sweep execution, instead of `github.run_number` (which increments on every trigger of this workflow, not only the sweep schedule — Devin review finding on `#1220`) or a wall-clock tick alone (which can repeat an offset when this single-flight, up-to-60-minute job runs behind schedule by an exact multiple of the repository count — CodeRabbit review finding on `#1223`). Falls back to the wall-clock tick only if the persistent counter itself is unavailable, so a fairness mechanism never blocks the sweep's review-dispatch/merge work. - Retried the Strix scan up to `STRIX_TRANSIENT_RETRY_PER_MODEL` times, same model, when the log shows the upstream strix-agent Caido sandbox bootstrap timing race (`loginAsGuest failed after N attempts` / `Failed to connect to 127.0.0.1 port `; tracked upstream as usestrix/strix#1036, #1037, #1056). A slow CI runner can exceed strix-agent's fixed 10-attempt sandbox-login budget before its local intercepting proxy is reachable, even though the penetration test itself never started and no vulnerability evidence was produced or lost; the Docker image is already cached from the failed attempt, so a same-model retry is cheap and typically clears the one-off boot race. Not wired into cross-model fallback, since switching LLM models cannot change local sandbox container boot timing. - Replaced nonexistent `job.workflow_repository` / `job.workflow_sha` / `job.workflow_ref` / `job.workflow_file_path` context references (actionlint: "property ... is not defined in object type") in `pr-review-fix-scheduler.yml`'s called-workflow source verification and `exact-artifact-sbom-attestation.yml`'s trusted-verifier checkout. Both always failed closed on the missing properties (ContextualWisdomLab/.github#1212) or, for the SBOM attestation checkout, silently resolved an empty repository/ref instead of the pinned trusted source (downstream `gh attestation verify --signer-repo`/`--signer-workflow`, using the separately hardcoded `SIGNER_REPOSITORY` constant rather than any workflow_ref, still failed closed on the resulting empty signer identity). `github.workflow_ref`/`github.workflow_sha` are real, documented properties, but for a `workflow_call` target they reflect the top-level *calling* workflow, not the reusable workflow's own file — a prefix match against the reusable workflow's own path can never succeed. `exact-artifact-sbom-attestation.yml`'s checkout now uses `github.workflow_sha` (correct today: it has no callers yet); `pr-review-fix-scheduler.yml`'s identity check instead validates `github.repository`, since every current caller uses a local, same-repo `uses: ./...` where caller and callee share one commit and `github.workflow_sha` is still the right pin. Tracked follow-up for the SBOM attestation checkout once a real (potentially cross-repo) caller exists: ContextualWisdomLab/.github#1228. - Used the receiving repository's workflow token for same-repository scheduler diff --git a/docs/doctoring/org-queue-sweep-rotation.md b/docs/doctoring/org-queue-sweep-rotation.md index e6240879e..8146de9fb 100644 --- a/docs/doctoring/org-queue-sweep-rotation.md +++ b/docs/doctoring/org-queue-sweep-rotation.md @@ -19,12 +19,46 @@ RankWeave's own turn. ## Decision -Rotate the sweep's repository walk order by `github.run_number` (a value -GitHub increments on every run of this workflow) before applying the -unchanged organization-wide budget. `rotation_offset = run_number % +Rotate the sweep's repository walk order by a rotation index before applying +the unchanged organization-wide budget. `rotation_offset = rotation_index % repository_count`; the walk starts at that offset and wraps. This spreads the exact same total per-tick dispatch budget across repositories over successive -ticks instead of raising it. +sweep executions instead of raising it. + +`ORG_SWEEP_ROTATION_INDEX`'s primary source is a persistent +`ORG_SWEEP_ROTATION_COUNTER` repository variable on `ContextualWisdomLab/.github` +itself, incremented by exactly one at the start of every actual +`org-queue-sweep` execution (`gh api .../actions/variables/ORG_SWEEP_ROTATION_COUNTER +-X PATCH`, falling back to `-X POST` to create it on the first run). It falls +back to a wall-clock tick (`$(date -u +%s) / 900`) only if the counter +read/write itself is unavailable (permissions, transient API failure) — a +fairness mechanism must never fail the sweep's much more important +review-dispatch/merge work. `ORG_SWEEP_ROTATION_INDEX` is left unset in the +job's `env:` block in production so the sweep step computes it; tests inject +it directly, or stub `gh` on `PATH`, for determinism. + +This design went through two prior, each independently review-flagged +iterations, both instructive about why neither alone is sufficient: + +1. **`github.run_number`** (original `#1220`). Rejected because `run_number` + increments on every trigger of this workflow — push, `pull_request_target`, + `pull_request_review`, `workflow_run` — not only the `*/15` sweep schedule, + so it cannot give the "bounded by `repository_count` executions" guarantee + a rotation is meant to provide (Devin review finding on `#1220`; that + version merged before the correction landed, since the review comment was + informational rather than a blocking request-changes). +2. **Wall-clock tick alone** (`#1223`, first revision). Rejected as the sole + source because `org-queue-sweep` is single-flight/non-cancelling with up to + a 60-minute `timeout-minutes`: a delayed or backlogged real execution can + let more than one 900-second window elapse before the next real run, and if + that elapsed-tick gap happens to be an exact multiple of `repository_count` + the modulo offset repeats — reintroducing the exact starvation `#1220` + fixed for a different reason (CodeRabbit review finding on `#1223`). + +A persistent per-execution counter is immune to both: it is untouched by +non-sweep triggers of this workflow (unlike `run_number`) and advances by +exactly one every time the sweep body actually runs, regardless of how much +wall-clock time a slow prior run consumed (unlike a wall-clock tick alone). The budget-sizing question in #1219 (is `1` a deliberate LLM-provider cost/rate ceiling, or an unconsidered default?) is explicitly **not** @@ -40,16 +74,21 @@ ceiling turns out to be conservative. - Every repository with ready work eventually reaches the front of the walk order and receives the shared dispatch, bounded by `repository_count` - ticks in the worst case, instead of never. + actual sweep executions in the worst case, instead of never. - Total review dispatches per tick, and therefore LLM-provider call volume per tick, are unchanged. - `rotation_offset` is logged (`Sweeping N repositories starting at rotation - offset O (run number R).`) so a specific tick's walk order is reconstructable - from the run log alone. + offset O (rotation tick T).`) so a specific execution's walk order is + reconstructable from the run log alone. - `ORG_SWEEP_ROTATION_INDEX` follows the same fail-closed numeric-validation pattern as the sibling `ORG_SWEEP_*_LIMIT` variables (reject non-digit input before it reaches arithmetic context, where an unguarded `set -e` - would not trap the error). + would not trap the error), applied after the persistent-counter/wall-clock + default fills it in when the environment does not already provide one. +- A degraded run (counter unavailable) still rotates by wall-clock time + rather than reverting to the original fixed order; it only loses the + strict per-execution guarantee for that one run, logged as a + `::warning::`. ## Verification @@ -59,9 +98,21 @@ ceiling turns out to be conservative. full permutation of the input, not a subset. - `test_org_queue_sweep_rotation_offset_is_safe_with_no_targets` covers the zero-repository edge case. +- `test_org_queue_sweep_rotation_index_uses_persistent_counter_when_available` + stubs `gh` on `PATH` to simulate a successful read-increment-write and + confirms the counter advances by exactly one. +- `test_org_queue_sweep_rotation_index_creates_counter_on_first_run` confirms + the POST-create fallback when the PATCH target does not exist yet. +- `test_org_queue_sweep_rotation_index_falls_back_to_wall_clock` confirms the + wall-clock degraded path and its `::warning::` when the counter is entirely + unavailable. +- `test_org_queue_sweep_rotation_index_override_is_preserved` and + `test_org_queue_sweep_rotation_index_rejects_malformed_override` cover the + test-injection and fail-closed-validation paths. - `test_org_queue_sweep_documents_rotation_leverage_and_validates_input` - locks the `#1219` cross-reference and confirms the shared budget constant - itself is untouched. + locks the `#1219` cross-reference, confirms `github.run_number` is not + reintroduced as the source, and confirms the shared budget constant itself + is untouched. - `actionlint` (with `shellcheck` on `PATH`) reports no findings against the modified workflow. @@ -69,3 +120,8 @@ ceiling turns out to be conservative. `ContextualWisdomLab/.github#1219` — original starvation report with sweep run evidence. +`ContextualWisdomLab/.github#1220` — original rotation fix; `run_number` vs. +per-execution-guarantee review discussion. +`ContextualWisdomLab/.github#1223` — wall-clock correction, then the +persistent-counter correction this document and the current workflow source +reflect. diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b440bc5b9..d7d0ec8ac 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -7,6 +7,7 @@ import subprocess import sys import textwrap +import time from pathlib import Path import pytest @@ -790,7 +791,7 @@ def _extract_org_sweep_rotation_snippet(workflow: str) -> str: `gh api`/dispatch logic that would require live network credentials.""" start_marker = " sweep_target_count=${#sweep_targets[@]}\n" - end_marker = 'run number ${ORG_SWEEP_ROTATION_INDEX})."\n' + end_marker = 'rotation tick ${ORG_SWEEP_ROTATION_INDEX})."\n' start = workflow.index(start_marker) end = workflow.index(end_marker, start) + len(end_marker) return textwrap.dedent(workflow[start:end]) @@ -846,20 +847,257 @@ def test_org_queue_sweep_rotation_offset_is_safe_with_no_targets() -> None: assert "starting at rotation offset 0" in result.stdout +def _extract_org_sweep_rotation_default_snippet(workflow: str) -> str: + """Return only the wall-clock-default/validation block for the rotation index, + without the surrounding `gh api` calls that would require network credentials.""" + + start_marker = " if [ -z \"${ORG_SWEEP_ROTATION_INDEX:-}\" ]; then\n" + end_marker = " exit 1\n fi\n\n repositories_json=" + start = workflow.index(start_marker) + end = workflow.index(end_marker, start) + len(" exit 1\n fi\n") + return textwrap.dedent(workflow[start:end]) + + +def _fake_gh_script(*, get_ok: bool, get_value: str, patch_ok: bool, post_ok: bool) -> str: + """A stand-in `gh` executable simulating the repository-variable API. + + ``get_ok`` controls whether `gh api .../variables/NAME --jq .value` + exits zero at all -- a real "does the variable exist and is it + readable" outcome, kept distinct from what value it prints on success + (``get_value``), so tests can simulate a *failed* read (transient error + or a genuinely missing variable) separately from a *successful* read + of an empty/malformed value. ``patch_ok``/``post_ok`` control whether + the corresponding mutation exits zero, so tests can force the + PATCH-then-POST-create fallback or the full-failure wall-clock + fallback without a real GitHub API call. + """ + get_exit = "0" if get_ok else "1" + patch_exit = "0" if patch_ok else "1" + post_exit = "0" if post_ok else "1" + return textwrap.dedent( + f"""\ + #!/usr/bin/env bash + set -euo pipefail + if [ "$1" != "api" ]; then + echo "unsupported fake gh invocation: $*" >&2 + exit 2 + fi + shift + if [[ "$1" == *"/variables/"* ]] && [[ "$*" == *"-X PATCH"* || "$*" == *"PATCH"* ]]; then + exit {patch_exit} + fi + if [[ "$1" == "repos/"*"/actions/variables" ]]; then + exit {post_exit} + fi + if [[ "$1" == *"/variables/"* ]]; then + if [ "{get_exit}" = "0" ]; then + printf '%s' "{get_value}" + fi + exit {get_exit} + fi + echo "unsupported fake gh api path: $1" >&2 + exit 2 + """ + ) + + +def _run_rotation_default_snippet( + snippet: str, + tmp_path: Path, + *, + get_ok: bool = True, + get_value: str, + patch_ok: bool, + post_ok: bool, +) -> subprocess.CompletedProcess[str]: + """Execute the extracted default/validation block with a fake `gh` on PATH.""" + + fake_gh = tmp_path / "gh" + fake_gh.write_text( + _fake_gh_script(get_ok=get_ok, get_value=get_value, patch_ok=patch_ok, post_ok=post_ok), + encoding="utf-8", + ) + fake_gh.chmod(0o755) + script = snippet + '\nprintf "%s\\n" "$ORG_SWEEP_ROTATION_INDEX"\n' + env = dict(os.environ) + env.pop("ORG_SWEEP_ROTATION_INDEX", None) + env["GITHUB_REPOSITORY"] = "ContextualWisdomLab/.github" + env["PATH"] = f"{tmp_path}{os.pathsep}{env.get('PATH', '')}" + return subprocess.run( + ["bash", "-euo", "pipefail", "-c", script], env=env, capture_output=True, text=True + ) + + +def test_org_queue_sweep_rotation_index_uses_persistent_counter_when_available( + tmp_path: Path, +) -> None: + """The primary source increments a persistent counter by exactly one per + actual sweep execution — immune to how much wall-clock time a prior + slow (up to 60-minute, non-cancelling) run consumed, which a wall-clock + tick alone cannot guarantee (CodeRabbit review finding on #1223).""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_value="7", patch_ok=True, post_ok=True + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "8" # incremented by exactly one + + +def test_org_queue_sweep_rotation_index_counter_increment_forces_base_10( + tmp_path: Path, +) -> None: + """A manually-seeded leading-zero value ("08") must not be parsed as + octal, where it would error under set -e (Devin review finding on + #1223) — unprefixed bash arithmetic treats a leading zero as an octal + literal, and "08"/"09" are not valid octal digits.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_value="08", patch_ok=True, post_ok=True + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "9" + + +def test_org_queue_sweep_rotation_index_creates_counter_on_first_run(tmp_path: Path) -> None: + """A failed read (variable does not exist yet) falls back to creating it.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_ok=False, get_value="", patch_ok=False, post_ok=True + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "1" + + +def test_org_queue_sweep_rotation_index_falls_back_to_wall_clock(tmp_path: Path) -> None: + """If the persistent counter is entirely unavailable (both the read and + the create-on-first-run POST fail), degrade to a wall-clock tick rather + than failing the whole sweep over a fairness mechanism.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_ok=False, get_value="", patch_ok=False, post_ok=False + ) + assert result.returncode == 0, result.stderr + stdout_lines = result.stdout.strip().splitlines() + computed_tick = int(stdout_lines[-1]) # last line: the printed value; earlier: the warning + expected_tick = int(time.time()) // 900 + assert abs(computed_tick - expected_tick) <= 1 # tolerate a tick boundary race + assert "could not read/write" in result.stdout # a `::warning::` workflow command + + +def test_org_queue_sweep_rotation_index_transient_read_failure_does_not_reset_counter( + tmp_path: Path, +) -> None: + """A *failed* read must never be treated as "the counter is 0 and safe to + PATCH": that would silently reset an already-accumulated counter value + back down to 1, restarting the rotation sequence instead of degrading to + the wall-clock fallback (Devin review finding on #1223). Simulated here + as: the read fails, and the create-on-first-run POST also fails (as it + should when the variable genuinely already exists and this run simply + could not see it) -- landing on the wall-clock fallback rather than a + PATCH that would have clobbered the real value.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_ok=False, get_value="", patch_ok=True, post_ok=False + ) + assert result.returncode == 0, result.stderr + stdout_lines = result.stdout.strip().splitlines() + computed_tick = int(stdout_lines[-1]) + expected_tick = int(time.time()) // 900 + assert abs(computed_tick - expected_tick) <= 1 + # Critically: never "1" -- that would mean the failed read was treated + # as a fresh-start reset rather than an unreadable existing value. + assert stdout_lines[-1] != "1" + + +def test_org_queue_sweep_rotation_index_successful_read_but_failed_patch_falls_back( + tmp_path: Path, +) -> None: + """A successful read of an existing value, followed by a failed PATCH, + must fall back to the wall-clock tick and log the value that could not + be written -- not silently drop the accumulated counter.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + + result = _run_rotation_default_snippet( + snippet, tmp_path, get_ok=True, get_value="41", patch_ok=False, post_ok=False + ) + assert result.returncode == 0, result.stderr + stdout_lines = result.stdout.strip().splitlines() + computed_tick = int(stdout_lines[-1]) + expected_tick = int(time.time()) // 900 + assert abs(computed_tick - expected_tick) <= 1 + assert "read ORG_SWEEP_ROTATION_COUNTER=41 but could not PATCH it" in result.stdout + + +def test_org_queue_sweep_rotation_index_override_is_preserved() -> None: + """An explicitly injected value (as tests do) is never overwritten.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + script = snippet + '\nprintf "%s\\n" "$ORG_SWEEP_ROTATION_INDEX"\n' + + result = subprocess.run( + ["bash", "-euo", "pipefail", "-c", script], + env={**os.environ, "ORG_SWEEP_ROTATION_INDEX": "42"}, + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "42" + + +def test_org_queue_sweep_rotation_index_rejects_malformed_override() -> None: + """A malformed override still fails closed rather than reaching arithmetic.""" + + workflow = workflow_text("pr-review-merge-scheduler.yml") + snippet = _extract_org_sweep_rotation_default_snippet(workflow) + script = snippet + '\nprintf "%s\\n" "$ORG_SWEEP_ROTATION_INDEX"\n' + + result = subprocess.run( + ["bash", "-euo", "pipefail", "-c", script], + env={**os.environ, "ORG_SWEEP_ROTATION_INDEX": "not-a-number"}, + capture_output=True, + text=True, + ) + assert result.returncode != 0 + assert "ORG_SWEEP_ROTATION_INDEX must be a non-negative integer" in result.stdout + + def test_org_queue_sweep_documents_rotation_leverage_and_validates_input() -> None: """Record why rotation exists and keep the new input on the same fail-closed contract.""" workflow = workflow_text("pr-review-merge-scheduler.yml") + assert "ContextualWisdomLab/.github#1219" in workflow assert ( - "ORG_SWEEP_ROTATION_INDEX: ${{ github.run_number }}" + 'ORG_SWEEP_ROTATION_INDEX=$(( $(date -u +%s) / 900 ))' ) in workflow - assert "ContextualWisdomLab/.github#1219" in workflow assert ( 'if ! [[ "$ORG_SWEEP_ROTATION_INDEX" =~ ^[0-9]+$ ]]; then' ) in workflow assert ( "rotation_offset=$(( ORG_SWEEP_ROTATION_INDEX % sweep_target_count ))" ) in workflow + # `github.run_number` increments on every trigger of this workflow, not + # only the sweep schedule, so it cannot give the per-sweep-tick rotation + # guarantee the fix is meant to provide (ContextualWisdomLab/.github#1220 + # review finding). The env-block default must not reintroduce it. + assert "ORG_SWEEP_ROTATION_INDEX: ${{ github.run_number }}" not in workflow # The fix must not change the org-wide budget itself, only which # repositories consume it — otherwise it reintroduces the exact # cost/rate-limit risk #1219 explicitly declined to guess at.