feat(feedback): port the feedback command to the refactor architecture - #2149
feat(feedback): port the feedback command to the refactor architecture#2149jariy17 wants to merge 9 commits into
Conversation
Adds `agentcore feedback <message> [--screenshot <path>] [--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
agentcore-devx-automation
Bot
added
agentcore-harness-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
![agentcore-devx-automation[bot]](https://avatars.githubusercontent.com/u/2232217?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
The port is a clean, contained addition:
- The consent flow mirrors
src/handlers/project/remove'sconfirmRemoveAll/promptForRemoveAllverbatim, including the--yes/--json/TTY handling and theSIGINT+closecancellation pattern — good reuse of an established convention. - Tests exercise the real handler through
CoreClient+createRootHandler, mocking only at the true I/O boundary (the injectedfetch) and using a real temp dir for the screenshot fixture. No excessive mocking. ApertureErrorextendsAgentCoreCLIErrorwithERROR_SOURCE.SERVICE, so failures are classified rather than falling through as unknown.- Screenshot handling correctly parses the S3 object key from the presigned URL path, sends
x-amz-checksum-sha256+scanstatus=NOT_SCANNED, and validates extension/size/regular-file up front. - Telemetry: I checked
src/handlers/**andsrc/router/**and the refactored handler architecture does not yet wire telemetry at the handler layer, so there is nothing to instrument here that other handlers are doing.
A couple of small things I noticed but that don't block merge:
FeedbackClient'sclients: AwsClientsconstructor arg is unused — feedback lives outside the SDK seam. Could be dropped for clarity later.submitFormcastsresponse.json()directly toFeedbackSubmissionResultwith no runtime validation; a partial response would silently be accepted.loadScreenshotreads the whole file before checkingMAX_SCREENSHOT_BYTES; you alreadystatthe file, so gating onstats.sizefirst would avoid loading a >100 MB file into memory only to reject it.
None of these require changes before merging.
agentcore-devx-automation
Bot
removed
the
agentcore-harness-reviewing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2149 +/- ##
============================================
- Coverage 97.16% 97.13% -0.04%
============================================
Files 495 497 +2
Lines 32676 32973 +297
============================================
+ Hits 31751 32027 +276
- Misses 925 946 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
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.
…ient 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.
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.
--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.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
The rationale now lives on the FeedbackClient constructor in core/feedback.tsx.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
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.
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
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).
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
aidandaly24
left a comment
aidandaly24
left a comment
There was a problem hiding this comment.
Left a few comments around compatibility and the request/fixture contracts.
| }, | ||
| userAgent, | ||
| ); | ||
| await this.uploadFileToS3( |
There was a problem hiding this comment.
I think the new invalid-presign handling runs too late. uploadFileToS3() receives the raw body before objectKeyFromPresignedUrl() validates it; with a 200 not-a-url response I still got TypeError: fetch() URL is invalid, so the intended ApertureError never runs. Parsing the object key before the PUT should fix the classification and avoid uploading before the reference is known to be usable.
| expect(result.reference).toBe("agentcore-cli"); | ||
| }, 120_000); | ||
|
|
||
| test("submits feedback with a screenshot (presign → S3 PUT → form)", async () => { |
There was a problem hiding this comment.
I do not think this golden test proves the request contract in its name. fixtureFetch keys only on method/path and ignores request headers and bodies; I replayed the PUT with no checksum/tagging headers and the form POST with {}, and both returned 200. I think we should keep the golden flow but restore a focused injected-fetch test for the checksum headers, scanstatus=NOT_SCANNED, and attachment object key.
| // 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 |
There was a problem hiding this comment.
Can we avoid making presigned URL cleanup a manual recording step? fixtureFetch writes response.text() verbatim, so RECORD=1 puts the live X-Amz-* query on disk before this cleanup can happen. A recording sanitizer could write the queryless URL while still returning the original URL to the live upload flow.
| @@ -0,0 +1,26 @@ | |||
| import type { CoreOptions } from "../../core/types"; | |||
|
|
|||
| export interface ScreenshotInput { | |||
There was a problem hiding this comment.
Can ScreenshotInput, SubmitFeedbackInput, and FeedbackSubmissionResult be type aliases? These are concrete request/result data shapes, while CoreFeedbackClient is the behavioral interface. The neighboring handler type modules use interfaces only for Core*Client contracts and types for determinate data; the internal records in core/feedback.tsx should probably follow the same pattern.
| description: "Send feedback about the AgentCore CLI to the team.", | ||
| // 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())], |
There was a problem hiding this comment.
Is the no-argument feedback wizard intentionally out of scope? Released v0.28.1 uses optional [message] and opens FeedbackScreen; this makes <message> required, and I confirmed bare agentcore feedback now exits 2. If this is staged, I think the compatibility gap should be tracked or called out; otherwise this should preserve the optional route.
There was a problem hiding this comment.
Let's not increase scope. Leaving out the TUI makes sense here.
| metadataList: { key: string; value: string }[]; | ||
| } | ||
|
|
||
| export class FeedbackClient implements CoreFeedbackClient { |
There was a problem hiding this comment.
Should this be a part of Core? I'm not so sure it should. Feedback seems like a different concern from doing stuff in the user's AWS account.
| } | ||
| } | ||
|
|
||
| async function readBody(response: Response): Promise<string> { |
There was a problem hiding this comment.
What value is this adding?
| description: "Send feedback about the AgentCore CLI to the team.", | ||
| // 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())], |
There was a problem hiding this comment.
Let's not increase scope. Leaving out the TUI makes sense here.
4 participants
What
Implements
agentcore feedbackusing a dedicated client and Handler.Command surface
<message>— required (max 1000 chars).--screenshot <path>— optional PNG/JPG, ≤100 MB.--yes— accept the AWS Customer Agreement and skip the consent prompt.--json— inherited global flag; envelope output.Screenshot flow: presign
POST→ S3PUT(SHA256 checksum +scanstatus=NOT_SCANNEDtag) → formPOST, referencing the object key parsed from the presigned URL (never fabricated).Files
New:
src/core/feedback.tsx(FeedbackClient +ApertureErrorextendingAgentCoreCLIErrorwithERROR_SOURCE.SERVICE),src/handlers/feedback/{types,index,feedback.test}.tsx.Wired:
src/handlers/types.tsx(Core.feedback),src/core/index.tsx(CoreClient.feedback, injected fetch),src/handlers/index.tsx(root registration),src/testing/TestCoreClient.tsx(TestFeedbackClient).Testing
bun test src/handlers/feedback— 7/7 pass (consent y/n, non-TTY guard, empty/oversized message, screenshot 3-call flow with checksum/tag headers + object-key assertion).tsc --noEmitclean, oxlint clean.f36eb155-…,2e32ed65-…,3525eaf4-…, allreference: agentcore-cli.