From 04b8141e7efe5169ab5003f3b750d7f4a8541e4c Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 13:20:42 -0400 Subject: [PATCH 1/7] feat: add agentcore feedback command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new top-level `agentcore feedback` command that lets users send feedback (and an optional screenshot) directly from the CLI: agentcore feedback "your message" [--screenshot path] [--json] agentcore feedback # multi-step wizard Both modes display the AWS Customer Agreement and require interactive y/N consent before submitting. Bare Enter defaults to No, and non-TTY stdin is refused so the consent decision is always explicit. The wizard walks through Message → optional Screenshot → Consent → Submit, with Esc stepping back and message text preserved across declines/retries. Screenshots are validated locally (PNG/JPG/JPEG, ≤100MB) before any network call, then uploaded via presigned URL with SHA-256 checksum headers. Submissions are tagged with cli-version, os, node-version, and cli-mode metadata to help triage. The CLI does not attach AWS account IDs, credentials, project names, or telemetry IDs. Includes README + docs/commands.md updates and a new docs/feedback.md covering syntax, consent text, and what not to include. --- README.md | 26 ++- docs/commands.md | 19 +++ docs/feedback.md | 55 ++++++ src/cli/cli.ts | 2 + .../feedback/__tests__/command.test.ts | 133 +++++++++++++++ .../feedback/__tests__/consent-prompt.test.ts | 52 ++++++ src/cli/commands/feedback/action.ts | 29 ++++ src/cli/commands/feedback/command.tsx | 93 +++++++++++ src/cli/commands/feedback/consent-prompt.ts | 39 +++++ src/cli/commands/feedback/index.ts | 1 + src/cli/commands/feedback/types.ts | 4 + .../feedback/__tests__/build-payload.test.ts | 53 ++++++ .../__tests__/submit-feedback.test.ts | 158 ++++++++++++++++++ .../operations/feedback/aperture-client.ts | 107 ++++++++++++ src/cli/operations/feedback/build-payload.ts | 84 ++++++++++ src/cli/operations/feedback/constants.ts | 27 +++ src/cli/operations/feedback/index.ts | 4 + .../operations/feedback/submit-feedback.ts | 121 ++++++++++++++ src/cli/operations/feedback/types.ts | 72 ++++++++ src/cli/tui/copy.ts | 1 + .../tui/screens/feedback/FeedbackScreen.tsx | 156 +++++++++++++++++ .../__tests__/useFeedbackFlow.test.tsx | 145 ++++++++++++++++ src/cli/tui/screens/feedback/index.ts | 3 + .../tui/screens/feedback/useFeedbackFlow.ts | 121 ++++++++++++++ 24 files changed, 1498 insertions(+), 7 deletions(-) create mode 100644 docs/feedback.md create mode 100644 src/cli/commands/feedback/__tests__/command.test.ts create mode 100644 src/cli/commands/feedback/__tests__/consent-prompt.test.ts create mode 100644 src/cli/commands/feedback/action.ts create mode 100644 src/cli/commands/feedback/command.tsx create mode 100644 src/cli/commands/feedback/consent-prompt.ts create mode 100644 src/cli/commands/feedback/index.ts create mode 100644 src/cli/commands/feedback/types.ts create mode 100644 src/cli/operations/feedback/__tests__/build-payload.test.ts create mode 100644 src/cli/operations/feedback/__tests__/submit-feedback.test.ts create mode 100644 src/cli/operations/feedback/aperture-client.ts create mode 100644 src/cli/operations/feedback/build-payload.ts create mode 100644 src/cli/operations/feedback/constants.ts create mode 100644 src/cli/operations/feedback/index.ts create mode 100644 src/cli/operations/feedback/submit-feedback.ts create mode 100644 src/cli/operations/feedback/types.ts create mode 100644 src/cli/tui/screens/feedback/FeedbackScreen.tsx create mode 100644 src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx create mode 100644 src/cli/tui/screens/feedback/index.ts create mode 100644 src/cli/tui/screens/feedback/useFeedbackFlow.ts diff --git a/README.md b/README.md index 42aeda5ec..ab0fcc944 100644 --- a/README.md +++ b/README.md @@ -135,12 +135,13 @@ agentcore invoke ### Utilities -| Command | Description | -| -------------- | ----------------------------------------- | -| `validate` | Validate configuration files | -| `package` | Package agent artifacts without deploying | -| `fetch access` | Fetch access info for deployed resources | -| `update` | Check for and install CLI updates | +| Command | Description | +| -------------- | ----------------------------------------------------- | +| `validate` | Validate configuration files | +| `package` | Package agent artifacts without deploying | +| `fetch access` | Fetch access info for deployed resources | +| `feedback` | Send feedback to the AgentCore team (with screenshot) | +| `update` | Check for and install CLI updates | ## Project Structure @@ -191,10 +192,21 @@ Projects use JSON schema files in the `agentcore/` directory: - [Gateway](docs/gateway.md) - Gateway setup, targets, and authentication - [Memory](docs/memory.md) - Memory strategies and sharing - [Local Development](docs/local-development.md) - Dev server and debugging +- [Feedback](docs/feedback.md) - Submit feedback from your terminal ## Feedback & Issues -Found a bug or have a feature request? [Open an issue](https://github.com/aws/agentcore-cli/issues/new) on GitHub. +Have a quick comment or suggestion? Send it from your terminal: + +```bash +agentcore feedback "your message" --screenshot path/to/screenshot.png +``` + +The CLI will display the AWS Customer Agreement and prompt for consent before submitting. See +[docs/feedback.md](docs/feedback.md) for usage details. + +For bugs, regressions, or feature requests that need discussion, +[open an issue](https://github.com/aws/agentcore-cli/issues/new) on GitHub instead. ## Security diff --git a/docs/commands.md b/docs/commands.md index 6d22b6bf0..8922102a6 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -955,6 +955,25 @@ agentcore package -d ./my-project | `-d, --directory ` | Project directory | | `-r, --runtime ` | Package specific runtime | +### feedback + +Send feedback about the AgentCore CLI. The CLI displays the AWS Customer Agreement and prompts for consent before +submitting; consent must be confirmed in an interactive terminal. + +```bash +agentcore feedback "the dev server is slow on Linux" +agentcore feedback "broken icon" --screenshot ~/Desktop/bug.png +agentcore feedback "automation works" --json +agentcore feedback # launches the wizard +``` + +| Flag | Description | +| --------------------- | ------------------------------------------------------------ | +| `--screenshot ` | Path to a `.png`, `.jpg`, or `.jpeg` file (max 100MB) | +| `--json` | Print result as JSON (`{success, id, timestamp, reference}`) | + +See [docs/feedback.md](feedback.md) for usage details. + ### update Check for and install CLI updates. Equivalent to `agentcore update cli`. diff --git a/docs/feedback.md b/docs/feedback.md new file mode 100644 index 000000000..9a5a31626 --- /dev/null +++ b/docs/feedback.md @@ -0,0 +1,55 @@ +# Feedback + +Send feedback about the AgentCore CLI directly from your terminal. + +## When to use + +| Use `agentcore feedback` | Use [GitHub Issues](https://github.com/aws/agentcore-cli/issues) instead | +| -------------------------------------- | ------------------------------------------------------------------------ | +| Quick comments, suggestions, papercuts | Bugs that need a conversation or repro steps | +| First impressions, onboarding friction | Regressions you want to track | +| Sharing a screenshot of confusing UX | Feature requests you want to discuss publicly | + +## Syntax + +```bash +# One-shot +agentcore feedback "your message" [--screenshot path/to/file.png] [--json] + +# Multi-step wizard +agentcore feedback +``` + +The wizard walks through: message → optional screenshot → consent → submit. Press `Esc` to step back one phase. + +## Screenshots + +- Allowed types: `.png`, `.jpg`, `.jpeg` +- Maximum size: 100 MB + +## Consent + +Every submission requires interactive consent. The CLI displays: + +> All feedback submissions, including any uploaded text and images, are subject to the AWS Customer Agreement +> (https://aws.amazon.com/agreement/). By submitting feedback, you agree that your submissions constitute "Suggestions" +> as defined in the AWS Customer Agreement. + +Bare `Enter` defaults to **No**. The command refuses to submit when stdin is not a TTY (e.g. piped input, CI). There is +no flag that bypasses the prompt. + +## What not to include + +Do not paste credentials, secrets, account IDs, or customer data into the message, and do not attach screenshots that +show those values. + +## Output + +Plain mode prints a confirmation line on success. JSON mode (`--json`) prints a single line: + +```json +{ "success": true, "id": "", "timestamp": "", "reference": "" } +``` + +On failure the CLI exits with code 1 and prints a human-readable error, or `{"success": false, "error": "..."}` when +`--json` is set. diff --git a/src/cli/cli.ts b/src/cli/cli.ts index e40732f52..5517afb9c 100644 --- a/src/cli/cli.ts +++ b/src/cli/cli.ts @@ -7,6 +7,7 @@ import { registerCreate } from './commands/create'; import { registerDeploy } from './commands/deploy'; import { registerDev } from './commands/dev'; import { registerEval } from './commands/eval'; +import { registerFeedback } from './commands/feedback'; import { registerFetch } from './commands/fetch'; import { registerHelp } from './commands/help'; import { registerImport } from './commands/import'; @@ -180,6 +181,7 @@ export function registerCommands(program: Command) { registerDeploy(program); registerCreate(program); registerEval(program); + registerFeedback(program); registerFetch(program); registerHelp(program); registerImport(program); diff --git a/src/cli/commands/feedback/__tests__/command.test.ts b/src/cli/commands/feedback/__tests__/command.test.ts new file mode 100644 index 000000000..e0f58eb19 --- /dev/null +++ b/src/cli/commands/feedback/__tests__/command.test.ts @@ -0,0 +1,133 @@ +import { registerFeedback } from '../command'; +import { Command } from '@commander-js/extra-typings'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockHandleFeedback = vi.fn(); +const mockRender = vi.fn(); +const mockRequireTTY = vi.fn(); + +vi.mock('../action', () => ({ + handleFeedback: (...args: unknown[]) => mockHandleFeedback(...args), +})); + +vi.mock('../../../tui/guards/tty', () => ({ + requireTTY: () => mockRequireTTY(), +})); + +vi.mock('../../../tui/screens/feedback', () => ({ + FeedbackScreen: () => null, +})); + +vi.mock('ink', () => ({ + render: (...args: unknown[]) => { + mockRender(...args); + return { clear: vi.fn(), unmount: vi.fn() }; + }, + Text: 'Text', + Box: 'Box', +})); + +const submittedOutcome = { + kind: 'submitted' as const, + result: { id: 'sub-1', timestamp: '2026-05-13T18:00:00Z', reference: 'S3' }, +}; + +describe('registerFeedback', () => { + let program: Command; + let mockExit: ReturnType; + let mockLog: ReturnType; + let mockError: ReturnType; + + beforeEach(() => { + program = new Command(); + program.exitOverride(); + registerFeedback(program); + + mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit'); + }); + mockLog = vi.spyOn(console, 'log').mockImplementation(() => undefined); + mockError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + }); + + afterEach(() => { + mockExit.mockRestore(); + mockLog.mockRestore(); + mockError.mockRestore(); + vi.clearAllMocks(); + }); + + it('registers a top-level feedback command', () => { + const cmd = program.commands.find(c => c.name() === 'feedback'); + expect(cmd).toBeDefined(); + }); + + it('emits success JSON when --json is supplied with a message', async () => { + mockHandleFeedback.mockResolvedValue(submittedOutcome); + + await program.parseAsync(['feedback', 'looks good', '--json'], { from: 'user' }); + + expect(mockHandleFeedback).toHaveBeenCalledWith('looks good', expect.objectContaining({ json: true })); + expect(mockLog).toHaveBeenCalledTimes(1); + const output = JSON.parse(mockLog.mock.calls[0]?.[0] as string); + expect(output).toEqual({ + success: true, + id: 'sub-1', + timestamp: '2026-05-13T18:00:00Z', + reference: 'S3', + }); + }); + + it('reports a TTY error when consent cannot be confirmed and exits 1', async () => { + mockHandleFeedback.mockResolvedValue({ kind: 'no-tty' }); + + await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit'); + + expect(mockError).toHaveBeenCalledWith(expect.stringContaining('consent must be confirmed interactively')); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it('prints a friendly cancellation message when the user declines consent', async () => { + mockHandleFeedback.mockResolvedValue({ kind: 'declined' }); + + await program.parseAsync(['feedback', 'msg'], { from: 'user' }); + + expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('Feedback cancelled.')); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it('reports submission errors with exit 1 in plain mode', async () => { + mockHandleFeedback.mockResolvedValue({ kind: 'error', error: 'HTTP 500' }); + + await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit'); + + expect(mockExit).toHaveBeenCalledWith(1); + expect(mockRender).toHaveBeenCalled(); + }); + + it('emits a JSON error envelope on submission failure when --json is set', async () => { + mockHandleFeedback.mockResolvedValue({ kind: 'error', error: 'HTTP 500' }); + + await expect(program.parseAsync(['feedback', 'msg', '--json'], { from: 'user' })).rejects.toThrow('process.exit'); + + const output = JSON.parse(mockLog.mock.calls[0]?.[0] as string); + expect(output).toEqual({ success: false, error: 'HTTP 500' }); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it('refuses --json when no message is supplied', async () => { + await expect(program.parseAsync(['feedback', '--json'], { from: 'user' })).rejects.toThrow('process.exit'); + + expect(mockError).toHaveBeenCalledWith(expect.stringContaining('--json requires a feedback message')); + expect(mockExit).toHaveBeenCalledWith(1); + expect(mockHandleFeedback).not.toHaveBeenCalled(); + }); + + it('hands off to the TUI when no message argument is provided', async () => { + await program.parseAsync(['feedback'], { from: 'user' }); + + expect(mockRequireTTY).toHaveBeenCalled(); + expect(mockRender).toHaveBeenCalled(); + expect(mockHandleFeedback).not.toHaveBeenCalled(); + }); +}); diff --git a/src/cli/commands/feedback/__tests__/consent-prompt.test.ts b/src/cli/commands/feedback/__tests__/consent-prompt.test.ts new file mode 100644 index 000000000..b10966573 --- /dev/null +++ b/src/cli/commands/feedback/__tests__/consent-prompt.test.ts @@ -0,0 +1,52 @@ +import { promptForConsent } from '../consent-prompt'; +import { PassThrough } from 'node:stream'; +import { describe, expect, it } from 'vitest'; + +function makeTtyStdin(input: string): NodeJS.ReadableStream & { isTTY?: boolean } { + const stream = new PassThrough() as PassThrough & { isTTY?: boolean }; + stream.isTTY = true; + stream.end(input); + return stream; +} + +describe('promptForConsent', () => { + it('returns no-tty when stdin is not a TTY', async () => { + const stdin = new PassThrough() as PassThrough & { isTTY?: boolean }; + stdin.isTTY = false; + const stdout = new PassThrough(); + + const result = await promptForConsent({ stdin, stdout }); + + expect(result).toEqual({ accepted: false, reason: 'no-tty' }); + }); + + it('accepts when the user types y', async () => { + const stdin = makeTtyStdin('y\n'); + const stdout = new PassThrough(); + stdout.resume(); + + const result = await promptForConsent({ stdin, stdout }); + + expect(result.accepted).toBe(true); + }); + + it('declines on bare Enter (defaults to No)', async () => { + const stdin = makeTtyStdin('\n'); + const stdout = new PassThrough(); + stdout.resume(); + + const result = await promptForConsent({ stdin, stdout }); + + expect(result).toEqual({ accepted: false, reason: 'declined' }); + }); + + it('declines when the user types n', async () => { + const stdin = makeTtyStdin('n\n'); + const stdout = new PassThrough(); + stdout.resume(); + + const result = await promptForConsent({ stdin, stdout }); + + expect(result).toEqual({ accepted: false, reason: 'declined' }); + }); +}); diff --git a/src/cli/commands/feedback/action.ts b/src/cli/commands/feedback/action.ts new file mode 100644 index 000000000..feec7c9db --- /dev/null +++ b/src/cli/commands/feedback/action.ts @@ -0,0 +1,29 @@ +import { submitFeedback } from '../../operations/feedback'; +import type { FeedbackSubmissionResult } from '../../operations/feedback'; +import { promptForConsent } from './consent-prompt'; +import type { FeedbackOptions } from './types'; + +export type FeedbackOutcome = + | { kind: 'submitted'; result: FeedbackSubmissionResult } + | { kind: 'declined' } + | { kind: 'no-tty' } + | { kind: 'error'; error: string }; + +export async function handleFeedback(message: string, options: FeedbackOptions): Promise { + const consent = await promptForConsent(); + if (!consent.accepted) { + return consent.reason === 'no-tty' ? { kind: 'no-tty' } : { kind: 'declined' }; + } + + try { + const result = await submitFeedback({ + message, + screenshot: options.screenshot ? { path: options.screenshot } : undefined, + mode: 'cli', + }); + return { kind: 'submitted', result }; + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + return { kind: 'error', error }; + } +} diff --git a/src/cli/commands/feedback/command.tsx b/src/cli/commands/feedback/command.tsx new file mode 100644 index 000000000..bd5d43273 --- /dev/null +++ b/src/cli/commands/feedback/command.tsx @@ -0,0 +1,93 @@ +import { getErrorMessage } from '../../errors'; +import { COMMAND_DESCRIPTIONS } from '../../tui/copy'; +import { requireTTY } from '../../tui/guards/tty'; +import { FeedbackScreen } from '../../tui/screens/feedback'; +import { handleFeedback } from './action'; +import type { FeedbackOptions } from './types'; +import type { Command } from '@commander-js/extra-typings'; +import { Text, render } from 'ink'; + +export const registerFeedback = (program: Command) => { + return program + .command('feedback') + .description(COMMAND_DESCRIPTIONS.feedback) + .argument('[message]', 'Feedback message [non-interactive]') + .option('--screenshot ', 'Path to a PNG or JPG screenshot (max 100MB) [non-interactive]') + .option('--json', 'Output result as JSON [non-interactive]') + .action(async (message: string | undefined, cliOptions: Record) => { + const options = cliOptions as FeedbackOptions; + + if (message === undefined) { + if (options.json) { + console.error('Error: --json requires a feedback message argument.'); + process.exit(1); + return; + } + requireTTY(); + const { clear, unmount } = render( + { + clear(); + unmount(); + }} + /> + ); + return; + } + + let outcome; + try { + outcome = await handleFeedback(message, options); + } catch (error) { + const errMessage = getErrorMessage(error); + if (options.json) { + console.log(JSON.stringify({ success: false, error: errMessage })); + } else { + render(Error: {errMessage}); + } + process.exit(1); + return; + } + + if (outcome.kind === 'no-tty') { + const errorText = 'Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.'; + if (options.json) { + console.log(JSON.stringify({ success: false, error: errorText })); + } else { + console.error(errorText); + } + process.exit(1); + return; + } + + if (outcome.kind === 'declined') { + if (options.json) { + console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' })); + } else { + console.log('Feedback cancelled. Nothing was submitted.'); + } + return; + } + + if (outcome.kind === 'error') { + if (options.json) { + console.log(JSON.stringify({ success: false, error: outcome.error })); + } else { + render(Error: {outcome.error}); + } + process.exit(1); + return; + } + + const result = outcome.result; + if (options.json) { + console.log( + JSON.stringify({ success: true, id: result.id, timestamp: result.timestamp, reference: result.reference }) + ); + return; + } + + render(Thank you. Your feedback has been submitted (id: {result.id}).); + }); +}; diff --git a/src/cli/commands/feedback/consent-prompt.ts b/src/cli/commands/feedback/consent-prompt.ts new file mode 100644 index 000000000..e0d8b8df6 --- /dev/null +++ b/src/cli/commands/feedback/consent-prompt.ts @@ -0,0 +1,39 @@ +import { CONSENT_TEXT } from '../../operations/feedback/constants'; +import * as readline from 'node:readline/promises'; + +const PROMPT_LINE = 'Submit feedback? [y/N] '; + +export interface ConsentPromptDeps { + stdin?: NodeJS.ReadableStream & { isTTY?: boolean }; + stdout?: NodeJS.WritableStream; +} + +export interface ConsentPromptResult { + accepted: boolean; + reason?: 'declined' | 'no-tty'; +} + +/** + * Renders the AWS Customer Agreement consent text and reads y/N from the + * interactive terminal. Defaults to "no" on bare Enter; refuses to read + * non-TTY stdin so the consent decision is always explicit. + */ +export async function promptForConsent(deps: ConsentPromptDeps = {}): Promise { + const stdin = deps.stdin ?? process.stdin; + const stdout = deps.stdout ?? process.stdout; + + if (!stdin.isTTY) { + return { accepted: false, reason: 'no-tty' }; + } + + stdout.write(`\n${CONSENT_TEXT}\n\n`); + + const rl = readline.createInterface({ input: stdin, output: stdout }); + try { + const answer = (await rl.question(PROMPT_LINE)).trim().toLowerCase(); + const accepted = answer === 'y' || answer === 'yes'; + return { accepted, reason: accepted ? undefined : 'declined' }; + } finally { + rl.close(); + } +} diff --git a/src/cli/commands/feedback/index.ts b/src/cli/commands/feedback/index.ts new file mode 100644 index 000000000..90cfcd492 --- /dev/null +++ b/src/cli/commands/feedback/index.ts @@ -0,0 +1 @@ +export { registerFeedback } from './command'; diff --git a/src/cli/commands/feedback/types.ts b/src/cli/commands/feedback/types.ts new file mode 100644 index 000000000..dc9969407 --- /dev/null +++ b/src/cli/commands/feedback/types.ts @@ -0,0 +1,4 @@ +export interface FeedbackOptions { + screenshot?: string; + json?: boolean; +} diff --git a/src/cli/operations/feedback/__tests__/build-payload.test.ts b/src/cli/operations/feedback/__tests__/build-payload.test.ts new file mode 100644 index 000000000..902010fae --- /dev/null +++ b/src/cli/operations/feedback/__tests__/build-payload.test.ts @@ -0,0 +1,53 @@ +import { buildFeedbackPayload } from '../build-payload'; +import { describe, expect, it } from 'vitest'; + +describe('buildFeedbackPayload', () => { + it('emits the message-only payload when no screenshot is supplied', () => { + const payload = buildFeedbackPayload({ + message: 'CLI ate my homework', + cliVersion: '0.1.0-alpha.42', + osDescriptor: 'darwin 25.3.0', + nodeVersion: 'v20.20.0', + mode: 'cli', + }); + + expect(payload.category).toBe('AgentCore'); + expect(payload.name).toBe('CLI'); + expect(payload.version).toBe('0.1.0'); + expect(payload.locale).toBe('en_US'); + expect(payload.reference).toBe('agentcore-cli'); + expect(payload.location).toContain('agentcore-cli@0.1.0-alpha.42'); + expect(payload.customerResponses).toHaveLength(1); + expect(payload.customerResponses[0]).toMatchObject({ + question: 'What feedback do you have for the AgentCore CLI', + pii: false, + response: { responseType: 'textArea', responseValue: 'CLI ate my homework' }, + }); + expect(payload.metadataList).toEqual([ + { key: 'cli-version', value: '0.1.0-alpha.42' }, + { key: 'os', value: 'darwin 25.3.0' }, + { key: 'node-version', value: 'v20.20.0' }, + { key: 'cli-mode', value: 'cli' }, + ]); + }); + + it('appends a fileUpload response when a screenshot reference is supplied', () => { + const payload = buildFeedbackPayload({ + message: 'broken icon', + screenshotReference: 'https://s3.example.com/bucket/key.png', + cliVersion: '0.1.0', + osDescriptor: 'linux 6.0 node v20', + }); + + expect(payload.customerResponses).toHaveLength(2); + const attachment = payload.customerResponses[1]; + expect(attachment).toMatchObject({ + question: 'Attachments', + pii: true, + response: { + responseType: 'fileUpload', + responseValue: ['https://s3.example.com/bucket/key.png'], + }, + }); + }); +}); diff --git a/src/cli/operations/feedback/__tests__/submit-feedback.test.ts b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts new file mode 100644 index 000000000..25f08c9a1 --- /dev/null +++ b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts @@ -0,0 +1,158 @@ +import { FeedbackValidationError, submitFeedback } from '../submit-feedback'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const successResponse = { + reference: 'S3', + id: 'submission-123', + timestamp: '2026-05-13T18:00:00Z', +}; + +function jsonResponse(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +function emptyOk(status = 200): Response { + return new Response('', { status }); +} + +type FetchCall = [string, { method: string; headers: Record; body: string | Uint8Array }]; + +describe('submitFeedback', () => { + let fetchMock: ReturnType; + + function callAt(index: number): FetchCall { + const all = fetchMock.mock.calls as unknown as FetchCall[]; + const call = all[index]; + if (!call) throw new Error(`Expected fetch call ${index}, only saw ${all.length}`); + return call; + } + + beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + it('rejects empty messages without making any network call', async () => { + await expect(submitFeedback({ message: ' ' })).rejects.toBeInstanceOf(FeedbackValidationError); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('submits a message-only form to the ingestion endpoint', async () => { + fetchMock.mockResolvedValueOnce(jsonResponse(successResponse)); + + const result = await submitFeedback({ message: 'looks great' }); + + expect(result).toEqual({ + id: 'submission-123', + timestamp: '2026-05-13T18:00:00Z', + reference: 'S3', + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + + const [url, init] = callAt(0); + expect(url).toBe('https://ingestion.aperture-public-api.feedback.console.aws.dev/form'); + expect(init.method).toBe('POST'); + const body = JSON.parse(init.body as string); + expect(body.customerResponses).toHaveLength(1); + expect(body.customerResponses[0].response.responseValue).toBe('looks great'); + }); + + it('uploads a screenshot via presigned URL before submitting the form', async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'feedback-test-')); + const screenshotPath = path.join(tmp, 'shot.png'); + await fs.writeFile(screenshotPath, Buffer.from([0x89, 0x50, 0x4e, 0x47])); + + fetchMock + .mockResolvedValueOnce(new Response('https://s3.example/key?sig=x', { status: 200 })) + .mockResolvedValueOnce(emptyOk(200)) + .mockResolvedValueOnce(jsonResponse(successResponse)); + + const result = await submitFeedback({ + message: 'see screenshot', + screenshot: { path: screenshotPath }, + }); + + expect(result.id).toBe('submission-123'); + expect(fetchMock).toHaveBeenCalledTimes(3); + + const [presignUrl, presignInit] = callAt(0); + expect(presignUrl).toBe('https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl'); + const presignBody = JSON.parse(presignInit.body as string); + expect(presignBody).toMatchObject({ + category: 'AgentCore', + name: 'CLI', + version: '0.1.0', + fileName: 'shot.png', + fileSize: 4, + }); + expect(typeof presignBody.uploadFileSHA256).toBe('string'); + // base64 sha256 length for any input is 44 chars + expect(presignBody.uploadFileSHA256).toHaveLength(44); + + const [uploadUrl, uploadInit] = callAt(1); + expect(uploadUrl).toBe('https://s3.example/key?sig=x'); + expect(uploadInit.method).toBe('PUT'); + expect(uploadInit.headers['content-type']).toBe('image/png'); + expect(uploadInit.headers['x-amz-checksum-algorithm']).toBe('SHA256'); + expect(uploadInit.headers['x-amz-checksum-sha256']).toBe(presignBody.uploadFileSHA256); + expect(uploadInit.headers['x-amz-tagging']).toBe('scanstatus=NOT_SCANNED'); + + const [, submitInit] = callAt(2); + const submitBody = JSON.parse(submitInit.body as string); + expect(submitBody.customerResponses).toHaveLength(2); + expect(submitBody.customerResponses[1]).toMatchObject({ + question: 'Attachments', + pii: true, + response: { responseType: 'fileUpload' }, + }); + // Object key shape: us-east-1/AgentCore/CLI/0.1.0/DDMMYYYY/.png, wrapped in an array + const responseValue = submitBody.customerResponses[1].response.responseValue; + expect(Array.isArray(responseValue)).toBe(true); + expect(responseValue[0]).toMatch(/^us-east-1\/AgentCore\/CLI\/0\.1\.0\/\d{8}\/[0-9a-f-]{36}\.png$/); + + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it('rejects screenshots with disallowed extensions before any network call', async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'feedback-test-')); + const gifPath = path.join(tmp, 'shot.gif'); + await fs.writeFile(gifPath, 'data'); + + await expect(submitFeedback({ message: 'msg', screenshot: { path: gifPath } })).rejects.toBeInstanceOf( + FeedbackValidationError + ); + + expect(fetchMock).not.toHaveBeenCalled(); + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it('reports a clear error when the screenshot file is missing', async () => { + await expect(submitFeedback({ message: 'msg', screenshot: { path: '/no/such/file.png' } })).rejects.toThrow( + /Could not read screenshot/ + ); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it('maps Aperture 412 responses to a missing-headers error', async () => { + fetchMock.mockResolvedValueOnce(new Response('missing headers', { status: 412 })); + + await expect(submitFeedback({ message: 'msg' })).rejects.toThrow(/HTTP 412/); + }); + + it('maps Aperture 500 responses to a retryable error message', async () => { + fetchMock.mockResolvedValueOnce(new Response('boom', { status: 500 })); + + await expect(submitFeedback({ message: 'msg' })).rejects.toThrow(/HTTP 500/); + }); +}); diff --git a/src/cli/operations/feedback/aperture-client.ts b/src/cli/operations/feedback/aperture-client.ts new file mode 100644 index 000000000..8eb646be5 --- /dev/null +++ b/src/cli/operations/feedback/aperture-client.ts @@ -0,0 +1,107 @@ +import { APERTURE_INGESTION_URL, APERTURE_PRESIGNED_URL_ENDPOINT } from './constants'; +import type { ApertureFormPayload, AperturePresignedUrlRequest, ApertureSubmitResponse } from './types'; + +export class ApertureError extends Error { + status?: number; + body?: string; + + constructor(message: string, opts: { status?: number; body?: string } = {}) { + super(message); + this.name = 'ApertureError'; + this.status = opts.status; + this.body = opts.body; + } +} + +async function readBody(response: Response): Promise { + try { + return await response.text(); + } catch { + return ''; + } +} + +/** + * Fetches an S3 presigned URL for uploading a screenshot. Aperture returns the + * URL as a plain text body, not JSON. + */ +export async function fetchPresignedUrl(request: AperturePresignedUrlRequest, userAgent: string): Promise { + const response = await fetch(APERTURE_PRESIGNED_URL_ENDPOINT, { + method: 'POST', + headers: { 'content-type': 'application/json', 'user-agent': userAgent }, + body: JSON.stringify(request), + }); + + if (!response.ok) { + const body = await readBody(response); + throw new ApertureError(`Failed to fetch screenshot upload URL (HTTP ${response.status}).`, { + status: response.status, + body, + }); + } + + return (await response.text()).trim(); +} + +/** + * Uploads a file to the presigned S3 URL. Aperture's bucket policy requires + * the SHA-256 checksum headers and a tagging header that marks the upload as + * not yet AV-scanned. + */ +export async function uploadFileToS3( + presignedUrl: string, + fileBuffer: Uint8Array, + contentType: string, + base64Sha256: string, + userAgent: string +): Promise { + const response = await fetch(presignedUrl, { + method: 'PUT', + headers: { + 'content-type': contentType, + 'x-amz-checksum-algorithm': 'SHA256', + 'x-amz-checksum-sha256': base64Sha256, + 'x-amz-tagging': 'scanstatus=NOT_SCANNED', + 'user-agent': userAgent, + }, + body: fileBuffer, + }); + + if (!response.ok) { + const body = await readBody(response); + throw new ApertureError(`Failed to upload screenshot (HTTP ${response.status}).`, { + status: response.status, + body, + }); + } +} + +export async function submitForm(payload: ApertureFormPayload, userAgent: string): Promise { + const response = await fetch(APERTURE_INGESTION_URL, { + method: 'POST', + headers: { 'content-type': 'application/json', 'user-agent': userAgent }, + body: JSON.stringify(payload), + }); + + if (!response.ok) { + const body = await readBody(response); + throw new ApertureError(mapStatusToMessage(response.status, body), { status: response.status, body }); + } + + return (await response.json()) as ApertureSubmitResponse; +} + +function mapStatusToMessage(status: number, body: string): string { + switch (status) { + case 400: + return `Feedback service rejected the submission (HTTP 400). ${body || 'Form payload may be malformed.'}`; + case 412: + return 'Feedback service is missing required headers (HTTP 412).'; + case 417: + return 'Feedback service rejected the request content type (HTTP 417).'; + case 500: + return 'Feedback service returned an internal error (HTTP 500). Please try again later.'; + default: + return `Feedback service returned HTTP ${status}.`; + } +} diff --git a/src/cli/operations/feedback/build-payload.ts b/src/cli/operations/feedback/build-payload.ts new file mode 100644 index 000000000..0533a4764 --- /dev/null +++ b/src/cli/operations/feedback/build-payload.ts @@ -0,0 +1,84 @@ +import { PACKAGE_VERSION } from '../../constants'; +import { + APERTURE_FORM_CATEGORY, + APERTURE_FORM_NAME, + APERTURE_FORM_VERSION, + APERTURE_LOCALE, + FEEDBACK_ATTACHMENT_QUESTION, + FEEDBACK_MESSAGE_QUESTION, + FEEDBACK_REFERENCE, + METADATA_KEY_CLI_MODE, + METADATA_KEY_CLI_VERSION, + METADATA_KEY_NODE_VERSION, + METADATA_KEY_OS, +} from './constants'; +import type { ApertureCustomerResponse, ApertureFormPayload, ApertureMetadata, FeedbackMode } from './types'; +import * as os from 'node:os'; + +interface BuildPayloadInput { + message: string; + screenshotReference?: string; + mode?: FeedbackMode; + cliVersion?: string; + osDescriptor?: string; + nodeVersion?: string; +} + +export function buildOsDescriptor(): string { + return `${process.platform} ${os.release()}`; +} + +export function buildLocationDescriptor(cliVersion: string, mode: FeedbackMode): string { + return `agentcore-cli@${cliVersion} (${process.platform}; ${mode})`; +} + +export function buildUserAgent(cliVersion: string): string { + return `AgentCoreCLI/${cliVersion} (${process.platform} ${os.release()}; node/${process.version})`; +} + +export function buildFeedbackPayload(input: BuildPayloadInput): ApertureFormPayload { + const cliVersion = input.cliVersion ?? PACKAGE_VERSION; + const osDescriptor = input.osDescriptor ?? buildOsDescriptor(); + const nodeVersion = input.nodeVersion ?? process.version; + const mode: FeedbackMode = input.mode ?? 'cli'; + + const customerResponses: ApertureCustomerResponse[] = [ + { + question: FEEDBACK_MESSAGE_QUESTION, + pii: false, + response: { + responseType: 'textArea', + responseValue: input.message, + }, + }, + ]; + + if (input.screenshotReference) { + customerResponses.push({ + question: FEEDBACK_ATTACHMENT_QUESTION, + pii: true, + response: { + responseType: 'fileUpload', + responseValue: [input.screenshotReference], + }, + }); + } + + const metadataList: ApertureMetadata[] = [ + { key: METADATA_KEY_CLI_VERSION, value: cliVersion }, + { key: METADATA_KEY_OS, value: osDescriptor }, + { key: METADATA_KEY_NODE_VERSION, value: nodeVersion }, + { key: METADATA_KEY_CLI_MODE, value: mode }, + ]; + + return { + category: APERTURE_FORM_CATEGORY, + name: APERTURE_FORM_NAME, + version: APERTURE_FORM_VERSION, + locale: APERTURE_LOCALE, + reference: FEEDBACK_REFERENCE, + location: buildLocationDescriptor(cliVersion, mode), + customerResponses, + metadataList, + }; +} diff --git a/src/cli/operations/feedback/constants.ts b/src/cli/operations/feedback/constants.ts new file mode 100644 index 000000000..1890903be --- /dev/null +++ b/src/cli/operations/feedback/constants.ts @@ -0,0 +1,27 @@ +export const APERTURE_FORM_CATEGORY = 'AgentCore'; +export const APERTURE_FORM_NAME = 'CLI'; +export const APERTURE_FORM_VERSION = '0.1.0'; +export const APERTURE_LOCALE = 'en_US'; + +export const APERTURE_INGESTION_URL = 'https://ingestion.aperture-public-api.feedback.console.aws.dev/form'; +export const APERTURE_PRESIGNED_URL_ENDPOINT = + 'https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl'; +export const APERTURE_S3_REGION = 'us-east-1'; + +export const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; +export const ALLOWED_SCREENSHOT_EXTENSIONS = ['.png', '.jpg', '.jpeg'] as const; + +export const FEEDBACK_MESSAGE_QUESTION = 'What feedback do you have for the AgentCore CLI'; +export const FEEDBACK_ATTACHMENT_QUESTION = 'Attachments'; +export const METADATA_KEY_CLI_VERSION = 'cli-version'; +export const METADATA_KEY_OS = 'os'; +export const METADATA_KEY_NODE_VERSION = 'node-version'; +export const METADATA_KEY_CLI_MODE = 'cli-mode'; + +export const FEEDBACK_REFERENCE = 'agentcore-cli'; + +export const CONSENT_TEXT = + 'All feedback submissions, including any uploaded text and images, are subject ' + + 'to the AWS Customer Agreement (https://aws.amazon.com/agreement/). By submitting ' + + 'feedback, you agree that your submissions constitute "Suggestions" as defined ' + + 'in the AWS Customer Agreement.'; diff --git a/src/cli/operations/feedback/index.ts b/src/cli/operations/feedback/index.ts new file mode 100644 index 000000000..6563cf32a --- /dev/null +++ b/src/cli/operations/feedback/index.ts @@ -0,0 +1,4 @@ +export { submitFeedback, FeedbackValidationError } from './submit-feedback'; +export { ApertureError } from './aperture-client'; +export { CONSENT_TEXT } from './constants'; +export type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types'; diff --git a/src/cli/operations/feedback/submit-feedback.ts b/src/cli/operations/feedback/submit-feedback.ts new file mode 100644 index 000000000..92749a66a --- /dev/null +++ b/src/cli/operations/feedback/submit-feedback.ts @@ -0,0 +1,121 @@ +import { PACKAGE_VERSION } from '../../constants'; +import { fetchPresignedUrl, submitForm, uploadFileToS3 } from './aperture-client'; +import { buildFeedbackPayload, buildUserAgent } from './build-payload'; +import { + ALLOWED_SCREENSHOT_EXTENSIONS, + APERTURE_FORM_CATEGORY, + APERTURE_FORM_NAME, + APERTURE_FORM_VERSION, + APERTURE_S3_REGION, + MAX_SCREENSHOT_BYTES, +} from './constants'; +import type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types'; +import { createHash, randomUUID } from 'node:crypto'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; + +export class FeedbackValidationError extends Error { + constructor(message: string) { + super(message); + this.name = 'FeedbackValidationError'; + } +} + +function contentTypeForExtension(ext: string): string { + if (ext === '.png') return 'image/png'; + return 'image/jpeg'; +} + +function validateMessage(message: string): void { + const trimmed = message.trim(); + if (!trimmed) { + throw new FeedbackValidationError('Feedback message cannot be empty.'); + } + if (trimmed.length > 1000) { + throw new FeedbackValidationError('Feedback message must be 1000 characters or fewer.'); + } +} + +interface LoadedScreenshot { + buffer: Uint8Array; + fileName: string; + extension: string; + contentType: string; + sha256Base64: string; + size: number; +} + +async function loadAndValidateScreenshot(filePath: string): Promise { + const ext = path.extname(filePath).toLowerCase(); + if (!ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number])) { + throw new FeedbackValidationError(`Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(', ')}.`); + } + + let buffer: Buffer; + try { + buffer = await fs.readFile(filePath); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + throw new FeedbackValidationError(`Could not read screenshot at ${filePath}: ${reason}`); + } + + if (buffer.byteLength > MAX_SCREENSHOT_BYTES) { + const sizeMb = (buffer.byteLength / (1024 * 1024)).toFixed(1); + throw new FeedbackValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } + + return { + buffer: new Uint8Array(buffer), + fileName: path.basename(filePath), + extension: ext.replace(/^\./, ''), + contentType: contentTypeForExtension(ext), + sha256Base64: createHash('sha256').update(buffer).digest('base64'), + size: buffer.byteLength, + }; +} + +function buildAttachmentObjectKey(extension: string, now = new Date()): string { + const day = now.getUTCDate().toString().padStart(2, '0'); + const month = (now.getUTCMonth() + 1).toString().padStart(2, '0'); + const year = now.getUTCFullYear().toString(); + const datePart = `${day}${month}${year}`; + return `${APERTURE_S3_REGION}/${APERTURE_FORM_CATEGORY}/${APERTURE_FORM_NAME}/${APERTURE_FORM_VERSION}/${datePart}/${randomUUID()}.${extension}`; +} + +export async function submitFeedback(input: SubmitFeedbackInput): Promise { + validateMessage(input.message); + + const userAgent = buildUserAgent(PACKAGE_VERSION); + let screenshotReference: string | undefined; + + if (input.screenshot) { + const file = await loadAndValidateScreenshot(input.screenshot.path); + const presignedUrl = await fetchPresignedUrl( + { + category: APERTURE_FORM_CATEGORY, + name: APERTURE_FORM_NAME, + version: APERTURE_FORM_VERSION, + fileName: file.fileName, + fileSize: file.size, + uploadFileSHA256: file.sha256Base64, + }, + userAgent + ); + await uploadFileToS3(presignedUrl, file.buffer, file.contentType, file.sha256Base64, userAgent); + screenshotReference = buildAttachmentObjectKey(file.extension); + } + + const payload = buildFeedbackPayload({ + message: input.message.trim(), + screenshotReference, + mode: input.mode, + }); + + const response = await submitForm(payload, userAgent); + + return { + id: response.id, + timestamp: response.timestamp, + reference: response.reference, + }; +} diff --git a/src/cli/operations/feedback/types.ts b/src/cli/operations/feedback/types.ts new file mode 100644 index 000000000..bbe813b09 --- /dev/null +++ b/src/cli/operations/feedback/types.ts @@ -0,0 +1,72 @@ +export interface ApertureMetadata { + key: string; + value: string; +} + +export interface ApertureTextResponse { + question: string; + pii: boolean; + response: { + responseType: 'textArea' | 'text'; + responseValue: string; + }; +} + +export interface ApertureFileUploadResponse { + question: string; + pii: boolean; + response: { + responseType: 'fileUpload'; + responseValue: string[]; + }; +} + +export type ApertureCustomerResponse = ApertureTextResponse | ApertureFileUploadResponse; + +export interface ApertureFormPayload { + category: string; + name: string; + version: string; + locale: string; + reference?: string; + location?: string; + customerResponses: ApertureCustomerResponse[]; + metadataList: ApertureMetadata[]; +} + +export type FeedbackMode = 'cli' | 'tui'; + +export interface SubmissionContext { + mode: FeedbackMode; +} + +export interface AperturePresignedUrlRequest { + category: string; + name: string; + version: string; + fileName: string; + fileSize: number; + uploadFileSHA256: string; +} + +export interface ApertureSubmitResponse { + reference: string; + id: string; + timestamp: string; +} + +export interface FeedbackSubmissionResult { + id: string; + timestamp: string; + reference: string; +} + +export interface ScreenshotInput { + path: string; +} + +export interface SubmitFeedbackInput { + message: string; + screenshot?: ScreenshotInput; + mode?: FeedbackMode; +} diff --git a/src/cli/tui/copy.ts b/src/cli/tui/copy.ts index 71c08df89..59b574a76 100644 --- a/src/cli/tui/copy.ts +++ b/src/cli/tui/copy.ts @@ -42,6 +42,7 @@ export const COMMAND_DESCRIPTIONS = { status: 'Show deployed resource details and status.', traces: 'View and download agent traces.', evals: 'View saved eval and batch eval results from past runs.', + feedback: 'Send feedback about the AgentCore CLI to the team.', fetch: 'Fetch access info for deployed resources.', pause: 'Pause a deployed resource (online eval config, A/B test).', resume: 'Resume a paused resource (online eval config, A/B test).', diff --git a/src/cli/tui/screens/feedback/FeedbackScreen.tsx b/src/cli/tui/screens/feedback/FeedbackScreen.tsx new file mode 100644 index 000000000..15eaf0eab --- /dev/null +++ b/src/cli/tui/screens/feedback/FeedbackScreen.tsx @@ -0,0 +1,156 @@ +import { CONSENT_TEXT } from '../../../operations/feedback/constants'; +import { + ErrorPrompt, + Panel, + PathInput, + PromptScreen, + ScreenLayout, + StepIndicator, + SuccessPrompt, + TextInput, +} from '../../components'; +import { useFeedbackFlow } from './useFeedbackFlow'; +import type { FeedbackPhase } from './useFeedbackFlow'; +import { Box, Text } from 'ink'; +import React from 'react'; + +interface FeedbackScreenProps { + initialScreenshot?: string; + onExit: () => void; +} + +type IndicatorStep = 'message' | 'screenshot' | 'consent' | 'submitting' | 'success'; + +const INDICATOR_STEPS: IndicatorStep[] = ['message', 'screenshot', 'consent', 'submitting', 'success']; +const INDICATOR_LABELS: Record = { + message: 'Message', + screenshot: 'Screenshot', + consent: 'Consent', + submitting: 'Submitting', + success: 'Done', +}; + +function indicatorStepFor(phase: FeedbackPhase): IndicatorStep { + switch (phase) { + case 'message': + return 'message'; + case 'screenshot-prompt': + case 'screenshot-path': + return 'screenshot'; + case 'consent': + return 'consent'; + case 'submitting': + case 'error': + return 'submitting'; + case 'success': + return 'success'; + } +} + +export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProps) { + const flow = useFeedbackFlow({ initialScreenshot }); + const { + state, + setMessage, + chooseAttachScreenshot, + skipScreenshot, + setScreenshot, + confirmConsent, + declineConsent, + goBack, + retry, + } = flow; + + const header = ( + + + + ); + + if (state.phase === 'message') { + return ( + + {header} + + setMessage(value)} + onCancel={onExit} + expandable + /> + Enter to continue · Esc to exit + + + ); + } + + if (state.phase === 'screenshot-prompt') { + return ( + + Attach a screenshot? + PNG or JPG, max 100MB. Optional. + + ); + } + + if (state.phase === 'screenshot-path') { + return ( + + {header} + + setScreenshot(value.trim() || undefined)} + onCancel={goBack} + /> + ↑↓ navigate · → open dir · Enter select file · Esc to go back + + + ); + } + + if (state.phase === 'consent') { + return ( + + AWS Customer Agreement + {CONSENT_TEXT} + {state.screenshotPath && Screenshot: {state.screenshotPath}} + + ); + } + + if (state.phase === 'submitting') { + return ( + + {header} + + Submitting feedback… + + + ); + } + + if (state.phase === 'success') { + return ( + + ); + } + + return ; +} diff --git a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx new file mode 100644 index 000000000..e49ac9453 --- /dev/null +++ b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx @@ -0,0 +1,145 @@ +import { useFeedbackFlow } from '../useFeedbackFlow'; +import { Text } from 'ink'; +import { render } from 'ink-testing-library'; +import React, { act, useImperativeHandle } from 'react'; +import { describe, expect, it, vi } from 'vitest'; + +type FlowReturn = ReturnType; + +interface HarnessHandle { + flow: FlowReturn; +} + +interface HarnessProps { + onSubmit: NonNullable[0]>['onSubmit']; + initialScreenshot?: string; +} + +const Harness = React.forwardRef((props, ref) => { + const flow = useFeedbackFlow({ onSubmit: props.onSubmit, initialScreenshot: props.initialScreenshot }); + useImperativeHandle(ref, () => ({ flow })); + return ( + + phase:{flow.state.phase} message:{flow.state.message || ''} screenshot: + {flow.state.screenshotPath ?? ''} error: + {flow.state.error ?? ''} + + ); +}); +Harness.displayName = 'Harness'; + +function setup(onSubmit: HarnessProps['onSubmit'], initialScreenshot?: string) { + const ref = React.createRef(); + const result = render(); + return { ref, ...result }; +} + +async function flushAsync() { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +const successResult = { id: 'sub-1', timestamp: '2026-05-13T18:00:00Z', reference: 'S3' }; + +describe('useFeedbackFlow', () => { + it('starts on the message phase', () => { + const { ref } = setup(vi.fn()); + expect(ref.current!.flow.state.phase).toBe('message'); + }); + + it('walks through message → screenshot prompt → screenshot path → consent → success', async () => { + const onSubmit = vi.fn().mockResolvedValue(successResult); + const { ref } = setup(onSubmit); + + act(() => ref.current!.flow.setMessage('hello')); + expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + + act(() => ref.current!.flow.chooseAttachScreenshot()); + expect(ref.current!.flow.state.phase).toBe('screenshot-path'); + + act(() => ref.current!.flow.setScreenshot('/tmp/shot.png')); + expect(ref.current!.flow.state.phase).toBe('consent'); + expect(ref.current!.flow.state.screenshotPath).toBe('/tmp/shot.png'); + + act(() => ref.current!.flow.confirmConsent()); + await flushAsync(); + + expect(onSubmit).toHaveBeenCalledWith({ + message: 'hello', + screenshot: { path: '/tmp/shot.png' }, + mode: 'tui', + }); + expect(ref.current!.flow.state.phase).toBe('success'); + expect(ref.current!.flow.state.result).toEqual(successResult); + }); + + it('skips screenshot via the prompt step', () => { + const { ref } = setup(vi.fn()); + act(() => ref.current!.flow.setMessage('hi')); + expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + + act(() => ref.current!.flow.skipScreenshot()); + expect(ref.current!.flow.state.phase).toBe('consent'); + expect(ref.current!.flow.state.screenshotPath).toBeUndefined(); + }); + + it('treats an empty screenshot path the same as skipping', () => { + const { ref } = setup(vi.fn()); + act(() => ref.current!.flow.setMessage('hi')); + act(() => ref.current!.flow.chooseAttachScreenshot()); + act(() => ref.current!.flow.setScreenshot(undefined)); + expect(ref.current!.flow.state.phase).toBe('consent'); + expect(ref.current!.flow.state.screenshotPath).toBeUndefined(); + }); + + it('returns to the message phase with the message preserved when consent is declined', () => { + const { ref } = setup(vi.fn()); + act(() => ref.current!.flow.setMessage('I want to keep this')); + act(() => ref.current!.flow.skipScreenshot()); + act(() => ref.current!.flow.declineConsent()); + expect(ref.current!.flow.state.phase).toBe('message'); + expect(ref.current!.flow.state.message).toBe('I want to keep this'); + }); + + it('moves to error phase when submission fails and supports retry', async () => { + const onSubmit = vi.fn().mockRejectedValueOnce(new Error('HTTP 500')).mockResolvedValueOnce(successResult); + + const { ref } = setup(onSubmit); + act(() => ref.current!.flow.setMessage('boom')); + act(() => ref.current!.flow.skipScreenshot()); + act(() => ref.current!.flow.confirmConsent()); + await flushAsync(); + expect(ref.current!.flow.state.phase).toBe('error'); + expect(ref.current!.flow.state.error).toBe('HTTP 500'); + + act(() => ref.current!.flow.retry()); + await flushAsync(); + expect(ref.current!.flow.state.phase).toBe('success'); + expect(onSubmit).toHaveBeenCalledTimes(2); + }); + + it('goBack() steps from screenshot-prompt → message and from consent → screenshot-prompt', () => { + const { ref } = setup(vi.fn()); + act(() => ref.current!.flow.setMessage('hi')); + expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + act(() => ref.current!.flow.goBack()); + expect(ref.current!.flow.state.phase).toBe('message'); + + act(() => ref.current!.flow.setMessage('hi again')); + act(() => ref.current!.flow.skipScreenshot()); + expect(ref.current!.flow.state.phase).toBe('consent'); + act(() => ref.current!.flow.goBack()); + expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + }); + + it('goBack() steps from screenshot-path → screenshot-prompt', () => { + const { ref } = setup(vi.fn()); + act(() => ref.current!.flow.setMessage('hi')); + act(() => ref.current!.flow.chooseAttachScreenshot()); + expect(ref.current!.flow.state.phase).toBe('screenshot-path'); + act(() => ref.current!.flow.goBack()); + expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + }); +}); diff --git a/src/cli/tui/screens/feedback/index.ts b/src/cli/tui/screens/feedback/index.ts new file mode 100644 index 000000000..bf019c067 --- /dev/null +++ b/src/cli/tui/screens/feedback/index.ts @@ -0,0 +1,3 @@ +export { FeedbackScreen } from './FeedbackScreen'; +export { useFeedbackFlow } from './useFeedbackFlow'; +export type { FeedbackPhase, FeedbackState, UseFeedbackFlowOptions } from './useFeedbackFlow'; diff --git a/src/cli/tui/screens/feedback/useFeedbackFlow.ts b/src/cli/tui/screens/feedback/useFeedbackFlow.ts new file mode 100644 index 000000000..a10c0d576 --- /dev/null +++ b/src/cli/tui/screens/feedback/useFeedbackFlow.ts @@ -0,0 +1,121 @@ +import { submitFeedback } from '../../../operations/feedback'; +import type { FeedbackSubmissionResult } from '../../../operations/feedback'; +import { useCallback, useEffect, useRef, useState } from 'react'; + +export type FeedbackPhase = + | 'message' + | 'screenshot-prompt' + | 'screenshot-path' + | 'consent' + | 'submitting' + | 'success' + | 'error'; + +export interface FeedbackState { + phase: FeedbackPhase; + message: string; + screenshotPath?: string; + result?: FeedbackSubmissionResult; + error?: string; +} + +export interface UseFeedbackFlowOptions { + initialScreenshot?: string; + onSubmit?: typeof submitFeedback; +} + +export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { + const onSubmit = options.onSubmit ?? submitFeedback; + + const [state, setState] = useState({ + phase: 'message', + message: '', + screenshotPath: options.initialScreenshot, + }); + + const mountedRef = useRef(true); + useEffect(() => { + return () => { + mountedRef.current = false; + }; + }, []); + + const setMessage = useCallback((message: string) => { + setState(prev => ({ ...prev, message, phase: 'screenshot-prompt' })); + }, []); + + const chooseAttachScreenshot = useCallback(() => { + setState(prev => ({ ...prev, phase: 'screenshot-path' })); + }, []); + + const skipScreenshot = useCallback(() => { + setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent' })); + }, []); + + const setScreenshot = useCallback((screenshotPath: string | undefined) => { + const normalized = screenshotPath && screenshotPath.length > 0 ? screenshotPath : undefined; + if (!normalized) { + setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent' })); + return; + } + setState(prev => ({ ...prev, screenshotPath: normalized, phase: 'consent' })); + }, []); + + const performSubmit = useCallback(async () => { + setState(prev => ({ ...prev, phase: 'submitting', error: undefined })); + try { + const result = await onSubmit({ + message: state.message, + screenshot: state.screenshotPath ? { path: state.screenshotPath } : undefined, + mode: 'tui', + }); + if (!mountedRef.current) return; + setState(prev => ({ ...prev, phase: 'success', result })); + } catch (err) { + if (!mountedRef.current) return; + const error = err instanceof Error ? err.message : String(err); + setState(prev => ({ ...prev, phase: 'error', error })); + } + }, [onSubmit, state.message, state.screenshotPath]); + + const confirmConsent = useCallback(() => { + void performSubmit(); + }, [performSubmit]); + + const declineConsent = useCallback(() => { + setState(prev => ({ ...prev, phase: 'message' })); + }, []); + + const goBack = useCallback(() => { + setState(prev => { + switch (prev.phase) { + case 'screenshot-prompt': + return { ...prev, phase: 'message' }; + case 'screenshot-path': + return { ...prev, phase: 'screenshot-prompt' }; + case 'consent': + return { ...prev, phase: 'screenshot-prompt' }; + case 'error': + return { ...prev, phase: 'consent', error: undefined }; + default: + return prev; + } + }); + }, []); + + const retry = useCallback(() => { + void performSubmit(); + }, [performSubmit]); + + return { + state, + setMessage, + chooseAttachScreenshot, + skipScreenshot, + setScreenshot, + confirmConsent, + declineConsent, + goBack, + retry, + }; +} From ffbb0fe84821c1324ca0e1b50fcebd5f305c9554 Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 13:39:02 -0400 Subject: [PATCH 2/7] feat(feedback): address review feedback Three changes from automated review: 1. Telemetry: register `feedback` in command-run schema with `mode` + `has_screenshot` attrs, instrument the CLI handler with client.withCommandRun (CANCELLED on declined consent), and the TUI submit path with withCommandRunTelemetry. 2. Screenshot S3 key: parse the actual object key from the presigned URL path instead of fabricating one client-side. The fabricated key could drift from Aperture's bucket layout (UTC/local date, prefix changes) and silently produce form references to non-existent objects. 3. TUI input validation: validate message in setMessage and screenshot in setScreenshot, surface the error inline on the input phase via state.inputError. Previously bad input was only caught after the user walked through consent, forcing them to re-do consent on retry. Exposes validateFeedbackMessage and validateScreenshotPath from the operations module so the hook can reuse the same validators as the submission orchestrator. --- .../feedback/__tests__/command.test.ts | 39 +++++-- src/cli/commands/feedback/command.tsx | 90 +++++++-------- .../__tests__/submit-feedback.test.ts | 11 +- src/cli/operations/feedback/constants.ts | 1 - src/cli/operations/feedback/index.ts | 8 +- .../operations/feedback/submit-feedback.ts | 60 +++++++--- src/cli/telemetry/schemas/command-run.ts | 7 ++ .../tui/screens/feedback/FeedbackScreen.tsx | 4 +- .../__tests__/useFeedbackFlow.test.tsx | 104 ++++++++++++++---- .../tui/screens/feedback/useFeedbackFlow.ts | 94 +++++++++++----- 10 files changed, 289 insertions(+), 129 deletions(-) diff --git a/src/cli/commands/feedback/__tests__/command.test.ts b/src/cli/commands/feedback/__tests__/command.test.ts index e0f58eb19..6ed908e66 100644 --- a/src/cli/commands/feedback/__tests__/command.test.ts +++ b/src/cli/commands/feedback/__tests__/command.test.ts @@ -1,3 +1,6 @@ +import { TelemetryClient } from '../../../telemetry/client'; +import { TelemetryClientAccessor } from '../../../telemetry/client-accessor'; +import { InMemorySink } from '../../../telemetry/sinks/in-memory-sink'; import { registerFeedback } from '../command'; import { Command } from '@commander-js/extra-typings'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -34,6 +37,7 @@ const submittedOutcome = { describe('registerFeedback', () => { let program: Command; + let sink: InMemorySink; let mockExit: ReturnType; let mockLog: ReturnType; let mockError: ReturnType; @@ -43,6 +47,9 @@ describe('registerFeedback', () => { program.exitOverride(); registerFeedback(program); + sink = new InMemorySink(); + vi.spyOn(TelemetryClientAccessor, 'get').mockResolvedValue(new TelemetryClient(sink)); + mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit'); }); @@ -51,9 +58,7 @@ describe('registerFeedback', () => { }); afterEach(() => { - mockExit.mockRestore(); - mockLog.mockRestore(); - mockError.mockRestore(); + vi.restoreAllMocks(); vi.clearAllMocks(); }); @@ -65,10 +70,12 @@ describe('registerFeedback', () => { it('emits success JSON when --json is supplied with a message', async () => { mockHandleFeedback.mockResolvedValue(submittedOutcome); - await program.parseAsync(['feedback', 'looks good', '--json'], { from: 'user' }); + await expect(program.parseAsync(['feedback', 'looks good', '--json'], { from: 'user' })).rejects.toThrow( + 'process.exit' + ); expect(mockHandleFeedback).toHaveBeenCalledWith('looks good', expect.objectContaining({ json: true })); - expect(mockLog).toHaveBeenCalledTimes(1); + expect(mockExit).toHaveBeenCalledWith(0); const output = JSON.parse(mockLog.mock.calls[0]?.[0] as string); expect(output).toEqual({ success: true, @@ -76,6 +83,14 @@ describe('registerFeedback', () => { timestamp: '2026-05-13T18:00:00Z', reference: 'S3', }); + + expect(sink.metrics).toHaveLength(1); + expect(sink.metrics[0]!.attrs).toMatchObject({ + command: 'feedback', + exit_reason: 'success', + mode: 'cli', + has_screenshot: 'false', + }); }); it('reports a TTY error when consent cannot be confirmed and exits 1', async () => { @@ -90,10 +105,10 @@ describe('registerFeedback', () => { it('prints a friendly cancellation message when the user declines consent', async () => { mockHandleFeedback.mockResolvedValue({ kind: 'declined' }); - await program.parseAsync(['feedback', 'msg'], { from: 'user' }); + await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit'); expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('Feedback cancelled.')); - expect(mockExit).not.toHaveBeenCalled(); + expect(mockExit).toHaveBeenCalledWith(0); }); it('reports submission errors with exit 1 in plain mode', async () => { @@ -102,7 +117,15 @@ describe('registerFeedback', () => { await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit'); expect(mockExit).toHaveBeenCalledWith(1); - expect(mockRender).toHaveBeenCalled(); + expect(mockError).toHaveBeenCalledWith(expect.stringContaining('HTTP 500')); + + expect(sink.metrics).toHaveLength(1); + expect(sink.metrics[0]!.attrs).toMatchObject({ + command: 'feedback', + exit_reason: 'failure', + mode: 'cli', + has_screenshot: 'false', + }); }); it('emits a JSON error envelope on submission failure when --json is set', async () => { diff --git a/src/cli/commands/feedback/command.tsx b/src/cli/commands/feedback/command.tsx index bd5d43273..6d5da33b4 100644 --- a/src/cli/commands/feedback/command.tsx +++ b/src/cli/commands/feedback/command.tsx @@ -1,4 +1,4 @@ -import { getErrorMessage } from '../../errors'; +import { runCliCommand } from '../../telemetry/cli-command-run.js'; import { COMMAND_DESCRIPTIONS } from '../../tui/copy'; import { requireTTY } from '../../tui/guards/tty'; import { FeedbackScreen } from '../../tui/screens/feedback'; @@ -36,58 +36,46 @@ export const registerFeedback = (program: Command) => { return; } - let outcome; - try { - outcome = await handleFeedback(message, options); - } catch (error) { - const errMessage = getErrorMessage(error); - if (options.json) { - console.log(JSON.stringify({ success: false, error: errMessage })); - } else { - render(Error: {errMessage}); - } - process.exit(1); - return; - } + const has_screenshot = !!options.screenshot; + const knownAttrs = { mode: 'cli' as const, has_screenshot }; - if (outcome.kind === 'no-tty') { - const errorText = 'Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.'; - if (options.json) { - console.log(JSON.stringify({ success: false, error: errorText })); - } else { - console.error(errorText); - } - process.exit(1); - return; - } + await runCliCommand( + 'feedback', + !!options.json, + async () => { + const outcome = await handleFeedback(message, options); - if (outcome.kind === 'declined') { - if (options.json) { - console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' })); - } else { - console.log('Feedback cancelled. Nothing was submitted.'); - } - return; - } - - if (outcome.kind === 'error') { - if (options.json) { - console.log(JSON.stringify({ success: false, error: outcome.error })); - } else { - render(Error: {outcome.error}); - } - process.exit(1); - return; - } - - const result = outcome.result; - if (options.json) { - console.log( - JSON.stringify({ success: true, id: result.id, timestamp: result.timestamp, reference: result.reference }) - ); - return; - } + if (outcome.kind === 'no-tty') { + throw new Error('Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.'); + } + if (outcome.kind === 'error') { + throw new Error(outcome.error); + } + if (outcome.kind === 'declined') { + if (options.json) { + console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' })); + } else { + console.log('Feedback cancelled. Nothing was submitted.'); + } + return knownAttrs; + } - render(Thank you. Your feedback has been submitted (id: {result.id}).); + const result = outcome.result; + if (options.json) { + console.log( + JSON.stringify({ + success: true, + id: result.id, + timestamp: result.timestamp, + reference: result.reference, + }) + ); + } else { + render(Thank you. Your feedback has been submitted (id: {result.id}).); + } + return knownAttrs; + }, + knownAttrs + ); }); }; diff --git a/src/cli/operations/feedback/__tests__/submit-feedback.test.ts b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts index 25f08c9a1..5706e4f35 100644 --- a/src/cli/operations/feedback/__tests__/submit-feedback.test.ts +++ b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts @@ -74,7 +74,9 @@ describe('submitFeedback', () => { await fs.writeFile(screenshotPath, Buffer.from([0x89, 0x50, 0x4e, 0x47])); fetchMock - .mockResolvedValueOnce(new Response('https://s3.example/key?sig=x', { status: 200 })) + .mockResolvedValueOnce( + new Response('https://s3.example/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?sig=x', { status: 200 }) + ) .mockResolvedValueOnce(emptyOk(200)) .mockResolvedValueOnce(jsonResponse(successResponse)); @@ -101,7 +103,7 @@ describe('submitFeedback', () => { expect(presignBody.uploadFileSHA256).toHaveLength(44); const [uploadUrl, uploadInit] = callAt(1); - expect(uploadUrl).toBe('https://s3.example/key?sig=x'); + expect(uploadUrl).toBe('https://s3.example/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?sig=x'); expect(uploadInit.method).toBe('PUT'); expect(uploadInit.headers['content-type']).toBe('image/png'); expect(uploadInit.headers['x-amz-checksum-algorithm']).toBe('SHA256'); @@ -116,10 +118,11 @@ describe('submitFeedback', () => { pii: true, response: { responseType: 'fileUpload' }, }); - // Object key shape: us-east-1/AgentCore/CLI/0.1.0/DDMMYYYY/.png, wrapped in an array + // Reference must be the actual S3 key parsed from the presigned URL path, + // not a key fabricated client-side. const responseValue = submitBody.customerResponses[1].response.responseValue; expect(Array.isArray(responseValue)).toBe(true); - expect(responseValue[0]).toMatch(/^us-east-1\/AgentCore\/CLI\/0\.1\.0\/\d{8}\/[0-9a-f-]{36}\.png$/); + expect(responseValue[0]).toBe('us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png'); await fs.rm(tmp, { recursive: true, force: true }); }); diff --git a/src/cli/operations/feedback/constants.ts b/src/cli/operations/feedback/constants.ts index 1890903be..c2c439759 100644 --- a/src/cli/operations/feedback/constants.ts +++ b/src/cli/operations/feedback/constants.ts @@ -6,7 +6,6 @@ export const APERTURE_LOCALE = 'en_US'; export const APERTURE_INGESTION_URL = 'https://ingestion.aperture-public-api.feedback.console.aws.dev/form'; export const APERTURE_PRESIGNED_URL_ENDPOINT = 'https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl'; -export const APERTURE_S3_REGION = 'us-east-1'; export const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; export const ALLOWED_SCREENSHOT_EXTENSIONS = ['.png', '.jpg', '.jpeg'] as const; diff --git a/src/cli/operations/feedback/index.ts b/src/cli/operations/feedback/index.ts index 6563cf32a..d51f5e6c2 100644 --- a/src/cli/operations/feedback/index.ts +++ b/src/cli/operations/feedback/index.ts @@ -1,4 +1,10 @@ -export { submitFeedback, FeedbackValidationError } from './submit-feedback'; +export { + submitFeedback, + FeedbackValidationError, + validateFeedbackMessage, + validateScreenshotPath, + FEEDBACK_MESSAGE_MAX_LENGTH, +} from './submit-feedback'; export { ApertureError } from './aperture-client'; export { CONSENT_TEXT } from './constants'; export type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types'; diff --git a/src/cli/operations/feedback/submit-feedback.ts b/src/cli/operations/feedback/submit-feedback.ts index 92749a66a..6084b8fe8 100644 --- a/src/cli/operations/feedback/submit-feedback.ts +++ b/src/cli/operations/feedback/submit-feedback.ts @@ -6,11 +6,10 @@ import { APERTURE_FORM_CATEGORY, APERTURE_FORM_NAME, APERTURE_FORM_VERSION, - APERTURE_S3_REGION, MAX_SCREENSHOT_BYTES, } from './constants'; import type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types'; -import { createHash, randomUUID } from 'node:crypto'; +import { createHash } from 'node:crypto'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; @@ -26,20 +25,49 @@ function contentTypeForExtension(ext: string): string { return 'image/jpeg'; } -function validateMessage(message: string): void { +export const FEEDBACK_MESSAGE_MAX_LENGTH = 1000; + +/** + * Synchronous message validator. Returns null when valid, an error message + * string otherwise. Reused by the TUI hook so users see errors at input time + * rather than after walking through consent. + */ +export function validateFeedbackMessage(message: string): string | null { const trimmed = message.trim(); if (!trimmed) { - throw new FeedbackValidationError('Feedback message cannot be empty.'); + return 'Feedback message cannot be empty.'; + } + if (trimmed.length > FEEDBACK_MESSAGE_MAX_LENGTH) { + return `Feedback message must be ${FEEDBACK_MESSAGE_MAX_LENGTH} characters or fewer.`; } - if (trimmed.length > 1000) { - throw new FeedbackValidationError('Feedback message must be 1000 characters or fewer.'); + return null; +} + +function validateMessage(message: string): void { + const error = validateFeedbackMessage(message); + if (error) { + throw new FeedbackValidationError(error); + } +} + +/** + * Async screenshot validator that reads, size-checks, and extension-checks the + * file. Returns null on success. Reused by the TUI hook so the user sees an + * error on the path-input screen rather than after consent. + */ +export async function validateScreenshotPath(filePath: string): Promise { + try { + await loadAndValidateScreenshot(filePath); + return null; + } catch (err) { + if (err instanceof FeedbackValidationError) return err.message; + throw err; } } interface LoadedScreenshot { buffer: Uint8Array; fileName: string; - extension: string; contentType: string; sha256Base64: string; size: number; @@ -67,19 +95,21 @@ async function loadAndValidateScreenshot(filePath: string): Promise { @@ -102,7 +132,7 @@ export async function submitFeedback(input: SubmitFeedbackInput): Promise + {state.inputError && {state.inputError}} Enter to continue · Esc to exit @@ -107,9 +108,10 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp setScreenshot(value.trim() || undefined)} + onSubmit={value => void setScreenshot(value.trim() || undefined)} onCancel={goBack} /> + {state.inputError && {state.inputError}} ↑↓ navigate · → open dir · Enter select file · Esc to go back diff --git a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx index e49ac9453..7dde38899 100644 --- a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx +++ b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx @@ -1,8 +1,22 @@ +import { TelemetryClient } from '../../../../telemetry/client'; +import { TelemetryClientAccessor } from '../../../../telemetry/client-accessor'; +import { InMemorySink } from '../../../../telemetry/sinks/in-memory-sink'; import { useFeedbackFlow } from '../useFeedbackFlow'; import { Text } from 'ink'; import { render } from 'ink-testing-library'; import React, { act, useImperativeHandle } from 'react'; -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +let sink: InMemorySink; + +beforeEach(() => { + sink = new InMemorySink(); + vi.spyOn(TelemetryClientAccessor, 'get').mockResolvedValue(new TelemetryClient(sink)); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); type FlowReturn = ReturnType; @@ -13,45 +27,57 @@ interface HarnessHandle { interface HarnessProps { onSubmit: NonNullable[0]>['onSubmit']; initialScreenshot?: string; + validateMessage?: NonNullable[0]>['validateMessage']; + validateScreenshot?: NonNullable[0]>['validateScreenshot']; } const Harness = React.forwardRef((props, ref) => { - const flow = useFeedbackFlow({ onSubmit: props.onSubmit, initialScreenshot: props.initialScreenshot }); + const flow = useFeedbackFlow({ + onSubmit: props.onSubmit, + initialScreenshot: props.initialScreenshot, + validateMessage: props.validateMessage, + validateScreenshot: props.validateScreenshot, + }); useImperativeHandle(ref, () => ({ flow })); return ( phase:{flow.state.phase} message:{flow.state.message || ''} screenshot: {flow.state.screenshotPath ?? ''} error: - {flow.state.error ?? ''} + {flow.state.error ?? ''} inputError:{flow.state.inputError ?? ''} ); }); Harness.displayName = 'Harness'; -function setup(onSubmit: HarnessProps['onSubmit'], initialScreenshot?: string) { +function setup(props: HarnessProps) { const ref = React.createRef(); - const result = render(); + const result = render(); return { ref, ...result }; } async function flushAsync() { await act(async () => { - await Promise.resolve(); - await Promise.resolve(); + for (let i = 0; i < 8; i++) await Promise.resolve(); }); } const successResult = { id: 'sub-1', timestamp: '2026-05-13T18:00:00Z', reference: 'S3' }; +const stubValidateMessage = () => null; +const stubValidateScreenshot = () => Promise.resolve(null); describe('useFeedbackFlow', () => { it('starts on the message phase', () => { - const { ref } = setup(vi.fn()); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); expect(ref.current!.flow.state.phase).toBe('message'); }); it('walks through message → screenshot prompt → screenshot path → consent → success', async () => { const onSubmit = vi.fn().mockResolvedValue(successResult); - const { ref } = setup(onSubmit); + const { ref } = setup({ + onSubmit, + validateMessage: stubValidateMessage, + validateScreenshot: stubValidateScreenshot, + }); act(() => ref.current!.flow.setMessage('hello')); expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); @@ -59,7 +85,9 @@ describe('useFeedbackFlow', () => { act(() => ref.current!.flow.chooseAttachScreenshot()); expect(ref.current!.flow.state.phase).toBe('screenshot-path'); - act(() => ref.current!.flow.setScreenshot('/tmp/shot.png')); + await act(async () => { + await ref.current!.flow.setScreenshot('/tmp/shot.png'); + }); expect(ref.current!.flow.state.phase).toBe('consent'); expect(ref.current!.flow.state.screenshotPath).toBe('/tmp/shot.png'); @@ -73,10 +101,43 @@ describe('useFeedbackFlow', () => { }); expect(ref.current!.flow.state.phase).toBe('success'); expect(ref.current!.flow.state.result).toEqual(successResult); + + expect(sink.metrics).toHaveLength(1); + expect(sink.metrics[0]!.attrs).toMatchObject({ + command: 'feedback', + exit_reason: 'success', + mode: 'tui', + has_screenshot: 'true', + }); + }); + + it('keeps the user on the message phase and shows inputError when message is invalid', () => { + const validateMessage = vi.fn(() => 'too short'); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage }); + act(() => ref.current!.flow.setMessage('')); + expect(ref.current!.flow.state.phase).toBe('message'); + expect(ref.current!.flow.state.inputError).toBe('too short'); + }); + + it('keeps the user on screenshot-path and shows inputError for invalid screenshots', async () => { + const validateScreenshot = vi.fn(() => Promise.resolve('not a png' as string | null)); + const { ref } = setup({ + onSubmit: vi.fn(), + validateMessage: stubValidateMessage, + validateScreenshot, + }); + act(() => ref.current!.flow.setMessage('hi')); + act(() => ref.current!.flow.chooseAttachScreenshot()); + await act(async () => { + await ref.current!.flow.setScreenshot('/tmp/foo.gif'); + }); + expect(ref.current!.flow.state.phase).toBe('screenshot-path'); + expect(ref.current!.flow.state.inputError).toBe('not a png'); + expect(ref.current!.flow.state.screenshotPath).toBe('/tmp/foo.gif'); }); it('skips screenshot via the prompt step', () => { - const { ref } = setup(vi.fn()); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('hi')); expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); @@ -85,17 +146,23 @@ describe('useFeedbackFlow', () => { expect(ref.current!.flow.state.screenshotPath).toBeUndefined(); }); - it('treats an empty screenshot path the same as skipping', () => { - const { ref } = setup(vi.fn()); + it('treats an empty screenshot path the same as skipping', async () => { + const { ref } = setup({ + onSubmit: vi.fn(), + validateMessage: stubValidateMessage, + validateScreenshot: stubValidateScreenshot, + }); act(() => ref.current!.flow.setMessage('hi')); act(() => ref.current!.flow.chooseAttachScreenshot()); - act(() => ref.current!.flow.setScreenshot(undefined)); + await act(async () => { + await ref.current!.flow.setScreenshot(undefined); + }); expect(ref.current!.flow.state.phase).toBe('consent'); expect(ref.current!.flow.state.screenshotPath).toBeUndefined(); }); it('returns to the message phase with the message preserved when consent is declined', () => { - const { ref } = setup(vi.fn()); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('I want to keep this')); act(() => ref.current!.flow.skipScreenshot()); act(() => ref.current!.flow.declineConsent()); @@ -105,8 +172,7 @@ describe('useFeedbackFlow', () => { it('moves to error phase when submission fails and supports retry', async () => { const onSubmit = vi.fn().mockRejectedValueOnce(new Error('HTTP 500')).mockResolvedValueOnce(successResult); - - const { ref } = setup(onSubmit); + const { ref } = setup({ onSubmit, validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('boom')); act(() => ref.current!.flow.skipScreenshot()); act(() => ref.current!.flow.confirmConsent()); @@ -121,7 +187,7 @@ describe('useFeedbackFlow', () => { }); it('goBack() steps from screenshot-prompt → message and from consent → screenshot-prompt', () => { - const { ref } = setup(vi.fn()); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('hi')); expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); act(() => ref.current!.flow.goBack()); @@ -135,7 +201,7 @@ describe('useFeedbackFlow', () => { }); it('goBack() steps from screenshot-path → screenshot-prompt', () => { - const { ref } = setup(vi.fn()); + const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('hi')); act(() => ref.current!.flow.chooseAttachScreenshot()); expect(ref.current!.flow.state.phase).toBe('screenshot-path'); diff --git a/src/cli/tui/screens/feedback/useFeedbackFlow.ts b/src/cli/tui/screens/feedback/useFeedbackFlow.ts index a10c0d576..dd535f1ef 100644 --- a/src/cli/tui/screens/feedback/useFeedbackFlow.ts +++ b/src/cli/tui/screens/feedback/useFeedbackFlow.ts @@ -1,5 +1,6 @@ -import { submitFeedback } from '../../../operations/feedback'; +import { submitFeedback, validateFeedbackMessage, validateScreenshotPath } from '../../../operations/feedback'; import type { FeedbackSubmissionResult } from '../../../operations/feedback'; +import { withCommandRunTelemetry } from '../../../telemetry/cli-command-run'; import { useCallback, useEffect, useRef, useState } from 'react'; export type FeedbackPhase = @@ -16,16 +17,23 @@ export interface FeedbackState { message: string; screenshotPath?: string; result?: FeedbackSubmissionResult; + /** Submission failure detail, shown on the 'error' phase. */ error?: string; + /** Inline validation error shown on the current input phase. */ + inputError?: string; } export interface UseFeedbackFlowOptions { initialScreenshot?: string; onSubmit?: typeof submitFeedback; + validateMessage?: typeof validateFeedbackMessage; + validateScreenshot?: typeof validateScreenshotPath; } export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { const onSubmit = options.onSubmit ?? submitFeedback; + const validateMessage = options.validateMessage ?? validateFeedbackMessage; + const validateScreenshot = options.validateScreenshot ?? validateScreenshotPath; const [state, setState] = useState({ phase: 'message', @@ -40,41 +48,69 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { }; }, []); - const setMessage = useCallback((message: string) => { - setState(prev => ({ ...prev, message, phase: 'screenshot-prompt' })); - }, []); + const setMessage = useCallback( + (message: string) => { + const validationError = validateMessage(message); + if (validationError) { + setState(prev => ({ ...prev, message, inputError: validationError })); + return; + } + setState(prev => ({ ...prev, message, phase: 'screenshot-prompt', inputError: undefined })); + }, + [validateMessage] + ); const chooseAttachScreenshot = useCallback(() => { - setState(prev => ({ ...prev, phase: 'screenshot-path' })); + setState(prev => ({ ...prev, phase: 'screenshot-path', inputError: undefined })); }, []); const skipScreenshot = useCallback(() => { - setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent' })); + setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent', inputError: undefined })); }, []); - const setScreenshot = useCallback((screenshotPath: string | undefined) => { - const normalized = screenshotPath && screenshotPath.length > 0 ? screenshotPath : undefined; - if (!normalized) { - setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent' })); - return; - } - setState(prev => ({ ...prev, screenshotPath: normalized, phase: 'consent' })); - }, []); + const setScreenshot = useCallback( + async (screenshotPath: string | undefined) => { + const normalized = screenshotPath && screenshotPath.length > 0 ? screenshotPath : undefined; + if (!normalized) { + setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent', inputError: undefined })); + return; + } + const validationError = await validateScreenshot(normalized); + if (!mountedRef.current) return; + if (validationError) { + setState(prev => ({ ...prev, screenshotPath: normalized, inputError: validationError })); + return; + } + setState(prev => ({ ...prev, screenshotPath: normalized, phase: 'consent', inputError: undefined })); + }, + [validateScreenshot] + ); const performSubmit = useCallback(async () => { setState(prev => ({ ...prev, phase: 'submitting', error: undefined })); - try { - const result = await onSubmit({ - message: state.message, - screenshot: state.screenshotPath ? { path: state.screenshotPath } : undefined, - mode: 'tui', - }); - if (!mountedRef.current) return; - setState(prev => ({ ...prev, phase: 'success', result })); - } catch (err) { - if (!mountedRef.current) return; - const error = err instanceof Error ? err.message : String(err); - setState(prev => ({ ...prev, phase: 'error', error })); + const has_screenshot = !!state.screenshotPath; + const result = await withCommandRunTelemetry( + 'feedback', + { mode: 'tui', has_screenshot }, + async (): Promise<{ success: true; submission: FeedbackSubmissionResult } | { success: false; error: Error }> => { + try { + const submission = await onSubmit({ + message: state.message, + screenshot: state.screenshotPath ? { path: state.screenshotPath } : undefined, + mode: 'tui', + }); + return { success: true, submission }; + } catch (err) { + return { success: false, error: err instanceof Error ? err : new Error(String(err)) }; + } + } + ); + + if (!mountedRef.current) return; + if (result.success) { + setState(prev => ({ ...prev, phase: 'success', result: result.submission })); + } else { + setState(prev => ({ ...prev, phase: 'error', error: result.error.message })); } }, [onSubmit, state.message, state.screenshotPath]); @@ -90,11 +126,11 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { setState(prev => { switch (prev.phase) { case 'screenshot-prompt': - return { ...prev, phase: 'message' }; + return { ...prev, phase: 'message', inputError: undefined }; case 'screenshot-path': - return { ...prev, phase: 'screenshot-prompt' }; + return { ...prev, phase: 'screenshot-prompt', inputError: undefined }; case 'consent': - return { ...prev, phase: 'screenshot-prompt' }; + return { ...prev, phase: 'screenshot-prompt', inputError: undefined }; case 'error': return { ...prev, phase: 'consent', error: undefined }; default: From 884a194d9c6b84738556254844630b6ebb8a91a1 Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 14:38:58 -0400 Subject: [PATCH 3/7] fix(feedback): preserve full Error object through outcome and state Per Hweinstock's review feedback. Was stringifying errors to .message at the boundary in handleFeedback and useFeedbackFlow, which discarded the stack/cause. Keeping the Error around lets callers retain that info and narrow with instanceof when they want; we still pull .message at the final UI render boundary. --- src/cli/commands/feedback/action.ts | 5 ++--- src/cli/commands/feedback/command.tsx | 2 +- src/cli/tui/screens/feedback/FeedbackScreen.tsx | 4 +++- .../tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx | 4 ++-- src/cli/tui/screens/feedback/useFeedbackFlow.ts | 6 +++--- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/cli/commands/feedback/action.ts b/src/cli/commands/feedback/action.ts index feec7c9db..2b32251a5 100644 --- a/src/cli/commands/feedback/action.ts +++ b/src/cli/commands/feedback/action.ts @@ -7,7 +7,7 @@ export type FeedbackOutcome = | { kind: 'submitted'; result: FeedbackSubmissionResult } | { kind: 'declined' } | { kind: 'no-tty' } - | { kind: 'error'; error: string }; + | { kind: 'error'; error: Error }; export async function handleFeedback(message: string, options: FeedbackOptions): Promise { const consent = await promptForConsent(); @@ -23,7 +23,6 @@ export async function handleFeedback(message: string, options: FeedbackOptions): }); return { kind: 'submitted', result }; } catch (err) { - const error = err instanceof Error ? err.message : String(err); - return { kind: 'error', error }; + return { kind: 'error', error: err instanceof Error ? err : new Error(String(err)) }; } } diff --git a/src/cli/commands/feedback/command.tsx b/src/cli/commands/feedback/command.tsx index 6d5da33b4..af66247f6 100644 --- a/src/cli/commands/feedback/command.tsx +++ b/src/cli/commands/feedback/command.tsx @@ -49,7 +49,7 @@ export const registerFeedback = (program: Command) => { throw new Error('Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.'); } if (outcome.kind === 'error') { - throw new Error(outcome.error); + throw outcome.error; } if (outcome.kind === 'declined') { if (options.json) { diff --git a/src/cli/tui/screens/feedback/FeedbackScreen.tsx b/src/cli/tui/screens/feedback/FeedbackScreen.tsx index f1497d69c..6e833a8a2 100644 --- a/src/cli/tui/screens/feedback/FeedbackScreen.tsx +++ b/src/cli/tui/screens/feedback/FeedbackScreen.tsx @@ -154,5 +154,7 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp ); } - return ; + return ( + + ); } diff --git a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx index 7dde38899..b5c458023 100644 --- a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx +++ b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx @@ -43,7 +43,7 @@ const Harness = React.forwardRef((props, ref) => { phase:{flow.state.phase} message:{flow.state.message || ''} screenshot: {flow.state.screenshotPath ?? ''} error: - {flow.state.error ?? ''} inputError:{flow.state.inputError ?? ''} + {flow.state.error?.message ?? ''} inputError:{flow.state.inputError ?? ''} ); }); @@ -178,7 +178,7 @@ describe('useFeedbackFlow', () => { act(() => ref.current!.flow.confirmConsent()); await flushAsync(); expect(ref.current!.flow.state.phase).toBe('error'); - expect(ref.current!.flow.state.error).toBe('HTTP 500'); + expect(ref.current!.flow.state.error?.message).toBe('HTTP 500'); act(() => ref.current!.flow.retry()); await flushAsync(); diff --git a/src/cli/tui/screens/feedback/useFeedbackFlow.ts b/src/cli/tui/screens/feedback/useFeedbackFlow.ts index dd535f1ef..f929e55e1 100644 --- a/src/cli/tui/screens/feedback/useFeedbackFlow.ts +++ b/src/cli/tui/screens/feedback/useFeedbackFlow.ts @@ -17,8 +17,8 @@ export interface FeedbackState { message: string; screenshotPath?: string; result?: FeedbackSubmissionResult; - /** Submission failure detail, shown on the 'error' phase. */ - error?: string; + /** Submission failure detail, shown on the 'error' phase. Preserved as Error so callers retain stack/cause. */ + error?: Error; /** Inline validation error shown on the current input phase. */ inputError?: string; } @@ -110,7 +110,7 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { if (result.success) { setState(prev => ({ ...prev, phase: 'success', result: result.submission })); } else { - setState(prev => ({ ...prev, phase: 'error', error: result.error.message })); + setState(prev => ({ ...prev, phase: 'error', error: result.error })); } }, [onSubmit, state.message, state.screenshotPath]); From 1f387d7e4cb01e0b8c0f03bccbc8e7cf11a784d6 Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 15:47:48 -0400 Subject: [PATCH 4/7] fix(feedback): drop metadata keys not registered on Aperture form template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug bash test 2.2 surfaced an HTTP 400 from Aperture: "There is a mismatch between the Form's metadata and the Form-Template's metadata" Root cause: a previous review-fix push added `node-version` and `cli-mode` keys to metadataList, but the published Aperture form template only registers `cli-version` and `os`. Aperture rejects any submission with unrecognized metadata keys. Fix: drop the two unrecognized keys from metadataList. Encode the same context (node version, cli/tui mode) into the `location` field, which is free-form text on Aperture's side. Verified end-to-end through the CLI against prod: agentcore feedback "post-fix CLI submission verification" --json → {"success":true,"id":"211f3041-...","reference":"agentcore-cli"} --- .../feedback/__tests__/build-payload.test.ts | 7 ++++--- src/cli/operations/feedback/build-payload.ts | 11 ++++------- src/cli/operations/feedback/constants.ts | 2 -- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/cli/operations/feedback/__tests__/build-payload.test.ts b/src/cli/operations/feedback/__tests__/build-payload.test.ts index 902010fae..746f0d8ce 100644 --- a/src/cli/operations/feedback/__tests__/build-payload.test.ts +++ b/src/cli/operations/feedback/__tests__/build-payload.test.ts @@ -7,7 +7,6 @@ describe('buildFeedbackPayload', () => { message: 'CLI ate my homework', cliVersion: '0.1.0-alpha.42', osDescriptor: 'darwin 25.3.0', - nodeVersion: 'v20.20.0', mode: 'cli', }); @@ -17,17 +16,19 @@ describe('buildFeedbackPayload', () => { expect(payload.locale).toBe('en_US'); expect(payload.reference).toBe('agentcore-cli'); expect(payload.location).toContain('agentcore-cli@0.1.0-alpha.42'); + expect(payload.location).toContain('cli'); expect(payload.customerResponses).toHaveLength(1); expect(payload.customerResponses[0]).toMatchObject({ question: 'What feedback do you have for the AgentCore CLI', pii: false, response: { responseType: 'textArea', responseValue: 'CLI ate my homework' }, }); + // Aperture form template only registers `cli-version` and `os`. Other + // context (node version, mode) lives in `location` until the template + // adds those keys; sending unknown metadata keys is a hard 400. expect(payload.metadataList).toEqual([ { key: 'cli-version', value: '0.1.0-alpha.42' }, { key: 'os', value: 'darwin 25.3.0' }, - { key: 'node-version', value: 'v20.20.0' }, - { key: 'cli-mode', value: 'cli' }, ]); }); diff --git a/src/cli/operations/feedback/build-payload.ts b/src/cli/operations/feedback/build-payload.ts index 0533a4764..cb10f6d61 100644 --- a/src/cli/operations/feedback/build-payload.ts +++ b/src/cli/operations/feedback/build-payload.ts @@ -7,9 +7,7 @@ import { FEEDBACK_ATTACHMENT_QUESTION, FEEDBACK_MESSAGE_QUESTION, FEEDBACK_REFERENCE, - METADATA_KEY_CLI_MODE, METADATA_KEY_CLI_VERSION, - METADATA_KEY_NODE_VERSION, METADATA_KEY_OS, } from './constants'; import type { ApertureCustomerResponse, ApertureFormPayload, ApertureMetadata, FeedbackMode } from './types'; @@ -21,7 +19,6 @@ interface BuildPayloadInput { mode?: FeedbackMode; cliVersion?: string; osDescriptor?: string; - nodeVersion?: string; } export function buildOsDescriptor(): string { @@ -29,7 +26,7 @@ export function buildOsDescriptor(): string { } export function buildLocationDescriptor(cliVersion: string, mode: FeedbackMode): string { - return `agentcore-cli@${cliVersion} (${process.platform}; ${mode})`; + return `agentcore-cli@${cliVersion} (${process.platform}; node ${process.version}; ${mode})`; } export function buildUserAgent(cliVersion: string): string { @@ -39,7 +36,6 @@ export function buildUserAgent(cliVersion: string): string { export function buildFeedbackPayload(input: BuildPayloadInput): ApertureFormPayload { const cliVersion = input.cliVersion ?? PACKAGE_VERSION; const osDescriptor = input.osDescriptor ?? buildOsDescriptor(); - const nodeVersion = input.nodeVersion ?? process.version; const mode: FeedbackMode = input.mode ?? 'cli'; const customerResponses: ApertureCustomerResponse[] = [ @@ -64,11 +60,12 @@ export function buildFeedbackPayload(input: BuildPayloadInput): ApertureFormPayl }); } + // Aperture validates metadata keys against the published form template; sending unknown + // keys is rejected with HTTP 400. Only `cli-version` and `os` are registered today. + // node-version and cli-mode are encoded into `location` until the template is updated. const metadataList: ApertureMetadata[] = [ { key: METADATA_KEY_CLI_VERSION, value: cliVersion }, { key: METADATA_KEY_OS, value: osDescriptor }, - { key: METADATA_KEY_NODE_VERSION, value: nodeVersion }, - { key: METADATA_KEY_CLI_MODE, value: mode }, ]; return { diff --git a/src/cli/operations/feedback/constants.ts b/src/cli/operations/feedback/constants.ts index c2c439759..5d39cfd75 100644 --- a/src/cli/operations/feedback/constants.ts +++ b/src/cli/operations/feedback/constants.ts @@ -14,8 +14,6 @@ export const FEEDBACK_MESSAGE_QUESTION = 'What feedback do you have for the Agen export const FEEDBACK_ATTACHMENT_QUESTION = 'Attachments'; export const METADATA_KEY_CLI_VERSION = 'cli-version'; export const METADATA_KEY_OS = 'os'; -export const METADATA_KEY_NODE_VERSION = 'node-version'; -export const METADATA_KEY_CLI_MODE = 'cli-mode'; export const FEEDBACK_REFERENCE = 'agentcore-cli'; From ee4920b99b60af17f17ff88078eefdda7ea3055d Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 16:02:02 -0400 Subject: [PATCH 5/7] =?UTF-8?q?fix(feedback):=20bug=20bash=20fixes=20?= =?UTF-8?q?=E2=80=94=20wizard=20exit,=20dir=20error,=20tilde=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues from bug bash: - 2.5: Esc/Ctrl+C in the wizard didn't terminate the process. The onExit callback unmounted the screen but Ink's stdin raw-mode listeners kept Node alive. Fix: await render()'s waitUntilExit() and call process.exit(0) once the wizard unmounts. Matches invoke/command.tsx's pattern. - 3.2: Pointing --screenshot at a directory ("/tmp") produced "Screenshot must be one of: .png, .jpg, .jpeg" because we extension- checked first. Fix: stat the path first; surface a directory-specific error before the extension check. - 3.6: Quoted paths starting with "~/" (e.g. ~/Desktop/foo.png) hit ENOENT because Node's fs APIs don't expand tildes — only the shell does, and quoting suppresses that. Fix: expand a leading "~" or "~/..." to os.homedir() before stat/read. Added unit tests for both new validator branches. --- .../feedback/__tests__/command.test.ts | 13 ++++++-- src/cli/commands/feedback/command.tsx | 7 ++-- .../__tests__/submit-feedback.test.ts | 27 ++++++++++++++++ .../operations/feedback/submit-feedback.ts | 32 ++++++++++++++++++- 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/src/cli/commands/feedback/__tests__/command.test.ts b/src/cli/commands/feedback/__tests__/command.test.ts index 6ed908e66..59b0be6ce 100644 --- a/src/cli/commands/feedback/__tests__/command.test.ts +++ b/src/cli/commands/feedback/__tests__/command.test.ts @@ -24,7 +24,11 @@ vi.mock('../../../tui/screens/feedback', () => ({ vi.mock('ink', () => ({ render: (...args: unknown[]) => { mockRender(...args); - return { clear: vi.fn(), unmount: vi.fn() }; + return { + clear: vi.fn(), + unmount: vi.fn(), + waitUntilExit: () => Promise.resolve(), + }; }, Text: 'Text', Box: 'Box', @@ -146,11 +150,14 @@ describe('registerFeedback', () => { expect(mockHandleFeedback).not.toHaveBeenCalled(); }); - it('hands off to the TUI when no message argument is provided', async () => { - await program.parseAsync(['feedback'], { from: 'user' }); + it('hands off to the TUI when no message argument is provided, then exits cleanly', async () => { + await expect(program.parseAsync(['feedback'], { from: 'user' })).rejects.toThrow('process.exit'); expect(mockRequireTTY).toHaveBeenCalled(); expect(mockRender).toHaveBeenCalled(); expect(mockHandleFeedback).not.toHaveBeenCalled(); + // After the wizard unmounts we must terminate the Node process; otherwise + // Ink's stdin raw-mode listeners keep the process alive. + expect(mockExit).toHaveBeenCalledWith(0); }); }); diff --git a/src/cli/commands/feedback/command.tsx b/src/cli/commands/feedback/command.tsx index af66247f6..e52ec41e7 100644 --- a/src/cli/commands/feedback/command.tsx +++ b/src/cli/commands/feedback/command.tsx @@ -24,7 +24,7 @@ export const registerFeedback = (program: Command) => { return; } requireTTY(); - const { clear, unmount } = render( + const { clear, unmount, waitUntilExit } = render( { @@ -33,7 +33,10 @@ export const registerFeedback = (program: Command) => { }} /> ); - return; + // Wait for the wizard to unmount, then exit. Without this Node sticks + // around because of Ink's stdin raw-mode listeners. + await waitUntilExit(); + process.exit(0); } const has_screenshot = !!options.screenshot; diff --git a/src/cli/operations/feedback/__tests__/submit-feedback.test.ts b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts index 5706e4f35..de6f5d474 100644 --- a/src/cli/operations/feedback/__tests__/submit-feedback.test.ts +++ b/src/cli/operations/feedback/__tests__/submit-feedback.test.ts @@ -147,6 +147,33 @@ describe('submitFeedback', () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it('rejects a directory with a directory-specific error, not the extension error', async () => { + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'feedback-test-')); + await expect(submitFeedback({ message: 'msg', screenshot: { path: tmp } })).rejects.toThrow( + /is a directory, not a file/ + ); + expect(fetchMock).not.toHaveBeenCalled(); + await fs.rm(tmp, { recursive: true, force: true }); + }); + + it('expands a leading tilde so quoted paths like "~/file.png" resolve', async () => { + // Drop a real file in $HOME so tilde-expansion has somewhere to land + const fileName = `feedback-tilde-${Date.now()}.png`; + const realPath = path.join(os.homedir(), fileName); + await fs.writeFile(realPath, Buffer.from([0x89, 0x50, 0x4e, 0x47])); + + fetchMock + .mockResolvedValueOnce(new Response('https://s3.example/key?sig=x', { status: 200 })) + .mockResolvedValueOnce(emptyOk(200)) + .mockResolvedValueOnce(jsonResponse(successResponse)); + + const result = await submitFeedback({ message: 'tilde', screenshot: { path: `~/${fileName}` } }); + expect(result.id).toBe('submission-123'); + expect(fetchMock).toHaveBeenCalledTimes(3); + + await fs.rm(realPath, { force: true }); + }); + it('maps Aperture 412 responses to a missing-headers error', async () => { fetchMock.mockResolvedValueOnce(new Response('missing headers', { status: 412 })); diff --git a/src/cli/operations/feedback/submit-feedback.ts b/src/cli/operations/feedback/submit-feedback.ts index 6084b8fe8..9b8205c05 100644 --- a/src/cli/operations/feedback/submit-feedback.ts +++ b/src/cli/operations/feedback/submit-feedback.ts @@ -11,6 +11,7 @@ import { import type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types'; import { createHash } from 'node:crypto'; import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; import * as path from 'node:path'; export class FeedbackValidationError extends Error { @@ -73,7 +74,36 @@ interface LoadedScreenshot { size: number; } -async function loadAndValidateScreenshot(filePath: string): Promise { +/** + * Expand a leading `~` or `~/...` to the user's home directory. Node's fs APIs + * don't expand tildes — the shell normally does — so users who quote a path + * like `"~/Desktop/foo.png"` to preserve spaces hit ENOENT. This handles that. + */ +function expandTilde(filePath: string): string { + if (filePath === '~') return os.homedir(); + if (filePath.startsWith('~/')) return path.join(os.homedir(), filePath.slice(2)); + return filePath; +} + +async function loadAndValidateScreenshot(rawFilePath: string): Promise { + const filePath = expandTilde(rawFilePath); + + // Stat the path first so we can give a precise error for directories or + // missing files, rather than letting the extension check mask them. + let stat: Awaited>; + try { + stat = await fs.stat(filePath); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + throw new FeedbackValidationError(`Could not read screenshot at ${filePath}: ${reason}`); + } + if (stat.isDirectory()) { + throw new FeedbackValidationError(`Screenshot path is a directory, not a file: ${filePath}`); + } + if (!stat.isFile()) { + throw new FeedbackValidationError(`Screenshot path is not a regular file: ${filePath}`); + } + const ext = path.extname(filePath).toLowerCase(); if (!ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number])) { throw new FeedbackValidationError(`Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(', ')}.`); From 90dbe0cb872b5f587ee6eb1b703f288385565447 Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 16:18:44 -0400 Subject: [PATCH 6/7] fix(feedback): collapse "Attach Y/N?" prompt; screenshot is just optional MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per tester feedback: the explicit Y/N prompt felt like an extra step for something that's already optional. Drop the screenshot-prompt phase entirely — the wizard now goes Message → Screenshot → Consent. The Screenshot phase is the path picker itself, with allowEmpty=true on PathInput. Submit with a path to attach; Enter on empty input or Esc to skip. Help text reflects this: "Enter select / skip · Esc skip". Verified end-to-end via TUI harness: live submission id 96e2874e-3a61-4f65-8140-c144393d841e through the new flow. --- .../tui/screens/feedback/FeedbackScreen.tsx | 43 +++++-------------- .../__tests__/useFeedbackFlow.test.tsx | 30 ++++--------- .../tui/screens/feedback/useFeedbackFlow.ts | 24 +++-------- 3 files changed, 25 insertions(+), 72 deletions(-) diff --git a/src/cli/tui/screens/feedback/FeedbackScreen.tsx b/src/cli/tui/screens/feedback/FeedbackScreen.tsx index 6e833a8a2..4ca9da7ed 100644 --- a/src/cli/tui/screens/feedback/FeedbackScreen.tsx +++ b/src/cli/tui/screens/feedback/FeedbackScreen.tsx @@ -34,8 +34,7 @@ function indicatorStepFor(phase: FeedbackPhase): IndicatorStep { switch (phase) { case 'message': return 'message'; - case 'screenshot-prompt': - case 'screenshot-path': + case 'screenshot': return 'screenshot'; case 'consent': return 'consent'; @@ -49,17 +48,7 @@ function indicatorStepFor(phase: FeedbackPhase): IndicatorStep { export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProps) { const flow = useFeedbackFlow({ initialScreenshot }); - const { - state, - setMessage, - chooseAttachScreenshot, - skipScreenshot, - setScreenshot, - confirmConsent, - declineConsent, - goBack, - retry, - } = flow; + const { state, setMessage, skipScreenshot, setScreenshot, confirmConsent, declineConsent, goBack, retry } = flow; const header = ( @@ -87,32 +76,21 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp ); } - if (state.phase === 'screenshot-prompt') { + if (state.phase === 'screenshot') { return ( - - Attach a screenshot? - PNG or JPG, max 100MB. Optional. - - ); - } - - if (state.phase === 'screenshot-path') { - return ( - + {header} - + void setScreenshot(value.trim() || undefined)} - onCancel={goBack} + onCancel={skipScreenshot} + allowEmpty + emptyHelpText="No screenshot will be attached." /> {state.inputError && {state.inputError}} - ↑↓ navigate · → open dir · Enter select file · Esc to go back + ↑↓ navigate · → open dir · Enter select / skip · Esc skip ); @@ -124,6 +102,7 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp helpText="Enter/Y submit · Esc/N cancel" onConfirm={confirmConsent} onExit={declineConsent} + onBack={goBack} borderColor="yellow" > AWS Customer Agreement diff --git a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx index b5c458023..06d9da4c2 100644 --- a/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx +++ b/src/cli/tui/screens/feedback/__tests__/useFeedbackFlow.test.tsx @@ -80,10 +80,7 @@ describe('useFeedbackFlow', () => { }); act(() => ref.current!.flow.setMessage('hello')); - expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); - - act(() => ref.current!.flow.chooseAttachScreenshot()); - expect(ref.current!.flow.state.phase).toBe('screenshot-path'); + expect(ref.current!.flow.state.phase).toBe('screenshot'); await act(async () => { await ref.current!.flow.setScreenshot('/tmp/shot.png'); @@ -119,7 +116,7 @@ describe('useFeedbackFlow', () => { expect(ref.current!.flow.state.inputError).toBe('too short'); }); - it('keeps the user on screenshot-path and shows inputError for invalid screenshots', async () => { + it('keeps the user on the screenshot phase and shows inputError for invalid screenshots', async () => { const validateScreenshot = vi.fn(() => Promise.resolve('not a png' as string | null)); const { ref } = setup({ onSubmit: vi.fn(), @@ -127,19 +124,18 @@ describe('useFeedbackFlow', () => { validateScreenshot, }); act(() => ref.current!.flow.setMessage('hi')); - act(() => ref.current!.flow.chooseAttachScreenshot()); await act(async () => { await ref.current!.flow.setScreenshot('/tmp/foo.gif'); }); - expect(ref.current!.flow.state.phase).toBe('screenshot-path'); + expect(ref.current!.flow.state.phase).toBe('screenshot'); expect(ref.current!.flow.state.inputError).toBe('not a png'); expect(ref.current!.flow.state.screenshotPath).toBe('/tmp/foo.gif'); }); - it('skips screenshot via the prompt step', () => { + it('skips the screenshot via Esc (skipScreenshot)', () => { const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('hi')); - expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + expect(ref.current!.flow.state.phase).toBe('screenshot'); act(() => ref.current!.flow.skipScreenshot()); expect(ref.current!.flow.state.phase).toBe('consent'); @@ -153,7 +149,6 @@ describe('useFeedbackFlow', () => { validateScreenshot: stubValidateScreenshot, }); act(() => ref.current!.flow.setMessage('hi')); - act(() => ref.current!.flow.chooseAttachScreenshot()); await act(async () => { await ref.current!.flow.setScreenshot(undefined); }); @@ -186,10 +181,10 @@ describe('useFeedbackFlow', () => { expect(onSubmit).toHaveBeenCalledTimes(2); }); - it('goBack() steps from screenshot-prompt → message and from consent → screenshot-prompt', () => { + it('goBack() steps from screenshot → message and from consent → screenshot', () => { const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); act(() => ref.current!.flow.setMessage('hi')); - expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + expect(ref.current!.flow.state.phase).toBe('screenshot'); act(() => ref.current!.flow.goBack()); expect(ref.current!.flow.state.phase).toBe('message'); @@ -197,15 +192,6 @@ describe('useFeedbackFlow', () => { act(() => ref.current!.flow.skipScreenshot()); expect(ref.current!.flow.state.phase).toBe('consent'); act(() => ref.current!.flow.goBack()); - expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); - }); - - it('goBack() steps from screenshot-path → screenshot-prompt', () => { - const { ref } = setup({ onSubmit: vi.fn(), validateMessage: stubValidateMessage }); - act(() => ref.current!.flow.setMessage('hi')); - act(() => ref.current!.flow.chooseAttachScreenshot()); - expect(ref.current!.flow.state.phase).toBe('screenshot-path'); - act(() => ref.current!.flow.goBack()); - expect(ref.current!.flow.state.phase).toBe('screenshot-prompt'); + expect(ref.current!.flow.state.phase).toBe('screenshot'); }); }); diff --git a/src/cli/tui/screens/feedback/useFeedbackFlow.ts b/src/cli/tui/screens/feedback/useFeedbackFlow.ts index f929e55e1..d47665693 100644 --- a/src/cli/tui/screens/feedback/useFeedbackFlow.ts +++ b/src/cli/tui/screens/feedback/useFeedbackFlow.ts @@ -3,14 +3,7 @@ import type { FeedbackSubmissionResult } from '../../../operations/feedback'; import { withCommandRunTelemetry } from '../../../telemetry/cli-command-run'; import { useCallback, useEffect, useRef, useState } from 'react'; -export type FeedbackPhase = - | 'message' - | 'screenshot-prompt' - | 'screenshot-path' - | 'consent' - | 'submitting' - | 'success' - | 'error'; +export type FeedbackPhase = 'message' | 'screenshot' | 'consent' | 'submitting' | 'success' | 'error'; export interface FeedbackState { phase: FeedbackPhase; @@ -55,19 +48,17 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { setState(prev => ({ ...prev, message, inputError: validationError })); return; } - setState(prev => ({ ...prev, message, phase: 'screenshot-prompt', inputError: undefined })); + setState(prev => ({ ...prev, message, phase: 'screenshot', inputError: undefined })); }, [validateMessage] ); - const chooseAttachScreenshot = useCallback(() => { - setState(prev => ({ ...prev, phase: 'screenshot-path', inputError: undefined })); - }, []); - + /** Skip the screenshot — used when the user presses Esc on the path picker. */ const skipScreenshot = useCallback(() => { setState(prev => ({ ...prev, screenshotPath: undefined, phase: 'consent', inputError: undefined })); }, []); + /** Submit a screenshot path. Empty/undefined skips. Validation failures stay on the screenshot phase. */ const setScreenshot = useCallback( async (screenshotPath: string | undefined) => { const normalized = screenshotPath && screenshotPath.length > 0 ? screenshotPath : undefined; @@ -125,12 +116,10 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { const goBack = useCallback(() => { setState(prev => { switch (prev.phase) { - case 'screenshot-prompt': + case 'screenshot': return { ...prev, phase: 'message', inputError: undefined }; - case 'screenshot-path': - return { ...prev, phase: 'screenshot-prompt', inputError: undefined }; case 'consent': - return { ...prev, phase: 'screenshot-prompt', inputError: undefined }; + return { ...prev, phase: 'screenshot', inputError: undefined }; case 'error': return { ...prev, phase: 'consent', error: undefined }; default: @@ -146,7 +135,6 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { return { state, setMessage, - chooseAttachScreenshot, skipScreenshot, setScreenshot, confirmConsent, From 91cb5da82faaf7e6351e975fecefc50da724b0e7 Mon Sep 17 00:00:00 2001 From: Tejas Kashinath Date: Wed, 20 May 2026 18:51:06 -0400 Subject: [PATCH 7/7] refactor(feedback): use shared toError + extend ValidationError Per Hweinstock's review: - handleFeedback / useFeedbackFlow: replace the local `err instanceof Error ? err : new Error(String(err))` pattern with `toError(err)` from src/lib/errors/types.ts. - FeedbackValidationError: extend `ValidationError` from src/lib/errors/types.ts so the shared error classification helpers treat it as a user error, which means telemetry's classifyError(err)/isUserError(err) will set error_source: 'user' on failure metrics for invalid screenshots / empty messages. --- src/cli/commands/feedback/action.ts | 3 ++- src/cli/operations/feedback/submit-feedback.ts | 10 ++++++++-- src/cli/tui/screens/feedback/useFeedbackFlow.ts | 3 ++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/feedback/action.ts b/src/cli/commands/feedback/action.ts index 2b32251a5..bd8d2be88 100644 --- a/src/cli/commands/feedback/action.ts +++ b/src/cli/commands/feedback/action.ts @@ -1,3 +1,4 @@ +import { toError } from '../../../lib/errors/types'; import { submitFeedback } from '../../operations/feedback'; import type { FeedbackSubmissionResult } from '../../operations/feedback'; import { promptForConsent } from './consent-prompt'; @@ -23,6 +24,6 @@ export async function handleFeedback(message: string, options: FeedbackOptions): }); return { kind: 'submitted', result }; } catch (err) { - return { kind: 'error', error: err instanceof Error ? err : new Error(String(err)) }; + return { kind: 'error', error: toError(err) }; } } diff --git a/src/cli/operations/feedback/submit-feedback.ts b/src/cli/operations/feedback/submit-feedback.ts index 9b8205c05..1d570b1e4 100644 --- a/src/cli/operations/feedback/submit-feedback.ts +++ b/src/cli/operations/feedback/submit-feedback.ts @@ -1,3 +1,4 @@ +import { ValidationError } from '../../../lib/errors/types'; import { PACKAGE_VERSION } from '../../constants'; import { fetchPresignedUrl, submitForm, uploadFileToS3 } from './aperture-client'; import { buildFeedbackPayload, buildUserAgent } from './build-payload'; @@ -14,10 +15,15 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; -export class FeedbackValidationError extends Error { +/** + * Thrown for any user-supplied input the feedback command refuses to send to + * Aperture (empty/oversized message, missing/oversized/wrong-type screenshot). + * Extends the shared `ValidationError` so telemetry classifies it as a user + * error rather than a service/client failure. + */ +export class FeedbackValidationError extends ValidationError { constructor(message: string) { super(message); - this.name = 'FeedbackValidationError'; } } diff --git a/src/cli/tui/screens/feedback/useFeedbackFlow.ts b/src/cli/tui/screens/feedback/useFeedbackFlow.ts index d47665693..ca4269528 100644 --- a/src/cli/tui/screens/feedback/useFeedbackFlow.ts +++ b/src/cli/tui/screens/feedback/useFeedbackFlow.ts @@ -1,3 +1,4 @@ +import { toError } from '../../../../lib/errors/types'; import { submitFeedback, validateFeedbackMessage, validateScreenshotPath } from '../../../operations/feedback'; import type { FeedbackSubmissionResult } from '../../../operations/feedback'; import { withCommandRunTelemetry } from '../../../telemetry/cli-command-run'; @@ -92,7 +93,7 @@ export function useFeedbackFlow(options: UseFeedbackFlowOptions = {}) { }); return { success: true, submission }; } catch (err) { - return { success: false, error: err instanceof Error ? err : new Error(String(err)) }; + return { success: false, error: toError(err) }; } } );