fix(status): exit non-zero when --type/--state filter value is invalid#1169
fix(status): exit non-zero when --type/--state filter value is invalid#1169aidandaly24 wants to merge 1 commit intomainfrom
Conversation
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Looks good. The fix correctly replaces return with process.exit(1) for the two validation branches, matching the existing convention used in the catch block of the same action (line 237). Tests cover both --type and --state paths and assert the exit code.
No blocking issues found. Nits I considered but did not flag:
- When
--jsonis set, the invalid-value branches still emit a non-JSON Ink-rendered error. This is pre-existing behavior (not introduced by this PR) and out of scope for fixing #984, but might be worth a follow-up so scripted/CI JSON consumers get a parseable error payload. render(...)is async in Ink andprocess.exit(1)runs on the next microtask line; the same pattern is already used elsewhere in the CLI and appears to flush reliably for simple<Text>output, so I don't think it's worth changing here.
Coverage Report
|
013d6f8 to
bbf9ee4
Compare
| }); | ||
|
|
||
| describe('registerStatus --type / --state validation exit code', () => { | ||
| const renderMock = vi.fn(); |
There was a problem hiding this comment.
the fact that we have to mock multiple modules to verify the validation logic feels like a code smell. I understand this fixes the bug, but I'm wondering if its worth refactoring the implementation to not only fix the bug, but also restructure it to make regressions more difficult to introduce.
Ex. extract the validation logic into a pure function and test that directly.
Description
agentcore status --type <invalid>(and--state <invalid>) printed an error message but returned exit code 0. Non-interactive callers — scripts, CI pipelines, automation — had no way to detect the rejection and would happily continue as if nothing was wrong.The fix is minimal: in
src/cli/commands/status/command.tsx, after rendering the validation error for an invalid--typeor--state, replace the barereturn;withprocess.exit(1);. This matches the convention already used in this same file's catch handler (line 297). Two lines changed in production code.Reviewer findings round 1: 1 finding, all approved.
Before
After
Related Issue
Closes #984
Documentation PR
N/A — no user-facing documentation changes.
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsAdded two unit tests in
src/cli/commands/status/__tests__/action.test.tsthat register thestatuscommand on a Commander program, parse it with--type bogus/--state bogus, and assertprocess.exitis called with1. All 36 tests in this file pass; full CI (format, lint, typecheck, unit-test 1/3 + 2/3 + 3/3, build, CodeQL, security, secrets, ai-review) is green.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.