Skip to content

use commander js classes for base44 commands#420

Merged
kfirstri merged 14 commits intomainfrom
run-command-architecture-change
Mar 17, 2026
Merged

use commander js classes for base44 commands#420
kfirstri merged 14 commits intomainfrom
run-command-architecture-change

Conversation

@kfirstri
Copy link
Collaborator

@kfirstri kfirstri commented Mar 16, 2026

Note

Description

This PR refactors the CLI command architecture by replacing the runCommand() higher-order function pattern with a Base44Command class that extends Commander.js's Command. The new approach moves the lifecycle logic (intro/outro, auth check, app config loading, error handling, non-interactive mode) into an OOP-based command wrapper, making command definitions cleaner and eliminating the need to pass CLIContext explicitly to every command factory. The CLIContext is now injected once via a preAction hook in createProgram().

Related Issue

None

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other (please describe):

Changes Made

  • Introduced Base44Command class (packages/cli/src/cli/utils/command/Base44Command.ts) that wraps command actions with the full Base44 lifecycle (intro/outro, auth, app config, error handling, upgrade notification)
  • Extracted rendering helpers into command/render.ts (showCommandStart, showCommandEnd, showThemedError, showPlainError) and middleware into command/middleware.ts (ensureAuth, ensureAppConfig)
  • Deleted runCommand.ts and migrated all ~30 command files from the runCommand() HOF pattern to new Base44Command(name, options).action(...)
  • Context injection now happens once via a Commander preAction hook in createProgram(), removing the CLIContext parameter from all command factory functions
  • Moved RunCommandResult type from runCommand.ts to cli/types.ts for shared access
  • Added formatPlainUpgradeMessage() to upgradeNotification.ts for proper CI/non-interactive upgrade notices written to stderr
  • Added non-interactive mode tests for create, deploy, eject, link, and site deploy commands
  • Updated docs/commands.md to document the new Base44Command pattern

Testing

  • I have tested these changes locally
  • I have added/updated tests as needed
  • All tests pass (npm test)

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have updated docs/ (AGENTS.md) if I made architectural changes

Additional Notes

The Base44Command constructor defaults are: requireAuth: true, requireAppConfig: true, fullBanner: false — matching the most common command profile. Commands like login that skip auth/config pass explicit overrides. Non-interactive mode (CI/piped output) skips all clack UI and writes errors to stderr as plain text; upgrade notifications also go to stderr in that mode.


🤖 Generated by Claude | 2026-03-17 00:00 UTC

@kfirstri kfirstri self-assigned this Mar 16, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 16, 2026

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.45-pr.420.1329187

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/cli@0.0.45-pr.420.1329187"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.45-pr.420.1329187"
  }
}

Preview published to npm registry — try new features instantly!

@kfirstri
Copy link
Collaborator Author

@claude please go over and review this PR, it's a big infra change. try to see if you can find any backward issues or bugs that might arise

@claude
Copy link

claude bot commented Mar 16, 2026

Claude finished @kfirstri's task in 6m 37s —— View job


PR Review: Commander.js Class Architecture Refactor

Tasks:

  • Understand request
  • Read core changed files (Base44Command.ts, display.ts, middleware.ts, program.ts)
  • Review command files for backward compatibility issues
  • Check error handling and non-interactive mode
  • Check tests
  • Post full review

Overview

This is a clean, well-executed refactoring. The core architecture change — moving from runCommand() HOF to Base44Command extending Commander's Command — is sound. Context injection via a root-level preAction hook works correctly because Commander fires that hook on the actual executing command (actionCommand), even for nested subcommands like base44 connectors push.

No critical regressions found. A few medium and minor issues below.


Issues Found

🟠 Medium: middleware.ts uses clack log.info in non-interactive mode

File: packages/cli/src/cli/utils/command/middleware.ts:15

export async function ensureAuth(errorReporter: ErrorReporter): Promise<void> {
  const loggedIn = await isLoggedIn();
  if (!loggedIn) {
    log.info("You need to login first to continue.");  // ← clack call, no isNonInteractive guard
    await login();
  }
  // ...
}

ensureAuth doesn't have access to isNonInteractive, so in CI mode this produces clack-styled output (◆ You need to login first...) while everything else in non-interactive mode goes through plain process.stderr.write. The inconsistency is minor since a non-logged-in CI run is going to fail anyway (the subsequent login() call is inherently interactive), but the plain-text error path should ideally be consistent.

Fix this →


🟠 Medium: deploy silently "succeeds" with exit code 0 in CI without --yes

File: packages/cli/src/cli/commands/project/deploy.ts:71-84

if (!options.yes) {
  const shouldDeploy = await confirm({ message: "Are you sure you want to continue?" });
  if (isCancel(shouldDeploy) || !shouldDeploy) {
    return { outroMessage: "Deployment cancelled" };  // exit 0 ← silent CI failure
  }
}

When CI=true (set by bin/run.js in non-interactive mode), clack's confirm() returns a cancel symbol, so isCancel(shouldDeploy) is true. This means base44 deploy in CI without --yes exits 0 with "Deployment cancelled" — a silent no-op. A CI pipeline that forgets --yes would appear to succeed without deploying anything.

This is pre-existing behavior that carried over unchanged, but the new command.isNonInteractive flag now makes it straightforward to detect this case and throw an explicit error instead. Now that isNonInteractive is available in the action, consider:

.action(async (options: DeployOptions, command: Base44Command) => {
  if (command.isNonInteractive && !options.yes) {
    throw new InvalidInputError("--yes is required in non-interactive mode");
  }
  // ...

Fix this →


🟡 Minor: Over-export of internal implementation details

File: packages/cli/src/cli/utils/command/index.ts

export * from "./Base44Command.js";
export * from "./display.js";     // showCommandStart, showCommandEnd, showThemedError, showPlainError
export * from "./middleware.js";  // ensureAuth, ensureAppConfig

These re-exports bubble through to utils/index.ts, making showPlainError, showThemedError, ensureAuth, ensureAppConfig, etc. part of the CLI utils public surface. Since utils/index.ts is the shared import path for all command files, these internal lifecycle functions are now accessible (and confusingly named) alongside runTask, theme, etc.

Consider either not re-exporting display.ts/middleware.ts from the barrel, or marking them with a comment to discourage external use.


🟡 Minor: Duplicate JSDoc tag on isNonInteractive getter

File: packages/cli/src/cli/utils/command/Base44Command.ts:93-97

/**
 * Whether the CLI is running in non-interactive mode...
 */
/** @public */          // ← redundant, second JSDoc for the same member
get isNonInteractive(): boolean {

Two consecutive JSDoc blocks — the second /** @public */ is accidentally placed and overrides the first in most tooling. Remove the /** @public */ line.


🟡 Minor: eject.ts doesn't pass isNonInteractive to deployAction

File: packages/cli/src/cli/commands/project/eject.ts:164

await deployAction({ yes: true, projectRoot: resolvedPath });
// isNonInteractive not forwarded ↑

Functionally safe (yes: true skips the confirm() call), but inconsistent with how deploy.ts calls deployAction. Should be:

await deployAction({ yes: true, projectRoot: resolvedPath, isNonInteractive: options.isNonInteractive });

What Changed in Default Behavior

The PR description calls this out clearly, but worth documenting for reviewers:

Behavior Old (runCommand) New (Base44Command)
requireAuth default false true
requireAppConfig default true true
intro() in non-interactive Always called Skipped
Upgrade notification in CI Written via clack note() to stdout Written via process.stderr.write

All existing commands were updated to pass explicit options where old defaults differed. The audit looks complete — login, logout, whoami, create, link, eject all correctly opt out of requireAppConfig: false and/or requireAuth: false where needed.


Test Notes

The version-check test change ("Update available!""Update available") is correct. Tests run with CI=true, so the new non-interactive code path fires formatPlainUpgradeMessage()"Update available: x → y" written to stderr. CLIResultMatcher.toContain() checks stdout + stderr combined, so the test works.


Architecture: No Issues Found

  • The program.hook("preAction", ...) correctly injects context into nested Base44Command instances (Commander fires the root preAction for all nested subcommands with actionCommand pointing to the deepest matching command).
  • Error re-throws are intentional and consistent with the old code — index.ts:runCLI handles CLIExitError vs other errors for the final exit code.
  • Base44Command.action() wrapping is lazy (reads this.context at execution time, not registration time), so command construction in createProgram() before the preAction hook runs is safe.
  • Plain Command group containers (getAgentsCommand, getConnectorsCommand, etc.) don't need Base44Command since they have no actions.

@kfirstri kfirstri requested a review from artemdemo March 17, 2026 12:24
@kfirstri kfirstri requested a review from artemdemo March 17, 2026 14:22
@kfirstri kfirstri merged commit 462e9f5 into main Mar 17, 2026
12 checks passed
@kfirstri kfirstri deleted the run-command-architecture-change branch March 17, 2026 14:31
netanelgilad added a commit that referenced this pull request Mar 18, 2026
Adapt exec.ts to use the new Base44Command class introduced in main
(#420) instead of the removed runCommand utility.

Made-with: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants