Skip to content
Merged
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
62 changes: 62 additions & 0 deletions .github/scripts/sync_codex_ok_labels.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

CODEX_OK_LABEL = "🤖 codex: ok"
CODEX_NEEDS_WORK_LABEL = "🤖 codex: needs work"
NEEDS_REBASE_LABEL = "needs rebase"
LEGACY_CODEX_LABELS = {"🤖 codex-ok"}
CODEX_REVIEW_AUTHORS = {
"chatgpt-codex-connector",
Expand All @@ -32,6 +33,8 @@
FAIL_CHECK_STATES = {"ACTION_REQUIRED", "CANCELLED", "ERROR", "FAILURE", "STALE", "TIMED_OUT"}
PENDING_CHECK_STATES = {"EXPECTED", "IN_PROGRESS", "PENDING", "QUEUED", "REQUESTED", "WAITING"}
UNMERGEABLE_STATES = {"DIRTY", "BLOCKED"}
NEEDS_REBASE_STATES = {"CONFLICTING", "DIRTY"}
NO_REBASE_STATES = {"BLOCKED", "CLEAN"}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove stale rebase labels for mergeable statuses

When a PR keeps a stale needs rebase label and GitHub reports UNSTABLE or HAS_HOOKS, this table makes needs_rebase_label_target() preserve the label even though GitHub documents those merge-state values as mergeable/non-conflict states, and the owning spec requires removing the label for known non-conflicts; the scheduled sync can therefore continue advertising a false rebase blocker until the PR reaches CLEAN. Include those mergeable statuses here or derive removal from the mergeable field instead. GitHub docs

Useful? React with 👍 / 👎.

CODEX_LB_REQUIRED_CHECKS = frozenset(
{
"Frontend lint (eslint)",
Expand Down Expand Up @@ -229,6 +232,9 @@ class SyncDecision:
has_needs_work_label: bool
wants_needs_work_label: bool
needs_work_action: str
has_needs_rebase_label: bool
wants_needs_rebase_label: bool
needs_rebase_action: str
legacy_labels: frozenset[str]
reason: str
review_url: str | None
Expand Down Expand Up @@ -933,6 +939,16 @@ def pr_merge_state(repo: str, number: int) -> str:
return merge_state or "UNKNOWN"


def needs_rebase_label_target(merge_state: str, *, has_label: bool) -> bool:
"""Sync confirmed conflicts and preserve the label when GitHub is ambiguous."""

if merge_state in NEEDS_REBASE_STATES:
return True
if merge_state in NO_REBASE_STATES:
return False
return has_label


def workflow_runs_requiring_approval(repo: str, head_sha: str) -> tuple[int, ...]:
runs = paged_api(f"/repos/{repo}/actions/runs?event=pull_request&head_sha={head_sha}")
run_ids: list[int] = []
Expand Down Expand Up @@ -1140,6 +1156,11 @@ def decide_pr(
)
has_ok_label = CODEX_OK_LABEL in labels
has_needs_work_label = CODEX_NEEDS_WORK_LABEL in labels
has_needs_rebase_label = NEEDS_REBASE_LABEL in labels
wants_needs_rebase_label = needs_rebase_label_target(
merge_state,
has_label=has_needs_rebase_label,
)
legacy_labels = frozenset(label for label in labels if label in LEGACY_CODEX_LABELS)

reason_parts: list[str] = []
Expand Down Expand Up @@ -1204,6 +1225,12 @@ def decide_pr(
needs_work_action = "remove"
else:
needs_work_action = "keep"
if wants_needs_rebase_label and not has_needs_rebase_label:
needs_rebase_action = "add"
elif not wants_needs_rebase_label and has_needs_rebase_label:
needs_rebase_action = "remove"
else:
needs_rebase_action = "keep"

review_url = unresolved_finding_urls[0] if unresolved_finding_urls else None
if review_url is None and isinstance(review_node, dict):
Expand All @@ -1219,6 +1246,9 @@ def decide_pr(
has_needs_work_label=has_needs_work_label,
wants_needs_work_label=wants_needs_work_label,
needs_work_action=needs_work_action,
has_needs_rebase_label=has_needs_rebase_label,
wants_needs_rebase_label=wants_needs_rebase_label,
needs_rebase_action=needs_rebase_action,
legacy_labels=legacy_labels,
reason="; ".join(reason_parts),
review_url=review_url,
Expand Down Expand Up @@ -1277,6 +1307,26 @@ def record(warning: str | None) -> None:
action=f"remove {CODEX_NEEDS_WORK_LABEL} from {decision.repo}#{decision.number}",
)
)
if decision.needs_rebase_action == "add":
record(
gh_api_write(
f"/repos/{decision.repo}/issues/{decision.number}/labels",
method="POST",
input_json={"labels": [NEEDS_REBASE_LABEL]},
tolerate_permission_errors=tolerate_permission_errors,
action=f"add {NEEDS_REBASE_LABEL} to {decision.repo}#{decision.number}",
)
)
elif decision.needs_rebase_action == "remove":
record(
gh_api_write(
f"/repos/{decision.repo}/issues/{decision.number}/labels/{quote(NEEDS_REBASE_LABEL, safe='')}",
method="DELETE",
tolerate_permission_errors=tolerate_permission_errors,
tolerate_missing=True,
action=f"remove {NEEDS_REBASE_LABEL} from {decision.repo}#{decision.number}",
)
)
for label in decision.legacy_labels:
record(
gh_api_write(
Expand Down Expand Up @@ -1414,6 +1464,16 @@ def main(argv: list[str] | None = None) -> int:
tolerate_permission_errors=args.tolerate_write_permission_errors,
)
)
setup_warnings.extend(
ensure_label(
repo,
NEEDS_REBASE_LABEL,
color="fbca04",
description="Needs rebase or conflict repair against current main",
apply=args.apply,
tolerate_permission_errors=args.tolerate_write_permission_errors,
)
)
for warning in setup_warnings:
print(f"warning: {warning}", file=sys.stderr, flush=True)
numbers = list_open_pr_numbers(repo) if args.all_open else list(args.pr or [])
Expand Down Expand Up @@ -1476,6 +1536,8 @@ def main(argv: list[str] | None = None) -> int:
f"ok={decision.has_ok_label}->{decision.wants_ok_label}/{decision.ok_action} "
f"needs_work={decision.has_needs_work_label}->{decision.wants_needs_work_label}/"
f"{decision.needs_work_action} "
f"needs_rebase={decision.has_needs_rebase_label}->{decision.wants_needs_rebase_label}/"
f"{decision.needs_rebase_action} "
f"legacy={','.join(sorted(decision.legacy_labels)) or '-'} "
f"approve_runs={','.join(str(run_id) for run_id in decision.approve_workflow_run_ids) or '-'} "
f"trigger_codex={decision.trigger_codex_review and not args.no_trigger_missing_codex} "
Expand Down
28 changes: 26 additions & 2 deletions openspec/specs/github-automation/spec.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,33 @@
# github-automation Specification

## Purpose
Repository automation around the Codex review merge gate: the `Codex review labels` workflow and its synchronization script keep `🤖 codex: ok` / `🤖 codex: needs work` labels faithful to current-head CI state and Codex review evidence, with token sourcing that stays within GitHub API quotas and degrades safely when privileged credentials are unavailable.
Repository automation around the Codex review merge gate: the `Codex review labels` workflow and its synchronization script keep `🤖 codex: ok` / `🤖 codex: needs work` labels faithful to current-head CI state and Codex review evidence, keep `needs rebase` faithful to confirmed merge-conflict state, and use token sourcing that stays within GitHub API quotas and degrades safely when privileged credentials are unavailable.
## Requirements
### Requirement: Needs-rebase label sync

The Codex label synchronization script MUST add `needs rebase` when GitHub
reports a confirmed merge conflict, MUST remove it when GitHub reports a known
non-conflict state, and MUST preserve its current value when merge state is
ambiguous. It MUST NOT infer a conflict from the pull request merely being
behind the base branch.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add the required OpenSpec change artifact

This commit changes scheduled GitHub label-sync behavior and updates the main spec directly, but it does not add or update a matching openspec/changes/<slug>/ proposal/tasks/spec delta for the behavior change; this repo treats OpenSpec changes as a hard readiness gate for behavior and operator-contract changes, so the PR would be blocked even though code and tests were added. Add the change artifact for the needs-rebase sync behavior before marking this ready.

Useful? React with 👍 / 👎.


#### Scenario: Confirmed conflict gains the label

- **WHEN** GitHub reports the pull request as `CONFLICTING` or `DIRTY`
- **THEN** the synchronizer adds `needs rebase`

#### Scenario: Review-blocked pull request loses a stale label

- **GIVEN** a pull request has `needs rebase`
- **WHEN** GitHub reports it as `BLOCKED` by review or status requirements
- **THEN** the synchronizer removes `needs rebase`

#### Scenario: Base lag alone does not create the label

- **WHEN** GitHub reports a pull request as `BEHIND` without a confirmed conflict
- **THEN** the synchronizer preserves the current label state
- **AND** it does not add `needs rebase` to an unlabelled pull request

### Requirement: Codex review label sync write-token fallback

The `Codex review labels` workflow MUST execute the label synchronization script from the trusted default branch and MUST prefer a dedicated GitHub App installation token, then a repository-provided write token, before falling back to the default `github.token`.
Expand Down Expand Up @@ -297,4 +322,3 @@ labels, so the override MUST NOT apply there: a change that would leave
- **WHEN** a budget is exceeded
- **THEN** no pull-request label set is resolved and the check fails
regardless of any label on the originating pull request

86 changes: 86 additions & 0 deletions tests/unit/test_sync_codex_ok_labels.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ def decision(module: ModuleType, **overrides: Any) -> Any:
"has_needs_work_label": False,
"wants_needs_work_label": False,
"needs_work_action": "keep",
"has_needs_rebase_label": False,
"wants_needs_rebase_label": False,
"needs_rebase_action": "keep",
"legacy_labels": frozenset(),
"reason": "checks are pending",
"review_url": None,
Expand All @@ -44,6 +47,89 @@ def decision(module: ModuleType, **overrides: Any) -> Any:
return module.SyncDecision(**values)


@pytest.mark.parametrize("merge_state", ["CONFLICTING", "DIRTY"])
def test_needs_rebase_label_target_adds_for_confirmed_conflicts(merge_state: str) -> None:
module = load_sync_module()

assert module.needs_rebase_label_target(merge_state, has_label=False) is True


@pytest.mark.parametrize("merge_state", ["BLOCKED", "CLEAN"])
def test_needs_rebase_label_target_removes_for_known_non_conflict_states(merge_state: str) -> None:
module = load_sync_module()

assert module.needs_rebase_label_target(merge_state, has_label=True) is False


@pytest.mark.parametrize("merge_state", ["BEHIND", "DRAFT", "HAS_HOOKS", "UNKNOWN", "UNSTABLE"])
@pytest.mark.parametrize("has_label", [False, True])
def test_needs_rebase_label_target_preserves_ambiguous_states(
merge_state: str,
has_label: bool,
) -> None:
module = load_sync_module()

assert module.needs_rebase_label_target(merge_state, has_label=has_label) is has_label


def test_apply_decision_adds_needs_rebase_label(monkeypatch: pytest.MonkeyPatch) -> None:
module = load_sync_module()
calls: list[tuple[str, str, Any | None]] = []

def capture_write(path: str, *, method: str = "GET", input_json: Any | None = None) -> None:
calls.append((method, path, input_json))

monkeypatch.setattr(module, "gh_api", capture_write)

warnings = module.apply_decision(
decision(
module,
ok_action="keep",
has_needs_rebase_label=False,
wants_needs_rebase_label=True,
needs_rebase_action="add",
)
)

assert warnings == ()
assert calls == [
(
"POST",
"/repos/Soju06/codex-lb/issues/714/labels",
{"labels": ["needs rebase"]},
)
]


def test_apply_decision_removes_stale_needs_rebase_label(monkeypatch: pytest.MonkeyPatch) -> None:
module = load_sync_module()
calls: list[tuple[str, str, Any | None]] = []

def capture_write(path: str, *, method: str = "GET", input_json: Any | None = None) -> None:
calls.append((method, path, input_json))

monkeypatch.setattr(module, "gh_api", capture_write)

warnings = module.apply_decision(
decision(
module,
ok_action="keep",
has_needs_rebase_label=True,
wants_needs_rebase_label=False,
needs_rebase_action="remove",
)
)

assert warnings == ()
assert calls == [
(
"DELETE",
"/repos/Soju06/codex-lb/issues/714/labels/needs%20rebase",
None,
)
]


def test_classify_check_state_uses_latest_run_for_duplicate_check_names() -> None:
module = load_sync_module()

Expand Down
Loading