mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
Resumes the backlog sweep at a chunk size that actually gets read. Six auditors, 84 files
each, and all six read their full scope case-by-case against production — the first wave
where every chunk closed with no gap. 33 case declarations removed across 22 files, 1 test
file deleted, 826 lines gone. No production code touched.
This wave exists because a conclusion of mine was wrong. I had recorded that yield collapsed
~36x and that deletion was no longer the high-value work. I was dividing cases removed by
files IN SCOPE while the fraction auditors actually READ fell from 100% to about 4%, because
I kept handing them 300-800 files. Recomputed against files read, yield has been flat at 4-7
per 100 with no downward trend. This wave came in at 8.2.
The most instructive removal looked like the most valuable test in scope.
`orchestration-worker-release-reap-fixed.func.test.ts` cites a production bug by two
identifiers, describes orphaned PTYs accumulating until `TasksMax=4096` aborts processes on
EAGAIN, and advertises itself as the functional tier wiring the real orchestration RPC
surface, the real `OrchestrationDb` and the real release modules. Deleting it leaves no
reference to that bug anywhere in `src`.
It still had to go: its fake runtime performed the fence it asserted —
if (pty.incarnationId !== inc) { return null }
handleTable.set('term_reminted', { ptyId, epoch: rendererGraphEpoch })
— so the case checking that a reused ptyId with a mismatched incarnation does not resolve was
checking a decision its own spy made twenty lines earlier. The real fence is owned by
`orca-runtime-terminal-handle-incarnation.test.ts:257`, and the other two cases replay
`orchestration-worker-release-incarnation-fallback.test.ts` (which uses a plain
`mockReturnValue` rather than reimplementing the remint) and `worker/worker-release.test.ts:23`.
"Integration test" and "wires real modules" describe the scaffolding, not the asserted step.
Other removals: a self-comparison disguised by an alias, where
`export const getIssueOwnerRepo = getOwnerRepo` makes a case asserting the two "agree" into
`f(x) === f(x)`; four cases whose `vi.mock` of `resolveIssueSource` made both the preference
value and the topology inert; five verdict-precedence cases owned by a verdict-agnostic block;
three call-shape probes on one-line store pass-throughs whose real contracts are driven by
behavioural neighbours; and a `export type _Ref = [...]` declaration whose own comment admits
it exists only to preserve test-only module-surface references.
Kept after checking production rather than shape. An auditor found two near-identical
ten-reconnect loops and kept both: one uses a test-local live-lease filter, the other the
shipped `sshRemotePtyLeaseAllowsReattach` predicate, and the file's own comment explains the
duality is deliberate "so the two cannot drift". Another kept a paths-alignment case that
looks like a validator tested against its own list, because adding a generated file without
registering its path does fail it — and `shellReadyWrappersExist` uses that registered list to
decide whether a partial tree needs regeneration.
Production duplication is now confirmed four times over, and it is why mirrored tests exist:
`createUpdateWorktreeLineage`/`createAssignWorktreeParent` differ by one `console.error`
string; `terminal-path-tap.ts` and `document/path-tap.ts` carry hand-maintained copies of
`matchFilePathAtColumn` under a docblock reading "keep the two in sync". In those cases both
test sides are load-bearing and the duplication belongs on a refactor list.
`mobile/tests-typecheck-baseline.txt` loses one entry. Trimming
`relay-host-signed-out-verdict.test.ts` made it typecheck clean, so the ratchet required
pruning its grandfathered entry — the file graduates from exempt to enforced. Baseline is now
124 entries, down from 125.
Verified: 690 test files / 7,560 cases pass across the touched desktop areas; the modified
mobile files pass (162 cases); `check-tests-typecheck-ratchet.mjs` OK (898 files in program,
124 grandfathered); `check-reliability-gates.mjs` 140 gates; the deleted file is absent from
the gate manifest, `cloud/package.json` and the mobile baseline; nothing under
`mobile/src/test-support/rpc-recording/` or `mobile/rpc-foundation/goldens/` touched.
136 lines
4.9 KiB
TypeScript
136 lines
4.9 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
import type * as GitRunner from '../git/runner'
|
|
|
|
// Fix for issue #7331: PR details failed to load on fork checkouts because
|
|
// getOwnerRepo() (the PR-lookup resolver) only consulted the `origin` remote
|
|
// (the fork), while the PRs live on the upstream parent. getOwnerRepo now
|
|
// prefers `upstream` when present, mirroring getIssueOwnerRepo.
|
|
|
|
const { gitExecFileAsyncMock, ghExecFileAsyncMock, readLocalGitConfigSignatureMock } = vi.hoisted(
|
|
() => ({
|
|
gitExecFileAsyncMock: vi.fn(),
|
|
ghExecFileAsyncMock: vi.fn(),
|
|
readLocalGitConfigSignatureMock: vi.fn(async () => 'sig')
|
|
})
|
|
)
|
|
|
|
vi.mock('../git/runner', async (importOriginal) => ({
|
|
...(await importOriginal<typeof GitRunner>()),
|
|
gitExecFileAsync: gitExecFileAsyncMock,
|
|
ghExecFileAsync: ghExecFileAsyncMock
|
|
}))
|
|
|
|
vi.mock('../providers/ssh-git-dispatch', () => ({
|
|
getSshGitProvider: () => null
|
|
}))
|
|
|
|
vi.mock('./local-git-config-signature', () => ({
|
|
readLocalGitConfigSignature: readLocalGitConfigSignatureMock
|
|
}))
|
|
|
|
import { _resetRemoteNameListingCache } from '../git/remote-name-listing'
|
|
import { getOwnerRepoForRemote, _resetOwnerRepoCache } from './github-repository-identity'
|
|
import { getOwnerRepo } from './github-owner-repo-selection'
|
|
import { getRepoUpstream } from './client'
|
|
|
|
const FORK_PATH = '/tmp/fork-checkout'
|
|
const NON_FORK_PATH = '/tmp/plain-checkout'
|
|
const SSH_FORK_PATH = '/tmp/ssh-fork-checkout'
|
|
|
|
// origin -> personal fork, upstream -> parent (the classic fork checkout).
|
|
const REMOTE_URLS_BY_REPO: Record<string, Record<string, string>> = {
|
|
[FORK_PATH]: {
|
|
origin: 'https://github.com/fsdwen/orca.git',
|
|
upstream: 'https://github.com/stablyai/orca.git'
|
|
},
|
|
[NON_FORK_PATH]: {
|
|
origin: 'https://github.com/stablyai/orca.git'
|
|
},
|
|
[SSH_FORK_PATH]: {
|
|
origin: 'git@github.com:fsdwen/orca.git',
|
|
upstream: 'git@github.com:stablyai/orca.git'
|
|
}
|
|
}
|
|
|
|
beforeEach(() => {
|
|
_resetOwnerRepoCache()
|
|
_resetRemoteNameListingCache()
|
|
gitExecFileAsyncMock.mockReset()
|
|
ghExecFileAsyncMock.mockReset()
|
|
gitExecFileAsyncMock.mockImplementation(
|
|
async (args: string[], options: { cwd?: string } = {}) => {
|
|
const configured = REMOTE_URLS_BY_REPO[options.cwd ?? ''] ?? {}
|
|
if (args[0] === 'remote' && args[1] !== 'get-url') {
|
|
return { stdout: `${Object.keys(configured).join('\n')}\n` }
|
|
}
|
|
const remoteName = args[2]
|
|
const url = configured[remoteName]
|
|
if (!url) {
|
|
const err = new Error(`fatal: No such remote '${remoteName}'`) as Error & { code?: number }
|
|
err.code = 128
|
|
throw err
|
|
}
|
|
return { stdout: url }
|
|
}
|
|
)
|
|
})
|
|
|
|
describe('issue #7331: fork PR owner/repo resolution', () => {
|
|
it('getOwnerRepo prefers the upstream parent on a fork checkout', async () => {
|
|
const prRepo = await getOwnerRepo(FORK_PATH)
|
|
|
|
// PRs live on the parent, so PR lookups must target it (matches
|
|
// getIssueOwnerRepo).
|
|
expect(prRepo).toEqual({ owner: 'stablyai', repo: 'orca' })
|
|
})
|
|
|
|
it('getOwnerRepo falls back to origin when there is no upstream remote', async () => {
|
|
const prRepo = await getOwnerRepo(NON_FORK_PATH)
|
|
|
|
expect(prRepo).toEqual({ owner: 'stablyai', repo: 'orca' })
|
|
})
|
|
|
|
it('skips git remote get-url upstream on origin-only clones and caches the listing', async () => {
|
|
await getOwnerRepo(NON_FORK_PATH)
|
|
const upstreamGetUrl = (): number =>
|
|
gitExecFileAsyncMock.mock.calls.filter(
|
|
([args]) => args[1] === 'get-url' && args[2] === 'upstream'
|
|
).length
|
|
const listCalls = (): number =>
|
|
gitExecFileAsyncMock.mock.calls.filter(
|
|
([args]) => args[0] === 'remote' && args[1] !== 'get-url'
|
|
).length
|
|
expect(upstreamGetUrl()).toBe(0)
|
|
expect(listCalls()).toBe(1)
|
|
|
|
await getOwnerRepo(NON_FORK_PATH)
|
|
expect(upstreamGetUrl()).toBe(0)
|
|
expect(listCalls()).toBe(1)
|
|
})
|
|
|
|
it('resolves the upstream parent for SSH-style remote URLs', async () => {
|
|
const prRepo = await getOwnerRepo(SSH_FORK_PATH)
|
|
|
|
expect(prRepo).toEqual({ owner: 'stablyai', repo: 'orca' })
|
|
})
|
|
|
|
it('getOwnerRepoForRemote(origin) still resolves the fork itself', async () => {
|
|
// Callers that combine origin+upstream themselves (getRepoUpstream,
|
|
// resolvePrWorkItemSource) depend on an origin-only primitive.
|
|
const origin = await getOwnerRepoForRemote(FORK_PATH, 'origin')
|
|
|
|
expect(origin).toEqual({ owner: 'fsdwen', repo: 'orca' })
|
|
})
|
|
|
|
it('getRepoUpstream still resolves the fork parent offline (regression)', async () => {
|
|
// If getRepoUpstream compared upstream against an upstream-first
|
|
// getOwnerRepo, the remotes would look identical and the offline fork
|
|
// fast-path would be skipped.
|
|
const upstream = await getRepoUpstream(FORK_PATH)
|
|
|
|
// Why: origin resolution pins github.com so host-scoped execution is explicit.
|
|
expect(upstream).toEqual({ owner: 'stablyai', repo: 'orca', host: 'github.com' })
|
|
expect(ghExecFileAsyncMock).not.toHaveBeenCalled()
|
|
})
|
|
})
|