test(github): pin getOriginOwnerRepo fork-origin resolution

The fork toggle regression fix (originCandidate must resolve via the raw
origin remote, not upstream-first getOwnerRepo) had no pinning coverage:
the client tests aliased getOriginOwnerRepo to the same vi.fn as
getOwnerRepo, so reverting the fix left every test green. gh-utils'
cache-separation test also mocked identical origin/upstream values, so a
cache-key mixing regression would pass undetected.

- Give getOriginOwnerRepo a dedicated mock in client-issue-source.test.ts
  and client-work-items.test.ts, delegating to getOwnerRepoMock by default
  so existing origin-candidate setups keep working unchanged.
- Add a regression test asserting originCandidate resolves via raw origin
  and differs from upstreamCandidate; verified it fails when the
  originCandidate fix is reverted.
- Restore real origin-vs-upstream cache separation coverage in
  gh-utils.test.ts with distinct values and direct getOriginOwnerRepo
  coverage.

Co-Authored-By: Claude <noreply@anthropic.com>

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
brennanb2025
2026-07-05 22:47:46 -07:00
co-authored by Claude Orca
parent 7a9b4b219f
commit 21df6803da
3 changed files with 58 additions and 8 deletions
+38 -3
View File
@@ -9,6 +9,7 @@ const {
execFileAsyncMock,
ghExecFileAsyncMock,
getOwnerRepoMock,
getOriginOwnerRepoMock,
getIssueOwnerRepoMock,
getOwnerRepoForRemoteMock,
resolveIssueSourceMock,
@@ -20,6 +21,7 @@ const {
execFileAsyncMock: vi.fn(),
ghExecFileAsyncMock: vi.fn(),
getOwnerRepoMock: vi.fn(),
getOriginOwnerRepoMock: vi.fn(),
getIssueOwnerRepoMock: vi.fn(),
getOwnerRepoForRemoteMock: vi.fn(),
resolveIssueSourceMock: vi.fn(),
@@ -37,9 +39,11 @@ vi.mock('./gh-utils', async () => {
ghExecFileAsync: ghExecFileAsyncMock,
getOwnerRepo: getOwnerRepoMock,
// Why: resolvePrWorkItemSource resolves the raw origin candidate via
// getOriginOwnerRepo; map it to the same mock these tests use for the
// origin PR repo.
getOriginOwnerRepo: getOwnerRepoMock,
// getOriginOwnerRepo (distinct from upstream-first getOwnerRepo). Keep it a
// separate mock so a regression that collapses originCandidate back onto
// getOwnerRepo is observable; it delegates to getOwnerRepoMock by default
// (see beforeEach) so existing origin-candidate setups keep working.
getOriginOwnerRepo: getOriginOwnerRepoMock,
getIssueOwnerRepo: getIssueOwnerRepoMock,
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
resolveIssueSource: resolveIssueSourceMock,
@@ -61,6 +65,7 @@ describe('GitHub issue source split', () => {
execFileAsyncMock.mockReset()
ghExecFileAsyncMock.mockReset()
getOwnerRepoMock.mockReset()
getOriginOwnerRepoMock.mockReset()
getIssueOwnerRepoMock.mockReset()
getOwnerRepoForRemoteMock.mockReset()
resolveIssueSourceMock.mockReset()
@@ -79,6 +84,13 @@ describe('GitHub issue source split', () => {
source: await getIssueOwnerRepoMock(),
fellBack: false
}))
// Why: originCandidate is resolved via getOriginOwnerRepo. Delegate to
// getOwnerRepoMock by default so the many existing tests that queue the
// origin PR repo on getOwnerRepoMock keep working unchanged; tests that
// must distinguish the two (the revert-guard below) override it.
getOriginOwnerRepoMock.mockImplementation((...args: unknown[]) =>
(getOwnerRepoMock as (...a: unknown[]) => Promise<unknown>)(...args)
)
// Default the upstream-candidate lookup to null so existing tests that
// only mock `getIssueOwnerRepo` + `getOwnerRepo` don't need to think
// about it. Tests that care set it explicitly.
@@ -689,5 +701,28 @@ describe('GitHub issue source split', () => {
upstreamCandidate: { owner: 'stablyai', repo: 'orca' }
})
})
it('resolves originCandidate via raw origin, not upstream-first getOwnerRepo', async () => {
// Why: regression guard for the fork toggle. originCandidate must be the
// raw `origin` remote so the renderer's IssueSourceSelector keeps
// rendering (it gates on originCandidate && upstreamCandidate && distinct).
// If this collapses back onto upstream-first getOwnerRepo, originCandidate
// would equal upstreamCandidate and the toggle vanishes for every fork.
resolveIssueSourceMock.mockResolvedValueOnce({
source: { owner: 'stablyai', repo: 'orca' },
fellBack: false
})
getOriginOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
// What upstream-first getOwnerRepo would return — must NOT reach originCandidate.
getOwnerRepoMock.mockResolvedValue({ owner: 'stablyai', repo: 'orca' })
getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
ghExecFileAsyncMock.mockResolvedValue({ stdout: '[]' })
const result = await listWorkItems('/repo-root', 10, 'is:pr', undefined, 'upstream')
expect(result.sources.originCandidate).toEqual({ owner: 'fork', repo: 'orca' })
expect(result.sources.upstreamCandidate).toEqual({ owner: 'stablyai', repo: 'orca' })
expect(getOriginOwnerRepoMock).toHaveBeenCalled()
})
})
})
+13 -2
View File
@@ -7,6 +7,7 @@ const {
execFileAsyncMock,
ghExecFileAsyncMock,
getOwnerRepoMock,
getOriginOwnerRepoMock,
getIssueOwnerRepoMock,
getOwnerRepoForRemoteMock,
resolveIssueSourceMock,
@@ -19,6 +20,7 @@ const {
execFileAsyncMock: vi.fn(),
ghExecFileAsyncMock: vi.fn(),
getOwnerRepoMock: vi.fn(),
getOriginOwnerRepoMock: vi.fn(),
getIssueOwnerRepoMock: vi.fn(),
getOwnerRepoForRemoteMock: vi.fn(),
resolveIssueSourceMock: vi.fn(),
@@ -51,8 +53,10 @@ vi.mock('./gh-utils', () => ({
: { cwd: context.repoPath, ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) },
getOwnerRepo: getOwnerRepoMock,
// Why: resolvePrWorkItemSource resolves the raw origin candidate via
// getOriginOwnerRepo; map it to the same mock these tests use for origin.
getOriginOwnerRepo: getOwnerRepoMock,
// getOriginOwnerRepo (distinct from upstream-first getOwnerRepo). Keep a
// separate mock; it delegates to getOwnerRepoMock by default (see beforeEach)
// so existing origin-candidate setups keep working.
getOriginOwnerRepo: getOriginOwnerRepoMock,
getIssueOwnerRepo: getIssueOwnerRepoMock,
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
resolveIssueSource: resolveIssueSourceMock,
@@ -85,6 +89,7 @@ describe('listWorkItems', () => {
execFileAsyncMock.mockReset()
ghExecFileAsyncMock.mockReset()
getOwnerRepoMock.mockReset()
getOriginOwnerRepoMock.mockReset()
getIssueOwnerRepoMock.mockReset()
getOwnerRepoForRemoteMock.mockReset()
resolveIssueSourceMock.mockReset()
@@ -102,6 +107,12 @@ describe('listWorkItems', () => {
source: await getIssueOwnerRepoMock(),
fellBack: false
}))
// Why: originCandidate is resolved via getOriginOwnerRepo. Delegate to
// getOwnerRepoMock by default so existing tests that queue the origin PR
// repo on getOwnerRepoMock keep working unchanged.
getOriginOwnerRepoMock.mockImplementation((...args: unknown[]) =>
(getOwnerRepoMock as (...a: unknown[]) => Promise<unknown>)(...args)
)
getOwnerRepoForRemoteMock.mockResolvedValue(null)
_resetOwnerRepoCache()
_resetMergeQueueCacheForTests()
+7 -3
View File
@@ -23,6 +23,7 @@ import {
classifyGhError,
classifyListIssuesError,
getIssueOwnerRepo,
getOriginOwnerRepo,
getOwnerRepo,
getOwnerRepoForRemote,
parseGitHubRemoteIdentity,
@@ -144,12 +145,15 @@ describe('github owner/repo resolution', () => {
})
it('does not mix origin and upstream cache entries for the same repo path', async () => {
// getOwnerRepo and getIssueOwnerRepo both resolve upstream-first now
// Why: getOriginOwnerRepo probes only `origin`; getIssueOwnerRepo is
// upstream-first. Distinct owner/repo values per remote prove the cache
// keys stay separate — a mixing regression would return `fork/orca` for
// the upstream lookup. Also gives getOriginOwnerRepo direct coverage.
gitExecFileAsyncMock
.mockResolvedValueOnce({ stdout: 'git@github.com:stablyai/orca.git\n' })
.mockResolvedValueOnce({ stdout: 'git@github.com:fork/orca.git\n' })
.mockResolvedValueOnce({ stdout: 'git@github.com:stablyai/orca.git\n' })
await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' })
await expect(getOriginOwnerRepo('/repo')).resolves.toEqual({ owner: 'fork', repo: 'orca' })
await expect(getIssueOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' })
})