Skip to content

Commit 94f3a55

Browse files
tejaskashtejaskash
committed
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.
1 parent 9c46424 commit 94f3a55

10 files changed

Lines changed: 265 additions & 124 deletions

File tree

‎src/cli/commands/feedback/__tests__/command.test.ts‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,24 @@ vi.mock('../../../tui/screens/feedback', () => ({
1818
FeedbackScreen: () => null,
1919
}));
2020

21+
vi.mock('../../../telemetry/cli-command-run.js', () => ({
22+
runCliCommand: async (_command: unknown, json: boolean, fn: () => Promise<unknown>) => {
23+
try {
24+
await fn();
25+
} catch (err) {
26+
const msg = err instanceof Error ? err.message : String(err);
27+
if (json) {
28+
console.log(JSON.stringify({ success: false, error: msg }));
29+
} else {
30+
console.error(msg);
31+
}
32+
process.exit(1);
33+
return;
34+
}
35+
process.exit(0);
36+
},
37+
}));
38+
2139
vi.mock('ink', () => ({
2240
render: (...args: unknown[]) => {
2341
mockRender(...args);
@@ -65,9 +83,12 @@ describe('registerFeedback', () => {
6583
it('emits success JSON when --json is supplied with a message', async () => {
6684
mockHandleFeedback.mockResolvedValue(submittedOutcome);
6785

68-
await program.parseAsync(['feedback', 'looks good', '--json'], { from: 'user' });
86+
await expect(program.parseAsync(['feedback', 'looks good', '--json'], { from: 'user' })).rejects.toThrow(
87+
'process.exit'
88+
);
6989

7090
expect(mockHandleFeedback).toHaveBeenCalledWith('looks good', expect.objectContaining({ json: true }));
91+
expect(mockExit).toHaveBeenCalledWith(0);
7192
expect(mockLog).toHaveBeenCalledTimes(1);
7293
const output = JSON.parse(mockLog.mock.calls[0]?.[0] as string);
7394
expect(output).toEqual({
@@ -90,10 +111,10 @@ describe('registerFeedback', () => {
90111
it('prints a friendly cancellation message when the user declines consent', async () => {
91112
mockHandleFeedback.mockResolvedValue({ kind: 'declined' });
92113

93-
await program.parseAsync(['feedback', 'msg'], { from: 'user' });
114+
await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit');
94115

95116
expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('Feedback cancelled.'));
96-
expect(mockExit).not.toHaveBeenCalled();
117+
expect(mockExit).toHaveBeenCalledWith(0);
97118
});
98119

99120
it('reports submission errors with exit 1 in plain mode', async () => {
@@ -102,7 +123,7 @@ describe('registerFeedback', () => {
102123
await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit');
103124

104125
expect(mockExit).toHaveBeenCalledWith(1);
105-
expect(mockRender).toHaveBeenCalled();
126+
expect(mockError).toHaveBeenCalledWith(expect.stringContaining('HTTP 500'));
106127
});
107128

108129
it('emits a JSON error envelope on submission failure when --json is set', async () => {

‎src/cli/commands/feedback/command.tsx‎

Lines changed: 39 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { getErrorMessage } from '../../errors';
1+
import { runCliCommand } from '../../telemetry/cli-command-run.js';
22
import { COMMAND_DESCRIPTIONS } from '../../tui/copy';
33
import { requireTTY } from '../../tui/guards/tty';
44
import { FeedbackScreen } from '../../tui/screens/feedback';
@@ -36,58 +36,46 @@ export const registerFeedback = (program: Command) => {
3636
return;
3737
}
3838

39-
let outcome;
40-
try {
41-
outcome = await handleFeedback(message, options);
42-
} catch (error) {
43-
const errMessage = getErrorMessage(error);
44-
if (options.json) {
45-
console.log(JSON.stringify({ success: false, error: errMessage }));
46-
} else {
47-
render(<Text color="red">Error: {errMessage}</Text>);
48-
}
49-
process.exit(1);
50-
return;
51-
}
39+
const has_screenshot = !!options.screenshot;
40+
const knownAttrs = { mode: 'cli' as const, has_screenshot };
5241

53-
if (outcome.kind === 'no-tty') {
54-
const errorText = 'Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.';
55-
if (options.json) {
56-
console.log(JSON.stringify({ success: false, error: errorText }));
57-
} else {
58-
console.error(errorText);
59-
}
60-
process.exit(1);
61-
return;
62-
}
42+
await runCliCommand(
43+
'feedback',
44+
!!options.json,
45+
async () => {
46+
const outcome = await handleFeedback(message, options);
6347

64-
if (outcome.kind === 'declined') {
65-
if (options.json) {
66-
console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' }));
67-
} else {
68-
console.log('Feedback cancelled. Nothing was submitted.');
69-
}
70-
return;
71-
}
72-
73-
if (outcome.kind === 'error') {
74-
if (options.json) {
75-
console.log(JSON.stringify({ success: false, error: outcome.error }));
76-
} else {
77-
render(<Text color="red">Error: {outcome.error}</Text>);
78-
}
79-
process.exit(1);
80-
return;
81-
}
82-
83-
const result = outcome.result;
84-
if (options.json) {
85-
console.log(
86-
JSON.stringify({ success: true, id: result.id, timestamp: result.timestamp, reference: result.reference })
87-
);
88-
return;
89-
}
48+
if (outcome.kind === 'no-tty') {
49+
throw new Error('Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.');
50+
}
51+
if (outcome.kind === 'error') {
52+
throw new Error(outcome.error);
53+
}
54+
if (outcome.kind === 'declined') {
55+
if (options.json) {
56+
console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' }));
57+
} else {
58+
console.log('Feedback cancelled. Nothing was submitted.');
59+
}
60+
return knownAttrs;
61+
}
9062

91-
render(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
63+
const result = outcome.result;
64+
if (options.json) {
65+
console.log(
66+
JSON.stringify({
67+
success: true,
68+
id: result.id,
69+
timestamp: result.timestamp,
70+
reference: result.reference,
71+
})
72+
);
73+
} else {
74+
render(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
75+
}
76+
return knownAttrs;
77+
},
78+
knownAttrs
79+
);
9280
});
9381
};

‎src/cli/operations/feedback/__tests__/submit-feedback.test.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,9 @@ describe('submitFeedback', () => {
7474
await fs.writeFile(screenshotPath, Buffer.from([0x89, 0x50, 0x4e, 0x47]));
7575

7676
fetchMock
77-
.mockResolvedValueOnce(new Response('https://s3.example/key?sig=x', { status: 200 }))
77+
.mockResolvedValueOnce(
78+
new Response('https://s3.example/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?sig=x', { status: 200 })
79+
)
7880
.mockResolvedValueOnce(emptyOk(200))
7981
.mockResolvedValueOnce(jsonResponse(successResponse));
8082

@@ -101,7 +103,7 @@ describe('submitFeedback', () => {
101103
expect(presignBody.uploadFileSHA256).toHaveLength(44);
102104

103105
const [uploadUrl, uploadInit] = callAt(1);
104-
expect(uploadUrl).toBe('https://s3.example/key?sig=x');
106+
expect(uploadUrl).toBe('https://s3.example/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?sig=x');
105107
expect(uploadInit.method).toBe('PUT');
106108
expect(uploadInit.headers['content-type']).toBe('image/png');
107109
expect(uploadInit.headers['x-amz-checksum-algorithm']).toBe('SHA256');
@@ -116,10 +118,11 @@ describe('submitFeedback', () => {
116118
pii: true,
117119
response: { responseType: 'fileUpload' },
118120
});
119-
// Object key shape: us-east-1/AgentCore/CLI/0.1.0/DDMMYYYY/<uuid>.png, wrapped in an array
121+
// Reference must be the actual S3 key parsed from the presigned URL path,
122+
// not a key fabricated client-side.
120123
const responseValue = submitBody.customerResponses[1].response.responseValue;
121124
expect(Array.isArray(responseValue)).toBe(true);
122-
expect(responseValue[0]).toMatch(/^us-east-1\/AgentCore\/CLI\/0\.1\.0\/\d{8}\/[0-9a-f-]{36}\.png$/);
125+
expect(responseValue[0]).toBe('us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png');
123126

124127
await fs.rm(tmp, { recursive: true, force: true });
125128
});

‎src/cli/operations/feedback/constants.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ export const APERTURE_LOCALE = 'en_US';
66
export const APERTURE_INGESTION_URL = 'https://ingestion.aperture-public-api.feedback.console.aws.dev/form';
77
export const APERTURE_PRESIGNED_URL_ENDPOINT =
88
'https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl';
9-
export const APERTURE_S3_REGION = 'us-east-1';
109

1110
export const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024;
1211
export const ALLOWED_SCREENSHOT_EXTENSIONS = ['.png', '.jpg', '.jpeg'] as const;

‎src/cli/operations/feedback/index.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
export { submitFeedback, FeedbackValidationError } from './submit-feedback';
1+
export {
2+
submitFeedback,
3+
FeedbackValidationError,
4+
validateFeedbackMessage,
5+
validateScreenshotPath,
6+
FEEDBACK_MESSAGE_MAX_LENGTH,
7+
} from './submit-feedback';
28
export { ApertureError } from './aperture-client';
39
export { CONSENT_TEXT } from './constants';
410
export type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types';

‎src/cli/operations/feedback/submit-feedback.ts‎

Lines changed: 45 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,10 @@ import {
66
APERTURE_FORM_CATEGORY,
77
APERTURE_FORM_NAME,
88
APERTURE_FORM_VERSION,
9-
APERTURE_S3_REGION,
109
MAX_SCREENSHOT_BYTES,
1110
} from './constants';
1211
import type { FeedbackSubmissionResult, SubmitFeedbackInput } from './types';
13-
import { createHash, randomUUID } from 'node:crypto';
12+
import { createHash } from 'node:crypto';
1413
import * as fs from 'node:fs/promises';
1514
import * as path from 'node:path';
1615

@@ -26,20 +25,49 @@ function contentTypeForExtension(ext: string): string {
2625
return 'image/jpeg';
2726
}
2827

29-
function validateMessage(message: string): void {
28+
export const FEEDBACK_MESSAGE_MAX_LENGTH = 1000;
29+
30+
/**
31+
* Synchronous message validator. Returns null when valid, an error message
32+
* string otherwise. Reused by the TUI hook so users see errors at input time
33+
* rather than after walking through consent.
34+
*/
35+
export function validateFeedbackMessage(message: string): string | null {
3036
const trimmed = message.trim();
3137
if (!trimmed) {
32-
throw new FeedbackValidationError('Feedback message cannot be empty.');
38+
return 'Feedback message cannot be empty.';
39+
}
40+
if (trimmed.length > FEEDBACK_MESSAGE_MAX_LENGTH) {
41+
return `Feedback message must be ${FEEDBACK_MESSAGE_MAX_LENGTH} characters or fewer.`;
3342
}
34-
if (trimmed.length > 1000) {
35-
throw new FeedbackValidationError('Feedback message must be 1000 characters or fewer.');
43+
return null;
44+
}
45+
46+
function validateMessage(message: string): void {
47+
const error = validateFeedbackMessage(message);
48+
if (error) {
49+
throw new FeedbackValidationError(error);
50+
}
51+
}
52+
53+
/**
54+
* Async screenshot validator that reads, size-checks, and extension-checks the
55+
* file. Returns null on success. Reused by the TUI hook so the user sees an
56+
* error on the path-input screen rather than after consent.
57+
*/
58+
export async function validateScreenshotPath(filePath: string): Promise<string | null> {
59+
try {
60+
await loadAndValidateScreenshot(filePath);
61+
return null;
62+
} catch (err) {
63+
if (err instanceof FeedbackValidationError) return err.message;
64+
throw err;
3665
}
3766
}
3867

3968
interface LoadedScreenshot {
4069
buffer: Uint8Array;
4170
fileName: string;
42-
extension: string;
4371
contentType: string;
4472
sha256Base64: string;
4573
size: number;
@@ -67,19 +95,21 @@ async function loadAndValidateScreenshot(filePath: string): Promise<LoadedScreen
6795
return {
6896
buffer: new Uint8Array(buffer),
6997
fileName: path.basename(filePath),
70-
extension: ext.replace(/^\./, ''),
7198
contentType: contentTypeForExtension(ext),
7299
sha256Base64: createHash('sha256').update(buffer).digest('base64'),
73100
size: buffer.byteLength,
74101
};
75102
}
76103

77-
function buildAttachmentObjectKey(extension: string, now = new Date()): string {
78-
const day = now.getUTCDate().toString().padStart(2, '0');
79-
const month = (now.getUTCMonth() + 1).toString().padStart(2, '0');
80-
const year = now.getUTCFullYear().toString();
81-
const datePart = `${day}${month}${year}`;
82-
return `${APERTURE_S3_REGION}/${APERTURE_FORM_CATEGORY}/${APERTURE_FORM_NAME}/${APERTURE_FORM_VERSION}/${datePart}/${randomUUID()}.${extension}`;
104+
/**
105+
* The presigned URL's path is the actual S3 object key. The form payload
106+
* must reference exactly that key — fabricating one client-side risks
107+
* pointing at an object that doesn't exist if Aperture's bucket layout,
108+
* region, or naming convention shifts.
109+
*/
110+
function objectKeyFromPresignedUrl(presignedUrl: string): string {
111+
const url = new URL(presignedUrl);
112+
return decodeURIComponent(url.pathname.replace(/^\/+/, ''));
83113
}
84114

85115
export async function submitFeedback(input: SubmitFeedbackInput): Promise<FeedbackSubmissionResult> {
@@ -102,7 +132,7 @@ export async function submitFeedback(input: SubmitFeedbackInput): Promise<Feedba
102132
userAgent
103133
);
104134
await uploadFileToS3(presignedUrl, file.buffer, file.contentType, file.sha256Base64, userAgent);
105-
screenshotReference = buildAttachmentObjectKey(file.extension);
135+
screenshotReference = objectKeyFromPresignedUrl(presignedUrl);
106136
}
107137

108138
const payload = buildFeedbackPayload({

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
GatewayTargetHost,
1919
GatewayTargetType,
2020
MemoryType,
21+
Mode,
2122
ModelProvider,
2223
NetworkMode,
2324
OutboundAuthType,
@@ -138,6 +139,11 @@ const FetchAccessAttrs = safeSchema({ resource_type: ResourceType });
138139

139140
const UpdateAttrs = safeSchema({ is_dry_run: z.boolean() });
140141

142+
const FeedbackAttrs = safeSchema({
143+
mode: Mode,
144+
has_screenshot: z.boolean(),
145+
});
146+
141147
const PauseResumeOnlineEvalAttrs = safeSchema({ ref_type: RefType });
142148

143149
const NoAttrs = safeSchema({});
@@ -166,6 +172,7 @@ export const COMMAND_SCHEMAS = {
166172
'logs.evals': LogsEvalsAttrs,
167173
'run.eval': RunEvalAttrs,
168174
'fetch.access': FetchAccessAttrs,
175+
feedback: FeedbackAttrs,
169176
update: UpdateAttrs,
170177
'pause.online-eval': PauseResumeOnlineEvalAttrs,
171178
'resume.online-eval': PauseResumeOnlineEvalAttrs,

‎src/cli/tui/screens/feedback/FeedbackScreen.tsx‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp
8080
onCancel={onExit}
8181
expandable
8282
/>
83+
{state.inputError && <Text color="red">{state.inputError}</Text>}
8384
<Text dimColor>Enter to continue · Esc to exit</Text>
8485
</Panel>
8586
</ScreenLayout>
@@ -107,9 +108,10 @@ export function FeedbackScreen({ initialScreenshot, onExit }: FeedbackScreenProp
107108
<PathInput
108109
initialValue={state.screenshotPath ?? ''}
109110
placeholder="Path to .png or .jpg"
110-
onSubmit={value => setScreenshot(value.trim() || undefined)}
111+
onSubmit={value => void setScreenshot(value.trim() || undefined)}
111112
onCancel={goBack}
112113
/>
114+
{state.inputError && <Text color="red">{state.inputError}</Text>}
113115
<Text dimColor>↑↓ navigate · → open dir · Enter select file · Esc to go back</Text>
114116
</Panel>
115117
</ScreenLayout>

0 commit comments

Comments
 (0)