This commit is contained in:
Neil
GitHub
parent
1d6d68e92f
commit
dfd97f069a
|
|
@ -0,0 +1,171 @@
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import {
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
readFileSync,
|
||||
readdirSync,
|
||||
rmSync,
|
||||
symlinkSync,
|
||||
writeFileSync
|
||||
} from 'fs'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
|
||||
// The service calls app.getPath('userData') for its overlay root. Point that
|
||||
// at a real tmp dir so we can exercise the filesystem behavior end-to-end.
|
||||
const userDataDir = mkdtempSync(join(tmpdir(), 'orca-pi-test-userdata-'))
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
app: {
|
||||
getPath: (name: string) => {
|
||||
if (name === 'userData') {
|
||||
return userDataDir
|
||||
}
|
||||
throw new Error(`unexpected app.getPath(${name})`)
|
||||
}
|
||||
}
|
||||
}))
|
||||
|
||||
import { PiTitlebarExtensionService, isSafeDescendCandidate } from './titlebar-extension-service'
|
||||
|
||||
describe('PiTitlebarExtensionService', () => {
|
||||
let piHome: string
|
||||
|
||||
beforeEach(() => {
|
||||
piHome = mkdtempSync(join(tmpdir(), 'orca-pi-test-pihome-'))
|
||||
// Seed a realistic Pi agent dir with skills, extensions, auth, sessions.
|
||||
mkdirSync(join(piHome, 'skills', 'my-skill', 'nested'), { recursive: true })
|
||||
writeFileSync(join(piHome, 'skills', 'my-skill', 'SKILL.md'), 'critical user skill')
|
||||
writeFileSync(join(piHome, 'skills', 'my-skill', 'nested', 'data.txt'), 'nested data')
|
||||
mkdirSync(join(piHome, 'extensions', 'user-ext'), { recursive: true })
|
||||
writeFileSync(join(piHome, 'extensions', 'user-ext', 'ext.ts'), 'user extension')
|
||||
mkdirSync(join(piHome, 'sessions'), { recursive: true })
|
||||
writeFileSync(join(piHome, 'sessions', 'session-1.json'), '{}')
|
||||
writeFileSync(join(piHome, 'auth.json'), 'secret token')
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(piHome, { recursive: true, force: true })
|
||||
rmSync(join(userDataDir, 'pi-agent-overlays'), { recursive: true, force: true })
|
||||
})
|
||||
|
||||
function expectPiHomeIntact(): void {
|
||||
expect(readFileSync(join(piHome, 'auth.json'), 'utf-8')).toBe('secret token')
|
||||
expect(readFileSync(join(piHome, 'skills', 'my-skill', 'SKILL.md'), 'utf-8')).toBe(
|
||||
'critical user skill'
|
||||
)
|
||||
expect(readFileSync(join(piHome, 'skills', 'my-skill', 'nested', 'data.txt'), 'utf-8')).toBe(
|
||||
'nested data'
|
||||
)
|
||||
expect(readFileSync(join(piHome, 'extensions', 'user-ext', 'ext.ts'), 'utf-8')).toBe(
|
||||
'user extension'
|
||||
)
|
||||
expect(readFileSync(join(piHome, 'sessions', 'session-1.json'), 'utf-8')).toBe('{}')
|
||||
}
|
||||
|
||||
it('buildPtyEnv mirrors the user Pi dir into an overlay under userData', () => {
|
||||
const svc = new PiTitlebarExtensionService()
|
||||
const env = svc.buildPtyEnv('pty-1', piHome)
|
||||
|
||||
expect(env.PI_CODING_AGENT_DIR).toBe(join(userDataDir, 'pi-agent-overlays', 'pty-1'))
|
||||
// Orca's titlebar extension is added alongside user extensions, not replacing them.
|
||||
const overlayExtensions = readdirSync(join(env.PI_CODING_AGENT_DIR!, 'extensions')).sort()
|
||||
expect(overlayExtensions).toEqual(['orca-titlebar-spinner.ts', 'user-ext'])
|
||||
// User's top-level resources are reachable via the overlay.
|
||||
expect(existsSync(join(env.PI_CODING_AGENT_DIR!, 'skills', 'my-skill', 'SKILL.md'))).toBe(true)
|
||||
expect(existsSync(join(env.PI_CODING_AGENT_DIR!, 'auth.json'))).toBe(true)
|
||||
expectPiHomeIntact()
|
||||
})
|
||||
|
||||
it('clearPty removes the overlay without touching the user Pi dir (issue #1083)', () => {
|
||||
const svc = new PiTitlebarExtensionService()
|
||||
svc.buildPtyEnv('pty-2', piHome)
|
||||
svc.clearPty('pty-2')
|
||||
|
||||
expect(existsSync(join(userDataDir, 'pi-agent-overlays', 'pty-2'))).toBe(false)
|
||||
// Critical regression guard: destroying the overlay MUST NOT destroy the
|
||||
// user's Pi home, even though every top-level entry in the overlay is a
|
||||
// symlink/junction pointing back into it.
|
||||
expectPiHomeIntact()
|
||||
})
|
||||
|
||||
it('rebuilding an overlay for the same ptyId does not corrupt the user Pi dir', () => {
|
||||
const svc = new PiTitlebarExtensionService()
|
||||
svc.buildPtyEnv('pty-3', piHome)
|
||||
svc.buildPtyEnv('pty-3', piHome)
|
||||
svc.buildPtyEnv('pty-3', piHome)
|
||||
expectPiHomeIntact()
|
||||
})
|
||||
|
||||
// Why: symlinkSync on Windows requires developer mode or admin — skip on
|
||||
// Windows rather than fail for environmental reasons. The isSafeDescendCandidate
|
||||
// unit tests above cover the Windows ordering invariant separately.
|
||||
it.skipIf(process.platform === 'win32')(
|
||||
'safely handles a pre-existing stale overlay with dangling symlinks',
|
||||
() => {
|
||||
// Why: simulate an overlay that was left behind by a prior Orca session,
|
||||
// where the original Pi home it mirrored has since moved. The teardown
|
||||
// should unlink the dangling symlinks in place without trying to follow them.
|
||||
const overlayDir = join(userDataDir, 'pi-agent-overlays', 'pty-4')
|
||||
mkdirSync(overlayDir, { recursive: true })
|
||||
symlinkSync('/nonexistent-pi-target/skills', join(overlayDir, 'skills'), 'dir')
|
||||
symlinkSync('/nonexistent-pi-target/auth.json', join(overlayDir, 'auth.json'), 'file')
|
||||
|
||||
const svc = new PiTitlebarExtensionService()
|
||||
const env = svc.buildPtyEnv('pty-4', piHome)
|
||||
|
||||
expect(env.PI_CODING_AGENT_DIR).toBe(overlayDir)
|
||||
expect(existsSync(join(overlayDir, 'skills', 'my-skill', 'SKILL.md'))).toBe(true)
|
||||
expectPiHomeIntact()
|
||||
}
|
||||
)
|
||||
|
||||
describe('isSafeDescendCandidate (Windows junction regression guard)', () => {
|
||||
// Why: the #1083 regression cannot reproduce on POSIX CI because
|
||||
// fs.rmSync({recursive:true}) handles symlinks correctly on macOS/Linux.
|
||||
// The behavior that DID cause the data loss on Windows was directory
|
||||
// junctions reporting BOTH isSymbolicLink() === true AND isDirectory()
|
||||
// === true from lstat/Dirent. These unit tests pin the predicate's
|
||||
// ordering so a future refactor cannot reverse it without the test suite
|
||||
// failing, regardless of which OS the tests run on.
|
||||
it('rejects a Windows directory junction (symlink + directory both true)', () => {
|
||||
const junctionLike = {
|
||||
isSymbolicLink: () => true,
|
||||
isDirectory: () => true
|
||||
}
|
||||
expect(isSafeDescendCandidate(junctionLike)).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects a plain symlink', () => {
|
||||
expect(isSafeDescendCandidate({ isSymbolicLink: () => true, isDirectory: () => false })).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
|
||||
it('rejects a regular file', () => {
|
||||
expect(
|
||||
isSafeDescendCandidate({ isSymbolicLink: () => false, isDirectory: () => false })
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('accepts a true directory (non-symlink)', () => {
|
||||
expect(isSafeDescendCandidate({ isSymbolicLink: () => false, isDirectory: () => true })).toBe(
|
||||
true
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
it('refuses to remove anything outside the overlay root', () => {
|
||||
// Why: hard guard against a misresolved overlay path (regression defense).
|
||||
// The overlay root is userData/pi-agent-overlays; any path outside it
|
||||
// must be a no-op, not a `rm -rf` on arbitrary filesystem locations.
|
||||
const svc = new PiTitlebarExtensionService() as unknown as {
|
||||
safeRemoveOverlay: (p: string) => void
|
||||
}
|
||||
svc.safeRemoveOverlay(piHome)
|
||||
svc.safeRemoveOverlay('/')
|
||||
svc.safeRemoveOverlay(join(userDataDir, 'pi-agent-overlays')) // root itself
|
||||
expectPiHomeIntact()
|
||||
})
|
||||
})
|
||||
|
|
@ -1,16 +1,17 @@
|
|||
import {
|
||||
cpSync,
|
||||
existsSync,
|
||||
lstatSync,
|
||||
linkSync,
|
||||
mkdirSync,
|
||||
readdirSync,
|
||||
rmSync,
|
||||
statSync,
|
||||
rmdirSync,
|
||||
symlinkSync,
|
||||
unlinkSync,
|
||||
writeFileSync
|
||||
} from 'fs'
|
||||
import { homedir } from 'os'
|
||||
import { basename, join } from 'path'
|
||||
import { basename, join, relative, resolve, sep } from 'path'
|
||||
import { app } from 'electron'
|
||||
|
||||
const ORCA_PI_EXTENSION_FILE = 'orca-titlebar-spinner.ts'
|
||||
|
|
@ -85,10 +86,17 @@ function getDefaultPiAgentDir(): string {
|
|||
}
|
||||
|
||||
function mirrorEntry(sourcePath: string, targetPath: string): void {
|
||||
const sourceStats = statSync(sourcePath)
|
||||
// Why: lstatSync (not statSync) so that if the user's Pi dir contains its
|
||||
// OWN symlinks (e.g. skills symlinked from ~/.agents/skills), we mirror the
|
||||
// link itself rather than resolving it to a type and then creating a junction
|
||||
// at an unrelated path. isSymbolicLink() MUST be checked before isDirectory()
|
||||
// on Windows because directory junctions/reparse points report both true.
|
||||
const sourceStats = lstatSync(sourcePath)
|
||||
const isSymlink = sourceStats.isSymbolicLink()
|
||||
const isDirectoryLike = !isSymlink && sourceStats.isDirectory()
|
||||
|
||||
if (process.platform === 'win32') {
|
||||
if (sourceStats.isDirectory()) {
|
||||
if (isDirectoryLike) {
|
||||
symlinkSync(sourcePath, targetPath, 'junction')
|
||||
return
|
||||
}
|
||||
|
|
@ -102,12 +110,112 @@ function mirrorEntry(sourcePath: string, targetPath: string): void {
|
|||
}
|
||||
}
|
||||
|
||||
symlinkSync(sourcePath, targetPath, sourceStats.isDirectory() ? 'dir' : 'file')
|
||||
symlinkSync(sourcePath, targetPath, isDirectoryLike ? 'dir' : 'file')
|
||||
}
|
||||
|
||||
// Exported for tests. A "descend candidate" is an entry whose children we
|
||||
// should recurse into when tearing down the overlay. Anything that is a
|
||||
// symlink (including a Windows directory junction) must NOT be a candidate
|
||||
// even if it also reports isDirectory() — following it would walk into the
|
||||
// link target and delete user data, which is the bug in #1083.
|
||||
export function isSafeDescendCandidate(stats: {
|
||||
isSymbolicLink(): boolean
|
||||
isDirectory(): boolean
|
||||
}): boolean {
|
||||
if (stats.isSymbolicLink()) {
|
||||
return false
|
||||
}
|
||||
return stats.isDirectory()
|
||||
}
|
||||
|
||||
export class PiTitlebarExtensionService {
|
||||
private getOverlayRoot(): string {
|
||||
return join(app.getPath('userData'), PI_OVERLAY_DIR_NAME)
|
||||
}
|
||||
|
||||
private getOverlayDir(ptyId: string): string {
|
||||
return join(app.getPath('userData'), PI_OVERLAY_DIR_NAME, ptyId)
|
||||
return join(this.getOverlayRoot(), ptyId)
|
||||
}
|
||||
|
||||
// Why: the overlay tree contains symlinks/junctions that point back into the
|
||||
// user's real Pi state (~/.pi/agent or $PI_CODING_AGENT_DIR). fs.rmSync with
|
||||
// { recursive: true } has repeatedly regressed on Windows when walking
|
||||
// NTFS junctions — it can follow them and delete the *target*, destroying
|
||||
// the user's skills, extensions, sessions, and auth.json. See issue #1083.
|
||||
//
|
||||
// Never descend into a symlink/junction here: for any non-real-directory
|
||||
// entry we unlink the link itself; only entries that are truly directories
|
||||
// on disk (our own extensions/ dir and the overlay root) are recursed into.
|
||||
// We also refuse to operate on any path outside the overlay root as a
|
||||
// last-line guard against PI_OVERLAY_DIR_NAME ever being mis-resolved.
|
||||
private safeRemoveOverlay(overlayDir: string): void {
|
||||
const overlayRoot = this.getOverlayRoot()
|
||||
const resolvedRoot = resolve(overlayRoot)
|
||||
const resolvedTarget = resolve(overlayDir)
|
||||
const rel = relative(resolvedRoot, resolvedTarget)
|
||||
if (rel === '' || rel.startsWith('..') || rel.includes(`..${sep}`)) {
|
||||
// Target is not strictly inside the overlay root — refuse to touch it.
|
||||
// Log so a misconfigured caller does not silently leak overlays forever
|
||||
// with no signal that this guard is firing.
|
||||
console.warn(
|
||||
`[pi-titlebar] refusing to remove overlay outside root: target=${resolvedTarget} root=${resolvedRoot}`
|
||||
)
|
||||
return
|
||||
}
|
||||
this.safeRemoveTree(resolvedTarget)
|
||||
}
|
||||
|
||||
private safeRemoveTree(path: string): void {
|
||||
let stat
|
||||
try {
|
||||
stat = lstatSync(path)
|
||||
} catch {
|
||||
return
|
||||
}
|
||||
|
||||
// Any symlink or Windows junction is unlinked in place, NEVER descended.
|
||||
// statSync would follow the link and report the target's stats, which is
|
||||
// exactly the bug we are guarding against, so the check uses lstat.
|
||||
//
|
||||
// On Windows, lstat on a directory junction can report BOTH
|
||||
// isSymbolicLink() === true AND isDirectory() === true, so we MUST check
|
||||
// isSymbolicLink first — otherwise a junction enters the recursive branch
|
||||
// and readdirSync enumerates the link's target, the exact bug in #1083.
|
||||
if (!isSafeDescendCandidate(stat)) {
|
||||
try {
|
||||
unlinkSync(path)
|
||||
} catch {
|
||||
// Best-effort: antivirus/indexers can hold handles briefly on Windows.
|
||||
// A leftover link is harmless; the next spawn rebuilds the overlay.
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
let entries
|
||||
try {
|
||||
entries = readdirSync(path, { withFileTypes: true })
|
||||
} catch {
|
||||
return
|
||||
}
|
||||
|
||||
for (const entry of entries) {
|
||||
const child = join(path, entry.name)
|
||||
if (isSafeDescendCandidate(entry)) {
|
||||
this.safeRemoveTree(child)
|
||||
continue
|
||||
}
|
||||
try {
|
||||
unlinkSync(child)
|
||||
} catch {
|
||||
// best-effort, see above
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
rmdirSync(path)
|
||||
} catch {
|
||||
// Directory may be non-empty if an unlink above failed; harmless.
|
||||
}
|
||||
}
|
||||
|
||||
private mirrorAgentDir(sourceAgentDir: string, overlayDir: string): void {
|
||||
|
|
@ -143,13 +251,13 @@ export class PiTitlebarExtensionService {
|
|||
const overlayDir = this.getOverlayDir(ptyId)
|
||||
|
||||
try {
|
||||
rmSync(overlayDir, { recursive: true, force: true })
|
||||
this.safeRemoveOverlay(overlayDir)
|
||||
} catch {
|
||||
// Why: on Windows the overlay directory can be locked by another process
|
||||
// (e.g. antivirus, indexer, or a previous Orca session that didn't clean up).
|
||||
// rmSync with force:true handles ENOENT but not EPERM/EBUSY. If we can't
|
||||
// remove the stale overlay, fall back to the user's own Pi agent dir so the
|
||||
// terminal still spawns — the titlebar spinner is not worth blocking the PTY.
|
||||
// If we can't remove the stale overlay, fall back to the user's own Pi agent
|
||||
// dir so the terminal still spawns — the titlebar spinner is not worth
|
||||
// blocking the PTY.
|
||||
return existingAgentDir ? { PI_CODING_AGENT_DIR: existingAgentDir } : {}
|
||||
}
|
||||
|
||||
|
|
@ -180,7 +288,7 @@ export class PiTitlebarExtensionService {
|
|||
|
||||
clearPty(ptyId: string): void {
|
||||
try {
|
||||
rmSync(this.getOverlayDir(ptyId), { recursive: true, force: true })
|
||||
this.safeRemoveOverlay(this.getOverlayDir(ptyId))
|
||||
} catch {
|
||||
// Why: on Windows the overlay dir can be locked (EPERM/EBUSY) by antivirus
|
||||
// or indexers. Overlay cleanup is best-effort — a stale directory in userData
|
||||
|
|
|
|||
Loading…
Reference in New Issue