Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
48 changes: 48 additions & 0 deletions docs/doctoring/scheduler-explicit-open-live-dispatch-guard.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Scheduler의 명시적 OPEN·현재 head 확인

## 원인과 범위

#1902의 후속 조사에서 CodeQL 복구 primitive보다 먼저 고칠 공통 결함을 확인했다.
`4bf80b99b6908c0323ac406d7e30e8346e09a50d`의
`scripts/ci/pr_review_merge_scheduler_core.py`는 GraphQL 공통 PR fragment에서
`state`를 요청하지 않았고, REST PR 정규화에서도 그 필드를 보존하지 않았다.
그런데 `live_dispatch_head_matches`는 누락되거나 빈 state를 OPEN으로 취급했다.
단일 PR 조회는 닫힌 PR도 반환하므로 head가 그대로면 닫힌 PR을 허용할 수 있었다.
또한 양쪽 head를 빈 문자열로 대체해 비교했으므로 빈 값끼리도 일치했다.

영향 범위는 OpenCode repository dispatch, Strix의 기존 job rerun,
Strix repository dispatch 직전의 공통 guard다. 이번 수정은 이 세 경로의
새 실행 요청을 막는 조건만 다룬다. 앞서 수행되는 stale-run cleanup의 순서나
cancellation 정책은 바꾸지 않는다.

## 수정

- GraphQL 공통 fragment가 PR state를 실제로 요청한다.
- REST fallback은 원본 state를 대문자로 보존하고, 누락은 빈 값으로 남긴다.
- guard는 정확히 한 PR, 명시적 `OPEN`, 양쪽의 문자열 타입 40자리 hex SHA,
대소문자를 제외한 동일 head를 모두 요구한다.
- 기존 정상 fixture는 `OPEN`을 명시한다. 누락 사례를 정상 fixture로 대체하지 않는다.

토큰, 권한, trigger, queue, concurrency, dispatch payload는 변경하지 않았다.
CodeQL primitive도 추가하지 않았다. 조회 직후 PR 상태가 바뀔 수 있는 경쟁 조건과
중복 전송의 원자성은 여전히 미해결이며, 이 guard는 exact-once 보장이 아니다.
Cross-repo target callback의 Actions-write 권한도 별도 미해결 조건이다.

## 회귀 검증

`tests/test_scheduler_live_dispatch_guard.py`는 실제 guard와 세 caller를 실행하고
외부 API 및 실행 요청만 대체한다. 누락·빈 값·CLOSED·MERGED·UNKNOWN은 dispatch와
rerun에 도달하지 않아야 하며, OPEN의 정상 경로는 계속 도달해야 한다.
별도 사례가 빈 값, 잘못된 길이, 비-hex, 비문자열, 서로 다른 SHA를 거부하고
GraphQL 실제 query와 REST fallback의 state 전달을 확인한다.

Production 수정 전 새 회귀는 17 failed / 19 passed였다. 이후 실제 live head만
잘못된 사례 두 건도 추가했다. 최종 관련 5파일은 `-W error`를 적용해 정상 환경에서
380 passed, `GITHUB_ACTIONS=true` 환경에서도 380 passed를 확인했다.
검증 명령은 다음과 같다.

```sh
python -m pytest -q -W error tests/test_scheduler_live_dispatch_guard.py tests/test_pr_review_merge_scheduler.py tests/test_strix_rerun_job_selection.py tests/test_repository_branch_coverage_review_schedulers.py tests/test_pr_review_fix_scheduler_rest_workflow_identity.py
```

로컬 회귀 통과는 실제 GitHub dispatch, protected merge, 대상 job 복구의 증거가 아니다.
52 changes: 52 additions & 0 deletions docs/doctoring/strix-rerun-job-identity-binding.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Strix 재실행 대상 job의 신원 결합

## 확인한 결함

현재 main `ee5567f7b15f0441a61ec2435415603b9518f1c6`과 #1902의
`951d0ecd1b5398a9eac293a13bba220a6528df24`에서 Strix 재실행 선택 경로를 비교했다.
관련 core 차이는 이전 OPEN-state 수리뿐이었다. 이번 작업은 951 위에서 진행하며
main을 merge하거나 다른 세션의 workflow 변경을 덮어쓰지 않았다.

기존 선택기는 check의 details URL에서 job ID만 추출했다. 직전 live guard는
PR snapshot이 최신인지 확인했지만, 선택한 job이 그 PR head를 스캔했는지는
확인하지 않았다. 로컬 mock-only 회귀에서 실제 caller와 rerun wrapper를 실행한
결과 8 failed / 2 passed였다. 실패 사례는 job/run 조회 없이 mock POST에 도달했다.
실제 GitHub 위조 요청이나 job 재실행을 실행한 결과가 아니다.

## 최소 수리와 보류 조건

기존 selector와 API 조회 helper를 유지하고 Strix rerun 분기에 검증 하나를 추가했다.
GraphQL과 REST 정규화는 selected check의 database ID를 보존한다.

- selected check URL은 같은 repo의 정확한 run/job을 지정해야 한다.
- 실제 job의 ID, run ID, 이름, 완료 상태와 재실행 가능한 실패 결론을 확인한다.
현재 허용 결론은 failure, cancelled, timed_out이다. 다른 결론은 자동 재실행을 보류한다.
- job이 가리키는 실제 check ID가 selected check와 같아야 한다. Check publisher는
github-actions여야 하며 check suite와 run의 연결도 일치해야 한다.
- 실제 run과 workflow 조회는 같은 repo의 `.github/workflows/strix.yml`,
`Strix Security Scan` 이름을 확인한다.
- pull_request_target은 정확히 하나의 PR association, base/head repository,
association의 PR head, event에서 생성한 정확한 run-name이 모두 일치해야 한다.
job/run의 top-level head_sha가 base SHA인 정상 사례를 허용한다. 이 필드를
PR head로 간주하지 않는다. 누락되거나 상충하는 repository 식별자는 거부한다.
- repository_dispatch는 제어 코드의 실행 SHA만으로 target head를 증명할 수 없다.
이 경로에는 인증된 target receipt를 소비하는 계약이 없으므로, 제목이 맞더라도
자동 재실행을 보류한다. push 등 다른 event도 새로 허용하지 않는다.
- 검증이 끝난 뒤 live PR을 다시 확인한다. API 실패나 불완전한 metadata는
`identity_unverified`로 보류하고 새 dispatch로 우회하지 않는다. 세 상위 caller도
이를 실행 완료가 아닌 wait로 보고한다.

## 검증과 한계

`tests/test_strix_job_binding.py`는 실제 REST 정규화, selector, live guard,
dispatch caller, actor 검사, rerun wrapper를 실행한다. 외부 명령은 모두 mock 경계에서
차단한다. 정상 대조군은 top-level base SHA와 PR head SHA, REST repository URL 형식을
포함한다. 음성 사례는 stale·상충·누락·다른 repo/workflow/publisher/event·API 실패 및
검증 중 head 이동을 포함한다. 정상 사례는 네 metadata GET과 단일 mock POST를 요구한다.

기존 state-only, 명령형식, sibling 선택 테스트 세 곳은 각자의 검증 대상을 유지하도록
새 guard만 국소적으로 대체했다. 신원 결합 자체는 별도 회귀에서 실제 구현을 사용한다.

권한, 토큰 선택, actor allowlist, queue, concurrency, CodeQL primitive는 변경하지 않았다.
조회와 POST 사이의 원자성, cross-repo callback 권한, hosted 복구는 해결했다고 주장하지
않는다. 신뢰할 provenance가 없는 역사적 run은 자동 복구가 보류될 수 있다.
105 changes: 101 additions & 4 deletions scripts/ci/pr_review_merge_scheduler_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -167,17 +167,23 @@ def review_dispatch_admitted(component: str, repo: str, pr: dict[str, Any]) -> b
def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool:
"""Re-read the authoritative PR immediately before an Actions side effect."""
live = fetch_pr(validate_github_repository(repo), int(pr["number"]))
expected_head = pr.get("headRefOid")
live_head = live[0].get("headRefOid") if len(live) == 1 else None
return (
len(live) == 1
and str(live[0].get("state") or "OPEN").upper() == "OPEN"
and str(live[0].get("headRefOid") or "").lower()
== str(pr.get("headRefOid") or "").lower()
and live[0].get("state") == "OPEN"
and isinstance(expected_head, str)
and isinstance(live_head, str)
and GIT_SHA_RE.fullmatch(expected_head) is not None
and GIT_SHA_RE.fullmatch(live_head) is not None
and live_head.lower() == expected_head.lower()
)


PULL_REQUEST_FIELDS_FRAGMENT = """\
fragment SchedulerPullRequestFields on PullRequest {
number
state
title
author { login }
isDraft
Expand Down Expand Up @@ -226,6 +232,7 @@ def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool:
nodes {
__typename
... on CheckRun {
databaseId
name
status
conclusion
Expand Down Expand Up @@ -306,7 +313,7 @@ def live_dispatch_head_matches(repo: str, pr: dict[str, Any]) -> bool:
nodes {
__typename
... on CheckRun {
name status conclusion startedAt detailsUrl
databaseId name status conclusion startedAt detailsUrl
checkSuite { createdAt workflowRun { workflow { name } } }
}
... on StatusContext { context state }
Expand Down Expand Up @@ -1276,6 +1283,7 @@ def rest_check_node(
workflow = {"name": workflow_name} if workflow_name else {}
return {
"__typename": "CheckRun",
"databaseId": check.get("id"),
"name": check.get("name"),
"status": (check.get("status") or "").upper(),
"conclusion": (check.get("conclusion") or "").upper() if check.get("conclusion") else None,
Expand Down Expand Up @@ -1341,6 +1349,7 @@ def rest_pr_node(repo: str, pr: dict[str, Any]) -> dict[str, Any]:
)
return {
"number": number,
"state": str(pr.get("state") or "").upper(),
"title": pr.get("title"),
"author": {"login": ((pr.get("user") or {}).get("login"))},
"isDraft": bool(pr.get("draft")),
Expand Down Expand Up @@ -2906,6 +2915,8 @@ def post_update_branch_followup(
if wait_reason:
return f"{head_note}; {wait_reason}"
dispatch_result = dispatch_strix_evidence(repo, security_workflow, updated_pr, dry_run=dry_run)
if dispatch_result in {"identity_unverified", "stale_head"}:
return f"{head_note}; Strix rerun waits for verified current-target job identity"
if dispatch_result == "admission_deferred":
return f"{head_note}; bounded admission budget is exhausted"
if dispatch_result == "already_running":
Expand Down Expand Up @@ -3757,6 +3768,84 @@ def is_strix_scan_check_run(node: dict[str, Any]) -> bool:
)


def strix_rerun_identity_verified(repo: str, pr: dict[str, Any], job_id: str) -> bool:
"""Bind a selected Strix job to authenticated native PR-target run evidence.

Dispatch runs require target provenance beyond their control-plane SHA;
without an authenticated target receipt this path deliberately defers them.
A PR-target execution SHA may be the base SHA, so it is never used as the
target PR head. Association and trusted workflow run-name must agree instead.
"""
try:
repo = validate_github_repository(repo)
head = validate_git_sha(pr["headRefOid"]).lower()
head_repo = validate_github_repository(pr["headRepository"]["nameWithOwner"])
if not re.fullmatch(r"[1-9][0-9]*", job_id):
return False
candidates = [node for node in context_nodes(pr)
if is_strix_scan_check_run(node)
and actions_job_id_from_details_url(node.get("detailsUrl")) == job_id]
if len(candidates) != 1:
return False
selected = candidates[0]
url_match = re.fullmatch(
rf"https://github\.com/{re.escape(repo)}/actions/runs/([1-9][0-9]*)/job/{job_id}",
selected.get("detailsUrl") or "",
)
if not url_match:
return False
run_id = url_match.group(1)
job = gh_api_json(f"repos/{repo}/actions/jobs/{job_id}")
if (job.get("id") != int(job_id) or job.get("run_id") != int(run_id)
or job.get("name") != "strix" or job.get("status") != "completed"
or job.get("conclusion") not in {"failure", "cancelled", "timed_out"}):
return False
check_match = re.fullmatch(
rf"https://api\.github\.com/repos/{re.escape(repo)}/check-runs/([1-9][0-9]*)",
job.get("check_run_url") or "",
)
if not check_match or selected.get("databaseId") != int(check_match.group(1)):
return False
check = gh_api_json(f"repos/{repo}/check-runs/{check_match.group(1)}")
run_data = gh_api_json(f"repos/{repo}/actions/runs/{run_id}")
if (check.get("id") != selected["databaseId"] or check.get("name") != "strix"
or check.get("app", {}).get("slug") != "github-actions"
or run_data.get("id") != int(run_id)
or run_data.get("repository", {}).get("full_name") != repo
or run_data.get("event") != "pull_request_target"
or run_data.get("status") != "completed"
or run_data.get("name") != "Strix Security Scan"
Comment thread
seonghobae marked this conversation as resolved.
Outdated
or run_data.get("path") != ".github/workflows/strix.yml"
or not check.get("check_suite", {}).get("id")
or check["check_suite"]["id"] != run_data.get("check_suite_id")):
return False
workflow_id = run_data.get("workflow_id")
if type(workflow_id) is not int or workflow_id <= 0:
return False
workflow = gh_api_json(f"repos/{repo}/actions/workflows/{workflow_id}")
if (workflow.get("id") != workflow_id
or workflow.get("name") != "Strix Security Scan"
or workflow.get("path") != ".github/workflows/strix.yml"):
return False
associations = run_data.get("pull_requests") or []
if len(associations) != 1 or associations[0].get("number") != int(pr["number"]):
return False
association = associations[0]
for side, expected_repo in (("base", repo), ("head", head_repo)):
repository = association[side]["repo"]
if (not (repository.get("full_name") or repository.get("url"))
or (repository.get("full_name") is not None and repository["full_name"] != expected_repo)
or (repository.get("url") is not None
and repository["url"] != f"https://api.github.com/repos/{expected_repo}")):
return False
return (
validate_git_sha(association["head"]["sha"]).lower() == head
and run_data.get("display_title") == f"Strix Security Scan {repo}#{pr['number']}@{head}"
)
except (RuntimeError, ValueError, TypeError, KeyError, AttributeError):
return False


def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> str:
"""Dispatch same-head Strix workflow evidence before OpenCode reviews."""
job_id = matching_actions_job_id(pr, is_strix_scan_check_run)
Expand All @@ -3765,6 +3854,10 @@ def dispatch_strix_evidence(repo: str, workflow: str, pr: dict[str, Any], *, dry
return "admission_deferred"
if not dry_run and not live_dispatch_head_matches(repo, pr):
return "stale_head"
if not dry_run and not strix_rerun_identity_verified(repo, pr, job_id):
return "identity_unverified"
if not dry_run and not live_dispatch_head_matches(repo, pr):
return "stale_head"
rerun_actions_job(repo, job_id, dry_run=dry_run, action="rerun-strix-evidence")
return "rerun" if not dry_run else "dry_run"
if dry_run:
Expand Down Expand Up @@ -4116,6 +4209,8 @@ def dispatch_draft_review_only(
f"draft PR review-only dispatch; current head has no completed Strix evidence; {wait_reason}",
)
dispatch_result = dispatch_strix_evidence(repo, security_workflow, pr, dry_run=dry_run)
if dispatch_result in {"identity_unverified", "stale_head"}:
return Decision(number, "wait", "Strix rerun waits for verified current-target job identity")
if dispatch_result == "admission_deferred":
return Decision(number, "wait", "draft PR review-only dispatch; bounded admission budget is exhausted")
if dispatch_result == "already_running":
Expand Down Expand Up @@ -4920,6 +5015,8 @@ def request_branch_update(freshness_reason: str, *, suffix: str = "") -> Decisio
if wait_reason:
return decide("wait", f"current head has no completed Strix evidence; {wait_reason}")
dispatch_result = dispatch_strix_evidence(repo, security_workflow, pr, dry_run=dry_run)
if dispatch_result in {"identity_unverified", "stale_head"}:
return decide("wait", "Strix rerun waits for verified current-target job identity")
if dispatch_result == "admission_deferred":
return decide("wait", "bounded admission budget is exhausted")
if dispatch_result == "already_running":
Expand Down
18 changes: 18 additions & 0 deletions tests/test_pr_review_merge_scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def fake_fine_grained_github_token(body):
def make_pr(**overrides):
value = {
"number": 1,
"state": "OPEN",
"title": "Central review",
"author": {"login": "pull-request-author"},
"isDraft": False,
Expand Down Expand Up @@ -4611,6 +4612,8 @@ def fake_run(args, stdin=None):
}
)
sched.dispatch_opencode_review("owner/repo", "OpenCode Review", required_workflow_pr, dry_run=False)
# Command-shape contract; selected-job binding is exercised separately.
monkeypatch.setattr(sched, "strix_rerun_identity_verified", lambda *_: True)
sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", required_workflow_pr, dry_run=False)
assert calls[:2] == [
[
Expand Down Expand Up @@ -7884,6 +7887,16 @@ def test_draft_pr_review_only_dispatch_waits_when_strix_already_running(monkeypa
assert decision.reason == "draft PR review-only dispatch; same-head Strix evidence is still running"


@pytest.mark.parametrize("result", ["identity_unverified", "stale_head"])
@pytest.mark.parametrize("draft", [False, True])
def test_unverified_strix_rerun_is_reported_as_wait(monkeypatch, result, draft):
"""A withheld rerun must never be reported as a successful security dispatch."""
monkeypatch.setattr(sched, "dispatch_strix_evidence", lambda *_, **__: result)
decision = inspect(make_pr(isDraft=draft), allow_draft_review_dispatch=draft)
assert decision.action == "wait"
assert "verified current-target job identity" in decision.reason


def test_draft_pr_review_only_dispatch_waits_when_repository_is_busy(monkeypatch):
monkeypatch.setattr(
sched,
Expand Down Expand Up @@ -8293,6 +8306,11 @@ def followup(updated_pr, **overrides):
statusCheckRollup={"contexts": {"nodes": [strix_check(status="IN_PROGRESS", conclusion="")]}},
)
)
for withheld in ("identity_unverified", "stale_head"):
monkeypatch.setattr(sched, "dispatch_strix_evidence", lambda *_, **__: withheld)
assert "waits for verified current-target job identity" in followup(
make_pr(headRefOid="new-head")
)
assert "same-head OpenCode review is already running" in followup(
make_pr(
headRefOid="new-head",
Expand Down
Loading
Loading