mirror of
https://github.com/stablyai/orca.git
synced 2026-10-16 16:03:26 +00:00
Carry the worktree head on PR-refresh candidates so fallback preservation is head-gated
Live validation caught a self-sustaining loop the unit seams missed: the Checks panel passes its displayed PR number as the refresh fallback, and the candidate path carried no worktree head, so main preserved a merged branch PR head-blind — the merged view survived new local commits instead of yielding to 'No pull request found'. Thread currentHeadOid through GitHubPRRefreshCandidate, both renderer candidate builders, the gh:prForBranch IPC, and the runtime RPC method — the same shape the hosted-review path already used. Main's explicit-head gate (equality or confirmed commit membership) now applies to every renderer-driven lookup; true no-context callers keep the designed deleted-head preservation. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -94,6 +94,28 @@ describe('pr-refresh-coordinator', () => {
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('forwards the candidate worktree head into the branch lookup options', async () => {
|
||||
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
|
||||
getPRForBranchOutcomeMock.mockResolvedValueOnce({
|
||||
kind: 'no-pr',
|
||||
fetchedAt: Date.now()
|
||||
})
|
||||
|
||||
reportVisiblePRRefreshCandidates([makeCandidate({ currentHeadOid: 'worktree-head-oid' })], 1, 1)
|
||||
await vi.runOnlyPendingTimersAsync()
|
||||
|
||||
// Why: without the head, a panel-supplied fallback number preserves a
|
||||
// merged PR head-blind after the branch moves on to new work.
|
||||
expect(getPRForBranchOutcomeMock).toHaveBeenCalledWith(
|
||||
'/repo',
|
||||
'feature/test',
|
||||
null,
|
||||
null,
|
||||
null,
|
||||
expect.objectContaining({ currentHeadOid: 'worktree-head-oid' })
|
||||
)
|
||||
})
|
||||
|
||||
it('does not show visible background refreshes as queued', async () => {
|
||||
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
|
||||
getPRForBranchOutcomeMock.mockResolvedValueOnce({
|
||||
|
||||
@@ -34,7 +34,7 @@ type PRRefreshOutcomeObserver = (
|
||||
|
||||
type PRBranchLookupCandidate = Pick<
|
||||
GitHubPRRefreshCandidate,
|
||||
'localGitOptions' | 'linkedPRNumber' | 'fallbackPRNumber' | 'fallbackPRSource'
|
||||
'localGitOptions' | 'linkedPRNumber' | 'fallbackPRNumber' | 'fallbackPRSource' | 'currentHeadOid'
|
||||
>
|
||||
|
||||
function shouldAcceptMergedFallbackPR(candidate: PRBranchLookupCandidate): boolean {
|
||||
@@ -55,6 +55,9 @@ function hostedReviewOptionArgs(
|
||||
if (shouldAcceptMergedFallbackPR(candidate)) {
|
||||
options.acceptMergedFallbackPR = true
|
||||
}
|
||||
if (typeof candidate.currentHeadOid === 'string' && candidate.currentHeadOid.trim().length > 0) {
|
||||
options.currentHeadOid = candidate.currentHeadOid
|
||||
}
|
||||
return Object.keys(options).length > 0 ? [options] : []
|
||||
}
|
||||
|
||||
|
||||
@@ -278,20 +278,28 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi
|
||||
linkedPRNumber?: number | null
|
||||
fallbackPRNumber?: number | null
|
||||
acceptMergedFallbackPR?: boolean
|
||||
currentHeadOid?: string | null
|
||||
}
|
||||
) => {
|
||||
const repo = assertRegisteredRepo(args, store)
|
||||
const localGitOptions = localGitOptionArgs(store, repo)[0]
|
||||
const hostedReviewOptionArgs: [] | [{ localGitExecOptions: { wslDistro?: string } }] =
|
||||
localGitOptions ? [{ localGitExecOptions: localGitOptions }] : []
|
||||
const currentHeadOid =
|
||||
typeof args.currentHeadOid === 'string' && args.currentHeadOid.trim().length > 0
|
||||
? args.currentHeadOid.trim()
|
||||
: null
|
||||
const lookupOptions: GitHubPRBranchLookupOptions | undefined = hostedReviewOptionArgs[0]
|
||||
? { ...hostedReviewOptionArgs[0] }
|
||||
: args.acceptMergedFallbackPR === true
|
||||
: args.acceptMergedFallbackPR === true || currentHeadOid !== null
|
||||
? {}
|
||||
: undefined
|
||||
if (lookupOptions && args.acceptMergedFallbackPR === true) {
|
||||
lookupOptions.acceptMergedFallbackPR = true
|
||||
}
|
||||
if (lookupOptions && currentHeadOid !== null) {
|
||||
lookupOptions.currentHeadOid = currentHeadOid
|
||||
}
|
||||
const lookupOptionArgs: [] | [GitHubPRBranchLookupOptions] = lookupOptions
|
||||
? [lookupOptions]
|
||||
: []
|
||||
|
||||
@@ -10799,7 +10799,8 @@ export class OrcaRuntimeService {
|
||||
branch: string,
|
||||
linkedPRNumber?: number | null,
|
||||
fallbackPRNumber?: number | null,
|
||||
acceptMergedFallbackPR?: boolean
|
||||
acceptMergedFallbackPR?: boolean,
|
||||
currentHeadOid?: string | null
|
||||
): Promise<Awaited<ReturnType<typeof getPRForBranch>>> {
|
||||
const repo = await this.resolveRepoSelector(repoSelector)
|
||||
const options: GitHubPRBranchLookupOptions = this.getHostedReviewExecutionOptions(repo) ?? {}
|
||||
@@ -10807,6 +10808,9 @@ export class OrcaRuntimeService {
|
||||
if (acceptMergedFallbackPR === true) {
|
||||
lookupOptions.acceptMergedFallbackPR = true
|
||||
}
|
||||
if (typeof currentHeadOid === 'string' && currentHeadOid.trim().length > 0) {
|
||||
lookupOptions.currentHeadOid = currentHeadOid.trim()
|
||||
}
|
||||
const lookupOptionArgs: [] | [GitHubPRBranchLookupOptions] =
|
||||
Object.keys(lookupOptions).length > 0 ? [lookupOptions] : []
|
||||
return getPRForBranch(
|
||||
|
||||
@@ -53,7 +53,8 @@ const PrForBranch = RepoSelector.extend({
|
||||
branch: requiredString('Missing branch'),
|
||||
linkedPRNumber: z.number().int().positive().nullable().optional(),
|
||||
fallbackPRNumber: z.number().int().positive().nullable().optional(),
|
||||
acceptMergedFallbackPR: z.boolean().optional()
|
||||
acceptMergedFallbackPR: z.boolean().optional(),
|
||||
currentHeadOid: z.string().nullable().optional()
|
||||
})
|
||||
|
||||
const Issue = RepoSelector.extend({
|
||||
@@ -361,7 +362,8 @@ export const GITHUB_METHODS: RpcMethod[] = [
|
||||
params.branch,
|
||||
params.linkedPRNumber,
|
||||
params.fallbackPRNumber,
|
||||
params.acceptMergedFallbackPR
|
||||
params.acceptMergedFallbackPR,
|
||||
params.currentHeadOid
|
||||
)
|
||||
}),
|
||||
defineMethod({
|
||||
|
||||
@@ -1257,6 +1257,7 @@ export type PreloadApi = {
|
||||
linkedPRNumber?: number | null
|
||||
fallbackPRNumber?: number | null
|
||||
acceptMergedFallbackPR?: boolean
|
||||
currentHeadOid?: string | null
|
||||
}) => Promise<PRInfo | null>
|
||||
refreshPRNow: (args: { candidate: GitHubPRRefreshCandidate }) => Promise<PRRefreshOutcome>
|
||||
enqueuePRRefresh: (args: {
|
||||
|
||||
@@ -1026,6 +1026,7 @@ const api = {
|
||||
linkedPRNumber?: number | null
|
||||
fallbackPRNumber?: number | null
|
||||
acceptMergedFallbackPR?: boolean
|
||||
currentHeadOid?: string | null
|
||||
}): Promise<unknown> => ipcRenderer.invoke('gh:prForBranch', args),
|
||||
|
||||
refreshPRNow: (args: { candidate: GitHubPRRefreshCandidate }): Promise<unknown> =>
|
||||
|
||||
@@ -3573,7 +3573,12 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
|
||||
// GitHub stops reporting the deleted head by branch name.
|
||||
expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
candidate: expect.objectContaining({ fallbackPRNumber: 13 })
|
||||
candidate: expect.objectContaining({
|
||||
fallbackPRNumber: 13,
|
||||
// Why: main can only head-gate fallback preservation when the
|
||||
// candidate carries the worktree head it was built for.
|
||||
currentHeadOid: 'behind-head'
|
||||
})
|
||||
})
|
||||
)
|
||||
})
|
||||
@@ -3960,7 +3965,7 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
|
||||
expect(runtimeEnvironmentCall).toHaveBeenCalledWith({
|
||||
selector: 'env-1',
|
||||
method: 'github.prForBranch',
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: 12 },
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: 12, currentHeadOid: null },
|
||||
timeoutMs: 30_000
|
||||
})
|
||||
expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({
|
||||
@@ -4010,7 +4015,7 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => {
|
||||
expect(runtimeEnvironmentCall).toHaveBeenCalledWith({
|
||||
selector: 'env-1',
|
||||
method: 'github.prForBranch',
|
||||
params: { repo: 'repo-runtime', branch, linkedPRNumber: null },
|
||||
params: { repo: 'repo-runtime', branch, linkedPRNumber: null, currentHeadOid: null },
|
||||
timeoutMs: 30_000
|
||||
})
|
||||
expect(store.getState().prCache[`runtime:env-1::repo-runtime::${branch}`]?.data).toMatchObject({
|
||||
@@ -4247,7 +4252,7 @@ describe('createGitHubSlice.refreshAllGitHub', () => {
|
||||
expect(runtimeEnvironmentCall).toHaveBeenCalledWith({
|
||||
selector: 'env-1',
|
||||
method: 'github.prForBranch',
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: null },
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: null, currentHeadOid: null },
|
||||
timeoutMs: 30_000
|
||||
})
|
||||
})
|
||||
@@ -4377,7 +4382,7 @@ describe('createGitHubSlice.refreshGitHubForWorktree', () => {
|
||||
expect(runtimeEnvironmentCall).toHaveBeenCalledWith({
|
||||
selector: 'env-1',
|
||||
method: 'github.prForBranch',
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: null },
|
||||
params: { repo: 'repo-1', branch, linkedPRNumber: null, currentHeadOid: null },
|
||||
timeoutMs: 30_000
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1066,6 +1066,7 @@ function buildPRRefreshCandidate(
|
||||
branch,
|
||||
cacheKey,
|
||||
worktreeId: worktree.id,
|
||||
currentHeadOid: worktree.head ?? null,
|
||||
// Why: persisted linked PR metadata is exact, while PR cache numbers are
|
||||
// only fallback hints after branch lookup misses.
|
||||
linkedPRNumber: worktree.linkedPR ?? null,
|
||||
@@ -2949,6 +2950,9 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
const request = (async () => {
|
||||
try {
|
||||
const runtimeRepo = getRuntimeRepoTarget(get(), repoPath, requestSettings)
|
||||
const candidateWorktree = options?.worktreeId
|
||||
? findWorktreeById(get(), options.worktreeId)
|
||||
: null
|
||||
const outcome = runtimeRepo
|
||||
? await callRuntimeRpc<PRInfo | null>(
|
||||
runtimeRepo.target,
|
||||
@@ -2957,6 +2961,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
repo: runtimeRepo.repo.id,
|
||||
branch,
|
||||
linkedPRNumber,
|
||||
currentHeadOid: candidateWorktree?.head ?? null,
|
||||
...(fallbackPRNumber !== null
|
||||
? { fallbackPRNumber, acceptMergedFallbackPR: fallbackPRSource !== null }
|
||||
: {})
|
||||
@@ -2975,6 +2980,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
branch,
|
||||
cacheKey,
|
||||
worktreeId: options?.worktreeId,
|
||||
currentHeadOid: candidateWorktree?.head ?? null,
|
||||
linkedPRNumber,
|
||||
fallbackPRNumber,
|
||||
fallbackPRSource,
|
||||
@@ -2996,7 +3002,9 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
branch,
|
||||
linkedPRNumber,
|
||||
fallbackPRNumber,
|
||||
acceptMergedFallbackPR: fallbackPRNumber !== null && fallbackPRSource !== null
|
||||
acceptMergedFallbackPR:
|
||||
fallbackPRNumber !== null && fallbackPRSource !== null,
|
||||
currentHeadOid: candidateWorktree?.head ?? null
|
||||
})
|
||||
.then((pr) =>
|
||||
pr
|
||||
|
||||
@@ -1196,6 +1196,10 @@ export type GitHubPRRefreshCandidate = GitHubPRRefreshAlias & {
|
||||
cachedMergeable?: PRMergeableState | null
|
||||
cachedMergeStateStatus?: string | null
|
||||
localGitOptions?: { wslDistro?: string }
|
||||
// Why: merged branch-matched PRs are only visible for heads that belong to
|
||||
// the PR; without the worktree head, a panel-supplied fallback number would
|
||||
// keep a merged PR alive head-blind after the branch moves on.
|
||||
currentHeadOid?: string | null
|
||||
}
|
||||
|
||||
export type GitHubPRRefreshSkippedReason =
|
||||
|
||||
Reference in New Issue
Block a user