mirror of
https://github.com/stablyai/orca.git
synced 2026-09-25 00:02:35 +00:00
feat(github): per-repo issue-source selector (#1317)
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -1,3 +1,7 @@
|
||||
/* eslint-disable max-lines -- Why: the issue-source test suite covers the
|
||||
heuristic split (#1076), the partial-failure envelope (feature 1), and the
|
||||
three-state preference matrix (feature 2) as one surface so a regression in
|
||||
any of them blocks the same merge gate. */
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type * as GhUtils from './gh-utils'
|
||||
|
||||
@@ -6,6 +10,8 @@ const {
|
||||
ghExecFileAsyncMock,
|
||||
getOwnerRepoMock,
|
||||
getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemoteMock,
|
||||
resolveIssueSourceMock,
|
||||
acquireMock,
|
||||
releaseMock
|
||||
} = vi.hoisted(() => ({
|
||||
@@ -13,6 +19,8 @@ const {
|
||||
ghExecFileAsyncMock: vi.fn(),
|
||||
getOwnerRepoMock: vi.fn(),
|
||||
getIssueOwnerRepoMock: vi.fn(),
|
||||
getOwnerRepoForRemoteMock: vi.fn(),
|
||||
resolveIssueSourceMock: vi.fn(),
|
||||
acquireMock: vi.fn(),
|
||||
releaseMock: vi.fn()
|
||||
}))
|
||||
@@ -25,6 +33,8 @@ vi.mock('./gh-utils', async () => {
|
||||
ghExecFileAsync: ghExecFileAsyncMock,
|
||||
getOwnerRepo: getOwnerRepoMock,
|
||||
getIssueOwnerRepo: getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
|
||||
resolveIssueSource: resolveIssueSourceMock,
|
||||
acquire: acquireMock,
|
||||
release: releaseMock,
|
||||
_resetOwnerRepoCache: vi.fn()
|
||||
@@ -39,9 +49,24 @@ describe('GitHub issue source split', () => {
|
||||
ghExecFileAsyncMock.mockReset()
|
||||
getOwnerRepoMock.mockReset()
|
||||
getIssueOwnerRepoMock.mockReset()
|
||||
getOwnerRepoForRemoteMock.mockReset()
|
||||
resolveIssueSourceMock.mockReset()
|
||||
acquireMock.mockReset()
|
||||
releaseMock.mockReset()
|
||||
acquireMock.mockResolvedValue(undefined)
|
||||
// Why: default the preference-aware resolver to 'auto' semantics so the
|
||||
// pre-existing test cases (which don't think about preference at all)
|
||||
// still pass. `listWorkItems` now calls `resolveIssueSource` instead of
|
||||
// `getIssueOwnerRepo` directly — we delegate back to the single-call
|
||||
// mock to preserve the one-fetch-per-test invariant each test sets up.
|
||||
resolveIssueSourceMock.mockImplementation(async () => ({
|
||||
source: await getIssueOwnerRepoMock(),
|
||||
fellBack: false
|
||||
}))
|
||||
// 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.
|
||||
getOwnerRepoForRemoteMock.mockResolvedValue(null)
|
||||
_resetOwnerRepoCache()
|
||||
})
|
||||
|
||||
@@ -237,7 +262,7 @@ describe('GitHub issue source split', () => {
|
||||
const result = await listWorkItems('/repo-root', 10)
|
||||
|
||||
expect(result.items).toEqual([])
|
||||
expect(result.sources).toEqual({
|
||||
expect(result.sources).toMatchObject({
|
||||
issues: { owner: 'stablyai', repo: 'orca' },
|
||||
prs: { owner: 'fork', repo: 'orca' }
|
||||
})
|
||||
@@ -288,4 +313,170 @@ describe('GitHub issue source split', () => {
|
||||
expect(getOwnerRepoMock).not.toHaveBeenCalled()
|
||||
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
describe('per-repo issue-source preference', () => {
|
||||
// Why: 3 preference states × 2 remote-topology states = 6 cases per the
|
||||
// design doc §9. These tests isolate `listWorkItems` against a mocked
|
||||
// `resolveIssueSource` to verify the preference is threaded all the way
|
||||
// to the gh call and that `fellBack` propagates into the envelope.
|
||||
|
||||
it("preference='auto' + upstream exists → queries upstream", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
const result = await listWorkItems('/repo-root', 10, undefined, undefined, 'auto')
|
||||
|
||||
expect(resolveIssueSourceMock).toHaveBeenCalledWith('/repo-root', 'auto')
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
[
|
||||
'api',
|
||||
'--cache',
|
||||
'120s',
|
||||
'repos/stablyai/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
],
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
expect(result.issueSourceFellBack).toBeUndefined()
|
||||
})
|
||||
|
||||
it("preference='auto' + no upstream → queries origin", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'solo', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'auto')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
[
|
||||
'api',
|
||||
'--cache',
|
||||
'120s',
|
||||
'repos/solo/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
],
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
})
|
||||
|
||||
it("preference='upstream' + upstream exists → queries upstream", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
const result = await listWorkItems('/repo-root', 10, undefined, undefined, 'upstream')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
expect.arrayContaining([
|
||||
'repos/stablyai/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
]),
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
expect(result.issueSourceFellBack).toBeUndefined()
|
||||
})
|
||||
|
||||
it("preference='upstream' + no upstream → falls back to origin with fellBack=true", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: true
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'solo', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
const result = await listWorkItems('/repo-root', 10, undefined, undefined, 'upstream')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
expect.arrayContaining([
|
||||
'repos/solo/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
]),
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
expect(result.issueSourceFellBack).toBe(true)
|
||||
})
|
||||
|
||||
it("preference='origin' + upstream exists → queries origin (not upstream)", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'fork', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'origin')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
expect.arrayContaining([
|
||||
'repos/fork/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
]),
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
})
|
||||
|
||||
it("preference='origin' + no upstream → queries origin", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'solo', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'origin')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
expect.arrayContaining([
|
||||
'repos/solo/orca/issues?per_page=10&state=open&sort=updated&direction=desc'
|
||||
]),
|
||||
{ cwd: '/repo-root' }
|
||||
)
|
||||
})
|
||||
|
||||
it('surfaces upstreamCandidate in sources regardless of effective preference', async () => {
|
||||
// Why: the renderer selector needs to keep rendering after the user picks
|
||||
// 'origin'. That requires the envelope to carry the raw upstream even
|
||||
// when `sources.issues` has collapsed onto origin.
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'fork', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
const result = await listWorkItems('/repo-root', 10, undefined, undefined, 'origin')
|
||||
|
||||
expect(result.sources).toEqual({
|
||||
issues: { owner: 'fork', repo: 'orca' },
|
||||
prs: { owner: 'fork', repo: 'orca' },
|
||||
upstreamCandidate: { owner: 'stablyai', repo: 'orca' }
|
||||
})
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,3 +1,6 @@
|
||||
/* eslint-disable max-lines -- Why: work-items coverage stays in one file so
|
||||
the fan-out mock plumbing (issue + PR gh calls, allSettled handling) does
|
||||
not drift across split files. */
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const {
|
||||
@@ -5,6 +8,8 @@ const {
|
||||
ghExecFileAsyncMock,
|
||||
getOwnerRepoMock,
|
||||
getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemoteMock,
|
||||
resolveIssueSourceMock,
|
||||
gitExecFileAsyncMock,
|
||||
acquireMock,
|
||||
releaseMock
|
||||
@@ -13,6 +18,8 @@ const {
|
||||
ghExecFileAsyncMock: vi.fn(),
|
||||
getOwnerRepoMock: vi.fn(),
|
||||
getIssueOwnerRepoMock: vi.fn(),
|
||||
getOwnerRepoForRemoteMock: vi.fn(),
|
||||
resolveIssueSourceMock: vi.fn(),
|
||||
gitExecFileAsyncMock: vi.fn(),
|
||||
acquireMock: vi.fn(),
|
||||
releaseMock: vi.fn()
|
||||
@@ -23,9 +30,13 @@ vi.mock('./gh-utils', () => ({
|
||||
ghExecFileAsync: ghExecFileAsyncMock,
|
||||
getOwnerRepo: getOwnerRepoMock,
|
||||
getIssueOwnerRepo: getIssueOwnerRepoMock,
|
||||
getOwnerRepoForRemote: getOwnerRepoForRemoteMock,
|
||||
resolveIssueSource: resolveIssueSourceMock,
|
||||
acquire: acquireMock,
|
||||
release: releaseMock,
|
||||
_resetOwnerRepoCache: vi.fn()
|
||||
_resetOwnerRepoCache: vi.fn(),
|
||||
classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
||||
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
||||
}))
|
||||
|
||||
vi.mock('../git/runner', () => ({
|
||||
@@ -40,10 +51,20 @@ describe('listWorkItems', () => {
|
||||
ghExecFileAsyncMock.mockReset()
|
||||
getOwnerRepoMock.mockReset()
|
||||
getIssueOwnerRepoMock.mockReset()
|
||||
getOwnerRepoForRemoteMock.mockReset()
|
||||
resolveIssueSourceMock.mockReset()
|
||||
gitExecFileAsyncMock.mockReset()
|
||||
acquireMock.mockReset()
|
||||
releaseMock.mockReset()
|
||||
acquireMock.mockResolvedValue(undefined)
|
||||
// Why: preference-aware `listWorkItems` calls `resolveIssueSource`.
|
||||
// Route through the same `getIssueOwnerRepoMock` so existing tests that
|
||||
// only set up `getIssueOwnerRepoMock` continue to work.
|
||||
resolveIssueSourceMock.mockImplementation(async () => ({
|
||||
source: await getIssueOwnerRepoMock(),
|
||||
fellBack: false
|
||||
}))
|
||||
getOwnerRepoForRemoteMock.mockResolvedValue(null)
|
||||
_resetOwnerRepoCache()
|
||||
})
|
||||
|
||||
@@ -81,7 +102,7 @@ describe('listWorkItems', () => {
|
||||
])
|
||||
})
|
||||
const { items, sources } = await listWorkItems('/repo-root', 10, 'assignee:@me')
|
||||
expect(sources).toEqual({
|
||||
expect(sources).toMatchObject({
|
||||
issues: { owner: 'acme', repo: 'widgets' },
|
||||
prs: { owner: 'acme', repo: 'widgets' }
|
||||
})
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
concurrency acquire/release pattern and error handling consistent across operations. */
|
||||
import type {
|
||||
ClassifiedError,
|
||||
IssueSourcePreference,
|
||||
ListWorkItemsResult,
|
||||
PRInfo,
|
||||
PRMergeableState,
|
||||
@@ -22,6 +23,8 @@ import {
|
||||
release,
|
||||
getOwnerRepo,
|
||||
getIssueOwnerRepo,
|
||||
getOwnerRepoForRemote,
|
||||
resolveIssueSource,
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
type OwnerRepo
|
||||
@@ -578,12 +581,20 @@ export async function listWorkItems(
|
||||
repoPath: string,
|
||||
limit = 24,
|
||||
query?: string,
|
||||
before?: string
|
||||
before?: string,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<ListWorkItemsResult<MainWorkItem>> {
|
||||
const [issueOwnerRepo, prOwnerRepo] = await Promise.all([
|
||||
getIssueOwnerRepo(repoPath),
|
||||
getOwnerRepo(repoPath)
|
||||
// Why: resolve the raw upstream candidate alongside the preference-aware
|
||||
// issue source. The selector needs to know whether an upstream remote
|
||||
// *exists* to decide whether to render — independent of whether the user
|
||||
// has picked 'origin' (which would otherwise make `sources.issues` equal
|
||||
// origin and hide the selector permanently).
|
||||
const [issueResolved, prOwnerRepo, upstreamCandidate] = await Promise.all([
|
||||
resolveIssueSource(repoPath, preference),
|
||||
getOwnerRepo(repoPath),
|
||||
getOwnerRepoForRemote(repoPath, 'upstream')
|
||||
])
|
||||
const issueOwnerRepo = issueResolved.source
|
||||
const trimmedQuery = query?.trim() ?? ''
|
||||
await acquire()
|
||||
try {
|
||||
@@ -605,8 +616,13 @@ export async function listWorkItems(
|
||||
const errors = partial.issuesError ? { issues: partial.issuesError } : undefined
|
||||
return {
|
||||
items: partial.items,
|
||||
sources: { issues: issueOwnerRepo, prs: prOwnerRepo },
|
||||
...(errors ? { errors } : {})
|
||||
sources: {
|
||||
issues: issueOwnerRepo,
|
||||
prs: prOwnerRepo,
|
||||
upstreamCandidate: upstreamCandidate ?? null
|
||||
},
|
||||
...(errors ? { errors } : {}),
|
||||
...(issueResolved.fellBack ? { issueSourceFellBack: true } : {})
|
||||
}
|
||||
} finally {
|
||||
release()
|
||||
@@ -701,11 +717,16 @@ function defaultOpenWorkItemQuery(): ParsedTaskQuery {
|
||||
// Why: uses GitHub's search API to get total_count without fetching items.
|
||||
// This powers the pagination bar so the user sees total pages upfront.
|
||||
// Cached for 120s to avoid burning the search rate limit (30 req/min).
|
||||
export async function countWorkItems(repoPath: string, query?: string): Promise<number> {
|
||||
const [issueOwnerRepo, prOwnerRepo] = await Promise.all([
|
||||
getIssueOwnerRepo(repoPath),
|
||||
export async function countWorkItems(
|
||||
repoPath: string,
|
||||
query?: string,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<number> {
|
||||
const [issueResolved, prOwnerRepo] = await Promise.all([
|
||||
resolveIssueSource(repoPath, preference),
|
||||
getOwnerRepo(repoPath)
|
||||
])
|
||||
const issueOwnerRepo = issueResolved.source
|
||||
const ownerRepo = prOwnerRepo ?? issueOwnerRepo
|
||||
if (!ownerRepo) {
|
||||
return 0
|
||||
|
||||
@@ -11,9 +11,12 @@ vi.mock('../git/runner', () => ({
|
||||
|
||||
import {
|
||||
_resetOwnerRepoCache,
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
getIssueOwnerRepo,
|
||||
getOwnerRepo,
|
||||
parseGitHubOwnerRepo
|
||||
parseGitHubOwnerRepo,
|
||||
resolveIssueSource
|
||||
} from './gh-utils'
|
||||
|
||||
describe('github owner/repo resolution', () => {
|
||||
@@ -83,3 +86,113 @@ describe('github owner/repo resolution', () => {
|
||||
await expect(getIssueOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' })
|
||||
})
|
||||
})
|
||||
|
||||
describe('resolveIssueSource', () => {
|
||||
beforeEach(() => {
|
||||
gitExecFileAsyncMock.mockReset()
|
||||
_resetOwnerRepoCache()
|
||||
})
|
||||
|
||||
it("'auto' + upstream exists → upstream, fellBack=false", async () => {
|
||||
gitExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: 'git@github.com:stablyai/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
})
|
||||
|
||||
it("'auto' + no upstream → origin, fellBack=false", async () => {
|
||||
gitExecFileAsyncMock
|
||||
.mockResolvedValueOnce({ stdout: 'git@example.com:stablyai/orca.git\n' })
|
||||
.mockResolvedValueOnce({ stdout: 'git@github.com:solo/orca.git\n' })
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'auto')).resolves.toEqual({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
})
|
||||
|
||||
it("'upstream' + upstream exists → upstream, fellBack=false", async () => {
|
||||
gitExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: 'git@github.com:stablyai/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'upstream')).resolves.toEqual({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
})
|
||||
|
||||
it("'upstream' + no upstream remote → origin, fellBack=true", async () => {
|
||||
// No upstream remote configured — the first call fails.
|
||||
gitExecFileAsyncMock
|
||||
.mockRejectedValueOnce(new Error('fatal: No such remote'))
|
||||
.mockResolvedValueOnce({ stdout: 'git@github.com:solo/orca.git\n' })
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'upstream')).resolves.toEqual({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: true
|
||||
})
|
||||
})
|
||||
|
||||
it("'origin' + upstream exists → origin (ignores upstream), fellBack=false", async () => {
|
||||
// Only one gh call should happen — origin. Upstream is never consulted.
|
||||
gitExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: 'git@github.com:fork/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'origin')).resolves.toEqual({
|
||||
source: { owner: 'fork', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1)
|
||||
expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['remote', 'get-url', 'origin'], {
|
||||
cwd: '/repo'
|
||||
})
|
||||
})
|
||||
|
||||
it("'origin' + no upstream → origin, fellBack=false", async () => {
|
||||
gitExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: 'git@github.com:solo/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(resolveIssueSource('/repo', 'origin')).resolves.toEqual({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
})
|
||||
|
||||
it('undefined preference is treated identically to auto', async () => {
|
||||
gitExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: 'git@github.com:stablyai/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(resolveIssueSource('/repo', undefined)).resolves.toEqual({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('gh error classification', () => {
|
||||
// Why: a fork with Issues turned off triggers `gh issue list` stderr
|
||||
// "the '<slug>' repository has disabled issues". Without a dedicated branch
|
||||
// the raw "Command failed: gh issue list …" line leaks into the Tasks banner
|
||||
// via the `unknown` fallback — which is what users see when they flip the
|
||||
// per-repo selector to an origin fork that has issues disabled.
|
||||
it('classifies "has disabled issues" stderr as issues_disabled', () => {
|
||||
const stderr =
|
||||
"Command failed: gh issue list --limit 36 --json number,title,state --repo brennanb2025/orca --state open\nthe 'brennanb2025/orca' repository has disabled issues"
|
||||
expect(classifyGhError(stderr)).toEqual({
|
||||
type: 'issues_disabled',
|
||||
message: 'Issues are disabled on this repository.'
|
||||
})
|
||||
expect(classifyListIssuesError(stderr)).toEqual({
|
||||
type: 'issues_disabled',
|
||||
message: 'Issues are disabled on this repository.'
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { execFile } from 'child_process'
|
||||
import { promisify } from 'util'
|
||||
import { gitExecFileAsync, ghExecFileAsync } from '../git/runner'
|
||||
import type { ClassifiedError, GitHubOwnerRepo } from '../../shared/types'
|
||||
import type { ClassifiedError, GitHubOwnerRepo, IssueSourcePreference } from '../../shared/types'
|
||||
|
||||
// Why: legacy generic execFile wrapper — only used by callers that don't need
|
||||
// WSL-aware routing (e.g. non-repo-scoped gh commands). Repo-scoped callers
|
||||
@@ -54,6 +54,14 @@ export function classifyGhError(stderr: string): ClassifiedError {
|
||||
if (s.includes('http 404') || s.includes('could not resolve to a repository')) {
|
||||
return { type: 'not_found', message: 'Issue not found — it may have been deleted.' }
|
||||
}
|
||||
// Why: `gh issue list` prints "the '<owner>/<repo>' repository has disabled
|
||||
// issues" when Issues are turned off in repo settings (common on forks). This
|
||||
// hits during feature-2 when a user flips the selector to an origin fork —
|
||||
// without a dedicated branch the raw "Command failed: gh issue list …" line
|
||||
// leaks verbatim into the banner via the `unknown` fallback.
|
||||
if (s.includes('has disabled issues')) {
|
||||
return { type: 'issues_disabled', message: 'Issues are disabled on this repository.' }
|
||||
}
|
||||
if (s.includes('http 422') || s.includes('validation failed')) {
|
||||
return { type: 'validation_error', message: `Invalid update — ${stderr.trim()}` }
|
||||
}
|
||||
@@ -89,6 +97,7 @@ export function classifyListIssuesError(stderr: string): ClassifiedError {
|
||||
permission_denied:
|
||||
"You don't have permission to read issues for this repository. Check your GitHub token scopes.",
|
||||
not_found: 'Repository not found.',
|
||||
issues_disabled: 'Issues are disabled on this repository.',
|
||||
validation_error: `Invalid request — ${trimmed}`,
|
||||
rate_limited: 'GitHub rate limit hit. Try again in a few minutes.',
|
||||
network_error: 'Network error — check your connection.',
|
||||
@@ -118,7 +127,7 @@ export function parseGitHubOwnerRepo(remoteUrl: string): OwnerRepo | null {
|
||||
return { owner: match[1], repo: match[2] }
|
||||
}
|
||||
|
||||
async function getOwnerRepoForRemote(
|
||||
export async function getOwnerRepoForRemote(
|
||||
repoPath: string,
|
||||
remoteName: string
|
||||
): Promise<OwnerRepo | null> {
|
||||
@@ -153,3 +162,44 @@ export async function getIssueOwnerRepo(repoPath: string): Promise<OwnerRepo | n
|
||||
}
|
||||
return getOwnerRepoForRemote(repoPath, 'origin')
|
||||
}
|
||||
|
||||
export type ResolvedIssueSource = {
|
||||
source: OwnerRepo | null
|
||||
/** True when the user preferred `upstream` but the upstream remote is no
|
||||
* longer configured and the resolver fell back to origin. Consumers
|
||||
* surface this as a one-time toast per session/repo. */
|
||||
fellBack: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the issue source for a repo honoring the user's per-repo preference.
|
||||
*
|
||||
* Do not delete `getIssueOwnerRepo`: it remains the right primitive for
|
||||
* `'auto'` mode and for preference-agnostic callers like typed work-item
|
||||
* detail lookups (where the issue-vs-PR disambiguation is orthogonal to
|
||||
* user choice).
|
||||
*/
|
||||
export async function resolveIssueSource(
|
||||
repoPath: string,
|
||||
preference: IssueSourcePreference | undefined
|
||||
): Promise<ResolvedIssueSource> {
|
||||
if (preference === 'upstream') {
|
||||
const upstream = await getOwnerRepoForRemote(repoPath, 'upstream')
|
||||
if (upstream) {
|
||||
return { source: upstream, fellBack: false }
|
||||
}
|
||||
// Why: explicit upstream is gone — fall back to origin but only flag the
|
||||
// fallback when it actually produced an origin source. If origin is also
|
||||
// missing (or non-GitHub), there's nothing to "fall back to" and the
|
||||
// UI toast "using origin" would be misleading. Do NOT auto-reset the
|
||||
// preference: the user may be mid-way through a workflow and expect
|
||||
// their choice to re-engage if `upstream` is re-added.
|
||||
const origin = await getOwnerRepoForRemote(repoPath, 'origin')
|
||||
return { source: origin, fellBack: origin !== null }
|
||||
}
|
||||
if (preference === 'origin') {
|
||||
return { source: await getOwnerRepoForRemote(repoPath, 'origin'), fellBack: false }
|
||||
}
|
||||
// 'auto' or undefined
|
||||
return { source: await getIssueOwnerRepo(repoPath), fellBack: false }
|
||||
}
|
||||
|
||||
@@ -1,9 +1,16 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type * as GhUtils from './gh-utils'
|
||||
|
||||
const { ghExecFileAsyncMock, getIssueOwnerRepoMock, acquireMock, releaseMock } = vi.hoisted(() => ({
|
||||
const {
|
||||
ghExecFileAsyncMock,
|
||||
getIssueOwnerRepoMock,
|
||||
resolveIssueSourceMock,
|
||||
acquireMock,
|
||||
releaseMock
|
||||
} = vi.hoisted(() => ({
|
||||
ghExecFileAsyncMock: vi.fn(),
|
||||
getIssueOwnerRepoMock: vi.fn(),
|
||||
resolveIssueSourceMock: vi.fn(),
|
||||
acquireMock: vi.fn(),
|
||||
releaseMock: vi.fn()
|
||||
}))
|
||||
@@ -14,6 +21,7 @@ vi.mock('./gh-utils', async () => {
|
||||
...actual,
|
||||
ghExecFileAsync: ghExecFileAsyncMock,
|
||||
getIssueOwnerRepo: getIssueOwnerRepoMock,
|
||||
resolveIssueSource: resolveIssueSourceMock,
|
||||
acquire: acquireMock,
|
||||
release: releaseMock
|
||||
}
|
||||
@@ -25,9 +33,17 @@ describe('issue source operations', () => {
|
||||
beforeEach(() => {
|
||||
ghExecFileAsyncMock.mockReset()
|
||||
getIssueOwnerRepoMock.mockReset()
|
||||
resolveIssueSourceMock.mockReset()
|
||||
acquireMock.mockReset()
|
||||
releaseMock.mockReset()
|
||||
acquireMock.mockResolvedValue(undefined)
|
||||
// Why: preference-aware paths call resolveIssueSource instead of
|
||||
// getIssueOwnerRepo. Route through the same mock so existing tests that
|
||||
// set up getIssueOwnerRepoMock continue to work.
|
||||
resolveIssueSourceMock.mockImplementation(async () => ({
|
||||
source: await getIssueOwnerRepoMock(),
|
||||
fellBack: false
|
||||
}))
|
||||
})
|
||||
|
||||
it('gets a single issue from the issue owner/repo', async () => {
|
||||
|
||||
+70
-12
@@ -1,23 +1,44 @@
|
||||
/* eslint-disable max-lines -- Why: co-locating issue list/create/update/
|
||||
comment operations keeps the shared acquire/release + error-classification
|
||||
pattern obvious. Each function is short; the file is long because the
|
||||
surface is broad. */
|
||||
import type {
|
||||
ClassifiedError,
|
||||
GitHubAssignableUser,
|
||||
GitHubCommentResult,
|
||||
GitHubIssueUpdate,
|
||||
IssueInfo,
|
||||
IssueSourcePreference,
|
||||
PRComment
|
||||
} from '../../shared/types'
|
||||
import { mapIssueInfo } from './mappers'
|
||||
// prettier-ignore
|
||||
import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, classifyGhError, classifyListIssuesError } from './gh-utils'
|
||||
import { ghExecFileAsync, acquire, release, getIssueOwnerRepo, resolveIssueSource, classifyGhError, classifyListIssuesError } from './gh-utils'
|
||||
|
||||
// Why: distinguishes a successful-empty listing from a failed fetch. The
|
||||
// previous `catch { return [] }` conflated a 403 on a private upstream with an
|
||||
// empty backlog. Callers decide how to surface `error`.
|
||||
export type IssueListResult = { items: IssueInfo[]; error?: ClassifiedError }
|
||||
//
|
||||
// Why no `fellBack` here: the fell-back signal for the renderer toast rides on
|
||||
// `ListWorkItemsResult.issueSourceFellBack` (the Tasks list's envelope). The
|
||||
// only consumer of `listIssues` — the `gh:listIssues` IPC handler — unwraps
|
||||
// to `.items` and has no UI hook to surface a fallback toast. Adding a dead
|
||||
// `fellBack` field here invited drift between the JSDoc promise and reality.
|
||||
export type IssueListResult = {
|
||||
items: IssueInfo[]
|
||||
error?: ClassifiedError
|
||||
}
|
||||
|
||||
/**
|
||||
* Get a single issue by number.
|
||||
* Uses gh api --cache so 304 Not Modified responses don't count against the rate limit.
|
||||
*
|
||||
* Why this path doesn't take a preference: linked-issue lookups persist a
|
||||
* number to a worktree at creation time. Routing detail lookups through the
|
||||
* live per-repo preference would silently flip an existing link to a
|
||||
* different repo after the user toggled the selector — the opposite of what
|
||||
* #1186 / the parent design doc guard against. List and create paths honor
|
||||
* preference; number-resolution stays on the heuristic.
|
||||
*/
|
||||
export async function getIssue(repoPath: string, issueNumber: number): Promise<IssueInfo | null> {
|
||||
const ownerRepo = await getIssueOwnerRepo(repoPath)
|
||||
@@ -61,8 +82,12 @@ export async function getIssue(repoPath: string, issueNumber: number): Promise<I
|
||||
* (§3) — silently hiding failures re-creates the same silent-source-switch
|
||||
* class of wrongness #1186 warned against, one level deeper.
|
||||
*/
|
||||
export async function listIssues(repoPath: string, limit = 20): Promise<IssueListResult> {
|
||||
const ownerRepo = await getIssueOwnerRepo(repoPath)
|
||||
export async function listIssues(
|
||||
repoPath: string,
|
||||
limit = 20,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<IssueListResult> {
|
||||
const { source: ownerRepo } = await resolveIssueSource(repoPath, preference)
|
||||
await acquire()
|
||||
try {
|
||||
if (ownerRepo) {
|
||||
@@ -92,10 +117,15 @@ export async function listIssues(repoPath: string, limit = 20): Promise<IssueLis
|
||||
{ cwd: repoPath }
|
||||
)
|
||||
const data = JSON.parse(stdout) as unknown[]
|
||||
return { items: data.map((d) => mapIssueInfo(d as Parameters<typeof mapIssueInfo>[0])) }
|
||||
return {
|
||||
items: data.map((d) => mapIssueInfo(d as Parameters<typeof mapIssueInfo>[0]))
|
||||
}
|
||||
} catch (err) {
|
||||
const stderr = err instanceof Error ? err.message : String(err)
|
||||
return { items: [], error: classifyListIssuesError(stderr) }
|
||||
return {
|
||||
items: [],
|
||||
error: classifyListIssuesError(stderr)
|
||||
}
|
||||
} finally {
|
||||
release()
|
||||
}
|
||||
@@ -109,13 +139,14 @@ export async function listIssues(repoPath: string, limit = 20): Promise<IssueLis
|
||||
export async function createIssue(
|
||||
repoPath: string,
|
||||
title: string,
|
||||
body: string
|
||||
body: string,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<{ ok: true; number: number; url: string } | { ok: false; error: string }> {
|
||||
const trimmedTitle = title.trim()
|
||||
if (!trimmedTitle) {
|
||||
return { ok: false, error: 'Title is required' }
|
||||
}
|
||||
const ownerRepo = await getIssueOwnerRepo(repoPath)
|
||||
const { source: ownerRepo } = await resolveIssueSource(repoPath, preference)
|
||||
if (!ownerRepo) {
|
||||
return { ok: false, error: 'Could not resolve GitHub owner/repo for this repository' }
|
||||
}
|
||||
@@ -154,6 +185,15 @@ export async function createIssue(
|
||||
/**
|
||||
* Update an existing GitHub issue. Fans out to separate gh commands for
|
||||
* state changes vs field edits since `gh issue edit` does not support state.
|
||||
*
|
||||
* Why this path doesn't take a preference (mirrors `getIssue`): mutations
|
||||
* target an issue number already bound to a worktree / linked elsewhere in
|
||||
* the UI. Routing an update through the live per-repo preference would let
|
||||
* a user open upstream#N, toggle the selector to origin, save, and silently
|
||||
* write to origin#N — a different issue (or 404). That is the exact
|
||||
* silent-source-switch class of wrongness #1186 / the parent design doc
|
||||
* guard against. List and create paths honor preference; mutations stay on
|
||||
* the heuristic `getIssueOwnerRepo`.
|
||||
*/
|
||||
export async function updateIssue(
|
||||
repoPath: string,
|
||||
@@ -230,6 +270,18 @@ export async function updateIssue(
|
||||
return { ok: true }
|
||||
}
|
||||
|
||||
/**
|
||||
* Add a comment to an existing GitHub issue.
|
||||
*
|
||||
* Why this path doesn't take a preference (mirrors `getIssue` / `updateIssue`):
|
||||
* a comment is posted against an issue number already bound to a worktree or
|
||||
* surfaced from a prior read. Routing through the live per-repo preference
|
||||
* would let a user read upstream#N, toggle the selector to origin, and have
|
||||
* their reply silently post on origin#N — a different issue entirely. That
|
||||
* is the same silent-source-switch class of wrongness #1186 / the parent
|
||||
* design doc guard against. List and create paths honor preference;
|
||||
* mutations stay on the heuristic `getIssueOwnerRepo`.
|
||||
*/
|
||||
export async function addIssueComment(
|
||||
repoPath: string,
|
||||
issueNumber: number,
|
||||
@@ -277,8 +329,11 @@ export async function addIssueComment(
|
||||
}
|
||||
}
|
||||
|
||||
export async function listLabels(repoPath: string): Promise<string[]> {
|
||||
const ownerRepo = await getIssueOwnerRepo(repoPath)
|
||||
export async function listLabels(
|
||||
repoPath: string,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<string[]> {
|
||||
const { source: ownerRepo } = await resolveIssueSource(repoPath, preference)
|
||||
if (!ownerRepo) {
|
||||
return []
|
||||
}
|
||||
@@ -305,8 +360,11 @@ export async function listLabels(repoPath: string): Promise<string[]> {
|
||||
}
|
||||
}
|
||||
|
||||
export async function listAssignableUsers(repoPath: string): Promise<GitHubAssignableUser[]> {
|
||||
const ownerRepo = await getIssueOwnerRepo(repoPath)
|
||||
export async function listAssignableUsers(
|
||||
repoPath: string,
|
||||
preference?: IssueSourcePreference
|
||||
): Promise<GitHubAssignableUser[]> {
|
||||
const { source: ownerRepo } = await resolveIssueSource(repoPath, preference)
|
||||
if (!ownerRepo) {
|
||||
return []
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user