diff --git a/apps/deploy-server/src/server.ts b/apps/deploy-server/src/server.ts index c32a5c6..c6609d7 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`, @@ -151,6 +170,112 @@ 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: 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 + * `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 rawValues = getQueryParamAll(req.url, "forceStepIds"); + if (rawValues.length === 0) { + return { ok: true, forceStepIds: undefined }; + } + + const ids = rawValues + .flatMap((raw) => 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 +880,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 +916,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 +930,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 +1041,7 @@ async function handleApplyConfig(req: IncomingMessage, res: ServerResponse): Pro deployedAddresses, executor: wrappedExecutor, stateDir: deploymentDir, + forceStepIds, }); } catch (caughtErr) { if (caughtErr instanceof ConfigExecError) { @@ -1065,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) { @@ -1090,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); @@ -1114,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(); @@ -1232,13 +1398,19 @@ 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 + * 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; @@ -1340,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/apply-config.test.ts b/apps/deploy-server/test/apply-config.test.ts index 0b2b190..24c9786 100644 --- a/apps/deploy-server/test/apply-config.test.ts +++ b/apps/deploy-server/test/apply-config.test.ts @@ -689,6 +689,312 @@ 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("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"; + + 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/deploy-server/test/verify-routes.test.ts b/apps/deploy-server/test/verify-routes.test.ts index 7e4b030..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(() => { @@ -395,6 +400,112 @@ 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 })); + 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 () => { + 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 aaa7720..8983cf6 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"; @@ -593,6 +594,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 @@ -1156,7 +1165,7 @@ export function App() { setVerifyError(null); const [driftOutcome, sourceOutcome] = await Promise.all([ - runVerifyConfig(config), + runVerifyConfig(config, fetch, selectedNetwork ?? undefined), runVerifySource(), ]); @@ -1187,19 +1196,34 @@ 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). 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 +1231,40 @@ 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. + // + // 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); setApplying(true); setApplyConfigError(null); setApplyConfigSuccess(null); setApplyConfigSteps([]); try { - const result = await runApplyConfig(config, fetch, selectedNetwork ?? undefined); + const result = await runApplyConfig(specToSend, fetch, selectedNetwork ?? undefined, forceStepIds); setApplyConfigSteps(result.steps); @@ -1233,6 +1280,30 @@ 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. + // + // 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, fetch, selectedNetwork ?? undefined); + 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 +1322,7 @@ export function App() { } finally { setApplying(false); } - }, [applying, config, selectedNetwork]); + }, [applying, config, selectedNetwork, pendingReapplyStepId]); const deployBtnStyle: React.CSSProperties = { ...btnStyle, @@ -1530,17 +1601,31 @@ 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}{" "} + 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. +

+ ) : ( +

+ 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 +1655,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 +1836,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..49f865a 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,24 +113,98 @@ 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; +} + +// 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)", }, @@ -140,15 +214,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 +326,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 +409,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 +427,7 @@ export function Inspector({ driftResults, sourceVerifyResults, applyConfigResults, + onReapplyStep, }: InspectorProps) { const { nodes, edges } = useMemo( () => deploymentViewToFlow(view, sourceVerifyResults), @@ -324,6 +484,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..73e0792 100644 --- a/apps/studio/src/deploy/apply-config-client.ts +++ b/apps/studio/src/deploy/apply-config-client.ts @@ -181,26 +181,81 @@ 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 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 + * (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, fetchFn: typeof fetch = fetch, network?: string, + forceStepIds?: readonly string[], ): 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. + // 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) { + for (const id of forceStepIds) { + queryParts.push(`forceStepIds=${encodeURIComponent(id)}`); + } + } 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/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/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 new file mode 100644 index 0000000..52c091b --- /dev/null +++ b/apps/studio/test/App.reapply.test.tsx @@ -0,0 +1,597 @@ +/** + * 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, within } 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("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: [ + { + 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"); + }); + + 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..71740f2 --- /dev/null +++ b/apps/studio/test/App.reapplyNarrowing.test.tsx @@ -0,0 +1,220 @@ +/** + * 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(); + // 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(); + }); + + 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: { 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 () => { + 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 88849ff..b78c2c1 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,283 @@ 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("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( + , + ); + 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 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( + , + ); + 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..af5ae88 100644 --- a/apps/studio/test/apply-config-client.test.ts +++ b/apps/studio/test/apply-config-client.test.ts @@ -336,3 +336,112 @@ 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 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&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 })); + + 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/apps/studio/test/narrow-config.test.ts b/apps/studio/test/narrow-config.test.ts new file mode 100644 index 0000000..6a5b243 --- /dev/null +++ b/apps/studio/test/narrow-config.test.ts @@ -0,0 +1,93 @@ +/** + * 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"]); + }); + + // 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/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(); 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..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"; @@ -934,6 +934,257 @@ 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(); + + // 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"); + + // "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(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(), + ); + }); + + 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 + .trim() + .split("\n") + .filter(Boolean) + .map((l) => (JSON.parse(l) as { id: string }).id); + + expect(ids.filter((id) => id === "set-fee")).toHaveLength(1); + }); + + 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 // ---------------------------------------------------------------------------