Repository navigation
Conversation
- Blank line between the progress lines and the summary, and before apply's progress with --yes. - The preview shows the full plan link first; the AGENTCORE_MIGRATE_CDK_LIBRARY note is one line, and the install command appears only after migrating. - The install command runs from the project root (npm install --prefix agentcore/cdk), so `agentcore deploy` follows without changing directory. - The plan file's stack section is flatter: one row per change under a 'First v1 deploy' line, no 'warning' label on expected in-place updates, a note on where the new logical IDs come from, and no separate 'Replacements' verdict.
Contributor
|
Claude Security Review: no high-confidence findings. (run) |
Contributor
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
This is a focused presentation/wording cleanup for agentcore migrate with matching test updates. The logic changes are small and sensible:
deployImpactnow computesmustKeepand renders the actual blocker count. Previously the branch always said(none that must be kept)even when there were blockers inreplaced/removed— this is a real bug fix.- The
rows()helper unifies the per-change rendering; action is padded to width 8, which fits all four verbs (creates/deletes/replaces/updates). overrideInstallCommandswitches fromcd agentcore/cdk && npm install …tonpm install --prefix agentcore/cdk … --no-save, which keeps the user in the project root so the follow-upagentcore deployjust works.--prefixhas been supported by npm for a long time.- The redundant
the v1 deploy updates it in place:prefix is dropped from the update reason now that the row's action column already saysupdates.
Nothing jumped out as needing changes before merge.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2612 +/- ##
=========================================
Coverage 96.58% 96.59%
=========================================
Files 700 700
Lines 57638 57631 -7
=========================================
- Hits 55672 55666 -6
+ Misses 1966 1965 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
notgitika
marked this pull request as ready for review
October 16, 2026 20:30
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses the readability feedback on
agentcore migratefrom the bug bash notes:✓ Planning the refactor of …) andMigration plan: …. With--yes, there's also one before apply's progress.AGENTCORE_MIGRATE_CDK_LIBRARYnote comes afterFull plan: …and is one line. The install command only appears after migrating, in the next steps, which is when it's needed. This only shows when the test override is set.npm install --prefix agentcore/cdk <lib> --no-saveinstead ofcd agentcore/cdk && …, soagentcore deployruns right after it without changing directory. It also works the same in shells wherecd … &&doesn't.First v1 deploy: …line and then one row per change (creates,deletes,replaces,updates).warninglabel. That label made it look like there were two warning sections.Replacements:verdict is folded into theFirst v1 deployline, which now also counts resources that must be kept.Before:
After:
Related Issue
No public issue; this addresses bug bash feedback on #2584.
Documentation PR
Not applicable (CLI output only).
Type of Change
Testing
bun testbun run test:e2e, or explained why they are not applicable. Instead of e2e, I ran it live: I deployed a new v0 project (runtime, memory and gateway) with CLI 0.31.1, previewed and migrated it with this build, then ran the printednpm install --prefix …andagentcore deployfrom the project root. The deploy succeeded.bun run typecheckbun run lint:checkbun run format:checkbun run buildsrc/assets/, I updated affected snapshots withbun test <test-file> --update-snapshotsand committed themChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.