feat(registry): resolve + load registry assets into tasks (#246) - #665
Conversation
…-check (aws-samples#246) The ADR cited cdk/src/handlers/shared/registry/ref.ts and agent/src/registry/ref.py as relative-path links, but those files ship in the implementation PRs (aws-samples#664/aws-samples#665), not on this ADR branch or main — so //docs:link-check failed with 2 dead links. Demoted both to inline code spans (with a note that they land with aws-samples#664/aws-samples#665) until the implementation merges. Mirror regenerated via docs sync.
d3dde44 to
e05bfce
Compare
Self-review (principal-architect pass)Ran a Verified correct:
One finding worth stating explicitly (documented tradeoff, not a blocker): a resolved Rebased onto |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
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
- Agent-side MCP load fails open (fail-closed is resolution-only).
agent/src/registry/loader.py:189-195— on an.mcp.jsonwrite error,apply_mcp_assetslogs 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 onlymcp_serverdegrades silently — and it mirrors the existingconfigure_channel_mcpposture. Consider surfacing a task-visible warning (progress event) when a resolved MCP asset fails to load, so a pinned tool silently missing is observable. - Registry cedar_text bypasses the 64 KB blueprint cap.
agent/src/policy.py:884-889counts onlyblueprint_hard_policies + blueprint_soft_policies; registry cedar modules arrive via the legacyextra_policiespath (runner.py:288), which is uncapped. Pre-existing property of theextra_policieskwarg, not introduced here, but the registry now makes it operator-reachable at scale — worth a follow-up to foldextra_policiesbyte-length into the cap. - onUpdate does not REMOVE dropped asset columns.
cdk/src/constructs/blueprint.ts:363-365,400-402gate the write onlength > 0, so redeploying an onboarded repo that removed all its asset refs leaves the stalemcp_servers/cedar_policy_modules/skillscolumns in DDB. This exactly matches the siblingcedar_policies/egress_allowlistbehavior, 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. forkBlueprintRepodemo hook is undocumented.cdk/src/stacks/agent.ts:176-192adds an opt-in context/env flag pinning hardcodedacme/*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 incdk/); 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):
ResolvedAssetTripleparity CDK↔CLI confirmed; agentresolved_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.pyextra_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 howcedar_policiesalready flows (orchestrator.ts:748-758). - Clarity — Mostly pass; one concern:
loader.py:189-195fail-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
left a comment
isadeks
left a comment
There was a problem hiding this comment.
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.
…ation, cutover) (aws-samples#246) Addresses review feedback from @krokoko, @scottschreckengaust, @isadeks: - Revert premature proposed→accepted (README rule: accepted on impl-PR merge; aws-samples#664/aws-samples#665 still in review). Soften "shipped / proven E2E on a live stack" to "targeted by aws-samples#664/aws-samples#665, exercised on a dev stack during review"; stop citing the parked DDB+S3 PRs (aws-samples#632-aws-samples#634) as current. Add a Status note in Decision. - Add short-vs-long-form kind-vocabulary migration note to sub-decision 1: WORKFLOWS.md short forms (registry://mcp/…) are lenient-only forward-decls; only the long form (mcp_server/ns/name@constraint) resolves. No auto-aliasing. - Add a federation / "registry of registries" Non-goal (answers Scott's Jul-8 question): single operator-curated catalog; external registries are discovery-only; no federation in aws-samples#246. - Promote the 2026-08-06 AgentCore namespace cutover from a cost input to a hard gate: no production dependency until the migration is GA in-region. Mirror regenerated via docs sync (idempotent).
d3e2b05 to
dc33a74
Compare
1eac65b to
bb85527
Compare
dc33a74 to
4f8da98
Compare
bb85527 to
8362f4f
Compare
|
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 @isadeks — P1/P2 blocking
@scottschreckengaust — approve-with-nits
Additional hardening from a second internal review pass
@isadeks — these were the items behind your change request; re-review welcome when you have a moment. |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
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:
- (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-addfirst if untracked). I verified this blocks bothgit add -uand an explicitgit 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. - Require indirection at the boundary: have
registry-publish.ts::validateRuntimereject literal secrets inheaders/args/url(accept only${ENV_VAR}placeholders, aschannel_mcp._jira_server_entry()already does withBearer ${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
loader.py:109and:123— docstrings contradict the code they document.:109says degraded conditions "(empty runtime, malformed existing config), which warn + skip", and:123lists "an empty / non-dict runtime payload" under fail-closed. The code at:151-154raises on empty runtime. The class docstring and theapply_mcp_assetslist disagree with each other;:123is the correct one. Same stale phrasing was copied intopipeline.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.loader.py:158-159— dead branch.if not written: return []is unreachable: the loop either appends towrittenor raises, and thenot mcp_assetsearly-return at:132already covers the empty case. Harmless, but it implies a skip path that no longer exists (and reinforces nit 1's wrong mental model).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 byif (props.agentRegistryId)at:479, andregistry-api.ts:111-122scopes the same actions toresourceName: props.agentRegistryIdand`${props.agentRegistryId}/record/*`. The orchestrator instead uses'*'and'*/record/*', granting read across every AgentCore registry in the account. The cdk-nag reason at:665justifies 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 inprops.agentRegistryIdto 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.- No cap on total skill
prompt_fragmentbytes.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 withPOLICIES_MAX_BYTES. stacks/agent.ts:194-207forkBlueprintRepo— my prior review's nit 4 is now resolved: documented atdocs/design/REGISTRY.md§12.1 with both invocation forms and an explicit "thoseacme/*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.81sacrosstest_registry_loader.py(25 tests),test_pipeline.py,test_policy_three_outcome.py,test_entrypoint.py. Coverage is genuinely invariant-oriented — the pipeline test assertsagent_ran is Falseand the FAILED write, rather than snapshotting output. - CDK: could not run locally —
npx jestdies withTS5103: 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 onmain. So I read the CDK tests rather than executing them — 10 newblueprint.test.tscases, 5 neworchestrate-task.test.tsregistry cases, and the newregistry-orchestrator.test.ts(+141). Marking the CDK run unverified locally; CI covers it. - CI at
8362f4fb:build (agentcore)success,Secrets, deps, and workflow scansuccess,Dead-code detection (advisory)success,Validate PR titlesuccess. (Green CI is not why I'd approve; noting it only for completeness.) - No CDK test-perf regression (#366): no
aws:cdk:bundling-stacksre-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.mdis empty, and correctly so: #665 introduces no new CloudFormation resource type. Its only infra deltas are oneiam.PolicyStatementon an existing Lambda role, one env var, and one conditionalBlueprint(anAwsCustomResourcetype already covered). The bootstrap artifacts (BOOTSTRAP_VERSION,BOOTSTRAP_HASH,bootstrap-template.yaml,policies/*.json,src/bootstrap/policies/*.ts) all moved in the base PR atd3e2b056/4f8da982where 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.tsandcli/src/types.tsare untouched by this PR (git diff 4f8da982..HEADempty for both), and I confirmedResolvedAssetTripleis 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/cedarpyuntouched), so no parity-fixture refresh owed. Solution UA (#319): clean — the diff adds nonew XxxClient({})and no bareboto3.client(...); the only client construction is the pre-existingmakeDocClient()atrepo-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 incdk/, docs + mirror together). CDK L2/IAM idioms clean;ArnFormat.SLASH_RESOURCE_NAMEused correctly. Found nit 3 (registry-id wildcard diverging from the siblingregistry-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) → outerexcept Exceptionat1746→write_terminal(FAILED)→raise. Confirmed the orchestrator resolve path also fails closed (orchestrator.ts:795, empty-cedar_textraise at:845-852). Remaining fail-open surfaces are all pre-existing and intentional (_read_existing_mcp_configwarn+treat-as-absent;strip_linear_mcp_serversbest-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 discriminatingexcept.ResolvedAssetTripleCDK↔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_keyinjectivity 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_configandemptyAssetFields()are each one small single-purpose function. The fixes reuse existing seams (extra_policiescap,emitTaskEvent) rather than inventing parallel machinery. - Coherence — Pass. The
transport→typemapping now agrees withchannel_mcp._jira_server_entry()and the SDK;REMOVEsemantics match theSETstructure; the registry merge sits in the correct pipeline slot (after clone, after the first strip, beforediscover_project_configatpipeline.py:1204) and re-applies the ADR-016 strip. One coherence seam:task-orchestrator.ts:487scopes registry IAM differently fromregistry-api.ts:111for the same actions (nit 3). - Clarity — Concern.
agent/src/registry/loader.py:109,:123, andagent/src/pipeline.py:1155-1156describe empty-runtime as "warn + skip" while:151-154raises. 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
PolicyEngineand the real.mcp.jsonconsumer 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.
4f8da98 to
c8e4790
Compare
8362f4f to
be84b37
Compare
…-check (aws-samples#246) The ADR cited cdk/src/handlers/shared/registry/ref.ts and agent/src/registry/ref.py as relative-path links, but those files ship in the implementation PRs (aws-samples#664/aws-samples#665), not on this ADR branch or main — so //docs:link-check failed with 2 dead links. Demoted both to inline code spans (with a note that they land with aws-samples#664/aws-samples#665) until the implementation merges. Mirror regenerated via docs sync.
…ation, cutover) (aws-samples#246) Addresses review feedback from @krokoko, @scottschreckengaust, @isadeks: - Revert premature proposed→accepted (README rule: accepted on impl-PR merge; aws-samples#664/aws-samples#665 still in review). Soften "shipped / proven E2E on a live stack" to "targeted by aws-samples#664/aws-samples#665, exercised on a dev stack during review"; stop citing the parked DDB+S3 PRs (aws-samples#632-aws-samples#634) as current. Add a Status note in Decision. - Add short-vs-long-form kind-vocabulary migration note to sub-decision 1: WORKFLOWS.md short forms (registry://mcp/…) are lenient-only forward-decls; only the long form (mcp_server/ns/name@constraint) resolves. No auto-aliasing. - Add a federation / "registry of registries" Non-goal (answers Scott's Jul-8 question): single operator-curated catalog; external registries are discovery-only; no federation in aws-samples#246. - Promote the 2026-08-06 AgentCore namespace cutover from a cost input to a hard gate: no production dependency until the migration is GA in-region. Mirror regenerated via docs sync (idempotent).
|
Thanks @scottschreckengaust — all 7 prior findings you re-verified stay fixed, and this round closes B4 (the |
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Verdict
Approve. We reviewed head 0b06ff56 against the composed base (feat/246-registry-catalog @ c8e4790c), re-verified every prior blocking finding in current code, and ran both touched test suites locally. All of @isadeks's 7 findings and @scottschreckengaust's B4 secret-exfiltration blocker are genuinely fixed with regression tests that assert the invariant. One narrow residual in the new commit-guard is filed inline as a non-blocking suggestion.
Disposition of prior blocking findings (verified in code at 0b06ff56, not from commit messages)
| Finding | Status | Where verified |
|---|---|---|
| isadeks 1 — typed field kind validation | Fixed | blueprint.ts RegistryRefValidation(field, refs, expectedKind) rejects cross-kind refs at synth; mismatch tests present |
| isadeks 2 — empty arrays don't detach refs | Fixed | emptyAssetFields()/buildRemoveClause() emit REMOVE on onUpdate; populated→empty tested |
| isadeks 3 — non-injective MCP key | Fixed | _server_key no longer collapses hyphens; collision test present |
| isadeks 4 — task not failed on unapplied asset | Fixed | RegistryAssetLoadError raised on missing repo_dir / empty runtime / invalid transport / write error; propagates to write_terminal(FAILED); pipeline test asserts agent_ran is False |
| isadeks 5 — registry Cedar bypasses 64 KB cap | Fixed | policy.py counts extra_policies (the path registry cedar arrives on via runner.py:289) in the cap; oversized-via-extra_policies test present |
| isadeks 6 — deprecation warnings not durable | Fixed | warnings persisted on the resolved_assets stamp + registry_asset_warning TaskEvent per warned asset |
isadeks 7 — transport vs type schema mismatch |
Fixed | _to_mcp_config maps the discriminant and validates shape (http/sse need url, stdio needs command) |
scottschreckengaust B4 — .mcp.json secret exfiltration via safety-net commit |
Fixed | _protect_mcp_json_from_commit marks the file skip-worktree (--intent-to-add first when untracked); regression tests cover both the tracked and untracked cases with secret-bearing payloads |
| scottschreckengaust nits 1–3 (stale docstrings, dead branch, registry-id IAM wildcard) | Fixed | Docstring/comment now match the fail-closed behavior; dead if not written removed; grant scoped to registry/{agentRegistryId} with only the server-assigned record/* suffix wildcarded, plus a construct test rejecting registry/* |
We independently re-probed the B4 mechanics in a scratch repo: with skip-worktree set, git add -u, explicit git add .mcp.json, git add -f, and git commit -a all fail to stage the file, and the flag survives git checkout -b (relevant because reconcile_agent_branch can move the agent to a different branch before ensure_committed runs). The exact chain Scott reproduced (tracked .mcp.json → add -u → staged secret) is closed.
Tests and CI
- Agent (ran locally):
229 passed in 2.23sacrosstest_registry_loader.py,test_pipeline.py,test_policy_three_outcome.py,test_entrypoint.py— includes the two newTestMcpJsonNotCommittablecases. - CDK (ran locally): the 4 touched suites (
blueprint,task-orchestrator,orchestrate-task,registry-orchestrator) —211 passed. (The coverage-threshold warning is an artifact of running a subset, not a failure.) - CI at
0b06ff56: all four checks green (build (agentcore), secrets/deps scan, dead-code advisory, PR title).
Residual notes (non-blocking)
- Commit-guard is best-effort even in the dangerous case — inline comment on
loader.py. Narrow window; suggestion is a two-line tightening. - Skill
prompt_fragmentbytes remain uncapped (Scott's prior nit 4, not addressed this round). Cost-bound, not authorization-bound — fine as the already-noted follow-up. - Repoless boundary observation, no action needed:
apply_resolved_assetsonly runs on the repo path. A repoless task can't reach it today because asset refs live on per-repo Blueprint config, so a repoless task never hasresolved_assetswith anmcp_server— but if assets ever attach to a non-repo scope, the mcp_server kind would be stamped resolved yet never loaded. Worth a one-line guard or comment in PR 3 territory.
Merge order remains #664 → #665 (retarget to main after #664 lands).
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Seven of the eight prior findings are genuinely fixed and locked by regression tests — good, disciplined work. The one P1-security blocker (B4, .mcp.json exfiltration into git) is PARTIALLY FIXED: the symptom on the exact path I reproduced is now blocked, but the capability survives on three concrete paths I reproduced against the real apply_mcp_assets code on this head commit (0b06ff5). The skip-worktree guard is a per-path index flag, and the exfiltration channel is "any tracked path the loader's write reaches, plus any state in which update-index fails" — the fix covers one path in the happy state.
update-index --skip-worktree also introduces a new silent-data-loss failure mode that is unrelated to security (blocking #3 below).
To be explicit about what does not discharge the standing change request: two APPROVEs and fully green CI. No CI check on this repo exercises the symlink or index.lock paths, and TestMcpJsonNotCommittable only covers the two states the fix was written for.
Vision alignment
Aligned with bounded blast radius and the fire-and-forget default: fail-closed resolve, mandatory pins, audit triple on the TaskRecord, ADR-016-style mechanical enforcement rather than prompt-level convention. The re-strip of Linear MCP entries after the registry merge (agent/src/pipeline.py:1172-1181) is exactly the right instinct — a registry-published Linear server would otherwise reintroduce what ADR-016 forbids, and I verified the rewrite does not shake the skip-worktree flag off.
Where it diverges from the tenets: the resolved runtime is still written to disk in cleartext inside a tree the model can Read under bypassPermissions. That is a confidentiality boundary the PR chose not to close, and the durable fix (${ENV_VAR}-placeholder validation at publish) was deferred to a follow-up that has no filed issue. See Documentation.
Status of prior blocking findings
| # | Finding (file:line then) | Status | Evidence |
|---|---|---|---|
| 1 | Wrong-kind ref accepted under a typed assets.* field (blueprint.ts:240) |
FIXED | RegistryRefValidation now takes expectedKind and rejects a kind mismatch at synth — cdk/src/constructs/blueprint.ts:539-561, wired at :261-263. Locked by the per-field mismatch tests in cdk/test/constructs/blueprint.test.ts. |
| 2 | Empty asset arrays never REMOVE stale DDB refs (blueprint.ts:397) |
FIXED | emptyAssetFields() / buildRemoveClause() / buildRemoveNames() — cdk/src/constructs/blueprint.ts:449-467, spliced into onUpdate at :336-341. Populated→empty transition tested. |
| 3 | Non-injective MCP server key collapsed foo-bar/foo_bar (loader.py:35) |
FIXED | Hyphen normalization removed; _server_key returns raw f"{namespace}__{name}" — agent/src/registry/loader.py:36-45, with the rationale in-comment. Collision test present. |
| 4 | Loader fail-open — result discarded, caller ignored completion (loader.py:148) |
FIXED | RegistryAssetLoadError raised on missing repo_dir (loader.py:184-188), empty/non-dict runtime (:200-203), structurally invalid config (_to_mcp_config, :66-81), write error (:212-213), and blank skill fragment (:244-248); propagated by pipeline.py:1165-1168. Locked end-to-end by agent/tests/test_pipeline.py:168 test_malformed_registry_asset_fails_the_task_closed, which asserts the agent never runs. |
| 5 | Registry Cedar bytes bypassed the 64 KiB cap (orchestrator.ts:852) |
FIXED | The cap now counts extra_policies — agent/src/policy.py:882-901. Verified the counted text is the raw operator bytes (pre-synthetic-wrapper), which is the right denominator. Locked by agent/tests/test_policy_three_outcome.py:718. |
| 6 | Deprecation warnings not durable (orchestrator.ts:543) |
FIXED | warnings persisted on the resolved_assets triple and a durable registry_asset_warning TaskEvent emitted per warned asset — cdk/src/handlers/shared/orchestrator.ts:797-838. |
| 7 | .mcp.json written with transport the SDK does not consume (loader.py:83) |
FIXED | _to_mcp_config maps transport → type and validates shape — agent/src/registry/loader.py:48-86. I confirmed against the installed SDK (claude_agent_sdk/types.py:611-636): McpSSEServerConfig / McpHttpServerConfig are discriminated on type. Tests assert a consumable config. |
| 8 (B4) | Unredacted runtime written into the live git tree, exfiltrated by ensure_committed (loader.py:210) |
PARTIALLY FIXED | Symptom blocked on the reproduced path (regular tracked/untracked file, update-index succeeding). Capability retained on three paths — reproduced below against the real loader. |
| nit | Stale "warn + skip" docstring (loader.py:109) |
FIXED | agent/src/registry/loader.py:108-115 now enumerates the fail-closed set and names _read_existing_mcp_config as the sole warn-and-continue case; the stale pipeline.py comment corrected at :1158-1163. All three now agree. |
| nit | Dead if not written: return [] (loader.py:158) |
FIXED | Removed. |
| nit | Registry grant wildcarded the registry id (task-orchestrator.ts:489) |
FIXED — and genuinely narrower, not widened | resourceName: props.agentRegistryId + `${props.agentRegistryId}/record/*` — cdk/src/constructs/task-orchestrator.ts:486-500; matches registry-api.ts:111-122. Actions unchanged (GetRegistryRecord, ListRegistryRecords — read-only, no Create*/Update*/Delete* crept in), agentRegistryId threaded from agent.ts:917, nag reason trimmed to match reality. Locked by cdk/test/constructs/task-orchestrator.test.ts:457. I checked specifically for a widened grant: there is none. |
I also confirmed @theagenticguy's undocumented property holds: skip-worktree survives git checkout -b, so reconcile_agent_branch's branch -f + checkout does not shake the flag off. Worth a one-line comment.
Blocking issues
1. B4 capability survives: a symlinked .mcp.json puts the secret in git under a different tracked path
agent/src/registry/loader.py:190 computes mcp_path = os.path.join(repo_dir, ".mcp.json") and :209 opens it "w". Python's open(..., "w") follows symlinks. _protect_mcp_json_from_commit then flags the literal pathspec .mcp.json — which is the symlink's index entry, not the entry for the file the bytes actually landed in.
Reproduced with the real loader on this head commit (untrusted repo ships .mcp.json -> config/mcp.json, both tracked — a normal shared-config layout):
$ apply_mcp_assets(repo, [secret_asset])
TASK Registry: merged 1 MCP server(s) into <repo>/.mcp.json
$ git ls-files -v
S .mcp.json <-- guard applied to the symlink
H config/mcp.json <-- secret actually lives here, UNGUARDED
$ git status --porcelain -> M config/mcp.json
$ git add -u # == post_hooks.ensure_committed:286
$ git diff --cached | grep -c 'sk-live-abc123\|SUPERSECRET' -> 2
$ git commit -m "chore(agent): save uncommitted work from session end"
config/mcp.json | 12 +++++++++++-
>>> SECRET IN GIT HISTORY: True
ensure_pushed (agent/src/post_hooks.py:418-436) then pushes it. Same chain as my original report, identical outcome — only the pathspec moved. The repo layout is attacker-controlled input (the agent clones untrusted repos, including a PR branch at agent/src/repo.py:299), so this is not a hypothetical.
Worse, the same symlink-follow makes the loader an arbitrary-file-write primitive. I verified .mcp.json -> .github/workflows/ci.yml gets the JSON blob written over the workflow (and add -u stages it), and .mcp.json -> .git/config corrupts the repo config so every later git call fails fatal: bad config line 1 in file .git/config.
Suggested fix — before writing, refuse a non-regular target, then resolve and guard the real path:
if os.path.islink(mcp_path):
raise RegistryAssetLoadError(
f"refusing to write resolved MCP config through a symlink: {mcp_path}"
)(Cheapest and strictest — .mcp.json is a config file the loader owns; a symlink there has no legitimate use. os.open(..., O_NOFOLLOW) is the race-free variant if you prefer.) Regression test: symlink .mcp.json at a second tracked file, apply_mcp_assets, assert it raises and git add -u && git diff --cached is empty.
2. B4 capability survives: the guard fails open on update-index failure, in states the agent reaches routinely
agent/src/registry/loader.py:150-158 logs WARN and continues when update-index --skip-worktree fails. The secret is already on disk at that point, so a failed guard means the full add -u → commit → push chain is live. Two states I reproduced with the real loader:
index.lockcontention (concurrent git — prek hooks, a parallel agent tool call, a crashed prior git):WARN Registry: could not skip-worktree … (exit 128): fatal: Unable to create '…/.git/index.lock': File exists. TASK Registry: merged 1 MCP server(s) into … # lock released $ git add -u -> SECRET STAGED: True- Unmerged index entry for
.mcp.json(a conflicted predecessor merge —repo.py:656does exactly this formerge_branches;--abortat:667is the clean path but the window exists):WARN Registry: could not skip-worktree … (exit 128): fatal: Unable to mark file .mcp.json $ git add -u -> SECRET STAGED: True
@theagenticguy raised this shape as non-blocking; I'm escalating it, because I can now name two reachable triggers rather than a hypothetical. Notably, in both cases the WARN is followed by a cheerful TASK Registry: merged 1 MCP server(s) success line — an operator reading logs sees success.
Suggested fix — fail closed on confidentiality once the bytes are on disk. Verify the flag rather than trusting the exit code, and treat a still-stageable file as fatal:
verify = _git("ls-files", "-v", "--", ".mcp.json").stdout
if not verify.startswith(("S", "h")):
raise RegistryAssetLoadError(
f"could not make {mcp_path} unstageable; refusing to run with a "
f"secret-bearing config in a committable tree"
)Checking the observed flag rather than the exit code also covers the symlink case in #1 for free. Regression test: monkeypatch subprocess.run to fail, assert the raise (the existing TestMcpJsonNotCommittable scaffolding makes this cheap).
3. NEW — the guard silently discards the agent's legitimate .mcp.json work, and lies to it about why
skip-worktree makes git ignore all worktree changes to the path, not just the loader's. Once a registry mcp_server asset is pinned, any task whose actual job touches .mcp.json (a very plausible task on this codebase — channel_mcp.py writes it, strip_linear_mcp_servers edits it) has its work silently dropped. Reproduced with the real loader:
# task: "register our new internal MCP server in .mcp.json"
agent: git add .mcp.json -> exit 1
"The following paths … matched paths that exist outside of your
sparse-checkout definition, so will not be updated in the index: .mcp.json"
agent: git commit -am … -> exit 1 ("nothing to commit, working tree clean")
git status --porcelain -> '' (edit is invisible)
newLegit reached HEAD -> False
Two harms: (a) silent loss of the agent's output while the PR reports success — the same class of failure ensure_committed exists to prevent; (b) the error text names sparse-checkout, which is not what happened, so a model will burn turns chasing a nonexistent sparse config, plausibly tripping stuck_guard.
This is the structural cost of masking a confidentiality problem with a VCS flag. It goes away under the option-2 fix (${ENV_VAR} placeholders at publish, so nothing secret is ever on disk) — which is why I'd rather see that than more hardening on the flag. If you keep the flag, at minimum log a TASK-level line naming .mcp.json as platform-managed-and-uncommittable for this task, so the trajectory records the cause.
Non-blocking suggestions / nits
- Still cleartext on disk, readable by the model. Independent of git: the resolved
headers/url-token/api_keysit in<repo_dir>/.mcp.json, and the SDK runsbypassPermissionswithsetting_sources=["project"](agent/src/runner.py:435,:547), soRead .mcp.jsonsucceeds and the value can reach a PR comment.output_scanneris a partial net at best — I checked the patterns (agent/src/output_scanner.py:33-72):BEARER_TOKENneeds ≥20 token chars (a 14-charsk-live-abc123slips through),CONNECTION_STRINGneedsuser:pass@so?token=…slips through, and bareapi_keyhas no pattern. The SDK's programmaticmcp_serversoption (ClaudeAgentOptions.mcp_servers: dict[str, McpServerConfig], already used for the clarification server atrunner.py:525-553) would let the registry runtime be passed in-process, never touching disk — that closes #1, #2, and #3 at once and is the fix I'd actually build. Worth costing before adding more flag hardening. _protect_mcp_json_from_committakes bothrepo_dirandmcp_path, but only usesmcp_pathfor log text and hardcodes the pathspec".mcp.json". Deriving the pathspec frommcp_path(os.path.relpath) would keep the two in step if the filename is ever parameterized.- Naming: the docstring at
:131-133says "Best-effort: git plumbing failures here are logged, not fatal" — accurate today, but it is the sentence blocking issue #2 asks you to change. If you adopt the fail-closed variant, this docstring andapply_mcp_assets's fail-closed list (:167-173) both need the new condition added, or you reintroduce exactly the three-way docstring disagreement that nit #9 just fixed. agent/src/stacks/agent.ts:194-208— theforkBlueprintRepohook hardcodes threeacme/*refs that are documented as not seeded, so enabling it fails every task closed until an operator publishes them. Correct fail-closed behavior and the docs say so; consider making the ref list contextable so the hook is usable without editing the stack.- No cap on the number of asset refs a Blueprint may pin.
resolveRegistryAssets(cdk/src/handlers/shared/orchestrator.ts:527-547) resolves them serially with no bound, so N refs = N sequential AgentCore round-trips inside the orchestrator's timeout. The Cedar-bytes cap is enforced, the ref-count one isn't. Low risk (operator-supplied), but a bounded loop is cheap.
Documentation
docs/design/REGISTRY.md§12.1 documents theforkBlueprintRepoE2E hook, and the Starlight mirror (docs/src/content/docs/architecture/Registry.md) is regenerated in the same PR with byte-identical content — I diffed both hunks. Mirror-sync is clean; no hand-edit of generated content.- Missing: the security posture of
.mcp.jsonunder registry assets is undocumented. Once a mechanism exists that makes a repo file platform-managed and uncommittable, that belongs inREGISTRY.md(§ near the loader) and is arguably an ADR-016 sibling — it is the second instance of "enforce mechanically in.mcp.jsonbecause a prompt is not a boundary." A future contributor removing theupdate-indexcall to fix issue #3 above has nothing telling them it is load-bearing. - Missing: the deferred option-2 work (
${ENV_VAR}-placeholder validation inregistry-publish.ts::validateRuntime) is promised in a PR comment only. Per Stage 4 that needs a filed issue with a priority label — I searched and found none. Please file it (P1,security+registry) and link it from the_protect_mcp_json_from_commitdocstring, so the reason this guard is a stopgap is discoverable from the code.
Tests & CI
- CI fully green on
0b06ff5(build/agentcore, secrets+deps+workflow scan, PR-title lint, advisory dead-code). I ranuv run pytest tests/test_registry_loader.pyin a scratch worktree: 27 passed. - Regression coverage for findings 1-7 and the nits is real and asserts behavior, not implementation —
test_malformed_registry_asset_fails_the_task_closed(agent/tests/test_pipeline.py:168) drivingpipeline.run_taskand asserting the agent never runs is the right altitude, andtest_registry_extra_policies_counted_in_64kb_capuses registry-shaped text rather than a synthetic blob. TestMcpJsonNotCommittable(agent/tests/test_registry_loader.py:288-348) is well-constructed but tests only the two states the fix was designed for. Neither the symlink case, theupdate-index-failure case, nor the legitimate-edit case is covered — which is exactly why green CI here is not evidence the capability is gone. Adding the three tests named in blocking issues #1-#3 is the ask.- Bootstrap policy coverage: not applicable. No new CFN resource type.
agent.ts:917threads an existing registry id into an existing construct, and the orchestrator change is an IAM statement narrowing on an existing Lambda role —bedrock-agentcoreregistry actions came in with #664.BOOTSTRAP_VERSION/artifacts/DEPLOYMENT_ROLES.mdcorrectly untouched. - CDK test performance:
createStackintask-orchestrator.test.tswas already per-test; the new test reuses it and adds no bundling. Noaws:cdk:bundling-stacksre-enable. Clean per #366. - #319 solution-UA: clean. No naked client anywhere in the diff (grepped
new *Client(/boto3.client(/boto3.resource(across all 21 files — zero hits). The registry client is built through the attributed factory:makeRegistryClient()→AgentCoreRegistryClient→makeClient(BedrockAgentCoreControlClient)(cdk/src/handlers/shared/registry/agentcore-client.ts:202). - Merge order: this PR is stacked on
feat/246-registry-catalog(#664), which has a standing change request. #664 must land first, and the shared code paths (registry-publish.ts::validateRuntime,registry-resolve.tsredaction) are where the durable fix for B4 lives. Please don't let both merge with the on-disk-cleartext posture unresolved in either.
Review agents run
I could not dispatch the pr-review-toolkit agents from this context (a workflow subagent cannot nest a further agent dispatch), so per the review_pr Stage-3 escape hatch I applied each agent's rubric by hand. Naming them explicitly so the gap is auditable:
- code-reviewer (guidelines/style/routing) — applied. Routing correct per the AGENTS.md table: agent runtime in
agent/src/, orchestrator/constructs incdk/src/, docs + regenerated mirror indocs/. No shared-API-shape change, so nocli/src/types.tssync obligation. Cedar engine pins untouched (nocedarpy/cedar-wasmlockstep obligation). No hardcoded ARNs/account ids — both registry ARNs go throughStack.of(this).formatArn. - silent-failure-hunter — applied, and it is where the surviving blockers came from. Two fail-open catches:
loader.py:151-156(non-zero exit → WARN + continue) and:157-158(except (OSError, SubprocessError)→ WARN + continue), both after the secret is already on disk, both followed by a success log line. The_read_existing_mcp_configwarn-and-continue at:103-105is correctly benign (the file is replaced). - type-design-analyzer — applied. New types are proportionate.
RegistryAssetLoadError(RuntimeError)is a single well-scoped error class, not a hierarchy.assets?: {mcpServers?, cedarPolicyModules?, skills?}onBlueprintProps(blueprint.ts:182-190) mirrors the existingsecurity/networking/pipelinegrouping. One weakness worth naming:resolved_assets: list[dict[str, Any]](agent/src/models.py:231-235) keeps the runtime bundle untyped end-to-end, so every consumer re-validates shape ad hoc (isinstance(runtime, dict)atloader.py:200,:243). A Pydantic model would move that to the boundary — non-blocking, but it is the reason the loader needs three defensiveisinstancechecks. - comment-analyzer — applied. The prior stale-docstring nit is genuinely fixed and all three sites now agree. New comments are accurate and explain why (the
_server_keynon-normalization rationale at:39-44and the cedar-drop-widens-permissions note atorchestrator.ts:836-840are both the good kind). One comment will become inaccurate if you take fix #2::131-133's "Best-effort … not fatal" (see nit 3). - pr-test-analyzer — applied. See Tests & CI: coverage for findings 1-7 is genuinely locking; B4's coverage is scoped to the states the fix anticipated, which is the AI005 failure mode (tests assert what the code does, not what it should — "cannot be exfiltrated" vs. "this one pathspec is flagged").
- security-review skill — applied by hand over the IAM + secrets surface: registry grant scoping verified narrowed not widened (read-only actions, registry-scoped ARNs,
record/*suffix justified); Cedar aggregate cap verified closed on theextra_policiespath; secrets handling is the surviving blocker set above. - Omitted: none in scope. No Cedar grammar/parity-fixture change, no network/VPC change, no input-gateway/guardrail change in this diff.
Human heuristics
- Proportionality — Concern.
_protect_mcp_json_from_commit(loader.py:118-158) spends 40 lines and a git-subprocess dependency inside the asset loader to stop a secret from being committed, when the root cause is that a secret is written to disk at all. The SDK's in-processmcp_serversoption (runner.py:525-553, already in use) is strictly less machinery and closes all three blockers. This is a mask sized like a mechanism. - Coherence — Pass. Terminology is consistent (
kind/namespace/name/version/runtimeend-to-end); registry cedar deliberately reuses the existingcedar_policiespayload field so the parity contract holds by construction (orchestrator.ts:831-834); IAM scoping now matches its sibling construct instead of diverging. The#246 reviewbreadcrumbs make the fix history legible. - Clarity — Concern. Names are good, but two error surfaces mislead: the guard logs
WARN … could not skip-worktreeimmediately followed byTASK Registry: merged 1 MCP server(s)(reads as success), and git reports the blocking-issue-#3 case as a sparse-checkout problem, which it isn't. - Appropriateness — Concern. AI001: the
skip-worktreebehavior was verified against real git (good — I re-verified it independently and the tracked-case claim holds), but only in the states the fix anticipated. The adversarial question "what is the next input that defeats this?" has three answers, two of which need no attacker at all — a symlink in an untrusted repo, and a concurrentindex.lock.
c8e4790 to
e3b2cae
Compare
0b06ff5 to
3aa1fe8
Compare
The base branch was changed.
Builds on the catalog PR to actually 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 the `{kind,id,version}` triples on the TaskRecord for
audit, merges resolved cedar_text into `cedar_policies`, and 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 merges into `.mcp.json`;
cedar_policy_module flows through PolicyEngine's unannotated `extra_policies`;
skill prompt fragments append to the system prompt (build_skill_prompt_fragment).
- TaskOrchestrator IAM: read-only bedrock-agentcore registry access so the
orchestrator can resolve refs.
Depends on the catalog PR (feat/246-registry-catalog): imports the RegistryClient
port, ref grammar, and resolver from that branch.
…l-closed load, cap (#246) Blueprint / orchestrator: - Validate each typed Blueprint field's ref kind at synth (reject a skill ref under assets.mcpServers, etc.) so a field typo can't silently activate a different asset class. - REMOVE asset columns that go empty on update, so a redeploy that cleared the last mcp_server/cedar_policy_module/skill actually detaches the stale refs. - Persist deprecation warnings: stamp them on resolved_assets and emit a durable registry_asset_warning TaskEvent (was a Lambda log only). Agent loader: - Use an injective MCP server key (drop hyphen->underscore collapse) so acme/foo-bar and acme/foo_bar don't clobber each other. - Normalize the MCP runtime transport -> the SDK's discriminant type key when writing .mcp.json, so a published server the docs describe is actually loaded. - Option C fail-closed: raise RegistryAssetLoadError on infrastructure failures (missing repo_dir, .mcp.json write error) so the task fails rather than running with a pinned-but-absent asset; warn+skip degraded-but-safe cases; return the loaded keys. Policy: - Count registry cedar_policy_module bytes (legacy extra_policies path) in the 64 KiB aggregate cap so a large registry policy can't bypass the bound. Docs: document the opt-in forkBlueprintRepo E2E hook in REGISTRY.md (+ mirror).
…or IAM (#246) Third review pass (@scottschreckengaust) on the integration PR: - B4 (P1 security): the resolved runtime may carry secrets (bearer headers, url tokens, --api-key args) and .mcp.json lives in the live git clone, so the post-hook `git add -u` → commit → push could exfiltrate it to the PR when the target repo tracks .mcp.json. apply_mcp_assets now marks the file skip-worktree (intent-to-add first if untracked), blocking both `git add -u` and an explicit add. Regression tests assert a secret-bearing asset cannot be staged (tracked + untracked cases). - nit: scope the orchestrator's registry read grant to registry/{agentRegistryId} (+ record/* suffix) instead of registry/* — mirrors registry-api.ts; nag reason trimmed; test asserts no bare "*"/registry-id wildcard. - nits: align the RegistryAssetLoadError docstring + pipeline comment with the fail-closed behavior (no more stale "warn+skip"); drop the now-dead `if not written` branch.
3aa1fe8 to
ee4011f
Compare
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Conditional approval with followup items in GitHub
…mples#246) (aws-samples#548) * docs(decisions): ADR-018 propose central agent asset registry (aws-samples#246) Draft ADR proposing a versioned, immutable-per-version asset registry for MCP servers, Cedar policy modules, and skills. Fixes the contract (semver grammar, immutability, resolve-at-create-task, descriptor validation, governance workflow) and defers substrate selection to the design PR. Ranks AWS Agent Registry as preferred with DynamoDB+S3 as fallback; surveys mcp-gateway-registry, agentregistry.ai, and Entra Agent Registry+AGT as considered alternatives. Status: proposed. Requires the aws-samples#246 `approved` label before opening a follow-up implementation PR (per ADR-003). Includes regenerated Starlight mirror. * docs(decisions): reconcile ADR-018 with shipped AgentCore implementation (aws-samples#246) Addresses review on aws-samples#548 (@scottschreckengaust, @isadeks) and records the resolved substrate decision. Refinements (not a reversal — the ranking, alternatives, and flip-conditions are retained as the decision record): - Status proposed → accepted; substrate resolved to AWS Agent Registry (Bedrock AgentCore), built behind the RegistryClient seam and proven E2E. - Resolution owner: TypeScript orchestrator owns catalog/semver resolution; Python loads the already-resolved bundle (mirrored resolver only for the parity contract + direct lookups). Keeps sub-decision 6 true. - Governance: separated substrate-provided lifecycle machinery from the thin ABCA MVP surface (publish + auto_approve + resolve/list/show); standalone promote/reject/deprecate, env-gated auto-approve, and event consumption named as future scope (aws-samples#478/aws-samples#230). - Descriptor validation: MVP delegates to native descriptor types (MCP server.json, skill frontmatter, CUSTOM); shared JSON Schema capability descriptor is future scope (aws-samples#481). - Grammar: reframed "already committed" → "extends the committed shape"; authoritative strict grammar in registry/ref.{ts,py}. - Accuracy: single canonical `approved` token; aws-samples#381-split ref 12 → 13; reference URLs normalized (cloud.google.com, learn.microsoft.com). - Added dated Changelog per docs/decisions/README refinement rules. Starlight mirror regenerated via docs sync (not hand-edited). * docs(decisions): de-link not-yet-merged ref.{ts,py} paths to fix link-check (aws-samples#246) The ADR cited cdk/src/handlers/shared/registry/ref.ts and agent/src/registry/ref.py as relative-path links, but those files ship in the implementation PRs (aws-samples#664/aws-samples#665), not on this ADR branch or main — so //docs:link-check failed with 2 dead links. Demoted both to inline code spans (with a note that they land with aws-samples#664/aws-samples#665) until the implementation merges. Mirror regenerated via docs sync. * docs(decisions): keep ADR-018 proposed + address review (vocab, federation, cutover) (aws-samples#246) Addresses review feedback from @krokoko, @scottschreckengaust, @isadeks: - Revert premature proposed→accepted (README rule: accepted on impl-PR merge; aws-samples#664/aws-samples#665 still in review). Soften "shipped / proven E2E on a live stack" to "targeted by aws-samples#664/aws-samples#665, exercised on a dev stack during review"; stop citing the parked DDB+S3 PRs (aws-samples#632-aws-samples#634) as current. Add a Status note in Decision. - Add short-vs-long-form kind-vocabulary migration note to sub-decision 1: WORKFLOWS.md short forms (registry://mcp/…) are lenient-only forward-decls; only the long form (mcp_server/ns/name@constraint) resolves. No auto-aliasing. - Add a federation / "registry of registries" Non-goal (answers Scott's Jul-8 question): single operator-curated catalog; external registries are discovery-only; no federation in aws-samples#246. - Promote the 2026-08-06 AgentCore namespace cutover from a cost input to a hard gate: no production dependency until the migration is GA in-region. Mirror regenerated via docs sync (idempotent). * docs(decisions): renumber registry ADR 018 -> 022 to avoid collision (aws-samples#246) ADR-018 is already taken on main (ADR-018-linear-agent-session-interaction), and 019/020/021 are claimed by open PR aws-samples#663 and merged main ADRs. 022 is the next unclaimed number. Renames the source + Starlight mirror and updates the H1 titles; numbers are never reused (docs/decisions/README.md). * docs(decisions): add read-path + descriptor-integrity invariants to ADR-022 (aws-samples#246) Second review pass (@scottschreckengaust): - Sub-decision 11 + the substrate-invariant list gain read-path confidentiality: runtime payloads reference credentials (never inline), and open read surfaces redact by allowlist, not denylist. - Sub-decision 7 requires the validated descriptor be carried isolated from caller-controlled discovery prose (non-bypassable validation), incl. CUSTOM. - Collapse the residual submitted/PENDING_APPROVAL dual token; bump Last-updated; add the 018->022 renumber changelog entry; mark previously-pending items landed. --------- Co-authored-by: bgagent <bgagent@noreply.github.com>
5 participants
Summary
Builds on the catalog PR (#664) to consume registry assets at task time.
resolveRegistryAssets): resolves a blueprint'sregistry://mcp_server/cedar_policy_module/skill refs at task start, fail-closed; stamps{kind,id,version}triples on the TaskRecord, merges resolved cedar_text intocedar_policies, threads the runtime bundle into the agent payload.assets.{mcpServers,cedarPolicyModules,skills}withRegistryRefValidation; the three onUpdate helpers now write the asset-ref columns so redeploying an onboarded repo no longer drops them.registry.loader): mcp_server →.mcp.json; cedar_policy_module → PolicyEngine unannotatedextra_policies; skill prompt fragments → system prompt.Test plan
mise run buildgreen--context forkBlueprintRepo=owner/repo; submit a task that pins all three asset kinds.mcp.jsonis not committed (last verified: task01KZVFVGP89077B385ZQDXGCZ4→ PR chore(project): update structure #7)Local end-to-end test
Verified against a fresh deploy of this branch (rebased on
main@d981c955). The E2E exercises all three MVP asset kinds in a single task.The three pinned assets
The ForkBlueprint (
cdk/src/stacks/agent.ts, opt-in via--context forkBlueprintRepo=) pins one of each kind at^1.0.0:mcp_serverregistry://mcp_server/acme/aws-knowledge@^1.0.0{transport: http, url: https://knowledge-mcp.global.api.aws/mcp}.mcp.json; the server is available to the agent (and, per the secret-containment fix,.mcp.jsonis not committed to the PR)cedar_policy_moduleregistry://cedar_policy_module/acme/guard@^1.0.0forbid (principal, action == Agent::Action::"invoke_tool", resource == Agent::Tool::"WebSearch");cedar_policies; theWebSearchtool is denied at the PolicyEngineskillregistry://skill/acme/readme-helper@^1.0.0tool_hints: [Edit, Write])ABCA-REVIEWEDmarkerSample payloads live in
spike/assets/(discovery.json/runtime.json,cedar-*.json,skill-*.json).Steps
1. Bootstrap + deploy (bundle is
v1.5.0; re-bootstrap so the CFN role has the registry IAM):2. Configure the CLI + authenticate:
3. Join the registry Cognito groups (publish needs
RegistryPublisher;--auto-approveneedsRegistryApprover), then re-login for fresh claims:4. Publish the three assets (APPROVED):
(cedar/skill use
--custom— verbatim CUSTOM storage; mcp_server uses the nativeMCPdescriptor.)5. Sanity-check resolution (fail-closed):
6. Submit a task against the onboarded fork repo (onboarded by the ForkBlueprint at deploy — no
repo onboardneeded):Expected result (last verified: task
01KZVFVGP89077B385ZQDXGCZ4-> PR #7)status: COMPLETED;resolved_assetslists all three{kind, id, version:"1.0.0"}triples.ABCA-REVIEWEDmarker (skill loaded)..mcp.json— the resolved MCP config is loaded for the run but kept out of the commit (secret-containment; skip-worktree guard).resolved_assetsis stamped on the TaskRecord for audit.