fix(cwd): resolve detached session cwd via persistence index, not host process cwd (#372)
* fix(cwd): resolve detached session cwd via persistence index, not host process cwd
sessionCwdOf fell back to process.cwd() when a session was not yet
attached and the client did not send a cwd — on Windows the host
process cwd is the DSH source root (dsh.cmd's pushd %DSH_ROOT%),
so every user-project path failed the workspace containment guard
with 'path "..." is outside workspace'. The bug surfaced when
clicking chat file links at first paint (session still hydrating)
or any flow that hit a sidebar route before the client's
fetchedCwd effect resolved.
Fix: thread ctx.get('sessionPersistence') as the fallback between
the client cwd and the host process cwd. The persistence index
holds every persisted session's header (including cwd), so a cold
(detached) session resolves its real project cwd instead of the
DSH source root. The host process cwd remains the final fallback
for deployments without persistence (tests / stripped-down hosts);
production always provides persistence.
sessionCwdOf is now async; all 8 call sites await it. The two tool
resolvers (registerTools / registerOpenTool) take an async
resolveCwd callback; the one execute path that used it
(terminal_create) is now async.
Tests: two new regression cases in smoke.spec.ts cover the cold
session via persistence path and the persistence-without-cwd
fallback. Existing stubs updated to the async resolveCwd signature.
* Address Copilot review: fix JSDoc accuracy and validate persistence cwd
- tools.ts / agent-opens.ts: correct the resolveCwd JSDoc — the resolver
does fall back to the host process cwd as a final tier (tests /
stripped-down hosts); the previous "never falls back" wording
contradicted the actual sessionCwdOf contract.
- index.ts: route the persistence meta.cwd through requireAbsolute,
matching the clientCwd path. A buggy/corrupt persistence layer that
stored a relative cwd would otherwise flow into the workspace guard,
be resolved against the host process cwd, and potentially recreate
the original "outside workspace" misclassification in a harder-to-
debug way.
- smoke.spec.ts: use resolvePath() for the cold-cwd fixture so it is
absolute on both POSIX and Windows; add a regression test for the
relative-cwd rejection path.
This commit is contained in:
+6
-3
@@ -188,14 +188,17 @@ function isWindowsDrivePrefix(raw: string): boolean {
|
||||
* own routes do.
|
||||
* @param ctx - host plugin context (carries the tools service).
|
||||
* @param registry - the open-request registry (per-session queue + views).
|
||||
* @param resolveCwd - live cwd resolver for one session id.
|
||||
* @param resolveCwd - async cwd resolver for one session id. Resolves through
|
||||
* the session header, the client-supplied cwd, and the persistence index
|
||||
* before falling back to the host process cwd (production always provides
|
||||
* persistence, so the fallback is reached only in tests / stripped-down hosts).
|
||||
* @param readPrefs - live resolved side card prefs (for tab enable gates).
|
||||
* @returns a disposer that unregisters the tool.
|
||||
*/
|
||||
export function registerOpenTool(
|
||||
ctx: Context,
|
||||
registry: AgentOpenRegistry,
|
||||
resolveCwd: (sessionId: string) => string,
|
||||
resolveCwd: (sessionId: string) => Promise<string>,
|
||||
readPrefs: () => SidebarPrefs,
|
||||
): () => void {
|
||||
return ctx.tools.register(defineTool({
|
||||
@@ -245,7 +248,7 @@ export function registerOpenTool(
|
||||
execute: async (args: { target: string; title?: string }, exec: ToolRunContext) => {
|
||||
exec.signal.throwIfAborted()
|
||||
const sessionId = sessionIdOf(exec)
|
||||
const cwd = resolveCwd(sessionId)
|
||||
const cwd = await resolveCwd(sessionId)
|
||||
const { kind, target, title: defaultTitle } = await classifyTarget(args.target, cwd)
|
||||
// A disabled target tab type would make the client no-op the open:
|
||||
// report the real cause to the model instead of a silent success.
|
||||
|
||||
+35
-18
@@ -105,12 +105,17 @@ export function mediaTypeForPath(path: string): string {
|
||||
* header wins; while the session is still hydrating from persistence (the
|
||||
* web client attaches the current conversation a moment after page load, so
|
||||
* the very first sidebar requests can arrive detached) the caller's own
|
||||
* list-summary cwd is used; the process cwd is the last resort (blank
|
||||
* sessions have no cwd anywhere yet). Never throws for a missing cwd, so
|
||||
* explorer/git/terminal work from first paint instead of surfacing
|
||||
* "session ... has no working directory".
|
||||
* list-summary cwd is used; the session-persistence index is queried as a
|
||||
* last resort for cold (not-yet-attached) sessions so a detached first
|
||||
* request still resolves the correct project instead of the host process
|
||||
* cwd (which on Windows is the DSH source root after `dsh.cmd`'s `pushd`,
|
||||
* causing every user-project path to be misclassified as "outside
|
||||
* workspace"). The host process cwd is the FINAL fallback for deployments
|
||||
* without persistence (tests / stripped-down hosts); production always
|
||||
* provides persistence, so the bug-fix path (header → client → persistence)
|
||||
* always resolves the real session cwd before reaching it.
|
||||
*/
|
||||
function sessionCwdOf(ctx: Context, sessionId: string, clientCwd?: string): string {
|
||||
async function sessionCwdOf(ctx: Context, sessionId: string, clientCwd?: string): Promise<string> {
|
||||
const session = ctx.sessions.get(sessionId)
|
||||
const headerCwd = session?.header.cwd
|
||||
if (headerCwd !== undefined && headerCwd !== '') return headerCwd
|
||||
@@ -121,6 +126,18 @@ function sessionCwdOf(ctx: Context, sessionId: string, clientCwd?: string): stri
|
||||
throw new SidebarError('bad-request', `invalid working directory "${clientCwd}"`)
|
||||
}
|
||||
}
|
||||
const persistence = ctx.get('sessionPersistence')
|
||||
if (persistence !== undefined) {
|
||||
const inspected = await persistence.inspect(sessionId)
|
||||
const metaCwd = inspected.meta.cwd
|
||||
if (metaCwd !== undefined && metaCwd !== '') {
|
||||
try {
|
||||
return requireAbsolute(metaCwd)
|
||||
} catch {
|
||||
throw new SidebarError('bad-request', `invalid working directory "${metaCwd}"`)
|
||||
}
|
||||
}
|
||||
}
|
||||
return process.cwd()
|
||||
}
|
||||
|
||||
@@ -268,16 +285,16 @@ function buildApi(
|
||||
terminalShell: string,
|
||||
getSettings: () => SidebarSettingsFace | undefined,
|
||||
): Record<string, ApiMethod> {
|
||||
const cwdOf = (payload: unknown): { sessionId: string; cwd: string } => {
|
||||
const cwdOf = async (payload: unknown): Promise<{ sessionId: string; cwd: string }> => {
|
||||
const sessionId = requireString(payload, 'sessionId')
|
||||
const record = payload as { cwd?: unknown } | null
|
||||
const clientCwd = typeof record?.cwd === 'string' && record.cwd !== '' ? record.cwd : undefined
|
||||
return { sessionId, cwd: sessionCwdOf(ctx, sessionId, clientCwd) }
|
||||
return { sessionId, cwd: await sessionCwdOf(ctx, sessionId, clientCwd) }
|
||||
}
|
||||
/** Resolve the optional Git-panel checkout selector against the authoritative
|
||||
* session repository. Unlike `cwd`, `worktree` is never trusted directly. */
|
||||
const gitCwdOf = async (payload: unknown): Promise<{ sessionId: string; cwd: string }> => {
|
||||
const base = cwdOf(payload)
|
||||
const base = await cwdOf(payload)
|
||||
const record = payload as { worktree?: unknown } | null
|
||||
const requested = typeof record?.worktree === 'string' && record.worktree !== '' ? record.worktree : undefined
|
||||
return { sessionId: base.sessionId, cwd: await git.resolveWorktree(base.cwd, requested) }
|
||||
@@ -293,12 +310,12 @@ function buildApi(
|
||||
// subagent runtime is absent (the page has no topology to show anyway).
|
||||
const subagentLiveApi: SidebarSubagentLiveRoutes = buildSubagentLiveApi(ctx)
|
||||
return {
|
||||
'session.cwd': (payload) => {
|
||||
const { sessionId, cwd } = cwdOf(payload)
|
||||
'session.cwd': async (payload) => {
|
||||
const { sessionId, cwd } = await cwdOf(payload)
|
||||
return { sessionId, cwd, root: rootLabel(cwd), parent: parentOf(cwd) ?? null }
|
||||
},
|
||||
'fs.tree': async (payload) => {
|
||||
const { cwd } = cwdOf(payload)
|
||||
const { cwd } = await cwdOf(payload)
|
||||
const record = payload as { path?: unknown }
|
||||
const target = record.path === undefined ? cwd : await ensureWorkspacePath(cwd, requireString(payload, 'path'))
|
||||
return listDirectory(target, resolved.listLimit)
|
||||
@@ -307,12 +324,12 @@ function buildApi(
|
||||
// The editor side panel's global name search: rooted at the session
|
||||
// cwd (not caller-targetable — the walk is unbounded by design and
|
||||
// must never escape the workspace), budgeted inside searchFiles.
|
||||
const { cwd } = cwdOf(payload)
|
||||
const { cwd } = await cwdOf(payload)
|
||||
const query = requireString(payload, 'query')
|
||||
return searchFiles(cwd, query)
|
||||
},
|
||||
'fs.read': async (payload) => {
|
||||
const { cwd } = cwdOf(payload)
|
||||
const { cwd } = await cwdOf(payload)
|
||||
// Relative paths are git-derived (status/diff report repo-root-relative
|
||||
// names; the untracked diff view reads the file through this route). A
|
||||
// child-repo path is relative to the selected repoRoot, not the session
|
||||
@@ -324,7 +341,7 @@ function buildApi(
|
||||
return { kind: 'text', content, truncated }
|
||||
},
|
||||
'fs.write': async (payload) => {
|
||||
const { cwd } = cwdOf(payload)
|
||||
const { cwd } = await cwdOf(payload)
|
||||
const path = await ensureWorkspaceWritePath(cwd, requireString(payload, 'path'))
|
||||
const content = requireString(payload, 'content')
|
||||
const tmp = `${path}.dsh-sidebar-tmp-${process.pid}`
|
||||
@@ -814,7 +831,7 @@ export function apply(ctx: Context, config?: SidebarConfig): void {
|
||||
if (sessionId === null || dir === null || relativePath === null || relativePath.trim() === '') {
|
||||
throw new SidebarError('bad-request', 'sessionId, dir, and relativePath are required')
|
||||
}
|
||||
const cwd = sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
const cwd = await sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
const { path, size } = await writeWorkspaceUpload({
|
||||
cwd,
|
||||
dir,
|
||||
@@ -855,7 +872,7 @@ export function apply(ctx: Context, config?: SidebarConfig): void {
|
||||
const sessionId = url.searchParams.get('sessionId')
|
||||
const raw = url.searchParams.get('path')
|
||||
if (sessionId === null || raw === null) throw new SidebarError('bad-request', 'sessionId and path are required')
|
||||
const cwd = sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
const cwd = await sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
const path = await ensureWorkspacePath(cwd, raw)
|
||||
const info = await stat(path)
|
||||
if (!info.isFile() || info.size > resolved.mediaLimit) {
|
||||
@@ -914,7 +931,7 @@ export function apply(ctx: Context, config?: SidebarConfig): void {
|
||||
// back to the process cwd and is normally refused by the workspace
|
||||
// real-path guard, with the same semantics as the media route's
|
||||
// fallback.
|
||||
const cwd = sessionCwdOf(ctx, sessionId)
|
||||
const cwd = await sessionCwdOf(ctx, sessionId)
|
||||
const absolute = await ensureWorkspacePath(cwd, path)
|
||||
const info = await stat(absolute)
|
||||
if (!info.isFile() || info.size > resolved.mediaLimit) {
|
||||
@@ -1131,7 +1148,7 @@ async function attachTerminal(
|
||||
ws.close(1011, PTY_DEPS_MISSING)
|
||||
return
|
||||
}
|
||||
const cwd = sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
const cwd = await sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined)
|
||||
// Settings-page shell overrides win over the yaml/auto shell for
|
||||
// terminals opened from now on (existing pty handles keep their shell).
|
||||
const overrides = shellOverridesOf(getSettings)
|
||||
|
||||
+8
-5
@@ -73,14 +73,17 @@ function sessionIdOf(exec: ToolRunContext): string {
|
||||
* session's terminals.
|
||||
* @param ctx - host plugin context (carries the tools service).
|
||||
* @param registry - the agent-owned terminal registry.
|
||||
* @param resolveCwd - live cwd resolver for one session id.
|
||||
* @param resolveCwd - async cwd resolver for one session id. Resolves through
|
||||
* the session header, the client-supplied cwd, and the persistence index
|
||||
* before falling back to the host process cwd (production always provides
|
||||
* persistence, so the fallback is reached only in tests / stripped-down hosts).
|
||||
* @returns a disposer that unregisters all eight tools (the caller gates
|
||||
* registration on the side-card setting and calls this to turn them off).
|
||||
*/
|
||||
export function registerTools(
|
||||
ctx: Context,
|
||||
registry: AgentPtyRegistry,
|
||||
resolveCwd: (sessionId: string) => string,
|
||||
resolveCwd: (sessionId: string) => Promise<string>,
|
||||
readShellOverrides: () => { shell?: string; shellArgs?: string[] },
|
||||
): () => void {
|
||||
const disposers: Array<() => void> = []
|
||||
@@ -124,13 +127,13 @@ export function registerTools(
|
||||
`Opened terminal "${v.title}" (uuid: ${v.uuid}). The sidebar tab appears automatically; use terminal_read to see output and terminal_send (with submit=true) to run more commands.`,
|
||||
),
|
||||
},
|
||||
execute: (args: { title: string; command: string }, exec) => {
|
||||
execute: async (args: { title: string; command: string }, exec) => {
|
||||
exec.signal.throwIfAborted()
|
||||
const sessionId = sessionIdOf(exec)
|
||||
const cwd = resolveCwd(sessionId)
|
||||
const cwd = await resolveCwd(sessionId)
|
||||
const { shell, shellArgs } = readShellOverrides()
|
||||
const uuid = registry.create(sessionId, args.title, args.command, cwd, 80, 24, shell, shellArgs)
|
||||
return Promise.resolve({ uuid, title: args.title })
|
||||
return { uuid, title: args.title }
|
||||
},
|
||||
}))
|
||||
|
||||
|
||||
@@ -28,7 +28,7 @@ function exec(sessionId: string): ToolRunContext {
|
||||
}
|
||||
|
||||
/** Register the tool against a fake service + real registry. */
|
||||
function mount(overrides: { prefs?: Partial<SidebarPrefs>; resolveCwd?: (sessionId: string) => string } = {}) {
|
||||
function mount(overrides: { prefs?: Partial<SidebarPrefs>; resolveCwd?: (sessionId: string) => Promise<string> } = {}) {
|
||||
const captured: CapturedTool[] = []
|
||||
let disposeCount = 0
|
||||
const ctx = {
|
||||
@@ -44,7 +44,7 @@ function mount(overrides: { prefs?: Partial<SidebarPrefs>; resolveCwd?: (session
|
||||
const dispose = registerOpenTool(
|
||||
ctx,
|
||||
registry,
|
||||
overrides.resolveCwd ?? (() => '/cwd'),
|
||||
overrides.resolveCwd ?? (async () => '/cwd'),
|
||||
() => prefs,
|
||||
)
|
||||
return { captured, registry, prefs, dispose: () => { dispose(); return disposeCount } }
|
||||
@@ -135,7 +135,7 @@ describe('sidebar_open tool', () => {
|
||||
try {
|
||||
const file = join(dir, 'a.txt')
|
||||
writeFileSync(file, 'hello')
|
||||
const { captured, registry } = mount({ resolveCwd: () => dir })
|
||||
const { captured, registry } = mount({ resolveCwd: async () => dir })
|
||||
const received: unknown[] = []
|
||||
const detach = registry.attach('s1', (request) => { received.push(request) })
|
||||
const tool = toolOf(captured, 'sidebar_open')
|
||||
@@ -153,7 +153,7 @@ describe('sidebar_open tool', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'sidebar-open-'))
|
||||
try {
|
||||
writeFileSync(join(dir, 'note.md'), '# hi')
|
||||
const { captured, registry } = mount({ resolveCwd: () => dir })
|
||||
const { captured, registry } = mount({ resolveCwd: async () => dir })
|
||||
const tool = toolOf(captured, 'sidebar_open')
|
||||
const value = await tool.execute({ target: 'note.md' }, exec('s1'))
|
||||
expect(value).toMatchObject({ kind: 'file', target: join(dir, 'note.md'), title: 'note.md', delivered: false })
|
||||
@@ -172,7 +172,7 @@ describe('sidebar_open tool', () => {
|
||||
try {
|
||||
const sub = join(dir, 'src')
|
||||
mkdirSync(sub)
|
||||
const { captured } = mount({ resolveCwd: () => dir })
|
||||
const { captured } = mount({ resolveCwd: async () => dir })
|
||||
const tool = toolOf(captured, 'sidebar_open')
|
||||
const value = await tool.execute({ target: sub, title: 'Sources' }, exec('s1'))
|
||||
expect(value).toEqual({ kind: 'folder', target: sub, title: 'Sources', delivered: false })
|
||||
@@ -198,7 +198,7 @@ describe('sidebar_open tool', () => {
|
||||
})
|
||||
|
||||
it('reports a missing local path instead of opening it', async () => {
|
||||
const { captured } = mount({ resolveCwd: () => '/definitely/missing' })
|
||||
const { captured } = mount({ resolveCwd: async () => '/definitely/missing' })
|
||||
const tool = toolOf(captured, 'sidebar_open')
|
||||
await expect(tool.execute({ target: '/definitely/missing/nope.txt' }, exec('s1')))
|
||||
.rejects.toThrow(/does not exist/)
|
||||
@@ -213,7 +213,7 @@ describe('sidebar_open tool', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'sidebar-open-'))
|
||||
try {
|
||||
writeFileSync(join(dir, 'f.txt'), 'x')
|
||||
const { captured: fileCaptured } = mount({ resolveCwd: () => dir, prefs: { tabsEnabled: { editor: false } } })
|
||||
const { captured: fileCaptured } = mount({ resolveCwd: async () => dir, prefs: { tabsEnabled: { editor: false } } })
|
||||
const fileTool = toolOf(fileCaptured, 'sidebar_open')
|
||||
// A URL is unaffected by the editor gate; a path is refused with cause.
|
||||
await expect(fileTool.execute({ target: 'https://example.com/' }, exec('s1'))).resolves.toMatchObject({ kind: 'url' })
|
||||
|
||||
+47
-1
@@ -485,6 +485,7 @@ describe('git destructive operations (scratch repository)', () => {
|
||||
describe('session cwd resolution over the API route', () => {
|
||||
interface CtxOverrides {
|
||||
sessions?: { get: (id: string) => { header: { cwd?: string } } | undefined }
|
||||
sessionPersistence?: { inspect: (id: string) => Promise<{ meta: { cwd?: string } }> }
|
||||
}
|
||||
|
||||
const mountAll = (overrides: CtxOverrides = {}): SidebarWebRoute[] => {
|
||||
@@ -502,7 +503,7 @@ describe('session cwd resolution over the API route', () => {
|
||||
// No settings service: the namespace registration never runs.
|
||||
inject: () => () => {},
|
||||
// No jobs/agents services in the smoke context: the routes degrade.
|
||||
get: () => undefined,
|
||||
get: (key: string) => key === 'sessionPersistence' ? overrides.sessionPersistence : undefined,
|
||||
}
|
||||
apply(ctx as never)
|
||||
return routes
|
||||
@@ -558,6 +559,51 @@ describe('session cwd resolution over the API route', () => {
|
||||
expect(result.value?.cwd).toBe(process.cwd())
|
||||
})
|
||||
|
||||
it('resolves a cold (detached) session cwd through the persistence index', async () => {
|
||||
// Regression: a detached first request (session not yet attached, no
|
||||
// client cwd) must resolve the cwd from the session-persistence index
|
||||
// instead of the host process cwd. On Windows the host process cwd is
|
||||
// the DSH source root (dsh.cmd's `pushd`), so every user-project path
|
||||
// was misclassified as "outside workspace" by the realpath guard.
|
||||
const coldCwd = resolvePath('/cold-project-cwd')
|
||||
const route = mount({
|
||||
sessionPersistence: {
|
||||
inspect: async (id) => ({
|
||||
meta: id === 's-cold' ? { cwd: coldCwd } : {},
|
||||
}),
|
||||
},
|
||||
})
|
||||
const result = await invoke(route, 'session.cwd', { sessionId: 's-cold' })
|
||||
expect(result.ok).toBe(true)
|
||||
expect(result.value?.cwd).toBe(coldCwd)
|
||||
})
|
||||
|
||||
it('rejects a relative cwd from the persistence index', async () => {
|
||||
// A buggy / corrupt persistence layer that stored a relative cwd must
|
||||
// be rejected by requireAbsolute instead of flowing into the workspace
|
||||
// guard, where it would be resolved against the host process cwd and
|
||||
// potentially recreate the original "outside workspace" misclassification.
|
||||
const route = mount({
|
||||
sessionPersistence: {
|
||||
inspect: async () => ({ meta: { cwd: 'relative/path' } }),
|
||||
},
|
||||
})
|
||||
const result = await invoke(route, 'session.cwd', { sessionId: 's-bad' })
|
||||
expect(result.ok).toBe(false)
|
||||
expect(result.error?.message).toMatch(/invalid working directory/)
|
||||
})
|
||||
|
||||
it('falls back to the process cwd when persistence has no cwd for the session', async () => {
|
||||
const route = mount({
|
||||
sessionPersistence: {
|
||||
inspect: async () => ({ meta: {} }),
|
||||
},
|
||||
})
|
||||
const result = await invoke(route, 'session.cwd', { sessionId: 's-blank' })
|
||||
expect(result.ok).toBe(true)
|
||||
expect(result.value?.cwd).toBe(process.cwd())
|
||||
})
|
||||
|
||||
it('prefers the attached session header over the client summary', async () => {
|
||||
const route = mount({
|
||||
sessions: {
|
||||
|
||||
+1
-1
@@ -87,7 +87,7 @@ function mount(): { captured: CapturedTool[]; registry: FakeRegistry; dispose: (
|
||||
},
|
||||
} as unknown as Context
|
||||
const registry = new FakeRegistry()
|
||||
const dispose = registerTools(ctx, registry as unknown as AgentPtyRegistry, () => '/cwd', () => ({}))
|
||||
const dispose = registerTools(ctx, registry as unknown as AgentPtyRegistry, async () => '/cwd', () => ({}))
|
||||
return { captured, registry, dispose: () => { dispose(); return disposeCount } }
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user