mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
fix(hosts): resolve the host in candidate selection too, not just in resolution
The first pass fixed how a repo row is classified once it reaches `resolveFolderWorkspaceHost`. The candidate filter decides which rows reach it at all, and it read `repo.connectionId` raw as well — so an SSH-only row outside the project-group subtree was dropped before the new logic could see it, and the execute-here bug survived for the population the fix was for, via a different path. Found in review by CodeRabbit. Three repo-row reads had the same root cause, not one: - the scope-connection filter, comparing a path repo's raw field against the workspace/group connection; - the group-connection set, built from group repos' raw fields; - that set's membership test against path repos' raw fields. The last two are one comparison with the mismatch on either side, so resolving only the path side would have reintroduced it from the other direction. All three, plus the resolution loop, now go through one `getRepoScopeConnectionId` helper. Non-SSH hosts still fall back to the raw field, so a `runtime:` row keeps contributing its nested target exactly as before. The new tests use a repo matched only by path, outside the subtree — the population every existing test missed, which is why four passing revert-tests did not catch this. One of them is labelled as pinning the resolver rather than the filter: under the old raw read both rows came back connectionless and matched each other by accident, so it survives a filter revert and must not be counted as coverage for it.
This commit is contained in:
@@ -284,6 +284,84 @@ describe('folder workspace execution host', () => {
|
||||
expect(resolved).toEqual({ kind: 'local' })
|
||||
})
|
||||
|
||||
// The candidate FILTER decides which rows reach the resolver, and it read `repo.connectionId` raw
|
||||
// too — so an SSH-only repo outside the project-group subtree was dropped before any of the above
|
||||
// could classify it. Every test before this one uses a repo inside the subtree, which is never
|
||||
// filtered, so none of them could have caught it (found in review by CodeRabbit).
|
||||
describe('a repo matched only by path, outside the project-group subtree', () => {
|
||||
const sshOnlyPathRepo = repo({
|
||||
id: 'repo-path',
|
||||
path: '/work/app/nested',
|
||||
executionHostId: 'ssh:box'
|
||||
})
|
||||
|
||||
it('survives the scope-connection filter instead of being dropped as connectionless', () => {
|
||||
const scoped = state({
|
||||
folderWorkspaces: [workspace({ connectionId: 'box' })],
|
||||
repos: [sshOnlyPathRepo]
|
||||
})
|
||||
|
||||
expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toEqual([sshOnlyPathRepo])
|
||||
expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' })
|
||||
})
|
||||
|
||||
// Pins the resolver, not the filter: under the old raw read BOTH rows came back connectionless,
|
||||
// so they matched each other by accident and this case survived the filter either way. The
|
||||
// legacy-vs-unified pairing below is the one that discriminates.
|
||||
it('survives the group-connection filter when the group is on that same SSH host', () => {
|
||||
const scoped = state({
|
||||
repos: [
|
||||
repo({
|
||||
id: 'repo-group',
|
||||
path: '/work/app/group',
|
||||
projectGroupId: 'group-1',
|
||||
executionHostId: 'ssh:box'
|
||||
}),
|
||||
sshOnlyPathRepo
|
||||
]
|
||||
})
|
||||
|
||||
expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toHaveLength(2)
|
||||
expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' })
|
||||
})
|
||||
|
||||
// Both sides of the group comparison are resolved, so the legacy spelling on one side and the
|
||||
// unified spelling on the other still match.
|
||||
it('matches a legacy-spelled group repo against a unified-spelled path repo', () => {
|
||||
const scoped = state({
|
||||
repos: [
|
||||
repo({
|
||||
id: 'repo-group',
|
||||
path: '/work/app/group',
|
||||
projectGroupId: 'group-1',
|
||||
connectionId: 'box'
|
||||
}),
|
||||
sshOnlyPathRepo
|
||||
]
|
||||
})
|
||||
|
||||
expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toHaveLength(2)
|
||||
expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' })
|
||||
})
|
||||
|
||||
// A `runtime:` row's nested target is still read from the raw field, so it matches a scope
|
||||
// connection exactly as it does today. Pinned so the carve-out stays a decision.
|
||||
it('leaves a runtime row matching the scope connection through its nested target', () => {
|
||||
const runtimePathRepo = repo({
|
||||
id: 'repo-path',
|
||||
path: '/work/app/nested',
|
||||
executionHostId: 'runtime:env-1',
|
||||
connectionId: 'box'
|
||||
})
|
||||
const scoped = state({
|
||||
folderWorkspaces: [workspace({ connectionId: 'box' })],
|
||||
repos: [runtimePathRepo]
|
||||
})
|
||||
|
||||
expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toEqual([runtimePathRepo])
|
||||
})
|
||||
})
|
||||
|
||||
it('reads each repository membership once while collecting candidates', () => {
|
||||
let membershipReads = 0
|
||||
const repos = Array.from({ length: 32 }, (_, index) => {
|
||||
|
||||
@@ -36,6 +36,24 @@ export function normalizeConnectionId(value: string | null | undefined): string
|
||||
return value?.trim() || null
|
||||
}
|
||||
|
||||
/**
|
||||
* The SSH target whose filesystem holds this repo's files, or `null` for anything else.
|
||||
*
|
||||
* SSH ownership has two spellings on a repo row — the legacy `connectionId` field and the unified
|
||||
* `executionHostId` — so reading the raw field sees only one of them and a row carrying only
|
||||
* `executionHostId: 'ssh:<target>'` reads as if it had no connection at all. Every comparison in
|
||||
* this file goes through here: the candidate filters decide which rows reach the resolver, so
|
||||
* reading raw in either place drops the row before the resolver can classify it.
|
||||
*
|
||||
* A non-SSH host falls back to the raw field so a `runtime:` row keeps contributing its nested
|
||||
* target exactly as it does today. That target is not this client's to dial, but changing it is a
|
||||
* separate defect with its own reasoning — see the note in `resolveFolderWorkspaceHost`.
|
||||
*/
|
||||
function getRepoScopeConnectionId(repo: Repo): string | null {
|
||||
const host = parseExecutionHostId(getRepoExecutionHostId(repo))
|
||||
return host?.kind === 'ssh' ? host.targetId : normalizeConnectionId(repo.connectionId)
|
||||
}
|
||||
|
||||
function getFolderScopeCandidateRepos(args: {
|
||||
folderPath: string
|
||||
projectGroupId: string
|
||||
@@ -59,18 +77,18 @@ function getFolderScopeCandidateRepos(args: {
|
||||
if (args.connectionId) {
|
||||
return [
|
||||
...groupRepos,
|
||||
...pathRepos.filter((repo) => normalizeConnectionId(repo.connectionId) === args.connectionId)
|
||||
...pathRepos.filter((repo) => getRepoScopeConnectionId(repo) === args.connectionId)
|
||||
]
|
||||
}
|
||||
if (groupRepos.length === 0) {
|
||||
return pathRepos
|
||||
}
|
||||
const groupConnectionIds = new Set(
|
||||
groupRepos.map((repo) => normalizeConnectionId(repo.connectionId))
|
||||
)
|
||||
// Both sides resolved: comparing a resolved path repo against a raw group read would reintroduce
|
||||
// the same mismatch from the other direction.
|
||||
const groupConnectionIds = new Set(groupRepos.map(getRepoScopeConnectionId))
|
||||
return [
|
||||
...groupRepos,
|
||||
...pathRepos.filter((repo) => groupConnectionIds.has(normalizeConnectionId(repo.connectionId)))
|
||||
...pathRepos.filter((repo) => groupConnectionIds.has(getRepoScopeConnectionId(repo)))
|
||||
]
|
||||
}
|
||||
|
||||
@@ -120,17 +138,7 @@ export function resolveFolderWorkspaceHost(
|
||||
let hasLocalRepo = false
|
||||
const connectionIds = new Set<string>()
|
||||
for (const repo of candidateRepos) {
|
||||
// Why not `repo.connectionId` alone: SSH ownership has two spellings on a repo row, and a row
|
||||
// carrying only `executionHostId: 'ssh:<target>'` has no `connectionId` to read. Reading the raw
|
||||
// field counted it as a local repo, so a folder workspace whose files live on an SSH host
|
||||
// resolved `local` — an execute-here answer for a remote path (#11163).
|
||||
//
|
||||
// Resolve the host first, then read the target off it. Every other row keeps its existing
|
||||
// contribution, including a `runtime:` row's nested target: that is not this client's to dial,
|
||||
// but narrowing it here would be a second behaviour change riding on this one.
|
||||
const host = parseExecutionHostId(getRepoExecutionHostId(repo))
|
||||
const connectionId =
|
||||
host?.kind === 'ssh' ? host.targetId : normalizeConnectionId(repo.connectionId)
|
||||
const connectionId = getRepoScopeConnectionId(repo)
|
||||
if (connectionId) {
|
||||
connectionIds.add(connectionId)
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user