feat(payment): add connector credential rotation - #2343
aidandaly24 merged 8 commits into
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Small, additive change that fits cleanly with the existing payment connector command family:
- Handler mirrors the shape of the sibling
get/listcommands, and mocks at the AWS SDK client boundary (no over-mocking). - Zod validation covers the enum values, minimum selection, and duplicate rejection — the negative tests exercise all three.
- Both single-secret and combined-secret paths are covered, plus error propagation.
- Docs call out the non-atomicity and wallet-secret disruption risk, which is the right place to surface it for a command that has no TUI confirmation step.
Minor observations (non-blocking, author's call):
- The test lives in
payment.read.test.tsxbut rotation is a mutating call; if you care about the naming split you may want to move the new cases into apayment.write.test.tsx(or rename the file). Not required. - No telemetry instrumentation, but the sibling read commands don't emit any either — consistent with the current payment surface.
Nothing here needs to change before merging.
5a01201 to
3680e4b
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2343 +/- ##
=========================================
Coverage 97.38% 97.38%
=========================================
Files 637 638 +1
Lines 46549 46594 +45
=========================================
+ Hits 45330 45376 +46
+ Misses 1219 1218 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3680e4b to
f9c0314
Compare
|
Claude Security Review: no high-confidence findings. (run) |
| Escape interrupts the local stream, not necessarily the remote process. | ||
| Use `runtime shell` for a native interactive terminal. | ||
|
|
||
| ### Rotate Payment Connector Credentials |
There was a problem hiding this comment.
can we not have this in the main readme? it's too specific. maybe somewhere in docs/?
There was a problem hiding this comment.
Update: removed the dedicated guide and its README link entirely in 7a41959, as requested. This PR now has no README changes and no separate rotation guide; only the generated command reference remains. The CLI implementation is unchanged.
| Wallet-secret rotation can interrupt wallet operations while the new credential | ||
| is installed. Selecting both credentials rotates the API key first, then the | ||
| wallet secret; this is not atomic. An error does not guarantee that credentials | ||
| are unchanged. |
There was a problem hiding this comment.
the SDK says a failed rotation leaves the existing creds unchanged and warns that every connector using the credential provider may be affected. it also does not promise API-key-first ordering.
can we just align with what it says?
again, I would rather this whole section be removed
There was a problem hiding this comment.
Updated the new guide to cite the SDK reference you linked: documented failed rotations leave the connector and its existing credential unchanged, and every connector sharing the credential provider is affected. Removed the API-key-first ordering and non-atomicity claims. The example now uses API_KEY for routine rotation; WALLET_SECRET is described as lost/compromised-only with the documented possible signing interruption. The whole detailed section is removed from the main README. Commit: cee9257.
|
Claude Security Review: no high-confidence findings. (run) |
notgitika
left a comment
There was a problem hiding this comment.
Thanks this looks good to me
|
Claude Security Review: no high-confidence findings. (run) |
Description
Adds the headless
agentcore payment connector rotate-credentialscommand for service-managedCoinbase Quick Create credentials.
^3.1143.0.The original SDK-publication blocker is resolved. This revision is rebased onto
refactorat
17e442b3, including the latest credential wizards, update TUI, and version badge.It leaves the README unchanged and includes the generated rotation command reference.
The rotation test no longer passes
--endpoint-url, which upstream deliberately disabled.The secret-backed Slack notification job skips fork PRs, which cannot receive its AWS role
secret. Build, test, and security checks remain enabled.
The earlier Harness-test compatibility commit was dropped because upstream #2480 supersedes it;
the Harness tests now match
refactorexactly.The PR's ready-for-review state is preserved. Live-rotation validation was not rerun during this repair.
Related Issue
Related to #2272.
Documentation PR
Only the generated
command.mdis updated. The dedicated rotation guide and README section/linkhave been removed entirely as requested.
Type of Change
Testing
bun testbun run test:e2e, or explained why they are not applicablebun run typecheckbun run lint:checkbun run format:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themCode validation before the documentation-only removal:
RECORD=0 bun test --coverage --coverage-reporter=lcov: 3,915 passed, 0 failed, across 269 files.git diff --checkpassed.bun auditreported no vulnerabilities.The subsequent documentation-only removal passed Markdown formatting and whitespace checks.
No source, test, dependency, workflow, or generated command-reference changes were made in that step.
The initial sandboxed run could not support normal subprocess/log-directory behavior; the full
suite and startup smoke tests were rerun successfully outside the sandbox.
No live credential rotation or AWS resource mutation was performed during this CI repair.
The repository's deployment E2E suite was not run: this update does not change deployment flows,
and live rotation requires a separately approved test connector. Offline tests exercise the real
router and Core with the SDK send boundary replaced, not a live service.
No assets were modified.
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.