From 3eba1290c93759172600fb7c41bb3ddccce09f1f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 06:51:20 -0500 Subject: [PATCH 1/8] refactor(paths): consolidate per-workspace state under WorkspaceDir Hard cutover: stop spraying per-workspace state across three sibling directories on the host and move everything beneath the canonical ~/.devsy/contexts//workspaces// tree so a single os.RemoveAll(WorkspaceDir) wipes a workspace's entire footprint. Previous layout (macOS host): ~/.devsy/contexts//workspaces// CLI metadata ~/.devsy/agent/contexts//workspaces// host-side agent state (workspace.json copy, content/, docker-credential helper cache) ~/.devsy/logs// desktop streaming logs New layout: ~/.devsy/contexts//workspaces// workspace.json, tunnel.{json,lock,log}, ... agent/ (was ~/.devsy/agent/...) logs/ (was ~/.devsy/logs//) Changes: - pkg/config/pathmanager.go, pkg/provider/dir.go: add WorkspaceAgentDir and WorkspaceLogDir to PathManager, plus thin GetWorkspaceAgentDir / GetWorkspaceLogDir helpers. - pkg/agent/workspace.go: GetAgentWorkspaceDir / CreateAgentWorkspaceDir / GetAgentBinariesDir branch on isHostAgentInvocation (no --agent-dir, no DEVSY_HOME env) and resolve via PathManager.WorkspaceAgentDir. Container-side calls keep using FindAgentHomeFolder unchanged (their --agent-dir is always set by the SSH command builder). - desktop/src/main/log-store.ts: rewrite to take (context, workspaceId) for every per-workspace operation and compose paths via /contexts//workspaces//logs/. prune() walks the whole contexts/*/workspaces/*/logs/ tree. - desktop/src/main/state.ts: add workspaceContext(id) lookup with a "default" fallback for workspaces whose context field is missing. - desktop/src/main/ipc.ts: every workspace_* IPC handler resolves context via state.workspaceContext before calling LogStore. - pkg/ide/ideparse/parse_test.go (new): lock down bug #3 with two tests covering the IDE switch save path. Both pass on origin/main, proving the CLI persists --ide=vscode correctly when it's received. - cmd/up/up_client.go: log cmd.IDE and cmd.IDELaunch at debug level before workspace.Resolve so the next bug #3 repro will reveal whether the desktop is actually forwarding --ide. --- cmd/up/up_client.go | 2 + desktop/src/main/__tests__/log-store.test.ts | 48 +++++++---- desktop/src/main/ipc.ts | 27 ++++-- desktop/src/main/log-store.ts | 72 +++++++++++----- desktop/src/main/state.ts | 9 ++ pkg/agent/workspace.go | 57 +++++++++++-- pkg/config/pathmanager.go | 28 ++++++ pkg/config/pathmanager_linux_test.go | 10 +++ pkg/ide/ideparse/parse_test.go | 90 ++++++++++++++++++++ pkg/provider/dir.go | 21 +++++ 10 files changed, 309 insertions(+), 55 deletions(-) create mode 100644 pkg/ide/ideparse/parse_test.go diff --git a/cmd/up/up_client.go b/cmd/up/up_client.go index 6bfa6f47a..f9d274672 100644 --- a/cmd/up/up_client.go +++ b/cmd/up/up_client.go @@ -90,6 +90,8 @@ func (cmd *UpCmd) prepareClient( //nolint:funlen cmd.resolveSSHConfig(devsyConfig) + log.Debugf("up: resolving workspace with cmd.IDE=%q ide-launch=%q", cmd.IDE, cmd.IDELaunch) + client, err := workspace2.Resolve( ctx, devsyConfig, diff --git a/desktop/src/main/__tests__/log-store.test.ts b/desktop/src/main/__tests__/log-store.test.ts index ef4c6315a..b057c1cec 100644 --- a/desktop/src/main/__tests__/log-store.test.ts +++ b/desktop/src/main/__tests__/log-store.test.ts @@ -4,6 +4,8 @@ import { join } from "node:path" import { afterEach, beforeEach, describe, expect, it } from "vitest" import { LogStore } from "../log-store.js" +const CTX = "default" + describe("LogStore", () => { let store: LogStore let tempDir: string @@ -17,14 +19,16 @@ describe("LogStore", () => { rmSync(tempDir, { recursive: true, force: true }) }) - it("creates a log file and returns its path", () => { - const logPath = store.createLogFile("ws-1") - expect(logPath).toContain("ws-1") + it("creates a log file under the per-workspace dir", () => { + const logPath = store.createLogFile(CTX, "ws-1") + expect(logPath).toContain( + join("contexts", CTX, "workspaces", "ws-1", "logs"), + ) expect(logPath).toMatch(/\.log$/) }) it("appends lines to a log file", () => { - const logPath = store.createLogFile("ws-1") + const logPath = store.createLogFile(CTX, "ws-1") store.appendLog(logPath, "line 1") store.appendLog(logPath, "line 2") const content = store.readLogByPath(logPath) @@ -33,10 +37,9 @@ describe("LogStore", () => { }) it("lists logs for a workspace, newest first", () => { - store.createLogFile("ws-1") - // Small delay to ensure different timestamps - const path2 = store.createLogFile("ws-1") - const entries = store.listLogs("ws-1") + store.createLogFile(CTX, "ws-1") + const path2 = store.createLogFile(CTX, "ws-1") + const entries = store.listLogs(CTX, "ws-1") expect(entries).toHaveLength(2) expect(entries[0].filename).toBe( path2.split("/").pop()?.split("\\").pop() ?? "", @@ -44,29 +47,38 @@ describe("LogStore", () => { }) it("returns empty list for unknown workspace", () => { - expect(store.listLogs("nonexistent")).toEqual([]) + expect(store.listLogs(CTX, "nonexistent")).toEqual([]) }) it("reads a log file by workspace and filename", () => { - const logPath = store.createLogFile("ws-1") + const logPath = store.createLogFile(CTX, "ws-1") store.appendLog(logPath, "test content") - const entries = store.listLogs("ws-1") - const content = store.readLog("ws-1", entries[0].filename) + const entries = store.listLogs(CTX, "ws-1") + const content = store.readLog(CTX, "ws-1", entries[0].filename) expect(content).toContain("test content") }) it("deletes a log file", () => { - const logPath = store.createLogFile("ws-1") + const logPath = store.createLogFile(CTX, "ws-1") store.appendLog(logPath, "data") - const entries = store.listLogs("ws-1") - store.deleteLog("ws-1", entries[0].filename) - expect(store.listLogs("ws-1")).toHaveLength(0) + const entries = store.listLogs(CTX, "ws-1") + store.deleteLog(CTX, "ws-1", entries[0].filename) + expect(store.listLogs(CTX, "ws-1")).toHaveLength(0) }) it("prunes logs older than maxAge (keeps recent)", () => { - store.createLogFile("ws-1") + store.createLogFile(CTX, "ws-1") const removed = store.prune(30) expect(removed).toBe(0) - expect(store.listLogs("ws-1")).toHaveLength(1) + expect(store.listLogs(CTX, "ws-1")).toHaveLength(1) + }) + + it("isolates logs per workspace and per context", () => { + store.createLogFile(CTX, "ws-1") + store.createLogFile("other-ctx", "ws-1") + store.createLogFile(CTX, "ws-2") + expect(store.listLogs(CTX, "ws-1")).toHaveLength(1) + expect(store.listLogs("other-ctx", "ws-1")).toHaveLength(1) + expect(store.listLogs(CTX, "ws-2")).toHaveLength(1) }) }) diff --git a/desktop/src/main/ipc.ts b/desktop/src/main/ipc.ts index c8905b9ab..e57861b29 100644 --- a/desktop/src/main/ipc.ts +++ b/desktop/src/main/ipc.ts @@ -302,21 +302,32 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M ipcMain.handle( "workspace_logs_list", async (_event, args: { workspaceId: string }) => { - return logStore.listLogs(args.workspaceId) + return logStore.listLogs( + state.workspaceContext(args.workspaceId), + args.workspaceId, + ) }, ) ipcMain.handle( "workspace_log_read", async (_event, args: { workspaceId: string; filename: string }) => { - return logStore.readLog(args.workspaceId, args.filename) + return logStore.readLog( + state.workspaceContext(args.workspaceId), + args.workspaceId, + args.filename, + ) }, ) ipcMain.handle( "workspace_log_delete", async (_event, args: { workspaceId: string; filename: string }) => { - logStore.deleteLog(args.workspaceId, args.filename) + logStore.deleteLog( + state.workspaceContext(args.workspaceId), + args.workspaceId, + args.filename, + ) }, ) @@ -345,7 +356,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M const wsId = args.workspaceId ?? args.source const cmdId = crypto.randomUUID() - const logPath = logStore.createLogFile(wsId) + const logPath = logStore.createLogFile(state.workspaceContext(wsId), wsId) const win = deps.getMainWindow() let signalledDone = false @@ -416,7 +427,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M tunnelProcesses.delete(args.workspaceId) } const cmdId = crypto.randomUUID() - const logPath = logStore.createLogFile(args.workspaceId) + const logPath = logStore.createLogFile(state.workspaceContext(args.workspaceId), args.workspaceId) const win = deps.getMainWindow() const cliArgs = ["stop", args.workspaceId] @@ -462,7 +473,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M tunnelProcesses.delete(args.workspaceId) } const cmdId = crypto.randomUUID() - const logPath = logStore.createLogFile(args.workspaceId) + const logPath = logStore.createLogFile(state.workspaceContext(args.workspaceId), args.workspaceId) const win = deps.getMainWindow() const cliArgs = ["delete", args.workspaceId] @@ -503,7 +514,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M async (_event, args: { workspaceId: string; debug?: boolean }) => { trackEvent("workspace_rebuild") const cmdId = crypto.randomUUID() - const logPath = logStore.createLogFile(args.workspaceId) + const logPath = logStore.createLogFile(state.workspaceContext(args.workspaceId), args.workspaceId) const win = deps.getMainWindow() const cliArgs = ["up", args.workspaceId, "--recreate"] @@ -543,7 +554,7 @@ export function registerIpcHandlers(deps: IpcDependencies): { tunnelProcesses: M async (_event, args: { workspaceId: string; debug?: boolean }) => { trackEvent("workspace_reset") const cmdId = crypto.randomUUID() - const logPath = logStore.createLogFile(args.workspaceId) + const logPath = logStore.createLogFile(state.workspaceContext(args.workspaceId), args.workspaceId) const win = deps.getMainWindow() const cliArgs = ["up", args.workspaceId, "--reset"] diff --git a/desktop/src/main/log-store.ts b/desktop/src/main/log-store.ts index e2f69420f..ba027d3f7 100644 --- a/desktop/src/main/log-store.ts +++ b/desktop/src/main/log-store.ts @@ -20,15 +20,31 @@ export interface LogEntry { let counter = 0 +// LogStore writes streaming command logs into the canonical per-workspace +// directory (~/.devsy/contexts//workspaces//logs/), so `devsy delete` +// wipes them via os.RemoveAll on the workspace dir. Callers must supply the +// workspace's context for every operation; resolve it from DaemonState +// (workspaceContext) before invoking. export class LogStore { - constructor(private baseDir: string) {} + constructor(private devsyHomeDir: string) {} static defaultPath(): LogStore { - return new LogStore(join(homedir(), ".devsy", "logs")) + return new LogStore(join(homedir(), ".devsy")) } - createLogFile(workspaceId: string): string { - const dir = join(this.baseDir, workspaceId) + private workspaceLogDir(context: string, workspaceId: string): string { + return join( + this.devsyHomeDir, + "contexts", + context, + "workspaces", + workspaceId, + "logs", + ) + } + + createLogFile(context: string, workspaceId: string): string { + const dir = this.workspaceLogDir(context, workspaceId) mkdirSync(dir, { recursive: true }) const timestamp = new Date().toISOString().replace(/[:.]/g, "-") const suffix = String(counter++).padStart(4, "0") @@ -46,8 +62,8 @@ export class LogStore { return readFileSync(logPath, "utf-8") } - listLogs(workspaceId: string): LogEntry[] { - const dir = join(this.baseDir, workspaceId) + listLogs(context: string, workspaceId: string): LogEntry[] { + const dir = this.workspaceLogDir(context, workspaceId) if (!existsSync(dir)) return [] const entries: LogEntry[] = [] @@ -67,32 +83,42 @@ export class LogStore { return entries } - readLog(workspaceId: string, filename: string): string { - return readFileSync(join(this.baseDir, workspaceId, filename), "utf-8") + readLog(context: string, workspaceId: string, filename: string): string { + return readFileSync( + join(this.workspaceLogDir(context, workspaceId), filename), + "utf-8", + ) } - deleteLog(workspaceId: string, filename: string): void { - unlinkSync(join(this.baseDir, workspaceId, filename)) + deleteLog(context: string, workspaceId: string, filename: string): void { + unlinkSync(join(this.workspaceLogDir(context, workspaceId), filename)) } + // prune walks every /contexts//workspaces//logs/ tree + // and removes log files older than maxAgeDays. Missing trees are skipped. prune(maxAgeDays: number): number { - if (!existsSync(this.baseDir)) return 0 + const contextsRoot = join(this.devsyHomeDir, "contexts") + if (!existsSync(contextsRoot)) return 0 const cutoff = Date.now() - maxAgeDays * 24 * 60 * 60 * 1000 let removed = 0 - for (const wsDir of readdirSync(this.baseDir)) { - const wsPath = join(this.baseDir, wsDir) - const wsStat = statSync(wsPath) - if (!wsStat.isDirectory()) continue - - for (const file of readdirSync(wsPath)) { - if (!file.endsWith(".log")) continue - const filePath = join(wsPath, file) - const fileStat = statSync(filePath) - if (fileStat.birthtimeMs < cutoff) { - unlinkSync(filePath) - removed++ + for (const ctx of readdirSync(contextsRoot)) { + const wsRoot = join(contextsRoot, ctx, "workspaces") + if (!existsSync(wsRoot)) continue + + for (const wsDir of readdirSync(wsRoot)) { + const logDir = join(wsRoot, wsDir, "logs") + if (!existsSync(logDir)) continue + + for (const file of readdirSync(logDir)) { + if (!file.endsWith(".log")) continue + const filePath = join(logDir, file) + const fileStat = statSync(filePath) + if (fileStat.birthtimeMs < cutoff) { + unlinkSync(filePath) + removed++ + } } } } diff --git a/desktop/src/main/state.ts b/desktop/src/main/state.ts index 0488c3968..33876296c 100644 --- a/desktop/src/main/state.ts +++ b/desktop/src/main/state.ts @@ -77,6 +77,15 @@ export class DaemonState { return { contexts: [...this.contexts], activeContext: this.activeContext } } + // workspaceContext resolves the context name for a workspace, falling back to + // "default" when the workspace is unknown or its context field is missing + // (e.g. an older workspace.json written before the field existed). + workspaceContext(workspaceId: string): string { + const ws = this.workspaces.get(workspaceId) + const ctx = ws && typeof ws.context === "string" ? ws.context : "" + return ctx || "default" + } + private mapsEqual(a: Map, b: Map): boolean { if (a.size !== b.size) return false for (const [key, val] of a) { diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index fac6dcfc4..24cdec891 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -190,14 +190,33 @@ func GetAgentBinariesDirFromWorkspaceDir(workspaceDir string) (string, error) { return "", os.ErrNotExist } +// isHostAgentInvocation reports whether this `devsy agent ...` call is +// running on the user's host (as opposed to inside the workspace container). +// The signal is implicit: container-side calls always receive an explicit +// agentFolder via --agent-dir or set DEVSY_HOME, while host-side calls have +// neither. When true, per-workspace agent state lives under the canonical +// PathManager.WorkspaceAgentDir so a single os.RemoveAll(WorkspaceDir) wipes +// it on delete. +func isHostAgentInvocation(agentFolder string) bool { + return agentFolder == "" && os.Getenv(config.EnvHome) == "" +} + func GetAgentBinariesDir(agentFolder, context, workspaceID string) (string, error) { + if context == "" { + context = config.DefaultContext + } + if isHostAgentInvocation(agentFolder) { + workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) + if err != nil { + return "", err + } + return GetAgentBinariesDirFromWorkspaceDir(workspaceDir) + } + homeFolder, err := FindAgentHomeFolder(agentFolder) if err != nil { return "", err } - if context == "" { - context = config.DefaultContext - } // workspace folder workspaceDir := filepath.Join(homeFolder, "contexts", context, "workspaces", workspaceID) @@ -207,13 +226,24 @@ func GetAgentBinariesDir(agentFolder, context, workspaceID string) (string, erro } func GetAgentWorkspaceDir(agentFolder, context, workspaceID string) (string, error) { + if context == "" { + context = config.DefaultContext + } + if isHostAgentInvocation(agentFolder) { + workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) + if err != nil { + return "", err + } + if _, statErr := os.Stat(workspaceDir); statErr == nil { + return workspaceDir, nil + } + return "", os.ErrNotExist + } + homeFolder, err := FindAgentHomeFolder(agentFolder) if err != nil { return "", err } - if context == "" { - context = config.DefaultContext - } // workspace folder workspaceDir := filepath.Join(homeFolder, "contexts", context, "workspaces", workspaceID) @@ -228,6 +258,21 @@ func GetAgentWorkspaceDir(agentFolder, context, workspaceID string) (string, err } func CreateAgentWorkspaceDir(agentFolder, context, workspaceID string) (string, error) { + if context == "" { + context = config.DefaultContext + } + if isHostAgentInvocation(agentFolder) { + workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) + if err != nil { + return "", err + } + // #nosec G301 -- mirrors the legacy 0o755 perms below. + if err := os.MkdirAll(workspaceDir, 0o755); err != nil { + return "", err + } + return workspaceDir, nil + } + homeFolder, err := PrepareAgentHomeFolder(agentFolder) if err != nil { return "", err diff --git a/pkg/config/pathmanager.go b/pkg/config/pathmanager.go index 14f037c8e..8a20b6dde 100644 --- a/pkg/config/pathmanager.go +++ b/pkg/config/pathmanager.go @@ -40,6 +40,8 @@ type PathManager interface { ContextDir(context string) (string, error) WorkspacesDir(context string) (string, error) WorkspaceDir(context, workspaceID string) (string, error) + WorkspaceAgentDir(context, workspaceID string) (string, error) + WorkspaceLogDir(context, workspaceID string) (string, error) MachinesDir(context string) (string, error) MachineDir(context, machineID string) (string, error) ProvidersDir(context string) (string, error) @@ -119,6 +121,32 @@ func (b *basePathManager) WorkspaceDir(context, workspaceID string) (string, err return filepath.Join(dir, workspaceID), nil } +// WorkspaceAgentDir is where host-side `devsy agent workspace ...` invocations +// store their per-workspace state. It lives under WorkspaceDir so a single +// os.RemoveAll(WorkspaceDir) wipes it during workspace delete. Container-side +// agent invocations still resolve their home via FindAgentHomeFolder and are +// unaffected. +func (b *basePathManager) WorkspaceAgentDir(context, workspaceID string) (string, error) { + dir, err := b.WorkspaceDir(context, workspaceID) + if err != nil { + return "", err + } + + return filepath.Join(dir, "agent"), nil +} + +// WorkspaceLogDir is the per-workspace log directory used by the desktop's +// streaming-log store. Lives under WorkspaceDir for the same reason as +// WorkspaceAgentDir. +func (b *basePathManager) WorkspaceLogDir(context, workspaceID string) (string, error) { + dir, err := b.WorkspaceDir(context, workspaceID) + if err != nil { + return "", err + } + + return filepath.Join(dir, "logs"), nil +} + func (b *basePathManager) MachinesDir(context string) (string, error) { dir, err := b.ContextDir(context) if err != nil { diff --git a/pkg/config/pathmanager_linux_test.go b/pkg/config/pathmanager_linux_test.go index 18b10f036..8ff235506 100644 --- a/pkg/config/pathmanager_linux_test.go +++ b/pkg/config/pathmanager_linux_test.go @@ -126,6 +126,16 @@ func TestContextDataSubPaths(t *testing.T) { func() (string, error) { return pm.WorkspaceDir(ctx, "ws1") }, filepath.Join(base, "workspaces", "ws1"), }, + { + "WorkspaceAgentDir", + func() (string, error) { return pm.WorkspaceAgentDir(ctx, "ws1") }, + filepath.Join(base, "workspaces", "ws1", "agent"), + }, + { + "WorkspaceLogDir", + func() (string, error) { return pm.WorkspaceLogDir(ctx, "ws1") }, + filepath.Join(base, "workspaces", "ws1", "logs"), + }, { "MachinesDir", func() (string, error) { return pm.MachinesDir(ctx) }, diff --git a/pkg/ide/ideparse/parse_test.go b/pkg/ide/ideparse/parse_test.go new file mode 100644 index 000000000..92281ed23 --- /dev/null +++ b/pkg/ide/ideparse/parse_test.go @@ -0,0 +1,90 @@ +package ideparse + +import ( + "testing" + + "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/provider" +) + +// setupTempHome redirects the path manager to a temp HOME so +// SaveWorkspaceConfig writes under the test's tempdir. +func setupTempHome(t *testing.T) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + config.ResetPathManager() + t.Cleanup(config.ResetPathManager) +} + +func emptyConfig() *config.Config { + return &config.Config{ + DefaultContext: config.DefaultContext, + Contexts: map[string]*config.ContextConfig{ + config.DefaultContext: {}, + }, + } +} + +// TestRefreshIDEOptions_SwitchPersistsToDisk locks down bug #3: switching +// from openvscode -> vscode on an existing workspace must update +// workspace.IDE.Name AND write the new value to workspace.json so the next +// `opener.Open` dispatch resolves to the right IDE. +func TestRefreshIDEOptions_SwitchPersistsToDisk(t *testing.T) { + setupTempHome(t) + + ws := &provider.Workspace{ + ID: "ws-1", + Context: config.DefaultContext, + IDE: provider.WorkspaceIDEConfig{ + Name: "openvscode", + }, + } + if err := provider.SaveWorkspaceConfig(ws); err != nil { + t.Fatalf("seed save: %v", err) + } + + got, err := RefreshIDEOptions(emptyConfig(), ws, "vscode", nil) + if err != nil { + t.Fatalf("RefreshIDEOptions: %v", err) + } + if got.IDE.Name != "vscode" { + t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, "vscode") + } + + reloaded, err := provider.LoadWorkspaceConfig(config.DefaultContext, "ws-1") + if err != nil { + t.Fatalf("reload: %v", err) + } + if reloaded.IDE.Name != "vscode" { + t.Errorf( + "on-disk workspace.IDE.Name = %q, want %q (the switch did not persist)", + reloaded.IDE.Name, "vscode", + ) + } +} + +// TestRefreshIDEOptions_EmptyIDEKeepsExisting ensures that calling with +// ide="" does NOT clobber an already-configured workspace IDE (this is the +// default `devsy up ` behavior when --ide is not supplied). +func TestRefreshIDEOptions_EmptyIDEKeepsExisting(t *testing.T) { + setupTempHome(t) + + ws := &provider.Workspace{ + ID: "ws-2", + Context: config.DefaultContext, + IDE: provider.WorkspaceIDEConfig{ + Name: "openvscode", + }, + } + if err := provider.SaveWorkspaceConfig(ws); err != nil { + t.Fatalf("seed save: %v", err) + } + + got, err := RefreshIDEOptions(emptyConfig(), ws, "", nil) + if err != nil { + t.Fatalf("RefreshIDEOptions: %v", err) + } + if got.IDE.Name != "openvscode" { + t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, "openvscode") + } +} diff --git a/pkg/provider/dir.go b/pkg/provider/dir.go index 280cf5901..b37fd9d92 100644 --- a/pkg/provider/dir.go +++ b/pkg/provider/dir.go @@ -72,6 +72,27 @@ func GetWorkspaceDir(context, workspaceID string) (string, error) { return config.DefaultPathManager().WorkspaceDir(context, workspaceID) } +// GetWorkspaceAgentDir returns the host-side per-workspace agent dir +// (.../workspaces//agent). Container-side agent code keeps using +// FindAgentHomeFolder. +func GetWorkspaceAgentDir(context, workspaceID string) (string, error) { + if workspaceID == "" { + return "", fmt.Errorf("workspace id is empty") + } + + return config.DefaultPathManager().WorkspaceAgentDir(context, workspaceID) +} + +// GetWorkspaceLogDir returns the per-workspace log dir +// (.../workspaces//logs) used by the desktop's streaming-log store. +func GetWorkspaceLogDir(context, workspaceID string) (string, error) { + if workspaceID == "" { + return "", fmt.Errorf("workspace id is empty") + } + + return config.DefaultPathManager().WorkspaceLogDir(context, workspaceID) +} + func GetProInstanceDir(context, proInstanceHost string) (string, error) { if proInstanceHost == "" { return "", fmt.Errorf("pro instance host is empty") From 779726ad85807041c4f06e1ba0fc684b2f5c8593 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 07:43:50 -0500 Subject: [PATCH 2/8] fix(desktop): surface workspaceContext fallback with warnings Distinguish three cases in DaemonState.workspaceContext: known + context set, known + missing context, and unknown workspace. Emit console.warn for the latter two so silent "default" fallbacks (which orphan logs when the workspace lives under another context) become visible. --- desktop/src/main/__tests__/state.test.ts | 48 +++++++++++++++++++++++- desktop/src/main/state.ts | 19 ++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/desktop/src/main/__tests__/state.test.ts b/desktop/src/main/__tests__/state.test.ts index a8b9d7e03..8557e95b8 100644 --- a/desktop/src/main/__tests__/state.test.ts +++ b/desktop/src/main/__tests__/state.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest" +import { describe, expect, it, vi } from "vitest" import type { Context, Machine, Provider, Workspace } from "../state.js" import { DaemonState } from "../state.js" @@ -90,4 +90,50 @@ describe("DaemonState", () => { expect(state.updateContexts(contexts, "default")).toBe(false) expect(state.updateContexts(contexts, "staging")).toBe(true) }) + + describe("workspaceContext", () => { + it("returns the workspace's context when known and set", () => { + const state = new DaemonState() + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + state.updateWorkspaces([ + { id: "ws1", lastUsed: "2024-01-01", context: "team-prod" }, + ]) + expect(state.workspaceContext("ws1")).toBe("team-prod") + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it("falls back to default and warns when workspace exists but context is missing", () => { + const state = new DaemonState() + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + state.updateWorkspaces([{ id: "ws1", lastUsed: "2024-01-01" }]) + expect(state.workspaceContext("ws1")).toBe("default") + expect(warn).toHaveBeenCalledTimes(1) + expect(warn.mock.calls[0][0]).toContain("ws1") + expect(warn.mock.calls[0][0]).toContain("no context field") + warn.mockRestore() + }) + + it("falls back to default and warns when workspace exists but context is empty string", () => { + const state = new DaemonState() + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + state.updateWorkspaces([ + { id: "ws1", lastUsed: "2024-01-01", context: "" }, + ]) + expect(state.workspaceContext("ws1")).toBe("default") + expect(warn).toHaveBeenCalledTimes(1) + expect(warn.mock.calls[0][0]).toContain("ws1") + warn.mockRestore() + }) + + it("falls back to default and warns when workspace is unknown", () => { + const state = new DaemonState() + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + expect(state.workspaceContext("missing")).toBe("default") + expect(warn).toHaveBeenCalledTimes(1) + expect(warn.mock.calls[0][0]).toContain("missing") + expect(warn.mock.calls[0][0]).toContain("not found") + warn.mockRestore() + }) + }) }) diff --git a/desktop/src/main/state.ts b/desktop/src/main/state.ts index 33876296c..647f2010c 100644 --- a/desktop/src/main/state.ts +++ b/desktop/src/main/state.ts @@ -79,11 +79,24 @@ export class DaemonState { // workspaceContext resolves the context name for a workspace, falling back to // "default" when the workspace is unknown or its context field is missing - // (e.g. an older workspace.json written before the field existed). + // (e.g. an older workspace.json written before the field existed). The + // fallback is noisy on purpose: silently writing logs under the wrong + // context dir orphans them from `devsy delete` sweeps. workspaceContext(workspaceId: string): string { const ws = this.workspaces.get(workspaceId) - const ctx = ws && typeof ws.context === "string" ? ws.context : "" - return ctx || "default" + if (!ws) { + console.warn( + `[state] workspaceContext: workspace ${workspaceId} not found in state; falling back to "default" (logs may be orphaned if it actually lives under another context)`, + ) + return "default" + } + if (typeof ws.context !== "string" || ws.context === "") { + console.warn( + `[state] workspaceContext: workspace ${workspaceId} has no context field; falling back to "default" (logs may be orphaned if it actually lives under another context)`, + ) + return "default" + } + return ws.context } private mapsEqual(a: Map, b: Map): boolean { From ac06770d00229ff68dbb67ab8f8a0f0736f61701 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 07:49:37 -0500 Subject: [PATCH 3/8] fix(agent): use explicit DEVSY_AGENT_IN_CONTAINER marker for path routing Replace the implicit "agentFolder=='' && DEVSY_HOME unset" heuristic with an explicit DEVSY_AGENT_IN_CONTAINER=1 env var set by every SSH command builder that launches `devsy agent ...` inside the workspace container or machine. The host never sets it. This eliminates two misclassifications introduced in 880fb82c3: 1. Users who export DEVSY_HOME on the host (non-default install) were silently routed to the legacy `/agent/contexts/...` layout instead of the canonical per-workspace WorkspaceAgentDir, so their agent state diverged from the rest of WorkspaceDir. 2. cmd/agent/daemon.go and the workspace logs daemon bypassed the predicate entirely via FindAgentHomeFolder, and would have silently scanned a non-existent legacy glob if invoked on the host. Changes: - pkg/agent/workspace.go: introduce EnvAgentInContainer (the marker) and ContainerAgentEnvPrefix (the inline shell snippet builders prepend to commands). Rename isHostAgentInvocation to exported IsHostAgentInvocation and use only agentFolder + the new marker. DEVSY_HOME is no longer consulted. - pkg/agent/workspace.go: route GetAgentDaemonLogFolder through IsHostAgentInvocation and reject host invocations explicitly. - cmd/agent/daemon.go, cmd/agent/workspace/logs.go and cmd/agent/workspace/logs_daemon.go: assert at Run() entry that the command is being invoked from inside a container/machine, since each reads daemon state or devcontainer logs that only exist there. - All SSH command builders that exec `devsy agent ...` over SSH now prepend ContainerAgentEnvPrefix to the shell snippet: cmd/up/agent.go, cmd/logs.go, cmd/logs_daemon.go, pkg/tunnel/container.go (update-config and container-tunnel), pkg/tunnel/services.go (credentials-server), and pkg/client/clientimplementation/workspace_client.go (delete/stop/ status/BuildAgentClient). - pkg/agent/workspace_test.go: cover all four quadrants of (agentFolder empty/non-empty) x (marker set/unset), plus a guard that DEVSY_HOME no longer flips the predicate and that only the exact "1" value of the marker is honoured. POSIX shells parse leading `NAME=value cmd ...` as a one-shot env assignment, which works even when the SSH server forbids SetEnv. --- cmd/agent/daemon.go | 18 +++- cmd/agent/workspace/logs.go | 9 ++ cmd/agent/workspace/logs_daemon.go | 9 ++ cmd/logs.go | 3 +- cmd/logs_daemon.go | 4 +- cmd/up/agent.go | 3 +- pkg/agent/workspace.go | 58 ++++++++++--- pkg/agent/workspace_test.go | 87 +++++++++++++++++++ .../clientimplementation/workspace_client.go | 12 ++- pkg/tunnel/container.go | 6 +- pkg/tunnel/services.go | 3 +- 11 files changed, 188 insertions(+), 24 deletions(-) create mode 100644 pkg/agent/workspace_test.go diff --git a/cmd/agent/daemon.go b/cmd/agent/daemon.go index e1888d05e..e22ff4535 100644 --- a/cmd/agent/daemon.go +++ b/cmd/agent/daemon.go @@ -3,6 +3,7 @@ package agent import ( "bytes" "context" + "fmt" "os" "path/filepath" "strings" @@ -50,6 +51,15 @@ func NewDaemonCmd(flags *flags.GlobalFlags) *cobra.Command { } func (cmd *DaemonCmd) Run(ctx context.Context) error { + // The agent daemon is a container/machine-side process; the host + // never runs `devsy agent daemon`. Reject host invocations explicitly + // so we don't silently scan a non-existent legacy glob. + if agent.IsHostAgentInvocation(cmd.AgentDir) { + return fmt.Errorf( + "`devsy agent daemon` is only valid inside the workspace container or machine", + ) + } + logFolder, err := agent.GetAgentDaemonLogFolder(cmd.AgentDir) if err != nil { return err @@ -93,7 +103,9 @@ func (cmd *DaemonCmd) doOnce(ctx context.Context) { var latestActivity *time.Time var workspace *provider2.AgentWorkspaceInfo - // get base folder + // get base folder — only reachable from Run, which rejects host + // invocations, so FindAgentHomeFolder always resolves the legacy + // container/machine layout here. baseFolder, err := agent.FindAgentHomeFolder(cmd.AgentDir) if err != nil { return @@ -200,7 +212,9 @@ func (cmd *DaemonCmd) runShutdownCommand( } func (cmd *DaemonCmd) initialTouch() { - // get base folder + // get base folder — only reachable from Run, which rejects host + // invocations, so this always resolves the legacy container/machine + // layout. baseFolder, err := agent.FindAgentHomeFolder(cmd.AgentDir) if err != nil { return diff --git a/cmd/agent/workspace/logs.go b/cmd/agent/workspace/logs.go index 66af7957b..ad30152ae 100644 --- a/cmd/agent/workspace/logs.go +++ b/cmd/agent/workspace/logs.go @@ -38,6 +38,15 @@ func NewLogsCmd(flags *flags.GlobalFlags) *cobra.Command { } func (cmd *LogsCmd) Run(ctx context.Context) error { + // `agent workspace logs` returns devcontainer logs from inside the + // workspace container/machine. Reject host invocations so we don't + // silently look at the wrong place. + if agent.IsHostAgentInvocation(cmd.AgentDir) { + return fmt.Errorf( + "`devsy agent workspace logs` is only valid inside the workspace container or machine", + ) + } + // get workspace info shouldExit, workspaceInfo, err := agent.ReadAgentWorkspaceInfo( cmd.AgentDir, diff --git a/cmd/agent/workspace/logs_daemon.go b/cmd/agent/workspace/logs_daemon.go index eb9ed799d..0ef029e69 100644 --- a/cmd/agent/workspace/logs_daemon.go +++ b/cmd/agent/workspace/logs_daemon.go @@ -38,6 +38,15 @@ func NewLogsDaemonCmd(flags *flags.GlobalFlags) *cobra.Command { } func (cmd *LogsDaemonCmd) Run(ctx context.Context) error { + // `agent workspace logs-daemon` reads agent-daemon.log, which only + // exists inside the workspace container or machine. Reject host + // invocations explicitly to surface misconfigurations early. + if agent.IsHostAgentInvocation(cmd.AgentDir) { + return fmt.Errorf( + "`devsy agent workspace logs-daemon` is only valid inside the workspace container or machine", + ) + } + // get workspace shouldExit, _, err := agent.ReadAgentWorkspaceInfo( cmd.AgentDir, diff --git a/cmd/logs.go b/cmd/logs.go index fabea88a4..5ecfefd08 100644 --- a/cmd/logs.go +++ b/cmd/logs.go @@ -124,7 +124,8 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { defer func() { _ = session.Close() }() agentCommand := fmt.Sprintf( - "'%s' agent workspace logs --context '%s' --id '%s'", + "%s'%s' agent workspace logs --context '%s' --id '%s'", + agent.ContainerAgentEnvPrefix, client.AgentPath(), client.Context(), client.Workspace(), diff --git a/cmd/logs_daemon.go b/cmd/logs_daemon.go index b0b8dcda7..a1b02f288 100644 --- a/cmd/logs_daemon.go +++ b/cmd/logs_daemon.go @@ -6,6 +6,7 @@ import ( "os" "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/config" provider2 "github.com/devsy-org/devsy/pkg/provider" @@ -65,7 +66,8 @@ func (cmd *LogsDaemonCmd) Run(ctx context.Context, args []string) error { } command := fmt.Sprintf( - "'%s' agent workspace logs-daemon --context '%s' --id '%s'", + "%s'%s' agent workspace logs-daemon --context '%s' --id '%s'", + agent.ContainerAgentEnvPrefix, workspaceClient.AgentPath(), workspaceClient.Context(), workspaceClient.Workspace(), diff --git a/cmd/up/agent.go b/cmd/up/agent.go index 5ce27c766..60f36702c 100644 --- a/cmd/up/agent.go +++ b/cmd/up/agent.go @@ -203,7 +203,8 @@ func (cmd *UpCmd) devsyUpMachineSSH( } agentCommand := fmt.Sprintf( - "'%s' agent workspace up --workspace-info '%s'", + "%s'%s' agent workspace up --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, client.AgentPath(), workspaceInfo, ) diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index 24cdec891..e6cc4c0e5 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -35,9 +35,37 @@ var extraSearchLocations = []string{ ContainerDataDir + "/agent", } +// EnvAgentInContainer is set to "1" by every SSH command builder that +// launches `devsy agent ...` inside the workspace container or machine. +// It is the single signal used by IsHostAgentInvocation to distinguish a +// container-side invocation from a host-side one. The host never sets it. +const EnvAgentInContainer = "DEVSY_AGENT_IN_CONTAINER" + +// EnvAgentInContainerTrue is the value the env var takes when set. +const EnvAgentInContainerTrue = "1" + +// ContainerAgentEnvPrefix is the inline env-var assignment SSH command +// builders prepend to the shell snippet that launches `devsy agent ...` +// inside the workspace container or machine. POSIX shells parse leading +// `NAME=value cmd ...` as a one-shot environment assignment for `cmd`, +// so this works without requiring the SSH server to honour SetEnv (which +// is often disabled). +const ContainerAgentEnvPrefix = EnvAgentInContainer + "=" + EnvAgentInContainerTrue + " " + var ErrFindAgentHomeFolder = fmt.Errorf("couldn't find devsy home directory") +// GetAgentDaemonLogFolder returns the folder that holds agent-daemon.log. +// The daemon is a container/machine-side process: it is started by the +// inject-and-run path during `devsy up` and the SSH command builder sets +// DEVSY_AGENT_IN_CONTAINER=1. A host-side invocation has no daemon to +// inspect and therefore is rejected explicitly instead of silently +// resolving to the legacy `/agent` glob. func GetAgentDaemonLogFolder(agentFolder string) (string, error) { + if IsHostAgentInvocation(agentFolder) { + return "", fmt.Errorf( + "agent daemon log folder is only available inside the workspace container or machine", + ) + } return FindAgentHomeFolder(agentFolder) } @@ -190,22 +218,28 @@ func GetAgentBinariesDirFromWorkspaceDir(workspaceDir string) (string, error) { return "", os.ErrNotExist } -// isHostAgentInvocation reports whether this `devsy agent ...` call is -// running on the user's host (as opposed to inside the workspace container). -// The signal is implicit: container-side calls always receive an explicit -// agentFolder via --agent-dir or set DEVSY_HOME, while host-side calls have -// neither. When true, per-workspace agent state lives under the canonical -// PathManager.WorkspaceAgentDir so a single os.RemoveAll(WorkspaceDir) wipes -// it on delete. -func isHostAgentInvocation(agentFolder string) bool { - return agentFolder == "" && os.Getenv(config.EnvHome) == "" +// IsHostAgentInvocation reports whether this `devsy agent ...` call is +// running on the user's host (as opposed to inside the workspace container +// or machine). The signal is explicit: every SSH command builder that +// launches `devsy agent ...` inside the container sets +// DEVSY_AGENT_IN_CONTAINER=1; the host never sets it. An explicit +// --agent-dir (agentFolder != "") also forces a non-host (legacy/explicit) +// routing, so unit tests and unusual deployments can still pin a folder. +// +// When this returns true, per-workspace agent state lives under the +// canonical PathManager.WorkspaceAgentDir so a single +// os.RemoveAll(WorkspaceDir) wipes it on delete. DEVSY_HOME is NOT +// consulted here — it is purely a host-side config-dir relocation knob +// and must not double as a container marker. +func IsHostAgentInvocation(agentFolder string) bool { + return agentFolder == "" && os.Getenv(EnvAgentInContainer) != EnvAgentInContainerTrue } func GetAgentBinariesDir(agentFolder, context, workspaceID string) (string, error) { if context == "" { context = config.DefaultContext } - if isHostAgentInvocation(agentFolder) { + if IsHostAgentInvocation(agentFolder) { workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) if err != nil { return "", err @@ -229,7 +263,7 @@ func GetAgentWorkspaceDir(agentFolder, context, workspaceID string) (string, err if context == "" { context = config.DefaultContext } - if isHostAgentInvocation(agentFolder) { + if IsHostAgentInvocation(agentFolder) { workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) if err != nil { return "", err @@ -261,7 +295,7 @@ func CreateAgentWorkspaceDir(agentFolder, context, workspaceID string) (string, if context == "" { context = config.DefaultContext } - if isHostAgentInvocation(agentFolder) { + if IsHostAgentInvocation(agentFolder) { workspaceDir, err := provider2.GetWorkspaceAgentDir(context, workspaceID) if err != nil { return "", err diff --git a/pkg/agent/workspace_test.go b/pkg/agent/workspace_test.go new file mode 100644 index 000000000..782e523b8 --- /dev/null +++ b/pkg/agent/workspace_test.go @@ -0,0 +1,87 @@ +package agent + +import ( + "testing" +) + +// TestIsHostAgentInvocation covers the four quadrants of +// (agentFolder empty/non-empty) x (DEVSY_AGENT_IN_CONTAINER set/unset). +func TestIsHostAgentInvocation(t *testing.T) { + tests := []struct { + name string + agentFolder string + inContainer string // empty == unset; "1" == container marker + want bool + }{ + { + name: "host: no agentFolder, no marker", + agentFolder: "", + inContainer: "", + want: true, + }, + { + name: "container: no agentFolder, marker set", + agentFolder: "", + inContainer: "1", + want: false, + }, + { + name: "explicit agentFolder, no marker (legacy/explicit)", + agentFolder: "/some/dir", + inContainer: "", + want: false, + }, + { + name: "explicit agentFolder and marker (container with --agent-dir)", + agentFolder: "/some/dir", + inContainer: "1", + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Use t.Setenv so the var is reverted after the test, regardless + // of subtest order. An empty value means "unset". + if tc.inContainer == "" { + // Setenv("", "") followed by automatic cleanup is fine; but + // to truly start from "unset" we set to "" which fails the + // "== EnvAgentInContainerTrue" check the same way unset does. + t.Setenv(EnvAgentInContainer, "") + } else { + t.Setenv(EnvAgentInContainer, tc.inContainer) + } + + got := IsHostAgentInvocation(tc.agentFolder) + if got != tc.want { + t.Fatalf( + "IsHostAgentInvocation(%q) with %s=%q = %v, want %v", + tc.agentFolder, EnvAgentInContainer, tc.inContainer, got, tc.want, + ) + } + }) + } +} + +// TestIsHostAgentInvocation_IgnoresDevsyHome guards the regression flagged +// in the bug report: setting DEVSY_HOME on the host must NOT flip the +// predicate to "container" — only DEVSY_AGENT_IN_CONTAINER does that. +func TestIsHostAgentInvocation_IgnoresDevsyHome(t *testing.T) { + t.Setenv("DEVSY_HOME", "/custom/devsy/home") + t.Setenv(EnvAgentInContainer, "") + + if !IsHostAgentInvocation("") { + t.Fatal("IsHostAgentInvocation should still report host when only DEVSY_HOME is set") + } +} + +// TestIsHostAgentInvocation_NonStandardMarkerValue ensures that only the +// exact "1" string flips the predicate, mirroring the strict equality +// check in the implementation. +func TestIsHostAgentInvocation_NonStandardMarkerValue(t *testing.T) { + t.Setenv(EnvAgentInContainer, "true") + + if !IsHostAgentInvocation("") { + t.Fatal("only DEVSY_AGENT_IN_CONTAINER=1 should be honoured; got false for value 'true'") + } +} diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index 68a09eaf8..fa51f895d 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -383,7 +383,8 @@ func (s *workspaceClient) Delete(ctx context.Context, opt client.DeleteOptions) return fmt.Errorf("agent info") } command := fmt.Sprintf( - "'%s' agent workspace delete --workspace-info '%s'", + "%s'%s' agent workspace delete --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, ) @@ -491,7 +492,8 @@ func (s *workspaceClient) Stop(ctx context.Context, opt client.StopOptions) erro return fmt.Errorf("agent info") } command := fmt.Sprintf( - "'%s' agent workspace stop --workspace-info '%s'", + "%s'%s' agent workspace stop --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, ) @@ -659,7 +661,8 @@ func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status return "", fmt.Errorf("get agent info") } command := fmt.Sprintf( - "'%s' agent workspace status --workspace-info '%s'", + "%s'%s' agent workspace status --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, ) @@ -990,7 +993,8 @@ func buildAgentCommand( agentCommand, workspaceInfo string, ) string { command := fmt.Sprintf( - "'%s' agent workspace %s --workspace-info '%s'", + "%s'%s' agent workspace %s --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, workspaceClient.AgentPath(), agentCommand, workspaceInfo, diff --git a/pkg/tunnel/container.go b/pkg/tunnel/container.go index c9ebab42c..449de6465 100644 --- a/pkg/tunnel/container.go +++ b/pkg/tunnel/container.go @@ -146,7 +146,8 @@ func (c *ContainerTunnel) updateConfig(ctx context.Context, sshClient *ssh.Clien // update workspace remotely buf := &bytes.Buffer{} command := fmt.Sprintf( - "'%s' agent workspace update-config --workspace-info '%s'", + "%s'%s' agent workspace update-config --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, c.client.AgentPath(), workspaceInfo, ) @@ -242,7 +243,8 @@ func (c *ContainerTunnel) runContainerTunnel(ctx context.Context, opts container defer log.Debugf("Container tunnel exited") command := fmt.Sprintf( - "'%s' agent container-tunnel --workspace-info '%s'", + "%s'%s' agent container-tunnel --workspace-info '%s'", + agent.ContainerAgentEnvPrefix, c.client.AgentPath(), opts.workspaceInfo, ) diff --git a/pkg/tunnel/services.go b/pkg/tunnel/services.go index 6a74410d0..d267e920c 100644 --- a/pkg/tunnel/services.go +++ b/pkg/tunnel/services.go @@ -134,7 +134,8 @@ func addGitSSHSigningKey(command string, explicitKey string, workingDir string) // buildCredentialsCommand builds the credentials server command. func buildCredentialsCommand(opts RunServicesOptions) string { command := fmt.Sprintf( - "%s agent container credentials-server --user %s", + "%s%s agent container credentials-server --user %s", + agent.ContainerAgentEnvPrefix, shellescape.Quote(agent.ContainerDevsyHelperLocation), shellescape.Quote(opts.User), ) From 9e67aa8f15fb5e65be301fdb1f027df0d0e19e20 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 07:56:09 -0500 Subject: [PATCH 4/8] fix(agent): cross-check container env var against fs indicators A stale DEVSY_AGENT_IN_CONTAINER=1 in a user's host rc file could flip IsHostAgentInvocation to "container", routing host calls through the legacy FindAgentHomeFolder path and orphaning workspace state. Defend by requiring at least one container indicator (/.dockerenv, /run/.containerenv, or a docker/containerd token in /proc/1/cgroup) alongside the env var. When the env is set without any indicator, warn once and treat the call as host. --- pkg/agent/container_detect.go | 80 ++++++++++++++++++ pkg/agent/container_detect_test.go | 131 +++++++++++++++++++++++++++++ pkg/agent/workspace.go | 28 +++++- pkg/agent/workspace_test.go | 114 +++++++++++++++++-------- 4 files changed, 315 insertions(+), 38 deletions(-) create mode 100644 pkg/agent/container_detect.go create mode 100644 pkg/agent/container_detect_test.go diff --git a/pkg/agent/container_detect.go b/pkg/agent/container_detect.go new file mode 100644 index 000000000..9d6f791ac --- /dev/null +++ b/pkg/agent/container_detect.go @@ -0,0 +1,80 @@ +package agent + +import ( + "errors" + "os" + "strings" +) + +// Container indicator paths. These are the well-known filesystem markers +// that a Linux container runtime drops into the rootfs. None of them +// exist on macOS or Windows hosts, and a non-containerized Linux host +// will not have them either. +const ( + dockerEnvPath = "/.dockerenv" + podmanEnvPath = "/run/.containerenv" + cgroupPath = "/proc/1/cgroup" + cgroupDockerToken = "docker" + cgroupContainerdToken = "containerd" +) + +// statExistsFn is the dependency-injection seam for filesystem existence +// checks. Production wires it to a thin os.Stat wrapper; tests provide a +// table-driven fake. +type statExistsFn func(path string) (bool, error) + +// readFileFn is the dependency-injection seam for reading the cgroup +// file. Production wires it to os.ReadFile; tests provide a fake. +type readFileFn func(path string) ([]byte, error) + +// defaultStatExists reports whether `path` exists. ENOENT is treated as +// "no, but not an error" so callers don't have to discriminate. +func defaultStatExists(path string) (bool, error) { + _, err := os.Stat(path) + if err == nil { + return true, nil + } + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + return false, err +} + +// defaultReadFile reads `path`. Missing files yield (nil, nil) so the +// caller can treat them the same as "no container indicator". +func defaultReadFile(path string) ([]byte, error) { + b, err := os.ReadFile(path) // #nosec G304 -- fixed allowlist of /proc paths + if err == nil { + return b, nil + } + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + return nil, err +} + +// isLikelyContainer reports whether the current process appears to be +// running inside a Linux container. It checks three independent +// indicators; any one of them is sufficient. +// +// Host platforms (macOS, Windows, non-containerized Linux) will see all +// three checks fail and the function returns false. +func isLikelyContainer() bool { + return isLikelyContainerWith(defaultStatExists, defaultReadFile) +} + +// isLikelyContainerWith is the testable core of isLikelyContainer. +func isLikelyContainerWith(stat statExistsFn, read readFileFn) bool { + for _, marker := range []string{dockerEnvPath, podmanEnvPath} { + if ok, _ := stat(marker); ok { + return true + } + } + data, _ := read(cgroupPath) + if len(data) == 0 { + return false + } + content := string(data) + return strings.Contains(content, cgroupDockerToken) || + strings.Contains(content, cgroupContainerdToken) +} diff --git a/pkg/agent/container_detect_test.go b/pkg/agent/container_detect_test.go new file mode 100644 index 000000000..e00f03691 --- /dev/null +++ b/pkg/agent/container_detect_test.go @@ -0,0 +1,131 @@ +package agent + +import ( + "errors" + "testing" +) + +var ( + noFiles statExistsFn = func(string) (bool, error) { return false, nil } + noRead readFileFn = func(string) ([]byte, error) { return nil, nil } +) + +type containerCase struct { + name string + stat statExistsFn + read readFileFn + want bool +} + +func markerCases() []containerCase { + return []containerCase{ + { + name: "host: no markers, no cgroup", + stat: noFiles, read: noRead, want: false, + }, + { + name: "container: /.dockerenv present", + stat: func(p string) (bool, error) { return p == dockerEnvPath, nil }, + read: noRead, want: true, + }, + { + name: "container: /run/.containerenv present (podman)", + stat: func(p string) (bool, error) { return p == podmanEnvPath, nil }, + read: noRead, want: true, + }, + { + name: "host: stat error on marker treated as absent", + stat: func(p string) (bool, error) { return false, errors.New("perm") }, + read: noRead, want: false, + }, + } +} + +func cgroupCases() []containerCase { + return []containerCase{ + { + name: "container: cgroup contains docker", + stat: noFiles, + read: func(p string) ([]byte, error) { + if p == cgroupPath { + return []byte("12:cpu:/docker/abcdef\n"), nil + } + return nil, nil + }, + want: true, + }, + { + name: "container: cgroup contains containerd", + stat: noFiles, + read: func(p string) ([]byte, error) { + if p == cgroupPath { + return []byte("0::/system.slice/containerd.service\n"), nil + } + return nil, nil + }, + want: true, + }, + { + name: "host: cgroup present but no container token", + stat: noFiles, + read: func(string) ([]byte, error) { + return []byte("0::/user.slice/user-1000.slice\n"), nil + }, + want: false, + }, + { + name: "host: cgroup unreadable (ENOENT swallowed)", + stat: noFiles, read: noRead, want: false, + }, + { + name: "host: cgroup read error propagates as host", + stat: noFiles, + read: func(string) ([]byte, error) { return nil, errors.New("eio") }, + want: false, + }, + } +} + +func runContainerCases(t *testing.T, cases []containerCase) { + t.Helper() + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := isLikelyContainerWith(tc.stat, tc.read) + if got != tc.want { + t.Fatalf("isLikelyContainerWith = %v, want %v", got, tc.want) + } + }) + } +} + +func TestIsLikelyContainer_Markers(t *testing.T) { + runContainerCases(t, markerCases()) +} + +func TestIsLikelyContainer_Cgroup(t *testing.T) { + runContainerCases(t, cgroupCases()) +} + +// TestDefaultStatExists_NotFound ensures the production stat wrapper +// treats ENOENT as a non-error "no" instead of bubbling up. +func TestDefaultStatExists_NotFound(t *testing.T) { + ok, err := defaultStatExists("/definitely/not/a/real/path/devsy-test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ok { + t.Fatal("expected ok=false for non-existent path") + } +} + +// TestDefaultReadFile_NotFound ensures the production reader treats +// ENOENT as a non-error empty read. +func TestDefaultReadFile_NotFound(t *testing.T) { + b, err := defaultReadFile("/definitely/not/a/real/path/devsy-test") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if b != nil { + t.Fatalf("expected nil bytes, got %q", string(b)) + } +} diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index e6cc4c0e5..3b8a4b012 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -12,6 +12,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "time" "github.com/devsy-org/api/pkg/devsy" @@ -231,8 +232,33 @@ func GetAgentBinariesDirFromWorkspaceDir(workspaceDir string) (string, error) { // os.RemoveAll(WorkspaceDir) wipes it on delete. DEVSY_HOME is NOT // consulted here — it is purely a host-side config-dir relocation knob // and must not double as a container marker. +// staleContainerEnvWarnOnce ensures the "stale env on host" warning is +// emitted at most once per process, so repeated CLI sub-calls in a tight +// loop don't spam the user's terminal. +var staleContainerEnvWarnOnce sync.Once + +// containerDetector is the package-level seam used by IsHostAgentInvocation +// to detect whether the process is running inside a container. Tests +// override this to make the predicate deterministic across platforms. +var containerDetector = isLikelyContainer + func IsHostAgentInvocation(agentFolder string) bool { - return agentFolder == "" && os.Getenv(EnvAgentInContainer) != EnvAgentInContainerTrue + if agentFolder != "" { + return false + } + if os.Getenv(EnvAgentInContainer) != EnvAgentInContainerTrue { + return true + } + if !containerDetector() { + staleContainerEnvWarnOnce.Do(func() { + log.Warnf( + "%s=1 is set but no container indicator file found; treating as host invocation", + EnvAgentInContainer, + ) + }) + return true + } + return false } func GetAgentBinariesDir(agentFolder, context, workspaceID string) (string, error) { diff --git a/pkg/agent/workspace_test.go b/pkg/agent/workspace_test.go index 782e523b8..259863909 100644 --- a/pkg/agent/workspace_test.go +++ b/pkg/agent/workspace_test.go @@ -1,74 +1,113 @@ package agent import ( + "sync" "testing" ) -// TestIsHostAgentInvocation covers the four quadrants of -// (agentFolder empty/non-empty) x (DEVSY_AGENT_IN_CONTAINER set/unset). +// resetOnce returns a fresh sync.Once so tests can re-arm the +// "warn once" latch between runs. +func resetOnce() sync.Once { return sync.Once{} } + +// withContainerDetector swaps the package-level container detector for +// the duration of the test, restoring the previous value on cleanup. +func withContainerDetector(t *testing.T, fn func() bool) { + t.Helper() + prev := containerDetector + containerDetector = fn + t.Cleanup(func() { + containerDetector = prev + // reset the warn-once latch so independent tests can each + // exercise the warning branch. + staleContainerEnvWarnOnce = resetOnce() + }) +} + +// TestIsHostAgentInvocation covers the matrix of +// (agentFolder empty/non-empty) x (env unset/"1") x (container indicator yes/no). func TestIsHostAgentInvocation(t *testing.T) { tests := []struct { - name string - agentFolder string - inContainer string // empty == unset; "1" == container marker - want bool + name string + agentFolder string + inContainer string // empty == unset; "1" == container marker + containerSeen bool + want bool }{ { - name: "host: no agentFolder, no marker", - agentFolder: "", - inContainer: "", - want: true, + name: "host: no agentFolder, no marker, no indicator", + agentFolder: "", + inContainer: "", + containerSeen: false, + want: true, + }, + { + name: "container: no agentFolder, marker set, indicator present", + agentFolder: "", + inContainer: "1", + containerSeen: true, + want: false, }, { - name: "container: no agentFolder, marker set", - agentFolder: "", - inContainer: "1", - want: false, + name: "host with stale env: marker set but no indicator → host + warn", + agentFolder: "", + inContainer: "1", + containerSeen: false, + want: true, }, { - name: "explicit agentFolder, no marker (legacy/explicit)", - agentFolder: "/some/dir", - inContainer: "", - want: false, + name: "host with rogue indicator but no env: → host", + agentFolder: "", + inContainer: "", + containerSeen: true, + want: true, }, { - name: "explicit agentFolder and marker (container with --agent-dir)", - agentFolder: "/some/dir", - inContainer: "1", - want: false, + name: "explicit agentFolder, no marker (legacy/explicit)", + agentFolder: "/some/dir", + inContainer: "", + containerSeen: false, + want: false, + }, + { + name: "explicit agentFolder beats stale env", + agentFolder: "/some/dir", + inContainer: "1", + containerSeen: false, + want: false, + }, + { + name: "explicit agentFolder and marker (container with --agent-dir)", + agentFolder: "/some/dir", + inContainer: "1", + containerSeen: true, + want: false, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Use t.Setenv so the var is reverted after the test, regardless - // of subtest order. An empty value means "unset". - if tc.inContainer == "" { - // Setenv("", "") followed by automatic cleanup is fine; but - // to truly start from "unset" we set to "" which fails the - // "== EnvAgentInContainerTrue" check the same way unset does. - t.Setenv(EnvAgentInContainer, "") - } else { - t.Setenv(EnvAgentInContainer, tc.inContainer) - } + t.Setenv(EnvAgentInContainer, tc.inContainer) + withContainerDetector(t, func() bool { return tc.containerSeen }) got := IsHostAgentInvocation(tc.agentFolder) if got != tc.want { t.Fatalf( - "IsHostAgentInvocation(%q) with %s=%q = %v, want %v", - tc.agentFolder, EnvAgentInContainer, tc.inContainer, got, tc.want, + "IsHostAgentInvocation(%q) with %s=%q indicator=%v = %v, want %v", + tc.agentFolder, EnvAgentInContainer, tc.inContainer, + tc.containerSeen, got, tc.want, ) } }) } } -// TestIsHostAgentInvocation_IgnoresDevsyHome guards the regression flagged -// in the bug report: setting DEVSY_HOME on the host must NOT flip the -// predicate to "container" — only DEVSY_AGENT_IN_CONTAINER does that. +// TestIsHostAgentInvocation_IgnoresDevsyHome guards the regression that +// setting DEVSY_HOME on the host must NOT flip the predicate to +// "container" — only DEVSY_AGENT_IN_CONTAINER + an indicator does that. func TestIsHostAgentInvocation_IgnoresDevsyHome(t *testing.T) { t.Setenv("DEVSY_HOME", "/custom/devsy/home") t.Setenv(EnvAgentInContainer, "") + withContainerDetector(t, func() bool { return false }) if !IsHostAgentInvocation("") { t.Fatal("IsHostAgentInvocation should still report host when only DEVSY_HOME is set") @@ -80,6 +119,7 @@ func TestIsHostAgentInvocation_IgnoresDevsyHome(t *testing.T) { // check in the implementation. func TestIsHostAgentInvocation_NonStandardMarkerValue(t *testing.T) { t.Setenv(EnvAgentInContainer, "true") + withContainerDetector(t, func() bool { return true }) if !IsHostAgentInvocation("") { t.Fatal("only DEVSY_AGENT_IN_CONTAINER=1 should be honoured; got false for value 'true'") From b2e5fdac00250fa5e592960612a13df3c403670d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 08:03:18 -0500 Subject: [PATCH 5/8] fix(agent): drop sync.Once gate on stale-env warning The package-level sync.Once silenced repeated stale DEVSY_AGENT_IN_CONTAINER warnings in long-running parents (test runners, daemonized supervisors) where multiple distinct stale-env scenarios can occur. Each CLI invocation calls IsHostAgentInvocation once, so warning on every detection won't spam terminals. --- pkg/agent/workspace.go | 21 +++++++++------------ pkg/agent/workspace_test.go | 8 -------- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/pkg/agent/workspace.go b/pkg/agent/workspace.go index 3b8a4b012..3e91e97de 100644 --- a/pkg/agent/workspace.go +++ b/pkg/agent/workspace.go @@ -12,7 +12,6 @@ import ( "path/filepath" "runtime" "strings" - "sync" "time" "github.com/devsy-org/api/pkg/devsy" @@ -232,11 +231,6 @@ func GetAgentBinariesDirFromWorkspaceDir(workspaceDir string) (string, error) { // os.RemoveAll(WorkspaceDir) wipes it on delete. DEVSY_HOME is NOT // consulted here — it is purely a host-side config-dir relocation knob // and must not double as a container marker. -// staleContainerEnvWarnOnce ensures the "stale env on host" warning is -// emitted at most once per process, so repeated CLI sub-calls in a tight -// loop don't spam the user's terminal. -var staleContainerEnvWarnOnce sync.Once - // containerDetector is the package-level seam used by IsHostAgentInvocation // to detect whether the process is running inside a container. Tests // override this to make the predicate deterministic across platforms. @@ -250,12 +244,15 @@ func IsHostAgentInvocation(agentFolder string) bool { return true } if !containerDetector() { - staleContainerEnvWarnOnce.Do(func() { - log.Warnf( - "%s=1 is set but no container indicator file found; treating as host invocation", - EnvAgentInContainer, - ) - }) + // Warn every time we detect a stale env on a non-container host. + // IsHostAgentInvocation is called once per CLI invocation, so this + // won't spam the terminal; long-running parents (test runners, + // daemonized supervisors) need every occurrence surfaced, not just + // the first. + log.Warnf( + "%s=1 is set but no container indicator file found; treating as host invocation", + EnvAgentInContainer, + ) return true } return false diff --git a/pkg/agent/workspace_test.go b/pkg/agent/workspace_test.go index 259863909..12d0d36fb 100644 --- a/pkg/agent/workspace_test.go +++ b/pkg/agent/workspace_test.go @@ -1,14 +1,9 @@ package agent import ( - "sync" "testing" ) -// resetOnce returns a fresh sync.Once so tests can re-arm the -// "warn once" latch between runs. -func resetOnce() sync.Once { return sync.Once{} } - // withContainerDetector swaps the package-level container detector for // the duration of the test, restoring the previous value on cleanup. func withContainerDetector(t *testing.T, fn func() bool) { @@ -17,9 +12,6 @@ func withContainerDetector(t *testing.T, fn func() bool) { containerDetector = fn t.Cleanup(func() { containerDetector = prev - // reset the warn-once latch so independent tests can each - // exercise the warning branch. - staleContainerEnvWarnOnce = resetOnce() }) } From 79b889d2b8dcc9833e6f58a6f34d4cb8882dd0ab Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 09:51:14 -0500 Subject: [PATCH 6/8] chore(lint): satisfy goconst + funlen on workspace_test and parse_test Three CI lint failures in PR #443 fixed: - pkg/ide/ideparse/parse_test.go: hoist "openvscode" / "vscode" literals to ideOpenVSCode / ideVSCode constants (goconst). - pkg/agent/workspace_test.go: hoist "/some/dir" to explicitAgentDir constant (goconst); extract the table-data into hostInvocationCases() so TestIsHostAgentInvocation drops below the funlen=60 threshold. No behavior change. --- pkg/agent/workspace_test.go | 36 +++++++++++++++++++++------------- pkg/ide/ideparse/parse_test.go | 23 +++++++++++++--------- 2 files changed, 36 insertions(+), 23 deletions(-) diff --git a/pkg/agent/workspace_test.go b/pkg/agent/workspace_test.go index 12d0d36fb..0877e99ba 100644 --- a/pkg/agent/workspace_test.go +++ b/pkg/agent/workspace_test.go @@ -4,6 +4,10 @@ import ( "testing" ) +// explicitAgentDir is a representative non-empty --agent-dir value used to +// exercise the "explicit folder always wins" branch of IsHostAgentInvocation. +const explicitAgentDir = "/some/dir" + // withContainerDetector swaps the package-level container detector for // the duration of the test, restoring the previous value on cleanup. func withContainerDetector(t *testing.T, fn func() bool) { @@ -15,16 +19,16 @@ func withContainerDetector(t *testing.T, fn func() bool) { }) } -// TestIsHostAgentInvocation covers the matrix of -// (agentFolder empty/non-empty) x (env unset/"1") x (container indicator yes/no). -func TestIsHostAgentInvocation(t *testing.T) { - tests := []struct { - name string - agentFolder string - inContainer string // empty == unset; "1" == container marker - containerSeen bool - want bool - }{ +type hostInvocationCase struct { + name string + agentFolder string + inContainer string // empty == unset; "1" == container marker + containerSeen bool + want bool +} + +func hostInvocationCases() []hostInvocationCase { + return []hostInvocationCase{ { name: "host: no agentFolder, no marker, no indicator", agentFolder: "", @@ -55,28 +59,32 @@ func TestIsHostAgentInvocation(t *testing.T) { }, { name: "explicit agentFolder, no marker (legacy/explicit)", - agentFolder: "/some/dir", + agentFolder: explicitAgentDir, inContainer: "", containerSeen: false, want: false, }, { name: "explicit agentFolder beats stale env", - agentFolder: "/some/dir", + agentFolder: explicitAgentDir, inContainer: "1", containerSeen: false, want: false, }, { name: "explicit agentFolder and marker (container with --agent-dir)", - agentFolder: "/some/dir", + agentFolder: explicitAgentDir, inContainer: "1", containerSeen: true, want: false, }, } +} - for _, tc := range tests { +// TestIsHostAgentInvocation covers the matrix of +// (agentFolder empty/non-empty) x (env unset/"1") x (container indicator yes/no). +func TestIsHostAgentInvocation(t *testing.T) { + for _, tc := range hostInvocationCases() { t.Run(tc.name, func(t *testing.T) { t.Setenv(EnvAgentInContainer, tc.inContainer) withContainerDetector(t, func() bool { return tc.containerSeen }) diff --git a/pkg/ide/ideparse/parse_test.go b/pkg/ide/ideparse/parse_test.go index 92281ed23..2b79ee366 100644 --- a/pkg/ide/ideparse/parse_test.go +++ b/pkg/ide/ideparse/parse_test.go @@ -7,6 +7,11 @@ import ( "github.com/devsy-org/devsy/pkg/provider" ) +const ( + ideOpenVSCode = "openvscode" + ideVSCode = "vscode" +) + // setupTempHome redirects the path manager to a temp HOME so // SaveWorkspaceConfig writes under the test's tempdir. func setupTempHome(t *testing.T) { @@ -36,29 +41,29 @@ func TestRefreshIDEOptions_SwitchPersistsToDisk(t *testing.T) { ID: "ws-1", Context: config.DefaultContext, IDE: provider.WorkspaceIDEConfig{ - Name: "openvscode", + Name: ideOpenVSCode, }, } if err := provider.SaveWorkspaceConfig(ws); err != nil { t.Fatalf("seed save: %v", err) } - got, err := RefreshIDEOptions(emptyConfig(), ws, "vscode", nil) + got, err := RefreshIDEOptions(emptyConfig(), ws, ideVSCode, nil) if err != nil { t.Fatalf("RefreshIDEOptions: %v", err) } - if got.IDE.Name != "vscode" { - t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, "vscode") + if got.IDE.Name != ideVSCode { + t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, ideVSCode) } reloaded, err := provider.LoadWorkspaceConfig(config.DefaultContext, "ws-1") if err != nil { t.Fatalf("reload: %v", err) } - if reloaded.IDE.Name != "vscode" { + if reloaded.IDE.Name != ideVSCode { t.Errorf( "on-disk workspace.IDE.Name = %q, want %q (the switch did not persist)", - reloaded.IDE.Name, "vscode", + reloaded.IDE.Name, ideVSCode, ) } } @@ -73,7 +78,7 @@ func TestRefreshIDEOptions_EmptyIDEKeepsExisting(t *testing.T) { ID: "ws-2", Context: config.DefaultContext, IDE: provider.WorkspaceIDEConfig{ - Name: "openvscode", + Name: ideOpenVSCode, }, } if err := provider.SaveWorkspaceConfig(ws); err != nil { @@ -84,7 +89,7 @@ func TestRefreshIDEOptions_EmptyIDEKeepsExisting(t *testing.T) { if err != nil { t.Fatalf("RefreshIDEOptions: %v", err) } - if got.IDE.Name != "openvscode" { - t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, "openvscode") + if got.IDE.Name != ideOpenVSCode { + t.Errorf("returned workspace.IDE.Name = %q, want %q", got.IDE.Name, ideOpenVSCode) } } From 281ae58cc3b22a30caf01dd026d071562a8f9e98 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 09:54:53 -0500 Subject: [PATCH 7/8] fix(desktop): coderabbit feedback on log-store and workspaceContext Three issues raised on PR #443: - log-store.ts: `readLog`/`deleteLog` joined `filename` directly, allowing `../`-style escapes out of the per-workspace logs dir. Now routes the input through a `safeLogFilename` helper that strips directory components (via path.basename) and rejects names that don't look like log files. A `../../etc/passwd` argument basenames to `passwd` and is rejected for the missing `.log` suffix; a `../sibling.log` basenames to `sibling.log` and is confined to the workspace's own logs dir (lookup misses harmlessly with ENOENT). - log-store.ts: `prune` walked `existsSync`-checked paths, which would throw mid-pass if `wsRoot`/`logDir` was a file or dangling symlink. Now uses an `isReadableDir(path)` helper (existsSync + statSync + isDirectory()) that skips non-directory entries silently so one malformed entry can't abort the whole prune. - state.ts: `workspaceContext` falls back to "default" when the workspace isn't yet in state. For users on a non-default active context, that orphans creation logs under `default/` away from where `devsy delete` will sweep them. Now falls back to the active context (via new `currentContext()` getter); only resorts to literal "default" when the watcher hasn't populated active context yet. Tests updated to cover the active-context fallback, path-traversal rejection, and non-directory entries during prune. --- desktop/src/main/__tests__/log-store.test.ts | 38 ++++++++++++++++- desktop/src/main/__tests__/state.test.ts | 23 +++++++--- desktop/src/main/log-store.ts | 44 ++++++++++++++++---- desktop/src/main/state.ts | 29 +++++++++---- 4 files changed, 111 insertions(+), 23 deletions(-) diff --git a/desktop/src/main/__tests__/log-store.test.ts b/desktop/src/main/__tests__/log-store.test.ts index b057c1cec..feb6b9ba0 100644 --- a/desktop/src/main/__tests__/log-store.test.ts +++ b/desktop/src/main/__tests__/log-store.test.ts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync } from "node:fs" +import { mkdtempSync, rmSync, writeFileSync } from "node:fs" import { tmpdir } from "node:os" import { join } from "node:path" import { afterEach, beforeEach, describe, expect, it } from "vitest" @@ -81,4 +81,40 @@ describe("LogStore", () => { expect(store.listLogs("other-ctx", "ws-1")).toHaveLength(1) expect(store.listLogs(CTX, "ws-2")).toHaveLength(1) }) + + it("rejects non-.log basenames after stripping traversal", () => { + // basename("../../etc/passwd") = "passwd" — no .log extension, rejected. + expect(() => store.readLog(CTX, "ws-1", "../../etc/passwd")).toThrow( + /invalid log filename/, + ) + expect(() => store.deleteLog(CTX, "ws-1", "../../etc/passwd")).toThrow( + /invalid log filename/, + ) + expect(() => store.readLog(CTX, "ws-1", "notes.txt")).toThrow( + /invalid log filename/, + ) + }) + + it("confines traversal-shaped .log filenames to the workspace dir", () => { + // Plant a sibling.log OUTSIDE the workspace logs dir. + const outside = join(tempDir, "sibling.log") + writeFileSync(outside, "outside-content") + // basename("../sibling.log") = "sibling.log" → looked up INSIDE the + // workspace logs dir, where nothing of that name exists. We must NOT + // read the planted file. + expect(() => store.readLog(CTX, "ws-1", "../sibling.log")).toThrow( + /ENOENT/, + ) + }) + + it("prune skips non-directory entries without aborting", () => { + // Create a normal log first. + store.createLogFile(CTX, "ws-1") + // Plant a regular file where prune would expect a workspace dir. + const contextsRoot = join(tempDir, "contexts", CTX, "workspaces") + const stray = join(contextsRoot, "stray-file") + writeFileSync(stray, "") + expect(() => store.prune(30)).not.toThrow() + expect(store.listLogs(CTX, "ws-1")).toHaveLength(1) + }) }) diff --git a/desktop/src/main/__tests__/state.test.ts b/desktop/src/main/__tests__/state.test.ts index 8557e95b8..343e849fa 100644 --- a/desktop/src/main/__tests__/state.test.ts +++ b/desktop/src/main/__tests__/state.test.ts @@ -103,37 +103,48 @@ describe("DaemonState", () => { warn.mockRestore() }) - it("falls back to default and warns when workspace exists but context is missing", () => { + it("falls back to active context and warns when workspace exists but context is missing", () => { const state = new DaemonState() + state.updateContexts([{ name: "team-prod" }], "team-prod") const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) state.updateWorkspaces([{ id: "ws1", lastUsed: "2024-01-01" }]) - expect(state.workspaceContext("ws1")).toBe("default") + expect(state.workspaceContext("ws1")).toBe("team-prod") expect(warn).toHaveBeenCalledTimes(1) expect(warn.mock.calls[0][0]).toContain("ws1") expect(warn.mock.calls[0][0]).toContain("no context field") warn.mockRestore() }) - it("falls back to default and warns when workspace exists but context is empty string", () => { + it("falls back to active context and warns when workspace exists but context is empty string", () => { const state = new DaemonState() + state.updateContexts([{ name: "team-prod" }], "team-prod") const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) state.updateWorkspaces([ { id: "ws1", lastUsed: "2024-01-01", context: "" }, ]) - expect(state.workspaceContext("ws1")).toBe("default") + expect(state.workspaceContext("ws1")).toBe("team-prod") expect(warn).toHaveBeenCalledTimes(1) expect(warn.mock.calls[0][0]).toContain("ws1") warn.mockRestore() }) - it("falls back to default and warns when workspace is unknown", () => { + it("falls back to active context and warns when workspace is unknown", () => { const state = new DaemonState() + state.updateContexts([{ name: "team-prod" }], "team-prod") const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) - expect(state.workspaceContext("missing")).toBe("default") + expect(state.workspaceContext("missing")).toBe("team-prod") expect(warn).toHaveBeenCalledTimes(1) expect(warn.mock.calls[0][0]).toContain("missing") expect(warn.mock.calls[0][0]).toContain("not found") warn.mockRestore() }) + + it("falls back to 'default' when the watcher hasn't populated active context yet", () => { + const state = new DaemonState() + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + expect(state.workspaceContext("missing")).toBe("default") + expect(warn).toHaveBeenCalledTimes(1) + warn.mockRestore() + }) }) }) diff --git a/desktop/src/main/log-store.ts b/desktop/src/main/log-store.ts index ba027d3f7..15626f0d1 100644 --- a/desktop/src/main/log-store.ts +++ b/desktop/src/main/log-store.ts @@ -9,7 +9,33 @@ import { writeFileSync, } from "node:fs" import { homedir } from "node:os" -import { join } from "node:path" +import { basename, join } from "node:path" + +// safeLogFilename strips any directory components from filename so a caller +// can't traverse out of the per-workspace logs dir with "../" segments. +// Throws when the cleaned name doesn't look like one of our log files. +function safeLogFilename(filename: string): string { + const clean = basename(filename) + if (clean === "" || clean === "." || clean === "..") { + throw new Error(`invalid log filename: ${filename}`) + } + if (!clean.endsWith(".log")) { + throw new Error(`invalid log filename: ${filename}`) + } + return clean +} + +// isReadableDir returns true only when path exists AND is a regular +// directory (not a symlink or file). Used by prune to skip non-directory +// entries that would make readdirSync throw mid-pass. +function isReadableDir(path: string): boolean { + if (!existsSync(path)) return false + try { + return statSync(path).isDirectory() + } catch { + return false + } +} export interface LogEntry { workspaceId: string @@ -85,31 +111,35 @@ export class LogStore { readLog(context: string, workspaceId: string, filename: string): string { return readFileSync( - join(this.workspaceLogDir(context, workspaceId), filename), + join(this.workspaceLogDir(context, workspaceId), safeLogFilename(filename)), "utf-8", ) } deleteLog(context: string, workspaceId: string, filename: string): void { - unlinkSync(join(this.workspaceLogDir(context, workspaceId), filename)) + unlinkSync( + join(this.workspaceLogDir(context, workspaceId), safeLogFilename(filename)), + ) } // prune walks every /contexts//workspaces//logs/ tree - // and removes log files older than maxAgeDays. Missing trees are skipped. + // and removes log files older than maxAgeDays. Missing trees or + // non-directory entries (files, dangling symlinks) are skipped without + // aborting the rest of the pass. prune(maxAgeDays: number): number { const contextsRoot = join(this.devsyHomeDir, "contexts") - if (!existsSync(contextsRoot)) return 0 + if (!isReadableDir(contextsRoot)) return 0 const cutoff = Date.now() - maxAgeDays * 24 * 60 * 60 * 1000 let removed = 0 for (const ctx of readdirSync(contextsRoot)) { const wsRoot = join(contextsRoot, ctx, "workspaces") - if (!existsSync(wsRoot)) continue + if (!isReadableDir(wsRoot)) continue for (const wsDir of readdirSync(wsRoot)) { const logDir = join(wsRoot, wsDir, "logs") - if (!existsSync(logDir)) continue + if (!isReadableDir(logDir)) continue for (const file of readdirSync(logDir)) { if (!file.endsWith(".log")) continue diff --git a/desktop/src/main/state.ts b/desktop/src/main/state.ts index 647f2010c..6cacdfa1c 100644 --- a/desktop/src/main/state.ts +++ b/desktop/src/main/state.ts @@ -77,24 +77,35 @@ export class DaemonState { return { contexts: [...this.contexts], activeContext: this.activeContext } } - // workspaceContext resolves the context name for a workspace, falling back to - // "default" when the workspace is unknown or its context field is missing - // (e.g. an older workspace.json written before the field existed). The - // fallback is noisy on purpose: silently writing logs under the wrong - // context dir orphans them from `devsy delete` sweeps. + // currentContext returns the active context, defaulting to "default" only + // when the watcher has yet to populate state. + currentContext(): string { + return this.activeContext || "default" + } + + // workspaceContext resolves the context name for a workspace. When the + // workspace is unknown in state (e.g. workspace_up runs BEFORE the + // watcher's first poll picks up the new workspace) the active context is + // used instead of a hard-coded "default", so creation logs land alongside + // the workspace they're describing rather than orphaned under `default/`. + // Both fallback branches emit a console.warn so log misrouting is visible. workspaceContext(workspaceId: string): string { const ws = this.workspaces.get(workspaceId) if (!ws) { + const active = this.currentContext() console.warn( - `[state] workspaceContext: workspace ${workspaceId} not found in state; falling back to "default" (logs may be orphaned if it actually lives under another context)`, + `[state] workspaceContext: workspace ${workspaceId} not found in state; falling back to active context %q`, + active, ) - return "default" + return active } if (typeof ws.context !== "string" || ws.context === "") { + const active = this.currentContext() console.warn( - `[state] workspaceContext: workspace ${workspaceId} has no context field; falling back to "default" (logs may be orphaned if it actually lives under another context)`, + `[state] workspaceContext: workspace ${workspaceId} has no context field; falling back to active context %q`, + active, ) - return "default" + return active } return ws.context } From f2aba61772efd721cdc454af31911db022b1b94d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 27 May 2026 10:08:22 -0500 Subject: [PATCH 8/8] chore: prefer %q over manual '%s' shell quoting; drop first-person comments Two style nits raised on PR #443: - Replace the `"%s'%s' agent workspace ... '%s'"` template with `"%s%q agent workspace ... %q"` at the eight SSH-command-builder sites the env-prefix change touched. %q is Go's quoting verb; the result is double-quoted instead of single-quoted but the effective shell behavior is identical for the controlled values (binary paths, validated workspace IDs, base64-encoded payloads). Sites that already use shellescape.Quote are untouched. - Reword four comments to drop first-person ("we"/"our") in favour of imperative phrasing. --- cmd/agent/daemon.go | 2 +- cmd/agent/workspace/logs.go | 4 ++-- cmd/logs.go | 2 +- cmd/logs_daemon.go | 4 ++-- cmd/up/agent.go | 2 +- desktop/src/main/__tests__/log-store.test.ts | 4 ++-- desktop/src/main/log-store.ts | 2 +- pkg/client/clientimplementation/workspace_client.go | 8 ++++---- pkg/tunnel/container.go | 6 +++--- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cmd/agent/daemon.go b/cmd/agent/daemon.go index e22ff4535..ab4b3ba77 100644 --- a/cmd/agent/daemon.go +++ b/cmd/agent/daemon.go @@ -53,7 +53,7 @@ func NewDaemonCmd(flags *flags.GlobalFlags) *cobra.Command { func (cmd *DaemonCmd) Run(ctx context.Context) error { // The agent daemon is a container/machine-side process; the host // never runs `devsy agent daemon`. Reject host invocations explicitly - // so we don't silently scan a non-existent legacy glob. + // to avoid silently scanning a non-existent legacy glob. if agent.IsHostAgentInvocation(cmd.AgentDir) { return fmt.Errorf( "`devsy agent daemon` is only valid inside the workspace container or machine", diff --git a/cmd/agent/workspace/logs.go b/cmd/agent/workspace/logs.go index ad30152ae..cee40945d 100644 --- a/cmd/agent/workspace/logs.go +++ b/cmd/agent/workspace/logs.go @@ -39,8 +39,8 @@ func NewLogsCmd(flags *flags.GlobalFlags) *cobra.Command { func (cmd *LogsCmd) Run(ctx context.Context) error { // `agent workspace logs` returns devcontainer logs from inside the - // workspace container/machine. Reject host invocations so we don't - // silently look at the wrong place. + // workspace container/machine. Reject host invocations explicitly + // to avoid silently reading from the wrong place. if agent.IsHostAgentInvocation(cmd.AgentDir) { return fmt.Errorf( "`devsy agent workspace logs` is only valid inside the workspace container or machine", diff --git a/cmd/logs.go b/cmd/logs.go index 5ecfefd08..af29e9b11 100644 --- a/cmd/logs.go +++ b/cmd/logs.go @@ -124,7 +124,7 @@ func (cmd *LogsCmd) Run(ctx context.Context, args []string) error { defer func() { _ = session.Close() }() agentCommand := fmt.Sprintf( - "%s'%s' agent workspace logs --context '%s' --id '%s'", + "%s%q agent workspace logs --context %q --id %q", agent.ContainerAgentEnvPrefix, client.AgentPath(), client.Context(), diff --git a/cmd/logs_daemon.go b/cmd/logs_daemon.go index a1b02f288..2491fa0f9 100644 --- a/cmd/logs_daemon.go +++ b/cmd/logs_daemon.go @@ -66,14 +66,14 @@ func (cmd *LogsDaemonCmd) Run(ctx context.Context, args []string) error { } command := fmt.Sprintf( - "%s'%s' agent workspace logs-daemon --context '%s' --id '%s'", + "%s%q agent workspace logs-daemon --context %q --id %q", agent.ContainerAgentEnvPrefix, workspaceClient.AgentPath(), workspaceClient.Context(), workspaceClient.Workspace(), ) if agentInfo.Agent.DataPath != "" { - command += fmt.Sprintf(" --agent-dir '%s'", agentInfo.Agent.DataPath) + command += fmt.Sprintf(" --agent-dir %q", agentInfo.Agent.DataPath) } // read daemon logs diff --git a/cmd/up/agent.go b/cmd/up/agent.go index 60f36702c..06bf118c9 100644 --- a/cmd/up/agent.go +++ b/cmd/up/agent.go @@ -203,7 +203,7 @@ func (cmd *UpCmd) devsyUpMachineSSH( } agentCommand := fmt.Sprintf( - "%s'%s' agent workspace up --workspace-info '%s'", + "%s%q agent workspace up --workspace-info %q", agent.ContainerAgentEnvPrefix, client.AgentPath(), workspaceInfo, diff --git a/desktop/src/main/__tests__/log-store.test.ts b/desktop/src/main/__tests__/log-store.test.ts index feb6b9ba0..60034eb7b 100644 --- a/desktop/src/main/__tests__/log-store.test.ts +++ b/desktop/src/main/__tests__/log-store.test.ts @@ -100,8 +100,8 @@ describe("LogStore", () => { const outside = join(tempDir, "sibling.log") writeFileSync(outside, "outside-content") // basename("../sibling.log") = "sibling.log" → looked up INSIDE the - // workspace logs dir, where nothing of that name exists. We must NOT - // read the planted file. + // workspace logs dir, where nothing of that name exists. The planted + // file outside the dir must NOT be reachable. expect(() => store.readLog(CTX, "ws-1", "../sibling.log")).toThrow( /ENOENT/, ) diff --git a/desktop/src/main/log-store.ts b/desktop/src/main/log-store.ts index 15626f0d1..418167673 100644 --- a/desktop/src/main/log-store.ts +++ b/desktop/src/main/log-store.ts @@ -13,7 +13,7 @@ import { basename, join } from "node:path" // safeLogFilename strips any directory components from filename so a caller // can't traverse out of the per-workspace logs dir with "../" segments. -// Throws when the cleaned name doesn't look like one of our log files. +// Throws when the cleaned name doesn't look like a streaming-log file. function safeLogFilename(filename: string): string { const clean = basename(filename) if (clean === "" || clean === "." || clean === "..") { diff --git a/pkg/client/clientimplementation/workspace_client.go b/pkg/client/clientimplementation/workspace_client.go index fa51f895d..0af126d1c 100644 --- a/pkg/client/clientimplementation/workspace_client.go +++ b/pkg/client/clientimplementation/workspace_client.go @@ -383,7 +383,7 @@ func (s *workspaceClient) Delete(ctx context.Context, opt client.DeleteOptions) return fmt.Errorf("agent info") } command := fmt.Sprintf( - "%s'%s' agent workspace delete --workspace-info '%s'", + "%s%q agent workspace delete --workspace-info %q", agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, @@ -492,7 +492,7 @@ func (s *workspaceClient) Stop(ctx context.Context, opt client.StopOptions) erro return fmt.Errorf("agent info") } command := fmt.Sprintf( - "%s'%s' agent workspace stop --workspace-info '%s'", + "%s%q agent workspace stop --workspace-info %q", agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, @@ -661,7 +661,7 @@ func (s *workspaceClient) getContainerStatus(ctx context.Context) (client.Status return "", fmt.Errorf("get agent info") } command := fmt.Sprintf( - "%s'%s' agent workspace status --workspace-info '%s'", + "%s%q agent workspace status --workspace-info %q", agent.ContainerAgentEnvPrefix, info.Agent.Path, compressed, @@ -993,7 +993,7 @@ func buildAgentCommand( agentCommand, workspaceInfo string, ) string { command := fmt.Sprintf( - "%s'%s' agent workspace %s --workspace-info '%s'", + "%s%q agent workspace %s --workspace-info %q", agent.ContainerAgentEnvPrefix, workspaceClient.AgentPath(), agentCommand, diff --git a/pkg/tunnel/container.go b/pkg/tunnel/container.go index 449de6465..7b244ce68 100644 --- a/pkg/tunnel/container.go +++ b/pkg/tunnel/container.go @@ -146,13 +146,13 @@ func (c *ContainerTunnel) updateConfig(ctx context.Context, sshClient *ssh.Clien // update workspace remotely buf := &bytes.Buffer{} command := fmt.Sprintf( - "%s'%s' agent workspace update-config --workspace-info '%s'", + "%s%q agent workspace update-config --workspace-info %q", agent.ContainerAgentEnvPrefix, c.client.AgentPath(), workspaceInfo, ) if agentInfo.Agent.DataPath != "" { - command += fmt.Sprintf(" --agent-dir '%s'", agentInfo.Agent.DataPath) + command += fmt.Sprintf(" --agent-dir %q", agentInfo.Agent.DataPath) } log.Debugf("Run command in container: %s", command) @@ -243,7 +243,7 @@ func (c *ContainerTunnel) runContainerTunnel(ctx context.Context, opts container defer log.Debugf("Container tunnel exited") command := fmt.Sprintf( - "%s'%s' agent container-tunnel --workspace-info '%s'", + "%s%q agent container-tunnel --workspace-info %q", agent.ContainerAgentEnvPrefix, c.client.AgentPath(), opts.workspaceInfo,