Skip to content

Commit ab65255

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 973bbd5 commit ab65255

10 files changed

Lines changed: 263 additions & 114 deletions

File tree

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

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

21+
vi.mock('../../../telemetry', () => {
22+
const CANCELLED = Symbol('cancelled');
23+
return {
24+
CANCELLED,
25+
TelemetryClientAccessor: {
26+
get: () =>
27+
Promise.resolve({
28+
withCommandRun: async (_command: unknown, fn: () => Promise<unknown>, _knownAttrs?: unknown) => {
29+
// Mirror the real client: bubble exceptions out so command.tsx's catch handles output.
30+
await fn();
31+
},
32+
}),
33+
},
34+
};
35+
});
36+
2137
vi.mock('ink', () => ({
2238
render: (...args: unknown[]) => {
2339
mockRender(...args);
@@ -102,7 +118,7 @@ describe('registerFeedback', () => {
102118
await expect(program.parseAsync(['feedback', 'msg'], { from: 'user' })).rejects.toThrow('process.exit');
103119

104120
expect(mockExit).toHaveBeenCalledWith(1);
105-
expect(mockRender).toHaveBeenCalled();
121+
expect(mockError).toHaveBeenCalledWith(expect.stringContaining('HTTP 500'));
106122
});
107123

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

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

Lines changed: 43 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { getErrorMessage } from '../../errors';
2+
import { CANCELLED, TelemetryClientAccessor } from '../../telemetry';
23
import { COMMAND_DESCRIPTIONS } from '../../tui/copy';
34
import { requireTTY } from '../../tui/guards/tty';
45
import { FeedbackScreen } from '../../tui/screens/feedback';
@@ -36,58 +37,56 @@ export const registerFeedback = (program: Command) => {
3637
return;
3738
}
3839

39-
let outcome;
40+
const has_screenshot = !!options.screenshot;
41+
const knownAttrs = { mode: 'cli' as const, has_screenshot };
42+
const client = await TelemetryClientAccessor.get();
43+
4044
try {
41-
outcome = await handleFeedback(message, options);
45+
await client.withCommandRun(
46+
'feedback',
47+
async () => {
48+
const outcome = await handleFeedback(message, options);
49+
50+
if (outcome.kind === 'no-tty') {
51+
throw new Error('Feedback consent must be confirmed interactively. Re-run agentcore feedback in a TTY.');
52+
}
53+
if (outcome.kind === 'error') {
54+
throw new Error(outcome.error);
55+
}
56+
if (outcome.kind === 'declined') {
57+
if (options.json) {
58+
console.log(JSON.stringify({ success: false, error: 'Feedback cancelled.' }));
59+
} else {
60+
console.log('Feedback cancelled. Nothing was submitted.');
61+
}
62+
return CANCELLED;
63+
}
64+
65+
const result = outcome.result;
66+
if (options.json) {
67+
console.log(
68+
JSON.stringify({
69+
success: true,
70+
id: result.id,
71+
timestamp: result.timestamp,
72+
reference: result.reference,
73+
})
74+
);
75+
} else {
76+
render(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
77+
}
78+
return knownAttrs;
79+
},
80+
knownAttrs
81+
);
4282
} catch (error) {
4383
const errMessage = getErrorMessage(error);
4484
if (options.json) {
4585
console.log(JSON.stringify({ success: false, error: errMessage }));
4686
} else {
47-
render(<Text color="red">Error: {errMessage}</Text>);
48-
}
49-
process.exit(1);
50-
return;
51-
}
52-
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-
}
63-
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>);
87+
console.error(errMessage);
7888
}
7989
process.exit(1);
80-
return;
8190
}
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-
}
90-
91-
render(<Text color="green">Thank you. Your feedback has been submitted (id: {result.id}).</Text>);
9291
});
9392
};

‎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: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
Language,
1717
Level,
1818
Memory,
19+
Mode,
1920
ModelProvider,
2021
NetworkMode,
2122
OutboundAuth,
@@ -133,6 +134,11 @@ const RunEvalAttrs = safeSchema({
133134

134135
const FetchAccessAttrs = safeSchema({ resource_type: ResourceType });
135136

137+
const FeedbackAttrs = safeSchema({
138+
mode: Mode,
139+
has_screenshot: z.boolean(),
140+
});
141+
136142
const UpdateAttrs = safeSchema({ check_only: z.boolean() });
137143

138144
const PauseResumeOnlineEvalAttrs = safeSchema({ ref_type: RefType });
@@ -177,6 +183,9 @@ export const COMMAND_SCHEMAS = {
177183
// fetch
178184
'fetch.access': FetchAccessAttrs,
179185

186+
// feedback
187+
feedback: FeedbackAttrs,
188+
180189
// update
181190
update: UpdateAttrs,
182191

‎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)