test: BEX-339 — cover the public-app lifecycle in the smoke test - #42
Open
Tribhuvan14 wants to merge 6 commits into
Open
test: BEX-339 — cover the public-app lifecycle in the smoke test#42Tribhuvan14 wants to merge 6 commits into
Tribhuvan14 wants to merge 6 commits into
Conversation
Rewrite `scripts/smoke-test.ts` around two lifecycles against a real backend: the private app (create → credentials → upload → verify rename → scaffold → start → delete) and the public app (create → upload → status → submit → submit again → status → withdraw → status → delete). Removed `stepPublicAppRejected` — `--distribution public` is valid now, so asserting the CLI rejects it was wrong. Replaced the `brevo app update` step with `brevo app upload` steps (that command no longer exists), and dropped the `--app-id` the scaffold step was passing (that flag is gone too), so those two steps stop failing on the current CLI. Every `create` now runs from a tracked tmp work root, because `create` writes `./<slug>` into the cwd — previously the run littered the directory it was started from. Both apps and every tmp dir are tracked on `State`, so the existing trap paths tear them down on success, failure and SIGINT. Negative probes assert a mapped message from `src/lang/en.ts` *and* an exit code from `src/lib/exit-codes.ts`, and reject raw stack traces: invalid `--distribution`, `upload` with no `app-config.json`, `submit` with no resolvable app, `submit` on a private app, unknown app ids, and withdraw's 422 → `NOT_SUBMITTED` at exit 0. Steps that need a command the installed CLI lacks are feature-detected from `brevo --help` and reported as skipped rather than failed, which keeps `--against=published` green while the sibling commands land. New `--with-public` / `--skip-public` flags; the public flow runs by default. The `--report=` artefact now goes through `redact()` like the log file. Test-only — no `src/` changes, so no agent-doc update is required. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… probe First live run surfaced this: `brevo app submit --app-id <private-app>` exits 1 with the API's `This activity is not supported for private apps.`, not the CLI's `APP_SUBMIT_NOT_PUBLIC` copy. `checkAppStatus` runs the review-state preflight before the `distribution_type !== 'public'` check, and the API refuses that read for a private app, so the CLI's own message is unreachable. The refusal is correct, so the probe accepts either string (and still asserts the exit code and the absence of a stack trace). The message ordering is a CLI issue, not a test issue — recorded in TODO.md rather than fixed here, to keep this change test-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ticks the six server-behaviour items that only a real backend could settle, with the observed values, and notes that submit did return a review-form link on prod so the public path ran for real. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces substantial new CLI functionality around app upload and the public-app review lifecycle (status/submit/withdraw), updates scaffolding/templates to branch by distribution (PKCE for public apps), and refreshes related docs and tests. Note: the current PR title/description frames the change as “smoke test only,” but the diff includes many src/ changes plus multiple changesets and documentation updates.
Changes:
- Replace
brevo app updatewithbrevo app upload, including wire-shape types, config migrations, and test coverage. - Add public-app review lifecycle commands:
brevo app status,brevo app submit, andbrevo app withdraw, plus supporting service methods and UI output. - Add distribution-aware template conditionals and update scaffold templates for PKCE (public) vs confidential-client (private).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| TODO.md | Branch-local work tracker added. |
| src/types.ts | Adds version/form-link fields and new payload/response types for upload/state. |
| src/templates/index.ts | Adds conditional templating + splits base vs feature manifests. |
| src/templates/files/src/oauth/token-store.js.tmpl | Branches PKCE verifier storage for public apps. |
| src/templates/files/src/oauth/server.js.tmpl | Branches env requirements + uses shared authorize URL builder. |
| src/templates/files/src/oauth/handler.js.tmpl | Adds PKCE flow for public apps and shared URL builder export. |
| src/templates/files/src/oauth/.env.local.tmpl | Omits CLIENT_SECRET for public apps. |
| src/templates/files/src/oauth/.env.example.tmpl | Public/private credential guidance and PKCE note. |
| src/templates/files/README.md.tmpl | Updates guidance from update → upload workflow. |
| src/templates/files/CLAUDE.md.tmpl | Documents public/private PKCE differences in scaffolded flow. |
| src/templates/files/app-config.json.tmpl | Writes version + distribution_type in scaffolded config. |
| src/templates/files/AGENTS.md.tmpl | Documents PKCE vs confidential-client branching. |
| src/services/scopes-html.ts | Removes outdated mention of app update --scope from comment. |
| src/services/app.ts | Adds fetchAppState/uploadApp/withdrawApp service methods. |
| src/lib/ui.ts | Adds printStatusCard() for status-style output. |
| src/lib/logger.ts | Exposes color helpers for shared UI formatting. |
| src/lib/constants.ts | Adds new endpoints + CLI strings (upload/status/submit/withdraw), removes update/scaffold app-id pattern. |
| src/lib/config.ts | Migrates config shape to distribution_type + adds server backfill helper. |
| src/commands/login.ts | Updates next-steps output to new scaffold CLI constant. |
| src/commands/init.ts | Adjusts scaffold invocation to be directory/config driven. |
| src/commands/definitions.ts | Registers new commands and replaces update with upload; updates scaffold options. |
| src/commands/app/withdraw.ts | New withdraw command implementation. |
| src/commands/app/upload.ts | New upload command implementation (diff + confirm + POST + writeback). |
| src/commands/app/update.ts | Removes the legacy update command. |
| src/commands/app/submit.ts | New submit command implementation with status preflight + drift checks. |
| src/commands/app/status.ts | New status command implementation using status card + JSON output. |
| src/commands/app/select-app.ts | Shared interactive app picker for delete/withdraw. |
| src/commands/app/list.ts | Adds version output and updates update→upload wording. |
| src/commands/app/delete.ts | Refactors picker into shared helper. |
| src/commands/app/credentials.ts | Backfills version/distribution_type into legacy app-config.json. |
| src/commands/app/create.ts | Enables public distribution and refactors create→scaffold flow. |
| src/bin/index.ts | Updates top-level help output to include new commands (but needs scaffold flag corrections). |
| src/api/client.ts | Surfaces error field when API omits message. |
| src/tests/templates/handler.test.ts | Expands template tests for public/private PKCE branching. |
| src/tests/templates/conditionals.test.ts | Adds direct tests for conditional template stripping. |
| src/tests/services/app.test.ts | Adds tests for new service methods (state/upload/withdraw). |
| src/tests/lib/constants.test.ts | Updates CLI constant expectations for new commands. |
| src/tests/lib/config.test.ts | Adds tests for distribution_type migration + backfill helper. |
| src/tests/lang/en.test.ts | Updates message expectations to reflect upload/scaffold changes. |
| src/tests/commands/init.test.ts | Updates init behavior expectations for scaffold call. |
| src/tests/commands/app/withdraw.test.ts | Adds withdraw command tests. |
| src/tests/commands/app/upload.test.ts | Adds upload command tests. |
| src/tests/commands/app/status.test.ts | Adds status command tests (JSON + card rendering). |
| src/tests/commands/app/start.test.ts | Updates error text expectation from update → upload. |
| src/tests/commands/app/scopes.test.ts | Updates scope guidance expectation from update → upload. |
| src/tests/commands/app/list.test.ts | Adds version output assertions. |
| src/tests/commands/app/credentials.test.ts | Adds backfill behavior assertions. |
| src/tests/api/client.test.ts | Adds tests for error-field fallback and precedence. |
| RELEASE-CHECKLIST.md | Introduces durable GA checklist + per-branch verification section. |
| README.md | Adds public-app availability notice; edits create row (currently inconsistent). |
| CLAUDE.md | Adds public-app notice rationale + working-docs policy. |
| AGENTS.md | Adds public-app notice rationale for repo agents. |
| agent-context/SKILL.md | Adds public-app “not available yet” policy + updated command guidance. |
| agent-context/AGENTS.md | Adds public-app “not available yet” policy + updated command guidance. |
| .changeset/public-apps-not-available-notice.md | New changeset for doc notice. |
| .changeset/enable-public-app-distribution.md | New changeset for public distribution enablement. |
| .changeset/curly-status-command.md | New changeset for status command. |
| .changeset/app-upload-replaces-update.md | New changeset for update→upload replacement. |
| .changeset/app-submit-command.md | New changeset for submit command. |
| .changeset/add-app-withdraw.md | New changeset for withdraw command. |
| .changeset/add-app-version-config.md | New changeset for version/config/scaffold changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3
to
+4
| Running work tracker for this branch. Delete before merging into `main` | ||
| (see `CLAUDE.md` → *Working docs*). |
The second live run exposed three defects, all in the smoke script. `trapDeleteApps` logged `trap: deleted app <id>` without checking the exit status. spawnSync doesn't throw on a non-zero exit, so a delete that 401'd was recorded as a success and the orphan went unreported — worse than no cleanup, because nobody goes looking. It now checks the status, logs the real reason, and prints an orphan block with the delete commands. `Logout` and `Final cleanup` ran as ordinary steps before the post-run safety net, destroying the credentials and the linked binary that net needs — so an app left behind by a failed delete could never be recovered. Added a `Cleanup: leftover apps` step ahead of both. A rate-limited API failed every later step, and made the negative probes assert mapped messages against "Rate limited. Retrying in 5 seconds...". `exec()` now retries centrally (5s/15s/30s) when a failed call looks rate-limited, and counts the waits. Leaks and throttling surface in the summary and the --report= JSON as `orphanedAppIds` / `rateLimitWaits`. Also fixes the seven Sonar smells the PR raised in this file. The regex one is a real ReDoS: `\n\s+at .+:\d+:\d+` backtracks super-linearly because `\s` also matches the newline and `.+` competes with the `:line:col` tail, so the stack-frame check is now two anchored per-line patterns. Zero security hotspots either way. Verified against a mock CLI: transient rate limit absorbed (one 5s wait, run green); every delete failing reports LEAKED with both ids and matches what's actually left on the account; clean run 26/26; gated run 14 passed / 12 skipped. A live re-run is still outstanding and noted in RELEASE-CHECKLIST.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sonar S4036 (the only new vulnerability, and what dropped the PR's security rating to B): spawning `brevo` by bare name searches PATH on every call, so a writable PATH entry could shadow the binary under test. The rule fired now because the previous commit rewrote one of those lines, turning a long-standing pattern into "new code". Worth fixing rather than marking Safe, because the practical failure is one we already hit: a live run resolved `brevo` to an unrelated `@dtsl/brevo-cli` install that predates the public-app commands, which would have skipped nearly every step and reported green. stepReinstall now resolves the binary once and stores it on State, and all 33 invocation sites go through brevoCmd(), pinning the whole run to the build that was just installed and verified. The bare name survives only as a fallback for trap paths that can fire before the install step. Verified against the mock: 36 invocations by absolute path, zero by bare name; clean run 26/26; gated run 14 passed / 12 skipped; delete-failure run still reports LEAKED with both ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were self-inflicted by the previous two commits, not pre-existing. S3776: adding the rate-limit retry loop inline pushed exec()'s cognitive complexity to 19 (limit 15). Extracted looksRateLimited() and execWithRateLimitRetry(), so exec() is back to log → run → log and the retry policy reads as one thing. S4624: the leftover-app failure message nested a ternary-with-template inside a template literal. Extracted `alsoDeleted`. Behaviour is unchanged and re-verified against the mock, including the refactored retry path: clean 26/26; gated 14 passed / 12 skipped; delete-failure still reports LEAKED with both ids; a transient rate limit is still absorbed with one 5s wait (rateLimitWaits: 1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| * Smoke test for @getbrevo/cli. | ||
| * | ||
| * Exercises two lifecycles against a real backend: | ||
| * |
Collaborator
There was a problem hiding this comment.
Please split file in to different modules.
Public and private
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrites
scripts/smoke-test.ts(yarn smoke) around two lifecycles run against a real backend:Test-only: nothing under
src/changes, so perCLAUDE.mdnoSKILL.md/AGENTS.mdupdate is required and no changeset is added.Refs BEX-339. Depends on the commands already landed on this branch (BEX-250/251/252/253/255/327).
Three steps were already broken on this branch
The smoke test still assumed pre-public-apps behaviour, so it would fail on the current CLI regardless of this ticket:
Negative: public app rejectedbrevo app create --distribution publicis rejected — it's valid now, so the assertion was invertedApp updatebrevo app update, which no longer existsScaffoldbrevo app scaffold --app-id <id>→error: unknown option '--app-id'The first is removed, the second is replaced by
brevo app uploadsteps, the third drops the flag.Also fixed: the run was littering the directory it started from
brevo app createnow creates./<slug>in the cwd. Everycreatetherefore runs from a tracked tmp work root, and both apps plus every tmp dir are tracked onStateso the existing trap paths (trapDeleteApps,bestEffortCleanup) tear them down on success, on failure, and on SIGINT.Negative probes assert message and exit code
Each one requires a mapped message from
src/lang/en.ts, an exit code fromsrc/lib/exit-codes.ts, and no raw stack trace — a command that fails for an unmapped reason is a regression even though it "failed as expected".create --distribution bogusInvalid --distribution/ exit 1uploadwith noapp-config.jsonNo app-config.json found/ exit 1submitwith no resolvable appCannot determine which app to submit/ exit 1submita private appstatus/withdrawon an unknown idwithdrawa never-submitted appNOT_SUBMITTED, exit 0 by designGating keeps
--against=publishedgreenCommands the installed CLI doesn't have are detected from
brevo --helpand their steps report ⊘ skipped, not failed — so the smoke stays green while sibling commands reach npm. Detection is help-text only on purpose:brevo app <unknown> --helpexits 0, so probing a subcommand can't tell present from absent.brevo app createdoubles as a canary for an unreadable help layout. Create's project-directory behaviour is likewise detected from its own--jsonoutput.New flags:
--with-public(default) /--skip-public. The--report=artefact now goes throughredact()like the log file.Test plan
No CI-visible behaviour changes (the smoke workflows are unchanged and their existing invocations still apply). Verified locally against a mock
brevoonPATH, since these paths only execute against a live account:SIGINT→ exit 130, app deleted by the trap, nobrevo-smoke-work-*dir leftyarn lint+ 733 tests passReviewer notes
submittedstate cannot be reached from the CLI.brevo app submithands back the review form URL; the app moves tosubmittedwhen the form is submitted. So the withdraw step lands on theNOT_SUBMITTEDbranch, and a server-side "already submitted" rejection cannot be produced here. The script handles thewithdrawn: truebranch when a backend does move it. Tracked inTODO.md.RELEASE-CHECKLIST.md: a real-backend run, listing the six assertions that encode a guess about server behaviour (publicdistribution_typeround-trip, no version bump on an unchanged upload, no drift on submit-after-upload, withdraw's 422-not-404, and so on).brevo --helpand the README still advertise the removedbrevo app scaffold --app-id. Left out to keep this test-only; recorded inTODO.mdwith the changeset it belongs to.🤖 Generated with Claude Code