From 8b09fa3479f803846d7baac784894da4022fdfaa Mon Sep 17 00:00:00 2001 From: mantrakp04 Date: Fri, 26 Jun 2026 17:54:44 -0700 Subject: [PATCH] fix: address PR review comments (commit-hash re-link, cancel stranding, elapsed timer, uuid, test gap) - index/commit route: gate commit_hash advance on committedRef identity so a mid-run repo re-link can't stamp a foreign commit SHA (cross-repo TOCTOU) - github-push-dialog: cancel handler now settles the dialog itself instead of relying on a poll loop that has already exited at awaiting_review - progress-content: useElapsedSeconds reacts to startedAt changes (fresh anchor) so a post-mount start time no longer freezes a stale offset - schema-fields: configAgentRunSchema.id uses .uuid() to match the @db.Uuid column - tests: cover the SyntaxError config-eval path and the re-link commit-hash case --- .../internal/config/github/commit/route.tsx | 2 +- apps/backend/src/lib/config/index.test.tsx | 34 +++++++++++++++++++ apps/backend/src/lib/config/index.tsx | 17 +++++++--- .../config-update/github-push-dialog.tsx | 11 ++++-- .../config-update/progress-content.tsx | 20 ++++++----- .../config-file.test.ts | 12 +++++++ packages/shared/src/config-eval.ts | 6 ++++ packages/shared/src/schema-fields.ts | 2 +- 8 files changed, 88 insertions(+), 16 deletions(-) diff --git a/apps/backend/src/app/api/latest/internal/config/github/commit/route.tsx b/apps/backend/src/app/api/latest/internal/config/github/commit/route.tsx index 0920bd8cb..81cf9bf07 100644 --- a/apps/backend/src/app/api/latest/internal/config/github/commit/route.tsx +++ b/apps/backend/src/app/api/latest/internal/config/github/commit/route.tsx @@ -85,7 +85,7 @@ export const POST = createSmartRouteHandler({ branchId, runId, nowMs: Date.now(), - outcome: { status: "success", commitUrl: result.commitUrl, newCommitHash: result.commitSha }, + outcome: { status: "success", commitUrl: result.commitUrl, newCommitHash: result.commitSha, committedRef: ref }, }); } catch (error) { if (!(error instanceof ConfigRepoCommitConflictError)) { diff --git a/apps/backend/src/lib/config/index.test.tsx b/apps/backend/src/lib/config/index.test.tsx index 765fdcce9..751a39af6 100644 --- a/apps/backend/src/lib/config/index.test.tsx +++ b/apps/backend/src/lib/config/index.test.tsx @@ -131,6 +131,7 @@ describe("config agent run state", () => { status: "success", commitUrl: "https://github.com/hexclave-validation/config-agent-validation/commit/new", newCommitHash: "new-commit", + committedRef: { owner: "hexclave-validation", repo: "config-agent-validation", branch: "main" }, }, }); @@ -139,6 +140,38 @@ describe("config agent run state", () => { expect(isRecord(source) ? source.commit_hash : null).toBe("new-commit"); }); + it("does not advance the commit hash when the branch was re-linked to a different repo mid-run", async () => { + const { projectId, branchId } = await createGithubLinkedBranch(); + const { runId } = await startConfigAgentRun({ projectId, branchId, nowMs: 1000 }); + await setConfigAgentRunAwaitingReview({ runId, change: { diff: "diff --git a/hexclave.config.ts b/hexclave.config.ts", baseSha: "abc123" } }); + + // The branch is re-linked to a DIFFERENT repo after the commit was pushed but + // before the result is recorded. + await globalPrismaClient.branchConfigOverride.update({ + where: { projectId_branchId: { projectId, branchId } }, + data: { source: { ...githubSource, repo: "some-other-repo", commit_hash: "other-base" } }, + }); + + await recordConfigAgentRunResult({ + projectId, + branchId, + runId, + nowMs: 3000, + outcome: { + status: "success", + commitUrl: "https://github.com/hexclave-validation/config-agent-validation/commit/new", + newCommitHash: "new-commit", + committedRef: { owner: "hexclave-validation", repo: "config-agent-validation", branch: "main" }, + }, + }); + + // The run still succeeds, but the new repo's source must NOT inherit a hash from + // the old repo's commit. + expect((await getConfigAgentRun({ projectId, branchId, runId }))?.status).toBe("success"); + const { source } = await readBranchRow(projectId, branchId); + expect(isRecord(source) ? source.commit_hash : null).toBe("other-base"); + }); + it("ignores a terminal result for an already-cancelled run", async () => { const { projectId, branchId } = await createGithubLinkedBranch(); const { runId } = await startConfigAgentRun({ projectId, branchId, nowMs: 1000 }); @@ -153,6 +186,7 @@ describe("config agent run state", () => { status: "success", commitUrl: "https://github.com/hexclave-validation/config-agent-validation/commit/stale", newCommitHash: "stale-commit", + committedRef: { owner: "hexclave-validation", repo: "config-agent-validation", branch: "main" }, }, }); diff --git a/apps/backend/src/lib/config/index.tsx b/apps/backend/src/lib/config/index.tsx index d3d82755f..bd3234209 100644 --- a/apps/backend/src/lib/config/index.tsx +++ b/apps/backend/src/lib/config/index.tsx @@ -15,7 +15,7 @@ import { PrismaClientTransaction, RawQuery, globalPrismaClient, rawQuery, retryT import { DEVELOPMENT_ENVIRONMENT_ENV_CONFIG_BLOCKED_MESSAGE, getEnvironmentConfigWriteBlockReason, isDevelopmentEnvironmentProject } from "../development-environment"; import { getLocalEmulatorFilePath, isLocalEmulatorEnabled, isLocalEmulatorProject, readConfigFromFile, writeConfigToFile } from "../local-emulator"; import { listPermissionDefinitionsFromConfig } from "../permissions"; -import type { CapturedChange, ConfigAgentInFlightStage } from "./repo-agent"; +import type { CapturedChange, ConfigAgentInFlightStage, GithubRepoRef } from "./repo-agent"; type BranchConfigSourceApi = yup.InferType; export type BranchConfigPushedError = { @@ -738,7 +738,7 @@ export async function recordConfigAgentRunResult(options: { runId: string, nowMs: number, outcome: - | { status: "success", commitUrl?: string, newCommitHash?: string } + | { status: "success", commitUrl?: string, newCommitHash?: string, committedRef: GithubRepoRef } | { status: "no-change" } | { status: "error", error: ConfigAgentSafeErrorMessage }, }): Promise { @@ -765,7 +765,11 @@ export async function recordConfigAgentRunResult(options: { data: { status: "success", finishedAt, commitUrl: options.outcome.commitUrl ?? null, sandboxId: null, stage: null, baseCommitSha: null }, }); // Advance the source's last-known commit when a commit landed and the branch - // is still GitHub-linked (locked in the same txn). + // is still linked to the SAME repo the commit was pushed against (locked in the + // same txn). A mid-run re-link to a different repo still reads as + // `pushed-from-github`, so identity — not just type — must match, or the new + // source would inherit a commit hash that only exists on the old repo. + const committedRef = options.outcome.committedRef; if (options.outcome.newCommitHash) { const sourceRows = await tx.$queryRaw<{ source: BranchConfigSourceApi | null }[]>` SELECT "source" FROM "BranchConfigOverride" @@ -773,7 +777,12 @@ export async function recordConfigAgentRunResult(options: { FOR UPDATE `; const source = sourceRows[0]?.source ?? null; - if (source?.type === "pushed-from-github") { + if ( + source?.type === "pushed-from-github" + && source.owner === committedRef.owner + && source.repo === committedRef.repo + && source.branch === committedRef.branch + ) { await tx.branchConfigOverride.update({ where: { projectId_branchId: { projectId: options.projectId, branchId: options.branchId } }, data: { source: { ...source, commit_hash: options.outcome.newCommitHash } as any }, diff --git a/apps/dashboard/src/components/config-update/github-push-dialog.tsx b/apps/dashboard/src/components/config-update/github-push-dialog.tsx index d92a23ec6..4f77ef6f3 100644 --- a/apps/dashboard/src/components/config-update/github-push-dialog.tsx +++ b/apps/dashboard/src/components/config-update/github-push-dialog.tsx @@ -428,8 +428,15 @@ function GithubPushBody({ } catch (error) { captureError("config-update-github-cancel", error); } - // The poll loop in handlePush will observe the terminal `cancelled` status and settle. - }, [adminApp, onErrorChange, onPhaseChange, onSettle]); + // Settle directly: the cancel request hard-stops the run, but the handlePush + // poll loop has already returned once the run reached "awaiting_review", so + // there is no observer to leave the non-dismissible "cancelling" phase. Drive + // the terminal transition here (mirroring the poll loop's `cancelled` branch) + // for every entry point and regardless of whether the cancel call threw. + onPhaseChange("check"); + onStageChange(null); + onSettle(false); + }, [adminApp, onErrorChange, onPhaseChange, onStageChange, onSettle]); const handleCommit = useCallback(async () => { const runId = runIdRef.current; diff --git a/apps/dashboard/src/components/config-update/progress-content.tsx b/apps/dashboard/src/components/config-update/progress-content.tsx index a3137e276..3be108ce7 100644 --- a/apps/dashboard/src/components/config-update/progress-content.tsx +++ b/apps/dashboard/src/components/config-update/progress-content.tsx @@ -26,20 +26,24 @@ function stageIndex(stage: AgentStage | null | undefined): number { /** * Live "seconds since the run started" counter. The run's `startedAt` is a - * wall-clock epoch value; we capture the start→mount offset ONCE (lazy state) - * and then advance on a monotonic clock. Recomputing the offset every render — + * wall-clock epoch value; we capture the start→now offset against a fresh + * monotonic anchor and then advance on that monotonic clock. The anchor and + * offset are recomputed whenever `startedAt` changes (e.g. when a flow renders + * the box before its real start timestamp is known), so the counter resets + * instead of freezing a stale offset. Recomputing the offset every render — * which is what the old code did — re-added the elapsed time on top of the * monotonic delta and made the timer tick at ~2× speed. */ function useElapsedSeconds(startedAt: number): number { - const [monotonicElapsedMs, setMonotonicElapsedMs] = useState(0); + const [elapsedMs, setElapsedMs] = useState(() => Math.max(0, currentEpochMsFromPerformance() - startedAt)); useEffect(() => { - const mountedAt = performance.now(); - const t = setInterval(() => setMonotonicElapsedMs(performance.now() - mountedAt), 1000); + const anchorPerfMs = performance.now(); + const offsetMs = Math.max(0, currentEpochMsFromPerformance() - startedAt); + setElapsedMs(offsetMs); + const t = setInterval(() => setElapsedMs(offsetMs + (performance.now() - anchorPerfMs)), 1000); return () => clearInterval(t); - }, []); - const [initialOffsetMs] = useState(() => Math.max(0, currentEpochMsFromPerformance() - startedAt)); - return Math.max(0, Math.floor((initialOffsetMs + monotonicElapsedMs) / 1000)); + }, [startedAt]); + return Math.max(0, Math.floor(elapsedMs / 1000)); } /** 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 7683eec0a..ecf1b3a58 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 @@ -215,6 +215,18 @@ describe("remote development environment config file", () => { ); }); + it("throws a helpful error when the config file is syntactically invalid", async () => { + const configPath = writeTempConfig(` + export const config = { + `); + + const { readConfigFile } = await import("./config-file"); + + await expect(readConfigFile(configPath)).rejects.toThrow( + `Failed to load config file ${configPath}.` + ); + }); + it("rejects modules without a valid config export", async () => { const configPath = writeTempConfig(` export const config = () => ({ auth: { allowSignUp: true } }); diff --git a/packages/shared/src/config-eval.ts b/packages/shared/src/config-eval.ts index 99009fa2b..c8bd39b16 100644 --- a/packages/shared/src/config-eval.ts +++ b/packages/shared/src/config-eval.ts @@ -129,3 +129,9 @@ import.meta.vitest?.test("evalConfigFileContent rejects missing config import ta export const config = { auth: missingConfigPart }; `, "/tmp/hexclave-missing-import-config.ts")).toThrow(); }); + +import.meta.vitest?.test("evalConfigFileContent rejects syntactically invalid content", ({ expect }) => { + // jiti surfaces a ParseError (not a ConfigFileEvalError), so callers route this + // to "Failed to load config file" rather than "Invalid config". + expect(() => evalConfigFileContent("export const config = {", "stack.config.ts")).toThrow(); +}); diff --git a/packages/shared/src/schema-fields.ts b/packages/shared/src/schema-fields.ts index 6cdb69cad..cb8669294 100644 --- a/packages/shared/src/schema-fields.ts +++ b/packages/shared/src/schema-fields.ts @@ -968,7 +968,7 @@ export const branchConfigSourceSchema = yupUnion( */ export const configAgentRunSchema = yupObject({ // The run's id (the `ConfigAgentRun` row id). The dashboard polls/cancels/commits this specific run by id. - id: yupString().defined(), + id: yupString().uuid().defined(), // "running": agent is working; "awaiting_review": agent done, diff ready, waiting for the user to commit; // "success" | "no-change" | "error" | "cancelled": terminal. status: yupString().oneOf(["running", "awaiting_review", "success", "no-change", "error", "cancelled"]).defined(),