fix(registry): treat a Delete for a never-created registry as a no-op (#866)

[vivibui]

Fixes #866.

Problem

Custom::AgentRegistry's Delete path assumed event.PhysicalResourceId is always a real registry id. A cancelled create breaks that assumption: when a sibling resource fails first, CloudFormation cancels this create before CreateRegistry returns, so no CREATE_FAILED marker is written — yet rollback still issues a Delete, carrying a CFN placeholder rather than a registry id.

DeleteRegistry rejects it:

AgentRegistry5D423F2A  Custom::AgentRegistry  DELETE_FAILED
Received response status [FAILED] from custom resource. Message returned:
1 validation error detected: Value at 'registryId' failed to satisfy constraint:
Member must satisfy regular expression pattern:
(arn:aws(-[^:]+)?:agent-registry:[a-z0-9-]+:[0-9]{12}:registry/)?[a-zA-Z0-9]{12,16}

That ValidationException is neither ResourceNotFoundException (→ absent) nor one of the retryable errors, so requestRegistryDeletion rethrows. The resource sticks in DELETE_FAILED, the nested stack cannot delete, and the parent ends in ROLLBACK_FAILED — recoverable only by hand:

aws cloudformation delete-stack --stack-name <nested> --retain-resources AgentRegistry5D423F2A
# the nested delete must fully drain before the parent will accept a delete
aws cloudformation delete-stack --stack-name backgroundagent-dev

I hit this on a fresh-account deploy where AWS::BedrockAgentCore::Runtime failed first — ServiceLimitExceeded, IAM rate limit while auto-creating the AgentCore service-linked role — cancelling the registry create.

Change

• Add REGISTRY_ID_PATTERN, mirroring the control plane's own registryId constraint, plus an isRegistryId() type guard.

• onEvent Delete: return success without an SDK call when the physical id cannot name a registry. A registry that was never created has nothing to delete, so this is a no-op, not an error.

• isComplete Delete: apply the same guard. The Provider polls isComplete after onEvent, so leaving it unguarded merely moves the identical wedge one step later, into GetRegistry.

• Correct the stale comment. The old code asserted:

  "The CDK Provider wrapper consumes its CREATE_FAILED marker before invoking this handler, so a validation error here is a real defect and must not be treated as an already-absent registry."

  True for a failed create; not for a cancelled one. That comment is what made the old behaviour read as deliberate, so it's replaced with the actual invariant.

Behavioural changes beyond the guard

Two fail-open paths were added while addressing review, and they matter to anyone reading this code later:

• ValidationException is treated as absent on both Delete paths (onEvent's requestRegistryDeletion and the isComplete poll). An id the shape guard admits but the service rejects logs a warning and is treated as already-gone rather than rethrowing into DELETE_FAILED. Delete-only by design — Create and Update stay strictly fail-closed, where a malformed request is a real defect that should fail the resource.
• The registry-id bound is {12,64}, deliberately wider than the service's {12,16}. The pattern is a positive discriminator, so anything it fails to match is skipped as never-created. A tight bound would therefore silently orphan real registries if AWS ever widens the id format, whereas erring wide costs a rejected call and a warning.

Both guards log at warn with the registry name rather than info, because skipping a delete can leave a billable registry orphaned: CloudFormation abandons a cancelled create rather than killing the Lambda, so CreateRegistry may have succeeded with its response going to a ResponseURL nobody reads. That trade — a visible leak over an unrecoverable stack — is intentional, and the log is where an operator meets it.

Testing

• mise run build — clean, 0 task failures, 4,477 cdk + 903 cli tests pass
• npx jest test/handlers/registry-provisioning/index.test.ts — 26 passed (6 new)
• New cases: placeholder / logical-id / empty / undefined physical ids are no-ops on both handlers, and a full registry ARN still deletes normally
• The pre-existing "rethrows an unexpected error from DeleteRegistry" case is untouched and still passes — a genuine failure against a valid id still surfaces rather than being swallowed

Run the build without AWS credentials in the environment — with them present, the unrelated buildApp — AgentCore AZ wiring cases in test/main.test.ts assert the unpinned path and fail because auto-pin resolves real zones.

Notes for reviewers

The guard is deliberately narrow: it only short-circuits when the physical id cannot be a registry id. Any id that could be real still goes to the API, so error handling for genuine registries is unchanged.

Independent of #864/#865 — different file, different failure mode. It does share the same deploy incident, which is where all three were found.

Adjacent, not fixed here: the trigger itself is a fresh-account race where AgentCore's service-linked role creation gets IAM-rate-limited and surfaces as ServiceLimitExceeded, which reads like a quota problem but isn't. aws iam create-service-linked-role --aws-service-name bedrock-agentcore.amazonaws.com fixes it permanently. Possibly worth a QUICK_START note or an AWS::IAM::ServiceLinkedRole in the stack — happy to file that separately if useful.

🤖 Generated with Claude Code

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@b8d049f). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #868   +/-   ##
=======================================
  Coverage        ?   92.48%           
=======================================
  Files           ?      328           
  Lines           ?    95141           
  Branches        ?    10512           
=======================================
  Hits            ?    87993           
  Misses          ?     7148           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vivibui]

@scottschreckengaust @krokoko — review request. (No write access here, so I can't set the Reviewers field; mentioning instead.)

Also cc @kalindiDev and @ayushtr-aws as the authors of registry-provisioning/ — you'll know fastest whether the invariant I'm changing was load-bearing for a case I haven't considered.

The specific thing to challenge: the old code carried a comment asserting the Provider always sanitises PhysicalResourceId before Delete runs. I claim that holds for a failed create but not a cancelled one, and this PR replaces the comment accordingly. If that assumption was protecting something else, this is where it matters.

The guard is intentionally narrow — it short-circuits only when the physical id cannot be a registry id, so anything possibly-real still reaches the API. The pre-existing "rethrows an unexpected error from DeleteRegistry" test is untouched and still passes, which is the evidence that genuine failures aren't being swallowed.

Reproduction is in #866: AWS::BedrockAgentCore::Runtime failed first on a fresh account (IAM rate limit creating the AgentCore service-linked role), which cancelled the registry create and left the stack removable only via --retain-resources.

[isadeks]

1. Verdict

Request changes — two defects the PR itself introduces, both small fixes. To be clear about proportion: the diagnosis is correct, the direction is right, the root-cause analysis in #866 is the best bug report I have read in this repo, and this should land quickly once the two are addressed. Neither is live-breaking today; both are latent. I am still calling them blocking because one of them ships a test that asserts a broken path works, and that is how a latent trap becomes a live one after an unrelated change.

2. Vision alignment

Squarely bounded blast radius, on the operability axis: an unrecoverable ROLLBACK_FAILED requiring --retain-resources surgery is exactly the "control plane must stay recoverable" failure this tenet exists to prevent. Fire-and-forget is untouched (no user-facing path), and the fix is a narrowing, not a widening. No tenet traded, no ADR needed.

The one tension is N-1: the fix makes rollback clean but can leave an orphaned billable registry — trading a loud unrecoverable failure for a quiet resource leak. That is the right trade, but it should be logged and documented rather than silent.

3. Blocking issues

B1. ?? 'registry-never-created' reproduces the exact wedge the PR fixes

cdk/src/handlers/registry-provisioning/index.ts:193

Verified against the real provider-framework source (aws-cdk-lib custom-resources/lib/provider-framework/runtime/framework.js). createResponseEvent hard-validates the Delete path:

const physicalResourceId = onEventResult.PhysicalResourceId || defaultPhysicalResourceId(cfnRequest);
if (cfnRequest.RequestType === "Delete" && physicalResourceId !== cfnRequest.PhysicalResourceId)
  throw new Error(`DELETE: cannot change the physical resource ID ... during deletion`);

So when PhysicalResourceId is absent, returning the invented literal makes the framework throw, safeHandler submits FAILED, and the resource lands in DELETE_FAILED. The fallback converts the benign case into the failure mode this PR exists to eliminate.

Reachable only if CloudFormation omits PhysicalResourceId on a Delete, which it does not — so this is dead defensive code that reads as if it works, and index.test.ts:242's .resolves.toBeDefined() cannot detect it.

Fix: return { PhysicalResourceId: physicalId }; — echo verbatim and let defaultPhysicalResourceId fill in. Never synthesize an id on Delete.

One cleared item, since it is the obvious thing to suspect here: the empty-string case is not a defect. The framework's fallback is ||, so '' resolves back to req.PhysicalResourceId (also ''), the equality check passes, and submitResponse substitutes MISSING_PHYSICAL_ID_MARKER. '' is safe precisely because the framework uses ||; it is the non-empty synthetic fallback that is not.

B2. The pattern accepts a full ARN, neither call site normalizes it, and the new test blesses the broken path

index.ts:81 (pattern), :195 (requestRegistryDeletion(physicalId)), :212 (GetRegistryCommand({ registryId })), index.test.ts:246-254 (the test)

REGISTRY_ID_PATTERN deliberately accepts arn:aws:agent-registry:...:registry/<id>. But registryIdFromArn — which sits six lines above the new code at :74, and which the Create branch already applies at :153 — is not applied on either Delete path. So a full-ARN physical id passes the guard, goes to DeleteRegistryCommand({ registryId: <full ARN> }), and is rejected by the very grammar the pattern mirrors: ValidationException, which is neither absent nor retryable, so it is rethrown at :125 and the resource lands in DELETE_FAILED. The wedge, reached through the branch this PR added.

It is latent — Create has always stored the bare id — but the new test "still deletes when the physical id is a full registry ARN" asserts only toHaveBeenCalledTimes(1) and never inspects the argument, so the suite actively certifies a path that would fail. That is the worst combination: broadened accept-set, no normalization, green test.

Fix: pick one and be consistent — either apply registryIdFromArn(physicalId) before both SDK calls (keeping the original string as the returned PhysicalResourceId, per B1), or drop the ARN alternation from the pattern and delete its test, since nothing this handler writes to PhysicalResourceId is ever an ARN. Either way, assert the argument: expect(mockSend.mock.calls[0][0].input.registryId).toBe('abc123def456').

4. Non-blocking suggestions

N-1. The guard's failure mode is a silent, billable orphan — and the comment asserts the opposite. :84 and :190 claim a non-matching id means "the registry was never created". That is not knowable: CloudFormation cancelling a create does not kill the in-flight Lambda — it stops waiting and marks the resource CREATE_FAILED. CreateRegistry may well have succeeded, with the response going to a ResponseURL CloudFormation now ignores. So in this PR's own target scenario the registry can exist, and the no-op orphans it. Downstream: the Create branch has no ConflictException handling (ConflictException is imported but used only by isRetryableDeleteError), and the next deploy mints a fresh clientToken from a new RequestId (:147), so the orphan surfaces as a duplicate-name failure pointing nowhere near this handler. The trade is still right — a leak beats an unrecoverable stack — but make it visible: logger.warn rather than info in both guards (:207 currently logs nothing at all), include RegistryName, and phrase it as an inference rather than an assertion. A name-based reconcile would actually fix the leak, but ListRegistries is not imported, so that is a follow-up issue rather than a line change.

N-2. Pattern drift orphans real registries. The discriminator is positive ("does this look like a registry id?"), so every unforeseen shape is classified "never created". If AWS widens registryId past 16 characters — as it has historically for other resource ids — a real registry silently survives cdk destroy while CloudFormation reports DELETE_COMPLETE, and both guards skip their API call. Cheap hardening: widen the upper bound ({12,64}) so length drift alone cannot leak, and/or add ValidationException → absent at :116-126 and :215-219.

N-3. On the issue's suggestion #2, which this PR skipped: the reasoning for skipping was sound — a blanket ValidationException → absent would mask genuine malformed-request bugs, which is exactly how B2 would ship unnoticed. But "narrow guard alone" is not strictly better, because the two mechanisms fail in opposite directions: the guard fails open on shape drift (invisible orphan, unrecoverable), the fallback fails open on service-rejected requests (recoverable, and visible if logged at warn). Recommend keeping both, gated to Delete only, and keeping Create/Update strictly fail-closed where a ValidationException genuinely should fail the resource.

N-4. The type predicate narrows the wrong branch. physicalId is string on a string | undefined input means the false branch subtracts string — so inside if (!isRegistryId(physicalId)) at :189-194, physicalId is typed undefined, the ?? at :193 is typed as always taking the right operand, and { physicalId } in the log is typed undefined even though at runtime it is usually a non-empty placeholder. At the second call site the narrowing is vacuous anyway, because the locally-declared IsCompleteRequest.PhysicalResourceId (:54) is required. The predicate conflates "is a string" with "is a well-formed registry id" and buys nothing at either site — a plain boolean return is the right cut.

N-5. Test assertions (see also B1, B2). The test.each at :238 cannot distinguish its four rows — all return truthy objects, so all four pass identically. Give it an expected-id column and assert toEqual. Add regex boundary cases: 11, 12, 16 and 17 characters, plus a short hyphenated id such as AgentReg-1234. Nothing currently pins the lower bound, so {12,16} could become {1,16} with the suite still green. Extend the isComplete guard test to the same matrix as onEvent, which covers four shapes to its one.

N-6. The test.each label "a logical id" over-claims. A bare CFN logical id such as AgentRegistry (13 alphanumeric characters) matches REGISTRY_ID_PATTERN; the chosen fixture is rejected only because its CFN hash suffix pushes it to 21 characters. The guard is length-based, not shape-based, and gives no protection for short logical ids — the label reads as if it does.

N-7. Comment precision, two spots. :183-184 "so for a failed create the id is already sanitised" over-generalises: the marker is written only when the failure surfaced inside onEvent before it returned a physical id. A failure in the waiter/isComplete phase, or a timeout, carries the real registry id instead — benign in consequence, but the stated reason is wrong. And :186-187 "CloudFormation cancels this create before CreateRegistry returns" — CloudFormation abandons rather than cancels; the handler keeps running. That distinction is the entire basis of N-1, so it is worth getting right in the comment.

N-8. Log message shape (:190): a 95-character explanatory sentence where every sibling handler uses a short noun phrase with the detail in the structured payload — this file's own logger.warn at :117 follows that convention. The reasoning already lives in the comment directly above.

5. Documentation

No docs required, and none shipped — correct call. No contract, environment variable, command, or user-visible behavior changes, and docs/src/content/docs/ is untouched, so there is no mirror obligation.

Two optional additions worth considering:

• docs/guides/QUICK_START.mdx:515-524 already has a troubleshooting table with rows of exactly this shape, including a DELETE_FAILED row and a ROLLBACK_COMPLETE row. The --retain-resources recovery for operators who are already wedged is documented nowhere in the repo — only in #866. One row would fix that.
• The adjacent finding in the PR description (AgentCore's service-linked role getting IAM-rate-limited and surfacing as the misleading ServiceLimitExceeded, fixed permanently by aws iam create-service-linked-role --aws-service-name bedrock-agentcore.amazonaws.com) is not filed — I searched, and it exists only inside #866's body. It is a genuinely valuable QUICK_START row and a strong candidate for an AWS::IAM::ServiceLinkedRole in the stack. Please do file it; that offer should not get lost.

Governance flag, for a maintainer rather than the author: #866 has no labels and no assignee. ADR-003:32 is explicit — "An issue is not workable until it is both approved and assigned" — so strictly, this work started on an unapproved issue. #864 and #865 have the same gap, which points at a triage lapse across this batch rather than author error. Someone with permission should apply approved plus a priority label retroactively. The work itself is clearly wanted.

6. Tests and CI

CI: 4/4 green (dead-code detection, secrets/deps/workflow scan, PR title validation, agentcore build). The reported mise run build result (4,477 cdk + 903 cli tests, 26 in the touched suite) is consistent with the diff.

Bootstrap synth-coverage: not applicable, verified. The diff touches only cdk/src/handlers/ and cdk/test/handlers/ — nothing under constructs/ or stacks/ — so no new CloudFormation resource type is synthesized, and no cdk/src/bootstrap/ change, BOOTSTRAP_VERSION bump, or artifact regeneration is owed. The only AWS calls on the new paths (DeleteRegistry, GetRegistry) are already granted on the handler's execution role at cdk/src/constructs/registry.ts:90-104. Separate pre-existing observation: cdk/src/bootstrap/ contains no agent-registry actions at all — orthogonal to this PR, but possibly worth its own issue if deploy-role coverage is expected to span it.

Test-performance rule (#366): not applicable, confirmed. Pure handler unit test; no new App() or Template.fromStack() anywhere under cdk/test/handlers/, and nothing here touches aws:cdk:bundling-stacks.

Coverage: six new cases in the right place, and the pre-existing suite genuinely still covers what matters — index.test.ts:164-173 sends a bare 12-character id through onEvent Delete (which doubles as the lower-bound-accept case), and the isComplete still-deleting, already-gone, conflict, throttle and DELETE_FAILED paths at :309-390 are unshadowed by the new early return. Mutation resistance is decent: inverting the guard or dropping the regex anchors both break existing tests. The weakness is assertion strength rather than case selection — see B1, B2 and N-5.

7. Human heuristics

• Proportionality — pass. One regex, one predicate, two call sites, for a bug with a confirmed unrecoverable-stack consequence. No abstraction invented. If anything it is slightly under-built (N-1's orphan is left unhandled) rather than over-built.
• Coherence — concern. registryIdFromArn sits six lines from code that should compose with it and does not (B2), and the guard duplicates the Create path's ARN-stripping intent without reusing it. Terminology is otherwise consistent with the file.
• Clarity — concern. The new comments are a real improvement on what they replaced, and the central thesis verified out against the framework source. But three claims overstate (N-1, N-7), the log message asserts a conclusion the code cannot support, and the isComplete guard is silent.
• Appropriateness — concern. This was verified against real API behavior rather than mock-shaped assumptions — reproduced on a live fresh-account deploy, with the actual control-plane error quoted, which is how REGISTRY_ID_PATTERN ended up character-for-character identical to the real constraint (I diffed it against the error text in #866; the only deltas are the added anchors and the escaped separator, both correct). That is the right kind of rigor. The gap is on the test side: assertions that pass regardless of the behavior they name, which is precisely what let B1 and B2 through green.

---

Bottom line: correct diagnosis, correct direction, unusually good bug report. Two small fixes before merge — echo the physical id verbatim instead of synthesizing one (B1), and either normalize the ARN or stop accepting it (B2) — plus tighten the two assertions that currently cannot fail. Everything else is optional hardening or a follow-up issue.

[vivibui]

Thanks for this — genuinely one of the most useful reviews I've had. B1 especially: that one would have shipped a fix that defeated itself, and I wouldn't have caught it. Pushed as 7cf186f (follow-up commit, not a force-push, so the delta stays reviewable).

B1 — you're right, and it was the important one

Verified against aws-cdk-lib/custom-resources/lib/provider-framework/runtime/framework.js; your quote is verbatim. Returning 'registry-never-created' when the id is absent makes createResponseEvent throw, safeHandler submits FAILED, and the resource lands in DELETE_FAILED — the very wedge this PR exists to remove. Now echoes the id verbatim, undefined included, and lets defaultPhysicalResourceId fill in.

Your empty-string carve-out is right too, and for exactly the reason you gave: the framework's || resolves '' back to the request's own value. I'd have "fixed" that case unnecessarily without your note.

B2 — fix taken; one clarification on the mechanism

Both Delete paths now apply registryIdFromArn before the SDK call, and the tests assert the argument instead of a call count. Your read on the test was fair — toHaveBeenCalledTimes(1) really was certifying nothing.

One thing I want to check with you though: I don't think a full ARN is actually rejected. The service's own constraint seems to admit the ARN prefix as an explicit alternative:

(arn:aws(-[^:]+)?:agent-registry:[a-z0-9-]+:[0-9]{12}:registry/)?[a-zA-Z0-9]{12,16}

And testing against the live API, a bare id returns ResourceNotFoundException while a full ARN returns AccessDeniedException (cross-account) — neither a ValidationException, so the ARN does appear to get parsed and resolved. If that's right, the normalisation is consistency with what Create stores rather than closing a reachable wedge. Either way I'd rather have it normalised, so the change stands — but I may be missing something about how the service resolves ARNs, so do tell me if you read it differently.

N-7 — and a follow-on that changed a fixture

Chasing your comment-precision point turned up something more interesting than a wording fix: safeHandler short-circuits the marker case entirely.

if (event.RequestType === "Delete" && event.PhysicalResourceId === CREATE_FAILED_PHYSICAL_ID_MARKER) {
  log("ignoring DELETE event caused by a failed CREATE event");
  await submitResponse("SUCCESS", event);
  return;
}

So a create that failed inside onEvent never reaches this handler, and the reachable marker is MISSING_PHYSICAL_ID. The comment says that now.

That also had a knock-on for your N-2 and N-6, which turn out to interact: 'AgentRegistry5D423F2A' was only failing the guard on length, exactly as you said in N-6 — so once I widened the bound per N-2, it started matching and two tests flipped red. The way out was that neither fixture was realistic in the first place. The framework's actual placeholders contain colons, so they fail on charset rather than length, which makes the widened bound safe rather than a trade-off. Fixtures now use the framework's own constants. Nice catch on both counts — I'd have missed the interaction.

The rest, all taken

• N-1 — both guards log at warn with registryName, phrased as inference. Your point that CloudFormation abandons rather than cancels is the crux of it, and it's stated that way now: CreateRegistry may well have succeeded with its response going to a ResponseURL nobody reads, so the no-op can orphan a billable registry. isComplete's guard logged nothing at all before.
• N-2 — bound widened to {12,64}, with the positive-discriminator reasoning recorded.
• N-3 — ValidationException → absent, Delete-only; Create/Update stay fail-closed. Your framing that the two mechanisms fail in opposite directions is what persuaded me, and the comment credits that reasoning.
• N-4 — plain boolean. You're right that the predicate narrowed the false branch to undefined, which is precisely where both callers do their work.
• N-5/N-6 — toEqual on returned ids, bounds pinned at 11/12/16/64/65 plus hyphenated and colon-bearing shapes, isComplete guard brought up to the same matrix, ARN and bare-id argument paths asserted.
• N-8 — short noun phrase, detail in the structured payload.

Documentation

Both rows added to docs/guides/QUICK_START.mdx with the mirror re-synced: the --retain-resources recovery for operators already wedged, and the AgentCore service-linked-role rate limit.

And the SLR finding is filed — #875. Thanks for making a point of it; it would otherwise have stayed buried in a PR description. It tracks the AWS::IAM::ServiceLinkedRole option too, including the wrinkle that CloudFormation won't adopt a pre-existing role, so accounts that have already used AgentCore need different handling.

Verification

• mise run build — exit 0, 0 task failures, 4,486 cdk + 903 cli
• npx jest test/handlers/registry-provisioning/index.test.ts — 35 passed (was 26)

Governance

Agreed on all of it. For whoever picks it up: #864, #865 and #866 are all unlabelled and unassigned, and I don't have triage permission here, so the retroactive approved and priority labels need someone who does.

One more from your review worth tracking separately — your observation that cdk/src/bootstrap/ carries no agent-registry actions at all. Nothing is broken today, since the registry's control-plane calls run under the custom resource's Lambda role, but if deploy-role coverage is meant to span every resource type the stack creates then that's a real gap. It rhymes with #865, where the bootstrap policies turned out not to authorize nested-stack resources either. Happy to file it if you'd like it tracked.

[isadeks]

Round 2 — supersedes my 2026-09-09 "request changes", which is now stale: both blockers are fixed.

1. Verdict

Approve with nits. B1 and B2 are correctly fixed and all eight prior nits were taken — verified against the code, not against the response. N-1 and N-2 below are worth doing before merge (both are mutation-proven test gaps); N-3 and N-4 are wording.

Two corrections to my own last review, since both shaped what you did:

B2's mechanism was wrong. I claimed a full ARN would be "rejected by the very grammar the pattern mirrors", which contradicts itself — the grammar admits the ARN prefix as an explicit alternative. Your live test settles it (bare id → ResourceNotFoundException, ARN → AccessDeniedException, neither a ValidationException), and UpdateRegistryRequest.registryId is documented "(ARN or ID)". The ARN path was never a reachable wedge, so the normalisation is consistency exactly as your comment now says. The test half of B2 was real, though — toHaveBeenCalledTimes(1) certified nothing.

N-2 caused the one remaining code question. Widening the bound was my suggestion, and it is what made AgentRegistry5D423F2A start matching. Your resolution is correct: I ran both framework markers through the widened regex and each returns false, so they fail on charset rather than length. N-3 is the residue of my own advice, not something you introduced.

2. Vision alignment

Bounded blast radius, recoverability axis — unchanged from last round. N-1's ask is met: the orphan trade is now visible at warn with registryName and phrased as an inference, so the leak-beats-unrecoverable-stack decision is recorded where an operator meets it.

3. Blocking issues

None.

4. Nits

N-1. index.ts:247 — the isComplete ARN normalisation is unverified. Deleting registryIdFromArn there leaves the suite at 35/35 green. No test passes an ARN to isComplete; every call uses a bare id, for which it is the identity. The onEvent twin is argument-asserted at index.test.ts:264-275. This is the same shape as the B2 test complaint — normalisation added on two paths, asserted on one. One test closes it.

N-2. index.ts:138, :224, :255 — none of the three new logger.warn calls is asserted. Removing any one of them leaves 35/35 green, and there is no logger spy anywhere in the file. Those warns are the N-1 remedy: they exist to make a billable orphan visible. Unpinned, they can be silently downgraded to info or dropped with CI still green. A spy on the two guard warns is enough.

N-3. Two isComplete comments claim more than the code does. Both are wording fixes; the code change is optional.

• :87-88 — "erring wide costs at most one rejected API call" is not quite true, because :268-272 has no ValidationException branch, so an id that passes the widened guard but the service rejects would rethrow into DELETE_FAILED. I could not construct a reachable input — the framework markers fail on ::, and a CFN RequestId fails on hyphens — so this is a defensive path rather than a live bug. Either soften the claim to the ids we can actually enumerate, or add the two-line branch.
• :249-253 — "Mirror onEvent's guard" is false for undefined, because :247 normalises above the guard and registryIdFromArn(undefined) throws. It needs the declared-required type at :57 violated, so it is theoretical; moving the normalisation below the guard makes the comment true and costs nothing.

Related, and worth a word because it reads as control flow: onEvent:239 discards the DeleteAttempt, so the ValidationException → absent branch is log-only on that path. Every completion decision is made in isComplete.

N-4. The body becomes the commit message on main. This repo squash-merges and preserves bodies verbatim (#870 and #873 both carry their full descriptions in git log). The current body omits both behavioural changes from 7cf186f0: the ValidationException → absent fallback at :137 and the widened {12,64} bound. Your response comment covers them well, but that does not reach git log — someone blaming :137 later gets a commit message that never mentions a fail-open path exists. Two sentences folded in before merge.

5. Documentation

Both suggestions taken, not one — the --retain-resources recovery and the service-linked-role rate limit, in source plus mirror. Mirror verified in sync: mise //docs:sync exits 0 and git status --porcelain docs/ is empty, so CI's mutation check will not fire. The claims match #866 including the nested-then-parent ordering and the "a delete stack operation is already in progress" caveat, and "nested stack" is literally correct (constructs/registry.ts:224 — AgentRegistryStack extends NestedStack). Rendering is fine: three columns like every other row, and the angle-bracket placeholders sit inside inline-code spans so MDX will not parse them as JSX. #875 is real and matches the finding — thank you for filing it rather than letting it stay buried in a PR description.

Governance, still open and still not yours to fix. #864, #865 and #866 remain unlabelled and unassigned. You are right that you lack triage permission — I have it, so say the word and I will apply the retroactive approved plus priority labels to all three.

6. Tests and CI

CI 4/4 green, 35/35 locally, worktree clean.

The prior round's assertion complaint is genuinely fixed, and I verified that by mutation rather than by reading. Killed: both guard inversions, {12,64} to {1,64} and to {12,16} (closing N-5's "could drift to {1,16} with the suite green" at both ends), dropping the onEvent normalisation, deleting the ValidationException branch, and flipping the guard's terminal IsComplete. The only survivors are N-1 and N-2 above.

Bootstrap synth-coverage: not applicable, re-confirmed — the diff touches only cdk/src/handlers/, cdk/test/handlers/ and docs/, so no new CloudFormation resource type is synthesized and no BOOTSTRAP_VERSION bump is owed. The #366 test-performance rule: not applicable — pure handler unit test, no App() or Template.fromStack().

7. Review agents run

The pr-review-toolkit agents are not installed in my session, so I ran two scoped agents covering the same ground: silent-failure-hunter plus comment-analyzer (error handling, the async state machine, and every comment's claims against the framework source), and pr-test-analyzer plus docs (the mutation matrix and mirror-sync check). Omitted /security-review — no IAM, Cedar, network or secrets change; the one validation-shaped addition is the physical-id regex, which I checked myself for catastrophic backtracking. Omitted type-design-analyzer — no new types, and the predicate-to-boolean change was N-4 last round.

In hindsight that fan-out was disproportionate to 31 lines of executable code, and it inflated my first draft of this review. The four nits above are what survived checking each candidate for provenance and reachability.

8. Human heuristics

• Proportionality — pass. One regex, one boolean helper, two guarded call sites, one Delete-only fallback. The response commit added reasoning, not machinery.
• Coherence — now pass (it was a concern last round). registryIdFromArn composes with both new Delete paths.
• Clarity — minor concern. The comments are much improved and most of them verified accurate against the framework source; two overstate, per N-3.
• Appropriateness — pass, and better than last round. You live-tested the ARN behaviour against the real API and used it to correct my review rather than defer to it, chased the comment-precision point into the framework source and found the safeHandler short-circuit, and pushed a follow-up commit instead of force-pushing so the delta stayed reviewable.

Out of scope

All four are pre-existing from #664 (git log -S confirmed they are untouched by this diff), so I am deliberately not loading them onto this PR — happy to file any or all:

• isComplete:288-293 reports UPDATE_COMPLETE on the first READY it sees, but READY is both the pre- and post-update state, so a poll that wins the race against the READY to UPDATING transition reports success with the property change dropped.
• isComplete:289 has no transient-error handling, so one ThrottlingException on a poll becomes CREATE_FAILED, where the Delete path treats the same error as "keep polling".
• res.registryArn! at :181 and :292 asserts on a nullable field; an absent value silently drops the RegistryArn attribute.
• :190 is the one remaining un-normalised SDK call site.

And yes, please do file the cdk/src/bootstrap/ agent-registry gap you offered — worth tracking alongside #865.

[vivibui]

Thanks — and thanks especially for the two self-corrections. Verifying by mutation rather than by reading my response is also the right instinct; it's how you found N-1 and N-2, neither of which I'd have spotted.

All four nits addressed in 1e79d15, plus your merge of main folded in.

N-1, N-2 — closed, and mutation-verified

You were right that both were unpinned. Deleting the isComplete normalisation, downgrading either guard warn to info, or dropping one outright all left 35/35 green.

Added an ARN-argument assertion on the isComplete path (matching the onEvent twin) and a logger spy across all three warns, following the convention already in reconcile-stranded-tasks.test.ts. Then re-ran your mutations to confirm they're actually dead:

mutation	before	after
drop isComplete registryIdFromArn	35/35 green	1 failed
guard warn → info	35/35 green	1 failed
drop onEvent guard warn	35/35 green	1 failed

Baseline 40/40. Your framing that the warns are the N-1 remedy is what made this feel worth pinning rather than cosmetic — an orphan you can't see is the same as an orphan you didn't notice.

N-3 — both fixed, and one turned out to be more than wording

:249-253 — you were right that "mirror onEvent's guard" was false for undefined, and chasing it found a latent throw rather than just a bad comment: normalising above the guard meant registryIdFromArn dereferenced its argument before the guard could run. Moved below, so the comment is now true and the throw is gone. Create/Update normalise separately and stay fail-closed.

:87-88 — took the two-line branch rather than softening the claim. You'd offered either, and adding the ValidationException branch to the isComplete poll makes "costs at most one rejected API call" actually hold, where softening would have documented a weaker guarantee than the code could give. Delete-only, matching the delete path.

Also noted your point that onEvent:239 discards the DeleteAttempt, so that branch is log-only there and every completion decision happens in isComplete. That reads as control flow and I'd missed it — it's part of why the isComplete branch was the one that mattered.

N-4 — good catch on the squash-merge body

I hadn't considered that the body lands verbatim in git log. Added a "Behavioural changes beyond the guard" section covering both omissions: the ValidationException → absent fail-open on both Delete paths (and that Create/Update stay fail-closed), and the deliberately-wide {12,64} bound with the positive-discriminator reasoning. It also records why the warns are warn and not info, so someone blaming :137 later finds the orphan trade in the commit message rather than only in this thread.

Filed, as offered

#878 — the cdk/src/bootstrap/ agent-registry gap. I framed it as a question rather than a defect, since the grants existing on the custom resource's Lambda role at registry.ts:90-104 may well be the intended pattern for a preview service; what's missing either way is the coverage or the rationale. Cross-referenced #865, since both are bootstrap-coverage gaps and may be worth resolving together. It also suggests checking whether synth-coverage.test.ts can be taught to see this class — a Custom:: resource whose real work happens under a different role is precisely what it currently can't.

Happy to file the four out-of-scope items too if you'd like them tracked. The READY race on isComplete:288-293 looks like the sharpest of them — a poll that beats the READY → UPDATING transition reports success with the property change silently dropped.

Labels — yes please

That would be great, thank you: retroactive approved plus priority on #864, #865 and #866. #875 and #878 are also unlabelled if you're applying them in a batch.

Verification

• mise run build — exit 0, 0 task failures, 4,491 cdk + 903 cli
• npx jest test/handlers/registry-provisioning/index.test.ts — 40 passed (was 35)
• mise run security:deps — No issues found (your main merge brought in fix(deps): clear 7 osv advisories blocking the merge queue #870's advisory fixes)
• mise //docs:sync clean, git status --porcelain docs/ empty

One note on history: I saw your merge of main on the branch and merged it into my work rather than force-pushing over it, so d6cc4a2a is a merge commit. If you'd prefer a linear branch before squashing, say so and I'll clean it up — I didn't want to rewrite anything you'd pushed.