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
54 changes: 53 additions & 1 deletion .github/scripts/sync_codex_ok_labels.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,57 @@ def timeline_head_oid(node: dict[str, Any]) -> str | None:
return None


def unresolved_review_comment_urls(repo: str, number: int) -> set[str]:
owner, name = repo.split("/", 1)
after: str | None = None
urls: set[str] = set()

while True:
fields: dict[str, object] = {"owner": owner, "name": name, "number": number}
if after is not None:
fields["after"] = after
payload = graphql(PR_REVIEW_THREADS_QUERY, **fields)
pr = payload.get("data", {}).get("repository", {}).get("pullRequest")
if not isinstance(pr, dict):
raise GhError(f"{repo}#{number}: GraphQL did not return a pull request")

threads = pr.get("reviewThreads")
if not isinstance(threads, dict):
raise GhError(f"{repo}#{number}: GraphQL did not return review threads")
nodes = threads.get("nodes", [])
if not isinstance(nodes, list):
raise GhError(f"{repo}#{number}: GraphQL did not return review thread nodes")

for thread in nodes:
if not isinstance(thread, dict):
continue
if thread.get("isResolved") or thread.get("isOutdated"):
continue
comments = thread.get("comments")
comment_nodes = comments.get("nodes") if isinstance(comments, dict) else []
if not isinstance(comment_nodes, list):
continue
for comment in comment_nodes:
if not isinstance(comment, dict):
continue
url = comment.get("url")
if isinstance(url, str):
urls.add(url)

page_info = threads.get("pageInfo")
if not isinstance(page_info, dict) or not page_info.get("hasNextPage"):
break
end_cursor = page_info.get("endCursor")
if not isinstance(end_cursor, str) or not end_cursor:
break
after = end_cursor

return urls


def pull_review_comment_nodes(repo: str, number: int, *, head_sha: str) -> list[dict[str, Any]]:
comments = paged_api(f"/repos/{repo}/pulls/{number}/comments")
unresolved_urls = unresolved_review_comment_urls(repo, number)
nodes: list[dict[str, Any]] = []
for comment in comments:
body = comment.get("body")
Expand All @@ -385,6 +434,9 @@ def pull_review_comment_nodes(repo: str, number: int, *, head_sha: str) -> list[
review_id = comment.get("pull_request_review_id")
if commit_id != head_sha and original_commit_id != head_sha:
continue
html_url = comment.get("html_url") or comment.get("url")
if is_needs_work_codex_body(body) and html_url not in unresolved_urls:
continue
user = comment.get("user")
login = user.get("login") if isinstance(user, dict) else None
nodes.append(
Expand All @@ -393,7 +445,7 @@ def pull_review_comment_nodes(repo: str, number: int, *, head_sha: str) -> list[
"author": {"login": login} if isinstance(login, str) else None,
"bodyText": body,
"createdAt": comment.get("created_at"),
"url": comment.get("html_url") or comment.get("url"),
"url": html_url,
"commit": {"oid": commit_id} if isinstance(commit_id, str) else None,
"pullRequestReviewDatabaseId": review_id if isinstance(review_id, int) else None,
}
Expand Down
19 changes: 19 additions & 0 deletions openspec/changes/ignore-resolved-codex-inline-findings/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
## Why

The Codex review label synchronizer can keep `πŸ€– codex: needs work` on a PR
after a maintainer resolves an inline Codex finding. The unresolved-thread check
already knows resolved review threads are no longer blockers, but the timeline
classifier still merges the old inline review comment and treats it as current
needs-work evidence.

## What Changes

- Ignore inline Codex finding comments from resolved or outdated review threads
when computing current-head review state.
- Keep unresolved inline Codex finding comments as needs-work evidence.
- Add regression coverage for both resolved and unresolved inline findings.

## Impact

- GitHub automation only.
- No application runtime behavior changes.
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## MODIFIED Requirements

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 Declare the new OpenSpec requirement as added

This delta is under MODIFIED Requirements, but openspec/specs/github-automation/spec.md does not contain an existing requirement named Codex review label sync review-thread state; it currently only defines the write-token fallback and write-denial resilience requirements. OpenSpec treats MODIFIED as changes to an existing requirement, so strict validation/archive will fail for this change instead of accepting the new review-thread-state contract. Since this is a new requirement, move it under ADDED Requirements or add the matching base requirement first.

Useful? React with πŸ‘Β / πŸ‘Ž.


### Requirement: Codex review label sync review-thread state

The Codex label synchronization script MUST treat unresolved, non-outdated
Codex inline review findings on the current head as needs-work evidence. It
MUST NOT treat inline Codex findings from resolved or outdated review threads as
active needs-work evidence.

#### Scenario: Resolved inline finding no longer blocks the ok label

- **WHEN** a current-head inline Codex finding comment belongs to a resolved
review thread
- **AND** a clean current-head Codex review exists
- **THEN** the script does not classify that inline finding as active
needs-work evidence

#### Scenario: Unresolved inline finding still blocks the ok label

- **WHEN** a current-head inline Codex finding comment belongs to an unresolved,
non-outdated review thread
- **THEN** the script classifies that inline finding as active needs-work
evidence
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Implementation

- [x] Filter current-head inline Codex finding comments by unresolved review-thread URLs.
- [x] Preserve unresolved inline finding comments as needs-work evidence.
- [x] Add unit coverage for resolved and unresolved inline findings.
- [x] Validate the focused test and OpenSpec gates.
54 changes: 54 additions & 0 deletions tests/unit/test_sync_codex_ok_labels.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,3 +211,57 @@ def fail_apply(*_args: Any, **_kwargs: Any) -> tuple[str, ...]:
captured = capsys.readouterr()
assert result == 1
assert "Soju06/codex-lb#714: gh: HTTP 500 while writing labels" in captured.err


def test_resolved_inline_codex_finding_does_not_count_as_review_news(
monkeypatch: pytest.MonkeyPatch,
) -> None:
module = load_sync_module()

monkeypatch.setattr(
module,
"paged_api",
lambda _path: [
{
"body": "**P1 Badge** resolved finding",
"commit_id": "a" * 40,
"original_commit_id": "a" * 40,
"pull_request_review_id": 123,
"html_url": "https://github.test/review/resolved",
"created_at": "2026-06-14T00:00:00Z",
"user": {"login": "chatgpt-codex-connector"},
}
],
)
monkeypatch.setattr(module, "unresolved_review_comment_urls", lambda *_args: set())

assert module.pull_review_comment_nodes("Soju06/codex-lb", 714, head_sha="a" * 40) == []


def test_unresolved_inline_codex_finding_counts_as_review_news(
monkeypatch: pytest.MonkeyPatch,
) -> None:
module = load_sync_module()
url = "https://github.test/review/unresolved"

monkeypatch.setattr(
module,
"paged_api",
lambda _path: [
{
"body": "**P1 Badge** unresolved finding",
"commit_id": "a" * 40,
"original_commit_id": "a" * 40,
"pull_request_review_id": 123,
"html_url": url,
"created_at": "2026-06-14T00:00:00Z",
"user": {"login": "chatgpt-codex-connector"},
}
],
)
monkeypatch.setattr(module, "unresolved_review_comment_urls", lambda *_args: {url})

nodes = module.pull_review_comment_nodes("Soju06/codex-lb", 714, head_sha="a" * 40)

assert len(nodes) == 1
assert nodes[0]["url"] == url
Loading