Skip to content

Commit effe187

Browse files
nicknisinicknisi
authored
fix: prevent registerSubcommand from injecting false positional args (#111)
* fix: prevent registerSubcommand from injecting false positional args The enriched usage string (e.g., `create --slug <string> --name <string>`) was passed directly as the yargs command string. Yargs interprets `<...>` tokens in command strings as required positional arguments, causing commands like `permission create` and `role create` to demand phantom positional args alongside the actual named options. Move the enriched string to `.usage()` (display-only for help text) and pass only the original command string to `yargs.command()`. Fixes: commands with demandOption flags (permission create, role create, invitation create, seed, etc.) rejecting valid `--flag value` invocations with "Not enough non-option arguments". * chore: formatting * test: add parsing regression tests for all affected commands Exercises yargs arg parsing end-to-end for all 16 commands that use registerSubcommand with demandOption named flags. Each test verifies that standard --flag syntax is accepted without "Not enough non-option arguments" errors. The existing smoke test bypasses yargs entirely (calls handlers directly), so it could not catch the parsing bug. These tests fill that gap. * fix: enrich description instead of command string for required flags Move the required-flag annotation from the yargs command string to the description. This fixes two issues with the previous approach: - P2: `$0` in `.usage()` resolved to the root script name, so nested commands like `workos role create --help` showed `workos create ...` instead of `workos role create ...` - P3: parent help lost enrichment entirely since the command string was reverted to plain usage Now parent help shows e.g.: create Create a permission (requires --slug, --name) Positional args already visible in the command string (e.g., <slug>) are excluded from the description enrichment to avoid redundancy. * fix: skip description enrichment when flag already mentioned Commands like `role delete` and `role remove-permission` already include `(requires --org)` in their description. The enrichment helper now filters out flags that the description already references, preventing duplicated text like `(requires --org) (requires --org)`. * chore: formatting * chore: simplify regression tests with parameterized it.each Convert 15 copy-paste test cases into data-driven it.each blocks and remove unused YargsOptions fields (string, number, boolean arrays).
1 parent 4cc38c6 commit effe187

3 files changed

Lines changed: 289 additions & 49 deletions

File tree

‎src/utils/register-subcommand.parsing.spec.ts‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,226 @@
1+
/**
2+
* Regression tests for registerSubcommand arg parsing.
3+
*
4+
* Verifies that commands with demandOption named flags accept standard
5+
* --flag syntax without yargs demanding phantom positional arguments.
6+
*
7+
* Bug: registerSubcommand previously appended `--slug <string>` to the yargs
8+
* command string, causing yargs to interpret <string> as required positionals.
9+
*/
10+
import { describe, it, expect } from 'vitest';
11+
import yargs from 'yargs';
12+
import type { Options, PositionalOptions } from 'yargs';
13+
import { registerSubcommand } from './register-subcommand.js';
14+
15+
/** Build a yargs parser with a single registerSubcommand call, capturing parse failures. */
16+
function buildParser(usage: string, builder: (y: yargs.Argv) => yargs.Argv) {
17+
let failMessage: string | undefined;
18+
let handlerArgs: Record<string, unknown> | undefined;
19+
const parser = yargs([])
20+
.exitProcess(false)
21+
.fail((msg) => {
22+
failMessage = msg;
23+
});
24+
registerSubcommand(parser, usage, 'test', builder, async (argv) => {
25+
handlerArgs = argv;
26+
});
27+
return {
28+
parseAsync: async (args: string[]) => {
29+
await parser.parseAsync(args);
30+
return handlerArgs!;
31+
},
32+
getError: () => failMessage,
33+
};
34+
}
35+
36+
interface OptionsOnlyCase {
37+
name: string;
38+
usage: string;
39+
options: Record<string, Options>;
40+
args: string[];
41+
expected: Record<string, unknown>;
42+
}
43+
44+
interface MixedPositionalCase extends OptionsOnlyCase {
45+
positionals: Record<string, PositionalOptions>;
46+
}
47+
48+
describe('registerSubcommand parsing (regression)', () => {
49+
const optionsOnlyCases: OptionsOnlyCase[] = [
50+
{
51+
name: 'role create: --slug --name',
52+
usage: 'create',
53+
options: {
54+
slug: { type: 'string', demandOption: true },
55+
name: { type: 'string', demandOption: true },
56+
description: { type: 'string' },
57+
},
58+
args: ['create', '--slug', 'admin', '--name', 'Admin'],
59+
expected: { slug: 'admin', name: 'Admin' },
60+
},
61+
{
62+
name: 'membership create: --org --user',
63+
usage: 'create',
64+
options: {
65+
org: { type: 'string', demandOption: true },
66+
user: { type: 'string', demandOption: true },
67+
role: { type: 'string' },
68+
},
69+
args: ['create', '--org', 'org_1', '--user', 'user_1'],
70+
expected: { org: 'org_1', user: 'user_1' },
71+
},
72+
{
73+
name: 'invitation send: --email',
74+
usage: 'send',
75+
options: {
76+
email: { type: 'string', demandOption: true },
77+
org: { type: 'string' },
78+
role: { type: 'string' },
79+
},
80+
args: ['send', '--email', '[email protected]'],
81+
expected: { email: '[email protected]' },
82+
},
83+
{
84+
name: 'directory list-groups: --directory',
85+
usage: 'list-groups',
86+
options: {
87+
directory: { type: 'string', demandOption: true },
88+
limit: { type: 'number' },
89+
},
90+
args: ['list-groups', '--directory', 'dir_1'],
91+
expected: { directory: 'dir_1' },
92+
},
93+
{
94+
name: 'event list: --events',
95+
usage: 'list',
96+
options: {
97+
events: { type: 'string', demandOption: true },
98+
after: { type: 'string' },
99+
},
100+
args: ['list', '--events', 'user.created'],
101+
expected: { events: 'user.created' },
102+
},
103+
{
104+
name: 'audit-log export: --org --range-start --range-end',
105+
usage: 'export',
106+
options: {
107+
org: { type: 'string', demandOption: true },
108+
'range-start': { type: 'string', demandOption: true },
109+
'range-end': { type: 'string', demandOption: true },
110+
},
111+
args: ['export', '--org', 'org_1', '--range-start', '2026-01-01', '--range-end', '2026-01-31'],
112+
expected: { org: 'org_1' },
113+
},
114+
{
115+
name: 'webhook create: --url --events',
116+
usage: 'create',
117+
options: {
118+
url: { type: 'string', demandOption: true },
119+
events: { type: 'string', demandOption: true },
120+
},
121+
args: ['create', '--url', 'https://example.com', '--events', 'user.created'],
122+
expected: { url: 'https://example.com', events: 'user.created' },
123+
},
124+
{
125+
name: 'portal generate-link: --intent --org',
126+
usage: 'generate-link',
127+
options: {
128+
intent: { type: 'string', demandOption: true },
129+
org: { type: 'string', demandOption: true },
130+
'return-url': { type: 'string' },
131+
},
132+
args: ['generate-link', '--intent', 'sso', '--org', 'org_1'],
133+
expected: { intent: 'sso', org: 'org_1' },
134+
},
135+
{
136+
name: 'vault create: --name --value',
137+
usage: 'create',
138+
options: {
139+
name: { type: 'string', demandOption: true },
140+
value: { type: 'string', demandOption: true },
141+
org: { type: 'string' },
142+
},
143+
args: ['create', '--name', 'secret', '--value', 'abc123'],
144+
expected: { name: 'secret', value: 'abc123' },
145+
},
146+
{
147+
name: 'api-key list: --org',
148+
usage: 'list',
149+
options: {
150+
org: { type: 'string', demandOption: true },
151+
limit: { type: 'number' },
152+
},
153+
args: ['list', '--org', 'org_1'],
154+
expected: { org: 'org_1' },
155+
},
156+
{
157+
name: 'api-key create: --org --name',
158+
usage: 'create',
159+
options: {
160+
org: { type: 'string', demandOption: true },
161+
name: { type: 'string', demandOption: true },
162+
permissions: { type: 'string' },
163+
},
164+
args: ['create', '--org', 'org_1', '--name', 'my-key'],
165+
expected: { org: 'org_1', name: 'my-key' },
166+
},
167+
];
168+
169+
it.each(optionsOnlyCases)('$name', async ({ usage, options, args, expected }) => {
170+
const { parseAsync, getError } = buildParser(usage, (y) => y.options(options));
171+
const argv = await parseAsync(args);
172+
expect(getError()).toBeUndefined();
173+
for (const [key, value] of Object.entries(expected)) {
174+
expect(argv[key]).toBe(value);
175+
}
176+
});
177+
178+
const mixedPositionalCases: MixedPositionalCase[] = [
179+
{
180+
name: 'role set-permissions <slug>: --permissions',
181+
usage: 'set-permissions <slug>',
182+
positionals: { slug: { type: 'string', demandOption: true } },
183+
options: { permissions: { type: 'string', demandOption: true } },
184+
args: ['set-permissions', 'admin', '--permissions', 'read,write'],
185+
expected: { slug: 'admin', permissions: 'read,write' },
186+
},
187+
{
188+
name: 'vault update <id>: --value',
189+
usage: 'update <id>',
190+
positionals: { id: { type: 'string', demandOption: true } },
191+
options: { value: { type: 'string', demandOption: true }, 'version-check': { type: 'string' } },
192+
args: ['update', 'vault_1', '--value', 'new-secret'],
193+
expected: { id: 'vault_1', value: 'new-secret' },
194+
},
195+
{
196+
name: 'audit-log create-schema <action>: --file',
197+
usage: 'create-schema <action>',
198+
positionals: { action: { type: 'string', demandOption: true } },
199+
options: { file: { type: 'string', demandOption: true } },
200+
args: ['create-schema', 'user.login', '--file', 'schema.json'],
201+
expected: { action: 'user.login', file: 'schema.json' },
202+
},
203+
{
204+
name: 'org-domain create <domain>: --org',
205+
usage: 'create <domain>',
206+
positionals: { domain: { type: 'string', demandOption: true } },
207+
options: { org: { type: 'string', demandOption: true } },
208+
args: ['create', 'example.com', '--org', 'org_1'],
209+
expected: { domain: 'example.com', org: 'org_1' },
210+
},
211+
];
212+
213+
it.each(mixedPositionalCases)('$name', async ({ usage, positionals, options, args, expected }) => {
214+
const { parseAsync, getError } = buildParser(usage, (y) => {
215+
for (const [key, opts] of Object.entries(positionals)) {
216+
y.positional(key, opts);
217+
}
218+
return y.options(options);
219+
});
220+
const argv = await parseAsync(args);
221+
expect(getError()).toBeUndefined();
222+
for (const [key, value] of Object.entries(expected)) {
223+
expect(argv[key]).toBe(value);
224+
}
225+
});
226+
});

‎src/utils/register-subcommand.spec.ts‎

Lines changed: 47 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import yargs from 'yargs';
33
import { registerSubcommand } from './register-subcommand.js';
44

55
describe('registerSubcommand', () => {
6-
it('enriches usage with one required string option', () => {
6+
it('enriches description with required flag names', () => {
77
const parent = yargs([]);
88
const commandSpy = vi.spyOn(parent, 'command');
99

@@ -16,14 +16,14 @@ describe('registerSubcommand', () => {
1616
);
1717

1818
expect(commandSpy).toHaveBeenCalledWith(
19-
'create --name <string>',
20-
'Create a resource',
19+
'create',
20+
'Create a resource (requires --name)',
2121
expect.any(Function),
2222
expect.any(Function),
2323
);
2424
});
2525

26-
it('enriches usage with multiple required options', () => {
26+
it('enriches description with multiple required flags', () => {
2727
const parent = yargs([]);
2828
const commandSpy = vi.spyOn(parent, 'command');
2929

@@ -39,12 +39,13 @@ describe('registerSubcommand', () => {
3939
async () => {},
4040
);
4141

42-
const usageArg = commandSpy.mock.calls[0]![0] as string;
43-
expect(usageArg).toContain('--email <string>');
44-
expect(usageArg).toContain('--org-id <string>');
42+
const descArg = commandSpy.mock.calls[0]![1] as string;
43+
expect(descArg).toContain('requires');
44+
expect(descArg).toContain('--email');
45+
expect(descArg).toContain('--org-id');
4546
});
4647

47-
it('leaves usage unchanged when no required options', () => {
48+
it('leaves description unchanged when no required options', () => {
4849
const parent = yargs([]);
4950
const commandSpy = vi.spyOn(parent, 'command');
5051

@@ -59,24 +60,29 @@ describe('registerSubcommand', () => {
5960
expect(commandSpy).toHaveBeenCalledWith('list', 'List resources', expect.any(Function), expect.any(Function));
6061
});
6162

62-
it('preserves positional args and appends required options', () => {
63+
it('excludes positional args from description enrichment', () => {
6364
const parent = yargs([]);
6465
const commandSpy = vi.spyOn(parent, 'command');
6566

6667
registerSubcommand(
6768
parent,
68-
'remove <name>',
69-
'Remove a resource',
70-
(y) => y.options({ force: { type: 'boolean', demandOption: true, describe: 'Force removal' } }),
69+
'update <slug>',
70+
'Update a resource',
71+
(y) =>
72+
y
73+
.positional('slug', { type: 'string', demandOption: true })
74+
.options({ value: { type: 'string', demandOption: true, describe: 'Value' } }),
7175
async () => {},
7276
);
7377

74-
const usageArg = commandSpy.mock.calls[0]![0] as string;
75-
expect(usageArg).toMatch(/^remove <name>/);
76-
expect(usageArg).toContain('--force <boolean>');
78+
const cmdArg = commandSpy.mock.calls[0]![0] as string;
79+
const descArg = commandSpy.mock.calls[0]![1] as string;
80+
expect(cmdArg).toBe('update <slug>');
81+
expect(descArg).toContain('--value');
82+
expect(descArg).not.toContain('--slug');
7783
});
7884

79-
it('filters out help and version from enriched options', () => {
85+
it('filters out help and version from enriched flags', () => {
8086
const parent = yargs([]);
8187
const commandSpy = vi.spyOn(parent, 'command');
8288

@@ -88,27 +94,41 @@ describe('registerSubcommand', () => {
8894
async () => {},
8995
);
9096

91-
const usageArg = commandSpy.mock.calls[0]![0] as string;
92-
expect(usageArg).not.toContain('--help');
93-
expect(usageArg).not.toContain('--version');
94-
expect(usageArg).toContain('--id <string>');
97+
const descArg = commandSpy.mock.calls[0]![1] as string;
98+
expect(descArg).not.toContain('--help');
99+
expect(descArg).not.toContain('--version');
100+
expect(descArg).toContain('--id');
101+
});
102+
103+
it('passes original builder directly (no wrapper)', () => {
104+
const parent = yargs([]);
105+
const commandSpy = vi.spyOn(parent, 'command');
106+
const builder = (y: yargs.Argv) => y.options({ count: { type: 'number', demandOption: true, describe: 'Count' } });
107+
108+
registerSubcommand(parent, 'set', 'Set a value', builder, async () => {});
109+
110+
// Builder is passed through directly — no wrapper
111+
expect(commandSpy).toHaveBeenCalledWith('set', 'Set a value (requires --count)', builder, expect.any(Function));
95112
});
96113

97-
it('handles number type option', () => {
114+
it('skips enrichment when description already mentions the flag', () => {
98115
const parent = yargs([]);
99116
const commandSpy = vi.spyOn(parent, 'command');
100117

101118
registerSubcommand(
102119
parent,
103-
'set',
104-
'Set a value',
105-
(y) => y.options({ count: { type: 'number', demandOption: true, describe: 'Count' } }),
120+
'delete <slug>',
121+
'Delete an org-scoped role (requires --org)',
122+
(y) =>
123+
y
124+
.positional('slug', { type: 'string', demandOption: true })
125+
.options({ org: { type: 'string', demandOption: true } }),
106126
async () => {},
107127
);
108128

109129
expect(commandSpy).toHaveBeenCalledWith(
110-
'set --count <number>',
111-
'Set a value',
130+
'delete <slug>',
131+
'Delete an org-scoped role (requires --org)',
112132
expect.any(Function),
113133
expect.any(Function),
114134
);
@@ -126,7 +146,7 @@ describe('registerSubcommand', () => {
126146
expect(result).toBe(parent);
127147
});
128148

129-
it('falls back to unenriched usage when builder throws', () => {
149+
it('falls back to unenriched description when builder throws', () => {
130150
const parent = yargs([]);
131151
const commandSpy = vi.spyOn(parent, 'command');
132152

0 commit comments

Comments
 (0)