docs(decisions): ADR-022 propose central agent asset registry (#246) - #548
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
=======================================
Coverage ? 91.93%
=======================================
Files ? 298
Lines ? 83853
Branches ? 9022
=======================================
Hits ? 77090
Misses ? 6763
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
How will the lookup "Registry of registries" be maintained? For example:
Is there an abstraction from the Cloud Computing Native Foundation (CCNF) that can be utilized versus rolling a bespoke system? |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
First off — this is genuinely one of the most thorough, well-reasoned ADRs I've read in this repo, and it was a pleasure to review. 🙌 You did the hard things right: it's template-compliant, it's honest about its own risks (the (!) section is excellent), and it correctly reaches for the repo's twice-learned parity lesson instead of re-discovering it. The contracts/registry-resolution/ proposal mirroring contracts/cedar-parity/ and contracts/workflow-validation/ is exactly the instinct we want to see — I verified both of those directories exist and this slots right in beside them. The four substrate flip-conditions turn "preferred with a fallback" into something falsifiable rather than hand-wavy, and resolving at the create-task boundary keeps you consistent with ADR-014's shipped workflow_ref model. Really strong work, and the effort clearly shows.
Everything below is in service of making an already-good document airtight before it graduates from proposed to accepted — I'm leaving this as a comment, not a change request, since it's a draft awaiting the approved label on #246. Please read the tone as "help me help this land," not "stop." 💛
Two things I'd fix before acceptance (both quick)
1. The "grammar already committed" claim doesn't hold against the shipped regex (inline on the sub-decision). I ran the ADR's own example refs against _REGISTRY_REF at validator.py:50, and the committed regex has no @ in its character class (so @<constraint> can't be parsed) and only allows hyphens in the kind segment (so mcp_server / cedar_policy_module / prompt_fragment don't match — only mcp-server does). This is totally fixable and doesn't weaken the design — it just means the framing "closing the loop rather than opening it" slightly overstates what's shipped. A one-line reframe ("this ADR extends the committed shape") plus a note on where the regex change lands fully resolves it. Details inline.
2. Small off-by-one in the References section (inline). It credits "sub-decision 12" for the #381 split, but 12 is the workflows-don't-migrate decision — the #381 split is 13 (your PR body has it right!). Tiny, but the test-plan box "sub-decisions referenced by number are internally consistent (1–13, no gaps)" is checked, so worth tidying so that claim stays true. Appears in both the source file and the generated mirror, so the fix lands in docs/decisions/ and re-syncs.
A few design-level thoughts (take or leave — you're the author)
These aren't blockers, just things I'd love your read on:
- Altitude. The doc is part ADR, part design spec, part market survey — and it's great diligence — but the ~15-vendor enumeration and the pinned implementation details (error-code strings, Cognito group names) are the most perishable content in an artifact whose whole job is to be durable. Totally your call, but you might keep the invariants + preferred/fallback ranking + flip-conditions here, and let the vendor deep-dive live in the design PR or a dated appendix. The decision survives; the survey ages.
- Governance-in-MVP sizing (inline). Sub-decision 10 is a strong principle ("governance is what separates a registry from a directory" — love it), but a full lifecycle + audit + event integration + auto-approve, on top of a two-language semver resolver and descriptor validation, is a big MVP for a reference project whose AC4 goal is one E2E kind. Might be worth an explicit sentence acknowledging governance may need staging if the estimate tightens.
- AgentCore timing. The ADR is dated 2026-07-08 and the preferred substrate hard-migrates namespaces on 2026-08-06 — likely mid-build. You flag it as a risk and lean on the
RegistryClientseam, which is right for code, but the seam doesn't confine data migration / IAM action renames as cleanly as the prose implies. Maybe promote "don't take a production dependency until post-cutover GA" from a risk bullet into the decision framework. - One status, two names (inline).
approved"(also calledactive)" then both get used interchangeably — in a byte-for-byte two-language contract that's a latent divergence, and it's exactly the parity hazard your own(−)bullet warns about. Pick one canonical token.
Nits
- Two reference URLs look off by host convention (didn't fetch):
www.docs.microsoft.com/...(usuallylearn.microsoft.com) anddocs.cloud.google.com/...(usuallycloud.google.com/...). Worth a spot-check since the test plan mentions link verification. - Reserving a
capabilitykind for workflows while sub-decision 12 says workflows won't migrate reads slightly oddly — fine as "declared but not loaded," just a one-clause note would smooth it.
Thank you for the care you put into this — the bones are excellent and none of the above is hard to address. Happy to pair on the grammar reconciliation (#1) if useful, since that's the one place code and contract need to meet. Excited to see this one land. 🚀
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict
Comment (not a change request). This is an exceptionally thorough, template-compliant ADR and the design is sound: contract-first, substrate-deferred with falsifiable flip-conditions, fail-closed resolution, and it correctly reaches for the repo's twice-learned parity lesson (contracts/registry-resolution/ mirroring the existing contracts/cedar-parity/ and contracts/workflow-validation/, both of which I verified exist). It should not merge yet only because it's a self-declared draft awaiting the approved label on #246 — governance-wise that gate is now satisfied per the review brief. Before it graduates proposed → accepted, two factual defects should be fixed (grammar-parity claim + a cross-reference off-by-one). I independently re-verified the prior maintainer review (@scottschreckengaust) against the current worktree at f3e8972; every claim it makes is still accurate — none are stale.
Vision alignment
Strong fit with VISION.md:
- Fire-and-forget default (tenet 1): assets resolve at the create-task boundary (sub-decision 5) — no live intervention introduced; the submitter path stays asynchronous.
- Bounded blast radius & cost (tenet 2): fail-closed on unresolvable refs (sub-decision 6), semver-only with
*/latest/>=rejected (sub-decision 2), immutability per version (sub-decision 3), governance lifecycle + coarse Cognito ACL (sub-decisions 10/11). Widening surface is documented, not silent. - Reviewable outcomes (tenet 8):
resolved_assetsstamped on every task record for audit + reproducibility; state transitions carry rich audit metadata. - Tenet trade-offs documented: the
(−)/(!)sections are exactly the explicit rationale the tenets require of an architectural decision. No undocumented tenet trade.
Blocking issues
None that block this draft PR. The two items below are fix-before-accepted correctness defects in the design-of-record (I'd re-raise them as blocking if this PR tried to move status to accepted without addressing them):
-
docs/decisions/ADR-018-agent-asset-registry.md:40/:134/:162— the "grammar already committed" claim is factually false against the shipped regex._REGISTRY_REFatagent/src/workflow/validator.py:50is^registry://[a-z][a-z0-9-]*/[a-z0-9][a-z0-9./-]*$. I ran the ADR's own example refs against it:registry://mcp_server/acme/foo@^1.4.1→ no match (no@in any char class; underscore not allowed in the kind segment)registry://cedar_policy_module/acme/foo→ no match (underscore)registry://prompt_fragment/acme/foo→ no match (underscore)- only
registry://mcp-server/acme/foo(hyphen, no constraint) matches.
So the ADR's MVP kinds (mcp_server,cedar_policy_module) and the@<constraint>grammar (line 34) are not parseable by the committed regex. Lines 134/162 ("the code already parses and validatesregistry://refs", "URI grammar already shipped") overstate what exists and would mislead #632+ implementers into thinking no parser change is needed. Fix: reframe as "this ADR extends the committed shape" and add a one-line note that the regex must widen (add@<constraint>, allow_in the kind segment) in the implementation PR. Risk if unfixed: the design-of-record is inaccurate about its own foundation.
-
docs/decisions/ADR-018-agent-asset-registry.md:152— off-by-one cross-reference. References credits "sub-decision 12" for the #381 split, but sub-decision 12 (line 62) is workflows-don't-migrate and the #381 split is sub-decision 13 (line 64; the PR body has it right). The test-plan box "sub-decisions referenced by number are internally consistent (1–13, no gaps)" is checked, so this contradicts a checked claim. One-character fix, lands in source + re-syncs to the mirror.
Non-blocking suggestions / nits
- Dual status token (
approved"also calledactive"). Lines 46/58/60 use both interchangeably. In a byte-for-byte two-language contract this is the exact parity hazard the ADR's own(−)bullet warns about — pick one canonical token now while it's cheap. - Reference host conventions.
docs.cloud.google.com/...(line 109) is normallycloud.google.com/...;www.docs.microsoft.com/...(line 171) is normallylearn.microsoft.com/.... Worth a spot-check since the test plan claims link verification. (Not fetched.) - Altitude / perishability. The ~15-vendor survey and pinned implementation strings (error codes, Cognito group names) are the most perishable content in an artifact whose job is durability. Consider keeping invariants + preferred/fallback + flip-conditions here and moving the vendor deep-dive to a dated appendix or the design PR. Author's call.
- Governance-in-MVP sizing. Sub-decision 10 (full lifecycle + audit + event integration + auto-approve) on top of a two-language semver resolver + descriptor validation is a large MVP for a reference project whose AC4 is one E2E kind. A sentence acknowledging governance may need staging if the estimate tightens would be honest.
- AgentCore 2026-08-06 rename. The
RegistryClientseam confines code but not data migration / IAM-action renames as cleanly as the prose implies. Consider promoting "no production dependency until post-cutover GA" from a risk bullet into the decision framework. - Reserved
capabilitykind vs sub-decision 12. Reservingcapability(= workflow) while sub-decision 12 says workflows won't migrate reads slightly oddly; a one-clause "declared but not loaded" note smooths it.
Documentation
- ADR follows the
docs/decisions/template (Status/Date/Context/Decision/Sub-decisions/Consequences/References), matching ADR-016/ADR-017 shape. ADR-018 number is free (no collision). Linked ADR-014 and ADR-003 files both exist. - Starlight mirror is correctly regenerated, not hand-edited.
docs/src/content/docs/decisions/Adr-018-agent-asset-registry.mdhas the generated frontmatter and site-rewritten internal links; after stripping frontmatter and normalizing link targets, the mirror body is identical to the source. CI "Fail build on mutation" will pass. This satisfies themise //docs:syncrequirement.
Tests & CI
- CI is green (Codecov note is a config warning about a missing BASE report, not a failure).
- No unit tests apply — the PR is strictly two docs files; no executable behavior added.
- Bootstrap synth-coverage: N/A for this PR. The review brief's CDK bootstrap-policy check (DynamoDB table, S3 bucket, new Lambda handlers,
BOOTSTRAP_VERSIONbump,resource-action-map.ts,DEPLOYMENT_ROLES.mdgolden baseline) applies to the implementation PRs (#632/#633/#634), not to this ADR. I confirmed the diff touches zero CDK/handler/CLI/agent code (git diff --name-only= the ADR source + its mirror only). No constructs changed here, so there is nothing to gate. When #632 lands, that PR MUST carry the full bootstrap bundle update — I'll enforce it there. - Shared-type sync: N/A.
cdk/src/handlers/shared/types.tsis untouched, so nocli/src/types.tssync is owed by this PR.
Review agents run
- Hand review by principal-architect judgment (primary): appropriate here because the diff is prose — the ADR itself is the reviewable artifact. I empirically validated the two load-bearing factual claims (regex parity via a Python harness against the actual
_REGISTRY_REF; sub-decision numbering) rather than trusting prose. - Omitted —
code-reviewer,silent-failure-hunter,type-design-analyzer,pr-test-analyzer: no executable code, no error-handling paths, no new types, no tests in the diff; nothing in their scope to analyze. - Omitted —
/security-review: no IAM, Cedar policy, network, secret, DynamoDB, S3, API-handler, or input-validation code in this diff. Those resources are proposed in the ADR and will be security-reviewed when their code lands in #632+. The security-relevant design assertions (fail-closed resolution, coarse-ACL trust decision, publish-endpoint exposure) are called out in the ADR's own(!)risks and I reviewed them by hand under Vision alignment. comment-analyzersensibility applied by hand: verified the ADR's cross-references and its claims about existing code/contracts dirs match reality — that's how I found blocking-issue #1 and #2.
Human heuristics
- Proportionality — concern (minor): governance-in-MVP + full vendor survey is heavy relative to AC4's one-E2E-kind goal (see nits). Not a blocker for an ADR; flagged for the design PR to right-size.
- Coherence — concern (minor): dual
approved/activestatus token (:46/:58/:60) risks the same-concept-two-terms hazard the ADR itself warns about; and the grammar claim (#1) conflicts with the shipped code. Both fixable. - Clarity — pass: names communicate intent; error/failure modes surface explicitly (fail-closed, specific reason codes); no defaults hiding failures.
- Appropriateness — pass: maintainable by this team; correctly slots beside existing parity-corpus mechanisms; integration claims mostly verified against real repo state (the one exception is blocking issue #1, where the claim was checked against the real regex and found overstated).
Stack / merge-order note
This ADR is the independent design-of-record (ADR-018) that the implementation PRs #632/#633/#634 build on. Recommended order: land this ADR first (after the two fixes and the approved label lands per the PR's own draft gate), so #632+ have a stable, accurate contract to cite. Critically, fix blocking issue #1 before #632 — otherwise #632's implementers may skip the required _REGISTRY_REF regex widening (add @<constraint>, allow _ in the kind segment) because the ADR told them the grammar was already shipped. The bootstrap-policy, shared-type-sync, and CDK synth-coverage gates all attach to #632, not here.
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: Request changes
I re-reviewed the current ADR head (f3e8972) against the composed implementation in #632-#634. The existing grammar and cross-reference findings remain valid; the points below are additional design-of-record gaps.
Blocking findings
-
[P1] Choose one resolution owner (
:48,:50,:54,:139). Sub-decision 5 says the TypeScript orchestrator resolves refs fully before dispatch, and sub-decision 6 says a running task never re-resolves. Sub-decision 8 and the consequences then require a PythonRegistryClientplus(ref, catalog) -> verdictfixtures against every implementation. Those are different architectures. The implementation exposes the contradiction: TypeScript is the only catalog/semver resolver; Python receives an already-resolved bundle and only checks URI syntax. There is no PythonRegistryClientand noresolve-*.jsoncorpus. Make the ADR explicit that TypeScript owns resolution and Python validates/loads the wire bundle, or define when Python resolves without violating sub-decisions 5/6. Scope the golden corpus to the implementations that actually own each semantic. -
[P1] Reconcile the required MVP governance with the tracked and shipped scope (
:58-60,:137). This ADR says transitions, rationale-bearing audit history, transition events, and dev-only auto-approval are MVP requirements. Issue #478 describes lifecycle as a follow-up, while the complete #632-#634 stack exposes only publish/resolve/list/show. A normalRegistryPublisherrecord therefore remainssubmittedforever, andauto_approve=trueis approver-gated but not environment-gated. Either make the transition workflow a merge gate for this stack, or explicitly defer it and remove the claims that governance ships in MVP. -
[P1] Make the descriptor contract enforceable (
:52,:140). The ADR requires tool surface, egress, Cedar actions, minimum compute, and permissions, with JSON Schema as the single source of truth. #632 implements ad hoc TypeScript checks only: several required fields are absent, arrays are not element-validated, and MCPserver_configis not checked against its declared transport/egress. No JSON Schema is present or consumed by both languages. Require that contract in the implementation before accepting this ADR, or narrow the ADR to the descriptor guarantees that actually ship.
Verification
The composed CDK registry suites pass, so these are contract and missing-case failures rather than existing test failures.
…ion (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).
Self-review (design-of-record pass)Ran a self-review after addressing the earlier review feedback (@scottschreckengaust, @isadeks), then a verification pass. Fixed (commit
@isadeks P1 items re-confirmed resolved against shipped code:
@scottschreckengaust — thanks for the review; each of your findings addressed:
Status moved
|
Independent review (post–Jul 27 reconcile)Thanks for the careful reconcile pass (@Kalindi-Dev) — and to @scottschreckengaust / @isadeks for the earlier reviews. I re-read head Bottom line: the contract is sound, and the Jul 27 edits honestly close Scott’s accuracy nits and isadeks’s three P1s (resolution owner, thin governance surface, native descriptors). I’d still hold merge for a short tightening pass + a human re-review dismissal. Prior findings — disposition
Suggested before merge
Merge order (suggestion)Corrected ADR (as Happy to clarify any of the above — none of this is meant to slow a strong design-of-record, just to keep it accurate the day it hits |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict
Approve (with two minor nits). At head e998a43 this ADR has flipped proposed → accepted and, in doing so, resolved every prior blocking finding from all three earlier reviews. I re-verified each claim against the current text and the shipped code rather than inheriting or dismissing the prior CHANGES_REQUESTED wholesale — the substrate pivot to AWS AgentCore (per #664/#665) genuinely moots the DDB+S3-based P1 findings. The document is now accurate about its own foundation, internally consistent (1–13, no gaps), and the Starlight mirror regenerates byte-identically. This is one of the most disciplined ADR revisions I've reviewed here.
Vision alignment
Strong fit with VISION.md:
- Fire-and-forget (tenet 1): assets resolve at the create-task boundary (sub-decision 5); no live intervention introduced. Sub-decision 5 now names the TypeScript orchestrator as the single resolution owner, keeping sub-decision 6 (no re-resolution) true.
- Bounded blast radius (tenet 2): fail-closed on unresolvable refs (sub-decision 6), semver-only rejecting
*/latest/>=(sub-decision 2), immutability per version (sub-decision 3), coarse Cognito ACL with documented Phase-3 tightening (sub-decision 11). Surface widening is documented, not silent. - Reviewable outcomes (tenet 8):
resolved_assetsstamped per task record for audit + reproducibility. - Tenet trades documented: the
(−)/(!)sections give the explicit rationale the tenets require. No undocumented trade.
Blocking issues
None. All five prior blocking findings are resolved at e998a43:
- Grammar-parity (prev. blocking, scottschreckengaust): RESOLVED. Sub-decision 1 (
:41) now reads "extends the shape pre-declared" and states outright the committed_REGISTRY_REF"admitted a 2-segmentregistry://kind/nameform but not the@<constraint>suffix or_(snake_case)". Consequences (:143) and References (:171) reframed to match. I independently re-ran the ADR's own example refs against the shipped regex^registry://[a-z][a-z0-9-]*/[a-z0-9][a-z0-9./-]*$atagent/src/workflow/validator.py:50:mcp_server/cedar_policy_module/@^1.4.1all fail, onlymcp-server(hyphen, no constraint) matches — exactly as the revised ADR now describes. The overstatement is gone. - Off-by-one #381 cross-reference (prev. blocking): RESOLVED. References (
:161) now credits sub-decision 13; Changelog (:191) records the fix. - [isadeks P1] Resolution-owner ambiguity vs sub-decisions 5/6: RESOLVED by the substrate pivot + rewrite. Sub-decision 5 (
:49) now explicitly: TypeScript owns catalog/semver resolution; Python receives the already-resolved bundle and loads it; Python's mirroredRegistryClientexists only for the parity contract and direct lookups. The architecture-vs-architecture contradiction isadeks flagged against #632–#634 no longer holds against this text. - [isadeks P1] MVP-governance vs shipped scope: RESOLVED. Sub-decision 10 (
:63–:67) now separates substrate-provided machinery from the thin ABCA surface actually shipped (publish/resolve/list/show), and names standalone promote/reject/deprecate, environment-gatedauto_approve, and event consumption as future scope tracked in #478/#230. The "submitted forever" and "auto_approve not env-gated" gaps isadeks raised are now stated as known deferrals, not implied guarantees. - [isadeks P1] Descriptor-contract enforceability / JSON-Schema single source: RESOLVED. Sub-decision 7 (
:55–:57) splits MVP (shipped) native/CUSTOMvalidation from the future shared-JSON-Schema capability descriptor, explicitly deferred to #481. The consequence bullet (:149) matches. The ADR no longer claims a single-source JSON Schema ships in MVP.
Non-blocking suggestions / nits
-
Accepted-status asserts implementation facts not visible on this branch. The ADR flips to
acceptedand asserts "shipped" / "proven E2E on a live stack" (:37,:55,:61,:65,:151,:185), butcdk/src/handlers/shared/registry/ref.ts,agent/src/registry/ref.py, andcontracts/registry-resolution/do not exist on this branch — they land in #664/#665. The ADR is honest about this (References:172explicitly caveats those paths as not-yet-on-main), which is exactly the discipline the original grammar claim lacked, so this is not blocking. But an ADR whose "shipped" claims are unverifiable until sibling PRs merge is mildly incoherent as a design-of-record. Consider either (a) merging this after/with #664/#665, or (b) a one-line header note thatacceptedis contingent on those PRs landing. Not a blocker: the claims are issue-tracked and caveated. -
Altitude / perishability (carried from prior review, author's call). The ~15-vendor survey and pinned strings (error codes, Cognito group names, the 2026-08-06 date) remain the most perishable content in a durability artifact. Retaining them as the "why AgentCore won" record is defensible; just flagging that this content will age fastest.
Documentation
- ADR follows the
docs/decisions/template; ADR-018 number is free; linked ADR-003/ADR-014 and WORKFLOWS.md all exist. - Starlight mirror is correctly generated, not hand-edited. I ran
node scripts/sync-starlight.mjsindocs/; the mirror regenerates with an empty git status (no drift) — CI "Fail build on mutation" will pass. Source and mirror bodies are identical after link-rewrite + whitespace normalization (35167 chars each). Frontmatter is the generatedtitle:block only. - Reference URLs normalized (
cloud.google.com,learn.microsoft.com) per the Changelog — verified at:118/:181. - Issue tracking: #246 carries
approved+P0; child issues #478/#479/#480/#481/#230 map 1:1 to deferred sub-decisions.
Tests & CI
- No unit tests apply — strictly two docs files (
git diff --name-only= ADR source + its mirror). No executable behavior. - Bootstrap synth-coverage: N/A. Diff touches zero CDK/handler/CLI/agent code. The bootstrap-policy /
BOOTSTRAP_VERSION/resource-action-map.ts/DEPLOYMENT_ROLES.mdgates attach to the implementation PRs (#664/#665), not this ADR. When those land they MUST carry the full bootstrap bundle (new AgentCore resource types) + shared-type parity — enforce there. - Shared-type sync: N/A.
cdk/src/handlers/shared/types.tsuntouched. - Cedar engine pin: N/A. No
cedarpy/cedar-wasmchange.
Review agents run
- Hand review by principal-architect judgment (primary): appropriate — the diff is prose; the ADR is the reviewable artifact. I empirically validated the load-bearing claims: re-ran the shipped
_REGISTRY_REFregex against the ADR's example refs; regenerated the Starlight mirror to prove zero drift; confirmedref.{ts,py}+contracts/registry-resolution/are absent (matching the ADR's own caveat); confirmed #246 governance labels; verified sub-decision numbering 1–13 with no gaps. - comment-analyzer (by hand): verified cross-references (#381 → sub-decision 13), the grammar caveat, and "shipped" assertions against actual repo state — this is how I confirmed each prior finding is resolved and surfaced nit #1.
- Omitted — code-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer: no executable code, error-handling paths, new types, or tests in the diff; nothing in scope.
- Omitted — /security-review: no IAM/Cedar/network/secret/API/input code in this diff. The security-relevant design assertions (fail-closed resolution
:51, coarse-ACL trust decision:156, publish-endpoint exposure:156) are called out in the ADR's own(!)risks and reviewed by hand under Vision alignment; their code lands in #664/#665 and must be security-reviewed there.
Human heuristics
- Proportionality — pass (improved): the governance-in-MVP over-scoping the prior review flagged is resolved — sub-decision 10 now right-sizes the shipped surface (publish + auto_approve + read) and defers the rest.
- Coherence — pass with minor concern: same-concept-same-term restored (dual
approved/activecollapsed to canonicalapproved,:47/:190). Minor residual:acceptedstatus asserting sibling-PR facts (nit #1). - Clarity — pass: names communicate intent; failure modes surface explicitly (fail-closed, specific reason codes
:51); MVP-vs-future boundaries are unambiguous. - Appropriateness — pass: maintainable by this team; slots beside the existing
contracts/cedar-parity/andcontracts/workflow-validation/parity corpora (both verified present); integration claims now checked against real repo state, and the one place code and contract must meet (the regex) is accurately described rather than overstated.
Excellent revision — thank you for taking the prior feedback seriously and re-grounding the whole document against what actually shipped. 🚀
isadeks
left a comment
isadeks
left a comment
There was a problem hiding this comment.
Verdict: Request changes on the current implementation claims
The Jul 27 rewrite does resolve my three earlier prose findings: TypeScript is now the single task-boundary resolver, the ABCA governance surface is explicitly thin, and the rich shared descriptor schema is correctly deferred. I rechecked those rather than carrying the stale review forward.
I still cannot dismiss the change request at head e998a430, because the ADR is now accepted and repeatedly labels behavior as shipped/proven while the exact #664/#665 heads violate those invariants. The executable findings are documented in the current reviews on #664 and #665. Either land those fixes before this ADR is accepted, or keep the ADR proposed/remove the shipped assertions until the implementation matches.
|
Thanks for the thorough review, @scottschreckengaust, @krokoko, @isadeks! The most common one, about the ADR being marked "Accepted," came from my misunderstanding: I assumed an accepted issue implied the ADR was accepted along with the PR. I'll flip that. |
…aws-samples#666) (aws-samples#669) Adopt EARS (Easy Approach to Requirements Syntax) as the required syntax for normative requirements in ADRs. The ADR states its own Decision in EARS to dogfood the standard; explanatory prose is intentionally out of scope. Regenerates the Starlight decisions mirror via docs:sync. Status: proposed (flips to accepted when this PR merges). Note: ADR-018 (aws-samples#548, aws-samples#662) and ADR-019 (aws-samples#663) are claimed by in-flight PRs; 020 is the first free number. May need renumbering if the queue shuffles before merge. Co-authored-by: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
- Drop stale candidate-numbering blockquote: ADR-018/020/021 are all on main now and ADR-019 is unambiguously free; PR aws-samples#548 (titled ADR-018) is the actual numbering collision, not this ADR. - Clarify the AGENTCORE_GATEWAY_MCP_SPIKE.md references live only on the upstream feat/agentcore-gateway-mcp branch, not merged to main. - Regenerate Starlight mirror.
Kalindi-Dev
changed the title
|
Thanks @scottschreckengaust, @isadeks, and @krokoko. Addressed the outstanding items: @krokoko (Alain) — suggested-before-merge
@scottschreckengaust — fix-before-accepted (all resolved; re-verified by Scott at the ADR-022 head)
@isadeks — P1 design-of-record itemsThe three P1 prose findings (single resolution owner; thin governance MVP surface; native descriptors vs shared JSON Schema) were resolved in the Jul 27 rewrite and re-confirmed. The remaining Renumber ADR-018 → ADR-022ADR-018 is already taken on |
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict
Request changes — narrowly. The prior blocking review is substantially discharged: the accepted flip was reverted to proposed with an explicit contingency note (:3, :39), and 4 of isadeks' 5 P1s are verified fixed in the sibling implementation heads with regression tests. I am not inheriting @ayushtr-aws' approval or isadeks' change request; I re-derived every disposition from the current code at #664 4f8da982 and #665 8362f4fb. Two blocking items remain, both one-paragraph prose fixes to this ADR: the design-of-record fixes seven invariants but omits the read-path credential-confidentiality invariant, and sub-decision 7's publish-validation invariant is architecturally defeated by the frontmatter carrier this ADR blesses at :59. Both are the ADR's problem, not only #664's — the implementation's fail-open followed from the contract's silence.
Vision alignment
Strong on most tenets, with one gap:
- Tenet 1 (fire-and-forget) — pass. Assets resolve at the create-task boundary (
:53); no live intervention added. TypeScript is named single resolution owner, keeping:55(no re-resolution) true. - Tenet 4 (fail closed on risk) — pass on the resolution path (
:55), concern on the read path. See blocking #1. - Tenet 5 (isolation and least privilege are non-negotiable) — concern.
:73grants resolve/read to "any authenticated caller" over a catalog that stores MCP connection runtime, and no invariant bounds what that runtime may contain. See blocking #1. - Tenet 7 (observable, attributable, replayable) — pass, and materially improved:
resolved_assetsnow carrieswarningsand emits a durable TaskEvent (#665cdk/src/handlers/shared/orchestrator.ts:800-824). - Tenet 8 (extensible without forking) — pass. The
RegistryClientseam (:63) is the right hedge while every candidate substrate is in preview. - Documented tenet trades — pass. The
(−)/(!)sections give explicit rationale; the:39Status note and the 2026-08-06 hard gate (:140) are exactly the falsifiable honesty the tenets ask for.
Disposition of each prior blocking finding
I re-tested all five against current sibling heads. Status framing first, because it changes the frame for all of them: isadeks' review-level demand was "Either land those fixes before this ADR is accepted, or keep the ADR proposed/remove the shipped assertions." The author took option two — :3 is proposed and :39 states outright that ship/validate/prove language describes "the intended contract, not … merged-to-main facts." That review-level blocker is discharged. So each finding below is judged as "is the ADR's contract still wrong," not "is it merged yet."
1. :51 — DEPRECATED warning event not shipped → FIXED (weak test). #665 cdk/src/handlers/shared/orchestrator.ts:803-810 persists warnings alongside the {kind,id,version} triple on the task record, and :816-825 emits a durable registry_asset_warning TaskEvent per warned asset. Exactly the remedy prescribed. Regression test is thin: cdk/test/handlers/shared/registry-orchestrator.test.ts:106 only asserts log.warn was called at resolveRegistryAssets level — it does not cover the DDB persistence or the TaskEvent emission, which live in a different function. Non-blocking nit, raised on #665.
2. :55 — composed runtime not fail-closed after resolution → FIXED with tests. Every leg isadeks probed now raises: #665 agent/src/registry/loader.py:135-138 (missing repo_dir), :153-154 (empty runtime), :166-167 (.mcp.json OSError), :65-74 (_to_mcp_config structural invalidity), :198 (skill with no usable prompt_fragment), and cdk/src/handlers/shared/orchestrator.ts:842-848 throws on empty cedar_text with the correct reasoning recorded (a dropped policy is usually a deny rule, so silent omission would widen agent authority). agent/src/pipeline.py:1151 documents the propagation. The foo-bar/foo_bar collapse is fixed at loader.py:32-41 with a non-normalizing key and an explicit comment, locked by agent/tests/test_registry_loader.py:87-100 asserting both keys survive with distinct URLs. Well done.
3. :55/:59 (orig. ADR-018:55) — stated validation broader than implementation → PARTLY FIXED; the frontmatter-contract clause is now live in a worse form. Fixed: cdk/src/handlers/registry-publish.ts:149-151 (isPlainObject rejects arrays for both discovery and runtime) and :160-193 (validateRuntime enforces a real discriminated contract per kind — transport enum, url/command required, headers object-typed), so the 201-on-garbage cases are closed. The apostrophe/YAML break is fixed by base64-encoding the runtime (cdk/src/handlers/shared/registry/agentcore-client.ts:122-125), locked by agent/tests/test_registry_agentcore_client.py:87. Still live: isadeks' "CUSTOM receives no equivalent structural validation" holds — registry-publish.ts:77 passes custom: body.custom straight through and validate() never inspects it, so :59's "CUSTOM (verbatim JSON, structurally validated)" is unearned. And the frontmatter contract clause reopens as blocking #2 below, in a form that is not a coverage gap but a bypass.
4. :71 — non-auto-approved publish never reaches PENDING_APPROVAL → FIXED with test. agentcore-client.ts:229-234 now sends SubmitRegistryRecordForApprovalCommand unconditionally, outside the autoApprove branch, with only the terminal APPROVED transition gated at :235-244 — precisely the fix isadeks prescribed ("submit every successful create and gate only the final approval transition"). Locked by cdk/test/handlers/registry-handlers.test.ts:133/143 asserting PENDING_APPROVAL. :71's prose now matches ("stays PENDING_APPROVAL from ABCA's side until acted on out-of-band"). The partial-record path even got a typed RegistryPublishIncompleteError with the orphan recordId surfaced instead of a bare 500 — better than asked for.
5. :148 — reproducibility not guaranteed by the shipped path → FIXED with tests, all three sub-claims. (a) No REMOVE on Blueprint update: cdk/src/constructs/blueprint.ts:443-457 emits REMOVE #mcp_servers, #cedar_policy_modules, #skills for emptied fields, with the reasoning comment naming the exact stale-refs hazard; three regression tests at cdk/test/constructs/blueprint.test.ts:340/344/359. (b) Cross-kind refs in typed fields: blueprint.ts:261-263 attaches a per-field RegistryRefValidation(..., 'mcp_server' | 'cedar_policy_module' | 'skill'), so a cross-kind ref fails at synth. (c) Non-injective key mapping: fixed per finding 2. This consequence is now factual.
Not a prior finding, but re-verified as still accurate: the grammar reframe at :43. I re-ran the ADR's own examples against the shipped _REGISTRY_REF (agent/src/workflow/validator.py:50, ^registry://[a-z][a-z0-9-]*/[a-z0-9][a-z0-9./-]*$): mcp_server/acme/foo, mcp_server/acme/foo@^1.4.1 → no match; mcp-server/acme/foo → match. The "extends the committed shape … admitted a 2-segment form but not the @<constraint> suffix or _" wording is exactly right. The new short-vs-long migration note (:45) is also empirically correct: all three WORKFLOWS.md illustrative refs (registry://prompt/…, registry://mcp/…, registry://skill/…) do match the lenient regex, so they will not break validation — and WORKFLOWS.md:98/:256 really do mark them as runner-ignored forward-declarations. Good, checkable prose.
New blocking issues
B1 — docs/decisions/ADR-022-agent-asset-registry.md:73 (and the missing (!) bullet near :163): the ADR fixes seven invariants but never bounds what a resolvable runtime payload may contain, so the read surface has no confidentiality contract.
:73 grants "Resolve/read … to any authenticated caller" and :142 enumerates the invariants that "hold regardless of substrate" — semver, immutability, resolve-at-boundary, descriptor validation, governance, fail-closed, resolver seam. Confidentiality of the runtime payload is not among them, and no (!) bullet raises it. :163 covers only the publish side.
This is not an abstract omission — the implementation inherited the silence. At #664 4f8da982, cdk/src/handlers/registry-resolve.ts:45-61 redacts by denylist over exactly three keys (headers, command, args) while registry-publish.ts:160-193 accepts arbitrary additional runtime keys. Concretely, on an endpoint open to every authenticated caller:
runtime.url = "https://mcp.example/sse?token=SECRET"—urlis never redacted (it is required for http/sse at:170-172), returned verbatim;runtime.env = { TOKEN: "…" },runtime.api_key,runtime.auth— unknown keys pass publish validation and pass throughredactRuntimeForResponseuntouched.
Risk: the catalog becomes a tenant-wide credential-read endpoint. That is a direct hit on tenet 5 ("isolation and least privilege are non-negotiable") and tenet 4 (fail closed) — a denylist over field names is fail-open by construction against a payload whose key space is open.
Suggested fix (prose only): add an invariant — as a sub-decision or an (!) bullet, and to the :142 list — stating (a) runtime payloads MUST NOT carry credentials (credential material is referenced, e.g. a Secrets Manager ARN the orchestrator dereferences, never inlined), and (b) any read surface reachable by a non-approver caller redacts by allowlist of known-safe fields, not denylist. That single sentence is what turns #664's redaction from fail-open into fail-closed, and it belongs in the design-of-record because #664 is entitled to cite this ADR as its contract.
B2 — docs/decisions/ADR-022-agent-asset-registry.md:57 + :59: sub-decision 7's publish-validation invariant is defeated by the storage encoding this ADR blesses in the very next paragraph.
:57 states the invariant absolutely — "Every published asset carries a typed descriptor validated at publish; malformed descriptors reject publish." :59 then blesses the carrier: "skills as Markdown frontmatter." Those two are in tension, and #664 shows why. At cdk/src/handlers/shared/registry/agentcore-client.ts:114-116 the caller-controlled discovery.description is interpolated raw into a YAML frontmatter line (:126) that sits above the runtime key (:128), and parseSkillRuntime at :152 recovers the runtime with a first-match, multiline regex (^x-abca-runtime:\s*(.+?)\s*$/m). A description containing a newline plus x-abca-runtime: <attacker-base64> therefore emits a shadowing key that is parsed first and wins — smuggling a runtime payload that validateRuntime (registry-publish.ts:160) never saw. validate() type-checks discovery only as a plain object (:138-139); no field of it is validated.
Risk: publish-time descriptor validation — the invariant :57 asserts unconditionally, and the only structural gate the MVP has now that the rich JSON Schema is deferred to #481 — is bypassable by any RegistryPublisher. This is a bypass, not a coverage gap, so it is not cured by the :39 "intended contract, not merged fact" caveat: the intent is unachievable under the blessed encoding.
Suggested fix (prose only): in sub-decision 7, require that the validated runtime descriptor be carried in a channel isolated from caller-controlled discovery prose — i.e. the runtime carrier must be either structurally serialized (not line-oriented text concatenation) or the recovery must be last-wins/uniqueness-enforced with all caller-supplied frontmatter values escaped — and state that publish-time validation MUST be non-bypassable by any caller-supplied field. Then #664 has a contract to implement against. (Note this also cures the residual CUSTOM-unvalidated gap from prior finding 3 if you extend the same sentence to CUSTOM.)
Non-blocking nits
:5—Last-updated: 2026-07-28is stale. The renumber (8b394c95, 2026-08-10) andd2055abfboth changed content.docs/decisions/README.md:81requires bumpingLast-updated:on in-place refinement. Bump to the renumber date.:190— the Changelog has no entry for the ADR-018 → ADR-022 renumber. This is the single most confusing thing about the file's history for a future reader (README:57says numbers are never reused, so a reader will wonder what happened to 018). One bullet: "renumbered 018 → 022; ADR-018 was taken by Linear agent-session interaction onmain.":192— the 2026-07-28 Changelog entry is stale. It lists as "Also pending in this round" the short-vs-long-form alias note, the federation non-goal, and the 2026-08-06 cutover gate — all three have since landed (:45,:162,:140). Promote them from "pending" to done, or the changelog understates the document.:51— a dual token for one state reappears."submitted(a.k.a. pending approval)"reintroduces two names for one state immediately after:198claims the dual-token parity hazard was collapsed. Cheap to make consistent (submittedis the ADR token,PENDING_APPROVAL` the substrate token — say that once).- Branch is 12 commits behind
main. No conflict (ADR-022 is a new file, and I confirmed 022 is free), but worth a merge before landing so lint/link CI runs against currentmain. - Altitude / perishability (carried, author's call). The ~15-vendor survey (
:120-131) and pinned strings remain the fastest-aging content in a durability artifact. Defensible as the "why AgentCore won" record; just flagging.
Documentation status
- Numbering: clean, and the renumber was necessary.
maincarriesADR-018-linear-agent-session-interaction.mdplus 020/021, so the old 018 slot genuinely collided. 019 is claimed by open PR #663 (AgentCore Gateway); 022 is the next free number — no collision. - No dangling
ADR-018references. Repo-wide grep across*.md/*.ts/*.py/*.mjs/*.json/*.yaml(excludingnode_modules,.venv) returns zero hits forADR-018/Adr-018/adr-018— including in the sibling heads (#6644f8da982, #6658362f4fb): the only ADR number either cites isADR-022, atorchestrator.ts:804. The rename is complete on both sides of the stack. - Starlight mirror verified generated, not hand-edited. I ran
node docs/scripts/sync-starlight.mjsat this head: emptygit status, zero drift — CI "Fail build on mutation" will pass. The only source↔mirror deltas are the generatedtitle:frontmatter and site link rewrites (./ADR-014-…md→/…/architecture/adr-014-workflow-driven-tasks), exactly as expected. - Status field is correct (
proposed,:3) and consistent withREADME.md:69("moves toacceptedwhen the implementing PR merges") and with the:39note. Template compliance (Status/Date/Context/Decision/Consequences/References/Changelog) matches ADR-016/017 shape. - All internal link targets exist at this head:
ADR-014,ADR-003,docs/design/WORKFLOWS.md,agent/src/workflow/validator.py,agent/src/workflow/loader.py.:179correctly de-linksregistry/ref.{ts,py}as paths-not-links since they land in #664 — that caveat is honest and is why link-check stays green. (docs/scripts/link-check.shcould not run here —markdown-link-checkbinary absent without a fullmise run install; unverified for external URLs.)
Tests and CI
- No unit tests apply.
git diff --name-onlyis exactly two files: the ADR source and its generated mirror. Zero executable behavior. - Bootstrap synth-coverage: N/A, correctly. This diff touches no
cdk/,agent/,cli/,constructs/,stacks/, orhandlers/code, so no new CloudFormation resource types are introduced and there is nothing to gate. The ADR-002/#350 bundle —cdk/src/bootstrap/policies/*.ts,resource-action-map.ts,BOOTSTRAP_VERSION, regeneratedbootstrap/policies/*.json+bootstrap-template.yaml+BOOTSTRAP_HASH, and thedocs/design/DEPLOYMENT_ROLES.mdgolden baseline — attaches to #664, which introduces the AgentCore registry resource types and has since moved the registry API into a nested stack (a change that can shift logical IDs and bootstrap expectations). I did not re-verify #664's bootstrap bundle here; that is enforced in #664's own review. - Shared API type sync: N/A.
cdk/src/handlers/shared/types.tsuntouched, so nocli/src/types.tsobligation. - Cedar engine pins: N/A. No
@cedar-policy/cedar-wasmorcedarpymovement, no parity-fixture refresh owed. - CDK test performance (#366): N/A. No test files in this diff.
- Solution UA (#319): N/A. No AWS client constructed. (Flagging for the stack: the registry clients in #664 must route through
makeClient/tenant_client.) - Do not approve on CI-green alone — CI green here only means "the prose lints," which is not evidence about B1/B2.
Review agents run
The Agent tool exposed to me in this context provides task-tracking only, not subagent dispatch, so I could not run the pr-review-toolkit subagents. I applied each rubric explicitly and label them rubric-applied, not agent-run:
- comment-analyzer — rubric-applied (primary, and the highest-yield lens here). For prose, the document is the artifact, so every load-bearing claim was checked against real code rather than read: re-ran the ADR's example refs against the shipped
_REGISTRY_REFregex; verifiedloader.py:107and WORKFLOWS.md:98/:256say what:18/:45claim; verified all five prior findings against #664/#665 heads; regenerated the Starlight mirror to prove zero drift; grepped the whole repo (and both sibling heads) for dangling ADR-018 refs; confirmed 022 is free againstmainand open PR #663. This lens produced B1, B2, and nits 1-4. - security-review — rubric-applied. In scope: the ADR's authz/confidentiality assertions are security contract, and its silence produced a real fail-open. Read
registry-resolve.ts,registry-publish.ts,agentcore-client.ts,blueprint.tsat the sibling heads and traced both P1 paths end to end (denylist redaction over an open key space; first-match frontmatter shadowing that precedes the runtime key). Produced B1 and B2. IAM resource-ARN scoping on the #664 role is out of scope for a docs diff and belongs to #664's review. - silent-failure-hunter — rubric-applied. No error handling in the diff, but the ADR's fail-closed invariant (
:55) is exactly this rubric's subject, so I audited the composed swallow-paths isadeks flagged (return 0, empty-fragment, write errors) and confirmed each now raises. Clean. - pr-test-analyzer — rubric-applied. No tests in the diff; I instead audited whether each claimed fix is locked in. Four of five carry real regression tests (cited above); the DEPRECATED-warning persistence/TaskEvent is the one fix with only indirect coverage — nit, raised for #665.
- code-reviewer — omitted. No executable code, no style/guideline surface. Its docs-adjacent concerns (template compliance, mirror sync) are covered under Documentation.
- type-design-analyzer — omitted. No new types declared; the ADR describes types that land in #664, where the rubric properly applies.
Human heuristics
- Proportionality — pass. Right-sized:
:69scopes the shipped surface topublish/resolve/list/showand:71names promote/reject/deprecate, env-gated auto-approve, and event consumption as tracked future scope. The earlier governance-in-MVP over-scoping is genuinely resolved, and each deferral maps to a real child issue (#478/#479/#480/#481/#230). My two blocking asks are each one paragraph — deliberately not a rewrite. - Coherence — concern.
:57("validated at publish", absolute) versus:59(frontmatter carrier) is an internal tension the document does not acknowledge — B2. Smaller::5staleLast-updated,:190silent on the renumber,:192calling landed items "pending",:51reintroducing a dual token that:198claims was collapsed. - Clarity — pass. Genuinely strong.
:39is the model of an honest status caveat;:45converts a latent vocabulary trap into checkable prose;:55names specific failure reason codes rather than gesturing at "errors"; MVP-vs-future boundaries are unambiguous throughout.:179de-linking not-yet-merged paths is the right instinct. - Appropriateness — concern (scoped to B1). The substrate choice, the
RegistryClientseam, and slotting the parity corpus beside the verified-existingcontracts/cedar-parity/andcontracts/workflow-validation/are all well matched to this team and this repo's twice-learned lessons. The one mismatch: for a catalog that stores MCP connection material behind an any-authenticated-caller read (:73), the invariant set at:142is under-specified for the threat model the substrate choice creates.
To be explicit about what would flip this to approve: B1 and B2 are prose-only. Two paragraphs — one invariant bounding runtime-payload contents and mandating allowlist redaction on the open read surface, one requiring the validated descriptor to be carried out of reach of caller-controlled prose — and I would approve. Everything else in this document is in good shape, and the disposition of the prior review is genuinely impressive work: four P1s fixed with real regression tests, plus the discipline to revert your own accepted flip rather than defend it. That is the harder call and you made it correctly.
…mples#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.
…ion (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).
…-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).
…ws-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).
…DR-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.
aws-samples#641) (aws-samples#663) * docs(decisions): ADR-019 unify agent tools behind an AgentCore Gateway (aws-samples#641) * fix(docs): correct Decisions sidebar autogenerate config Starlight requires `autogenerate` as a sibling of `label`, not wrapped in `items[]`. The nested form failed `astro check` on main; this unblocks the docs CI build for the ADR-019 PR. * docs(decisions): rescope ADR-019 to lead with simplest target, drop Linear/Jira MCP (aws-samples#641) Address review on aws-samples#663: - Fix Context/Scope facts: zero functional platform-managed MCP servers today (one non-functional jira placeholder; Linear removed and stripped per ADR-016), not "Linear + Jira". - Reorder phasing: P1 leads with a Lambda tool target (IAM execution role, no stored credential, no consent) per aws-samples#641; the 3LO-OAuth remote-MCP path is exercised last. - Remove Linear + Jira from near-term scope; Linear stays deterministic (ADR-016), Jira stays the REST shim (ADR-015). Gateway-unbreaks-Jira-MCP demoted to a separately-gated speculative P4 experiment touching neither deterministic path. - Drop "admin direction" framing and the deferred ADR-016 tension callout; no reversal remains to reconcile. * fix(docs): revert Decisions sidebar to nested autogenerate form Commit 23e9810 rewrote the Decisions sidebar entry to the sibling form `autogenerate: { directory: 'decisions' }`, but Starlight's config schema rejects that here (sidebar.6 'Did not match union' → the `build (agentcore)` CI job failed at //docs:build). The nested form on main — `items: [{ autogenerate: { directory: 'decisions' } }]` — is what both `astro check` (0 errors) and `docs:build` (69 pages) accept. Restore it. * docs(decisions): resolve ADR-019 review nits (aws-samples#641) - Drop stale candidate-numbering blockquote: ADR-018/020/021 are all on main now and ADR-019 is unambiguously free; PR aws-samples#548 (titled ADR-018) is the actual numbering collision, not this ADR. - Clarify the AGENTCORE_GATEWAY_MCP_SPIKE.md references live only on the upstream feat/agentcore-gateway-mcp branch, not merged to main. - Regenerate Starlight mirror. --------- Co-authored-by: bgagent <bgagent@noreply.github.com> Co-authored-by: Maya Kosteski <130111194+mayakost@users.noreply.github.com>
8b394c9 to
e5e5049
Compare
|
Thanks @scottschreckengaust — both blocking prose items added to ADR-022: B1 read-path confidentiality invariant (no inlined credentials; allowlist redaction) in sub-decision 11 + the substrate-invariant list, and B2 non-bypassable descriptor validation (carrier isolated from caller-controlled prose, incl. |
theagenticguy
left a comment
theagenticguy
left a comment
There was a problem hiding this comment.
Verified both blocking findings from the 2026-08-11 review round against this branch head, plus the non-blocking nits. Everything checks out:
[B1] Read-path credential confidentiality — fixed. Sub-decision 11 now states both invariants: runtime payloads reference credentials (Secrets Manager ARN dereferenced by the orchestrator at connect time), never inline them; and any read surface reachable by a non-approver caller redacts by allowlist, with the correct rationale spelled out (a name-denylist is fail-open against an open key space — the url-query-string and unknown-key cases from the original finding are exactly what the allowlist closes). The invariant also landed in the Context "Substrate is not the invariant" list (line 26), so it is contract-level rather than substrate-behavior. The orchestrator connect-path exemption is the right carve-out.
[B2] Descriptor-carrier isolation — fixed. Sub-decision 7 now requires the validated runtime descriptor be carried isolated from caller-controlled discovery prose (structurally serialized, or duplicate-rejecting/last-wins with escaped values), states validation MUST be non-bypassable by any caller-supplied field, and explicitly extends the requirement to CUSTOM records including the custom flag itself. That also discharges the residual on the previously-unearned "CUSTOM … structurally validated" claim: the contract now demands what the claim asserts, and the :39 Status note correctly scopes it as intended-contract-under-review until #664/#665 merge.
Nits from the prior round — all fixed. The submitted/PENDING_APPROVAL mapping is stated once in sub-decision 4 and submitted is used consistently after that; Last-updated is bumped to 2026-08-11; the Changelog carries the 018 → 022 renumber entry with the "numbers are never reused" rationale; and the 2026-07-28 entry now records the alias note, federation non-goal, and cutover gate as landed rather than pending.
One non-blocking consistency nit inline (the closing substrate-invariant recap omits the new read-path invariant). The Starlight mirror carries the same fixes, and all four checks are green. This is ready.
scottschreckengaust
left a comment
scottschreckengaust
left a comment
There was a problem hiding this comment.
Verdict
Approve. Both blocking items from my 2026-08-11 change request are genuinely fixed in this ADR's prose, and I verified the fix removes the capability rather than the symptom — including re-deriving js-yaml's actual duplicate-key and block-scalar behavior rather than taking the commit message's word for it. I did not inherit @theagenticguy's approval or @ayushtr-aws' dismissal; I re-read the current text at e5e5049b line by line and re-traced both attack paths against the sibling implementation at #664 c8e4790c. What remains is four nits, none of which is a defect in the decision.
Vision alignment
- Tenet 1 (fire-and-forget) — pass. Resolution stays at the create-task boundary (
:53); no live intervention added. - Tenet 4 (fail closed on risk) — now pass on the read path too. This was the gap.
:77makes the read surface fail-closed by construction ("projects the known-safe fields for the asset kind and drops everything else") and states the reason a name-denylist cannot be made safe ("fail-open by construction against a payload whose key space is open"). - Tenet 5 (isolation and least privilege are non-negotiable) — now pass.
:75still grants resolve/read to any authenticated caller, but:77bounds what that caller may see, and bounds what may be stored in the first place ("runtime payloads MUST NOT carry credential material — credentials are referenced … never inlined"). The tenet-5 concern I raised was precisely that nothing bounded the payload; it is now bounded at both boundaries. - Tenet 7 (observable, attributable, replayable) — pass.
resolved_assets+ warnings + durable TaskEvent. - Tenet 8 (extensible without forking) — pass.
RegistryClientseam (:65). - Documented tenet trades — pass. The
(−)/(!)sections, the:39Status contingency, and the 2026-08-06 hard gate (:144) remain the falsifiable honesty the tenets ask for.
Status of prior blocking findings
| Prior finding | Status | Evidence |
|---|---|---|
| B1 — ADR silent on read-path credential confidentiality; no bound on runtime-payload contents; no allowlist mandate on the any-authenticated-caller read surface | FIXED | docs/decisions/ADR-022-agent-asset-registry.md:77 (new normative paragraph in sub-decision 11) + :26 (Context "Substrate is not the invariant" list). Both requested halves are present and normative, not prose gesture: (a) "runtime payloads MUST NOT carry credential material — credentials are referenced (e.g. a Secrets Manager ARN the orchestrator dereferences at connect time), never inlined"; (b) "any read surface reachable by a non-approver caller redacts by allowlist — it projects the known-safe fields for the asset kind and drops everything else, rather than masking a fixed denylist of field names." It goes one step further than I asked and closes the write boundary too: "Publish-time validation enforces the same closed key set (unknown runtime keys reject)." The orchestrator connect-path carve-out (:77 last sentence) is correctly scoped to the RegistryClient port rather than the human-facing API. Locked in code + tests: allowlist at cdk/src/handlers/registry-resolve.ts:43-81 (#664 c8e4790c), closed publish key set at cdk/src/handlers/registry-publish.ts:172-186, regression tests cdk/test/handlers/registry-handlers.test.ts:259 (url-query token → origin only; header values masked) and :286 (api_key/env/command/args dropped though on no denylist — the exact fail-open case from my finding). |
| B2 — sub-decision 7's absolute "validated at publish" claim defeated by the Markdown-frontmatter carrier blessed in the next paragraph | FIXED | :59 adds the invariant I asked for, verbatim in substance: "Validation must be non-bypassable by any caller-supplied field. The validated runtime descriptor MUST be carried in a channel isolated from caller-controlled discovery prose — either structurally serialized (not line-oriented text concatenation), or recovered with a parser that is last-wins / rejects duplicate keys and escapes every caller-supplied value," and it names the defeating case explicitly ("A carrier where a free-text discovery field (e.g. a skill description) can inject or shadow the runtime key defeats this invariant"). The absolute claim at :57 is no longer unqualified — :59 converts it from an assertion into an obligation with an enforcement condition, which is the right resolution of the tension. :61 also qualifies the carrier itself ("emitted and recovered through a real YAML serializer (never string concatenation, so a description newline cannot inject a key)"). The CUSTOM residual from prior finding 3 is swept in too: "applies equally to CUSTOM records — a CUSTOM body's structural fields (including the custom flag itself) are validated, not passed through verbatim" — which is now earned, since registry-publish.ts:146 rejects a non-boolean custom. Capability check, not symptom check: I ran js-yaml myself against the contract the ADR now mandates. yaml.dump({description:'benign\nx-abca-runtime: EVIL', 'x-abca-runtime':'LEGIT'}) emits the description as a block scalar (description: |-), so the injected line stays inside the value; and yaml.load(dup, {json:true}) is last-wins on duplicate keys. Both branches of the ADR's stated requirement therefore hold, and agentcore-client.ts:149-160 / :174-183 recover the key from the parsed object rather than a first-match regex, so no caller-supplied field can shadow it. Locked by agent/tests/test_registry_agentcore_client.py:107-129 and the TS mirror. |
| Renumber 018 → 022 hygiene | FIXED (one stale banner elsewhere — nit 3) | File, mirror, and all self-references are consistently ADR-022; the Changelog :196 records the renumber with the "numbers are never reused" rationale, which is the right thing to preserve. No ADR-018 reference inside this ADR points at the registry. docs/decisions/README.md has no ADR index table, so nothing to update there. |
| Docs mirror consistency | FIXED (independently verified) | I ran node docs/scripts/sync-starlight.mjs at this head: git status --porcelain came back empty — zero drift, so CI "Fail build on mutation" will pass. The only source↔mirror deltas are the generated title: frontmatter and site link rewrites (./ADR-014-…md → /…/architecture/adr-014-workflow-driven-tasks), exactly as the generator produces. The new B1/B2 paragraphs are present in the mirror. |
Nits from the prior round (Last-updated, renumber changelog entry, "pending"→landed, dual status token) |
FIXED | :5 = 2026-08-11 (matches the head commit date); :196 renumber entry; :197 now reads "Landed in this round: the kind-vocabulary alias note …, the federation Non-goal, and the 2026-08-06-cutover gate"; :51 collapses the token to one mapped statement ("submitted is the ABCA token; the AgentCore substrate names that state PENDING_APPROVAL — one state, two names, mapped once here"). |
Blocking issues
None.
Non-blocking suggestions / nits
:146— the closing invariant recap still omits read-path confidentiality (inline). "Regardless of substrate, the invariants above (semver, immutability, resolve-at-boundary, descriptor validation, governance workflow, fail-closed, resolver interface as the seam) hold." The new invariant made it into the:26list but not this one. Not blocking — the normative statement lives in sub-decision 11 (MUST NOT/MUST) and:26— but a reader who skims to the "regardless of substrate" recap when picking a replacement substrate would not see it, which is exactly the reader that sentence exists for. Same nit @theagenticguy raised.:59— "last-wins" is the weaker of the two options you offer; consider requiring duplicate-key rejection (inline). I confirmedyaml.load(s, {json:true})is last-wins, not throw-on-duplicate, so the sentence is accurate about whatjs-yamldoes. Under last-wins, a record whose frontmatter was authored outside the ABCA publish gateway (direct substrate write, or a legacy record) and that appends a secondx-abca-runtime:key wins over the validated one. That caller needs registry write IAM, so it is a privileged-actor path and materially weaker than the original finding — but "rejects duplicate keys" is strictly the safer of your own two alternatives and costs one line to make mandatory rather than optional.docs/decisions/ADR-021-lambda-microvms-compute-backend.md:3— stale number banner left by the renumber. It reads "ADR-018 is claimed by open PR #548, ADR-019 by open PR #663." #548 now claims 022, and #663 has merged as ADR-019. That file is onmainand untouched by this diff, so I am not asking you to reach into it here — but it is the one dangling artifact of the 018→022 move, and a one-line edit either in this PR or a follow-up would close it.:73usesPENDING_APPROVALafter:51promises "referred to assubmittedthroughout" (inline). Defensible where you are describing a substrate state, but it slightly undercuts the single-token discipline:51establishes; "stayssubmitted(substratePENDING_APPROVAL)" would keep the promise.- Cross-PR, belongs to #664's review, not here:
registry-publish.ts:172comments "Kept in sync with the allowlist inregistry-resolve.ts," but the resolve allowlist projectsruntime.type(registry-resolve.ts:65) whileALLOWED_RUNTIME_KEYS.mcp_serverdoes not permittype— so atypekey can never survive publish and that branch is dead. Harmless today (fail-closed direction), but the two lists are the kind of paired constant that drifts; a shared constant would make the "kept in sync" comment enforceable instead of aspirational.
Documentation
- This PR is the documentation. Docs-only diff:
docs/decisions/ADR-022-agent-asset-registry.md(+204) and its generated mirror. No code, no contracts, no env vars, no commands changed. - Mirror sync: verified clean by regenerating (see table above). Mirror was regenerated, not hand-edited.
- Template compliance: Status / Date / Last-updated / Context / Decision / Consequences / References / Changelog all present and in
docs/decisions/README.mdorder.Date:(2026-07-08) correctly preserved whileLast-updated:moved — the exact discipline the README asks for. - Status handling is correct: stays
proposedwith the:39note scoping ship/validate/prove language to "the intended contract, not … merged-to-mainfacts" until #664/#665 land. That is the README rule applied honestly, and it is what makes the new invariants readable as requirements on #664 rather than claims about it. - Issue tracking: #246 is
approved+ P0 and covers the stack; child issues #478–#481 / #230 are each mapped to a specific sub-decision (:157). Governance settled — not re-litigated here. Branchfeat/246-adr-agent-asset-registrymatches the required(feat\|fix\|chore\|docs)/<issue>-slugshape.
Tests & CI
- CI: all four checks green at
e5e5049b(build (agentcore), secrets/deps/workflow scan, dead-code advisory, PR title). Green CI does not discharge a change request and I did not treat it as evidence — the fix verification above is independent of it. - Bootstrap policy checklist (Stage 3): not applicable. No constructs, stacks, handlers, or CFN resource types in this diff;
BOOTSTRAP_VERSION/resource-action-map.ts/DEPLOYMENT_ROLES.md/ synth-coverage are untouched and correctly so. - CDK synth-test performance (#366): not applicable — no CDK tests in this diff.
- #319 solution-UA rule: not applicable to this diff (no AWS clients). Spot-checked the invariant in the sibling anyway since this ADR is its contract:
agentcore-client.ts:202builds throughmakeClient(BedrockAgentCoreControlClient), so attribution is intact — no nakednew XxxClient({}). - Regression coverage for the two findings lives in the siblings, and I ran my eyes over the assertions rather than the titles.
cdk/test/handlers/registry-handlers.test.ts:259/:286andagent/tests/test_registry_agentcore_client.py:107assert on the absence of the secret in the serialized response and on the validated runtime winning over the injected one — i.e. they test what the code should do (AI005), not merely that a function was called. That is the right shape for a security regression test. An ADR PR cannot itself carry executable tests, which is why I weighted the sibling evidence.
Review agents run
Process disclosure, stated plainly as required: I am executing inside a workflow-spawned subagent, and nested agent dispatch is unavailable from this context — I could not invoke the pr-review-toolkit agents as separate agents. I did not silently skip Stage 3: I applied each agent's rubric by hand against this diff, and I am naming the rubric per dimension so the omission is auditable.
- code-reviewer (rubric applied by hand) — Repo conventions for
docs/decisions/: template field order,DatevsLast-updatedsemantics, dated Changelog per refinement, numbering ("never reused"), mirror generated not hand-edited, routing (docs change lands indocs/, mirror regenerated via the script). All pass; findings became nits 1, 3, 4. - comment-analyzer (rubric applied by hand) — In scope and the highest-value rubric here, because an ADR is 100% prose asserting things about code. Every factual claim re-derived against source, not accepted: the
_REGISTRY_REF"extends, not matches" framing (:43) againstagent/src/workflow/validator.py:50; the:61"real YAML serializer" claim againstyaml.dumpoutput I generated myself; the:59last-wins claim againstyaml.load(…, {json:true})I ran myself; the:77allowlist claim againstregistry-resolve.ts:43-81; the:59CUSTOM-flag claim againstregistry-publish.ts:146; the:71thin-surface claim against the four handlers actually present (registry-{publish,resolve,list,show}.ts). One inaccuracy found and it is external to this file (nit 3). No claim in this ADR is now broader than its implementation — which was the whole substance of my prior review. - security-review (rubric applied by hand) — In scope: this ADR is the security contract for an endpoint open to every authenticated caller. Re-traced both attack paths end to end at #664
c8e4790c: (i) credential read via the open resolve surface — closed by allowlist projection + url-origin reduction + header-value masking + closed publish key set; (ii) publish-validation bypass via frontmatter shadowing — closed by whole-block YAML parse + object-keyed recovery + block-quoted emission. Residual capability, stated precisely: last-wins duplicate-key recovery leaves a privileged-actor shadowing path for records authored outside the ABCA publish gateway (nit 2) — materially narrower than the original finding, which was reachable by any publisher through the public API. IAM ARN scoping on the #664 role remains #664's review, not a docs diff's. - type-design-analyzer — omitted. No types, interfaces, or schemas in this diff (docs-only). The type-shaped assertions the ADR makes about
RegistryClient/resolved_assetswere checked under the comment-analyzer rubric instead. - silent-failure-hunter — omitted as a diff scope, applied as a lens. No error-handling code here. I did apply the fail-open lens to the contract, which is exactly what produced B1 last round; this revision now mandates fail-closed on the read path (
:77), so the lens comes back clean. - pr-test-analyzer (rubric applied by hand) — An ADR cannot carry unit tests, so I assessed whether the invariants are lockable and whether they are in fact locked in the siblings. Both are, with assertions that would fail if the capability returned (see Tests & CI). No coverage gap I can attribute to this PR.
Human heuristics
- Proportionality — pass. Two prose paragraphs and one qualifying clause, exactly the surface my finding required. No new abstraction, no new machinery, no defensive over-specification. 204 lines is long for an ADR but the length is the vendor-survey record, which is deliberate and previously discussed.
- Coherence — pass (improved). The
:57↔:59tension that I flagged as an unacknowledged internal contradiction is now acknowledged and resolved in place — the document names the defeating case instead of hiding it, which is the harder and better authorial choice.:26and:77use the same vocabulary (allowlist / referenced-not-inlined) as the implementation's own comments, so the two-artifact story reads as one. Residual: nits 1 and 4 are small coherence frays, not contradictions. - Clarity — pass.
:77does the thing most security prose fails to do: it states why the weaker mechanism cannot be repaired ("a denylist over field names is fail-open by construction against a payload whose key space is open"), so a future contributor cannot "simplify" it back to a denylist without knowingly contradicting the record. That sentence is the single most valuable line added in this round, and it is precisely what stops the hole from being legitimately reintroduced. - Appropriateness — pass. Maintainable by this team, and the invariants are verified against real behavior rather than self-written mocks (AI001): the frontmatter claim is backed by actual
js-yamlsemantics, which I re-ran independently.
Closing
This is the disposition I hoped for and did not assume: you took two findings that were easy to wave off as "the ADR is just prose, fix it in #664," and instead fixed the design of record and the code, and locked the code with tests that assert the secret's absence rather than a call count. The :77 rationale sentence in particular is the difference between a fix and a durable fix. Clearing my change request — the two blocking items are closed, and the four nits are all one-liners you can take here or in the follow-up. Nice work.
Sphia's changes are addressed and replied to with fixes, and this been has since then been through another two rounds of review. Dismissing her review as she is currently unavailable to review this changes and there are two reviews in place.
7 participants
Status
**
approvedlabel on #246 per ADR-003.**Summary
Proposes ADR-022, which fixes the contract for a central agent asset registry closing #246:
registry://<kind>/<namespace>/<name>@<constraint>(already declared atagent/src/workflow/validator.py:50).mcp_server,cedar_policy_module,skill. Others declared but not loaded.^/~only. Rejects*,latest,>=.(kind, namespace, name, version)immutable once published; republish returns 409.draft → submitted → approved | rejected → deprecated → removed.workflow_refresolves today;resolved_assetsstamped on the task record.RegistryClientabstraction; substrate changes and any AgentCore Aug 6 2026 rename are confined to one implementation file per language.docs/+ plugin markdown.Substrate
The ADR does not pick the substrate. It ranks candidates by fit for ABCA and defers the pick to the design PR:
Design PR decision framework names four concrete conditions under which the design PR flips from preferred to fallback (region GA, immutability guards, semver-on-top complexity, Aug 2026 rename cost).
Addressing #381 (per @scottschreckengaust's comment on #246)
Sub-decision 13 splits the two concerns cleanly:
docs/and plugin markdown with frontmatter edges + parity linter.If a skill record's descriptor eventually cites an ADR, that's metadata on the record — not a graph edge #381 owns. Happy to iterate on the boundary if you see it differently.
Test plan
docs/decisions/ADR-022-agent-asset-registry.mddocs/src/content/docs/decisions/Adr-022-agent-asset-registry.mdapprovedlabel on feat(registry): central agent asset registry for capabilities, skills, plugins, and MCP servers #246proposedstatusRelated
Progresses #246. See also #381 (documentation graph — split resolved in sub-decision 13), ADR-014 (workflow-driven tasks — resolver-interface precedent), and the ADR-003 contribution governance model.