feat(registry): Cedar policy modules + skills support (#246)#634
feat(registry): Cedar policy modules + skills support (#246)#634Kalindi-Dev wants to merge 4 commits into
Conversation
PR 1 of 3 for the central agent asset registry (ADR-018). Purely additive infrastructure — nothing in the orchestrator or agent reads from the registry yet; that follows in PR 2 (orchestrator + agent MCP E2E) and PR 3 (Cedar modules + skills). Substrate: DynamoDB + S3 (ADR-018 fallback option 2). The four fallback conditions already select it at authoring time — AgentCore Registry is in public preview and hard-migrates namespaces on 2026-08-06. The RegistryClient seam keeps the AgentCore path open as a later swap. - docs/design/REGISTRY.md: MVP asset kinds, DDB/S3 schema, API contract, resolution semantics, upstream-registry/federation stance (out of MVP scope). - Grammar: extend _REGISTRY_REF to registry://kind/namespace/name@constraint (snake_case kinds, mandatory semver pin). New contracts/registry-resolution parity corpus proves the Python regex and TS parseRef agree byte-for-byte. - Shared types (canonical status='approved'; 'active' is not a code value), mirrored CDK<->CLI. - registry-resolver.ts: parseRef/resolveRef/resolveAll, fail-closed, semver ranked in code (DDB sorts versions lexicographically). - RegistryAssetsTable (pk/sk + kind-index GSI) and RegistryArtifactsBucket (versioned, TLS-only, SSE) constructs, wired into AgentStack. - Publish/resolve/list/show handlers: immutability 409, descriptor validation, Cognito RegistryPublisher/Approver gating; /registry routes on TaskApi with least-privilege IAM grants. Depends on aws-samples#548 (ADR-018) — the REGISTRY.md link to the ADR resolves once that PR merges to main; docs link-check stays red until then by design.
PR 2 of 3 for the central agent asset registry (ADR-018). Wires the registry
(PR 1) end-to-end for one asset kind: a blueprint pins an MCP server, the
orchestrator resolves it at the create-task boundary, stamps the resolved
{kind,id,version} on the TaskRecord, and the agent merges the resolved server
config into .mcp.json alongside the channel MCP.
- Blueprint: assets.mcpServers prop -> RepoConfig mcp_servers column, with a
synth-time RegistryRefValidation (rejects floating/malformed refs). Grammar
extracted to a dependency-free registry-ref.ts so the construct layer can
validate without pulling aws-sdk into the synth graph.
- Orchestrator: resolveRegistryAssetsForTask resolves the blueprint's pins
before hydration (fail-fast/fail-closed), stamps resolved_assets, threads the
bundle into the agent payload. resolved_assets added to TaskRecord/TaskDetail/
TaskSummary (+ CLI mirror).
- Agent: registry_loader.py applies the resolved bundle — apply_mcp_assets
merges server_config into .mcp.json; cedar/skill loaders stubbed for PR 3.
resolved_assets threaded server.py -> pipeline.py -> TaskConfig.
- CLI: bgagent registry publish/resolve/list/show.
Deploy-hardening fixes found validating against a live stack:
- Create RegistryPublisher/RegistryApprover Cognito groups (were referenced by
the publish handler but never created). Bootstrap action-map + policy +
DEPLOYMENT_ROLES.md updated for cognito-idp group actions.
- CLI publish flag --asset-version (--version collided with commander's global).
- memorySize=256 on the four registry Lambdas (128MB default OOM'd on init).
- mcp_server publish accepts inline descriptor server_config in lieu of an
artifact.
- Orchestrator Lambda gets REGISTRY_ASSETS_TABLE_NAME env + read grant (resolve
runs there during hydration).
Validated live (backgroundagent-pr246): resolved_assets correctly stamped on a
real TaskRecord. Task itself fails on a pre-existing 'claude' Exec-format image
bug, unrelated to the registry. Registry MCP secret-injection (${VAR} from
Secrets Manager) is a tracked follow-up, out of MVP scope per ADR-018.
PR 3 of 3 for the central agent asset registry (ADR-018), completing the MVP — all three asset kinds now load end-to-end. - Resolver: ResolvedAsset gains a substrate-agnostic ``content`` field, populated inline from the descriptor (cedar_text / prompt_fragment). The seam confines a future S3/AgentCore-backed fetch to the resolver — orchestrator and agent never depend on where the bytes live. - Cedar policy modules: resolved text is merged ORCHESTRATOR-side into the existing cedar_policies payload, so registry Cedar is byte-identical to inline blueprint policies and flows through the same PolicyEngine path (parity holds by construction). - Skills: resolved prompt_fragment is appended to the system prompt (prompt_builder._registry_skill_addendum); tool_hints render as advisory prose (a skill cannot invoke tools — no transitive MCP dependency). - Blueprint: assets.cedarPolicyModules + assets.skills props → RepoConfig columns with synth-time ref validation; orchestrator collects all three ref kinds. Mixed inline-cedar + registry-cedar coexistence tested. - Publish: cedar_text / prompt_fragment required inline (64 KiB cap); inline content satisfies the artifact requirement. Fixes the publish handler to skip the S3 upload when no artifact_b64 is present (inline kinds) instead of Buffer.from(undefined). - Remove the AbcaForkBlueprint E2E-validation scaffolding so the shipped stack carries no fork-specific pins. Docs: REGISTRY.md §2 updated (all kinds implemented, content seam, parity). Validated live on backgroundagent-pr246: a task resolved and stamped all three kinds (mcp_server, cedar_policy_module, skill) on the TaskRecord. After this lands, all six aws-samples#246 acceptance criteria are met.
b3cf5bf to
7ade5df
Compare
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict: Approve (with minor doc nits) — stacked, merge AFTER #548 → #632 → #633
PR 3 of 3 for the #246 central agent asset registry (ADR-018). It completes the MVP: cedar_policy_module and skill now load end-to-end alongside PR 2's mcp_server. The incremental diff (18 files, +533/-70) is small, cohesive, well-tested, and — importantly for the security-critical Cedar path — fail-closed by construction. I verified this against the current worktree (all agent + CDK tests green, tsc/ruff/ty clean, docs mirror in sync). No blocking issues in this layer.
Deep review focused on the incremental diff (7ade5df on top of #633/#632). #632 and #633 are under separate review and are not re-judged here.
Vision alignment
Advances the registry vision (decouple asset authoring from CDK deploys) while respecting the bounded-blast-radius tenet: registry-sourced Cedar reaches the agent through the same cedar_policies payload field as inline blueprint policies (orchestrator.ts merge → runner.py:288 extra_policies= → PolicyEngine), and extra_policies are force-wrapped as @tier("soft") and reject any @tier/@rule_id self-annotation (agent/src/policy.py:891-899). Net effect: a registry cedar_policy_module can only tighten policy (add soft-deny/HITL gates) — it cannot inject a hard permit, override a built-in rule, or widen authority. Parity holds by construction, exactly as the ADR-018 seam claims. Skills are prompt-text only (tool_hints are advisory prose, no transitive MCP dependency), so no tool-surface expansion. The 64 KiB publish cap + the engine's POLICIES_MAX_BYTES cap bound the payload.
Blocking issues
None in the incremental (#634) layer.
Non-blocking suggestions / nits
- Stale doc comment —
cdk/src/handlers/shared/registry-descriptor.ts:47still saysKINDS_REQUIRING_ARTIFACT= "kinds whose artifact bytes are required at publish"; after this PR, inlinecedar_text/prompt_fragmentsatisfy the requirement, so the constant now really means "kinds that MAY carry loadable bytes / get a presigned URL" (its live use isregistry-resolve.ts:78). Tighten the wording. - Stale doc comment —
hasInlineDescriptorContentheader atregistry-descriptor.ts:137-138says "Today onlymcp_serversupports this", but the function now handles all three publishable kinds. Update. - Publish-time Cedar semantics not validated —
validateInlineContentchecks presence/type/size ofcedar_textbut not that it parses as Cedar. Malformed text fails fail-loud at task start (soft-deny policy validation failed→ FAILED task), which is acceptable fail-closed behavior and matches inline-blueprint behavior — but publish-time parse-validation would move the failure left. Consider as a follow-up.
Documentation
docs/design/REGISTRY.md§2 updated (all three kinds implemented,contentseam, cedar-parity note). Clear and accurate.- Starlight mirror
docs/src/content/docs/architecture/Registry.mdregenerated and in sync — I rannode scripts/sync-starlight.mjs; it produced no drift (cleangit status). Not hand-edited. CI's "Fail build on mutation" will pass on the mirror. - Shared-type sync verified:
ResolvedAsset.content?: stringpresent and byte-identical incdk/src/handlers/shared/types.ts:180andcli/src/types.ts:105.
Tests & CI
- Tests (verified in worktree): agent
test_registry_loader.py+test_registry_skill_prompt.py= 18 passed; CDKblueprint/registry-publish/orchestrator-registry/registry-descriptor= 87 passed.blueprint.ts100% coverage. Coverage includes failure paths: size-cap rejection, missingcontent, mixed inline+registry cedar coexistence, the no-S3-upload regression (Buffer.from(undefined)fix), and three-kind stamping. Good failure-path discipline (not just happy path). - Bootstrap synth-coverage: NOT APPLICABLE to this layer. The incremental diff touches no bootstrap files and introduces no new CFN resource types — the blueprint change adds only DynamoDB item columns (
cedar_policy_modules/skills), not new resources. The new table/bucket/handlers live in #632, where the bootstrap check applies. Confirmed 0 bootstrap references in the incremental diff. - CI is RED — both failures are INHERITED / environmental, NOT introduced by this Cedar/skills layer:
build (agentcore)→//docs:link-checkexit 123, 1 dead link:../decisions/ADR-018-agent-asset-registry.md(Status 400). Root cause: ADR-018 (PR #548) is not yet onmain. The link was introduced by 773764f (#632, PR 1) — 0 occurrences in #634's incremental diff — and the PR description explicitly calls this out as red-by-design until #548 merges. Resolves automatically once the base merges.Secrets, deps, and workflow scan→security:depsosv-scanner flagssvgo 4.0.1(GHSA-2p49-hgcm-8545, fix 4.0.2), a docs/Starlight transitive dep inyarn.lock. It is present onmain(line 8060) and unchanged across the whole stack — a post-lockfile CVE, orthogonal to this PR. (Heads-up: merge-queue re-scan can eject the PR on this; a separateyarn.lockbump onmainclears it.)
- Cedar engine pins UNCHANGED —
cedarpy==4.8.4/@cedar-policy/cedar-wasm 4.8.2are untouched (the 4 grep hits are comment prose). No pin move ⇒ no parity-fixture refresh required. Good.
Review agents run
- code-reviewer (style/guidelines): ran — ruff, ty, tsc (cdk + cli), and full test suites clean; blueprint parallel-structure follows the existing
mcp_serverspattern verbatim. - silent-failure-hunter: ran — validation is fail-loud, resolver fail-closed, Cedar validation raises at task start;
prompt_builder._registry_skill_addendumskips malformed skills (tested) rather than defaulting silently. No swallowed errors. - type-design-analyzer: ran —
ResolvedAsset.contentoptional & synced;RepoConfig/BlueprintConfigadditions consistent with existing shape. - pr-test-analyzer: ran — new behavior and failure paths covered; assertions test intended contract (e.g. no-S3-upload, byte-identical cedar merge).
- comment-analyzer: ran — surfaced nits #1 and #2 (stale comments).
- /security-review: ran (REQUIRED for Cedar) — see Vision alignment; conclusion: fail-closed, registry Cedar can only add soft-deny, cannot widen authority.
- Omitted: none in scope.
Human heuristics
- Proportionality — Pass. Change is minimal for the outcome; the
contentseam is a one-field abstraction with a documented future-swap rationale, not speculative over-engineering. - Coherence — Pass. New columns/props mirror the existing
mcpServerspattern exactly (blueprint.ts); same term (cedar_policy_modules,skills) used consistently across CDK↔CLI↔docs↔agent. - Clarity — Pass with nits. Names communicate intent; two stale comments (
registry-descriptor.ts:47,:137-138) lag the behavior. - Appropriateness — Pass. Integration verified against the real PolicyEngine path (
extra_policieswrapping), not only mocks; the parity claim is substantiated by the actual soft-deny wrapping inpolicy.py.
Stack / merge-order note
This is the top of a 4-deep stack. Merge order is strict: ADR #548 → PR 1 (#632) → PR 2 (#633) → PR 3 (this, #634). Do NOT merge #634 before #548/#632/#633 — the docs:link-check red clears only when ADR-018 lands on main. This layer itself is sound and ready; approval is contingent on the base stack merging first.
Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagedocs— guides or design sources (docs/guides/,docs/design/)Related
mainalso shows PR 1+2's commits — review commitb3cf5bffor PR-3-only changes.Changes
Completes the MVP — the two remaining asset kinds now load end-to-end.
ResolvedAssetgains a substrate-agnosticcontentfield, populated inline from the descriptor (cedar_text/prompt_fragment). The seam confines a future S3/AgentCore-backed fetch to the resolver — orchestrator/agent never depend on where the bytes live.cedar_policiespayload, so registry Cedar is byte-identical to inline blueprint policies and flows through the samePolicyEnginepath (parity holds by construction).prompt_fragmentis appended to the system prompt (prompt_builder._registry_skill_addendum);tool_hintsrender as advisory prose (a skill cannot invoke tools — no transitive MCP dependency).assets.cedarPolicyModules+assets.skillsprops →RepoConfigcolumns with synth-time ref validation; orchestrator now collects all three ref kinds. Mixed inline-cedar + registry-cedar coexistence tested.cedar_text/prompt_fragmentrequired inline (64 KiB cap); inline content satisfies the artifact requirement. Fixes the publish handler to skip the S3 upload when noartifact_b64is present (inline kinds) instead ofBuffer.from(undefined).REGISTRY.md§2 updated (all kinds implemented, content seam, cedar-parity note).Validated live: a task resolved and stamped all three kinds (
mcp_server,cedar_policy_module,skill) on the TaskRecord.Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.