-
Notifications
You must be signed in to change notification settings - Fork 10
AUTH-6733 fix: accept deprecated --no-commit/--commit flags #220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import { describe, it, expect, beforeEach, afterEach } from 'vitest'; | ||
| import { spawnSync } from 'node:child_process'; | ||
| import { mkdtempSync, rmSync } from 'node:fs'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| /** | ||
| * Integration test for the deprecated --commit/--no-commit backward-compat shim. | ||
| * | ||
| * The installer never commits changes, but scripts written against older | ||
| * versions still pass --no-commit (or --commit). These flags must be accepted | ||
| * as no-ops: strict parsing must not reject them, and a deprecation warning | ||
| * must go to stderr (never stdout, so JSON streams stay clean). | ||
| * | ||
| * bin.ts runs runCli() at import and exposes no seams, so the only honest way | ||
| * to prove the shim is to drive the real CLI as a subprocess. With no | ||
| * credentials and an unroutable API base, a successful parse falls through to | ||
| * the auth-required exit (4); a strict-parser rejection exits 1 with | ||
| * "Unknown argument" instead. | ||
| */ | ||
| const binPath = fileURLToPath(new URL('./bin.ts', import.meta.url)); | ||
| const forceInsecureStorageImport = fileURLToPath(new URL('./test/force-insecure-storage.ts', import.meta.url)); | ||
| const repoRoot = fileURLToPath(new URL('..', import.meta.url)); | ||
|
|
||
| let sandboxTmp: string; | ||
|
|
||
| beforeEach(() => { | ||
| sandboxTmp = mkdtempSync(join(tmpdir(), 'wos-cli-deprecated-flags-it-')); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| rmSync(sandboxTmp, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| function runCli(args: string[]) { | ||
| const env: NodeJS.ProcessEnv = { | ||
| PATH: process.env.PATH, | ||
| HOME: sandboxTmp, | ||
| USERPROFILE: sandboxTmp, | ||
| TMPDIR: sandboxTmp, | ||
| TMP: sandboxTmp, | ||
| TEMP: sandboxTmp, | ||
| WORKOS_MODE: 'agent', | ||
| // Keep machine streams clean: no telemetry, no update check network calls. | ||
| WORKOS_TELEMETRY: 'false', | ||
| // Unroutable API base so provisioning fails fast and falls back to auth. | ||
| WORKOS_API_URL: 'http://127.0.0.1:59999', | ||
| }; | ||
|
|
||
| return spawnSync('bun', ['--preload', forceInsecureStorageImport, binPath, ...args], { | ||
| cwd: repoRoot, | ||
| encoding: 'utf-8', | ||
| env, | ||
| }); | ||
| } | ||
|
|
||
| describe('deprecated install flags (backward-compat shims)', () => { | ||
| it('--no-commit is accepted as a no-op and warns on stderr', () => { | ||
| const result = runCli(['install', '--no-commit']); | ||
|
|
||
| // Past strict parsing: auth-required (4), not a validation error (1). | ||
| expect(result.status).toBe(4); | ||
| expect(result.stderr).not.toContain('Unknown argument'); | ||
| expect(result.stderr).toContain('Deprecated flag: --no-commit'); | ||
| // JSON/machine stdout stays clean. | ||
| expect(result.stdout).not.toContain('Deprecated flag'); | ||
| }, 30_000); | ||
|
|
||
| it('--commit is accepted as a no-op and warns on stderr', () => { | ||
| const result = runCli(['install', '--commit']); | ||
|
|
||
| expect(result.status).toBe(4); | ||
| expect(result.stderr).not.toContain('Unknown argument'); | ||
| expect(result.stderr).toContain('Deprecated flag: --commit'); | ||
| expect(result.stdout).not.toContain('Deprecated flag'); | ||
| }, 30_000); | ||
|
|
||
| it('omitting the flag emits no deprecation warning', () => { | ||
| const result = runCli(['install']); | ||
|
|
||
| expect(result.status).toBe(4); | ||
| expect(result.stderr).not.toContain('Deprecated flag'); | ||
| }, 30_000); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,9 @@ import { | |
| outputError, | ||
| exitWithError, | ||
| } from './utils/output.js'; | ||
| import ui, { PromptUnavailableError } from './utils/ui.js'; | ||
| import ui, { PromptUnavailableError, pill } from './utils/ui.js'; | ||
| import { renderStderrNotice } from './utils/box.js'; | ||
| import chalk from 'chalk'; | ||
| import { registerSubcommand } from './utils/register-subcommand.js'; | ||
| import { installCrashReporter, sanitizeMessage } from './utils/crash-reporter.js'; | ||
| import { installStoreForward, recoverPendingEvents } from './utils/telemetry-store-forward.js'; | ||
|
|
@@ -211,13 +213,10 @@ const installerOptions = { | |
| type: 'boolean' as const, | ||
| }, | ||
| commit: { | ||
| default: true, | ||
| describe: 'Auto-commit after installation (use --no-commit to skip)', | ||
| type: 'boolean' as const, | ||
| }, | ||
| 'create-pr': { | ||
| default: false, | ||
| describe: 'Auto-create pull request after installation', | ||
| // Deprecated no-op kept for backward compatibility: the installer never | ||
| // commits, but scripts that still pass --commit/--no-commit must not fail | ||
| // strict parsing. No default so usage is detectable (undefined = absent). | ||
| describe: 'Deprecated: no-op flag, the installer never commits changes', | ||
| type: 'boolean' as const, | ||
| }, | ||
|
Comment on lines
213
to
221
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Older automation scripts using the pull-request flag still crash The backward-compatibility shim only accepts the two commit-related flags ( Strict yargs parsing rejects the removed --create-pr optionThe base commit removed both Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
| 'git-check': { | ||
|
|
@@ -242,6 +241,20 @@ const installerOptions = { | |
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * Warn (stderr, so JSON stdout stays clean) when a script passes the removed | ||
| * --commit/--no-commit flags. They are accepted as no-ops for backward | ||
| * compatibility only. | ||
| */ | ||
| function warnIfDeprecatedCommitFlag(argv: { commit?: boolean }): void { | ||
| if (argv.commit === undefined) return; | ||
| const flag = argv.commit ? '--commit' : '--no-commit'; | ||
| renderStderrNotice( | ||
| `${pill('WARN', 'warn')} ${chalk.bold(`Deprecated flag: ${flag}`)} ${chalk.dim('— accepted as a no-op.')}`, | ||
| chalk.dim('The installer never commits changes; review and commit manually when ready.'), | ||
| ); | ||
| } | ||
|
Comment on lines
+249
to
+256
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Deprecation notice is emitted even in JSON/agent runs
Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| // Check for updates (blocks up to 500ms, skip in JSON/non-human modes to keep machine streams clean) | ||
| if (!isJsonMode() && isPromptAllowed()) await checkForUpdates(); | ||
|
|
||
|
|
@@ -2542,6 +2555,7 @@ async function runCli(): Promise<void> { | |
| (yargs) => yargs.options(installerOptions), | ||
| async (argv) => { | ||
| await applyInsecureStorage(argv.insecureStorage); | ||
| warnIfDeprecatedCommitFlag(argv); | ||
| await resolveInstallCredentials(argv.apiKey, argv.installDir, argv.skipAuth, ensureAuthenticated); | ||
| const { handleInstall } = await import('./commands/install.js'); | ||
| await handleInstall(argv); | ||
|
|
@@ -2754,6 +2768,7 @@ async function runCli(): Promise<void> { | |
| (yargs) => yargs.options(installerOptions), | ||
| async (argv) => { | ||
| await applyInsecureStorage(argv.insecureStorage); | ||
| warnIfDeprecatedCommitFlag(argv); | ||
| await resolveInstallCredentials(argv.apiKey, argv.installDir, argv.skipAuth, ensureAuthenticated); | ||
| const { handleInstall } = await import('./commands/install.js'); | ||
| await handleInstall({ ...argv, dashboard: true }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔍 New spec drives the real CLI as a subprocess — slow and environment-sensitive
This spec is picked up by the default
vitest runinclude glob (src/**/*.spec.tsinvitest.config.ts), so everybun run testspawns three realbun src/bin.ts install ...subprocesses (up to 30s each). The assertions depend on the CLI reaching the auth-required exit (4) after credential resolution fails against an unroutableWORKOS_API_URL; any change that makes credential resolution fail differently (e.g. a network error path that classifies as a general error) will flip the exit code to 1 and break these tests for reasons unrelated to flag parsing. Consider isolating it behind a separate integration test script/project, or asserting only that stderr lacksUnknown argumentand contains the deprecation text rather than pinning the exit code.Was this helpful? React with 👍 or 👎 to provide feedback.