feat(registry): resolve + load registry assets into tasks (#246)

[Kalindi-Dev]

Summary

Builds on the catalog PR (#664) to consume registry assets at task time.

• Orchestrator resolve-step (resolveRegistryAssets): resolves a blueprint's registry:// mcp_server/cedar_policy_module/skill refs at task start, fail-closed; stamps {kind,id,version} triples on the TaskRecord, merges resolved cedar_text into cedar_policies, threads the runtime bundle into the agent payload.
• Blueprint asset props + onUpdate fix: assets.{mcpServers,cedarPolicyModules,skills} with RegistryRefValidation; the three onUpdate helpers now write the asset-ref columns so redeploying an onboarded repo no longer drops them.
• Agent loaders (registry.loader): mcp_server → .mcp.json; cedar_policy_module → PolicyEngine unannotated extra_policies; skill prompt fragments → system prompt.
• TaskOrchestrator IAM: read-only bedrock-agentcore registry access.

Depends on #664. Base is feat/246-registry-catalog; retarget to main once #664 merges.

Test plan

• mise run build green (2595 tests total)
• Deploy with --context forkBlueprintRepo=owner/repo; submit a task that pins all three asset kinds
• Confirm the opened PR shows skill marker + MCP-sourced content + cedar rule loaded (repro of task 01KYJ5FT0RDZJ72J7R5BKCDM38)

[Kalindi-Dev]

Self-review (principal-architect pass)

Ran a /review_pr-style self-review plus a re-review after rebasing onto the updated base (#664) so the parity \Z fix is physically present in this branch. Verdict: no blocking issues.

Verified correct:

• Fail-closed resolution — resolveRegistryAssets throws on an unresolvable/malformed ref; hydrateAndTransition fails the task at HYDRATING before the RUNNING transition. No silent proceed.
• Cedar merge safety by construction — resolved cedar_text flows through PolicyEngine's legacy extra_policies, which hard-raises on @tier/@rule_id and force-wraps every rule as @tier("soft"). A registry module cannot escalate to hard-deny.
• onUpdate fix — all three helpers (buildUpdateFields/buildExpressionNames/buildExpressionValues) now write the asset-ref columns, with a regression test asserting it — closes the "redeploy silently drops asset refs" footgun.
• Loader ordering — apply_resolved_assets runs after setup.repo_dir exists; .mcp.json merge preserves existing servers (test-covered).

One finding worth stating explicitly (documented tradeoff, not a blocker): a resolved mcp_server url is written into .mcp.json verbatim and is bounded only by the VPC-wide DNS firewall (observation-mode + fail-open by default). This is the same posture as existing channel MCP (LINEAR_MCP_URL/JIRA_MCP_URL) and repo-committed MCP servers — this PR doesn't weaken an existing boundary, it adds a registry-mediated way to populate the same file, gated by fail-closed resolution + IAM-gated publish. Binding registry MCP URLs to the blueprint's egress allowlist at resolve time would need a design decision. Treat registry publish as a privileged, audited operation.

Rebased onto d3e2b056; full build green (2596 tests). CI green on the rebased tip.

[scottschreckengaust]

Verdict

Approve with nits. Stacked on #664 (base feat/246-registry-catalog, present locally at d3e2b056). The three critical foci — fail-closed resolution, Cedar merge safety, and least-privilege registry IAM — all hold. The onUpdate column-drop fix is real and has a regression test. Nits below are non-blocking (two are pre-existing shared patterns). Merge order: #664 first, then retarget to main.

Vision alignment

Advances bounded blast radius and reviewable outcomes (VISION.md): registry pins are semver-locked, resolved fail-closed at admission, and stamped as an immutable {kind,id,version} audit triple on the TaskRecord. Cedar modules ride the same cedar_policies payload as inline blueprint policies, so the PolicyEngine's soft-tier force-wrap invariant is preserved by construction — a registry module cannot widen authorization. The documented tradeoff (registry MCP url is bounded only by the VPC DNS firewall, same posture as channel MCP) is a coherent tenet trade, not a regression; registry publish is correctly treated as a privileged, IAM-gated operation. No undocumented tenet trade.

Blocking issues

None.

Non-blocking suggestions / nits

1. Agent-side MCP load fails open (fail-closed is resolution-only). agent/src/registry/loader.py:189-195 — on an .mcp.json write error, apply_mcp_assets logs ERROR, returns 0, and the task proceeds without the operator-pinned MCP server. The fail-closed guarantee is entirely at the orchestrator resolve step (resolveRegistryAssets → failTask(HYDRATING)); once a bundle is threaded, a load failure is silent. cedar_policy_module still fails-closed (PolicyEngine raises at construction) and skill is pure prompt text, so only mcp_server degrades silently — and it mirrors the existing configure_channel_mcp posture. Consider surfacing a task-visible warning (progress event) when a resolved MCP asset fails to load, so a pinned tool silently missing is observable.
2. Registry cedar_text bypasses the 64 KB blueprint cap. agent/src/policy.py:884-889 counts only blueprint_hard_policies + blueprint_soft_policies; registry cedar modules arrive via the legacy extra_policies path (runner.py:288), which is uncapped. Pre-existing property of the extra_policies kwarg, not introduced here, but the registry now makes it operator-reachable at scale — worth a follow-up to fold extra_policies byte-length into the cap.
3. onUpdate does not REMOVE dropped asset columns. cdk/src/constructs/blueprint.ts:363-365,400-402 gate the write on length > 0, so redeploying an onboarded repo that removed all its asset refs leaves the stale mcp_servers/cedar_policy_modules/skills columns in DDB. This exactly matches the sibling cedar_policies/egress_allowlist behavior, so it is consistent — but the PR's own framing ("redeploy must not drop asset refs") is the mirror-image gap. Non-blocking; note it as a known limitation.
4. forkBlueprintRepo demo hook is undocumented. cdk/src/stacks/agent.ts:176-192 adds an opt-in context/env flag pinning hardcoded acme/* refs. It is clearly commented and opt-in, but a one-line mention in the deployment guide would help operators reproduce the E2E test plan.

Documentation

No docs changed in this PR, and none are strictly required: the resolve-step + loader behavior and the registry:// grammar were documented ahead of implementation in docs/design/Registry.md (mirror docs/src/content/docs/architecture/Registry.md, lines 176/189), which already describes "PR 2" resolve+load and the fail-closed pin semantics. Mirror is in sync (this PR touches no doc sources). Only gap is the undocumented forkBlueprintRepo demo flag (nit 4). Issue #246 is approved (P0) — governance satisfied.

Tests & CI

Strong coverage on both sides. Agent: agent/tests/test_registry_loader.py (15 tests) covers merge/preserve/multi-server, non-mcp skip, empty-runtime skip, missing repo_dir, malformed-existing-treated-as-absent, skill fragment assembly + tool_hints + ordering + blank skip. CDK: cdk/test/handlers/shared/registry-orchestrator.test.ts covers happy path, multi-ref order, fail-closed on malformed ref (resolve never called), fail-closed on NO_MATCHING_VERSION, DEPRECATED-warns-but-resolves, and all three kinds together; cdk/test/constructs/blueprint.test.ts adds onCreate mapping, omit-when-empty, the onUpdate regression guard, and synth-time rejection of floating/malformed refs. I ran both locally: agent 15/15 pass; CDK registry-orchestrator + blueprint 54/54 pass. All PR CI checks green (build agentcore, secrets/deps scan, dead-code advisory, PR title). No new CDK per-test synth or re-enabled bundling. No bootstrap update needed — the IAM change is a runtime data-plane grant on the orchestrator Lambda role, not a new CFN resource type (the registry construct + BOOTSTRAP_VERSION bump landed in #664, d3e2b056).

Review agents run

Specialized pr-review-toolkit subagents were not separately dispatchable in this execution context, so I performed the equivalent analysis by hand and state that explicitly:

• code-reviewer (by hand): routing correct (agent runtime in agent/, orchestration/IAM in cdk/); L2/IAM idioms clean; ArnFormat.SLASH_RESOURCE_NAME wildcards justified.
• silent-failure-hunter (by hand): resolution path fail-closed and verified against the caller (orchestrate-task.ts:154); found the one fail-open surface (nit 1, agent MCP write) and confirmed the other two kinds fail-closed.
• type-design-analyzer (by hand): ResolvedAssetTriple parity CDK↔CLI confirmed; agent resolved_assets: list[dict[str,Any]] is a pragmatic passthrough shape.
• comment-analyzer (by hand): comments accurate; the "byte-identical from PolicyEngine's view" claim verified against policy.py extra_policies handling.
• pr-test-analyzer (by hand): happy + failure paths both covered; ran the suites.
• security-review scope (by hand): IAM least-privilege + Cedar privilege-surface reviewed; no secrets/network regressions.

Human heuristics

• Proportionality — Pass. Per-kind loaders are small and single-purpose; no over-abstraction. Resolve-step reuses the existing cedar_policies threading rather than inventing a parallel channel.
• Coherence — Pass. registry:// grammar, ResolvedAsset, and the triple are spelled consistently across TS/Py/CLI; resolve-step mirrors how cedar_policies already flows (orchestrator.ts:748-758).
• Clarity — Mostly pass; one concern: loader.py:189-195 fail-open on write hides a missing pinned tool behind an ERROR log (nit 1).
• Appropriateness — Pass. Cedar merge verified against real PolicyEngine behavior (policy.py:856-905), not a self-written mock; tests assert intended fail-closed semantics, not just current output.

[isadeks]

Verdict: Request changes

I re-reviewed approved head e05bfce3 independently against ADR-018 and the composed #664 base. The focused suites remain green (112/112 CDK, 39/39 Python), but live in-process probes show that typed Blueprint refs, detach/update behavior, loader completion, MCP identity, Cedar limits, and deprecation audit are not enforced. These let the task record claim a pinned asset that was not actually applied, so they block the advertised reproducibility/fail-closed contract.

[Kalindi-Dev]

Thanks @scottschreckengaust and @isadeks. This round closes all of @isadeks's P1/P2 inline findings and the fail-open surface @scottschreckengaust flagged. Rebased on the updated #664 and latest main; build (agentcore) green. Details:

@isadeks — P1/P2 blocking

1. Validate expected kind per typed Blueprint field (blueprint.ts:240) — RegistryRefValidation now takes an expectedKind and rejects a mismatch at synth (a skill ref under mcpServers, etc.). Added blueprint tests for each cross-kind rejection.

2. Emit REMOVE so empty asset arrays detach stored refs (blueprint.ts:397) — onUpdate now emits REMOVE #mcp_servers, … for empty asset arrays, so redeploying a repo that dropped its last ref actually detaches it in DynamoDB. Added populated→empty transition tests.

3. Use an injective MCP server key (loader.py:35) — _server_key no longer collapses hyphen→underscore, so foo-bar and foo_bar stay distinct instead of one silently overwriting the other. Added a collision test asserting both survive with distinct URLs.

4. Fail the task when a resolved asset can't be applied (loader.py:148) — The loader is now fail-closed: apply_mcp_assets raises RegistryAssetLoadError on empty/malformed runtime, and build_skill_prompt_fragment raises on a missing/blank prompt_fragment — instead of warn-and-skip. So a pinned asset either loads or the task fails; the stamped audit bundle can no longer claim an asset that wasn't applied. Added a pipeline.run_task propagation test proving a malformed asset marks the task FAILED and the agent never runs.

5. Include registry Cedar bytes in the 64 KiB cap (orchestrator.ts:852 / policy.py) — policy.py now counts extra_policies (the path registry cedar arrives on) toward the 64 KiB aggregate, so a large registry policy can't bypass the bound. Added a cap test with registry-supplied text.

6. Write the MCP config schema the runtime consumes (loader.py:83) — _to_mcp_config now maps transport → type (the discriminant the Claude Agent SDK + channel_mcp actually read) and validates the shape (http/sse require url, stdio requires command, unknown transport rejected). Tests assert a consumable config, not byte-for-byte persistence.

7. [P2] Persist deprecation warnings in the task audit surface (orchestrator.ts:543) — Deprecation now emits a durable registry_asset_warning TaskEvent and keeps warnings on the stamped resolved_assets triple, not just a Lambda log. Added an orchestrator test asserting the TaskEvent + stamped warning.

@scottschreckengaust — approve-with-nits

1. Agent-side MCP load failing open — Addressed by @isadeks feat: add FargateAgentStack as alternative compute backend #4 above: the loader is now fail-closed on a pinned asset, so a missing pinned tool fails the task rather than silently proceeding.
2. Registry cedar_text bypassing the 64 KB cap — Fixed (same as @isadeks feat: add Iteration 3e for memory security and integrity (OWASP ASI06) #5).
3. onUpdate not REMOVE-ing dropped columns — Fixed (same as @isadeks Docs: specify that you can't use the agent with the canned repo #2).
4. forkBlueprintRepo demo hook undocumented — Documented the demo flag (REGISTRY.md §12.1).

Additional hardening from a second internal review pass

• Security (ADR-016 Linear bypass): the registry MCP merge ran after strip_linear_mcp_servers, so a registry-published Linear server could be re-added under bypassPermissions. Fixed by re-running the strip after the registry merge, with a WARN log if it removes a registry-introduced Linear entry. Test added.
• Fail-open on empty cedar: an empty/whitespace cedar_text on a pinned module was silently dropped by a .filter(length>0) while still stamped as applied (a dropped deny rule = widened permissions). Now throws RegistryResolutionError. Test added.
• Fail-closed propagation tests for both hydrateAndTransition (CDK) and pipeline.run_task (agent) proving an unresolvable/malformed asset fails the task.

@isadeks — these were the items behind your change request; re-review welcome when you have a moment.

[scottschreckengaust]

Verdict

Request changes — all 7 of isadeks' prior blocking findings are genuinely FIXED at 8362f4fb, each with a regression test; I verified every one against current code, not the commit message. But the newly-added loader creates one new P1 that #665 itself introduces: the registry merge writes the unredacted runtime payload (which the base PR's own redaction docstring says may carry Authorization headers / --api-key args) into <repo_dir>/.mcp.json inside a live git working tree, and the post-hook safety net (git add -u → commit → push) will exfiltrate it to the PR whenever the target repo tracks .mcp.json. That is a #665-created exposure, not a re-report of #664's resolve-endpoint blocker.

This is a high-quality revision. The gap is narrow and mechanically fixable.

Vision alignment

Strongly advances bounded blast radius and reviewable outcomes (VISION.md, ADR-022). The whole point of the force-push was to convert five fail-open seams into fail-closed ones, and it does: synth-time kind validation, DDB REMOVE for detach, RegistryAssetLoadError propagating to write_terminal(FAILED), the 64 KB cap folded over extra_policies, and a durable registry_asset_warning TaskEvent. The resolved_assets audit triple is now accurate by construction rather than by hope — that is exactly the right invariant.

One genuinely excellent touch nobody asked for: agent/src/pipeline.py:1168 re-runs strip_linear_mcp_servers after the registry merge, closing an ADR-016 bypass (a registry-published Linear server would otherwise land post-strip and run under bypassPermissions). That is the house fail-closed instinct applied unprompted.

Where it drifts from the tenet is secret containment: the loader treats the runtime payload as inert config, but #664 explicitly classifies it as secret-bearing. Fail-closed on availability is done; fail-closed on confidentiality is not.

Disposition of each prior blocking finding

I re-tested all 7 at the current head. 7 FIXED, 0 still live, 0 moot.

(1) P1 typed Blueprint fields don't validate expected kind — FIXED.
cdk/src/constructs/blueprint.ts:538-559: RegistryRefValidation now takes an expectedKind third arg and rejects result.ref.kind !== this.expectedKind; wired at :261-263 with 'mcp_server' / 'cedar_policy_module' / 'skill'. Regression test: cdk/test/constructs/blueprint.test.ts — 'rejects a ref whose kind does not match its field at synth' asserts the exact message for a skill ref under mcpServers. Locks the fix in.

(2) P1 empty arrays don't remove stored refs — FIXED.
blueprint.ts:443-461: new emptyAssetFields() / buildRemoveClause() / buildRemoveNames(); :337 composes SET … ${this.buildUpdateFields(props)}${this.buildRemoveClause()} and :342 merges buildRemoveNames(). Two regression tests: 'onUpdate REMOVEs asset columns that are now empty' (partial: REMOVE #cedar_policy_modules, #skills) and '…when none are pinned' (all three), plus the populated case asserting not.toContain('REMOVE'). Populated→empty transition is covered as asked.

(3) P1 MCP server key not injective — FIXED.
agent/src/registry/loader.py:32-41: the hyphen→underscore normalization is gone; _server_key returns f"{namespace}__{name}" raw, with a comment stating why. foo-bar → acme__foo-bar and foo_bar → acme__foo_bar are now distinct. Regression test: test_hyphen_and_underscore_names_do_not_collide (loader.py test file, line 86) — asserts two keys survive. Verified injective.

(4) P1 task not failed when a resolved asset can't be applied — FIXED, and this is the deepest fix.
New RegistryAssetLoadError (loader.py:104) raised on: missing/non-dir repo_dir (:135), empty/non-dict runtime (:151), structurally invalid transport (:63-77), OSError on write (:166). The wrapper apply_resolved_assets (:209) now returns the written keys and propagates. The pipeline caller at agent/src/pipeline.py:1157-1160 no longer discards it, and the raise reaches the outer handler at pipeline.py:1746 → task_state.write_terminal(config.task_id, "FAILED", …) → raise. Skills fail closed too (loader.py:198, blank prompt_fragment raises). Regression tests: test_pipeline.py::test_malformed_registry_asset_fails_the_task_closed asserts agent_ran is False and a write_terminal(…, 'FAILED', …) call — i.e. it tests the invariant, not the current output. Plus 6 loader-level raise tests. I ran the agent suite: 226 passed.

(5) P1 registry Cedar bytes excluded from the enforced cap — FIXED.
agent/src/policy.py:882-902: operator_text now concatenates blueprint_hard_policies + blueprint_soft_policies + *(extra_policies or []) before the POLICIES_MAX_BYTES check. isadeks' 65,574-byte probe would now raise. Regression test: test_policy_three_outcome.py::test_registry_extra_policies_counted_in_64kb_cap passes an oversized policy via extra_policies= alone and expects ValueError(/64 KB cap/). This also closes what my own prior review filed as non-blocking nit 2 — nice.

(6) P2 deprecation warnings dropped from the audit surface — FIXED (exceeded the ask).
cdk/src/handlers/shared/orchestrator.ts:803-810 persists ...(a.warnings.length > 0 && { warnings: [...a.warnings] }) into the resolved_assets stamp, and :816-827 emits a durable emitTaskEvent(task.task_id, 'registry_asset_warning', …) per warned asset. ADR-022 sub-decision 4 is now actually satisfied. Regression test: orchestrate-task.test.ts — 'emits a registry_asset_warning TaskEvent for a DEPRECATED asset' asserts both the Put and [':ra'][0].warnings.

(7) P1 MCP config schema mismatch (transport vs type) — FIXED.
loader.py:44-82: new _to_mcp_config maps transport → type (the discriminant McpHttpServerConfig/McpSSEServerConfig and channel_mcp._jira_server_entry() at channel_mcp.py:78 actually use), passes everything else through, and is idempotent for payloads already in SDK shape (:78-79). It also validates — http/sse without url, stdio without command, unknown transport all raise. Regression tests: test_normalizes_transport_to_type, test_http_without_url_raises, test_stdio_without_command_raises, test_unknown_transport_raises, test_stdio_with_command_loads — and per isadeks' explicit request these assert a consumable config, not byte-for-byte persistence.

On the base PR (#664) credential finding — not double-reported

I treated as established fact that #664's resolve path returns credential-bearing runtime data in cleartext (denylist redaction over 3 field names vs. publish accepting unknown runtime keys). That is #664's blocker and I am not re-reporting it here. But per the two implied questions:

• Does #665 lean on a fail-closed guarantee #664 doesn't provide? No. #665's fail-closed claims are all about availability/audit accuracy (a pinned asset either loads or the task fails), and it enforces those itself. It does not depend on #664's redaction for anything.
• Does #665 WIDEN the exposure? Yes — see B1. #664 leaks to an authenticated API caller; #665 is what writes the same values into a file inside a git working tree that gets committed and pushed to a public-capable PR. The additional exposure is created by #665 code (loader.py:161-165), so it is reported here.

New blocking issues

B1 [P1-security] agent/src/registry/loader.py:161-165 — the unredacted runtime payload is written into a tracked git working tree and can be committed + pushed to the PR

config["mcpServers"] = servers; json.dump(config, f, indent=2) writes the resolved runtime verbatim into <repo_dir>/.mcp.json. _to_mcp_config (:80-82) deliberately passes every non-transport key through untouched — so headers: {Authorization: "Bearer …"}, url: "https://…?token=…", api_key, env.TOKEN, and args: ["--api-key=…"] all land on disk in cleartext. #664's own redactRuntimeForResponse docstring (cdk/src/handlers/registry-resolve.ts:31-43) states these fields are secret-bearing, and publish validation (registry-publish.ts:174-176) only type-checks headers — it does not require ${ENV_VAR} placeholders, so literal secrets are publishable.

Risk — verified empirically, not reasoned: repo_dir is the live clone. If the target repo tracks .mcp.json (common — ABCA gitignores it, but arbitrary onboarded repos commit theirs; the strip logic at channel_mcp.py:200 exists precisely because "a repo could COMMIT a .mcp.json"), then the registry merge shows as a modification to a tracked file. I reproduced the full chain in a scratch repo:

$ git status --porcelain     →   M .mcp.json
$ git add -u                 # exactly post_hooks.ensure_committed:286
$ git diff --cached --quiet  →  STAGED (exit 1)
$ git diff --cached | grep -c 'SUPERSECRET|sk-live-abc123|ghp_realtoken'  →  3

post_hooks.ensure_committed (agent/src/post_hooks.py:253-324, invoked at pipeline.py:1434) stages tracked-but-modified files, commits chore(agent): save uncommitted work from session end, then ensure_pushed (post_hooks.py:418-436) does git push -u origin <branch>. Net effect: an operator-pinned MCP server's bearer token is committed to the agent's branch and pushed to the PR. There is no secret scan on the agent's push path (output_scanner.py screens tool output, not commits). This breaks bounded blast radius: the blast radius of one pinned asset becomes "the credential is now in git history."

Note git add -u does not stage it when .mcp.json is untracked — so the exposure is conditional on the target repo tracking it. That makes it a narrow window, not a theoretical one, and the agent can also git add <specific files> per its own prompt (prompts/new_task.py:67).

Suggested fix — pick either, both are small:

1. (preferred, defense in depth) After writing, mark the file so git cannot stage it: git update-index --skip-worktree .mcp.json (add --add / git add --intent-to-add first if untracked). I verified this blocks both git add -u and an explicit git add .mcp.json ("paths … outside of your sparse-checkout definition, so will not be updated in the index"). This mirrors the ADR-016 posture: enforce mechanically, don't rely on the agent's good behavior.
2. Require indirection at the boundary: have registry-publish.ts::validateRuntime reject literal secrets in headers/args/url (accept only ${ENV_VAR} placeholders, as channel_mcp._jira_server_entry() already does with Bearer ${JIRA_API_TOKEN}), so nothing secret can be published and therefore nothing secret can be written. This is the real fix but it touches #664's handler.

Either way please add a regression test: write a header/api_key-bearing asset, run apply_mcp_assets, then assert git add -u && git diff --cached is empty (or that the value on disk is a placeholder).

Non-blocking nits

1. loader.py:109 and :123 — docstrings contradict the code they document. :109 says degraded conditions "(empty runtime, malformed existing config), which warn + skip", and :123 lists "an empty / non-dict runtime payload" under fail-closed. The code at :151-154 raises on empty runtime. The class docstring and the apply_mcp_assets list disagree with each other; :123 is the correct one. Same stale phrasing was copied into pipeline.py:1155-1156 ("Degraded-but-safe cases (empty runtime) are warn+skip inside the loader") — that comment is now false. Please align all three; a comment that misstates fail-open/fail-closed is the kind of thing a future reader will trust.
2. loader.py:158-159 — dead branch. if not written: return [] is unreachable: the loop either appends to written or raises, and the not mcp_assets early-return at :132 already covers the empty case. Harmless, but it implies a skip path that no longer exists (and reinforces nit 1's wrong mental model).
3. cdk/src/constructs/task-orchestrator.ts:487-497 — IAM resource ARNs use '*' for the registry id when the id is in scope. The statement is guarded by if (props.agentRegistryId) at :479, and registry-api.ts:111-122 scopes the same actions to resourceName: props.agentRegistryId and `${props.agentRegistryId}/record/*`. The orchestrator instead uses '*' and '*/record/*', granting read across every AgentCore registry in the account. The cdk-nag reason at :665 justifies the wildcard as "record ids are server-assigned" — true for the /record/* suffix, but it does not justify wildcarding the registry id, which is known. Swap in props.agentRegistryId to match the sibling construct. I am filing this as a nit rather than blocking because (a) it is a read-only grant on a preview API, and (b) the near-identical '*'-ARN concern is already an open P1 on #664 — fix it in whichever PR you prefer, but please don't let it merge as-is in both.
4. No cap on total skill prompt_fragment bytes. build_skill_prompt_fragment (loader.py:173-206) concatenates unbounded operator text into the system prompt. Cedar text now has a 64 KB cap (finding 5) and MCP has no size axis, so skills are the remaining unbounded operator-supplied surface — it burns context/cost rather than widening authorization, so it is a cost-bound nit, not a security one. Worth a follow-up cap for symmetry with POLICIES_MAX_BYTES.
5. stacks/agent.ts:194-207 forkBlueprintRepo — my prior review's nit 4 is now resolved: documented at docs/design/REGISTRY.md §12.1 with both invocation forms and an explicit "those acme/* records must be published first" caveat. Thanks.

Documentation status

Good, and the mirror is correctly synced. docs/design/REGISTRY.md §12.1 (+19) and its Starlight mirror docs/src/content/docs/architecture/Registry.md (+19) were both regenerated — I diffed the new section between source and mirror and they are byte-identical, so the "Fail build on mutation" step will not trip. No hand-edit of docs/src/content/docs/ beyond the generated mirror. The prose does not over-claim: it says the acme/* records are "illustrative, not seeded" and that admission "fails closed on the unresolved pins", which matches what the code actually does.

Tests and CI

• Agent (ran locally): 226 passed in 1.81s across test_registry_loader.py (25 tests), test_pipeline.py, test_policy_three_outcome.py, test_entrypoint.py. Coverage is genuinely invariant-oriented — the pipeline test asserts agent_ran is False and the FAILED write, rather than snapshotting output.
• CDK: could not run locally — npx jest dies with TS5103: Invalid value for '--ignoreDeprecations' (stale TypeScript 5.9.3 in my environment vs. cdk/package.json's "typescript": "^6.0.3"). This is my environment, not the PR: it reproduces identically on main. So I read the CDK tests rather than executing them — 10 new blueprint.test.ts cases, 5 new orchestrate-task.test.ts registry cases, and the new registry-orchestrator.test.ts (+141). Marking the CDK run unverified locally; CI covers it.
• CI at 8362f4fb: build (agentcore) success, Secrets, deps, and workflow scan success, Dead-code detection (advisory) success, Validate PR title success. (Green CI is not why I'd approve; noting it only for completeness.)
• No CDK test-perf regression (#366): no aws:cdk:bundling-stacks re-enable, no per-test synth introduced.
• Bootstrap synth coverage (ADR-002 / #350): no update needed in this PR, verified. git diff 4f8da982..HEAD -- cdk/src/bootstrap cdk/bootstrap docs/design/DEPLOYMENT_ROLES.md is empty, and correctly so: #665 introduces no new CloudFormation resource type. Its only infra deltas are one iam.PolicyStatement on an existing Lambda role, one env var, and one conditional Blueprint (an AwsCustomResource type already covered). The bootstrap artifacts (BOOTSTRAP_VERSION, BOOTSTRAP_HASH, bootstrap-template.yaml, policies/*.json, src/bootstrap/policies/*.ts) all moved in the base PR at d3e2b056/4f8da982 where the registry construct + nested stack actually landed — that is the right PR for them. I did not re-audit #664's ARN patterns here.
• Repo-specific sync checks: cdk/src/handlers/shared/types.ts and cli/src/types.ts are untouched by this PR (git diff 4f8da982..HEAD empty for both), and I confirmed ResolvedAssetTriple is already identical in the two (types.ts:56-60 ≡ cli/src/types.ts:37-41) — types-sync contract holds. No Cedar engine pin movement (cedar-wasm/cedarpy untouched), so no parity-fixture refresh owed. Solution UA (#319): clean — the diff adds no new XxxClient({}) and no bare boto3.client(...); the only client construction is the pre-existing makeDocClient() at repo-config.ts:1328.

Review agents run

The pr-review-toolkit subagents were not dispatchable in this execution context (I am myself a subagent; nesting is limited to one level and the Agent tool is unavailable — ToolSearch for it returned no match). Per instruction I applied each rubric explicitly and label it rubric-applied, not agent-run:

• code-reviewer (rubric-applied): change routing correct per AGENTS.md (agent runtime in agent/, orchestration/IAM in cdk/, docs + mirror together). CDK L2/IAM idioms clean; ArnFormat.SLASH_RESOURCE_NAME used correctly. Found nit 3 (registry-id wildcard diverging from the sibling registry-api.ts).
• silent-failure-hunter (rubric-applied): the primary lens for this re-review. Traced every new error path to a terminal state: loader raise → pipeline.py:1157 (no try/except swallow) → outer except Exception at 1746 → write_terminal(FAILED) → raise. Confirmed the orchestrator resolve path also fails closed (orchestrator.ts:795, empty-cedar_text raise at :845-852). Remaining fail-open surfaces are all pre-existing and intentional (_read_existing_mcp_config warn+treat-as-absent; strip_linear_mcp_servers best-effort). No swallowed failure found in new code — this rubric is what confirms finding 4 is truly fixed.
• type-design-analyzer (rubric-applied): RegistryAssetLoadError(RuntimeError) is the right granularity — it distinguishes infra failure from degraded-safe, and is narrow enough that the pipeline needs no discriminating except. ResolvedAssetTriple CDK↔CLI parity re-confirmed. resolved_assets: list[dict[str, Any]] (models.py:235) stays an untyped passthrough — pragmatic, but it is why the secret-bearing keys in B1 are invisible to the type system; a typed per-kind runtime model would have surfaced it.
• comment-analyzer (rubric-applied): found nits 1 and 2. The new comments are unusually good where they explain why (the _server_key injectivity note, the ADR-016 re-strip rationale), but three fail-closed/fail-open descriptions are now stale relative to the code they sit on.
• pr-test-analyzer (rubric-applied): every one of the 7 fixes has a matching regression test that asserts the intended invariant. Gaps: no test for B1's disk-exposure path, and no test that a skill fragment is bounded (nit 4).
• security-review skill scope (rubric-applied): in scope (IAM statement, Cedar policy limits, secrets on disk, publish input gateway). Produced B1 and nit 3. Confirmed the ADR-016 re-strip actually closes the registry-Linear bypass, and that the 64 KB cap now genuinely bounds registry Cedar.
• Omitted: none — the diff touches every rubric's scope.

Human heuristics

• Proportionality — Pass. Seven findings addressed with ~+120 lines of production code and ~+400 of tests. No speculative abstraction; _to_mcp_config and emptyAssetFields() are each one small single-purpose function. The fixes reuse existing seams (extra_policies cap, emitTaskEvent) rather than inventing parallel machinery.
• Coherence — Pass. The transport→type mapping now agrees with channel_mcp._jira_server_entry() and the SDK; REMOVE semantics match the SET structure; the registry merge sits in the correct pipeline slot (after clone, after the first strip, before discover_project_config at pipeline.py:1204) and re-applies the ADR-016 strip. One coherence seam: task-orchestrator.ts:487 scopes registry IAM differently from registry-api.ts:111 for the same actions (nit 3).
• Clarity — Concern. agent/src/registry/loader.py:109, :123, and agent/src/pipeline.py:1155-1156 describe empty-runtime as "warn + skip" while :151-154 raises. The single most important property of this file is which conditions fail the task, and the docstrings currently get it wrong (nits 1-2).
• Appropriateness — Pass, with one gap. Tests assert intended semantics against the real PolicyEngine and the real .mcp.json consumer shape, not a self-written mock — and the pipeline test verifies the agent never ran, which is the property that actually matters. The gap is that "fail closed" was interpreted purely as availability; confidentiality of the payload the loader now persists to disk was not considered (B1).

---

To be explicit about what I did not inherit: I did not treat ayushtr-aws' same-day APPROVE as discharging isadeks' change request, and I did not carry forward my own earlier APPROVE. Every disposition above is from reading current code at 8362f4fb. Fix B1 (a two-line git update-index --skip-worktree plus a test is sufficient) and align the three stale comments, and I would approve — the prior-finding work is done and done well. Merge order remains #664 → #665.

[scottschreckengaust]

[P1-security / BLOCKING] The unredacted runtime payload is written into a live git working tree and can be committed + pushed to the PR.

_to_mcp_config passes every non-transport key through untouched (:80-82), so headers: {Authorization: "Bearer …"}, url: "…?token=…", api_key, env.TOKEN, and args: ["--api-key=…"] land here verbatim. #664's own redaction docstring (cdk/src/handlers/registry-resolve.ts:31-43) classifies exactly these fields as secret-bearing, and registry-publish.ts:174-176 only type-checks headers — literal secrets are publishable.

repo_dir is the live clone. I reproduced the full exfiltration chain in a scratch repo where .mcp.json is tracked:

$ git status --porcelain     →   M .mcp.json
$ git add -u                 # == post_hooks.ensure_committed:286
$ git diff --cached --quiet  →  STAGED
$ git diff --cached | grep -c 'SUPERSECRET|sk-live-abc123|ghp_realtoken'  →  3

post_hooks.ensure_committed (agent/src/post_hooks.py:253-324, called from pipeline.py:1434) stages tracked-but-modified files and commits; ensure_pushed (:418-436) then git push -u origin <branch>. A pinned MCP server's bearer token ends up in the PR's git history. No secret scan guards the agent's push path (output_scanner.py screens tool output, not commits).

This is distinct from #664's resolve-endpoint leak (already reported there): that one exposes to an authenticated API caller, whereas this line is what puts the value on disk in a tree that gets pushed. git add -u alone won't stage it when .mcp.json is untracked, so the window is conditional on the target repo tracking it — narrow, but real, and the strip logic at channel_mcp.py:200 exists precisely because repos do commit .mcp.json.

Suggested fix (either; #1 verified):

1. After writing, mark it unstageable: git update-index --skip-worktree .mcp.json (--add/--intent-to-add first if untracked). I confirmed this blocks both git add -u and an explicit git add .mcp.json. Mirrors the ADR-016 posture — enforce mechanically.
2. Make registry-publish.ts::validateRuntime reject literal secrets in headers/args/url, accepting only ${ENV_VAR} placeholders (as channel_mcp._jira_server_entry() already does with Bearer ${JIRA_API_TOKEN}). This is the deeper fix but touches feat(registry): agent asset catalog on AgentCore — provisioning, port/adapter, API, CLI (#246) #664.

Please add a regression test: write a header/api_key-bearing asset, call apply_mcp_assets, then assert git add -u && git diff --cached is empty (or that the on-disk value is a placeholder).

[scottschreckengaust]

[nit — comment accuracy] This docstring contradicts the code.

It says degraded conditions "(empty runtime, malformed existing config), which warn + skip", but :151-154 raises RegistryAssetLoadError on an empty/non-dict runtime. The apply_mcp_assets docstring at :123 correctly lists it under fail-closed, so the two disagree with each other — :123 is right.

The same stale phrasing was copied to agent/src/pipeline.py:1155-1156 ("Degraded-but-safe cases (empty runtime) are warn+skip inside the loader"), which is now false.

Which conditions fail the task is the load-bearing property of this module; please align all three so a future reader doesn't trust the wrong one. Only _read_existing_mcp_config (:85-101) is genuinely warn-and-continue.

[scottschreckengaust]

[nit — dead code] Unreachable branch.

if not written: return [] can never fire: the loop above either appends to written or raises, and the not mcp_assets early return at :132 already handles the empty-input case. Harmless, but it implies a skip path that no longer exists after the fail-closed rework — which reinforces the stale "warn + skip" wording flagged at :109. Suggest deleting it.

[scottschreckengaust]

[nit — IAM least privilege] Wildcards the registry id even though the id is in scope.

This statement is guarded by if (props.agentRegistryId) at :479, so the concrete id is available — but the resources are registry/* and registry/*/record/*, granting GetRegistryRecord/ListRegistryRecords against every AgentCore registry in the account.

The sibling construct scopes the identical actions properly (cdk/src/constructs/registry-api.ts:111-122): resourceName: props.agentRegistryId and `${props.agentRegistryId}/record/*`. The cdk-nag reason added at :665 justifies the wildcard as "record ids are server-assigned" — accurate for the /record/* suffix, but it does not justify wildcarding the registry id itself.

Suggested change

	resourceName: '*',
	resourceName: props.agentRegistryId,

(and `${props.agentRegistryId}/record/*` for the second ARN, then trim the nag reason accordingly).

Filing as a nit rather than blocking because it is a read-only grant on a preview API and a near-identical '*'-ARN concern is already an open P1 on #664 — fix it in whichever PR you prefer, but please don't let both merge with it.