fix(source): resolve PR merge state on first load, not on refresh

[kyleseaman]

Problem

In the chat SidePanel's Changes view, a pull request with merge conflicts showed no merge-blocker banner at all when the panel first loaded. The conflict only appeared after the user clicked the panel's refresh button.

Why it matters

The banner is the panel's only signal that a branch cannot be merged, and it carries the "Add to chat" handoff that asks the agent to resolve the conflict. Silently showing nothing reads as "nothing blocks this merge" — the user learns the branch is conflicting somewhere else (or not at all) and the handoff is never offered. Anyone babysitting a PR from the panel was getting a clean-looking header on a branch that could not merge.

Fix (symptom → root cause → change)

Symptom: conflicts appear only after a manual refresh.

Root cause: both providers compute mergeability lazily. The first read of a pull request they have not evaluated recently answers "not known yet" — GitHub UNKNOWN, GitLab checking/unchecked — and that read is itself what starts the computation. A single read therefore normalizes to mergeable: 'unknown', which pullRequestMergeBlocker correctly treats as "no known blocker", so no banner renders. The user's refresh click was simply the second read, which got the real answer.

Reproduced directly against this repository:

$ gh pr view 443 --json mergeable,mergeStateStatus   # first read
UNKNOWN UNKNOWN
$ gh pr view 443 --json mergeable,mergeStateStatus   # one second later
CONFLICTING DIRTY

Change, in two parts, because there were two distinct ways a conflict stayed hidden:

1. First load (_github_settled_merge_state / _gitlab_settled_merge_state) — each full fetch now re-reads only the merge fields until they settle: at most _MERGE_STATE_REREADS (2) attempts spaced _MERGE_STATE_REREAD_DELAY_SECS (0.8s) apart. The re-read coroutine is dispatched inside the existing secondary fanout, so its wait overlaps the files/discussions/pipelines calls the request was already making rather than adding to them. A value that is empty rather than unknown is never re-read — the provider omitted the field, so re-reading cannot settle it. An unsettled, failed, or malformed re-read degrades to unknown rather than raising: an unknown merge state costs one banner, never the panel.

2. Conflicts that begin after the panel opened — the panel's payload is pinned (staleTime: Infinity) and only refetches on a manual refresh, so a branch that started conflicting while the panel was open would never be noticed. This rides the chip↔payload coherence protocol fix(chat): keep pull-request state in sync across sidebar and detail panel #443 already built rather than adding a parallel one: the merge pair is recorded in the short-TTL chip entry, so it participates in the existing change detection — a chip refresh that sees the pair move drops the full payload for that URL and pushes a source_status delta, and every owner window invalidates its detail query and re-reads. The banner therefore always comes from an authoritative provider read, and it converges across windows.

   Three lines of integration make that work: the pair is recorded in the chip entry (each field independently, only once real, never as unknown); status_from_full_payload projects it too; and parseStatusDelta/applyStatusDelta accept it so a merge-only change is not discarded as a field-less delta. PullRequestPanel.tsx is unchanged — there is no frontend component change in this PR.

   The projection in status_from_full_payload is load-bearing, not tidiness: without it every full fetch would rewrite the chip entry without the fields the chip read records, so the next chip refresh would judge that a change, drop the full payload, and the write-through would strip them again — the repeating chip↔full transition fix(chat): keep pull-request state in sync across sidebar and detail panel #443's flap damper exists to contain, spun by a projection gap.

Tests

Backend (test/test_source_providers.py, 17 new):

• ..._rereads_merge_state_until_the_provider_settles_it (GitHub + GitLab) — first read UNKNOWN/checking, re-read settles to conflicting; also asserts the GitHub re-read requests the merge fields alone, not another full fanout.

• ..._does_not_reread_settled_merge_state — a settled first read issues zero re-reads.

• ..._degrades_to_unknown_when_reread_cannot_settle (GitHub parametrized over unsettled / provider-error / invalid-payload, plus GitLab) — payload keeps unknown, the rest of the payload survives, and the re-read budget is respected (failure stops immediately; unsettled uses the full bound).

• ..._skips_reread_when_provider_omits_merge_fields — an omitted field is not "still computing".

• _merge_state_settled parametrized over all seven pair shapes, plus ..._treats_a_detail_only_answer_as_settled — locks pair-based settledness so GitLab's need_rebase is not re-read and discarded.

• ..._check_status_carries_settled_merge_state (GitHub + GitLab), ..._carries_a_detail_only_answer, and ..._omits_unsettled_merge_state (parametrized over UNKNOWN / absent) — locks the chip-cache contract: each field recorded independently, only once real, never as unknown.

• status_from_full_payload projects the pair, and both projections agree on a GitLab detail-only answer — the two tests that pin the flap-loop gap described above.

Frontend (website/src/test/pullRequestStatusDelta.test.ts, 5 new):

• a merge-only delta survives parseStatusDelta and lands via applyStatusDelta (without this it is discarded as field-less and the banner waits for a refresh);
• a GitLab detail-only answer (mergeStateStatus with no settled mergeable) survives;
• malformed/oversized merge values are rejected, matching the existing fail-closed narrowing for state/ci;
• a merge-only move produces a new object (re-render) while an unchanged entry keeps identity (no re-render).

All 22 were revert-probed: with each fix semantically reverted, the corresponding tests fail rather than passing vacuously.

Manual verification

Root cause confirmed against live GitHub, as shown above — the lazy-computation behavior is what the fix targets, and it is not reproducible from unit fixtures alone. The rendered behavior is covered by the frontend test asserting the banner appears from a poll with no refetch, so no additional manual pass was needed.

Screenshots

N/A — no new or changed UI surface. This makes an existing component (the merge-blocker banner, added in an earlier PR) appear when it already should have; its markup, copy, and styling are untouched.

Local gates

17969 pytest · 4682 vitest · isort / flake8 / mypy (481 files) / tsc -b / eslint / scrub-lint all clean. Three pre-existing test/test_dashboard_origin.py::TestParseDashboardUrlMalformed failures reproduce on a clean main checkout at 54fac64a and are unrelated to this change.

Review history

Two independent read-only pre-submit reviewers cleared the diff with no Critical/High findings; their two Low items (spec to_dict() clause and the TypeScript chip types, both about owner-gated sidebar chips spreading the whole cached entry) were fixed before the first push.

Round 1 of GPT 5.6 raised two legitimate findings, both since resolved — see the disposition comments for the full reasoning:

• BLOCKING: the first attempt compared dataUpdatedAt on two queries, i.e. response times rather than data freshness, so a chip entry served before its background refresh could overwrite a fresher payload. Round 2 replaced the copy with a forced re-read on disagreement; the rebase onto fix(chat): keep pull-request state in sync across sidebar and detail panel #443 then removed the client-side comparison entirely, since fix(chat): keep pull-request state in sync across sidebar and detail panel #443's server-driven chip→payload invalidation already does this correctly and across windows. PullRequestPanel.tsx ends up unchanged.
• FINDING: GitLab's need_rebase/branch-protection states settle in the detail field with mergeable == "unknown", so gating on mergeable alone dropped them from the chip cache and left exactly those banners invisible. Resolved by judging settledness on the pair and recording the two fields independently.

Arbiter passed on round 1 and classed both as deferrable follow-ups; they were fixed rather than deferred since both were in newly added code.

The rebase onto #443 also surfaced a latent flap-loop hazard (a chip↔full projection gap) that is fixed and pinned by tests here — see the Fix section.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 79c2de128e506da7cd70d950c6c1adeeb0069cce and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[CODEX-REVIEWED] 79c2de1

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 79c2de128e506da7cd70d950c6c1adeeb0069cce: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 79c2de128e506da7cd70d950c6c1adeeb0069cce — updated in place on each push; does not block merge.

Design-Verdict: PASS

Sound root-cause fix: bounds a merge-field re-read to defeat lazy mergeability, and reuses #443's chip↔payload coherence protocol rather than bolting on a parallel path.

Suggestions

• The _keep_known_merge_state carry-forward is a fourth mechanism (~40 lines) not enumerated among the PR body's "two parts / three lines"; a one-line note in the description would keep the fix summary honest for future readers.

[DESIGN-REVIEWED] 79c2de1

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 79c2de128e506da7cd70d950c6c1adeeb0069cce.

Second-order review for 79c2de128e506da7cd70d950c6c1adeeb0069cce; this comment is updated in place on each push.

Review details

Both line-level reviewers (Claude and GPT 5.6) reported no findings at all, and the design reviewer passed with a single suggestion — a note that the _keep_known_merge_state carry-forward mechanism isn't enumerated in the PR body's description. That is a documentation-honesty nit about the PR description text, not a code, contract, or data decision: it locks nothing in and can trigger no harm. Nothing here approaches the one-way-door or concrete-harm bar.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

• Design reviewer's suggestion: add a one-line note to the PR description (or the commit body) mentioning the _keep_known_merge_state carry-forward as a fourth mechanism beyond the "two parts / three lines" summary — purely editorial, safe to do any time, and lives in the PR/commit metadata rather than the code.

[ARBITER-REVIEWED] 79c2de1

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter 79c2de128e506da7cd70d950c6c1adeeb0069cce: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed 79c2de128e506da7cd70d950c6c1adeeb0069cce — this comment is updated in place on each push.

No findings.

Verdict recorded via the action's structured output for commit 79c2de128e506da7cd70d950c6c1adeeb0069cce.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 79c2de128e506da7cd70d950c6c1adeeb0069cce: <one-sentence reason>

[kyleseaman]

Disposition for round 1 (reviewed SHA 6502cce5aeb13f17880c9acbe47e782e4961e8ca)

Both findings were legitimate and both are fixed. No rebuttals.

BLOCKING — response time mistaken for provider-data freshness (PullRequestPanel.tsx, poll-vs-payload gate) — fixed.
Confirmed reachable: /api/source/pull-request/status serves the chip cache and only schedules the background refresh, so a poll that returns after a full fetch can still describe an older provider read. Comparing dataUpdatedAt on the two queries compares response times, not data freshness, so an expired-then-served chip entry could fold a staler merge answer over the pinned payload and mis-render the banner for a cycle.

Rather than make the freshness comparison smarter, the stale-copy hazard is removed: the poll's answer is never written into the payload. A disagreement (mergeStateDiverges, each field compared independently, an omitted field treated as no news) now triggers a forced re-read of the pull request, bypassing the server's short full-payload cache so a cached answer cannot repeat the state being questioned. The banner is therefore always sourced from a real provider read, in either direction of staleness. withStatusMergeState and the timestamp gate are gone.

Loop safety: one disagreeing answer causes at most one re-read, tracked per URL by the poll's merge-pair signature, so a chip entry that persistently contradicts the payload (exactly the stale case above) cannot drive a refetch loop. Covered by a new test that polls three more times and asserts the re-read count stays at one and the authoritative conflict is never cleared.

FINDING — GitLab need_rebase/blocked excluded from the chip cache — fixed.
Confirmed: those normalize to mergeable == "unknown" with the answer in mergeStateStatus, so gating on mergeable alone dropped them and left exactly those banners invisible to the poll. Two changes: _merge_state_settled now judges the pair (either field being real settles it, so a detail-only answer is no longer re-read and then discarded), and _record_merge_state records the two fields independently, each only once real and never as unknown.

Tests added (all revert-probed — reverting each fix fails them): _fetch_gitlab treats a detail-only answer as settled with one read; _merge_state_settled parametrized over the seven pair shapes; GitLab chip status carries a detail-only answer; frontend re-read-on-divergence including the force=true argument; frontend no-loop/no-overwrite.

Spec (docs/system-specs/modules/learn-cron-dashboard.md) updated for all three contract changes: pair-based settledness, the chip pair as a hint rather than an answer, and the divergence-triggered re-read.

Gates on the new head: 17915 pytest, 4654 vitest, isort/flake8/mypy/tsc/eslint/scrub-lint clean. The three test_dashboard_origin.py::TestParseDashboardUrlMalformed failures reproduce on a clean main and are unrelated.

[kyleseaman]

Disposition for round 2 (reviewed SHA 998247a2)

Round 2 passed GPT (run 30275878770; the CANCELLED entry on the same head is a superseded duplicate run). This push is a rebase onto current main, which landed #443 — and #443 changes the right shape of the round-1 BLOCKING fix, so the fix was rewired rather than carried forward. No open findings.

Round-1 BLOCKING (response time mistaken for provider-data freshness) — now fixed structurally, with the client-side comparison deleted entirely.

#443 added a server-side chip↔payload coherence protocol that already does what the finding asked for, better than any client-side comparison can: a full fetch write-throughs its projection into the chip cache (record_full_payload_status), a chip refresh that observes a changed status drops the full payload for that URL and emits a source_status delta, and every owner window invalidates ['pull-request-source', url] on it. The banner therefore always renders from an authoritative provider read, and it converges across windows rather than only in the one holding the panel.

So the round-2 client-side machinery is gone — mergeStateDiverges, the divergence effect, and the forced re-read. website/src/components/PullRequestPanel.tsx is now byte-identical to main. What replaces it is three lines of integration:

• the merge pair is recorded in the chip entry, so it participates in the existing change detection that drives the invalidation and the delta;
• status_from_full_payload projects the pair too;
• parseStatusDelta / applyStatusDelta accept it, so a merge-only change is not discarded as a field-less delta.

Latent defect the rebase surfaced, fixed here. Had the merge pair been added to the chip read without also being projected by status_from_full_payload, every full fetch would have rewritten the chip entry without the fields the chip read records → the next chip refresh judges that a change → drops the full payload → write-through strips them again. That is precisely the repeating chip↔full transition #443's _note_check_flap damper exists to contain, spun by nothing but a projection gap: a provider-polling loop damped only into a permanently stale glyph. Two revert-probed tests now pin it (..._projects_the_merge_pair, ..._agree_on_the_merge_pair).

Round-1 FINDING (GitLab need_rebase/blocked excluded) — still fixed, unchanged by the rebase: _merge_state_settled judges the pair (either field being real settles it), and _record_merge_state records the two fields independently, each only once real and never as unknown. This is also what makes the pair safe as a chip value under #443's change detection — an unknown written into the chip would read as a change away from a real answer on every refresh.

Rebase conflicts were in source_providers.py (3 hunks) and the spec (1). All resolved in main's favour, re-adding only this PR's lines: main's _project_state lifecycle projection and _gitlab_aggregate_ci CI aggregate are kept verbatim, with _record_merge_state appended to each chip branch and the merge-state tuple fallback restored in the GitLab payload builder.

Tests: 17 backend (12 merge-state + 2 projection-agreement + 3 settledness shapes) and 5 frontend delta tests, every one revert-probed. Gates on this head: 17969 pytest, 4682 vitest, isort/flake8/mypy/tsc/eslint/scrub-lint clean. The three test_dashboard_origin.py::TestParseDashboardUrlMalformed failures reproduce on clean main.

Net effect of the rebase: the diff shrank — no frontend component change at all, and the fix now rides an existing, already-reviewed invalidation protocol instead of a parallel client-side one.

[kyleseaman]

Disposition for round 3 (reviewed SHA c6bbc35f)

All four bots passed on c6bbc35f. Two sub-threshold items remained; they resolve in opposite directions.

Design Review — "phantom spec text" — fixed

Legitimate, and a defect in my own change. The round-3 rebase onto main (which landed #443) replaced my client-side merge-state comparison with #443's server-driven chip↔payload invalidation, and I deleted the code but left the spec describing it. mergeStateDiverges appears nowhere in the codebase — verified repo-wide. AGENTS.md requires the spec to track the change, so this is a real documentation-accuracy defect even though it ships no behavior.

The spec sentence now describes the mechanism that actually shipped: the merge pair rides both the chip-status projection and the source_status delta, so a chip refresh observing a changed merge pair drops the full payload server-side and notifies owner dashboards; each field is recorded only once real (an unanswered field is omitted, never written as unknown, so "still computing" cannot erase a real answer); the two fields are recorded independently because GitLab settles need_rebase and its branch-protection gates in the detail field while mergeable stays unknown; and status_from_full_payload must project the pair too, since a field the chip path records but the full-payload path omits would re-appear as a changed transition every refresh and spin the invalidation loop that _CHECK_FLAP_DAMP_THRESHOLD bounds. Each of those claims was checked against the code before writing it.

GPT 5.6 FINDING — source_providers.py:920, GitLab unchecked re-read without with_merge_status_recheck — rebutted, no change

The finding does not apply to the endpoint this code uses, and the suggested fix would be a no-op.

with_merge_status_recheck is documented as a supported attribute of the list endpoints only (list merge requests / list project merge requests / list group merge requests), with the matching note: "Listing merge requests might not proactively update merge_status (which also affects has_conflicts), as this can be an expensive operation. If you need the value of these fields from this endpoint, set the with_merge_status_recheck parameter to true in the query."

The re-read at :920 does not use a list endpoint. mr_api is projects/{project}/merge_requests/{iid} — the single merge request endpoint, whose supported-attributes table is id, merge_request_iid, include_diverged_commits_count, include_rebase_in_progress, render_html; with_merge_status_recheck is not among them. That endpoint's own response notes state the opposite behavior: "The mergeability (merge_status) of each merge request is checked asynchronously when a request is made to this endpoint. Poll this API endpoint to get the updated status."

Requesting the single-MR endpoint therefore already triggers the asynchronous recomputation, and polling it is the documented way to observe the settled value — which is exactly what _gitlab_settled_merge_state does. Appending the parameter would pass an unsupported query attribute to an endpoint that ignores it, while changing nothing about whether the status settles. The GitLab path is not left blind; it settles by the mechanism GitLab documents for it.

Source: https://docs.gitlab.com/api/merge_requests/ ("Merge requests list response notes" vs "Single merge request response notes").

Verification

Docs-only change on top of the reviewed tree. scripts/scrub-lint.sh --no-history clean, inclusive-language clean. The functional diff is byte-identical to c6bbc35f, which was 39/40 green with all four bots passing (the one red, Publish readiness signal, was three concurrency-superseded cancelled runs; the authoritative PR Readiness commit status was success). Windows shard 3 on that head was a re-run-cleared flake in test_mcp_gateway_bluegreen.py / test_knowledge_sync_local_file.py — sleep-based timing tests in modules this diff does not touch.

[kyleseaman]

Disposition for round 4 (reviewed SHA 94ec5683)

BLOCKING — source_providers.py:2687, unsettled polls erase settled merge state — accepted and fixed. No rebuttal; the finding is correct and the reasoning behind it is worth recording, because it exposes a premise I carried across a design change where it had stopped holding.

_record_merge_state deliberately OMITS a field the provider has not settled, and the round-1/2 design justified that with "an absent field means no news, not nothing blocks the merge." That was sound while the consumer was my client-side comparison, which explicitly treated an omitted field as no-news. The round-3 rebase onto #443 deleted that comparison and made the chip entry the thing the invalidation protocol diffs — and every writer replaces the entry wholesale. Absence stopped being neutral and became destructive. I kept the original justification in the docstring without re-deriving it against the new consumer.

Confirmed reachable, and not a rare path. Both providers evaluate mergeability lazily, so a poll arriving after the provider's evaluation lapsed returns unknown for a source whose conflict is already known — the same lazy behavior this PR exists to fix. The consequences compound past the erased field:

1. The pair vanishes from the owner-gated sidebar payload, which spreads the chip entry whole.
2. The stripped entry reads as a CHANGED chip status, so the protocol drops the full payload and emits a source_status delta; the resulting refetch re-projects the real answer straight back into the cache, and the next lapsed poll strips it again. That is exactly the repeating chip↔full transition _CHECK_FLAP_DAMP_THRESHOLD exists to contain — so the banner would have survived only until the damper tripped, then gone stale, which is the original bug restored by a different route.

Fix. Both writers now carry a settled merge field forward when the fresh read has no answer, via a shared _keep_known_merge_state — the same keep-known rule already applied to the ci glyph on partial payloads, so this follows existing precedent rather than inventing a mechanism. Applied at both cache-write sites: record_full_payload_status (a first full fetch commonly returns unknown, so the write-through had the identical defect the finding describes for the chip path) and the chip refresh.

Two bounds keep the carry from becoming a stale-verdict pin:

• A real answer always wins, including one that CHANGES the value. The carry only ever fills a gap.
• Carry-forward stops once the source leaves an open state. A merged/closed source is never asked about mergeability again, so a value carried there could never be cleared.

Tests — 4 added, all revert-probed in BOTH directions, since two of them guard against over-carrying rather than under-carrying:

• test_record_full_payload_keeps_settled_merge_state_when_the_read_is_unsettled — settled conflict + unsettled read keeps the pair AND emits no delta (proving the invalidation loop never starts). Fails when the carry is neutered.
• test_chip_refresh_keeps_settled_merge_state_and_starts_no_invalidation_loop — same property on the chip path. Fails when the carry is neutered.
• test_record_full_payload_lets_a_real_merge_answer_replace_a_settled_one — fails when the carry is made unconditional.
• test_record_full_payload_stops_carrying_merge_state_once_the_source_closes — fails when the lifecycle gate is removed.

I verified the two directions independently: neutering the carry fails the first two and passes the last two; making it unconditional fails the last two and passes the first two. No test passes vacuously under either defect.

Spec. The learn-cron-dashboard.md sentence previously asserted that omission alone made "still computing" harmless — which was the incorrect premise. It now states that omission is insufficient because writers replace the entry wholesale, documents the keep-known carry on both writers, and records both bounds.

Verification. 17,959 pytest pass; isort, flake8, scrub-lint, inclusive-language clean. No frontend files changed in this round. The one local mypy error is in vector_memory.py, which is byte-identical to origin/main (git diff --name-only origin/main returns empty) and whose type check passed in CI on the prior head — a local faiss-stub version difference, not a change from this PR.