mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fix(runtime): let a scoped worktree listing report the host it could not cover (#18645)
* fix(runtime): let a scoped worktree listing report the host it could not cover
`orca worktree list --repo <id>` passed `[]` as `knownHostIds`, so a scoped
listing could never report a gap — for any host kind, reachable or not. With zero
matched rows both scope lists are empty by construction, and the answer is
`{hostIds: [], omittedHostIds: []}`: byte-identical to a repo that genuinely has
no worktrees. docs/reference/ssh-execution-boundary.md forbids a listing from
implying exactly that.
Measured on one runtime with one refusing SSH host, in the same second:
unscoped totalCount 0 omittedHostIds ["local","ssh:<target>"] (+ --host selectors)
scoped totalCount 0 hostScope {"hostIds":[],"omittedHostIds":[]}
The same runtime reports nine omitted hosts unscoped and zero scoped against a
live profile, so this is not a subtle inconsistency: it is one runtime giving two
contradictory answers about its own coverage.
A scoped listing now names the one host the caller asked about. That costs
nothing when rows come back — the host lands in `covered`, so it is never
reported omitted — and is the whole answer when they do not. Hosts the caller
scoped out are still never named, which a test pins, because naming them all is
the obvious over-correction.
* test(runtime): pin the scoped host derivation for local and executionHostId repos
Review flagged that the host-scope cases only covered a connectionId repo.
getRepoExecutionHostId reads two spellings, and a scoped listing naming the
wrong host would be worse than naming none, so both are pinned. Both fail with
the fix reverted.
This commit is contained in:
@@ -40,14 +40,18 @@ function metadata(overrides: Partial<WorktreeMeta> = {}): WorktreeMeta {
|
||||
}
|
||||
}
|
||||
|
||||
function queries(store: RuntimeStore): RuntimeManagedWorktreeQueries {
|
||||
function queries(
|
||||
store: RuntimeStore,
|
||||
overrides: Partial<ConstructorParameters<typeof RuntimeManagedWorktreeQueries>[0]> = {}
|
||||
): RuntimeManagedWorktreeQueries {
|
||||
return new RuntimeManagedWorktreeQueries({
|
||||
getStore: () => store,
|
||||
listResolved: async () => [],
|
||||
resolveRepo: async () => store.getRepos()[0]!,
|
||||
selectRepos: () => store.getRepos(),
|
||||
scanRepo: async () => ({ ok: true, worktrees: [] }),
|
||||
listKnownHostIds: () => []
|
||||
listKnownHostIds: () => [],
|
||||
...overrides
|
||||
})
|
||||
}
|
||||
|
||||
@@ -105,3 +109,115 @@ describe('RuntimeManagedWorktreeQueries.listDetected', () => {
|
||||
expect(legacy.worktrees[0]).not.toHaveProperty('visibilitySource')
|
||||
})
|
||||
})
|
||||
|
||||
describe('RuntimeManagedWorktreeQueries.list host scope', () => {
|
||||
// Measured on hardware before this fix, same runtime and same refusing SSH host in the same
|
||||
// second: the UNSCOPED listing reported `omittedHostIds: ["local","ssh:ssh-scope-refused"]` with
|
||||
// `--host` selectors, while the SCOPED listing reported `{"hostIds":[],"omittedHostIds":[]}`.
|
||||
// A listing that covered nothing, reporting no gaps, is indistinguishable from a repo that
|
||||
// genuinely has no worktrees -- the thing docs/reference/ssh-execution-boundary.md forbids.
|
||||
function sshStore(): RuntimeStore {
|
||||
const repo = folderRepo({
|
||||
id: 'repo-ssh',
|
||||
kind: 'git',
|
||||
connectionId: 'conn-1',
|
||||
path: '/home/dev/app'
|
||||
})
|
||||
return {
|
||||
getRepos: () => [repo],
|
||||
getRepo: () => repo,
|
||||
getAllWorktreeMeta: () => ({}),
|
||||
getWorktreeMeta: () => undefined,
|
||||
setWorktreeMeta: vi.fn(),
|
||||
getAllWorktreeLineage: () => ({}),
|
||||
getSettings: () => settings
|
||||
} as unknown as RuntimeStore
|
||||
}
|
||||
|
||||
it('names the scoped repo host as omitted when the listing covered nothing', async () => {
|
||||
const result = await queries(sshStore()).list('repo-ssh', 50)
|
||||
|
||||
expect(result.totalCount).toBe(0)
|
||||
expect(result.hostScope).toEqual({
|
||||
hostIds: [],
|
||||
omittedHostIds: ['ssh:conn-1']
|
||||
})
|
||||
})
|
||||
|
||||
it('does not report the scoped host as omitted once it contributes rows', async () => {
|
||||
const store = sshStore()
|
||||
const result = await queries(store, {
|
||||
listResolved: async () =>
|
||||
[
|
||||
{
|
||||
id: 'repo-ssh::/home/dev/app',
|
||||
repoId: 'repo-ssh',
|
||||
path: '/home/dev/app',
|
||||
hostId: 'ssh:conn-1'
|
||||
}
|
||||
] as never
|
||||
}).list('repo-ssh', 50)
|
||||
|
||||
expect(result.hostScope?.hostIds).toEqual(['ssh:conn-1'])
|
||||
expect(result.hostScope?.omittedHostIds).toEqual([])
|
||||
})
|
||||
|
||||
// The caller scoped the listing, so the hosts they excluded must not come back as gaps.
|
||||
it('never names a host the caller scoped out', async () => {
|
||||
const scoped = await queries(sshStore(), {
|
||||
listKnownHostIds: () => ['local', 'ssh:other', 'runtime:elsewhere'] as never
|
||||
}).list('repo-ssh', 50)
|
||||
|
||||
expect(scoped.hostScope?.omittedHostIds).toEqual(['ssh:conn-1'])
|
||||
})
|
||||
|
||||
// `getRepoExecutionHostId` derives the host from two spellings, and a scoped listing that named
|
||||
// the wrong one would be worse than naming none. These pin both.
|
||||
it('names the local host for a scoped local repo', async () => {
|
||||
const repo = folderRepo({ id: 'repo-local', kind: 'git', path: '/workspace/local' })
|
||||
const store = {
|
||||
getRepos: () => [repo],
|
||||
getRepo: () => repo,
|
||||
getAllWorktreeMeta: () => ({}),
|
||||
getWorktreeMeta: () => undefined,
|
||||
setWorktreeMeta: vi.fn(),
|
||||
getAllWorktreeLineage: () => ({}),
|
||||
getSettings: () => settings
|
||||
} as unknown as RuntimeStore
|
||||
|
||||
const result = await queries(store).list('repo-local', 50)
|
||||
|
||||
expect(result.hostScope?.omittedHostIds).toEqual(['local'])
|
||||
})
|
||||
|
||||
it('prefers executionHostId over connectionId for the scoped host', async () => {
|
||||
const repo = folderRepo({
|
||||
id: 'repo-runtime',
|
||||
kind: 'git',
|
||||
connectionId: 'conn-legacy',
|
||||
executionHostId: 'runtime:env-1',
|
||||
path: '/workspace/runtime'
|
||||
})
|
||||
const store = {
|
||||
getRepos: () => [repo],
|
||||
getRepo: () => repo,
|
||||
getAllWorktreeMeta: () => ({}),
|
||||
getWorktreeMeta: () => undefined,
|
||||
setWorktreeMeta: vi.fn(),
|
||||
getAllWorktreeLineage: () => ({}),
|
||||
getSettings: () => settings
|
||||
} as unknown as RuntimeStore
|
||||
|
||||
const result = await queries(store).list('repo-runtime', 50)
|
||||
|
||||
expect(result.hostScope?.omittedHostIds).toEqual(['runtime:env-1'])
|
||||
})
|
||||
|
||||
it('still reports every configured host when the listing is unscoped', async () => {
|
||||
const unscoped = await queries(sshStore(), {
|
||||
listKnownHostIds: () => ['local', 'ssh:conn-1'] as never
|
||||
}).list(undefined, 50)
|
||||
|
||||
expect(unscoped.hostScope?.omittedHostIds).toEqual(['local', 'ssh:conn-1'])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2,7 +2,7 @@ import type { DetectedWorktreeListResult, Worktree } from '../../shared/worktree
|
||||
import type { Repo } from '../../shared/repo-types'
|
||||
import type { RuntimeWorktreeListResult } from '../../shared/runtime-types'
|
||||
import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host'
|
||||
import { buildWorktreeListingPage } from './worktree-listing-host-scope'
|
||||
import { buildWorktreeListingPage, listingKnownHostIds } from './worktree-listing-host-scope'
|
||||
import { readWorktreeMetaForHost } from '../persistence/host-qualified-worktree-meta'
|
||||
import { getRepoOwnedWorktreeMeta } from '../worktree-metadata-ownership'
|
||||
import type { WorktreeMeta } from '../../shared/worktree/meta-types'
|
||||
@@ -80,7 +80,7 @@ export class RuntimeManagedWorktreeQueries {
|
||||
throw new Error('invalid_limit')
|
||||
}
|
||||
const resolved = await this.deps.listResolved()
|
||||
const repoId = repoSelector ? (await this.deps.resolveRepo(repoSelector)).id : null
|
||||
const scopedRepo = repoSelector ? await this.deps.resolveRepo(repoSelector) : null
|
||||
const pathsByRepo = new Map<string, string[]>()
|
||||
for (const worktree of resolved) {
|
||||
const paths = pathsByRepo.get(worktree.repoId) ?? []
|
||||
@@ -100,12 +100,12 @@ export class RuntimeManagedWorktreeQueries {
|
||||
)
|
||||
const worktrees = resolved.filter(
|
||||
(worktree) =>
|
||||
(!repoId || worktree.repoId === repoId) &&
|
||||
(!scopedRepo || worktree.repoId === scopedRepo.id) &&
|
||||
this.isVisible(worktree, matchers.get(worktree.repoId), sourceDefaultsSupported)
|
||||
)
|
||||
// Why: a `--repo` listing was scoped by the caller, so naming every configured host as
|
||||
// omitted would report a gap the caller deliberately excluded.
|
||||
return buildWorktreeListingPage(worktrees, limit, repoId ? [] : this.deps.listKnownHostIds())
|
||||
// See `listingKnownHostIds`: a scoped listing must still name the host it was asked about.
|
||||
const knownHostIds = listingKnownHostIds(scopedRepo, () => this.deps.listKnownHostIds())
|
||||
return buildWorktreeListingPage(worktrees, limit, knownHostIds)
|
||||
}
|
||||
|
||||
resolveRepoForConnection(selector: string, connectionId?: string | null): Promise<Repo> {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { ExecutionHostId } from '../../shared/execution-host'
|
||||
import type { Repo } from '../../shared/repo-types'
|
||||
import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host'
|
||||
import { selectHostBalancedPage } from '../../shared/host-balanced-listing-page'
|
||||
import type { RuntimeListingHostScope } from '../../shared/runtime-listing-host-scope'
|
||||
|
||||
@@ -62,3 +63,28 @@ export function buildWorktreeListingHostScope(args: {
|
||||
}
|
||||
return { hostIds: [...covered].sort(), omittedHostIds: [...omitted].sort() }
|
||||
}
|
||||
|
||||
/**
|
||||
* Which hosts a listing claims to have been looking at.
|
||||
*
|
||||
* A `--repo` listing was scoped by the caller, so naming every configured host would report gaps
|
||||
* the caller deliberately excluded. Naming NONE — which is what a scoped listing did before — means
|
||||
* the scope can never report a gap at all, for any host kind, because `covered` and `omitted` are
|
||||
* both derived from the returned rows plus this list. A scoped listing whose scan did not succeed
|
||||
* then answers `{hostIds: [], omittedHostIds: []}`: byte-identical to a repo that genuinely has no
|
||||
* worktrees, which is the one thing docs/reference/ssh-execution-boundary.md forbids a listing from
|
||||
* implying.
|
||||
*
|
||||
* Measured before the fix, on one runtime with one refusing SSH host, in the same second: the
|
||||
* unscoped listing reported `omittedHostIds: ["local", "ssh:<target>"]` while the scoped listing
|
||||
* reported `[]`.
|
||||
*
|
||||
* Naming the single host the caller asked about costs nothing when rows come back — it lands in
|
||||
* `covered`, so it is never reported omitted — and is the whole answer when they do not.
|
||||
*/
|
||||
export function listingKnownHostIds(
|
||||
scopedRepo: Repo | null,
|
||||
listKnownHostIds: () => Iterable<ExecutionHostId>
|
||||
): Iterable<ExecutionHostId> {
|
||||
return scopedRepo ? [getRepoExecutionHostId(scopedRepo)] : listKnownHostIds()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user