Uh oh!
There was an error while loading. Please reload this page.
feat: project add/remove policy-engine and policy with natural-language generation - #2121
Conversation
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice, focused change. The tests exercise real filesystems (via tmp dirs) and mock only at the AWS SDK boundary in policy.test.ts, which matches the guidelines. TestPolicyClient gives the handler tests a clean seam without over-mocking. The specPatch mechanism in FsProjectManager.addResource cleanly folds the "policy-engine also updates gateways" case into the existing rollback flow, and the engine-removal path correctly strips dangling policyEngineConfiguration references. The new custom validator for gateway → policy engine references is a good safety net.
A few small things that are non-blocking, in case you want to address them in a follow-up:
src/handlers/project/add/policy-engine/index.tsdoesn't check the deployed service name length.policyEngineResourceName(projectName, name)(projectName_engineName) can exceed the service's 48-char limit even when the localPolicyEngineNameSchema(≤48 chars) accepts the name.add gatewayalready does this length check viagatewayResourceName; the policy-engine handler could do the same to fail fast at add time rather than at deploy time.- In
src/handlers/project/add/policy/index.ts, the--generatepath only pre-checks that the engine exists; it doesn't pre-check that the policy name is unique across engines. Since generation can take ~2 minutes, a user could hit the duplicate-name error from the project manager only after waiting. Consider running the same "policy name already exists" check up front. - In
src/core/policy.tsx, if polling hitsGENERATION_MAX_POLLSwhile stillGENERATING, the error becomespolicy generation did not complete: GENERATING(sincestatusReasonsis undefined). A "timed out after ~2m" message would be friendlier.
None of these block merging.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## refactor #2121 +/- ##
============================================
+ Coverage 97.22% 97.24% +0.02%
============================================
Files 463 465 +2 Lines 28160 28417 +257 ============================================
+ Hits 27378 27635 +257
Misses 782 782 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
559ef77 to
b201660CompareClaude Security Review: no high-confidence findings. (run) |
Claude Security Review: no high-confidence findings. (run) |
b201660 to
fec8782CompareClaude Security Review: no high-confidence findings. (run) |
fec8782 to
505f698Compare| @@ -0,0 +1,128 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
| import { | |||
There was a problem hiding this comment.
The golden/flow coverage does live only in the handler tests: they drive the real root handler against a real temp project and assert on agentcore.json, with TestPolicyClient standing in for this client. policy.test.ts is the other altitude: a unit test faking the SDK at .send(), covering behavior the handler tests cannot reach through the fake, namely nextToken pagination, poll exhaustion, GENERATE_FAILED reasons, Dogwood policy definition members, and findings-only assets (the last two were caught live during the e2e). Same split as gateway.tsx / eval.tsx and their tests. If you meant something else by this, happy to adjust.
There was a problem hiding this comment.
Just follow what batch-evaluation does, it puts all the edge cases through a separate handler test file using regular mocking techniques. (batch-evaluation.test.ts)
There was a problem hiding this comment.
Done on #2127 in ef9f23c, following the batch-evaluation shape: src/core/policy.test.ts is deleted and the edge cases (deployed-resource resolution, poll exhaustion, GENERATE_FAILED reasons, Dogwood definition members, findings-only assets) now run through the real root handler in src/handlers/project/add/policy/generate.test.ts, with the control client mocked at .send().
| @@ -0,0 +1,137 @@ | |||
| import { setTimeout as sleep } from "node:timers/promises"; | |||
There was a problem hiding this comment.
Can we split up this PR into imperative based and project based? Also, I noticed that we are creating a seperate CorePolicyClient just for the projects add policy command.
There was a problem hiding this comment.
Done: --generate and PolicyClient are extracted to #2127, stacked on this branch. This PR is now purely the spec-editing surface (add/remove for policy-engine and policy with --statement), no core client. Extraction was a pure move: the stacked branch tree is byte-identical to this branch's pre-split tip.
Claude Security Review: no high-confidence findings. (run) |
| /** | ||
| The deployed service name of a policy engine; mirrors the L3 AgentCorePolicyEngine construct's rule. | ||
| **/ | ||
| export function policyEngineResourceName(projectName: string, engineName: string): string { |
There was a problem hiding this comment.
shift to handler since its only used there.
Claude Security Review: no high-confidence findings. (run) |
| /** | ||
| The deployed service name of a gateway; mirrors the L3 Gateway construct's rule. | ||
| **/ | ||
| export function gatewayResourceName( |
There was a problem hiding this comment.
Why are you adding helper functions here if its only used by the handler.
There was a problem hiding this comment.
Done in 9065f01: gatewayResourceName moved into the add gateway handler, same treatment as policyEngineResourceName. Both schema modules are back to pure shape definitions.
Claude Security Review: no high-confidence findings. (run) |
Uh oh!
There was an error while loading. Please reload this page.
What
Adds project-based Policy support per the locked design in the workspace doc AddPolicy.md:
agentcore project add policy-engine --name <n> [--description] [--encryption-key-arn] [--tags] [--attach-to-gateways <names...>] [--attach-mode log-only|enforce]agentcore project add policy --engine <e> --name <n> --statement <inline|file://|-> [--validation-mode] [--enforcement-mode] [--authorization-phase]agentcore project remove policy --name <n> [--engine <e>](engine inferred when unambiguous)agentcore project remove policy-engine --name <n>(strips gatewaypolicyEngineConfigurationreferences)Supporting changes:
gatewayResourceName/policyEngineResourceNamenext to the schemas, mirroring the L3 constructs and honoring the spec'sresourceNameoverride. Theadd gatewayhandler uses the shared rule.ProjectSpecSchemagained a referential check: a gatewaypolicyEngineConfiguration.policyEngineNamemust name a declared engine (design decision 3 relies on schema validation of hand-edits).Per review,
--generate(natural-language policy generation) and itsPolicyClientcore sub-client moved to the stacked follow-up #2127.Not in this PR
--generateandPolicyClient: feat: generate Cedar policies from natural language in project add policy #2127 (stacked on this branch).projectsection, so no README change (per plan).enforcementMode/authorizationPhasein the L3PolicySchema, passEnforcementModetoCfnPolicy, add policy construct tests.Testing
bun test src), typecheck clean, per-slice and whole-branch simplify reviews applied.@aws/agentcore-cdk@0.1.0-alpha.45: project created, gateway/engine/policy added, deployed, enginePolicyE2E_Guardrailsand policyDenyAllverified ACTIVE via the control plane, both removes exercised (including engine-inference without--engine), gateway reference stripped, stack deleted.Service-side Cedar validation learnings recorded in the design doc: wildcard resources are rejected at CreatePolicy (constrain to
resource is AgentCore::Gateway), and blanket forbids failFAIL_ON_ANY_FINDINGSas Overly Restrictive, so deny-all policies need--validation-mode ignore-all-findings.