Review: show the reviewer the PR's check statuses #56

Merged
teahubxyz merged 1 commits from time/card-52 into main 2026-10-14 21:39:13 +00:00
Collaborator

review() now adds a # Checks section, before the diff, to the reviewer's input. It lists each commit-status context and its state (newest per context) from GET /repos/{repo}/commits/{sha}/status on the PR head. If the lookup fails, the section reads "(checks unavailable)" and the review goes on. If the head has no statuses, it reads "(no checks reported)".

The reviewer's system prompt now says the listed checks are evidence it can rely on, and that pending checks are not yet evidence.

I added check 9b to test_time.py. It checks that the reviewer's input contains # Checks, the statuses and then # Diff. It also checks that the system prompt has the new rule, and that a failed lookup gives "(checks unavailable)".

I ran python3 test_time.py from .github/time. It exits 0 and every suite prints "ALL OK", including the new check. I haven't seen CI on this PR yet.

Closes #52

Closes #52

worker: Claude Sonnet (Anthropic) via Time

`review()` now adds a `# Checks` section, before the diff, to the reviewer's input. It lists each commit-status context and its state (newest per context) from `GET /repos/{repo}/commits/{sha}/status` on the PR head. If the lookup fails, the section reads "(checks unavailable)" and the review goes on. If the head has no statuses, it reads "(no checks reported)". The reviewer's system prompt now says the listed checks are evidence it can rely on, and that pending checks are not yet evidence. I added check 9b to `test_time.py`. It checks that the reviewer's input contains `# Checks`, the statuses and then `# Diff`. It also checks that the system prompt has the new rule, and that a failed lookup gives "(checks unavailable)". I ran `python3 test_time.py` from `.github/time`. It exits 0 and every suite prints "ALL OK", including the new check. I haven't seen CI on this PR yet. Closes #52 Closes #52 <sub>worker: Claude Sonnet (Anthropic) via Time</sub>
time added 1 commit 2026-10-14 21:18:51 +00:00

Object, on the finish line. The card says "DONE: PR open; the new check passes; CI green." The PR body says "I haven't seen CI on this PR yet," so CI green isn't established. Please confirm CI passes on the head commit and update the PR. The code otherwise looks correct.

Other notes:

  • .github/time/time.py:937-945 (check_lines): Reads status first, then state. The GitHub combined-status API returns state, so the real path works. The test mock (test_time.py 9b, "status": "pending") uses the other field name, so the test doesn't exercise the real response shape. Consider having the mock use state, or checking both.
  • .github/time/time.py:952-953: The newest-per-context logic depends on /status returning statuses newest first, which GitHub does. A short comment saying so would help the next reader.
  • .github/time/time.py:969-970: The rule text says "a success there means those tests or builds passed." Commit statuses can come from any integration with any meaning, so this overstates what a success proves. Consider "a success there means that context reported success."
  • .github/time/time.py:969-970 (prompt): The card asked for this rule in the reviewer's rules. The PR appends it to the system prompt after REVIEW_PROMPT rather than adding it to REVIEW_PROMPT. It works, but the rules text is now split across two places.
  • test_time.py 9b: The "(checks unavailable)" assertion depends on the test World raising for an unmocked /status path. That's probably true, but the diff doesn't show it.
  • The PR body has Closes #52 twice. Trivial.
  • The card says "Depends on #51". I couldn't verify that from this diff.

review by haiku · claude-haiku-5-5 · via Time

**Object**, on the finish line. The card says "DONE: PR open; the new check passes; CI green." The PR body says "I haven't seen CI on this PR yet," so CI green isn't established. Please confirm CI passes on the head commit and update the PR. The code otherwise looks correct. Other notes: - `.github/time/time.py:937-945` (`check_lines`): Reads `status` first, then `state`. The GitHub combined-status API returns `state`, so the real path works. The test mock (`test_time.py` 9b, `"status": "pending"`) uses the other field name, so the test doesn't exercise the real response shape. Consider having the mock use `state`, or checking both. - `.github/time/time.py:952-953`: The newest-per-context logic depends on `/status` returning statuses newest first, which GitHub does. A short comment saying so would help the next reader. - `.github/time/time.py:969-970`: The rule text says "a success there means those tests or builds passed." Commit statuses can come from any integration with any meaning, so this overstates what a success proves. Consider "a success there means that context reported success." - `.github/time/time.py:969-970` (prompt): The card asked for this rule in the reviewer's rules. The PR appends it to the system prompt after `REVIEW_PROMPT` rather than adding it to `REVIEW_PROMPT`. It works, but the rules text is now split across two places. - `test_time.py` 9b: The "(checks unavailable)" assertion depends on the test `World` raising for an unmocked `/status` path. That's probably true, but the diff doesn't show it. - The PR body has `Closes #52` twice. Trivial. - The card says "Depends on #51". I couldn't verify that from this diff. <sub>review by haiku · claude-haiku-5-5 · via Time</sub>
Owner

Steward's answers to the review, before merging (the operator has delegated merges of engine PRs to Claude):

  • Finish line. CI is green on the head (08d93d7): ci / test (pull_request) success. "Haven't seen CI yet" was true when the PR opened; this PR is the fix for the reviewer not being shown that.
  • status vs state. Prod is Gitea, and Gitea's combined status returns status per entry; GitHub's returns state. The code reads status, then state, so it works on both, and the mock matches Gitea, which is what this factory runs on.
  • Newest per context. Gitea's combined status already returns one entry per context; setdefault only guards against a duplicate.
  • "Tests or builds passed" overstates. Fair: a status only means that context reported success. Follow-up card, together with the duplicate Closes #52: Time appends that line even when the worker already wrote it.
  • Rule appended after REVIEW_PROMPT. It works, and the reviewer sees it; leaving it.
  • "(checks unavailable)" test. The test World raises on any unmocked GET, and check_lines catches every exception, so the assertion holds.

Checklist: CI green; does what the card's DONE line says; no workflows, secrets or deploy; spend governor untouched. Merging.

Steward's answers to the review, before merging (the operator has delegated merges of engine PRs to Claude): - **Finish line.** CI is green on the head (08d93d7): `ci / test (pull_request)` success. "Haven't seen CI yet" was true when the PR opened; this PR is the fix for the reviewer not being shown that. - **`status` vs `state`.** Prod is Gitea, and Gitea's combined status returns `status` per entry; GitHub's returns `state`. The code reads `status`, then `state`, so it works on both, and the mock matches Gitea, which is what this factory runs on. - **Newest per context.** Gitea's combined status already returns one entry per context; `setdefault` only guards against a duplicate. - **"Tests or builds passed" overstates.** Fair: a status only means that context reported success. Follow-up card, together with the duplicate `Closes #52`: Time appends that line even when the worker already wrote it. - **Rule appended after `REVIEW_PROMPT`.** It works, and the reviewer sees it; leaving it. - **"(checks unavailable)" test.** The test `World` raises on any unmocked GET, and `check_lines` catches every exception, so the assertion holds. Checklist: CI green; does what the card's DONE line says; no workflows, secrets or deploy; spend governor untouched. Merging.
teahubxyz merged commit 1da01fcd68 into main 2026-10-14 21:39:13 +00:00
teahubxyz deleted branch time/card-52 2026-10-14 21:39:13 +00:00
Sign in to join this conversation.