Uh oh!
There was an error while loading. Please reload this page.
fix(run-insights): reject empty --name "" instead of auto-generating - #1600
Conversation
Passing an explicit empty string for --name silently fell through to the
auto-generation path. Reject it with a ValidationError so the user gets a
clear "must not be empty" error instead of an opaque success with a
random name.
Also tightens the lookback-days error message to match the existing
service-side `^[a-zA-Z][a-zA-Z0-9_]{0,47}$` BatchEvaluationName contract.Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1600-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.20.2.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for the fix — the empty-string bypass is a real bug worth catching. One change request before merge:
The empty---name check is placed outside runCliCommand, which breaks two behaviors the rest of this file relies on:
No telemetry on failure. Errors thrown inside
runCliCommandare classified and emitted ascli.command_runfailure events (seerunCliCommand→trackCommandRuninsrc/cli/telemetry/cli-command-run.ts). Throwing at line 402 bypasses that entirely, so therun.insightscommand never records this validation failure. Persrc/cli/telemetry/README.md, validation errors should flow through the telemetry-wrapped path.Breaks the
--jsoncontract.runCliCommandformats errors as{ success: false, error: ... }and exits 1 when--jsonis set. With the check outside the wrapper, the throw propagates to Commander's default handler, which prints a plain-text error / stack to stderr. Soagentcore run insights --name "" --jsonwill not produce parseable JSON on stdout, even though the PR description implies it does. (The manual test in the PR description only asserts the message text, not that stdout is valid JSON.)
The fix is to move the check inside the runCliCommand callback, right alongside the existing --lookback-days check (which this PR correctly leaves inside the wrapper):
awaitrunCliCommand('run.job',!!cliOptions.json,async()=>{if(cliOptions.name!==undefined&&cliOptions.name.trim()===''){thrownewValidationError('--name must not be empty. Omit the flag to auto-generate a name.');}constengine=createJobEngine(newConfigIO());// ...This matches the pattern used by run ingest further down in the same file (line 672-674), where --name validation is inside runCliCommand.
Two smaller observations (not blocking):
- No unit test was added for the new behavior. The existing test file only covers
validateLookbackDays. Sincecommand.tsxdoesn't have a test harness, that's understandable, but worth noting given this is a P1 bug-bash item. cliOptions.name?.trim() === ''also rejects whitespace-only names (e.g.--name " "), which is probably desirable but isn't called out in the PR description or message. Consider whether the user-facing message should mention whitespace, or whether to trim before validating againstNAME_PATTERN.
Uh oh!
There was an error while loading. Please reload this page.
Addresses review feedback: placing the check inside runCliCommand ensures validation failures are recorded by telemetry and produce parseable JSON when --json is set.
Claude Security Review: no high-confidence findings. (run) |
Uh oh!
There was an error while loading. Please reload this page.
Summary
agentcore run insights --name \"\"was silently accepted: the empty string slipped through the truthy check inresolveInsightsNameand the CLI auto-generated a name. Users who explicitly typed--name \"\"now get a clearValidationErrorinstead.--lookback-dayserror message wording to align with the service-sideBatchEvaluationNamecontract (^[a-zA-Z][a-zA-Z0-9_]{0,47}$) referenced inAgentCoreEvaluationCommonModel.Bug : "agentcore run insights --name "" is accepted — the CLI silently ignores the empty string and auto-generates a name."