From 6dbebaf7423e30fa7c425ead570d1b5eafa0f0ec Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:09:11 +0000 Subject: [PATCH 01/14] feat(feedback): port the feedback command to the refactor architecture Adds `agentcore feedback [--screenshot ] [--yes]`, which submits to the Aperture public feedback API. Consent for the AWS Customer Agreement uses the project/remove imperative pattern: a readline y/N prompt on a TTY, --yes to accept non-interactively, and a hard failure (not a silent submit) when neither a TTY nor --yes is present. Screenshot attachments go presign -> S3 PUT (SHA256 checksum + NOT_SCANNED tag) -> form POST, referencing the object key parsed from the presigned URL. - src/core/feedback.tsx: FeedbackClient (injected fetch) + ApertureError (ERROR_SOURCE.SERVICE) + payload/validation ported from the pre-refactor CLI - src/handlers/feedback/: leaf handler with inline consent + types + flow tests - wired onto Core, CoreClient, the root handler, and TestCoreClient --- src/core/feedback.tsx | 326 ++++++++++++++++++++++++ src/core/index.tsx | 4 + src/handlers/feedback/feedback.test.tsx | 156 ++++++++++++ src/handlers/feedback/index.tsx | 77 ++++++ src/handlers/feedback/types.tsx | 26 ++ src/handlers/index.tsx | 2 + src/handlers/types.tsx | 2 + src/testing/TestCoreClient.tsx | 36 +++ 8 files changed, 629 insertions(+) create mode 100644 src/core/feedback.tsx create mode 100644 src/handlers/feedback/feedback.test.tsx create mode 100644 src/handlers/feedback/index.tsx create mode 100644 src/handlers/feedback/types.tsx diff --git a/src/core/feedback.tsx b/src/core/feedback.tsx new file mode 100644 index 000000000..b2c3c24b9 --- /dev/null +++ b/src/core/feedback.tsx @@ -0,0 +1,326 @@ +import { createHash } from "node:crypto"; +import { stat, readFile } from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { AgentCoreCLIError, ERROR_SOURCE, InputValidationError } from "../errors"; +import { PACKAGE_VERSION } from "../constants"; +import type { AwsClients, CoreFetch, CoreOptions } from "./types"; +import type { + CoreFeedbackClient, + FeedbackSubmissionResult, + SubmitFeedbackInput, +} from "../handlers/feedback/types"; + +// Aperture public feedback API. These are commercial-partition (.aws.dev) endpoints +// with no partition variant, so feedback is unavailable in GovCloud/China — carried +// over from the pre-refactor CLI, flagged here rather than silently. +const INGESTION_URL = "https://ingestion.aperture-public-api.feedback.console.aws.dev/form"; +const PRESIGN_URL = + "https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl"; +const FORM_CATEGORY = "AgentCore"; +const FORM_NAME = "CLI"; +const FORM_VERSION = "0.1.0"; +const LOCALE = "en_US"; +const REFERENCE = "agentcore-cli"; +const MESSAGE_QUESTION = "What feedback do you have for the AgentCore CLI"; +const ATTACHMENT_QUESTION = "Attachments"; +const MESSAGE_MAX_LENGTH = 1000; +const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; +const ALLOWED_SCREENSHOT_EXTENSIONS = [".png", ".jpg", ".jpeg"] as const; + +// Rendered by the feedback command's consent prompt before every submission. +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."; + +// Extends the CLI error hierarchy (the pre-refactor ApertureError extended plain +// Error, so telemetry classified it as unknown) so failures record error_source=service. +export class ApertureError extends AgentCoreCLIError { + constructor( + message: string, + readonly status?: number, + readonly body?: string, + ) { + super(message, { source: ERROR_SOURCE.SERVICE, name: "ApertureError" }); + } +} + +interface LoadedScreenshot { + buffer: Uint8Array; + fileName: string; + contentType: string; + sha256Base64: string; + size: number; +} + +interface ApertureCustomerResponse { + question: string; + pii: boolean; + response: + | { responseType: "textArea"; responseValue: string } + | { responseType: "fileUpload"; responseValue: string[] }; +} + +interface ApertureFormPayload { + category: string; + name: string; + version: string; + locale: string; + reference: string; + location: string; + customerResponses: ApertureCustomerResponse[]; + metadataList: { key: string; value: string }[]; +} + +export class FeedbackClient implements CoreFeedbackClient { + constructor( + private readonly clients: AwsClients, + private readonly fetch: CoreFetch, + ) {} + + async submitFeedback( + input: SubmitFeedbackInput, + _options: CoreOptions, + ): Promise { + const message = input.message.trim(); + if (!message) { + throw new InputValidationError("Feedback message cannot be empty."); + } + if (message.length > MESSAGE_MAX_LENGTH) { + throw new InputValidationError( + `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, + ); + } + + const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; + + let screenshotReference: string | undefined; + if (input.screenshot) { + const file = await this.loadScreenshot(input.screenshot.path); + const presignedUrl = await this.fetchPresignedUrl( + { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + fileName: file.fileName, + fileSize: file.size, + uploadFileSHA256: file.sha256Base64, + }, + userAgent, + ); + await this.uploadFileToS3( + presignedUrl, + file.buffer, + file.contentType, + file.sha256Base64, + userAgent, + ); + screenshotReference = objectKeyFromPresignedUrl(presignedUrl); + } + + const payload = buildFeedbackPayload({ message, screenshotReference }); + const response = await this.submitForm(payload, userAgent); + return { + id: response.id, + timestamp: response.timestamp, + reference: response.reference, + }; + } + + // Aperture returns the presigned URL as a plain-text body (not JSON). + private async fetchPresignedUrl( + request: { + category: string; + name: string; + version: string; + fileName: string; + fileSize: number; + uploadFileSHA256: string; + }, + userAgent: string, + ): Promise { + const response = await this.fetch(PRESIGN_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(request), + }); + if (!response.ok) { + throw new ApertureError( + `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, + response.status, + await readBody(response), + ); + } + return (await response.text()).trim(); + } + + // Aperture's bucket policy requires the SHA-256 checksum headers and a tag + // marking the object as not yet AV-scanned; omitting either is rejected. + private async uploadFileToS3( + presignedUrl: string, + fileBuffer: Uint8Array, + contentType: string, + base64Sha256: string, + userAgent: string, + ): Promise { + const response = await this.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) { + throw new ApertureError( + `Failed to upload screenshot (HTTP ${response.status}).`, + response.status, + await readBody(response), + ); + } + } + + private async submitForm( + payload: ApertureFormPayload, + userAgent: string, + ): Promise { + const response = await this.fetch(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), response.status, body); + } + return (await response.json()) as FeedbackSubmissionResult; + } + + private async loadScreenshot(rawFilePath: string): Promise { + const filePath = expandTilde(rawFilePath); + + let stats: Awaited>; + try { + stats = await stat(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (stats.isDirectory()) { + throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); + } + if (!stats.isFile()) { + throw new InputValidationError(`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 InputValidationError( + `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, + ); + } + + let buffer: Buffer; + try { + buffer = await readFile(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (buffer.byteLength > MAX_SCREENSHOT_BYTES) { + const sizeMb = (buffer.byteLength / (1024 * 1024)).toFixed(1); + throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } + + return { + buffer: new Uint8Array(buffer), + fileName: path.basename(filePath), + contentType: ext === ".png" ? "image/png" : "image/jpeg", + sha256Base64: createHash("sha256").update(buffer).digest("base64"), + size: buffer.byteLength, + }; + } +} + +// Expand a leading ~ / ~/... to $HOME. Node's fs APIs don't expand tildes (the +// shell normally does), so a quoted path like "~/shot.png" would otherwise ENOENT. +function expandTilde(filePath: string): string { + if (filePath === "~") return os.homedir(); + if (filePath.startsWith("~/")) return path.join(os.homedir(), filePath.slice(2)); + return filePath; +} + +// The presigned URL's path IS the S3 object key the form must reference; +// fabricating one client-side risks pointing at a nonexistent object if +// Aperture's bucket layout or region shifts. +function objectKeyFromPresignedUrl(presignedUrl: string): string { + return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); +} + +function buildFeedbackPayload(input: { + message: string; + screenshotReference?: string; +}): ApertureFormPayload { + const customerResponses: ApertureCustomerResponse[] = [ + { + question: MESSAGE_QUESTION, + pii: false, + response: { responseType: "textArea", responseValue: input.message }, + }, + ]; + if (input.screenshotReference) { + customerResponses.push({ + question: ATTACHMENT_QUESTION, + pii: true, + response: { responseType: "fileUpload", responseValue: [input.screenshotReference] }, + }); + } + + // Aperture rejects unknown metadata keys with HTTP 400; only cli-version and os + // are registered in the form template, so node version + mode ride in `location`. + return { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + locale: LOCALE, + reference: REFERENCE, + location: `agentcore-cli@${PACKAGE_VERSION} (${process.platform}; node ${process.version}; cli)`, + customerResponses, + metadataList: [ + { key: "cli-version", value: PACKAGE_VERSION }, + { key: "os", value: `${process.platform} ${os.release()}` }, + ], + }; +} + +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}.`; + } +} + +async function readBody(response: Response): Promise { + try { + return await response.text(); + } catch { + return ""; + } +} diff --git a/src/core/index.tsx b/src/core/index.tsx index 5a6bab9fb..95060bcfe 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -3,6 +3,7 @@ import { BedrockAgentCoreClient } from "@aws-sdk/client-bedrock-agentcore"; import { IAMClient } from "@aws-sdk/client-iam"; import { CloudWatchLogsClient } from "@aws-sdk/client-cloudwatch-logs"; import { EvalClient } from "./eval"; +import { FeedbackClient } from "./feedback"; import { GatewayClient } from "./gateway"; import { HarnessClient } from "./harness"; import { IdentityClient } from "./identity"; @@ -72,6 +73,7 @@ export class CoreClient implements AwsClients { readonly runtime: RuntimeClient; readonly gateway: GatewayClient; readonly eval: EvalClient; + readonly feedback: FeedbackClient; readonly observability: ObservabilityClient; readonly projectManager: ProjectManager; @@ -86,6 +88,8 @@ export class CoreClient implements AwsClients { const fetch = config.fetch ?? globalThis.fetch; this.runtime = new RuntimeClient(this, fetch, this.logger.child({ module: "runtime" })); this.gateway = new GatewayClient(this, fetch, this.logger.child({ module: "gateway" })); + // Feedback posts to the Aperture public API via the injected fetch, outside the SDK seam. + this.feedback = new FeedbackClient(this, fetch); // EvalClient shares the injected fetch: dataset content is served from a // presigned S3 URL, outside the SDK seam the other operations use. The logger // is used for batch-evaluation result-log diagnostics. diff --git a/src/handlers/feedback/feedback.test.tsx b/src/handlers/feedback/feedback.test.tsx new file mode 100644 index 000000000..144785a65 --- /dev/null +++ b/src/handlers/feedback/feedback.test.tsx @@ -0,0 +1,156 @@ +import { test, expect, describe } from "bun:test"; +import { CoreClient } from "../../core"; +import { createRootHandler } from "../index"; +import { + createSilentLogger, + fixtureFactories, + TestGlobalConfigAccessor, + testIO, + type TestIOOptions, +} from "../../testing"; +import { InputValidationError, UserCancellationError } from "../../errors"; +import { join } from "node:path"; +import { mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; + +// Command-flow tests for `agentcore feedback`. The Aperture POST/PUT calls go +// through an injected `fetch` stub (feedback is the one Core path outside the SDK +// seam), so these run offline and assert the exact HTTP the client makes. + +const REGION = "us-east-1"; +const FIXTURES = join(import.meta.dir, "__fixtures__"); + +interface Recorded { + url: string; + method: string; + headers: Record; + body: unknown; +} + +// stubFetch answers the three Aperture calls (presign POST, S3 PUT, form POST) +// and records each so tests can assert the request shape. +function stubFetch(calls: Recorded[]) { + const presignedUrl = + "https://aperture-bucket.s3.us-east-1.amazonaws.com/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?X-Amz-Signature=sig"; + return (async (input: Parameters[0], init?: Parameters[1]) => { + const url = String(input); + calls.push({ + url, + method: init?.method ?? "GET", + headers: (init?.headers as Record) ?? {}, + body: init?.body, + }); + if (url.includes("/presignedurl")) { + return new Response(presignedUrl, { status: 200 }); + } + if (url.includes("/form")) { + return new Response( + JSON.stringify({ + id: "11111111-2222-3333-4444-555555555555", + timestamp: "2026-08-31T00:00:00Z", + reference: "agentcore-cli", + }), + { status: 200, headers: { "content-type": "application/json" } }, + ); + } + return new Response(null, { status: 200 }); // the S3 PUT + }) as unknown as typeof fetch; +} + +async function run( + args: string[], + opts: { io?: TestIOOptions; calls?: Recorded[] } = {}, +): Promise<{ stdout: string; stderr: string }> { + const factories = fixtureFactories(FIXTURES); + const core = new CoreClient({ + ...factories, + logger: createSilentLogger(), + fetch: stubFetch(opts.calls ?? []), + }); + const io = testIO(opts.io); + const root = createRootHandler(core, { + io: io.io, + logger: createSilentLogger(), + globalConfigAccessor: new TestGlobalConfigAccessor(), + }); + await root.route(["node", "agentcore", "feedback", ...args, "--region", REGION]); + return { stdout: io.stdout(), stderr: io.stderr() }; +} + +describe("feedback", () => { + test("--yes --json submits and prints the result envelope", async () => { + const calls: Recorded[] = []; + const { stdout } = await run(["great tool", "--yes", "--json"], { calls }); + const parsed = JSON.parse(stdout); + expect(parsed.success).toBe(true); + expect(parsed.id).toBe("11111111-2222-3333-4444-555555555555"); + // Text-only: exactly one call, the form POST. + expect(calls).toHaveLength(1); + expect(calls[0]!.url).toContain("/form"); + expect(calls[0]!.method).toBe("POST"); + }); + + test("prompts on a TTY and submits on 'y'", async () => { + const calls: Recorded[] = []; + const { stderr } = await run(["nice cli"], { io: { isTTY: true, stdin: "y\n" }, calls }); + expect(stderr).toContain("AWS Customer Agreement"); + expect(stderr).toContain("Submit feedback? (y/N)"); + expect(calls).toHaveLength(1); + }); + + test("declining the prompt cancels without submitting", async () => { + const calls: Recorded[] = []; + await expect( + run(["nope"], { io: { isTTY: true, stdin: "n\n" }, calls }), + ).rejects.toBeInstanceOf(UserCancellationError); + expect(calls).toHaveLength(0); + }); + + test("non-interactive without --yes fails and does not submit", async () => { + const calls: Recorded[] = []; + const removal = run(["headless"], { calls }); + await expect(removal).rejects.toBeInstanceOf(InputValidationError); + await expect(run(["headless"], { calls: [] })).rejects.toThrow(/--yes/); + expect(calls).toHaveLength(0); + }); + + test("an empty message is rejected before any network call", async () => { + const calls: Recorded[] = []; + await expect(run([" ", "--yes"], { calls })).rejects.toThrow(/cannot be empty/); + expect(calls).toHaveLength(0); + }); + + test("a message over 1000 chars is rejected", async () => { + const calls: Recorded[] = []; + await expect(run(["x".repeat(1001), "--yes"], { calls })).rejects.toThrow(/1000 characters/); + expect(calls).toHaveLength(0); + }); + + test("a screenshot drives presign -> S3 PUT (checksum + tag) -> form POST", async () => { + const dir = await mkdtemp(join(tmpdir(), "agentcore-fb-")); + const shot = join(dir, "shot.png"); + await writeFile(shot, Buffer.from("iVBORw0KGgoAAAANSUhEUgAA", "base64")); + + const calls: Recorded[] = []; + const { stdout } = await run(["with shot", "--screenshot", shot, "--yes", "--json"], { calls }); + expect(JSON.parse(stdout).success).toBe(true); + + expect( + calls.map( + (c) => + `${c.method} ${c.url.includes("/presignedurl") ? "presign" : c.url.includes("/form") ? "form" : "s3"}`, + ), + ).toEqual(["POST presign", "PUT s3", "POST form"]); + const put = calls[1]!; + expect(put.headers["x-amz-checksum-algorithm"]).toBe("SHA256"); + expect(put.headers["x-amz-tagging"]).toBe("scanstatus=NOT_SCANNED"); + // The form references the exact object key parsed from the presigned URL path. + const form = JSON.parse(String(calls[2]!.body)); + const attachment = form.customerResponses.find( + (r: { response: { responseType: string } }) => r.response.responseType === "fileUpload", + ); + expect(attachment.response.responseValue).toEqual([ + "us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png", + ]); + }); +}); diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx new file mode 100644 index 000000000..1bcfacc5b --- /dev/null +++ b/src/handlers/feedback/index.tsx @@ -0,0 +1,77 @@ +import { createInterface } from "node:readline/promises"; +import z from "zod"; +import { argument, createHandler, flag } from "../../router"; +import { JsonRendererKey } from "../../tui"; +import { InputValidationError, UserCancellationError } from "../../errors"; +import { coreOptsFromCtx } from "../utils.tsx"; +import { JsonKey } from "../keys.tsx"; +import { CONSENT_TEXT } from "../../core/feedback"; +import type { Core } from "../types.tsx"; +import type { AppIO } from "../../io"; + +export const createFeedbackHandler = (core: Core, io: AppIO) => + createHandler({ + name: "feedback", + description: "Send feedback about the AgentCore CLI to the team.", + arguments: [argument("message", "the feedback message to send", z.string().max(1000))], + flags: [ + flag( + "screenshot", + "path to a PNG or JPG screenshot to attach (max 100MB)", + z.string().optional(), + ), + flag( + "yes", + "accept the AWS Customer Agreement and skip the consent prompt", + z.boolean().default(false), + ), + ], + handle: async (ctx, flags, args) => { + await confirmConsent(io, ctx.require(JsonKey), flags.yes); + + const result = await core.feedback.submitFeedback( + { + message: args["message"], + screenshot: flags["screenshot"] ? { path: flags["screenshot"] } : undefined, + }, + coreOptsFromCtx(ctx), + ); + + ctx.require(JsonRendererKey).renderJson({ success: true, ...result }); + }, + }); + +// Mirrors project/remove's confirmRemoveAll: --yes bypasses the prompt, a +// non-interactive session (or --json) fails rather than submitting without +// consent, and a decline (or SIGINT) raises UserCancellationError. +async function confirmConsent(io: AppIO, jsonOutput: boolean, confirmed: boolean): Promise { + if (confirmed) return; + const canPrompt = !jsonOutput && io.stdin.isTTY && io.stdout.isTTY && io.stderr.isTTY; + if (!canPrompt) { + throw new InputValidationError( + "submitting feedback requires accepting the AWS Customer Agreement; re-run with --yes to confirm non-interactively", + ); + } + if (!(await promptForConsent(io))) { + throw new UserCancellationError(); + } +} + +async function promptForConsent(io: AppIO): Promise { + // Prompt on stderr so --json / piped stdout stays a clean machine-readable stream. + const readline = createInterface({ input: io.stdin, output: io.stderr }); + try { + const cancelled = new Promise((_resolve, reject) => { + const cancel = () => reject(new UserCancellationError()); + readline.once("SIGINT", cancel); + readline.once("close", cancel); + }); + const answer = await Promise.race([ + readline.question(`\n${CONSENT_TEXT}\n\nSubmit feedback? (y/N) `), + cancelled, + ]); + return /^(?:y|yes)$/i.test(answer.trim()); + } finally { + readline.close(); + } +} diff --git a/src/handlers/feedback/types.tsx b/src/handlers/feedback/types.tsx new file mode 100644 index 000000000..f5536cdd9 --- /dev/null +++ b/src/handlers/feedback/types.tsx @@ -0,0 +1,26 @@ +import type { CoreOptions } from "../../core/types"; + +export interface ScreenshotInput { + path: string; +} + +export interface SubmitFeedbackInput { + message: string; + screenshot?: ScreenshotInput; +} + +export interface FeedbackSubmissionResult { + id: string; + timestamp: string; + reference: string; +} + +// Consumer-defined interface (dependency inversion): the handler depends on this, +// src/core/feedback.tsx implements it. Message/screenshot validation happens inside +// submitFeedback so the one code path guards every caller. +export interface CoreFeedbackClient { + submitFeedback( + input: SubmitFeedbackInput, + options: CoreOptions, + ): Promise; +} diff --git a/src/handlers/index.tsx b/src/handlers/index.tsx index a2ff49e03..cd03e75f2 100644 --- a/src/handlers/index.tsx +++ b/src/handlers/index.tsx @@ -1,5 +1,6 @@ import { Router } from "../router"; import { createEvalHandler } from "./eval/index.tsx"; +import { createFeedbackHandler } from "./feedback/index.tsx"; import { createGatewayHandler } from "./gateway/index.tsx"; import { createHarnessHandler } from "./harness/index.tsx"; import { createIdentityHandler } from "./identity/index.tsx"; @@ -53,6 +54,7 @@ export function createRootHandler(core: Core, config: RootHandlerConfig): Router root.handler(createMemoryHandler(core, io)); root.handler(createGatewayHandler(core, io)); root.handler(createEvalHandler(core, io)); + root.handler(createFeedbackHandler(core, io)); root.handler(createConfigHandler()); root.handler(createProjectHandler({ core, io })); diff --git a/src/handlers/types.tsx b/src/handlers/types.tsx index 3d76827b8..ccf802421 100644 --- a/src/handlers/types.tsx +++ b/src/handlers/types.tsx @@ -1,4 +1,5 @@ import type { CoreEvalClient } from "./eval/types.tsx"; +import type { CoreFeedbackClient } from "./feedback/types.tsx"; import type { CoreGatewayClient } from "./gateway/types.tsx"; import type { CoreHarnessClient } from "./harness/types.tsx"; import type { CoreIdentityClient } from "./identity/types.tsx"; @@ -15,6 +16,7 @@ export interface Core { runtime: CoreRuntimeClient; gateway: CoreGatewayClient; eval: CoreEvalClient; + feedback: CoreFeedbackClient; observability: CoreObservabilityClient; projectManager: ProjectManager; /** Describes a Bedrock Agent + alias for `--type import`. */ diff --git a/src/testing/TestCoreClient.tsx b/src/testing/TestCoreClient.tsx index a6da20a9e..b4833c57b 100644 --- a/src/testing/TestCoreClient.tsx +++ b/src/testing/TestCoreClient.tsx @@ -126,6 +126,11 @@ import type { UpdateOauth2CredentialProviderInput, } from "../handlers/identity/types"; import type { CoreMemoryClient } from "../handlers/memory/types"; +import type { + CoreFeedbackClient, + FeedbackSubmissionResult, + SubmitFeedbackInput, +} from "../handlers/feedback/types"; import type { CoreObservabilityClient, CoreRuntimeClient, @@ -2297,6 +2302,36 @@ export class TestObservabilityClient implements CoreObservabilityClient { } } +// TestFeedbackClient is the feedback sub-client of TestCoreClient. +export class TestFeedbackClient implements CoreFeedbackClient { + readonly calls: RecordedCall[] = []; + private response: FeedbackSubmissionResult = { + id: "feedback-test-id", + timestamp: "2026-01-01T00:00:00Z", + reference: "agentcore-cli", + }; + private error?: Error; + + setSubmitResponse(response: FeedbackSubmissionResult): this { + this.response = response; + return this; + } + + setError(error: Error | undefined): this { + this.error = error; + return this; + } + + async submitFeedback( + input: SubmitFeedbackInput, + options: CoreOptions, + ): Promise { + this.calls.push({ method: "submitFeedback", args: [input, options] }); + if (this.error) throw this.error; + return this.response; + } +} + // TestCoreClient implements the Core contract with fully controllable sub-clients. export class TestCoreClient implements Core { readonly harness = new TestHarnessClient(); @@ -2305,6 +2340,7 @@ export class TestCoreClient implements Core { readonly runtime = new TestRuntimeClient(); readonly gateway = new TestGatewayClient(); readonly eval = new TestEvalClient(); + readonly feedback = new TestFeedbackClient(); readonly observability = new TestObservabilityClient(); readonly projectManager: ProjectManager; From 2d80c457906f737f8ab01002fd55bbaabd8fd9db Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:56:43 +0000 Subject: [PATCH 02/14] fix(feedback): make core the single message-length validator The argument schema z.string().max(1000) double-validated the raw (untrimmed) message and fired a generic zod error before core's friendlier, trim-aware 'must be 1000 characters or fewer' guard could run. Drop the arg constraint so core.submitFeedback is the one code path that validates, matching the intent noted in feedback/types.tsx. --- src/handlers/feedback/index.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx index 1bcfacc5b..01bbe1e0c 100644 --- a/src/handlers/feedback/index.tsx +++ b/src/handlers/feedback/index.tsx @@ -13,7 +13,9 @@ export const createFeedbackHandler = (core: Core, io: AppIO) => createHandler({ name: "feedback", description: "Send feedback about the AgentCore CLI to the team.", - arguments: [argument("message", "the feedback message to send", z.string().max(1000))], + // Length/empty validation lives solely in core.submitFeedback so one code path + // guards every caller; the arg is unconstrained here beyond being a string. + arguments: [argument("message", "the feedback message to send", z.string())], flags: [ flag( "screenshot", From 0ecd641f4a0b076e63d20dd754475b8e0dccdd74 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:57:01 +0000 Subject: [PATCH 03/14] fix(feedback): reject oversized screenshots from stat before reading The 100MB cap was checked on buffer.byteLength after readFile loaded the whole file, so a multi-GB file was read entirely into memory just to be rejected. Check stats.size before readFile instead. --- src/core/feedback.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/core/feedback.tsx b/src/core/feedback.tsx index b2c3c24b9..b85ef323e 100644 --- a/src/core/feedback.tsx +++ b/src/core/feedback.tsx @@ -218,6 +218,12 @@ export class FeedbackClient implements CoreFeedbackClient { if (!stats.isFile()) { throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); } + // Reject oversized files from stat before readFile, so a hostile/huge file + // is never loaded into memory just to be rejected. + if (stats.size > MAX_SCREENSHOT_BYTES) { + const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); + throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } const ext = path.extname(filePath).toLowerCase(); if ( @@ -236,11 +242,6 @@ export class FeedbackClient implements CoreFeedbackClient { `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, ); } - if (buffer.byteLength > MAX_SCREENSHOT_BYTES) { - const sizeMb = (buffer.byteLength / (1024 * 1024)).toFixed(1); - throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); - } - return { buffer: new Uint8Array(buffer), fileName: path.basename(filePath), From 04e24ea64c16bbd1325f8fa3fb89f100f0919b23 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:57:24 +0000 Subject: [PATCH 04/14] refactor(feedback): drop unused AwsClients dependency from FeedbackClient FeedbackClient only uses the injected fetch (Aperture is outside the SDK seam), so the stored AwsClients param was dead. Take only CoreFetch and update the CoreClient construction site. --- src/core/feedback.tsx | 9 ++++----- src/core/index.tsx | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/core/feedback.tsx b/src/core/feedback.tsx index b85ef323e..c747dfe38 100644 --- a/src/core/feedback.tsx +++ b/src/core/feedback.tsx @@ -4,7 +4,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { AgentCoreCLIError, ERROR_SOURCE, InputValidationError } from "../errors"; import { PACKAGE_VERSION } from "../constants"; -import type { AwsClients, CoreFetch, CoreOptions } from "./types"; +import type { CoreFetch, CoreOptions } from "./types"; import type { CoreFeedbackClient, FeedbackSubmissionResult, @@ -75,10 +75,9 @@ interface ApertureFormPayload { } export class FeedbackClient implements CoreFeedbackClient { - constructor( - private readonly clients: AwsClients, - private readonly fetch: CoreFetch, - ) {} + // Feedback posts to the Aperture public API via the injected fetch only; it makes + // no AWS SDK calls, so it does not take the AwsClients aggregate its siblings do. + constructor(private readonly fetch: CoreFetch) {} async submitFeedback( input: SubmitFeedbackInput, diff --git a/src/core/index.tsx b/src/core/index.tsx index 95060bcfe..4492d02ea 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -89,7 +89,7 @@ export class CoreClient implements AwsClients { this.runtime = new RuntimeClient(this, fetch, this.logger.child({ module: "runtime" })); this.gateway = new GatewayClient(this, fetch, this.logger.child({ module: "gateway" })); // Feedback posts to the Aperture public API via the injected fetch, outside the SDK seam. - this.feedback = new FeedbackClient(this, fetch); + this.feedback = new FeedbackClient(fetch); // EvalClient shares the injected fetch: dataset content is served from a // presigned S3 URL, outside the SDK seam the other operations use. The logger // is used for batch-evaluation result-log diagnostics. From 8f53b7b13f599f424ae24bf92971ea58c0137677 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:57:36 +0000 Subject: [PATCH 05/14] fix(feedback): classify a non-URL presign body as ApertureError If Aperture returns a 2xx presign body that isn't a URL, new URL() threw a bare TypeError that mapped to an internal-source error. Wrap it in ApertureError so telemetry attributes the failure to the service. --- src/core/feedback.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/feedback.tsx b/src/core/feedback.tsx index c747dfe38..136ccd822 100644 --- a/src/core/feedback.tsx +++ b/src/core/feedback.tsx @@ -263,7 +263,13 @@ function expandTilde(filePath: string): string { // fabricating one client-side risks pointing at a nonexistent object if // Aperture's bucket layout or region shifts. function objectKeyFromPresignedUrl(presignedUrl: string): string { - return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); + try { + return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); + } catch { + // A 2xx presign body that isn't a URL is a service fault, not a bare TypeError — + // classify it so telemetry attributes it to the service, not internal. + throw new ApertureError("Feedback service returned an invalid screenshot upload URL."); + } } function buildFeedbackPayload(input: { From b980b90afa81aa0d0149d9b8912b1bfdf2d7693c Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 19:57:50 +0000 Subject: [PATCH 06/14] fix(feedback): reject an explicitly-empty --screenshot value --screenshot "" was falsy so it silently submitted with no attachment, unlike every other bad screenshot value which errors. Reject a present-but-blank path with an InputValidationError. --- src/handlers/feedback/index.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx index 01bbe1e0c..d2f4ddaba 100644 --- a/src/handlers/feedback/index.tsx +++ b/src/handlers/feedback/index.tsx @@ -29,12 +29,19 @@ export const createFeedbackHandler = (core: Core, io: AppIO) => ), ], handle: async (ctx, flags, args) => { + // An explicitly-empty --screenshot "" is a mistake, not "no screenshot": + // reject it rather than silently submitting without an attachment. + const screenshotPath = flags["screenshot"]; + if (screenshotPath !== undefined && screenshotPath.trim() === "") { + throw new InputValidationError("--screenshot requires a file path"); + } + await confirmConsent(io, ctx.require(JsonKey), flags.yes); const result = await core.feedback.submitFeedback( { message: args["message"], - screenshot: flags["screenshot"] ? { path: flags["screenshot"] } : undefined, + screenshot: screenshotPath ? { path: screenshotPath } : undefined, }, coreOptsFromCtx(ctx), ); From 311de1b5e032e8934a0c847eb126994e3958122e Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 20:01:20 +0000 Subject: [PATCH 07/14] chore(feedback): drop redundant comment at the FeedbackClient wiring The rationale now lives on the FeedbackClient constructor in core/feedback.tsx. --- src/core/index.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/index.tsx b/src/core/index.tsx index 4492d02ea..e561664b7 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -88,7 +88,6 @@ export class CoreClient implements AwsClients { const fetch = config.fetch ?? globalThis.fetch; this.runtime = new RuntimeClient(this, fetch, this.logger.child({ module: "runtime" })); this.gateway = new GatewayClient(this, fetch, this.logger.child({ module: "gateway" })); - // Feedback posts to the Aperture public API via the injected fetch, outside the SDK seam. this.feedback = new FeedbackClient(fetch); // EvalClient shares the injected fetch: dataset content is served from a // presigned S3 URL, outside the SDK seam the other operations use. The logger From 6d051c00eca450530c349407d18342a21d9de513 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 20:05:21 +0000 Subject: [PATCH 08/14] test(feedback): replace unit test with golden fixture test Follows the batch-evaluation pattern: golden-backed happy paths (text-only and screenshot presign->S3 PUT->form, recorded against Aperture, replayed offline) plus rejects.toThrow validation/consent cases (non-TTY without --yes, decline, empty message, >1000 chars, empty --screenshot). Each submit test uses its own fixtureFetch subdir since the fetch fixture key is method+path only and both POST to /form. The presign response fixture has its X-Amz-* query stripped so no signed URL is committed; replay keys on the stable object path. --- src/handlers/feedback/__fixtures__/shot.png | Bin 0 -> 69 bytes .../submit-screenshot.golden.json | 6 + .../Fetch.499b0768d488bbcf.json | 5 + .../Fetch.92c60dabfb9be7f2.json | 5 + .../Fetch.f7c65f1fef88f718.json | 5 + .../__fixtures__/submit-text.golden.json | 6 + .../submit-text/Fetch.92c60dabfb9be7f2.json | 5 + .../feedback/feedback.fixture.test.tsx | 118 +++++++++++++ src/handlers/feedback/feedback.test.tsx | 156 ------------------ 9 files changed, 150 insertions(+), 156 deletions(-) create mode 100644 src/handlers/feedback/__fixtures__/shot.png create mode 100644 src/handlers/feedback/__fixtures__/submit-screenshot.golden.json create mode 100644 src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.499b0768d488bbcf.json create mode 100644 src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.92c60dabfb9be7f2.json create mode 100644 src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.f7c65f1fef88f718.json create mode 100644 src/handlers/feedback/__fixtures__/submit-text.golden.json create mode 100644 src/handlers/feedback/__fixtures__/submit-text/Fetch.92c60dabfb9be7f2.json create mode 100644 src/handlers/feedback/feedback.fixture.test.tsx delete mode 100644 src/handlers/feedback/feedback.test.tsx diff --git a/src/handlers/feedback/__fixtures__/shot.png b/src/handlers/feedback/__fixtures__/shot.png new file mode 100644 index 0000000000000000000000000000000000000000..875245de1db9e1bf24138d7d97a8b8df252915e6 GIT binary patch literal 69 zcmeAS@N?(olHy`uVBq!ia0vp^j3CUx0wlM}@Gt=>Zci7-kcwN$DL>9LFfcPRGMxN3 R={-=K!PC{xWt~$(69Acj4;KIc literal 0 HcmV?d00001 diff --git a/src/handlers/feedback/__fixtures__/submit-screenshot.golden.json b/src/handlers/feedback/__fixtures__/submit-screenshot.golden.json new file mode 100644 index 000000000..a72012723 --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-screenshot.golden.json @@ -0,0 +1,6 @@ +{ + "success": true, + "id": "9385c2bc-e013-4c31-a5c4-e430f221037a", + "timestamp": "2026-08-31T20:04:07.165808755Z", + "reference": "agentcore-cli" +} \ No newline at end of file diff --git a/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.499b0768d488bbcf.json b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.499b0768d488bbcf.json new file mode 100644 index 000000000..0e218e8c7 --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.499b0768d488bbcf.json @@ -0,0 +1,5 @@ +{ + "status": 200, + "statusText": "OK", + "body": "" +} \ No newline at end of file diff --git a/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.92c60dabfb9be7f2.json b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.92c60dabfb9be7f2.json new file mode 100644 index 000000000..676884db3 --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.92c60dabfb9be7f2.json @@ -0,0 +1,5 @@ +{ + "status": 200, + "statusText": "OK", + "body": "{\"reference\":\"agentcore-cli\",\"id\":\"9385c2bc-e013-4c31-a5c4-e430f221037a\",\"timestamp\":\"2026-08-31T20:04:07.165808755Z\"}" +} \ No newline at end of file diff --git a/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.f7c65f1fef88f718.json b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.f7c65f1fef88f718.json new file mode 100644 index 000000000..7c0298d14 --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-screenshot/Fetch.f7c65f1fef88f718.json @@ -0,0 +1,5 @@ +{ + "status": 200, + "statusText": "OK", + "body": "https://aperture-forms-uploaded-files-prod-us-east-1.s3.amazonaws.com/us-east-1/AgentCore/CLI/0.1.0/31082026/3066c98d-4cda-42d5-ad1e-b00e8f69b96f.png" +} diff --git a/src/handlers/feedback/__fixtures__/submit-text.golden.json b/src/handlers/feedback/__fixtures__/submit-text.golden.json new file mode 100644 index 000000000..ecd3e34af --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-text.golden.json @@ -0,0 +1,6 @@ +{ + "success": true, + "id": "395e1470-9d77-40a8-af81-9b1181bca976", + "timestamp": "2026-08-31T20:03:36.685002013Z", + "reference": "agentcore-cli" +} \ No newline at end of file diff --git a/src/handlers/feedback/__fixtures__/submit-text/Fetch.92c60dabfb9be7f2.json b/src/handlers/feedback/__fixtures__/submit-text/Fetch.92c60dabfb9be7f2.json new file mode 100644 index 000000000..f82879966 --- /dev/null +++ b/src/handlers/feedback/__fixtures__/submit-text/Fetch.92c60dabfb9be7f2.json @@ -0,0 +1,5 @@ +{ + "status": 200, + "statusText": "OK", + "body": "{\"reference\":\"agentcore-cli\",\"id\":\"395e1470-9d77-40a8-af81-9b1181bca976\",\"timestamp\":\"2026-08-31T20:03:36.685002013Z\"}" +} \ No newline at end of file diff --git a/src/handlers/feedback/feedback.fixture.test.tsx b/src/handlers/feedback/feedback.fixture.test.tsx new file mode 100644 index 000000000..728850dea --- /dev/null +++ b/src/handlers/feedback/feedback.fixture.test.tsx @@ -0,0 +1,118 @@ +import { describe, expect, test } from "bun:test"; +import { join } from "node:path"; +import { CoreClient } from "../../core"; +import { createRootHandler } from "../index"; +import { + createSilentLogger, + fixtureFactories, + fixtureFetch, + matchGolden, + TestGlobalConfigAccessor, + testIO, + type TestIOOptions, +} from "../../testing"; +import { UserCancellationError } from "../../errors"; + +// Record with: RECORD=1 bun test src/handlers/feedback/feedback.fixture.test.tsx +// +// The two "submits …" tests are WRITES: a record run posts a REAL feedback +// submission to the Aperture public API (and, for the screenshot case, uploads +// shot.png through a real presigned S3 PUT) — there is no undo, same as the +// batch-evaluation evaluate/simulate fixtures that submit real jobs. After a +// record run, strip the X-Amz-* query from the recorded presign Fetch fixture +// so no signed URL is committed (the object-path key it replays on is unchanged). +// Every other run replays the committed fixtures offline. +// +// Aperture is the one Core path outside the AWS SDK `.send()` seam, so it is +// driven through the injected `fetch` (fixtureFetch) rather than the SDK +// factories. Each submit test uses its own fixture subdir because fixtureFetch +// keys on method+path only, and both submits POST to the same /form path. +const REGION = "us-east-1"; +const FIXTURES = join(import.meta.dir, "__fixtures__"); +const SHOT = join(FIXTURES, "shot.png"); + +function createFixtureCore(fetchDir: string): CoreClient { + const { createControlClient, createDataClient, createIamClient, createLogsClient } = + fixtureFactories(FIXTURES); + return new CoreClient({ + createControlClient, + createDataClient, + createIamClient, + createLogsClient, + logger: createSilentLogger(), + fetch: fixtureFetch(join(FIXTURES, fetchDir)), + }); +} + +// run drives the real router (parsing → consent → handler → CoreClient → +// Aperture fetch) against the fixture-backed clients and returns captured IO. +async function run( + args: string[], + opts: { fetchDir?: string; io?: TestIOOptions } = {}, +): Promise<{ stdout: string; stderr: string }> { + const io = testIO(opts.io); + const root = createRootHandler(createFixtureCore(opts.fetchDir ?? "unused"), { + io: io.io, + logger: createSilentLogger(), + globalConfigAccessor: new TestGlobalConfigAccessor(), + }); + await root.route(["node", "agentcore", "feedback", ...args, "--region", REGION]); + return { stdout: io.stdout(), stderr: io.stderr() }; +} + +describe("feedback (fixture-backed)", () => { + test("submits text-only feedback and prints the result envelope", async () => { + const { stdout } = await run( + ["[agentcore-cli golden fixture] text submit — please ignore", "--yes", "--json"], + { fetchDir: "submit-text" }, + ); + + matchGolden(FIXTURES, "submit-text.golden.json", stdout); + const result = JSON.parse(stdout); + expect(result.success).toBe(true); + expect(typeof result.id).toBe("string"); + expect(result.reference).toBe("agentcore-cli"); + }, 120_000); + + test("submits feedback with a screenshot (presign → S3 PUT → form)", async () => { + const { stdout } = await run( + [ + "[agentcore-cli golden fixture] screenshot submit — please ignore", + "--screenshot", + SHOT, + "--yes", + "--json", + ], + { fetchDir: "submit-screenshot" }, + ); + + matchGolden(FIXTURES, "submit-screenshot.golden.json", stdout); + expect(JSON.parse(stdout).success).toBe(true); + }, 120_000); + + // ── validation / consent errors (no network, no fixtures — like batch-evaluation's not-found) ── + + test("without --yes and without a TTY it fails rather than submitting", async () => { + await expect(run(["headless", "--json"])).rejects.toThrow(/--yes/); + }); + + test("declining the consent prompt cancels", async () => { + await expect(run(["no thanks"], { io: { isTTY: true, stdin: "n\n" } })).rejects.toBeInstanceOf( + UserCancellationError, + ); + }); + + test("an empty message is rejected", async () => { + await expect(run([" ", "--yes"])).rejects.toThrow(/cannot be empty/); + }); + + test("a message over 1000 characters is rejected", async () => { + await expect(run(["x".repeat(1001), "--yes"])).rejects.toThrow(/1000 characters/); + }); + + test("an explicitly-empty --screenshot is rejected", async () => { + await expect(run(["msg", "--screenshot", "", "--yes"])).rejects.toThrow( + /--screenshot requires a file path/, + ); + }); +}); diff --git a/src/handlers/feedback/feedback.test.tsx b/src/handlers/feedback/feedback.test.tsx deleted file mode 100644 index 144785a65..000000000 --- a/src/handlers/feedback/feedback.test.tsx +++ /dev/null @@ -1,156 +0,0 @@ -import { test, expect, describe } from "bun:test"; -import { CoreClient } from "../../core"; -import { createRootHandler } from "../index"; -import { - createSilentLogger, - fixtureFactories, - TestGlobalConfigAccessor, - testIO, - type TestIOOptions, -} from "../../testing"; -import { InputValidationError, UserCancellationError } from "../../errors"; -import { join } from "node:path"; -import { mkdtemp, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; - -// Command-flow tests for `agentcore feedback`. The Aperture POST/PUT calls go -// through an injected `fetch` stub (feedback is the one Core path outside the SDK -// seam), so these run offline and assert the exact HTTP the client makes. - -const REGION = "us-east-1"; -const FIXTURES = join(import.meta.dir, "__fixtures__"); - -interface Recorded { - url: string; - method: string; - headers: Record; - body: unknown; -} - -// stubFetch answers the three Aperture calls (presign POST, S3 PUT, form POST) -// and records each so tests can assert the request shape. -function stubFetch(calls: Recorded[]) { - const presignedUrl = - "https://aperture-bucket.s3.us-east-1.amazonaws.com/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?X-Amz-Signature=sig"; - return (async (input: Parameters[0], init?: Parameters[1]) => { - const url = String(input); - calls.push({ - url, - method: init?.method ?? "GET", - headers: (init?.headers as Record) ?? {}, - body: init?.body, - }); - if (url.includes("/presignedurl")) { - return new Response(presignedUrl, { status: 200 }); - } - if (url.includes("/form")) { - return new Response( - JSON.stringify({ - id: "11111111-2222-3333-4444-555555555555", - timestamp: "2026-08-31T00:00:00Z", - reference: "agentcore-cli", - }), - { status: 200, headers: { "content-type": "application/json" } }, - ); - } - return new Response(null, { status: 200 }); // the S3 PUT - }) as unknown as typeof fetch; -} - -async function run( - args: string[], - opts: { io?: TestIOOptions; calls?: Recorded[] } = {}, -): Promise<{ stdout: string; stderr: string }> { - const factories = fixtureFactories(FIXTURES); - const core = new CoreClient({ - ...factories, - logger: createSilentLogger(), - fetch: stubFetch(opts.calls ?? []), - }); - const io = testIO(opts.io); - const root = createRootHandler(core, { - io: io.io, - logger: createSilentLogger(), - globalConfigAccessor: new TestGlobalConfigAccessor(), - }); - await root.route(["node", "agentcore", "feedback", ...args, "--region", REGION]); - return { stdout: io.stdout(), stderr: io.stderr() }; -} - -describe("feedback", () => { - test("--yes --json submits and prints the result envelope", async () => { - const calls: Recorded[] = []; - const { stdout } = await run(["great tool", "--yes", "--json"], { calls }); - const parsed = JSON.parse(stdout); - expect(parsed.success).toBe(true); - expect(parsed.id).toBe("11111111-2222-3333-4444-555555555555"); - // Text-only: exactly one call, the form POST. - expect(calls).toHaveLength(1); - expect(calls[0]!.url).toContain("/form"); - expect(calls[0]!.method).toBe("POST"); - }); - - test("prompts on a TTY and submits on 'y'", async () => { - const calls: Recorded[] = []; - const { stderr } = await run(["nice cli"], { io: { isTTY: true, stdin: "y\n" }, calls }); - expect(stderr).toContain("AWS Customer Agreement"); - expect(stderr).toContain("Submit feedback? (y/N)"); - expect(calls).toHaveLength(1); - }); - - test("declining the prompt cancels without submitting", async () => { - const calls: Recorded[] = []; - await expect( - run(["nope"], { io: { isTTY: true, stdin: "n\n" }, calls }), - ).rejects.toBeInstanceOf(UserCancellationError); - expect(calls).toHaveLength(0); - }); - - test("non-interactive without --yes fails and does not submit", async () => { - const calls: Recorded[] = []; - const removal = run(["headless"], { calls }); - await expect(removal).rejects.toBeInstanceOf(InputValidationError); - await expect(run(["headless"], { calls: [] })).rejects.toThrow(/--yes/); - expect(calls).toHaveLength(0); - }); - - test("an empty message is rejected before any network call", async () => { - const calls: Recorded[] = []; - await expect(run([" ", "--yes"], { calls })).rejects.toThrow(/cannot be empty/); - expect(calls).toHaveLength(0); - }); - - test("a message over 1000 chars is rejected", async () => { - const calls: Recorded[] = []; - await expect(run(["x".repeat(1001), "--yes"], { calls })).rejects.toThrow(/1000 characters/); - expect(calls).toHaveLength(0); - }); - - test("a screenshot drives presign -> S3 PUT (checksum + tag) -> form POST", async () => { - const dir = await mkdtemp(join(tmpdir(), "agentcore-fb-")); - const shot = join(dir, "shot.png"); - await writeFile(shot, Buffer.from("iVBORw0KGgoAAAANSUhEUgAA", "base64")); - - const calls: Recorded[] = []; - const { stdout } = await run(["with shot", "--screenshot", shot, "--yes", "--json"], { calls }); - expect(JSON.parse(stdout).success).toBe(true); - - expect( - calls.map( - (c) => - `${c.method} ${c.url.includes("/presignedurl") ? "presign" : c.url.includes("/form") ? "form" : "s3"}`, - ), - ).toEqual(["POST presign", "PUT s3", "POST form"]); - const put = calls[1]!; - expect(put.headers["x-amz-checksum-algorithm"]).toBe("SHA256"); - expect(put.headers["x-amz-tagging"]).toBe("scanstatus=NOT_SCANNED"); - // The form references the exact object key parsed from the presigned URL path. - const form = JSON.parse(String(calls[2]!.body)); - const attachment = form.customerResponses.find( - (r: { response: { responseType: string } }) => r.response.responseType === "fileUpload", - ); - expect(attachment.response.responseValue).toEqual([ - "us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png", - ]); - }); -}); From c25a04ca68922888def1460e443d4c01ecf9f32b Mon Sep 17 00:00:00 2001 From: jariy17 Date: Mon, 31 Aug 2026 20:14:59 +0000 Subject: [PATCH 09/14] test(feedback): add feedback to the root command-tree assertion The feedback command was registered on the root handler in PR #2149 but root.test.tsx's expected subcommand list was not updated, so 'builds the agentcore command tree with its subcommands' failed in CI. Add 'feedback' in its registration position (after eval). --- src/handlers/root.test.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/handlers/root.test.tsx b/src/handlers/root.test.tsx index b3f4e3386..63f5dfc1c 100644 --- a/src/handlers/root.test.tsx +++ b/src/handlers/root.test.tsx @@ -17,6 +17,7 @@ describe("createRootHandler", () => { "memory", "gateway", "eval", + "feedback", "config", "project", ]); From bd6bbbc78621f282d4ce760f4da22396aa0e5798 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Tue, 1 Sep 2026 14:42:48 +0000 Subject: [PATCH 10/14] refactor(feedback): move Aperture logic out of Core into the handler Feedback is an outbound product-API call, not work in the user's AWS account, so it no longer belongs on the AWS-focused Core surface (review: AlexanderRichey). Moves the submit logic to src/handlers/feedback/submit.ts, injected with a fetch threaded through RootHandlerConfig (defaults to global fetch; tests inject their own), and drops Core.feedback / CoreClient.feedback / TestFeedbackClient and src/core/feedback.tsx. Folds in the review fixes to the moved code: - Derive+validate the S3 object key BEFORE the PUT, so an invalid presign body raises ApertureError instead of a bare TypeError inside the upload (aidandaly24). - Validate the form response shape (string id/timestamp/reference) instead of an unchecked cast; a partial/non-JSON 2xx now raises ApertureError (harness bot). - Inline the over-defensive readBody into `response.text().catch(() => "")` (AlexanderRichey). - Feedback data shapes (ScreenshotInput/SubmitFeedbackInput/FeedbackSubmissionResult and internal records) are `type` aliases; `interface` is reserved for contracts (aidandaly24). --- src/core/feedback.tsx | 332 ------------------------------- src/core/index.tsx | 3 - src/handlers/feedback/index.tsx | 15 +- src/handlers/feedback/submit.ts | 334 ++++++++++++++++++++++++++++++++ src/handlers/feedback/types.tsx | 26 +-- src/handlers/index.tsx | 7 +- src/handlers/types.tsx | 2 - src/testing/TestCoreClient.tsx | 36 ---- 8 files changed, 355 insertions(+), 400 deletions(-) delete mode 100644 src/core/feedback.tsx create mode 100644 src/handlers/feedback/submit.ts diff --git a/src/core/feedback.tsx b/src/core/feedback.tsx deleted file mode 100644 index 136ccd822..000000000 --- a/src/core/feedback.tsx +++ /dev/null @@ -1,332 +0,0 @@ -import { createHash } from "node:crypto"; -import { stat, readFile } from "node:fs/promises"; -import * as os from "node:os"; -import * as path from "node:path"; -import { AgentCoreCLIError, ERROR_SOURCE, InputValidationError } from "../errors"; -import { PACKAGE_VERSION } from "../constants"; -import type { CoreFetch, CoreOptions } from "./types"; -import type { - CoreFeedbackClient, - FeedbackSubmissionResult, - SubmitFeedbackInput, -} from "../handlers/feedback/types"; - -// Aperture public feedback API. These are commercial-partition (.aws.dev) endpoints -// with no partition variant, so feedback is unavailable in GovCloud/China — carried -// over from the pre-refactor CLI, flagged here rather than silently. -const INGESTION_URL = "https://ingestion.aperture-public-api.feedback.console.aws.dev/form"; -const PRESIGN_URL = - "https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl"; -const FORM_CATEGORY = "AgentCore"; -const FORM_NAME = "CLI"; -const FORM_VERSION = "0.1.0"; -const LOCALE = "en_US"; -const REFERENCE = "agentcore-cli"; -const MESSAGE_QUESTION = "What feedback do you have for the AgentCore CLI"; -const ATTACHMENT_QUESTION = "Attachments"; -const MESSAGE_MAX_LENGTH = 1000; -const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; -const ALLOWED_SCREENSHOT_EXTENSIONS = [".png", ".jpg", ".jpeg"] as const; - -// Rendered by the feedback command's consent prompt before every submission. -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."; - -// Extends the CLI error hierarchy (the pre-refactor ApertureError extended plain -// Error, so telemetry classified it as unknown) so failures record error_source=service. -export class ApertureError extends AgentCoreCLIError { - constructor( - message: string, - readonly status?: number, - readonly body?: string, - ) { - super(message, { source: ERROR_SOURCE.SERVICE, name: "ApertureError" }); - } -} - -interface LoadedScreenshot { - buffer: Uint8Array; - fileName: string; - contentType: string; - sha256Base64: string; - size: number; -} - -interface ApertureCustomerResponse { - question: string; - pii: boolean; - response: - | { responseType: "textArea"; responseValue: string } - | { responseType: "fileUpload"; responseValue: string[] }; -} - -interface ApertureFormPayload { - category: string; - name: string; - version: string; - locale: string; - reference: string; - location: string; - customerResponses: ApertureCustomerResponse[]; - metadataList: { key: string; value: string }[]; -} - -export class FeedbackClient implements CoreFeedbackClient { - // Feedback posts to the Aperture public API via the injected fetch only; it makes - // no AWS SDK calls, so it does not take the AwsClients aggregate its siblings do. - constructor(private readonly fetch: CoreFetch) {} - - async submitFeedback( - input: SubmitFeedbackInput, - _options: CoreOptions, - ): Promise { - const message = input.message.trim(); - if (!message) { - throw new InputValidationError("Feedback message cannot be empty."); - } - if (message.length > MESSAGE_MAX_LENGTH) { - throw new InputValidationError( - `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, - ); - } - - const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; - - let screenshotReference: string | undefined; - if (input.screenshot) { - const file = await this.loadScreenshot(input.screenshot.path); - const presignedUrl = await this.fetchPresignedUrl( - { - category: FORM_CATEGORY, - name: FORM_NAME, - version: FORM_VERSION, - fileName: file.fileName, - fileSize: file.size, - uploadFileSHA256: file.sha256Base64, - }, - userAgent, - ); - await this.uploadFileToS3( - presignedUrl, - file.buffer, - file.contentType, - file.sha256Base64, - userAgent, - ); - screenshotReference = objectKeyFromPresignedUrl(presignedUrl); - } - - const payload = buildFeedbackPayload({ message, screenshotReference }); - const response = await this.submitForm(payload, userAgent); - return { - id: response.id, - timestamp: response.timestamp, - reference: response.reference, - }; - } - - // Aperture returns the presigned URL as a plain-text body (not JSON). - private async fetchPresignedUrl( - request: { - category: string; - name: string; - version: string; - fileName: string; - fileSize: number; - uploadFileSHA256: string; - }, - userAgent: string, - ): Promise { - const response = await this.fetch(PRESIGN_URL, { - method: "POST", - headers: { "content-type": "application/json", "user-agent": userAgent }, - body: JSON.stringify(request), - }); - if (!response.ok) { - throw new ApertureError( - `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, - response.status, - await readBody(response), - ); - } - return (await response.text()).trim(); - } - - // Aperture's bucket policy requires the SHA-256 checksum headers and a tag - // marking the object as not yet AV-scanned; omitting either is rejected. - private async uploadFileToS3( - presignedUrl: string, - fileBuffer: Uint8Array, - contentType: string, - base64Sha256: string, - userAgent: string, - ): Promise { - const response = await this.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) { - throw new ApertureError( - `Failed to upload screenshot (HTTP ${response.status}).`, - response.status, - await readBody(response), - ); - } - } - - private async submitForm( - payload: ApertureFormPayload, - userAgent: string, - ): Promise { - const response = await this.fetch(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), response.status, body); - } - return (await response.json()) as FeedbackSubmissionResult; - } - - private async loadScreenshot(rawFilePath: string): Promise { - const filePath = expandTilde(rawFilePath); - - let stats: Awaited>; - try { - stats = await stat(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - if (stats.isDirectory()) { - throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); - } - if (!stats.isFile()) { - throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); - } - // Reject oversized files from stat before readFile, so a hostile/huge file - // is never loaded into memory just to be rejected. - if (stats.size > MAX_SCREENSHOT_BYTES) { - const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); - throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); - } - - const ext = path.extname(filePath).toLowerCase(); - if ( - !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) - ) { - throw new InputValidationError( - `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, - ); - } - - let buffer: Buffer; - try { - buffer = await readFile(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - return { - buffer: new Uint8Array(buffer), - fileName: path.basename(filePath), - contentType: ext === ".png" ? "image/png" : "image/jpeg", - sha256Base64: createHash("sha256").update(buffer).digest("base64"), - size: buffer.byteLength, - }; - } -} - -// Expand a leading ~ / ~/... to $HOME. Node's fs APIs don't expand tildes (the -// shell normally does), so a quoted path like "~/shot.png" would otherwise ENOENT. -function expandTilde(filePath: string): string { - if (filePath === "~") return os.homedir(); - if (filePath.startsWith("~/")) return path.join(os.homedir(), filePath.slice(2)); - return filePath; -} - -// The presigned URL's path IS the S3 object key the form must reference; -// fabricating one client-side risks pointing at a nonexistent object if -// Aperture's bucket layout or region shifts. -function objectKeyFromPresignedUrl(presignedUrl: string): string { - try { - return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); - } catch { - // A 2xx presign body that isn't a URL is a service fault, not a bare TypeError — - // classify it so telemetry attributes it to the service, not internal. - throw new ApertureError("Feedback service returned an invalid screenshot upload URL."); - } -} - -function buildFeedbackPayload(input: { - message: string; - screenshotReference?: string; -}): ApertureFormPayload { - const customerResponses: ApertureCustomerResponse[] = [ - { - question: MESSAGE_QUESTION, - pii: false, - response: { responseType: "textArea", responseValue: input.message }, - }, - ]; - if (input.screenshotReference) { - customerResponses.push({ - question: ATTACHMENT_QUESTION, - pii: true, - response: { responseType: "fileUpload", responseValue: [input.screenshotReference] }, - }); - } - - // Aperture rejects unknown metadata keys with HTTP 400; only cli-version and os - // are registered in the form template, so node version + mode ride in `location`. - return { - category: FORM_CATEGORY, - name: FORM_NAME, - version: FORM_VERSION, - locale: LOCALE, - reference: REFERENCE, - location: `agentcore-cli@${PACKAGE_VERSION} (${process.platform}; node ${process.version}; cli)`, - customerResponses, - metadataList: [ - { key: "cli-version", value: PACKAGE_VERSION }, - { key: "os", value: `${process.platform} ${os.release()}` }, - ], - }; -} - -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}.`; - } -} - -async function readBody(response: Response): Promise { - try { - return await response.text(); - } catch { - return ""; - } -} diff --git a/src/core/index.tsx b/src/core/index.tsx index e561664b7..5a6bab9fb 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -3,7 +3,6 @@ import { BedrockAgentCoreClient } from "@aws-sdk/client-bedrock-agentcore"; import { IAMClient } from "@aws-sdk/client-iam"; import { CloudWatchLogsClient } from "@aws-sdk/client-cloudwatch-logs"; import { EvalClient } from "./eval"; -import { FeedbackClient } from "./feedback"; import { GatewayClient } from "./gateway"; import { HarnessClient } from "./harness"; import { IdentityClient } from "./identity"; @@ -73,7 +72,6 @@ export class CoreClient implements AwsClients { readonly runtime: RuntimeClient; readonly gateway: GatewayClient; readonly eval: EvalClient; - readonly feedback: FeedbackClient; readonly observability: ObservabilityClient; readonly projectManager: ProjectManager; @@ -88,7 +86,6 @@ export class CoreClient implements AwsClients { const fetch = config.fetch ?? globalThis.fetch; this.runtime = new RuntimeClient(this, fetch, this.logger.child({ module: "runtime" })); this.gateway = new GatewayClient(this, fetch, this.logger.child({ module: "gateway" })); - this.feedback = new FeedbackClient(fetch); // EvalClient shares the injected fetch: dataset content is served from a // presigned S3 URL, outside the SDK seam the other operations use. The logger // is used for batch-evaluation result-log diagnostics. diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx index d2f4ddaba..07a61dee1 100644 --- a/src/handlers/feedback/index.tsx +++ b/src/handlers/feedback/index.tsx @@ -2,18 +2,17 @@ import { createInterface } from "node:readline/promises"; import z from "zod"; import { argument, createHandler, flag } from "../../router"; import { JsonRendererKey } from "../../tui"; -import { InputValidationError, UserCancellationError } from "../../errors"; -import { coreOptsFromCtx } from "../utils.tsx"; import { JsonKey } from "../keys.tsx"; -import { CONSENT_TEXT } from "../../core/feedback"; -import type { Core } from "../types.tsx"; +import { InputValidationError, UserCancellationError } from "../../errors"; +import { CONSENT_TEXT, submitFeedback } from "./submit"; import type { AppIO } from "../../io"; +import type { CoreFetch } from "../../core/types"; -export const createFeedbackHandler = (core: Core, io: AppIO) => +export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => createHandler({ name: "feedback", description: "Send feedback about the AgentCore CLI to the team.", - // Length/empty validation lives solely in core.submitFeedback so one code path + // Length/empty validation lives solely in submitFeedback so one code path // guards every caller; the arg is unconstrained here beyond being a string. arguments: [argument("message", "the feedback message to send", z.string())], flags: [ @@ -38,12 +37,12 @@ export const createFeedbackHandler = (core: Core, io: AppIO) => await confirmConsent(io, ctx.require(JsonKey), flags.yes); - const result = await core.feedback.submitFeedback( + const result = await submitFeedback( { message: args["message"], screenshot: screenshotPath ? { path: screenshotPath } : undefined, }, - coreOptsFromCtx(ctx), + fetch, ); ctx.require(JsonRendererKey).renderJson({ success: true, ...result }); diff --git a/src/handlers/feedback/submit.ts b/src/handlers/feedback/submit.ts new file mode 100644 index 000000000..9b7e83198 --- /dev/null +++ b/src/handlers/feedback/submit.ts @@ -0,0 +1,334 @@ +import { createHash } from "node:crypto"; +import { stat, readFile } from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { AgentCoreCLIError, ERROR_SOURCE, InputValidationError } from "../../errors"; +import { PACKAGE_VERSION } from "../../constants"; +import type { CoreFetch } from "../../core/types"; +import type { FeedbackSubmissionResult, SubmitFeedbackInput } from "./types"; + +// Aperture public feedback API. These are commercial-partition (.aws.dev) endpoints +// with no partition variant, so feedback is unavailable in GovCloud/China — carried +// over from the pre-refactor CLI, flagged here rather than silently. +const INGESTION_URL = "https://ingestion.aperture-public-api.feedback.console.aws.dev/form"; +const PRESIGN_URL = + "https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl"; +const FORM_CATEGORY = "AgentCore"; +const FORM_NAME = "CLI"; +const FORM_VERSION = "0.1.0"; +const LOCALE = "en_US"; +const REFERENCE = "agentcore-cli"; +const MESSAGE_QUESTION = "What feedback do you have for the AgentCore CLI"; +const ATTACHMENT_QUESTION = "Attachments"; +const MESSAGE_MAX_LENGTH = 1000; +const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; +const ALLOWED_SCREENSHOT_EXTENSIONS = [".png", ".jpg", ".jpeg"] as const; + +// Rendered by the feedback command's consent prompt before every submission. +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."; + +// Extends the CLI error hierarchy (the pre-refactor ApertureError extended plain +// Error, so telemetry classified it as unknown) so failures record error_source=service. +export class ApertureError extends AgentCoreCLIError { + constructor( + message: string, + readonly status?: number, + readonly body?: string, + ) { + super(message, { source: ERROR_SOURCE.SERVICE, name: "ApertureError" }); + } +} + +type LoadedScreenshot = { + buffer: Uint8Array; + fileName: string; + contentType: string; + sha256Base64: string; + size: number; +}; + +type ApertureCustomerResponse = { + question: string; + pii: boolean; + response: + | { responseType: "textArea"; responseValue: string } + | { responseType: "fileUpload"; responseValue: string[] }; +}; + +type ApertureFormPayload = { + category: string; + name: string; + version: string; + locale: string; + reference: string; + location: string; + customerResponses: ApertureCustomerResponse[]; + metadataList: { key: string; value: string }[]; +}; + +// submitFeedback posts a message (and optional screenshot) to the Aperture public +// feedback API using the injected fetch. It lives with the handler rather than in +// Core because feedback is an outbound product-API call, not work in the user's AWS +// account, and it makes no AWS SDK calls. +export async function submitFeedback( + input: SubmitFeedbackInput, + fetch: CoreFetch, +): Promise { + const message = input.message.trim(); + if (!message) { + throw new InputValidationError("Feedback message cannot be empty."); + } + if (message.length > MESSAGE_MAX_LENGTH) { + throw new InputValidationError( + `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, + ); + } + + const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; + + let screenshotReference: string | undefined; + if (input.screenshot) { + const file = await loadScreenshot(input.screenshot.path); + const presignedUrl = await fetchPresignedUrl( + fetch, + { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + fileName: file.fileName, + fileSize: file.size, + uploadFileSHA256: file.sha256Base64, + }, + userAgent, + ); + // Derive (and validate) the object key BEFORE the upload: fetch(badUrl) would + // otherwise throw a bare TypeError inside uploadFileToS3, masking the classified + // ApertureError and wasting a PUT against an unusable reference. + screenshotReference = objectKeyFromPresignedUrl(presignedUrl); + await uploadFileToS3( + fetch, + presignedUrl, + file.buffer, + file.contentType, + file.sha256Base64, + userAgent, + ); + } + + const payload = buildFeedbackPayload({ message, screenshotReference }); + return submitForm(fetch, payload, userAgent); +} + +// Aperture returns the presigned URL as a plain-text body (not JSON). +async function fetchPresignedUrl( + fetch: CoreFetch, + request: { + category: string; + name: string; + version: string; + fileName: string; + fileSize: number; + uploadFileSHA256: string; + }, + userAgent: string, +): Promise { + const response = await fetch(PRESIGN_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(request), + }); + if (!response.ok) { + throw new ApertureError( + `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); + } + return (await response.text()).trim(); +} + +// Aperture's bucket policy requires the SHA-256 checksum headers and a tag marking +// the object as not yet AV-scanned; omitting either is rejected. +async function uploadFileToS3( + fetch: CoreFetch, + 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) { + throw new ApertureError( + `Failed to upload screenshot (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); + } +} + +async function submitForm( + fetch: CoreFetch, + payload: ApertureFormPayload, + userAgent: string, +): Promise { + const response = await fetch(INGESTION_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(payload), + }); + if (!response.ok) { + const body = await response.text().catch(() => ""); + throw new ApertureError(mapStatusToMessage(response.status, body), response.status, body); + } + // A 2xx with a non-JSON or unexpectedly-shaped body is a service fault; validate + // rather than casting so a partial response never surfaces `id: undefined`. + const data = (await response + .json() + .catch(() => null)) as Partial | null; + if ( + !data || + typeof data.id !== "string" || + typeof data.timestamp !== "string" || + typeof data.reference !== "string" + ) { + throw new ApertureError("Feedback service returned an unexpected response."); + } + return { id: data.id, timestamp: data.timestamp, reference: data.reference }; +} + +async function loadScreenshot(rawFilePath: string): Promise { + const filePath = expandTilde(rawFilePath); + + let stats: Awaited>; + try { + stats = await stat(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (stats.isDirectory()) { + throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); + } + if (!stats.isFile()) { + throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); + } + // Reject oversized files from stat before readFile, so a hostile/huge file is + // never loaded into memory just to be rejected. + if (stats.size > MAX_SCREENSHOT_BYTES) { + const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); + throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } + + const ext = path.extname(filePath).toLowerCase(); + if ( + !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) + ) { + throw new InputValidationError( + `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, + ); + } + + let buffer: Buffer; + try { + buffer = await readFile(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + return { + buffer: new Uint8Array(buffer), + fileName: path.basename(filePath), + contentType: ext === ".png" ? "image/png" : "image/jpeg", + sha256Base64: createHash("sha256").update(buffer).digest("base64"), + size: buffer.byteLength, + }; +} + +// Expand a leading ~ / ~/... to $HOME. Node's fs APIs don't expand tildes (the +// shell normally does), so a quoted path like "~/shot.png" would otherwise ENOENT. +function expandTilde(filePath: string): string { + if (filePath === "~") return os.homedir(); + if (filePath.startsWith("~/")) return path.join(os.homedir(), filePath.slice(2)); + return filePath; +} + +// The presigned URL's path IS the S3 object key the form must reference; fabricating +// one client-side risks pointing at a nonexistent object if Aperture's bucket layout +// or region shifts. +function objectKeyFromPresignedUrl(presignedUrl: string): string { + try { + return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); + } catch { + // A 2xx presign body that isn't a URL is a service fault, not a bare TypeError — + // classify it so telemetry attributes it to the service, not internal. + throw new ApertureError("Feedback service returned an invalid screenshot upload URL."); + } +} + +function buildFeedbackPayload(input: { + message: string; + screenshotReference?: string; +}): ApertureFormPayload { + const customerResponses: ApertureCustomerResponse[] = [ + { + question: MESSAGE_QUESTION, + pii: false, + response: { responseType: "textArea", responseValue: input.message }, + }, + ]; + if (input.screenshotReference) { + customerResponses.push({ + question: ATTACHMENT_QUESTION, + pii: true, + response: { responseType: "fileUpload", responseValue: [input.screenshotReference] }, + }); + } + + // Aperture rejects unknown metadata keys with HTTP 400; only cli-version and os + // are registered in the form template, so node version + mode ride in `location`. + return { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + locale: LOCALE, + reference: REFERENCE, + location: `agentcore-cli@${PACKAGE_VERSION} (${process.platform}; node ${process.version}; cli)`, + customerResponses, + metadataList: [ + { key: "cli-version", value: PACKAGE_VERSION }, + { key: "os", value: `${process.platform} ${os.release()}` }, + ], + }; +} + +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/handlers/feedback/types.tsx b/src/handlers/feedback/types.tsx index f5536cdd9..6e822975e 100644 --- a/src/handlers/feedback/types.tsx +++ b/src/handlers/feedback/types.tsx @@ -1,26 +1,16 @@ -import type { CoreOptions } from "../../core/types"; +// Concrete request/result data shapes for the feedback command. These are data, +// not behavioral contracts, so they are type aliases (interfaces are reserved for +// the Core*Client contracts elsewhere in handlers/). -export interface ScreenshotInput { - path: string; -} +export type ScreenshotInput = { path: string }; -export interface SubmitFeedbackInput { +export type SubmitFeedbackInput = { message: string; screenshot?: ScreenshotInput; -} +}; -export interface FeedbackSubmissionResult { +export type FeedbackSubmissionResult = { id: string; timestamp: string; reference: string; -} - -// Consumer-defined interface (dependency inversion): the handler depends on this, -// src/core/feedback.tsx implements it. Message/screenshot validation happens inside -// submitFeedback so the one code path guards every caller. -export interface CoreFeedbackClient { - submitFeedback( - input: SubmitFeedbackInput, - options: CoreOptions, - ): Promise; -} +}; diff --git a/src/handlers/index.tsx b/src/handlers/index.tsx index cd03e75f2..3fbd8b087 100644 --- a/src/handlers/index.tsx +++ b/src/handlers/index.tsx @@ -13,6 +13,7 @@ import { renderTui } from "../tui"; import { withRegion, withJsonRenderer, withLogging, withGlobalConfigAccessor } from "../middleware"; import type { AppIO } from "../io"; import type { Core } from "./types.tsx"; +import type { CoreFetch } from "../core/types"; import type { Logger } from "../logging"; import type { GlobalConfigAccessor } from "../globalConfig"; import { PACKAGE_VERSION } from "../constants"; @@ -21,10 +22,14 @@ export interface RootHandlerConfig { io: AppIO; logger: Logger; globalConfigAccessor: GlobalConfigAccessor; + // Outbound HTTP for handlers that call non-AWS APIs directly (e.g. feedback → + // Aperture). Defaults to the global fetch; tests inject a fixture/capturing fetch. + fetch?: CoreFetch; } export function createRootHandler(core: Core, config: RootHandlerConfig): Router { const { io, logger } = config; + const fetch = config.fetch ?? globalThis.fetch; const root = new Router("agentcore", "the platform for production AI agents"); // `agentcore --version` prints the build-time package version. @@ -54,7 +59,7 @@ export function createRootHandler(core: Core, config: RootHandlerConfig): Router root.handler(createMemoryHandler(core, io)); root.handler(createGatewayHandler(core, io)); root.handler(createEvalHandler(core, io)); - root.handler(createFeedbackHandler(core, io)); + root.handler(createFeedbackHandler(io, fetch)); root.handler(createConfigHandler()); root.handler(createProjectHandler({ core, io })); diff --git a/src/handlers/types.tsx b/src/handlers/types.tsx index ccf802421..3d76827b8 100644 --- a/src/handlers/types.tsx +++ b/src/handlers/types.tsx @@ -1,5 +1,4 @@ import type { CoreEvalClient } from "./eval/types.tsx"; -import type { CoreFeedbackClient } from "./feedback/types.tsx"; import type { CoreGatewayClient } from "./gateway/types.tsx"; import type { CoreHarnessClient } from "./harness/types.tsx"; import type { CoreIdentityClient } from "./identity/types.tsx"; @@ -16,7 +15,6 @@ export interface Core { runtime: CoreRuntimeClient; gateway: CoreGatewayClient; eval: CoreEvalClient; - feedback: CoreFeedbackClient; observability: CoreObservabilityClient; projectManager: ProjectManager; /** Describes a Bedrock Agent + alias for `--type import`. */ diff --git a/src/testing/TestCoreClient.tsx b/src/testing/TestCoreClient.tsx index b4833c57b..a6da20a9e 100644 --- a/src/testing/TestCoreClient.tsx +++ b/src/testing/TestCoreClient.tsx @@ -126,11 +126,6 @@ import type { UpdateOauth2CredentialProviderInput, } from "../handlers/identity/types"; import type { CoreMemoryClient } from "../handlers/memory/types"; -import type { - CoreFeedbackClient, - FeedbackSubmissionResult, - SubmitFeedbackInput, -} from "../handlers/feedback/types"; import type { CoreObservabilityClient, CoreRuntimeClient, @@ -2302,36 +2297,6 @@ export class TestObservabilityClient implements CoreObservabilityClient { } } -// TestFeedbackClient is the feedback sub-client of TestCoreClient. -export class TestFeedbackClient implements CoreFeedbackClient { - readonly calls: RecordedCall[] = []; - private response: FeedbackSubmissionResult = { - id: "feedback-test-id", - timestamp: "2026-01-01T00:00:00Z", - reference: "agentcore-cli", - }; - private error?: Error; - - setSubmitResponse(response: FeedbackSubmissionResult): this { - this.response = response; - return this; - } - - setError(error: Error | undefined): this { - this.error = error; - return this; - } - - async submitFeedback( - input: SubmitFeedbackInput, - options: CoreOptions, - ): Promise { - this.calls.push({ method: "submitFeedback", args: [input, options] }); - if (this.error) throw this.error; - return this.response; - } -} - // TestCoreClient implements the Core contract with fully controllable sub-clients. export class TestCoreClient implements Core { readonly harness = new TestHarnessClient(); @@ -2340,7 +2305,6 @@ export class TestCoreClient implements Core { readonly runtime = new TestRuntimeClient(); readonly gateway = new TestGatewayClient(); readonly eval = new TestEvalClient(); - readonly feedback = new TestFeedbackClient(); readonly observability = new TestObservabilityClient(); readonly projectManager: ProjectManager; From 8f475db3ac311f60dc32fa2a4ed976a4f596f525 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Tue, 1 Sep 2026 14:43:06 +0000 Subject: [PATCH 11/14] test(fixtures): sanitize presigned URLs in fixtureFetch on record RECORD=1 wrote response.text() verbatim, so a recorded presign response put the live X-Amz-* signature on disk until a manual cleanup step. Sanitize the persisted body (return the original to the live upload); the fixture key is the object path, unchanged by sanitizing, so replay is unaffected. Removes the manual scrub. --- src/testing/fixtures.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/testing/fixtures.tsx b/src/testing/fixtures.tsx index 2ea230002..bc5f994ea 100644 --- a/src/testing/fixtures.tsx +++ b/src/testing/fixtures.tsx @@ -302,15 +302,20 @@ export function fixtureFetch(dir: string): CoreFetch { if (isRecording()) { mkdirSync(dir, { recursive: true }); const response = await globalThis.fetch(input, init); + const body = await response.text(); + // Persist a queryless copy so a recorded presign response never commits its + // X-Amz-* signature to disk, but return the original body so the live upload + // still works during the record run. The fixture key is the object path, which + // sanitizing does not change, so replay is unaffected. const fixture: FetchFixture = { status: response.status, statusText: response.statusText, - body: await response.text(), + body: sanitizePresignedUrls(body), }; writeFileSync(path, stringify(fixture)); - return new Response(fixture.body, { - status: fixture.status, - statusText: fixture.statusText, + return new Response(body, { + status: response.status, + statusText: response.statusText, }); } From b8c535a94835fa2fa7b583ca23b797a220cf0ce1 Mon Sep 17 00:00:00 2001 From: jariy17 Date: Tue, 1 Sep 2026 14:43:06 +0000 Subject: [PATCH 12/14] test(feedback): inject fetch via RootHandlerConfig + add request-contract tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The golden flow now injects fetch through createRootHandler (feedback no longer uses Core). Adds capturing-fetch tests (the core/gatewayInvoke pattern) asserting the checksum + NOT_SCANNED headers and the parsed object key, a malformed form response → ApertureError, and that an invalid presign body fails before the PUT. --- .../feedback/feedback.fixture.test.tsx | 149 +++++++++++++----- 1 file changed, 113 insertions(+), 36 deletions(-) diff --git a/src/handlers/feedback/feedback.fixture.test.tsx b/src/handlers/feedback/feedback.fixture.test.tsx index 728850dea..a90b55b61 100644 --- a/src/handlers/feedback/feedback.fixture.test.tsx +++ b/src/handlers/feedback/feedback.fixture.test.tsx @@ -1,60 +1,51 @@ import { describe, expect, test } from "bun:test"; import { join } from "node:path"; -import { CoreClient } from "../../core"; import { createRootHandler } from "../index"; import { createSilentLogger, - fixtureFactories, fixtureFetch, matchGolden, + TestCoreClient, TestGlobalConfigAccessor, testIO, type TestIOOptions, } from "../../testing"; import { UserCancellationError } from "../../errors"; +import { ApertureError } from "./submit"; +import type { CoreFetch } from "../../core/types"; // Record with: RECORD=1 bun test src/handlers/feedback/feedback.fixture.test.tsx // // The two "submits …" tests are WRITES: a record run posts a REAL feedback // submission to the Aperture public API (and, for the screenshot case, uploads // shot.png through a real presigned S3 PUT) — there is no undo, same as the -// batch-evaluation evaluate/simulate fixtures that submit real jobs. After a -// record run, strip the X-Amz-* query from the recorded presign Fetch fixture -// so no signed URL is committed (the object-path key it replays on is unchanged). -// Every other run replays the committed fixtures offline. +// batch-evaluation evaluate/simulate fixtures. `fixtureFetch` sanitizes the +// recorded presign response, so no X-Amz-* signature is committed. Every other run +// replays the committed fixtures offline. // -// Aperture is the one Core path outside the AWS SDK `.send()` seam, so it is -// driven through the injected `fetch` (fixtureFetch) rather than the SDK -// factories. Each submit test uses its own fixture subdir because fixtureFetch -// keys on method+path only, and both submits POST to the same /form path. +// Feedback calls the Aperture API through an injected fetch (fixtureFetch), passed +// via RootHandlerConfig — the handler owns the HTTP, not Core. Each submit test uses +// its own fixture subdir because fixtureFetch keys on method+path only, and both +// submits POST to the same /form path. const REGION = "us-east-1"; const FIXTURES = join(import.meta.dir, "__fixtures__"); const SHOT = join(FIXTURES, "shot.png"); -function createFixtureCore(fetchDir: string): CoreClient { - const { createControlClient, createDataClient, createIamClient, createLogsClient } = - fixtureFactories(FIXTURES); - return new CoreClient({ - createControlClient, - createDataClient, - createIamClient, - createLogsClient, - logger: createSilentLogger(), - fetch: fixtureFetch(join(FIXTURES, fetchDir)), - }); -} +// A fetch that fails loudly: used for cases that must reject before any network. +const neverFetch = (async () => { + throw new Error("network should not be reached"); +}) as unknown as CoreFetch; -// run drives the real router (parsing → consent → handler → CoreClient → -// Aperture fetch) against the fixture-backed clients and returns captured IO. async function run( args: string[], - opts: { fetchDir?: string; io?: TestIOOptions } = {}, + opts: { fetch?: CoreFetch; io?: TestIOOptions } = {}, ): Promise<{ stdout: string; stderr: string }> { const io = testIO(opts.io); - const root = createRootHandler(createFixtureCore(opts.fetchDir ?? "unused"), { + const root = createRootHandler(new TestCoreClient(), { io: io.io, logger: createSilentLogger(), globalConfigAccessor: new TestGlobalConfigAccessor(), + fetch: opts.fetch, }); await root.route(["node", "agentcore", "feedback", ...args, "--region", REGION]); return { stdout: io.stdout(), stderr: io.stderr() }; @@ -64,7 +55,7 @@ describe("feedback (fixture-backed)", () => { test("submits text-only feedback and prints the result envelope", async () => { const { stdout } = await run( ["[agentcore-cli golden fixture] text submit — please ignore", "--yes", "--json"], - { fetchDir: "submit-text" }, + { fetch: fixtureFetch(join(FIXTURES, "submit-text")) }, ); matchGolden(FIXTURES, "submit-text.golden.json", stdout); @@ -83,36 +74,122 @@ describe("feedback (fixture-backed)", () => { "--yes", "--json", ], - { fetchDir: "submit-screenshot" }, + { fetch: fixtureFetch(join(FIXTURES, "submit-screenshot")) }, ); matchGolden(FIXTURES, "submit-screenshot.golden.json", stdout); expect(JSON.parse(stdout).success).toBe(true); }, 120_000); - // ── validation / consent errors (no network, no fixtures — like batch-evaluation's not-found) ── + // ── validation / consent errors (no network — like batch-evaluation's not-found) ── test("without --yes and without a TTY it fails rather than submitting", async () => { - await expect(run(["headless", "--json"])).rejects.toThrow(/--yes/); + await expect(run(["headless", "--json"], { fetch: neverFetch })).rejects.toThrow(/--yes/); }); test("declining the consent prompt cancels", async () => { - await expect(run(["no thanks"], { io: { isTTY: true, stdin: "n\n" } })).rejects.toBeInstanceOf( - UserCancellationError, - ); + await expect( + run(["no thanks"], { fetch: neverFetch, io: { isTTY: true, stdin: "n\n" } }), + ).rejects.toBeInstanceOf(UserCancellationError); }); test("an empty message is rejected", async () => { - await expect(run([" ", "--yes"])).rejects.toThrow(/cannot be empty/); + await expect(run([" ", "--yes"], { fetch: neverFetch })).rejects.toThrow(/cannot be empty/); }); test("a message over 1000 characters is rejected", async () => { - await expect(run(["x".repeat(1001), "--yes"])).rejects.toThrow(/1000 characters/); + await expect(run(["x".repeat(1001), "--yes"], { fetch: neverFetch })).rejects.toThrow( + /1000 characters/, + ); }); test("an explicitly-empty --screenshot is rejected", async () => { - await expect(run(["msg", "--screenshot", "", "--yes"])).rejects.toThrow( + await expect(run(["msg", "--screenshot", "", "--yes"], { fetch: neverFetch })).rejects.toThrow( /--screenshot requires a file path/, ); }); }); + +// The golden flow proves the rendered output but not the wire contract — fixtureFetch +// keys on method+path and ignores headers/body. These capturing-fetch tests assert the +// request contract (checksum + tagging headers, the object key) and response handling, +// the same pattern as core/gatewayInvoke.test.ts. +type Recorded = { url: string; method: string; headers: Headers; body: unknown }; + +function capturingFetch(canned: { presign?: string; form?: string; formStatus?: number }): { + fetch: CoreFetch; + calls: Recorded[]; +} { + const calls: Recorded[] = []; + const fetch = (async (input: Parameters[0], init?: Parameters[1]) => { + const url = String(input); + calls.push({ + url, + method: init?.method ?? "GET", + headers: new Headers(init?.headers), + body: init?.body, + }); + if (url.includes("/presignedurl")) { + return new Response(canned.presign ?? "", { status: 200 }); + } + if (url.includes("/form")) { + return new Response(canned.form ?? "{}", { + status: canned.formStatus ?? 200, + headers: { "content-type": "application/json" }, + }); + } + return new Response(null, { status: 200 }); // the S3 PUT + }) as unknown as CoreFetch; + return { fetch, calls }; +} + +const OK_FORM = JSON.stringify({ + reference: "agentcore-cli", + id: "id-1", + timestamp: "2026-01-01T00:00:00Z", +}); +const PRESIGN_URL = + "https://bucket.s3.us-east-1.amazonaws.com/us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png?X-Amz-Signature=sig"; + +describe("feedback (request contract)", () => { + test("screenshot flow sends checksum + NOT_SCANNED headers and the parsed object key", async () => { + const { fetch, calls } = capturingFetch({ presign: PRESIGN_URL, form: OK_FORM }); + await run(["with shot", "--screenshot", SHOT, "--yes", "--json"], { fetch }); + + expect( + calls.map((c) => + c.url.includes("/presignedurl") ? "presign" : c.url.includes("/form") ? "form" : "s3", + ), + ).toEqual(["presign", "s3", "form"]); + + const put = calls[1]!; + expect(put.method).toBe("PUT"); + expect(put.headers.get("x-amz-checksum-algorithm")).toBe("SHA256"); + expect(put.headers.get("x-amz-checksum-sha256")).toBeTruthy(); + expect(put.headers.get("x-amz-tagging")).toBe("scanstatus=NOT_SCANNED"); + + // The form references the exact object key parsed from the presigned URL path. + const form = JSON.parse(String(calls[2]!.body)); + const attachment = form.customerResponses.find( + (r: { response: { responseType: string } }) => r.response.responseType === "fileUpload", + ); + expect(attachment.response.responseValue).toEqual([ + "us-east-1/AgentCore/CLI/0.1.0/13052026/abc-123.png", + ]); + }); + + test("a malformed form response is rejected as an ApertureError", async () => { + const { fetch } = capturingFetch({ form: "{}" }); // missing id/timestamp/reference + await expect(run(["hi", "--yes", "--json"], { fetch })).rejects.toBeInstanceOf(ApertureError); + }); + + test("an invalid presign body fails before the upload (no PUT)", async () => { + const { fetch, calls } = capturingFetch({ presign: "not-a-url", form: OK_FORM }); + await expect( + run(["with shot", "--screenshot", SHOT, "--yes", "--json"], { fetch }), + ).rejects.toBeInstanceOf(ApertureError); + // Only the presign call happened — the object key is validated before the PUT. + expect(calls).toHaveLength(1); + expect(calls[0]!.url).toContain("/presignedurl"); + }); +}); From 0290388d514381a7e90b127501e7f5c300b82b7d Mon Sep 17 00:00:00 2001 From: jariy17 Date: Tue, 1 Sep 2026 16:21:47 +0000 Subject: [PATCH 13/14] refactor(feedback): make submit a FeedbackService class + drop code comments The submit logic is now a FeedbackService class whose constructor takes the injected fetch (the handler constructs it once), replacing the free function. Also removes the explanatory code comments across the feedback files and the two comments this PR added to shared files (handlers/index.tsx, testing/fixtures.tsx). --- .../feedback/feedback.fixture.test.tsx | 26 +- src/handlers/feedback/index.tsx | 27 +- src/handlers/feedback/submit.ts | 343 ++++++++---------- src/handlers/feedback/types.tsx | 4 - src/handlers/index.tsx | 2 - src/testing/fixtures.tsx | 4 - 6 files changed, 166 insertions(+), 240 deletions(-) diff --git a/src/handlers/feedback/feedback.fixture.test.tsx b/src/handlers/feedback/feedback.fixture.test.tsx index a90b55b61..b4d8776a3 100644 --- a/src/handlers/feedback/feedback.fixture.test.tsx +++ b/src/handlers/feedback/feedback.fixture.test.tsx @@ -14,24 +14,10 @@ import { UserCancellationError } from "../../errors"; import { ApertureError } from "./submit"; import type { CoreFetch } from "../../core/types"; -// Record with: RECORD=1 bun test src/handlers/feedback/feedback.fixture.test.tsx -// -// The two "submits …" tests are WRITES: a record run posts a REAL feedback -// submission to the Aperture public API (and, for the screenshot case, uploads -// shot.png through a real presigned S3 PUT) — there is no undo, same as the -// batch-evaluation evaluate/simulate fixtures. `fixtureFetch` sanitizes the -// recorded presign response, so no X-Amz-* signature is committed. Every other run -// replays the committed fixtures offline. -// -// Feedback calls the Aperture API through an injected fetch (fixtureFetch), passed -// via RootHandlerConfig — the handler owns the HTTP, not Core. Each submit test uses -// its own fixture subdir because fixtureFetch keys on method+path only, and both -// submits POST to the same /form path. const REGION = "us-east-1"; const FIXTURES = join(import.meta.dir, "__fixtures__"); const SHOT = join(FIXTURES, "shot.png"); -// A fetch that fails loudly: used for cases that must reject before any network. const neverFetch = (async () => { throw new Error("network should not be reached"); }) as unknown as CoreFetch; @@ -81,8 +67,6 @@ describe("feedback (fixture-backed)", () => { expect(JSON.parse(stdout).success).toBe(true); }, 120_000); - // ── validation / consent errors (no network — like batch-evaluation's not-found) ── - test("without --yes and without a TTY it fails rather than submitting", async () => { await expect(run(["headless", "--json"], { fetch: neverFetch })).rejects.toThrow(/--yes/); }); @@ -110,10 +94,6 @@ describe("feedback (fixture-backed)", () => { }); }); -// The golden flow proves the rendered output but not the wire contract — fixtureFetch -// keys on method+path and ignores headers/body. These capturing-fetch tests assert the -// request contract (checksum + tagging headers, the object key) and response handling, -// the same pattern as core/gatewayInvoke.test.ts. type Recorded = { url: string; method: string; headers: Headers; body: unknown }; function capturingFetch(canned: { presign?: string; form?: string; formStatus?: number }): { @@ -138,7 +118,7 @@ function capturingFetch(canned: { presign?: string; form?: string; formStatus?: headers: { "content-type": "application/json" }, }); } - return new Response(null, { status: 200 }); // the S3 PUT + return new Response(null, { status: 200 }); }) as unknown as CoreFetch; return { fetch, calls }; } @@ -168,7 +148,6 @@ describe("feedback (request contract)", () => { expect(put.headers.get("x-amz-checksum-sha256")).toBeTruthy(); expect(put.headers.get("x-amz-tagging")).toBe("scanstatus=NOT_SCANNED"); - // The form references the exact object key parsed from the presigned URL path. const form = JSON.parse(String(calls[2]!.body)); const attachment = form.customerResponses.find( (r: { response: { responseType: string } }) => r.response.responseType === "fileUpload", @@ -179,7 +158,7 @@ describe("feedback (request contract)", () => { }); test("a malformed form response is rejected as an ApertureError", async () => { - const { fetch } = capturingFetch({ form: "{}" }); // missing id/timestamp/reference + const { fetch } = capturingFetch({ form: "{}" }); await expect(run(["hi", "--yes", "--json"], { fetch })).rejects.toBeInstanceOf(ApertureError); }); @@ -188,7 +167,6 @@ describe("feedback (request contract)", () => { await expect( run(["with shot", "--screenshot", SHOT, "--yes", "--json"], { fetch }), ).rejects.toBeInstanceOf(ApertureError); - // Only the presign call happened — the object key is validated before the PUT. expect(calls).toHaveLength(1); expect(calls[0]!.url).toContain("/presignedurl"); }); diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx index 07a61dee1..0f5c759b8 100644 --- a/src/handlers/feedback/index.tsx +++ b/src/handlers/feedback/index.tsx @@ -4,16 +4,15 @@ import { argument, createHandler, flag } from "../../router"; import { JsonRendererKey } from "../../tui"; import { JsonKey } from "../keys.tsx"; import { InputValidationError, UserCancellationError } from "../../errors"; -import { CONSENT_TEXT, submitFeedback } from "./submit"; +import { CONSENT_TEXT, FeedbackService } from "./submit"; import type { AppIO } from "../../io"; import type { CoreFetch } from "../../core/types"; -export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => - createHandler({ +export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => { + const feedbackService = new FeedbackService(fetch); + return createHandler({ name: "feedback", description: "Send feedback about the AgentCore CLI to the team.", - // Length/empty validation lives solely in submitFeedback so one code path - // guards every caller; the arg is unconstrained here beyond being a string. arguments: [argument("message", "the feedback message to send", z.string())], flags: [ flag( @@ -28,8 +27,6 @@ export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => ), ], handle: async (ctx, flags, args) => { - // An explicitly-empty --screenshot "" is a mistake, not "no screenshot": - // reject it rather than silently submitting without an attachment. const screenshotPath = flags["screenshot"]; if (screenshotPath !== undefined && screenshotPath.trim() === "") { throw new InputValidationError("--screenshot requires a file path"); @@ -37,21 +34,16 @@ export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => await confirmConsent(io, ctx.require(JsonKey), flags.yes); - const result = await submitFeedback( - { - message: args["message"], - screenshot: screenshotPath ? { path: screenshotPath } : undefined, - }, - fetch, - ); + const result = await feedbackService.submitFeedback({ + message: args["message"], + screenshot: screenshotPath ? { path: screenshotPath } : undefined, + }); ctx.require(JsonRendererKey).renderJson({ success: true, ...result }); }, }); +}; -// Mirrors project/remove's confirmRemoveAll: --yes bypasses the prompt, a -// non-interactive session (or --json) fails rather than submitting without -// consent, and a decline (or SIGINT) raises UserCancellationError. async function confirmConsent(io: AppIO, jsonOutput: boolean, confirmed: boolean): Promise { if (confirmed) return; const canPrompt = !jsonOutput && io.stdin.isTTY && io.stdout.isTTY && io.stderr.isTTY; @@ -66,7 +58,6 @@ async function confirmConsent(io: AppIO, jsonOutput: boolean, confirmed: boolean } async function promptForConsent(io: AppIO): Promise { - // Prompt on stderr so --json / piped stdout stays a clean machine-readable stream. const readline = createInterface({ input: io.stdin, output: io.stderr }); try { const cancelled = new Promise((_resolve, reject) => { diff --git a/src/handlers/feedback/submit.ts b/src/handlers/feedback/submit.ts index 9b7e83198..9a41be6c3 100644 --- a/src/handlers/feedback/submit.ts +++ b/src/handlers/feedback/submit.ts @@ -7,9 +7,6 @@ import { PACKAGE_VERSION } from "../../constants"; import type { CoreFetch } from "../../core/types"; import type { FeedbackSubmissionResult, SubmitFeedbackInput } from "./types"; -// Aperture public feedback API. These are commercial-partition (.aws.dev) endpoints -// with no partition variant, so feedback is unavailable in GovCloud/China — carried -// over from the pre-refactor CLI, flagged here rather than silently. const INGESTION_URL = "https://ingestion.aperture-public-api.feedback.console.aws.dev/form"; const PRESIGN_URL = "https://presignedurl.aperture-public-api.feedback.console.aws.dev/presignedurl"; @@ -24,15 +21,12 @@ const MESSAGE_MAX_LENGTH = 1000; const MAX_SCREENSHOT_BYTES = 100 * 1024 * 1024; const ALLOWED_SCREENSHOT_EXTENSIONS = [".png", ".jpg", ".jpeg"] as const; -// Rendered by the feedback command's consent prompt before every submission. 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."; -// Extends the CLI error hierarchy (the pre-refactor ApertureError extended plain -// Error, so telemetry classified it as unknown) so failures record error_source=service. export class ApertureError extends AgentCoreCLIError { constructor( message: string, @@ -70,214 +64,189 @@ type ApertureFormPayload = { metadataList: { key: string; value: string }[]; }; -// submitFeedback posts a message (and optional screenshot) to the Aperture public -// feedback API using the injected fetch. It lives with the handler rather than in -// Core because feedback is an outbound product-API call, not work in the user's AWS -// account, and it makes no AWS SDK calls. -export async function submitFeedback( - input: SubmitFeedbackInput, - fetch: CoreFetch, -): Promise { - const message = input.message.trim(); - if (!message) { - throw new InputValidationError("Feedback message cannot be empty."); - } - if (message.length > MESSAGE_MAX_LENGTH) { - throw new InputValidationError( - `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, - ); - } +export class FeedbackService { + constructor(private readonly fetch: CoreFetch) {} - const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; + async submitFeedback(input: SubmitFeedbackInput): Promise { + const message = input.message.trim(); + if (!message) { + throw new InputValidationError("Feedback message cannot be empty."); + } + if (message.length > MESSAGE_MAX_LENGTH) { + throw new InputValidationError( + `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, + ); + } - let screenshotReference: string | undefined; - if (input.screenshot) { - const file = await loadScreenshot(input.screenshot.path); - const presignedUrl = await fetchPresignedUrl( - fetch, - { - category: FORM_CATEGORY, - name: FORM_NAME, - version: FORM_VERSION, - fileName: file.fileName, - fileSize: file.size, - uploadFileSHA256: file.sha256Base64, - }, - userAgent, - ); - // Derive (and validate) the object key BEFORE the upload: fetch(badUrl) would - // otherwise throw a bare TypeError inside uploadFileToS3, masking the classified - // ApertureError and wasting a PUT against an unusable reference. - screenshotReference = objectKeyFromPresignedUrl(presignedUrl); - await uploadFileToS3( - fetch, - presignedUrl, - file.buffer, - file.contentType, - file.sha256Base64, - userAgent, - ); - } + const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; - const payload = buildFeedbackPayload({ message, screenshotReference }); - return submitForm(fetch, payload, userAgent); -} + let screenshotReference: string | undefined; + if (input.screenshot) { + const file = await this.loadScreenshot(input.screenshot.path); + const presignedUrl = await this.fetchPresignedUrl( + { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + fileName: file.fileName, + fileSize: file.size, + uploadFileSHA256: file.sha256Base64, + }, + userAgent, + ); + screenshotReference = objectKeyFromPresignedUrl(presignedUrl); + await this.uploadFileToS3( + presignedUrl, + file.buffer, + file.contentType, + file.sha256Base64, + userAgent, + ); + } -// Aperture returns the presigned URL as a plain-text body (not JSON). -async function fetchPresignedUrl( - fetch: CoreFetch, - request: { - category: string; - name: string; - version: string; - fileName: string; - fileSize: number; - uploadFileSHA256: string; - }, - userAgent: string, -): Promise { - const response = await fetch(PRESIGN_URL, { - method: "POST", - headers: { "content-type": "application/json", "user-agent": userAgent }, - body: JSON.stringify(request), - }); - if (!response.ok) { - throw new ApertureError( - `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, - response.status, - await response.text().catch(() => ""), - ); + const payload = buildFeedbackPayload({ message, screenshotReference }); + return this.submitForm(payload, userAgent); } - return (await response.text()).trim(); -} -// Aperture's bucket policy requires the SHA-256 checksum headers and a tag marking -// the object as not yet AV-scanned; omitting either is rejected. -async function uploadFileToS3( - fetch: CoreFetch, - 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, + private async fetchPresignedUrl( + request: { + category: string; + name: string; + version: string; + fileName: string; + fileSize: number; + uploadFileSHA256: string; }, - body: fileBuffer, - }); - if (!response.ok) { - throw new ApertureError( - `Failed to upload screenshot (HTTP ${response.status}).`, - response.status, - await response.text().catch(() => ""), - ); + userAgent: string, + ): Promise { + const response = await this.fetch(PRESIGN_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(request), + }); + if (!response.ok) { + throw new ApertureError( + `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); + } + return (await response.text()).trim(); } -} -async function submitForm( - fetch: CoreFetch, - payload: ApertureFormPayload, - userAgent: string, -): Promise { - const response = await fetch(INGESTION_URL, { - method: "POST", - headers: { "content-type": "application/json", "user-agent": userAgent }, - body: JSON.stringify(payload), - }); - if (!response.ok) { - const body = await response.text().catch(() => ""); - throw new ApertureError(mapStatusToMessage(response.status, body), response.status, body); + private async uploadFileToS3( + presignedUrl: string, + fileBuffer: Uint8Array, + contentType: string, + base64Sha256: string, + userAgent: string, + ): Promise { + const response = await this.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) { + throw new ApertureError( + `Failed to upload screenshot (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); + } } - // A 2xx with a non-JSON or unexpectedly-shaped body is a service fault; validate - // rather than casting so a partial response never surfaces `id: undefined`. - const data = (await response - .json() - .catch(() => null)) as Partial | null; - if ( - !data || - typeof data.id !== "string" || - typeof data.timestamp !== "string" || - typeof data.reference !== "string" - ) { - throw new ApertureError("Feedback service returned an unexpected response."); + + private async submitForm( + payload: ApertureFormPayload, + userAgent: string, + ): Promise { + const response = await this.fetch(INGESTION_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(payload), + }); + if (!response.ok) { + const body = await response.text().catch(() => ""); + throw new ApertureError(mapStatusToMessage(response.status, body), response.status, body); + } + const data = (await response + .json() + .catch(() => null)) as Partial | null; + if ( + !data || + typeof data.id !== "string" || + typeof data.timestamp !== "string" || + typeof data.reference !== "string" + ) { + throw new ApertureError("Feedback service returned an unexpected response."); + } + return { id: data.id, timestamp: data.timestamp, reference: data.reference }; } - return { id: data.id, timestamp: data.timestamp, reference: data.reference }; -} -async function loadScreenshot(rawFilePath: string): Promise { - const filePath = expandTilde(rawFilePath); + private async loadScreenshot(rawFilePath: string): Promise { + const filePath = expandTilde(rawFilePath); - let stats: Awaited>; - try { - stats = await stat(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - if (stats.isDirectory()) { - throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); - } - if (!stats.isFile()) { - throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); - } - // Reject oversized files from stat before readFile, so a hostile/huge file is - // never loaded into memory just to be rejected. - if (stats.size > MAX_SCREENSHOT_BYTES) { - const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); - throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); - } + let stats: Awaited>; + try { + stats = await stat(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (stats.isDirectory()) { + throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); + } + if (!stats.isFile()) { + throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); + } + if (stats.size > MAX_SCREENSHOT_BYTES) { + const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); + throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } - const ext = path.extname(filePath).toLowerCase(); - if ( - !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) - ) { - throw new InputValidationError( - `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, - ); - } + const ext = path.extname(filePath).toLowerCase(); + if ( + !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) + ) { + throw new InputValidationError( + `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, + ); + } - let buffer: Buffer; - try { - buffer = await readFile(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); + let buffer: Buffer; + try { + buffer = await readFile(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + return { + buffer: new Uint8Array(buffer), + fileName: path.basename(filePath), + contentType: ext === ".png" ? "image/png" : "image/jpeg", + sha256Base64: createHash("sha256").update(buffer).digest("base64"), + size: buffer.byteLength, + }; } - return { - buffer: new Uint8Array(buffer), - fileName: path.basename(filePath), - contentType: ext === ".png" ? "image/png" : "image/jpeg", - sha256Base64: createHash("sha256").update(buffer).digest("base64"), - size: buffer.byteLength, - }; } -// Expand a leading ~ / ~/... to $HOME. Node's fs APIs don't expand tildes (the -// shell normally does), so a quoted path like "~/shot.png" would otherwise ENOENT. function expandTilde(filePath: string): string { if (filePath === "~") return os.homedir(); if (filePath.startsWith("~/")) return path.join(os.homedir(), filePath.slice(2)); return filePath; } -// The presigned URL's path IS the S3 object key the form must reference; fabricating -// one client-side risks pointing at a nonexistent object if Aperture's bucket layout -// or region shifts. function objectKeyFromPresignedUrl(presignedUrl: string): string { try { return decodeURIComponent(new URL(presignedUrl).pathname.replace(/^\/+/, "")); } catch { - // A 2xx presign body that isn't a URL is a service fault, not a bare TypeError — - // classify it so telemetry attributes it to the service, not internal. throw new ApertureError("Feedback service returned an invalid screenshot upload URL."); } } @@ -301,8 +270,6 @@ function buildFeedbackPayload(input: { }); } - // Aperture rejects unknown metadata keys with HTTP 400; only cli-version and os - // are registered in the form template, so node version + mode ride in `location`. return { category: FORM_CATEGORY, name: FORM_NAME, diff --git a/src/handlers/feedback/types.tsx b/src/handlers/feedback/types.tsx index 6e822975e..988370a89 100644 --- a/src/handlers/feedback/types.tsx +++ b/src/handlers/feedback/types.tsx @@ -1,7 +1,3 @@ -// Concrete request/result data shapes for the feedback command. These are data, -// not behavioral contracts, so they are type aliases (interfaces are reserved for -// the Core*Client contracts elsewhere in handlers/). - export type ScreenshotInput = { path: string }; export type SubmitFeedbackInput = { diff --git a/src/handlers/index.tsx b/src/handlers/index.tsx index 3fbd8b087..b05af9792 100644 --- a/src/handlers/index.tsx +++ b/src/handlers/index.tsx @@ -22,8 +22,6 @@ export interface RootHandlerConfig { io: AppIO; logger: Logger; globalConfigAccessor: GlobalConfigAccessor; - // Outbound HTTP for handlers that call non-AWS APIs directly (e.g. feedback → - // Aperture). Defaults to the global fetch; tests inject a fixture/capturing fetch. fetch?: CoreFetch; } diff --git a/src/testing/fixtures.tsx b/src/testing/fixtures.tsx index bc5f994ea..663e7ff89 100644 --- a/src/testing/fixtures.tsx +++ b/src/testing/fixtures.tsx @@ -303,10 +303,6 @@ export function fixtureFetch(dir: string): CoreFetch { mkdirSync(dir, { recursive: true }); const response = await globalThis.fetch(input, init); const body = await response.text(); - // Persist a queryless copy so a recorded presign response never commits its - // X-Amz-* signature to disk, but return the original body so the live upload - // still works during the record run. The fixture key is the object path, which - // sanitizing does not change, so replay is unaffected. const fixture: FetchFixture = { status: response.status, statusText: response.statusText, From fa466da981fe6a872cfed4b046d29ee107167a9d Mon Sep 17 00:00:00 2001 From: jariy17 Date: Tue, 1 Sep 2026 17:21:31 +0000 Subject: [PATCH 14/14] refactor(feedback): module submitFeedback + inject fetch via Core Reverts the FeedbackService class back to a module submitFeedback(input, fetch) function. The handler gets its fetch from the Core contract (core.fetch, exposed on CoreClient's existing injected fetch) instead of a bespoke RootHandlerConfig field, so the golden test injects via the familiar CoreClient construction (fixtureFactories + fixtureFetch). TestCoreClient gains a fetch stub. --- src/core/index.tsx | 2 + .../feedback/feedback.fixture.test.tsx | 11 +- src/handlers/feedback/index.tsx | 21 +- src/handlers/feedback/submit.ts | 314 +++++++++--------- src/handlers/index.tsx | 5 +- src/handlers/types.tsx | 3 + src/testing/TestCoreClient.tsx | 5 +- 7 files changed, 188 insertions(+), 173 deletions(-) diff --git a/src/core/index.tsx b/src/core/index.tsx index 5a6bab9fb..9ae2f021a 100644 --- a/src/core/index.tsx +++ b/src/core/index.tsx @@ -76,6 +76,7 @@ export class CoreClient implements AwsClients { readonly projectManager: ProjectManager; readonly describeBedrockAgent: DescribeBedrockAgent; + readonly fetch: CoreFetch; constructor(config: CoreClientConfig) { this.createControlClient = config.createControlClient; @@ -84,6 +85,7 @@ export class CoreClient implements AwsClients { this.createLogsClient = config.createLogsClient; this.logger = config.logger; const fetch = config.fetch ?? globalThis.fetch; + this.fetch = fetch; this.runtime = new RuntimeClient(this, fetch, this.logger.child({ module: "runtime" })); this.gateway = new GatewayClient(this, fetch, this.logger.child({ module: "gateway" })); // EvalClient shares the injected fetch: dataset content is served from a diff --git a/src/handlers/feedback/feedback.fixture.test.tsx b/src/handlers/feedback/feedback.fixture.test.tsx index b4d8776a3..d0657a22d 100644 --- a/src/handlers/feedback/feedback.fixture.test.tsx +++ b/src/handlers/feedback/feedback.fixture.test.tsx @@ -1,11 +1,12 @@ import { describe, expect, test } from "bun:test"; import { join } from "node:path"; +import { CoreClient } from "../../core"; import { createRootHandler } from "../index"; import { createSilentLogger, + fixtureFactories, fixtureFetch, matchGolden, - TestCoreClient, TestGlobalConfigAccessor, testIO, type TestIOOptions, @@ -27,11 +28,15 @@ async function run( opts: { fetch?: CoreFetch; io?: TestIOOptions } = {}, ): Promise<{ stdout: string; stderr: string }> { const io = testIO(opts.io); - const root = createRootHandler(new TestCoreClient(), { + const core = new CoreClient({ + ...fixtureFactories(FIXTURES), + logger: createSilentLogger(), + fetch: opts.fetch ?? neverFetch, + }); + const root = createRootHandler(core, { io: io.io, logger: createSilentLogger(), globalConfigAccessor: new TestGlobalConfigAccessor(), - fetch: opts.fetch, }); await root.route(["node", "agentcore", "feedback", ...args, "--region", REGION]); return { stdout: io.stdout(), stderr: io.stderr() }; diff --git a/src/handlers/feedback/index.tsx b/src/handlers/feedback/index.tsx index 0f5c759b8..ad3bc5e95 100644 --- a/src/handlers/feedback/index.tsx +++ b/src/handlers/feedback/index.tsx @@ -4,13 +4,12 @@ import { argument, createHandler, flag } from "../../router"; import { JsonRendererKey } from "../../tui"; import { JsonKey } from "../keys.tsx"; import { InputValidationError, UserCancellationError } from "../../errors"; -import { CONSENT_TEXT, FeedbackService } from "./submit"; +import { CONSENT_TEXT, submitFeedback } from "./submit"; import type { AppIO } from "../../io"; -import type { CoreFetch } from "../../core/types"; +import type { Core } from "../types.tsx"; -export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => { - const feedbackService = new FeedbackService(fetch); - return createHandler({ +export const createFeedbackHandler = (core: Core, io: AppIO) => + createHandler({ name: "feedback", description: "Send feedback about the AgentCore CLI to the team.", arguments: [argument("message", "the feedback message to send", z.string())], @@ -34,15 +33,17 @@ export const createFeedbackHandler = (io: AppIO, fetch: CoreFetch) => { await confirmConsent(io, ctx.require(JsonKey), flags.yes); - const result = await feedbackService.submitFeedback({ - message: args["message"], - screenshot: screenshotPath ? { path: screenshotPath } : undefined, - }); + const result = await submitFeedback( + { + message: args["message"], + screenshot: screenshotPath ? { path: screenshotPath } : undefined, + }, + core.fetch, + ); ctx.require(JsonRendererKey).renderJson({ success: true, ...result }); }, }); -}; async function confirmConsent(io: AppIO, jsonOutput: boolean, confirmed: boolean): Promise { if (confirmed) return; diff --git a/src/handlers/feedback/submit.ts b/src/handlers/feedback/submit.ts index 9a41be6c3..a6369d944 100644 --- a/src/handlers/feedback/submit.ts +++ b/src/handlers/feedback/submit.ts @@ -64,177 +64,181 @@ type ApertureFormPayload = { metadataList: { key: string; value: string }[]; }; -export class FeedbackService { - constructor(private readonly fetch: CoreFetch) {} +export async function submitFeedback( + input: SubmitFeedbackInput, + fetch: CoreFetch, +): Promise { + const message = input.message.trim(); + if (!message) { + throw new InputValidationError("Feedback message cannot be empty."); + } + if (message.length > MESSAGE_MAX_LENGTH) { + throw new InputValidationError( + `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, + ); + } - async submitFeedback(input: SubmitFeedbackInput): Promise { - const message = input.message.trim(); - if (!message) { - throw new InputValidationError("Feedback message cannot be empty."); - } - if (message.length > MESSAGE_MAX_LENGTH) { - throw new InputValidationError( - `Feedback message must be ${MESSAGE_MAX_LENGTH} characters or fewer.`, - ); - } + const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; - const userAgent = `AgentCoreCLI/${PACKAGE_VERSION} (${process.platform} ${os.release()}; node/${process.version})`; + let screenshotReference: string | undefined; + if (input.screenshot) { + const file = await loadScreenshot(input.screenshot.path); + const presignedUrl = await fetchPresignedUrl( + fetch, + { + category: FORM_CATEGORY, + name: FORM_NAME, + version: FORM_VERSION, + fileName: file.fileName, + fileSize: file.size, + uploadFileSHA256: file.sha256Base64, + }, + userAgent, + ); + screenshotReference = objectKeyFromPresignedUrl(presignedUrl); + await uploadFileToS3( + fetch, + presignedUrl, + file.buffer, + file.contentType, + file.sha256Base64, + userAgent, + ); + } - let screenshotReference: string | undefined; - if (input.screenshot) { - const file = await this.loadScreenshot(input.screenshot.path); - const presignedUrl = await this.fetchPresignedUrl( - { - category: FORM_CATEGORY, - name: FORM_NAME, - version: FORM_VERSION, - fileName: file.fileName, - fileSize: file.size, - uploadFileSHA256: file.sha256Base64, - }, - userAgent, - ); - screenshotReference = objectKeyFromPresignedUrl(presignedUrl); - await this.uploadFileToS3( - presignedUrl, - file.buffer, - file.contentType, - file.sha256Base64, - userAgent, - ); - } + const payload = buildFeedbackPayload({ message, screenshotReference }); + return submitForm(fetch, payload, userAgent); +} - const payload = buildFeedbackPayload({ message, screenshotReference }); - return this.submitForm(payload, userAgent); +async function fetchPresignedUrl( + fetch: CoreFetch, + request: { + category: string; + name: string; + version: string; + fileName: string; + fileSize: number; + uploadFileSHA256: string; + }, + userAgent: string, +): Promise { + const response = await fetch(PRESIGN_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(request), + }); + if (!response.ok) { + throw new ApertureError( + `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); } + return (await response.text()).trim(); +} - private async fetchPresignedUrl( - request: { - category: string; - name: string; - version: string; - fileName: string; - fileSize: number; - uploadFileSHA256: string; +async function uploadFileToS3( + fetch: CoreFetch, + 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, }, - userAgent: string, - ): Promise { - const response = await this.fetch(PRESIGN_URL, { - method: "POST", - headers: { "content-type": "application/json", "user-agent": userAgent }, - body: JSON.stringify(request), - }); - if (!response.ok) { - throw new ApertureError( - `Failed to fetch screenshot upload URL (HTTP ${response.status}).`, - response.status, - await response.text().catch(() => ""), - ); - } - return (await response.text()).trim(); + body: fileBuffer, + }); + if (!response.ok) { + throw new ApertureError( + `Failed to upload screenshot (HTTP ${response.status}).`, + response.status, + await response.text().catch(() => ""), + ); } +} - private async uploadFileToS3( - presignedUrl: string, - fileBuffer: Uint8Array, - contentType: string, - base64Sha256: string, - userAgent: string, - ): Promise { - const response = await this.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) { - throw new ApertureError( - `Failed to upload screenshot (HTTP ${response.status}).`, - response.status, - await response.text().catch(() => ""), - ); - } +async function submitForm( + fetch: CoreFetch, + payload: ApertureFormPayload, + userAgent: string, +): Promise { + const response = await fetch(INGESTION_URL, { + method: "POST", + headers: { "content-type": "application/json", "user-agent": userAgent }, + body: JSON.stringify(payload), + }); + if (!response.ok) { + const body = await response.text().catch(() => ""); + throw new ApertureError(mapStatusToMessage(response.status, body), response.status, body); } - - private async submitForm( - payload: ApertureFormPayload, - userAgent: string, - ): Promise { - const response = await this.fetch(INGESTION_URL, { - method: "POST", - headers: { "content-type": "application/json", "user-agent": userAgent }, - body: JSON.stringify(payload), - }); - if (!response.ok) { - const body = await response.text().catch(() => ""); - throw new ApertureError(mapStatusToMessage(response.status, body), response.status, body); - } - const data = (await response - .json() - .catch(() => null)) as Partial | null; - if ( - !data || - typeof data.id !== "string" || - typeof data.timestamp !== "string" || - typeof data.reference !== "string" - ) { - throw new ApertureError("Feedback service returned an unexpected response."); - } - return { id: data.id, timestamp: data.timestamp, reference: data.reference }; + const data = (await response + .json() + .catch(() => null)) as Partial | null; + if ( + !data || + typeof data.id !== "string" || + typeof data.timestamp !== "string" || + typeof data.reference !== "string" + ) { + throw new ApertureError("Feedback service returned an unexpected response."); } + return { id: data.id, timestamp: data.timestamp, reference: data.reference }; +} - private async loadScreenshot(rawFilePath: string): Promise { - const filePath = expandTilde(rawFilePath); +async function loadScreenshot(rawFilePath: string): Promise { + const filePath = expandTilde(rawFilePath); - let stats: Awaited>; - try { - stats = await stat(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - if (stats.isDirectory()) { - throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); - } - if (!stats.isFile()) { - throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); - } - if (stats.size > MAX_SCREENSHOT_BYTES) { - const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); - throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); - } + let stats: Awaited>; + try { + stats = await stat(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + if (stats.isDirectory()) { + throw new InputValidationError(`Screenshot path is a directory, not a file: ${filePath}`); + } + if (!stats.isFile()) { + throw new InputValidationError(`Screenshot path is not a regular file: ${filePath}`); + } + if (stats.size > MAX_SCREENSHOT_BYTES) { + const sizeMb = (stats.size / (1024 * 1024)).toFixed(1); + throw new InputValidationError(`Screenshot is ${sizeMb} MB; maximum allowed size is 100 MB.`); + } - const ext = path.extname(filePath).toLowerCase(); - if ( - !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) - ) { - throw new InputValidationError( - `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, - ); - } + const ext = path.extname(filePath).toLowerCase(); + if ( + !ALLOWED_SCREENSHOT_EXTENSIONS.includes(ext as (typeof ALLOWED_SCREENSHOT_EXTENSIONS)[number]) + ) { + throw new InputValidationError( + `Screenshot must be one of: ${ALLOWED_SCREENSHOT_EXTENSIONS.join(", ")}.`, + ); + } - let buffer: Buffer; - try { - buffer = await readFile(filePath); - } catch (err) { - throw new InputValidationError( - `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, - ); - } - return { - buffer: new Uint8Array(buffer), - fileName: path.basename(filePath), - contentType: ext === ".png" ? "image/png" : "image/jpeg", - sha256Base64: createHash("sha256").update(buffer).digest("base64"), - size: buffer.byteLength, - }; + let buffer: Buffer; + try { + buffer = await readFile(filePath); + } catch (err) { + throw new InputValidationError( + `Could not read screenshot at ${filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); } + return { + buffer: new Uint8Array(buffer), + fileName: path.basename(filePath), + contentType: ext === ".png" ? "image/png" : "image/jpeg", + sha256Base64: createHash("sha256").update(buffer).digest("base64"), + size: buffer.byteLength, + }; } function expandTilde(filePath: string): string { diff --git a/src/handlers/index.tsx b/src/handlers/index.tsx index b05af9792..cd03e75f2 100644 --- a/src/handlers/index.tsx +++ b/src/handlers/index.tsx @@ -13,7 +13,6 @@ import { renderTui } from "../tui"; import { withRegion, withJsonRenderer, withLogging, withGlobalConfigAccessor } from "../middleware"; import type { AppIO } from "../io"; import type { Core } from "./types.tsx"; -import type { CoreFetch } from "../core/types"; import type { Logger } from "../logging"; import type { GlobalConfigAccessor } from "../globalConfig"; import { PACKAGE_VERSION } from "../constants"; @@ -22,12 +21,10 @@ export interface RootHandlerConfig { io: AppIO; logger: Logger; globalConfigAccessor: GlobalConfigAccessor; - fetch?: CoreFetch; } export function createRootHandler(core: Core, config: RootHandlerConfig): Router { const { io, logger } = config; - const fetch = config.fetch ?? globalThis.fetch; const root = new Router("agentcore", "the platform for production AI agents"); // `agentcore --version` prints the build-time package version. @@ -57,7 +54,7 @@ export function createRootHandler(core: Core, config: RootHandlerConfig): Router root.handler(createMemoryHandler(core, io)); root.handler(createGatewayHandler(core, io)); root.handler(createEvalHandler(core, io)); - root.handler(createFeedbackHandler(io, fetch)); + root.handler(createFeedbackHandler(core, io)); root.handler(createConfigHandler()); root.handler(createProjectHandler({ core, io })); diff --git a/src/handlers/types.tsx b/src/handlers/types.tsx index 3d76827b8..570ccb1b0 100644 --- a/src/handlers/types.tsx +++ b/src/handlers/types.tsx @@ -5,6 +5,7 @@ import type { CoreIdentityClient } from "./identity/types.tsx"; import type { CoreMemoryClient } from "./memory/types.tsx"; import type { CoreObservabilityClient, CoreRuntimeClient } from "./runtime/types.tsx"; import type { Context } from "../router"; +import type { CoreFetch } from "../core/types"; import type { ProjectManager } from "./project/types.ts"; import type { DescribeBedrockAgent } from "../core/project/bedrockAgent"; @@ -19,6 +20,8 @@ export interface Core { projectManager: ProjectManager; /** Describes a Bedrock Agent + alias for `--type import`. */ describeBedrockAgent: DescribeBedrockAgent; + /** Shared outbound HTTP for handlers that call non-AWS APIs directly (e.g. feedback → Aperture). */ + fetch: CoreFetch; } // ScreenProps is the common prop set every TUI screen receives. `ctx` carries the diff --git a/src/testing/TestCoreClient.tsx b/src/testing/TestCoreClient.tsx index a6da20a9e..ce526cc4f 100644 --- a/src/testing/TestCoreClient.tsx +++ b/src/testing/TestCoreClient.tsx @@ -168,7 +168,7 @@ import type { } from "../handlers/eval/types"; import { isTerminalStatus } from "../core/batchEvaluationResults"; import { abortable } from "../core/abortable"; -import type { CoreOptions, CreateCloudFormationClient } from "../core/types"; +import type { CoreFetch, CoreOptions, CreateCloudFormationClient } from "../core/types"; import type { Project, ProjectManager } from "../handlers/project/types"; import type { Logger } from "../logging"; import type { ReadWriteJson } from "../io"; @@ -2306,6 +2306,9 @@ export class TestCoreClient implements Core { readonly gateway = new TestGatewayClient(); readonly eval = new TestEvalClient(); readonly observability = new TestObservabilityClient(); + fetch: CoreFetch = (async () => { + throw new Error("TestCoreClient.fetch is not configured; set it in the test that needs it"); + }) as unknown as CoreFetch; readonly projectManager: ProjectManager; // Commands the project manager would have run (npm install, git init, ...),