Skip to content

fix(tui): validate empty input in SecretInput before triggering cancel#1667

Closed
notgitika wants to merge 72 commits into
mainfrom
fix/secret-input-empty-validation
Closed

fix(tui): validate empty input in SecretInput before triggering cancel#1667
notgitika wants to merge 72 commits into
mainfrom
fix/secret-input-empty-validation

Conversation

@notgitika

Copy link
Copy Markdown
Contributor

Summary

  • Fixes the "add credential" TUI flow going back to the previous step when pressing Enter with no API key input
  • SecretInput now runs customValidation on empty values before falling through to onCancel, showing the validation error instead of navigating back
  • Moved the empty-value early return in validateValue() to after customValidation runs, so validators that reject empty strings are respected

Test plan

  • Added unit test: "shows validation error instead of calling onCancel when customValidation rejects empty value"
  • All 24 existing SecretInput tests continue to pass
  • Behavior unchanged when no customValidation is provided (empty Enter still calls onCancel)
  • Behavior unchanged when onSkip is provided (empty Enter still calls onSkip)

Fixes #1618

avi-alpert and others added 30 commits March 5, 2026 13:20
GITHUB_TOKEN lacks the 'workflows' permission, so pushing workflow
file changes from the public repo causes the sync to fail. Use
--no-commit --no-ff and restore .github/workflows/ from HEAD before
committing, in both the clean merge and conflict paths.
dependabot Bot and others added 18 commits May 26, 2026 12:53
…180)

Bumps the github-actions group with 5 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [actions/checkout](https://github.com/actions/checkout) | `4` | `6` |
| [actions/create-github-app-token](https://github.com/actions/create-github-app-token) | `1` | `3` |
| [aws-actions/aws-secretsmanager-get-secrets](https://github.com/aws-actions/aws-secretsmanager-get-secrets) | `2` | `3` |
| [softprops/action-gh-release](https://github.com/softprops/action-gh-release) | `2` | `3` |
| [slackapi/slack-github-action](https://github.com/slackapi/slack-github-action) | `2.1.1` | `3.0.3` |



Updates `actions/checkout` from 4 to 6
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v4...v6)

Updates `actions/create-github-app-token` from 1 to 3
- [Release notes](https://github.com/actions/create-github-app-token/releases)
- [Changelog](https://github.com/actions/create-github-app-token/blob/main/CHANGELOG.md)
- [Commits](actions/create-github-app-token@v1...v3)

Updates `aws-actions/aws-secretsmanager-get-secrets` from 2 to 3
- [Release notes](https://github.com/aws-actions/aws-secretsmanager-get-secrets/releases)
- [Commits](aws-actions/aws-secretsmanager-get-secrets@v2...v3)

Updates `softprops/action-gh-release` from 2 to 3
- [Release notes](https://github.com/softprops/action-gh-release/releases)
- [Changelog](https://github.com/softprops/action-gh-release/blob/master/CHANGELOG.md)
- [Commits](softprops/action-gh-release@v2...v3)

Updates `slackapi/slack-github-action` from 2.1.1 to 3.0.3
- [Release notes](https://github.com/slackapi/slack-github-action/releases)
- [Changelog](https://github.com/slackapi/slack-github-action/blob/main/CHANGELOG.md)
- [Commits](slackapi/slack-github-action@v2.1.1...v3.0.3)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-version: '6'
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: actions/create-github-app-token
  dependency-version: '3'
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: aws-actions/aws-secretsmanager-get-secrets
  dependency-version: '3'
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: slackapi/slack-github-action
  dependency-version: 3.0.3
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: softprops/action-gh-release
  dependency-version: '3'
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Includes the telemetry migration that removed src/cli/telemetry/error-classification.ts
in favor of BaseError.errorSource, plus harness primitive, dataset polish, and
related bug fixes. Workflow conflicts (.github/workflows/{e2e-tests,release-main-and-preview}.yml,
prerelease-tarball.yml, sync-preview.yml) resolved in favor of staging's CI infra.
The fuzzy text search (`--search "Merge public/main"`) was matching
unrelated open PRs, causing the sync to skip creating conflict PRs.
Switch to branch prefix matching (sync-conflict-main-* /
sync-conflict-preview-*) which is exact and consistent with how
sync-preview.yml detects its own PRs.

Co-authored-by: jariy17 <tjariy+jariy17@users.noreply.github.com>
* chore: simplify release-main-and-preview workflow (#1415)

Rename workflow to "Release Main and Preview" to distinguish it from the
single-release workflow in the Actions UI. Remove redundant test-main and
test-preview jobs since the release PR already triggers build-and-test CI.

Confidence: high
Scope-risk: narrow

* Release v0.16.0 + preview v1.0.0-preview.10 (#1417)

* chore: bump main to 0.16.0, preview to 1.0.0-preview.10

* Update CHANGELOG for version 0.16.0

Document notable changes for version 0.16.0, including new features, fixes, and updates.

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jesse Turner <57651174+jesseturner21@users.noreply.github.com>

* fix: restore --skills invoke override for harness (preview regression) (#1418)

The --skills flag was present on the preview branch but was accidentally
dropped during the preview→main feature flag consolidation. The type
field existed in InvokeOptions but was never wired to a CLI flag or
passed through buildHarnessBaseOpts.

Restores:
- --skills <paths> CLI flag (preview-gated)
- skills handling in buildHarnessBaseOpts to send to InvokeHarness API

* fix(telemetry): track preflight error for telemetry (#1403)

* fix(telemetry): track preflight error for telemetry

* feat: model userCancellationError in telemetry

* chore: remove sync-preview job from sync-from-public workflow (#1416)

* ci: disable telemetry in e2e and integ test workflows (#1421)

* ci: disable telemetry in e2e and integ test workflows

* fix: avoid test reliance on env var

* fix: remove test that is no longer testable

* feat: wire telemetry for validate command (#1423)

* refactor(telemetry): rename AgentType to AgentSource to remove ambiguity (#1422)

---------

Co-authored-by: Jesse Turner <57651174+jesseturner21@users.noreply.github.com>
Co-authored-by: agentcore-devx-automation[bot] <282717993+agentcore-devx-automation[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Gitika <53349492+notgitika@users.noreply.github.com>
Co-authored-by: Hweinstock <42325418+Hweinstock@users.noreply.github.com>
)

* feat: add CMK support for batch evaluation and recommendation APIs

Thread `kmsKeyArn` through the CLI for both `run batch-evaluation` and
`run recommendation` commands, enabling customers to encrypt evaluation
results with their own KMS key.

- CLI: adds `--kms-key <arn>` flag to both commands
- API layer: sends kmsKeyArn in StartBatchEvaluation request, maps it
  from GetBatchEvaluation response
- TUI: adds optional KMS Key ARN step to batch eval and recommendation
  wizards (press Enter to skip)

* fix: add KMS ARN validation and unit tests for batch eval API

- Add customValidation for KMS key ARN on TextInput in both
  RunBatchEvalFlow and RecommendationScreen (uses existing
  isValidKmsKeyArn from schema)
- Add comprehensive unit tests for agentcore-batch-evaluation API
  covering all endpoints and kmsKeyArn input/output
- Add kmsKeyArn inclusion/omission tests to recommendation API tests

Addresses PR review comments from jariy17.

* test: add isValidKmsKeyArn validation tests

Cover commercial, GovCloud, and China partition ARNs along with
rejection cases (wrong service, alias, bad account ID, invalid UUID).
Pulls 58 commits from public/main onto private staging main.
Includes the public-side updates since the prior sync (May 27):
- payments primitive promotion
- e2e harness updates and Gemini test enablement
- security-review workflow hardening (#1482)
- agent inspector model config plumbing (#1516)
- agent-inspector / harness e2e wire-up

Conflicts (if any) resolved in favor of staging's CI infrastructure
where the file shape diverged (sync-from-public.yml uses GITHUB_TOKEN
on staging vs an app token on public; e2e workflow scaffolding
preserves staging-only steps).

# Conflicts:
#	.github/workflows/e2e-tests.yml
#	.github/workflows/pr-security-review.yml
#	.github/workflows/release-main-and-preview.yml
#	.github/workflows/slack-issue-notification.yml
#	.github/workflows/sync-from-public.yml
#	CHANGELOG.md
#	package.json
#	preview-version.json
#	src/cli/commands/validate/command.tsx
#	src/cli/telemetry/schemas/command-run.ts
#	src/cli/tui/screens/create/useCreateFlow.ts
#	src/lib/errors/types.ts
Same fix as the merge to feat/summit_release in #291. main's copy of
the workflow lives at pr-security-review.yml (not pr-security-reviewer.yml
like the summit branch); both branches need the inlined-prompt fix
since each runs the workflow definition from its own base on every PR.

Root cause: anthropics/claude-code-action@v1's restoreConfigFromBase()
runs `git fetch origin <base> --depth=1` on startup, which shallows the
runner's clone. The bundled /security-review skill's first action is
`git diff origin/HEAD...`, which fails with "no merge base" on a
shallow clone — so the SDK exits clean with num_turns=0 and the
workflow had no choice but to fail.

Replace the bundled skill with an inline prompt: compute the diff to
/tmp/pr.diff before claude-code-action runs, then have the model read
that file plus the working tree (Read/Grep/Glob/Task) and post findings
via the inline-comment MCP tool. Restrict the trigger to PRs targeting
main and feat/summit_release.
Previous version wrote the prompt to a file and passed `prompt_file:`
to anthropics/claude-code-action@v1, but the action only accepts
`prompt:` (a string) — `prompt_file` is silently ignored, so the
action sees no prompt, fails its internal trigger detection
("No trigger found"), and skips before the model is invoked.

Build the prompt in a step (existing), then load it into GITHUB_ENV
as PROMPT_BODY, then pass `prompt: ${{ env.PROMPT_BODY }}` to the
action. Standard multi-line env-var idiom with a randomized heredoc
sentinel.
Previously, pressing Enter with an empty value in SecretInput would
call onCancel (which navigates back) even when customValidation
rejected empty values. Now validation runs first, showing the error
message instead of silently going back.

Fixes #1618
@notgitika notgitika requested a review from a team June 29, 2026 23:01
@github-actions github-actions Bot added size/xl PR size: XL agentcore-harness-reviewing AgentCore Harness review in progress labels Jun 29, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-0.19.0.tgz

How to install

gh release download pr-1667-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.19.0.tgz

@agentcore-cli-automation agentcore-cli-automation left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the empty-Enter regression — the core change to validateValue() and the onSubmit handler in SecretInput.tsx looks correct, and the new test covers the bug well.

A few things to address before this can merge:

  1. The branch is severely out of date. GitHub reports 497 changed files / +41,833 / -21,925, and a git diff origin/main HEAD for this branch shows 272 files changed (+3,480 / -13,701). The branch was forked from main at ef1e0667 and is missing ~57 commits, so as-is this PR would revert a large amount of unrelated work (workflows, e2e/integ tests, assets, schemas, AWS clients, etc.). GitHub also marks it as mergeable: false. Please rebase onto current main (or open a fresh PR off main) so the diff only contains the two files you actually intended to change.

  2. Unrelated removal of stripAnsi in the placeholder test is likely a regression — see the inline comment.

  3. onSkip is silently unreachable when an empty-rejecting customValidation is also provided — minor, see the inline comment.

No telemetry concerns and no excessive mocking in the test — those look fine.

@@ -35,7 +34,7 @@ describe('SecretInput', () => {
<SecretInput prompt="Key" placeholder="sk-..." onSubmit={vi.fn()} onCancel={vi.fn()} />
);

expect(stripAnsi(lastFrame()!)).toContain('sk-...');
expect(lastFrame()).toContain('sk-...');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (removing stripAnsi and the import on line 4) is unrelated to the bug being fixed, and it will almost certainly break this test.

When placeholder is set, the empty-value branch in SecretInput renders the first character through <Cursor> (which wraps it in backgroundColor="white" color="black" ANSI codes) and placeholder.slice(1) separately through <Text dimColor> (which wraps it in dim ANSI codes). So the raw frame for "sk-..." ends up roughly as \x1b[47m\x1b[30ms\x1b[39m\x1b[49m\x1b[2mk-...\x1b[22m — the s and the k-... are separated by escape sequences, so lastFrame().toContain('sk-...') no longer matches.

The same pattern is visible elsewhere in this file (e.g. expect(lastFrame()).toContain('PENAI_API_KEY') on what is otherwise an OPENAI_API_KEY placeholder — the O is missing precisely because it goes through Cursor). Please restore the stripAnsi wrapper (and the import) so the test continues to pass, or drop this hunk entirely since it isn't part of the fix.

const validationError = validateValue(trimmed, schema, customValidation);
if (validationError) {
setShowError(true);
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When onSkip is provided and customValidation rejects empty values, this branch makes onSkip unreachable: an empty Enter will always hit the setShowError(true); return; path above. None of the current call sites combine both, but the prop docs still advertise "Leave empty to skip", so this is a footgun for future callers.

A couple of options:

  • Document the constraint on the customValidation JSDoc (e.g. "if onSkip is provided, customValidation must accept empty input, otherwise skip will be unreachable").
  • Or skip the validation step when onSkip is set, so that empty Enter always means "skip" regardless of customValidation — matching the existing prop semantics more closely.
  • Or only run customValidation on empty input when onSkip is not provided (i.e. move the validation call inside the else branch).

Up to you which one you prefer.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 29, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 29, 2026
@notgitika notgitika closed this Jun 29, 2026
@notgitika notgitika deleted the fix/secret-input-empty-validation branch June 29, 2026 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xl PR size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add credential TUI flow simply goes back to the previous step if there is no input at the API key step

8 participants