fix(github): stop a stale review request or an "Update branch" merge from clearing a rejection - #909
Conversation
…to it `derivePrReferenceStatus` read any pending `reviewRequests.totalCount > 0` as the author clicking re-request, and so demoted a live "changes requested" to "waiting for review". That is only true of a request made AFTER the review. On a PR where several reviewers were asked at once and one of them rejected it, the others stay listed indefinitely with nothing having happened since — observed on a real PR: three reviewers requested at 2026-08-18T10:28, changes requested 2026-08-19T11:00, head commit older than both, one request still pending. The chip said "Waiting for review · no one has reviewed it yet" over a rejection. The standing request now has to postdate the review to count. Its date comes from `timelineItems(last: 1, itemTypes: [REVIEW_REQUESTED_EVENT])`, riding the same aliased node at no extra request. Two behaviours are deliberately kept: an empty `reviews` with a pending request still hands the ball back (the case with no other tell), and a commit newer than the review still does too — the reporter called that one correct. An undated request against a dated review is conservative: the review stands.
A merge commit at the head no longer counts as the push that clears a rejection. GitHub's Update-branch button writes `Merge branch 'main' into <branch>` dated NOW — newer than any review, addressing none of it — so the one click people make reflexively on a stale PR flipped the chip from "Changes requested" to "Waiting for review" with nothing addressed. `parents.totalCount >= 2` on the head commit is what tells that commit apart from work. It rides the same aliased node as the rest (`parents(first: 0)`: the count is the whole question, so no parent is fetched). Shape confirmed live on a real PR, whose head is exactly such a merge. An absent count reads as an ordinary commit — the push rule is the common path and must not switch off on a field GitHub declined to send. Merging the base AND pushing fixes together, with the merge landing last, reads as unanswered until the next real commit. Does not cover "Update with rebase", which rewrites every committer date and is indistinguishable from real work; the spec records why that trade was not taken.
`main` grew a second axis for reference chips — `mergeable` on the same batched node, surfaced as `Mergeability`/`mergeabilityOf` — and it landed on the three spots this branch was already editing. Resolved by keeping both sides in each: - the `refStatusQuery` doc block: this branch's paragraph on `parents` and the `ReviewRequestedEvent`, followed by main's on `mergeable` riding the same node; - the query string itself: `mergeable` plus `parents(first: 0)` and the `timelineItems` review-request event; - the helpers: main's `Mergeability`/`mergeabilityOf` kept whole, and this branch's `isAfter` in place of `pushedSince`, which it replaced and which now has no callers. The schema and the `derivePrReferenceStatus` call site auto-merged with both sides' fields intact — verified by reading, not assumed. The combined query was re-validated against the live GraphQL API on a real PR: `mergeable`, `parents.totalCount` and the timeline event all come back together in one request.
📦 npm preview published —
|
|
🤖 |
patzick
left a comment
patzick
left a comment
There was a problem hiding this comment.
🔍 Code Review
PR #909 — fix(github): stop a stale review request or an "Update branch" merge from clearing a rejection
First review (no prior review on this PR). Scope: main...HEAD — 3 files, +304 −45.
🚫 Verdict posted as a comment, not a formal review. This PR is authored by the same account running the review, and GitHub refuses a formal verdict on your own pull request (
Can not request changes on your own pull request). The verdict below is CHANGES REQUESTED and is treated as such by this run — the labels, the handoff and the autofix loop all follow it — but it cannot carry theREQUEST_CHANGESreview state, and it does not satisfy this repository's review requirement: #909 still needs an independent reviewer to clearBLOCKED.
⚙️ Engine note. The configured review engine
om-code-reviewis not installed on this machine, so this pass ran the locally availablecode-reviewengine instead, athigheffort, plus the repository's ownvalidation.commandsgate in full. The severity model, verdict rule, and report structure below areom-auto-review-pr's as specified. Flagging the substitution because the skill contract names that engine explicitly.
📋 Summary
The PR narrows derivePrReferenceStatus's "the author already answered the review" test along two axes: a standing review request must now postdate the review to count, and a merge commit at the head no longer counts as a push. Both are well-motivated, both carry real observed data behind them, and both are covered by tests that were demonstrated to fail against the pre-fix logic. The combination matrix grew to 256 rows with two new invariants, which is the right shape for a precedence function that has now been edited five times.
Two defects found, both in the second axis's implementation rather than its intent.
🚧 Blockers
packages/cezar/src/server/forge/github.ts:1349 — a merge landing on top erases an earlier genuine answer, flipping the chip back to red.
pushed reads only commits(last: 1), so any merge at the head forces it false regardless of what came before:
const pushed = (pr.headParentCount ?? 1) < 2 && isAfter(pr.headCommittedAt, pr.changesRequestedAt);Failure sequence, all four steps ordinary:
- Reviewer requests changes.
- Author pushes a real fix. Head is a normal commit newer than the review →
pushedtrue → chip is bluereview-required. Correct. - The base moves on and the PR goes conflicting.
- Author presses Resolve conflicts in this very cockpit — whose prompt is literally
Merge head branch and resolve conflicts in PR number N— or GitHub's Update branch. The head is now a 2-parent merge.
headParentCount is 2, so pushed collapses to false, answered goes false, and the chip flips back to red changes-requested, blaming an author who answered two steps ago and has since only fixed conflicts.
This is a regression, not a pre-existing gap: before this PR, step 4's merge was newer than the review and read review-required, which was the right answer. It also lands squarely in the PR's own subject — producing exactly the "wrong person is blamed" outcome the change exists to prevent — and it is reachable through a first-class cockpit button, so it is not a corner case.
The commit message anticipates a neighbouring case ("merging the base AND pushing fixes together … reads as unanswered until the next real commit") and calls it conservative. That reasoning holds when there was never an answer to lose; it does not hold here, where a correct blue chip is actively downgraded to a wrong red one.
Fix. Judge the push by the newest commit that is actual work rather than by the head: when the head is a merge, its first parent is the branch's previous tip — precisely the author's last real commit. parents(first: 1) returns that parent's committedDate alongside the totalCount already being fetched, so the correction costs no extra request.
⚠️ Major
packages/cezar/src/server/forge/github.ts:1503 — reviewRequestedAt and reviewRequested answer questions about different requests.
reviewRequested is reviewRequests.totalCount > 0 — requests standing now. reviewRequestedAt is the newest ReviewRequestedEvent — a request made at some point, whether or not it still stands. Nothing ties the two to the same reviewer.
So: an old request stands (predating the review, the exact case this PR fixes), and a second request was created after the review and then removed. reviewRequested is true from the old one; reviewRequestedAt is dated from the removed one and postdates the review; reRequested goes true and the live rejection is hidden behind "waiting for review" again.
Rarer than the bug this PR fixes — it needs an added-then-removed request alongside a standing older one — but it is the same class, and it defeats the same fix.
Fix. Correlate the two by reviewer: take the newest ReviewRequestedEvent whose requestedReviewer is still in reviewRequests. Both connections are already on the aliased node, so this stays one request.
✅ Verified and cleared
- Backward compatibility (
BACKWARD_COMPATIBILITY.md:60, the protectedGET /api/v1/github/ref-statussurface): the status vocabulary is untouched — no value added, renamed, or removed. Only which existing value a given PR receives changes, which is the bug fix. Every new field onderivePrReferenceStatus's parameter object is optional, so existing callers compile unchanged, and the function has exactly one caller.mergeabilityOf/ theconflictsaxis from #904 survived the merge intact. - Query cost: both additions ride the existing aliased node —
parents(first: 0)fetches the count and no parent. Verified against the live GraphQL API, including a 100-alias batch (theREFERENCE_STATUS_MAXworst case), which returns in the same time as the pre-change query. No extra round trip. - Defensive defaults: an absent parent count reads as an ordinary commit and an absent timeline reads as an undated request — both directions deliberate and both test-covered.
- Test quality: the new tests were each demonstrated to fail against the pre-fix logic, so none can pass vacuously.
- Hygiene scan: no secrets, debug statements,
any,@ts-ignore,.only/.skip, TODOs, dependency changes, or CI/workflow edits in the diff.
🧪 Validation
Full validation.commands gate, in order, in the PR's own worktree at 0dfa79e7:
| # | Command | Result |
|---|---|---|
| 1 | npm run typecheck |
✅ pass |
| 2 | npm test |
|
| 3 | npm run test:unit |
✅ 35 pass, 0 fail |
| 4 | npm run build |
✅ pass — check:pack ok, 475 files |
| 5 | npm run test:package |
✅ 15 pass, 0 fail |
The step-2 failures are git-worktree, git, git-changes, health-forge, automations-api, automations-gate, projects-api, route-parity — environment/spawn-dependent suites, unrelated to this diff. Every suite this PR touches passes: 224/224.
CI is fully green on 0dfa79e7: Publish npm snapshot ✅, Unit, build, E2E, and package ✅, license/cla ✅. No pending checks, no conflicts (MERGEABLE).
🏁 Verdict
Changes requested — one blocker and one major, per the severity rule (any blocker, or any major without a documented waiver, requests changes).
Both are contained in derivePrReferenceStatus and its query, both have a concrete fix that costs no extra request, and neither touches the PR's design — the two axes it adds are right; the merge-head test is simply reading the wrong commit.
patzick
added
changes-requested
|
🏷️ label rationale
|
Review of #909 found two defects in the merge-head test this branch added. **A merge landing on top erased an answer the author had already given.** `pushed` read only `commits(last: 1)`, so any 2-parent head forced it false regardless of what came before. Sequence: changes requested; author pushes a real fix (chip correctly turns blue); base moves on; author presses "Resolve conflicts" (this cockpit's own button) or "Update branch". The head is now a merge, the fix underneath it is discarded, and the chip flips BACK to red — blaming an author who answered two steps ago. That was a regression: before this branch, the merge was newer than the review and read `review-required`, which was right. The push is now judged by the newest commit that is actual WORK: the head, or, when the head is a merge, its FIRST parent — the branch's previous tip, which is what the merge sat on top of. A plain "Update branch" on a PR that was never fixed still reads as unanswered, because that parent is the pre-review commit. `parents(first: 1)` returns the date beside the `totalCount` already fetched, so it stays one request. **`reviewRequestedAt` could date a request that no longer existed.** `reviewRequests` says who is on the hook NOW and carries no date; a `ReviewRequestedEvent` carries the date and survives the request being withdrawn. Nothing tied the two to the same reviewer, so a request created after the review and then removed could date a standing older one, and the live rejection was hidden behind "waiting for review" again — the bug this branch fixes, one step rarer. The date now comes from the newest event whose reviewer is still on the request list, matched by login (User/Bot) or slug (Team). Four tests added, all four verified to fail against the reviewed behaviour. The enlarged query was re-validated against the live GraphQL API. Spec updated: a merge is transparent to the push rule, not disqualifying.
🔍 Re-review — both findings fixedHead is now ✅ Blocker — merge head erasing an earlier answer — fixed
const workCommittedAt =
(pr.headParentCount ?? 1) < 2 ? pr.headCommittedAt : pr.headFirstParentCommittedAt;
const pushed = isAfter(workCommittedAt, pr.changesRequestedAt);The merge's first parent is the branch's previous tip, so the fix underneath a conflict-resolution merge keeps its answer. Re-checked against all three cases:
✅ Major —
|
patzick
added
merge-queue
|
🤖 Autofix ran (PR authored by the running account, so autofix-eligible without
|
|
🤖 All checks pass on
Run: https://github.com/open-mercato/cezar/actions/runs/32376832429 CI agrees with the local gate, so the review findings and the checks are both clear. The PR remains |
…ibution) One conflict, in CHANGELOG.md: both sides appended to Unreleased. Resolved by keeping both — this branch's `## 🔧 Changed` seed-once entry and its automations fix, plus main's four Fixes entries from #901, #904, #905 and #909, merged into a single `## 🐛 Fixes` list. Everything else auto-merged, including server.ts, which both sides touched. Verified the branch's own changes survived: the coordinator's `pinned` wiring, the root-keyed automation store, and both copy fixes.
…us facet The Tasks table's search box only ever matched title, branch and workflow, so a task known by its tracker number — the way it is named in a PR body or a standup — could only be found by remembering its title. `referenceNeedle` recognizes `open-mercato#909`, a bare `909`, `pr 909` and `issue 42`, and `filterRuns` answers them against `taskReferences`: the same list the row's own chip is built from, so search can never surface a number the table would not show. Alongside it, the pure model the status filter needs: `TaskListFilters`, `filterTaskList` (facet AND search), `statusFacetOptions` with counts computed the way the global page computes them, and the toggle/count helpers the Clear affordance reads. The facet's vocabulary is the status PILL's word (`deriveAttention().label`), not `RunRecord.status`, so it can offer `scheduled` and `monitoring` — sub-states the record spells as failed/running — and can never disagree with the column beside it.
Stamp the release set (contract, api-client, cezar, web, alias-cezar) and the lockfile 0.10.0 -> 0.10.1, with the intra-release ranges rewritten to ^0.10.1. A patch bump: every change since 0.10.0 is a feature or a fix, no breaking surface. Turn `# Unreleased` into `# 0.10.1 (2026-09-04)` and make it a real release entry: - keep the five essay-length entries already drafted (pin tasks, agent-account Continue, the CEZ:ASK brace repair, the merge-conflict chip, and the three reference-chip fixes), fixing their PR citations along the way (the pin feature cited its tracking issue, #935, instead of the PR that shipped it, #938; the CEZ:ASK and merge-conflict entries had no citation at all) - add the ~14 PRs that merged since with no changelog note at all: PDF/TXT/MD composer attachments (#951), cross-repo reference-chip safety (#946), a stale review request or an "Update branch" merge clearing a rejection (#909), cross-project task 404s (#905), the Changes tab's independent scroll (#918), a clearable composer skill pill (#919), a resumed session keeping its step's tools (#928), Alt quick replies eating Polish characters (#943), host-local Claude model discovery (#841), GitHub search across every state (#732), two mobile run-header/history fixes (#764, #873), and a fresh task's /skill expansion (#947) - add a CI/CD & Infrastructure section for the GITHUB_RUN_ATTEMPT e2e fix (#911) - add a Highlights paragraph and credit the seven contributors No code changes.
1 participant
A rejected PR could show Waiting for review (blue, the reviewer's move) instead of Changes requested (red, the author's move), with nobody having addressed anything. Reported from the cockpit with the chip live on screen.
Two independent causes, one commit each.
1. A review request older than the review is not an answer to it
derivePrReferenceStatusread any pendingreviewRequests.totalCount > 0as the author clicking re-request. But that field says only that someone is on the hook, never since when.Live data for the reported PR:
reviewDecisionCHANGES_REQUESTED2026-08-19T11:00:27Z2026-08-18T10:23:40Z— older2026-08-18T10:28:43Z— also olderThree reviewers were asked at once, one rejected, the other two never looked — so the count sits at 1 with nothing having happened since. Read as a re-request, that stale entry painted "Waiting for review" over a live rejection.
A standing request now has to postdate the review. Its date comes from
timelineItems(last: 1, itemTypes: [REVIEW_REQUESTED_EVENT]).2. "Update branch" is not the author answering a review
The other signal — a commit newer than the review — was cleared by GitHub's Update branch button, which writes
Merge branch 'main' into <branch>dated now: newer than any review, addressing none of it. The click people make reflexively on a stale PR wiped the rejection off the chip.A head commit with
parents.totalCount >= 2no longer counts as a push. Shape confirmed live on a real PR whose head is exactly such a merge.Deliberately unchanged
reviewDecisionstayingCHANGES_REQUESTEDforever doesn't pin the blame on the author.reviewswith a pending request still counts — the earlier feat(workspace): register the boot folder only while the registry is empty #774 finding, the case with no other tell.Known gap
Update with rebase still clears a rejection: it rewrites the committer date on every commit and is indistinguishable from real work. The only fix is dropping the pushed-since rule entirely, which would leave a sticky red chip for authors who push a fix without clicking re-request. That trade was offered and not taken; the spec records why.
Cost
No extra request. Both new fields ride the same aliased node as the rest —
parents(first: 0)fetches the count and no parent.Verification
packages/cezar/src/server/forge/github.test.ts. Each fix's new tests were checked to fail against the pre-fix logic (4 and 3 respectively), so they can't pass vacuously.npm run typecheck,npm test,npm run test:unitall green apart from 8 failures confirmed pre-existing on a clean tree (health-forge,git-worktree,git,git-changes,automations-*,projects-api,route-parity— all environment/flaky, none inforge/)..ai/specs/2026-08-11-reference-status-chips.mdupdated: it is the design record this code cites.