Skip to content

Commit 347e8dd

Browse files
claudeclaude
committed
Fix Strix retry evidence staleness and draft-artifact-read permission
Two adversarial-review findings against the strix_evidence_state() fix: - strix_evidence_state() walked every Strix context node directly, so a rerun's stale failed CheckRun attempt (GitHub keeps every prior attempt's CheckRun node in the rollup alongside the latest one) could permanently keep the gate "failed" even after a later retry succeeded. Extract the CheckRun-identity dedup failed_status_checks() already used into a shared latest_check_run_attempts() helper and evaluate only the latest attempt per Strix CheckRun identity; failed_status_checks() now calls the same helper instead of duplicating the dedup logic. - active_draft_review_request()'s Actions-artifact read used the generic target-repository read credential, but the artifact always lives in the central .github repository regardless of which repository the PR belongs to, and the OpenCode app installation has no Actions permission. For a cross-repository dispatch with only the OpenCode app credential configured, the read would fail, so the initial mention-triggered request for a draft PR outside .github could never get past its own authorization check. New gh_api_json_via_dispatch_token() reads through the same central-repository dispatch credential already used to create the repository dispatch there, which the workflow always sets to the runner's own github.token. Added regression tests for both: a stale failed attempt vs. a later success (and the reverse), a running retry after a failure, and the dispatch-token vs. read-token credential selection. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
1 parent 48ebd77 commit 347e8dd

3 files changed

Lines changed: 171 additions & 39 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,39 @@ Semantic Versioning where the repository publishes a release.
8181
exhaustive regression fixtures for every non-passing terminal
8282
conclusion/state plus authoritative success, for both CheckRun and
8383
classic commit-status shapes.
84+
- Two more adversarial-review findings against that same fix, both fixed:
85+
- `strix_evidence_state()` walked every Strix context node in the
86+
rollup directly, so a rerun's stale failed CheckRun attempt (GitHub
87+
keeps every prior attempt's CheckRun node alongside the latest one)
88+
could permanently keep the gate `"failed"` even after a later retry
89+
succeeded. Extracted the CheckRun-identity dedup `failed_status_checks()`
90+
already used (latest attempt per `(workflow, name)`, by `startedAt`
91+
then rollup order) into a shared `latest_check_run_attempts()` helper
92+
and evaluate only the latest attempt per Strix CheckRun identity.
93+
`failed_status_checks()` itself now calls the same helper instead of
94+
duplicating the dedup logic, with no behavior change. Added
95+
regression tests for an older failed attempt followed by a newer
96+
success, the reverse ordering, and a running retry after a failure.
97+
- `active_draft_review_request()`'s Actions-artifact read used the
98+
generic target-repository read credential
99+
(`gh_api_json`/`SCHEDULER_READ_TOKEN`), but the artifact always lives
100+
in the central `.github` repository regardless of which repository
101+
the PR belongs to, and — per `scheduler_dispatch_env()`'s own
102+
pre-existing documented fact — "the OpenCode app installation has no
103+
Actions permission." For a cross-repository dispatch with only the
104+
OpenCode app credential configured (no `PR_REVIEW_MERGE_TOKEN`/
105+
`OPENCODE_APPROVE_TOKEN` secret), the read credential resolved to
106+
that same Actions-permission-less app token, so the artifact read
107+
would fail and the initial mention-triggered request for a draft PR
108+
outside `.github` could never get past its own authorization check.
109+
New `gh_api_json_via_dispatch_token()` reads through
110+
`run_github_dispatch()`/`SCHEDULER_DISPATCH_TOKEN` instead — the same
111+
central-repository dispatch credential already used to create the
112+
`repository_dispatch` there — which the workflow always sets to the
113+
runner's own `github.token`, valid for `.github`'s own Actions
114+
artifacts regardless of the PR's actual repository. Added a
115+
regression test proving the read uses the dispatch token, not
116+
whatever generic `GH_TOKEN` the OpenCode app credential resolves to.
84117
- Raise `contextual_orchestrator_review_sidecar.sh`'s
85118
`ORCHESTRATOR_CATALOG_FAMILY_CAP` default from 4 to 8: root-caused the
86119
live "no provider route passed the Strix plain-chat preflight" outage

‎scripts/ci/pr_review_merge_scheduler.py‎

Lines changed: 77 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,23 @@ def gh_api_json(path: str) -> Any:
765765
return json.loads(run_github_read(["gh", "api", path]))
766766

767767

768+
def gh_api_json_via_dispatch_token(path: str) -> Any:
769+
"""Run a GitHub REST API GET via the central-repository dispatch credential.
770+
771+
The OpenCode app installation has no Actions permission (see
772+
:func:`scheduler_dispatch_env`), and the target-repository read
773+
credential (:func:`gh_api_json`) is not guaranteed to have it either for
774+
a cross-repository dispatch. A read against ``.github``'s own Actions
775+
artifacts -- which always host the central draft-review-request marker
776+
regardless of which repository the PR belongs to -- must use the same
777+
central-repository dispatch credential already used for creating a
778+
``repository_dispatch`` there, not the target-repository read
779+
credential.
780+
"""
781+
782+
return json.loads(run_github_dispatch(["gh", "api", path]))
783+
784+
768785
def rest_review_node(review: dict[str, Any]) -> dict[str, Any]:
769786
"""Convert a REST review payload into the GraphQL shape used by the scheduler."""
770787

@@ -1151,18 +1168,64 @@ def opencode_in_progress(pr: dict[str, Any], *, stale_after_minutes: int | None
11511168
_STRIX_SUCCESS_CONCLUSIONS = {"SUCCESS"}
11521169

11531170

1171+
def latest_check_run_attempts(nodes: list[dict[str, Any]]) -> list[dict[str, Any]]:
1172+
"""Return each CheckRun's most recent attempt per (workflow, name) identity.
1173+
1174+
A rerun leaves every earlier attempt's CheckRun node in the rollup
1175+
alongside the latest one, so callers that walk ``context_nodes`` directly
1176+
can see a stale failed attempt outlive a later successful retry. This
1177+
resolves each CheckRun identity to only its most recently started
1178+
attempt (falling back to rollup order when ``startedAt`` is missing),
1179+
while passing every non-CheckRun (classic commit-status) node through
1180+
unchanged. The result preserves the original relative ordering.
1181+
"""
1182+
latest: dict[tuple[str, str], tuple[datetime | None, int, dict[str, Any]]] = {}
1183+
ordered: list[tuple[int, dict[str, Any]]] = []
1184+
for index, node in enumerate(nodes):
1185+
if node.get("__typename") != "CheckRun":
1186+
ordered.append((index, node))
1187+
continue
1188+
workflow = (
1189+
(((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name")
1190+
or ""
1191+
)
1192+
key = (workflow, node.get("name") or "check-run")
1193+
started_at = parse_github_datetime(node.get("startedAt"))
1194+
previous = latest.get(key)
1195+
if previous is None:
1196+
latest[key] = (started_at, index, node)
1197+
continue
1198+
previous_started_at, previous_index, _ = previous
1199+
if started_at is None and previous_started_at is not None:
1200+
continue
1201+
if previous_started_at is None and started_at is not None:
1202+
latest[key] = (started_at, index, node)
1203+
continue
1204+
if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= (
1205+
previous_started_at or datetime.min.replace(tzinfo=timezone.utc),
1206+
previous_index,
1207+
):
1208+
latest[key] = (started_at, index, node)
1209+
for started_at, index, node in latest.values():
1210+
ordered.append((index, node))
1211+
ordered.sort(key=lambda item: item[0])
1212+
return [node for _, node in ordered]
1213+
1214+
11541215
def strix_evidence_state(pr: dict[str, Any]) -> str:
11551216
"""Return missing, running, failed, or complete for current-head Strix evidence.
11561217
11571218
"complete" requires authoritative success (CheckRun conclusion or classic
11581219
commit-status state of SUCCESS). Any other terminal outcome -- failure,
11591220
error, cancelled, timed out, skipped, neutral, action_required, stale,
11601221
startup_failure -- is reported as "failed" rather than "complete" so
1161-
callers fail closed instead of unlocking on non-passing evidence.
1222+
callers fail closed instead of unlocking on non-passing evidence. Only
1223+
the latest attempt per Strix CheckRun identity is evaluated, so a stale
1224+
failed attempt cannot outlive a later successful retry.
11621225
"""
11631226
found = False
11641227
saw_failure = False
1165-
for node in context_nodes(pr):
1228+
for node in latest_check_run_attempts(context_nodes(pr)):
11661229
if not is_strix_context(node):
11671230
continue
11681231
found = True
@@ -1499,43 +1562,16 @@ def dismiss_stale_opencode_change_requests(repo: str, pr: dict[str, Any], *, dry
14991562
def failed_status_checks(pr: dict[str, Any]) -> list[str]:
15001563
"""Return failing check or status context names from the PR rollup."""
15011564
failed: list[str] = []
1502-
latest_check_runs: dict[
1503-
tuple[str, str],
1504-
tuple[datetime | None, int, dict[str, Any]],
1505-
] = {}
1506-
status_contexts: list[dict[str, Any]] = []
1507-
for index, node in enumerate(context_nodes(pr)):
1508-
if node.get("__typename") != "CheckRun":
1509-
status_contexts.append(node)
1510-
continue
1511-
workflow = (
1512-
(((node.get("checkSuite") or {}).get("workflowRun") or {}).get("workflow") or {}).get("name")
1513-
or ""
1514-
)
1515-
key = (workflow, node.get("name") or "check-run")
1516-
started_at = parse_github_datetime(node.get("startedAt"))
1517-
previous = latest_check_runs.get(key)
1518-
if previous is None:
1519-
latest_check_runs[key] = (started_at, index, node)
1520-
continue
1521-
previous_started_at, previous_index, _ = previous
1522-
if started_at is None and previous_started_at is not None:
1523-
continue
1524-
if previous_started_at is None and started_at is not None:
1525-
latest_check_runs[key] = (started_at, index, node)
1526-
continue
1527-
if (started_at or datetime.min.replace(tzinfo=timezone.utc), index) >= (
1528-
previous_started_at or datetime.min.replace(tzinfo=timezone.utc),
1529-
previous_index,
1530-
):
1531-
latest_check_runs[key] = (started_at, index, node)
1532-
1565+
nodes = latest_check_run_attempts(context_nodes(pr))
1566+
status_contexts = [node for node in nodes if node.get("__typename") != "CheckRun"]
15331567
successful_status_contexts = {
15341568
node.get("context")
15351569
for node in status_contexts
15361570
if (node.get("state") or "").upper() == "SUCCESS"
15371571
}
1538-
for _, _, node in sorted(latest_check_runs.values(), key=lambda item: item[1]):
1572+
for node in nodes:
1573+
if node.get("__typename") != "CheckRun":
1574+
continue
15391575
conclusion = (node.get("conclusion") or "").upper()
15401576
if conclusion in FAILED_CHECK_CONCLUSIONS:
15411577
if is_strix_context(node) and "strix" in successful_status_contexts:
@@ -2458,14 +2494,19 @@ def active_draft_review_request(repo: str, pr: dict[str, Any]) -> bool:
24582494
``repository_dispatch`` ``client_payload`` of its own, most commonly the
24592495
Strix-completion ``workflow_run`` that follows an initial
24602496
``security_dispatch`` -- checks the same durable signal here rather than
2461-
trusting anything the triggering event itself claims.
2497+
trusting anything the triggering event itself claims. The read always
2498+
uses the central-repository dispatch credential
2499+
(:func:`gh_api_json_via_dispatch_token`), because the artifact always
2500+
lives in that central repository regardless of which repository ``repo``
2501+
names, and the target-repository read credential is not guaranteed to
2502+
have Actions permission there for a cross-repository dispatch.
24622503
"""
24632504
head_sha = pr.get("headRefOid")
24642505
if not isinstance(head_sha, str) or not head_sha:
24652506
return False
24662507
dispatch_repo = repository_dispatch_target(validate_github_repository(repo))
24672508
artifact_name = draft_review_request_artifact_name(repo, pr["number"], head_sha)
2468-
response = gh_api_json(
2509+
response = gh_api_json_via_dispatch_token(
24692510
f"repos/{dispatch_repo}/actions/artifacts?name={artifact_name}&per_page=100"
24702511
)
24712512
return bool(_draft_review_request_records(response, expected_name=artifact_name))

‎tests/test_pr_review_merge_scheduler.py‎

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,31 @@ def test_context_review_and_check_helpers(monkeypatch):
10581058
classic_success = make_pr(statusCheckRollup={"contexts": {"nodes": [{"context": "strix", "state": "SUCCESS"}]}})
10591059
assert sched.strix_evidence_state(classic_success) == "complete"
10601060

1061+
# A stale failed attempt must not outlive a later successful retry, and a
1062+
# later failed attempt must override an earlier success -- only the
1063+
# latest attempt per Strix CheckRun identity counts (regression for a
1064+
# rerun leaving every earlier attempt's CheckRun node in the rollup).
1065+
older_failed_then_newer_success = make_pr(
1066+
statusCheckRollup={
1067+
"contexts": {"nodes": [strix_check(conclusion="FAILURE"), strix_check()]}
1068+
}
1069+
)
1070+
assert sched.strix_evidence_state(older_failed_then_newer_success) == "complete"
1071+
newer_failed_after_older_success = make_pr(
1072+
statusCheckRollup={
1073+
"contexts": {"nodes": [strix_check(), strix_check(conclusion="FAILURE")]}
1074+
}
1075+
)
1076+
assert sched.strix_evidence_state(newer_failed_after_older_success) == "failed"
1077+
running_retry_after_failure = make_pr(
1078+
statusCheckRollup={
1079+
"contexts": {
1080+
"nodes": [strix_check(conclusion="FAILURE"), strix_check(status="IN_PROGRESS", conclusion="")]
1081+
}
1082+
}
1083+
)
1084+
assert sched.strix_evidence_state(running_retry_after_failure) == "running"
1085+
10611086
threaded = make_pr(
10621087
reviewThreads={
10631088
"nodes": [
@@ -3821,7 +3846,7 @@ def fake_gh_api_json(path):
38213846
return {"total_count": 0, "artifacts": []}
38223847

38233848
monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github")
3824-
monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json)
3849+
monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json)
38253850

38263851
pr = make_pr(headRefOid="b" * 40)
38273852
assert sched.active_draft_review_request("owner/repo", pr) is False
@@ -3839,7 +3864,7 @@ def fake_gh_api_json(path):
38393864
"artifacts": [{"id": 7, "name": expected_name, "expired": False}],
38403865
}
38413866

3842-
monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json)
3867+
monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json)
38433868
pr = make_pr(headRefOid="b" * 40)
38443869
assert sched.active_draft_review_request("owner/repo", pr) is True
38453870

@@ -3852,7 +3877,7 @@ def fake_gh_api_json(path):
38523877
"artifacts": [{"id": 7, "name": expected_name, "expired": True}],
38533878
}
38543879

3855-
monkeypatch.setattr(sched, "gh_api_json", fake_gh_api_json)
3880+
monkeypatch.setattr(sched, "gh_api_json_via_dispatch_token", fake_gh_api_json)
38563881
pr = make_pr(headRefOid="b" * 40)
38573882
assert sched.active_draft_review_request("owner/repo", pr) is False
38583883

@@ -3861,6 +3886,39 @@ def test_active_draft_review_request_false_without_a_head_sha():
38613886
assert sched.active_draft_review_request("owner/repo", make_pr(headRefOid=None)) is False
38623887

38633888

3889+
def test_active_draft_review_request_uses_dispatch_token_not_opencode_app_token(monkeypatch):
3890+
"""Regression: the OpenCode app installation has no Actions permission, so
3891+
reading the draft-review-request artifact must use the same
3892+
central-repository dispatch credential as creating a repository
3893+
dispatch there, never the target-repository read credential -- which,
3894+
for a cross-repository dispatch with only the OpenCode app credential
3895+
configured, resolves to a token with no Actions permission."""
3896+
monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github")
3897+
monkeypatch.setenv("GH_TOKEN", "opencode-app-token")
3898+
monkeypatch.setenv("SCHEDULER_READ_TOKEN", "opencode-app-token")
3899+
monkeypatch.setenv("SCHEDULER_DISPATCH_TOKEN", "runner-token")
3900+
3901+
read_calls = []
3902+
dispatch_calls = []
3903+
monkeypatch.setattr(
3904+
sched,
3905+
"run_github_read",
3906+
lambda args, stdin=None: read_calls.append(args) or "{}",
3907+
)
3908+
monkeypatch.setattr(
3909+
sched,
3910+
"run_with_env",
3911+
lambda args, stdin=None, env=None: dispatch_calls.append((args, env["GH_TOKEN"]))
3912+
or '{"total_count": 0, "artifacts": []}',
3913+
)
3914+
3915+
pr = make_pr(headRefOid="b" * 40)
3916+
assert sched.active_draft_review_request("owner/repo", pr) is False
3917+
assert read_calls == []
3918+
assert len(dispatch_calls) == 1
3919+
assert dispatch_calls[0][1] == "runner-token"
3920+
3921+
38643922
def test_draft_review_request_records_fail_closed_on_malformed_responses():
38653923
name = "cwl-draft-review-request-owner-repo-1-" + "a" * 40
38663924
with pytest.raises(ValueError, match="must be an object"):

0 commit comments

Comments
 (0)