Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
25 changes: 25 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,31 @@ this file. The format follows Keep a Changelog, and versioned releases follow
Semantic Versioning where the repository publishes a release.

## [Unreleased]
- **Fix stale test assertions and dead-code gaps left by `#1654`, `#1656`, and `#1658`.**
Reproduced all failures on a fresh unmodified `main` clone before attributing blame.
`#1654` (introducing `scripts/ci/current_head_run_coalescer.py` and hardening several
review-workflow polling loops with retry-with-backoff) left 7 stale assertions: one
genuinely dead-code check (`_run_matches_head_identity` already rejects any non-PR-event
candidate before a later, narrower "not a pull-request" check could ever run -- removed
the redundant check and updated the test to the correct, now-authoritative "head moved"
message), two synthetic-sentinel-vs-real-retry-loop mismatches (a fixture's unmocked-call
exit code no longer reaches the script's own exit status once a 3-attempt backoff loop
absorbs it), two literal-text contract drifts ("sleep 30" -> `poll_interval_seconds`; the
reviews endpoint gained `?per_page=100`), and two renamed/relocated message assertions (a
jq field rename `current_head`->`classified_head`; a diagnostic moved from the workflow
YAML into the `scripts/ci/revalidate_queue_cancellation.sh` helper it now delegates to).
While re-verifying `current_head_run_coalescer.py`'s own coverage in isolation, found and
closed two more, unrelated gaps in the same file: a second dead-code instance
(`select_duplicate_queued_run_ids` re-derived `workflow_id` behind a redundant guard
`_run_identity_matches` already guarantees) and six genuinely-reachable but untested
early-return guard clauses in `_run_pr_scope_is_safe` plus one in the sibling-authority
loop, closed with eight new targeted regression tests. `#1656` (removing ten no-op
`cancel-closed-pr-runs` runner jobs) and `#1658` (removing the 300s `LLM_TIMEOUT` cap, in
service of the org's now-unlimited-by-default LLM timeout policy) each left their own
runner-image-count and literal-value contract tests asserting pre-change reality; updated
four more test files to match. Full suite: 2600+ passed, 100% branch coverage, 100%
docstrings; no production behavior change except the two dead-code removals (both
provably unreachable, so behavior-neutral).
- **Pin the three central required review workflows (Strix, OpenCode Review, Noema Review) off the observed starved floating `ubuntu-latest` runner image.** Following the same repair already rolled out to security gates (`#1618`) and the merge scheduler (`#1609`), `strix.yml`, `opencode-review.yml`, and `noema-review.yml` now request the explicit `ubuntu-24.04` image on every job. These three workflows are the org's own required-workflow gate for every sibling repository, so a starved floating image here directly contributes to organization-wide required-check queuing. New `tests/test_required_review_runner_image_contract.py` asserts no job in any of the three files still requests the floating image. Also fixed 4 pre-existing, unrelated test failures on `main` left by `#1630`'s organization-sweep rotation cadence change (every 15 minutes to hourly, to reduce control-plane pressure under the same Actions saturation): `tests/test_required_workflow_queue_contract.py`'s rotation-index tests still asserted the old `/ 900` (15-minute) divisor against the new `/ 3600` (hourly) production value.
- **Refresh Noema reviewer App authority after long model work (`#1616`).** A real `naruon#1497` review outlived its repository-scoped GitHub App installation token and failed the next exact-head GitHub operation with HTTP 401. The trusted workflow now prepares the validated verdict into a private runner-local envelope, remints the same least-privilege repository-scoped App authority after model work, independently re-fetches exact live head/reviewer identity, and only then publishes. Skipped preparation creates no envelope, predecessor App tokens cannot authorize publication, PAT/OIDC remain explicit fail-closed sources, malformed handoffs are cleaned up, and executable plus step-scoped regressions cover stale-head, identity, alias, workflow wiring, and migration of legacy broader-suite contracts away from the retired single-process reviewer path.
- Fix `existing_noema_review()` treating a "legacy" Noema review (one posted before
Expand Down
23 changes: 23 additions & 0 deletions docs/product-technical-gap-baseline.md
Original file line number Diff line number Diff line change
Expand Up @@ -2590,3 +2590,26 @@ Higgins, S. S., Crepalde, N., & Fernandes, L. (2021). Segmented multiplexity: A
**Validation.** Full suite `2407 passed, 1 skipped, 21 subtests`; `coverage` 100% on `scripts/ci`; `interrogate` 100%; all four touched/added workflow files re-parse as valid YAML; `test_opencode_workflow_shell_syntax.py` and related shell-syntax tests pass unchanged.

**Residual.** This closes the specific floating-image contribution from these three central workflows; it does not by itself guarantee the organization-wide Actions queue is fully drained, since other repositories' own workflows and any remaining unpinned central workflows may still request the floating image. Worth a follow-up sweep across the rest of `.github/workflows/` and sibling-repo workflows if queuing persists after this lands.

## 2026-09-02 GitHub Actions review sidecar pool pinned to `orchestrator/free`; `auto` removed as an accepted value

**Problem.** `scripts/ci/contextual_orchestrator_review_sidecar.sh` — the script every central required review workflow (Strix, OpenCode Review, Noema Review, the PR-review autofix sidecar) provisions to talk to `contextual-orchestrator` — read an operator-settable `CONTEXTUAL_ORCHESTRATOR_POOL` environment variable, defaulted it to `free`, and validated it against exactly two accepted values: `free` or `auto` (`case "$orchestrator_pool" in free|auto) ...`). `auto` is a real, load-bearing value one layer down: `scripts/ci/contextual_orchestrator_review_launcher.py --pool auto` admits *priced* discovered routes as a fallback stage once the free pool is exhausted (`build_zdr_prioritized_catalog(..., pool="auto")`), by design, for callers that want that behavior. Nothing in this repository's own review-provisioning code path currently sets `CONTEXTUAL_ORCHESTRATOR_POOL=auto` — the only workflow that sets the variable at all, `strix.yml`, sets it to `free`; every other central review workflow simply relies on the script's own `:-free` default — so this was not a live incident, it was an unaudited, structurally-reachable escape hatch: a future edit to any of the four workflows above, or a manually-triggered `workflow_dispatch` with a custom env override, could set `CONTEXTUAL_ORCHESTRATOR_POOL=auto` and the sidecar would accept it silently, with no cost ceiling, no budget/authorization gate, and no reviewer visibility that priced models were now in scope for a required check.

**Why this matters now, not hypothetically.** The org's explicit standing operating directive (the perpetual PR review→fix→merge→develop loop this session runs under) states plainly that the free+ZDR routing combination is not yet solved reliably in central CI — this exact gap-baseline document's own accumulated 2026-08-30/08-31 entries above record a real `orchestrator/free` exhaustion incident, a crowding-out bug between shared-endpoint credentials, and multiple rounds of Devin-Review-caught admission-priority defects in `contextual_orchestrator_review_policy.py`, all specifically about getting the *free* pool right. Admitting a priced-inclusive `auto` pool into required review workflows before that work is solid would let one misconfiguration or one well-intentioned "let's widen coverage" workflow edit start spending real provider credit on every PR's required Strix/OpenCode/Noema review, with no operator-visible signal that this had happened — the sidecar's own `log` lines print the resolved pool, but nothing downstream alerts on it, and there is no spend cap in this repository's own review-provisioning path (unlike `contextual-orchestrator`'s own cost-ledger, which this vendored sidecar path does not call into for CI review spend).

**Alternatives considered.**
1. *Leave `auto` accepted but never set it.* Rejected: this is the status quo, and the status quo is exactly the unaudited escape hatch described above — "nobody currently sets it" is not a control, it is an absence of one.
2. *Remove the `CONTEXTUAL_ORCHESTRATOR_POOL` environment variable entirely, hard-coding `--pool free` with no override mechanism.* Considered and rejected in favor of the fail-closed `case` statement kept below: removing the variable removes the ability to reason about *why* an override was rejected (a caller setting `auto` would instead see an unrelated "unrecognized flag" or `--pool` argparse error further downstream, or silently fall through to whatever the launcher's own default resolves to, depending on how the removal was implemented) and removes a natural place to extend validation later (e.g. if the org ever explicitly re-authorizes `auto` for CI with a budget gate, only this one `case` arm needs to change). A `case` statement that explicitly names and rejects `auto` with a clear diagnostic is this repository's own established idiom (see the sibling `CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR` validation two lines above it in the same file) and is more auditable, not less.
3. *Narrow the launcher's own `--pool` argparse choices to just `("free",)`.* Rejected: the launcher (`contextual_orchestrator_review_launcher.py`) is a general-purpose CLI, not GitHub-Actions-specific — it is invoked directly (outside any workflow) for local testing and by other, non-CI-review callers that may have a legitimate reason to exercise the `auto` pool's priced-fallback behavior. Narrowing it there would remove functionality the tool's own design intentionally provides, contradicting the directive's explicit scoping ("GitHub Actions Workflow 이용에 관해" — regarding GitHub Actions Workflow *usage* specifically, not the tool in general). `test_launcher_uses_orchestrator_discovery_and_governed_pools`'s existing pin of `choices=("free", "auto")` on the launcher was therefore left unchanged.

**Fix.** `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s `case "$orchestrator_pool" in` now accepts only `free`; every other value (`auto` included, and any typo/unexpected value) falls to the `*)` arm and calls `fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"`, matching this script's own existing fail-closed idiom for `CONTEXTUAL_ORCHESTRATOR_REQUIRE_ZDR`. The variable's default (`${CONTEXTUAL_ORCHESTRATOR_POOL:-free}`) is unchanged, so every existing caller (all of which already resolve to `free`, explicitly or by default) is unaffected — this is a pure narrowing of previously-unused surface, not a behavior change for any current workflow run.

**Developer experience.** New `test_sidecar_pins_the_pool_to_free_for_github_actions` in `tests/test_contextual_orchestrator_review_sidecar_contract.py` extracts the sidecar's own `case "$orchestrator_pool" in ... esac` block as text and *executes* it (not just string-matches it) in a minimal bash harness against four inputs — `free` (must succeed, `pool_args=--pool free`), `auto` (must fail closed with the new diagnostic), empty string (must resolve to the `:-free` default and succeed, since bash's `:-` operator treats empty and unset identically), and an arbitrary bogus value (must fail closed) — so a future edit that silently re-widens the accepted set back to include `auto` (or any other value) breaks this test rather than passing unnoticed. Static assertions confirm the exact new source text (`case "$orchestrator_pool" in\n free)` and the new fail message) and the absence of the old text (`free|auto`, `must be free or auto`).

**Verified before touching anything.** Grepped every `.github/workflows/*.yml` for `CONTEXTUAL_ORCHESTRATOR_POOL` and any `--pool auto`/`pool.*auto` pattern: only `strix.yml` sets the variable, and it sets `free`. Grepped `scripts/ci/contextual_orchestrator_review_launcher.py`'s own `--pool` argparse and its one internal `pool="auto"` use (the priced-fallback stage, gated on `args.pool == "auto"` already being true from the CLI flag) to confirm that stage is reachable only when a caller explicitly requests `--pool auto` on the launcher directly — never as a side effect of the sidecar's own resolved value once this fix lands, since the sidecar can no longer produce `--pool auto`.

**Risk of this fix itself.** Low and one-directional: this can only ever cause a caller that was setting `CONTEXTUAL_ORCHESTRATOR_POOL=auto` to start failing closed with a clear diagnostic instead of silently proceeding with priced routes; grep confirms no current caller does this, so no existing workflow run's behavior changes. The failure mode if this fix is ever wrong (e.g. a legitimate future need for `auto` in CI) is a clear, immediate `fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"` diagnostic in the workflow log, not a silent behavior change — trivially reversible by widening the one `case` arm back, with the new regression test updated in the same PR to match.

**Expected effect.** No observable change to any current GitHub Actions review run (every current invocation already resolves to `free`). The effect is structural: it is no longer possible for a future workflow edit or manual dispatch override to admit priced-model spend into a required review check without an explicit, reviewed code change to this one `case` statement (and its now-locked-in regression test) first.

**Follow-up.** If the organization later solves free+ZDR routing robustly enough to deliberately widen required-review CI to `orchestrator/auto` (e.g. once a spend ceiling and reviewer-visible cost evidence exist for that path), the change is exactly one `case` arm plus the corresponding assertions in `test_sidecar_pins_the_pool_to_free_for_github_actions` — this entry is the record of *why* it was narrowed, not a permanent prohibition.
10 changes: 8 additions & 2 deletions scripts/ci/contextual_orchestrator_review_sidecar.sh
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,17 @@ esac

orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"
case "$orchestrator_pool" in
free|auto)
free)
pool_args=(--pool "$orchestrator_pool")
;;
*)
fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free or auto"
# GitHub Actions Workflow usage of contextual-orchestrator is pinned to
# orchestrator/free: the org has not solved cost-safe free+ZDR routing
# well enough yet to justify a priced-inclusive "auto" pool in central CI,
# so "auto" is rejected here even though the launcher's own --pool flag
# (a general-purpose CLI also used outside GitHub Actions) still accepts
# it.
fail "CONTEXTUAL_ORCHESTRATOR_POOL must be free"
;;
esac

Expand Down
23 changes: 17 additions & 6 deletions scripts/ci/current_head_run_coalescer.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,16 +140,21 @@ def select_duplicate_queued_run_ids(
branch: str,
head_sha: str,
) -> list[int]:
"""Select redundant queued runs while retaining one authoritative sibling."""
"""Select redundant queued runs while retaining one authoritative sibling.

``_run_identity_matches`` already requires a positive-int ``workflow_id``
before a run reaches this loop body, so re-deriving it here is only ever
non-``None`` -- grouping unconditionally, rather than behind a redundant
``is not None`` guard, avoids a branch no input can ever fail.
"""
groups: dict[int, list[dict[str, Any]]] = {}
for run_data in runs:
if not _run_identity_matches(
run_data, repository=repository, branch=branch, head_sha=head_sha
):
continue
workflow_id = _positive_int(run_data.get("workflow_id"))
if workflow_id is not None:
groups.setdefault(workflow_id, []).append(run_data)
groups.setdefault(workflow_id, []).append(run_data)

redundant: list[int] = []
for group in groups.values():
Expand Down Expand Up @@ -222,7 +227,15 @@ def validate_candidate_against_live_state(
current_pr_number: int | None = None,
associated_prs: Mapping[int, Mapping[str, Any]] | None = None,
) -> None:
"""Fail closed unless a queued candidate still has an authoritative sibling."""
"""Fail closed unless a queued candidate still has an authoritative sibling.

``_run_matches_head_identity`` already rejects any candidate whose
``event`` is not in ``PR_EVENTS`` before comparing repository, branch, or
SHA, so a non-pull-request candidate always fails the head-identity check
below rather than reaching a later, narrower event-only check -- there is
no candidate shape that can satisfy head identity while carrying a
disqualifying event.
"""
if candidate.get("status") != "queued":
raise CoalescingRefused("candidate is no longer queued")
if live_pr.get("state") != "open":
Expand All @@ -241,8 +254,6 @@ def validate_candidate_against_live_state(
workflow_id = _positive_int(candidate.get("workflow_id"))
if candidate_id is None or workflow_id is None:
raise CoalescingRefused("candidate identity is malformed")
if candidate.get("event") not in PR_EVENTS:
raise CoalescingRefused("candidate is not a pull-request workflow run")

association_map = associated_prs or {}
if current_pr_number is not None and not _run_pr_scope_is_safe(
Expand Down
43 changes: 43 additions & 0 deletions tests/test_contextual_orchestrator_review_sidecar_contract.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,49 @@ def test_sidecar_feeds_discovery_and_policy_artifacts_to_the_launcher() -> None:
assert "https://openrouter.ai/api/v1/endpoints/zdr" in text


def test_sidecar_pins_the_pool_to_free_for_github_actions() -> None:
"""GitHub Actions Workflow usage of contextual-orchestrator is pinned free.

``auto`` was removed from the sidecar's own accepted
``CONTEXTUAL_ORCHESTRATOR_POOL`` values: the org has not solved cost-safe
free+ZDR routing well enough yet to justify a priced-inclusive pool in
central CI. The launcher's own ``--pool`` flag (a general-purpose CLI
also used outside GitHub Actions, asserted separately above) still
accepts ``auto`` -- only this repo's GitHub-Actions-facing sidecar script
is narrowed.
"""
text = _read(SIDECAR)
assert 'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"' in text
assert 'case "$orchestrator_pool" in\n free)' in text
assert "fail \"CONTEXTUAL_ORCHESTRATOR_POOL must be free\"" in text
assert "free|auto" not in text
assert "must be free or auto" not in text

pool_case = text.split('orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"', 1)[
1
].split("esac", 1)[0]
for candidate, should_fail in (("free", False), ("auto", True), ("", False), ("bogus", True)):
result = subprocess.run(
[
"bash",
"-c",
'fail() { echo "FAIL: $*"; exit 7; }\n'
f'CONTEXTUAL_ORCHESTRATOR_POOL="{candidate}"\n'
'orchestrator_pool="${CONTEXTUAL_ORCHESTRATOR_POOL:-free}"\n'
+ pool_case
+ "esac\necho \"pool_args=${pool_args[*]}\"",
],
capture_output=True,
text=True,
)
if should_fail:
assert result.returncode == 7, (candidate, result.stdout, result.stderr)
assert "must be free" in result.stdout
else:
assert result.returncode == 0, (candidate, result.stdout, result.stderr)
assert "pool_args=--pool free" in result.stdout


def test_sidecar_exports_gateway_env_for_review_steps() -> None:
"""Only a private token-file path crosses the GitHub step boundary."""
text = _read(SIDECAR)
Expand Down
Loading
Loading