Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 54 additions & 7 deletions .github/workflows/pr-review-merge-scheduler.yml
Comment thread
seonghobae marked this conversation as resolved.
Resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -826,8 +832,49 @@ 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.
if [ -z "${ORG_SWEEP_ROTATION_INDEX:-}" ]; then
counter_variable_name="ORG_SWEEP_ROTATION_COUNTER"
counter_current="$(
gh api "repos/${GITHUB_REPOSITORY}/actions/variables/${counter_variable_name}" \
--jq '.value' 2>/dev/null || true
)"
if ! [[ "$counter_current" =~ ^[0-9]+$ ]]; then
counter_current=0
fi
Comment thread
seonghobae marked this conversation as resolved.
Resolved
counter_next=$(( counter_current + 1 ))
Comment thread
seonghobae marked this conversation as resolved.
Resolved
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"
elif gh api "repos/${GITHUB_REPOSITORY}/actions/variables" \
-X POST -f "name=${counter_variable_name}" -f "value=${counter_next}" >/dev/null 2>&1; then
ORG_SWEEP_ROTATION_INDEX="$counter_next"
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
Comment thread
seonghobae marked this conversation as resolved.
Resolved
fi
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Resolved
Comment thread
seonghobae marked this conversation as resolved.
Resolved
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

Expand Down Expand Up @@ -860,7 +907,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
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
- 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
Actions inventory and read calls, while retaining the established mutation
Expand Down
76 changes: 66 additions & 10 deletions docs/doctoring/org-queue-sweep-rotation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**
Expand All @@ -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

Expand All @@ -59,13 +98,30 @@ 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.

## References

`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.
Loading
Loading