From fbf46864d9e4fb71922e92ba24b627bbabf44488 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 2 Jun 2026 19:30:22 +0000 Subject: [PATCH] Address bot review comments for updateConfigObject - Roll back the config file and its referenced files if the agent fails or its result doesn't validate, so no half-applied update is left behind - Validate the rendered fast-path config in memory before writing to disk - Only schedule a background sync after a successful update (manager.ts) - Treat empty-object update values as leaves and skip undefined values so the agent prompt matches override() semantics - Detect agent completion via "result" in message (matches the CLI) - Accept the export { config } specifier form in the structural check - Add importAttributes to parseStackConfigFileContent for parser consistency - Drop the unsafe Record cast in stripClaudeCodeEnv - Add a bounded, configurable timeout to the agent run Co-Authored-By: mantra --- .../config-file.test.ts | 25 +++++ .../config-file.ts | 106 +++++++++++++----- .../config-update-agent.ts | 78 +++++++++---- .../remote-development-environment/manager.ts | 7 +- .../stack-shared/src/stack-config-file.ts | 53 ++++++++- 5 files changed, 215 insertions(+), 54 deletions(-) diff --git a/apps/dashboard/src/lib/remote-development-environment/config-file.test.ts b/apps/dashboard/src/lib/remote-development-environment/config-file.test.ts index e4feaa5fa..57346ce1c 100644 --- a/apps/dashboard/src/lib/remote-development-environment/config-file.test.ts +++ b/apps/dashboard/src/lib/remote-development-environment/config-file.test.ts @@ -305,4 +305,29 @@ describe("remote development environment config file", () => { await expect(updateConfigObject(configPath, { "auth.allowSignUp": false })) .rejects.toThrow(/validation failed/); }); + + it("rolls back the config and its referenced files when the agent's result fails validation", async () => { + const templatePath = writeTempFile("welcome-email.tsx", "export default
Old email
;\n"); + const configSource = `import welcomeEmail from "./welcome-email.tsx" with { type: "text" };\n\nexport const config = {\n emails: { templates: { welcome: welcomeEmail } },\n};\n`; + const configPath = writeTempConfig(configSource); + + const { updateConfigObject } = await import("./config-file"); + + // The agent edits both files but then fails, so the partially-applied edits + // must be rolled back and the failure surfaced. + mockAgentImpl = () => { + writeFileSync(templatePath, "export default
Corrupted
;\n", "utf-8"); + writeFileSync(configPath, `export const config = { auth: { allowSignUp: true } };\n`, "utf-8"); + throw new Error("agent blew up"); + }; + + await expect(updateConfigObject(configPath, { + "emails.templates.welcome": "export default
New email
;\n", + })).rejects.toThrow("agent blew up"); + + // Both the config file and the externally-referenced file are back to their + // original contents \u2014 no half-applied update is left behind. + expect(readFileSync(configPath, "utf-8")).toBe(configSource); + expect(readFileSync(templatePath, "utf-8")).toBe("export default
Old email
;\n"); + }); }); diff --git a/apps/dashboard/src/lib/remote-development-environment/config-file.ts b/apps/dashboard/src/lib/remote-development-environment/config-file.ts index 507028b7f..2b232abd0 100644 --- a/apps/dashboard/src/lib/remote-development-environment/config-file.ts +++ b/apps/dashboard/src/lib/remote-development-environment/config-file.ts @@ -3,9 +3,9 @@ import "server-only"; import { showOnboardingStackConfigValue } from "@hexclave/shared/dist/config-authoring"; import { Config, ConfigValue, NormalizedConfig, isValidConfig, normalize, override } from "@hexclave/shared/dist/config/format"; import { detectImportPackageFromDir, renderConfigFileContent } from "@hexclave/shared/dist/config-rendering"; -import { stackConfigFileExportsConfig, tryParseStackConfigFileContent } from "@hexclave/shared/dist/stack-config-file"; +import { getRelativeImportSpecifiers, stackConfigFileExportsConfig, tryParseStackConfigFileContent } from "@hexclave/shared/dist/stack-config-file"; import { createHash } from "crypto"; -import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "fs"; +import { existsSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "fs"; import { createJiti } from "jiti"; import path from "path"; import { runConfigUpdateAgent } from "./config-update-agent"; @@ -88,23 +88,31 @@ export async function readConfigFile(configFilePath: string): Promise<{ config: } /** - * Deterministically renders a config object into the file, overwriting whatever - * was there. This is the canonical, lossy representation (a single - * `export const config = { ...JSON... }`); it does not preserve imports, helper - * wrappers, comments, or external file references. Only use it when there is no - * existing structure to preserve (a brand-new/empty file, or a file that is - * already a plain static literal). Otherwise use {@link updateConfigObject}. + * Renders a config object to its canonical, lossy source text (a single + * `export const config = { ...JSON... }`). It does not preserve imports, helper + * wrappers, comments, or external file references, so only render this for a + * file that has no existing structure to preserve (a brand-new/empty file, or a + * plain static literal). Otherwise use {@link updateConfigObject}. */ -function renderConfigObjectToFile(configFilePath: string, config: Config): void { +function renderConfigObjectToString(configFilePath: string, config: Config): string { + const importPackage = detectImportPackageFromDir(path.dirname(configFilePath)); + return renderConfigFileContent(config, importPackage); +} + +/** Writes `content` to `configFilePath` atomically (write to temp, then rename). */ +function writeFileAtomic(configFilePath: string, content: string): void { const dir = path.dirname(configFilePath); mkdirSync(dir, { recursive: true }); - const importPackage = detectImportPackageFromDir(dir); - const content = renderConfigFileContent(config, importPackage); const tempPath = path.join(dir, `.stack.config.${Math.random().toString(36).slice(2)}.tmp`); writeFileSync(tempPath, content, "utf-8"); renameSync(tempPath, configFilePath); } +/** Renders `config` to its canonical source text and writes it to disk, overwriting the file. */ +function renderConfigObjectToFile(configFilePath: string, config: Config): void { + writeFileAtomic(configFilePath, renderConfigObjectToString(configFilePath, config)); +} + /** * Applies a config update to the file at `configFilePath`, merging `configUpdate` * (a partial config, possibly using dot-notation keys) over the current config. @@ -139,11 +147,14 @@ export async function updateConfigObject(configFilePath: string, configUpdate: C throw new Error(`Config in ${configFilePath} parsed to a static literal that is not a valid config object.`); } const target = override(current, configUpdate); - renderConfigObjectToFile(configFilePath, target); - const written = tryParseStackConfigFileContent(readFileSync(configFilePath, "utf-8"), configFilePath); + // Validate the exact bytes we are about to write *before* touching the file, + // so a bad render can never leave the user's config in a broken state. + const rendered = renderConfigObjectToString(configFilePath, target); + const written = tryParseStackConfigFileContent(rendered, configFilePath); if (written == null || written === showOnboardingStackConfigValue || !isValidConfig(written) || !configsEqual(canonicalizeConfig(written), canonicalizeConfig(target))) { throw new Error(`Config update validation failed for ${configFilePath}: the regenerated file does not match the expected configuration.`); } + writeFileAtomic(configFilePath, rendered); return; } @@ -151,12 +162,50 @@ export async function updateConfigObject(configFilePath: string, configUpdate: C // full semantic check afterwards), then let the agent apply the change. const baselineConfig = await tryReadConfigForValidation(configFilePath); - await runConfigUpdateAgent({ - prompt: buildConfigUpdatePrompt(path.basename(configFilePath), configUpdate), - cwd: path.dirname(configFilePath), - }); + // Snapshot the config file and any externally-referenced files the agent might + // edit, so we can roll back to the original state if the agent fails or its + // result doesn't validate — never leaving a half-applied update behind. + const snapshots = snapshotConfigFiles(configFilePath, content); + try { + await runConfigUpdateAgent({ + prompt: buildConfigUpdatePrompt(path.basename(configFilePath), configUpdate), + cwd: path.dirname(configFilePath), + }); + await validateAgentUpdate(configFilePath, baselineConfig, configUpdate); + } catch (error) { + restoreConfigFiles(snapshots); + throw error; + } +} - await validateAgentUpdate(configFilePath, baselineConfig, configUpdate); +/** A captured file state used for rollback. `content` is `null` if the file did not exist. */ +type ConfigFileSnapshot = { path: string, content: string | null }; + +/** + * Captures the config file plus every file it imports via a relative path (e.g. + * `import x from "./welcome-email.tsx" with { type: "text" }`), which are exactly + * the files an in-place update is expected to touch. Files are read as UTF-8 + * text, which matches this feature's text-import use case. + */ +function snapshotConfigFiles(configFilePath: string, configContent: string): ConfigFileSnapshot[] { + const dir = path.dirname(configFilePath); + const snapshots: ConfigFileSnapshot[] = [{ path: configFilePath, content: configContent }]; + for (const specifier of getRelativeImportSpecifiers(configContent)) { + const resolved = path.resolve(dir, specifier); + if (snapshots.some((snapshot) => snapshot.path === resolved)) continue; + snapshots.push({ path: resolved, content: existsSync(resolved) ? readFileSync(resolved, "utf-8") : null }); + } + return snapshots; +} + +function restoreConfigFiles(snapshots: ConfigFileSnapshot[]): void { + for (const { path: filePath, content } of snapshots) { + if (content === null) { + if (existsSync(filePath)) rmSync(filePath); + } else { + writeFileSync(filePath, content, "utf-8"); + } + } } /** @@ -196,20 +245,27 @@ async function validateAgentUpdate(configFilePath: string, baselineConfig: Confi } } -type ConfigChange = { path: string, value: ConfigValue | undefined }; +type ConfigChange = { path: string, value: ConfigValue }; /** * Flattens a (possibly dot-notation) config update into individual leaf changes, - * so the agent gets an explicit list of which config paths to change. Arrays and - * primitives are leaves; nested plain objects are walked. A `value` of - * `undefined` denotes a field that should be removed. + * so the agent gets an explicit list of which config paths to set. Arrays, + * primitives, and empty objects are leaves; nested non-empty plain objects are + * walked. + * + * `undefined` values are skipped, matching `override` (which filters them out as + * a no-op rather than treating them as removals); emitting removals here would + * diverge from the configuration the update is validated against. */ function flattenConfigUpdate(update: Config): ConfigChange[] { const changes: ConfigChange[] = []; const walk = (prefix: string, obj: Config): void => { for (const [key, value] of Object.entries(obj)) { const fullPath = prefix === "" ? key : `${prefix}.${key}`; - if (value !== null && typeof value === "object" && !Array.isArray(value)) { + if (value === undefined) continue; + // An empty object is a leaf value (an explicit `{}`); only recurse into + // objects that actually have keys. + if (value !== null && typeof value === "object" && !Array.isArray(value) && Object.keys(value).length > 0) { walk(fullPath, value); } else { changes.push({ path: fullPath, value }); @@ -223,9 +279,6 @@ function flattenConfigUpdate(update: Config): ConfigChange[] { function buildConfigUpdatePrompt(configFileName: string, configUpdate: Config): string { const changes = flattenConfigUpdate(configUpdate); const changeLines = changes.map(({ path: configPath, value }) => { - if (value === undefined) { - return `- \`${configPath}\`: (remove this field)`; - } return `- \`${configPath}\`: set to ${JSON.stringify(value)}`; }).join("\n"); @@ -246,7 +299,6 @@ Rules: - Change ONLY the config paths listed above. Leave every other part of the file byte-for-byte unchanged: imports, comments, formatting, helper wrappers, and any config fields not listed. - If a listed path's value is currently provided by an imported external file (like the \`import ... with { type: "text" }\` example above), DO NOT inline the new value into the config file. Instead, overwrite that external file with the new value and keep the import statement intact. - If a listed path's value is a plain inline literal, edit it inline. -- For a path marked "(remove this field)", delete that field from the config. - Keep the file valid: it must still export a \`config\` that, once evaluated, reflects the new values exactly. - Do not run any shell commands and do not create files other than what is required to apply these changes.`; } diff --git a/apps/dashboard/src/lib/remote-development-environment/config-update-agent.ts b/apps/dashboard/src/lib/remote-development-environment/config-update-agent.ts index 6fa24edac..f925e1a50 100644 --- a/apps/dashboard/src/lib/remote-development-environment/config-update-agent.ts +++ b/apps/dashboard/src/lib/remote-development-environment/config-update-agent.ts @@ -19,13 +19,27 @@ const ANTHROPIC_PROXY_BASE_URL: string = process.env.STACK_CLAUDE_PROXY_URL ?? D const LOG_PREFIX = "[Stack RDE]"; -function stripClaudeCodeEnv(): Record { +// Upper bound on how long the agent may run before we abort it, so a stuck or +// runaway agent can't hang the dashboard's config-update request forever. +// Overridable for slow environments via STACK_CONFIG_UPDATE_AGENT_TIMEOUT_MS. +const DEFAULT_AGENT_TIMEOUT_MS = 120_000; +const AGENT_TIMEOUT_MS: number = (() => { + const raw = process.env.STACK_CONFIG_UPDATE_AGENT_TIMEOUT_MS; + if (raw == null || raw.trim() === "") return DEFAULT_AGENT_TIMEOUT_MS; + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed <= 0) { + throw new Error(`Invalid STACK_CONFIG_UPDATE_AGENT_TIMEOUT_MS: ${JSON.stringify(raw)}. Expected a positive number of milliseconds.`); + } + return parsed; +})(); + +function stripClaudeCodeEnv(): Record { const env = { ...process.env }; // Removing CLAUDECODE prevents the SDK from detecting a nested agent. The // ANTHROPIC_API_KEY must be non-empty or users without Claude Code installed // hit a login error (it is ignored by the proxy). delete env.CLAUDECODE; - return env as Record; + return env; } /** @@ -38,25 +52,49 @@ export async function runConfigUpdateAgent(options: { prompt: string, cwd: string, }): Promise { - for await (const message of query({ - prompt: options.prompt, - options: { - // Bash is intentionally omitted: applying a config delta only needs file - // inspection and editing, and withholding shell access reduces the blast - // radius of running an agent against the user's project. - allowedTools: ["Read", "Write", "Edit", "Glob", "Grep"], - permissionMode: "dontAsk", - cwd: options.cwd, - env: { - ...stripClaudeCodeEnv(), - ANTHROPIC_BASE_URL: ANTHROPIC_PROXY_BASE_URL, - ANTHROPIC_API_KEY: "stack-auth-proxy", + const abortController = new AbortController(); + const timeout = setTimeout(() => abortController.abort(), AGENT_TIMEOUT_MS); + // Marks whether we observed a terminal "result" message from the SDK. The + // stream can otherwise end without one (e.g. the process dies), which we must + // treat as a failure rather than silently succeeding. + let sawResult = false; + try { + for await (const message of query({ + prompt: options.prompt, + options: { + // Bash is intentionally omitted: applying a config delta only needs file + // inspection and editing, and withholding shell access reduces the blast + // radius of running an agent against the user's project. + allowedTools: ["Read", "Write", "Edit", "Glob", "Grep"], + permissionMode: "dontAsk", + cwd: options.cwd, + abortController, + env: { + ...stripClaudeCodeEnv(), + ANTHROPIC_BASE_URL: ANTHROPIC_PROXY_BASE_URL, + ANTHROPIC_API_KEY: "stack-auth-proxy", + }, + stderr: (data: string) => { console.warn(`${LOG_PREFIX} [agent] ${data}`); }, }, - stderr: (data: string) => { console.warn(`${LOG_PREFIX} [agent] ${data}`); }, - }, - })) { - if (message.type === "result" && (message.is_error || message.subtype !== "success")) { - throw new Error(`Config update agent failed (${message.subtype}). It was unable to apply the config changes to the file.`); + })) { + // Detect the terminal message via `"result" in message` to match the CLI + // (`runClaudeAgent`); only a successful result carries a `result` field, + // so any other terminal subtype is reported as a failure. + if ("result" in message) { + sawResult = true; + } else if (message.type === "result") { + throw new Error(`Config update agent failed (${message.subtype}). It was unable to apply the config changes to the file.`); + } } + } catch (error) { + if (abortController.signal.aborted) { + throw new Error(`Config update agent timed out after ${AGENT_TIMEOUT_MS}ms. It was unable to apply the config changes to the file.`); + } + throw error; + } finally { + clearTimeout(timeout); + } + if (!sawResult) { + throw new Error("Config update agent ended without reporting a result. It was unable to apply the config changes to the file."); } } diff --git a/apps/dashboard/src/lib/remote-development-environment/manager.ts b/apps/dashboard/src/lib/remote-development-environment/manager.ts index 656e93b91..1fea9d71c 100644 --- a/apps/dashboard/src/lib/remote-development-environment/manager.ts +++ b/apps/dashboard/src/lib/remote-development-environment/manager.ts @@ -651,16 +651,19 @@ export async function applyRemoteDevelopmentEnvironmentConfigUpdate(options: { // The membership is held until the update resolves and then cleared after a // debounce so the file-change events our own edits produce are ignored too. state.synchronouslyUpdatingConfigFiles.add(configFilePath); + let updateSucceeded = false; try { await updateConfigObject(configFilePath, options.configUpdate); + updateSucceeded = true; } finally { setTimeout(() => { state.synchronouslyUpdatingConfigFiles.delete(configFilePath); // For fire-and-forget updates the sync is scheduled only after the // suppression window clears, otherwise scheduleSync would be swallowed // by its own guard while the path is still marked as synchronously - // updating. - if (options.waitForSync === false) { + // updating. Only sync a successful update: a failed one is rolled back, + // so there is nothing new to push. + if (options.waitForSync === false && updateSucceeded) { scheduleSync(configFilePath); } }, SYNC_DEBOUNCE_MS).unref(); diff --git a/packages/stack-shared/src/stack-config-file.ts b/packages/stack-shared/src/stack-config-file.ts index 0e4286d85..48c69369b 100644 --- a/packages/stack-shared/src/stack-config-file.ts +++ b/packages/stack-shared/src/stack-config-file.ts @@ -126,23 +126,66 @@ export function stackConfigFileExportsConfig(content: string, filePath: string): return false; } for (const statement of ast.program.body) { - if (!t.isExportNamedDeclaration(statement) || !t.isVariableDeclaration(statement.declaration)) { + if (!t.isExportNamedDeclaration(statement)) { continue; } - for (const declaration of statement.declaration.declarations) { - if (t.isIdentifier(declaration.id) && declaration.id.name === "config" && declaration.init != null) { - return true; + // `export const config = ...` + if (t.isVariableDeclaration(statement.declaration)) { + for (const declaration of statement.declaration.declarations) { + if (t.isIdentifier(declaration.id) && declaration.id.name === "config" && declaration.init != null) { + return true; + } + } + } + // `export { config }` / `export { somethingElse as config }` + for (const specifier of statement.specifiers) { + if (t.isExportSpecifier(specifier)) { + const exportedName = t.isIdentifier(specifier.exported) ? specifier.exported.name : specifier.exported.value; + if (exportedName === "config") { + return true; + } } } } return false; } +/** + * Returns the relative import sources (those starting with `./` or `../`) + * declared in `content`. Used to discover the external files a config update may + * touch — e.g. `import x from "./welcome-email.tsx" with { type: "text" }` — so + * they can be snapshotted and rolled back if an in-place update fails. Returns + * an empty array if the file can't be parsed. + */ +export function getRelativeImportSpecifiers(content: string): string[] { + let ast: parser.ParseResult; + try { + ast = parser.parse(content, { + sourceType: "module", + plugins: ["typescript", "importAttributes"], + }); + } catch { + return []; + } + const sources: string[] = []; + for (const statement of ast.program.body) { + if (t.isImportDeclaration(statement)) { + const source = statement.source.value; + if (source.startsWith("./") || source.startsWith("../")) { + sources.push(source); + } + } + } + return sources; +} + export function parseStackConfigFileContent(content: string, filePath: string): ParsedStackConfig { if (content.trim() === "") return {}; const ast = parser.parse(content, { sourceType: "module", - plugins: ["typescript"], + // `importAttributes` matches `stackConfigFileExportsConfig` so both parsers + // accept the same files (e.g. `import x from "./f.txt" with { type: "text" }`). + plugins: ["typescript", "importAttributes"], }); for (const statement of ast.program.body) {