mirror of
https://github.com/stack-auth/stack.git
synced 2026-06-30 21:01:54 +08:00
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
This commit is contained in:
parent
9b088a89d5
commit
8b09fa3479
@ -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)) {
|
||||
|
||||
@ -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" },
|
||||
},
|
||||
});
|
||||
|
||||
|
||||
@ -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<typeof branchConfigSourceSchema>;
|
||||
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<void> {
|
||||
@ -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 },
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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));
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@ -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 } });
|
||||
|
||||
@ -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();
|
||||
});
|
||||
|
||||
@ -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(),
|
||||
|
||||
Loading…
Reference in New Issue
Block a user