diff --git a/.changeset/fix-app-submit-missing-fields.md b/.changeset/fix-app-submit-missing-fields.md new file mode 100644 index 00000000..a6d354c0 --- /dev/null +++ b/.changeset/fix-app-submit-missing-fields.md @@ -0,0 +1,27 @@ +--- +'@getbrevo/cli': minor +--- + +feat(public apps): public app distribution and the review lifecycle are now GA (BEX-405) + +`brevo app create --distribution public` is accepted, `Public` is selectable in the interactive distribution prompt, and `brevo app submit`, `brevo app status` and `brevo app withdraw` ship in the published package. They were built but eliminated from published builds at compile time; that gate is gone and the build now asserts the three commands are present in every artifact. + +- **`brevo app status`** — an app's review lifecycle state (`draft`, `submitted`, `in_review`, `approved`, `rejected`, `changes_requested`, or `unknown`) with a human message. Read-only; `--json` gives `{ state, message }`. +- **`brevo app submit`** — opens the public-app review submission form. Requires `distribution_type: public`, an uploaded app, and a local `app-config.json` that matches the server (shown as a field-by-field diff with `(local only)` / `(server only)` tags on drift). `--json` prints `{"app_id","form_url"}` on stdout with the next-steps notes on stderr. The app is submitted only once the form itself is completed — the command changes nothing server-side. +- **`brevo app withdraw`** — withdraws an app from submission (`--force`, `--json`). An app that was never submitted prints a hint and exits `0`. + +All three resolve the target app from `--app-id`, the linked `app-config.json`, or an interactive picker. + +The scaffolded OAuth flow now branches on distribution: a **public** app gets Authorization Code + PKCE (RFC 7636) — `/auth/login` generates a `code_verifier` and sends `code_challenge` + `code_challenge_method=S256`, and the token exchange and refresh send the verifier with no `client_secret`, so the generated `.env.local` / `.env.example` carry none. **Private** apps keep the confidential-client flow unchanged. + +Note that Brevo currently refuses public app creation from the CLI at the platform level: `brevo app create --distribution public` sends the request and the API answers `400`, which the CLI reports as *"Public apps can't be created from the CLI yet"* with the server's own message quoted. The CLI-side commands are all in place and will work as soon as the platform allows it. + +`distribution_type` remains immutable after `brevo app create` — pick `private` for apps used exclusively by your own organisation and `public` for apps distributed to end users or marketplace listings. Only a public app can be submitted for review. + +fix(app submit): refuse an app that was never uploaded before reading its review state, naming the real cause. A never-uploaded app has no version for a review state to hang off, and the server's message for that failure listed `name`, `logo_uri`, `scopes` and `redirect_uris` as the fields to fix — all of which could already be correct. + +fix(app submit): skip the redundant app fetch when the app isn't submittable, and show missing required-field names exactly as returned by the API (BEX-454) + +fix(app status): show the status message returned by the API, falling back to the built-in per-state copy when absent (BEX-454) + +docs: `agent-context/SKILL.md` and `agent-context/AGENTS.md` document the publication and review flow — the route from a public create to an approved app, the five refusals `submit` applies in order, the review states, and the fact that a successful `submit` has not yet submitted anything. diff --git a/.github/workflows/smoke-pre-merge.yml b/.github/workflows/smoke-pre-merge.yml index 9a09f148..f5746e06 100644 --- a/.github/workflows/smoke-pre-merge.yml +++ b/.github/workflows/smoke-pre-merge.yml @@ -36,9 +36,12 @@ jobs: # so a suite that has never run headless can prove itself here without # gating a merge, and it runs on every push to main — which always # precedes a release, so the post-merge lane inherits proven ground. - # Selecting 'public' makes the runner build PREVIEW=1, unchanged from when - # this lane ran `private,public`; the published surface is covered by the - # post-merge lane, which installs from npm. + # Selecting 'public' used to make the runner build PREVIEW=1, because the + # review-lifecycle commands existed nowhere else. Since public-apps GA + # (BEX-405) every local build is the published surface, so this lane now + # exercises the review lifecycle against the same artifact npm ships. The + # post-merge lane still installs from npm and stays pinned narrower — see + # its own note. suite: all account: default non_blocking: true diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml index 525bac54..d15d2022 100644 --- a/.github/workflows/smoke.yml +++ b/.github/workflows/smoke.yml @@ -32,9 +32,12 @@ name: Smoke # post-publish dispatch — pins both explicitly, so retuning a dropdown can never # change what a release gate verifies. Widen a gate by editing that lane. # -# The manual default is the LIVE surface: private + ui. Public apps are not live -# (BEX-405), so the preview suite is opt-in and refused unless against=local — -# see the "Reject a preview suite" step, which is the rule, not the comment. +# The manual default is the LIVE surface, which since public-apps GA (BEX-405) +# is every non-interactive suite: private + ui + public. Nothing is gated any +# more, so there is no longer a preview-only suite to refuse — the step that used +# to reject a public-containing suite against a published package is gone with it. +# The release lanes remain pinned at their own narrower sets on purpose; widening +# one is a deliberate edit there, and needs the suite proven headless first. on: workflow_call: inputs: @@ -46,7 +49,7 @@ on: description: 'Comma-separated suites for --suite (blank = the live surface)' required: false type: string - default: 'private,ui' + default: 'private,ui,public' account: description: 'Which test-account secret to authenticate with' required: false @@ -81,28 +84,27 @@ on: # The values ARE the `--suite` argument, so what the dropdown shows is # what the runner receives — no label-to-flag mapping to keep in sync. # - # `private,ui` is the LIVE surface and the default: private apps plus UI - # apps, both GA and both in the published build. - # - # Anything containing `public` is the PREVIEW surface and only means - # something against=local, where the runner builds PREVIEW=1. Against a - # published package those steps could only ever skip — the commands are - # eliminated from the bundle (BEX-405) — so the pairing is refused - # outright rather than reported as a pass with holes in it. + # `private,ui,public` is the LIVE surface and the default: all three app + # types are GA and all three are in the published build. `public` joined + # at public-apps GA (BEX-405) — before that it was the preview surface, + # meaningful only against=local where the runner built PREVIEW=1, and a + # public-containing suite against a published package was refused outright + # rather than reported as a pass with holes in it. Both the preview build + # and that refusal are gone; a public suite now runs anywhere. # # `init` drives the wizard through scripted stdin; `ui` drives create # through a pty. Both work headless, so both can run here. - description: 'Which app types to smoke (--suite). public/all = preview, local only' + description: 'Which app types to smoke (--suite)' type: choice required: true - default: private,ui + default: private,ui,public options: + - private,ui,public - private,ui - private - ui - - init - public - - private,ui,public + - init - all account: # Names a SECRET, never a key. workflow_dispatch inputs are recorded on @@ -159,24 +161,13 @@ jobs: timeout-minutes: 20 steps: - # Fail before checkout: an impossible pairing should cost nothing and say - # why. The preview surface exists only in a PREVIEW=1 build, which only - # `against=local` produces — against a published package the public steps - # would skip and the run would still go green, which reads as coverage the - # run never had. - - name: Reject a preview suite against a published package - if: contains(inputs.suite, 'public') || inputs.suite == 'all' - env: - AGAINST: ${{ inputs.against }} - SUITE: ${{ inputs.suite }} - run: | - set -eu - if [ "$AGAINST" != 'local' ]; then - echo "::error::suite '${SUITE}' includes the public-app (preview) surface, which only exists in a PREVIEW=1 build. Re-run with against=local, or pick a suite without 'public'." - exit 1 - fi - echo "preview suite '${SUITE}' against a local build — ok" - + # No suite/against pairing is impossible any more, so nothing is rejected + # before checkout. There used to be a guard here: the review-lifecycle + # surface existed only in a PREVIEW=1 build, so a public-containing suite + # against a published package would skip every step and still go green — + # coverage the run never had. Public apps went GA (BEX-405) and the guard + # went with it. Re-add one if a suite is ever build-specific again; the + # failure it prevented is a green run, which is the worst kind. - name: Checkout uses: actions/checkout@v7 with: diff --git a/AGENTS.md b/AGENTS.md index 94c8ac7b..a3ff0185 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,21 +6,17 @@ Brevo Developer CLI (`@getbrevo/cli`) — TypeScript CLI for managing OAuth app Public CLI reference: https://developers.brevo.com/docs/cli-reference — the authoritative user-facing command/flag/exit-code documentation. Keep this in sync when changing user-visible behaviour. -## Public app distribution is not GA — the published build omits it (BEX-405) +## Public apps are GA — the whole surface ships (BEX-405) -Public app distribution is **not live on the Brevo platform**. The repo contains the whole surface — `brevo app create --distribution public`, `brevo app submit`, `brevo app status`, `brevo app withdraw` — but a **published build does not**: `scripts/build.mjs` eliminates the review-lifecycle commands from the bundle, and `--distribution public` is refused with a typed `CliError`. +Public app distribution and the review lifecycle are **live**: `brevo app create --distribution public`, `brevo app submit`, `brevo app status` and `brevo app withdraw` all ship in the published build. A build-time gate used to eliminate them; `scripts/build.mjs` now asserts they are **present** in the bundle (`GA_MARKERS`). -**This does not restrict work in this repo.** Building, changing, testing, and QA-ing the public-app code paths is expected and unblocked — write the code, run the commands, create public apps against test environments. Build with `PREVIEW=1 yarn link:dev` (or `yarn build:preview`) and the full surface is there. Never refuse or hedge on a public-app task in this repo. +**Nothing is gated, and the gate is gone** — `src/lib/preview.ts`, `src/globals.d.ts`, the esbuild `define` block, the `LEAK_MARKERS` / `LEAK_STRINGS` checks, `build:preview` and the three preview modules (`commands/preview-definitions.ts`, `lang/preview-messages.ts`, `lib/preview-constants.ts`) are all deleted. **There is one build** — no `PREVIEW=1`, no `__BREVO_PREVIEW__`. If a feature ever has to be held back from a published build again, read `CLAUDE.md` → *If you ever need to gate a feature again* first: the mechanism and its two traps are written down there, and flipping a readiness row is not sufficient on its own. -- **The guard is the build, not the docs.** This replaced a documentation-only notice (and then a runtime check). `agent-context/SKILL.md` and `agent-context/AGENTS.md` no longer carry a *⚠️ not available yet* section or an *Exception — internal Brevo accounts* clause; they carry one rule instead — `brevo --help` is the complete surface. Don't reintroduce prohibition prose: an agent can't be led into a command that isn't in the binary. -- **There is deliberately no runtime escape hatch.** The earlier gate unlocked on an `@brevo.com` account or `BREVO_ENABLE_PREVIEW=1`; both are gone. A compile-time guard any user can switch back on is a runtime guard wearing a costume, and it has to ship the surface in order to reveal it. **Do not add one back.** -- **Two layers, no soft middle.** The build removes the surface; the Brevo API refuses public-app creation independently (`400 invalid_parameter`). -- **`FEATURE_STAGE` in `src/lib/preview.ts` is the single source of truth** for what is gated — but flipping a row to `'ga'` is necessary and **not sufficient** for a command, because gated definitions live in `src/commands/preview-definitions.ts` behind a *build* flag. See `RELEASE-CHECKLIST.md`. -- **When public apps go GA**, work through `RELEASE-CHECKLIST.md` → *Before public-apps GA* in one pass. +**`brevo app submit` is a form hand-off, not a state transition** — it opens a Google Form and changes nothing server-side, so exit `0` does not mean "submitted". The initial review state is `draft` (not `configured`, renamed by BEX-382), and reviewer feedback goes out by email, never through `app status`. -## UI apps are GA — they ship in every build (BEX-290) +## UI apps are GA — the whole surface ships (BEX-290) -UI apps (action links that render inside Brevo CRM records) are **out of the pre-GA gate**: the *UI app* choice at `brevo app create`'s app-type prompt, `brevo app install [account-id]` and `brevo app uninstall [account-id]` all ship in the published build. Their `FEATURE_STAGE` rows are `'ga'`, their command definitions live in `src/commands/definitions.ts`, their strings in `src/lang/en.ts`, and their names are gone from `LEAK_MARKERS` in `scripts/build.mjs`. Only the public-apps surface above remains gated. +UI apps (action links that render inside Brevo CRM records) shipped at BEX-290: the *UI app* choice at `brevo app create`'s app-type prompt, `brevo app install [account-id]` and `brevo app uninstall [account-id]` are all in the published build. Their command definitions live in `src/commands/definitions.ts`, their strings in `src/lang/en.ts`, and their bindings are asserted **present** by `GA_MARKERS` in `scripts/build.mjs`. Public apps followed at BEX-405 and the pre-GA gate was torn down after it. A UI app is **prompt-only**: there is no `--type` flag and no per-field flags, so non-interactive runs always create an OAuth app. `extension_type` values are camelCase (`actionLink`, `iframeExtension`, `legacyComponent`) and the old snake_case spellings are rejected. The `ui_app` block's **field names are confirmed** against both of the platform's consumers, the manifest read path and the extensibility UI kit (BEX-308 / BEX-350) — it is the stored app snapshot verbatim. See `CLAUDE.md` → *UI apps are GA* for the full contract. diff --git a/CLAUDE.md b/CLAUDE.md index 86b2fe3d..58ed47af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,21 +9,33 @@ Brevo Developer CLI (`@getbrevo/cli`) — create, manage, and test OAuth integra - **Package manager:** Yarn >= 1.19.1 - **Public CLI reference:** https://developers.brevo.com/docs/cli-reference — keep behaviour, flags, and exit codes in sync with this page when changing user-facing commands. -## Public app distribution is not GA — the published build omits it (BEX-405) +## Public apps are GA — the whole surface ships (BEX-405) -Public app distribution is **not live on the Brevo platform**. The repo contains the whole surface — `brevo app create --distribution public`, `brevo app submit`, `brevo app status`, `brevo app withdraw` — but a **published build does not**: `scripts/build.mjs` eliminates the review-lifecycle commands from the bundle, and `--distribution public` is refused with a typed `CliError`. +Public app distribution and the review lifecycle are **live**. `brevo app create --distribution public`, `brevo app submit`, `brevo app status` and `brevo app withdraw` all ship in the published build. This replaced a build-time gate that eliminated them from the bundle; `scripts/build.mjs` now asserts the three command bindings are **present** in the bundle (`GA_MARKERS`) rather than absent. -**This does not restrict work in this repo.** Building, changing, testing, and QA-ing the public-app code paths is expected and unblocked — write the code, run the commands, create public apps against test environments. Build with `PREVIEW=1 yarn link:dev` (or `yarn build:preview`) and the full surface is there. Never refuse or hedge on a public-app task in this repo. +**Nothing is gated, and the gate is gone.** It was kept all-`'ga'` for one release after GA to keep that diff reviewable, then torn down: `src/lib/preview.ts` (`FEATURE_STAGE`, `isFeatureAvailable`, `assertFeatureAvailable`), `src/globals.d.ts`, the esbuild `define` block and `LEAK_MARKERS` / `LEAK_STRINGS` / `orphanedPreviewMessageKeys` in `scripts/build.mjs`, the `build:preview` script, `help.ts`'s `gatedSection` / `distributionValues` / `createDescription`, `messages.PREVIEW_FEATURE_UNAVAILABLE`, and the `preview.test.ts` / `preview-gate.test.ts` suites are all deleted. The three modules that carried gated surface (`commands/preview-definitions.ts`, `lang/preview-messages.ts`, `lib/preview-constants.ts`) went at GA itself. **There is one build again** — no `PREVIEW=1`, no `__BREVO_PREVIEW__`, and the jest suite runs against the artifact users get. -- **The guard is the build, not the docs.** This replaced a documentation-only notice (and then a runtime check). `agent-context/SKILL.md` and `AGENTS.md` no longer carry a *⚠️ not available yet* section or an *Exception — internal Brevo accounts* clause; they carry one rule instead — `brevo --help` is the complete surface. Don't reintroduce prohibition prose: an agent can't be led into a command that isn't in the binary. -- **There is deliberately no runtime escape hatch.** The earlier gate unlocked on an `@brevo.com` account or `BREVO_ENABLE_PREVIEW=1`; both are gone. A compile-time guard any user can switch back on is a runtime guard wearing a costume, and it has to ship the surface in order to reveal it. Internal testing is a different artifact, not a different flag. **Do not add one back.** -- **Two layers, no soft middle.** The build removes the surface; the Brevo API refuses public-app creation independently (`400 invalid_parameter`). There is no longer a client-side check for a user to talk past, so the old "guardrail, not a security boundary" caveat no longer applies. -- **`src/lib/preview.ts` → `FEATURE_STAGE` is the single source of truth** for what is gated. Flipping a row to `'ga'` is necessary but **not sufficient** for a command: gated command definitions live in `src/commands/preview-definitions.ts` and are referenced from behind `__BREVO_PREVIEW__`, a *build* flag, so a GA feature left in that module is still eliminated. See the GA runbook (`RELEASE-CHECKLIST.md` on `feature_set-brevo-cli-v2`) → *Before public-apps GA* for the full sequence. -- **When public apps go GA**, work through the runbook's *Before public-apps GA* section in one pass. +Two things survived the teardown on purpose and must not be swept up as leftovers. `CommandDefinition.requires` is a `Capability` from `src/app-types/capabilities.ts`, not gate config — `command-capabilities.test.ts` asserts every value is reachable for some app type/distribution, and `lib/help.ts`'s two section headings are its prose copy. And `jest.setup.js` stays: the flag line is gone from it, but its other job — scrubbing every ambient `BREVO_*` var out of `process.env` — is load-bearing and unrelated, so the file and its `setupFiles` entry both stay. -## UI apps are GA — they ship in every build (BEX-290) +### If you ever need to gate a feature again -UI apps (action links that render inside Brevo CRM records) are **out of the pre-GA gate**: the *UI app* choice at `brevo app create`'s app-type prompt, `brevo app install [account-id]` and `brevo app uninstall [account-id]` all ship in the published build. `FEATURE_STAGE['ui-app-type']` and `FEATURE_STAGE['account-install']` are `'ga'`, the two command definitions live in `src/commands/definitions.ts` (not `preview-definitions.ts`), their strings live in `src/lang/en.ts` (not `preview-messages.ts`), and their names are gone from `LEAK_MARKERS` in `scripts/build.mjs`. Only the public-apps surface above remains gated. +The mechanism is written down here rather than in the code because the code is gone. It cost two releases to get right, and neither trap is discoverable by reading a `FEATURE_STAGE`-shaped table. + +**The shape.** A build-time flag, not a runtime one: `scripts/build.mjs` substitutes a bare global (`__BREVO_PREVIEW__`, declared in a `globals.d.ts`) via esbuild's `define`, `minifySyntax` folds the dead branch, and tree-shaking then drops the modules only that branch referenced. Declare it as a **bare global**, never as an exported constant — esbuild folds a constant only inside the module that declares it, so an importer still emits `PREVIEW_BUILD ? gatedCommands : []` as a runtime ternary, which keeps `gatedCommands` live and ships every gated command, unreachable but present. Assert on the **emitted bundle**, not on the config: a define typo or a stray static import leaves the config looking correct while the bundle quietly regains the surface. And give it **no runtime escape hatch** — not an env var, not an `@brevo.com` account check. A compile-time guard any user can switch back on is a runtime guard in costume, and it has to ship the surface in order to be able to reveal it, which defeats building it out. Internal testing is a genuinely different artifact (`PREVIEW=1 yarn link:dev`). + +**Trap 1 — a gated command's definition must be out of reach of a live import.** Gated entries need their own module (`commands/preview-definitions.ts` was the name), referenced from `definitions.ts` only from behind the flag. Written inline, the import is a live reference and the command ships. + +**Trap 2 — the build flag is the OUTER authority, above any readiness table.** A help section or prompt branch wrapped in `__BREVO_PREVIEW__` stays hidden in a published build no matter what the table says, so flipping a row to `'ga'` is necessary but not sufficient — the wrapper has to come off by hand in the same change. Both UI-apps GA and public-apps GA shipped a feature that was still invisible for exactly this reason. + +**Object literals need their own module, for a third reason.** esbuild cannot prune a property from an object literal, so anything reached as `OBJECT.KEY` survives at zero references: gated strings in `messages`, `CLI.*` command names and `ENDPOINTS` paths all stayed readable via `strings` on the published binary until they were split into modules (`lang/preview-messages.ts`, `lib/preview-constants.ts`) and spread in behind the flag. The inverse failure is worse and shipped once: a **live** reader left holding a key whose definition was eliminated resolves to `undefined`, and `new CliError(undefined)` prints a bare `✗` and exits 1. If you recreate a gated strings module, recreate the build assertion that catches that too. + +**`brevo app submit` is a form hand-off, not a state transition.** It opens a Google Form (`app.google_form_link` off the app payload) and changes nothing server-side; the submission is real only when the developer completes that form. Exit `0` does not mean "submitted", and the agent docs carry that as a hard rule. There is **no CLI-side refusal of `upload` or `delete` while an app is under review** — don't add doc claims to that effect without building the check. + +**The initial review state is `draft`, not `configured`** (BEX-382 renamed it on the wire with no alias). The full set is `draft`, `submitted`, `in_review`, `approved`, `rejected`, `changes_requested`, plus the `unknown` sentinel `status.ts` normalises an empty state to. Reviewer feedback is delivered by email and deliberately never surfaced by `app status` (BEX-252). + +## UI apps are GA — the whole surface ships (BEX-290) + +UI apps (action links that render inside Brevo CRM records) shipped at BEX-290: the *UI app* choice at `brevo app create`'s app-type prompt, `brevo app install [account-id]` and `brevo app uninstall [account-id]` are all in the published build. Their command definitions live in `src/commands/definitions.ts`, their strings in `src/lang/en.ts`, and their bindings are in `GA_MARKERS` in `scripts/build.mjs` so every build asserts they ship. Public apps followed at BEX-405 and the pre-GA gate was torn down after it — see that section above for what is left of it and for the recipe if a feature ever has to be held back again. Everything below in this section is durable technical reference for the `ui_app` contract and stays load-bearing. @@ -129,10 +141,12 @@ footgun this feature exists to remove. The internal working docs are committed on feature branches only and **must never land on `main`** — this repo is public, and they consolidate internal release state: +- `docs.md` and `QA-TESTCASES.md` on `feature_set-brevo-cli-v2` (this branch — the + public-apps halves). `RELEASE-CHECKLIST.md` and `PUBLIC-APPS-RELEASE-STATUS.md` were + here too until public apps went GA; the runbook was worked through and both were + deleted, so do not go looking for them. - `UI-APPS-RELEASE-STATUS.md`, `RELEASE-CHECKLIST.md`, `docs.md` and `QA-TESTCASES.md` - on `feat/bex-416-entry-size` (this branch — the UI-apps halves) -- `PUBLIC-APPS-RELEASE-STATUS.md`, `RELEASE-CHECKLIST.md`, `docs.md` and - `QA-TESTCASES.md` on `feature_set-brevo-cli-v2` (the public-apps halves) + on `feat/bex-416-entry-size` (the UI-apps halves) Before merging any branch that carries one of these files into `main`, delete the file from the branch first. Do not "helpfully" move their content into README, docs, or the @@ -270,17 +284,20 @@ The CLI ships two agent-facing docs at the repo root, both bundled into the publ - Changed defaults (new opt-in/opt-out, changed prompt behavior). - Changed exit codes or error messages that scripts may match on. - Removed features that the docs currently advertise (e.g. removing `brevo skill:cli update` requires removing it from both docs). -- **A feature going GA.** Taking a feature out of a `## Before …GA` section in `RELEASE-CHECKLIST.md` is a user-visible change like any other — it adds commands, flags or prompts to the published CLI. **Update `agent-context/SKILL.md` and `agent-context/AGENTS.md` for that feature in the same PR**, before ticking it off the checklist. Their reference text for a gated feature was *deleted* rather than hidden, so recover it from git rather than rewriting it — each GA section names the commit to recover from. A GA release that ships the commands but not the docs is the worst of both: agents keep telling users the feature doesn't exist, and the docs say so in writing. +- **A feature being released.** Whatever held it back — a build gate, a hidden command, a platform flag — the release itself adds commands, flags or prompts to the published CLI and is a user-visible change like any other. **Update `agent-context/SKILL.md` and `agent-context/AGENTS.md` for that feature in the same PR.** A release that ships the commands but not the docs is the worst of both: agents keep telling users the feature doesn't exist, and the docs say so in writing. + + If the feature's reference text was *deleted* rather than hidden, recover it from git rather than rewriting it — but **verify the recovered text before pasting it**, and don't trust a recorded `git log -S` recipe. Public-apps GA is the worked example: the runbook's two recipes returned nothing (the search strings had never existed at those paths), the real source was `51cdf52`, and the text recovered from it carried two stale claims — a review state renamed on the wire (`configured` → `draft`, BEX-382) and a wire field that had since been renamed (`app_version` → `version`). Recovered text is a draft, not an answer. **What does NOT count:** internal refactors, bug fixes that preserve UX, dependency bumps, test-only changes, log-line formatting tweaks that aren't part of the documented contract. -**A feature going GA must join the smoke test's live suites, in the same PR.** The smoke runner splits by surface: `scripts/smoke-test.ts`'s `SUITES` registry holds one suite per app type, and `.github/workflows/smoke.yml`'s `suite` input defaults to the **live** set — the suites whose commands are in the published bundle. A preview suite (today: `public`) is only meaningful on a `PREVIEW=1` build, which is why `smoke.yml` **refuses** a public-containing suite unless `against=local` rather than letting its steps auto-skip into a green run. So when a feature leaves `FEATURE_STAGE`'s `'preview'`: +**A new command surface must join the smoke test, in the same PR.** The smoke runner splits by surface: `scripts/smoke-test.ts`'s `SUITES` registry holds one suite per app type, and `.github/workflows/smoke.yml`'s `suite` input defaults to `private,ui,public` — every suite there is, since there is one build and nothing is held back. So when a feature adds or changes a command: -1. Move its steps out of the preview suite, or add a suite for it, in `scripts/smoke/`. -2. Add it to `smoke.yml`'s `suite` **default** and its `options`, so the manual button and any new lane cover it. -3. Decide, deliberately, whether the release lanes should cover it — `smoke-pre-merge.yml`, `smoke-post-merge.yml` and `release.yaml`'s dispatch each **pin** `suite` explicitly so a retuned default can never silently change what a publish gate verifies. Widening a gate is a real decision: verify the suite passes on `ubuntu-latest` first, since a suite that only ever ran on a dev machine (a pty-driven one especially) has not been proven headless. +1. Add its steps to the suite for its app type, or add a suite for it, in `scripts/smoke/`. +2. Add any new suite to `smoke.yml`'s `suite` **default** and its `options`, so the manual button and any new lane cover it. +3. **A local run always builds the published artifact.** `scripts/smoke/core.ts`'s `stepReinstall` used to fork on `PREVIEW=1` when the selected suites needed a gated surface; that fork went with the gate. If a suite is ever build-specific again, re-add it there *and* re-add `smoke.yml`'s refusal of the impossible pairing — letting a suite run against a package that cannot satisfy it auto-skips every step into a green run, which reads as coverage the run never had. +4. Decide, deliberately, whether the release lanes should cover it — `smoke-pre-merge.yml`, `smoke-post-merge.yml` and `release.yaml`'s dispatch each **pin** `suite` explicitly so a retuned default can never silently change what a publish gate verifies. Widening a gate is a real decision: verify the suite passes on `ubuntu-latest` first, since a suite that only ever ran on a dev machine (a pty-driven one especially) has not been proven headless. As of public-apps GA, `smoke-pre-merge.yml` covers `all` (non-blocking, `against=local`) while `smoke-post-merge.yml` stays pinned at `private,ui` against the published package — so the review lifecycle is **not** yet a publish gate. Tracked in `docs.md`. -A GA'd feature the smoke never exercises is the mirror of the docs problem above: the release gate reports green on a surface it never touched. +A shipped feature the smoke never exercises is the mirror of the docs problem above: the release gate reports green on a surface it never touched. **Skill version tracks the CLI version automatically.** `SKILL_CATALOG[brevo-cli].version` is computed at module-init from `package.json` (`CLI_VERSION` in `src/skills/index.ts`), so every published CLI release auto-refreshes installed skills — even when `SKILL.md` content didn't change. You only need to land your changeset; the skill version takes care of itself. @@ -291,46 +308,41 @@ A GA'd feature the smoke never exercises is the mirror of the docs problem above - Services are tested against mocked API client responses. - Template tests verify variable substitution, not file I/O. -## Working docs: `RELEASE-CHECKLIST.md`, `docs.md` and `QA-TESTCASES.md` - -Three working docs with different jobs, **split by feature across the two feature -branches** (all branch-local — see the never-merge rule above): this branch's copies -carry the **UI-apps halves**, and `feature_set-brevo-cli-v2`'s copies carry the -**public-apps halves** (release copy, the *Before public-apps GA* runbook, QA suites 2, -5, 6, 7, 10 and 13). They lived on a separate `docs/public-cli-ui-apps-feature-changes` -branch until 2026-08-24; that branch is deleted (final pre-split state in closed PR -#53). Read this before editing any of them. - -- **`RELEASE-CHECKLIST.md` — the GA runbook.** Ordered, mechanical steps for the day a - feature ships. The public-apps runbook (on `feature_set-brevo-cli-v2`) is **durable**: - it stays until public apps ship — do not delete it as cleanup. This branch's copy is - the record of the UI-apps flip (worked through at BEX-290); it stays until the release - publishes. -- **`docs.md` — the open-questions log.** Part 1 is release copy held until GA (the - UI-apps half is superseded by this branch's pending changeset and kept as background); - Part 2 is everything still unknown or undecided, including assumptions that survive a - GA. -- **`QA-TESTCASES.md` — the manual test plan**, with the recorded sweep results. This - branch's copy is Suite 12 (UI apps), which matches this branch's surface and runs on a - plain build; the public-app suites (on `feature_set-brevo-cli-v2`) need `PREVIEW=1`. - -None of the three is in `package.json` `files:`, so nothing ships to npm — but the -never-merge rule applies regardless, because branches are public too. - -The split is *what to do on the day* versus *what is still unknown*. An item moves from -`docs.md` to `RELEASE-CHECKLIST.md` when it turns into a release step, and is deleted from -`docs.md` when it resolves. +## Working docs: `docs.md` and `QA-TESTCASES.md` + +Two working docs at this branch's root, both **branch-local, never merged into `main`** +(see the rule above). They carry the **public-apps halves**; the UI-apps halves live on +`feat/bex-416-entry-size`. Read this before editing either. + +- **`docs.md` — the open-questions log.** Everything still open on public apps after GA: + the release-gate coverage gap, TC-6.3's remaining half, the BEX-355 / BEX-350 / BEX-437 + sign-offs, and the QA gaps. It used to have a *Part 1* holding release copy until GA; + that copy was consumed into the changeset and the split is gone. The gate-machinery + teardown was tracked here too and is now done. +- **`QA-TESTCASES.md` — the manual public-apps test plan** (suites 2, 5, 6, 7, 10, 13), + with the recorded sweep results. The recorded results were taken on the pre-GA + `PREVIEW=1` artifacts, which no longer exist — there is one build now; re-baselining + them is tracked in `docs.md`. + +Neither is in `package.json` `files:`, so nothing ships to npm — but the never-merge rule +applies regardless, because branches are public too. Working rules: - **Whenever you identify follow-up work that isn't done in the current change**, add it to - `docs.md` → *Part 2* rather than letting it fall through silently — this branch's copy - for UI-apps work, `feature_set-brevo-cli-v2`'s for public-apps work. (There was a - `TODO.md` here; it was folded into `docs.md` because its contents were entirely - public-app / UI-app follow-ups.) -- **Per-branch verification notes are scratch.** When a branch keeps a - `## Per-branch verification` section in a local `RELEASE-CHECKLIST.md`, clear it before - merging into `main` — per-branch working state doesn't belong in `main`'s history. + `docs.md` rather than letting it fall through silently — this branch's copy for + public-apps work, `feat/bex-416-entry-size`'s for UI-apps work. (There was a `TODO.md` + here; it was folded into `docs.md` because its contents were entirely public-app / + UI-app follow-ups.) +- **When an item in `docs.md` turns into a set of ordered release steps, it needs a + runbook.** There was a `RELEASE-CHECKLIST.md` here for exactly that and it was deleted + once public apps shipped — recreate one rather than growing `docs.md` into it. Keep the + split: `docs.md` is *what is still unknown*, a runbook is *what to do on the day*. +- **Before merging this branch into `main`, delete both working docs from it** (the + never-merge rule above). +- **Per-branch verification notes are scratch.** If a branch keeps a + `## Per-branch verification` section in a working doc, clear it before merging into + `main` — per-branch working state doesn't belong in `main`'s history. ## Adding a new command @@ -364,7 +376,7 @@ yarn publish:packages # publish to npm Shared: required files present, forbidden files absent (the branch-local working docs above, plus `.env` / `credentials.json` / `.brevo.json` / keys), every `src/templates/files/*.tmpl` shipped, no secret-shaped string in packed content, and **the tarball installs into an empty tree where `brevo --version` runs** — the only check on the dependency closure, since deps stay external and one that drifted into `devDependencies` would pack fine and die on the first install. `post` adds the registry metadata: `latest` moved, a SLSA provenance attestation is attached, the publisher is the OIDC identity, and the download matches `dist.integrity`. If those last two fail, fix the trusted publisher on npmjs.com — **do not** add an `NPM_TOKEN`. -Two things it deliberately doesn't do: re-check the gated public-app surface (`scripts/build.mjs` owns `LEAK_MARKERS` / `GA_MARKERS` and `prepublishOnly` reruns it, so a copy here could only drift), and stand in for the smoke test (`smoke.yml` authenticates and drives real commands; this only proves the artifact). `.tmpl` paths are skipped in the forbidden-*filename* scan — the template set includes `.env.example.tmpl` and `app-config.json.tmpl` on purpose — but their content is still secret-scanned. +Two things it deliberately doesn't do: re-check which surface the bundle carries (`scripts/build.mjs` owns `GA_MARKERS` and `prepublishOnly` reruns it, so a copy here could only drift), and stand in for the smoke test (`smoke.yml` authenticates and drives real commands; this only proves the artifact). `.tmpl` paths are skipped in the forbidden-*filename* scan — the template set includes `.env.example.tmpl` and `app-config.json.tmpl` on purpose — but their content is still secret-scanned. **npm auth: Trusted Publishing (OIDC), no long-lived token.** Publishes authenticate to npm via the GitHub Actions OIDC token (`id-token: write`) — there is no `NPM_TOKEN` secret. The trust relationship is configured on npmjs.com for `@getbrevo/cli` and binds publishes to: repo `getbrevo/brevo-cli`, the specific workflow file, and the GitHub environment (`npm-publish` for stable, `npm-prerelease` for alphas). See https://docs.npmjs.com/trusted-publishers. diff --git a/QA-TESTCASES.md b/QA-TESTCASES.md new file mode 100644 index 00000000..85ed4611 --- /dev/null +++ b/QA-TESTCASES.md @@ -0,0 +1,428 @@ +# QA Manual Test Cases — public apps + +Manual test suite for public app distribution (BEX-405, **GA**): the review +lifecycle (`app status` / `app submit` / `app withdraw`) and `--distribution public`. It +sits at this branch's root alongside `docs.md` — **branch-local, never merge into +`main`** (see `CLAUDE.md`). `RELEASE-CHECKLIST.md` and `PUBLIC-APPS-RELEASE-STATUS.md` +were here too until public apps shipped; both were worked through and deleted. The public-apps halves of the working docs +moved here from the retired `docs/public-cli-ui-apps-feature-changes` branch on +2026-08-24; the UI-apps halves (Suite 12 included) live on `feat/bex-416-entry-size`. + +> **Suite and case numbers are unchanged from the original combined plan**, which is why +> they are sparse here (2, 5.13–5.16, 6, 7, 10.2–10.3, 13.4). Renumbering would break +> every reference to a case in commit messages, PRs and `docs.md`. Suite 12 (UI apps) +> lives on `feat/bex-416-entry-size`; the private-app half was per-branch scratch on +> `features_set_public_cli`, recoverable from its history +> (`git show abedd75^:QA-TESTCASES.md`). + +> **⚠️ Entry condition changed at GA — re-read before running any suite.** Every case +> here used to require `PREVIEW=1 yarn link:dev`, because the commands were eliminated +> from a published build and `--distribution public` was refused. **That is no longer +> true: `yarn link:dev` runs every suite below, and it is the only build there is** — the +> pre-GA gate was torn down after GA, so `PREVIEW=1` and `build:preview` no longer exist. +> Any case whose expected result was "refused" or "unknown command" on a published build +> is stale: those were assertions about the gate, not about the feature. +> +> **The recorded sweep results below were all taken on the old `PREVIEW=1` artifacts.** +> Those builds differed from the published one by a single unreachable byte, so the +> observations still stand, but re-baselining the sweep once is tracked in `docs.md`. +> +> **The distribution question offers both values now.** `Distribution type?` is asked in +> every build and lists `Private` then `Public`, with `Private` first so a bare Enter +> still selects the conservative default. A one-item list (the old gated behaviour) is +> now a defect, not expected. Off a TTY and under `--json` the question is not asked and +> the value defaults to `private`. +> +> If an end-to-end path isn't live in your environment yet, note it on the case rather +> than skipping the whole section. + +--- + +## What these suites cover + +| Area | Change | +|------|--------| +| **BEX-252** | `brevo app status` — shows an app's review status as a coloured card. | +| **BEX-253** | `brevo app withdraw` — withdraws an app from submission. | +| **Public apps** | `brevo app create --distribution public`; interactive picker offers Public. | +| **US-2** | `brevo app upload` is blocked while an app is `Submitted` / `In Review`. **Server-side only — there is no CLI-side check.** Verified absent in `src/commands/app/upload.ts`; if this case passes, the platform is enforcing it. Do not record a CLI refusal here. | +| **BEX-405** | Public apps are **GA**: `--distribution public` is accepted and all three review commands ship in the published bundle. The build now asserts they are *present* (`GA_MARKERS`) rather than absent. | +| **TC-6.3 fix** | `brevo app submit` refuses a never-uploaded app locally, naming the real cause, instead of relaying the server's misleading four-field message. `brevo app status` still relays it — see `docs.md`. | + +--- + +## Test environment & global preconditions + +- **Node.js** ≥ 20.15.0, **Yarn** ≥ 1.19.1. +- **Build the binary** — one build runs every suite here; there is no `PREVIEW=1` variant + any more (the gate that needed one is gone): + ```bash + yarn install && yarn link:dev + brevo --version # confirm the branch build is on PATH + brevo app --help # status / submit / withdraw must all be listed + ``` +- A **Brevo test/staging account** you are authorised to use. Do **not** use production + customer data. +- Authenticated session: `brevo login` completed (or `BREVO_API_KEY=xkeysib-test-… brevo login`). +- Placeholder conventions in this doc: API keys `xkeysib-test-…`, app IDs like `42` / ``, + hosts `localhost` / `example.com`. Substitute real test-account values when running. +- Terminal is a **TTY** unless a case says "non-TTY / piped". + +### Exit-code reference (`src/lib/exit-codes.ts`) + +| Code | Meaning | +|------|---------| +| `0` | Success (also: withdraw of a not-submitted app, up-to-date upload) | +| `1` | Generic error (`CliError`, 403, generic API error) | +| `2` | Aborted | +| `3` | Auth failure (HTTP 401) | +| `4` | Network error | +| `5` | Not found (HTTP 404) | + +Check the exit code after any command with `echo $?`. + +--- + +## Suite 2 — `brevo app create`: public apps + +> **⚠️ Entry condition changed at GA — read before running this suite.** Public +> distribution *was* gated at build time: a published build refused +> `--distribution public` with *"That command is not available yet…"*, exit `1`, and +> asked the distribution prompt with `Private` as its only choice. **All of that is +> gone.** A plain build accepts `--distribution public` and offers both choices, so: +> +> - TC-2.1's correct result is **an app**, on any build. A refusal is now a defect. +> - TC-2.4 (the refusal path) no longer has a CLI-side trigger. What remains is the +> *server's* refusal for an account without `app-store-bo-be-public-apps` — still worth +> testing, but the expected error is the platform's, relayed as `ERR_UI_APP_NOT_ENABLED` +> or a `400`, not the CLI's unreleased-feature message. +> - There is one build, so nothing in this suite depends on which one you have. +> +> The platform used to refuse public creates from the CLI independently of the build, so +> even a preview build could hit the server's own rejection. That was lifted before GA — +> public creates are accepted on production for every account — which is what makes +> TC-2.1 a live end-to-end case rather than a preview-only one. TC-2.4 still needs an +> account the flag was never rolled out to, if one can still be found. + +### TC-2.1 — Create a public app with the flag +**Priority:** High +**Preconditions:** Authenticated; empty directory. +**Steps:** `brevo app create --name "QA Public App" --distribution public` +**Expected:** +- **No "coming soon"/unavailable error** and no early exit before the API call. +- App is created as **public**; API receives `distribution_type: "public"`. +- `app-config.json` records `"distribution_type": "public"`. +- Exit `0`. + +> **Account-dependent.** The platform allows a CLI public create only for accounts +> carrying the `app-store-bo-be-public-apps` flag. On an account **without** it the +> correct result is the mapped refusal (TC-2.4 / `APP_CREATE_PUBLIC_REJECTED`), not an +> app — record which kind of account you ran on. + +**Result:** ✅ Pass — 2026-08-13, preview build, internal production account (flag +enabled). Evidence is the written project: `distribution_type: "public"` and +`version: "0.0.2"`, i.e. created *and* uploaded. Not re-run on a flag-less account, so +the refusal half is untested. + +### TC-2.2 — Public is selectable in the interactive picker +**Priority:** High +**Steps:** Run `brevo app create` interactively; at the distribution prompt inspect the choices. +**Expected:** Two choices — **Private** and **Public** — both selectable (Public is **not** disabled/greyed as "coming soon"). Selecting Public creates a public app. + +### TC-2.3 — Public app appears correctly in `list` +**Priority:** Medium +**Steps:** After TC-2.1, `brevo app list --json`. +**Expected:** The public app is present with the public distribution reflected in server data. + +### TC-2.4 — The platform's own refusal is explained, not dumped +**Priority:** High +**Why it's here:** the preamble above has always pointed at TC-2.4; the case itself was +missing. Added 2026-08-13. +**Preconditions:** An account **without** the +`app-store-bo-be-public-apps` flag — i.e. the opposite of TC-2.1's precondition. The two +cases are mutually exclusive on any one account. +**Steps:** `brevo app create --name "QA Public Refused" --distribution public`, then the +same with `--json`. +**Expected:** The CLI **still attempts the create** (it deliberately does not mirror the +platform's per-account policy locally), then translates the `400`: a lead line saying +public apps can't be created from the CLI yet, a `Do this:` line naming +`--distribution private`, a `Note:` that `distribution_type` is fixed at creation, and a +`Brevo said:` line quoting the server verbatim. Exit non-zero. Under `--json`, the same +text arrives inside the single `{"error": {…}}` document on stdout. An unrelated `400` on +a public create must keep its own text — the translation is narrowed to messages naming +`distribution_type`. + +--- + +## Suite 5 (public-app subset) — `brevo app upload` under review states + +> The rest of Suite 5 — the diff, the confirm prompt, the payload contract, the legacy-scope +> and redirect-URL refusals — is private-app behaviour and stays in the feature branch's +> `QA-TESTCASES.md`. Only the four review-state cases are here, because only a public app +> has a review state. + +### TC-5.13 — Upload blocked while app is `Submitted` (US-2) +**Priority:** High +**Preconditions:** A public app currently in the `submitted` state (verify with `brevo app status`); a local change vs the server. +**Steps:** `brevo app upload --yes` +**Expected:** +- `upload` reads the app's current state **before** any push. +- Blocked with a friendly `CliError` explaining the app can't be modified while under review, plus a hint to withdraw first (`brevo app withdraw --app-id `). +- **No upload API call** is made; server state is unchanged. +- Exit `1`. + +### TC-5.14 — Upload blocked while app is `In Review` (US-2) +**Priority:** High +**Preconditions:** A public app currently in the `in_review` state; a local change vs the server. +**Steps:** `brevo app upload --yes` +**Expected:** Same as TC-5.13 — blocked with the friendly under-review `CliError` + withdraw hint; no push; exit `1`. + +### TC-5.15 — Blocked-state upload with `--json` surfaces structured reason (US-2) +**Priority:** High +**Preconditions:** A public app in `submitted` or `in_review`; a local change vs the server. +**Steps:** `brevo app upload --yes --json` +**Expected:** A single valid JSON blob describing the blocked reason (e.g. `{ "uploaded": false, "reason": "UNDER_REVIEW", "state": "", "message": …, "withdrawCommand": "brevo app withdraw --app-id " }`) — **not** a thrown stack trace. No push. `jq .` parses cleanly. + +### TC-5.16 — Allowed states still upload normally (US-2) +**Priority:** High +**Preconditions:** A public app in a non-blocking state (e.g. `draft`, `changes_requested`, `rejected`, `approved`) or a private app with no review state; a local change vs the server. +**Steps:** `brevo app upload --yes` +**Expected:** The state check passes through; the normal upload flow runs (diff shown, then push); "App uploaded." + `Version: …`; exit `0`. Confirms the block is scoped to `Submitted`/`In Review` only. + +--- + +## Suite 6 — `brevo app status` + +### TC-6.1 — Status via `--app-id` +**Priority:** High +**Steps:** `brevo app status --app-id ` +**Expected:** An aligned card: bold **App status** title, a `─` rule, a coloured icon + label, then the message indented under the label. Exit `0`. + +**Result:** ✅ Pass — 2026-08-13, preview build, on a public app resolved from the linked +`app-config.json` (no `--app-id`, so this covers TC-6.5 case 1 too). Rendered exactly as +written: bold `App status`, the `─` rule, `◇ Configured`, and *"Your app is set up but +hasn't been submitted for review yet."* indented under the label. Ran **after** an upload +— before one it fails, see TC-6.3. + +### TC-6.2 — State → tone/label mapping +**Priority:** Medium +**Steps:** Inspect status for apps in different states (use whatever states your test account can reach). +**Expected (label / colour / icon):** +- `approved` → "Approved" / green ✓ +- `rejected` → "Rejected" / red ✗ +- `changes_requested` → "Changes Requested" / yellow ⚠ +- `in_review` → "In Review" / yellow ◐ +- `submitted` → "Submitted" / blue ◔ +- `draft` → "Draft" / cyan ◇ ← **renamed from `configured` on the wire by BEX-382** (clean rename, no alias; the server migration renamed every existing row). A card reading *Configured* means the CLI is talking to a backend that predates the rename. +- unknown/other → gray ○ +Messages match the canned copy per state (e.g. `submitted` → "Your app has been submitted and is waiting to be reviewed."). + +### TC-6.3 — Empty/missing state → friendly "Unknown" +**Priority:** High +**Preconditions:** An app with no review state (e.g. a private app never submitted). +**Steps:** `brevo app status --app-id ` +**Expected:** Header "App status: Unknown"; message "Status information isn't available for your app yet. Make sure your app is public and has been uploaded with `brevo app upload`." Exit `0`. + +**Result:** ✗ **Fail — 2026-08-13. A never-uploaded app does not reach this path at all.** +On a public app freshly created by `brevo app init` and not yet uploaded, both +`brevo app status` and `brevo app submit` printed a **raw server message**: + +``` +✗ Please ensure your app is correctly configured with the following required data: name, logo_uri, scopes and redirect_uris +``` + +Not the friendly Unknown card. The cause is that `statusCommand` only reaches its +`state ?? 'unknown'` normalization when `fetchAppState` **resolves** — here +`GET` the state endpoint *rejects*, so `withCommandHandler` surfaces the `ApiError` +copy verbatim and the card never renders. Two separate problems: + +1. **The message is unmapped.** It is server copy reaching the user directly, which + `src/lang/en.ts` exists to prevent. Nothing in `apiCodeMessages` covers it. +2. **The message is misleading** — it names four fields that were all present (the + upload summary immediately afterwards showed a name, a server-defaulted + `logo_uri`, four scopes and one redirect URI). The real precondition is *"has never + been uploaded"*, i.e. the app has no `app_versions` row for the state endpoint to + read. Running `brevo app upload` once made the same command answer `◇ Configured`. + +**So TC-6.3 as written is unreachable via "never submitted"** — the app must be +*uploaded* but unsubmitted, which is the `draft` state (TC-6.1, recorded above as +`configured` before BEX-382 renamed it), not `unknown`. + +**Resolved for `app submit`, still open for `app status`.** The precondition was the +wrong half of the diagnosis: the real cause is the absent `app_versions` row, and the +server's copy for that failure names four fields that can all be present. `app submit` +now refuses locally on the certain signal — an app with no `version` has never been +uploaded — before the review-state read happens at all (`APP_SUBMIT_NOT_UPLOADED`). +`app status` reads the state directly and never fetches the app, so it still relays the +server text; closing that needs the server's error `code` and HTTP status from a live +repro, then one line in `apiCodeMessages`. Tracked in `docs.md`. Exit code was not +captured (`echo $?` not run) — it is whatever +`ApiError` maps to, not the documented `0`. Tracked in the sweep entry in +`RELEASE-CHECKLIST.md`. + +### TC-6.4 — `--json` output +**Priority:** High +**Steps:** `brevo app status --app-id --json` +**Expected:** `{ "state": "", "message": "" }`. Empty state serialises as `"state": "unknown"`. Exit `0`. + +### TC-6.5 — App resolution: linked config, then picker +**Priority:** Medium +**Steps:** +1. Inside a project dir (no `--app-id`): `brevo app status`. +2. Outside any project dir (no `--app-id`): `brevo app status`. +**Expected:** (1) auto-uses `appId` from `app-config.json`, no picker. (2) shows the interactive app picker. + +### TC-6.6 — Colour honours `NO_COLOR` / `FORCE_COLOR` +**Priority:** Low +**Steps:** `NO_COLOR=1 brevo app status --app-id ` and `FORCE_COLOR=1 brevo app status --app-id `. +**Expected:** `NO_COLOR=1` → no raw ANSI codes; `FORCE_COLOR=1` → coloured even when piped. + +--- + +## Suite 7 — `brevo app withdraw` + +> **Unlisted, not unavailable.** `app withdraw` is marked `hidden` so it appears on +> neither help screen (see TC-10.3). Every case below still runs exactly as written — +> type the command and it works. Discovery is the only thing that changed. + +### TC-7.1 — Withdraw a submitted app (force) +**Priority:** High +**Preconditions:** An app currently in `submitted`/`in_review`. +**Steps:** `brevo app withdraw --app-id --force` +**Expected:** POST to `/v3/app-store/apps//withdraw`; "App `` withdrawn from submission." Exit `0`. + +### TC-7.2 — Confirmation prompt (default No) +**Priority:** High +**Steps:** `brevo app withdraw --app-id `; press Enter (default), then rerun and confirm `y`. +**Expected:** Default is **No** → "Withdrawal cancelled.", no API call, exit `0`. Confirming → withdrawn. + +### TC-7.3 — Withdraw a not-submitted app → hint, exit 0 +**Priority:** High +**Preconditions:** An app that was never submitted (server returns HTTP 422). +**Steps:** `brevo app withdraw --app-id --force` +**Expected:** "App `` has not been submitted yet." + "Submit it first: brevo app submit --app-id ``". **Exit `0`** (verify with `echo $?`). + +### TC-7.4 — Not-submitted with `--json` +**Priority:** Medium +**Steps:** `brevo app withdraw --app-id --force --json` (on a not-submitted app). +**Expected:** `{ "withdrawn": false, "appId": "", "reason": "NOT_SUBMITTED", "message": …, "submitCommand": "brevo app submit --app-id " }`. Exit `0`. + +### TC-7.5 — Success `--json` +**Priority:** Medium +**Steps:** `brevo app withdraw --app-id --force --json` (on a submitted app). +**Expected:** `{ "withdrawn": true, "appId": "" }`. Exit `0`. + +### TC-7.6 — App resolution (linked config vs picker) and `--app-id` override +**Priority:** Medium +**Steps:** +1. Inside a project dir, no `--app-id`: `brevo app withdraw`. +2. Outside a project dir, no `--app-id`: `brevo app withdraw`. +3. Inside a project dir, explicit `--app-id `. +**Expected:** (1) auto-picks `appId` from `app-config.json`, no picker. (2) interactive picker. (3) explicit flag overrides the config. + +### TC-7.7 — Unknown app → not found +**Priority:** Medium +**Steps:** `brevo app withdraw --app-id 999999 --force` +**Expected:** "App 999999 not found." (HTTP 404 → exit `5`). + +--- + +## Suite 10 (public-app subset) — `brevo --help` layout + +> TC-10.1 (column alignment) was build-agnostic, private-half scratch (dropped with that +> half — recover from `features_set_public_cli` history). These two +> cases are the ones that read the gate: run them on **both** builds, since the useful +> assertion is the difference between them. + +### TC-10.2 — Public-app command grouping +**Priority:** Low +**Steps:** `brevo --help`. +**Expected:** `brevo app submit`, `brevo app status` **and `brevo app withdraw`** all appear under **"App-review commands (public apps only):"**, in that order. `app create` advertises `--distribution private|public`. `upload` is listed; `update` is not. There is one build, so there is nothing to compare against — the `PREVIEW=1` artifact this case used to be run twice for no longer exists. + +**Result (predates both GA flips and the `install`/`uninstall` rename — re-baseline +needed):** ✅ Pass — 2026-08-13, both builds. **Preview:** the *App-review commands +(public apps only)* heading carried `status` + `submit` and no `withdraw` (TC-10.3), the +then-gated UI-apps heading carried `deploy` + `rollback`, and `app create` advertised +`--distribution private|public`. **Published:** both headings and all four commands were +absent, and `app create` read `--distribution private` / *"Create a new OAuth app"*. + +**That build-to-build difference no longer exists** — it was the whole thing this case +measured, and public-apps GA removed it. Re-baseline the case as written above: one +expected screen, asserted on either build, with `withdraw` now listed. + +### TC-10.3 — `withdraw` is advertised on both help screens +**Priority:** Medium +**Steps:** `brevo --help`, then `brevo app --help`, then `brevo app withdraw --help`. +**Expected:** `withdraw` appears on **both** screens — the hand-aligned root screen (under *App-review commands*) and Commander's generated `brevo app --help`. `brevo app withdraw --help` prints its own usage (`Usage: brevo app withdraw [options]`) with `--app-id`, `--force` and `--json`, exit `0`. + +**Inverted at GA.** This case used to assert the opposite: `withdraw` carried +`hidden: true` while the review lifecycle was being finished, so it was callable but +advertised nowhere. Both suppressions are gone — the flag, and the matching hand-made +omission in `formatRootHelp`. **Check both screens, not one:** Commander's `hidden` +governs only its own output and cannot reach the root screen's string, so the two are +independent and a half-done change is exactly what this case catches. + +--- + +## Suite 13 (public-app subset) — `brevo app submit` + +> TC-13.1–13.3 (`init`, `credentials`, `delete`) were build-agnostic, private-half scratch +> (dropped with that half — recover from `features_set_public_cli` history). `app submit` +> is a public-app command, eliminated from a published build. + +### TC-13.4 — `brevo app submit` previews the config and opens the form +**Priority:** High +**Preconditions:** A **public**, uploaded app (see TC-6.3 — a never-uploaded app fails +first). Preview build. +**Steps:** `brevo app submit` from the linked project. +**Expected:** `No configuration mismatch detected.` when the local config matches the +server, then the full config preview (App ID, Name, Distribution, Redirect URLs, Scopes, +Logo URL, Version), then `Submit this app for review?`. Confirming opens a browser tab and +prints the form URL plus the caveat that **the app is only submitted once the form is +completed** and that `brevo app status` tracks it. Exit `0`. + +**Result:** ◐ Partial pass — 2026-08-13, preview build. As written, twice in a row. Note +what this means and what it does not: `app submit` is a **signpost to a Google Form**, not +an API submission, so it is idempotent and repeatable and never moves the app's state — +`brevo app status` still reported `◇ Configured` after it. **The mismatch branch was not +exercised** (no drift was introduced), and the form was not completed, so no `submitted` / +`in_review` state was ever reached — which is why TC-5.13–5.16, TC-6.2's later states and +all of Suite 7 remain unrunnable on this account. + +--- + +## Sign-off + +Cases carrying a **Result:** line were run on 2026-08-13 against **production** on a real +TTY, in **sweep 2** of the two manual sweeps run on `features_set_public_cli` — a +**preview** build covering `app init` on a public app, `status`, `submit`, `delete`, and +`scaffold` in both modes. (Sweep 1 was a published build and exercised the OAuth happy +path only, so it contributes nothing here except the published half of TC-10.2. The same +sweep's UI-app results live with Suite 12 on `feat/bex-416-entry-size`.) + +A Result line says which build it ran on when it matters. Nothing here is signed off. + +| Suite | Owner | Result (Pass/Fail) | Notes | +|-------|-------|--------------------|-------| +| 2 — create: public | Piyush | ◐ Partial pass | TC-2.1 ✅ — created live on a preview build + flag-enabled account, then uploaded to `0.0.2`. TC-2.4's refusal path still untested (needs an account **without** `app-store-bo-be-public-apps`); TC-2.2 / TC-2.3 not run. | +| 5 (subset) — upload under review | | **Blocked** | TC-5.13–5.16 not runnable on this account — see Suite 7. | +| 6 — status | Piyush | ✗ **Fail** (partly fixed at GA) | TC-6.1 ✅ (`◇ Configured` — **note the state is `Draft` now**, BEX-382 renamed it on the wire; aligned card, resolved from the linked config — also TC-6.5 case 1). **TC-6.3:** the `app submit` half is **fixed** — a never-uploaded app is refused locally naming the real cause, before the review-state read. The `app status` half still fails: it reads the state directly, so the raw misleading server message still reaches the user. Closing it needs the server's error `code` + HTTP status from a live repro, then one line in `apiCodeMessages` — tracked in `docs.md`. TC-6.2's other states, 6.4 (`--json`) and 6.6 (colour) not run. | +| 7 — withdraw | | **Blocked** | Not run, and **not runnable on this account**: `app submit` only opens a Google Form, so no app can be driven into `submitted`/`in_review` from the CLI. Reaching Suite 7 (and TC-5.13–5.16, and TC-6.2's review states) needs the form completed or the state set server-side. | +| 10 (subset) — help layout | Piyush | ✅ Pass | TC-10.2 / 10.3 confirmed on **both** builds, which is what makes the BEX-405 elimination visible. | +| 13 (subset) — submit | Piyush | ◐ Partial pass | TC-13.4 ◐ — ran twice, but the mismatch branch was not exercised and the form was never completed. | + +**Overall verdict:** ☐ Ready for GA ☑ Not yet signed off. + +What sweep 2 establishes: a public app can be created, uploaded and previewed for +submission end to end. + +What still blocks sign-off, in priority order: + +1. **TC-6.3 is a confirmed failure** — an unmapped, misleading server message on a + never-uploaded app. It needs a decision (map the message, or fix the case's + precondition and delete the dead empty-state path). +2. **Suite 7 and TC-5.13–5.16 are blocked, not merely unrun** — they need a submitted + app, which the CLI cannot produce on its own. +3. **No `--json` / non-TTY path has been run** for any public-app suite. +4. **TC-2.4's refusal path is untested** — it needs an account without the public-apps + flag, which is mutually exclusive with TC-2.1's precondition on any one account. diff --git a/README.md b/README.md index 0422c15a..2647ed56 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Or step by step: ```bash brevo app list - brevo app create --name "My App" --distribution private + brevo app create --name "My App" --distribution private # or --distribution public brevo app scaffold --app-id 3f8c1a2e-5b47-4d9c-8e10-6a2b7d4f0c93 brevo app start oauth --port 3000 ``` @@ -93,7 +93,7 @@ Run `brevo --help` or `brevo --help` for full command and option lists | `brevo logout` | Clear stored credentials (`--force` to skip confirmation) | | `brevo whoami` | Show the authenticated user | | `brevo app init` | Guided setup — login, create app, and scaffold in one go | -| `brevo app create` | Create an app — an OAuth app (`--name`, `--distribution private`, repeatable `--redirect-uri`, `--logo-uri`), or a UI app via the interactive prompts (there is no `--type` flag; non-interactive runs always create an OAuth app) | +| `brevo app create` | Create an app — an OAuth app (`--name`, `--distribution private\|public`, repeatable `--redirect-uri`, `--logo-uri`), or a UI app via the interactive prompts (there is no `--type` flag; non-interactive runs always create an OAuth app). `distribution_type` is immutable after create. | | `brevo app list` | List apps in your account (each row names its type) | | `brevo app credentials` | Show client ID and secret (`--app-id`, `--reveal-secret`) | | `brevo app upload` | Push `app-config.json` to Brevo after showing a local-vs-server diff — field by field, including every `ui_app` placement (`--yes`) | @@ -102,11 +102,14 @@ Run `brevo --help` or `brevo --help` for full command and option lists | `brevo app start` | Run a scaffolded feature locally (e.g. `brevo app start oauth --port 3000`) | | `brevo app install` | Install a UI app into a Brevo account, after showing the configuration and version it will install (`[account-id]` optional — a regular account installs into itself; a corporate account is prompted to pick a sub-account, so pass the ID explicitly in scripts; `--app-id`, `--force`) | | `brevo app uninstall` | Uninstall a UI app from a Brevo account (same arguments as `install`) | +| `brevo app submit` | Submit a public app for review — opens the submission form after checking the app is complete, public, and in sync with `app-config.json` (`--app-id`, `--json`) | +| `brevo app status` | Show an app's review state: `draft`, `submitted`, `in_review`, `approved`, `rejected` or `changes_requested` (`--app-id`, `--json`) | +| `brevo app withdraw` | Withdraw an app from submission (`--app-id`, `--force`) | | `brevo app available-scopes` | List the OAuth scopes the IdP supports (`--web` opens the catalog in a browser) | Most commands require a successful `brevo login` first, except authentication/help flows (`brevo login`, `brevo logout`, `brevo app init`, `--help`). Every command accepts `--json` for machine-readable output. -The table above is the complete command surface of a published release. Features that aren't live on the Brevo platform yet aren't built into the package — `brevo --help` always lists everything the binary can do, so there is nothing hidden behind a flag or an environment variable. +The table above is the complete command surface of a published release, and `brevo --help` always lists everything the binary can do — there is nothing hidden behind a flag, an environment variable, or an account setting. (Public app distribution and the review lifecycle were once built but withheld from published packages; they ship in every build now.) If a command listed here is missing from your install, it is older than this README — upgrade with `npm install -g @getbrevo/cli`. ### UI apps diff --git a/agent-context/AGENTS.md b/agent-context/AGENTS.md index 2db3048a..df9a5576 100644 --- a/agent-context/AGENTS.md +++ b/agent-context/AGENTS.md @@ -5,15 +5,17 @@ This project uses the [Brevo Developer CLI](https://www.npmjs.com/package/@getbr ## `brevo --help` is the source of truth -Brevo features that haven't been released are **not built into the published CLI at all** — not as hidden commands, not behind a flag or an env var. `brevo --help` and `brevo app --help` list everything the binary can do. Treat that as the complete surface and build from it. +`brevo --help` and `brevo app --help` list everything the installed binary can do. Treat that as the complete surface and build from it — **the version in front of you, not the version you remember.** + +Every command in the table below ships in every published build. Nothing is held back behind a flag, an env var, or an account setting. So when a documented command is missing, one explanation is left: **the install is older than this file.** (It was not always so — public app distribution and the review lifecycle were eliminated from published builds until they went GA, and this section existed to stop agents recommending them. That gate is gone.) What this means in practice: -- **If a command isn't in `--help`, it doesn't exist here.** Invoking it gives Commander's `unknown command` and exit `1`. That is a final answer, not a transient failure and not a permissions problem — there is no flag, config edit, account setting, or environment variable that reveals it. Don't retry, don't hunt for an alternative route, and don't tell the user to request access. Say the feature isn't available in this CLI and offer the nearest thing that works. -- **Don't act on a command you remember rather than one you can see.** Your training data may include commands from a newer or unreleased build of this CLI. `--help` in the current session is the only reliable source, and the table below documents only what a published build ships. -- **A flag can be rejected even when its command exists.** `--distribution` is a real flag, but a value tied to an unreleased feature is refused with *"That command is not available yet. It is part of a Brevo feature that has not been released."* Same rule: it's final. Use `--distribution private`. +- **Don't act on a command you remember rather than one you can see.** Your training data may be a newer release than the user has installed, or an older one. `--help` in the current session is the only reliable source. +- **`unknown command` plus exit `1` means "upgrade", not "retry".** Don't hunt for an alternative route and don't tell the user to request access — there is none to request. Compare `brevo --version` against this reference (see *Before starting a new session*) and point at `npm install -g @getbrevo/cli` if they are behind. +- **A flag value can be refused even when its command exists.** `--distribution public` on an install predating public-apps GA is refused with *"That command is not available yet. It is part of a Brevo feature that has not been released."* — a flag must parse before it can be rejected, so that is what an older binary says instead of `unknown option`. It reads like a permissions error and is not one: it means upgrade. -The Brevo API enforces the same boundaries independently, so nothing is gained by routing around the CLI — a hand-rolled `curl` hits the platform's own refusal. +The Brevo API enforces its own boundaries independently, so nothing is gained by routing around the CLI — a hand-rolled `curl` hits the platform's own refusal. ## AI agents — start here @@ -77,14 +79,17 @@ Don't fall back to raw HTTP against `api.brevo.com` — the `brevo` binary is th | `brevo whoami` | Show the authenticated account (`--json`) | | `brevo app init` | Guided setup (login, create, scaffold) | | `brevo app list` | List apps (`--json`). Each row names its type. | -| `brevo app create` | Create an app (`--name`, `--distribution private`, `--redirect-uri`, `--logo-uri`, `--json`). **Pass `--distribution private`** — run `brevo app create --help` for the values this build accepts and don't pass one it doesn't list. Defaults to scopes `contacts:read`, `contacts:write`, `crm:read`, `crm:write`. Interactively it asks for the name before the OAuth prompts, and asks the **app type** (OAuth vs UI app) as the last question before the flow splits — there is still no `--type` flag, but a UI app is also reachable non-interactively via `--ui-app --record-page --placement --label --url [--more-info ]`, or `--ui-config ` (JSON: `{ extension_type, record_page, surface_point_name, label, more_info?, redirect_link }`) — both `extension_type: "actionLink"` only today. A non-interactive run with neither flag still creates an OAuth app, as before. **Errors immediately if `app-config.json` already exists in the working directory** — move elsewhere or use `brevo app scaffold` there instead. Otherwise resolves (creates/`cd`s into) a target directory, creates the app, and writes the basic project structure (`app-config.json` + `.gitignore`/`AGENTS.md`/`CLAUDE.md`/`README.md`). It scaffolds a feature (OAuth test server) only when the interactive confirm (*"Scaffold the Test OAuth App?"*, default yes) is answered yes; non-interactive runs (`--json` or piped) stay base-only — add the feature afterward with `brevo app scaffold`. | +| `brevo app create` | Create an app (`--name`, `--distribution `, `--redirect-uri`, `--logo-uri`, `--json`). **`private` for apps used exclusively by the user's own organisation, `public` for apps distributed to end users or marketplace listings; default to `private` when the user hasn't said which.** `distribution_type` is **immutable after create** — `brevo app upload` refuses to change it, so a wrong choice means a new app; only a `public` app can be submitted for review. Defaults to scopes `contacts:read`, `contacts:write`, `crm:read`, `crm:write`. Interactively it asks for the name before the OAuth prompts, and asks the **app type** (OAuth vs UI app) as the last question before the flow splits — there is still no `--type` flag, but a UI app is also reachable non-interactively via `--ui-app --record-page --placement --label --url [--more-info ]`, or `--ui-config ` (JSON: `{ extension_type, record_page, surface_point_name, label, more_info?, redirect_link }`) — both `extension_type: "actionLink"` only today. A non-interactive run with neither flag still creates an OAuth app, as before. **Errors immediately if `app-config.json` already exists in the working directory** — move elsewhere or use `brevo app scaffold` there instead. Otherwise resolves (creates/`cd`s into) a target directory, creates the app, and writes the basic project structure (`app-config.json` + `.gitignore`/`AGENTS.md`/`CLAUDE.md`/`README.md`). It scaffolds a feature (OAuth test server) only when the interactive confirm (*"Scaffold the Test OAuth App?"*, default yes) is answered yes; non-interactive runs (`--json` or piped) stay base-only — add the feature afterward with `brevo app scaffold`. | | `brevo app upload` | Push `app-config.json` to Brevo (`--yes`, `--json`). No edit flags — change name/redirect URLs/scopes/logo/version by editing `app-config.json` directly, then run `upload`. **`distribution_type` is immutable** — set at `app create` time; if the local value differs from the server, `upload` errors before pushing (restore the local value, or create a new app). Always fetches the remote app first and shows a local-vs-server diff (even under `--yes`/`--json`); exits 0 with no network push if nothing differs. **For a UI app the diff covers the `ui_app` block placement by placement** — every changed value as `before → after`, added placements tagged `(new)`, dropped ones trailing `(removed)`, matched by slot slug so a reordered `surface_point_list` is not a change — and the command then warns that the app may already be installed in Brevo accounts and asks *"Proceed with upload and update every account this app is installed in?"*. There is no separate publish step: a successful upload changes what every account the app is installed in renders, immediately and with no re-install. `--yes` skips the question but still prints the warning; `--json` prints neither and stays a single parseable document. | | `brevo app credentials` | Show client ID / secret (`--app-id`, `--reveal-secret`, `--json`). **`--app-id` is required when scripting** — see the picker note below. **OAuth apps only** — a UI app has no OAuth credentials, so the command refuses it with exit `1` and points at `brevo app list` for the app's type. Also backfills a missing top-level `version` / `distribution_type` into cwd's `app-config.json` when its `appId` matches (fill-only-when-missing, silent). | | `brevo app delete` | Delete an app (`--app-id`, `--force`, `--json`). **`--app-id` is required when scripting** — see the picker note below. | -| `brevo app scaffold` | Add a feature to the app in the current directory (`--app-id`, `--overwrite`, `--json`). Requires an `app-config.json` in cwd **unless `--app-id ` is passed or you accept its bootstrap offer**; reads the linked app from it. `--app-id` in a directory with no config fetches that app and writes `app-config.json` + the base files first — the only way to get a config for an app that already exists, and the migration path off the removed `brevo app update --app-id`. Interactively, omitting `--app-id` in a config-less directory prints *"No app-config.json in this directory…"*, asks **"Set up a project for an app you already have?"** (default yes) and on yes shows an app picker; answering no exits `0` with the remaining routes on screen. Every interactive bootstrap (picker or `--app-id`) then asks `Output directory:` defaulted to `./`, creates it and `cd`s into it — answer `.` to stay in the current directory — and the *Next steps* box opens with `cd `. Under `--json` or off a TTY there is no offer — it errors, listing the three ways out. It refuses, before any network call: if the directory is already linked to a different app — either the one the command ran in or the one it was pointed at (naming the app it is already linked to is a no-op), if the directory is **inside** an existing app project (a nested second config would make a later `brevo app upload` from there push the wrong app). Diffs the local config against the server and, on drift, updates `app-config.json` to match (on consent) before writing the feature files — including when a bootstrap is pointed at a directory that already holds a project, where answering **Merge** to the directory question does *not* skip that refresh. When feature files already exist, prompts Overwrite / Merge / Cancel (default Merge — existing files kept); `--overwrite` forces overwrite and skips the prompt. The scaffolded OAuth flow is the confidential-client flow: the token exchange is authenticated with the `CLIENT_SECRET` written into the generated `.env.local`. | +| `brevo app scaffold` | Add a feature to the app in the current directory (`--app-id`, `--overwrite`, `--json`). Requires an `app-config.json` in cwd **unless `--app-id ` is passed or you accept its bootstrap offer**; reads the linked app from it. `--app-id` in a directory with no config fetches that app and writes `app-config.json` + the base files first — the only way to get a config for an app that already exists, and the migration path off the removed `brevo app update --app-id`. Interactively, omitting `--app-id` in a config-less directory prints *"No app-config.json in this directory…"*, asks **"Set up a project for an app you already have?"** (default yes) and on yes shows an app picker; answering no exits `0` with the remaining routes on screen. Every interactive bootstrap (picker or `--app-id`) then asks `Output directory:` defaulted to `./`, creates it and `cd`s into it — answer `.` to stay in the current directory — and the *Next steps* box opens with `cd `. Under `--json` or off a TTY there is no offer — it errors, listing the three ways out. It refuses, before any network call: if the directory is already linked to a different app — either the one the command ran in or the one it was pointed at (naming the app it is already linked to is a no-op), if the directory is **inside** an existing app project (a nested second config would make a later `brevo app upload` from there push the wrong app). Diffs the local config against the server and, on drift, updates `app-config.json` to match (on consent) before writing the feature files — including when a bootstrap is pointed at a directory that already holds a project, where answering **Merge** to the directory question does *not* skip that refresh. When feature files already exist, prompts Overwrite / Merge / Cancel (default Merge — existing files kept); `--overwrite` forces overwrite and skips the prompt. The scaffolded OAuth flow is the distribution-dependent OAuth flow: a **private** app gets the confidential-client flow, where the token exchange is authenticated with the `CLIENT_SECRET` written into the generated `.env.local`; a **public** app gets Authorization Code + **PKCE** (RFC 7636) — `/auth/login` generates a `code_verifier` and sends `code_challenge` + `code_challenge_method=S256`, the exchange and refresh send the verifier with **no client secret**, and the generated env files carry none. Don't send a public-app developer after their client secret. | | `brevo app start oauth` | Run the scaffolded OAuth test server (`--port`) | | `brevo app install` | Install a **UI app** into a Brevo account (`[account-id]` positional, `--app-id`, `--force`, `--json`). UI apps only — an OAuth app has nothing to install and the CLI refuses with exit `1`. `[account-id]` is optional: omitted, a plain account installs into itself (no prompt, `--json`/CI safe) and a corporate account picks a sub-account interactively — non-interactive corporate runs must pass it. Refused locally until the app has been validated by a `brevo app upload` (the `version` field is the signal). Interactively, omitting `--app-id` outside a linked project opens an app picker that lists **only UI apps**; with no UI app to offer it errors (exit `1`) naming `brevo app create`. That picker needs a terminal: under `--json` or off a TTY, omitting `--app-id` outside a linked project is refused with exit `1`. **Prints the configuration it will install before acting** — app ID, name, `version`, extension type and every placement, read from the server, because the account renders the stored snapshot and not the local `app-config.json`; under `--json` the same facts come back as additive `version` / `ui_app` keys on the result. When the linked project's block has drifted from the stored one it says so and names `brevo app upload`, then installs anyway (a notice, not a refusal — the stored configuration is a legitimate thing to install). | | `brevo app uninstall` | Uninstall a UI app from a Brevo account (same arguments and target resolution as `install`). Uninstalling an app that isn't installed is informational, exit `0` — not an error. | +| `brevo app submit` | Submit a **public** app for review (`--app-id`, `--json`). Fetches the app, then runs a status preflight (the same review-state read as `brevo app status`) and aborts if that read fails; refuses in order: the app has no `version` because it was never uploaded (checked locally, before the state read — *"has never been uploaded, so it has no version to review"*), the app is not `submittable` (the state API's own `missing_fields` keys are printed verbatim, e.g. `logoLink`, `oauth.scopes` — fix in `app-config.json` and `brevo app upload`), its `distribution_type` is not `public`, or `app-config.json` describes this app and has drifted from the server (field-by-field diff tagged `(local only)` / `(server only)`; resolve by matching the server or pushing with `brevo app upload`). Then shows the full app definition and asks for confirmation (interactive TTY only; skipped when stdin is not a TTY) and opens the submission form in the browser; `--json` prints `{"app_id","form_url"}` on stdout with the next-steps notes on **stderr**, and no prompt — use it in CI/headless contexts. **The app is only actually submitted once the Google Form is completed and submitted — the command itself changes nothing server-side, so exit `0` does not mean "submitted".** A repeat call is either idempotent (same `form_url`, exit `0`) or refused with *"Review submission is currently unavailable"* (exit `1`), which is also the answer for an app already under review. | +| `brevo app status` | Show an app's review status (`--app-id`, `--json`). Read-only. `--json` returns `{ state, message }` where `state` is one of `draft`, `submitted`, `in_review`, `approved`, `rejected`, `changes_requested`, or `unknown` when the server returns no state; `message` is the server's copy, falling back to built-in per-state text. Reviewer feedback is sent by email, never in this output — so `rejected` / `changes_requested` carry no reason here. | +| `brevo app withdraw` | Withdraw an app from submission (`--app-id`, `--force`, `--json`); `--app-id` optional inside a scaffolded project (reads `app-config.json`); if the app was never submitted, prints a submit hint and exits `0` with `{"withdrawn": false, "reason": "NOT_SUBMITTED", …}` — a normal outcome, not an error to retry | | `brevo app available-scopes` | List OAuth scopes supported by the IdP (`--json`, `--web`) | | `brevo skill:cli install` | Install the brevo-cli Claude Code skill (Claude-only; auto-refreshes on every `brevo` run) | | `brevo skill:cli uninstall` | Remove the brevo-cli skill from `~/.claude/skills/` (Claude-only) | @@ -98,12 +103,12 @@ Run `brevo --help` or `brevo --help` for the full set. - **`brevo app create` refuses to run inside an already-linked directory.** If `app-config.json` exists in cwd, it throws immediately (no confirm, no override) — the error points at moving elsewhere or running `brevo app scaffold` there. - **`brevo app create` resolves its target directory before creating the app**, then writes the **basic project structure only** (`app-config.json` + `.gitignore`/`AGENTS.md`/`CLAUDE.md`/`README.md`) — the OAuth server code is a *feature*, not part of the base. Interactive mode prompts for the target directory (default `./`, `cd`s into it) before the API call, how to handle an existing one (overwrite / merge / choose a different path), and — after the app is created — whether to scaffold a feature (*"Scaffold the Test OAuth App?"*, default **yes**). There is no follow-up "which feature?" question while the CLI ships one: a list of one is not asked, and the confirm names it instead. A second feature would bring the picker back. Non-interactive runs stay base-only: `--json` (and piped, non-TTY) create the app and write the base files but never scaffold a feature — run `brevo app scaffold` afterward for the OAuth code. Under `--json` the same default directory is used and `cd`d into if it doesn't already exist; if it already exists, both directory setup and scaffolding are skipped (the app is still created). The JSON response always includes `directory` (absolute path) alongside the app fields, plus either `scaffolded` (base file count, on success) or `scaffoldSkipped` (a message, when the directory already existed). - **`brevo app scaffold` adds a feature to an already-created project, or sets an empty directory up for an app that already exists.** It **requires** an `app-config.json` in cwd unless `--app-id` is passed or its bootstrap offer is accepted, and only the bootstrap mode ever creates a directory (the feature-add mode always writes into the project it was run in). **`--app-id ` bootstraps a project for an app that already exists**: it fetches the app, writes `app-config.json` + the base files, and then continues into the feature flow. That is the only command that produces a config for an existing app (`app create` creates a new one, `app upload` only reads the linked project), which makes it the migration path off the removed `brevo app update --app-id`. **Interactively, `--app-id` is optional**: in a config-less directory the command explains there is no app here, asks *"Set up a project for an app you already have?"* (default **yes**), and on yes runs the same app picker `app delete` uses — because a user who has lost their project folder has the app but not necessarily its ID. Declining is a normal outcome that exits `0` after printing the remaining routes; the offer is skipped entirely under `--json` or off a TTY, where the no-config error (naming all three ways out: `cd` into a project, `--app-id`, `brevo app create`) is raised instead, so scripts behave exactly as before. **An interactive bootstrap also asks where to put the project** — `Output directory:`, defaulted to `./`, the same prompt (and the same overwrite / merge / choose-a-different-path follow-up on an existing directory) `app create` uses; it creates the directory, `cd`s the CLI process into it, writes and reports the project, then asks *"Scaffold the Test OAuth App?"* (default yes; declining leaves the project and exits `0`), and opens *Next steps* with `cd ` since the user's shell stayed behind. Answering `.` keeps the current directory and drops that step. This too is interactive-only: under `--json` or off a TTY the files go into the current directory as they always have, which is what makes `scaffold --app-id` safe to script. In bootstrap mode the config is written from the server's copy of the app, since there is nothing local to read it from. Bootstrapping is refused, before any network call or write, in two cases: a directory already linked to a **different** app (passing the app it is already linked to changes nothing), and a directory **inside** an existing app project — `readProjectConfig` reads cwd only and never walks up, so without that check a stray `cd` would nest a second `app-config.json` inside the first and a later `app upload` from there would push the wrong app silently. The different-app check applies to the answer to `Output directory:` as well as to cwd, and there it is the only thing standing between you and a project whose `app-config.json` and `src/oauth/.env.local` name two different apps. **A target directory that already holds a project for the same app makes the bootstrap a refresh**: its config is diffed against the server and rewritten only on consent, and the directory question's **Merge** answer does not suppress that. The two answers address different things — Merge means "don't clobber my own files" and is implemented by skipping any path that already exists, which `app-config.json` always does here, so letting it govern the base write meant the command fetched the app, discarded every field, wrote nothing, and still printed its success box. No drift leaves `app-config.json` as it is with a one-line notice; the feature is still offered either way. It otherwise reads the linked app id from that config (no picker — the picker is only for the config-less bootstrap), diffs the local config against the server, and if fields drifted it shows them and asks consent to update `app-config.json` (and the other base files) to match before writing the feature files. When any feature file already exists it prompts **Overwrite / Merge / Cancel** (default **Merge** — existing, e.g. hand-edited, files are kept and only missing files added; Cancel aborts without writing). The `--overwrite` flag forces a full overwrite of feature files and skips that prompt (works interactively and under `--json`). **Under `--json` it never prompts**: a config diff comes back as `{ "cancelled": true, "reason": "...", "diffs": [...] }`; otherwise it writes the feature (merging existing files unless `--overwrite` is passed) and returns `{ "scaffolded": , "directory": "..." }`. -- **`app-config.json`** in the working directory pins the linked app — `brevo app upload` and `brevo app start` read from it. `upload` is the *only* command that pushes config changes, and it has no `--app-id` override (it always resolves the app from cwd's `app-config.json`, hard-erroring if that file is missing/invalid/lacks `appId`); `brevo app start` accepts `--app-id` to target a different app. The top-level `logoUri` string is pushed as `logo_uri`; leave it empty to keep the API value untouched. The top-level `version` string is round-tripped as `version` on the wire — `upload` sends the local value (falling back to the server's current value if locally absent) and writes back whatever the server confirms. `brevo app credentials` additionally backfills a missing top-level `version` / `distribution_type` into cwd's `app-config.json` when its `appId` matches the inspected app — fill-only-when-missing (never overwrites an existing local value), silent in all modes — so legacy projects that are never `upload`ed still converge to the current shape. -- **Commands that pick an app interactively refuse to do so when scripted.** `brevo app credentials` and `brevo app delete` fall back to an interactive app picker when `--app-id` is absent. Under `--json` **or** off a TTY that picker is refused up front — before any network call — with a `CliError` naming the exact command to run (`brevo app credentials --app-id `) and exiting `1`. **Always pass `--app-id` when scripting these.** The refusal exists because the picker renders its choice list to stdout, which would otherwise corrupt the single-JSON-document contract below and leak app ids into whatever is parsing it. `brevo app delete` is the one that matters most: it is destructive, so a script that relied on the picker was never doing what its author thought. +- **`app-config.json`** in the working directory pins the linked app — `brevo app upload`, `brevo app start`, `brevo app status`, `brevo app submit` and `brevo app withdraw` read from it. `upload` is the *only* command that pushes config changes, and it has no `--app-id` override (it always resolves the app from cwd's `app-config.json`, hard-erroring if that file is missing/invalid/lacks `appId`); `brevo app start` accepts `--app-id` to target a different app. The top-level `logoUri` string is pushed as `logo_uri`; leave it empty to keep the API value untouched. The top-level `version` string is round-tripped as `version` on the wire — `upload` sends the local value (falling back to the server's current value if locally absent) and writes back whatever the server confirms. `brevo app credentials` additionally backfills a missing top-level `version` / `distribution_type` into cwd's `app-config.json` when its `appId` matches the inspected app — fill-only-when-missing (never overwrites an existing local value), silent in all modes — so legacy projects that are never `upload`ed still converge to the current shape. +- **Commands that pick an app interactively refuse to do so when scripted.** `brevo app credentials`, `brevo app delete`, `brevo app status`, `brevo app submit` and `brevo app withdraw` fall back to an interactive app picker when `--app-id` is absent (the review-lifecycle three try the linked `app-config.json` first). Under `--json` **or** off a TTY that picker is refused up front — before any network call — with a `CliError` naming the exact command to run (`brevo app credentials --app-id `) and exiting `1`. **Always pass `--app-id` when scripting these.** The refusal exists because the picker renders its choice list to stdout, which would otherwise corrupt the single-JSON-document contract below and leak app ids into whatever is parsing it. `brevo app delete` is the one that matters most: it is destructive, so a script that relied on the picker was never doing what its author thought. - **There is no `brevo app update`.** It was removed and replaced by `brevo app upload`, with no shim and no flag-for-flag equivalent — change an app's name, redirect URLs, scopes or logo by editing `app-config.json`, then run `brevo app upload`. That is what to replace it with wherever you find it: a user's script, a CI job, a README, or your own recollection. Invoking it — with any of the old flags (`--name`, `--redirect-uri`, `--scope`, `--logo-uri`, `--app-id`), with `--help`, or as `brevo app help update` — prints a message naming `upload` and exits `1` **without uploading anything**, so a `1` from `brevo app update` means the command is gone, not that an upload failed. It is absent from every help screen, and needs no login to reach. - **Credentials** live at `~/.brevo/credentials.json`. Never commit this file or any `.env.local`. - **Non-interactive auth:** `BREVO_API_KEY=xkeysib-... brevo login`. The legacy `--api-key` flag was removed because it leaks into shell history. -- **Skip prompts:** `--force` for `app delete`, `app install`, `app uninstall` and `logout`; `--yes` for `app upload`. `app delete --force` still prints the install-loss warning line (kept out of `--json` output, which stays parseable JSON only). `app upload --yes` behaves the same way for a UI app: it skips the confirmation but still prints the line warning that the app may already be installed. +- **Skip prompts:** `--force` for `app delete`, `app install`, `app uninstall`, `app withdraw` and `logout`; `--yes` for `app upload`. `app delete --force` still prints the install-loss warning line (kept out of `--json` output, which stays parseable JSON only). `app upload --yes` behaves the same way for a UI app: it skips the confirmation but still prints the line warning that the app may already be installed. - **Forced update:** when the installed CLI is a full **major** version behind the latest npm release, every command except `--help`/`--version` prints a blocking update banner to stderr and exits `1` without running. Update with `npm install -g @getbrevo/cli` (or `yarn global add`). The gate honors the same opt-outs as the soft update notice (`BREVO_NO_UPDATE_NOTIFIER=1`, `--no-update-notifier`, CI, non-TTY), so it never fires in those contexts. - **Update notice wording:** the update/force-update banners take their first line from the app-store service (`GET /cli/info`). It is called directly, not through the v3 API gateway, and needs no API key — so it works while logged out or with expired credentials. It runs once per invocation, **before** the command, and the response is cached at `~/.brevo/cli-info-cache.json` for **15 minutes**, keyed to the installed `cliVersion` — so reworded text or a new block reaches the CLI within minutes rather than after the old 12h npm-style TTL. Whether an update banner appears is still decided from the npm registry, and if the call fails the banner still appears with local wording (a failed call never overwrites a good cache entry). - **Soft update notice on failures:** the non-blocking update banner also prints after a command *fails*, not just after it succeeds — so stderr may hold the error message followed by the update box. Exit codes are unchanged (still the command's own), the box never appears twice in one run, and a Ctrl-C abort skips it. Parse stderr accordingly, or set `BREVO_NO_UPDATE_NOTIFIER=1` / pass `--no-update-notifier`. @@ -126,7 +131,20 @@ Under `--json`, a failing command writes **one** JSON document to stdout describ | `statusCode` | `ApiError` only | HTTP status behind the failure | | `code` | `ApiError`, when classified | `AUTH_INVALID`, `AUTH_EXPIRED`, `ACCESS_DENIED`, `APP_NOT_FOUND`, `REDIRECT_INVALID`, `PORT_IN_USE`, `NETWORK_ERROR`, `RATE_LIMITED`, `APP_LIMIT_REACHED`, `REGISTRY_ERROR`, `AUTH_GATEWAY` | -Stdout is always **exactly one** parseable document. Commands that already describe their own failure keep their shape rather than emitting this envelope — `brevo whoami --json` returns `{"authenticated": false, "reason": "no_key"}` (exit `1`). Check for `error` first, then fall back to the command's own shape. +Stdout is always **exactly one** parseable document. Commands that already describe their own outcome keep their shape rather than emitting this envelope — `brevo whoami --json` returns `{"authenticated": false, "reason": "no_key"}` (exit `1`), and `brevo app withdraw --json` on a never-submitted app returns `{"withdrawn": false, "reason": "NOT_SUBMITTED", …}` at exit **`0`**. Don't read a `false` in a command's own payload as a failure — `withdraw`'s is a normal outcome. Check for `error` first, then fall back to the command's own shape. + +## Publication & review + +Only a **public** OAuth app goes through review. A private app is usable by the creating organisation immediately, and a UI app goes live through `brevo app upload` → `brevo app install` with no review step at all. + +1. `brevo app create --distribution public …` — immutable from here; a private app cannot be promoted. **Brevo currently refuses public creates from the CLI**: the flag is accepted, the request is sent, and the platform answers `400` — reported as *"Public apps can't be created from the CLI yet — Brevo rejected this request"* with the server's text quoted under `Brevo said:`. Verified against production. It is a server-side policy keyed on the caller being the CLI, not an account permission and not a CLI gate, so there is nothing to retry or request. Offer `--distribution private` and say the review lifecycle is unreachable until the platform changes. +2. `brevo app upload` — writes the `version` the app needs to be reviewable. +3. `brevo app submit` — opens the review form after its five checks (see the command table). +4. **The user completes the form in the browser.** This is the step that submits. +5. `brevo app status` — poll for `approved` / `rejected` / `changes_requested`. +6. `brevo app withdraw` — pull a submission back. + +**`brevo app submit` exiting `0` does not mean the app was submitted.** It is a form hand-off, not a state transition: the CLI opens a Google Form and changes nothing server-side. Report that the form is open and what the user must still do — never "submitted for review". Reviewer feedback arrives by email (BEX-252), so `rejected` and `changes_requested` carry no reason in `status`; don't invent one and don't send the user looking for it there. ## Command help diff --git a/agent-context/SKILL.md b/agent-context/SKILL.md index 5dd6cbf7..4961a0a8 100644 --- a/agent-context/SKILL.md +++ b/agent-context/SKILL.md @@ -9,15 +9,17 @@ This project uses the Brevo Developer CLI to create and manage Brevo OAuth appli ## `brevo --help` is the source of truth -Brevo features that haven't been released are **not built into the published CLI at all** — not as hidden commands, not behind a flag or an env var. `brevo --help` and `brevo app --help` list everything the binary can do. Treat that as the complete surface and build from it. +`brevo --help` and `brevo app --help` list everything the installed binary can do. Treat that as the complete surface and build from it — **the version in front of you, not the version you remember.** + +Every command this file documents ships in every published build. Nothing is held back behind a flag, an env var, or an account setting. So when a documented command is missing, there is exactly one explanation left: **the user's install is older than this file.** (It was not always so. Public app distribution and the review lifecycle were eliminated from published builds until they went GA, and this section used to exist to stop agents recommending them. That gate is gone.) What this means in practice: -- **If a command isn't in `--help`, it doesn't exist here.** Invoking it gives Commander's `unknown command` and exit `1`. That is a final answer, not a transient failure and not a permissions problem — there is no flag, config edit, account setting, or environment variable that reveals it. Don't retry, don't hunt for an alternative route, and don't tell the user to get access. Say the feature isn't available in this CLI and offer the nearest thing that works. -- **Don't act on a command you remember rather than one you can see.** Your recollection of the Brevo CLI may include commands from a newer or unreleased build. `--help` in the current session is the only reliable source. -- **A flag can be rejected even when its command exists.** `--distribution` is a real flag, but a value tied to an unreleased feature is refused with *"That command is not available yet. It is part of a Brevo feature that has not been released."* Same rule: it's final. Use `--distribution private`. +- **Don't act on a command you remember rather than one you can see.** Your recollection may be a newer release than the user has, or an older one. `--help` in the current session is the only reliable source. +- **`unknown command` plus exit `1` means "upgrade", not "retry".** Don't hunt for an alternative route and don't tell the user to request access — there is no access to request. Check `brevo --version` against the skill version (see *Before starting a new session*), and if they're behind, say so and point at `npm install -g @getbrevo/cli`. +- **A flag value can be refused even when its command exists.** `--distribution public` on an install that predates public-apps GA is refused with *"That command is not available yet. It is part of a Brevo feature that has not been released."* — a flag has to parse before it can be rejected, so this is what an older binary says instead of `unknown option`. It reads like a permissions error and is not one: it means upgrade. Same for the *UI app* choice, absent from the app-type prompt on an install predating BEX-290. -The Brevo API enforces the same boundaries independently, so nothing is gained by trying to route around the CLI — a hand-rolled `curl` hits the platform's own refusal. +The Brevo API enforces its own boundaries independently, so nothing is gained by routing around the CLI — a hand-rolled `curl` hits the platform's own refusal. > **Reading this from the repo rather than `~/.claude/skills/brevo-cli/`?** > @@ -52,17 +54,20 @@ Don't fall back to raw HTTP against `api.brevo.com` — the `brevo` binary is th - "Authenticate" → `brevo login` (or `BREVO_API_KEY=xkeysib-... brevo login` for CI) - "Who am I logged in as?" → `brevo whoami --json` - "Show / pick an app" → `brevo app list --json` -- "Create an app" → `brevo app create --name "" --distribution private --redirect-uri --json` (add `--logo-uri ` to set the app logo at creation time; new apps default to scopes `contacts:read`, `contacts:write`, `crm:read`, `crm:write`). **Use `--distribution private`** — check `brevo app create --help` for the values your account accepts, and don't pass one it doesn't list. **Fails immediately if run from a directory that already has `app-config.json`** — `cd` elsewhere first, or use `brevo app scaffold` in that directory instead. Otherwise resolves (creates/`cd`s into) its target directory, creates the app, and writes the **basic project structure** (`app-config.json` + `.gitignore`/`AGENTS.md`/`CLAUDE.md`/`README.md`). It scaffolds a feature (the OAuth test server) **only** when the interactive confirm (*"Scaffold the Test OAuth App?"*, default yes) is answered yes — there is no separate "which feature?" question while the CLI ships one; **non-interactive runs (`--json` or piped) stay base-only** — run `brevo app scaffold` afterward to add the OAuth code. Under `--json`, the response's `directory` field is where it landed and `scaffolded` is the base file count; check for `scaffoldSkipped` instead of `scaffolded` if that directory already existed (both directory setup and scaffolding are skipped together in that case, but the app is still created). +- "Create an app" → `brevo app create --name "" --distribution --redirect-uri --json` (add `--logo-uri ` to set the app logo at creation time; new apps default to scopes `contacts:read`, `contacts:write`, `crm:read`, `crm:write`). **`private` for apps used exclusively by the user's own organisation, `public` for apps distributed to end users or marketplace listings; default to `private` when the user hasn't said which.** `distribution_type` is **immutable after create** — `brevo app upload` refuses to change it, so getting this wrong means creating a new app. Only a `public` app can be submitted for review (see *Publication & review*). **Fails immediately if run from a directory that already has `app-config.json`** — `cd` elsewhere first, or use `brevo app scaffold` in that directory instead. Otherwise resolves (creates/`cd`s into) its target directory, creates the app, and writes the **basic project structure** (`app-config.json` + `.gitignore`/`AGENTS.md`/`CLAUDE.md`/`README.md`). It scaffolds a feature (the OAuth test server) **only** when the interactive confirm (*"Scaffold the Test OAuth App?"*, default yes) is answered yes — there is no separate "which feature?" question while the CLI ships one; **non-interactive runs (`--json` or piped) stay base-only** — run `brevo app scaffold` afterward to add the OAuth code. Under `--json`, the response's `directory` field is where it landed and `scaffolded` is the base file count; check for `scaffoldSkipped` instead of `scaffolded` if that directory already existed (both directory setup and scaffolding are skipped together in that case, but the app is still created). - "Create a UI app" (an action link that renders inside Brevo CRM records) → run `brevo app create` **interactively** and pick *UI app* at the *"What type of app are you building?"* prompt, **or non-interactively** with `--ui-app --record-page --placement --label --url [--more-info ]`, or `--ui-config ` (a JSON file: `{ extension_type, record_page, surface_point_name, label, more_info?, redirect_link }`). There is still **no `--type` flag** — a plain non-interactive run with neither `--ui-config` nor `--ui-app` creates an OAuth app, exactly as before. Both non-interactive routes only support `extension_type: "actionLink"` today; an unknown `--record-page`/`--placement` fails with the valid options listed in the error. Interactively, the flow asks for the integration type, one record page, one placement on it, a label, optional supporting text, and the destination URL — every route authors exactly **one** placement; add more by hand as further `surface_point_list` entries in `app-config.json` (each with its own `label` and `redirect_link`), then `brevo app upload`. - "Install a UI app into an account" → `brevo app install [account-id] --app-id --force --json`. **UI apps only** — an OAuth app has nothing to install (it becomes usable when a user authorizes it) and the CLI refuses with exit `1`. The `[account-id]` positional is optional: omitted, a plain account installs into itself (no prompt, so `--json`/CI works) and a corporate account picks a sub-account interactively (non-interactive corporate runs must pass it explicitly). The app must have been validated with `brevo app upload` first — installing before an upload is refused locally. Interactively, omitting `--app-id` outside a linked project opens an app picker that lists **only UI apps**; with no UI app to offer, the command errors (exit `1`) naming `brevo app create`. That picker needs a terminal: under `--json` or off a TTY, omitting `--app-id` outside a linked project is refused with exit `1` — so always pass `--app-id` when scripting from an unlinked directory. Before it acts, `install` prints the configuration it will install **as stored on the server** — app ID, name, `version`, extension type and every placement — because that is what the account will render, not whatever the local `app-config.json` now says; under `--json` the same facts come back as `version` and `ui_app` on the result. If the linked project's `ui_app` block has drifted from the stored one, it warns and names `brevo app upload`, then installs anyway (exit `0`) — the stored configuration is a legitimate thing to install, so this is a notice, not a refusal. - "Uninstall a UI app from an account" → `brevo app uninstall [account-id] --app-id --force --json`. Same target resolution as `install`. Uninstalling an app that isn't installed is **informational, exit `0`** — not an error. - "Update app metadata" → edit the relevant field(s) in `app-config.json` (`appName`, `auth.redirectUris`, `auth.scopes`, `logoUri`, `version`) (older projects may still say `auth.redirectUrls` — the CLI reads it and migrates the file to `redirectUris` on its next write), then run `brevo app upload --json` (no `--app-id`/`--name`/`--redirect-uri`/`--scope`/`--logo-uri` flags exist — `upload` always pushes the whole file, resolved only from cwd's `app-config.json`). **`distribution_type` is immutable** — it's set at `app create` time and cannot be changed via `upload`; if the local value differs from the server, `upload` errors and tells you to restore it (create a new app to get a different distribution). For a **UI app**, `upload` is also the way to change what an installed app renders — there is no re-install and no publish step: the diff prints every `ui_app` placement field by field (`before → after`, plus `(new)` / `(removed)` placements), then warns that the app may already be installed in Brevo accounts and asks *"Proceed with upload and update every account this app is installed in?"*. `--yes` skips the question but still prints the warning; `--json` prints neither and stays a single parseable document. **There is no `brevo app update`** — it was removed, with no shim and no flag-for-flag equivalent; if you find it in a user's script, CI job, README, or your own recollection, replace it with the edit-then-`upload` flow above. Invoking it — with any of the old flags, with `--help`, or as `brevo app help update` — prints a message naming `brevo app upload` and exits `1` **without uploading anything**, so a `1` from `brevo app update` means the command is gone, not that an upload failed. - "Get client credentials" → `brevo app credentials --app-id --json` (add `--reveal-secret` to print the secret). **`--app-id` is not optional here** — without it the command wants an interactive app picker, and under `--json`/off a TTY it refuses with exit `1` rather than prompting. **OAuth apps only** — a UI app has no OAuth credentials (no client ID, secret, scopes, or callbacks), so the command refuses it with exit `1` and points at `brevo app list` for the app's type. - "Set up a project for an app that already exists" → `brevo app scaffold --app-id ` (`brevo app list` gives the IDs). Fetches the app and writes `app-config.json` + the base files, then adds a feature as below. Interactively it asks which directory to write into first (default `./`); under `--json`/off a TTY it uses the current one. **This is the only way to get an `app-config.json` for an existing app** — `brevo app create` makes a *new* app, and `brevo app upload` only ever reads the project in the current directory. It refuses if the directory is already linked to a *different* app; pointing it at the app the directory is already linked to is a no-op. **Interactively you can omit `--app-id`**: plain `brevo app scaffold` in a directory with no `app-config.json` says so, asks *"Set up a project for an app you already have?"* (default yes), and on yes lists the account's apps so you can pick one — no need to look the ID up first. **Every interactive bootstrap (picker or `--app-id`) then asks `Output directory:`, defaulted to `./`** — the same question `brevo app create` asks — creates that directory and `cd`s into it, so the files don't land in whatever folder you happened to be standing in; answer `.` to use the current directory instead. It then writes the project (`app-config.json` + the base files), shows what it wrote, and asks *"Scaffold the Test OAuth App?"* (default yes) — declining is normal and leaves the project in place, exit `0`. The *Next steps* box opens with `cd `, because the CLI can only move its own process, never your shell. **Under `--json` or off a TTY there is no directory question and the files go into the current directory**, so scripted `brevo app scaffold --app-id ` runs are unchanged — `mkdir` and `cd` yourself first if you want them somewhere specific. Answering **no** is a normal outcome, not an error: it exits `0` after printing the remaining routes (`brevo app create` here, or `cd` into an existing project). **Always pass `--app-id` when scripting** — the offer needs a terminal, so under `--json` or off a TTY the command errors instead of prompting. **If the directory you point it at already holds a project, the bootstrap is a refresh, not a fresh write**: the config found there is diffed against the server and rewritten only on consent (*"…will update app-config.json to match the server. Continue?"*, default yes) — answering **Merge** at the directory question does not suppress that, because merging keeps the file that exists and so would skip the very file a bootstrap is for. No drift means `app-config.json` is left alone with a one-line notice, and the feature still gets added. A directory holding a project for a **different** app is refused outright, naming both apps. Two refusals apply to both forms, before any write: that different-app case, and the directory must not be **inside** an existing project (a nested second `app-config.json` would make a later `brevo app upload` push the wrong app). -- "Add a feature (e.g. the OAuth test server) to an existing project" → `brevo app scaffold` (run **inside** the project directory; it reads the linked app from `app-config.json`, so `--app-id` is only needed to bootstrap a directory that has none). Not needed right after `app create` if you already accepted the feature prompt there. If feature files already exist it prompts Overwrite / Merge / Cancel (default Merge); pass `--overwrite` to force a full overwrite without prompting. **The scaffolded OAuth flow is the confidential-client flow:** `/auth/callback` authenticates the token exchange with the `CLIENT_SECRET` written into the scaffolded `.env.local`. +- "Add a feature (e.g. the OAuth test server) to an existing project" → `brevo app scaffold` (run **inside** the project directory; it reads the linked app from `app-config.json`, so `--app-id` is only needed to bootstrap a directory that has none). Not needed right after `app create` if you already accepted the feature prompt there. If feature files already exist it prompts Overwrite / Merge / Cancel (default Merge); pass `--overwrite` to force a full overwrite without prompting. **The scaffolded OAuth flow depends on the app's distribution:** a **private** app gets the confidential-client flow — `/auth/callback` authenticates the token exchange with the `CLIENT_SECRET` written into the scaffolded `.env.local`. A **public** app gets Authorization Code + **PKCE** (RFC 7636) instead: `/auth/login` generates a `code_verifier` and sends `code_challenge` + `code_challenge_method=S256`, the exchange and refresh send the verifier with **no client secret**, and the generated `.env.local` / `.env.example` carry none. So don't tell a public-app developer to fetch their client secret — they don't need it and the scaffold won't read it. - "Run the OAuth test server" → `brevo app start oauth --port 3009` (must be inside the scaffolded directory) - "Delete an app" → `brevo app delete --app-id --force`. **`--app-id` is not optional here either** — omitting it means an interactive picker, which under `--json`/off a TTY is refused with exit `1`. Never script a delete without naming the app. `--force` skips the prompt but still prints the install-loss warning line; under `--json` stdout stays JSON only. - "List supported OAuth scopes" → `brevo app available-scopes --json` +- "Check an app's review status" → `brevo app status --app-id --json` (read-only; returns `{ state, message }`, `state` ∈ `draft`/`submitted`/`in_review`/`approved`/`rejected`/`changes_requested`, or `unknown` when the server returns no state. Reviewer feedback comes by email, not here.) `--app-id` is optional — it falls back to the linked `app-config.json`, then to an interactive picker. +- "Submit a public app for review" → `brevo app submit --app-id --json` (prints the submission form URL as `{"app_id","form_url"}` without opening a browser; without `--json` it shows the full app definition, asks for confirmation, then opens the form in the user's browser — the prompt is skipped when stdin is not a TTY). Before any of that it runs a status preflight (the same review-state read as `brevo app status`) and aborts if that read fails. The app's `distribution_type` must be `public`, and when `app-config.json` describes the target app it must match the server — if the command reports drift, either update the local config with the server values or push local changes with `brevo app upload`. **The app is only actually submitted once the Google Form is completed and submitted; the command itself changes nothing server-side** — so exit `0` here does not mean "submitted". See *Publication & review*. +- "Withdraw an app from submission" → `brevo app withdraw --app-id --force` (omit `--app-id` inside a scaffolded project to use the app pinned in `app-config.json`; if the app was never submitted, it prints a hint to submit first and exits `0` — not an error) - "Sign out" → `brevo logout --force` ## Hard rules @@ -74,6 +79,7 @@ Don't fall back to raw HTTP against `api.brevo.com` — the `brevo` binary is th 5. **Prefer flag-driven over interactive** in agent contexts: `--name`, `--app-id`, `--force`, `--yes` so the command doesn't block on prompts. 6. **Write only the `app-config.json` keys this file documents.** `brevo app upload` validates the whole file and rejects keys it doesn't recognise, so an invented block fails at upload rather than doing anything useful. 7. **Never mix the two app types in one `app-config.json`.** The presence of the `ui_app` block is the app-type discriminator: an OAuth app has a populated `auth` block and no `ui_app`; a UI app has a `ui_app` block and an **empty** `auth: {}` (it has no OAuth callback, scopes, or credentials). See *UI apps* below for the block's shape. +8. **Never report a successful `brevo app submit` as "submitted for review".** It opens a form and changes nothing server-side; the submission happens when the user completes that form. Say the form is open and what they must do next. See *Publication & review*. ## UI apps @@ -99,9 +105,62 @@ Install semantics worth knowing: only UI apps install into an account; the app m **An installed UI app tracks the server's configuration, not the account's copy of it.** There is one stored snapshot per app, so `brevo app upload` changes what every account it is installed in renders, immediately and with no re-install — which is why `upload` warns and asks before pushing a UI app, and why `install` shows the stored configuration it is about to make visible. Tell a user to edit `app-config.json` and run `brevo app upload`; never tell them to uninstall and re-install to pick up a change. +## Publication & review + +Only a **public** OAuth app goes through review. A private app is usable by the creating organisation the moment it exists, and a UI app goes live through `upload` → `install` (see *UI apps*) with no review step at all. + +The route, in order: + +1. `brevo app create --distribution public …` — `distribution_type` is **immutable after this point**. A private app cannot be promoted; `brevo app upload` refuses the change and tells you to create a new app. + + **The platform can refuse a public create, and currently does.** The CLI accepts the flag and sends the request; Brevo answers `400` and the CLI reports *"Public apps can't be created from the CLI yet — Brevo rejected this request"*, quoting the server's own text under `Brevo said:`. Verified against production. This is a **server-side policy keyed on the caller being the CLI** — not an account permission, not a CLI gate, and not something a flag, env var or account setting changes. So: don't retry, don't look for a missing scope, and don't tell the user to request access. Report that Brevo is refusing CLI-created public apps, offer `--distribution private`, and note that the review lifecycle below is unreachable until that changes. Read the `Brevo said:` line rather than assuming — it is quoted precisely so a reworded refusal is still visible. +2. `brevo app upload` — the app needs a `version`, which only a successful upload writes. An app that was never uploaded is not reviewable. +3. `brevo app submit` — shows the exact app definition being submitted, asks for confirmation, and opens the review form. +4. **Complete and submit the form in the browser.** This is the step that actually submits. +5. `brevo app status` — poll for the outcome. Reviewer feedback arrives **by email**, never in this output. +6. `brevo app withdraw` — pulls a submission back. + +### `brevo app submit` exiting `0` does not mean the app was submitted + +This is the single sharpest edge here. `submit` is a **form hand-off, not a state transition**: it opens a Google Form (the URL comes back on the app payload) and changes nothing server-side by itself. The CLI says so — *"Your app will be submitted for review only after you complete and submit the Google Form"* — and then *"You'll receive an email once your app has been reviewed."* + +So never report a successful `submit` as "submitted for review". Tell the user the form is open and they must complete it. Under `--json` the form URL comes back as `{"app_id","form_url"}` on stdout while those two notes go to **stderr**, so stdout stays one parseable document. + +Running `submit` twice is safe: the second call either returns the same `form_url` and exit `0`, or is refused with *"Review submission is currently unavailable"* and exit `1` — which is also what you get for an app already under review. Neither means the first submission was lost; check `brevo app status`. + +### The five refusals, in the order they fire + +`submit` does its checks before opening anything, so read the first failure you get — later ones may also be true. + +1. **The app has never been uploaded** — *"App `` has never been uploaded, so it has no version to review."* An app's `version` is written only by a successful `brevo app upload`, and the review state lives on that version. Run `brevo app upload`, then re-run `submit`. This is checked locally, before any review-state read. +2. **The review-state read fails** (network, auth, unknown app) → aborts. `submit` runs the same read `brevo app status` does, deliberately, so a broken read never becomes a half-submission. +3. **The app isn't ready** — the state API reports `submittable: false` plus `missing_fields`. The CLI prints the server's own field keys verbatim (e.g. `logoLink`, `oauth.scopes`) with **no relabelling**, so what you see is what the API calls them. Fix by editing `app-config.json` and running `brevo app upload`, then re-run `submit`. Note this fires *before* the public check, so an incomplete private app reports its missing fields first. +4. **The app is private** — *"Private apps cannot be submitted for review."* There is no fix but a new app: distribution is immutable. +5. **Local `app-config.json` has drifted from the server** — refused with a field-by-field diff tagging each value `(local only)` or `(server only)`. Two ways out, and the CLI names both: bring the local file in line with the server, or push the local state with `brevo app upload`. The check only runs when the local config describes *this* app — a different `--app-id` makes it irrelevant, not an error, since `submit` never writes locally. + +### Review states + +`brevo app status --json` returns `{ state, message }`. `state` is one of: + +| state | meaning | +|---|---| +| `draft` | set up, not yet submitted | +| `submitted` | submitted, waiting to be picked up | +| `in_review` | being reviewed | +| `approved` | approved | +| `rejected` | not approved — details by email | +| `changes_requested` | changes needed — details by email | +| `unknown` | the server returned no state (normalised sentinel) | + +`rejected` and `changes_requested` carry **no reason in the CLI** (BEX-252) — don't invent one, and don't tell the user to look for it in `status`. Point them at their email. An unrecognised value renders as *"Your app is in state \"\""*, so a state the platform adds later still reads cleanly rather than erroring. + +### Withdrawing + +`brevo app withdraw --app-id --force` pulls a submission back; `--json` returns `{ withdrawn: true, appId }`. Withdrawing an app that was **never submitted** is informational, not a failure: exit `0`, and under `--json` `{ withdrawn: false, reason: "NOT_SUBMITTED", … }` plus the command to submit it. Treat a `withdrawn: false` with that reason as "nothing to do", not an error to retry. + ## Locating the linked app -If `app-config.json` exists in the working directory, it pins the app — `brevo app upload` and `brevo app start` use it automatically. `brevo app start` accepts an `--app-id` override to target a different app; `upload` does **not** — it only ever reads cwd's `app-config.json`, hard-erroring if that file is missing, invalid, or lacks `appId`. +If `app-config.json` exists in the working directory, it pins the app — `brevo app upload`, `brevo app start`, `brevo app status`, `brevo app submit` and `brevo app withdraw` use it automatically. All of those except `upload` accept an `--app-id` override to target a different app; `upload` does **not** — it only ever reads cwd's `app-config.json`, hard-erroring if that file is missing, invalid, or lacks `appId`. `app-config.json` carries an optional top-level `logoUri` string. When set, `brevo app upload` pushes it as `logo_uri`; when empty / absent, the field is left untouched on the API. @@ -144,7 +203,7 @@ Under `--json`, a command that fails writes **one** JSON document to stdout desc { "error": { "name": "ApiError", "message": "App not found", "exitCode": 5, "code": "APP_NOT_FOUND", "statusCode": 404 } } ``` -Two things to rely on: stdout is always **exactly one** parseable document, and commands that describe their own failure keep doing so instead of emitting this envelope — `brevo whoami --json` still returns `{"authenticated": false, "reason": "no_key"}` (exit `1`). Check for `error` first, then fall back to the command's own shape. +Two things to rely on: stdout is always **exactly one** parseable document, and commands that describe their own outcome keep doing so instead of emitting this envelope — `brevo whoami --json` still returns `{"authenticated": false, "reason": "no_key"}` (exit `1`), and `brevo app withdraw --json` on a never-submitted app returns `{"withdrawn": false, "reason": "NOT_SUBMITTED", …}` at exit **`0`**. Check for `error` first, then fall back to the command's own shape — and don't read a `false` in a command's own payload as a failure, since `withdraw`'s is a normal outcome. ## Command help diff --git a/docs.md b/docs.md new file mode 100644 index 00000000..60de7497 --- /dev/null +++ b/docs.md @@ -0,0 +1,177 @@ +# Public apps — outstanding work (post-GA) + +**Branch-local — never merge into `main`** (see `CLAUDE.md`). Not in `package.json` +`files:`, so nothing here ships to npm — but the never-merge rule applies regardless, +because branches are public too. + +**Public apps went GA in this branch.** The release copy that used to sit in *Part 1* has +been consumed into `.changeset/fix-app-submit-missing-fields.md`, and the GA runbook +(`RELEASE-CHECKLIST.md`) and the consolidated status view (`PUBLIC-APPS-RELEASE-STATUS.md`) +were worked through and deleted. What remains is this: the open-questions log. + +When an item here resolves, delete it. When it turns into a release step, it needs a +runbook again — recreate one rather than growing this file into one. + +--- + +## ⚠️ BLOCKER — the platform still refuses CLI-created public apps + +**Verified against production on 2026-09-02**, authenticated, no `BREVO_API_URL` +override, on a build of this branch: + +``` +$ brevo app create --name "…" --distribution public --json +{"error":{"name":"CliError","message":"Public apps can't be created from the CLI yet … + Brevo said: public apps cannot be created with source \"cli\"; use distribution_type \"private\""}} +``` + +The CLI side is open and correct — the flag parses, validates and is sent. The +refusal is the **platform's**, and it is keyed on the caller being the CLI, not on an +account flag: the CLI stopped sending `source: 'cli'` (see the BEX-355 item below), so +the backend is deriving it from the `User-Agent` and applying the policy regardless of +the body. That makes it global rather than per-account, and nothing client-side can +change it. + +Consequence: **the review lifecycle is unreachable end to end.** `app submit` / +`app status` / `app withdraw` all ship and all work, but there is no way to obtain a +public app to use them on. The CLI degrades gracefully — `APP_CREATE_PUBLIC_REJECTED` +maps the 400 to actionable copy and quotes the server verbatim — and both agent docs now +tell an agent to read that line and not retry. + +- [ ] **Decide what ships.** Two options, and this is a product call, not a code one: + - **Land as-is.** The CLI is complete and lights up the moment the backend policy + lifts, with no further release. Cost: a documented flag returns a server `400`, + so users meet the refusal rather than the feature. + - **Hold the flip.** Re-gate `public-distribution` and `review-lifecycle`, keeping + everything else (strings and constants in `en.ts` / `constants.ts`, docs, smoke, + the TC-6.3 fix). **This got more expensive and the price should be in the + decision:** the gate machinery was torn down after GA (see *Closed* below), so + this now means rebuilding it from the recipe in `CLAUDE.md` → *If you ever need to + gate a feature again* — the build flag, the `define` block, the bundle assertions + — on top of the work it always needed, which is the larger half: moving the three + command definitions into a gated module, the review-lifecycle strings into a gated + messages module, and re-adding the help/prompt wrappers. Those three modules were + deleted at GA itself, before the teardown, so the teardown adds to the bill rather + than creating it. Still a day's work either way, and a revert of the GA commit is + probably the cheaper route to the same place. +- [ ] **Get the backend policy lifted (BEX-355).** This is the real unblock. The + `source "cli"` policy needs to allow public creates from the CLI, or expose a + per-account allowance the CLI can be granted. Until then, GA is CLI-side only. + +## Release gates + +- [ ] **`smoke-post-merge.yml` does not exercise the review lifecycle.** It stays pinned at + `suite: private,ui` against the published package, so no publish gate touches + `app submit` / `app status` / `app withdraw`. `smoke-pre-merge.yml` covers them via + `suite: all`, but it is `non_blocking` and runs `against=local`. Widening the + post-merge lane needs `scripts/smoke/public-app.ts` proven headless on + `ubuntu-latest` first (CLAUDE.md is explicit that a suite only ever run on a dev + machine has not been proven headless) — run it from `smoke.yml`'s manual button, + then decide. + +## TC-6.3 — half fixed + +The `submit` half is **closed**: an app with no `version` has never been uploaded and +cannot have a review state, so `submitCommand` now fetches the app first and refuses +locally with `APP_SUBMIT_NOT_UPLOADED`, before the review-state read that produced the +misleading copy. Same gate `app install` already applies (`assertInstallable`'s +`requireUploaded`). Covered in `__tests__/commands/app/submit.test.ts`. + +- [ ] **`brevo app status` still relays the raw server message** on a never-uploaded app. + It reads the review state directly and never fetches the app, so the local `version` + signal isn't available without an extra round trip on a read-only command's happy + path. Closing it properly means mapping the failure in `apiCodeMessages` + (`src/api/client.ts`), which needs **the server's error `code` and HTTP status + captured from a live repro** — neither is recorded anywhere today, and TC-6.3 never + captured the exit code either. Get those two values, then add one line to the map. + The server's copy names `name`, `logo_uri`, `scopes` and `redirect_uris` as the + fields to fix; all four can be present, and the real cause is the absent + `app_versions` row. + +## Wire contracts / sign-offs still open + +- [ ] **BEX-355 sign-off that an absent `source` is contract-valid.** The CLI stopped sending + `source: 'cli'` after the platform started reading it as policy (`400 + invalid_parameter`, *public apps cannot be created with source "cli"*). The backend + derives the caller from the `User-Agent` header instead. **Staging accepts the omission** + — a private create with no `source` and no `cli_version` returned `201` (2026-08-12) — + but that only proves it is not *rejected*. Still needs the owners to confirm it does + not change attribution, rate-limiting or gating. +- [ ] **The app-read responses disagree on shape, and the CLI absorbs it.** Confirmed on + staging 2026-08-12: `POST /apps` and `POST /apps/{id}/upload` return OAuth fields + **nested** under `auth`, while `GET /apps/{id}` returns them **flat** (`client_id`, + `redirect_uris` at the top level). The CLI copes — `flattenCreateAuth` tolerates both + on create, and the read path expects flat — so nothing is broken. But it is one + resource described two ways, which is how the original nesting regression hid as long + as it did. Worth raising on BEX-355 rather than leaving each new consumer to + rediscover it. +- [ ] **BEX-350 coordinated release.** UI kit + reseeded registry + backend must land + together in every target environment. The schema spec is verified; the + per-environment data is not. +- [ ] **BEX-437 (bo-be, Backlog)** — UI-app authoring is still coupled to the + `app-store-bo-be-public-apps` feature toggle (`gateUIApp` 403s un-flagged accounts, + surfaced as `ERR_UI_APP_NOT_ENABLED`). Decoupling it removes an accidental dependency + between the two releases. + +## QA gaps + +`QA-TESTCASES.md` at this branch's root carries the public-app suites. The recorded +2026-08-13 results predate both the install/uninstall rename and this GA change. + +- [ ] **Suites 5 (TC-5.13–5.16) and 7 (withdraw) are BLOCKED, not just unrun** — they need + an app in `submitted`/`in_review`, which the CLI cannot produce, because `submit` + only opens a form. Needs the form completed or the state set server-side. Same + blocker for TC-6.2's review states. This is a property of the design, not an + oversight: while submission is a form hand-off, no CLI-only test can reach those + states. +- [ ] **TC-2.4 refusal path untested** — needs an account **without** + `app-store-bo-be-public-apps` (mutually exclusive with TC-2.1's account). +- [ ] **Unrun:** TC-2.2 (interactive Public choice), TC-2.3 (list), TC-6.2 (state→tone map), + TC-6.4 (`--json`), TC-6.6 (NO_COLOR/FORCE_COLOR), TC-13.4's mismatch branch. +- [ ] **No `--json` / non-TTY path has been run for any public-app suite.** +- [ ] **Re-baseline every public suite against the shipped build.** The recorded results + were all taken on `PREVIEW=1` artifacts, which no longer exist — that build differed + from the published one by a single unreachable byte, and the gate that produced it + has been torn down. "Needs a preview build" is not a precondition anywhere any more; + re-run the sweep once on `yarn link:dev` to say so with evidence. + +**Closed:** the PKCE expectation. `src/templates/index.ts` really does branch on the +`public` / `private` template flag, `.env.example.tmpl` omits `CLIENT_SECRET` and notes +PKCE for a public app, and `__tests__/templates/handler.test.ts` covers the +*public (PKCE, no secret)* variant. The expectation was not stale. + +**Closed:** `yarn smoke --against=local` building the wrong artifact. `stepReinstall` no +longer forks on the selected suites — there is one build, so every local run is the +published surface and the public suite exercises what npm ships. + +**Closed: the gate machinery is torn down.** It was kept all-`'ga'` for one release after +GA to keep that diff reviewable; this closes it out. Deleted: `src/lib/preview.ts`, +`src/globals.d.ts`, the esbuild `define` block and the `LEAK_MARKERS` / `LEAK_STRINGS` / +`orphanedPreviewMessageKeys` checks in `scripts/build.mjs`, the `build:preview` script, +`previewFeatureOf` and the registry's gate branch, `help.ts`'s `gatedSection` / +`distributionValues` / `createDescription`, both `isFeatureAvailable` calls and the +`assertFeatureAvailable` in `app create`, `messages.PREVIEW_FEATURE_UNAVAILABLE`, and the +`preview.test.ts` / `preview-gate.test.ts` suites. The vestigial `messages` / +`CLI` spread-in wrappers (`coreMessages`, `coreCli`) collapsed too. + +Three things were deliberately **kept**, and each has a note saying so where it lives: +`CommandDefinition.requires` (a `Capability`, not gate config), `jest.setup.js` and its +`setupFiles` entry (its `BREVO_*` env scrub is unrelated and load-bearing), and esbuild +itself (it is the build now). The `.github/workflows/` comments that mention the historical +`PREVIEW=1` build were left alone — nothing functional reads them, and editing a workflow +file needs a PAT this repo's OAuth-App credentials cannot substitute for. + +**The argument against teardown was that `preview.ts`'s header was the only record of the +two traps.** It is now `CLAUDE.md` → *If you ever need to gate a feature again*, expanded +with the `define`-as-bare-global reasoning from `globals.d.ts` and the object-literal rule +from `build.mjs`. That the note needed moving somewhere load-bearing was itself the +evidence: `globals.d.ts` had been telling readers to "use `PREVIEW_BUILD` from +`lib/build-flags`" for two releases after that module was deleted, and nobody noticed. +Two other facts settled it — the gate was not, as its own header claimed, "a no-op that +every build folds away" (`isFeatureAvailable`, `assertFeatureAvailable`, +`previewFeatureOf`, `FEATURE_STAGE` and the unreachable *"That command is not available +yet"* string were all still in the published bundle, the last one readable via `strings`), +and `PREVIEW=1 yarn build` differed from `yarn build` by exactly one byte (`||!1` vs +`||!0`, inside a disjunction whose left operand was already always true) — i.e. the +`build:preview` script produced a behaviourally identical artifact, which the teardown +brief itself called worse than no script. diff --git a/jest.setup.js b/jest.setup.js index eea32e6f..24b8fbce 100644 --- a/jest.setup.js +++ b/jest.setup.js @@ -1,25 +1,11 @@ -/** - * Define the build-time globals that esbuild substitutes in a real build (BEX-405). - * - * `__BREVO_PREVIEW__` does not exist under jest — nothing is bundled, so nothing is - * substituted — and every module that reads it would throw `ReferenceError` on import. - * Defining it here gives the suite a single, explicit build state to run against. - * - * **It is `true`, i.e. the preview build.** The ~80 tests covering `app deploy`, - * `app rollback`, `app submit`, `app status`, `app withdraw`, UI-app creation and - * `--distribution public` are tests of those features, not of the gate; running them - * against a public build would mean asserting that five commands don't exist. The gate - * itself is covered separately in `src/__tests__/lib/preview.test.ts` and - * `preview-gate.test.ts`, which flip this global and re-import through - * `jest.isolateModules` so both build states are exercised in one run — that is the - * whole reason the flag is read through a global rather than baked by the bundler - * alone. - */ -globalThis.__BREVO_PREVIEW__ = true; - /** * Scrub every ambient `BREVO_*` variable out of the environment. * + * This file's *only* job. It also used to define `__BREVO_PREVIEW__`, the pre-GA gate's + * build-time global, which is gone with the gate (BEX-405) — the suite now runs against + * the one artifact that exists. **Do not delete the file with it:** the scrub below is + * unrelated and load-bearing, and so is its `setupFiles` entry in `jest.config.js`. + * * The CLI reads its own configuration from `process.env` — `BREVO_API_KEY` (which * `getApiKey()`/`getAuthCred()` return *before* ever touching the credentials file), * `BREVO_API_URL`, `BREVO_CONFIG_HOME`, `BREVO_DEBUG`, `BREVO_OAUTH_PROXY_URL`, diff --git a/package.json b/package.json index ce2fda77..af774c5f 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,6 @@ }, "scripts": { "build": "tsc --noEmit && node scripts/build.mjs", - "build:preview": "PREVIEW=1 yarn build", "dev": "tsc --noEmit --watch", "unlink": "yarn unlink", "link:dev": "yarn build && yarn link", diff --git a/scripts/build.mjs b/scripts/build.mjs index 9ca28686..76429c66 100644 --- a/scripts/build.mjs +++ b/scripts/build.mjs @@ -1,16 +1,14 @@ /** - * Build the CLI (BEX-405). + * Build the CLI. * - * esbuild rather than `tsc` for one reason: the pre-GA command surface has to be - * *absent* from the published package, not merely unreachable. `tsc` does no dead-code - * elimination, so a `if (PREVIEW_BUILD)` guard would still emit every gated command - * into `dist/`. esbuild folds the flag to a literal, drops the dead branch, and then - * tree-shakes the handler modules that only the dead branch referenced. - * - * `PREVIEW=1` opts into a full-surface build for local testing (`PREVIEW=1 yarn - * link:dev`). The default is a gated build, so `prepublishOnly` cannot accidentally - * publish the preview surface — the safe value is the one you get by not thinking - * about it. + * esbuild rather than `tsc`. It was adopted for the pre-GA gate, which needed unreleased + * commands *absent* from the published package rather than merely unreachable — `tsc` + * does no dead-code elimination. **That gate is gone (BEX-405, torn down after GA), and + * esbuild stays**: it is the build now, and reverting to `tsc` would change the published + * layout (`dist/bin/index.js` as a single-file entry, `dist/bin/files`, `sideEffects: + * false`) for no gain. If a feature ever has to be held back from a published build + * again, read `CLAUDE.md` → *If you ever need to gate a feature again* first — the + * mechanism and both of its traps are written down there. * * ## Two things here are load-bearing and easy to break * @@ -38,7 +36,6 @@ import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; const root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); -const preview = process.env.PREVIEW === '1' || process.env.PREVIEW === 'true'; const outfile = path.join(root, 'dist/bin/index.js'); fs.rmSync(path.join(root, 'dist'), { recursive: true, force: true }); @@ -52,42 +49,29 @@ await esbuild.build({ format: 'cjs', packages: 'external', sourcemap: true, - // The map ships — `package.json` `files:` publishes the whole of `dist/` — so its - // contents are as public as the bundle's. With `sourcesContent` on, esbuild embeds the - // untouched TypeScript of every surviving module, comments included, which quietly - // undid the scrubbing `minifyWhitespace` performs below: `brevo app submit` was gone - // from `index.js` and one grep away in `index.js.map`. Off, the map still resolves a - // stack trace to `src/lib/foo.ts:123` (that is `sources` + `mappings`, not the text); - // a maintainer who wants the source alongside it has the repo, which is public. + // The map ships — `package.json` `files:` publishes the whole of `dist/` — so every + // byte of it is downloaded by every install. With `sourcesContent` on, esbuild embeds + // the untouched TypeScript of every module, comments included, roughly tripling the + // map for nothing: off, it still resolves a stack trace to `src/lib/foo.ts:123` (that + // is `sources` + `mappings`, not the text), and a maintainer who wants the source + // alongside it has the repo, which is public. sourcesContent: false, legalComments: 'none', // No `metafile`: nothing reads one. `logLevel: 'info'` is what prints the per-output - // sizes this build reports, and the gate's assertions below read the emitted file - // rather than esbuild's own accounting — deliberately, see the comment there. + // sizes this build reports, and the assertion below reads the emitted file rather than + // esbuild's own accounting — deliberately, see the comment there. logLevel: 'info', - // `minifySyntax` is what actually performs the elimination: without it esbuild - // substitutes the define but leaves `...false ? previewAppCommands : []` standing, - // which is still a live reference and keeps every gated module in the bundle. - // Folding the ternary is what makes the branch unreachable and the modules - // droppable. - // - // `minifyWhitespace` is not cosmetic either — esbuild preserves comments in - // unminified output, and the comments around the gated code name the commands they - // guard. Stripping them keeps the public bundle free of the surface in prose as well - // as in code. + // `minifySyntax` is what enables the dead-code elimination and tree-shaking that + // choosing a bundler was for; `minifyWhitespace` drops the comments esbuild otherwise + // preserves verbatim. Together they are most of the difference between the bundle and + // a concatenation of the sources. // - // `minifyIdentifiers` stays OFF: mangled names would make a user's stack trace - // useless in bug reports, and it buys nothing here. The sourcemap covers the rest. + // `minifyIdentifiers` stays OFF: mangled names would make a user's stack trace useless + // in bug reports, and it buys little here. It is also what keeps `GA_MARKERS` below + // able to look for a binding by its real name. minifySyntax: true, minifyWhitespace: true, minifyIdentifiers: false, - // Substituted before parsing, at every use site. A bare global rather than the - // exported `PREVIEW_BUILD` constant because esbuild folds a constant only inside its - // declaring module — an importer would still emit a runtime ternary and keep the - // dead branch's imports alive. See src/globals.d.ts. - define: { - __BREVO_PREVIEW__: preview ? 'true' : 'false', - }, }); fs.cpSync(path.join(root, 'src/templates/files'), path.join(root, 'dist/bin/files'), { @@ -107,173 +91,37 @@ fs.cpSync(path.join(root, 'src/templates/files'), path.join(root, 'dist/bin/file const builtMode = fs.statSync(outfile).mode; fs.chmodSync(outfile, builtMode | ((builtMode & 0o444) >> 2)); -// Fail the build rather than publish a gated package that still carries the surface. -// The check is deliberately on the OUTPUT, not on the config: a define typo, a stray -// static import, or a future refactor that makes a gated module reachable would all -// leave the config looking correct while the bundle quietly regained the commands. -// Markers are top-level bindings that exist ONLY inside gated modules, so finding one -// means that module survived. `minifyIdentifiers` is off, so these names appear -// verbatim if the module does. -// -// WHAT THIS CANNOT CATCH, and why it is not a bug in the list: esbuild cannot prune -// individual properties from an object literal, so anything reached as `OBJECT.KEY` -// survives at zero references. One such object still carries a gated name in a public -// build — the `withdrawApp` method on the `appService` literal (`services/app.ts`). It -// is inert: no command reaches it and no help lists it. (`CLI.APP_INSTALL`/ -// `APP_UNINSTALL`, the `/installs` endpoint and the `installApp`/`uninstallApp` methods -// used to be residue too; they became live surface at UI-apps GA.) -// -// `lang/en.ts` had the same problem and was fixed by moving the gated strings into -// `lang/preview-messages.ts` and spreading that in behind the build flag. `CLI` and -// `ENDPOINTS` carried the same residue — `brevo app submit --app-id `, `brevo app -// withdraw --app-id `, `brevo app status` and the `/withdraw` and `/state` paths -// were all readable via `strings` on the published binary — and have now had the same -// treatment (`lib/preview-constants.ts`), which is why `previewCli` and -// `previewEndpoints` are markers below. `appService` is the remaining case; the same -// treatment would work for it if the residue ever matters. Tracked in the GA runbook -// (`RELEASE-CHECKLIST.md` on `feature_set-brevo-cli-v2`; see CLAUDE.md → Working docs -// for why it is branch-local). -// -// So: a marker here must name a MODULE-level binding, never an object property, or the -// check fails in a way no amount of correct gating can clear. The one property-level case -// that is NOT inert — a live reader left holding a key whose definition was eliminated — -// is caught by `orphanedPreviewMessageKeys` below, which works the opposite way round: -// it asserts on names that must be ABSENT from a public build's surviving code. -const LEAK_MARKERS = [ - 'previewAppCommands', // commands/preview-definitions.ts - 'submitCommand', // commands/app/submit.ts - 'statusCommand', // commands/app/status.ts - 'withdrawCommand', // commands/app/withdraw.ts - 'previewCli', // lib/preview-constants.ts — the gated `brevo app …` command strings - 'previewEndpoints', // lib/preview-constants.ts — the gated `/withdraw` + `/state` paths -]; - -// What a reader actually sees. LEAK_MARKERS names bindings, which is the right check for -// "did a gated module survive" but says nothing about what `strings dist/bin/index.js` -// prints — and the published tarball is public, so the command names themselves are the -// leak that matters. These stayed readable long after the modules were correctly -// eliminated, because they arrived as properties of `CLI` (see the note above); they are -// checkable only now that `lib/preview-constants.ts` makes them genuinely absent. -// -// Substrings, matched verbatim against the bundle. Keep them specific enough not to -// collide with GA copy: `brevo app status` must not match `brevo app start`, and a bare -// path fragment like `/withdraw` would false-positive on unrelated text. -const LEAK_STRINGS = ['brevo app submit', 'brevo app withdraw', 'brevo app status']; - -// Every file the tarball carries, because that is the scope this particular check has -// always claimed: not "what did the bundler emit" but "what can someone read in an -// installed copy". `dist/` ships whole, so the sourcemap and the scaffold templates are -// published artifacts exactly as `index.js` is. Scanning only the bundle is what let the -// map carry the surface the bundle had been cleared of; `sourcesContent: false` removes -// that text, and reading the directory rather than one path is what stops the next file -// we add to `dist/` from repeating it. +// Bindings that must be PRESENT in the bundle. The check is deliberately on the OUTPUT, +// not on the config: a stray refactor that makes one of these modules unreachable — the +// last import moved behind a condition esbuild can fold, a definition moved into a module +// nothing live references — leaves the config looking correct while the bundle quietly +// loses the commands. That is not hypothetical; it is what the pre-GA gate did on purpose, +// twice by accident, and it is why this list is asserted against the emitted file. // -// `LEAK_MARKERS` deliberately stays on the bundle alone: it asks whether a gated MODULE -// survived elimination, which is a fact about the bundle and answerable only there. -function publishedFiles() { - const dist = path.join(root, 'dist'); - return fs - .readdirSync(dist, { recursive: true, encoding: 'utf-8' }) - .map((entry) => path.join(dist, entry)) - .filter((file) => fs.statSync(file).isFile()); -} - -// The mirror image of LEAK_MARKERS, for surface that went GA: bindings that must be -// PRESENT in every build. Without this, only the jest gate suite notices a refactor -// that re-routes an install import behind `__BREVO_PREVIEW__` (or back into -// `preview-definitions.ts`) — the build would silently publish a package with no -// `brevo app install`, against this file's own philosophy of checking the output. -// Same rule as above: module-level bindings only, never object properties. +// **Module-level bindings only, never an object property.** esbuild cannot prune a +// property from an object literal, so anything reached as `OBJECT.KEY` survives at zero +// references and would pass this check without proving anything. `minifyIdentifiers` is +// off, so a surviving module's bindings appear verbatim. const GA_MARKERS = [ 'appInstallCommand', // commands/app/install.ts — GA at BEX-290 'appUninstallCommand', // commands/app/uninstall.ts — GA at BEX-290 'resolveInstallTarget', // commands/app/account-install.ts — GA at BEX-290 + 'submitCommand', // commands/app/submit.ts — GA at BEX-405 + 'statusCommand', // commands/app/status.ts — GA at BEX-405 + 'withdrawCommand', // commands/app/withdraw.ts — GA at BEX-405 ]; -// The INVERSE leak, and the one `LEAK_MARKERS` is structurally blind to: not a gated -// module surviving, but surviving code reading a gated *string*. `messages` spreads -// `previewMessages` in behind `__BREVO_PREVIEW__`, so on a public build the definition is -// gone while `messages.SOME_KEY` at a live call site remains — and reads as `undefined`. -// The failure is silent and awful: `new CliError(undefined)` has `message === ''`, so the -// command exits 1 having printed a bare `✗` with no text. That shipped once, for -// `LEGACY_ALL_SCOPE_DEPRECATED_BLOCK` — a GA string parked in the gated module by BEX-405 -// and read by `app upload`, which is in every build. -// -// Checked against the key names in the SOURCE rather than a hand-kept list, so the guard -// covers keys added to `preview-messages.ts` later without anyone remembering this file. -// `minifyIdentifiers` is off and property reads keep their names, so a surviving -// `messages.KEY` appears verbatim; the definition cannot, because the module is dropped. -// A hit therefore means exactly one thing: a live reader with no definition. -function orphanedPreviewMessageKeys(bundle) { - const source = fs.readFileSync(path.join(root, 'src/lang/preview-messages.ts'), 'utf-8'); - const keys = [...source.matchAll(/^ {2}([A-Z][A-Z0-9_]*)\s*:/gm)].map((m) => m[1]); - return keys.filter((key) => bundle.includes(key)); -} - const bundle = fs.readFileSync(outfile, 'utf-8'); -// GA surface must survive in BOTH builds — a preview build is a superset, never a -// replacement. const missingGa = GA_MARKERS.filter((marker) => !bundle.includes(marker)); if (missingGa.length > 0) { throw new Error( - `GA surface missing from the ${preview ? 'preview' : 'public'} build: ${missingGa.join(', ')}.\n` + - 'A shipped module was eliminated. Check that nothing moved its only reference ' + - 'behind `__BREVO_PREVIEW__` or into `commands/preview-definitions.ts`.', + `Shipped surface missing from the build: ${missingGa.join(', ')}.\n` + + 'A module that must ship was eliminated. Check that nothing moved its only ' + + 'reference behind a condition esbuild can fold to false, or into a module no live ' + + 'code imports.', ); } -if (!preview) { - const leaked = LEAK_MARKERS.filter((marker) => bundle.includes(marker)); - if (leaked.length > 0) { - throw new Error( - `Gated surface leaked into a public build: ${leaked.join(', ')}.\n` + - 'A gated module is reachable from live code. Check that it is referenced only ' + - 'from behind `__BREVO_PREVIEW__` (not the imported PREVIEW_BUILD constant, which ' + - 'esbuild cannot fold across modules) and that nothing else imports it.', - ); - } - const leakedStrings = publishedFiles().flatMap((file) => { - const content = fs.readFileSync(file, 'utf-8'); - return LEAK_STRINGS.filter((s) => content.includes(s)).map( - (s) => `${s} (${path.relative(root, file)})`, - ); - }); - if (leakedStrings.length > 0) { - throw new Error( - `Gated command strings are readable in a public build: ${leakedStrings.join(', ')}.\n` + - 'No command is registered for them, but `strings` on the published files names ' + - 'an unreleased feature. Move the string into `lib/preview-constants.ts` (or ' + - '`lang/preview-messages.ts` if it is user-facing copy) so the object carrying it ' + - 'is eliminated, rather than deleting the check.', - ); - } - const orphaned = orphanedPreviewMessageKeys(bundle); - if (orphaned.length > 0) { - throw new Error( - `Public build reads gated message keys that have no definition: ${orphaned.join(', ')}.\n` + - 'These resolve to `undefined` at runtime — a CliError built from one prints an ' + - 'empty message. Move the string to `lang/en.ts` if its feature is GA, or move the ' + - 'code that reads it behind `__BREVO_PREVIEW__`.', - ); - } -} else { - // Inverted on a preview build: a marker going missing here means the elimination is - // firing when it shouldn't, which would silently ship a preview build with no preview - // surface — the failure that looks like everything working. - const missing = [ - ...LEAK_MARKERS.filter((marker) => !bundle.includes(marker)), - ...LEAK_STRINGS.filter((s) => !bundle.includes(s)), - ]; - if (missing.length > 0) { - throw new Error( - `Preview build is missing gated surface: ${missing.join(', ')}.\n` + - 'PREVIEW=1 should include every gated module.', - ); - } -} - const bytes = fs.statSync(outfile).size; -console.log( - `${preview ? 'preview' : 'public'} build → dist/bin/index.js (${(bytes / 1024).toFixed(1)} kB)`, -); +console.log(`build → dist/bin/index.js (${(bytes / 1024).toFixed(1)} kB)`); diff --git a/scripts/release-check.mjs b/scripts/release-check.mjs index ed360189..cee44986 100644 --- a/scripts/release-check.mjs +++ b/scripts/release-check.mjs @@ -8,8 +8,8 @@ * Both feed the same `assertTarball()`: a pre-publish gate on a different * artifact than the post-publish one isn't a gate on the release. * - * The gated public-app surface is NOT checked here — build.mjs owns that - * (LEAK_MARKERS / GA_MARKERS) and `prepublishOnly` reruns it on the publish. + * Which surface the bundle carries is NOT checked here — build.mjs owns that + * (GA_MARKERS) and `prepublishOnly` reruns it on the publish. */ import { execFileSync } from 'node:child_process'; diff --git a/scripts/smoke-test.ts b/scripts/smoke-test.ts index 0cc19c9a..f6363e21 100644 --- a/scripts/smoke-test.ts +++ b/scripts/smoke-test.ts @@ -213,23 +213,21 @@ uninstall) always run, whichever suites are selected. Examples: yarn smoke --ci --suite=private,public # both (the default) Steps that need a command the installed CLI doesn't have (notably ---against=published, where 'app submit' / 'app status' / 'app withdraw' / -'app upload' may not be released yet) are auto-detected and reported as -skipped rather than failed. The same detection covers gated *features* that -come with no command of their own — '--distribution public', which a published -build refuses (BEX-405). - ---against=local builds what the selected suites need, and one 'yarn link' can -only hold one build: - * with 'public' selected (the default) it builds PREVIEW=1, because the - public lifecycle only exists on the preview surface; - * with only 'private' selected it builds the published surface, i.e. what npm - actually ships. Run 'yarn smoke --suite=private' when that is the thing you - want to verify. -The ui suite needs neither: UI apps are GA (BEX-290) and ship in every build, so -it runs on whichever artefact the other selected suites decided on. It DOES need -a pty — 'brevo app create' only offers the UI app type on a real terminal — so -the suite drives the prompts through script(1) and is opt-in like init. +--against=published, where the npm 'latest' tag may predate a command this +branch adds) are auto-detected and reported as skipped rather than failed. The +same detection covers a *feature* that comes with no command of its own. + +--against=local builds what npm ships, because that is now the only thing to +build: public apps and the review lifecycle went GA at BEX-405, UI apps at +BEX-290, and the build gate was torn down after them. This used to fork on the +selected suites, building PREVIEW=1 whenever 'public' was picked; the fork is +gone rather than merely unused, because leaving it would mean the one suite +exercising the review lifecycle never ran against the artifact users install. + +The ui and init suites are opt-in for a reason that was never about the gate: +they drive interactive prompts. 'brevo app create' only offers the UI app type +on a real terminal, so the ui suite drives it through script(1) and init through +scripted stdin. `); } diff --git a/scripts/smoke/core.ts b/scripts/smoke/core.ts index b360afaa..6d1f2c49 100644 --- a/scripts/smoke/core.ts +++ b/scripts/smoke/core.ts @@ -215,8 +215,8 @@ export interface ExecOptions { inherit?: boolean; // Hard cap, used by the trap paths so cleanup can't hang on a signal. timeoutMs?: number; - // Merged over process.env. Only the build in stepReinstall needs this, to ask - // for the preview surface (PREVIEW=1); every `brevo` call inherits plain env. + // Merged over process.env. Used by the suites that need to steer a single command + // (e.g. a scripted pty run); every plain `brevo` call inherits unmodified env. env?: Record; } @@ -796,15 +796,19 @@ export const GATED_COMMANDS = [ export type GatedCommand = (typeof GATED_COMMANDS)[number]; /** - * Gated *features* — surface that is missing from a build without a command going with - * it, so command detection can't see it. + * *Features* — surface that can be missing from an installed CLI without a command going + * with it, so command detection can't see it. * - * `public-distribution` is the one that matters: since BEX-405 the published build drops - * `--distribution public` (`assertFeatureAvailable('public-distribution')` refuses it with - * a typed CliError) while `app create` itself is obviously still there. The public suite - * opens by creating a public app, so without this the whole lifecycle *failed* on a - * published-surface build instead of skipping — and `yarn build` has produced that surface - * by default since `link:dev` stopped implying preview. + * `public-distribution` is the one that matters. It named a real build-time gate until + * public-apps GA: `yarn build` dropped `--distribution public` while `app create` itself + * was obviously still there, so without this entry the public suite — which opens by + * creating a public app — *failed* on a published-surface build instead of skipping. + * + * It still earns its place on `--against=published`, where the npm `latest` tag can be an + * older CLI that genuinely refuses the flag. That is now the only way this fires: a local + * build has the whole surface, and the gate that could have removed it is gone. Detection + * is unchanged either way — it probes the installed binary rather than assuming anything + * about which build produced it. */ export const GATED_FEATURES = ['public-distribution'] as const; @@ -817,12 +821,17 @@ export function listedInHelp(helpText: string, command: string): boolean { /** * Commands that are registered but appear on no help screen. * - * `withdraw` carries `hidden: true` (see `src/commands/preview-definitions.ts`): fully - * callable, simply not advertised. Root-help detection reads that as *absent* and would - * skip the withdraw step on a build that has it — a silent loss of coverage, which is - * the one failure mode a smoke run must not have. These are probed directly instead. + * **Empty since public-apps GA.** `withdraw` was the only entry: it carried + * `hidden: true` while the review lifecycle was being finished — fully callable, simply + * not advertised — and root-help detection reads that as *absent*, which would have + * skipped the withdraw step on a build that has the command. That is a silent loss of + * coverage, the one failure mode a smoke run must not have, so it was probed directly + * instead. The flag is gone and `brevo app withdraw` is on both help screens now. + * + * Kept as the mechanism rather than deleted: a `hidden` command is a normal thing to + * ship (a deprecation shim, a command mid-rollout) and this is where its name goes. */ -const UNLISTED_COMMANDS: ReadonlySet = new Set(['withdraw']); +const UNLISTED_COMMANDS: ReadonlySet = new Set(); /** * Ask a subcommand for its own help and see whether it answers as itself. @@ -839,13 +848,14 @@ function respondsToOwnHelp(state: State, command: string): boolean { } /** - * Does this build offer `--distribution public`? + * Does the INSTALLED CLI offer `--distribution public`? * - * Read off `app create`'s own help, where the flag's description is built from - * `distributionValues()` (`src/lib/help.ts`) — `Distribution type (private|public)` when the - * feature is available, `Distribution type (private)` when it isn't. Help is the only safe - * probe: actually running `create --distribution public` would either create a real app or - * burn an API call to be told it can't. + * Read off `app create`'s own help: this repo prints `Distribution type (private|public)` + * and always has both values now, but an older published CLI printed `Distribution type + * (private)` while the value was held back — which is the case this still probes for + * (`--against=published`). Help is the only safe probe: actually running `create + * --distribution public` would either create a real app or burn an API call to be told it + * can't. */ export function publicDistributionOffered(state: State): boolean { const r = exec(brevoCmd(state), ['app', 'create', '--help'], state); @@ -899,9 +909,10 @@ export function requireCommand(state: State, name: GatedCommand): void { export function requireFeature(state: State, name: GatedFeature): void { if (state.caps?.[name] !== false) return; // A runtime downgrade outranks the build explanation. The build DID offer the - // command in that case — a PREVIEW=1 run has the whole public surface — so - // saying "not available in this build" would be plainly false, and points a - // reader at scripts/build.mjs when the refusal came from the API. + // command in that case, so saying "not available in this build" would be plainly + // false, and points a reader at scripts/build.mjs when the refusal came from the API. + // With nothing gated, the build branch below can now only fire on + // `--against=published` where npm's `latest` predates a command on this branch. const downgraded = state.capDowngrades[name]; skip( downgraded @@ -920,10 +931,10 @@ export function featureMissing(state: State, name: GatedFeature): boolean { * * `detectCapabilities` can only read the CLIENT: it greps `--help`, so it answers "does * this build offer the flag", never "will the server accept it". Those come apart for - * `--distribution public` — a preview build offers the flag and Brevo declines the - * request (`public apps cannot be created with source "cli"`), which is the expected - * state until public apps go GA. Without this, the whole public lifecycle reports nine - * hard failures on every default run, and a permanently-red suite is one nobody reads. + * `--distribution public` — the CLI offers the flag and Brevo declines the request + * (`public apps cannot be created with source "cli"`), which is still the expected state + * on the platform side. Without this, the whole public lifecycle reports nine hard + * failures on every default run, and a permanently-red suite is one nobody reads. * * Called by the step that discovers the refusal, so every later `requireFeature` on the * same feature skips for the same reason instead of raising "no public app from the @@ -1015,15 +1026,15 @@ export function stepReinstall(state: State): string { let buildNote = ''; if (state.opts.against === 'local') { - // `yarn build` produces the *published* surface — the pre-GA commands and - // `--distribution public` are eliminated from it (BEX-405). The public suite exists to - // exercise exactly that surface, so it needs the preview artefact; asking for it here - // is what keeps the coverage rather than skipping the suite on a local run. Everything - // the private suite touches is present in both, so a private-only run stays published — - // and is then the only local run that tests what npm actually ships. - const needsPreview = state.opts.suites.includes('public'); - buildNote = needsPreview ? ', build=preview' : ', build=published'; - execOrThrow(PKG_YARN, ['build'], state, needsPreview ? { env: { PREVIEW: '1' } } : {}); + // There is one build, and it is what npm ships. This used to fork: `yarn build` + // eliminated the review-lifecycle commands and `--distribution public` (BEX-405), so a + // run that selected the public suite asked for `PREVIEW=1` instead, since that surface + // existed nowhere else. Public apps went GA, the gate was torn down after them, and the + // fork went with it — deliberately, not by neglect: keeping it would have left the one + // suite that exercises the review lifecycle permanently pointed at an artifact nobody + // installs. + buildNote = ', build=published'; + execOrThrow(PKG_YARN, ['build'], state); execOrThrow(PKG_YARN, ['link'], state); } else { execOrThrow(PKG_NPM, ['install', '-g', `${PACKAGE_NAME}@latest`], state); diff --git a/scripts/smoke/public-app.ts b/scripts/smoke/public-app.ts index 7fe724c6..c3f04709 100644 --- a/scripts/smoke/public-app.ts +++ b/scripts/smoke/public-app.ts @@ -30,12 +30,16 @@ import { uploadApp, } from './core'; -// Every state src/lang/en.ts (APP_STATUS_MESSAGE) has canned copy for, plus the -// 'unknown' sentinel status.ts normalises an empty state to. An unrecognised -// value means the server grew a state the CLI doesn't describe yet. +// Every state APP_STATUS_MESSAGE (src/lang/en.ts) has canned copy +// for, plus the 'unknown' sentinel status.ts normalises an empty state to. An +// unrecognised value means the server grew a state the CLI doesn't describe yet. +// +// BEX-382 renamed the initial state `configured` → `draft` on the wire (the +// server migration renames every existing row, so `configured` no longer +// appears — a clean rename with no alias), and BEX-383 reads the new value. const KNOWN_REVIEW_STATES = [ 'unknown', - 'configured', + 'draft', 'submitted', 'in_review', 'approved', diff --git a/scripts/smoke/ui-app.ts b/scripts/smoke/ui-app.ts index d31d76a8..bf627c6c 100644 --- a/scripts/smoke/ui-app.ts +++ b/scripts/smoke/ui-app.ts @@ -2,9 +2,8 @@ * UI-app lifecycle: interactive create (pty) -> upload no-op -> per-entry edit * upload -> install -> uninstall -> uninstall again -> delete. * - * UI apps are GA (BEX-290) and ship in every build, so this suite runs on the - * published surface — it does NOT need a PREVIEW=1 artefact. What it does need - * is a real terminal: `app create` gates its app-type prompt on + * UI apps are GA (BEX-290), so this suite runs on the published surface. What it + * does need is a real terminal: `app create` gates its app-type prompt on * `process.stdin.isTTY`, so a UI app can only be authored through a pty (see * execExpectPty in ./core). That is why the suite is opt-in, same as `init`. * diff --git a/src/__tests__/commands/app/create.test.ts b/src/__tests__/commands/app/create.test.ts index 886ccdfc..f7eecfd4 100644 --- a/src/__tests__/commands/app/create.test.ts +++ b/src/__tests__/commands/app/create.test.ts @@ -289,20 +289,20 @@ describe('app/create', () => { expect(asked.indexOf('redirectUrl')).toBeGreaterThan(asked.indexOf('appType')); }); - // The inverse of the two published-build assertions further down: a preview build - // offers the gated choice on both questions. Without this, gating everything to a - // one-item list would pass the public-build tests and ship a preview build that - // cannot reach the features it exists to exercise. - it('offers both choices on each gated question in a preview build', async () => { + // Both choice lists in ONE full interactive run, which is what the two assertions + // further down cannot say: they each drive a run that answers only their own question. + // A regression that narrowed one list depending on how the other was answered would + // pass both of those and fail here. + it('offers both choices on both questions in a single interactive run', async () => { (appService.createApp as jest.Mock).mockResolvedValue({ app_id: 3, - name: 'Preview App', - client_id: 'cli-preview', - client_secret: 'secret-preview', + name: 'Both Choices App', + client_id: 'cli-choices', + client_secret: 'secret-choices', redirect_uris: ['http://localhost:3009/auth/callback'], }); mockPrompt.mockResolvedValue({ - name: 'Preview App', + name: 'Both Choices App', logoUrl: '', distribution: 'private', appType: 'oauth', @@ -2558,18 +2558,14 @@ describe('app/create', () => { }); }); - // ──────── The pre-GA gate inside `app create` (BEX-405) ──────── - // Two of the four gated features are not commands, so `command-registry`'s - // interceptor cannot reach them: the *UI app* choice is a prompt option and - // `public` is a flag VALUE. They are gated inside the flow instead, and the - // shape of the refusal differs — a prompt choice is withheld, a flag is refused. - describe('in a published (public) build', () => { + // ──────── The full app-type / distribution surface ──────── + // Neither of these two choices is a command, so neither could ever be reached by a + // guard in `command-registry`: the *UI app* choice is a prompt option and `public` is a + // flag VALUE. Both were held back inside this flow while they were unreleased — a + // prompt choice withheld, a flag value refused — and both have shipped (`ui-app-type` + // at BEX-290, `public-distribution` at BEX-405). These assert the surface a user gets. + describe('app type and distribution', () => { beforeEach(() => { - // jest.setup.js runs the suite as a preview build so the feature tests above - // exercise the features rather than the gate. These want the public artifact. - // `create.ts` reads the flag per call, so flipping the global is enough — no - // module re-import needed, unlike the definitions/help tests. - globalThis.__BREVO_PREVIEW__ = false; (appService.createApp as jest.Mock).mockResolvedValue({ app_id: 42, name: 'Test App', @@ -2579,45 +2575,21 @@ describe('app/create', () => { }); }); - afterEach(() => { - globalThis.__BREVO_PREVIEW__ = true; - }); - - it('refuses --distribution public with the unreleased-feature message', async () => { - await expect( - createCommand({ name: 'Test App', distribution: 'public', json: true }), - ).rejects.toThrow(messages.PREVIEW_FEATURE_UNAVAILABLE); - }); - - // The refusal must land BEFORE any filesystem work. `app create` decides its - // target directory and then applies it (mkdir + chdir), so a refusal arriving - // after the apply step would leave a stray directory and a moved cwd behind for - // a command that failed — the same failure mode a server-side refusal used to - // cause before the decide/apply split. - it('refuses before creating anything', async () => { - await expect( - createCommand({ name: 'Test App', distribution: 'public', json: true }), - ).rejects.toThrow(messages.PREVIEW_FEATURE_UNAVAILABLE); + it('accepts --distribution public and sends it on the wire', async () => { + await createCommand({ name: 'Test App', distribution: 'public', json: true }); - expect(appService.createApp).not.toHaveBeenCalled(); - expect(resolveProjectDirectory).not.toHaveBeenCalled(); - expect(chdirSpy).not.toHaveBeenCalled(); + const payload = (appService.createApp as jest.Mock).mock.calls[0][0]; + expect(payload.distribution_type).toBe('public'); }); - // A genuine typo must still read as a typo. Routing every bad value through the - // unreleased-feature message would send the user hunting for a feature flag. - it('still rejects an invalid distribution as an invalid value', async () => { + // A bad value must read as a bad value, naming the flag — not as anything the user + // could mistake for a feature they need to unlock. + it('rejects an invalid distribution as an invalid value', async () => { await expect( createCommand({ name: 'Test App', distribution: 'privte', json: true }), ).rejects.toThrow(/--distribution/); - await expect( - createCommand({ name: 'Test App', distribution: 'privte', json: true }), - ).rejects.not.toThrow(messages.PREVIEW_FEATURE_UNAVAILABLE); }); - // The question is asked in every build. UI apps are GA, so the published build - // offers both app types — the choices no longer differ between builds; only the - // distribution question below still withholds its gated value. it('asks for the app type, offering OAuth and UI app alike', async () => { mockPrompt.mockResolvedValue({ appType: 'oauth', redirectUrl: '', logoUrl: '' }); @@ -2640,8 +2612,10 @@ describe('app/create', () => { expect(payload).not.toHaveProperty('ui_app'); }); - // Same rule for the distribution question, whose gated choice is `public`. - it('asks for the distribution, offering only private, and defaults to private', async () => { + // The ORDER matters and is asserted — `private` stays first so it remains what a bare + // Enter selects, which is the conservative default a developer should land on and the + // one every non-interactive run gets. + it('asks for the distribution, offering both values, and defaults to private', async () => { mockPrompt.mockResolvedValue({ appType: 'oauth', distribution: 'private', @@ -2655,16 +2629,17 @@ describe('app/create', () => { .flatMap((call) => call[0]) .find((question) => question?.name === 'distribution'); expect(distributionQuestion).toBeDefined(); - expect(distributionQuestion.choices).toHaveLength(1); - expect(distributionQuestion.choices[0].value).toBe('private'); + expect(distributionQuestion.choices).toHaveLength(2); + expect(distributionQuestion.choices.map((choice: { value: string }) => choice.value)).toEqual( + ['private', 'public'], + ); const payload = (appService.createApp as jest.Mock).mock.calls[0][0]; expect(payload.distribution_type).toBe('private'); }); - // The prompts are interactive-only in every build. This is what the removed - // `!isFeatureAvailable(...) → 'private'` early return used to guarantee by - // accident: without it, a `--json` run would block on a question it can't answer. + // Both prompts are interactive-only: without the non-interactive early returns a + // `--json` run would block on a question it cannot answer. it('asks neither question under --json, and still defaults to private + OAuth', async () => { await createCommand({ name: 'Test App', json: true }); @@ -2675,16 +2650,5 @@ describe('app/create', () => { expect(payload.distribution_type).toBe('private'); expect(payload).not.toHaveProperty('ui_app'); }); - - // The same command in a preview build, which is what `PREVIEW=1 yarn link:dev` - // produces and how this path is tested locally. - it('allows --distribution public in a preview build', async () => { - globalThis.__BREVO_PREVIEW__ = true; - - await createCommand({ name: 'Test App', distribution: 'public', json: true }); - - const payload = (appService.createApp as jest.Mock).mock.calls[0][0]; - expect(payload.distribution_type).toBe('public'); - }); }); }); diff --git a/src/__tests__/commands/app/status.test.ts b/src/__tests__/commands/app/status.test.ts index 7acab934..dd39bf70 100644 --- a/src/__tests__/commands/app/status.test.ts +++ b/src/__tests__/commands/app/status.test.ts @@ -57,6 +57,46 @@ describe('app/status', () => { }); }); + it('should prefer the message returned by the API over the canned per-state copy', async () => { + mockFetchAppState.mockResolvedValue({ + state: 'in_review', + message: 'Server-authored status note.', + }); + + await statusCommand({ appId: '42' }); + + const out = output(); + // The state still drives the label/tone… + expect(out).toContain('In Review'); + // …but the body is the server's message, not the canned copy. + expect(out).toContain('Server-authored status note.'); + expect(out).not.toContain('currently being reviewed'); + }); + + it('should surface the API message in --json output', async () => { + mockFetchAppState.mockResolvedValue({ + state: 'approved', + message: 'Congratulations — your app is live.', + }); + + await statusCommand({ appId: '42', json: true }); + + const parsed = JSON.parse(stdoutSpy.mock.calls[0][0]); + expect(parsed).toEqual({ + state: 'approved', + message: 'Congratulations — your app is live.', + }); + }); + + it('should fall back to the canned copy when the API message is blank', async () => { + mockFetchAppState.mockResolvedValue({ state: 'approved', message: ' ' }); + + await statusCommand({ appId: '42', json: true }); + + const parsed = JSON.parse(stdoutSpy.mock.calls[0][0]); + expect(parsed.message).toBe('Your app has been approved.'); + }); + it('should resolve the app id from app-config.json when no flag is given', async () => { mockReadProjectConfig.mockReturnValue({ appId: '77' }); mockFetchAppState.mockResolvedValue({ state: 'submitted' }); @@ -70,7 +110,7 @@ describe('app/status', () => { it('should prompt the app picker when no flag and no config', async () => { mockReadProjectConfig.mockReturnValue(null); mockPickApp.mockResolvedValue('88'); - mockFetchAppState.mockResolvedValue({ state: 'configured' }); + mockFetchAppState.mockResolvedValue({ state: 'draft' }); await statusCommand({}); @@ -87,6 +127,18 @@ describe('app/status', () => { expect(mockFetchAppState).toHaveBeenCalledWith('42'); }); + it('should render the draft state with an info tone and canned copy', async () => { + mockFetchAppState.mockResolvedValue({ state: 'draft' }); + + await statusCommand({ appId: '42' }); + + const out = output(); + expect(out).toContain('Draft'); + expect(out).toContain("hasn't been submitted for review yet"); + // The info tone renders a diamond glyph. + expect(out).toContain('◇'); + }); + it('should render the changes_requested canned copy', async () => { mockFetchAppState.mockResolvedValue({ state: 'changes_requested' }); diff --git a/src/__tests__/commands/app/submit.test.ts b/src/__tests__/commands/app/submit.test.ts index 8afc6a06..7f38748d 100644 --- a/src/__tests__/commands/app/submit.test.ts +++ b/src/__tests__/commands/app/submit.test.ts @@ -75,7 +75,11 @@ describe('app/submit', () => { // The status preflight must pass by default so flow tests reach the submit // logic; the preflight-failure test overrides this. (clearAllMocks resets // call data but not implementations, so re-establish it each test.) - (appService.fetchAppState as jest.Mock).mockResolvedValue({ state: 'configured' }); + (appService.fetchAppState as jest.Mock).mockResolvedValue({ + state: 'draft', + submittable: true, + missing_fields: [], + }); // Interactive runs now confirm before opening the form — accept by default // so pre-existing flow tests exercise the full path. (inquirer.prompt as unknown as jest.Mock).mockResolvedValue({ confirmed: true }); @@ -96,7 +100,11 @@ describe('app/submit', () => { it('runs the status check before opening the submission form', async () => { (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); - (appService.fetchAppState as jest.Mock).mockResolvedValue({ state: 'configured' }); + (appService.fetchAppState as jest.Mock).mockResolvedValue({ + state: 'draft', + submittable: true, + missing_fields: [], + }); (appService.fetchApp as jest.Mock).mockResolvedValue(PUBLIC_APP); await submitCommand({ appId: '42' }); @@ -111,12 +119,39 @@ describe('app/submit', () => { (appService.fetchApp as jest.Mock).mockResolvedValue(PUBLIC_APP); await expect(submitCommand({ appId: '42' })).rejects.toThrow('network unreachable'); - // The preflight runs first, so a failed status read stops the flow before - // fetching the app or opening the form. - expect(appService.fetchApp).not.toHaveBeenCalled(); + // A failed status read still stops the flow — the form is never opened. The app + // fetch DOES run first now (it feeds the never-uploaded gate, TC-6.3), so the + // abort is one round trip later than it used to be; what matters is that nothing + // submit-side happens, which is what `openBrowser` witnesses. + expect(appService.fetchApp).toHaveBeenCalledWith('42'); expect(openBrowser).not.toHaveBeenCalled(); }); + // ── The never-uploaded gate (TC-6.3) ── + + it('refuses an app that has never been uploaded, before reading the review state', async () => { + (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); + (appService.fetchApp as jest.Mock).mockResolvedValue({ ...PUBLIC_APP, version: undefined }); + + const error = await submitCommand({ appId: '42' }).catch((e: Error) => e); + expect((error as Error).message).toContain('never been uploaded'); + expect((error as Error).message).toContain('brevo app upload'); + // The whole point of the gate: the review-state read is what produced the + // misleading server copy (four fields named, all four present), so it must not + // happen at all for an app that cannot have a review state. + expect(appService.fetchAppState).not.toHaveBeenCalled(); + expect(openBrowser).not.toHaveBeenCalled(); + }); + + it('treats a blank version as never uploaded', async () => { + (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); + (appService.fetchApp as jest.Mock).mockResolvedValue({ ...PUBLIC_APP, version: ' ' }); + + const error = await submitCommand({ appId: '42' }).catch((e: Error) => e); + expect((error as Error).message).toContain('never been uploaded'); + expect(appService.fetchAppState).not.toHaveBeenCalled(); + }); + // ── Happy paths ── it('opens the submission form when the app is public and in sync', async () => { @@ -288,6 +323,60 @@ describe('app/submit', () => { await expect(submitCommand({ appId: '42' })).rejects.toThrow('cannot be submitted for review'); }); + // ── Submittability gate (BEX-383) ── + + it('blocks and lists the missing fields (raw server keys) when the app is not submittable', async () => { + (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); + // Pinned rather than inherited: `clearAllMocks` resets call data but not + // implementations, so without this the test leans on whatever an earlier test left + // on `fetchApp` — and it needs a versioned app to get past the gate above. + (appService.fetchApp as jest.Mock).mockResolvedValue(PUBLIC_APP); + (appService.fetchAppState as jest.Mock).mockResolvedValue({ + state: 'draft', + submittable: false, + missing_fields: ['logoLink', 'oauth.scopes'], + }); + + const error = await submitCommand({ appId: '42' }).catch((e: Error) => e); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain("isn't ready to submit"); + // Field keys are shown exactly as the state API returns them — no relabelling. + expect((error as Error).message).toContain('logoLink'); + expect((error as Error).message).toContain('oauth.scopes'); + expect((error as Error).message).toContain('brevo app upload'); + // The gate stops the flow before the form opens. It no longer runs before the app + // fetch — that moved ahead of the state read for the never-uploaded gate above — so + // `openBrowser` is what witnesses the refusal. + expect(openBrowser).not.toHaveBeenCalled(); + }); + + it('reports the raw missing-field keys in --json mode when not submittable', async () => { + (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); + (appService.fetchApp as jest.Mock).mockResolvedValue(PUBLIC_APP); + (appService.fetchAppState as jest.Mock).mockResolvedValue({ + state: 'draft', + submittable: false, + missing_fields: ['logoLink', 'oauth.scopes'], + }); + + const error = await submitCommand({ appId: '42', json: true }).catch((e: Error) => e); + expect(error).toBeInstanceOf(Error); + // --json keeps the compact raw-key form (no label translation). + expect((error as Error).message).toContain('logoLink'); + expect((error as Error).message).toContain('oauth.scopes'); + expect(openBrowser).not.toHaveBeenCalled(); + }); + + it('still submits when the server omits the submittable flag (older server)', async () => { + (readProjectConfig as jest.Mock).mockReturnValue(MATCHING_CONFIG); + (appService.fetchApp as jest.Mock).mockResolvedValue(PUBLIC_APP); + (appService.fetchAppState as jest.Mock).mockResolvedValue({ state: 'draft' }); + + await submitCommand({ appId: '42' }); + + expect(openBrowser).toHaveBeenCalledWith(FORM_URL); + }); + // ── Sync check ── it('blocks submission and shows a value-level diff when local config drifted', async () => { diff --git a/src/__tests__/lib/help-surface.test.ts b/src/__tests__/lib/help-surface.test.ts new file mode 100644 index 00000000..a3fcb084 --- /dev/null +++ b/src/__tests__/lib/help-surface.test.ts @@ -0,0 +1,149 @@ +/** + * The command surface as a user meets it: what `--help` shows, and what the parser + * registers. + * + * These build the real command tree the way `bin/index.ts` does, and exist because the + * CLI has **two independent help renderers** and nothing propagates between them: + * Commander generates `brevo app --help` from the definitions, while `lib/help.ts`'s + * `formatRootHelp` is a hand-aligned string that `hidden`, `description` and the option + * list cannot reach. A command added, removed or hidden in one has to be changed in the + * other by hand — and they have silently disagreed before, which is what this file is + * here to catch. + * + * Descended from `preview-gate.test.ts`, which asserted the same surface twice, once per + * build state, while the pre-GA gate could remove commands from a published build. That + * gate is gone (BEX-405) and there is one artifact again, so the per-build + * parametrization and the `jest.isolateModules` re-imports it needed went with it. The + * surface assertions did not: they were never really about the gate. + */ +import { Command } from 'commander'; +import { createHelpFormatter } from '../../lib/help'; +import { registerAll } from '../../lib/command-registry'; +import { appCommandGroup, skillCommandGroup, topLevelCommands } from '../../commands/definitions'; + +function render(cmd: Command): string { + let captured = ''; + cmd.configureOutput({ writeOut: (s) => (captured += s), writeErr: (s) => (captured += s) }); + cmd.outputHelp(); + return captured; +} + +function buildTree(): { program: Command; rootHelp: string; appHelp: string } { + const program = new Command(); + program + .name('brevo') + .description('Brevo Developer CLI — create, manage, and test OAuth integrations') + .version('0.0.0-test') + .configureHelp({ formatHelp: createHelpFormatter(program) }); + registerAll(program, topLevelCommands, [appCommandGroup, skillCommandGroup]); + + return { + program, + rootHelp: render(program), + appHelp: render(program.commands.find((c) => c.name() === 'app')!), + }; +} + +/** + * Every `brevo app` subcommand. + * + * Padded matches throughout: a bare `toContain('install')` is satisfied by `uninstall`'s + * help entry, so the one test proving `app install` is listed would stay green if only + * `install` were dropped. Same trap for `status` against `start`. + */ +const APP_COMMANDS = [ + 'init', + 'create', + 'list', + 'credentials', + 'upload', + 'delete', + 'scaffold', + 'start', + 'install', + 'uninstall', + 'submit', + 'status', + 'withdraw', +]; + +/** The two `requires`-derived groupings, stated as prose on the root screen. */ +const SECTION_HEADINGS = ['App-review commands', 'App-install commands']; + +/** The review lifecycle — the three commands whose `requires` is `review-lifecycle`. */ +const REVIEW_LIFECYCLE = ['submit', 'status', 'withdraw']; + +describe('the command surface', () => { + let tree: ReturnType; + beforeAll(() => { + tree = buildTree(); + }); + + it.each(APP_COMMANDS)('lists `app %s` on `brevo app --help`', (name) => { + expect(tree.appHelp).toContain(` ${name} `); + }); + + it.each(REVIEW_LIFECYCLE)('registers `app %s` on the parser', (name) => { + const app = tree.program.commands.find((c) => c.name() === 'app')!; + expect(app.commands.find((c) => c.name() === name)).toBeDefined(); + }); + + it.each(SECTION_HEADINGS)('renders the "%s" section on the root help', (heading) => { + expect(tree.rootHelp).toContain(heading); + }); + + // The two-renderer trap, as a worked example. `withdraw` was marked `hidden` while the + // review lifecycle was being finished — which suppressed its Commander help entry and + // did nothing at all to the hand-aligned root screen, where the same omission had to be + // made and then undone by hand. A change to one and not the other is what this pair + // catches. + it('lists `app withdraw` on both help screens', () => { + expect(tree.appHelp).toContain(' withdraw '); + expect(tree.rootHelp).toContain('brevo app withdraw'); + expect(tree.rootHelp).toContain('Withdraw an app from submission'); + }); + + it('answers `app withdraw --help` with its own usage', () => { + const app = tree.program.commands.find((c) => c.name() === 'app')!; + const withdraw = app.commands.find((c) => c.name() === 'withdraw')!; + const own = render(withdraw); + expect(own).toContain('Usage: brevo app withdraw'); + expect(own).toContain('--app-id'); + expect(own).toContain('--force'); + }); + + it('keeps the "App-install commands" section on the root help', () => { + expect(tree.rootHelp).toContain('App-install commands (UI apps only):'); + expect(tree.rootHelp).toContain('brevo app install'); + expect(tree.rootHelp).toContain('brevo app uninstall'); + }); + + // `--distribution`'s value list and `app create`'s description are each written twice — + // once in `definitions.ts` for Commander, once inline in `formatRootHelp`. Both spellings + // are asserted so a change to one is not mistaken for a change to both. + describe('--distribution and the create description, in both renderers', () => { + it('advertises both distribution values on the root help', () => { + expect(tree.rootHelp).toContain('private|public'); + }); + + it('offers both --distribution values in `app create --help`', () => { + const createHelp = render( + tree.program.commands + .find((c) => c.name() === 'app')! + .commands.find((c) => c.name() === 'create')!, + ); + expect(createHelp).toContain('Distribution type (private|public)'); + expect(createHelp).toContain('--distribution private'); + expect(createHelp).toContain('--distribution public'); + }); + + // The `app --help` copy is compared whitespace-normalized: Commander wraps a long + // description across its two-column layout, so the string is present but broken over + // lines. The root screen is hand-aligned onto one line and is matched verbatim. + it('advertises UI apps in the create description on both screens', () => { + const expected = 'Create a new app (OAuth, or a UI app via the prompts)'; + expect(tree.rootHelp).toContain(expected); + expect(tree.appHelp.replace(/\s+/g, ' ')).toContain(expected); + }); + }); +}); diff --git a/src/__tests__/lib/preview-gate.test.ts b/src/__tests__/lib/preview-gate.test.ts deleted file mode 100644 index a0872fe9..00000000 --- a/src/__tests__/lib/preview-gate.test.ts +++ /dev/null @@ -1,264 +0,0 @@ -/** - * The gate as a user meets it (BEX-405): what `--help` shows, and what happens when - * a hidden command is invoked anyway. - * - * These build the real command tree the way `bin/index.ts` does, rather than testing - * `isFeatureAvailable` again — the unit coverage in `preview.test.ts` already owns - * the decision. What is worth asserting here is the wiring: that the decision reaches - * two independent renderers (the hand-aligned root screen and Commander's generated - * subcommand screen) and the parser, and that it reaches nothing else. - */ -import { Command } from 'commander'; - -type Tree = { - program: Command; - rootHelp: string; - appHelp: string; -}; - -/** - * Build the command tree as a given build state would produce it. - * - * `isolateModules` is what makes this possible at all: `commands/definitions.ts` - * resolves its command list, `app create`'s description, the `--distribution` values - * and the example list at module load, all from `__BREVO_PREVIEW__`. Re-importing with - * the global flipped reproduces what esbuild bakes into each artifact, so both builds - * are covered by one test run without building twice. - */ -function buildTree(previewBuild: boolean): Tree { - const original = globalThis.__BREVO_PREVIEW__; - globalThis.__BREVO_PREVIEW__ = previewBuild; - - let tree: Tree | undefined; - jest.isolateModules(() => { - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { createHelpFormatter } = require('../../lib/help'); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { registerAll } = require('../../lib/command-registry'); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const defs = require('../../commands/definitions'); - - const program = new Command(); - program - .name('brevo') - .description('Brevo Developer CLI — create, manage, and test OAuth integrations') - .version('0.0.0-test') - .configureHelp({ formatHelp: createHelpFormatter(program) }); - registerAll(program, defs.topLevelCommands, [defs.appCommandGroup, defs.skillCommandGroup]); - - tree = { - program, - rootHelp: render(program), - appHelp: render(program.commands.find((c) => c.name() === 'app')!), - }; - }); - globalThis.__BREVO_PREVIEW__ = original; - return tree!; -} - -/** - * Load the `messages` object as a given build state would produce it. - * - * Same `isolateModules` trick as {@link buildTree}, for the same reason: `lang/en.ts` - * decides at module load whether to spread `previewMessages` in, so the only way to see - * a public build's `messages` from a suite that runs with `__BREVO_PREVIEW__= true` - * (jest.setup.js, deliberately) is to re-import with the flag flipped. - */ -function loadMessages(previewBuild: boolean): Record { - const original = globalThis.__BREVO_PREVIEW__; - globalThis.__BREVO_PREVIEW__ = previewBuild; - - let loaded: Record | undefined; - jest.isolateModules(() => { - // eslint-disable-next-line @typescript-eslint/no-var-requires - loaded = require('../../lang/en').messages; - }); - globalThis.__BREVO_PREVIEW__ = original; - return loaded!; -} - -function render(cmd: Command): string { - let captured = ''; - cmd.configureOutput({ writeOut: (s) => (captured += s), writeErr: (s) => (captured += s) }); - cmd.outputHelp(); - return captured; -} - -/** - * Every command the pre-GA gate covers, and the section heading it sits under. - * `install` / `uninstall` left this list at UI-apps GA — they ship in every build now - * and are asserted alongside the other released commands below. - */ -const GATED = ['submit', 'status', 'withdraw']; -const GATED_HEADINGS = ['App-review commands']; - -/** - * The gated commands a preview build actually advertises. - * - * `withdraw` is the exception, and for a different reason than the gate: it carries - * `hidden: true` in `commands/preview-definitions.ts`, which suppresses its help entry - * without touching the parser. So a preview build registers it and runs it but lists it - * nowhere — asserted on its own below, since "hidden" and "absent" are different claims - * and only the gate makes the second one. - */ -const GATED_LISTED = GATED.filter((name) => name !== 'withdraw'); - -/** A representative ungated command per section, to prove the filter is not too wide. */ -const UNGATED = [ - 'init', - 'create', - 'list', - 'credentials', - 'upload', - 'delete', - 'scaffold', - 'start', - 'install', - 'uninstall', -]; - -describe('the pre-GA gate, end to end', () => { - describe('a published (public) build', () => { - let tree: Tree; - beforeAll(() => { - tree = buildTree(false); - }); - - it.each(GATED)('hides `app %s` from `brevo app --help`', (name) => { - expect(tree.appHelp).not.toContain(` ${name} `); - }); - - it.each(GATED_HEADINGS)('drops the "%s" section from the root help', (heading) => { - expect(tree.rootHelp).not.toContain(heading); - }); - - // Padded like the GATED checks above, and for the mirror-image reason: a bare - // `toContain('install')` is satisfied by `uninstall`'s help entry, so the one test - // proving `app install` survived a public build would stay green if only `install` - // were dropped. - it.each(UNGATED)('still lists `app %s`', (name) => { - expect(tree.appHelp).toContain(` ${name} `); - }); - - // The flag is GA; only the `public` value is gated. Dropping the flag would be - // wrong — `--distribution private` is the documented default path. - it('keeps --distribution but narrows its advertised values', () => { - expect(tree.rootHelp).toContain('--distribution private]'); - expect(tree.rootHelp).not.toContain('private|public'); - }); - - // UI apps are GA: the published build advertises the choice and the install - // section exactly as a preview build does. - it('advertises UI apps in the create description', () => { - expect(tree.rootHelp).toContain('Create a new app (OAuth, or a UI app via the prompts)'); - }); - - it('keeps the "App-install commands" section on the root help', () => { - expect(tree.rootHelp).toContain('App-install commands (UI apps only):'); - expect(tree.rootHelp).toContain('brevo app install'); - expect(tree.rootHelp).toContain('brevo app uninstall'); - }); - - it('drops the --distribution public example from `app create --help`', () => { - const createHelp = render( - tree.program.commands - .find((c) => c.name() === 'app')! - .commands.find((c) => c.name() === 'create')!, - ); - expect(createHelp).toContain('--distribution private'); - expect(createHelp).not.toContain('--distribution public'); - }); - - // Not registered at all, so Commander answers `unknown command` rather than the - // typed refusal. That is the honest answer here and a deliberate change from the - // earlier runtime gate: with the modules eliminated at build time the command - // genuinely does not exist in this artifact, so claiming it exists-but-is-withheld - // would be the lie. The typed refusal survives only where a value must still be - // parsed and rejected — see `--distribution public` in create.test.ts. - it.each(GATED)('does not register `app %s`', (name) => { - const app = tree.program.commands.find((c) => c.name() === 'app')!; - expect(app.commands.find((c) => c.name() === name)).toBeUndefined(); - }); - - // Regression: the legacy-'all'-scope deprecation (BEX-214) is GA, and its strings are - // read by `app upload` and `app start`, which ship in every build. BEX-405 moved - // `_DEPRECATED_BLOCK` into `lang/preview-messages.ts` with the genuinely gated strings, - // so a public build eliminated the definition while leaving the read — and - // `new CliError(undefined)` has `message === ''`. `brevo app upload` on any app still - // holding the 'all' scope printed a bare `✗` and exited 1, telling the one group of - // users who need the migration text precisely nothing. - // - // Asserted on the whole family rather than the one key that broke: they are GA - // together, and a future tidy-up that sweeps "legacy scope" strings into the gated - // module would take the others the same way. The suite runs preview-side by design - // (jest.setup.js), which is why this has to flip the flag to see the bug at all. - // `scripts/build.mjs` enforces the general rule on the emitted bundle. - it.each([ - 'LEGACY_ALL_SCOPE_DEPRECATED_BLOCK', - 'LEGACY_ALL_SCOPE_START_BLOCK', - 'LEGACY_ALL_SCOPE_LIST_TAG', - 'LEGACY_ALL_SCOPE_SCAFFOLD_SUBSTITUTED', - 'LEGACY_ALL_SCOPE_UPDATE_MIGRATING', - ])('still defines messages.%s', (key) => { - const value = loadMessages(false)[key]; - expect(value).toBeDefined(); - expect(typeof value === 'string' ? value : 'fn').not.toBe(''); - }); - - // The failure mode above, stated as the user-visible symptom rather than the cause: - // a CliError built from a missing message renders as an empty line, not an error. - it('builds a non-empty CliError from the legacy-scope block', () => { - const messages = loadMessages(false); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const { CliError } = require('../../lib/errors'); - const err = new CliError(messages.LEGACY_ALL_SCOPE_DEPRECATED_BLOCK as string); - expect(err.message).not.toBe(''); - expect(err.message).toContain("'all'"); - }); - }); - - describe('a preview build (PREVIEW=1 yarn link:dev)', () => { - let tree: Tree; - beforeAll(() => { - tree = buildTree(true); - }); - - it.each(GATED_LISTED)('lists `app %s`', (name) => { - expect(tree.appHelp).toContain(name); - }); - - it.each(GATED_HEADINGS)('restores the "%s" section', (heading) => { - expect(tree.rootHelp).toContain(heading); - }); - - // Both renderers, because they are independent: Commander's `hidden` filters the - // generated `brevo app --help`, and the hand-aligned root screen is a string it - // cannot reach, so that omission is maintained by hand in `lib/help.ts`. A change - // to one and not the other is exactly what this pair is here to catch. - it('lists `app withdraw` on neither help screen', () => { - expect(tree.appHelp).not.toContain('withdraw'); - expect(tree.rootHelp).not.toContain('withdraw'); - }); - - // Hidden, not withheld. The section it would sit in is still rendered, and the - // command itself is registered, parses its flags and reaches its handler — so - // anyone who types it (QA suite 7, the public-app smoke script, the hint `app - // upload` prints when an app is under review) gets the command, not a refusal. - it('still registers `app withdraw` and answers its own --help', () => { - const app = tree.program.commands.find((c) => c.name() === 'app')!; - const withdraw = app.commands.find((c) => c.name() === 'withdraw'); - - expect(withdraw).toBeDefined(); - - const own = render(withdraw!); - expect(own).toContain('Usage: brevo app withdraw'); - expect(own).toContain('--app-id'); - expect(own).toContain('--force'); - }); - - it('advertises both distribution values and the UI-app choice', () => { - expect(tree.rootHelp).toContain('private|public'); - expect(tree.rootHelp).toMatch(/UI app/i); - }); - }); -}); diff --git a/src/__tests__/lib/preview.test.ts b/src/__tests__/lib/preview.test.ts deleted file mode 100644 index b9bdd410..00000000 --- a/src/__tests__/lib/preview.test.ts +++ /dev/null @@ -1,120 +0,0 @@ -import { CliError } from '../../lib/errors'; -import { messages } from '../../lang/en'; -import { FEATURE_STAGE, assertFeatureAvailable, isFeatureAvailable } from '../../lib/preview'; - -/** - * Run a block as a given build state would. - * - * `isFeatureAvailable` reads `__BREVO_PREVIEW__` per call, so setting the global is - * enough — no module re-import. That is deliberate: an earlier version captured the - * flag in a module constant, which made the gate untestable without `isolateModules` - * and, worse, meant an importing module froze the value at load. Only the *elimination* - * sites (`definitions.ts`, `help.ts`) still need re-importing, because they read the - * global at module scope; `preview-gate.test.ts` covers those. - */ -function asBuild(previewBuild: boolean): void { - beforeEach(() => { - globalThis.__BREVO_PREVIEW__ = previewBuild; - }); - afterEach(() => { - globalThis.__BREVO_PREVIEW__ = true; - }); -} - -describe('lib/preview', () => { - describe('FEATURE_STAGE', () => { - // Guards the intent of the current release state: UI apps (the create choice and - // install/uninstall) are GA, public distribution and its review lifecycle are not. - // When another feature ships, this is the assertion that fails and points at the - // GA checklist. - it('matches the released feature set', () => { - expect(FEATURE_STAGE).toEqual({ - 'account-install': 'ga', - 'review-lifecycle': 'preview', - 'ui-app-type': 'ga', - 'public-distribution': 'preview', - }); - }); - }); - - describe('a published (public) build', () => { - asBuild(false); - - it('reports every preview-staged feature as unavailable, and every GA one as available', () => { - for (const [feature, stage] of Object.entries(FEATURE_STAGE)) { - expect(isFeatureAvailable(feature as keyof typeof FEATURE_STAGE)).toBe(stage === 'ga'); - } - }); - - it('refuses with a typed CliError and exit code 1', () => { - expect(() => assertFeatureAvailable('public-distribution')).toThrow(CliError); - try { - assertFeatureAvailable('public-distribution'); - throw new Error('expected a refusal'); - } catch (err) { - expect((err as CliError).name).toBe('CliError'); - expect((err as CliError).message).toBe(messages.PREVIEW_FEATURE_UNAVAILABLE); - expect((err as CliError).exitCode).toBe(1); - } - }); - - // The whole point of moving the flag to build time. If any of these re-enabled the - // gate, the guard would be a runtime one again and the surface would have to ship - // in order to be revealable. - it.each([ - ['BREVO_ENABLE_PREVIEW', '1'], - ['BREVO_PREVIEW', '1'], - ['BREVO_PREVIEW_BUILD', '1'], - ])('cannot be unlocked by %s=%s', (name, value) => { - const original = process.env[name]; - process.env[name] = value; - try { - expect(isFeatureAvailable('review-lifecycle')).toBe(false); - } finally { - if (original === undefined) delete process.env[name]; - else process.env[name] = original; - } - }); - - // The account-based escape hatch was removed with the env var. The gate must not - // read credentials at all — a build-time flag that consults who you are logged in - // as is a runtime flag. - it('does not consult the logged-in account', () => { - const config = require('../../lib/config'); - const spy = jest.spyOn(config, 'getEmail'); - isFeatureAvailable('review-lifecycle'); - expect(spy).not.toHaveBeenCalled(); - spy.mockRestore(); - }); - - // Not named in the refusal: an end user can act on neither, so mentioning either - // would only invite an attempt. - it('does not leak the build flag in the message', () => { - expect(messages.PREVIEW_FEATURE_UNAVAILABLE).not.toMatch(/PREVIEW|brevo\.com/i); - }); - }); - - describe('a preview build (PREVIEW=1)', () => { - asBuild(true); - - it('reports every preview feature as available', () => { - for (const feature of Object.keys(FEATURE_STAGE)) { - expect(isFeatureAvailable(feature as keyof typeof FEATURE_STAGE)).toBe(true); - } - }); - - it('does not refuse', () => { - expect(() => assertFeatureAvailable('review-lifecycle')).not.toThrow(); - }); - }); - - // The suite runs as a preview build (jest.setup.js), so the directly imported - // bindings should agree with the preview gate — a guard against the setup file - // drifting from what these tests assume. - describe('the suite default', () => { - it('runs as a preview build', () => { - expect(isFeatureAvailable('review-lifecycle')).toBe(true); - expect(() => assertFeatureAvailable('public-distribution')).not.toThrow(); - }); - }); -}); diff --git a/src/__tests__/services/app.test.ts b/src/__tests__/services/app.test.ts index 8a3568b5..851fa2e1 100644 --- a/src/__tests__/services/app.test.ts +++ b/src/__tests__/services/app.test.ts @@ -296,6 +296,20 @@ describe('services/app', () => { expect(mockClient.get).toHaveBeenCalledWith(`/v3/app-store/apps/${UUID}/state`); }); + it('should pass through the submittability fields (BEX-383)', async () => { + (mockClient.get as jest.Mock).mockResolvedValue({ + state: 'draft', + submittable: false, + missing_fields: ['logoLink', 'oauth.scopes'], + }); + const result = await service.fetchAppState('42'); + expect(result).toEqual({ + state: 'draft', + submittable: false, + missing_fields: ['logoLink', 'oauth.scopes'], + }); + }); + it('should map a 404 to an app-not-found CliError', async () => { const { ApiError } = jest.requireActual('../../lib/errors'); (mockClient.get as jest.Mock).mockRejectedValue(new ApiError('nope', 404)); diff --git a/src/app-types/contract.ts b/src/app-types/contract.ts index 8a848cbd..27b310c6 100644 --- a/src/app-types/contract.ts +++ b/src/app-types/contract.ts @@ -62,11 +62,12 @@ export interface AppTypeModule { * `preview` = shipped in the CLI but not live on the Brevo platform; `ga` = live. * Both types are `'ga'` today (UI apps since BEX-290). * - * This is METADATA ONLY and must stay that way: the CLI deliberately has no runtime - * guard on app types — pre-GA surface is removed at *build* time instead - * (`scripts/build.mjs`), and `CLAUDE.md` forbids reintroducing a runtime gate or an - * internal-account escape hatch. The field exists so docs and a future type can state - * their stage in one place instead of five hand-maintained notices. + * Not the pre-GA build gate, which was a different mechanism and is gone (BEX-405): + * this describes the *platform*, not the artifact. It is METADATA ONLY and must stay + * that way — the CLI deliberately has no runtime guard on app types, and `CLAUDE.md` + * forbids reintroducing one or an internal-account escape hatch. The field exists so + * docs and a future type can state their stage in one place instead of five + * hand-maintained notices. */ availability: 'ga' | 'preview'; diff --git a/src/commands/app/create.ts b/src/commands/app/create.ts index 6b3b9133..b6dfae36 100644 --- a/src/commands/app/create.ts +++ b/src/commands/app/create.ts @@ -15,7 +15,6 @@ import { ApiError, AuthExpiredError, CliError, ErrorCode } from '../../lib/error import { withCommandHandler } from '../../lib/command-handler'; import { jsonOutput } from '../../lib/json-output'; import { validateEnum, validateAppName, validateYesNo } from '../../lib/validators'; -import { assertFeatureAvailable, isFeatureAvailable } from '../../lib/preview'; import { printBox, createSpinner, indentChoices } from '../../lib/ui'; import { saveAppCredentials, @@ -235,21 +234,13 @@ async function resolveAppType(interactive: boolean): Promise { if (!interactive) { return 'oauth'; } - // The question is always asked; only the *choices* are gated. A build that offers one - // app type still names it, so the flow reads the same everywhere and the user is told - // what they are getting rather than having it applied silently. - // - // `ui-app-type` is GA, so `isFeatureAvailable` answers true in every build — the call - // stays so the choice keeps reading the same `FEATURE_STAGE` table as everything - // else. The `__BREVO_PREVIEW__ &&` guard this site carried pre-GA is gone with the - // gate; the UI-authoring layer (registry reads, placement prompts, the summary box) - // now ships in the published bundle. + // OAuth first, so a bare Enter selects the type that every non-interactive run also + // gets — the answer a script would have produced, for someone who did not read the + // question. const choices: Array<{ name: string; value: AppType }> = [ { name: messages.APP_CREATE_APP_TYPE_OAUTH, value: 'oauth' }, + { name: messages.APP_CREATE_APP_TYPE_UI, value: 'ui' }, ]; - if (isFeatureAvailable('ui-app-type')) { - choices.push({ name: messages.APP_CREATE_APP_TYPE_UI, value: 'ui' }); - } const answer = await inquirer.prompt([ { type: 'list', @@ -268,17 +259,13 @@ async function resolveAppType(interactive: boolean): Promise { // made to answer questions, otherwise `--distribution typo` costs a logo prompt // first. Pure — no I/O, no prompts — so it is safe this early. // -// Public distribution is pre-GA (BEX-405). The flag keeps validating against the -// full set so `--distribution public` still fails as an *unreleased feature* rather -// than as an unknown value — the second would be a lie, and would send the user -// looking for a typo. `validateEnum` runs first so a genuine typo still gets the -// "invalid value" error it deserves. +// Both values are accepted; `distribution_type` is immutable after create, so the +// only thing to check is that the flag names one of the two. Kept as its own +// function rather than inlined into `resolveDistribution` for the ordering reason +// above. function assertDistributionFlag(distributionFlag: string | undefined): void { const VALID_DISTRIBUTIONS = ['private', 'public'] as const; validateEnum(distributionFlag, VALID_DISTRIBUTIONS, '--distribution'); - if (distributionFlag === 'public') { - assertFeatureAvailable('public-distribution'); - } } // 3. Distribution type — the flag is already validated by `assertDistributionFlag`. @@ -289,24 +276,23 @@ async function resolveDistribution( if (distributionFlag) { return distributionFlag; } - // Load-bearing, not a tidy-up: this used to be covered by the feature check below - // returning early, so removing that check without this one would put a prompt in - // front of every `--json` / piped run and hang CI on a question it cannot answer. + // Load-bearing, not a tidy-up: a `--json` or piped run must never reach the prompt + // below, or CI hangs on a question it cannot answer. `private` is the conservative + // default, and it is what every scripted `app create` produced before the prompt + // existed. if (!interactive) { return 'private'; } - // Same shape as the app-type prompt: the question is always asked, only the choices - // are gated. See the ELIMINATION SITE note there for why the raw global appears - // alongside `isFeatureAvailable`. + // `private` stays FIRST so it remains what a bare Enter selects — the same + // conservative default the non-interactive path takes, for the same reason. + // `distribution_type` is immutable after create, so a mis-hit here costs a new app. const choices: Array<{ name: string; value: string }> = [ { name: 'Private (Used exclusively by your organisation)', value: 'private' }, - ]; - if (__BREVO_PREVIEW__ && isFeatureAvailable('public-distribution')) { - choices.push({ + { name: 'Public (Distributed to end users or marketplace listings)', value: 'public', - }); - } + }, + ]; const answer = await inquirer.prompt([ { type: 'list', diff --git a/src/commands/app/status.ts b/src/commands/app/status.ts index 344b5a0a..92856e49 100644 --- a/src/commands/app/status.ts +++ b/src/commands/app/status.ts @@ -27,7 +27,7 @@ function toTone(state: string): StatusTone { return 'progress'; case 'submitted': return 'pending'; - case 'configured': + case 'draft': return 'info'; default: return 'neutral'; @@ -57,7 +57,11 @@ export const statusCommand = withCommandHandler( // Normalize a missing/empty state to a non-empty sentinel so both the // header label and --json output stay meaningful. const state = typeof raw.state === 'string' && raw.state ? raw.state : 'unknown'; - const message = messages.APP_STATUS_MESSAGE(state); + // Prefer the server-provided message; fall back to the CLI's per-state copy + // when the API omits it (older server) or sends a blank string. + const apiMessage = + typeof raw.message === 'string' && raw.message.trim() ? raw.message : undefined; + const message = apiMessage ?? messages.APP_STATUS_MESSAGE(state); if (options.json) { jsonOutput({ state, message }); diff --git a/src/commands/app/submit.ts b/src/commands/app/submit.ts index 7a5a43f4..941db8d5 100644 --- a/src/commands/app/submit.ts +++ b/src/commands/app/submit.ts @@ -9,7 +9,7 @@ import { EXIT_CODES } from '../../lib/exit-codes'; import { jsonOutput } from '../../lib/json-output'; import { logDebug, logInfo, logSuccess } from '../../lib/logger'; import { createSpinner } from '../../lib/ui'; -import { OAuthApp } from '../../types'; +import { AppStateResponse, OAuthApp } from '../../types'; import { assertCapability, resolveFromRecord, type Distribution } from '../../app-types'; interface SubmitOptions { @@ -165,19 +165,39 @@ async function resolveAppId(options: SubmitOptions, config: ProjectConfig | null } // Preflight through the canonical review-state read (`brevo app status`'s path) -// before doing any submit work. Only a failed fetch — network, auth, or a -// not-found app — blocks the flow; the returned state value is not a gate, so -// it's read and discarded. A thrown error propagates to the command handler, -// which aborts the submission. -async function checkAppStatus(appId: string, silent: boolean | undefined): Promise { +// before doing any submit work. A failed fetch — network, auth, or a not-found +// app — propagates to the command handler and aborts the submission. The +// returned state also carries the submittability signal (BEX-383), consumed by +// `assertSubmittable` immediately after this read. +async function preflightAppState( + appId: string, + silent: boolean | undefined, +): Promise { const spinner = createSpinner(messages.APP_SUBMIT_CHECKING_STATUS, { silent }); try { - await appService.fetchAppState(appId); + return await appService.fetchAppState(appId); } finally { spinner.stop(); } } +// Block a submission the backend would reject for incompleteness. The state API +// reports `submittable` plus the specific `missing_fields`; only an explicit +// `false` gates, so an older server that omits the flag still submits (matches the +// optional type in AppStateResponse). Both modes show the field keys exactly as the +// server returns them (e.g. `logoLink`, `oauth.scopes`) — no local relabelling — so +// the developer sees the same name the API uses. --json is a compact inline list; +// humans get the multiline list. +function assertSubmittable(state: AppStateResponse, jsonMode: boolean, appId: string): void { + if (state.submittable !== false) return; + const fields = state.missing_fields ?? []; + if (jsonMode) { + throw new CliError(messages.APP_SUBMIT_NOT_SUBMITTABLE(fields, appId)); + } + const diff = fields.map((f) => ` ${f}`).join('\n'); + throw new CliError(messages.APP_SUBMIT_NOT_SUBMITTABLE_DIFF(diff, appId)); +} + async function fetchExistingApp(appId: string, silent: boolean | undefined): Promise { const spinner = createSpinner(messages.APP_SUBMIT_FETCHING, { silent }); let app: OAuthApp | null; @@ -196,11 +216,31 @@ export const submitCommand = withCommandHandler(async (options: SubmitOptions): const config = readProjectConfig(); const appId = await resolveAppId(options, config); - // Run the status check first — a failed read aborts before we attempt to - // submit. - await checkAppStatus(appId, options.json); - + // Fetch the app BEFORE the review-state read, so an app that was never uploaded is + // refused in the CLI's own words (TC-6.3). The order used to be the other way round — + // state read first, "a failed read aborts before we attempt to submit" — and it still + // does abort, just one round trip later. What the old order cost was the error message: + // the state read fails on an app with no `app_versions` row, and the server's copy for + // that failure names `name`, `logo_uri`, `scopes` and `redirect_uris` as the things to + // fix. All four can be present. Nothing in `apiCodeMessages` maps the code, so it + // reached the user verbatim and sent them auditing fields that were already correct. + // + // `version` is the certain signal: it is written only by a successful `app upload`, so + // its absence means the app has never been uploaded and cannot have a review state. + // Same gate `app install` already applies for the same reason — see + // `assertInstallable`'s `requireUploaded` in `account-install.ts`, and CLAUDE.md on why + // a local pre-flight is kept even where the server also checks. const app = await fetchExistingApp(appId, options.json); + if (!app.version?.trim()) { + throw new CliError(messages.APP_SUBMIT_NOT_UPLOADED(appId)); + } + + // The response carries the submittability signal used just below. + const state = await preflightAppState(appId, options.json); + + // Block when the app is still missing fields required for review (BEX-383) before + // doing any submit work. + assertSubmittable(state, !!options.json, appId); // Only public apps can be submitted for review — expressed as a capability so the rule // lives in one table (`src/app-types/capabilities.ts`) instead of being restated by each diff --git a/src/commands/definitions.ts b/src/commands/definitions.ts index 7957f7f6..05445854 100644 --- a/src/commands/definitions.ts +++ b/src/commands/definitions.ts @@ -1,13 +1,6 @@ import { CommandDefinition, SubcommandGroupDefinition } from '../lib/command-registry'; import { parseAppId, parsePositiveInt, collectUrls, validateUrl } from '../lib/validators'; import { EXAMPLE_APP_ID } from '../lib/constants'; -import { isFeatureAvailable } from '../lib/preview'; -import { createDescription, distributionValues } from '../lib/help'; -// The gated subcommands are referenced only through this binding, and only from behind -// `__BREVO_PREVIEW__`. That is what lets esbuild drop them — and their three handler -// modules — from a published build. Importing any of those handlers directly here would -// make them live references again and ship the whole surface. See ./preview-definitions.ts. -import { previewAppCommands } from './preview-definitions'; import { initCommand } from './init'; import { loginCommand } from './login'; @@ -23,6 +16,9 @@ import { scopesCommand } from './app/scopes'; import { startCommand } from './app/start'; import { appInstallCommand } from './app/install'; import { appUninstallCommand } from './app/uninstall'; +import { submitCommand } from './app/submit'; +import { statusCommand } from './app/status'; +import { withdrawCommand } from './app/withdraw'; import { installCommand as skillInstallCommand } from './skill/install'; import { uninstallCommand as skillUninstallCommand } from './skill/uninstall'; @@ -70,17 +66,14 @@ export const appCommandGroup: SubcommandGroupDefinition = { }, { name: 'create', - description: createDescription(), - // The `--distribution public` example is filtered out while public distribution - // is pre-GA (BEX-405) — `brevo app create --help` must not advertise a value the - // command will refuse. Filtered from the same table the refusal reads, so GA - // restores it without an edit here. + // Duplicated by hand in `lib/help.ts`'s hand-aligned root screen, which nothing + // propagates into — see the warning there. `help-surface.test.ts` asserts the two + // agree. + description: 'Create a new app (OAuth, or a UI app via the prompts)', examples: [ 'brevo app create', 'brevo app create --name "My App" --distribution private', - ...(isFeatureAvailable('public-distribution') - ? ['brevo app create --name "My App" --distribution public'] - : []), + 'brevo app create --name "My App" --distribution public', 'brevo app create --name "My App" --distribution private --redirect-uri http://localhost:3009/auth/callback', 'brevo app create --name "My App" --distribution private --redirect-uri http://localhost:3009/auth/callback --redirect-uri https://myapp.com/callback --json', 'brevo app create --name "My App" --distribution private --logo-uri https://example.com/logo.png', @@ -96,7 +89,7 @@ export const appCommandGroup: SubcommandGroupDefinition = { { flags: '--name ', description: 'App name' }, { flags: '--distribution ', - description: `Distribution type (${distributionValues()})`, + description: 'Distribution type (private|public)', }, { flags: '--redirect-uri ', @@ -281,10 +274,10 @@ export const appCommandGroup: SubcommandGroupDefinition = { port: opts.port as number | undefined, }), }, - // Moved here from ./preview-definitions.ts when UI apps went GA. `requires` stays: - // it is the capability the command applies to (UI apps only — see - // `src/app-types/capabilities.ts`), and with its `FEATURE_STAGE` row at 'ga' it no - // longer hides or refuses anything. + // `requires` is the capability these two commands apply to (UI apps only — see + // `src/app-types/capabilities.ts`). It is declarative metadata: the registry does + // not enforce it, each command refuses in its own words. Also the source of the + // "App-install commands (UI apps only)" heading in `lib/help.ts`. { name: 'install', requires: 'account-install', @@ -351,16 +344,84 @@ export const appCommandGroup: SubcommandGroupDefinition = { json: Boolean(opts.json), }), }, - // ELIMINATION SITE — the raw global rather than `isFeatureAvailable()` on purpose: - // esbuild substitutes the global here, folds the ternary to `[]`, and can then drop - // `previewAppCommands` and the three handler modules only it imports. Importing the - // constant instead leaves a runtime ternary and ships the whole gated surface. See - // src/globals.d.ts. + // `requires` is the capability these three commands apply to (public apps only — see + // `src/app-types/capabilities.ts`), on the same declarative terms as the two + // `account-install` commands above. // - // Appended, not interleaved, so the spread is one foldable expression. Ordering in - // `brevo app --help` is unaffected in a public build (there is nothing to order); - // a preview build simply lists these three last. - ...(__BREVO_PREVIEW__ ? previewAppCommands : []), + // Listed in lifecycle order — submit, status, withdraw — matching the App-review + // block in `formatRootHelp` so the two help renderers read the same way. + { + name: 'submit', + requires: 'review-lifecycle', + description: 'Submit a public app for review', + examples: [ + 'brevo app submit', + `brevo app submit --app-id ${EXAMPLE_APP_ID}`, + `brevo app submit --app-id ${EXAMPLE_APP_ID} --json`, + ], + options: [ + { + flags: '--app-id ', + description: 'App ID (uses app-config.json if omitted)', + parser: (v) => parseAppId(v), + }, + { + flags: '--json', + description: 'Print the submission form URL as JSON instead of opening a browser', + }, + ], + handler: (opts) => + submitCommand({ + appId: opts.appId as string | undefined, + json: Boolean(opts.json), + }), + }, + { + name: 'status', + requires: 'review-lifecycle', + description: "Show an app's review status", + examples: [ + 'brevo app status', + `brevo app status --app-id ${EXAMPLE_APP_ID}`, + `brevo app status --app-id ${EXAMPLE_APP_ID} --json`, + ], + options: [ + { + flags: '--app-id ', + description: 'App ID (uses app-config.json if omitted)', + parser: (v) => parseAppId(v), + }, + { flags: '--json', description: 'Output as JSON' }, + ], + handler: (opts) => + statusCommand({ appId: opts.appId as string | undefined, json: Boolean(opts.json) }), + }, + { + name: 'withdraw', + requires: 'review-lifecycle', + description: 'Withdraw an app from submission', + examples: [ + 'brevo app withdraw', + `brevo app withdraw --app-id ${EXAMPLE_APP_ID}`, + `brevo app withdraw --app-id ${EXAMPLE_APP_ID} --force`, + `brevo app withdraw --app-id ${EXAMPLE_APP_ID} --json`, + ], + options: [ + { + flags: '--app-id ', + description: 'App ID (uses app-config.json if omitted)', + parser: (v) => parseAppId(v), + }, + { flags: '--force', description: 'Skip confirmation (for CI)' }, + { flags: '--json', description: 'Output as JSON' }, + ], + handler: (opts) => + withdrawCommand({ + appId: opts.appId as string | undefined, + force: Boolean(opts.force), + json: Boolean(opts.json), + }), + }, ], }; diff --git a/src/commands/preview-definitions.ts b/src/commands/preview-definitions.ts deleted file mode 100644 index b03a1749..00000000 --- a/src/commands/preview-definitions.ts +++ /dev/null @@ -1,113 +0,0 @@ -/** - * Command definitions for features that have not shipped (BEX-405). - * - * These live in their own module for a build reason, not an organisational one. - * `definitions.ts` references this array behind `__BREVO_PREVIEW__`, which esbuild folds - * to `false` in a published build; the array then becomes unreachable and the bundler - * drops it *along with the three handler modules only it imports*. Inline in - * `definitions.ts` the imports would be live references and every gated command would - * ship, unreachable but present — which is the thing the build-time gate exists to - * avoid. `scripts/build.mjs` asserts on the output that they really are gone. - * - * `app install` / `app uninstall` used to live here too; they moved to - * `definitions.ts` when UI apps went GA. - * - * So: do not import anything from here anywhere else, and do not move these entries - * back into `definitions.ts` "now that they're gated" — the gate is what this - * separation implements. - * - * At GA, move the released entries back into `definitions.ts` and delete this file - * when it empties. See `RELEASE-CHECKLIST.md`. - */ -import type { CommandDefinition } from '../lib/command-registry'; -import { EXAMPLE_APP_ID } from '../lib/constants'; -import { parseAppId } from '../lib/validators'; - -import { statusCommand } from './app/status'; -import { submitCommand } from './app/submit'; -import { withdrawCommand } from './app/withdraw'; - -/** The `brevo app ` subcommands gated behind an unreleased feature. */ -export const previewAppCommands: CommandDefinition[] = [ - { - name: 'status', - requires: 'review-lifecycle', - description: "Show an app's review status", - examples: [ - 'brevo app status', - `brevo app status --app-id ${EXAMPLE_APP_ID}`, - `brevo app status --app-id ${EXAMPLE_APP_ID} --json`, - ], - options: [ - { - flags: '--app-id ', - description: 'App ID (uses app-config.json if omitted)', - parser: (v) => parseAppId(v), - }, - { flags: '--json', description: 'Output as JSON' }, - ], - handler: (opts) => - statusCommand({ appId: opts.appId as string | undefined, json: Boolean(opts.json) }), - }, - { - name: 'withdraw', - requires: 'review-lifecycle', - // Unlisted even in a preview build, unlike its four siblings here. The command works - // — QA suite 7 and the public-app smoke script both drive it, and `app upload`'s - // under-review refusal still points at it by name — it is simply not advertised on - // either help screen while the review lifecycle is being finished. - // - // This is the *second* renderer, not the only one: `lib/help.ts`'s hand-aligned root - // screen is a plain string that Commander's `hidden` cannot filter, so the matching - // `brevo app withdraw` lines were removed from its `review-lifecycle` section too. - // Un-hiding means editing both. See `RELEASE-CHECKLIST.md` → *Before public-apps GA*. - hidden: true, - description: 'Withdraw an app from submission', - examples: [ - `brevo app withdraw --app-id ${EXAMPLE_APP_ID}`, - `brevo app withdraw --app-id ${EXAMPLE_APP_ID} --force`, - `brevo app withdraw --app-id ${EXAMPLE_APP_ID} --json`, - ], - options: [ - { - flags: '--app-id ', - description: 'App ID', - parser: (v) => parseAppId(v), - }, - { flags: '--force', description: 'Skip confirmation (for CI)' }, - { flags: '--json', description: 'Output as JSON' }, - ], - handler: (opts) => - withdrawCommand({ - appId: opts.appId as string | undefined, - force: Boolean(opts.force), - json: Boolean(opts.json), - }), - }, - { - name: 'submit', - requires: 'review-lifecycle', - description: 'Submit a public app for review', - examples: [ - 'brevo app submit', - `brevo app submit --app-id ${EXAMPLE_APP_ID}`, - `brevo app submit --app-id ${EXAMPLE_APP_ID} --json`, - ], - options: [ - { - flags: '--app-id ', - description: 'App ID (uses app-config.json if omitted)', - parser: (v) => parseAppId(v), - }, - { - flags: '--json', - description: 'Print the submission form URL as JSON instead of opening a browser', - }, - ], - handler: (opts) => - submitCommand({ - appId: opts.appId as string | undefined, - json: Boolean(opts.json), - }), - }, -]; diff --git a/src/globals.d.ts b/src/globals.d.ts deleted file mode 100644 index e3b108df..00000000 --- a/src/globals.d.ts +++ /dev/null @@ -1,30 +0,0 @@ -/** - * Build-time globals substituted by esbuild (BEX-405). - * - * `__BREVO_PREVIEW__` is replaced with the literal `true` or `false` at every use site - * before parsing — see `define` in `scripts/build.mjs`. It is never a real global at - * runtime in a published build, because no reference to it survives substitution. - * - * **Why a bare global and not just the exported `PREVIEW_BUILD` constant?** Because - * esbuild folds a constant within the module that declares it but does *not* propagate - * it across module boundaries. `PREVIEW_BUILD` correctly becomes `false` inside - * `lib/build-flags.js`, yet an importing module still emits - * `PREVIEW_BUILD ? previewAppCommands : []` as a runtime ternary — which keeps - * `previewAppCommands` live and ships every gated command. Substituting a global - * instead makes the fold local to each use site, so the dead branch and everything it - * referenced can actually be eliminated. This was verified by inspecting the bundle, - * and `scripts/build.mjs` asserts it on every public build. - * - * Use `PREVIEW_BUILD` from `lib/build-flags` for ordinary branching. Reach for this - * global only where the goal is *elimination* — dropping a module or a block of help - * text from the published bundle — and say so in a comment at the site. - * - * Under jest nothing is substituted, so `jest.setup.js` defines it on `globalThis`. - * - * Declared `var` rather than `const` so it is reachable as `globalThis.__BREVO_PREVIEW__`, - * which is how the setup file assigns it and how the gate's tests flip build states - * between `jest.isolateModules` re-imports. In a real build the name never survives - * substitution, so the mutability this implies exists only under test. - */ -// eslint-disable-next-line no-var -declare var __BREVO_PREVIEW__: boolean; diff --git a/src/lang/en.ts b/src/lang/en.ts index e062ef7d..06f9da29 100644 --- a/src/lang/en.ts +++ b/src/lang/en.ts @@ -1,5 +1,4 @@ import { CLI, BREVO_CLI_REFERENCE_URL, BREVO_OAUTH_SCOPES_DOCS_URL } from '../lib/constants'; -import { previewMessages } from './preview-messages'; /** * How a scaffold report describes what it just did, given the files it wrote and the @@ -39,7 +38,7 @@ function numberedSteps(cdDir: string | undefined, steps: Array<[string, string]> ]; } -const coreMessages = { +export const messages = { // Update notifier UPDATE_AVAILABLE: (current: string, latest: string): string => `Update available: ${current} → ${latest}`, @@ -99,17 +98,6 @@ const coreMessages = { WHOAMI_CREDENTIAL_MISMATCH: (fields: string[]) => `Local credentials mismatch with API for: ${fields.join(', ')}. Run \`${CLI.LOGIN}\` to re-authenticate.`, - // Pre-GA gate (BEX-405). One message for every gated command, prompt choice and - // flag value — see `src/lib/preview.ts` for why this one is shared while the - // capability refusals each keep their own wording. - // - // Deliberately says nothing about the internal-account escape hatch or the env - // var: an end user cannot use either, so naming them would only invite an attempt. - // Both are documented where the people who need them will look — the agent docs - // and the README. - PREVIEW_FEATURE_UNAVAILABLE: - 'That command is not available yet. It is part of a Brevo feature that has not been released.', - // App create APP_CREATE_NAME_PROMPT: 'App name:', // A `list` choice prompt, same as APP_CREATE_APP_TYPE_PROMPT right below it — but this @@ -195,7 +183,7 @@ const coreMessages = { [CLI.APP_INSTALL(), 'make it available in an account'], ]), - // App create — UI app (BEX-290). Moved here from `preview-messages.ts` at UI-apps GA. + // App create — UI app (BEX-290). // Placement choices are read from the platform's extension-point registry at prompt // time (BEX-361) — fetch-only, no local fallback, so a partner can never author a slot // the platform doesn't have. Two loads: the record pages, then the placements on the @@ -283,7 +271,6 @@ const coreMessages = { APP_CREATE_UI_BOX_HINT: `Edit the \`ui_app\` block in app-config.json to change any of this — add more placements as extra \`surface_point_list\` entries, each with its own label and redirect link — then run \`${CLI.APP_UPLOAD}\`.`, // App install / uninstall — per-account availability for UI apps (BEX-290). - // Moved here from `preview-messages.ts` at UI-apps GA. APP_INSTALL_SELECT: 'Select an app to install:', /** * How an account is named in every install/uninstall line. @@ -516,11 +503,12 @@ const coreMessages = { LEGACY_ALL_SCOPE_SCAFFOLD_SUBSTITUTED: (writtenScopes: string): string => `This app still has the legacy 'all' OAuth scope (deprecated). Wrote ${writtenScopes} to app-config.json instead of 'all'. Migrate the app by editing \`auth.scopes\` and running \`${CLI.APP_UPLOAD}\`.`, LEGACY_ALL_SCOPE_UPDATE_MIGRATING: `Migrating from legacy 'all' scope — 'all' will be removed.`, - // Deliberately in core rather than `preview-messages`: the legacy-scope deprecation - // (BEX-214) is GA, and `app upload` — the only reader — ships in every build. It lived - // in `preview-messages` between BEX-405 and this fix, which meant a public build read - // the key as `undefined` and `new CliError(undefined)` rendered as a bare `✗` with no - // text. The build now refuses that class of leak; see `scripts/build.mjs`. + // The legacy-scope deprecation (BEX-214) is GA and `app upload` is its only reader. + // Worth knowing if a build gate is ever reintroduced (see `CLAUDE.md` → *If you ever + // need to gate a feature again*): this string spent BEX-405 parked in the gated + // strings module, which eliminated the definition while leaving the live read, so a + // published `app upload` read the key as `undefined` and `new CliError(undefined)` + // rendered as a bare `✗` with no text. LEGACY_ALL_SCOPE_DEPRECATED_BLOCK: `This app currently has the legacy 'all' OAuth scope, which is being deprecated.\n Replace 'all' with the specific scopes your integration uses in app-config.json's \`auth.scopes\`.\n Run \`${CLI.APP_SCOPES}\` to see the catalog, then run \`${CLI.APP_UPLOAD}\` to migrate.`, // App delete @@ -725,8 +713,6 @@ const coreMessages = { // `app start oauth`: an action link has no OAuth flow and no local server to run, // so the OAuth line pointed at a command that would fail. It also contradicted the // Next steps box printed directly above it, which already said upload → install. - // Deliberately in core rather than `preview-messages`: `init` is not a gated command, - // and a hand-edited `ui_app` block can reach this line in a published build. INIT_DONE_UI_APP: `All set! Follow the next steps above, or run \`${CLI.HELP}\` to see all commands.`, // Skill @@ -775,16 +761,93 @@ const coreMessages = { OAUTH_METADATA_FETCH_FAILED: (url: string, status: number): string => `Failed to fetch OAuth scopes from ${url} (HTTP ${status}).`, + // App submit / status / withdraw — the public-app review lifecycle (BEX-221 / BEX-251 + // / BEX-252 / BEX-383), GA at BEX-405. + // App submit (BEX-221) + APP_SUBMIT_CHECKING_STATUS: 'Checking app status...', + APP_SUBMIT_FETCHING: 'Fetching app...', + APP_SUBMIT_PICK_APP: 'Which app do you want to submit for review?', + APP_SUBMIT_NO_APP_RESOLVED: + 'Cannot determine which app to submit. Provide --app-id or run from a directory with app-config.json.', + APP_SUBMIT_NOT_FOUND: (appId: string): string => `App ${appId} not found.`, + APP_SUBMIT_OUT_OF_SYNC: (fields: string[], appId: string): string => + `Configuration mismatch detected — your local app-config.json differs from the app on Brevo (${fields.join(', ')}).\n Please update your local configuration with the latest server values, or run \`${CLI.APP_UPLOAD}\` to upload your local changes to the server, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, + APP_SUBMIT_OUT_OF_SYNC_DIFF: (diff: string, appId: string): string => + `Configuration mismatch detected — your local app-config.json differs from the app on Brevo:\n${diff}\n\n Please update your local configuration with the latest server values, or run \`${CLI.APP_UPLOAD}\` to upload your local changes to the server, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, + // Submittability gate (BEX-383). The state API reports which required fields the + // app still lacks; block before opening the form so the developer isn't sent to + // complete a submission the backend would reject (`422 app_not_submittable`). + // Two shapes, matching OUT_OF_SYNC above, both showing the raw server field keys + // (no local relabelling): a compact inline list for --json, a multiline list for + // humans. + APP_SUBMIT_NOT_SUBMITTABLE: (fields: string[], appId: string): string => + `Your app isn't ready to submit yet — required fields are still missing (${fields.join(', ')}).\n Add them to app-config.json, run \`${CLI.APP_UPLOAD}\`, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, + APP_SUBMIT_NOT_SUBMITTABLE_DIFF: (diff: string, appId: string): string => + `Your app isn't ready to submit yet — these required fields are still missing:\n${diff}\n\n Add them to app-config.json, run \`${CLI.APP_UPLOAD}\`, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, + APP_SUBMIT_IN_SYNC: + 'No configuration mismatch detected. Showing the submission confirmation prompt with the complete app configuration below.', + APP_SUBMIT_CONFIRM_HEADER: 'You are about to submit this app for review:', + APP_SUBMIT_CONFIRM_PROMPT: 'Submit this app for review?', + APP_SUBMIT_CANCELLED: 'Submission cancelled.', + APP_SUBMIT_FORM_GATE: + 'Note: Your app will be submitted for review only after you complete and submit the Google Form.', + APP_SUBMIT_BROWSER_OPENED: (url: string, appId: string): string => + `We've opened a browser tab with the submission form for app ${appId}:\n ${url}`, + APP_SUBMIT_BROWSER_FAILED: (url: string, appId: string): string => + `We couldn't open a browser automatically. Open the submission form for app ${appId} yourself:\n ${url}`, + APP_SUBMIT_NEXT_STEPS: `Please submit the form for review. You'll receive an email once your app has been reviewed — check its status anytime with \`${CLI.APP_STATUS}\`.`, + // TC-6.3. A never-uploaded app has no `app_versions` row, so the review-state read + // fails — and the server's copy for that failure names `name`, `logo_uri`, `scopes` + // and `redirect_uris` as the things to fix, which is misleading: all four can be + // present and the read still fails. Nothing in `apiCodeMessages` maps it, so it was + // relayed verbatim. Refused locally instead, before the read, on the one signal that + // is certain — an app with no `version` has never been uploaded. Same shape and + // reasoning as `assertInstallable`'s `requireUploaded` gate (`APP_INSTALL_NOT_UPLOADED`), + // and a separate string because this names a different precondition in its own voice. + APP_SUBMIT_NOT_UPLOADED: (appId: string): string => + `App ${appId} has never been uploaded, so it has no version to review.\n Run \`${CLI.APP_UPLOAD}\` first, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, + APP_SUBMIT_NO_FORM_URL: `Review submission is currently unavailable. This may happen if your app has not been uploaded yet or if it has already been submitted and is under review. You can check the current status of your app using \`${CLI.APP_STATUS}\`.`, + + // App status + APP_STATUS_SELECT: 'Select an app:', + APP_STATUS_TITLE: 'App status', + // Canned copy per review state (server-side `app_submission_states.state`). + // Reviewer feedback is delivered by email, not surfaced here (BEX-252). + APP_STATUS_MESSAGE: (state: string): string => { + switch (state) { + // Empty/missing state is normalized to the "unknown" sentinel upstream + // (src/commands/app/status.ts); '' is kept as a defensive fallthrough. + case '': + case 'unknown': + return `Status information isn't available for your app yet. Make sure your app is public and has been uploaded with \`${CLI.APP_UPLOAD}\`.`; + case 'draft': + return "Your app is set up but hasn't been submitted for review yet."; + case 'submitted': + return 'Your app has been submitted and is waiting to be reviewed.'; + case 'in_review': + return 'Your app is currently being reviewed by our team.'; + case 'approved': + return 'Your app has been approved.'; + case 'rejected': + return 'Your app was not approved. Check your email for details.'; + case 'changes_requested': + return 'Changes have been requested for your app. Check your email for details.'; + default: + return `Your app is in state "${state}".`; + } + }, + + // App withdraw + APP_WITHDRAW_SELECT: 'Select an app to withdraw:', + APP_WITHDRAW_CONFIRM: (name: string, id: string) => + `Withdraw app "${name}" (${id}) from submission?`, + APP_WITHDRAW_CANCELLED: 'Withdrawal cancelled.', + APP_WITHDRAW_SUCCESS: (id: string) => `App ${id} withdrawn from submission.`, + APP_WITHDRAW_NOT_SUBMITTED: (id: string) => `App ${id} has not been submitted yet.`, + APP_WITHDRAW_SUBMIT_HINT: (id: string) => `Submit it first: ${CLI.APP_SUBMIT(id)}`, + APP_SUBMIT_NOT_PUBLIC: (appId: string): string => + `App ${appId} is private. Private apps cannot be submitted for review. Only public apps are eligible for the approval process. Please make your app public before submitting it for review.`, + // General ABORTED: 'Aborted.', } as const; - -// ELIMINATION SITE — the raw global rather than `isFeatureAvailable`, so esbuild folds -// the spread to `{}` and drops ./preview-messages entirely. `messages` is one object -// literal, so a property can only be removed by removing the whole object it arrived in. -// The `as typeof previewMessages` cast keeps every call site type-safe in both builds; -// see that module for why the lie is safe. -export const messages = { - ...coreMessages, - ...(__BREVO_PREVIEW__ ? previewMessages : ({} as typeof previewMessages)), -}; diff --git a/src/lang/preview-messages.ts b/src/lang/preview-messages.ts deleted file mode 100644 index 2901d164..00000000 --- a/src/lang/preview-messages.ts +++ /dev/null @@ -1,88 +0,0 @@ -import { CLI } from '../lib/constants'; - -/** - * User-facing strings for features that have not shipped (BEX-405). - * - * Split out of `en.ts` for a build reason, not a tidiness one. `messages` is a single - * object literal, and esbuild cannot prune properties from one — so with these inline, - * every string for `app submit`, `app status` and `app withdraw` shipped in the - * published bundle even though no surviving code referenced them: `strings` on the - * binary read back the whole unreleased feature set. As a separate module spread in - * behind `__BREVO_PREVIEW__`, the object becomes unreachable and the bundler drops it. - * (The UI-app authoring and `app install` / `app uninstall` strings lived here too, - * until UI apps went GA and they moved back into `en.ts`.) - * - * `en.ts` types the spread as `typeof previewMessages` even when it is empty, so every - * call site stays type-safe. That is a deliberate lie about the runtime shape, and a - * safe one: the only code reading these keys lives in the modules eliminated alongside - * them, so nothing can observe the absence. - * - * At GA, move the released strings back into `en.ts` and delete this file when it - * empties. See `RELEASE-CHECKLIST.md`. - */ -export const previewMessages = { - // App submit (BEX-221) - APP_SUBMIT_CHECKING_STATUS: 'Checking app status...', - APP_SUBMIT_FETCHING: 'Fetching app...', - APP_SUBMIT_PICK_APP: 'Which app do you want to submit for review?', - APP_SUBMIT_NO_APP_RESOLVED: - 'Cannot determine which app to submit. Provide --app-id or run from a directory with app-config.json.', - APP_SUBMIT_NOT_FOUND: (appId: string): string => `App ${appId} not found.`, - APP_SUBMIT_OUT_OF_SYNC: (fields: string[], appId: string): string => - `Configuration mismatch detected — your local app-config.json differs from the app on Brevo (${fields.join(', ')}).\n Please update your local configuration with the latest server values, or run \`${CLI.APP_UPLOAD}\` to upload your local changes to the server, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, - APP_SUBMIT_OUT_OF_SYNC_DIFF: (diff: string, appId: string): string => - `Configuration mismatch detected — your local app-config.json differs from the app on Brevo:\n${diff}\n\n Please update your local configuration with the latest server values, or run \`${CLI.APP_UPLOAD}\` to upload your local changes to the server, then re-run \`${CLI.APP_SUBMIT(appId)}\`.`, - APP_SUBMIT_IN_SYNC: - 'No configuration mismatch detected. Showing the submission confirmation prompt with the complete app configuration below.', - APP_SUBMIT_CONFIRM_HEADER: 'You are about to submit this app for review:', - APP_SUBMIT_CONFIRM_PROMPT: 'Submit this app for review?', - APP_SUBMIT_CANCELLED: 'Submission cancelled.', - APP_SUBMIT_FORM_GATE: - 'Note: Your app will be submitted for review only after you complete and submit the Google Form.', - APP_SUBMIT_BROWSER_OPENED: (url: string, appId: string): string => - `We've opened a browser tab with the submission form for app ${appId}:\n ${url}`, - APP_SUBMIT_BROWSER_FAILED: (url: string, appId: string): string => - `We couldn't open a browser automatically. Open the submission form for app ${appId} yourself:\n ${url}`, - APP_SUBMIT_NEXT_STEPS: `Please submit the form for review. You'll receive an email once your app has been reviewed — check its status anytime with \`${CLI.APP_STATUS}\`.`, - APP_SUBMIT_NO_FORM_URL: `Review submission is currently unavailable. This may happen if your app has not been uploaded yet or if it has already been submitted and is under review. You can check the current status of your app using \`${CLI.APP_STATUS}\`.`, - - // App status - APP_STATUS_SELECT: 'Select an app:', - APP_STATUS_TITLE: 'App status', - // Canned copy per review state (server-side `app_submission_states.state`). - // Reviewer feedback is delivered by email, not surfaced here (BEX-252). - APP_STATUS_MESSAGE: (state: string): string => { - switch (state) { - // Empty/missing state is normalized to the "unknown" sentinel upstream - // (src/commands/app/status.ts); '' is kept as a defensive fallthrough. - case '': - case 'unknown': - return `Status information isn't available for your app yet. Make sure your app is public and has been uploaded with \`${CLI.APP_UPLOAD}\`.`; - case 'configured': - return "Your app is set up but hasn't been submitted for review yet."; - case 'submitted': - return 'Your app has been submitted and is waiting to be reviewed.'; - case 'in_review': - return 'Your app is currently being reviewed by our team.'; - case 'approved': - return 'Your app has been approved.'; - case 'rejected': - return 'Your app was not approved. Check your email for details.'; - case 'changes_requested': - return 'Changes have been requested for your app. Check your email for details.'; - default: - return `Your app is in state "${state}".`; - } - }, - - // App withdraw - APP_WITHDRAW_SELECT: 'Select an app to withdraw:', - APP_WITHDRAW_CONFIRM: (name: string, id: string) => - `Withdraw app "${name}" (${id}) from submission?`, - APP_WITHDRAW_CANCELLED: 'Withdrawal cancelled.', - APP_WITHDRAW_SUCCESS: (id: string) => `App ${id} withdrawn from submission.`, - APP_WITHDRAW_NOT_SUBMITTED: (id: string) => `App ${id} has not been submitted yet.`, - APP_WITHDRAW_SUBMIT_HINT: (id: string) => `Submit it first: ${CLI.APP_SUBMIT(id)}`, - APP_SUBMIT_NOT_PUBLIC: (appId: string): string => - `App ${appId} is private. Private apps cannot be submitted for review. Only public apps are eligible for the approval process. Please make your app public before submitting it for review.`, -} as const; diff --git a/src/lib/command-registry.ts b/src/lib/command-registry.ts index 9039d21a..77ad9a64 100644 --- a/src/lib/command-registry.ts +++ b/src/lib/command-registry.ts @@ -1,8 +1,6 @@ import { Command } from 'commander'; import type { Capability } from '../app-types/capabilities'; import { CliError } from './errors'; -import { FEATURE_STAGE, assertFeatureAvailable, isFeatureAvailable } from './preview'; -import type { PreviewFeature } from './preview'; import { removedCommandsIn } from './removed-commands'; import type { RemovedCommand } from './removed-commands'; @@ -29,11 +27,11 @@ export interface CommandDefinition { * `src/app-types/capabilities.ts`. * * **Declarative metadata, NOT a runtime guard.** The registry does not enforce it, and - * that is deliberate rather than unfinished: each gated command already throws its own - * tested message with its own exit code, and a generic interceptor here would replace - * them all with one string — which `CLAUDE.md` counts as a user-visible break for any - * script matching on it. Enforcement stays in the commands, via `assertCapability`, which - * reads the same table. + * that is deliberate rather than unfinished: each of these commands already throws its + * own tested message with its own exit code, and a generic interceptor here would + * replace them all with one string — which `CLAUDE.md` counts as a user-visible break + * for any script matching on it. Enforcement stays in the commands, via + * `assertCapability`, which reads the same table. * * What it is for: making the rule enumerable. `bin/index.ts` currently states it as prose * ("App-review commands (public apps only):") in a hand-aligned help block, and the agent @@ -45,14 +43,16 @@ export interface CommandDefinition { /** * Keep the command out of `brevo app --help` while leaving it registered and callable. * - * Distinct from the pre-GA gate below, which hides *and* refuses: this one hides only. - * The command runs exactly as it always did for anyone who types it — it just stops - * being advertised. Used for a command we don't want to put in front of users yet but - * still need working for QA and the smoke tests (`app withdraw`). + * Hides only — it never refuses. The command runs exactly as it always did for anyone + * who types it, it just stops being advertised. Used for a command we don't want to put + * in front of users yet but still need working for QA and the smoke tests (`app + * withdraw` was the last one, un-hidden when the review lifecycle shipped; nothing sets + * it today). * * Hiding is only half the job: the hand-aligned root screen in `lib/help.ts` is a * separate renderer that Commander's `hidden` cannot reach, so a command set hidden - * here must also be absent from `formatRootHelp`. `preview-gate.test.ts` asserts both. + * here must also be absent from `formatRootHelp`. `help-surface.test.ts` asserts both + * renderers agree. */ hidden?: boolean; handler: (opts: Record, ...args: unknown[]) => void | Promise; @@ -64,42 +64,19 @@ export interface SubcommandGroupDefinition { commands: CommandDefinition[]; } -/** - * The pre-GA feature a command's `requires` names, if any. - * - * `Capability` is the wider set — `oauth-flow`, `redirect-uris` and `scaffold-feature` - * are capabilities that no gate applies to. Only the names that also appear in - * `FEATURE_STAGE` are gateable, so the lookup is a membership test rather than a cast. - */ -export function previewFeatureOf(def: CommandDefinition): PreviewFeature | undefined { - if (!def.requires) return undefined; - return def.requires in FEATURE_STAGE ? (def.requires as PreviewFeature) : undefined; -} - /** * Register a flat command on the program. * - * A command gated behind an unreleased feature is registered `hidden` rather than - * skipped. Skipping would drop it from the parser too, so invoking it would produce - * Commander's `unknown command` — which tells the user the CLI has no such command, - * when in fact it has one that isn't released. Registering it hidden keeps the typed - * refusal (`assertFeatureAvailable`) and its exit code. - * - * Note this is a *feature* gate, not the capability gate `requires` is documented as - * not being. The distinction is real: a capability gate depends on which app you are - * acting on and each command answers it in its own words, while this one depends only - * on whether the feature has shipped and is the same answer for every command. That - * is why one interceptor is right here and wrong there. - * - * `def.hidden` is the other route to an unlisted command, and it is not the same thing: - * it suppresses the help entry and nothing else. The command still parses, still runs, - * and gains no refusal from being hidden. + * Every declared command is registered and callable. `def.hidden` is the only route to + * an unlisted command and it suppresses the help entry and nothing else — the command + * still parses and still runs. A command that should *refuse* says so itself, in its own + * words, via `assertCapability`; there is deliberately no interceptor here, for the + * reason `CommandDefinition.requires` gives. */ function registerCommand(parent: Command, def: CommandDefinition): void { - const gatedBehind = previewFeatureOf(def); - const gateHides = Boolean(gatedBehind) && !isFeatureAvailable(gatedBehind!); - const hidden = def.hidden === true || gateHides; - const cmd = parent.command(def.name, { hidden }).description(def.description); + const cmd = parent + .command(def.name, { hidden: def.hidden === true }) + .description(def.description); if (def.arguments) { for (const arg of def.arguments) { @@ -125,11 +102,6 @@ function registerCommand(parent: Command, def: CommandDefinition): void { } cmd.action((...actionArgs) => { - // Re-checked here rather than reusing `hidden` above: that was computed at - // registration, and the refusal must reflect the state at invocation. Same answer - // in practice, but the gate reads the credentials file and the env, and neither - // belongs frozen in a module-init constant. - if (gatedBehind) assertFeatureAvailable(gatedBehind); // Commander passes positional args first, then options object, then Command const opts = actionArgs.at(-2) as Record; const positionalArgs = actionArgs.slice(0, -2); diff --git a/src/lib/constants.ts b/src/lib/constants.ts index 747da6bd..e3950b00 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -1,5 +1,4 @@ import { CliError } from './errors'; -import { previewCli, previewEndpoints } from './preview-constants'; // Track whether URL suffix parts were stripped for deferred warning (avoid side effects at import time) let strippedUrlSuffix: string | undefined; @@ -147,14 +146,21 @@ const coreEndpoints = { // record-page prompt and then narrows the row read with `?location=`, rather than // pulling the whole registry to derive the same handful of strings client-side. APP_STORE_SURFACE_POINT_LOCATIONS: '/v3/app-store/surface-points/locations', + // The public-app review lifecycle (BEX-405). `APP_STATE` is the canonical review-state + // read, shared by + // `app status` and `app submit`'s preflight; the withdraw route answers 404 for both + // "no such app" and "no such submission" — see `uninstallApp`/`withdrawApp` in + // `services/app.ts` for why neither can be told apart by status code. + APP_STATE: (appId: string): string => `/v3/app-store/apps/${encodeURIComponent(appId)}/state`, + APP_STORE_APP_WITHDRAW: (appId: string): string => + `/v3/app-store/apps/${encodeURIComponent(appId)}/withdraw`, OAUTH_AUTHORIZE: '/oauth/authorize', OAUTH_TOKEN: '/oauth/token', } as const; -// ELIMINATION SITE — see the note on `CLI` below; same reasoning, same shape. +// Kept as a wrapper around `coreEndpoints` — see the note on `CLI` below. export const ENDPOINTS = { ...coreEndpoints, - ...(__BREVO_PREVIEW__ ? previewEndpoints : ({} as typeof previewEndpoints)), }; /** @@ -170,7 +176,7 @@ export const ENDPOINTS = { */ export const EXAMPLE_APP_ID = '3f8c1a2e-5b47-4d9c-8e10-6a2b7d4f0c93'; -const coreCli = { +export const CLI = { LOGIN: 'brevo login', INIT: 'brevo app init', HELP: 'brevo --help', @@ -208,20 +214,18 @@ const coreCli = { APP_START: (feature?: string) => feature ? `brevo app start ${feature}` : 'brevo app start ', APP_SCOPES: 'brevo app available-scopes', + // The public-app review lifecycle (BEX-405). `APP_STATUS` takes no app ID because every message quoting it is + // already about a resolved app; the other two take an optional one so an error can name + // the exact app to re-run against, falling back to a `` placeholder. + APP_STATUS: 'brevo app status', + APP_SUBMIT: (appId?: string): string => + appId ? `brevo app submit --app-id ${appId}` : 'brevo app submit --app-id ', + APP_WITHDRAW: (appId?: string): string => + appId ? `brevo app withdraw --app-id ${appId}` : 'brevo app withdraw --app-id ', SKILL_INSTALL: 'brevo skill:cli install', SKILL_UNINSTALL: 'brevo skill:cli uninstall', } as const; -// ELIMINATION SITE — the raw global rather than `isFeatureAvailable`, so esbuild folds -// the spread to `{}` and drops ./preview-constants entirely. `CLI` is one object literal, -// so a property can only be removed by removing the whole object it arrived in. The -// `as typeof previewCli` cast keeps every call site type-safe in both builds; see that -// module for why the lie is safe. -export const CLI = { - ...coreCli, - ...(__BREVO_PREVIEW__ ? previewCli : ({} as typeof previewCli)), -}; - export const DEFAULT_APP_FOLDER = 'my-app'; export const DEFAULT_PORT = 3009; export const DEFAULT_REDIRECT_URI = `http://localhost:${DEFAULT_PORT}/auth/callback`; diff --git a/src/lib/help.ts b/src/lib/help.ts index 7f8581a8..5bf8a20a 100644 --- a/src/lib/help.ts +++ b/src/lib/help.ts @@ -1,32 +1,5 @@ import { Command, Help } from 'commander'; import { BREVO_CLI_REFERENCE_URL } from './constants'; -import { isFeatureAvailable } from './preview'; -import type { PreviewFeature } from './preview'; - -/** - * A block of help lines that only renders when its feature has shipped. - * - * The gated sections here line up 1:1 with the `requires` values in - * `commands/definitions.ts`, which is what makes `FEATURE_STAGE` the single source - * of truth for both the help screen and the runtime refusal (BEX-405). Keeping the - * capability name on the section rather than hardcoding a boolean is the whole - * point: flipping the table at GA restores these sections with no edit here. - */ -function gatedSection(feature: PreviewFeature, lines: string[]): string[] { - return isFeatureAvailable(feature) ? lines : []; -} - -/** Accepted values of `--distribution`, narrowed while public distribution is pre-GA. */ -export function distributionValues(): string { - return isFeatureAvailable('public-distribution') ? 'private|public' : 'private'; -} - -/** `app create`'s one-line description — mentions UI apps only when they're offered. */ -export function createDescription(): string { - return isFeatureAvailable('ui-app-type') - ? 'Create a new app (OAuth, or a UI app via the prompts)' - : 'Create a new OAuth app'; -} /** * The root `brevo --help` screen. @@ -34,13 +7,14 @@ export function createDescription(): string { * Hand-aligned rather than generated: it groups commands by what they're for * (app / install / review / skill / scope) and shows each one's flags inline, * which Commander's default two-column layout can't express. The grouping - * mirrors the capability gates in `commands/definitions.ts` — see + * mirrors the capability declarations in `commands/definitions.ts` — see * `command-capabilities.test.ts`, which is the executable copy of that rule. * - * Because it is hand-aligned, the pre-GA sections have to be filtered here too — - * Commander's `hidden` flag governs its own generated output (`brevo app --help`) - * and cannot reach this string. Both read `isFeatureAvailable`, so there is one - * decision and two renderers, not two decisions. + * **This screen is a second renderer, and nothing propagates into it.** Commander's + * `hidden` flag governs only its own generated output (`brevo app --help`) and cannot + * reach this string, so a command added, removed or hidden in `definitions.ts` has to be + * added, removed or hidden here by hand in the same change. `help-surface.test.ts` + * asserts the two agree, and exists because they have silently disagreed before. */ function formatRootHelp(description: string): string { return [ @@ -59,12 +33,9 @@ function formatRootHelp(description: string): string { ``, `App commands:`, ` brevo app init Quick setup — login, create app, and scaffold`, - // `--distribution` itself is GA — only the `public` VALUE is gated, so the flag - // stays and its value list narrows. Same for the app-type prompt: a locked run - // is OAuth-only, so the description stops advertising a choice it won't offer. - ` brevo app create [--name] [--distribution ${distributionValues()}]`, + ` brevo app create [--name] [--distribution private|public]`, ` [--redirect-uri ...] [--logo-uri ] [--json]`, - ` ${createDescription()}`, + ` Create a new app (OAuth, or a UI app via the prompts)`, ` brevo app list [--json] List all apps in your account`, ` brevo app credentials [--app-id ] [--reveal-secret] [--json]`, ` Show an app's client ID and secret`, @@ -74,47 +45,28 @@ function formatRootHelp(description: string): string { ` brevo app delete [--app-id ] [--force] [--json]`, ` Delete an app`, ``, - // GA (UI apps shipped): `account-install` is 'ga' in FEATURE_STAGE, so this renders - // in every build. What GA removed is the `__BREVO_PREVIEW__` wrapper (still on the - // review-lifecycle block below) — the gatedSection call stays, so the section keeps - // lining up 1:1 with the commands' `requires: 'account-install'`: an emergency flip - // of the row back to 'preview' hides it here exactly as registerCommand hides the - // commands, instead of root help advertising what the runtime refuses. - ...gatedSection('account-install', [ - `App-install commands (UI apps only):`, - ` brevo app install [account-id] [--app-id ] [--force] [--json]`, - ` Install an app into an account`, - ` brevo app uninstall [account-id] [--app-id ] [--force] [--json]`, - ` Uninstall an app from an account`, - ``, - ]), - // ELIMINATION SITE — `__BREVO_PREVIEW__` wraps the call rather than living inside a - // helper, because an array passed as a function *argument* is still evaluated: a - // `previewOnlySection(feature, [...])` helper left every one of these lines in the - // published bundle as a readable string. Folding `false ? … : []` at the call site is - // what removes the array itself. + // Groups the two commands whose `requires` is 'account-install' (UI apps only). + // The heading is the prose copy of that rule; `command-capabilities.test.ts` holds + // the executable one. + `App-install commands (UI apps only):`, + ` brevo app install [account-id] [--app-id ] [--force] [--json]`, + ` Install an app into an account`, + ` brevo app uninstall [account-id] [--app-id ] [--force] [--json]`, + ` Uninstall an app from an account`, + ``, + // The three commands whose `requires` is 'review-lifecycle' (public apps only). // - // **The build flag is therefore the outer authority for help text, above - // `FEATURE_STAGE`.** At GA that is a trap — flipping a row to `'ga'` is not enough, - // since a published build still has `__BREVO_PREVIEW__ === false` and would keep - // hiding the restored section. The wrapper must be removed by hand at the same time - // (exactly what happened to the App-install section above at UI-apps GA); the GA - // runbook (`RELEASE-CHECKLIST.md` on `feature_set-brevo-cli-v2`) lists it, alongside - // the identical trap in `commands/preview-definitions.ts`. - ...(__BREVO_PREVIEW__ - ? gatedSection('review-lifecycle', [ - `App-review commands (public apps only):`, - ` brevo app submit [--app-id ] [--json] Submit a public app for review`, - ` brevo app status [--app-id ] [--json] Show an app's review status`, - // `brevo app withdraw` is deliberately absent. It is registered and callable in - // a preview build, just marked `hidden` in `commands/preview-definitions.ts` so - // it is advertised on neither help screen. Commander's `hidden` governs its own - // generated output and cannot reach this hand-written string, so the omission - // has to be made here by hand — the same two-renderers/one-decision split the - // comment above describes for the gate. Restore both together. - ``, - ]) - : []), + // `brevo app withdraw` is the worked example of the two-renderer warning above: it + // was marked `hidden` while the review lifecycle was being finished, which suppressed + // its Commander help entry and did nothing at all to this string — so its two lines + // had to be removed here by hand, and restored the same way when it was un-hidden. + // Never assume `hidden` covers this screen. + `App-review commands (public apps only):`, + ` brevo app submit [--app-id ] [--json] Submit a public app for review`, + ` brevo app status [--app-id ] [--json] Show an app's review status`, + ` brevo app withdraw [--app-id ] [--force] [--json]`, + ` Withdraw an app from submission`, + ``, `Skill commands:`, ` brevo skill:cli install [--json] Install the brevo-cli Claude Code skill`, ` brevo skill:cli uninstall [--json] Remove the brevo-cli skill`, diff --git a/src/lib/preview-constants.ts b/src/lib/preview-constants.ts deleted file mode 100644 index 0abbf138..00000000 --- a/src/lib/preview-constants.ts +++ /dev/null @@ -1,40 +0,0 @@ -/** - * Command references and endpoints for features that have not shipped (BEX-405). - * - * Split out of `constants.ts` for the same build reason `preview-messages.ts` was split - * out of `en.ts`: `CLI` and `ENDPOINTS` are each a single object literal, and esbuild - * cannot prune properties from one — so with these inline, `brevo app submit --app-id - * `, `brevo app withdraw --app-id `, `brevo app status` and the `/withdraw` and - * `/state` paths all shipped in the published bundle at zero references. Nothing could - * reach them (no command is registered, no help lists them), but `strings` on the - * published binary read back the names of three unreleased commands — which is exactly - * what the build-level gate exists to prevent. As separate objects spread in behind - * `__BREVO_PREVIEW__`, they become unreachable and the bundler drops them. - * - * `constants.ts` types each spread as `typeof previewCli` / `typeof previewEndpoints` - * even when it is empty, so every call site stays type-safe. That is a deliberate lie - * about the runtime shape, and a safe one for the same reason it is safe in `en.ts`: - * the only code reading these keys (`commands/app/submit.ts`, `status.ts`, `withdraw.ts` - * and `appService.fetchAppState` / `withdrawApp`) lives in modules eliminated alongside - * them, so nothing can observe the absence. - * - * At GA, move these back into `constants.ts` and delete this file when it empties. - * See `RELEASE-CHECKLIST.md`. - */ - -// Read only by `lang/preview-messages.ts` and `commands/app/withdraw.ts`. -export const previewCli = { - APP_STATUS: 'brevo app status', - APP_WITHDRAW: (appId?: string): string => - appId ? `brevo app withdraw --app-id ${appId}` : 'brevo app withdraw --app-id ', - APP_SUBMIT: (appId?: string): string => - appId ? `brevo app submit --app-id ${appId}` : 'brevo app submit --app-id ', -} as const; - -// Read only by `appService.fetchAppState` (the state read behind `app status` / -// `app submit`) and `appService.withdrawApp`. -export const previewEndpoints = { - APP_STATE: (appId: string): string => `/v3/app-store/apps/${encodeURIComponent(appId)}/state`, - APP_STORE_APP_WITHDRAW: (appId: string): string => - `/v3/app-store/apps/${encodeURIComponent(appId)}/withdraw`, -} as const; diff --git a/src/lib/preview.ts b/src/lib/preview.ts deleted file mode 100644 index 10616a57..00000000 --- a/src/lib/preview.ts +++ /dev/null @@ -1,105 +0,0 @@ -/** - * The pre-GA gate (BEX-405). - * - * Public app distribution is built in this repo but **not live on the Brevo - * platform**. The published package must not expose it: not in help, not to a direct - * invocation, and — because the guard is applied at build time — not in the shipped - * code at all. `scripts/build.mjs` eliminates every gated branch and tree-shakes the - * command modules only those branches referenced. (UI apps — the *UI app* create - * choice and `app install` / `app uninstall` — went GA and ship in every build; their - * rows below are flipped to `'ga'` and their modules are referenced live.) - * - * ## Why this has no runtime escape hatch - * - * The first version of this gate unlocked on an `@brevo.com` / `@sendinblue.com` - * account or an opt-in env var, mirroring the clause the agent docs used to carry. - * Both were removed when the flag moved to build time. A compile-time guard that any - * user can switch back on is a runtime guard wearing a costume — and worse, it has to - * ship the surface in order to be able to reveal it, which defeats the point of - * building it out. Internal testing is `PREVIEW=1 yarn link:dev`, which produces a - * genuinely different artifact. - * - * That also means this is no longer "a guardrail, not a security boundary": there is - * nothing client-side left to bypass. The Brevo API remains the real authority and - * refuses the gated feature per account independently (`400 invalid_parameter` on a - * public create), so the two layers are the build and the server, with nothing in - * between for a user to talk their way past. - * - * ## One table for readiness — but GA is a sequence, not one edit - * - * `FEATURE_STAGE` is the only place a feature's readiness is *stated*. Help filtering, - * the runtime refusal, the command registry and the two `app create` prompts all read - * it through `isFeatureAvailable`. Flipping a row to `'ga'` is necessary but NOT - * sufficient for a command: gated definitions live in `commands/preview-definitions.ts` - * and gated help sections sit behind `__BREVO_PREVIEW__`, a *build* flag, so both must - * be moved/unwrapped by hand in the same change — UI-apps GA (BEX-290) touched 17 files - * doing exactly that. The full sequence is the GA runbook, `RELEASE-CHECKLIST.md` on - * `feature_set-brevo-cli-v2` → *Before public-apps GA*. - * - * Two of the four names are `Capability` values from `app-types/capabilities.ts`, - * deliberately: commands already declare `requires` in `commands/definitions.ts`, so - * command gating falls straight out of the field that is already there. The other two - * gate a prompt choice and a flag value, neither of which is a command. - */ -import { CliError } from './errors'; -import { messages } from '../lang/en'; - -export type PreviewFeature = - /** `app install` / `app uninstall`. Also a `Capability`. */ - | 'account-install' - /** `app submit` / `app status` / `app withdraw`. Also a `Capability`. */ - | 'review-lifecycle' - /** The *UI app* choice in `app create`'s app-type prompt. */ - | 'ui-app-type' - /** `app create --distribution public`. */ - | 'public-distribution'; - -export type FeatureStage = 'ga' | 'preview'; - -/** - * Readiness per feature. Flipping a row to `'ga'` is the first step of releasing it — - * a gated *command* also needs its definition moved out of `preview-definitions.ts` - * and its help section unwrapped, see the header. - * - * Everything not listed here is GA by construction — absence from this table is not a - * gate, so a new command is public unless someone opts it in. - */ -export const FEATURE_STAGE: Readonly> = { - 'account-install': 'ga', - 'review-lifecycle': 'preview', - 'ui-app-type': 'ga', - 'public-distribution': 'preview', -} as const; - -/** - * Is this feature usable in this build — either released, or built with `PREVIEW=1`? - * - * Reads the build global directly rather than a module-level constant, for two reasons. - * esbuild substitutes it here, so a published build folds this to - * `FEATURE_STAGE[feature] === 'ga'` with no runtime flag left. And under jest the read - * happens per call, which is what lets a test flip build states without re-importing - * every module that has already captured a constant — the bug that a `PREVIEW_BUILD` - * export caused when this was first written. - */ -export function isFeatureAvailable(feature: PreviewFeature): boolean { - return FEATURE_STAGE[feature] === 'ga' || __BREVO_PREVIEW__; -} - -/** - * Refuse a gated feature. - * - * One message for all four, unlike `assertCapability` in `app-types/capabilities.ts`, - * which takes the caller's wording so each command keeps the error string it shipped - * with. Not an inconsistency: those are existing contracts a script may match on, - * while this is new surface with no callers to break, and it answers a different - * question ("this feature isn't released") than a capability error does ("this app - * doesn't support that"). - * - * Reachable in a published build only through `app create --distribution public` — - * the flag parses before the gate sees it, so the value has to be refused rather than - * hidden. Every gated *command* is gone from the binary and never reaches here. - */ -export function assertFeatureAvailable(feature: PreviewFeature): void { - if (isFeatureAvailable(feature)) return; - throw new CliError(messages.PREVIEW_FEATURE_UNAVAILABLE); -} diff --git a/src/types.ts b/src/types.ts index f19172cd..fa08ebad 100644 --- a/src/types.ts +++ b/src/types.ts @@ -375,9 +375,14 @@ export interface OAuthApp { * `app_submission_states.state` enum (BEX-318). Kept as a union for reference; * `AppStateResponse.state` stays a plain string so a server-added state never * breaks the read path — the CLI maps unknown states to a generic message. + * + * The backend (BEX-382) renamed the initial state `configured` → `draft` (the + * server migration renames every existing row, so `configured` no longer appears + * on the wire) and made `draft` the state an app enters at creation; this CLI + * change (BEX-383) reads the new value. */ export type AppState = - | 'configured' + | 'draft' | 'submitted' | 'in_review' | 'approved' @@ -388,6 +393,18 @@ export interface AppStateResponse { // Optional: the read path tolerates a missing/empty state and normalizes it // to an "unknown" sentinel (see src/commands/app/status.ts). state?: string; + // Submittability (BEX-383), meaningful only for public apps — the server + // computes it from the app's latest snapshot. `submittable` is true exactly + // when `missing_fields` is empty; `missing_fields` lists the server field keys + // still required (e.g. `logoLink`, `oauth.scopes`). Both are optional so an + // older server that omits them is tolerated — `app submit` only gates on an + // explicit `submittable === false`. + submittable?: boolean; + missing_fields?: string[]; + // Human-readable status message the server computes for the current state. + // Optional so an older server that omits it falls back to the CLI's canned + // per-state copy (APP_STATUS_MESSAGE); a blank string falls back too. + message?: string; } /**