diff --git a/scripts/smoke-help.sh b/scripts/smoke-help.sh new file mode 100755 index 000000000..07f68cbac --- /dev/null +++ b/scripts/smoke-help.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# Smoke test: help must render without login (specs/help-without-login.md). +# +# Runs the CLI in an isolated HOME (no credentials, empty feature-flag cache) +# and asserts that help invocations exit 0, render help output, and never +# emit an interactive login prompt. +set -u + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$ROOT_DIR" + +if [ ! -d lib ]; then + echo "lib/ not found — building..." + yarn build || exit 1 +fi + +FAKE_HOME="$(mktemp -d)" +trap 'rm -rf "$FAKE_HOME"' EXIT + +# On a fresh build the init hook autofixes the node_modules/vtex symlink and +# exits asking for a re-run. Prime it so it doesn't interfere with the cases. +HOME="$FAKE_HOME" node bin/run --version < /dev/null > /dev/null 2>&1 || true + +FAILURES=0 + +run_case() { + local description="$1" + shift + local output + local status + + # Isolated HOME/XDG dirs => no session token, empty feature-flag cache. + output="$(HOME="$FAKE_HOME" XDG_CONFIG_HOME="$FAKE_HOME/.config" XDG_CACHE_HOME="$FAKE_HOME/.cache" \ + node bin/run "$@" < /dev/null 2>&1)" + status=$? + + if [ $status -ne 0 ]; then + echo "FAIL [$description]: exited with status $status" + echo "$output" | head -30 + FAILURES=$((FAILURES + 1)) + return + fi + + if ! echo "$output" | grep -qi "usage"; then + echo "FAIL [$description]: output does not look like help (no 'Usage' section)" + echo "$output" | head -30 + FAILURES=$((FAILURES + 1)) + return + fi + + if echo "$output" | grep -qiE "Account:|previous login|previous account"; then + echo "FAIL [$description]: interactive login prompt detected" + echo "$output" | head -30 + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS [$description]" +} + +run_case "vtex help" help +run_case "vtex help " help link +run_case "vtex --help" link --help +run_case "vtex -h" link -h +run_case "vtex --help" --help + +if [ $FAILURES -gt 0 ]; then + echo "$FAILURES smoke test case(s) failed" + exit 1 +fi + +echo "All help smoke tests passed" diff --git a/specs/help-without-login.md b/specs/help-without-login.md new file mode 100644 index 000000000..afa53f4a5 --- /dev/null +++ b/specs/help-without-login.md @@ -0,0 +1,94 @@ +# Spec: Remove login dependency for `help` in the toolbelt CLI + +## Problem + +`src/oclif/hooks/init.ts` → `main()` always calls `checkLogin(options.id)`, which +forces an interactive login for any command not present in an `allowedList`. +Running `vtex --help` (e.g. `vtex deploy --help`) sets +`options.id = 'deploy'` (not in the allowed list) and triggers a login prompt, +even though only static help text is requested. This blocks agents (and users) +from discovering commands without credentials. + +## Goal + +All help invocations render without requiring login, so agents on any machine can +discover the proper commands to run. + +## Scope + +**In scope** — skip login for every help form: + +- `vtex help` +- `vtex help ` +- `vtex --help` / `-h` +- `vtex --help` / `-h` +- `vtex` (no args — already allowed via `undefined`) + +**In scope (hardening)** — defensive fallback for the empty-feature-flag-cache +crash in `showHelp` (see design decision 6). Logged-out help on clean machines +makes this crash much more likely, so it lands in the same PR. + +**Out of scope** — anything beyond the login gate and the help-rendering +fallback (e.g. reworking how feature flags are fetched/updated). + +## Design decisions + +1. **Generic help detection, not per-command whitelisting.** Whitelisting each + command cannot cover `vtex deploy --help`, so we detect "this is a help + invocation" generically. + +2. **New helper `isHelpInvocation(commandId, argv)` in + `src/oclif/hooks/utils.ts`** (next to `getHelpSubject`). Returns `true` when: + - `commandId` is `help`, `--help` or `-h` (oclif passes bare `vtex --help` / + `vtex -h` as the command id itself, with empty argv), or + - `argv` contains `--help` or `-h` **before** any `--` separator. + - `-h` / `--help` are safe to treat as help: `-h` is globally reserved for + help in `src/api/oclif/CustomCommand.ts`, with no other usage. + +3. **Respect the `--` separator** when scanning args, consistent with the existing + `getHelpSubject`, so `vtex cmd -- --help` is not treated as help. + +4. **Wire into `checkLogin` via an explicit `argv` parameter.** Change the + signature to `checkLogin(command: string, argv: string[])` and early-return + when `isHelpInvocation(command, argv)` is `true`. The init hook already + receives the parsed args (`options.argv` in `src/oclif/hooks/init.ts`), so + the call site becomes `checkLogin(options.id, options.argv)`. Passing argv + explicitly (instead of reading `process.argv` inside `checkLogin`) keeps the + function testable. + +5. **No hidden dependency in help rendering.** `FeatureFlag.getSingleton()` reads + a local Configstore file (no network/auth at render time), so skipping the + login gate fully unblocks help. + +6. **Defensive feature-flag fallback in `showHelp`.** `showHelp` reads + `COMMANDS_GROUP` / `COMMANDS_GROUP_ID` from the feature-flag Configstore, + which is populated by a non-blocking child process + (`FeatureFlagUpdateChecker` → `spawnUnblockingChildProcess`). On a fresh + install both can be `undefined`, and `Object.keys(commandsId)` throws. + Guard with sensible defaults so help always renders: when either value is + missing, fall back to rendering all commands under a single default + ("Other") group. This is a small hardening change and ships in the same PR. + +7. **Document the `--` separator handling.** Add a short comment in + `src/oclif/hooks/utils.ts` (next to `isHelpInvocation` / `getHelpSubject`) + explaining why args after `--` are ignored, to avoid future regressions. + +## Testing + +- Add `src/oclif/hooks/utils.test.ts` with table-driven cases for + `isHelpInvocation`: + - `help` → true; `help deploy` → true; `deploy --help` → true; + `deploy -h` → true; `--help` / `-h` → true + - `deploy` → false; `deploy -- --help` → false; no args → false +- Unit test for the `checkLogin` early return (export `checkLogin` from + `init.ts` to make it testable): with + `SessionManager.getSingleton().checkValidCredentials()` mocked to `false` and + a help argv (e.g. `deploy --help`), assert `authLogin` is **not** called; + with a non-help, non-allowed command, assert it **is** called. +- Integration smoke test (shell): run `node bin/run help` (and + `node bin/run deploy --help`) in an environment simulating no credentials + (isolated `HOME`/config dir with no session and an empty feature-flag cache), + asserting the process exits 0, renders help output, and emits no interactive + login prompt. This also exercises the feature-flag fallback (decision 6). +- Beyond the above, the init hook is not unit-tested directly (heavy side + effects); coverage is via the pure helper and the smoke test. diff --git a/src/oclif/hooks/init.test.ts b/src/oclif/hooks/init.test.ts new file mode 100644 index 000000000..cb4375437 --- /dev/null +++ b/src/oclif/hooks/init.test.ts @@ -0,0 +1,75 @@ +// bin/run has import-time side effects (and uses `node:module`, which the +// jest resolver can't handle) — mock it before importing the hook. +jest.mock('../../../bin/run', () => ({ initTimeStartTime: [0, 0] })) + +import { checkLogin } from './init' +import authLogin from '../../modules/auth/login' + +const mockCheckValidCredentials = jest.fn() + +jest.mock('../../api/session/SessionManager', () => ({ + SessionManager: { + getSingleton: () => ({ checkValidCredentials: mockCheckValidCredentials }), + }, +})) + +jest.mock('../../modules/auth/login', () => ({ + __esModule: true, + default: jest.fn().mockResolvedValue(undefined), +})) + +const mockedAuthLogin = authLogin as jest.MockedFunction + +beforeEach(() => { + jest.clearAllMocks() + mockCheckValidCredentials.mockReturnValue(false) +}) + +describe('checkLogin', () => { + it('does not trigger login for `vtex help`', async () => { + await checkLogin('help', []) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('does not trigger login for `vtex help `', async () => { + await checkLogin('help', ['deploy']) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('does not trigger login for `vtex --help`', async () => { + await checkLogin('deploy', ['--help']) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('does not trigger login for `vtex -h`', async () => { + await checkLogin('deploy', ['-h']) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('does not trigger login for bare `vtex --help` / `vtex -h`', async () => { + await checkLogin('--help', []) + await checkLogin('-h', []) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('triggers login for a non-help, non-allowed command without credentials', async () => { + await checkLogin('deploy', []) + expect(mockedAuthLogin).toHaveBeenCalledTimes(1) + }) + + it('does not trigger login for `vtex -- --help` when logged in', async () => { + mockCheckValidCredentials.mockReturnValue(true) + await checkLogin('deploy', ['--', '--help']) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) + + it('treats `vtex -- --help` as a normal command (login required)', async () => { + await checkLogin('deploy', ['--', '--help']) + expect(mockedAuthLogin).toHaveBeenCalledTimes(1) + }) + + it('does not trigger login for allowed commands', async () => { + await checkLogin('login', []) + expect(mockedAuthLogin).not.toHaveBeenCalled() + }) +}) diff --git a/src/oclif/hooks/init.ts b/src/oclif/hooks/init.ts index f42eb7c98..fb2954e98 100644 --- a/src/oclif/hooks/init.ts +++ b/src/oclif/hooks/init.ts @@ -24,7 +24,8 @@ import { ErrorReport } from '../../api/error/ErrorReport' import * as fse from 'fs-extra' import path from 'path' import { sortBy, uniqBy } from 'ramda' -import { getHelpSubject, CommandI, renderCommands } from './utils' +import { getHelpSubject, isHelpInvocation, CommandI, renderCommands } from './utils' +import { OTHER_GROUP_ID } from './constants' // eslint-disable-next-line @typescript-eslint/no-var-requires const { initTimeStartTime } = require('../../../bin/run') @@ -35,7 +36,12 @@ const logToolbeltVersion = () => { log.debug(`Toolbelt version: ${pkg.version}`) } -const checkLogin = async (command: string) => { +export const checkLogin = async (command: string, argv: string[]) => { + // Help invocations only render static text — never require login for them. + if (isHelpInvocation(command, argv)) { + return + } + /** * Commands for which previous login is not necessary. There's some exceptions: * - link: It's necessary to be logged in, but there's some login logic there before running the link per se @@ -107,7 +113,7 @@ const main = async (options?: HookKeyOrOptions<'init'>, calculateInitTime?: bool log.debug('node %s - %s %s', process.version, os.platform(), os.release()) log.debug(args) - await checkLogin(options.id) + await checkLogin(options.id, options.argv ?? []) await checkAndFixSymlink(options) @@ -271,12 +277,14 @@ export default async function(options: HookKeyOrOptions<'init'>) { error(`command ${subject} not found`) } - const commandsGroup: Record = FeatureFlag.getSingleton().getFeatureFlagInfo>( - 'COMMANDS_GROUP' - ) + // The feature-flag store is populated by a non-blocking child process, so + // on a fresh install these values may be undefined. Fall back to rendering + // all commands under a single default ("Other") group so help always works. + const commandsGroup: Record = + FeatureFlag.getSingleton().getFeatureFlagInfo>('COMMANDS_GROUP') ?? {} const commandsId: Record = FeatureFlag.getSingleton().getFeatureFlagInfo>( 'COMMANDS_GROUP_ID' - ) + ) ?? { [OTHER_GROUP_ID]: 'Other' } const commandsGroupLength: number = Object.keys(commandsId).length const commands = this.config.commands diff --git a/src/oclif/hooks/utils.test.ts b/src/oclif/hooks/utils.test.ts new file mode 100644 index 000000000..7cc50f9a7 --- /dev/null +++ b/src/oclif/hooks/utils.test.ts @@ -0,0 +1,25 @@ +import { isHelpInvocation } from './utils' + +describe('isHelpInvocation', () => { + test.each<[string | undefined, string[], boolean]>([ + // help forms → true + ['help', [], true], + ['help', ['deploy'], true], + ['deploy', ['--help'], true], + ['deploy', ['-h'], true], + // oclif passes bare `vtex --help` / `vtex -h` as the command id itself + ['--help', [], true], + ['-h', [], true], + [undefined, ['--help'], true], + [undefined, ['-h'], true], + ['workspace', ['use', '--help'], true], + // non-help forms → false + ['deploy', [], false], + ['deploy', ['--', '--help'], false], + ['deploy', ['--', '-h'], false], + [undefined, [], false], + ['link', ['--verbose'], false], + ])('commandId=%p argv=%p → %p', (commandId, argv, expected) => { + expect(isHelpInvocation(commandId, argv)).toBe(expected) + }) +}) diff --git a/src/oclif/hooks/utils.ts b/src/oclif/hooks/utils.ts index 063646087..5c699daa4 100644 --- a/src/oclif/hooks/utils.ts +++ b/src/oclif/hooks/utils.ts @@ -1,6 +1,6 @@ import chalk from 'chalk' import indent from 'indent-string' -import { COLORS } from '../../api' +import { COLORS } from '../../api/constants/Colors' import { renderList } from '@oclif/plugin-help/lib/list' import RootHelp from '@oclif/plugin-help/lib/root' import { OTHER_GROUP_ID } from './constants' @@ -18,6 +18,28 @@ export function getHelpSubject(args: string[]): string | undefined { } } +/** + * Detects whether the current invocation is a help request, covering every + * help form: `vtex help`, `vtex help `, `vtex --help` / `-h` (oclif + * passes the bare flag as the command id itself) and `vtex --help` / + * `-h` (`-h` is globally reserved for help in CustomCommand). + * + * Note on the `--` separator: everything after `--` is forwarded verbatim to + * the underlying command as positional values (consistent with + * `getHelpSubject` above), so `vtex cmd -- --help` must NOT be treated as a + * help invocation. Only flags appearing before `--` are considered. + */ +export function isHelpInvocation(commandId: string | undefined, argv: string[]): boolean { + if (commandId != null && ['help', '--help', '-h'].includes(commandId)) return true + + for (const arg of argv) { + if (arg === '--') break + if (arg === '--help' || arg === '-h') return true + } + + return false +} + function renderCommand(commands: CommandI[], ctx: any): string { return renderList( commands.map(c => [chalk.hex(COLORS.PINK)(c.name), c.description && ctx.render(c.description.split('\n')[0])]),