feat(project): implement project status - #2165
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Reviewed commit d3997453 per the PR description (rest of the branch belongs to #2158).
What I checked
src/handlers/project/status/index.ts– handler logicsrc/handlers/project/index.ts–withProjectwrapping matches the deploy/build/remove/dev patternsrc/handlers/project/project.test.ts– the stub test replacement asserts thewithProjectwiringsrc/handlers/project/status/index.test.ts– four handler-level tests throughroot.route
Notes
describe()'s spec walk mirrors the declared set inCdkBackend.resolveDeployedResources, and every field it iterates is either.default([])in the schema or guarded with?? [](unassignedTargets,payments). No undefined‑array footguns.- The composite
key(resourceType, name, parent)used to match resolver rows against declared rows is symmetric with what the CDK backend produces, so a resource returned by the resolver will always match its declared row. - Tests keep the real
FsProjectManagerand scaffold a real project in a temp directory, stubbing only at theProjectBackendboundary — matches the guidance on avoiding excessive mocking. - Telemetry isn't wired into sibling handlers (
deploy,build,remove, etc.), so its absence here isn't a regression against an existing pattern. - Scope caveats (
pending-removal, datasets,capacity-providerpending l3-cdk-constructs#336,gateway-targetreported by Id rather than Arn) are all documented in code or the PR description.
No blocking issues. Ready to merge once #2158 lands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2165 +/- ##
============================================
+ Coverage 97.12% 97.13% +0.01%
============================================
Files 535 535
Lines 36844 36980 +136
============================================
+ Hits 35783 35919 +136
Misses 1061 1061 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
{ "projectName": "bbStatus30455", "target": "default", "region": "us-west-2",
"resources": [
{ "resourceType": "harness", "name": "bbStatus30455", "deploymentState": "deployed",
"identifier": "arn:aws:bedrock-agentcore:us-west-2:725476964917:harness/bbStatus30455_bbStatus30455-R7ttcnrOG8" },
{ "resourceType": "memory", "name": "user_mem", "deploymentState": "deployed",
"identifier": "arn:aws:bedrock-agentcore:us-west-2:725476964917:memory/bbStatus30455_user_mem-9ndWxKF91x" },
{ "resourceType": "gateway", "name": "tools", "deploymentState": "deployed",
"identifier": "arn:aws:bedrock-agentcore:us-west-2:725476964917:gateway/bbstatus30455-tools-hwyi9qiqkn" },
{ "resourceType": "policy-engine", "name": "guards", "deploymentState": "deployed",
"identifier": "arn:aws:bedrock-agentcore:us-west-2:725476964917:policy-engine/bbStatus30455_guards-qjcpxplfak",
"children": [
{ "resourceType": "policy", "name": "no_pii", "deploymentState": "deployed",
"identifier": "arn:aws:bedrock-agentcore:us-west-2:725476964917:policy-engine/bbStatus30455_guards-qjcpxplfak/policy/no_pii-i9vsi_xtqh" } ] } ] } |
AlexanderRichey
left a comment
There was a problem hiding this comment.
This doesn't look like it implements the approach we discussed of calling DescribeStacks. What motivated changing direction here?
There was a problem hiding this comment.
Why is this a binary file?
There was a problem hiding this comment.
My agent add NUL bytes to this file which made github make it into a binary that is now fixed.
d399745 to
40d7f76
Compare
First-run fix:
|
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
| This doesn't look like it implements the approach we discussed of calling DescribeStacks. What motivated changing direction here?
|
There was a problem hiding this comment.
Built it and drove it against a real account in us-west-2. Status resolves every resource correctly and headless invoke works. Two blockers before merge: the interactive invoke picker is broken (ARNs sent where ids are expected) and child nesting matches parents by name only. Details inline.
| let active = true; | ||
| void core.projectManager | ||
| .resolveDeployedResources(project, { target: "default" }) | ||
| .resolveProjectResources(project, { target: "default" }) |
There was a problem hiding this comment.
Regression from this PR. The manager never forwards an identifier here, so the resolver defaults to ARNs and select() passes them as harnessId/agentRuntimeId. Picking a deployed harness in the TUI fails with Value 'arn:aws:...:harness/...' at 'harnessId' failed to satisfy constraint. Headless invoke was fixed with identifier: "id", this path was not. Returning both id and arn on each row would remove the toggle entirely.
| const top: ResourceStatus[] = []; | ||
| for (const { resource, row } of entries) { | ||
| const owner = resource.parent | ||
| ? entries.find(({ resource: other }) => !other.parent && other.name === resource.parent) |
There was a problem hiding this comment.
Parent is matched by name only. Names are unique per collection, not across them, so a gateway and a policy engine can both be guards. Reproduced live: the deployed policy showed under the gateway and the engine had no children. Match on the expected parent type too.
| region: resolved.target.region, | ||
| resources: nest(resolved.resources), | ||
| }; | ||
| ctx.require(JsonRendererKey).renderJson(status); |
There was a problem hiding this comment.
deploy, build, dev and export in this router check JsonKey and print a human line otherwise. This always dumps JSON, so a bare agentcore project status in a terminal gets a raw blob.
There was a problem hiding this comment.
agentcore project status will just print out the json output for now. If customers want a human readable, Alex is implementing the tui experience after this pr gets merged.
| }); | ||
| }); | ||
|
|
||
| test("rejects a freshly created project that declares no targets", async () => { |
There was a problem hiding this comment.
project create leaves aws-targets.json empty and deploy fills it, so right after create status tells the user to hand-write the file with an example account id. Commit bfef4ea fixed this and drop allowMissing undid it. The "all local-only" test above only passes because the fake backend never throws, the real one always does here.
| const { target, identifier = "arn" } = input; | ||
| const deployedState = await readDeployedState(this.json, project.rootPath); | ||
| const stackArn = deployedState.targets[target.name]?.stackArn; | ||
| if (!stackArn) { |
There was a problem hiding this comment.
For status, a missing stack just means nothing is deployed yet, which is exactly what local-only describes. Deploy and remove can keep the hard failure.
There was a problem hiding this comment.
Yeah that makes sense. I'll let the handlers throw the ProjectStateError if they want too.
| } | ||
| }; | ||
|
|
||
| const declared: Declared[] = [ |
There was a problem hiding this comment.
The spec also has datasets and mcpRuntimeTools, and declaresNothingDeployable in deploy counts both. They are missing here, so a deployed dataset gets no row at all and deploy and status disagree on what a resource is.
There was a problem hiding this comment.
Datasets are being deprecated in favor of the imperative based commands. I think mcpRuntimeTools is also deprecated because I don't see any add command for it. declaresNothingDeployable doesn't exist.
| return byExportName("Policy", parent ?? "", name, suffix); | ||
| case "config-bundle": | ||
| return byExportName("ConfigBundle", name, suffix); | ||
| case "capacity-provider": |
There was a problem hiding this comment.
No capacityProviders field exists anywhere in the spec, so this case and the union member are unreachable, and the never default forces every future switch to keep handling it.
There was a problem hiding this comment.
I was thinking we will add capacityProvider in the new cli but I can remove it for now.
| case "capacity-provider": | ||
| return byExportName("CapacityProvider", name, suffix); | ||
| case "payment": | ||
| // Payments doesn't set a ExportName so we search for OutputKey |
There was a problem hiding this comment.
This links to the L3 AgentCorePayments construct, but the generated project never uses it. The Payment<name>ManagerArn output comes from toCdkId in the CLI's cdk-stack.ts template. Also the workspace rule is no line comments, so these should go.
There was a problem hiding this comment.
But I like these comments. It help developers understand why the credentials and Payments are special cases.
| | "policy-engine" | ||
| | "policy" | ||
| | "config-bundle" | ||
| | "payment" |
There was a problem hiding this comment.
project add and project remove call this payment-manager (see ProjectResource below), status calls it payment. Deriving this from ProjectResource keeps the two from drifting.
|
|
||
| describe("CdkBackend.resolveDeployedResources", () => { | ||
| test("describes the stack once and returns only resources with deployed ID outputs", async () => { | ||
| describe("CdkBackend.resolveProjectResources", () => { |
There was a problem hiding this comment.
The base branch tested -RuntimeId and -Harness-<name>-Id. Now nothing here asks for identifier: "id", so a typo in the suffix logic would make headless invoke report a deployed runtime as not deployed with CI green. One id-mode case would cover it.
| (r.resourceType === "runtime" || r.resourceType === "harness") && | ||
| r.deploymentState === "deployed", | ||
| ) | ||
| .map((resource) => { |
There was a problem hiding this comment.
Why is this code changing in this PR? I thought the scope was the status command.
|
Claude Security Review: no high-confidence findings. (run) |
| const project = ctx.require(ProjectKey); | ||
| const name = selectProjectResource(project, "runtime", flags.name); | ||
| const deployed = await core.projectManager.resolveDeployedResource(project, { | ||
| const deployed = await core.projectManager.resolveProjectResource(project, { |
There was a problem hiding this comment.
There doesn't seem to be anything pressing that necessitates this name change. It's causing a lot of cascading changes that make this code hard to review and increase the risk of introducing errors. Let's omit this part of the PR and focus on just the status functionality.
There was a problem hiding this comment.
I renamed resolveDeployResource to resolveProjectResources because it now returns whether a resource was deployed or not. I'll just make resolveProjectResource it's own function in the next revision. Later we can merge the functions.
e93ac29 to
ffed816
Compare
|
Claude Security Review: no high-confidence findings. (run) |
ffed816 to
212072b
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
| })), | ||
| ]; | ||
| }, | ||
| async resolveProjectResources() { |
There was a problem hiding this comment.
This was meant to prevent invoke handlers from using this resolvedProjectResources because they should use resolveDeployedResource. We will merge these functions later.
Adds resolveProjectResources alongside resolveDeployedResources rather than widening it. The two answer different questions: invoke asks which Runtimes and Harnesses are live and addresses them by the bare id its data-plane APIs accept, while status reports every declared resource by ARN, including the ones the target has not deployed. Keeping them separate leaves the invoke path untouched. The new resolver reports a resource the stack does not publish as local-only instead of omitting it, so status distinguishes "declared but not deployed" from "not declared". A target with no recorded stack reports everything local-only without reaching AWS, and a project that has never deployed stands in an in-memory default target from the region the CLI resolved, rather than telling the user to hand-write aws-targets.json. Children nest under the resource that owns them, matched on the owner's type as well as its name: names are unique per spec collection but not across them, so a gateway and a policy engine can both be called 'guards'.
|
Claude Security Review: no high-confidence findings. (run) |
The status handler calls
ProjectManager.resolveProjectResourcesto build its report.resolveProjectResourcesreturnsevery resource the project declares, by ARN, reading
CfnOutputvalues fromDescribeStacks.A resource the target's stack has not deployed is reported
local-onlyrather than omitted,and children such as gateway targets and policies are nested under the resource that owns them.
Given an
agentcore.jsonlike this:{ "name": "orders", "version": 1, "runtimes": [{ "name": "assistant" }], "memories": [{ "name": "user_mem" }], "agentCoreGateways": [ { "name": "tools", "targets": [{ "name": "search" }] } ], "policyEngines": [ { "name": "guards", "policies": [{ "name": "no_pii" }] } ] }agentcore project statusproduces:{ "projectName": "orders", "target": "prod", "region": "us-west-2", "resources": [ { "resourceType": "runtime", "name": "assistant", "deploymentState": "deployed", "id": "arn:aws:bedrock-agentcore:us-west-2:111122223333:runtime/assistant-AbCdEf1234" }, { "resourceType": "memory", "name": "user_mem", "deploymentState": "deployed", "id": "arn:aws:bedrock-agentcore:us-west-2:111122223333:memory/user_mem-K3l4M5n6O7" }, { "resourceType": "gateway", "name": "tools", "deploymentState": "local-only", "children": [ { "resourceType": "gateway-target", "name": "search", "deploymentState": "local-only" } ] }, { "resourceType": "policy-engine", "name": "guards", "deploymentState": "deployed", "id": "arn:aws:bedrock-agentcore:us-west-2:111122223333:policy-engine/guards-1a2b3c", "children": [ { "resourceType": "policy", "name": "no_pii", "deploymentState": "deployed", "id": "arn:aws:bedrock-agentcore:us-west-2:111122223333:policy/no_pii-7d8e9f" } ] } ] }If the target isn't declared in
aws-targets.json, it throwsProjectStateError:No deployment targets are configured for project 'orders'.