mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
Lesley Murfin
946dacc65d
* test(worktrees): cover local worktree owner routing with saved runtimes (#16733) A local git worktree whose rows carry no host stamp fails every owner-routed operation closed as soon as any runtime environment is saved, however unrelated. resolveWorktreeOperationRouteResult establishes positive identity from the worktree/repo catalogs, then discards it: with no runtime active the only exit is the legacy-local gate, which demands an empty saved-runtime list. One saved environment makes that false and the call returns { kind: 'missing' }. These tests state the contract before the fix, so the claim that the fix is purely additive can be checked rather than asserted. Committed red on purpose. Observed at5631aa00dd(vitest run, both files): Tests 7 failed | 52 passed (59) The 7 failing are exactly the states that must become local, plus their two consumers: - an unrelated runtime is saved (the reported bug) - several unrelated runtimes are saved - the repo is known before its worktree row is listed - the saved-runtime catalog has not hydrated - an unrelated runtime was removed - resolveTerminalWorktreeRoute on such a worktree (the gate in front of the "Terminal creation is unavailable" reply) - the folder/worktree parity state: identical store, folder local, worktree missing The other 52 pass now and must keep passing: connection-owned and runtime-stamped repos never route local, a stamped worktree row still outranks its repo, contradictory repo rows stay ambiguous, an ambiguous or hydrating runtime focus still fails closed, and a genuinely unknown id still fails closed. That set is the additive-only guarantee. * test(worktrees): re-aim two fail-closed cases at genuinely missing owners (#16733) Two cases in src/renderer/src/lib/worktree-operation-route.test.ts assert the behaviour #16733 reports as the bug, so they have to move: - :158 'fails a paired-client ownerless stale publication closed instead of routing it locally' - :171 'fails ownerless rows closed until the saved-runtime catalog is hydrated' Both arrived with #9994 (41751dd90d, route HUB-owned SSH worktrees through their owning runtime), whose stated goal was to fail closed for missing or stale owners. That goal is right and is kept. The premise being rebutted is narrower: neither fixture describes a missing or stale owner. Each carries a present repo row that is merely unstamped -- repos: [{ id: 'repo-1' }] -- and three places in this codebase already read exactly that row as locally owned: - shared/execution-host.ts getRepoExecutionHostId returns LOCAL_EXECUTION_HOST_ID - main/ipc/worktrees/listing/worktree-host-ownership.ts resolveRepoOwnershipEvidence falls back to LOCAL_EXECUTION_HOST_ID, and the listing and removal paths trust it - shared/repo-types.ts documents executionHostId as the field runtime-host repos need precisely because they otherwise look identical to local repos attribute. It swept in the legacy-local case because at the time nothing in this resolver consulted the repo index for an unstamped row. So each case is re-aimed at the state it was actually defending, and neither is deleted -- the fail-closed coverage is not reduced, it is pointed at a real missing owner: - the first becomes 'fails a paired-client publication closed when no repo row can own it': same runtime state, repos: []. A worktree row alone is not host evidence, so this still returns missing, before and after the fix. - the second becomes 'fails ownerless rows closed mid-hydration while a saved runtime could own them': an active runtime with an ambiguous saved catalog during hydration. This returns missing from the active-runtime branch and is untouched by the fix. It is worded to stay distinct from the neighbouring case at :185, which already covers focus-is-not-ownership with an empty catalog, rather than duplicating it. The states these two cases vacate are re-asserted with their corrected expected result in the #16733 block added by the previous commit. Suite unchanged at 7 failed | 52 passed (59): the rewrites pass, the 7 reds are still the 7 states the fix must convert. * fix(worktrees): keep unstamped local worktrees routable when runtimes are saved (#16733) resolveWorktreeOperationRouteResult establishes positive identity from the worktree and repo catalogs, then discards it. With no runtime active the only exit is the legacy-local gate, which requires an empty saved-runtime list, so one saved runtime environment -- connected or not, related or not -- made it false and the call returned { kind: 'missing' }. Every owner-routed operation on a genuinely local git worktree then failed closed, and because resolveTerminalWorktreeRoute is the sole gate in front of terminal-request-ipc-bridge.ts, the user saw "Terminal creation is unavailable because the worktree owner could not be resolved". Folder workspaces hit the same gate and were carved out in #10251/#10269, whose comment in this file states the principle and names this exact failure mode: a found record is positive identity evidence, and the worktree legacy hydration gates "would fail local folders closed whenever unrelated runtimes exist". Git worktrees never got the equivalent. This adds it, in the same shape and the same function. The rule is not new. An unstamped repo row is read as locally owned by getRepoExecutionHostId, by main's resolveRepoOwnershipEvidence, and by Repo.executionHostId's own documentation; and the repo write path (repoWithFetchedOwner) stamps runtime: and ssh: owners at fetch time, so an unstamped row is a legacy row that predates owner projection -- local by construction. The router now consults that evidence instead of contradicting it. The sidebar already rendered these worktrees as Local; this removes the disagreement rather than adding a heuristic. Four properties this change holds to: 1. The branch sits after the active-runtime block, so an unambiguous active runtime still wins (routes runtime:<id>, not local) and an ambiguous or mid-hydration focus still returns missing. That ordering is structural, not incidental. 2. mayBeLegacyLocal is left byte-identical (verified: both 7-line hunks hash to f0ed1b4287c646cb). The new branch does take over the two states where a local repo row exists and no runtime is saved, but returns the identical local route, so no input changes its answer -- only which branch produced it. 3. The helper returns null on anything but unanimous local, so the branch can only ever convert missing into local. It never returns ambiguous: a contradiction between repo rows is already decided upstream by resolveExplicitWorktreeOperationRouteResult, and answering it here would be a second, divergent authority. 4. It reads neither runtimeEnvironmentCatalogHydrated nor removedRuntimeEnvironmentIds. That is sound rather than merely convenient, because it consults host evidence rather than runtime-environment inference: a runtime-owned repo row is stamped runtime:<id> at fetch time, so neither an unhydrated runtime catalog nor a removed environment can turn an unstamped row into a remote one. Control only reaches this point after the explicit catalog resolver returned missing, which means every worktree row and every repo row for this id is unstamped -- any stamped row routes ssh: or runtime: earlier, and two disagreeing rows return ambiguous earlier. There is no remote-owned state left here to leak. Out of scope, deliberately: who wins when a runtime is focused (#11512), and back-filling Worktree.hostId at creation time, which is a persistence migration over worktreeMeta and does nothing for the users already carrying unstamped rows. The 7 cases red in the two preceding commits now pass; the 52 that guard the fail-closed contract are unchanged. Test Files 2 passed (2) Tests 59 passed (59) * test(worktrees): defer repo-row-only routing to #16841's fail-closed rule (#16733) Upstream #16841 (mergedd3475957f3) landed its own fix for #16733 and drew the positive-identity line one notch tighter than this branch did: its 'does not treat a repo row alone as positive local identity' case asserts that a worktree id no row has ever listed stays `missing`, even when the repo row for its repoId is local. This branch's 'routes a known local repo before its worktree row has been listed' asserted the opposite result for that identical state, so the two cannot both hold. Main's rule is the safer reading — a repo row is repo identity, not worktree identity — so the reconciled code gates resolveUnstampedLocalWorktreeRoute on hasKnownWorktree and this case is dropped rather than re-pinned. Every state that actually reproduces #16733 keeps a worktree row (listed or detected), so the reported bug and both extra fail-closed edge cases this branch fixes are unaffected. * refactor(worktrees): drop the unreachable disagreement loop in resolveUnstampedLocalWorktreeRoute resolveWorktreeOperationRouteResult only calls resolveUnstampedLocalWorktreeRoute after resolveExplicitWorktreeOperationRouteResult has already returned 'missing' for this repoId. That function (worktree-operation-catalog-route.ts) indexes every repo row carrying a non-empty executionHostId or connectionId and resolves/ambiguous-es on any of them, so by construction every row resolveUnstampedLocalWorktreeRoute ever sees is unstamped -- and getRepoExecutionHostId's own fallback (shared/execution-host.ts) always resolves an unstamped row to local. The per-row disagreement check could never actually return null; it was dead defensive code describing a state the caller's short-circuit already rules out. Reduced to an existence check with identical behavior (verified: same 66/66 tests, same mutation-proof property -- reverting only this file still fails exactly the same 6 tests it did before). Also harmonized a same-function 'local' string literal to the LOCAL_EXECUTION_HOST_ID constant already in use one branch above it, and dropped a dangling getWorktreeExecutionHostId doc reference the shipped code never actually calls. * docs(routing): document the undocumented worktree operation route helpers