fix(session): keep terminated status when an open PR remains#2392
Open
abhay-codes07 wants to merge 1 commit into
Open
fix(session): keep terminated status when an open PR remains#2392abhay-codes07 wants to merge 1 commit into
abhay-codes07 wants to merge 1 commit into
Conversation
deriveStatus's terminated branch short-circuited to `merged` whenever any
owned PR was merged, even if the session still owned an open PR. That
contradicts the function's documented invariant ("Merged/closed PRs only
matter once no open PR remains") and is inconsistent with the
non-terminated branch, which gates on openPRs before falling back to
anyMerged.
The mismatch is a regression from the single-PR -> multi-PR change: the
old branch tested `pr != nil && pr.Merged`, which in the single-PR world
implied no open PR remained. Generalizing to `anyMerged(prs)` dropped that
implicit condition. A session killed while it still owns an open PR (for
example, only the bottom of a stack merged, or one of two independent PRs
merged) was reported as cleanly "merged", hiding the abandoned open PR.
Gate the merged short-circuit on there being no open PR, mirroring the
non-terminated branch. Existing behavior is preserved (a terminated
session whose only PR is merged still reports merged); only the
merged+open combination changes to terminated. Add a table case covering it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2390.
What
deriveStatus's terminated branch returnedmergedwhenever any owned PR was merged, even when the session still owned an open PR. That contradicts the function's documented invariant — "Merged/closed PRs only matter once no open PR remains" — and is inconsistent with the non-terminated branch, which gates onopenPRsbefore falling back toanyMerged.Why
Regression from the single-PR → multi-PR change (#230). The branch used to be
if pr != nil && pr.Merged, which in the single-PR world implied "no open PR remains." Generalizing toanyMerged(prs)dropped that implicit condition. A session killed while it still owns an open PR (e.g. only the bottom of a stack merged, or one of two independent PRs merged) was shown as cleanly "merged", hiding the abandoned open PR.Change
Gate the merged short-circuit on there being no open PR, mirroring the non-terminated branch:
Existing behavior is preserved — a terminated session whose only PR is merged still reports
merged(the existingmerged-prcase). Only themerged + opencombination changes toterminated.Tests
terminated-with-open-pr-stays-terminatedto thederiveStatustable test. It fails before the change (got "merged" want "terminated") and passes after.go test ./internal/service/session/...green; fullgo test ./...green apart from the pre-existing Windows-onlyTestApplySymlinks, which needs the symlink privilege and fails onmaintoo (unrelated to this change).go vetand gofmt clean. I couldn't rungo test -racelocally (no 64-bit CGO toolchain on my box) — leaving that to CI.cc @harshitsinghbhandari — small correctness fix, happy to adjust if you'd rather gate it another way.