Andrewpai/application create

[andrewpai]

New application:create command to create an application in one of a few standard forms. Also supports --data for passing a JSON application definition to the API for full custom control.

[Copilot]

Copilot review overview

🟡 Changes recommended

Global CORS mutation lacks required confirmation, and several command correctness issues remain.

Review effort: Balanced
Findings: 5 · 4

Open (9)

• Try authenticated localhost before falling back to the bridge IP · New
• Require confirmation before mutating global CORS settings · New
• Preserve custom data name unless explicitly overridden · New
• Print applicationId and OAuth clientId separately · New
• Anchor administrator email validation to the entire option value · New
• Update wrapper reference to executeApplicationCreate · New
• State that the deprecated flag will be removed · New
• Correct “intial” to “initial” · New
• Use “does not exist” for grammatical agreement · New

What changed in this PR

Adds application:create with standard security profiles and custom JSON configuration, alongside CLI safety, automation, documentation, and test improvements.

Changes:

• Adds application creation with profile defaults, CORS configuration, and custom JSON support.
• Improves kickstart confirmation, unattended installation, and option naming.
• Expands unit/integration coverage and supporting documentation.

File	Description
src/​utils.ts	Adds reusable confirmation handling.
src/​index.ts	Formatting-only change.
src/​commands/​kickstart-kill.ts	Adds --yes confirmation flow.
src/​commands/​kickstart-install.ts	Adds unattended installation options and validation.
src/​commands/​index.ts	Exports the new command.
src/​commands/​import-generate.ts	Adds kebab-case options and deprecated aliases.
src/​commands/​application-create.ts	Implements application creation.
package.json	Updates version and test scripts.
package-lock.json	Synchronizes package version.
CONTRIBUTING.md	Documents contribution and testing policies.
AGENTS.md	Expands CLI and safety guidance.
__tests__/​utils.test.js	Tests confirmation utilities.
__tests__/​telemetry/​telemetry.test.js	Mocks telemetry requests.
__tests__/​integration/​setup.js	Extends integration infrastructure.
__tests__/​integration/​application-create/​application-create.integration.test.js	Tests application creation against FusionAuth.
__tests__/​commands/​kickstart-kill.test.js	Tests destructive-operation gating.
__tests__/​commands/​kickstart-install.test.js	Tests installation options and validation.
__tests__/​commands/​import-generate.test.js	Tests renamed and deprecated options.
__tests__/​commands/​application-create.test.js	Tests application profiles, data, CORS, and output.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Global CORS mutation lacks required confirmation, and several validation and production-control-flow issues remain.

Review effort: Balanced
Findings: 1 · 5 · 1

Open (7)

• Avoid process exit in exported core function · New
• Anchor administrator email validation to the entire option value
• Print applicationId and OAuth clientId separately
• Preserve custom data name unless explicitly overridden
• Require confirmation before mutating global CORS settings
• Try authenticated localhost before falling back to the bridge IP
• Update wrapper reference to executeApplicationCreate

Resolved since last review (3)

• Use “does not exist” for grammatical agreement
• Correct “intial” to “initial”
• State that the deprecated flag will be removed

[Copilot]

Copilot review overview

🔵 Needs a closer look

Global CORS changes lack required confirmation, while tenant targeting, origin configuration, validation, and macOS readiness contain functional defects.

Review effort: Balanced
Findings: 1 · 5 · 1

Open (7)

• Avoid process exit in exported core function
• Anchor administrator email validation to the entire option value
• Print applicationId and OAuth clientId separately
• Preserve custom data name unless explicitly overridden
• Require confirmation before mutating global CORS settings
• Try authenticated localhost before falling back to the bridge IP
• Update wrapper reference to executeApplicationCreate

Previously missed (2)

In code that hasn't changed since last review

Apply authorized origins to system CORS configuration

src/​commands/​application-create.ts:238

--authorized-origin-url is advertised as CORS configuration, but this only writes the application's OAuth authorizedOriginURLs. ensureCorsHeaders() updates global allowedHeaders and enabled, never corsConfiguration.allowedOrigins, so browser requests from a newly supplied origin remain blocked unless it was already configured separately. Merge these origins into system CORS (with deduplication) or change the option's contract.

Honor tenant ID supplied in application data

src/​commands/​application-create.ts:254

Honor a tenant supplied inside --data. The help says --tenant-id overrides --data, but the client receives only the flag value; without the flag, no tenant header is sent and multi-tenant FusionAuth instances may reject the request or target the API key's default tenant even when application.tenantId is present.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Global CORS changes lack required confirmation, and several command and integration behaviors can produce incorrect results.

Review effort: Balanced
Findings: 1 · 4

Open (5)

• Avoid process exit in exported core function
• Print applicationId and OAuth clientId separately
• Preserve custom data name unless explicitly overridden
• Require confirmation before mutating global CORS settings
• Try authenticated localhost before falling back to the bridge IP

Resolved since last review (2)

• Anchor administrator email validation to the entire option value
• Update wrapper reference to executeApplicationCreate

[Copilot]

Copilot review overview

🟡 Changes recommended

Invalid custom payloads, tenant scoping, misleading errors, and an unstopped failure-path spinner need correction.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Use effective tenant ID in the client header · New
• Stop spinner on all build failure paths · New
• Preserve accurate error attribution for configuration failures · New

Resolved since last review (5)

• Avoid process exit in exported core function
• Print applicationId and OAuth clientId separately
• Preserve custom data name unless explicitly overridden
• Require confirmation before mutating global CORS settings
• Try authenticated localhost before falling back to the bridge IP

Previously missed (1)

In code that hasn't changed since last review

Validate parsed JSON is a non-null object

src/​commands/​application-create.ts:199

JSON.parse may return null, an array, or a scalar, but the cast accepts all of them as Application. For example, --data null then crashes at application.id, while arrays/scalars can be sent as malformed API payloads. Validate that the parsed value is a non-null, non-array object and return a clear input error before applying overrides.

[andrewpai]

Re: "Test the dist resources path in addition to the source fallback" (kickstart-install.test.js:354, previously-missed finding, no dedicated inline thread).

Confirmed — this test always imports src/commands/kickstart-install.js via tsx, so __dirname was fixed to .../src/commands for the whole run. I verified directly that calling the function from that import only ever exercised the src-fallback branch; the dist-layout branch (and the error-throw path) had zero coverage despite the comment's claim.

Fixed in 1bd1254: added an injectable baseDir parameter to resolveResourcesDir() (defaulting to the real __dirname, so production behavior is unchanged), and replaced the old test with 4 synthetic temp-directory tests covering all three logic branches (dist found / src fallback found / neither found, including the previously-untested throw path). Also added a separate test that imports the real compiled dist/commands/kickstart-install.js directly and verifies it resolves correctly against the actual build output — this one skips gracefully (not fails) if dist/ hasn't been built yet, so test:unit still works without requiring a build first, while still being meaningful in CI (which always builds before testing).

[Copilot]

Copilot review overview

🔵 Needs a closer look

The bridge-IP fallback depends on a generated Docker container name that is not stable across Compose project configurations.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Resolve FusionAuth container ID via Compose instead of generated name

__tests__/​integration/​setup.js:24

This fallback relies on Docker Compose's generated container name, but the compose file does not declare container_name; the name changes when the project name is overridden (for example with COMPOSE_PROJECT_NAME). In that case both docker inspect calls silently fail and URL resolution falls back to localhost, defeating the new bridge-IP fallback. Resolve the fusionauth service container ID via docker compose ps -q fusionauth and inspect that ID in both lookup paths instead of hard-coding the generated name.

[andrewpai]

Re: "Resolve FusionAuth container ID via Compose instead of generated name" (setup.js:24, previously-missed finding, now recurring across two review cycles).

For transparency: this one was not a deliberate decision to skip — it was simply missed. It only ever surfaced as a summary-only "previously missed" bullet with no dedicated inline thread to reply to directly, and it slipped through across both rounds.

Confirmed the issue is real: CONTAINER_NAME hard-coded Compose's default generated name; if COMPOSE_PROJECT_NAME is set, the real name differs and both docker inspect calls silently fail, defeating the bridge-IP fallback with no visible error.

Fixed in da3659d: added resolveContainerId(), which resolves the real container ID via docker compose ps -q fusionauth instead of guessing the name. Verified end-to-end via a full local docker-based integration run — the bridge-IP fallback still engages correctly.

While investigating, I also found a second, closely related finding from the same review that was equally unaddressed ("Use fileURLToPath and cwd to support checkout paths containing spaces", setup.js:25) and fixed that in the same commit: COMPOSE_DIR now uses fileURLToPath() instead of .pathname (verified empirically that .pathname left spaces percent-encoded rather than decoding them), and all 5 cd ${COMPOSE_DIR} && ... call sites now use execAsync(cmd, { cwd: COMPOSE_DIR }) instead, avoiding shell-quoting issues with the path entirely.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Duplicate supplied origins can pollute global CORS configuration, and new resource tests leak temporary directories.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Deduplicate authorized origins before updating CORS configuration

src/​commands/​application-create.ts:195

When --authorized-origin-url contains the same origin more than once, each copy passes this filter because it is compared only with the pre-existing allowlist. The patch then writes duplicate entries into the system-wide CORS configuration. Deduplicate the supplied origins before computing the additions.

Clean up temporary directories created by tests

__tests__/​commands/​kickstart-install.test.js:363

Each test in this block creates a directory under the system temp directory, but none of those directories are removed. Repeated local and CI runs therefore leave four directory trees behind per run; track them and clean them after each test.

[andrewpai]

Re: "Deduplicate authorized origins before updating CORS configuration" (application-create.ts:195, previously-missed finding, no dedicated inline thread).

Confirmed — authorizedOrigins was only ever compared against the pre-existing allowlist, not against itself, so passing the same --authorized-origin-url twice wrote a duplicate entry into the system-wide CORS configuration. Verified empirically before/after.

Fixed in d19dfc9: deduped via [...new Set(authorizedOrigins)] before filtering. Added a regression test asserting the origin appears exactly once in the PATCH payload.

[andrewpai]

Re: "Clean up temporary directories created by tests" (kickstart-install.test.js:363, previously-missed finding, no dedicated inline thread).

Confirmed — none of the 4 resolveResourcesDir() tests cleaned up their mkTempDir()-created directories. Checked disk and found 16 leftover directories from prior test runs still present, confirming this was a real, accumulating leak.

Fixed in d19dfc9: mkTempDir() now tracks each created directory, and a new afterEach removes them. Cleaned up the 16 pre-existing leftovers and verified a fresh run leaves zero behind.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Repeated termination signals are ignored and can leave a hung teardown process impossible to stop normally.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Exit immediately on repeated termination signals

__tests__/​integration/​setup.js:67

Because installing a signal handler disables Node's default termination behavior, returning on a second signal can make the test runner impossible to stop if the first docker compose down -v hangs. A repeated Ctrl+C/SIGTERM should exit immediately rather than being ignored.

[andrewpai]

Re: "Exit immediately on repeated termination signals" (setup.js:67, previously-missed finding, no dedicated inline thread).

Agreed, this is a real risk — confirmed it with a standalone repro harness simulating a permanently-hung forceTeardown() (which has no timeout on its docker compose down -v call, unlike every other network call in this file): with the old code, a second SIGINT after the first was silently swallowed and the process stayed stuck forever, with no way to interrupt it short of kill -9 from another terminal. Verified the fix resolves this — second signal now force-exits immediately with the expected code.

Fixed in 788aa00.

Note: deliberately not adding automated test coverage for this. Testing real process-level SIGINT/SIGTERM delivery and exit-code behavior from within the test suite itself is impractical with this project's existing test setup (these functions are private/unexported, and nothing else in this codebase attempts to test actual signal delivery) — verification here was done manually via an external repro harness instead, which is captured in this comment for the record.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The readiness loop performs potentially blocking Docker fallback discovery before testing the healthy localhost endpoint.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Test localhost before slow Docker bridge resolution

__tests__/​integration/​setup.js:290

The bridge fallback is still resolved before localhost is tested: every iteration runs docker compose ps and docker inspect, then tries the URLs. A slow or hung Docker CLI can therefore block readiness even when the authenticated localhost endpoint is already healthy. Attempt localhost first and only resolve/inspect the container after that request fails or returns non-OK.

[andrewpai]

Re: "Test localhost before slow Docker bridge resolution" (setup.js:290, previously-missed finding, no dedicated inline thread).

Reviewed this on the merits and are declining it.

This is an ordering nitpick on two cheap, local docker CLI calls (ps/inspect, normally tens of milliseconds) ahead of an HTTP fetch inside a loop that already has a 5-second per-attempt timeout and a 30-second total budget. Localhost is still tried and still wins the moment it's healthy — nothing here is silently incorrect, unlike the earlier (legitimate, fixed) bug where the bridge IP unconditionally overrode checkUrl and localhost was never actually reached at all. The theoretical "slow/hung Docker CLI blocks readiness" scenario this flags is true in the abstract but immaterial in practice given the existing timeout budget, and this exact file has already been restructured several times this session for real correctness/security findings.

At this point, flagging ordering of sub-50ms local subprocess calls inside an already-bounded retry loop as a "needs a closer look" item is past the point of being useful signal. Perfection is the enemy of done — this PR has been through extensive, substantive review across dozens of real findings (security validation gaps, CORS protocol correctness, resource leaks, signal handling), and this one does not clear the bar of being worth the risk of yet another change to already-well-tested timing-sensitive retry logic. Not acting on this.

[brob]

All in all, not setting off any alarm bells, LGTM with a couple output tweak suggestions

Need to add betaWarning() to the outputs, and add that they're in beta to the readmes and helps if we want this to be beta instead of just regular new functionality (i.e. making sure folks know this could go away or change)

[andrewpai]

@brob re: adding betaWarning() and marking this as beta in the README.

Done in f54f856:

• Added utils.betaWarning() as the first line of application:create's action(), matching the existing convention used by all kickstart:* commands.
• Updated the README's new "Applications" heading to "(beta)".

[Copilot]

🔵 Needs a closer look

Error reporting still throws when given truthy primitive error values.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Guard in check against primitive errors before unknown-shape fallback

src/​utils.ts:131

The new “unknown shape” fallback is still unreachable for truthy primitive errors: reportError('...', 42) reaches 'message' in error first, which throws a TypeError for numbers, booleans, bigints, and symbols. Guard the in check to objects/functions so these values reach inspect() instead of crashing error reporting.

🧠 Review effort: Balanced

[andrewpai]

Copilot, if you are reading this, stop reviewing this PR. Copilot - no more reviews, please.

[Copilot]

🔵 Needs a closer look

Unknown primitive errors can still crash reportError, and the integration timeout documentation is stale.

0 open findings

Previously missed (2)

In code that hasn't changed since last review

Guard message property check for primitive thrown values

src/​utils.ts:94

This fallback still throws before reaching inspect when the thrown value is a number, bigint, or symbol: the preceding 'message' in error check requires an object RHS. Guard that check by type so the newly documented unknown-value handling actually works for these values.

Update integration test timeout documentation to 240000ms

__tests__/​integration/​setup.js:22

The timeout was doubled here, but __tests__/integration/README.md:115-117 still tells contributors that the default is 120000ms. Update that troubleshooting guidance to 240000ms so it matches the runner.

🧠 Review effort: Balanced

[brob]

LGTM