Repository navigation
fix(deploy): show CloudFormation resource failure after rollback - #2760
Conversation
agentcore-devx-automation
Bot
added
agentcore-harness-reviewing
Package TarballHow to installgh release download pr-2760-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-1.0.1.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
| operation.kind === "deploy" && | ||
| error instanceof Error && | ||
| firstFailure && | ||
| !error.message.includes(firstFailure.reason) |
There was a problem hiding this comment.
This dedupe misses on the update path. When the stack already exists, Toolkit's own diagnosis works and already includes the reason. But it wraps each reason at 120 chars (formatResourceErrorBlock in diagnosing/diagnosis-formatting.js:128, wrapText(120, e.message)). AgentCore reasons are almost always longer than that, so includes(reason) is false and users see the cause twice.
The only case where the cause is lost is NoStack. ToolkitError sets name to the error code (toolkit-error.js:85), so gating on that is tighter:
if (error instanceof Error && error.name === "NoStack" && firstFailure) {That also stops the failure from being attached to unrelated errors thrown later in performCdkOperation (e.g. the "deployed no stack" error at L251). The test would need to throw an error with name = "NoStack" instead of a plain Error.
| return await performCdkOperation(loaded, operation, options); | ||
| } catch (error) { | ||
| if ( | ||
| operation.kind === "deploy" && |
There was a problem hiding this comment.
Bootstrap goes through the same monitorDeployment path, so a first bootstrap that rolls back can hit the same NoStack. Is excluding it intentional? If you gate on NoStack (comment below), this kind check can go.
| ...(message.code && { code: message.code }), | ||
| }) | ||
| .debug(message.message); | ||
| if (message.code === "CDK_TOOLKIT_I5502" && onResourceFailure) { |
There was a problem hiding this comment.
Can we add a why-comment here? Two things aren't obvious from the code:
I5502is Toolkit's per-event stack activity message.- This whole path is a stopgap. In toolkit-lib 1.38.2,
deploy-stack.js:411readsfinalState.wrappedbeforediagnoseFromErrorCollection, so a first create that rolls back throwsNoStackand drops Toolkit's own resource diagnosis. Thecancelledfilter mirrors Toolkit'sisCancellationEvent.
dc965a9 to
bddb182
Compare
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v1.x #2760 +/- ##
=======================================
Coverage 96.54% 96.55%
=======================================
Files 706 706
Lines 58626 58657 +31
=======================================
+ Hits 56603 56635 +32
+ Misses 2023 2022 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Capture the failed resource event emiitted by CDK toolkit and include it in the reason field when deploy fails. This makes the cause visible when Toolkit returns a
NoStackerror after a rollback. The original error remains visible as well.Type of Change
Testing
How have you tested the change?
bun testbun run test:e2e, or explained why they are not applicable(CI does not run them for you)
bun run typecheckbun run lint:checkbun run format:checkbun run auditbun run secrets:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themby this PR
command.md(the release workflow regenerates it)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.