From 8b9c877497d3ca73d31d9ab3aa8e5604ae8c4084 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 13:50:30 +0200 Subject: [PATCH 01/15] feat(cli): add credential context for error remedies --- .../utils/error/credential-context.test.ts | 40 +++++++++++++++++++ .../cli/src/utils/error/credential-context.ts | 34 ++++++++++++++++ packages/cli/src/utils/error/index.ts | 1 + 3 files changed, 75 insertions(+) create mode 100644 packages/cli/src/utils/error/credential-context.test.ts create mode 100644 packages/cli/src/utils/error/credential-context.ts diff --git a/packages/cli/src/utils/error/credential-context.test.ts b/packages/cli/src/utils/error/credential-context.test.ts new file mode 100644 index 000000000..23b2096b2 --- /dev/null +++ b/packages/cli/src/utils/error/credential-context.test.ts @@ -0,0 +1,40 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { + getCredentialContext, + resetCredentialContext, + setCredentialContext, +} from "./credential-context"; + +describe("credential context", () => { + beforeEach(() => { + resetCredentialContext(); + }); + + it("should default to an unknown credential kind", () => { + expect(getCredentialContext()).toEqual({ kind: "unknown" }); + }); + + it("should return the context that was set", () => { + setCredentialContext({ kind: "oauth", spaces: [{ id: 1, region: "eu" }], space: "1" }); + + expect(getCredentialContext()).toEqual({ + kind: "oauth", + spaces: [{ id: 1, region: "eu" }], + space: "1", + }); + }); + + it("should replace the previous context rather than merging into it", () => { + setCredentialContext({ kind: "oauth", spaces: [{ id: 1, region: "eu" }] }); + setCredentialContext({ kind: "pat" }); + + expect(getCredentialContext()).toEqual({ kind: "pat" }); + }); + + it("should reset back to unknown", () => { + setCredentialContext({ kind: "pat" }); + resetCredentialContext(); + + expect(getCredentialContext()).toEqual({ kind: "unknown" }); + }); +}); diff --git a/packages/cli/src/utils/error/credential-context.ts b/packages/cli/src/utils/error/credential-context.ts new file mode 100644 index 000000000..8222218dd --- /dev/null +++ b/packages/cli/src/utils/error/credential-context.ts @@ -0,0 +1,34 @@ +export type CredentialKind = "oauth" | "pat" | "unknown"; + +/** + * The active credential, pushed in by `program.ts`'s preAction hook so the error + * layer can tailor remedies without importing `session.ts` (which would create an + * import cycle through the `utils` barrel). + * + * `kind` stays `"unknown"` until a session is initialized. That is deliberate: + * during `storyblok login` there is no session yet, and the login actions must keep + * their own messages. + */ +export type CredentialContext = { + kind: CredentialKind; + /** Spaces the grant is restricted to, when known. Empty or absent means unrestricted. */ + spaces?: { id: number; region: string }[]; + /** The space the current command targets, when one was resolved. */ + space?: string | number; +}; + +const UNKNOWN_CONTEXT: CredentialContext = { kind: "unknown" }; + +let currentContext: CredentialContext = UNKNOWN_CONTEXT; + +export function setCredentialContext(context: CredentialContext): void { + currentContext = context; +} + +export function getCredentialContext(): CredentialContext { + return currentContext; +} + +export function resetCredentialContext(): void { + currentContext = UNKNOWN_CONTEXT; +} diff --git a/packages/cli/src/utils/error/index.ts b/packages/cli/src/utils/error/index.ts index e9c33637e..1377704b1 100644 --- a/packages/cli/src/utils/error/index.ts +++ b/packages/cli/src/utils/error/index.ts @@ -1,3 +1,4 @@ +export * from "./credential-context"; export * from "./api-error"; export * from "./command-error"; export * from "./error"; From 4a1259fe04336c24d5aea3a5d96dd1203fdcaf3e Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 13:54:18 +0200 Subject: [PATCH 02/15] feat(cli): add credential error message catalog --- .../src/utils/error/credential-hint.test.ts | 135 ++++++++++++++++++ .../cli/src/utils/error/credential-hint.ts | 99 +++++++++++++ packages/cli/src/utils/error/index.ts | 1 + 3 files changed, 235 insertions(+) create mode 100644 packages/cli/src/utils/error/credential-hint.test.ts create mode 100644 packages/cli/src/utils/error/credential-hint.ts diff --git a/packages/cli/src/utils/error/credential-hint.test.ts b/packages/cli/src/utils/error/credential-hint.test.ts new file mode 100644 index 000000000..3fdf5fe14 --- /dev/null +++ b/packages/cli/src/utils/error/credential-hint.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it } from "vitest"; +import type { CredentialContext } from "./credential-context"; +import { formatSpaceNotAllowedMessage, matchCredentialError } from "./credential-hint"; + +const oauth: CredentialContext = { kind: "oauth" }; +const pat: CredentialContext = { kind: "pat" }; +const unknown: CredentialContext = { kind: "unknown" }; + +describe("matchCredentialError", () => { + it("should name the missing permission and the consent screen for an OAuth session", () => { + const hint = matchCredentialError(403, "Insufficient scope: stories:write is required", oauth); + + expect(hint).toEqual({ + errorId: "insufficient_scope", + fatal: true, + message: + 'Your OAuth login is missing the "stories:write" permission. Re-run `storyblok login` and grant it at the consent screen.', + }); + }); + + it("should name the missing scope and token creation for a PAT session", () => { + const hint = matchCredentialError(403, "Insufficient scope: assets:write is required", pat); + + expect(hint?.errorId).toBe("insufficient_scope"); + expect(hint?.message).toBe( + 'Your personal access token is missing the "assets:write" scope. Create a new token with that scope under My account, Personal access tokens.', + ); + }); + + it("should tell OAuth users to use a personal access token for unsupported endpoints", () => { + const hint = matchCredentialError(403, "This endpoint does not support this token type", oauth); + + expect(hint?.errorId).toBe("forbidden"); + expect(hint?.message).toBe( + "This command is not available with an OAuth login yet. Re-run `storyblok login --token ` with a personal access token instead.", + ); + }); + + it("should tell PAT users to sign in with email and password for unsupported endpoints", () => { + const hint = matchCredentialError(403, "This endpoint does not support this token type", pat); + + expect(hint?.message).toBe( + "This command is not available with a personal access token. Run `storyblok login` and sign in with your email and password.", + ); + }); + + it("should list the authorized space ids when the credential is space restricted", () => { + const hint = matchCredentialError(403, "This token is restricted to specific spaces", { + kind: "oauth", + spaces: [ + { id: 12345, region: "eu" }, + { id: 67890, region: "eu" }, + ], + }); + + expect(hint?.message).toBe( + "Your OAuth login is limited to specific spaces. Pass --space with one of: 12345, 67890.", + ); + }); + + it("should fall back to a generic --space hint when the space ids are unknown", () => { + const hint = matchCredentialError(403, "This token is restricted to specific spaces", oauth); + + expect(hint?.message).toBe( + "Your OAuth login is limited to specific spaces. Pass --space .", + ); + }); + + it("should reuse the pre-flight wording when the space is outside the grant", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "oauth", + space: 999, + spaces: [ + { id: 1, region: "eu" }, + { id: 2, region: "eu" }, + ], + }); + + expect(hint?.message).toBe(formatSpaceNotAllowedMessage(999, [1, 2])); + }); + + it("should treat a 401 as a dead session regardless of the body", () => { + expect(matchCredentialError(401, "Unauthorized", oauth)?.message).toBe( + "Your OAuth login is no longer valid, it may have been revoked or expired. Run `storyblok login` to sign in again.", + ); + expect(matchCredentialError(401, undefined, pat)?.message).toBe( + "Your personal access token was rejected. It may have been revoked, create a new one and run `storyblok login --token `.", + ); + }); + + it("should mark every credential failure as fatal", () => { + const cases = [ + matchCredentialError(403, "Insufficient scope: stories:write is required", oauth), + matchCredentialError(403, "This endpoint does not support this token type", oauth), + matchCredentialError(403, "This token is restricted to specific spaces", oauth), + matchCredentialError(403, "This token does not have access to this space", oauth), + matchCredentialError(401, "Unauthorized", oauth), + ]; + + expect(cases.every((hint) => hint?.fatal === true)).toBe(true); + }); + + it("should ignore every status when the credential kind is unknown", () => { + expect( + matchCredentialError(403, "Insufficient scope: stories:write is required", unknown), + ).toBeUndefined(); + expect(matchCredentialError(401, "Unauthorized", unknown)).toBeUndefined(); + }); + + it("should not claim unrelated forbidden responses", () => { + expect( + matchCredentialError(403, "Asset conversion is not available on this plan", oauth), + ).toBeUndefined(); + expect(matchCredentialError(403, undefined, oauth)).toBeUndefined(); + }); + + it("should ignore statuses that are not 401 or 403", () => { + expect(matchCredentialError(422, "slug has already been taken", oauth)).toBeUndefined(); + expect(matchCredentialError(404, "Not Found", oauth)).toBeUndefined(); + expect(matchCredentialError(500, "Internal Server Error", oauth)).toBeUndefined(); + }); + + it("should emit single-line messages only", () => { + const messages = [ + matchCredentialError(403, "Insufficient scope: stories:write is required", oauth)?.message, + matchCredentialError(403, "This endpoint does not support this token type", pat)?.message, + matchCredentialError(401, "Unauthorized", oauth)?.message, + formatSpaceNotAllowedMessage(9, [1]), + ]; + + for (const message of messages) { + expect(message).not.toContain("\n"); + } + }); +}); diff --git a/packages/cli/src/utils/error/credential-hint.ts b/packages/cli/src/utils/error/credential-hint.ts new file mode 100644 index 000000000..a25cac4b5 --- /dev/null +++ b/packages/cli/src/utils/error/credential-hint.ts @@ -0,0 +1,99 @@ +import type { CredentialContext } from "./credential-context"; + +export type CredentialHint = { + errorId: "insufficient_scope" | "forbidden" | "unauthorized"; + message: string; + /** Credential failures are identical for every item, so bulk loops stop on the first one. */ + fatal: true; +}; + +// Signatures rendered by storyrails: scope_enforceable.rb and application_controller.rb. +const INSUFFICIENT_SCOPE = /^Insufficient scope: (\S+) is required$/; +const UNSUPPORTED_TOKEN_TYPE = "This endpoint does not support this token type"; +const SPACE_RESTRICTED = "This token is restricted to specific spaces"; +const SPACE_NOT_ALLOWED = "This token does not have access to this space"; + +/** + * Shared by the `oauth/space-guard.ts` pre-flight check and the post-flight 403 branch, + * so a user sees identical wording whichever one catches the problem first. + */ +export function formatSpaceNotAllowedMessage( + space: string | number, + grantedSpaceIds: number[], +): string { + return ( + `Space ${space} is not covered by your OAuth login (authorized spaces: ${grantedSpaceIds.join(", ")}). ` + + `Re-run \`storyblok login\` and select this space at the consent screen.` + ); +} + +function credentialLabel(kind: CredentialContext["kind"]): string { + return kind === "oauth" ? "Your OAuth login" : "Your personal access token"; +} + +export function matchCredentialError( + status: number, + serverError: string | undefined, + context: CredentialContext, +): CredentialHint | undefined { + if (context.kind === "unknown") { + return undefined; + } + const isOAuth = context.kind === "oauth"; + + if (status === 401) { + return { + errorId: "unauthorized", + fatal: true, + message: isOAuth + ? "Your OAuth login is no longer valid, it may have been revoked or expired. Run `storyblok login` to sign in again." + : "Your personal access token was rejected. It may have been revoked, create a new one and run `storyblok login --token `.", + }; + } + + if (status !== 403 || !serverError) { + return undefined; + } + + const missingScope = serverError.match(INSUFFICIENT_SCOPE)?.[1]; + if (missingScope) { + return { + errorId: "insufficient_scope", + fatal: true, + message: isOAuth + ? `Your OAuth login is missing the "${missingScope}" permission. Re-run \`storyblok login\` and grant it at the consent screen.` + : `Your personal access token is missing the "${missingScope}" scope. Create a new token with that scope under My account, Personal access tokens.`, + }; + } + + if (serverError === UNSUPPORTED_TOKEN_TYPE) { + return { + errorId: "forbidden", + fatal: true, + message: isOAuth + ? "This command is not available with an OAuth login yet. Re-run `storyblok login --token ` with a personal access token instead." + : "This command is not available with a personal access token. Run `storyblok login` and sign in with your email and password.", + }; + } + + if (serverError === SPACE_RESTRICTED) { + const ids = context.spaces?.map((space) => space.id) ?? []; + const target = ids.length > 0 ? `one of: ${ids.join(", ")}` : ""; + return { + errorId: "forbidden", + fatal: true, + message: `${credentialLabel(context.kind)} is limited to specific spaces. Pass --space ${ids.length > 0 ? "with " : ""}${target}.`, + }; + } + + if (serverError === SPACE_NOT_ALLOWED) { + const ids = context.spaces?.map((space) => space.id) ?? []; + return { + errorId: "forbidden", + fatal: true, + message: formatSpaceNotAllowedMessage(context.space ?? "unknown", ids), + }; + } + + return undefined; +} diff --git a/packages/cli/src/utils/error/index.ts b/packages/cli/src/utils/error/index.ts index 1377704b1..3227feddb 100644 --- a/packages/cli/src/utils/error/index.ts +++ b/packages/cli/src/utils/error/index.ts @@ -1,4 +1,5 @@ export * from "./credential-context"; +export * from "./credential-hint"; export * from "./api-error"; export * from "./command-error"; export * from "./error"; From e561a88667b0fae05f69aeb334b39681db83cd7d Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 13:58:05 +0200 Subject: [PATCH 03/15] refactor(cli): share the out-of-grant space message with the error layer --- packages/cli/src/commands/oauth/space-guard.test.ts | 7 +++++-- packages/cli/src/commands/oauth/space-guard.ts | 9 +++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/oauth/space-guard.test.ts b/packages/cli/src/commands/oauth/space-guard.test.ts index 5552927db..5f09bcc21 100644 --- a/packages/cli/src/commands/oauth/space-guard.test.ts +++ b/packages/cli/src/commands/oauth/space-guard.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { formatSpaceNotAllowedMessage } from "../../utils"; import { assertSpaceAllowed } from "./space-guard"; describe("assertSpaceAllowed", () => { @@ -6,8 +7,10 @@ describe("assertSpaceAllowed", () => { expect(() => assertSpaceAllowed(123, [{ id: 123 }])).not.toThrow(); }); - it("should throw when the space is outside the grant", () => { - expect(() => assertSpaceAllowed(999, [{ id: 123 }])).toThrow(/not covered by your OAuth login/); + it("should reject a space outside the grant using the shared wording", () => { + expect(() => assertSpaceAllowed(999, [{ id: 1 }, { id: 2 }])).toThrow( + formatSpaceNotAllowedMessage(999, [1, 2]), + ); }); it("should pass when the grant has no space restriction", () => { diff --git a/packages/cli/src/commands/oauth/space-guard.ts b/packages/cli/src/commands/oauth/space-guard.ts index c59225da0..224aea3b2 100644 --- a/packages/cli/src/commands/oauth/space-guard.ts +++ b/packages/cli/src/commands/oauth/space-guard.ts @@ -1,4 +1,4 @@ -import { CommandError } from "../../utils"; +import { CommandError, formatSpaceNotAllowedMessage } from "../../utils"; // A grant with an empty/absent space list is not space-restricted (storyrails token_scopeable.rb). export const assertSpaceAllowed = ( @@ -13,10 +13,11 @@ export const assertSpaceAllowed = ( } const target = Number(space); if (!grantedSpaces.some((granted) => granted.id === target)) { - const allowed = grantedSpaces.map((granted) => granted.id).join(", "); throw new CommandError( - `Space ${space} is not covered by your OAuth login (authorized spaces: ${allowed}).\n` + - `Re-run \`storyblok login\` and select this space at the consent screen.`, + formatSpaceNotAllowedMessage( + space, + grantedSpaces.map((granted) => granted.id), + ), ); } }; From 2904d37cd8671dc0fe39bd7c50f86c039aa331ea Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:01:35 +0200 Subject: [PATCH 04/15] feat(cli): rewrite credential 401/403 responses into actionable errors Wires matchCredentialError/getCredentialContext into APIError's constructor so every handleAPIError call site benefits without being touched. Also fixes 403 falling through getErrorId() to "generic" (rendered as "Error fetching data from the API") by adding explicit forbidden/insufficient_scope error ids. The rewrite runs as the final constructor step, after the existing server-message promotion, and only replaces the last messageStack entry so the API_ACTIONS[action] context line stays visible. --- .../cli/src/utils/error/api-error.test.ts | 94 ++++++++++++++++++- packages/cli/src/utils/error/api-error.ts | 23 +++++ 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/utils/error/api-error.test.ts b/packages/cli/src/utils/error/api-error.test.ts index 504984699..357925c41 100644 --- a/packages/cli/src/utils/error/api-error.test.ts +++ b/packages/cli/src/utils/error/api-error.test.ts @@ -1,6 +1,7 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { ClientError } from "@storyblok/management-api-client"; -import { APIError, handleAPIError } from "./api-error"; +import { API_ACTIONS, APIError, handleAPIError } from "./api-error"; +import { resetCredentialContext, setCredentialContext } from "./credential-context"; import { FetchError } from "../fetch"; // ClientError tests verify that mapi-client errors (which have a .response property) @@ -416,3 +417,92 @@ describe("aPIError server message extraction", () => { } }); }); + +describe("APIError credential rewrites", () => { + afterEach(() => { + resetCredentialContext(); + }); + + it("should classify a plain 403 as forbidden rather than generic", () => { + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: {}, + }); + + try { + handleAPIError("push_component", error); + } catch (e) { + expect((e as APIError).errorId).toBe("forbidden"); + expect((e as APIError).code).toBe(403); + } + }); + + it("should rewrite an insufficient-scope 403 into an actionable message", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.errorId).toBe("insufficient_scope"); + expect(apiError.fatal).toBe(true); + expect(apiError.message).toBe( + 'Your OAuth login is missing the "stories:write" permission. Re-run `storyblok login` and grant it at the consent screen.', + ); + } + }); + + it("should keep the failed action as the first message stack entry", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.messageStack[0]).toBe(API_ACTIONS.update_story); + expect(apiError.messageStack.at(-1)).toBe(apiError.message); + } + }); + + it("should leave the raw server string alone when the credential kind is unknown", () => { + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.message).toBe("Insufficient scope: stories:write is required"); + expect(apiError.fatal).toBe(false); + } + }); + + it("should not mark unrelated errors as fatal", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Unprocessable", { + status: 422, + statusText: "Unprocessable Entity", + data: { slug: ["has already been taken"] }, + }); + + try { + handleAPIError("create_story", error); + } catch (e) { + expect((e as APIError).fatal).toBe(false); + } + }); +}); diff --git a/packages/cli/src/utils/error/api-error.ts b/packages/cli/src/utils/error/api-error.ts index 79bf6e4e3..9f500c77e 100644 --- a/packages/cli/src/utils/error/api-error.ts +++ b/packages/cli/src/utils/error/api-error.ts @@ -1,3 +1,5 @@ +import { getCredentialContext } from "./credential-context"; +import { matchCredentialError } from "./credential-hint"; import { FetchError } from "../fetch"; export const API_ACTIONS = { @@ -69,6 +71,8 @@ export const API_ERRORS = { not_found: "The requested resource was not found", unprocessable_entity: "The request was well-formed but was unable to be followed due to semantic errors", + forbidden: "The user is not allowed to perform this action", + insufficient_scope: "The credential is missing a required permission", } as const; function getErrorId(status: number): keyof typeof API_ERRORS { @@ -79,6 +83,8 @@ function getErrorId(status: number): keyof typeof API_ERRORS { return "not_found"; case 422: return "unprocessable_entity"; + case 403: + return "forbidden"; default: return status >= 500 ? "server_error" : "generic"; } @@ -180,6 +186,8 @@ export class APIError extends Error { messageStack: string[]; error: FetchError | undefined; response: FetchError["response"] | undefined; + /** True when the failure is credential-level, so bulk loops should stop instead of retrying. */ + fatal: boolean; constructor( errorId: keyof typeof API_ERRORS, action: keyof typeof API_ACTIONS, @@ -194,6 +202,7 @@ export class APIError extends Error { this.messageStack = []; this.error = error; this.response = error?.response; + this.fatal = false; if (!customMessage) { this.messageStack.push(API_ACTIONS[action]); @@ -243,6 +252,20 @@ export class APIError extends Error { this.cause = this.message; } } + + // A credential-level 401/403 gets a rewritten, actionable message. This runs last so it + // wins over the raw server string, and replaces only the final stack entry so the + // `API_ACTIONS[action]` context line above it survives. + const hint = customMessage + ? undefined + : matchCredentialError(this.code, serverMessage, getCredentialContext()); + if (hint) { + this.errorId = hint.errorId; + this.message = hint.message; + this.cause = hint.message; + this.fatal = hint.fatal; + this.messageStack[this.messageStack.length - 1] = hint.message; + } } getInfo() { From c0f5c3609b3c7b8e3e8a9de59ebcd02703721aa5 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:08:54 +0200 Subject: [PATCH 05/15] feat(cli): populate the credential context in the preAction hook --- .../src/program.credential-context.test.ts | 33 +++++++++++++++++++ packages/cli/src/program.ts | 9 +++++ 2 files changed, 42 insertions(+) create mode 100644 packages/cli/src/program.credential-context.test.ts diff --git a/packages/cli/src/program.credential-context.test.ts b/packages/cli/src/program.credential-context.test.ts new file mode 100644 index 000000000..6f38cf02a --- /dev/null +++ b/packages/cli/src/program.credential-context.test.ts @@ -0,0 +1,33 @@ +// Integration coverage for credential-context wiring in the shared preAction hook. +// The matcher cannot be verified in isolation: only a real command run proves the +// context is populated before the action executes. +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { getCredentialContext, resetCredentialContext } from "./utils/error/credential-context"; + +vi.mock("./api", () => ({ + getMapiClient: vi.fn(), +})); + +describe("program preAction credential context", () => { + beforeEach(() => { + resetCredentialContext(); + vi.clearAllMocks(); + }); + + it("should expose the PAT session to the error layer", async () => { + // Deliberately no vi.resetModules() here: resetting the module registry would + // give program.ts a fresh instance of ./utils/error/credential-context, distinct + // from the one statically imported above, so setCredentialContext and + // getCredentialContext would read and write different singletons. + const { getProgram } = await import("./program"); + const program = getProgram(); + program + .command("context-probe-pat") + .option("-s, --space ", "space ID") + .action(() => {}); + + await program.parseAsync(["node", "test", "context-probe-pat", "--space", "4242"]); + + expect(getCredentialContext()).toMatchObject({ kind: "pat", space: "4242" }); + }); +}); diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 5d50cab6b..c1f82cded 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -15,6 +15,7 @@ import { getMapiClient } from "./api"; import { isExpiringSoon } from "./commands/oauth/expiry"; import { refreshOAuthTokens } from "./commands/oauth/refresh"; import { assertSpaceAllowed } from "./commands/oauth/space-guard"; +import { setCredentialContext } from "./utils/error/credential-context"; import { applyConfigToCommander, getCommandAncestry, @@ -105,6 +106,14 @@ export function getProgram(): Command { }); } + // Tell the error layer which credential is in play so 401/403 responses can name + // the right remedy. Set for every credential kind, including none at all. + setCredentialContext({ + kind: state.authType ?? "unknown", + spaces: state.oauthSpaces, + space: targetCommand.optsWithGlobals().space, + }); + // Guard OAuth sessions against operating on spaces outside their consent grant. // A thrown CommandError here propagates out of the preAction hook, rejecting // `program.parseAsync()` in index.ts, which handles it once at the top level. From 053727b2253a54df88e65227fab429dcd106587e Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:14:02 +0200 Subject: [PATCH 06/15] refactor(cli): drop the duplicated oauth 401 message from the user command getUser's local 401 message duplicated the centralized credential rewrite and, worse, suppressed it for OAuth sessions by always passing a customMessage. Gate the local message on an unknown credential context (the login --token validation path, before any session exists) so an OAuth session's 401 goes through the centralized "OAuth login is no longer valid" message instead of the masked-token text. Also collapse the message to a single line; the old template literal leaked its source indentation into the terminal output. --- .../cli/src/commands/login/actions.test.ts | 3 +-- .../cli/src/commands/user/actions.test.ts | 24 ++++++++++++++++--- packages/cli/src/commands/user/actions.ts | 20 +++++++++------- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/login/actions.test.ts b/packages/cli/src/commands/login/actions.test.ts index 30f4ad120..6c59ba318 100644 --- a/packages/cli/src/commands/login/actions.test.ts +++ b/packages/cli/src/commands/login/actions.test.ts @@ -49,8 +49,7 @@ describe("login actions", () => { it("should throw an masked error for invalid token", async () => { await expect(loginWithToken("invalid-token", "eu")).rejects.toThrow( - `The token provided ${chalk.bold("inva*********")} is invalid. - Please make sure you are using the correct token and try again.`, + `The token provided ${chalk.bold("inva*********")} is invalid. Please make sure you are using the correct token and try again.`, ); }); diff --git a/packages/cli/src/commands/user/actions.test.ts b/packages/cli/src/commands/user/actions.test.ts index 379594fda..32ada0a72 100644 --- a/packages/cli/src/commands/user/actions.test.ts +++ b/packages/cli/src/commands/user/actions.test.ts @@ -2,6 +2,7 @@ import { getUser } from "./actions"; import { http, HttpResponse } from "msw"; import { setupServer } from "msw/node"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { resetCredentialContext, setCredentialContext } from "../../utils"; const handlers = [ http.get("https://mapi.storyblok.com/v1/users/me", async ({ request }) => { @@ -19,7 +20,10 @@ const server = setupServer(...handlers); beforeAll(() => server.listen({ onUnhandledRequest: "error" })); -afterEach(() => server.resetHandlers()); +afterEach(() => { + server.resetHandlers(); + resetCredentialContext(); +}); afterAll(() => server.close()); describe("user actions", () => { @@ -37,8 +41,7 @@ describe("user actions", () => { it("should throw an masked error for invalid token", async () => { await expect(getUser("invalid-token", "eu")).rejects.toThrow( - `The token provided inva********* is invalid. - Please make sure you are using the correct token and try again.`, + "The token provided inva********* is invalid. Please make sure you are using the correct token and try again.", ); }); @@ -51,3 +54,18 @@ describe("user actions", () => { await expect(getUser("any-token", "eu")).rejects.toThrow("The server returned an error"); }); }); + +describe("getUser credential errors", () => { + it("should use the centralized message for an OAuth session", async () => { + setCredentialContext({ kind: "oauth" }); + + await expect(getUser({ oauthToken: "sb_oat_dead" }, "eu")).rejects.toThrow( + "Your OAuth login is no longer valid", + ); + }); + + it("should keep the masked-token message while validating a token at login", async () => { + // No session yet: the context is still unknown, so the matcher stays inactive. + await expect(getUser("sb_pat_invalid", "eu")).rejects.toThrow("is invalid"); + }); +}); diff --git a/packages/cli/src/commands/user/actions.ts b/packages/cli/src/commands/user/actions.ts index 96e90cb22..36dca45d0 100644 --- a/packages/cli/src/commands/user/actions.ts +++ b/packages/cli/src/commands/user/actions.ts @@ -1,6 +1,12 @@ import chalk from "chalk"; import type { ApiCredential } from "../../utils"; -import { getResponseStatus, handleAPIError, maskToken, toError } from "../../utils"; +import { + getCredentialContext, + getResponseStatus, + handleAPIError, + maskToken, + toError, +} from "../../utils"; import { createMapiClient } from "../../api"; import type { RegionCode } from "../../constants"; @@ -14,7 +20,6 @@ export type { User } from "../../types"; export const getUser = async (credential: string | ApiCredential, region: RegionCode) => { const config: ApiCredential = typeof credential === "string" ? { personalAccessToken: credential } : credential; - const isOauth = "oauthToken" in config; const token = "personalAccessToken" in config ? config.personalAccessToken : config.oauthToken; try { const client = createMapiClient({ @@ -30,13 +35,12 @@ export const getUser = async (credential: string | ApiCredential, region: Region } catch (maybeError) { const error = toError(maybeError); const status = getResponseStatus(maybeError); + // Only when no session is established, which is the `login --token` validation path. + // With a session present, the centralized credential rewrite owns 401 messaging, and a + // customMessage here would suppress it. const customMessage = - status === 401 - ? isOauth - ? `Your OAuth session has expired or been revoked. - Please run \`storyblok login --oauth\` to authenticate again.` - : `The token provided ${chalk.bold(maskToken(token))} is invalid. - Please make sure you are using the correct token and try again.` + status === 401 && getCredentialContext().kind === "unknown" + ? `The token provided ${chalk.bold(maskToken(token))} is invalid. Please make sure you are using the correct token and try again.` : undefined; handleAPIError("get_user", error, customMessage); } From 4aecedbdf8f2003f09ae941a2f19c3716d5afba4 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:22:07 +0200 Subject: [PATCH 07/15] feat(cli): stop bulk pushes on credential-level failures Without stories:write, `stories push` printed the same missing-permission message once per story, ballooning to ~1500 lines at 500 stories. A missing permission is a property of the credential, not any individual story, so every remaining item would fail identically. FailureCollector now tracks whether a recorded failure is credential-level (APIError.fatal) via hasFatal, and each of the four push loop bodies checks it to short-circuit further recording once set. --- .../stories/push/failure-report.test.ts | 35 +++++++++++++++++++ .../commands/stories/push/failure-report.ts | 12 +++++++ .../src/commands/stories/push/index.test.ts | 34 ++++++++++++++++++ .../cli/src/commands/stories/push/index.ts | 21 +++++++++++ 4 files changed, 102 insertions(+) create mode 100644 packages/cli/src/commands/stories/push/failure-report.test.ts diff --git a/packages/cli/src/commands/stories/push/failure-report.test.ts b/packages/cli/src/commands/stories/push/failure-report.test.ts new file mode 100644 index 000000000..9c39a4fdf --- /dev/null +++ b/packages/cli/src/commands/stories/push/failure-report.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; +import { FailureCollector } from "./failure-report"; +import { APIError, resetCredentialContext, setCredentialContext } from "../../../utils"; +import { FetchError } from "../../../utils/fetch"; + +const insufficientScopeError = (): APIError => { + setCredentialContext({ kind: "oauth" }); + const apiError = new APIError( + "insufficient_scope", + "update_story", + new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }), + ); + resetCredentialContext(); + return apiError; +}; + +describe("FailureCollector fatal tracking", () => { + it("should report no fatal failure for ordinary errors", () => { + const failures = new FailureCollector(); + failures.record({ filename: "a.json" }, new Error("boom")); + + expect(failures.hasFatal).toBe(false); + }); + + it("should report a fatal failure once a credential error is recorded", () => { + const failures = new FailureCollector(); + failures.record({ filename: "a.json" }, insufficientScopeError()); + + expect(failures.hasFatal).toBe(true); + }); +}); diff --git a/packages/cli/src/commands/stories/push/failure-report.ts b/packages/cli/src/commands/stories/push/failure-report.ts index 529511cc4..f77823590 100644 --- a/packages/cli/src/commands/stories/push/failure-report.ts +++ b/packages/cli/src/commands/stories/push/failure-report.ts @@ -112,6 +112,7 @@ function renderFailureReport(ui: UI, records: FailedStoryRecord[], verbose: bool */ export class FailureCollector { private records = new Map(); + private fatal = false; private keyFor(story: StoryIdentity): string { return story.full_slug ?? story.uuid ?? story.filename ?? `__unknown_${this.records.size}`; @@ -123,6 +124,9 @@ export class FailureCollector { * the caller should skip counter updates to avoid double-billing). */ record(story: StoryIdentity, error: Error): boolean { + if (error instanceof APIError && error.fatal) { + this.fatal = true; + } const key = this.keyFor(story); if (this.records.has(key)) { return false; @@ -142,6 +146,14 @@ export class FailureCollector { return this.records.size === 0; } + /** + * True when a recorded failure is credential-level. Such a failure is identical for + * every remaining item, so the caller should stop rather than repeat it per story. + */ + get hasFatal(): boolean { + return this.fatal; + } + get size(): number { return this.records.size; } diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index 46fb2819a..ab2ee9b4b 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -153,6 +153,18 @@ const preconditions = { ); } }, + failsToUpdateStoriesWithInsufficientScope(stories: MockStory[], space = DEFAULT_SPACE) { + for (const story of stories) { + server.use( + http.put(`https://mapi.storyblok.com/v1/spaces/${space}/stories/${story.id}`, () => { + return HttpResponse.json( + { error: "Insufficient scope: stories:write is required" }, + { status: 403 }, + ); + }), + ); + } + }, canListStories(stories: MockStory[], space = DEFAULT_SPACE) { // The push command issues targeted list calls filtered by `by_slugs` or // `by_ids`. Honor both filters so the handler matches the real MAPI shape; @@ -2033,5 +2045,27 @@ describe("stories push command", () => { // The grouped report still lists the story exactly once. expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); }); + + it("should stop after the first credential failure instead of repeating it per story", async () => { + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const storyC = makeMockStory({ slug: "story-c" }); + const localStories = [storyA, storyB, storyC]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // The shared harness mocks a PAT session, so the rendered message is the + // PAT-flavored variant ("Your personal access token is missing the + // \"stories:write\" scope…"), not the OAuth one. Assert on the scope + // name, which both variants contain, so this stays about the count + // rather than the wording. + const errorCalls = (console.error as unknown as ReturnType).mock.calls; + const rendered = errorCalls.map((call) => call[0]).join("\n"); + expect(rendered.match(/stories:write/g)).toHaveLength(1); + }); }); }); diff --git a/packages/cli/src/commands/stories/push/index.ts b/packages/cli/src/commands/stories/push/index.ts index 3bb17d871..7bf4fe486 100644 --- a/packages/cli/src/commands/stories/push/index.ts +++ b/packages/cli/src/commands/stories/push/index.ts @@ -273,6 +273,12 @@ pushCmd.action(async (options, command) => { creationProgress.increment(); }, onStoryError(error, entry) { + // A credential-level failure is identical for every remaining story, so + // once one is recorded, treat any already-queued concurrent entry as a + // no-op instead of repeating the same failure per story. + if (failures.hasFatal) { + return; + } if (failures.record(entry, error)) { summary.creationResults.failed += 1; summary.processResults.total -= 1; @@ -316,6 +322,11 @@ pushCmd.action(async (options, command) => { updateProgress.setTotal(total); }, onStoryError(error, filename) { + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } if (failures.record({ filename }, error)) { summary.processResults.failed += 1; } else { @@ -342,6 +353,11 @@ pushCmd.action(async (options, command) => { summary.processResults.succeeded += 1; }, onStoryError(error, localStory) { + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } // Always keep the audit trail — even when we suppress the // user-facing summary entry for stories that already failed // creation, the file log should retain the full per-phase error. @@ -378,6 +394,11 @@ pushCmd.action(async (options, command) => { summary.updateResults.succeeded += 1; }, onStoryError(error, localStory) { + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } logOnlyError(error, { storyId: localStory.uuid }); if (failures.record(localStory, error)) { summary.updateResults.failed += 1; From ecd026ae9c52cdac7f022696d64f4c040e429087 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:32:15 +0200 Subject: [PATCH 08/15] fix(cli): actually halt the push pipeline on a credential failure, and exit nonzero The prior fix only suppressed repeated reporting; readLocalStoriesStream, mapReferencesStream, and writeStoryStream kept running and writeStoryStream kept issuing PUT requests for every remaining story after the first credential-level failure. Add a shouldStop hook to all three stream stages plus a break in the pass-1 level loop, all keyed off FailureCollector.hasFatal, so no-longer-useful work is skipped instead of merely going unreported. Also: stories push never set process.exitCode, so a run that failed every story exited 0. Set it in the finally block (mirroring assets/push), guarded so it never overrides an exit code handleError already set. Move the mapReferencesStream onStoryError log call above its fatal guard so the audit trail comment stays true, and note why scanLocalStoryIndex's onError needs no guard (its errors are never fatal APIErrors). --- .../src/commands/stories/push/index.test.ts | 54 +++++++++++++++---- .../cli/src/commands/stories/push/index.ts | 30 +++++++++-- packages/cli/src/commands/stories/streams.ts | 36 +++++++++++++ 3 files changed, 106 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index ab2ee9b4b..7853d0127 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -153,17 +153,27 @@ const preconditions = { ); } }, + /** + * Every update fails with the same credential-level 403. Returns a counter + * object tracking how many PUT requests the handler actually received, so + * a test can prove the pipeline stopped issuing requests early rather than + * attempting every story. + */ failsToUpdateStoriesWithInsufficientScope(stories: MockStory[], space = DEFAULT_SPACE) { - for (const story of stories) { - server.use( - http.put(`https://mapi.storyblok.com/v1/spaces/${space}/stories/${story.id}`, () => { - return HttpResponse.json( - { error: "Insufficient scope: stories:write is required" }, - { status: 403 }, - ); - }), - ); - } + const requestCount = { current: 0 }; + const ids = new Set(stories.map((s) => String(s.id))); + server.use( + http.put(`https://mapi.storyblok.com/v1/spaces/${space}/stories/:id`, ({ params }) => { + if (ids.has(String(params.id))) { + requestCount.current += 1; + } + return HttpResponse.json( + { error: "Insufficient scope: stories:write is required" }, + { status: 403 }, + ); + }), + ); + return requestCount; }, canListStories(stories: MockStory[], space = DEFAULT_SPACE) { // The push command issues targeted list calls filtered by `by_slugs` or @@ -202,6 +212,7 @@ describe("stories push command", () => { server.resetHandlers(); getProgram().setOptionValueWithSource("path", undefined, "default"); resetReporter(); + process.exitCode = undefined; }); afterAll(() => server.close()); @@ -2066,6 +2077,29 @@ describe("stories push command", () => { const errorCalls = (console.error as unknown as ReturnType).mock.calls; const rendered = errorCalls.map((call) => call[0]).join("\n"); expect(rendered.match(/stories:write/g)).toHaveLength(1); + // An aborted run is not a silent success. + expect(process.exitCode).toBe(1); + }); + + it("should stop issuing update requests once a credential failure is detected, instead of attempting every story", async () => { + // Comfortably larger than the pipeline's concurrency limit (12 in-flight + // requests by default) so that, if the run kept going, the mock would + // see every story attempted. Proves the pipeline actually halts rather + // than merely going quiet about repeats. + const localStories = Array.from({ length: 40 }, (_, i) => + makeMockStory({ slug: `story-${i}` }), + ); + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + const requestCount = preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Far below the 40 local stories: the pipeline stopped dispatching new + // update requests once the first credential failure was recorded. + expect(requestCount.current).toBeLessThan(localStories.length / 2); + expect(process.exitCode).toBe(1); }); }); }); diff --git a/packages/cli/src/commands/stories/push/index.ts b/packages/cli/src/commands/stories/push/index.ts index 7bf4fe486..04c48a950 100644 --- a/packages/cli/src/commands/stories/push/index.ts +++ b/packages/cli/src/commands/stories/push/index.ts @@ -177,6 +177,8 @@ pushCmd.action(async (options, command) => { scanProgress.increment(); }, onError(error, filename) { + // No `hasFatal` guard here: a local filesystem/parse error can never + // be an `APIError`, so it can never be fatal — nothing to short-circuit. if (failures.record({ filename }, error)) { summary.creationResults.failed += 1; } @@ -290,6 +292,12 @@ pushCmd.action(async (options, command) => { logOnlyError(error, { storyId: entry.uuid }); }, }); + // A credential failure inside this level was recorded; the same + // failure would repeat for every remaining level, so stop creating + // placeholders rather than issuing more doomed requests. + if (failures.hasFatal) { + break; + } } if (summary.creationResults.failed > 0) { @@ -310,6 +318,7 @@ pushCmd.action(async (options, command) => { // Read local stories from `.json` files. readLocalStoriesStream({ directoryPath: storiesDirectoryPath, + shouldStop: () => failures.hasFatal, fileFilter({ filename }) { // Only load files that were successfully created and mapped. const uuid = uuidByFilename.get(filename); @@ -344,6 +353,7 @@ pushCmd.action(async (options, command) => { mapReferencesStream({ schemas, maps, + shouldStop: () => failures.hasFatal, onIncrement() { processProgress.increment(); }, @@ -353,15 +363,17 @@ pushCmd.action(async (options, command) => { summary.processResults.succeeded += 1; }, onStoryError(error, localStory) { + // Always keep the audit trail — even when we suppress the + // user-facing summary entry for stories that already failed + // creation (or that are skipped below because a credential + // failure was already recorded), the file log should retain the + // full per-phase error. + logOnlyError(error, { storyId: localStory.uuid }); // See the creation-phase `onStoryError` above: a credential-level // failure is identical for every remaining story, so stop repeating it. if (failures.hasFatal) { return; } - // Always keep the audit trail — even when we suppress the - // user-facing summary entry for stories that already failed - // creation, the file log should retain the full per-phase error. - logOnlyError(error, { storyId: localStory.uuid }); if (failures.record(localStory, error)) { summary.processResults.failed += 1; } else { @@ -374,6 +386,7 @@ pushCmd.action(async (options, command) => { }), // Update remote stories with correct references. writeStoryStream({ + shouldStop: () => failures.hasFatal, transports: { writeStory: options.dryRun ? async (story: Story) => story @@ -449,5 +462,14 @@ pushCmd.action(async (options, command) => { reporter.addMeta("failedStories", failures.toReporterMeta()); } reporter.finalize(); + + // Per-story failures (including a credential-level one) go through + // `logOnlyError`, not `handleError`, so they never touch `process.exitCode` + // on their own — without this, a push that failed every story would still + // exit 0. Guarded so we never downgrade an exit code a thrown error + // already set via `handleError` in the `catch` block above. + if (process.exitCode === undefined) { + process.exitCode = failures.isEmpty ? 0 : 1; + } } }); diff --git a/packages/cli/src/commands/stories/streams.ts b/packages/cli/src/commands/stories/streams.ts index 174a2173f..0613f03e5 100644 --- a/packages/cli/src/commands/stories/streams.ts +++ b/packages/cli/src/commands/stories/streams.ts @@ -141,6 +141,7 @@ export const readLocalStoriesStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { directoryPath: string; /** @@ -155,6 +156,13 @@ export const readLocalStoriesStream = ({ onIncrement?: () => void; onStorySuccess?: (story: Story) => void; onStoryError?: (error: Error, filename: string) => void; + /** + * Checked before each file is read. Once it returns `true`, the generator + * stops yielding — a clean end-of-stream, not an error — so a caller-level + * fatal failure (e.g. a credential error) halts the rest of the pipeline + * instead of reading and forwarding every remaining file. + */ + shouldStop?: () => boolean; }) => { const listGenerator = async function* localStoryIterator() { const files = (await readDirectory(directoryPath)).filter( @@ -163,6 +171,9 @@ export const readLocalStoriesStream = ({ setTotalStories?.(files.length); for (const file of files) { + if (shouldStop?.()) { + break; + } try { const filePath = join(directoryPath, file); const fileContent = await readFile(filePath, "utf-8"); @@ -186,16 +197,27 @@ export const mapReferencesStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { schemas: ComponentSchemas; maps: RefMaps; onIncrement?: () => void; onStorySuccess?: (localStory: Story) => void; onStoryError?: (error: Error, story: Story) => void; + /** + * Checked before each chunk is mapped. Once it returns `true`, the chunk is + * dropped without pushing it downstream, so a caller-level fatal failure + * stops feeding `writeStoryStream` further work. + */ + shouldStop?: () => boolean; }) => { return new Transform({ objectMode: true, transform(localStory: Story, _encoding, callback) { + if (shouldStop?.()) { + callback(); + return; + } try { const mappedStory = storyRefMapper(localStory, { schemas, maps }); onStorySuccess?.(mappedStory); @@ -527,6 +549,7 @@ export const writeStoryStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { transports: { writeStory: WriteStoryTransport; @@ -535,12 +558,25 @@ export const writeStoryStream = ({ onIncrement?: () => void; onStorySuccess?: (mappedLocalStory: Story, remoteStory: Story) => void; onStoryError?: (error: Error, story: Story) => void; + /** + * Checked before each write is dispatched. Once it returns `true`, the + * story is dropped without calling `writeStory` (no API call, no + * increment) — a caller-level fatal failure stops issuing further + * requests instead of repeating the same failure per story. Stories + * already dispatched to the transport before the flag flipped still run + * to completion; only not-yet-started work is skipped. + */ + shouldStop?: () => boolean; }) => { const processing = new Set>(); return new Writable({ objectMode: true, async write(mappedLocalStory: Story, _encoding, callback) { + if (shouldStop?.()) { + callback(); + return; + } await getPipelineSlot().acquire(); const task = (async () => { From 86fce112a0eddfeddeb37660526666c3108b5879 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:42:52 +0200 Subject: [PATCH 09/15] fix(cli): report only stories that were actually pushed The push summary headline counted attempted creations as "pushed" and folded skipped creations into "succeeded", so a run that wrote nothing (e.g. every story already existed and every update failed) still printed a fully green summary. The headline now counts updateResults.succeeded only, since a story's content is only written during the update phase; creation only reserves a placeholder id, so summing creation and update successes would double count every newly-created story. Skipped creations get their own label in the per-phase row instead of being folded into "succeeded". --- .../src/commands/stories/push/index.test.ts | 85 +++++++++++++++---- .../cli/src/commands/stories/push/index.ts | 14 ++- 2 files changed, 79 insertions(+), 20 deletions(-) diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index 7853d0127..bd71a64c7 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -427,7 +427,7 @@ describe("stories push command", () => { expect.stringContaining("Push results: 5 stories pushed, 0 stories failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 5/5 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 5/5 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 5/5 succeeded, 0 failed."), @@ -878,8 +878,10 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Push results: 1 story pushed, 0 stories failed"), ); + // The story already existed remotely, so creation was skipped, not + // succeeded. Only the update phase actually wrote anything. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 0 failed, 1 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 1/1 succeeded, 0 failed."), @@ -1396,12 +1398,12 @@ describe("stories push command", () => { // UI — deferred, grouped summary (no inline console.error during streaming) expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); - // UI + // UI — creation itself failed (manifest append), so nothing was written. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1690,7 +1692,7 @@ describe("stories push command", () => { const report = getReport(); expect(report?.status).toBe("FAILURE"); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1841,12 +1843,12 @@ describe("stories push command", () => { // UI — deferred, grouped summary (no inline console.error during streaming) expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining("story-a.json")); - // UI + // UI — the local file failed to load, so no story was ever pushed. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1883,12 +1885,12 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Failed to create placeholder story"), ); - // UI + // UI — placeholder creation failed, so nothing reached the remote. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1935,11 +1937,13 @@ describe("stories push command", () => { expect.stringContaining("Invalid bloks field: expected an array"), ); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); + // The placeholder was created, but mapping references failed before + // the update phase ever ran, so the story's content never got written. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/1 succeeded, 1 failed."), @@ -1998,11 +2002,13 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed to update story")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); + // The placeholder was created and references were mapped, but the + // update call itself failed, so the story's real content never wrote. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 1/1 succeeded, 0 failed."), @@ -2046,9 +2052,10 @@ describe("stories push command", () => { // story disappears from this phase's counts. updateResults: { total: 0, succeeded: 0, failed: 0 }, }); - // Top summary must reflect the distinct failure (1), not 0. + // Top summary must reflect the distinct failure (1), not 0, and nothing + // was actually written, so pushed stays 0. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Updating stories: 0/0 succeeded, 0 failed."), @@ -2057,6 +2064,48 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); }); + it("should not report stories as pushed when every update fails", async () => { + // Regression for a live run measured with `stories:write` missing: + // the headline said "10 stories pushed" while nothing was written. + // Uses a 422 (not 403) so this stays independent of the credential + // short-circuit path covered above. + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const localStories = [storyA, storyB]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + // Matching uuids in the target space mean creation is skipped for + // both stories; only the update phase determines whether anything + // was actually written. + preconditions.canListStories(localStories); + preconditions.failsToUpdateStories(localStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Push results: 0 stories pushed, 2 stories failed"), + ); + expect(console.error).not.toHaveBeenCalledWith(expect.stringContaining("2 stories pushed")); + }); + + it("should count skipped creations separately from succeeded ones", async () => { + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const localStories = [storyA, storyB]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + preconditions.canListStories(localStories); + preconditions.failsToUpdateStories(localStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Every story already existed remotely, so all creations were + // skipped, not succeeded — "0 succeeded" must not fold skipped in. + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Creating stories: 0/2 succeeded, 0 failed, 2 skipped."), + ); + }); + it("should stop after the first credential failure instead of repeating it per story", async () => { const storyA = makeMockStory({ slug: "story-a" }); const storyB = makeMockStory({ slug: "story-b" }); diff --git a/packages/cli/src/commands/stories/push/index.ts b/packages/cli/src/commands/stories/push/index.ts index 04c48a950..5cf8511a8 100644 --- a/packages/cli/src/commands/stories/push/index.ts +++ b/packages/cli/src/commands/stories/push/index.ts @@ -432,11 +432,21 @@ pushCmd.action(async (options, command) => { ui.br(); const failedCount = failures.size; + // A story's content is only actually written to the remote during the + // update phase (creation only reserves a placeholder id, and a + // newly-created story still goes through update to receive its real + // content). So `updateResults.succeeded` alone is "stories actually + // pushed" — summing it with `creationResults.succeeded` would double + // count every newly-created story that also updated successfully. + // `skipped` creations still proceed to update and are counted there, so + // they are neither a success nor a failure of their own and get no + // separate weight in this headline. + const pushedCount = summary.updateResults.succeeded; ui.info( - `Push results: ${summary.creationResults.total} ${summary.creationResults.total === 1 ? "story" : "stories"} pushed, ${failedCount} ${failedCount === 1 ? "story" : "stories"} failed`, + `Push results: ${pushedCount} ${pushedCount === 1 ? "story" : "stories"} pushed, ${failedCount} ${failedCount === 1 ? "story" : "stories"} failed`, ); ui.list([ - `Creating stories: ${summary.creationResults.succeeded + summary.creationResults.skipped}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed.`, + `Creating stories: ${summary.creationResults.succeeded}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed, ${summary.creationResults.skipped} skipped.`, `Processing stories: ${summary.processResults.succeeded}/${summary.processResults.total} succeeded, ${summary.processResults.failed} failed.`, `Updating stories: ${summary.updateResults.succeeded}/${summary.updateResults.total} succeeded, ${summary.updateResults.failed} failed.`, ]); From 8f5496c45fef91bd11560d0240cf06b568799041 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:53:47 +0200 Subject: [PATCH 10/15] fix(cli): keep asset pushes and pulls working when library discovery is forbidden shared_asset_folders carries no scope annotations, so an OAuth credential is 403'd regardless of consent. `push --target auto` and `pull --target with-referenced` now degrade to the space scope with a warning instead of failing outright; explicit `--target shared`/`--target all`/`--library` requests still fail with the actionable message. --- .../src/commands/assets/pull/index.test.ts | 55 +++++++++++++++++++ .../cli/src/commands/assets/pull/index.ts | 36 +++++++++--- .../src/commands/assets/push/index.test.ts | 48 ++++++++++++++++ .../cli/src/commands/assets/push/index.ts | 29 +++++++--- .../cli/src/commands/assets/scope.test.ts | 49 ++++++++++++++++- packages/cli/src/commands/assets/scope.ts | 45 +++++++++++++-- 6 files changed, 237 insertions(+), 25 deletions(-) diff --git a/packages/cli/src/commands/assets/pull/index.test.ts b/packages/cli/src/commands/assets/pull/index.test.ts index ce78e2ee1..c7ea48785 100644 --- a/packages/cli/src/commands/assets/pull/index.test.ts +++ b/packages/cli/src/commands/assets/pull/index.test.ts @@ -143,6 +143,16 @@ const preconditions = { ), ); }, + forbidsLibraryDiscovery() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, hasLocalStoriesReferencing(assetIds: number[]) { vol.fromJSON({ ".storyblok/stories/12345/home_uuid.json": JSON.stringify({ @@ -589,6 +599,51 @@ describe("assets pull command", () => { expect(sharedSpy).not.toHaveBeenCalled(); }); + it("should pull space assets and warn when referenced-library discovery is forbidden", async () => { + const spaceAsset = makeMockAsset({ id: 42 }); + preconditions.hasLocalStoriesReferencing([42, 90]); + preconditions.canFetchRemoteFolders([]); + preconditions.canFetchRemoteAssetPages([[spaceAsset]]); + preconditions.canDownloadAssets([spaceAsset]); + preconditions.sharedAssetResolves({ + id: 90, + filename: "https://a.storyblok.com/g/1/x.png", + asset_folder_id: 7, + }); + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync(["node", "test", "pull", "--space", "12345"]); + + expect(assetFileExists(spaceAsset)).toBeTruthy(); + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(0); + }); + + it("should fail when libraries were explicitly requested but are forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync([ + "node", + "test", + "pull", + "--space", + "12345", + "--target", + "shared", + ]); + + expect( + (console.error as ReturnType).mock.calls + .flat() + .some( + (arg) => typeof arg === "string" && arg.includes("This command is not available with"), + ), + ).toBe(true); + expect(process.exitCode).toBe(1); + }); + it("rejects an invalid --target value", async () => { const pull = assetsCommand.commands.find((command) => command.name() === "pull")!; pull.exitOverride(); diff --git a/packages/cli/src/commands/assets/pull/index.ts b/packages/cli/src/commands/assets/pull/index.ts index e832d630f..042ba2760 100644 --- a/packages/cli/src/commands/assets/pull/index.ts +++ b/packages/cli/src/commands/assets/pull/index.ts @@ -10,6 +10,7 @@ import { resolveCommandPath } from "../../../utils/filesystem"; import { getLogger } from "../../../lib/logger/logger"; import { getReporter } from "../../../lib/reporter/reporter"; import { requireAuthentication } from "../../../utils/auth"; +import { APIError } from "../../../utils/error/api-error"; import { CommandError } from "../../../utils/error/command-error"; import { handleError, logOnlyError, toError } from "../../../utils/error/error"; import { @@ -350,16 +351,33 @@ pullCmd.action(async (options, command) => { const sharedAssets = resolved.filter((asset): asset is Asset => Boolean(asset?.id)); if (sharedAssets.length > 0) { - const resolveRoot = await buildLibraryRootResolver(space); - const byLibrary = new Map(); - for (const asset of sharedAssets) { - const libraryId = resolveRoot(asset.asset_folder_id ?? 0); - const bucket = byLibrary.get(libraryId) ?? []; - bucket.push(asset); - byLibrary.set(libraryId, bucket); + // `with-referenced` is implicit: the user asked to pull assets, not libraries. A + // credential that cannot reach library discovery degrades to the space-only pull + // with a warning rather than failing the whole command. An explicit `--target + // shared`/`--target all` still fails via `listReadableLibraries`. + let resolveRoot: ((assetFolderId: number) => number) | undefined; + try { + resolveRoot = await buildLibraryRootResolver(space); + } catch (error) { + if (error instanceof APIError && error.code === 403) { + getUI().warn( + "Shared libraries are unavailable with this login; skipping referenced library assets.", + ); + } else { + throw error; + } } - for (const [libraryId, assets] of byLibrary) { - await pullReferencedAssets(libraryId, assets); + if (resolveRoot) { + const byLibrary = new Map(); + for (const asset of sharedAssets) { + const libraryId = resolveRoot(asset.asset_folder_id ?? 0); + const bucket = byLibrary.get(libraryId) ?? []; + bucket.push(asset); + byLibrary.set(libraryId, bucket); + } + for (const [libraryId, assets] of byLibrary) { + await pullReferencedAssets(libraryId, assets); + } } } } diff --git a/packages/cli/src/commands/assets/push/index.test.ts b/packages/cli/src/commands/assets/push/index.test.ts index d8dd7ec09..9b18230b5 100644 --- a/packages/cli/src/commands/assets/push/index.test.ts +++ b/packages/cli/src/commands/assets/push/index.test.ts @@ -457,6 +457,16 @@ const preconditions = { [filePath]: content, }); }, + forbidsLibraryDiscovery({ space = DEFAULT_SPACE }: { space?: string } = {}) { + server.use( + http.get(`https://mapi.storyblok.com/v1/spaces/${space}/shared_asset_folders`, () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, hasLibraries( libraries: { id: number; name: string; accessLevel: "read" | "write" }[], { space = DEFAULT_SPACE }: { space?: string } = {}, @@ -2570,5 +2580,43 @@ describe("assets push command", () => { stderrCalls.some((msg: unknown) => typeof msg === "string" && msg.includes("ENOENT")), ).toBe(false); }); + + it("should push space assets and warn when library discovery is forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + const asset = makeMockAsset(); + preconditions.canLoadFolders([]); + preconditions.canLoadAssets([asset]); + preconditions.canUpsertRemoteAssets([asset]); + + await assetsCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(0); + }); + + it("should fail when libraries were explicitly requested but are forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync([ + "node", + "test", + "push", + "--space", + DEFAULT_SPACE, + "--target", + "shared", + ]); + + expect( + (console.error as ReturnType).mock.calls + .flat() + .some( + (arg) => typeof arg === "string" && arg.includes("This command is not available with"), + ), + ).toBe(true); + expect(process.exitCode).toBe(1); + }); }); }); diff --git a/packages/cli/src/commands/assets/push/index.ts b/packages/cli/src/commands/assets/push/index.ts index 06474d446..6a336154b 100644 --- a/packages/cli/src/commands/assets/push/index.ts +++ b/packages/cli/src/commands/assets/push/index.ts @@ -62,6 +62,7 @@ import { makeWriteStoryAPITransport } from "../../stories/streams"; import { assertLibraryWritable, listWritableLibraries, + listWritableLibrariesOrDegrade, resolveScopeBaseDir, type Scope, } from "../scope"; @@ -211,14 +212,26 @@ pushCmd.action(async (assetInput, options, command) => { scopes.push({ kind: "space", spaceId: fromSpace }); } if (target === "shared" || target === "auto") { - // `listWritableLibraries` already filters to writable libraries, so - // these scopes need no further access check. - const libraries = await listWritableLibraries(targetSpace); - scopes.push( - ...libraries.map( - (library) => ({ kind: "library", libraryId: library.id }) satisfies Scope, - ), - ); + // `auto` is implicit: the user asked to push assets, not libraries. A credential that + // cannot reach library discovery degrades to the space scope with a warning rather + // than failing the whole command. An explicit `--target shared` still fails. + // `listWritableLibraries`/`listWritableLibrariesOrDegrade` already filter to writable + // libraries, so these scopes need no further access check. + const libraries = + target === "auto" + ? await listWritableLibrariesOrDegrade(targetSpace) + : await listWritableLibraries(targetSpace); + if (libraries === undefined) { + getUI().warn( + "Shared libraries are unavailable with this login; continuing with space assets only.", + ); + } else { + scopes.push( + ...libraries.map( + (library) => ({ kind: "library", libraryId: library.id }) satisfies Scope, + ), + ); + } } } } catch (maybeError) { diff --git a/packages/cli/src/commands/assets/scope.test.ts b/packages/cli/src/commands/assets/scope.test.ts index 16f6408f1..b41453e44 100644 --- a/packages/cli/src/commands/assets/scope.test.ts +++ b/packages/cli/src/commands/assets/scope.test.ts @@ -1,5 +1,50 @@ -import { describe, expect, it } from "vitest"; -import { resolveScopeBaseDir, type Scope } from "./scope"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { http, HttpResponse } from "msw"; +import { setupServer } from "msw/node"; +import { getMapiClient } from "../../api"; +import { listLibrariesOrDegrade, resolveScopeBaseDir, type Scope } from "./scope"; + +const server = setupServer(); +beforeAll(() => server.listen({ onUnhandledRequest: "error" })); +beforeEach(() => { + getMapiClient({ personalAccessToken: "valid-token", region: "eu" }); +}); +afterEach(() => server.resetHandlers()); +afterAll(() => server.close()); + +const preconditions = { + forbidsLibraryDiscovery() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, + hasNoLibraries() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json({ shared_asset_folders: [] }), + ), + ); + }, +}; + +describe("listLibrariesOrDegrade", () => { + it("should return undefined when the credential cannot reach library discovery", async () => { + preconditions.forbidsLibraryDiscovery(); + + await expect(listLibrariesOrDegrade("12345")).resolves.toBeUndefined(); + }); + + it("should return an empty list when the space simply has no libraries", async () => { + preconditions.hasNoLibraries(); + + await expect(listLibrariesOrDegrade("12345")).resolves.toEqual([]); + }); +}); describe("resolveScopeBaseDir", () => { it("returns the space subtree for a space scope", () => { diff --git a/packages/cli/src/commands/assets/scope.ts b/packages/cli/src/commands/assets/scope.ts index d95d494ac..8566991de 100644 --- a/packages/cli/src/commands/assets/scope.ts +++ b/packages/cli/src/commands/assets/scope.ts @@ -1,7 +1,7 @@ import { join } from "pathe"; import { directories } from "../../constants"; import { getMapiClient } from "../../api"; -import { handleAPIError } from "../../utils/error/api-error"; +import { APIError, handleAPIError } from "../../utils/error/api-error"; import { toError } from "../../utils/error/error"; import { resolveCommandPath } from "../../utils/filesystem"; import type { SharedAssetFolder } from "./types"; @@ -65,6 +65,33 @@ export const listReadableLibraries = (spaceId: string): Promise => export const listWritableLibraries = async (spaceId: string): Promise => (await listLibraries(spaceId)).filter((library) => library.accessLevel === "write"); +/** + * Library discovery that tolerates a credential which cannot reach the endpoint at all. + * Returns `undefined` when the lookup was forbidden, distinct from `[]` for a space with + * no libraries, so callers can warn only in the former case. + * + * The shared-asset controllers carry no scope annotations, so an OAuth grant is refused + * regardless of consent (storyrails credential.rb). Implicit callers degrade to the space + * scope rather than failing a command the user did not aim at libraries. + */ +export async function listLibrariesOrDegrade(spaceId: string): Promise { + try { + return await listLibraries(spaceId); + } catch (error) { + if (error instanceof APIError && error.code === 403) { + return undefined; + } + throw error; + } +} + +export const listWritableLibrariesOrDegrade = async ( + spaceId: string, +): Promise => { + const libraries = await listLibrariesOrDegrade(spaceId); + return libraries?.filter((library) => library.accessLevel === "write"); +}; + /** * Builds a resolver mapping any shared `asset_folder_id` to its top-level * library root id (walking `parent_id` up). Fetches the space's shared folders @@ -73,12 +100,18 @@ export const listWritableLibraries = async (spaceId: string): Promise export async function buildLibraryRootResolver( spaceId: string, ): Promise<(assetFolderId: number) => number> { - const { data } = await getMapiClient().sharedAssetFolders.list({ - path: { space_id: Number(spaceId) }, - throwOnError: true, - }); + let folders: SharedAssetFolder[]; + try { + const { data } = await getMapiClient().sharedAssetFolders.list({ + path: { space_id: Number(spaceId) }, + throwOnError: true, + }); + folders = data?.shared_asset_folders ?? []; + } catch (maybeError) { + handleAPIError("pull_shared_asset_folders", toError(maybeError)); + } const parentById = new Map(); - for (const folder of data?.shared_asset_folders ?? []) { + for (const folder of folders) { parentById.set(folder.id, folder.parent_id ?? null); } return (assetFolderId: number): number => { From 8b7a0aa0e21cace1796549f69c375a297d6f65b5 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 14:59:59 +0200 Subject: [PATCH 11/15] fix(cli): scope the missing oauth client advisory to each caller resolveOAuthClient() threw one message advising --oauth and a PAT login regardless of caller. logout and the preAction refresh path surfaced that login advice while removing credentials or silently refreshing a session. Reduce the thrown error to the cause only; each caller now appends its own consequence and remedy. --- packages/cli/src/commands/logout/oauth.test.ts | 13 +++++++++++++ packages/cli/src/commands/oauth/client.test.ts | 12 ++++++++++++ packages/cli/src/commands/oauth/client.ts | 8 +++----- packages/cli/src/commands/oauth/login-flow.ts | 9 ++++++++- packages/cli/src/commands/oauth/refresh.ts | 9 ++++++++- 5 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/commands/logout/oauth.test.ts b/packages/cli/src/commands/logout/oauth.test.ts index 1041f7378..997cc372e 100644 --- a/packages/cli/src/commands/logout/oauth.test.ts +++ b/packages/cli/src/commands/logout/oauth.test.ts @@ -6,6 +6,7 @@ import { setupServer } from "msw/node"; import "../../index"; import { logoutCommand } from "./index"; import { getOAuthEntry } from "../oauth/store"; +import { getUI } from "../../lib/ui"; vi.mock("node:fs"); vi.mock("node:fs/promises"); @@ -99,4 +100,16 @@ describe("logout with an oauth session", () => { expect(revokeRequests).toHaveLength(0); expect(await getOAuthEntry("eu")).toEqual({}); }); + + it("should not advise a login flag while logging out", async () => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + const warnSpy = vi.spyOn(getUI(), "warn").mockImplementation(() => {}); + + await logoutCommand.parseAsync(["node", "test"]); + + const warned = warnSpy.mock.calls.flat().join("\n"); + expect(warned).toContain("Could not revoke the OAuth session server-side"); + expect(warned).not.toContain("--oauth"); + }); }); diff --git a/packages/cli/src/commands/oauth/client.test.ts b/packages/cli/src/commands/oauth/client.test.ts index 2ee2f8db7..8200f6b90 100644 --- a/packages/cli/src/commands/oauth/client.test.ts +++ b/packages/cli/src/commands/oauth/client.test.ts @@ -20,4 +20,16 @@ describe("resolveOAuthClient", () => { expect(OAUTH_CLIENT_ID.startsWith(OAUTH_CLIENT_PLACEHOLDER_PREFIX)).toBe(true); expect(() => resolveOAuthClient()).toThrow(/ships without OAuth client credentials/); }); + + it("should throw a cause-only message that names no command flag", () => { + expect(() => resolveOAuthClient()).toThrow( + "This build of the CLI ships without OAuth client credentials.", + ); + try { + resolveOAuthClient(); + } catch (error) { + expect((error as Error).message).not.toContain("--oauth"); + expect((error as Error).message).not.toContain("\n"); + } + }); }); diff --git a/packages/cli/src/commands/oauth/client.ts b/packages/cli/src/commands/oauth/client.ts index 1edc6f75a..2cad63446 100644 --- a/packages/cli/src/commands/oauth/client.ts +++ b/packages/cli/src/commands/oauth/client.ts @@ -12,11 +12,9 @@ export const resolveOAuthClient = (): OAuthClientCredentials => { } if (OAUTH_CLIENT_ID.startsWith(OAUTH_CLIENT_PLACEHOLDER_PREFIX)) { - throw new CommandError( - `This build of the CLI ships without OAuth client credentials, so \`--oauth\` cannot be used yet.\n` + - `Log in with a Personal Access Token (\`storyblok login --token \`), or set the ` + - `STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET environment variables to use your own OAuth app.`, - ); + // Cause only. Each caller appends the consequence and remedy, because only the caller + // knows whether the user was logging in, refreshing a session, or revoking one. + throw new CommandError("This build of the CLI ships without OAuth client credentials."); } return { client_id: OAUTH_CLIENT_ID, client_secret: OAUTH_CLIENT_SECRET }; diff --git a/packages/cli/src/commands/oauth/login-flow.ts b/packages/cli/src/commands/oauth/login-flow.ts index 82772b0bb..54eb6def7 100644 --- a/packages/cli/src/commands/oauth/login-flow.ts +++ b/packages/cli/src/commands/oauth/login-flow.ts @@ -51,7 +51,14 @@ export const performOAuthLogin = async (options: { const openBrowser = options.openBrowser ?? ((url) => open(url)); const ui = getUI(); - const client = resolveOAuthClient(); + let client; + try { + client = resolveOAuthClient(); + } catch (error) { + throw new CommandError( + `${(error as Error).message} Log in with a Personal Access Token (\`storyblok login --token \`), or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app.`, + ); + } const scopes = OAUTH_LOGIN_SCOPES; const { verifier, challenge } = generatePkce(); const state = generateState(); diff --git a/packages/cli/src/commands/oauth/refresh.ts b/packages/cli/src/commands/oauth/refresh.ts index b7b48ae45..600fb82cf 100644 --- a/packages/cli/src/commands/oauth/refresh.ts +++ b/packages/cli/src/commands/oauth/refresh.ts @@ -20,7 +20,14 @@ const doRefresh = async (region: RegionCode): Promise => { throw new CommandError("No OAuth refresh token stored. Run `storyblok login` to authenticate."); } - const client = resolveOAuthClient(); + let client; + try { + client = resolveOAuthClient(); + } catch (error) { + throw new CommandError( + `Your OAuth session cannot be refreshed: ${(error as Error).message} Log in with a Personal Access Token (\`storyblok login --token \`).`, + ); + } let response; try { From 446e21f0330ff7481df217b3e1dd55ea5db0e5d3 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 15:05:33 +0200 Subject: [PATCH 12/15] test(cli): cover the login and refresh oauth-client remedy strings The try/catch blocks added for the login and refresh remedies had zero coverage: login-flow.test.ts mocks ./client wholesale, and refresh.test.ts always sets the env-var override, so neither catch path ever ran. Add one test per caller that reaches the real placeholder-client guard and asserts on each caller's distinctive remedy text. --- .../cli/src/commands/oauth/login-flow.test.ts | 29 +++++++++++++++++++ .../cli/src/commands/oauth/refresh.test.ts | 12 ++++++++ 2 files changed, 41 insertions(+) diff --git a/packages/cli/src/commands/oauth/login-flow.test.ts b/packages/cli/src/commands/oauth/login-flow.test.ts index ec3ec0ee9..c8281ca5a 100644 --- a/packages/cli/src/commands/oauth/login-flow.test.ts +++ b/packages/cli/src/commands/oauth/login-flow.test.ts @@ -105,3 +105,32 @@ describe("performOAuthLogin", () => { expect(await getOAuthActiveRegion()).toBeUndefined(); }); }); + +// This suite's top-level `vi.mock("./client", ...)` stubs resolveOAuthClient for every other +// test above so they don't trip the placeholder-client guard. Here we unmock it for real, so +// performOAuthLogin runs against the actual resolveOAuthClient() and its thrown cause, letting +// this test verify the login-specific remedy the catch block in login-flow.ts appends. +describe("performOAuthLogin when no oauth client credentials are available", () => { + afterEach(() => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + vi.doMock("./client", () => ({ + resolveOAuthClient: vi.fn(() => ({ client_id: "cid", client_secret: "sec" })), + })); + vi.resetModules(); + }); + + it("should surface the login remedy naming a token login and the client env vars", async () => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + vi.doUnmock("./client"); + vi.resetModules(); + const { performOAuthLogin: performOAuthLoginWithRealClient } = await import("./login-flow"); + + await expect( + performOAuthLoginWithRealClient({ region: "eu", openBrowser: async () => {} }), + ).rejects.toThrow( + "or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app", + ); + }); +}); diff --git a/packages/cli/src/commands/oauth/refresh.test.ts b/packages/cli/src/commands/oauth/refresh.test.ts index f3c9c455a..c1f27ffa8 100644 --- a/packages/cli/src/commands/oauth/refresh.test.ts +++ b/packages/cli/src/commands/oauth/refresh.test.ts @@ -123,4 +123,16 @@ describe("refreshOAuthTokens", () => { }); await expect(refreshOAuthTokens("eu")).rejects.toThrow(); }); + + it("should surface the refresh remedy when no oauth client credentials are available", async () => { + // Override this suite's beforeEach, which sets these so refreshes normally resolve their + // credentials through the env-var override; deleting them here reaches the placeholder + // guard in resolveOAuthClient() and exercises the catch block's refresh-specific remedy. + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + + await expect(refreshOAuthTokens("eu")).rejects.toThrow( + "Your OAuth session cannot be refreshed:", + ); + }); }); From fcad8efa0771517ec417fd34f73a6a66e2e9f570 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 15:10:18 +0200 Subject: [PATCH 13/15] docs(cli): document oauth permission errors and the library limitation Also documents three stories push behavior changes from the preceding tasks: halting on the first credential-level failure, exiting 1 on any story failure, and the summary headline counting only stories whose content was actually written. --- packages/cli/src/commands/login/README.md | 3 +++ packages/cli/test/GUIDE.md | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/packages/cli/src/commands/login/README.md b/packages/cli/src/commands/login/README.md index 9433717d6..06d001dbf 100644 --- a/packages/cli/src/commands/login/README.md +++ b/packages/cli/src/commands/login/README.md @@ -66,6 +66,9 @@ own OAuth app instead, for example while developing against a self-hosted instan - If you're already logged in, you'll need to logout first to switch accounts - For CI environments, it's recommended to use the `--token` option - The CLI supports two-factor authentication (2FA) when using email login +- If a command reports a missing permission, the message names the scope and the fix. For an OAuth + login, re-run `storyblok login` and grant it at the consent screen. For a personal access token, + create a new token with that scope. > If you sign in with SSO (e.g., Google, GitHub, Azure AD), you must use a Personal Access Token. > Generate one in your account settings: diff --git a/packages/cli/test/GUIDE.md b/packages/cli/test/GUIDE.md index 004bf2d3f..ecd5d9f70 100644 --- a/packages/cli/test/GUIDE.md +++ b/packages/cli/test/GUIDE.md @@ -26,6 +26,14 @@ files. - IMPORTANT: When running `assets push --update-stories` or `stories push`, make sure you run `components pull` first! +- `stories push` stops at the first credential-level failure (for example a missing scope) instead + of repeating the same error for every remaining story, and it now exits 1 whenever any story + failed, where it previously always exited 0. Watch for this when scripting CI checks around the + exit code. +- The `stories push` summary headline ("N stories pushed") counts only stories whose content was + actually written, not stories attempted. A run that creates a placeholder but fails to write its + content reports 0 pushed; skipped creations get their own "skipped" label in the per-phase rows + instead of counting as succeeded. ### Scenario seeds @@ -83,6 +91,13 @@ read or write access. `assets pull` and `assets push` reach libraries through `- `--library`. Library assets live under `.storyblok/assets/shared//`, parallel to the space subtree at `.storyblok/assets//`, each with its own `manifest.jsonl`. +Shared libraries are unreachable with an OAuth login. `shared_asset_folders_controller` and +`shared_internal_tags_controller` carry no `require_token_scopes`, so storyrails default-denies any +scoped credential, and `credential.rb` never grants an OAuth grant `user_permission?`. Consent +cannot change this. `assets push` and `assets pull` therefore warn and continue with the space +scope; `--target shared`, `--target all`, and `--library` still fail. Use a personal access token to +test libraries. + ```bash # Pull only the readable libraries (writes .storyblok/assets/shared//). ./dist/index.mjs assets pull --space $STORYBLOK_SPACE_ID --target shared From a01905f43e410daa4c6f8dd794d69ef75de05c55 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Wed, 12 Aug 2026 15:32:12 +0200 Subject: [PATCH 14/15] fix(cli): correct PAT space-restriction wording and tighten library-discovery degrade - credential-hint.ts: give PATs their own space-not-allowed message instead of reusing OAuth wording (consent screen, empty authorized-spaces list); omit the authorized-spaces parenthetical entirely when no space ids are known, for both credential kinds. - APIError now exposes the raw pre-rewrite serverError string so listLibrariesOrDegrade/buildLibraryRootResolver can degrade only on the unsupported-token-type signature instead of any 403. - Add missing coverage: customMessage suppression invariant, pat 401 context, and a different-403-still-fails regression for the library discovery degrade paths. - login-flow.ts/refresh.ts: type `client` explicitly and drop `as Error` casts in favor of instanceof narrowing. - stories/push: fix a stale "422" comment (helper returns 500), reset process.exitCode in afterAll so it can't leak into other suites, and document the exit-1-on-any-failure behavior in the push command README. --- .../src/commands/assets/pull/index.test.ts | 31 +++++++++++++++++ .../cli/src/commands/assets/pull/index.ts | 7 +++- .../cli/src/commands/assets/scope.test.ts | 16 +++++++++ packages/cli/src/commands/assets/scope.ts | 7 +++- packages/cli/src/commands/oauth/login-flow.ts | 7 ++-- packages/cli/src/commands/oauth/refresh.ts | 7 ++-- .../cli/src/commands/stories/push/README.md | 3 ++ .../src/commands/stories/push/index.test.ts | 7 ++-- .../cli/src/commands/user/actions.test.ts | 8 +++++ .../cli/src/utils/error/api-error.test.ts | 17 ++++++++++ packages/cli/src/utils/error/api-error.ts | 9 +++++ .../src/utils/error/credential-hint.test.ts | 25 ++++++++++++++ .../cli/src/utils/error/credential-hint.ts | 33 +++++++++++++++++-- 13 files changed, 164 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/assets/pull/index.test.ts b/packages/cli/src/commands/assets/pull/index.test.ts index c7ea48785..b4ba2b6b1 100644 --- a/packages/cli/src/commands/assets/pull/index.test.ts +++ b/packages/cli/src/commands/assets/pull/index.test.ts @@ -153,6 +153,16 @@ const preconditions = { ), ); }, + forbidsLibraryDiscoveryForAnotherReason() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This token is restricted to specific spaces" }, + { status: 403 }, + ), + ), + ); + }, hasLocalStoriesReferencing(assetIds: number[]) { vol.fromJSON({ ".storyblok/stories/12345/home_uuid.json": JSON.stringify({ @@ -621,6 +631,27 @@ describe("assets pull command", () => { expect(process.exitCode).toBe(0); }); + it("should fail rather than degrade when referenced-library discovery fails for a non-token-type reason", async () => { + const spaceAsset = makeMockAsset({ id: 42 }); + preconditions.hasLocalStoriesReferencing([42, 90]); + preconditions.canFetchRemoteFolders([]); + preconditions.canFetchRemoteAssetPages([[spaceAsset]]); + preconditions.canDownloadAssets([spaceAsset]); + preconditions.sharedAssetResolves({ + id: 90, + filename: "https://a.storyblok.com/g/1/x.png", + asset_folder_id: 7, + }); + preconditions.forbidsLibraryDiscoveryForAnotherReason(); + + await assetsCommand.parseAsync(["node", "test", "pull", "--space", "12345"]); + + expect(console.warn).not.toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(1); + }); + it("should fail when libraries were explicitly requested but are forbidden", async () => { preconditions.forbidsLibraryDiscovery(); diff --git a/packages/cli/src/commands/assets/pull/index.ts b/packages/cli/src/commands/assets/pull/index.ts index 042ba2760..25ed6d0ca 100644 --- a/packages/cli/src/commands/assets/pull/index.ts +++ b/packages/cli/src/commands/assets/pull/index.ts @@ -12,6 +12,7 @@ import { getReporter } from "../../../lib/reporter/reporter"; import { requireAuthentication } from "../../../utils/auth"; import { APIError } from "../../../utils/error/api-error"; import { CommandError } from "../../../utils/error/command-error"; +import { isUnsupportedTokenTypeServerError } from "../../../utils/error/credential-hint"; import { handleError, logOnlyError, toError } from "../../../utils/error/error"; import { downloadAssetStream, @@ -359,7 +360,11 @@ pullCmd.action(async (options, command) => { try { resolveRoot = await buildLibraryRootResolver(space); } catch (error) { - if (error instanceof APIError && error.code === 403) { + if ( + error instanceof APIError && + error.code === 403 && + isUnsupportedTokenTypeServerError(error.serverError) + ) { getUI().warn( "Shared libraries are unavailable with this login; skipping referenced library assets.", ); diff --git a/packages/cli/src/commands/assets/scope.test.ts b/packages/cli/src/commands/assets/scope.test.ts index b41453e44..5734cf223 100644 --- a/packages/cli/src/commands/assets/scope.test.ts +++ b/packages/cli/src/commands/assets/scope.test.ts @@ -30,6 +30,16 @@ const preconditions = { ), ); }, + forbidsLibraryDiscoveryForAnotherReason() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This token is restricted to specific spaces" }, + { status: 403 }, + ), + ), + ); + }, }; describe("listLibrariesOrDegrade", () => { @@ -44,6 +54,12 @@ describe("listLibrariesOrDegrade", () => { await expect(listLibrariesOrDegrade("12345")).resolves.toEqual([]); }); + + it("should still fail loudly for a different 403 from the same endpoint", async () => { + preconditions.forbidsLibraryDiscoveryForAnotherReason(); + + await expect(listLibrariesOrDegrade("12345")).rejects.toThrow(); + }); }); describe("resolveScopeBaseDir", () => { diff --git a/packages/cli/src/commands/assets/scope.ts b/packages/cli/src/commands/assets/scope.ts index 8566991de..5e8a370f5 100644 --- a/packages/cli/src/commands/assets/scope.ts +++ b/packages/cli/src/commands/assets/scope.ts @@ -2,6 +2,7 @@ import { join } from "pathe"; import { directories } from "../../constants"; import { getMapiClient } from "../../api"; import { APIError, handleAPIError } from "../../utils/error/api-error"; +import { isUnsupportedTokenTypeServerError } from "../../utils/error/credential-hint"; import { toError } from "../../utils/error/error"; import { resolveCommandPath } from "../../utils/filesystem"; import type { SharedAssetFolder } from "./types"; @@ -78,7 +79,11 @@ export async function listLibrariesOrDegrade(spaceId: string): Promise open(url)); const ui = getUI(); - let client; + let client: OAuthClientCredentials; try { client = resolveOAuthClient(); } catch (error) { + const message = error instanceof Error ? error.message : String(error); throw new CommandError( - `${(error as Error).message} Log in with a Personal Access Token (\`storyblok login --token \`), or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app.`, + `${message} Log in with a Personal Access Token (\`storyblok login --token \`), or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app.`, ); } const scopes = OAUTH_LOGIN_SCOPES; diff --git a/packages/cli/src/commands/oauth/refresh.ts b/packages/cli/src/commands/oauth/refresh.ts index 600fb82cf..9181baf6a 100644 --- a/packages/cli/src/commands/oauth/refresh.ts +++ b/packages/cli/src/commands/oauth/refresh.ts @@ -2,7 +2,7 @@ import type { RegionCode } from "../../constants"; import { CommandError } from "../../utils"; import { resolveOAuthClient } from "./client"; import { getOAuthEntry, updateOAuthEntry } from "./store"; -import type { OAuthTokens } from "./store"; +import type { OAuthClientCredentials, OAuthTokens } from "./store"; import { exchangeToken } from "./token-endpoint"; export const computeExpiresAt = (expiresInSeconds: number, nowMs: number = Date.now()): string => { @@ -20,12 +20,13 @@ const doRefresh = async (region: RegionCode): Promise => { throw new CommandError("No OAuth refresh token stored. Run `storyblok login` to authenticate."); } - let client; + let client: OAuthClientCredentials; try { client = resolveOAuthClient(); } catch (error) { + const message = error instanceof Error ? error.message : String(error); throw new CommandError( - `Your OAuth session cannot be refreshed: ${(error as Error).message} Log in with a Personal Access Token (\`storyblok login --token \`).`, + `Your OAuth session cannot be refreshed: ${message} Log in with a Personal Access Token (\`storyblok login --token \`).`, ); } diff --git a/packages/cli/src/commands/stories/push/README.md b/packages/cli/src/commands/stories/push/README.md index 418f28eeb..0adeb2653 100644 --- a/packages/cli/src/commands/stories/push/README.md +++ b/packages/cli/src/commands/stories/push/README.md @@ -31,3 +31,6 @@ storyblok stories push --space YOUR_SPACE_ID - Component schemas are required to map story references correctly. Always run `storyblok components pull` to fetch the latest schemas before running `storyblok stories push`. +- The command exits with code 1 if any story fails to push, even if most stories succeed. Check the + exit code in CI pipelines and scripts that run `storyblok stories push`, since a partial failure + no longer exits 0. diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index bd71a64c7..15a551ee7 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -214,7 +214,10 @@ describe("stories push command", () => { resetReporter(); process.exitCode = undefined; }); - afterAll(() => server.close()); + afterAll(() => { + server.close(); + process.exitCode = undefined; + }); describe("first-time push", () => { it("should push stories with mapped references", async () => { @@ -2067,7 +2070,7 @@ describe("stories push command", () => { it("should not report stories as pushed when every update fails", async () => { // Regression for a live run measured with `stories:write` missing: // the headline said "10 stories pushed" while nothing was written. - // Uses a 422 (not 403) so this stays independent of the credential + // Uses a 500 (not 403) so this stays independent of the credential // short-circuit path covered above. const storyA = makeMockStory({ slug: "story-a" }); const storyB = makeMockStory({ slug: "story-b" }); diff --git a/packages/cli/src/commands/user/actions.test.ts b/packages/cli/src/commands/user/actions.test.ts index 32ada0a72..0cc85041f 100644 --- a/packages/cli/src/commands/user/actions.test.ts +++ b/packages/cli/src/commands/user/actions.test.ts @@ -64,6 +64,14 @@ describe("getUser credential errors", () => { ); }); + it("should use the centralized message for a PAT session", async () => { + setCredentialContext({ kind: "pat" }); + + await expect(getUser({ personalAccessToken: "sb_pat_dead" }, "eu")).rejects.toThrow( + "Your personal access token was rejected", + ); + }); + it("should keep the masked-token message while validating a token at login", async () => { // No session yet: the context is still unknown, so the matcher stays inactive. await expect(getUser("sb_pat_invalid", "eu")).rejects.toThrow("is invalid"); diff --git a/packages/cli/src/utils/error/api-error.test.ts b/packages/cli/src/utils/error/api-error.test.ts index 357925c41..280d9727a 100644 --- a/packages/cli/src/utils/error/api-error.test.ts +++ b/packages/cli/src/utils/error/api-error.test.ts @@ -491,6 +491,23 @@ describe("APIError credential rewrites", () => { } }); + it("should let a customMessage suppress the credential rewrite entirely", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error, "Custom override message"); + } catch (e) { + const apiError = e as APIError; + expect(apiError.message).toBe("Custom override message"); + expect(apiError.fatal).toBe(false); + } + }); + it("should not mark unrelated errors as fatal", () => { setCredentialContext({ kind: "oauth" }); const error = new FetchError("Unprocessable", { diff --git a/packages/cli/src/utils/error/api-error.ts b/packages/cli/src/utils/error/api-error.ts index 9f500c77e..83869895c 100644 --- a/packages/cli/src/utils/error/api-error.ts +++ b/packages/cli/src/utils/error/api-error.ts @@ -188,6 +188,13 @@ export class APIError extends Error { response: FetchError["response"] | undefined; /** True when the failure is credential-level, so bulk loops should stop instead of retrying. */ fatal: boolean; + /** + * The raw `data.error`/`data.message` string extracted from the response, before any + * rewrite. Undefined when a `customMessage` suppressed extraction, or none was present. + * Callers that need to distinguish specific server signatures (e.g. the unsupported-token-type + * 403) beyond the generic `errorId`/`fatal` classification should match on this. + */ + serverError: string | undefined; constructor( errorId: keyof typeof API_ERRORS, action: keyof typeof API_ACTIONS, @@ -203,6 +210,7 @@ export class APIError extends Error { this.error = error; this.response = error?.response; this.fatal = false; + this.serverError = undefined; if (!customMessage) { this.messageStack.push(API_ACTIONS[action]); @@ -215,6 +223,7 @@ export class APIError extends Error { const serverMessage = customMessage ? undefined : extractServerString(responseData ?? {}, this.code, statusText); + this.serverError = serverMessage; const stackLengthBefore422 = this.messageStack.length; diff --git a/packages/cli/src/utils/error/credential-hint.test.ts b/packages/cli/src/utils/error/credential-hint.test.ts index 3fdf5fe14..f449717a6 100644 --- a/packages/cli/src/utils/error/credential-hint.test.ts +++ b/packages/cli/src/utils/error/credential-hint.test.ts @@ -79,6 +79,31 @@ describe("matchCredentialError", () => { expect(hint?.message).toBe(formatSpaceNotAllowedMessage(999, [1, 2])); }); + it("should name the personal access token and a remedy it can act on for a space-restricted PAT", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "pat", + space: 222, + spaces: [], + }); + + expect(hint?.message).toBe( + "Space 222 is not covered by your personal access token. Create a new token that covers this space under My account, Personal access tokens.", + ); + }); + + it("should omit the empty parenthetical for an OAuth grant with no known space list", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "oauth", + space: 222, + spaces: [], + }); + + expect(hint?.message).toBe( + "Space 222 is not covered by your OAuth login. Re-run `storyblok login` and select this space at the consent screen.", + ); + expect(hint?.message).not.toContain("(authorized spaces: )"); + }); + it("should treat a 401 as a dead session regardless of the body", () => { expect(matchCredentialError(401, "Unauthorized", oauth)?.message).toBe( "Your OAuth login is no longer valid, it may have been revoked or expired. Run `storyblok login` to sign in again.", diff --git a/packages/cli/src/utils/error/credential-hint.ts b/packages/cli/src/utils/error/credential-hint.ts index a25cac4b5..a37ffd373 100644 --- a/packages/cli/src/utils/error/credential-hint.ts +++ b/packages/cli/src/utils/error/credential-hint.ts @@ -13,20 +13,45 @@ const UNSUPPORTED_TOKEN_TYPE = "This endpoint does not support this token type"; const SPACE_RESTRICTED = "This token is restricted to specific spaces"; const SPACE_NOT_ALLOWED = "This token does not have access to this space"; +function formatAuthorizedSpacesParenthetical(grantedSpaceIds: number[]): string { + return grantedSpaceIds.length > 0 ? ` (authorized spaces: ${grantedSpaceIds.join(", ")})` : ""; +} + /** * Shared by the `oauth/space-guard.ts` pre-flight check and the post-flight 403 branch, - * so a user sees identical wording whichever one catches the problem first. + * so a user sees identical wording whichever one catches the problem first. OAuth only: + * the backend's `enforce_pat_space_restriction` check runs for any scoped credential, but + * the pre-flight guard this mirrors only ever runs for an OAuth grant (see `program.ts`). */ export function formatSpaceNotAllowedMessage( space: string | number, grantedSpaceIds: number[], ): string { return ( - `Space ${space} is not covered by your OAuth login (authorized spaces: ${grantedSpaceIds.join(", ")}). ` + + `Space ${space} is not covered by your OAuth login${formatAuthorizedSpacesParenthetical(grantedSpaceIds)}. ` + `Re-run \`storyblok login\` and select this space at the consent screen.` ); } +function formatPatSpaceNotAllowedMessage( + space: string | number, + grantedSpaceIds: number[], +): string { + return ( + `Space ${space} is not covered by your personal access token${formatAuthorizedSpacesParenthetical(grantedSpaceIds)}. ` + + `Create a new token that covers this space under My account, Personal access tokens.` + ); +} + +/** + * Identifies the credential-does-not-support-this-endpoint signature, distinct from other + * `forbidden`-classified cases (space restriction) that must still fail loudly rather than + * degrade gracefully. See `assets/scope.ts#listLibrariesOrDegrade`. + */ +export function isUnsupportedTokenTypeServerError(serverError: string | undefined): boolean { + return serverError === UNSUPPORTED_TOKEN_TYPE; +} + function credentialLabel(kind: CredentialContext["kind"]): string { return kind === "oauth" ? "Your OAuth login" : "Your personal access token"; } @@ -91,7 +116,9 @@ export function matchCredentialError( return { errorId: "forbidden", fatal: true, - message: formatSpaceNotAllowedMessage(context.space ?? "unknown", ids), + message: isOAuth + ? formatSpaceNotAllowedMessage(context.space ?? "unknown", ids) + : formatPatSpaceNotAllowedMessage(context.space ?? "unknown", ids), }; } From 21d42849c071ff187e1ffb00dc21002aceca1833 Mon Sep 17 00:00:00 2001 From: Markus Oberlehner Date: Thu, 13 Aug 2026 12:29:20 +0200 Subject: [PATCH 15/15] fix(cli): address manual QA findings on scoped-credential handling Manual QA against a real OAuth grant surfaced five issues: - Shared-folder discovery reported "Failed to pull library folders" on the push path. The lookup serves both pull and push, so the action is now verb-neutral: "Failed to list library folders". - A fatal credential failure stops a phase mid-run, but the summary still printed the full denominator ("0/10 succeeded, 1 failed"), leaving nine stories silently unaccounted for. Phase lines now name them explicitly. - `stories push` exited 0 when it bailed on a precondition (no local components, schema issues), so CI read an aborted push as a clean run. Both now go through `handleError`, matching the sibling --space check. - `login --oauth` opened a browser before discovering the callback port was taken, sending the user through a consent screen whose redirect it could never receive. `waitForCallback` becomes `startCallbackServer`, which resolves only once bound, so a conflict fails before the tab opens. - test/GUIDE.md claimed a port conflict hangs for the full callback timeout, which is no longer true, and lacked the headless consent procedure that makes the whole OAuth flow testable without a browser. Fixes DX-486 --- packages/cli/src/commands/assets/actions.ts | 2 +- packages/cli/src/commands/assets/scope.ts | 4 +- packages/cli/src/commands/login/oauth.test.ts | 5 +- .../cli/src/commands/oauth/login-flow.test.ts | 21 +++++++- packages/cli/src/commands/oauth/login-flow.ts | 17 ++++-- packages/cli/src/commands/oauth/pkce.test.ts | 4 +- .../cli/src/commands/oauth/server.test.ts | 33 ++++++++---- packages/cli/src/commands/oauth/server.ts | 54 +++++++++++++++---- .../src/commands/stories/push/index.test.ts | 36 +++++++++++++ .../cli/src/commands/stories/push/index.ts | 33 ++++++++---- packages/cli/src/utils/error/api-error.ts | 4 +- packages/cli/test/GUIDE.md | 33 +++++++++++- 12 files changed, 200 insertions(+), 46 deletions(-) diff --git a/packages/cli/src/commands/assets/actions.ts b/packages/cli/src/commands/assets/actions.ts index 4aa05f894..23b765174 100644 --- a/packages/cli/src/commands/assets/actions.ts +++ b/packages/cli/src/commands/assets/actions.ts @@ -512,7 +512,7 @@ export const fetchSharedAssetFolders = async ({ } return { asset_folders: all.filter((folder) => keep.has(folder.id)) }; } catch (maybeError) { - handleAPIError("pull_shared_asset_folders", toError(maybeError)); + handleAPIError("list_shared_asset_folders", toError(maybeError)); } }; diff --git a/packages/cli/src/commands/assets/scope.ts b/packages/cli/src/commands/assets/scope.ts index 5e8a370f5..53e4d9c23 100644 --- a/packages/cli/src/commands/assets/scope.ts +++ b/packages/cli/src/commands/assets/scope.ts @@ -56,7 +56,7 @@ export async function listLibraries(spaceId: string): Promise { ?.access_level ?? "read", })); } catch (maybeError) { - handleAPIError("pull_shared_asset_folders", toError(maybeError)); + handleAPIError("list_shared_asset_folders", toError(maybeError)); } } @@ -113,7 +113,7 @@ export async function buildLibraryRootResolver( }); folders = data?.shared_asset_folders ?? []; } catch (maybeError) { - handleAPIError("pull_shared_asset_folders", toError(maybeError)); + handleAPIError("list_shared_asset_folders", toError(maybeError)); } const parentById = new Map(); for (const folder of folders) { diff --git a/packages/cli/src/commands/login/oauth.test.ts b/packages/cli/src/commands/login/oauth.test.ts index 1d891842a..28100438d 100644 --- a/packages/cli/src/commands/login/oauth.test.ts +++ b/packages/cli/src/commands/login/oauth.test.ts @@ -14,7 +14,10 @@ vi.mock("node:fs/promises"); // Avoid opening a real browser and a real socket in tests. vi.mock("open", () => ({ default: vi.fn(async () => undefined) })); vi.mock("../oauth/server", () => ({ - waitForCallback: vi.fn(async () => ({ code: "auth-code", state: "ignored" })), + startCallbackServer: vi.fn(async () => ({ + callback: Promise.resolve({ code: "auth-code", state: "ignored" }), + close: vi.fn(), + })), })); // Force the state check to pass by returning the same state generatePkce/generateState produced. vi.mock("../oauth/pkce", async (importOriginal) => { diff --git a/packages/cli/src/commands/oauth/login-flow.test.ts b/packages/cli/src/commands/oauth/login-flow.test.ts index c8281ca5a..a17126dee 100644 --- a/packages/cli/src/commands/oauth/login-flow.test.ts +++ b/packages/cli/src/commands/oauth/login-flow.test.ts @@ -15,7 +15,10 @@ vi.mock("./pkce", () => ({ generateState: () => "state-abc", })); vi.mock("./server", () => ({ - waitForCallback: vi.fn(async () => ({ code: "auth-code", state: "state-abc" })), + startCallbackServer: vi.fn(async () => ({ + callback: Promise.resolve({ code: "auth-code", state: "state-abc" }), + close: vi.fn(), + })), })); vi.mock("./token-endpoint", () => ({ exchangeToken: vi.fn(async () => ({ access_token: "at", refresh_token: "rt", expires_in: 900 })), @@ -24,6 +27,7 @@ vi.mock("./grant", () => ({ introspectGrant: vi.fn() })); const { introspectGrant } = await import("./grant"); const { resolveOAuthClient } = await import("./client"); +const { startCallbackServer } = await import("./server"); describe("buildAuthorizeUrl", () => { it("should build an /oauth/init URL with PKCE and space-safe params", () => { @@ -94,6 +98,21 @@ describe("performOAuthLogin", () => { expect(params.get("scope")).toBe(OAUTH_LOGIN_SCOPES.join(" ")); }); + it("should not open a browser when the callback port cannot be bound", async () => { + vi.mocked(startCallbackServer).mockRejectedValueOnce( + new Error("Port 4900 is already in use by nc (PID 1)"), + ); + const openBrowser = vi.fn(async () => {}); + + await expect(performOAuthLogin({ region: "eu", openBrowser })).rejects.toThrow( + "already in use", + ); + + // Sending a user through consent whose redirect can never be received is worse than + // failing before the tab opens. + expect(openBrowser).not.toHaveBeenCalled(); + }); + it("should not persist tokens when introspection fails", async () => { vi.mocked(introspectGrant).mockRejectedValueOnce(new Error("introspection failed")); diff --git a/packages/cli/src/commands/oauth/login-flow.ts b/packages/cli/src/commands/oauth/login-flow.ts index 7488c84dc..d7ce06dfb 100644 --- a/packages/cli/src/commands/oauth/login-flow.ts +++ b/packages/cli/src/commands/oauth/login-flow.ts @@ -13,7 +13,7 @@ import { import { introspectGrant } from "./grant"; import { generatePkce, generateState } from "./pkce"; import { computeExpiresAt } from "./refresh"; -import { waitForCallback } from "./server"; +import { startCallbackServer } from "./server"; import { setOAuthActiveRegion, updateOAuthEntry } from "./store"; import type { OAuthClientCredentials, OAuthGrantSpace, OAuthTokens } from "./store"; import { exchangeToken } from "./token-endpoint"; @@ -64,8 +64,10 @@ export const performOAuthLogin = async (options: { const { verifier, challenge } = generatePkce(); const state = generateState(); - // Start listening before opening the browser so no callback is missed. - const callbackPromise = waitForCallback(OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH); + // Bind the callback port before opening the browser: no callback can be missed, and a port + // conflict fails here rather than after sending the user through a consent screen whose + // redirect the CLI could never have received. + const listener = await startCallbackServer(OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH); const authorizeUrl = buildAuthorizeUrl({ region, @@ -77,9 +79,14 @@ export const performOAuthLogin = async (options: { ui.info( `Opening your browser to authorize the Storyblok CLI.\nIf it does not open, visit:\n${authorizeUrl}`, ); - await openBrowser(authorizeUrl); + try { + await openBrowser(authorizeUrl); + } catch (error) { + listener.close(); + throw error; + } - const { code, state: returnedState } = await callbackPromise; + const { code, state: returnedState } = await listener.callback; if (returnedState !== state) { throw new CommandError( "OAuth state mismatch; aborting for your safety. Please try `storyblok login` again.", diff --git a/packages/cli/src/commands/oauth/pkce.test.ts b/packages/cli/src/commands/oauth/pkce.test.ts index 586142a73..fc34cb7f5 100644 --- a/packages/cli/src/commands/oauth/pkce.test.ts +++ b/packages/cli/src/commands/oauth/pkce.test.ts @@ -7,13 +7,13 @@ describe("generatePkce", () => { expect(verifier.length).toBeGreaterThanOrEqual(43); expect(verifier.length).toBeLessThanOrEqual(128); expect(verifier).toMatch(/^[\w\-.~]+$/); - expect(challenge).toMatch(/^[\w\-]+$/); + expect(challenge).toMatch(/^[\w-]+$/); expect(challenge).not.toBe(verifier); }); }); describe("generateState", () => { it("should produce a non-empty url-safe string", () => { - expect(generateState()).toMatch(/^[\w\-]+$/); + expect(generateState()).toMatch(/^[\w-]+$/); }); }); diff --git a/packages/cli/src/commands/oauth/server.test.ts b/packages/cli/src/commands/oauth/server.test.ts index 9ee176726..8b7830e7e 100644 --- a/packages/cli/src/commands/oauth/server.test.ts +++ b/packages/cli/src/commands/oauth/server.test.ts @@ -1,37 +1,40 @@ import { createServer } from "node:http"; import { describe, expect, it } from "vitest"; -import { waitForCallback } from "./server"; +import { startCallbackServer } from "./server"; const PATH = "/oauth/callback"; const callback = (port: number, query: string) => fetch(`http://127.0.0.1:${port}${PATH}${query}`); -describe("waitForCallback", () => { +describe("startCallbackServer", () => { it("should resolve with code and state and serve a 200 success page", async () => { - const pending = waitForCallback(4917, PATH); + const listener = await startCallbackServer(4917, PATH); const response = await callback(4917, "?code=auth-code&state=state-abc"); expect(response.status).toBe(200); - await expect(pending).resolves.toEqual({ code: "auth-code", state: "state-abc" }); + await expect(listener.callback).resolves.toEqual({ code: "auth-code", state: "state-abc" }); }); it("should serve a non-200 page and reject when the callback carries an error", async () => { + const listener = await startCallbackServer(4918, PATH); // Attach the rejection assertion before triggering the callback so the // rejection is handled the moment it settles. - const rejected = expect(waitForCallback(4918, PATH)).rejects.toThrow(/access_denied/); + const rejected = expect(listener.callback).rejects.toThrow(/access_denied/); const response = await callback(4918, "?error=access_denied&error_description=denied"); expect(response.status).toBe(400); await rejected; }); - it("should explain which process blocks the callback port when it is already in use", async () => { + it("should reject before listening when the callback port is already in use", async () => { const blocker = createServer(); await new Promise((resolve) => blocker.listen(4920, "127.0.0.1", resolve)); try { - // The holder lookup shells out to lsof/netstat, which may be unavailable in CI, so the - // assertion covers the always-present parts: the port, the cause, and the way out. - await expect(waitForCallback(4920, PATH)).rejects.toThrow( + // Rejecting the start promise (rather than the callback promise) is what lets the login + // flow report the conflict before it opens a browser tab. The holder lookup shells out to + // lsof/netstat, which may be unavailable in CI, so the assertion covers the always-present + // parts: the port, the cause, and the way out. + await expect(startCallbackServer(4920, PATH)).rejects.toThrow( /Port 4920 is already in use .*run `storyblok login --oauth` again/s, ); } finally { @@ -40,10 +43,20 @@ describe("waitForCallback", () => { }); it("should serve a non-200 page and reject when code or state is missing", async () => { - const rejected = expect(waitForCallback(4919, PATH)).rejects.toThrow(/code and state/); + const listener = await startCallbackServer(4919, PATH); + const rejected = expect(listener.callback).rejects.toThrow(/code and state/); const response = await callback(4919, "?code=only-code"); expect(response.status).toBe(400); await rejected; }); + + it("should stop listening once closed so the port is free again", async () => { + const listener = await startCallbackServer(4921, PATH); + listener.close(); + + // A second bind on the same port only succeeds if the first server really let it go. + const reopened = await startCallbackServer(4921, PATH); + reopened.close(); + }); }); diff --git a/packages/cli/src/commands/oauth/server.ts b/packages/cli/src/commands/oauth/server.ts index 45cd7b8cb..a1300f4d3 100644 --- a/packages/cli/src/commands/oauth/server.ts +++ b/packages/cli/src/commands/oauth/server.ts @@ -16,13 +16,33 @@ const ERROR_PAGE = page( "Authorization failed. You can close this tab and return to the terminal.", ); -export const waitForCallback = ( +export interface CallbackListener { + /** Settles when the browser hits the redirect URI, or the wait times out. */ + callback: Promise<{ code: string; state: string }>; + /** Stops listening. Safe to call after `callback` has already settled. */ + close: () => void; +} + +/** + * Binds the loopback callback server and resolves only once it is actually listening, so a + * caller can confirm the port is free *before* sending the user to the consent screen. A bind + * failure (typically `EADDRINUSE`) rejects this promise rather than the callback promise — + * otherwise a doomed run would still open a browser tab it can never collect a code from. + */ +export const startCallbackServer = ( port: number, path: string, timeoutMs = 300_000, -): Promise<{ code: string; state: string }> => { - return new Promise((resolve, reject) => { +): Promise => { + return new Promise((ready, failToStart) => { + let listening = false; let timer: NodeJS.Timeout; + let settleCallback: (result: { code: string; state: string }) => void; + let failCallback: (error: Error) => void; + const callback = new Promise<{ code: string; state: string }>((resolve, reject) => { + settleCallback = resolve; + failCallback = reject; + }); const server = createServer((req, res) => { const url = new URL(req.url ?? "/", `http://localhost:${port}`); @@ -38,7 +58,7 @@ export const waitForCallback = ( const fail = (error: CommandError): void => { res.writeHead(400, { "Content-Type": "text/html" }); res.end(ERROR_PAGE); - reject(error); + failCallback(error); }; const error = url.searchParams.get("error"); @@ -59,17 +79,12 @@ export const waitForCallback = ( res.writeHead(200, { "Content-Type": "text/html" }); res.end(SUCCESS_PAGE); - resolve({ code, state }); + settleCallback({ code, state }); }); - timer = setTimeout(() => { - server.close(); - reject(new CommandError("Timed out waiting for the browser authorization callback.")); - }, timeoutMs); - timer.unref?.(); - server.on("error", (err) => { clearTimeout(timer); + const reject = listening ? failCallback : failToStart; if ((err as NodeJS.ErrnoException).code === "EADDRINUSE") { describePortConflict(port).then( (message) => reject(new CommandError(message)), @@ -79,6 +94,23 @@ export const waitForCallback = ( } reject(err); }); + + server.on("listening", () => { + listening = true; + timer = setTimeout(() => { + server.close(); + failCallback(new CommandError("Timed out waiting for the browser authorization callback.")); + }, timeoutMs); + timer.unref?.(); + ready({ + callback, + close: () => { + clearTimeout(timer); + server.close(); + }, + }); + }); + // Bind to loopback only so the authorization code is never accepted from other hosts. server.listen(port, "127.0.0.1"); }); diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index 15a551ee7..bffaba92f 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -1436,6 +1436,8 @@ describe("stories push command", () => { ); expect(actions.createStory).not.toHaveBeenCalled(); expect(actions.updateStory).not.toHaveBeenCalled(); + // Bailing on a precondition is a failed push; CI must not read it as a clean run. + expect(process.exitCode).toBe(2); }); it("should abort with a hard error when a story references a missing component schema", async () => { @@ -1461,6 +1463,7 @@ describe("stories push command", () => { ); const report = getReport(); expect(report?.status).toBe("FAILURE"); + expect(process.exitCode).toBe(2); const logFile = getLogFileContents(LOG_PREFIX); expect(logFile).toContain("Schema validation failed"); }); @@ -2153,5 +2156,38 @@ describe("stories push command", () => { expect(requestCount.current).toBeLessThan(localStories.length / 2); expect(process.exitCode).toBe(1); }); + + it("should name the stories an aborted run never attempted", async () => { + const localStories = Array.from({ length: 5 }, (_, i) => + makeMockStory({ slug: `story-${i}` }), + ); + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Without the "not attempted" tail this reads as "0/5 succeeded, 1 failed", leaving + // four stories silently unaccounted for and the abort looking like data loss. + const errorCalls = (console.error as unknown as ReturnType).mock.calls; + const rendered = errorCalls.map((call) => call[0]).join("\n"); + expect(rendered).toContain("Updating stories: 0/5 succeeded, 1 failed, 4 not attempted."); + }); + + it("should not add a not-attempted tail when every story is accounted for", async () => { + const localStories = [makeMockStory({ slug: "story-a" })]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.canUpdateStories(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Updating stories: 1/1 succeeded, 0 failed."), + ); + expect(console.error).not.toHaveBeenCalledWith(expect.stringContaining("not attempted")); + }); }); }); diff --git a/packages/cli/src/commands/stories/push/index.ts b/packages/cli/src/commands/stories/push/index.ts index 5cf8511a8..62cc1b94b 100644 --- a/packages/cli/src/commands/stories/push/index.ts +++ b/packages/cli/src/commands/stories/push/index.ts @@ -32,6 +32,16 @@ import { prefetchTargetStoriesByKeys } from "../actions"; import { collectSchemaIssues, formatSchemaIssues, hasSchemaIssues } from "../validate-story"; import { FailureCollector } from "./failure-report"; +/** + * A fatal credential failure stops a phase mid-run, leaving stories the CLI deliberately + * never tried. Spelling those out keeps every phase line adding up to its total instead of + * leaving a silent shortfall between "succeeded" and the denominator. + */ +function formatNotAttempted(total: number, ...accounted: number[]): string { + const remaining = total - accounted.reduce((sum, count) => sum + count, 0); + return remaining > 0 ? `, ${remaining} not attempted` : ""; +} + const pushCmd = storiesCommand .command("push") .option("-s, --space ", "space ID") @@ -99,10 +109,14 @@ pushCmd.action(async (options, command) => { resolveCommandPath(directories.components, fromSpace, basePath), ); if (Object.keys(schemas).length === 0) { - const message = - "No components found. Please run `storyblok components pull` to fetch the latest components."; - ui.error(message); - logger.error(message); + // `handleError` rather than a bare `ui.error`: bailing on a precondition is still a + // failed push, and CI must see a non-zero exit for it. + handleError( + new CommandError( + "No components found. Please run `storyblok components pull` to fetch the latest components.", + ), + verbose, + ); return; } @@ -116,9 +130,8 @@ pushCmd.action(async (options, command) => { schemas, }); if (hasSchemaIssues(schemaIssues)) { - const message = formatSchemaIssues(schemaIssues); - ui.error(message); - logger.error(message); + // See the components precondition above: this aborts the push, so it must exit non-zero. + handleError(new CommandError(formatSchemaIssues(schemaIssues)), verbose); // Surface the failure in the run summary so the report status is // FAILURE rather than a trivial zero-counts SUCCESS. const total = Math.max(schemaIssues.total, 1); @@ -446,9 +459,9 @@ pushCmd.action(async (options, command) => { `Push results: ${pushedCount} ${pushedCount === 1 ? "story" : "stories"} pushed, ${failedCount} ${failedCount === 1 ? "story" : "stories"} failed`, ); ui.list([ - `Creating stories: ${summary.creationResults.succeeded}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed, ${summary.creationResults.skipped} skipped.`, - `Processing stories: ${summary.processResults.succeeded}/${summary.processResults.total} succeeded, ${summary.processResults.failed} failed.`, - `Updating stories: ${summary.updateResults.succeeded}/${summary.updateResults.total} succeeded, ${summary.updateResults.failed} failed.`, + `Creating stories: ${summary.creationResults.succeeded}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed, ${summary.creationResults.skipped} skipped${formatNotAttempted(summary.creationResults.total, summary.creationResults.succeeded, summary.creationResults.failed, summary.creationResults.skipped)}.`, + `Processing stories: ${summary.processResults.succeeded}/${summary.processResults.total} succeeded, ${summary.processResults.failed} failed${formatNotAttempted(summary.processResults.total, summary.processResults.succeeded, summary.processResults.failed)}.`, + `Updating stories: ${summary.updateResults.succeeded}/${summary.updateResults.total} succeeded, ${summary.updateResults.failed} failed${formatNotAttempted(summary.updateResults.total, summary.updateResults.succeeded, summary.updateResults.failed)}.`, ]); if (pendingWarnings.length > 0 || !failures.isEmpty) { diff --git a/packages/cli/src/utils/error/api-error.ts b/packages/cli/src/utils/error/api-error.ts index 83869895c..b5bd74004 100644 --- a/packages/cli/src/utils/error/api-error.ts +++ b/packages/cli/src/utils/error/api-error.ts @@ -42,7 +42,9 @@ export const API_ACTIONS = { transfer_asset: "Failed to transfer asset", pull_shared_assets: "Failed to pull library assets", pull_shared_asset: "Failed to pull library asset", - pull_shared_asset_folders: "Failed to pull library folders", + // Folder discovery runs on both the pull and the push path, so this stays verb-neutral: + // a push must not report a failure to "pull" anything. + list_shared_asset_folders: "Failed to list library folders", pull_shared_asset_folder: "Failed to pull library folder", pull_shared_internal_tags: "Failed to pull library tags", push_shared_asset_create: "Failed to create library asset", diff --git a/packages/cli/test/GUIDE.md b/packages/cli/test/GUIDE.md index ecd5d9f70..298a92403 100644 --- a/packages/cli/test/GUIDE.md +++ b/packages/cli/test/GUIDE.md @@ -76,14 +76,43 @@ it, and `logout` revokes the grant server-side. `PATH` swallows the launch. The authorize URL is also printed to stderr, so you can paste it into a browser yourself to finish consent. - **Occupy port 4900 with `nc -l 127.0.0.1 4900`, not `nc -l 4900`.** The wildcard bind does not - conflict with the CLI's loopback bind under `SO_REUSEADDR`, so the CLI starts normally and hangs - for the full 5 minute callback timeout instead of reporting the conflict. + conflict with the CLI's loopback bind under `SO_REUSEADDR`, so the CLI never notices it. Against a + loopback bind the CLI fails before it opens a browser, naming the blocking process and its PID. Worth checking manually: port 4900 occupied, denied consent, out-of-grant space (restrict `permitted_space_ids` via `PUT /v1/oauth_clients/`), and refresh (set `expires_at` to the past, then run any command). Manage apps through `/v1/oauth_clients` with a Personal Access Token, where `client_id` is `oauth_identifier` and `client_secret` is `oauth_secret`. +### Headless consent + +Consent needs a logged-in `app.storyblok.com` session, but that session can be minted from a +Personal Access Token instead of a browser, which makes the whole flow scriptable: + +1. `POST /oauth/init` with `Authorization: ` sets `session[:user_id]` and returns a + `_storyrails_session` cookie. Keep it in a cookie jar. +2. `GET /oauth/authorize?` with that cookie renders the consent page. Take the query string + from the authorize URL the CLI prints to stderr, and swap `/oauth/init` for `/oauth/authorize` + (`/oauth/init` only bounces to the SPA, which primes the session step 1 already did). +3. Parse the page for `authenticity_token`, the hidden OAuth params, the `scope[]` values, and the + `space_ids[]` checkbox values. `POST /oauth/authorize` with those plus `approve=true`, or + `deny=true` to test a refused grant. +4. Follow the `Location` header, which points at `http://localhost:4900/oauth/callback`, to hand the + code to the waiting CLI. + +Drop scopes or space IDs from that POST to produce a genuinely narrow grant, which is the only way +to exercise the scoped-credential errors against the real API: + +- **Space restriction.** Post a single `space_ids[]`. Any other space then trips the pre-flight + guard in `program.ts`, and the API answers + `403 {"error":"This token does not have access to this space"}`. +- **Insufficient scope.** Dropping `stories:write` alone is not enough: `covers_scope?` in + `token_scopeable.rb` treats `publish` as implying `write`, and `write` as implying `read`. Drop + `stories:write` **and** `stories:publish` to get + `403 {"error":"Insufficient scope: stories:write is required"}`. + +Combine this with the browser stub above to run the entire login end to end with no browser at all. + ## Shared asset libraries A shared asset library is a top-level shared asset folder owned by the organization, with per-space