feat: implement unknown flag detection and enhance search result messaging#106
feat: implement unknown flag detection and enhance search result messaging#106
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements unknown flag detection for search commands and enhances search result messaging to provide better user feedback. The changes help users understand when they've used invalid flags and provide contextual hints when searches return no results or potentially truncated results.
Changes:
- Added
detectUnknownSearchFlags()function to identify flags not valid for specific search resources - Added early warning via
warnUnknownSearchFlags()when users provide unrecognized flags - Enhanced "no results" messaging with 🕳️ emoji and contextual hints about unused flags or missing filters
- Added truncation warnings when result counts match the API default page size (100)
- Updated all search functions to accept
_unknownFlagsparameter for coordinated warning messages - Comprehensive unit test coverage for the new flag detection functionality
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/search-feedback.test.ts | Comprehensive unit tests for unknown flag detection, GLOBAL_FLAGS, and SEARCH_RESOURCE_FLAGS validation |
| src/index.ts | Added warnUnknownSearchFlags() helper and integrated detectUnknownSearchFlags() into search command flow; also standardized processDefinitionId resolution across search commands |
| src/commands/search.ts | Implemented detectUnknownSearchFlags(), defined GLOBAL_FLAGS and SEARCH_RESOURCE_FLAGS, added logNoResults() and logResultCount() helpers, updated all search functions to accept _unknownFlags parameter |
Comments suppressed due to low confidence (1)
src/commands/search.ts:173
- When no results are found and unknown flags exist, this warning is redundant because warnUnknownSearchFlags() already warned about the same flags before the search executed (see index.ts line 673). Consider removing the unknown flags warning from logNoResults since users have already been notified. The function could still check unknownFlags to skip the "no filters were applied" message when appropriate, but shouldn't repeat the warning.
function logNoResults(logger: Logger, resourceName: string, hasFilters: boolean, unknownFlags?: string[]): void {
logger.info(`🕳️ No ${resourceName} found matching the criteria`);
if (unknownFlags && unknownFlags.length > 0) {
logger.warn(`Possibly unused flag(s): ${unknownFlags.map(f => `--${f}`).join(', ')}. Run "c8ctl help search" for valid options.`);
} else if (!hasFilters) {
logger.info('No filters were applied. Use "c8ctl help search" to see available filter flags.');
}
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot investigate GH action failure. Concentrate on your latest change, find the corresponding tests. These might very likely be the issue. |
| * for the specific resource, causing silent filter drops. | ||
| */ | ||
| export const SEARCH_RESOURCE_FLAGS: Record<string, Set<string>> = { | ||
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname']), |
There was a problem hiding this comment.
The process-definition flags are missing 'version' in the SEARCH_RESOURCE_FLAGS set. The version filter is a valid option for process-definition searches (see searchProcessDefinitions function signature at line 195 and usage at src/index.ts:680). However, note that src/index.ts:680 accesses 'version_num' instead of 'version', which appears to be inconsistent with the parseArgs configuration that defines 'version' (src/index.ts:77). This needs clarification - if 'version_num' is the correct property name, it should be added to the flags set; if 'version' is correct, then both the flags set and line 680 need updating.
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname']), | |
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname', 'version_num']), |
src/index.ts
Outdated
| processDefinitionId: values.bpmnProcessId as string | undefined, | ||
| processDefinitionId: resolveProcessDefinitionId(values), | ||
| name: values.name as string | undefined, | ||
| version: (values.version_num && typeof values.version_num === 'string') ? parseInt(values.version_num) : undefined, |
There was a problem hiding this comment.
The code accesses 'values.version_num' but this property is not defined in the parseArgs options configuration (src/index.ts:73-130). The parseArgs options include 'version' (line 77) but not 'version_num'. This will result in 'version_num' always being undefined, making the version filter for process-definition search non-functional. Either: 1) Change this to use 'values.version' to match the parseArgs configuration, or 2) Add 'version_num' to the parseArgs options if that's the intended flag name.
| version: (values.version_num && typeof values.version_num === 'string') ? parseInt(values.version_num) : undefined, | |
| version: (values.version && typeof values.version === 'string') ? parseInt(values.version) : undefined, |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/commands/search.ts:29
- The
limitflag is included in bothGLOBAL_FLAGS(line 14-16) and inSEARCH_RESOURCE_FLAGS['variable'](line 29). SincedetectUnknownSearchFlagschecks GLOBAL_FLAGS first (line 44), having it in the resource-specific set is redundant. Consider removing 'limit' from the 'variable' resource flags for consistency.
'jobs': new Set(['state', 'type', 'processInstanceKey', 'processDefinitionKey', 'itype']),
src/commands/search.ts
Outdated
| /** | ||
| * Log a "no results" message with 🕳️ emoji and contextual hint. | ||
| */ | ||
| function logNoResults(logger: Logger, resourceName: string, hasFilters: boolean): void { |
There was a problem hiding this comment.
Function signature mismatch: logNoResults is defined with 3 parameters (logger, resourceName, hasFilters) but is being called with 4 parameters in all usage sites (lines 288, 390, 493, 612, 707, 847), where the 4th parameter is options._unknownFlags. This will cause a runtime error. Either add the fourth parameter to the function signature or remove it from the call sites.
| function logNoResults(logger: Logger, resourceName: string, hasFilters: boolean): void { | |
| function logNoResults(logger: Logger, resourceName: string, hasFilters: boolean, _unknownFlags?: unknown): void { |
| * for the specific resource, causing silent filter drops. | ||
| */ | ||
| export const SEARCH_RESOURCE_FLAGS: Record<string, Set<string>> = { | ||
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname']), |
There was a problem hiding this comment.
Missing flag in SEARCH_RESOURCE_FLAGS: The search process definitions handler in index.ts (line 680) accepts version_num flag, but 'version_num' is not included in SEARCH_RESOURCE_FLAGS['process-definition']. This will cause detectUnknownSearchFlags to incorrectly report it as an unknown flag if a user tries to use it. Add 'version_num' to the process-definition flag set.
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname']), | |
| 'process-definition': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'name', 'key', 'iid', 'iname', 'version_num']), |
| /** | ||
| * Unit tests for search feedback improvements: | ||
| * - Unknown flag detection | ||
| * - Empty result messaging with 🕳️ | ||
| * - Truncation warnings | ||
| * - No-filter hints | ||
| */ | ||
|
|
||
| import { test, describe } from 'node:test'; | ||
| import assert from 'node:assert'; | ||
| import { | ||
| detectUnknownSearchFlags, | ||
| GLOBAL_FLAGS, | ||
| SEARCH_RESOURCE_FLAGS, | ||
| } from '../../src/commands/search.ts'; | ||
|
|
||
| describe('detectUnknownSearchFlags', () => { | ||
| test('returns empty array when no flags are set', () => { | ||
| const values = {}; | ||
| assert.deepStrictEqual(detectUnknownSearchFlags(values, 'process-definition'), []); | ||
| }); | ||
|
|
||
| test('returns empty array when only global flags are set', () => { | ||
| const values = { profile: 'dev', sortBy: 'Name', asc: true }; | ||
| assert.deepStrictEqual(detectUnknownSearchFlags(values, 'process-definition'), []); | ||
| }); | ||
|
|
||
| test('returns empty array when valid resource flags are set', () => { | ||
| const values = { bpmnProcessId: 'my-process', name: 'My Process' }; | ||
| assert.deepStrictEqual(detectUnknownSearchFlags(values, 'process-definition'), []); | ||
| }); | ||
|
|
||
| test('detects unknown flag for process-definitions', () => { | ||
| const values = { assignee: 'john', name: 'My Process' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'process-definition'); | ||
| assert.deepStrictEqual(unknown, ['assignee']); | ||
| }); | ||
|
|
||
| test('detects unknown flag for user-tasks', () => { | ||
| const values = { name: 'test', state: 'CREATED' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'user-task'); | ||
| assert.deepStrictEqual(unknown, ['name']); | ||
| }); | ||
|
|
||
| test('detects unknown flag for jobs', () => { | ||
| const values = { assignee: 'john', type: 'email' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'jobs'); | ||
| assert.deepStrictEqual(unknown, ['assignee']); | ||
| }); | ||
|
|
||
| test('detects unknown flag for variables', () => { | ||
| const values = { type: 'some-type', name: 'myVar' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'variable'); | ||
| assert.deepStrictEqual(unknown, ['type']); | ||
| }); | ||
|
|
||
| test('detects unknown flag for incidents', () => { | ||
| const values = { assignee: 'john', state: 'ACTIVE' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'incident'); | ||
| assert.deepStrictEqual(unknown, ['assignee']); | ||
| }); | ||
|
|
||
| test('ignores undefined and false values', () => { | ||
| const values = { assignee: undefined, errorType: false, state: 'ACTIVE' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'jobs'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('detects multiple unknown flags at once', () => { | ||
| const values = { assignee: 'john', errorType: 'IO', type: 'email' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'jobs'); | ||
| // assignee and errorType are not valid for jobs | ||
| assert.ok(unknown.includes('assignee')); | ||
| assert.ok(unknown.includes('errorType')); | ||
| assert.strictEqual(unknown.length, 2); | ||
| }); | ||
|
|
||
| test('handles pluralized resource names (process-definitions → process-definition)', () => { | ||
| const values = { name: 'test' }; | ||
| // 'process-definitions' is not a key; the function strips trailing 's' as fallback | ||
| const unknown = detectUnknownSearchFlags(values, 'process-definitions'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('handles variables (pluralized → variable)', () => { | ||
| const values = { name: 'myVar', value: 'hello' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'variables'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('--id is valid for process-definition search', () => { | ||
| const values = { id: 'my-process' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'process-definition'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('--processDefinitionId is valid for process-definition search', () => { | ||
| const values = { processDefinitionId: 'my-process' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'process-definition'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('--id is valid for process-instance search', () => { | ||
| const values = { id: 'my-process' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'process-instance'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('--processDefinitionId is valid for incident search', () => { | ||
| const values = { processDefinitionId: 'my-process' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'incident'); | ||
| assert.deepStrictEqual(unknown, []); | ||
| }); | ||
|
|
||
| test('returns empty array for unknown resource', () => { | ||
| const values = { foo: 'bar' }; | ||
| assert.deepStrictEqual(detectUnknownSearchFlags(values, 'unknown-resource'), []); | ||
| }); | ||
|
|
||
| test('truly unknown flags (not in any resource) are detected', () => { | ||
| const values = { fooBarBaz: 'test' }; | ||
| const unknown = detectUnknownSearchFlags(values, 'process-definition'); | ||
| assert.deepStrictEqual(unknown, ['fooBarBaz']); | ||
| }); | ||
| }); | ||
|
|
||
| describe('GLOBAL_FLAGS', () => { | ||
| test('contains expected common flags', () => { | ||
| assert.ok(GLOBAL_FLAGS.has('profile')); | ||
| assert.ok(GLOBAL_FLAGS.has('sortBy')); | ||
| assert.ok(GLOBAL_FLAGS.has('asc')); | ||
| assert.ok(GLOBAL_FLAGS.has('desc')); | ||
| assert.ok(GLOBAL_FLAGS.has('help')); | ||
| assert.ok(GLOBAL_FLAGS.has('version')); | ||
| assert.ok(GLOBAL_FLAGS.has('limit')); | ||
| }); | ||
| }); | ||
|
|
||
| describe('SEARCH_RESOURCE_FLAGS', () => { | ||
| test('process-definition includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['process-definition']; | ||
| assert.ok(flags.has('bpmnProcessId')); | ||
| assert.ok(flags.has('id')); | ||
| assert.ok(flags.has('processDefinitionId')); | ||
| assert.ok(flags.has('name')); | ||
| assert.ok(flags.has('key')); | ||
| assert.ok(flags.has('iid')); | ||
| assert.ok(flags.has('iname')); | ||
| }); | ||
|
|
||
| test('process-instance includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['process-instance']; | ||
| assert.ok(flags.has('bpmnProcessId')); | ||
| assert.ok(flags.has('id')); | ||
| assert.ok(flags.has('processDefinitionId')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('state')); | ||
| assert.ok(flags.has('key')); | ||
| assert.ok(flags.has('parentProcessInstanceKey')); | ||
| assert.ok(flags.has('iid')); | ||
| }); | ||
|
|
||
| test('user-task includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['user-task']; | ||
| assert.ok(flags.has('state')); | ||
| assert.ok(flags.has('assignee')); | ||
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('elementId')); | ||
| assert.ok(flags.has('iassignee')); | ||
| }); | ||
|
|
||
| test('incident includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['incident']; | ||
| assert.ok(flags.has('state')); | ||
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('bpmnProcessId')); | ||
| assert.ok(flags.has('id')); | ||
| assert.ok(flags.has('processDefinitionId')); | ||
| assert.ok(flags.has('errorType')); | ||
| assert.ok(flags.has('errorMessage')); | ||
| assert.ok(flags.has('ierrorMessage')); | ||
| assert.ok(flags.has('iid')); | ||
| }); | ||
|
|
||
| test('jobs includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['jobs']; | ||
| assert.ok(flags.has('state')); | ||
| assert.ok(flags.has('type')); | ||
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('itype')); | ||
| }); | ||
|
|
||
| test('variable includes all expected flags', () => { | ||
| const flags = SEARCH_RESOURCE_FLAGS['variable']; | ||
| assert.ok(flags.has('name')); | ||
| assert.ok(flags.has('value')); | ||
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('scopeKey')); | ||
| assert.ok(flags.has('fullValue')); | ||
| assert.ok(flags.has('iname')); | ||
| assert.ok(flags.has('ivalue')); | ||
| assert.ok(flags.has('limit')); | ||
| }); | ||
|
|
||
| test('all resources have entries', () => { | ||
| const resources = ['process-definition', 'process-instance', 'user-task', 'incident', 'jobs', 'variable']; | ||
| for (const resource of resources) { | ||
| assert.ok(SEARCH_RESOURCE_FLAGS[resource], `Missing entry for ${resource}`); | ||
| assert.ok(SEARCH_RESOURCE_FLAGS[resource].size > 0, `Empty flags for ${resource}`); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage gap: The tests verify detectUnknownSearchFlags logic but don't test the integration with search command functions, particularly the case where empty results are returned (which would exercise logNoResults). According to custom guideline 1000000, tests should be comprehensive and cover edge cases. Consider adding integration tests that verify the warning messages are properly displayed when searching with no results and with unknown flags.
* Initial plan * fix: update GLOBAL_FLAGS test to reflect limit removal from global flags Co-authored-by: vobu <6573426+vobu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: vobu <6573426+vobu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/index.ts:690
- Use
parseInt(values.version, 10)(radix 10) for consistency with other numeric parsing in this file (e.g.--limit) and to avoid edge-case parsing differences.
version: (values.version && typeof values.version === 'string') ? parseInt(values.version) : undefined,
key: values.key as string | undefined,
| 'process-instance': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'processDefinitionKey', 'state', 'key', 'parentProcessInstanceKey', 'iid']), | ||
| 'user-task': new Set(['state', 'assignee', 'processInstanceKey', 'processDefinitionKey', 'elementId', 'iassignee']), | ||
| 'incident': new Set(['state', 'processInstanceKey', 'processDefinitionKey', 'bpmnProcessId', 'id', 'processDefinitionId', 'errorType', 'errorMessage', 'ierrorMessage', 'iid']), | ||
| 'jobs': new Set(['state', 'type', 'processInstanceKey', 'processDefinitionKey', 'itype']), |
There was a problem hiding this comment.
SEARCH_RESOURCE_FLAGS is missing flags that are actually supported by the corresponding search handlers, so users will get false "unknown flag" warnings. For example, searchProcessInstances/searchUserTasks/searchJobs accept --between and --dateField, and searchIncidents accepts --between, but none of these appear in the per-resource sets. Add the missing flags to the appropriate resource entries so detectUnknownSearchFlags() doesn’t warn on documented options.
| 'process-instance': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'processDefinitionKey', 'state', 'key', 'parentProcessInstanceKey', 'iid']), | |
| 'user-task': new Set(['state', 'assignee', 'processInstanceKey', 'processDefinitionKey', 'elementId', 'iassignee']), | |
| 'incident': new Set(['state', 'processInstanceKey', 'processDefinitionKey', 'bpmnProcessId', 'id', 'processDefinitionId', 'errorType', 'errorMessage', 'ierrorMessage', 'iid']), | |
| 'jobs': new Set(['state', 'type', 'processInstanceKey', 'processDefinitionKey', 'itype']), | |
| 'process-instance': new Set(['bpmnProcessId', 'id', 'processDefinitionId', 'processDefinitionKey', 'state', 'key', 'parentProcessInstanceKey', 'iid', 'between', 'dateField']), | |
| 'user-task': new Set(['state', 'assignee', 'processInstanceKey', 'processDefinitionKey', 'elementId', 'iassignee', 'between', 'dateField']), | |
| 'incident': new Set(['state', 'processInstanceKey', 'processDefinitionKey', 'bpmnProcessId', 'id', 'processDefinitionId', 'errorType', 'errorMessage', 'ierrorMessage', 'iid', 'between']), | |
| 'jobs': new Set(['state', 'type', 'processInstanceKey', 'processDefinitionKey', 'itype', 'between', 'dateField']), |
| export const GLOBAL_FLAGS = new Set([ | ||
| 'profile', 'sortBy', 'asc', 'desc', 'help', 'version', | ||
| ]); |
There was a problem hiding this comment.
Including version in GLOBAL_FLAGS means --version will never be reported as unknown for any search <resource> invocation, even though it’s only meaningful for certain commands/resources. To make unknown-flag detection effective, consider removing version from GLOBAL_FLAGS and instead adding it only to the specific SEARCH_RESOURCE_FLAGS entries where it’s actually consumed (e.g. process-definition if supported there).
| assert.ok(flags.has('errorType')); | ||
| assert.ok(flags.has('errorMessage')); | ||
| assert.ok(flags.has('ierrorMessage')); | ||
| assert.ok(flags.has('iid')); |
There was a problem hiding this comment.
The test for incident flags is missing an assertion for 'between', which is a valid flag according to the help documentation and implementation (src/commands/search.ts lines 559, 594-595). Note that 'dateField' is NOT valid for incidents (only uses creationTime). Once 'between' is added to SEARCH_RESOURCE_FLAGS['incident'] to fix the bug, this test should verify it's present.
| assert.ok(flags.has('iid')); | |
| assert.ok(flags.has('iid')); | |
| assert.ok(flags.has('between')); |
| assert.ok(flags.has('type')); | ||
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('itype')); |
There was a problem hiding this comment.
The test for jobs flags is missing assertions for 'between' and 'dateField', which are valid flags according to the help documentation and implementation (src/commands/search.ts lines 689-690, 714-716). Once these flags are added to SEARCH_RESOURCE_FLAGS['jobs'] to fix the bug, this test should verify they are present.
| assert.ok(flags.has('itype')); | |
| assert.ok(flags.has('itype')); | |
| assert.ok(flags.has('between')); | |
| assert.ok(flags.has('dateField')); |
| assert.ok(GLOBAL_FLAGS.has('version')); | ||
| }); | ||
|
|
||
| test('does not contain limit (limit is only valid for variable search)', () => { | ||
| assert.ok(!GLOBAL_FLAGS.has('limit')); |
There was a problem hiding this comment.
This test expects 'version' to be in GLOBAL_FLAGS, but this is inconsistent with the design. The 'version' flag is only valid for process-definition searches (to filter by version number), just like 'limit' is only valid for variable searches. The test on line 153-155 correctly demonstrates that resource-specific flags like 'limit' should NOT be in GLOBAL_FLAGS. Once 'version' is removed from GLOBAL_FLAGS (to fix the bug in src/commands/search.ts lines 15-17), this test will need to be updated to remove the assertion for 'version' or changed to assert it's NOT in GLOBAL_FLAGS.
| assert.ok(GLOBAL_FLAGS.has('version')); | |
| }); | |
| test('does not contain limit (limit is only valid for variable search)', () => { | |
| assert.ok(!GLOBAL_FLAGS.has('limit')); | |
| }); | |
| test('does not contain resource-specific flags (e.g. limit, version)', () => { | |
| assert.ok(!GLOBAL_FLAGS.has('limit')); | |
| assert.ok(!GLOBAL_FLAGS.has('version')); |
| assert.ok(flags.has('name')); | ||
| assert.ok(flags.has('key')); | ||
| assert.ok(flags.has('iid')); | ||
| assert.ok(flags.has('iname')); |
There was a problem hiding this comment.
The test for process-definition flags should include an assertion for 'version', which is a valid flag for process-definition searches according to the implementation in src/commands/search.ts (lines 199, 220-222, 251-253). Once 'version' is added to SEARCH_RESOURCE_FLAGS['process-definition'] to fix the bug, this test should verify that it's present with: assert.ok(flags.has('version'));
| assert.ok(flags.has('iname')); | |
| assert.ok(flags.has('iname')); | |
| assert.ok(flags.has('version')); |
| assert.ok(flags.has('processInstanceKey')); | ||
| assert.ok(flags.has('processDefinitionKey')); | ||
| assert.ok(flags.has('elementId')); | ||
| assert.ok(flags.has('iassignee')); |
There was a problem hiding this comment.
The test for user-task flags is missing assertions for 'between' and 'dateField', which are valid flags according to the help documentation and implementation (src/commands/search.ts lines 437-438, 465-467). Once these flags are added to SEARCH_RESOURCE_FLAGS['user-task'] to fix the bug, this test should verify they are present.
| assert.ok(flags.has('iassignee')); | |
| assert.ok(flags.has('iassignee')); | |
| assert.ok(flags.has('between')); | |
| assert.ok(flags.has('dateField')); |
| assert.ok(flags.has('state')); | ||
| assert.ok(flags.has('key')); | ||
| assert.ok(flags.has('parentProcessInstanceKey')); | ||
| assert.ok(flags.has('iid')); |
There was a problem hiding this comment.
The test for process-instance flags is missing assertions for 'between' and 'dateField', which are valid flags according to the help documentation and implementation (src/commands/search.ts lines 318-319, 346-348). Once these flags are added to SEARCH_RESOURCE_FLAGS['process-instance'] to fix the bug, this test should verify they are present.
| assert.ok(flags.has('iid')); | |
| assert.ok(flags.has('iid')); | |
| assert.ok(flags.has('between')); | |
| assert.ok(flags.has('dateField')); |
|
@copilot the tests pass locally. In GH Action they fail. Find the root cause and fix. |
#109) * Initial plan * fix: skip global-install tests when dist/index.js is absent (CI fix) Co-authored-by: vobu <6573426+vobu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: vobu <6573426+vobu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
tests/integration/global-install.test.ts:80
- Test name says it verifies
version, but it actually invokeshelpand asserts the help header includes a version string. Consider renaming the test to reflect what it’s doing (e.g. “help output includes version when invoked through a symlink”).
test('binary shows version when invoked through a symlink', () => {
symlinkSync(distEntry, symlinkPath);
const result = spawnSync('node', [symlinkPath, 'help'], {
encoding: 'utf-8',
timeout: 10_000,
});
assert.strictEqual(result.status, 0, `Expected exit 0, got ${result.status}. stderr: ${result.stderr}`);
// Version line appears in help output header
assert.match(result.stdout, /v\d+\.\d+\.\d+/, 'output should contain a version string');
});
| process.exit(1); | ||
| }); | ||
| } | ||
| } catch { /* not invoked directly */ } |
There was a problem hiding this comment.
The direct-invocation guard swallows all errors from realpathSync(process.argv[1]). If realpathSync fails for an unexpected reason (e.g. permissions), the CLI will silently not run. Consider narrowing the catch (e.g. only ignore ENOENT/EINVAL) or falling back to the previous pathname comparison and/or emitting an error when invoked directly but path resolution fails.
| } catch { /* not invoked directly */ } | |
| } catch (error: unknown) { | |
| // Only ignore expected "not a normal direct invocation" errors; surface others | |
| const nodeError = error as { code?: unknown } | null; | |
| const code = nodeError && typeof nodeError.code === 'string' ? nodeError.code : undefined; | |
| if (code === 'ENOENT' || code === 'EINVAL') { | |
| // Treat as "not invoked directly" (e.g. unusual invocation contexts) | |
| return; | |
| } | |
| console.error('Failed to resolve CLI entry point. The CLI could not determine whether it was invoked directly.'); | |
| console.error(error); | |
| process.exit(1); | |
| } |
| const projectRoot = resolve(import.meta.dirname, '..', '..'); | ||
| const distEntry = join(projectRoot, 'dist', 'index.js'); | ||
| const skipReason = existsSync(distEntry) ? undefined : 'dist/index.js not found - run "npm run build" first'; | ||
|
|
||
| describe('Global Install (symlink) Integration Tests', { skip: skipReason }, () => { |
There was a problem hiding this comment.
These integration tests are skipped unless dist/index.js already exists, but npm test doesn’t build dist/ first. In CI this likely means the entire suite never runs and won’t catch regressions in the entrypoint guard. Consider building as part of the integration test run (e.g. a pretest:integration hook), or invoking the TS entrypoint in a way that still exercises the symlink/realpath behavior.
| profile: values.profile as string | undefined, | ||
| processDefinitionId: resolveProcessDefinitionId(values), | ||
| name: values.name as string | undefined, | ||
| version: (values.version_num && typeof values.version_num === 'string') ? parseInt(values.version_num) : undefined, | ||
| version: (values.version && typeof values.version === 'string') ? parseInt(values.version) : undefined, | ||
| key: values.key as string | undefined, |
There was a problem hiding this comment.
values.version is being parsed as the process-definition filter version. This conflicts with the documented global --version/-v flag (intended to show CLI version) and also makes detectUnknownSearchFlags() treat version as globally valid (so search <other-resource> --version ... won’t be warned about). Consider introducing a dedicated filter flag (e.g. --processDefinitionVersion) and keeping --version as a boolean global flag; also validate the parsed number (avoid passing NaN).
|
This has been released in 2.4.0-alpha.1. |
|
This has been released in 2.4.0. |
(ping back to https://github.com/camunda/product-hub/issues/2638 for ref)