Skip to content

feat(project): add deploy command contract - #2056

Open
notgitika wants to merge 7 commits into
refactorfrom
feat/project-deploy-contract
Open

feat(project): add deploy command contract#2056
notgitika wants to merge 7 commits into
refactorfrom
feat/project-deploy-contract

Conversation

@notgitika

@notgitika notgitika commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add the project deploy handler and --target contract
  • define deployment target validation and deployment result types
  • synchronize the closed AgentCore region enum with the current supported-region list
  • wire manager/backend deploy interfaces with an intentionally unimplemented CDK operation

Stack

This is PR 2 of 4 decomposing #2001. The command remains nonfunctional until the final PR. Review and merge bottom-up.

  1. refactor(project): move build behind a project backend #2055 - backend build boundary
  2. feat(project): add deploy command contract #2056 - deploy command contract (this PR)
  3. feat(project): add CDK Toolkit adapter #2057 - CDK Toolkit adapter
  4. feat(project): implement safe CDK deployment #2058 - safe deployment implementation

typecheck, lint, format, test all pass

@github-actions github-actions Bot added the size/m PR size: M label Aug 20, 2026
@github-actions github-actions Bot added agentcore-harness-reviewing AgentCore Harness review in progress size/m PR size: M and removed size/m PR size: M agentcore-harness-reviewing AgentCore Harness review in progress labels Aug 20, 2026
@notgitika
notgitika marked this pull request as ready for review August 20, 2026 04:10
@notgitika
notgitika marked this pull request as draft August 20, 2026 04:19
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Aug 20, 2026
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Aug 20, 2026
@notgitika
notgitika marked this pull request as ready for review August 20, 2026 04:24
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Aug 20, 2026

@Hweinstock Hweinstock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small questions, but approach lgtm. I can take another look once the dependent changes are merged.

Comment thread src/handlers/project/project.test.ts
Comment thread src/handlers/project/types.ts Outdated
Comment thread src/handlers/project/deploy/index.test.ts Outdated
Base automatically changed from feat/project-backend-build to refactor August 20, 2026 16:07
@notgitika
notgitika force-pushed the feat/project-deploy-contract branch from 81c98a5 to e485e02 Compare August 20, 2026 17:06
@codecov-commenter

codecov-commenter commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.16%. Comparing base (d7f3fd4) to head (e8aabe3).

Additional details and impacted files
@@             Coverage Diff              @@
##           refactor    #2056      +/-   ##
============================================
+ Coverage     97.15%   97.16%   +0.01%     
============================================
  Files           387      388       +1     
  Lines         23142    23248     +106     
============================================
+ Hits          22483    22589     +106     
  Misses          659      659              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@notgitika
notgitika force-pushed the feat/project-deploy-contract branch from 53bd2b5 to 78a8644 Compare August 21, 2026 16:16
Comment thread src/projectSchemas/aws-targets.ts Outdated
@Hweinstock

Copy link
Copy Markdown
Contributor

I think there are some merge conflicts: src/handlers/project/project.test.ts.

@notgitika
notgitika force-pushed the feat/project-deploy-contract branch from 78a8644 to f3c53bf Compare August 21, 2026 17:40
Comment thread src/core/project/manager.tsx
@notgitika
notgitika force-pushed the feat/project-deploy-contract branch from b9422f7 to e5af2c4 Compare August 21, 2026 18:21
nborges-aws
nborges-aws previously approved these changes Aug 21, 2026

@nborges-aws nborges-aws left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for fixes!

@Hweinstock Hweinstock left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments on testing and some nits, but I like the direction!

Comment thread src/core/project/manager.tsx
Comment thread src/io/json.ts
throw new DeserializationError(filePath, { cause: parseResult.error });
throw new DeserializationError(filePath, {
cause: parseResult.error,
details: z.prettifyError(parseResult.error),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha I refreshed and you implemented exactly what I was thinking, crazy!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧙🏼

Comment thread src/testing/TestCoreClient.tsx Outdated
Comment thread src/handlers/project/deploy/index.test.ts Outdated
Comment thread src/handlers/project/deploy/index.test.ts Outdated
Comment thread src/handlers/project/deploy/index.test.ts Outdated
Comment thread src/handlers/project/deploy/index.ts Outdated
The test assembled its own ValueContext with ProjectKey, JsonKey and a stub
JsonRenderer, which re-implemented what createRootHandler installs — so it
could pass while the real wiring was wrong. It now goes through
createRootHandler with the fake manager injected via TestCoreClient, the same
pattern project.test.ts and add/harness/index.test.ts use, exercising the real
--json group flag, the real JSON renderer and the real withProject wrap.
Verified by unwrapping deploy from withProject: both tests now fail, where the
hand-built context passed.

No fixture setup needed since the fake's resolve() returns a project.

Also drops the CloudFormation wording from DeployResult.outputs. The shape is
already backend-neutral; only the doc comment implied CDK.
Resolve the named aws-targets.json entry in FsProjectManager and pass the
resolved account and region to the backend, mirroring build(). deploy()
previously threw NotImplementedError before dispatching, which left
CdkBackend.deploy unreachable and duplicated the same message in two places;
the backend is now the single place that reports deployment as unimplemented.

Narrow the TestCoreClient seam from a whole-manager override to a single
backend override. Handler tests keep the real FsProjectManager, so target
resolution and withProject run for real and only the deploy boundary is faked.
Match the name pattern the schema on main already enforces. The CDK
normalizes underscores to hyphens, so accepting them here would let a
target be written one way and deployed under another.
The root handler prints only `error.message`, so the zod detail that
`FsReadWriteJson.read` put in `cause` never reached the user: a typo'd
region or a duplicated target name in a hand-edited file produced only
`Failed to deserialize file at "<path>"`, naming the file but not the
field.

DeserializationError now requires a `details` string and appends it to
the message, and json.ts passes the prettified zod error (or the parse
error) through. Fixing it in the IO layer rather than catching in
`ProjectManager.deploy` covers every hand-edited file at once. Making
`details` required rather than optional keeps the opaque message from
being reintroduced.

Tested at the handler level, since the schema tests pass in isolation
without proving the detail reaches stderr.
- Inline runDeploy and renderResult. Neither is reused; the manual .next()
  loop only exists because the outputs are the generator's return value,
  which `for await` discards, so that reason is now a comment where the
  loop lives. `build` already drains its generator inline.
- Rename the test factory `harness` to `testDeployCommand`. "Harness" is a
  real AgentCore resource in this codebase, and the name matches the
  existing `testMemoryCommand` convention.
- Collapse inProjectWithRawTargets/inProjectWithTargets into one helper
  that takes the file contents. Callers pass JSON.stringify(...), so the
  malformed-JSON case is just another call rather than a second function.
- Drop the TestCoreClientOptions comment; the Partial already says it.
- Drop the comment above the aws-targets.json tests describing the opaque
  message, which no longer exists now that DeserializationError requires
  `details`. The describe name carries the intent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants