From 253ef3a9eca2eae7e35e6d233795331c758fdaf8 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Thu, 6 Aug 2026 20:55:57 +0200 Subject: [PATCH 1/5] feat(config,deploy-server,studio): config-drift dashboard - drift detail panel + one-click re-apply (issue #153) Adds forceStepIds to @redeploy/config's applyConfig() to allow re-executing already-journaled steps on demand, threads it through deploy-server's POST /api/apply-config as a validated ?forceStepIds= query param (never in the body, which is already the spec), and wires a studio drift detail panel (expected vs actual) with a confirm-gated per-step "Re-apply" button that refreshes drift badges via runVerifyConfig on success. Co-Authored-By: Claude Sonnet 5 --- apps/deploy-server/src/server.ts | 117 +++++- apps/deploy-server/test/apply-config.test.ts | 256 +++++++++++++ apps/studio/src/App.tsx | 83 ++++- apps/studio/src/components/Inspector.tsx | 148 +++++++- apps/studio/src/deploy/apply-config-client.ts | 48 ++- apps/studio/test/App.reapply.test.tsx | 352 ++++++++++++++++++ apps/studio/test/Inspector.test.tsx | 223 ++++++++++- apps/studio/test/apply-config-client.test.ts | 87 +++++ packages/config/src/execute/execute.ts | 67 +++- packages/config/src/execute/types.ts | 27 ++ packages/config/test/execute.test.ts | 191 ++++++++++ packages/config/test/ordered-steps.test.ts | 103 +++++ 12 files changed, 1661 insertions(+), 41 deletions(-) create mode 100644 apps/studio/test/App.reapply.test.tsx diff --git a/apps/deploy-server/src/server.ts b/apps/deploy-server/src/server.ts index c32a5c6..46477c6 100644 --- a/apps/deploy-server/src/server.ts +++ b/apps/deploy-server/src/server.ts @@ -151,6 +151,94 @@ function resolveNetworkForRequest( return { name: resolution.name, config: resolution.config }; } +/** Maximum number of ids accepted in a `?forceStepIds=` query param (issue #153). */ +const MAX_FORCE_STEP_IDS = 500; + +/** Maximum length (chars) of a single id within `?forceStepIds=` (issue #153). */ +const MAX_FORCE_STEP_ID_LENGTH = 256; + +/** + * Parse the OPTIONAL `?forceStepIds=,,...` query param for + * `POST /api/apply-config` (config-drift one-click re-apply — issue #153). + * + * IMPORTANT: `forceStepIds` is deliberately a QUERY PARAM, not a body field. + * `handleApplyConfig` passes the ENTIRE POST body as `spec` to `applyConfig()` + * (see its doc comment) — adding a sibling field to the body would corrupt + * spec validation and be a breaking wire-format change. The query string is + * the only place to carry this out-of-band control input. + * + * Format: a comma-separated list of step ids. Each id is trimmed; empty + * entries (from a leading/trailing/doubled comma, or the whole param being + * an empty string) are dropped silently — NOT an error. Duplicate ids (after + * trimming) are deduped. The list is capped at `MAX_FORCE_STEP_IDS` entries + * and each id at `MAX_FORCE_STEP_ID_LENGTH` characters — both violations are + * clean 400s. + * + * Ids that don't match any step in the spec are NOT validated here — + * `applyConfig()` silently ignores unknown forced ids (see + * `packages/config/src/execute/execute.ts`), which is exactly what we want + * for a drift report that may reference stale ids. + * + * On success: `{ ok: true, forceStepIds }` — `forceStepIds` is `undefined` + * when the param is absent OR present-but-empty-after-parsing (both mean "no + * forcing", matching `ApplyConfigOptions.forceStepIds`'s own + * omitted/empty-array equivalence). + * + * On failure: WRITES a 400 JSON response itself (mirroring + * `resolveNetworkForRequest`) and returns `{ ok: false }`. The error message + * is a FIXED string — the raw query value is never echoed back to the client. + * + * Callers MUST check `ok` and stop processing (the response may already be + * sent) — and MUST call this BEFORE opening the SSE stream, so a malformed + * value is a clean 400, not a `done` SSE frame. + */ +function parseForceStepIds( + req: IncomingMessage, + res: ServerResponse, +): { ok: true; forceStepIds: string[] | undefined } | { ok: false } { + const raw = getQueryParam(req.url, "forceStepIds"); + if (raw === undefined) { + return { ok: true, forceStepIds: undefined }; + } + + const ids = raw + .split(",") + .map((id) => id.trim()) + .filter((id) => id !== ""); + + if (ids.length === 0) { + // Empty string, or a string made up entirely of commas/whitespace — no + // ids to force. Not an error. + return { ok: true, forceStepIds: undefined }; + } + + if (ids.length > MAX_FORCE_STEP_IDS) { + const body = JSON.stringify({ + error: `forceStepIds: too many ids (max ${MAX_FORCE_STEP_IDS})`, + }); + res.writeHead(400, { + "Content-Type": "application/json", + "Content-Length": Buffer.byteLength(body), + }); + res.end(body); + return { ok: false }; + } + + if (ids.some((id) => id.length > MAX_FORCE_STEP_ID_LENGTH)) { + const body = JSON.stringify({ + error: `forceStepIds: id exceeds max length (${MAX_FORCE_STEP_ID_LENGTH} chars)`, + }); + res.writeHead(400, { + "Content-Type": "application/json", + "Content-Length": Buffer.byteLength(body), + }); + res.end(body); + return { ok: false }; + } + + return { ok: true, forceStepIds: [...new Set(ids)] }; +} + /** * Read this package's `version` field, for stamping into deployment * snapshots as `toolVersion`. Never throws — falls back to @@ -755,11 +843,23 @@ async function handleDeploy(req: IncomingMessage, res: ServerResponse): Promise< * come from the resolved network's config (server-side registry only — see * networks.ts), never from client input. * + * Forced re-apply (config-drift one-click re-apply — issue #153): accepts the + * OPTIONAL `?forceStepIds=,` query param (see `parseForceStepIds`), + * ALSO resolved/validated BEFORE the SSE stream opens so a malformed value + * (too many ids, or an id that's too long) yields a clean 400 rather than an + * SSE `done` frame. It is a query param, not a body field, because the whole + * POST body is already `spec` (see below) — adding a body field would corrupt + * spec validation. Parsed ids are forwarded to `applyConfig({ forceStepIds })` + * unchanged; ids that don't match any step in the spec are silently ignored + * by the engine (see `packages/config/src/execute/execute.ts`). + * * Idempotency / resumability: `stateDir` is set to the SAME * `deploymentDir` the network's deployment journal lives in, so * `applyConfig()`'s own `config-state.jsonl` journal makes a re-run of an * already-completed spec a no-op — every step comes back in - * `skippedStepIds`, none in `executedStepIds`, and `success` is still `true`. + * `skippedStepIds`, none in `executedStepIds`, and `success` is still `true` + * — UNLESS its id is listed in `forceStepIds`, in which case it re-executes + * and lands in `executedStepIds` instead. * * Error handling around `applyConfig()`: * - `ConfigExecError` — mapped exactly like `handleDeploy` maps @@ -779,7 +879,8 @@ async function handleDeploy(req: IncomingMessage, res: ServerResponse): Promise< * * Error responses (non-SSE): * - 413 body exceeds MAX_BODY_BYTES - * - 400 malformed JSON, or an unknown `?network=` name + * - 400 malformed JSON, an unknown `?network=` name, or an invalid + * `?forceStepIds=` value (too many ids / an id too long) * - 500 network configuration could not be loaded */ async function handleApplyConfig(req: IncomingMessage, res: ServerResponse): Promise { @@ -792,6 +893,11 @@ async function handleApplyConfig(req: IncomingMessage, res: ServerResponse): Pro const network = resolveNetworkForRequest(req, res); if (network === undefined) return; + // --- Parse + validate forceStepIds BEFORE opening the SSE stream --------- + const forceStepIdsResult = parseForceStepIds(req, res); + if (!forceStepIdsResult.ok) return; + const { forceStepIds } = forceStepIdsResult; + // --- Open SSE stream first so all outcomes flow through it --------------- res.writeHead(200, { "Content-Type": "text/event-stream", @@ -898,6 +1004,7 @@ async function handleApplyConfig(req: IncomingMessage, res: ServerResponse): Pro deployedAddresses, executor: wrappedExecutor, stateDir: deploymentDir, + forceStepIds, }); } catch (caughtErr) { if (caughtErr instanceof ConfigExecError) { @@ -1239,6 +1346,12 @@ async function handleVerifySource(req: IncomingMessage, res: ServerResponse): Pr * these four. `GET /api/networks` takes no query params but is likewise * matched on `pathname` for consistency (harmless if a client appends one * anyway). + * + * `POST /api/apply-config` ALSO accepts an OPTIONAL `?forceStepIds=,` + * query param (see `parseForceStepIds`) that forces re-execution of specific + * already-journaled steps — the one-click config-drift re-apply (issue #153). + * It is a query param rather than a body field because the whole POST body is + * already the config spec (see `handleApplyConfig`'s doc comment). */ export function handleRequest(req: IncomingMessage, res: ServerResponse): void { const { method, url } = req; diff --git a/apps/deploy-server/test/apply-config.test.ts b/apps/deploy-server/test/apply-config.test.ts index 0b2b190..a60887f 100644 --- a/apps/deploy-server/test/apply-config.test.ts +++ b/apps/deploy-server/test/apply-config.test.ts @@ -689,6 +689,262 @@ describe("POST /api/apply-config — secret leak prevention", () => { }); }); +// --------------------------------------------------------------------------- +// forceStepIds query param (issue #153 — config-drift one-click re-apply) +// --------------------------------------------------------------------------- + +describe("POST /api/apply-config — forceStepIds", () => { + it("parses and forwards forceStepIds to applyConfig", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: ["set-fee"], + skippedStepIds: ["grant-minter"], + completedStepIds: ["set-fee", "grant-minter"], + }); + + await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=set-fee,grant-minter", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: ["set-fee", "grant-minter"] }), + ); + }); + + it("composes correctly with ?network= (both params present)", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: ["set-fee"], + skippedStepIds: [], + completedStepIds: ["set-fee"], + }); + + const res = await doRequest( + port, + "POST", + "/api/apply-config?network=default&forceStepIds=set-fee", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(200); + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: ["set-fee"] }), + ); + }); + + it("absent forceStepIds param → forwarded as undefined", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + await doRequest(port, "POST", "/api/apply-config", JSON.stringify(VALID_CONFIG_SPEC)); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: undefined }), + ); + }); + + it("empty string forceStepIds param → treated as no ids (undefined)", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + const res = await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(200); + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: undefined }), + ); + }); + + it("a value made only of commas/whitespace → treated as no ids (undefined)", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=%20,,%20,", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: undefined }), + ); + }); + + it("dedupes repeated ids (and trims whitespace) before forwarding", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=set-fee,%20set-fee%20,grant-minter,set-fee", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: ["set-fee", "grant-minter"] }), + ); + }); + + it("over-cap id count → 400 Bad Request (non-SSE), applyConfig never called", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + const tooMany = Array.from({ length: 501 }, (_, i) => `s${i}`).join(","); + + const res = await doRequest( + port, + "POST", + `/api/apply-config?forceStepIds=${tooMany}`, + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(400); + expect(res.headers["content-type"]).toBe("application/json"); + const body = JSON.parse(res.body) as Record; + expect(typeof body["error"]).toBe("string"); + expect(configMod.applyConfig).not.toHaveBeenCalled(); + }); + + it("an id exceeding the max length → 400 Bad Request (non-SSE), never echoes the value", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + const tooLongId = "a".repeat(257); + + const res = await doRequest( + port, + "POST", + `/api/apply-config?forceStepIds=${tooLongId}`, + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(400); + const body = JSON.parse(res.body) as Record; + expect(typeof body["error"]).toBe("string"); + expect(res.body).not.toContain(tooLongId); + expect(configMod.applyConfig).not.toHaveBeenCalled(); + }); + + it("a 400 from an invalid forceStepIds value happens BEFORE the SSE stream opens (no event-stream header)", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + const tooLongId = "b".repeat(300); + + const res = await doRequest( + port, + "POST", + `/api/apply-config?forceStepIds=${tooLongId}`, + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(400); + expect(res.headers["content-type"]).not.toMatch(/text\/event-stream/); + }); + + it("max-length id (exactly 256 chars) is accepted", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + const exactId = "c".repeat(256); + + const res = await doRequest( + port, + "POST", + `/api/apply-config?forceStepIds=${exactId}`, + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(200); + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: [exactId] }), + ); + }); + + it("max-count ids (exactly 500) is accepted", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + const exactly500 = Array.from({ length: 500 }, (_, i) => `s${i}`).join(","); + + const res = await doRequest( + port, + "POST", + `/api/apply-config?forceStepIds=${exactly500}`, + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(res.statusCode).toBe(200); + const callArgs = configMod.applyConfig.mock.calls[0]![0]; + expect(callArgs.forceStepIds).toHaveLength(500); + }); +}); + // --------------------------------------------------------------------------- // Regression — existing routes unaffected // --------------------------------------------------------------------------- diff --git a/apps/studio/src/App.tsx b/apps/studio/src/App.tsx index aaa7720..dc79ea8 100644 --- a/apps/studio/src/App.tsx +++ b/apps/studio/src/App.tsx @@ -593,6 +593,14 @@ export function App() { const [applyConfigSuccess, setApplyConfigSuccess] = useState(null); const [applyConfigSteps, setApplyConfigSteps] = useState([]); + // Config-drift one-click re-apply (issue #153): when the user clicks + // "Re-apply" on a single drifted step's detail panel in the Inspector, we + // record WHICH step id here and reuse the SAME confirm modal / guard flow + // as the full "Apply config" button below — there is no separate + // unconfirmed write path. `null` means "no pending single-step re-apply" — + // a normal confirm applies the FULL current config as before. + const [pendingReapplyStepId, setPendingReapplyStepId] = useState(null); + // Provenance of `liveView`, tracked SEPARATELY from `viewKind` (bugfix, // issue #101 review). `viewKind` is the RENDER discriminator — it changes // to "plan" as soon as the user clicks Plan, even though `liveView` itself @@ -1193,13 +1201,28 @@ export function App() { // (mirrors onOpenDeployModal above; broadcasts real transactions). const onOpenApplyConfigModal = useCallback(() => { if (applying) return; + setPendingReapplyStepId(null); setShowApplyConfigModal(true); }, [applying]); const onCancelApplyConfig = useCallback(() => { setShowApplyConfigModal(false); + setPendingReapplyStepId(null); }, []); + // "Re-apply" (issue #153 — config-drift one-click re-apply): invoked from + // the Inspector's per-step drift detail panel. Records the target step id + // and opens the SAME confirm modal used by the full "Apply config" button + // — this is a REAL on-chain write and must never bypass that guard. + const onReapplyStep = useCallback( + (stepId: string) => { + if (applying) return; + setPendingReapplyStepId(stepId); + setShowApplyConfigModal(true); + }, + [applying], + ); + // "Apply config" (issue #151) — runs the current ConfigSpec's steps against // a REAL chain via POST /api/apply-config. Modeled on handleDeploy (real, // irreversible, confirm-gated) with handleVerify's config-source: the spec @@ -1207,17 +1230,27 @@ export function App() { // a different endpoint that actually broadcasts. On success, `liveView` is // refreshed from the server's post-apply DeploymentView so the Inspector's // config-step badges immediately reflect completion. + // + // Config-drift one-click re-apply (issue #153): when `pendingReapplyStepId` + // is set (via onReapplyStep, gated behind the SAME confirm modal as the + // full apply below), only that single step is forced to re-execute — + // forwarded as `forceStepIds: [pendingReapplyStepId]`. On a SUCCESSFUL + // forced re-apply we automatically re-run runVerifyConfig so the + // Inspector's drift badges refresh to reflect the newly-applied state, + // without requiring the user to click "Verify" again. const handleApplyConfig = useCallback(async () => { if (applying) return; + const forceStepIds = pendingReapplyStepId !== null ? [pendingReapplyStepId] : undefined; // Close the confirm modal immediately so a second confirm can't double-fire. setShowApplyConfigModal(false); + setPendingReapplyStepId(null); setApplying(true); setApplyConfigError(null); setApplyConfigSuccess(null); setApplyConfigSteps([]); try { - const result = await runApplyConfig(config, fetch, selectedNetwork ?? undefined); + const result = await runApplyConfig(config, fetch, selectedNetwork ?? undefined, forceStepIds); setApplyConfigSteps(result.steps); @@ -1233,6 +1266,18 @@ export function App() { setApplyConfigSuccess( `Config applied — ${executed} step(s) executed, ${skipped} already up to date (skipped).`, ); + + // Re-apply refresh (issue #153): a forced re-apply just changed + // on-chain state for `forceStepIds`, so the drift badges computed + // from the PREVIOUS /api/verify/config run are now stale. Re-run it + // (best-effort — a failure here doesn't turn the successful apply + // into an error) so the badges immediately reflect the new state. + if (forceStepIds !== undefined) { + const driftOutcome = await runVerifyConfig(config); + if (driftOutcome.ok) { + setDriftResults(driftOutcome.result.results); + } + } } else { // Surface the failing step(s)' own message(s), when present, alongside // the generic banner — without breaking the rest of the UI. @@ -1251,7 +1296,7 @@ export function App() { } finally { setApplying(false); } - }, [applying, config, selectedNetwork]); + }, [applying, config, selectedNetwork, pendingReapplyStepId]); const deployBtnStyle: React.CSSProperties = { ...btnStyle, @@ -1530,17 +1575,30 @@ export function App() {

- Confirm apply config + {pendingReapplyStepId !== null ? "Confirm re-apply step" : "Confirm apply config"}

-

- This will broadcast real transactions (setX / - grantRole / wire calls) against the persisted deployment on the - configured network. It is irreversible — gas - will be spent. Steps already applied in a previous run are - skipped, not re-run. -

+ {pendingReapplyStepId !== null ? ( +

+ This will broadcast a real transaction for step{" "} + {pendingReapplyStepId}{" "} + — FORCING it to re-run even though it was already applied + (config-drift re-apply). It is irreversible — + gas will be spent. All other steps are unaffected. +

+ ) : ( +

+ This will broadcast real transactions (setX / + grantRole / wire calls) against the persisted deployment on the + configured network. It is irreversible — gas + will be spent. Steps already applied in a previous run are + skipped, not re-run. +

+ )}

- Steps: {configStepCount} + Steps:{" "} + + {pendingReapplyStepId !== null ? 1 : configStepCount} +
Network:{" "} @@ -1570,7 +1628,7 @@ export function App() { onClick={() => { void handleApplyConfig(); }} data-testid="apply-config-confirm" > - Apply for real + {pendingReapplyStepId !== null ? "Re-apply for real" : "Apply for real"}

@@ -1751,6 +1809,7 @@ export function App() { driftResults={driftResults ?? undefined} sourceVerifyResults={sourceVerifyResult?.results} applyConfigResults={applyConfigSteps.length > 0 ? applyConfigSteps : undefined} + onReapplyStep={onReapplyStep} /> )} diff --git a/apps/studio/src/components/Inspector.tsx b/apps/studio/src/components/Inspector.tsx index 2b27a32..05ccbf9 100644 --- a/apps/studio/src/components/Inspector.tsx +++ b/apps/studio/src/components/Inspector.tsx @@ -20,7 +20,7 @@ * data constraint. */ -import { useMemo } from "react"; +import { useMemo, useState } from "react"; import { ReactFlow, Background, Controls } from "@xyflow/react"; import type { NodeTypes } from "@xyflow/react"; import "@xyflow/react/dist/style.css"; @@ -113,6 +113,70 @@ const failedBadgeStyle: React.CSSProperties = { color: "var(--color-danger-text)", }; +const driftDetailStyle: React.CSSProperties = { + marginTop: 6, + padding: 8, + borderRadius: 4, + background: "var(--color-bg-panel)", + border: "1px solid var(--color-border)", + fontSize: 11, +}; + +const driftDetailRowStyle: React.CSSProperties = { + marginBottom: 4, + wordBreak: "break-word", +}; + +const driftDetailLabelStyle: React.CSSProperties = { + fontWeight: 600, + color: "var(--color-text-secondary)", + marginRight: 4, +}; + +const reapplyButtonStyle: React.CSSProperties = { + marginTop: 6, + fontSize: 11, + fontWeight: 600, + padding: "3px 8px", + borderRadius: 4, + border: "1px solid var(--color-danger-text)", + background: "var(--color-danger-bg)", + color: "var(--color-danger-text)", + cursor: "pointer", +}; + +/** Maximum length (chars) of a rendered expected/actual value before truncation. */ +const MAX_DRIFT_DETAIL_VALUE_LENGTH = 200; + +/** + * Safely stringify an `unknown` drift `expected`/`actual` value for display. + * + * - Strings render as-is (no extra quoting). + * - Everything else is JSON.stringify'd (falling back to String() if that + * throws, e.g. for a value containing a BigInt or a circular reference). + * - Rendered via plain JSX text content ONLY (never dangerouslySetInnerHTML) + * — React escapes it automatically. + * - Truncated to MAX_DRIFT_DETAIL_VALUE_LENGTH chars so a pathologically + * large on-chain value never blows up the panel layout. + */ +function stringifyDriftValue(value: unknown): string { + let str: string; + if (value === undefined) { + str = "undefined"; + } else if (typeof value === "string") { + str = value; + } else { + try { + str = JSON.stringify(value) ?? String(value); + } catch { + str = String(value); + } + } + return str.length > MAX_DRIFT_DETAIL_VALUE_LENGTH + ? `${str.slice(0, MAX_DRIFT_DETAIL_VALUE_LENGTH)}… (truncated)` + : str; +} + const DRIFT_BADGE_STYLES: Record = { match: { ...badgeBaseStyle, @@ -140,15 +204,33 @@ const DRIFT_BADGE_STYLES: Record = { // Sub-components // --------------------------------------------------------------------------- +/** Drift statuses for which a one-click "Re-apply" is offered (issue #153). */ +const REAPPLIABLE_DRIFT_STATUSES: ReadonlySet = new Set([ + "drift", + "error", +]); + function ConfigStepCard({ step, drift, applyResult, + onReapplyStep, }: { step: ConfigStepStatus; drift?: ConfigDriftResultEntry; applyResult?: ApplyConfigStepResult; + /** + * Callback invoked with `step.id` when the user clicks "Re-apply" for a + * drifted/errored step (issue #153). The actual on-chain re-apply + + * confirmation flow lives in App.tsx — this component only surfaces the + * user's intent. + */ + onReapplyStep?: (stepId: string) => void; }) { + // Detail panel expand/collapse state — local to this card, closed by + // default. Only meaningful when `drift` is defined (see the toggle below). + const [detailExpanded, setDetailExpanded] = useState(false); + const badge = step.completed ? ( {badge} {/* Config-drift badge (issue #138) — only rendered once a - /api/verify/config run has produced a result for this step. */} + /api/verify/config run has produced a result for this step. + Clickable (issue #153) to expand/collapse the expected-vs-actual + detail panel below. */} {drift !== undefined && ( setDetailExpanded((v) => !v)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + setDetailExpanded((v) => !v); + } + }} > {drift.status} @@ -213,6 +307,43 @@ function ConfigStepCard({ {step.completedAt} )} + {/* Drift detail panel (issue #153) — expected vs actual + message, + revealed by clicking the drift badge above. Only rendered when a + drift result exists AND the panel is expanded. Values are rendered + as plain text (React-escaped, never dangerouslySetInnerHTML) and + defensively stringified/truncated via stringifyDriftValue. */} + {drift !== undefined && detailExpanded && ( +
+
+ expected: + {stringifyDriftValue(drift.expected)} +
+
+ actual: + {stringifyDriftValue(drift.actual)} +
+ {drift.message !== undefined && ( +
+ message: + {drift.message} +
+ )} + {/* Re-apply (issue #153) — only offered for a drifted/errored step; + a "match" step never gets a re-apply button. Only invokes the + callback — App.tsx owns the confirmation/guard flow before any + on-chain write actually happens. */} + {REAPPLIABLE_DRIFT_STATUSES.has(drift.status) && onReapplyStep !== undefined && ( + + )} +
+ )} ); } @@ -259,6 +390,15 @@ export interface InspectorProps { * "failed" badge, it never overrides the completed/pending badge. */ applyConfigResults?: ApplyConfigStepResult[]; + /** + * Callback invoked with a step id when the user clicks "Re-apply" on a + * drifted/errored step's detail panel (issue #153 — config-drift one-click + * re-apply). Omitted => no Re-apply button is rendered at all (e.g. a + * read-only inspector view). The actual on-chain write, confirmation modal, + * and post-success drift refresh are owned by the parent (App.tsx) — this + * component only surfaces user intent. + */ + onReapplyStep?: (stepId: string) => void; } export function Inspector({ @@ -268,6 +408,7 @@ export function Inspector({ driftResults, sourceVerifyResults, applyConfigResults, + onReapplyStep, }: InspectorProps) { const { nodes, edges } = useMemo( () => deploymentViewToFlow(view, sourceVerifyResults), @@ -324,6 +465,7 @@ export function Inspector({ step={step} drift={driftById.get(step.id)} applyResult={applyResultById.get(step.id)} + onReapplyStep={onReapplyStep} /> )) )} diff --git a/apps/studio/src/deploy/apply-config-client.ts b/apps/studio/src/deploy/apply-config-client.ts index 2798aa8..53d0bfb 100644 --- a/apps/studio/src/deploy/apply-config-client.ts +++ b/apps/studio/src/deploy/apply-config-client.ts @@ -181,26 +181,52 @@ export function parseApplyConfigFrame(frame: string): ApplyConfigEvent | null { * POST the config spec to /api/apply-config and stream the SSE response into * an ApplyConfigResult. * - * @param config - The ConfigSpec JSON object to send (as the bare request - * body — no envelope, same convention as runVerifyConfig). - * @param fetchFn - The fetch implementation to use (defaults to global fetch; - * accepted as a parameter for testability). - * @param network - Optional target network name (issue #139 convention), - * sent as `?network=` (URI-encoded). Omitted/undefined - * ⇒ no query param at all — resolves to the deploy-server's - * default network. + * @param config - The ConfigSpec JSON object to send (as the bare + * request body — no envelope, same convention as + * runVerifyConfig). + * @param fetchFn - The fetch implementation to use (defaults to global + * fetch; accepted as a parameter for testability). + * @param network - Optional target network name (issue #139 + * convention), sent as `?network=` + * (URI-encoded). Omitted/undefined ⇒ no `network` + * query param at all — resolves to the deploy-server's + * default network. + * @param forceStepIds - Optional list of step ids to force re-execution for, + * even if already journaled (config-drift one-click + * re-apply — issue #153). Sent as + * `?forceStepIds=,` (each id URI-encoded). + * Omitted/undefined/empty ⇒ no `forceStepIds` query + * param at all — identical to today's behaviour. See + * `apps/deploy-server/src/server.ts`'s + * `parseForceStepIds` for the server-side contract + * (comma-separated, trimmed, deduped, capped). */ export async function runApplyConfig( config: unknown, fetchFn: typeof fetch = fetch, network?: string, + forceStepIds?: readonly string[], ): Promise { let response: Response; + // Build the query string manually (not via URLSearchParams.toString(), + // which encodes spaces as "+" rather than "%20" and would change the + // wire format of the pre-existing `?network=` param) so `network` and + // `forceStepIds` compose correctly regardless of which are present. + // `forceStepIds` values are individually URI-encoded, then joined with a + // literal (unencoded) comma — matching the server's + // `?forceStepIds=,` contract (see `parseForceStepIds` in + // apps/deploy-server/src/server.ts, which decodes via URLSearchParams and + // then splits on ","). + const queryParts: string[] = []; + if (network !== undefined && network !== "") { + queryParts.push(`network=${encodeURIComponent(network)}`); + } + if (forceStepIds !== undefined && forceStepIds.length > 0) { + queryParts.push(`forceStepIds=${forceStepIds.map(encodeURIComponent).join(",")}`); + } const url = - network !== undefined && network !== "" - ? `/api/apply-config?network=${encodeURIComponent(network)}` - : "/api/apply-config"; + queryParts.length > 0 ? `/api/apply-config?${queryParts.join("&")}` : "/api/apply-config"; try { response = await fetchFn(url, { diff --git a/apps/studio/test/App.reapply.test.tsx b/apps/studio/test/App.reapply.test.tsx new file mode 100644 index 0000000..4e6bb5a --- /dev/null +++ b/apps/studio/test/App.reapply.test.tsx @@ -0,0 +1,352 @@ +/** + * App.reapply.test.tsx + * + * Integration tests for the config-drift one-click "Re-apply" flow (issue + * #153), wiring together: + * - Inspector's drift detail panel (click the drift badge to expand + * expected/actual/message, only offering "Re-apply" for a + * drifted/errored step). + * - App.tsx's onReapplyStep, which reuses the SAME confirm modal / guard + * flow as the existing "Apply config" button (no unconfirmed write path). + * - A successful forced re-apply automatically re-running + * POST /api/verify/config so the drift badge refreshes. + * + * Mirrors the patterns in App.verify.test.tsx (drift/verify fetch mocking) + * and App.applyConfig.test.tsx (SSE apply-config mocking + confirm gating). + */ + +import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { describe, it, expect, vi, afterEach } from "vitest"; +import App from "../src/App.js"; + +// --------------------------------------------------------------------------- +// SSE helpers (mirrors App.applyConfig.test.tsx) +// --------------------------------------------------------------------------- + +function enc(text: string): Uint8Array { + return new TextEncoder().encode(text); +} + +function makeStream(chunks: Uint8Array[]): ReadableStream { + return new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue(chunk); + } + controller.close(); + }, + }); +} + +function stepFrame( + stepId: string, + kind: "setX" | "grantRole" | "wire", + status: "executing" | "completed" | "failed", +): string { + return `event: step\ndata: ${JSON.stringify({ stepId, kind, status })}\n\n`; +} + +interface ConfiguredStep { + id: string; + kind: string; + completed: boolean; + completedAt?: string | null; +} + +function doneOkFrame( + executedStepIds: string[], + skippedStepIds: string[], + completedStepIds: string[], + configSteps: ConfiguredStep[], +): string { + const deployment = { + contracts: [], + configSteps: configSteps.map((s) => ({ + id: s.id, + kind: s.kind, + completed: s.completed, + completedAt: s.completedAt ?? null, + })), + warnings: [], + }; + return `event: done\ndata: ${JSON.stringify({ + success: true, + executedStepIds, + skippedStepIds, + completedStepIds, + deployment, + })}\n\n`; +} + +// --------------------------------------------------------------------------- +// Combined fetch dispatcher: /api/verify/config (queued responses, one per +// call — mirrors the "refresh after re-apply" scenario), /api/verify/source, +// and /api/apply-config?... (SSE, prefix-matched since it carries a query +// string). +// --------------------------------------------------------------------------- + +function mockCombinedFetch(opts: { + verifyConfigResponses: { status: number; body: unknown }[]; + verifySourceResponse: { status: number; body: unknown }; + applyConfigRaw: string; +}): { fetchSpy: ReturnType; verifyConfigCallUrls: string[] } { + const verifyConfigCallUrls: string[] = []; + let verifyConfigCallIndex = 0; + + const fetchSpy = vi.fn().mockImplementation((url: string) => { + if (url === "/api/verify/config") { + verifyConfigCallUrls.push(url); + const idx = Math.min(verifyConfigCallIndex, opts.verifyConfigResponses.length - 1); + const response = opts.verifyConfigResponses[idx]; + verifyConfigCallIndex += 1; + return Promise.resolve(new Response(JSON.stringify(response.body), { status: response.status })); + } + if (url === "/api/verify/source") { + return Promise.resolve( + new Response(JSON.stringify(opts.verifySourceResponse.body), { + status: opts.verifySourceResponse.status, + }), + ); + } + if (url.startsWith("/api/apply-config")) { + return Promise.resolve( + new Response(makeStream([enc(opts.applyConfigRaw)]), { + status: 200, + headers: { "Content-Type": "text/event-stream" }, + }), + ); + } + return Promise.reject(new Error(`Unexpected fetch to ${url}`)); + }); + + return { fetchSpy, verifyConfigCallUrls }; +} + +afterEach(() => { + vi.restoreAllMocks(); + document.body.innerHTML = ""; +}); + +/** Runs Verify so the Inspector shows a "drift" badge for the sample view's "setFee" step. */ +async function setupWithDrift(fetchSpy: ReturnType) { + render(); + fireEvent.click(screen.getByTestId("deploy-verify-button")); + await waitFor(() => { + expect(screen.getByTestId("config-step-setFee-drift")).not.toBeNull(); + }); + expect(fetchSpy).toHaveBeenCalled(); +} + +// --------------------------------------------------------------------------- +// Drift detail panel — expand/collapse via the drift badge +// --------------------------------------------------------------------------- + +describe("App — drift detail panel", () => { + it("clicking the drift badge reveals expected/actual/message; a Re-apply button is present for 'drift'", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { + clean: false, + results: [{ id: "setFee", status: "drift", expected: 500, actual: 999, message: "mismatch" }], + }, + }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + + const detail = screen.getByTestId("config-step-setFee-drift-detail"); + expect(detail).not.toBeNull(); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("500"); + expect(screen.getByTestId("config-step-setFee-drift-actual").textContent).toContain("999"); + expect(screen.getByTestId("config-step-setFee-drift-message").textContent).toContain("mismatch"); + expect(screen.getByTestId("config-step-setFee-reapply")).not.toBeNull(); + + // Clicking again collapses it. + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + }); + + it("does NOT render a Re-apply button for a 'match' step", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { status: 200, body: { clean: true, results: [{ id: "setFee", status: "match", expected: 500, actual: 500 }] } }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + render(); + fireEvent.click(screen.getByTestId("deploy-verify-button")); + await waitFor(() => { + expect(screen.getByTestId("config-step-setFee-drift")).not.toBeNull(); + }); + + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-drift-detail")).not.toBeNull(); + expect(screen.queryByTestId("config-step-setFee-reapply")).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// Re-apply flow — confirm-gated, forceStepIds, post-success drift refresh +// --------------------------------------------------------------------------- + +describe("App — Re-apply flow", () => { + it("clicking Re-apply opens the SAME confirm modal (no unconfirmed write)", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + + // Modal open; no POST to /api/apply-config yet. + const modal = screen.getByTestId("apply-config-modal"); + expect(modal).not.toBeNull(); + expect(modal.textContent).toContain("setFee"); + expect(screen.getByTestId("apply-config-step-count").textContent).toBe("1"); + expect(fetchSpy.mock.calls.some((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config"))).toBe( + false, + ); + }); + + it("confirming the re-apply modal POSTs /api/apply-config with forceStepIds=", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + { status: 200, body: { clean: true, results: [{ id: "setFee", status: "match", expected: 999, actual: 999 }] } }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame( + ["setFee"], + [], + ["setFee"], + [{ id: "setFee", kind: "setX", completed: true, completedAt: "2026-08-06T00:00:00.000Z" }], + ), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + await waitFor(() => { + const applyCalls = fetchSpy.mock.calls.filter((c: unknown[]) => + (c[0] as string).startsWith("/api/apply-config"), + ); + expect(applyCalls).toHaveLength(1); + }); + + const applyUrl = fetchSpy.mock.calls.find((c: unknown[]) => + (c[0] as string).startsWith("/api/apply-config"), + )![0] as string; + expect(applyUrl).toBe("/api/apply-config?forceStepIds=setFee"); + }); + + it("on success, automatically re-runs /api/verify/config so the drift badge refreshes to 'match'", async () => { + const { fetchSpy, verifyConfigCallUrls } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + { status: 200, body: { clean: true, results: [{ id: "setFee", status: "match", expected: 999, actual: 999 }] } }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: + stepFrame("setFee", "setX", "executing") + + stepFrame("setFee", "setX", "completed") + + doneOkFrame( + ["setFee"], + [], + ["setFee"], + [{ id: "setFee", kind: "setX", completed: true, completedAt: "2026-08-06T00:00:00.000Z" }], + ), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + await waitFor(() => { + expect(verifyConfigCallUrls.length).toBe(2); + }); + + await waitFor(() => { + expect(screen.getByTestId("config-step-setFee-drift").textContent).toBe("match"); + }); + }); + + it("Cancel on the re-apply confirm modal does NOT POST /api/apply-config", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-cancel")); + + expect(screen.queryByTestId("apply-config-modal")).toBeNull(); + expect( + fetchSpy.mock.calls.some((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config")), + ).toBe(false); + }); + + it("re-apply confirm wording is distinct from the normal full-apply confirm wording", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + + const modal = screen.getByTestId("apply-config-modal"); + expect(modal.textContent).toContain("Confirm re-apply step"); + expect(modal.textContent).not.toContain("Confirm apply config"); + expect(screen.getByTestId("apply-config-confirm").textContent).toBe("Re-apply for real"); + }); +}); diff --git a/apps/studio/test/Inspector.test.tsx b/apps/studio/test/Inspector.test.tsx index 88849ff..cf558e8 100644 --- a/apps/studio/test/Inspector.test.tsx +++ b/apps/studio/test/Inspector.test.tsx @@ -9,8 +9,8 @@ * Mirrors App.test.tsx and ConfigPanel.test.tsx patterns. */ -import { render, screen } from "@testing-library/react"; -import { describe, it, expect } from "vitest"; +import { render, screen, fireEvent } from "@testing-library/react"; +import { describe, it, expect, vi } from "vitest"; import { resolve } from "node:path"; import { Inspector } from "../src/components/Inspector.js"; import { readDeployment } from "../src/inspector/load-deployment.js"; @@ -272,6 +272,225 @@ describe("Inspector — config-drift badges", () => { }); }); +// --------------------------------------------------------------------------- +// Inspector — drift detail panel + Re-apply button (issue #153) +// --------------------------------------------------------------------------- + +describe("Inspector — drift detail panel", () => { + it("no detail panel is rendered before the drift badge is clicked", () => { + render( + , + ); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + }); + + it("clicking the drift badge for a drifted step reveals expected/actual/message", () => { + render( + , + ); + + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + + const detail = screen.getByTestId("config-step-setFee-drift-detail"); + expect(detail).not.toBeNull(); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("500"); + expect(screen.getByTestId("config-step-setFee-drift-actual").textContent).toContain("999"); + expect(screen.getByTestId("config-step-setFee-drift-message").textContent).toContain( + "Expected 500 but got 999", + ); + }); + + it("clicking the drift badge again collapses the detail panel", () => { + render( + , + ); + + const badge = screen.getByTestId("config-step-setFee-drift"); + fireEvent.click(badge); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).not.toBeNull(); + fireEvent.click(badge); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + }); + + it("the badge is keyboard-activatable (Enter key toggles the panel)", () => { + render( + , + ); + const badge = screen.getByTestId("config-step-setFee-drift"); + fireEvent.keyDown(badge, { key: "Enter" }); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).not.toBeNull(); + }); + + it("a detail panel is offered for a 'match' step too, but with no Re-apply button", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-drift-detail")).not.toBeNull(); + expect(screen.queryByTestId("config-step-setFee-reapply")).toBeNull(); + }); + + it("stringifies a non-string expected/actual value (object) as JSON, not [object Object]", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("0xabc"); + expect(screen.getByTestId("config-step-setFee-drift-actual").textContent).toContain("0xdef"); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).not.toContain( + "[object Object]", + ); + }); + + it("truncates a very long expected/actual value instead of rendering it in full", () => { + const longValue = "x".repeat(5000); + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + const expectedEl = screen.getByTestId("config-step-setFee-drift-expected"); + expect(expectedEl.textContent!.length).toBeLessThan(longValue.length); + expect(expectedEl.textContent).toContain("truncated"); + }); + + it("falls back to String() when JSON.stringify throws (e.g. a BigInt value)", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + // JSON.stringify(500n) throws a TypeError; the fallback String(500n) is "500". + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("500"); + }); + + it("renders no message row when drift.message is undefined", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.queryByTestId("config-step-setFee-drift-message")).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// Inspector — Re-apply button (issue #153) +// --------------------------------------------------------------------------- + +describe("Inspector — Re-apply button", () => { + it("renders a Re-apply button for a 'drift' status step once the panel is expanded", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + const btn = screen.getByTestId("config-step-setFee-reapply"); + expect(btn).not.toBeNull(); + expect(btn.textContent).toBe("Re-apply"); + }); + + it("renders a Re-apply button for an 'error' status step", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-reapply")).not.toBeNull(); + }); + + it("does NOT render a Re-apply button for a 'match' status step", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.queryByTestId("config-step-setFee-reapply")).toBeNull(); + }); + + it("does NOT render a Re-apply button for a 'skipped' status step", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.queryByTestId("config-step-setFee-reapply")).toBeNull(); + }); + + it("clicking Re-apply calls onReapplyStep with the step's id", () => { + const onReapplyStep = vi.fn(); + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + expect(onReapplyStep).toHaveBeenCalledTimes(1); + expect(onReapplyStep).toHaveBeenCalledWith("setFee"); + }); + + it("no Re-apply button is rendered at all when onReapplyStep is omitted, even for a drifted step", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.queryByTestId("config-step-setFee-reapply")).toBeNull(); + }); +}); + // --------------------------------------------------------------------------- // Inspector — source-verification badges (issue #138) // --------------------------------------------------------------------------- diff --git a/apps/studio/test/apply-config-client.test.ts b/apps/studio/test/apply-config-client.test.ts index d1bf262..6b2ce10 100644 --- a/apps/studio/test/apply-config-client.test.ts +++ b/apps/studio/test/apply-config-client.test.ts @@ -336,3 +336,90 @@ describe("runApplyConfig — network param", () => { expect(mockFetch).toHaveBeenCalledWith("/api/apply-config", expect.anything()); }); }); + +// --------------------------------------------------------------------------- +// runApplyConfig — forceStepIds param (issue #153) +// --------------------------------------------------------------------------- + +describe("runApplyConfig — forceStepIds param", () => { + it("omitted forceStepIds → POSTs with no query string (when network also omitted)", async () => { + const raw = doneOkFrame([], [], [], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch); + + expect(mockFetch).toHaveBeenCalledWith("/api/apply-config", expect.anything()); + }); + + it("undefined forceStepIds → identical to omitted", async () => { + const raw = doneOkFrame([], [], [], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, undefined, undefined); + + expect(mockFetch).toHaveBeenCalledWith("/api/apply-config", expect.anything()); + }); + + it("empty forceStepIds array → treated the same as omitted (no query string)", async () => { + const raw = doneOkFrame([], [], [], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, undefined, []); + + expect(mockFetch).toHaveBeenCalledWith("/api/apply-config", expect.anything()); + }); + + it("a single forceStepIds id → POSTs to /api/apply-config?forceStepIds=", async () => { + const raw = doneOkFrame(["set-fee"], [], ["set-fee"], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, undefined, ["set-fee"]); + + expect(mockFetch).toHaveBeenCalledWith("/api/apply-config?forceStepIds=set-fee", expect.anything()); + }); + + it("multiple forceStepIds ids are comma-joined", async () => { + const raw = doneOkFrame(["set-fee", "grant-minter"], [], ["set-fee", "grant-minter"], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, undefined, ["set-fee", "grant-minter"]); + + expect(mockFetch).toHaveBeenCalledWith( + "/api/apply-config?forceStepIds=set-fee,grant-minter", + expect.anything(), + ); + }); + + it("composes correctly with a network param: both present, joined with &", async () => { + const raw = doneOkFrame(["set-fee"], [], ["set-fee"], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, "sepolia", ["set-fee"]); + + expect(mockFetch).toHaveBeenCalledWith( + "/api/apply-config?network=sepolia&forceStepIds=set-fee", + expect.anything(), + ); + }); + + it("network omitted but forceStepIds present → only forceStepIds in the query string", async () => { + const raw = doneOkFrame(["set-fee"], [], ["set-fee"], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, "", ["set-fee"]); + + expect(mockFetch).toHaveBeenCalledWith("/api/apply-config?forceStepIds=set-fee", expect.anything()); + }); + + it("a step id with special characters is URI-encoded", async () => { + const raw = doneOkFrame([], [], [], null, "warn"); + const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); + + await runApplyConfig({}, mockFetch, undefined, ["weird id/1"]); + + expect(mockFetch).toHaveBeenCalledWith( + `/api/apply-config?forceStepIds=${encodeURIComponent("weird id/1")}`, + expect.anything(), + ); + }); +}); diff --git a/packages/config/src/execute/execute.ts b/packages/config/src/execute/execute.ts index 06a9b22..e20668a 100644 --- a/packages/config/src/execute/execute.ts +++ b/packages/config/src/execute/execute.ts @@ -87,6 +87,37 @@ * wire → target = safeAddresses.get(step.into) * function = step.function * args = [safeAddresses.get(step.source)] + * + * FORCING RE-EXECUTION (config-drift one-click re-apply) + * ======================================================== + * + * `ApplyConfigOptions.forceStepIds` (optional) lets a caller force specific + * already-journaled steps to re-execute — e.g. a drift-detection UI found + * that a step's live on-chain state no longer matches the declared spec and + * the user clicked "re-apply". + * + * - `forceStepIds` is folded into a `Set` once, alongside + * `safeAddresses`, at the top of `applyConfig`. + * - `executeStep` treats a forced step exactly like a non-journaled step: + * it is NOT skipped, so its refs/read args ARE resolved and + * `executor.execute()` IS called even though `alreadyCompleted` already + * contains its id. + * - On success, a NEW completion record is appended to the journal for that + * id. The journal is append-only NDJSON, and `readCompletedStepIds` folds + * ALL lines (in file order) into a `Set` of ids — duplicate ids + * collapse harmlessly (the newer `completedAt` value is simply never read + * back by anything other than external tooling inspecting the raw file), + * so appending a second record for the same id is always safe. See + * journal.ts's DESIGN and APPEND-ONLY DURABILITY sections. + * - Forced steps land in `executedStepIds`, never in `skippedStepIds`. + * - `forceStepIds` entries that don't match any step id in the (validated) + * spec are silently ignored — a drift report may reference ids from a + * spec that has since changed. + * - Forcing never changes the two ordering guarantees above: + * `spec.steps` still fully precedes `spec.orderedSteps`, and + * `orderedSteps` still execute in strict array order with + * journal-before-next semantics — a forced step still blocks the next + * `orderedSteps` entry until its own execution (forced or not) completes. */ import { validateConfig } from "../steps/validate.js"; @@ -310,7 +341,7 @@ async function buildConfigCall( * wrapped. */ export async function applyConfig(options: ApplyConfigOptions): Promise { - const { spec, deployedAddresses, executor, stateDir } = options; + const { spec, deployedAddresses, executor, stateDir, forceStepIds } = options; // --- 1. Build a safe address lookup (defence-in-depth) -------------------- // @@ -321,6 +352,12 @@ export async function applyConfig(options: ApplyConfigOptions): Promise(Object.entries(deployedAddresses)); + // Fold forceStepIds (if any) into a Set once, up front. Ids that don't + // match any step in the spec are simply never looked up below — no + // validation or filtering against the spec is needed here (see FORCING + // RE-EXECUTION in the module doc comment above). + const forcedStepIds = new Set(forceStepIds ?? []); + // --- 2. Validate the spec FIRST (fail fast) -------------------------------- // // Pass the same keys (from the Map) so validateConfig can check that all @@ -346,20 +383,28 @@ export async function applyConfig(options: ApplyConfigOptions): Promise { - if (alreadyCompleted.has(step.id)) { - // Already journaled from a previous run — skip. No ref/read resolution - // and no executor call of any kind happens for a skipped step. + const forced = forcedStepIds.has(step.id); + if (alreadyCompleted.has(step.id) && !forced) { + // Already journaled from a previous run and not forced — skip. No + // ref/read resolution and no executor call of any kind happens for a + // skipped step. skippedStepIds.push(step.id); return; } diff --git a/packages/config/src/execute/types.ts b/packages/config/src/execute/types.ts index f78972b..c5fa912 100644 --- a/packages/config/src/execute/types.ts +++ b/packages/config/src/execute/types.ts @@ -181,6 +181,33 @@ export interface ApplyConfigOptions { * REQUIRED for idempotency — pass a stable, deployment-specific path. */ stateDir: string; + /** + * Optional list of step ids to FORCE re-execution for, even if they are + * already recorded as complete in the journal. Intended for one-click + * "re-apply" of a drifted step (live on-chain state no longer matches the + * declared spec) — see the config-drift-reapply feature. + * + * Semantics: + * - A step whose id appears here executes exactly like a fresh + * (non-journaled) step: its ref/read args are resolved, executor.execute() + * is called, and — only on success — a FRESH completion record is + * appended to the journal (the journal is append-only NDJSON and + * `readCompletedStepIds` folds all records for an id into a Set, so a + * duplicate id with a newer `completedAt` is safe; see journal.ts). + * - Forced steps are reported in `executedStepIds`, never in + * `skippedStepIds`. + * - Ids listed here that do not match any step in the (validated) spec are + * silently ignored — this keeps the option resilient to stale ids from a + * previously-computed drift report referencing a spec that has since + * changed. + * - Forcing does NOT change ordering guarantees: `spec.steps` still run + * before `spec.orderedSteps`, `orderedSteps` still run in strict array + * order, and a forced step still blocks the next `orderedSteps` entry + * until it completes (journal-before-next semantics). + * - Omitting `forceStepIds` (or passing `undefined`) is byte-for-byte + * behaviour-identical to today: every already-journaled step is skipped. + */ + forceStepIds?: readonly string[]; } // --------------------------------------------------------------------------- diff --git a/packages/config/test/execute.test.ts b/packages/config/test/execute.test.ts index 5da9e70..7a5fec4 100644 --- a/packages/config/test/execute.test.ts +++ b/packages/config/test/execute.test.ts @@ -934,6 +934,197 @@ describe("applyConfig — journal malformed-line tolerance", () => { }); }); +// --------------------------------------------------------------------------- +// Test: forceStepIds — force re-execution of already-journaled steps +// --------------------------------------------------------------------------- + +describe("applyConfig — forceStepIds", () => { + it("re-executes an already-journaled step whose id is in forceStepIds", async () => { + const stateDir = await makeTempDir(); + + // First run: fully applies the spec. + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + // Second run: force "grant-minter" — it must re-execute even though it's + // already journaled; the other two steps remain skipped. + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["grant-minter"], + }); + + expect(result2.success).toBe(true); + expect(executor2.calls.map((c) => c.stepId)).toEqual(["grant-minter"]); + expect(result2.executedStepIds).toEqual(["grant-minter"]); + expect(result2.skippedStepIds).toEqual(["set-fee", "wire-token-into-vault"]); + expect(result2.completedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + }); + + it("non-forced journaled steps still skip when forceStepIds targets a different step", async () => { + const stateDir = await makeTempDir(); + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["set-fee"], + }); + + expect(executor2.calls.map((c) => c.stepId)).toEqual(["set-fee"]); + expect(result2.executedStepIds).toEqual(["set-fee"]); + expect(result2.skippedStepIds).toEqual(["grant-minter", "wire-token-into-vault"]); + }); + + it("a forced id absent from the spec is silently ignored (no-op, no error)", async () => { + const stateDir = await makeTempDir(); + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["this-step-does-not-exist-in-the-spec"], + }); + + expect(result2.success).toBe(true); + expect(executor2.calls).toHaveLength(0); + expect(result2.executedStepIds).toEqual([]); + expect(result2.skippedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + }); + + it("a forced step appends a NEW journal record (duplicate id) alongside the original", async () => { + const stateDir = await makeTempDir(); + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + const executor2 = new FakeExecutor(); + await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["set-fee"], + }); + + const journalFile = path.join(stateDir, "config-state.jsonl"); + const content = await fs.promises.readFile(journalFile, "utf8"); + const ids = content + .trim() + .split("\n") + .filter(Boolean) + .map((l) => (JSON.parse(l) as { id: string }).id); + + // "set-fee" appears twice: once from the first run, once from the forced + // re-run. readCompletedStepIds folds duplicates into a Set, so this is + // safe and does not affect skip/resume behaviour. + expect(ids.filter((id) => id === "set-fee")).toHaveLength(2); + }); + + it("forced steps still resolve read args and invoke executor.read() again", async () => { + const stateDir = await makeTempDir(); + const spec: ConfigSpec = { + version: 1, + steps: [ + { + kind: "setX", + id: "set-decimals", + target: "vault", + function: "setDecimalsCache", + args: [{ kind: "read", contract: "token", function: "decimals" }], + }, + ], + }; + const deployedAddresses = { vault: ADDRESSES.vault, token: ADDRESSES.token }; + + const executor1 = new ReadFakeExecutor("18"); + await applyConfig({ spec, deployedAddresses, executor: executor1, stateDir }); + expect(executor1.reads).toHaveLength(1); + + const executor2 = new ReadFakeExecutor("19"); + const result2 = await applyConfig({ + spec, + deployedAddresses, + executor: executor2, + stateDir, + forceStepIds: ["set-decimals"], + }); + + expect(executor2.reads).toHaveLength(1); + expect(executor2.calls).toHaveLength(1); + expect(executor2.calls[0].args[0]).toBe("19"); + expect(result2.executedStepIds).toEqual(["set-decimals"]); + expect(result2.skippedStepIds).toEqual([]); + }); + + it("empty forceStepIds array is a no-op — identical to omitting it", async () => { + const stateDir = await makeTempDir(); + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: [], + }); + + expect(executor2.calls).toHaveLength(0); + expect(result2.executedStepIds).toEqual([]); + expect(result2.skippedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + }); + + it("undefined forceStepIds is a no-op — identical to today's behaviour", async () => { + const stateDir = await makeTempDir(); + const executor1 = new FakeExecutor(); + const result1 = await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + expect(result1.executedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + + const executor2 = new FakeExecutor(); + const result2 = await applyConfig( + makeOptions(threeStepSpec, executor2, stateDir), + ); + + expect(executor2.calls).toHaveLength(0); + expect(result2.executedStepIds).toEqual([]); + expect(result2.skippedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + }); + + it("forcing a step that has never run (fresh spec) executes it normally — not an error", async () => { + const stateDir = await makeTempDir(); + const executor = new FakeExecutor(); + const result = await applyConfig({ + ...makeOptions(threeStepSpec, executor, stateDir), + forceStepIds: ["grant-minter"], + }); + + // Fresh run: forcing has no observable effect since nothing was journaled yet. + expect(result.executedStepIds).toEqual([ + "set-fee", + "grant-minter", + "wire-token-into-vault", + ]); + expect(result.skippedStepIds).toEqual([]); + }); +}); + // --------------------------------------------------------------------------- // Test: `read` args — resolved via executor.read() // --------------------------------------------------------------------------- diff --git a/packages/config/test/ordered-steps.test.ts b/packages/config/test/ordered-steps.test.ts index f7e7124..5f9e11a 100644 --- a/packages/config/test/ordered-steps.test.ts +++ b/packages/config/test/ordered-steps.test.ts @@ -746,6 +746,109 @@ describe("applyConfig — orderedSteps resume semantics", () => { }); }); +// --------------------------------------------------------------------------- +// Execution: forceStepIds preserves ordering guarantees across both lists +// --------------------------------------------------------------------------- + +describe("applyConfig — forceStepIds preserves ordering guarantees", () => { + it("forcing an unordered step still runs all steps before all orderedSteps", async () => { + const stateDir = await makeTempDir(); + const spec: ConfigSpec = { + version: 1, + steps: [ + { kind: "setX", id: "u1", target: "feeController", function: "f" }, + { kind: "setX", id: "u2", target: "token", function: "g" }, + ], + orderedSteps: [ + { kind: "wire", id: "o1", source: "token", into: "vault", function: "setToken" }, + { + kind: "grantRole", + id: "o2", + target: "token", + role: "MINTER_ROLE", + account: { kind: "ref", contract: "minterContract" }, + }, + ], + }; + + // Full first run. + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(spec, executor1, stateDir)); + + // Force the first unordered step on the second run — steps must still + // run (conceptually) before orderedSteps, and orderedSteps must still be + // skipped (not forced) in strict order. + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(spec, executor2, stateDir), + forceStepIds: ["u1"], + }); + + expect(executor2.calls.map((c) => c.stepId)).toEqual(["u1"]); + expect(result2.executedStepIds).toEqual(["u1"]); + expect(result2.skippedStepIds).toEqual(["u2", "o1", "o2"]); + }); + + it("forcing an orderedSteps step re-executes it in place, without disturbing array order", async () => { + const stateDir = await makeTempDir(); + const spec: ConfigSpec = { + version: 1, + steps: [{ kind: "setX", id: "u1", target: "feeController", function: "f" }], + orderedSteps: [ + { kind: "setX", id: "o1", target: "token", function: "g" }, + { kind: "setX", id: "o2", target: "vault", function: "h" }, + { kind: "wire", id: "o3", source: "token", into: "vault", function: "setToken" }, + ], + }; + + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(spec, executor1, stateDir)); + + // Force the MIDDLE ordered step ("o2"). It must execute again while "o1" + // and "o3" remain skipped — execution order (were it to run) is still + // steps-then-orderedSteps in strict array order. + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(spec, executor2, stateDir), + forceStepIds: ["o2"], + }); + + expect(executor2.calls.map((c) => c.stepId)).toEqual(["o2"]); + expect(result2.executedStepIds).toEqual(["o2"]); + expect(result2.skippedStepIds).toEqual(["u1", "o1", "o3"]); + }); + + it("forcing multiple steps across both lists re-executes each exactly once, in spec order", async () => { + const stateDir = await makeTempDir(); + const spec: ConfigSpec = { + version: 1, + steps: [ + { kind: "setX", id: "u1", target: "feeController", function: "f" }, + { kind: "setX", id: "u2", target: "token", function: "g" }, + ], + orderedSteps: [ + { kind: "setX", id: "o1", target: "vault", function: "h" }, + { kind: "setX", id: "o2", target: "registry", function: "i" }, + ], + }; + + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(spec, executor1, stateDir)); + + const executor2 = new FakeExecutor(); + const result2 = await applyConfig({ + ...makeOptions(spec, executor2, stateDir), + forceStepIds: ["u2", "o1"], + }); + + // Order of execution follows spec order (steps then orderedSteps), NOT + // the order ids were listed in forceStepIds. + expect(executor2.calls.map((c) => c.stepId)).toEqual(["u2", "o1"]); + expect(result2.executedStepIds).toEqual(["u2", "o1"]); + expect(result2.skippedStepIds).toEqual(["u1", "o2"]); + }); +}); + // --------------------------------------------------------------------------- // Execution: address references (RefArg) in orderedSteps // --------------------------------------------------------------------------- From 6213e2f8af501cc62b0cd4e7d080626a8ce3b52d Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Thu, 6 Aug 2026 21:31:48 +0200 Subject: [PATCH 2/5] fix(config,deploy-server,studio): address review findings on config-drift re-apply (issue #153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes B1-B5 from the review of feat/issue-159... err feat/issue-153-config-drift-reapply: - B1 (correctness+security, most serious): the re-apply confirm modal claimed "1 step, all others unaffected" while POSTing the studio's ENTIRE config spec — applyConfig() executes every not-yet-applied step in the POSTed spec regardless of forceStepIds. Now narrows the POSTed spec to just the target step (narrowConfigToStep), making the modal's claim structurally true, and rewords the "already applied" claim which was false for never-journaled drifted steps. - B2: pin that a forced step whose executor throws does not double-journal. - B3: cover the `if (applying) return;` guard in onReapplyStep — the only thing preventing a second real broadcast from a second click in flight. - B4: cover the untested `driftOutcome.ok` false branch on the post-re-apply drift refresh, and document the deliberate on-failure behaviour (leave driftResults stale rather than clearing, unlike handleVerify). - B5: fix the forceStepIds comma round-trip — client now sends one repeated query param per id (rejecting any id containing a comma before sending) instead of comma-joining; server merges repeated params via URLSearchParams.getAll() instead of dropping all but the first. Also folds in the non-blocking cleanup items from the same review pass (fresh completedAt assertion, full-apply verify-call-count test, Inspector keyboard/stringify branch coverage, constant drift-badge style objects, and a corrected doc comment on the forceStepIds cap-before-dedupe ordering). Co-Authored-By: Claude Sonnet 5 --- apps/deploy-server/src/server.ts | 57 +++- apps/deploy-server/test/apply-config.test.ts | 50 ++++ apps/studio/src/App.tsx | 35 ++- apps/studio/src/components/Inspector.tsx | 20 +- apps/studio/src/deploy/apply-config-client.ts | 51 +++- apps/studio/src/spec/narrow-config.ts | 63 +++++ apps/studio/test/App.reapply.test.tsx | 247 +++++++++++++++++- .../studio/test/App.reapplyNarrowing.test.tsx | 196 ++++++++++++++ apps/studio/test/Inspector.test.tsx | 58 ++++ apps/studio/test/apply-config-client.test.ts | 26 +- apps/studio/test/narrow-config.test.ts | 63 +++++ packages/config/test/execute.test.ts | 49 +++- 12 files changed, 878 insertions(+), 37 deletions(-) create mode 100644 apps/studio/src/spec/narrow-config.ts create mode 100644 apps/studio/test/App.reapplyNarrowing.test.tsx create mode 100644 apps/studio/test/narrow-config.test.ts diff --git a/apps/deploy-server/src/server.ts b/apps/deploy-server/src/server.ts index 46477c6..b573f8c 100644 --- a/apps/deploy-server/src/server.ts +++ b/apps/deploy-server/src/server.ts @@ -87,6 +87,25 @@ function getQueryParam(url: string | undefined, name: string): string | undefine return value === null ? undefined : value; } +/** + * Like `getQueryParam`, but returns EVERY value of a repeated query param + * (`?name=a&name=b` → `["a", "b"]`) instead of only the first + * (`URLSearchParams.get` semantics). Returns `[]` when `url` is undefined, + * has no `?`, or the param is absent — never throws (same WHATWG-parser + * avoidance rationale as `getQueryParam`). + * + * Used by `parseForceStepIds` (issue #153 review, finding B5) so that + * `?forceStepIds=a&forceStepIds=b` MERGES both values instead of silently + * dropping all but the first, as a bare `.get()` would. + */ +function getQueryParamAll(url: string | undefined, name: string): string[] { + if (url === undefined) return []; + const qIdx = url.indexOf("?"); + if (qIdx === -1) return []; + const queryString = url.slice(qIdx + 1).split("#")[0] ?? ""; + return new URLSearchParams(queryString).getAll(name); +} + /** * Resolve the target network for an incoming request, from the OPTIONAL * `?network=` query param — shared by `handleSimulate`, `handleDeploy`, @@ -167,12 +186,30 @@ const MAX_FORCE_STEP_ID_LENGTH = 256; * spec validation and be a breaking wire-format change. The query string is * the only place to carry this out-of-band control input. * - * Format: a comma-separated list of step ids. Each id is trimmed; empty - * entries (from a leading/trailing/doubled comma, or the whole param being - * an empty string) are dropped silently — NOT an error. Duplicate ids (after - * trimming) are deduped. The list is capped at `MAX_FORCE_STEP_IDS` entries - * and each id at `MAX_FORCE_STEP_ID_LENGTH` characters — both violations are - * clean 400s. + * Format: BOTH forms are accepted and MERGED — + * - a comma-separated list within a single value: `?forceStepIds=a,b` + * - a repeated query param: `?forceStepIds=a&forceStepIds=b` + * `getQueryParamAll` collects every occurrence of the param (unlike a bare + * `URLSearchParams.get`, which silently keeps only the first), and each + * occurrence is then split on `,` — so `?forceStepIds=a,b&forceStepIds=c` + * yields `["a", "b", "c"]`. Each id is trimmed; empty entries (from a + * leading/trailing/doubled comma, or a value being an empty string) are + * dropped silently — NOT an error. + * + * ORDER OF CHECKS (matters for the cap): the `MAX_FORCE_STEP_IDS` count cap + * is enforced on the trimmed, non-empty id list BEFORE deduping — so, e.g., + * 501 copies of the SAME id is a 400, even though it would collapse to a + * single id after dedup. This is a deliberate input-size guard (bounding the + * work done per request before any dedup pass), not a bug. The + * `MAX_FORCE_STEP_ID_LENGTH` per-id length cap is checked next, and + * duplicate ids (after trimming) are deduped only in the final returned + * list. + * + * RESERVED DELIMITER: because a single value is split on `,`, a step id + * containing a literal comma can never be forced via this param — it would + * always be split into two ids. `apply-config-client.ts`'s `runApplyConfig` + * enforces this client-side (rejecting such an id before sending) so this is + * a documented contract, not a silent footgun. * * Ids that don't match any step in the spec are NOT validated here — * `applyConfig()` silently ignores unknown forced ids (see @@ -196,13 +233,13 @@ function parseForceStepIds( req: IncomingMessage, res: ServerResponse, ): { ok: true; forceStepIds: string[] | undefined } | { ok: false } { - const raw = getQueryParam(req.url, "forceStepIds"); - if (raw === undefined) { + const rawValues = getQueryParamAll(req.url, "forceStepIds"); + if (rawValues.length === 0) { return { ok: true, forceStepIds: undefined }; } - const ids = raw - .split(",") + const ids = rawValues + .flatMap((raw) => raw.split(",")) .map((id) => id.trim()) .filter((id) => id !== ""); diff --git a/apps/deploy-server/test/apply-config.test.ts b/apps/deploy-server/test/apply-config.test.ts index a60887f..24c9786 100644 --- a/apps/deploy-server/test/apply-config.test.ts +++ b/apps/deploy-server/test/apply-config.test.ts @@ -919,6 +919,56 @@ describe("POST /api/apply-config — forceStepIds", () => { ); }); + it("repeated ?forceStepIds= params MERGE (not just the first one) — issue #153 review finding B5", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=set-fee&forceStepIds=grant-minter", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ forceStepIds: ["set-fee", "grant-minter"] }), + ); + }); + + it("a repeated param value can itself be comma-separated — both forms combine", async () => { + process.env["DEPLOYER_PRIVATE_KEY"] = + "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + const configMod = vi.mocked(await import("@redeploy/config")); + configMod.applyConfig.mockResolvedValue({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + }); + + await doRequest( + port, + "POST", + "/api/apply-config?forceStepIds=set-fee,grant-minter&forceStepIds=wire-token-into-vault", + JSON.stringify(VALID_CONFIG_SPEC), + ); + + expect(configMod.applyConfig).toHaveBeenCalledWith( + expect.objectContaining({ + forceStepIds: ["set-fee", "grant-minter", "wire-token-into-vault"], + }), + ); + }); + it("max-count ids (exactly 500) is accepted", async () => { process.env["DEPLOYER_PRIVATE_KEY"] = "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; diff --git a/apps/studio/src/App.tsx b/apps/studio/src/App.tsx index dc79ea8..6536c2a 100644 --- a/apps/studio/src/App.tsx +++ b/apps/studio/src/App.tsx @@ -68,6 +68,7 @@ import { graphToTemplate } from "./templates/serialize.js"; import type { ParamSelection } from "./templates/serialize.js"; import { graphToSpec } from "./spec/graph-to-spec.js"; import type { GraphEdge } from "./spec/graph-to-spec.js"; +import { narrowConfigToStep } from "./spec/narrow-config.js"; import { toGraphNodes } from "./spec/project-nodes.js"; import { overviewEdges } from "./spec/overview-edges.js"; import type { ContractNodeData, ViewMode } from "./spec/types.js"; @@ -1238,9 +1239,22 @@ export function App() { // forced re-apply we automatically re-run runVerifyConfig so the // Inspector's drift badges refresh to reflect the newly-applied state, // without requiring the user to click "Verify" again. + // + // SPEC NARROWING (issue #153 review, finding B1): `forceStepIds` alone only + // controls which ALREADY-journaled step is forced to re-run — it does + // nothing to stop every OTHER not-yet-applied step still on the canvas from + // also executing, since `applyConfig()` server-side runs every step in the + // POSTed spec that isn't already journaled. The confirm modal below + // promises "1 step, nothing else broadcast" — to make that literally true + // (not just a UI claim), a single-step re-apply POSTs a spec NARROWED to + // just `pendingReapplyStepId` (see narrowConfigToStep's doc comment for why + // this is safe: ref resolution never depends on sibling steps). A full + // (non-forced) apply still POSTs the complete `config` unchanged. const handleApplyConfig = useCallback(async () => { if (applying) return; const forceStepIds = pendingReapplyStepId !== null ? [pendingReapplyStepId] : undefined; + const specToSend = + pendingReapplyStepId !== null ? narrowConfigToStep(config, pendingReapplyStepId) : config; // Close the confirm modal immediately so a second confirm can't double-fire. setShowApplyConfigModal(false); setPendingReapplyStepId(null); @@ -1250,7 +1264,7 @@ export function App() { setApplyConfigSteps([]); try { - const result = await runApplyConfig(config, fetch, selectedNetwork ?? undefined, forceStepIds); + const result = await runApplyConfig(specToSend, fetch, selectedNetwork ?? undefined, forceStepIds); setApplyConfigSteps(result.steps); @@ -1272,6 +1286,18 @@ export function App() { // from the PREVIOUS /api/verify/config run are now stale. Re-run it // (best-effort — a failure here doesn't turn the successful apply // into an error) so the badges immediately reflect the new state. + // + // On-failure behaviour, chosen deliberately (issue #153 review, + // finding B4/non-blocking): unlike `handleVerify` — a user-initiated + // check where a failed drift call clears `driftResults` to `null` so + // no stale badge is shown as if it were current — this refresh is a + // best-effort background follow-up to a real state-changing action + // that already succeeded. Clearing to `null` here would replace a + // (possibly still-accurate) previous drift badge with "no drift data + // at all", which reads as MORE alarming/uncertain than simply leaving + // the last-known badge in place. So on failure we intentionally leave + // `driftResults` untouched — the UI keeps showing the last-known + // (now possibly stale) drift status rather than clearing it. if (forceStepIds !== undefined) { const driftOutcome = await runVerifyConfig(config); if (driftOutcome.ok) { @@ -1581,9 +1607,10 @@ export function App() {

This will broadcast a real transaction for step{" "} {pendingReapplyStepId}{" "} - — FORCING it to re-run even though it was already applied - (config-drift re-apply). It is irreversible — - gas will be spent. All other steps are unaffected. + only — FORCING it to run now, regardless of whether it ran + before (config-drift re-apply). It is{" "} + irreversible — gas will be spent. No other + step will be sent or broadcast.

) : (

diff --git a/apps/studio/src/components/Inspector.tsx b/apps/studio/src/components/Inspector.tsx index 05ccbf9..1b8a9b7 100644 --- a/apps/studio/src/components/Inspector.tsx +++ b/apps/studio/src/components/Inspector.tsx @@ -177,24 +177,34 @@ function stringifyDriftValue(value: unknown): string { : str; } +// Clickable badge base style (issue #153 review, non-blocking performance +// note): the drift badge is always `role="button"`/clickable (toggles the +// detail panel), so `cursor: "pointer"` is baked into every entry below AT +// MODULE SCOPE — these are constant object references, never recreated per +// render. Previously the render spread `{ ...DRIFT_BADGE_STYLES[status], +// cursor: "pointer" }` inline in JSX, allocating a brand-new style object for +// every badge on every render; baking the property in here restores a +// constant reference (matching every other style constant in this file). +const clickableBadgeBaseStyle: React.CSSProperties = { ...badgeBaseStyle, cursor: "pointer" }; + const DRIFT_BADGE_STYLES: Record = { match: { - ...badgeBaseStyle, + ...clickableBadgeBaseStyle, background: "var(--color-success-bg-strong)", color: "var(--color-success-text-strong)", }, drift: { - ...badgeBaseStyle, + ...clickableBadgeBaseStyle, background: "var(--color-danger-bg)", color: "var(--color-danger-text)", }, error: { - ...badgeBaseStyle, + ...clickableBadgeBaseStyle, background: "var(--color-warning-bg)", color: "var(--color-warning-text)", }, skipped: { - ...badgeBaseStyle, + ...clickableBadgeBaseStyle, background: "var(--color-bg-elevated)", color: "var(--color-text-muted)", }, @@ -266,7 +276,7 @@ function ConfigStepCard({ detail panel below. */} {drift !== undefined && ( ,` (each id URI-encoded). - * Omitted/undefined/empty ⇒ no `forceStepIds` query - * param at all — identical to today's behaviour. See + * re-apply — issue #153). Sent as ONE REPEATED query + * param per id — `?forceStepIds=&forceStepIds=` + * (each id URI-encoded) — rather than a single + * comma-joined value. This avoids the comma round-trip + * bug where `encodeURIComponent(",")` (`%2C`) gets + * URL-DECODED back to a literal `,` by the server's + * `URLSearchParams` BEFORE the server splits on `,`, + * silently splitting a single id containing a comma + * into two ids (issue #153 review finding B5). Because + * of this, `,` is a RESERVED DELIMITER: no forced step + * id may contain one. Any id containing a comma is + * rejected HERE, client-side, before any request is + * sent (`ok: false`, no network call, no fetch made). + * Omitted/undefined/empty + * ⇒ no `forceStepIds` query param at all — identical + * to today's behaviour. See * `apps/deploy-server/src/server.ts`'s * `parseForceStepIds` for the server-side contract - * (comma-separated, trimmed, deduped, capped). + * (repeated params AND the legacy comma-separated + * single-value form are both accepted and merged, + * then trimmed/deduped/capped). */ export async function runApplyConfig( config: unknown, @@ -209,21 +223,36 @@ export async function runApplyConfig( ): Promise { let response: Response; + // RESERVED DELIMITER guard (issue #153 review finding B5) — reject BEFORE + // building the URL / sending anything. See the `forceStepIds` param doc + // above for the full comma round-trip rationale. + if (forceStepIds !== undefined) { + const commaId = forceStepIds.find((id) => id.includes(",")); + if (commaId !== undefined) { + return { + ok: false, + error: `forceStepIds: id "${commaId}" cannot contain a comma — "," is a reserved delimiter`, + steps: [], + }; + } + } + // Build the query string manually (not via URLSearchParams.toString(), // which encodes spaces as "+" rather than "%20" and would change the // wire format of the pre-existing `?network=` param) so `network` and // `forceStepIds` compose correctly regardless of which are present. - // `forceStepIds` values are individually URI-encoded, then joined with a - // literal (unencoded) comma — matching the server's - // `?forceStepIds=,` contract (see `parseForceStepIds` in - // apps/deploy-server/src/server.ts, which decodes via URLSearchParams and - // then splits on ","). + // Each `forceStepIds` id is sent as its OWN repeated `forceStepIds=` + // query param (URI-encoded) — never comma-joined — matching the server's + // `getQueryParamAll`-based merge in `parseForceStepIds` + // (apps/deploy-server/src/server.ts). const queryParts: string[] = []; if (network !== undefined && network !== "") { queryParts.push(`network=${encodeURIComponent(network)}`); } if (forceStepIds !== undefined && forceStepIds.length > 0) { - queryParts.push(`forceStepIds=${forceStepIds.map(encodeURIComponent).join(",")}`); + for (const id of forceStepIds) { + queryParts.push(`forceStepIds=${encodeURIComponent(id)}`); + } } const url = queryParts.length > 0 ? `/api/apply-config?${queryParts.join("&")}` : "/api/apply-config"; diff --git a/apps/studio/src/spec/narrow-config.ts b/apps/studio/src/spec/narrow-config.ts new file mode 100644 index 0000000..902daac --- /dev/null +++ b/apps/studio/src/spec/narrow-config.ts @@ -0,0 +1,63 @@ +/** + * narrow-config.ts + * + * Narrows a full `ConfigSpec` down to a single step, for the config-drift + * one-click re-apply flow (issue #153). + * + * ## Why this exists (issue #153 review, finding B1) + * + * `App.tsx`'s "Re-apply" confirm modal claims that clicking it will broadcast + * a real transaction for ONE named step and that "All other steps are + * unaffected". Before this fix, `handleApplyConfig` POSTed the studio's + * ENTIRE current `config` (every step on the canvas) to `/api/apply-config` + * with only `forceStepIds` narrowed to the single target step. The server + * forwards the whole POST body as `spec` to `applyConfig()` + * (`apps/deploy-server/src/server.ts`), and `applyConfig()` executes EVERY + * step in that spec that is not already journaled — `forceStepIds` only + * controls which ALREADY-journaled steps are forced to re-run; it does + * nothing to steps that were never journaled in the first place. So clicking + * "Re-apply" on one drifted step could ALSO broadcast real transactions for + * every not-yet-applied step still on the canvas — directly contradicting the + * modal's promise, on an irreversible, gas-spending path. + * + * The fix: when re-applying a single step, narrow the POSTed spec itself down + * to just that one step, so the server-side `applyConfig()` has no other step + * to execute even before `forceStepIds` is considered — the modal's "1 step, + * nothing else" claim becomes structurally true, not just a UI label. + * + * ## Why this is safe + * + * `validateConfig` (`packages/config/src/steps/validate.ts`) resolves every + * ref (`target`/`source`/`into`/`account`/args of kind `ref` or `read`) ONLY + * against the deployment's known deployed contract ids (`deployedAddresses` + * on the server) — never against sibling steps in the spec. Dropping every + * other step from the spec therefore cannot break the forced step's own ref + * resolution; the deployed contracts it references are still deployed and + * still resolvable regardless of which other config steps happen to be + * present in the same POST body. + */ + +import type { ConfigSpec } from "@redeploy/config/steps"; + +/** + * Return a new `ConfigSpec` containing ONLY the step whose `id` matches + * `stepId`, preserving every other top-level field (`version`) unchanged. + * + * The matching step is kept in whichever list it was found in (`steps` vs + * `orderedSteps`) — the other list is filtered to empty rather than dropped, + * so the returned spec always has the same shape (`{ version, steps, + * orderedSteps }`) as the input, just narrowed. + * + * If no step in either list has the given id, the returned spec has both + * lists empty — `applyConfig()` will then execute nothing, which is the + * safe/inert outcome for a stale/unknown step id (mirrors how a forced id + * that matches no step is silently ignored server-side, see + * `packages/config/src/execute/execute.ts`'s FORCING RE-EXECUTION section). + */ +export function narrowConfigToStep(spec: ConfigSpec, stepId: string): ConfigSpec { + return { + ...spec, + steps: spec.steps.filter((s) => s.id === stepId), + orderedSteps: spec.orderedSteps?.filter((s) => s.id === stepId), + }; +} diff --git a/apps/studio/test/App.reapply.test.tsx b/apps/studio/test/App.reapply.test.tsx index 4e6bb5a..52c091b 100644 --- a/apps/studio/test/App.reapply.test.tsx +++ b/apps/studio/test/App.reapply.test.tsx @@ -15,7 +15,7 @@ * and App.applyConfig.test.tsx (SSE apply-config mocking + confirm gating). */ -import { render, screen, fireEvent, waitFor } from "@testing-library/react"; +import { render, screen, fireEvent, waitFor, within } from "@testing-library/react"; import { describe, it, expect, vi, afterEach } from "vitest"; import App from "../src/App.js"; @@ -303,6 +303,56 @@ describe("App — Re-apply flow", () => { }); }); + it("when the post-re-apply drift refresh ITSELF fails (500), the apply success banner still shows, no error banner appears, and the drift badge stays at its last-known ('drift') status — issue #153 review finding B4", async () => { + const { fetchSpy, verifyConfigCallUrls } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + // The SECOND /api/verify/config call (the post-re-apply refresh) + // fails — exercises the `if (driftOutcome.ok)` false branch at + // App.tsx's handleApplyConfig, which was previously untested and + // inexpressible with this helper. + { status: 500, body: { error: "internal error" } }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: + stepFrame("setFee", "setX", "executing") + + stepFrame("setFee", "setX", "completed") + + doneOkFrame( + ["setFee"], + [], + ["setFee"], + [{ id: "setFee", kind: "setX", completed: true, completedAt: "2026-08-06T00:00:00.000Z" }], + ), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + // Both /api/verify/config calls happened (the second one failed). + await waitFor(() => { + expect(verifyConfigCallUrls.length).toBe(2); + }); + + // The apply itself succeeded — its success banner shows, unaffected by + // the drift-refresh failure (which is best-effort, per handleApplyConfig's + // doc comment). + await waitFor(() => { + expect(screen.queryByTestId("apply-config-success")).not.toBeNull(); + }); + expect(screen.queryByTestId("apply-config-error")).toBeNull(); + + // The drift badge keeps its LAST-KNOWN status ("drift") rather than being + // cleared to a blank/no-data state or crashing the app — see the + // deliberate on-failure-behaviour comment in App.tsx's handleApplyConfig. + expect(screen.getByTestId("config-step-setFee-drift").textContent).toBe("drift"); + }); + it("Cancel on the re-apply confirm modal does NOT POST /api/apply-config", async () => { const { fetchSpy } = mockCombinedFetch({ verifyConfigResponses: [ @@ -349,4 +399,199 @@ describe("App — Re-apply flow", () => { expect(modal.textContent).not.toContain("Confirm apply config"); expect(screen.getByTestId("apply-config-confirm").textContent).toBe("Re-apply for real"); }); + + it("the re-apply modal's copy is accurate (issue #153 review finding B1) — no false 'already applied' claim, and truthfully says no other step is sent", async () => { + const { fetchSpy } = mockCombinedFetch({ + verifyConfigResponses: [ + { + status: 200, + body: { clean: false, results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }] }, + }, + ], + verifySourceResponse: { status: 200, body: { success: true, skipped: false, results: [] } }, + applyConfigRaw: doneOkFrame([], [], [], []), + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + + const modal = screen.getByTestId("apply-config-modal"); + // Old (fixed) copy asserted the step "was already applied" — false when a + // drifted step was never journaled. The reworded copy makes no claim + // about journal history at all. + expect(modal.textContent).not.toContain("already applied"); + // Now structurally true (the POSTed spec is narrowed to this one step — + // see App.reapplyNarrowing.test.tsx), not just an unchecked UI label. + expect(modal.textContent?.toLowerCase()).toContain("no other step"); + }); +}); + +// --------------------------------------------------------------------------- +// `if (applying) return;` guard in onReapplyStep (issue #153 review finding +// B3) — the Inspector and its Re-apply button stay mounted and clickable +// while an apply is in flight, so this guard is the only thing preventing a +// SECOND real on-chain broadcast from a second click. +// --------------------------------------------------------------------------- + +describe("App — onReapplyStep guard while an apply is already in flight", () => { + it("clicking Re-apply again while an apply is in flight is a no-op: no second modal, no second POST", async () => { + // The /api/apply-config fetch NEVER resolves during this test — keeps + // `applying` true for its whole duration, so we can assert the guard. + let resolveApply: ((r: Response) => void) | null = null; + const neverResolvingApply = new Promise((resolve) => { + resolveApply = resolve; + }); + + const fetchSpy = vi.fn().mockImplementation((url: string) => { + if (url === "/api/verify/config") { + return Promise.resolve( + new Response( + JSON.stringify({ + clean: false, + results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }], + }), + { status: 200 }, + ), + ); + } + if (url === "/api/verify/source") { + return Promise.resolve( + new Response(JSON.stringify({ success: true, skipped: false, results: [] }), { status: 200 }), + ); + } + if (url.startsWith("/api/apply-config")) { + return neverResolvingApply; + } + return Promise.reject(new Error(`Unexpected fetch to ${url}`)); + }); + vi.stubGlobal("fetch", fetchSpy); + + await setupWithDrift(fetchSpy); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + // The confirm modal closes immediately (handleApplyConfig closes it + // synchronously before awaiting the fetch) — this confirms the apply is + // genuinely in flight (`applying === true`) with the request never + // resolving. + await waitFor(() => { + expect(screen.queryByTestId("apply-config-modal")).toBeNull(); + }); + const applyCallsBefore = fetchSpy.mock.calls.filter((c: unknown[]) => + (c[0] as string).startsWith("/api/apply-config"), + ).length; + expect(applyCallsBefore).toBe(1); + + // The drift detail panel + its Re-apply button are still mounted (App + // never unmounts the Inspector while applying) — clicking Re-apply again + // must be a no-op: the `if (applying) return;` guard in onReapplyStep. + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + + expect(screen.queryByTestId("apply-config-modal")).toBeNull(); + const applyCallsAfter = fetchSpy.mock.calls.filter((c: unknown[]) => + (c[0] as string).startsWith("/api/apply-config"), + ).length; + expect(applyCallsAfter).toBe(1); + + // Cleanup: resolve the hung fetch so the test doesn't leave a dangling + // unhandled promise / act() warning after the test completes. + resolveApply!( + new Response( + `event: done\ndata: ${JSON.stringify({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + deployment: null, + })}\n\n`, + { status: 200, headers: { "Content-Type": "text/event-stream" } }, + ), + ); + }); +}); + +// --------------------------------------------------------------------------- +// Non-blocking cleanup item (issue #153 review): nothing previously asserted +// that a NON-forced full "Apply config" run triggers exactly ONE +// /api/verify/config call (i.e. does NOT also run the re-apply drift +// refresh). Removing the `if (forceStepIds !== undefined)` guard at +// App.tsx's handleApplyConfig survived the suite before this test existed. +// --------------------------------------------------------------------------- + +function addNodeByName(name: string) { + if (!screen.queryByTestId("contracts-browser")) { + fireEvent.click(screen.getByTestId("toggle-contracts-browser")); + } + const browser = screen.getByTestId("contracts-browser"); + fireEvent.click(within(browser).getByTestId(`contract-row-${name}`)); +} + +function fillArg(index: number, value: string) { + fireEvent.change(screen.getByLabelText(`arg-${index}`), { target: { value } }); +} + +function addGrantRoleStep(nodeIndex = 0) { + const configSection = document.querySelectorAll( + "[data-testid^='node-config-section-']", + )[nodeIndex] as HTMLElement; + fireEvent.click(within(configSection).getByText("Add config call")); + fireEvent.click(within(configSection).getByText("grantRole(bytes32,address)")); +} + +describe("App — a full (non-forced) apply does not trigger a second drift refresh", () => { + it("a full 'Apply config' run POSTs to /api/verify/config exactly ZERO additional times (no re-apply drift refresh)", async () => { + let verifyConfigCallCount = 0; + const fetchSpy = vi.fn().mockImplementation((url: string) => { + if (url === "/api/verify/config") { + verifyConfigCallCount += 1; + return Promise.resolve( + new Response(JSON.stringify({ clean: true, results: [] }), { status: 200 }), + ); + } + if (url.startsWith("/api/apply-config")) { + return Promise.resolve( + new Response( + makeStream([ + enc( + doneOkFrame( + ["grant-minter"], + [], + ["grant-minter"], + [{ id: "grant-minter", kind: "grantRole", completed: true }], + ), + ), + ]), + { status: 200, headers: { "Content-Type": "text/event-stream" } }, + ), + ); + } + return Promise.reject(new Error(`Unexpected fetch to ${url}`)); + }); + vi.stubGlobal("fetch", fetchSpy); + + render(); + addNodeByName("Registry"); + fillArg(0, "0x0000000000000000000000000000000000000001"); + addGrantRoleStep(0); + + fireEvent.click(screen.getByTestId("deploy-apply-config-button")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + await waitFor(() => { + expect( + fetchSpy.mock.calls.some((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config")), + ).toBe(true); + }); + await waitFor(() => { + expect(screen.queryByTestId("apply-config-success")).not.toBeNull(); + }); + + // No /api/verify/config call at all — the re-apply drift refresh only + // runs when `forceStepIds` was set (a single-step re-apply), never for a + // full apply. + expect(verifyConfigCallCount).toBe(0); + }); }); diff --git a/apps/studio/test/App.reapplyNarrowing.test.tsx b/apps/studio/test/App.reapplyNarrowing.test.tsx new file mode 100644 index 0000000..c2a07a5 --- /dev/null +++ b/apps/studio/test/App.reapplyNarrowing.test.tsx @@ -0,0 +1,196 @@ +/** + * App.reapplyNarrowing.test.tsx + * + * Pins issue #153 review finding B1 (the most serious finding — flagged by + * BOTH the correctness and security lenses): the "Re-apply" confirm modal + * promises that clicking it broadcasts a real transaction for ONE named step + * and that "no other step will be sent". Before this fix, `handleApplyConfig` + * POSTed the studio's ENTIRE current `config` (every step on the canvas) with + * only `forceStepIds` narrowed — but `applyConfig()` server-side executes + * EVERY step in the POSTed spec that isn't already journaled, regardless of + * `forceStepIds`. So a single-step re-apply could ALSO broadcast every + * not-yet-applied step still on the canvas, contradicting the modal. + * + * The fix narrows the POSTed spec itself (via `narrowConfigToStep`) to just + * the target step. This file verifies that end-to-end, two ways: + * 1. `narrowConfigToStep` (spied via a module mock that still delegates to + * the real implementation) is called with `(config, stepId)` exactly + * when a single-step re-apply is confirmed — and NOT for a full + * (non-forced) apply. + * 2. The actual request body POSTed to `/api/apply-config` for a re-apply + * is deep-equal to `narrowConfigToStep`'s real return value — i.e. the + * network request is genuinely built from the narrowed spec. + * + * Isolated in its own file (module-mock scope) — mirrors the isolation + * rationale in App.overview-edges-wiring.test.tsx. + */ + +import { render, screen, fireEvent, waitFor, within } from "@testing-library/react"; +import { describe, it, expect, vi, afterEach } from "vitest"; + +// --------------------------------------------------------------------------- +// Module mock — must be declared before importing App so the mock factory +// runs first. Delegates to the REAL narrowConfigToStep so behaviour is +// unchanged; only records calls/return values for assertions. +// --------------------------------------------------------------------------- + +const { narrowConfigToStepSpy } = vi.hoisted(() => ({ narrowConfigToStepSpy: vi.fn() })); + +vi.mock("../src/spec/narrow-config.js", async (importOriginal) => { + const actual = await importOriginal(); + narrowConfigToStepSpy.mockImplementation(actual.narrowConfigToStep); + return { + ...actual, + narrowConfigToStep: narrowConfigToStepSpy, + }; +}); + +// Import App AFTER vi.mock so the mock factory runs first. +import App from "../src/App.js"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function enc(text: string): Uint8Array { + return new TextEncoder().encode(text); +} + +function makeStream(chunks: Uint8Array[]): ReadableStream { + return new ReadableStream({ + start(controller) { + for (const chunk of chunks) controller.enqueue(chunk); + controller.close(); + }, + }); +} + +function doneOkFrame(): string { + return `event: done\ndata: ${JSON.stringify({ + success: true, + executedStepIds: [], + skippedStepIds: [], + completedStepIds: [], + deployment: { contracts: [], configSteps: [], warnings: [] }, + })}\n\n`; +} + +function mockFetch(): ReturnType { + return vi.fn().mockImplementation((url: string) => { + if (url === "/api/verify/config") { + return Promise.resolve( + new Response( + JSON.stringify({ + clean: false, + results: [{ id: "setFee", status: "drift", expected: 500, actual: 999 }], + }), + { status: 200 }, + ), + ); + } + if (url === "/api/verify/source") { + return Promise.resolve( + new Response(JSON.stringify({ success: true, skipped: false, results: [] }), { status: 200 }), + ); + } + if (url.startsWith("/api/apply-config")) { + return Promise.resolve( + new Response(makeStream([enc(doneOkFrame())]), { + status: 200, + headers: { "Content-Type": "text/event-stream" }, + }), + ); + } + return Promise.reject(new Error(`Unexpected fetch to ${url}`)); + }); +} + +function addNodeByName(name: string) { + if (!screen.queryByTestId("contracts-browser")) { + fireEvent.click(screen.getByTestId("toggle-contracts-browser")); + } + const browser = screen.getByTestId("contracts-browser"); + fireEvent.click(within(browser).getByTestId(`contract-row-${name}`)); +} + +function fillArg(index: number, value: string) { + fireEvent.change(screen.getByLabelText(`arg-${index}`), { target: { value } }); +} + +/** Mirrors App.applyConfig.test.tsx's helper: adds a real grantRole-shaped call. */ +function addGrantRoleStep(nodeIndex = 0) { + const configSection = document.querySelectorAll( + "[data-testid^='node-config-section-']", + )[nodeIndex] as HTMLElement; + fireEvent.click(within(configSection).getByText("Add config call")); + fireEvent.click(within(configSection).getByText("grantRole(bytes32,address)")); +} + +afterEach(() => { + vi.restoreAllMocks(); + narrowConfigToStepSpy.mockClear(); + document.body.innerHTML = ""; +}); + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("App — Re-apply narrows the POSTed spec to a single step", () => { + it("calls narrowConfigToStep(config, stepId) exactly once, and the POSTed body equals its return value", async () => { + const fetchSpy = mockFetch(); + vi.stubGlobal("fetch", fetchSpy); + + render(); + fireEvent.click(screen.getByTestId("deploy-verify-button")); + await waitFor(() => { + expect(screen.getByTestId("config-step-setFee-drift")).not.toBeNull(); + }); + + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + fireEvent.click(screen.getByTestId("config-step-setFee-reapply")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + await waitFor(() => { + expect(narrowConfigToStepSpy).toHaveBeenCalledTimes(1); + }); + // Second arg is the target step id. + expect(narrowConfigToStepSpy.mock.calls[0]![1]).toBe("setFee"); + + await waitFor(() => { + expect( + fetchSpy.mock.calls.some((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config")), + ).toBe(true); + }); + const applyCall = fetchSpy.mock.calls.find((c: unknown[]) => + (c[0] as string).startsWith("/api/apply-config"), + )!; + const sentBody: unknown = JSON.parse((applyCall[1] as { body: string }).body); + const narrowedSpec = narrowConfigToStepSpy.mock.results[0]!.value; + + // The exact spec that was POSTed must be exactly what narrowConfigToStep + // produced — not the full, unnarrowed `config`. + expect(sentBody).toEqual(narrowedSpec); + }); + + it("a full (non-forced) 'Apply config' does NOT call narrowConfigToStep — the full config is sent unchanged", async () => { + const fetchSpy = mockFetch(); + vi.stubGlobal("fetch", fetchSpy); + + render(); + addNodeByName("Registry"); + fillArg(0, "0x0000000000000000000000000000000000000001"); + addGrantRoleStep(0); + + fireEvent.click(screen.getByTestId("deploy-apply-config-button")); + fireEvent.click(screen.getByTestId("apply-config-confirm")); + + await waitFor(() => { + expect( + fetchSpy.mock.calls.some((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config")), + ).toBe(true); + }); + + expect(narrowConfigToStepSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/studio/test/Inspector.test.tsx b/apps/studio/test/Inspector.test.tsx index cf558e8..b78c2c1 100644 --- a/apps/studio/test/Inspector.test.tsx +++ b/apps/studio/test/Inspector.test.tsx @@ -337,6 +337,33 @@ describe("Inspector — drift detail panel", () => { expect(screen.queryByTestId("config-step-setFee-drift-detail")).not.toBeNull(); }); + it("the badge is keyboard-activatable (Space key toggles the panel)", () => { + render( + , + ); + const badge = screen.getByTestId("config-step-setFee-drift"); + fireEvent.keyDown(badge, { key: " " }); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).not.toBeNull(); + // Pressing it again toggles back closed, mirroring the click-toggle test. + fireEvent.keyDown(badge, { key: " " }); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + }); + + it("a key other than Enter/Space is a no-op (no toggle, no preventDefault branch taken)", () => { + render( + , + ); + const badge = screen.getByTestId("config-step-setFee-drift"); + fireEvent.keyDown(badge, { key: "Tab" }); + expect(screen.queryByTestId("config-step-setFee-drift-detail")).toBeNull(); + }); + it("a detail panel is offered for a 'match' step too, but with no Re-apply button", () => { render( { expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("500"); }); + it("renders the literal string 'undefined' when expected/actual is undefined", () => { + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("undefined"); + }); + + it("falls back to String() when JSON.stringify returns undefined (e.g. a function value)", () => { + // JSON.stringify(fn) returns `undefined` (not a string, not a throw) — a + // distinct branch from both the plain-string case and the throw/catch + // case above. stringifyDriftValue's `?? String(value)` fallback covers + // it: String(a function) always contains "function". + const fnValue = function namedFn() { + return 1; + }; + render( + , + ); + fireEvent.click(screen.getByTestId("config-step-setFee-drift")); + expect(screen.getByTestId("config-step-setFee-drift-expected").textContent).toContain("function"); + }); + it("renders no message row when drift.message is undefined", () => { render( { expect(mockFetch).toHaveBeenCalledWith("/api/apply-config?forceStepIds=set-fee", expect.anything()); }); - it("multiple forceStepIds ids are comma-joined", async () => { + it("multiple forceStepIds ids are sent as REPEATED query params (issue #153 review finding B5) — not comma-joined", async () => { const raw = doneOkFrame(["set-fee", "grant-minter"], [], ["set-fee", "grant-minter"], null, "warn"); const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); await runApplyConfig({}, mockFetch, undefined, ["set-fee", "grant-minter"]); expect(mockFetch).toHaveBeenCalledWith( - "/api/apply-config?forceStepIds=set-fee,grant-minter", + "/api/apply-config?forceStepIds=set-fee&forceStepIds=grant-minter", expect.anything(), ); }); + it("a step id containing a comma is rejected client-side — no fetch is made at all", async () => { + const mockFetch = vi.fn(); + + const result = await runApplyConfig({}, mockFetch, undefined, ["set,fee"]); + + expect(mockFetch).not.toHaveBeenCalled(); + expect(result.ok).toBe(false); + if (result.ok) throw new Error("expected error"); + expect(result.error).toContain("set,fee"); + expect(result.error.toLowerCase()).toContain("comma"); + expect(result.steps).toEqual([]); + }); + + it("a comma-containing id is rejected even when mixed with other valid ids (fail before any are sent)", async () => { + const mockFetch = vi.fn(); + + const result = await runApplyConfig({}, mockFetch, undefined, ["set-fee", "grant,minter"]); + + expect(mockFetch).not.toHaveBeenCalled(); + expect(result.ok).toBe(false); + }); + it("composes correctly with a network param: both present, joined with &", async () => { const raw = doneOkFrame(["set-fee"], [], ["set-fee"], null, "warn"); const mockFetch = vi.fn().mockResolvedValue(new Response(makeStream([raw]), { status: 200 })); diff --git a/apps/studio/test/narrow-config.test.ts b/apps/studio/test/narrow-config.test.ts new file mode 100644 index 0000000..3ec20bd --- /dev/null +++ b/apps/studio/test/narrow-config.test.ts @@ -0,0 +1,63 @@ +/** + * narrow-config.test.ts + * + * Unit tests for narrowConfigToStep (issue #153 review, finding B1) — pins + * the guarantee that narrowing a spec to a single step id yields a spec that + * can execute AT MOST that one step, regardless of which list (`steps` vs + * `orderedSteps`) it originated in. + */ + +import { describe, it, expect } from "vitest"; +import { narrowConfigToStep } from "../src/spec/narrow-config.js"; +import type { ConfigSpec } from "@redeploy/config/steps"; + +const SPEC: ConfigSpec = { + version: 1, + steps: [ + { kind: "setX", id: "set-fee", target: "token", function: "setFee", args: [{ kind: "literal", value: 500 }] }, + { kind: "grantRole", id: "grant-minter", target: "vault", role: "MINTER_ROLE", account: { kind: "ref", contract: "minter" } }, + ], + orderedSteps: [ + { kind: "wire", id: "wire-token-into-vault", source: "token", into: "vault", function: "setToken" }, + ], +}; + +describe("narrowConfigToStep", () => { + it("keeps only the matching step from `steps`, empties `orderedSteps`", () => { + const narrowed = narrowConfigToStep(SPEC, "set-fee"); + expect(narrowed.steps).toEqual([SPEC.steps[0]]); + expect(narrowed.orderedSteps).toEqual([]); + expect(narrowed.version).toBe(1); + }); + + it("keeps only the matching step from `orderedSteps`, empties `steps`", () => { + const narrowed = narrowConfigToStep(SPEC, "wire-token-into-vault"); + expect(narrowed.steps).toEqual([]); + expect(narrowed.orderedSteps).toEqual([SPEC.orderedSteps![0]]); + }); + + it("an unknown step id narrows to a spec with both lists empty (safe/inert, not an error)", () => { + const narrowed = narrowConfigToStep(SPEC, "does-not-exist"); + expect(narrowed.steps).toEqual([]); + expect(narrowed.orderedSteps).toEqual([]); + }); + + it("does not mutate the input spec", () => { + const before = JSON.parse(JSON.stringify(SPEC)) as ConfigSpec; + narrowConfigToStep(SPEC, "set-fee"); + expect(SPEC).toEqual(before); + }); + + it("a spec with no orderedSteps field narrows to orderedSteps: undefined (not [])", () => { + const specWithoutOrdered: ConfigSpec = { version: 1, steps: SPEC.steps }; + const narrowed = narrowConfigToStep(specWithoutOrdered, "set-fee"); + expect(narrowed.orderedSteps).toBeUndefined(); + expect(narrowed.steps).toEqual([SPEC.steps[0]]); + }); + + it("narrowing to a single step id never includes any other step's id in the result", () => { + const narrowed = narrowConfigToStep(SPEC, "grant-minter"); + const allIds = [...narrowed.steps, ...(narrowed.orderedSteps ?? [])].map((s) => s.id); + expect(allIds).toEqual(["grant-minter"]); + }); +}); diff --git a/packages/config/test/execute.test.ts b/packages/config/test/execute.test.ts index 7a5fec4..b9d7cee 100644 --- a/packages/config/test/execute.test.ts +++ b/packages/config/test/execute.test.ts @@ -1013,6 +1013,50 @@ describe("applyConfig — forceStepIds", () => { forceStepIds: ["set-fee"], }); + const journalFile = path.join(stateDir, "config-state.jsonl"); + const content = await fs.promises.readFile(journalFile, "utf8"); + + // "set-fee" appears twice: once from the first run, once from the forced + // re-run. readCompletedStepIds folds duplicates into a Set, so this is + // safe and does not affect skip/resume behaviour. + const setFeeLines = content + .trim() + .split("\n") + .filter(Boolean) + .map((l) => JSON.parse(l) as { id: string; completedAt: string }) + .filter((r) => r.id === "set-fee"); + expect(setFeeLines).toHaveLength(2); + + // The re-journaled completedAt must be a fresh timestamp — not a stale + // copy of the first run's record — and both must be valid ISO strings. + for (const record of setFeeLines) { + expect(() => new Date(record.completedAt).toISOString()).not.toThrow(); + expect(new Date(record.completedAt).toISOString()).toBe(record.completedAt); + } + expect(new Date(setFeeLines[1]!.completedAt).getTime()).toBeGreaterThanOrEqual( + new Date(setFeeLines[0]!.completedAt).getTime(), + ); + }); + + it("a forced step whose executor THROWS does not append a second journal record (no double-journal on failure)", async () => { + const stateDir = await makeTempDir(); + + // First run: fully applies the spec, journaling "set-fee" once. + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + // Second run: force "set-fee" to re-execute, but its executor throws on + // this (its only) call. The error must propagate, and — since + // executeStep only appends to the journal AFTER a successful + // executor.execute() — "set-fee" must NOT be journaled a second time. + const executor2 = new FakeExecutor(1); + await expect( + applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["set-fee"], + }), + ).rejects.toThrow(/simulated failure/); + const journalFile = path.join(stateDir, "config-state.jsonl"); const content = await fs.promises.readFile(journalFile, "utf8"); const ids = content @@ -1021,10 +1065,7 @@ describe("applyConfig — forceStepIds", () => { .filter(Boolean) .map((l) => (JSON.parse(l) as { id: string }).id); - // "set-fee" appears twice: once from the first run, once from the forced - // re-run. readCompletedStepIds folds duplicates into a Set, so this is - // safe and does not affect skip/resume behaviour. - expect(ids.filter((id) => id === "set-fee")).toHaveLength(2); + expect(ids.filter((id) => id === "set-fee")).toHaveLength(1); }); it("forced steps still resolve read args and invoke executor.read() again", async () => { From 0db737dc003a8b9a8688fffa28311a36189b40b5 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Thu, 6 Aug 2026 21:56:01 +0200 Subject: [PATCH 3/5] fix(studio,config): address TESTS lens findings on config-drift re-apply (issue #153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - App.reapplyNarrowing.test.tsx: T1 (blocking) — the B1 regression test was vacuous because the canvas config was empty, making the narrowed-vs-full spec comparison trivially true. Add a real canvas step before triggering re-apply and assert directly on the POSTed request body (no step id other than the re-apply target may appear), independent of the narrowConfigToStep spy. Mutation-verified: fails when the narrowed spec is computed then discarded, passes against the real implementation. - execute.test.ts: strengthen the fresh-completedAt assertion with a controlled clock so a stale-copy regression (not just a genuinely newer timestamp) is caught, instead of toBeGreaterThanOrEqual which a byte-copy would also satisfy. - narrow-config.test.ts: add a case for the same step id appearing in both `steps` and `orderedSteps`, pinning that narrowConfigToStep keeps both entries (de-duplication/rejection happens downstream via validateConfig's DUPLICATE_STEP_ID check). - Inspector.tsx: guard DRIFT_BADGE_STYLES lookup with Object.hasOwn so a server-supplied drift.status of "constructor"/"toString" can't return an inherited Object.prototype function instead of falling through to the default badge style (security lens nit, assessed harmless but cheap to fix). Co-Authored-By: Claude Sonnet 5 --- apps/studio/src/components/Inspector.tsx | 11 +++++- .../studio/test/App.reapplyNarrowing.test.tsx | 26 ++++++++++++- apps/studio/test/narrow-config.test.ts | 30 +++++++++++++++ packages/config/test/execute.test.ts | 37 ++++++++++++++----- 4 files changed, 93 insertions(+), 11 deletions(-) diff --git a/apps/studio/src/components/Inspector.tsx b/apps/studio/src/components/Inspector.tsx index 1b8a9b7..49f865a 100644 --- a/apps/studio/src/components/Inspector.tsx +++ b/apps/studio/src/components/Inspector.tsx @@ -276,7 +276,16 @@ function ConfigStepCard({ detail panel below. */} {drift !== undefined && ( { vi.stubGlobal("fetch", fetchSpy); render(); + // Non-empty canvas config, issue #153 review (TESTS lens, finding T1): + // this step (id "setX-1") is UNRELATED to the "setFee" step being + // re-applied below and must NEVER be broadcast alongside it. Without a + // node on the canvas, `config` is `{version:1,steps:[]}` and the + // `toEqual(narrowedSpec)` assertion below is vacuous — narrowed and + // un-narrowed are both empty, so it can't distinguish a correctly + // narrowed POST from an accidentally-full one. + addNodeByName("Registry"); + fillArg(0, "0x0000000000000000000000000000000000000001"); + addGrantRoleStep(0); + fireEvent.click(screen.getByTestId("deploy-verify-button")); await waitFor(() => { expect(screen.getByTestId("config-step-setFee-drift")).not.toBeNull(); @@ -165,12 +176,25 @@ describe("App — Re-apply narrows the POSTed spec to a single step", () => { const applyCall = fetchSpy.mock.calls.find((c: unknown[]) => (c[0] as string).startsWith("/api/apply-config"), )!; - const sentBody: unknown = JSON.parse((applyCall[1] as { body: string }).body); + const sentBody: { steps?: Array<{ id: string }>; orderedSteps?: Array<{ id: string }> } = JSON.parse( + (applyCall[1] as { body: string }).body, + ); const narrowedSpec = narrowConfigToStepSpy.mock.results[0]!.value; // The exact spec that was POSTed must be exactly what narrowConfigToStep // produced — not the full, unnarrowed `config`. expect(sentBody).toEqual(narrowedSpec); + + // Request-body assertion independent of the spy (issue #153 review, + // TESTS lens finding T1): even if `narrowConfigToStep` were called but + // its result discarded (e.g. `specToSend = config`), this must fail. No + // step id other than the re-apply target ("setFee") may appear anywhere + // in the POSTed body — in particular the unrelated canvas step + // ("setX-1") added above must be absent, since broadcasting it would + // reintroduce finding B1 (re-apply sending transactions for other + // steps). + const ids = [...(sentBody.steps ?? []), ...(sentBody.orderedSteps ?? [])].map((s) => s.id); + expect(ids.filter((id) => id !== "setFee")).toEqual([]); }); it("a full (non-forced) 'Apply config' does NOT call narrowConfigToStep — the full config is sent unchanged", async () => { diff --git a/apps/studio/test/narrow-config.test.ts b/apps/studio/test/narrow-config.test.ts index 3ec20bd..6a5b243 100644 --- a/apps/studio/test/narrow-config.test.ts +++ b/apps/studio/test/narrow-config.test.ts @@ -60,4 +60,34 @@ describe("narrowConfigToStep", () => { const allIds = [...narrowed.steps, ...(narrowed.orderedSteps ?? [])].map((s) => s.id); expect(allIds).toEqual(["grant-minter"]); }); + + // Issue #153 review (TESTS lens, nit): the same step id present in BOTH + // `steps` AND `orderedSteps` is an already-invalid spec (packages/config's + // `validateConfig` rejects any id duplicated across the two lists with a + // `DUPLICATE_STEP_ID` -> `INVALID_SPEC` error before any step ever + // executes — see packages/config/src/steps/validate.ts). narrowConfigToStep + // itself does not de-duplicate: it independently filters each list by id, + // so a duplicate id is kept in BOTH lists. Pinning that here so a future + // change to the filtering logic can't silently start dropping one of the + // duplicates (which would just as silently mask an otherwise-caught + // INVALID_SPEC). + it("a step id present in BOTH `steps` and `orderedSteps` is kept in both (narrowConfigToStep does not de-duplicate; the resulting spec is later rejected as DUPLICATE_STEP_ID by validateConfig, not silently collapsed here)", () => { + const duplicateIdSpec: ConfigSpec = { + version: 1, + steps: [ + { + kind: "setX", + id: "dup", + target: "token", + function: "setFee", + args: [{ kind: "literal", value: 500 }], + }, + ], + orderedSteps: [{ kind: "wire", id: "dup", source: "token", into: "vault", function: "setToken" }], + }; + + const narrowed = narrowConfigToStep(duplicateIdSpec, "dup"); + expect(narrowed.steps).toEqual([duplicateIdSpec.steps[0]]); + expect(narrowed.orderedSteps).toEqual([duplicateIdSpec.orderedSteps![0]]); + }); }); diff --git a/packages/config/test/execute.test.ts b/packages/config/test/execute.test.ts index b9d7cee..e9b4d5e 100644 --- a/packages/config/test/execute.test.ts +++ b/packages/config/test/execute.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, vi } from "vitest"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; @@ -1004,14 +1004,29 @@ describe("applyConfig — forceStepIds", () => { it("a forced step appends a NEW journal record (duplicate id) alongside the original", async () => { const stateDir = await makeTempDir(); - const executor1 = new FakeExecutor(); - await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); - const executor2 = new FakeExecutor(); - await applyConfig({ - ...makeOptions(threeStepSpec, executor2, stateDir), - forceStepIds: ["set-fee"], - }); + // Issue #153 review (TESTS lens, nit): control the system clock across + // the two runs so the second record's `completedAt` is verifiably a + // FRESH timestamp, not merely `>=` the first (which a stale copy of the + // first record would also satisfy, since equal values pass + // `toBeGreaterThanOrEqual`). Advancing fake time between runs makes a + // regression that re-journals a copy of the original timestamp provably + // fail here. + vi.useFakeTimers(); + try { + vi.setSystemTime(new Date("2024-01-01T00:00:00.000Z")); + const executor1 = new FakeExecutor(); + await applyConfig(makeOptions(threeStepSpec, executor1, stateDir)); + + vi.setSystemTime(new Date("2024-01-01T00:05:00.000Z")); + const executor2 = new FakeExecutor(); + await applyConfig({ + ...makeOptions(threeStepSpec, executor2, stateDir), + forceStepIds: ["set-fee"], + }); + } finally { + vi.useRealTimers(); + } const journalFile = path.join(stateDir, "config-state.jsonl"); const content = await fs.promises.readFile(journalFile, "utf8"); @@ -1033,7 +1048,11 @@ describe("applyConfig — forceStepIds", () => { expect(() => new Date(record.completedAt).toISOString()).not.toThrow(); expect(new Date(record.completedAt).toISOString()).toBe(record.completedAt); } - expect(new Date(setFeeLines[1]!.completedAt).getTime()).toBeGreaterThanOrEqual( + expect(setFeeLines[0]!.completedAt).toBe("2024-01-01T00:00:00.000Z"); + expect(setFeeLines[1]!.completedAt).toBe("2024-01-01T00:05:00.000Z"); + // Strict inequality: a stale copy of the first record's timestamp would + // fail this, unlike the previous `toBeGreaterThanOrEqual`. + expect(new Date(setFeeLines[1]!.completedAt).getTime()).toBeGreaterThan( new Date(setFeeLines[0]!.completedAt).getTime(), ); }); From c92ec6630cb57f221fe6bb6c15b89071012a0833 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Fri, 7 Aug 2026 00:16:48 +0200 Subject: [PATCH 4/5] fix(deploy-server,studio): make config-drift check network-aware (issue #153) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security review (finding H1) caught that POST /api/verify/config always read the server's default-network deployment while the forced re-apply it gates writes to the user-selected network — a wrong-network drift reading could let a one-click "Re-apply" broadcast a real transaction for a mismatch that doesn't exist (or hide one that does) on the network actually being written to. Thread the same ?network= convention already used by /api/apply-config through /api/verify/config and runVerifyConfig, and fix a route-matching bug (matching the query-string-bearing url instead of pathname) that this change exposed. --- apps/deploy-server/src/server.ts | 56 +++++++--- apps/deploy-server/test/verify-routes.test.ts | 104 ++++++++++++++++++ apps/studio/src/App.tsx | 6 +- apps/studio/src/deploy/verify-client.ts | 18 ++- apps/studio/test/verify-client.test.ts | 47 ++++++++ 5 files changed, 212 insertions(+), 19 deletions(-) diff --git a/apps/deploy-server/src/server.ts b/apps/deploy-server/src/server.ts index b573f8c..d24efb8 100644 --- a/apps/deploy-server/src/server.ts +++ b/apps/deploy-server/src/server.ts @@ -1209,13 +1209,22 @@ function writeJsonResponse(res: ServerResponse, statusCode: number, payload: unk /** * Shared by both /api/verify/config and /api/verify/source: read the - * persisted deployment from the server-resolved deploymentDir, treating a - * fresh/never-deployed directory as the EMPTY_DEPLOYMENT_VIEW (not an error) - * exactly like handleGetDeployment. On any other ReadError (or unexpected - * error), writes a 500 response and returns `null` so the caller bails out. + * persisted deployment from a deploymentDir, treating a fresh/never-deployed + * directory as the EMPTY_DEPLOYMENT_VIEW (not an error) exactly like + * handleGetDeployment. On any other ReadError (or unexpected error), writes a + * 500 response and returns `null` so the caller bails out. + * + * `deploymentDir` defaults to the server-resolved (env-only) directory — + * /api/verify/source has no network selection, so it always uses the + * default. /api/verify/config instead passes the RESOLVED network's own + * `deploymentDir` (see handleVerifyConfig) so a drift check against a + * non-default network reads that network's deployment, not the default one. */ -function readPersistedDeploymentOr500(res: ServerResponse, logLabel: string): DeploymentView | null { - const deploymentDir = resolveDeploymentDir(); +function readPersistedDeploymentOr500( + res: ServerResponse, + logLabel: string, + deploymentDir: string = resolveDeploymentDir(), +): DeploymentView | null { try { return readDeployment({ deploymentDir }); } catch (err) { @@ -1234,18 +1243,28 @@ function readPersistedDeploymentOr500(res: ServerResponse, logLabel: string): De * * Reads the JSON body as a ConfigSpec (structurally validated via * validateConfigSpecShape — a 400 for anything not shaped like - * `{version, steps, orderedSteps?}`), reads the persisted deployment (server - * env only — see readPersistedDeploymentOr500), builds a read-only - * (never-signing) chain reader over RPC_URL/FOUNDRY_OUT, and runs - * runConfigDrift() (see verify/run-config-drift.ts for the full + * `{version, steps, orderedSteps?}`), reads the persisted deployment for the + * RESOLVED network (see readPersistedDeploymentOr500), builds a read-only + * (never-signing) chain reader over that network's rpcUrl/FOUNDRY_OUT, and + * runs runConfigDrift() (see verify/run-config-drift.ts for the full * graceful-degradation contract: unresolvable refs and non-derivable getter * mappings become per-step "error"/"skipped" results, never a 500). * + * Network selection: accepts the OPTIONAL `?network=` query param (see + * `resolveNetworkForRequest`), mirroring `handleApplyConfig`. This MUST stay + * network-aware the same way the forced-re-apply write path is (issue #153 + * security review, finding H1): drift badges gate a real one-click re-apply + * broadcast, so computing them against the wrong chain would let a user + * "fix" a mismatch that doesn't exist (or miss one that does) on the network + * actually being written to. + * * Response: 200 `{ clean: boolean, results: ConfigDriftResultEntry[] }`. * Error responses (non-streaming JSON): * - 413 body exceeds MAX_BODY_BYTES - * - 400 malformed JSON, or body not shaped like a ConfigSpec - * - 500 the persisted deployment could not be read + * - 400 malformed JSON, body not shaped like a ConfigSpec, or an unknown + * `?network=` name + * - 500 network configuration could not be loaded, or the persisted + * deployment could not be read */ async function handleVerifyConfig(req: IncomingMessage, res: ServerResponse): Promise { const bodyResult = await readAndParseBody(req, res); @@ -1258,10 +1277,13 @@ async function handleVerifyConfig(req: IncomingMessage, res: ServerResponse): Pr return; } - const deployment = readPersistedDeploymentOr500(res, "verify/config"); + const network = resolveNetworkForRequest(req, res); + if (network === undefined) return; + + const deployment = readPersistedDeploymentOr500(res, "verify/config", network.config.deploymentDir); if (deployment === null) return; - const rpcUrl = process.env["RPC_URL"] ?? "http://127.0.0.1:8545"; + const rpcUrl = network.config.rpcUrl; const outDir = process.env["FOUNDRY_OUT"] ?? DEFAULT_FOUNDRY_OUT; const addressToContractName = new Map(); @@ -1490,8 +1512,12 @@ export function handleRequest(req: IncomingMessage, res: ServerResponse): void { return; } - if (method === "POST" && url === "/api/verify/config") { + if (method === "POST" && pathname === "/api/verify/config") { // handleVerifyConfig is async; fire-and-forget — errors are handled internally. + // Uses `pathname` (query-stripped), NOT `url`, because this route now + // accepts an optional `?network=` query param (issue #153 security + // review, finding H1) — matching the raw `url` would 404 any request + // that includes one. handleVerifyConfig(req, res).catch(() => { if (!res.headersSent) { writeJsonResponse(res, 500, { error: "Internal Server Error" }); diff --git a/apps/deploy-server/test/verify-routes.test.ts b/apps/deploy-server/test/verify-routes.test.ts index 7e4b030..4710fe7 100644 --- a/apps/deploy-server/test/verify-routes.test.ts +++ b/apps/deploy-server/test/verify-routes.test.ts @@ -395,6 +395,110 @@ describe("POST /api/verify/config", () => { }); }); +// --------------------------------------------------------------------------- +// POST /api/verify/config — multi-network (?network=) +// +// issue #153 security review, finding H1: the drift check MUST read the +// SELECTED network's deployment (not always the server's default env-based +// one), the same way handleApplyConfig's forced re-apply already writes to +// the selected network — otherwise a user could see drift computed against +// one chain while one-click re-applying against another. +// --------------------------------------------------------------------------- + +describe("POST /api/verify/config — multi-network (?network=)", () => { + let networksTmpDir: string; + let savedNetworksConfig: string | undefined; + + beforeEach(() => { + networksTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "redeploy-verify-config-networks-test-")); + savedNetworksConfig = process.env["NETWORKS_CONFIG"]; + }); + + afterEach(() => { + fs.rmSync(networksTmpDir, { recursive: true, force: true }); + if (savedNetworksConfig === undefined) { + delete process.env["NETWORKS_CONFIG"]; + } else { + process.env["NETWORKS_CONFIG"] = savedNetworksConfig; + } + }); + + function writeNetworksConfig(config: unknown): void { + const configPath = path.join(networksTmpDir, "networks.json"); + fs.writeFileSync(configPath, JSON.stringify(config), "utf8"); + process.env["NETWORKS_CONFIG"] = configPath; + } + + const ALPHA_ADDRESS = "0x3333333333333333333333333333333333333333"; + const BETA_ADDRESS = "0x4444444444444444444444444444444444444444"; + const NETWORK_SET_FEE_SPEC = JSON.stringify({ + version: 1, + steps: [ + { + kind: "setX", + id: "set-fee", + target: "feeController", + function: "setFee", + args: [{ kind: "literal", value: 500 }], + }, + ], + }); + + function writeNetworkDeployment(dir: string, address: string): void { + fs.mkdirSync(dir, { recursive: true }); + writeJournal(dir, [ + { + type: "DEPLOYMENT_EXECUTION_STATE_INITIALIZE", + futureId: "Deployment#feeController", + contractName: "FeeController", + constructorArgs: [], + libraries: {}, + dependencies: [], + }, + { + type: "DEPLOYMENT_EXECUTION_STATE_COMPLETE", + futureId: "Deployment#feeController", + result: { type: "SUCCESS", address }, + }, + ]); + writeDeployedAddresses(dir, { "Deployment#feeController": address }); + } + + it("reads the SELECTED network's deployment, not the server's default", async () => { + const alphaDir = path.join(networksTmpDir, "alpha-journal"); + const betaDir = path.join(networksTmpDir, "beta-journal"); + writeNetworkDeployment(alphaDir, ALPHA_ADDRESS); + writeNetworkDeployment(betaDir, BETA_ADDRESS); + writeNetworksConfig({ + networks: { + alpha: { rpcUrl: "http://alpha-rpc.internal.example.com", deploymentDir: alphaDir }, + beta: { rpcUrl: "http://beta-rpc.internal.example.com", deploymentDir: betaDir }, + }, + }); + + readContractSpy.mockResolvedValue(500n); + + const resAlpha = await doRequest(port, "POST", "/api/verify/config?network=alpha", NETWORK_SET_FEE_SPEC); + expect(resAlpha.statusCode).toBe(200); + expect(readContractSpy).toHaveBeenLastCalledWith(expect.objectContaining({ address: ALPHA_ADDRESS })); + + const resBeta = await doRequest(port, "POST", "/api/verify/config?network=beta", NETWORK_SET_FEE_SPEC); + expect(resBeta.statusCode).toBe(200); + expect(readContractSpy).toHaveBeenLastCalledWith(expect.objectContaining({ address: BETA_ADDRESS })); + }); + + it("an unknown ?network= value -> 400, the chain is never read", async () => { + const res = await doRequest(port, "POST", "/api/verify/config?network=nonexistent", NETWORK_SET_FEE_SPEC); + + expect(res.statusCode).toBe(400); + expect(res.headers["content-type"]).toBe("application/json"); + const body = JSON.parse(res.body) as Record; + expect(typeof body["error"]).toBe("string"); + expect(res.body).not.toContain("nonexistent"); + expect(readContractSpy).not.toHaveBeenCalled(); + }); +}); + // --------------------------------------------------------------------------- // POST /api/verify/source // --------------------------------------------------------------------------- diff --git a/apps/studio/src/App.tsx b/apps/studio/src/App.tsx index 6536c2a..8983cf6 100644 --- a/apps/studio/src/App.tsx +++ b/apps/studio/src/App.tsx @@ -1165,7 +1165,7 @@ export function App() { setVerifyError(null); const [driftOutcome, sourceOutcome] = await Promise.all([ - runVerifyConfig(config), + runVerifyConfig(config, fetch, selectedNetwork ?? undefined), runVerifySource(), ]); @@ -1196,7 +1196,7 @@ export function App() { } setVerifying(false); - }, [verifying, config]); + }, [verifying, config, selectedNetwork]); // "Apply config" opens a confirmation modal — it never POSTs directly // (mirrors onOpenDeployModal above; broadcasts real transactions). @@ -1299,7 +1299,7 @@ export function App() { // `driftResults` untouched — the UI keeps showing the last-known // (now possibly stale) drift status rather than clearing it. if (forceStepIds !== undefined) { - const driftOutcome = await runVerifyConfig(config); + const driftOutcome = await runVerifyConfig(config, fetch, selectedNetwork ?? undefined); if (driftOutcome.ok) { setDriftResults(driftOutcome.result.results); } diff --git a/apps/studio/src/deploy/verify-client.ts b/apps/studio/src/deploy/verify-client.ts index dda5701..f6d110d 100644 --- a/apps/studio/src/deploy/verify-client.ts +++ b/apps/studio/src/deploy/verify-client.ts @@ -111,14 +111,30 @@ async function readJsonOrError(response: Response): Promise<{ ok: true; resul * @param spec - The ConfigSpec JSON object to send (as the request body). * @param fetchFn - The fetch implementation to use (defaults to global fetch; * accepted as a parameter for testability). + * @param network - Optional target network name (issue #139 convention), + * sent as `?network=` (URI-encoded). Omitted/undefined + * ⇒ no `network` param, server resolves its default network. + * MUST match whatever network a subsequent forced re-apply + * targets (issue #153 security review, finding H1) — the + * drift badges this produces gate a real on-chain write, so + * reading drift from one chain while writing to another + * would let a "Re-apply" click broadcast a transaction for + * a mismatch that doesn't exist (or hide one that does) on + * the network actually being written to. */ export async function runVerifyConfig( spec: unknown, fetchFn: typeof fetch = fetch, + network?: string, ): Promise { + const url = + network !== undefined && network !== "" + ? `/api/verify/config?network=${encodeURIComponent(network)}` + : "/api/verify/config"; + let response: Response; try { - response = await fetchFn("/api/verify/config", { + response = await fetchFn(url, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(spec), diff --git a/apps/studio/test/verify-client.test.ts b/apps/studio/test/verify-client.test.ts index 01eba79..6e59201 100644 --- a/apps/studio/test/verify-client.test.ts +++ b/apps/studio/test/verify-client.test.ts @@ -76,6 +76,53 @@ describe("runVerifyConfig", () => { }); }); +// --------------------------------------------------------------------------- +// runVerifyConfig — network param (issue #153 security review, finding H1: +// the drift check MUST target the same network a subsequent forced re-apply +// would write to, mirroring runApplyConfig's `?network=` convention). +// --------------------------------------------------------------------------- + +describe("runVerifyConfig — network param", () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it("omitted network param → POSTs to /api/verify/config with no query string", async () => { + const mockFetch = vi.fn().mockResolvedValue(new Response(JSON.stringify({ clean: true, results: [] }), { status: 200 })); + + await runVerifyConfig({}, mockFetch); + + expect(mockFetch).toHaveBeenCalledWith("/api/verify/config", expect.anything()); + }); + + it("a network name → POSTs to /api/verify/config?network=", async () => { + const mockFetch = vi.fn().mockResolvedValue(new Response(JSON.stringify({ clean: true, results: [] }), { status: 200 })); + + await runVerifyConfig({}, mockFetch, "sepolia"); + + expect(mockFetch).toHaveBeenCalledWith("/api/verify/config?network=sepolia", expect.anything()); + }); + + it("a network name with special characters is URI-encoded", async () => { + const mockFetch = vi.fn().mockResolvedValue(new Response(JSON.stringify({ clean: true, results: [] }), { status: 200 })); + + await runVerifyConfig({}, mockFetch, "my network/1"); + + expect(mockFetch).toHaveBeenCalledWith( + `/api/verify/config?network=${encodeURIComponent("my network/1")}`, + expect.anything(), + ); + }); + + it("an empty-string network param is treated the same as omitted (no query string)", async () => { + const mockFetch = vi.fn().mockResolvedValue(new Response(JSON.stringify({ clean: true, results: [] }), { status: 200 })); + + await runVerifyConfig({}, mockFetch, ""); + + expect(mockFetch).toHaveBeenCalledWith("/api/verify/config", expect.anything()); + }); +}); + describe("runVerifySource", () => { beforeEach(() => { vi.restoreAllMocks(); From 700a7cffdad601a7ae5849504f2bd969359f3023 Mon Sep 17 00:00:00 2001 From: Roberto Cano <3525807+robercano@users.noreply.github.com> Date: Fri, 7 Aug 2026 00:26:27 +0200 Subject: [PATCH 5/5] fix(deploy-server): close review gaps on the H1 network-aware fix (issue #153) Assert the rpcUrl half of network isolation in the new verify/config multi-network test (previously only deploymentDir was checked, so a revert of the rpcUrl line would have gone undetected), and update the handleRequest route doc comment to list /api/verify/config among the pathname-matched, ?network=-aware routes. --- apps/deploy-server/src/server.ts | 14 +++++++------- apps/deploy-server/test/verify-routes.test.ts | 9 ++++++++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/apps/deploy-server/src/server.ts b/apps/deploy-server/src/server.ts index d24efb8..c6609d7 100644 --- a/apps/deploy-server/src/server.ts +++ b/apps/deploy-server/src/server.ts @@ -1398,13 +1398,13 @@ async function handleVerifySource(req: IncomingMessage, res: ServerResponse): Pr * POST /api/verify/config → 200 JSON { clean, results } (config-drift check) * POST /api/verify/source → 200 JSON { success, skipped, reason?, results } (Etherscan source verification) * - * `GET /api/deployment`, `POST /api/simulate`, `POST /api/deploy`, and - * `POST /api/apply-config` all accept an OPTIONAL `?network=` query - * param (see `resolveNetworkForRequest` / networks.ts) — hence routing - * matches on `pathname` (query-string-stripped), not the raw `url`, for - * these four. `GET /api/networks` takes no query params but is likewise - * matched on `pathname` for consistency (harmless if a client appends one - * anyway). + * `GET /api/deployment`, `POST /api/simulate`, `POST /api/deploy`, + * `POST /api/apply-config`, and `POST /api/verify/config` all accept an + * OPTIONAL `?network=` query param (see `resolveNetworkForRequest` / + * networks.ts) — hence routing matches on `pathname` (query-string-stripped), + * not the raw `url`, for these five. `GET /api/networks` takes no query + * params but is likewise matched on `pathname` for consistency (harmless if + * a client appends one anyway). * * `POST /api/apply-config` ALSO accepts an OPTIONAL `?forceStepIds=,` * query param (see `parseForceStepIds`) that forces re-execution of specific diff --git a/apps/deploy-server/test/verify-routes.test.ts b/apps/deploy-server/test/verify-routes.test.ts index 4710fe7..47e7d83 100644 --- a/apps/deploy-server/test/verify-routes.test.ts +++ b/apps/deploy-server/test/verify-routes.test.ts @@ -20,6 +20,7 @@ import * as path from "node:path"; const getChainIdSpy = vi.fn(); const readContractSpy = vi.fn(); +const httpSpy = vi.fn(); vi.mock("viem", async (importOriginal) => { const original = await importOriginal(); @@ -29,7 +30,10 @@ vi.mock("viem", async (importOriginal) => { getChainId: (...args: unknown[]) => getChainIdSpy(...args), readContract: (...args: unknown[]) => readContractSpy(...args), })), - http: vi.fn((url: string) => ({ type: "http", url })), + http: vi.fn((url: string) => { + httpSpy(url); + return { type: "http", url }; + }), }; }); @@ -184,6 +188,7 @@ beforeEach(() => { getChainIdSpy.mockReset(); readContractSpy.mockReset(); + httpSpy.mockReset(); }); afterEach(() => { @@ -481,10 +486,12 @@ describe("POST /api/verify/config — multi-network (?network=)", () => { const resAlpha = await doRequest(port, "POST", "/api/verify/config?network=alpha", NETWORK_SET_FEE_SPEC); expect(resAlpha.statusCode).toBe(200); expect(readContractSpy).toHaveBeenLastCalledWith(expect.objectContaining({ address: ALPHA_ADDRESS })); + expect(httpSpy).toHaveBeenLastCalledWith("http://alpha-rpc.internal.example.com"); const resBeta = await doRequest(port, "POST", "/api/verify/config?network=beta", NETWORK_SET_FEE_SPEC); expect(resBeta.statusCode).toBe(200); expect(readContractSpy).toHaveBeenLastCalledWith(expect.objectContaining({ address: BETA_ADDRESS })); + expect(httpSpy).toHaveBeenLastCalledWith("http://beta-rpc.internal.example.com"); }); it("an unknown ?network= value -> 400, the chain is never read", async () => {