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.

Author: tejaskash

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<typeof vi.spyOn>;
   let mockLog: ReturnType<typeof vi.spyOn>;
   let mockError: ReturnType<typeof vi.spyOn>;
@@ -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,17 +70,27 @@ 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,
       id: 'sub-1',
       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 () => {

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(<Text color="red">Error: {errMessage}</Text>);
-        }
-        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(<Text color="red">Error: {outcome.error}</Text>);
-        }
-        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(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
+          const result = outcome.result;
+          if (options.json) {
+            console.log(
+              JSON.stringify({
+                success: true,
+                id: result.id,
+                timestamp: result.timestamp,
+                reference: result.reference,
+              })
+            );
+          } else {
+            render(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
+          }
+          return knownAttrs;
+        },
+        knownAttrs
+      );
     });
 };

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/<uuid>.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 });
   });

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;

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';

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<string | null> {
+  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<LoadedScreen
   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}`;
+/**
+ * The presigned URL's path is the actual S3 object key. The form payload
+ * must reference exactly that key — fabricating one client-side risks
+ * pointing at an object that doesn't exist if Aperture's bucket layout,
+ * region, or naming convention shifts.
+ */
+function objectKeyFromPresignedUrl(presignedUrl: string): string {
+  const url = new URL(presignedUrl);
+  return decodeURIComponent(url.pathname.replace(/^\/+/, ''));
 }
 
 export async function submitFeedback(input: SubmitFeedbackInput): Promise<FeedbackSubmissionResult> {
@@ -102,7 +132,7 @@ export async function submitFeedback(input: SubmitFeedbackInput): Promise<Feedba
       userAgent
     );
     await uploadFileToS3(presignedUrl, file.buffer, file.contentType, file.sha256Base64, userAgent);
-    screenshotReference = buildAttachmentObjectKey(file.extension);
+    screenshotReference = objectKeyFromPresignedUrl(presignedUrl);
   }
 
   const payload = buildFeedbackPayload({

src/cli/telemetry/schemas/command-run.ts

@@ -18,6 +18,7 @@ import {
   GatewayTargetHost,
   GatewayTargetType,
   MemoryType,
+  Mode,
   ModelProvider,
   NetworkMode,
   OutboundAuthType,
@@ -138,6 +139,11 @@ const FetchAccessAttrs = safeSchema({ resource_type: ResourceType });
 
 const UpdateAttrs = safeSchema({ is_dry_run: z.boolean() });
 
+const FeedbackAttrs = safeSchema({
+  mode: Mode,
+  has_screenshot: z.boolean(),
+});
+
 const PauseResumeOnlineEvalAttrs = safeSchema({ ref_type: RefType });
 
 const NoAttrs = safeSchema({});
@@ -166,6 +172,7 @@ export const COMMAND_SCHEMAS = {
   'logs.evals': LogsEvalsAttrs,
   'run.eval': RunEvalAttrs,
   'fetch.access': FetchAccessAttrs,
+  feedback: FeedbackAttrs,
   update: UpdateAttrs,
   'pause.online-eval': PauseResumeOnlineEvalAttrs,
   'resume.online-eval': PauseResumeOnlineEvalAttrs,