fix(server): don't drop sticky PR fallback when remote URL can't be resolved#4289
Conversation
…esolved The sticky-PR fallback (introduced in #4281 to avoid blanking the badge on a transient gh failure) compared the resolved head remote URL strictly: if either the cached or current key was null, it treated that as a mismatch and dropped the fallback. But readConfigValueNullable maps ANY failed git-config read to null the same way it maps a genuinely absent remote — so a one-off config-read hiccup while resolving the *current* head context looked identical to "the remote actually changed," and cleared an otherwise-valid PR badge. Only invalidate the fallback when both the cached and current remote-URL keys are known and disagree; treat a null on either side as inconclusive and fall through to the next signal (remoteName, gated the same way). This preserves the fix for a genuinely retargeted branch while no longer punishing a transient read failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ApprovabilityVerdict: Approved Straightforward bug fix that makes sticky PR logic more resilient to transient git-config read failures. The change is defensive (retain existing state when state comparison is ambiguous), well-tested, and authored by the recent maintainer of this code. You can customize Macroscope's approvability policy. Learn more. |
…esolved (pingdotgg#4289) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
nullon either side as a hard mismatch, dropping an already-known PR badge whenever the current lookup transiently failed to readremote.<name>.url— indistinguishable from a genuine "no remote configured" state.resolveLastKnownPrnow only invalidates the fallback when both the cached and current remote-URL keys (and, one level down,remoteName) are known and disagree; a null on either side is treated as inconclusive rather than as proof the remote changed.remote.origin.urlbetween two status calls (reproducing the ambiguous-null case via real git config, without touching branch tracking) and asserts the sticky PR survives.Test plan
pnpm typecheck(apps/server) — cleanpnpm vp linton changed files — cleanapps/server/src/git/GitManager.test.tsfull suite — 64/64 passed, including the new regression test and the existing sticky-PR/retarget/repoint testsNote
Low Risk
Narrow change to PR badge fallback validation in git status; regression test added; genuine remote repoint/retarget behavior still covered by existing tests.
Overview
Fixes sticky PR fallback logic in
resolveLastKnownPrso a transient or ambiguous missingremote.<name>.urlno longer clears an already-known PR badge.Remote URL comparison now only treats a mismatch as real when both the cached and current normalized remote URL keys are non-null and differ. A
nullfromreadConfigValueNullable(real “no remote” vs config read failure) is inconclusive, not proof the remote changed.The upstream /
remoteNamefallback uses the same rule: both sides must be known before disagreement invalidates the cache; otherwise the last known PR is kept (including first-push null→non-null transitions).Adds a regression test that unsets
remote.origin.urlbetween status calls (withghfailing on the second lookup) and expects PR #217 to remain.Reviewed by Cursor Bugbot for commit 3572fed. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Preserve cached PR when remote URL or upstream identity can't be resolved
When reconciling a last-known PR with the current repo state in GitManager.ts, null checks are now required on both sides of each comparison before treating a mismatch as invalidating the cached PR. Previously, a transient failure to read
remote.origin.urlor the upstream/remote identity would produce a null value that caused the cached PR to be dropped silently. A new integration test in GitManager.test.ts covers this scenario by unsetting the remote URL mid-session and asserting the last-known PR is preserved.Macroscope summarized 3572fed.