feat: bring harness flows into latest version of cli#1598
Conversation
Package TarballHow to installgh release download pr-1598-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.20.2.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Cutover looks correct and the production code paths are clean. Two issues that need to be addressed before merging, plus one minor follow-up:
1. computeInvokeAttrs still has a dead preview parameter
src/cli/commands/invoke/utils.ts still gates isHarnessInvoke() on options.preview:
const isHarness = options.preview && isHarnessInvoke(options);Both call sites this PR touches now hardcode preview: true (src/cli/commands/invoke/command.tsx:384, src/cli/tui/screens/invoke/useInvokeFlow.ts:179), so the parameter no longer carries any signal. It should be removed entirely (function param + both call sites + the preview: false test cases in src/cli/commands/invoke/__tests__/utils.test.ts). Otherwise the code reads as if there is still a preview gate when there isn't, and there's a real risk of a future caller accidentally passing preview: false and silently misclassifying telemetry.
2. Harness integration tests are now silently skipped in CI
harness-e2e-helper.ts was correctly updated to drop the BUILD_PREVIEW gate, but the integration tests were missed:
integ-tests/add-remove-harness.test.ts:9,15,90,170,197— everydescribein the file isdescribe.skipIf(!isPreviewBuild)integ-tests/create-edge-cases.test.ts:200,202—describe.skipIf(!isPreviewBuild || ...)for the harness create flowinteg-tests/dev-server.test.ts:178,180—describe.skipIf(!isPreviewBuild || ...)for harness dev mode
build-and-test.yml runs npx vitest run --project integ without setting BUILD_PREVIEW, so all of these suites silently skip on every PR. After this PR the harness path is the default agentcore create flow, so it really needs integration coverage. Please drop the isPreviewBuild gate from these three files (the prereqs.npm/prereqs.git/hasUv checks should stay).
3. (Minor) Leftover BUILD_PREVIEW / __PREVIEW__ references
These are no longer consumed by anything, but they're confusing to a future reader. Worth either cleaning up here or in an immediate follow-up:
vitest.config.ts:30still defines__PREVIEW__as a global —feature-flags.tsno longer declares it, so thisdefineis dead..github/workflows/e2e-tests-full.yml:33,66,115still runs acli-build: [preview, ga]matrix and setsBUILD_PREVIEWon the build/test steps. Sinceesbuild.config.mjsno longer reads it andbuild:previewwas removed frompackage.json, this matrix produces two identical builds and roughly doubles full-suite runtime for no signal..github/workflows/release-main-and-preview.yml:381setsBUILD_PREVIEW: '1'on the preview-publish build, which is now a no-op.
Happy to treat #3 as a follow-up if you'd rather keep this PR scoped, but #1 and #2 should land before merge.
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
The fixes from the previous review look good — the preview parameter is gone from computeInvokeAttrs, the isPreviewBuild gates are removed from the integ tests, and the leftover __PREVIEW__ / BUILD_PREVIEW references have been cleaned up.
One new issue with the new harness/agent path routing in create that I think should be addressed before merge — see the inline comment on HARNESS_ONLY_FLAGS.
|
Claude Security Review: no high-confidence findings. (run) |
tejaskash
left a comment
There was a problem hiding this comment.
Reviewed the full diff and traced control flow beyond the hunks. This is a clean, overwhelmingly mechanical cutover of the __PREVIEW__ / isPreviewEnabled() build-time gate to always-on, with the gating machinery removed.
Verified:
- Every
isPreviewEnabledcaller (26 files) is touched by the PR — no dangling references against the deleted export. harnessPrimitiveis now non-optional and all!assertions are correctly dropped.- The
canary.ymlmatrixexcludeis the right tool — it drops the nonexistentPrerelease/Previewtarball while preservingReleased/Preview(verifies the still-published@previewtag). getHarnessDryRunInfois reached by the existing--defaults --dry-runtests (--defaultsisn't an agent-path flag, so it routes through the harness path).- The
useAddHarnessWizardprovider-routing rewrite only deletes a branch that was unreachable under the preview-gated wizard; bedrock still reachescontainervia the unchangedallStepssequence — no behavior change.
LGTM.
Summary
The
__PREVIEW__/isPreviewEnabled()build-time flag gated harness and related flows (export,add/remove tool,add/remove skill) to preview-only builds. This PR cuts over so those flows are the only path: every preview-enabled branch is inlined as always-on, the GA-only branches are deleted, and the build-time machinery is removed.Result: harness is now available in the standard (
@latest) release — no preview build required.Closes #1569
What changed
isPreviewEnabled()+ the__PREVIEW__esbuild define, theBUILD_PREVIEWwiring inindex.ts, thebuild:previewnpm script, and theAGENTCORE_PREVIEWCDK passthrough (wrapper.ts+ vendedassets/cdk/bin/cdk.ts).create-promptflow).scripts/bundle.mjsdual-tarball logic — GA and preview builds are now byte-identical.-previewtarball:e2e-tests.yml,prerelease-tarball.yml,canary.yml.README.md: harness section no longer says it requires@aws/agentcore@preview.preview-flag.test.ts; update tests that mocked the flag.Notes
@previewnpm tag so existing1.0.0-preview.XXcustomers don't have to switch overTesting