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.
| 'invoke', | ||
| computeInvokeAttrs({ | ||
| preview: isPreviewEnabled(), | ||
| preview: true, |
There was a problem hiding this comment.
Now that preview is always on, this hardcoded preview: true plus the preview: boolean parameter on computeInvokeAttrs (in src/cli/commands/invoke/utils.ts) is dead code that still gates harness detection:
// utils.ts
const isHarness = options.preview && isHarnessInvoke(options);Please drop the preview parameter from computeInvokeAttrs, simplify to const isHarness = isHarnessInvoke(options);, remove preview: true from this call site and from useInvokeFlow.ts, and update __tests__/utils.test.ts (the preview: false test cases are no longer meaningful).
| 'invoke', | ||
| computeInvokeAttrs({ | ||
| preview: isPreviewEnabled(), | ||
| preview: true, |
There was a problem hiding this comment.
Same as the CLI path — this hardcoded preview: true indicates that computeInvokeAttrs's preview parameter is now dead. Drop the param from the function signature and remove it here.
| // Harness features are only available in preview builds (BUILD_PREVIEW=1). | ||
| const isPreviewBuild = process.env.BUILD_PREVIEW === '1'; | ||
| const baseCanRun = prereqs.npm && prereqs.git && hasAws && isPreviewBuild; | ||
| const baseCanRun = prereqs.npm && prereqs.git && hasAws; |
There was a problem hiding this comment.
Good — but the same BUILD_PREVIEW gate is still in place in the integration tests and they're silently skipped on every CI run:
integ-tests/add-remove-harness.test.ts:9,15,90,170,197integ-tests/create-edge-cases.test.ts:200,202(harness create suite)integ-tests/dev-server.test.ts:178,180(harness dev suite)
build-and-test.yml runs vitest run --project integ without setting BUILD_PREVIEW, so after this PR the harness path is the default agentcore create flow but has no integ coverage in CI. Please drop the isPreviewBuild constant + skipIf from these three files (keep the prereqs.npm / prereqs.git / hasUv checks).
| E2E_FILESYSTEM_SUBNET_ID: ${{ env.E2E_FILESYSTEM_SUBNET_ID }} | ||
| E2E_FILESYSTEM_SECURITY_GROUP_ID: ${{ env.E2E_FILESYSTEM_SECURITY_GROUP_ID }} | ||
| BUILD_PREVIEW: '1' | ||
| run: npx vitest run --project e2e e2e-tests/harness-bedrock.test.ts ${{ steps.changed.outputs.harness_extra }} |
There was a problem hiding this comment.
(Minor / follow-up) BUILD_PREVIEW is no longer consumed by the build (esbuild.config.mjs and package.json's build:preview were both removed in this PR), but e2e-tests-full.yml still runs a cli-build: [preview, ga] matrix and sets BUILD_PREVIEW on the build/test steps. That now produces two identical builds and roughly doubles full-suite runtime for no signal. release-main-and-preview.yml:381 also still sets BUILD_PREVIEW: '1' for the preview publish build (also a no-op now). And vitest.config.ts:30 still defines __PREVIEW__ as a global even though feature-flags.ts no longer declares it. Worth cleaning these up here or in an immediate follow-up.
|
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.
| @@ -102,17 +107,17 @@ const HARNESS_ONLY_FLAGS = [ | |||
| 'truncationStrategy', | |||
There was a problem hiding this comment.
container is harness-only (only consumed in handleCreateHarnessCLI via harnessPrimitive.parseContainerFlag(options.container) around line 236, never read in handleCreateCLI/createProjectWithAgent), but it isn't listed here. As a result, the isAgentPath(opts) && hasHarnessOnlyFlags(opts) conflict check at line ~601 doesn't fire for it, so:
agentcore create --name foo --framework Strands --container ./Dockerfile
routes to the agent path and silently drops --container instead of erroring out. Same shape as the kind of bug HARNESS_ONLY_FLAGS exists to prevent.
Fix: add 'container' (and most likely 'harnessMemory' for symmetry with --no-harness-memory) to this list.
While you're here: the hasAnyFlag chain at line ~557 is also missing container, withConfigBundle, efsAccessPointArn, efsMountPath, s3AccessPointArn, s3MountPath, and sessionStorageMountPath. In practice --name is required so users are unlikely to hit this, but agentcore create --container ./Dockerfile (no other flags) currently falls through to the TUI instead of erroring with a missing---name message. Worth syncing that list too.
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.Out of scope / notes
isGatedFeaturesEnabled()/ENABLE_GATED_FEATURES— that is a separate runtime flag and is unrelated.@previewnpm tag so existing1.0.0-preview.XXcustomers don't have to switch overe2e-tests-full.ymlstill has a[preview, ga]matrix (now running identical builds) — left as-is for a follow-up release-pipeline simplification.Testing