From 0f743f93fffa7926fa22f2f9d22d13881a0bdcdf Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Thu, 25 Jun 2026 21:54:46 +0000 Subject: [PATCH] fix: address P0-P2 review feedback from Cubic review P0: Strip OAuth token from git origin after clone so LLM agent never sees credentials (repo-agent.tsx) P1: Replace raw error.message with safe hardcoded text in API response and dashboard UI (apply/route.tsx, config-update.tsx) P1: E2E spike script now requires explicit env vars instead of falling back to pushing to main (spike-orchestrator-e2e.mts) P2: Use urlSchema for commit_url (schema-fields.ts) P2: Return commitSha directly instead of parsing from URL (repo-agent.tsx, apply/route.tsx) P2: Support LINK_BRANCH_ID env var (link-project-to-github.ts) P2: Widen structural fallback regex (config-updater.ts) P2: Log warning when cancel has no sandboxId (cancel/route.tsx) P2: Reject arbitrary string config values (config-eval.ts) Co-Authored-By: mantra --- .../backend/scripts/link-project-to-github.ts | 4 +++- .../scripts/spike-orchestrator-e2e.mts | 10 +++++++--- .../internal/config/github/apply/route.tsx | 5 ++--- .../internal/config/github/cancel/route.tsx | 10 +++++++--- apps/backend/src/lib/config/repo-agent.tsx | 19 +++++++++++++------ apps/dashboard/src/lib/config-update.tsx | 5 ++--- packages/shared-backend/src/config-updater.ts | 5 +++-- packages/shared/src/config-eval.ts | 11 ++++++++++- packages/shared/src/schema-fields.ts | 2 +- 9 files changed, 48 insertions(+), 23 deletions(-) diff --git a/apps/backend/scripts/link-project-to-github.ts b/apps/backend/scripts/link-project-to-github.ts index d6c060b7a..d1bff0a00 100644 --- a/apps/backend/scripts/link-project-to-github.ts +++ b/apps/backend/scripts/link-project-to-github.ts @@ -14,6 +14,7 @@ * LINK_BRANCH=main * LINK_CONFIG_PATH=hexclave.config.ts * LINK_WORKFLOW_PATH=.github/workflows/hexclave-config-sync.yml + * LINK_BRANCH_ID= (defaults to DEFAULT_BRANCH_ID) * LINK_COMMIT= (informational; the agent pulls latest) */ import { setBranchConfigOverrideSource } from "@/lib/config"; @@ -30,9 +31,10 @@ async function main() { const workflowPath = getEnvVariable("LINK_WORKFLOW_PATH", ".github/workflows/hexclave-config-sync.yml"); const commitHash = getEnvVariable("LINK_COMMIT", "") || "0".repeat(40); + const branchId = getEnvVariable("LINK_BRANCH_ID", "") || DEFAULT_BRANCH_ID; await setBranchConfigOverrideSource({ projectId, - branchId: DEFAULT_BRANCH_ID, + branchId, source: { type: "pushed-from-github", owner, diff --git a/apps/backend/scripts/spike-orchestrator-e2e.mts b/apps/backend/scripts/spike-orchestrator-e2e.mts index be7869e08..92c789834 100644 --- a/apps/backend/scripts/spike-orchestrator-e2e.mts +++ b/apps/backend/scripts/spike-orchestrator-e2e.mts @@ -21,10 +21,14 @@ import { type GithubRepoRef, } from "../src/lib/config/repo-agent"; +if (!process.env.SPIKE_OWNER || !process.env.SPIKE_REPO || !process.env.SPIKE_BRANCH) { + console.error("SPIKE_OWNER, SPIKE_REPO, and SPIKE_BRANCH must all be set explicitly.\nThis script pushes commits to a real repo — refusing to fall back to defaults."); + process.exit(1); +} const REF: GithubRepoRef = { - owner: process.env.SPIKE_OWNER ?? "hexclave", - repo: process.env.SPIKE_REPO ?? "stackframe-website-2026", - branch: process.env.SPIKE_BRANCH ?? "main", + owner: process.env.SPIKE_OWNER, + repo: process.env.SPIKE_REPO, + branch: process.env.SPIKE_BRANCH, }; // The COMPLETE config we want the repo's config file to reflect (the real flow diff --git a/apps/backend/src/app/api/latest/internal/config/github/apply/route.tsx b/apps/backend/src/app/api/latest/internal/config/github/apply/route.tsx index 9aa17cb22..5d9782754 100644 --- a/apps/backend/src/app/api/latest/internal/config/github/apply/route.tsx +++ b/apps/backend/src/app/api/latest/internal/config/github/apply/route.tsx @@ -123,8 +123,7 @@ export const POST = createSmartRouteHandler({ branchId, nowMs: Date.now(), outcome: result.mode === "commit-to-branch" - // The pushed commit sha is the last path segment of the commit URL. - ? { status: "success", commitUrl: result.commitUrl, newCommitHash: result.commitUrl.split("/").pop() } + ? { status: "success", commitUrl: result.commitUrl, newCommitHash: result.commitSha } : { status: "no-change" }, }); } catch (error) { @@ -133,7 +132,7 @@ export const POST = createSmartRouteHandler({ projectId, branchId, nowMs: Date.now(), - outcome: { status: "error", error: error instanceof Error ? error.message : "The config agent failed to apply the change." }, + outcome: { status: "error", error: "The config agent failed to apply the change." }, }).catch((e) => captureError("config-github-apply-record-error", e)); } }); diff --git a/apps/backend/src/app/api/latest/internal/config/github/cancel/route.tsx b/apps/backend/src/app/api/latest/internal/config/github/cancel/route.tsx index abe57ec2f..b40123b9d 100644 --- a/apps/backend/src/app/api/latest/internal/config/github/cancel/route.tsx +++ b/apps/backend/src/app/api/latest/internal/config/github/cancel/route.tsx @@ -6,15 +6,17 @@ import { stopConfigAgentSandbox } from "@/lib/config/repo-agent"; import { createSmartRouteHandler } from "@/route-handlers/smart-route-handler"; import { runAsynchronouslyAndWaitUntil } from "@/utils/background-tasks"; import { adaptSchema, adminAuthTypeSchema, yupNumber, yupObject, yupString } from "@hexclave/shared/dist/schema-fields"; -import { StatusError } from "@hexclave/shared/dist/utils/errors"; +import { StatusError, captureError } from "@hexclave/shared/dist/utils/errors"; export const maxDuration = 60; /** * Cancels the in-flight dashboard→GitHub config run. Atomically flips the run to * the terminal `cancelled` status (so the original run's late result is ignored) - * and hard-stops its sandbox. There is no revert: if the agent hadn't pushed yet - * the change is undone; if a commit already landed it stays (and the repo's + * and hard-stops its sandbox when available. If the sandbox id was never recorded + * (e.g. the run failed before booting a sandbox), the status is still flipped but + * no sandbox stop is attempted. There is no revert: if the agent hadn't pushed + * yet the change is undone; if a commit already landed it stays (and the repo's * config-sync workflow will eventually reconcile it). */ export const POST = createSmartRouteHandler({ @@ -54,6 +56,8 @@ export const POST = createSmartRouteHandler({ if (sandboxId) { runAsynchronouslyAndWaitUntil(stopConfigAgentSandbox(sandboxId)); + } else { + captureError("config-github-cancel", new Error("Cancelled a config agent run but no sandboxId was recorded; the sandbox may still be running.")); } return { statusCode: 200, bodyType: "json", body: { status: "cancelling" } }; diff --git a/apps/backend/src/lib/config/repo-agent.tsx b/apps/backend/src/lib/config/repo-agent.tsx index 12f156b37..e3c91d698 100644 --- a/apps/backend/src/lib/config/repo-agent.tsx +++ b/apps/backend/src/lib/config/repo-agent.tsx @@ -64,7 +64,7 @@ export type GithubRepoRef = { owner: string, repo: string, branch: string }; export type GithubTokenProvider = () => Promise; export type ConfigUpdatePushResult = - | { mode: "commit-to-branch", branch: string, commitUrl: string } + | { mode: "commit-to-branch", branch: string, commitUrl: string, commitSha: string } | { mode: "no-change" }; export class ConfigRepoAgentError extends Error { @@ -140,6 +140,10 @@ function tokenUrl(token: string, ref: Pick): st return `https://x-access-token:${token}@github.com/${ref.owner}/${ref.repo}.git`; } +function tokenlessUrl(ref: Pick): string { + return `https://github.com/${ref.owner}/${ref.repo}.git`; +} + // --------------------------------------------------------------------------- // Agent // --------------------------------------------------------------------------- @@ -413,11 +417,13 @@ export async function applyConfigUpdate(options: { await run(sandbox, "git", ["config", "--global", "user.email", GIT_BOT_EMAIL]); await run(sandbox, "git", ["config", "--global", "user.name", GIT_BOT_NAME]); - // Fresh shallow clone of just the target branch. `origin` keeps the tokenized - // URL so the agent's git (and our push) are authenticated; it dies with this - // sandbox, which is never snapshotted, so the token is never persisted. + // Fresh shallow clone of just the target branch. The tokenized URL is used + // only for the clone; immediately after, we reset `origin` to a tokenless URL + // so the agent (which has Bash access) cannot read the token from `.git/config` + // or `git remote -v`. The token is re-injected only for our own push command. await step(`Cloning ${ref.owner}/${ref.repo}@${ref.branch}…`); await run(sandbox, "git", ["clone", "--depth", "1", "--single-branch", "--branch", ref.branch, tokenUrl(githubToken, ref), REPO_DIR]); + await run(sandbox, "git", ["-C", REPO_DIR, "remote", "set-url", "origin", tokenlessUrl(ref)]); // Agent writes the COMPLETE config to the file — no dependency install, no // typecheck (the linked repo's CI validates the committed change). See buildUpdatePrompt. @@ -431,8 +437,9 @@ export async function applyConfigUpdate(options: { await run(sandbox, "git", ["-C", REPO_DIR, "commit", "-m", commitMessage]); const commitSha = await gitHead(sandbox); await step("Pushing the commit…"); - await run(sandbox, "git", ["-C", REPO_DIR, "push", "origin", `HEAD:refs/heads/${ref.branch}`]); - return { mode: "commit-to-branch", branch: ref.branch, commitUrl: `https://github.com/${ref.owner}/${ref.repo}/commit/${commitSha}` }; + // Re-inject the token for the push only (origin was reset to tokenless after clone). + await run(sandbox, "git", ["-C", REPO_DIR, "push", tokenUrl(githubToken, ref), `HEAD:refs/heads/${ref.branch}`]); + return { mode: "commit-to-branch", branch: ref.branch, commitUrl: `https://github.com/${ref.owner}/${ref.repo}/commit/${commitSha}`, commitSha }; } finally { await sandbox.stop().catch(() => {}); } diff --git a/apps/dashboard/src/lib/config-update.tsx b/apps/dashboard/src/lib/config-update.tsx index 111e22c36..1c5d357ea 100644 --- a/apps/dashboard/src/lib/config-update.tsx +++ b/apps/dashboard/src/lib/config-update.tsx @@ -550,7 +550,7 @@ function GithubPushBody({ onRunPhaseChange("idle"); dialogContext?.setGithubRunActive(false); if (run.status === "error") { - setErrorMessage(run.error ?? "The config agent failed to apply your change."); + setErrorMessage("The config agent failed to apply your change."); return "prevent-close"; } if (run.status === "cancelled") { @@ -569,7 +569,6 @@ function GithubPushBody({ setErrorMessage("Timed out waiting for the config agent. Your change may still be in progress — check the linked repository."); return "prevent-close"; } catch (error) { - const message = error instanceof Error ? error.message : "Unknown error pushing to GitHub."; captureError("config-update-github-agent", { projectId, owner: source.owner, @@ -581,7 +580,7 @@ function GithubPushBody({ setProgressMessage(null); onRunPhaseChange("idle"); dialogContext?.setGithubRunActive(false); - setErrorMessage(message); + setErrorMessage("Unknown error pushing to GitHub."); return "prevent-close"; } }, [adminApp, commitMessage, configUpdate, dialogContext, onRunPhaseChange, onSettle, projectId, scopeCheck, source]); diff --git a/packages/shared-backend/src/config-updater.ts b/packages/shared-backend/src/config-updater.ts index dff65e7b8..7f471e56d 100644 --- a/packages/shared-backend/src/config-updater.ts +++ b/packages/shared-backend/src/config-updater.ts @@ -188,8 +188,9 @@ function configFileExportsConfig(content: string, configFilePath: string): boole // jiti may fail to resolve imports that are valid in the user's project but // absent from the current process (e.g. relative asset imports, workspace // packages). For the structural sanity check we only need to know a runtime - // `config` binding still exists after the agent edited the file. - return /\bexport\s+const\s+config\b/.test(content); + // `config` binding still exists after the agent edited the file. Covers + // `export const config`, `export let config`, `export { config }`, etc. + return /\bexport\s+(?:const|let|var)\s+config\b/.test(content) || /\bexport\s*\{[^}]*\bconfig\b/.test(content); } } diff --git a/packages/shared/src/config-eval.ts b/packages/shared/src/config-eval.ts index 840be3eee..84dd8aeef 100644 --- a/packages/shared/src/config-eval.ts +++ b/packages/shared/src/config-eval.ts @@ -70,7 +70,12 @@ export function evalConfigFileContent(content: string, filePath: string): Parsed if (config === undefined) { throw new ConfigFileEvalError(`Invalid config in ${filePath}. The file must export a plain \`config\` object or "show-onboarding".`); } - if (typeof config === "string") return config; + if (typeof config === "string") { + if (config !== "show-onboarding") { + throw new ConfigFileEvalError(`Invalid config in ${filePath}. String config values must be "show-onboarding", got "${config}".`); + } + return config; + } if (isRecord(config)) return config; throw new ConfigFileEvalError(`Invalid config in ${filePath}. The file must export a plain \`config\` object or "show-onboarding".`); } @@ -116,6 +121,10 @@ import.meta.vitest?.test("evalConfigFileContent rejects content without config e expect(() => evalConfigFileContent("export const other = {};", "stack.config.ts")).toThrow(/must export/); }); +import.meta.vitest?.test("evalConfigFileContent rejects arbitrary string config values", ({ expect }) => { + expect(() => evalConfigFileContent('export const config = "arbitrary-string";', "stack.config.ts")).toThrow(/must be "show-onboarding"/); +}); + import.meta.vitest?.test("tryEvalConfigFileContent returns the config for valid exports", ({ expect }) => { expect(tryEvalConfigFileContent("export const config = { auth: { allowSignUp: true } };", "stack.config.ts")).toEqual({ auth: { allowSignUp: true }, diff --git a/packages/shared/src/schema-fields.ts b/packages/shared/src/schema-fields.ts index 8d94d0913..b604302e2 100644 --- a/packages/shared/src/schema-fields.ts +++ b/packages/shared/src/schema-fields.ts @@ -945,7 +945,7 @@ export const branchConfigSourceSchema = yupUnion( status: yupString().oneOf(["running", "success", "no-change", "error", "cancelled"]).defined(), started_at: yupNumber().defined(), finished_at: yupNumber().optional(), - commit_url: yupString().optional(), + commit_url: urlSchema.optional(), error: yupString().optional(), // Vercel Sandbox id of the in-flight run, recorded while `status === "running"` // so a cancel request (a different invocation) can hard-stop the sandbox.