fix(agent): stop hard-coding log-delivery logical ids

[isadeks]

Closes #703.

Problem

The stack overrode the logical ids of the six AgentCore log-delivery resources from a table keyed by stack name, holding values read off one account's live stack.

Stack name is not a proxy for deployed state. Two accounts running a stack named backgroundagent-dev had diverged, so the table was correct for one and actively caused the rename on the other:

account	live ids	table wanted	result
A	RuntimeCDKSource…	RuntimeCDKSource…	deploys clean
B	Runtime…818497BD	RuntimeCDKSource…	rollback

The rename is fatal rather than cosmetic. A DeliverySource is unique per (resource ARN, log type) account-wide, and the runtime ARN does not change when the resource is renamed — so CloudFormation's create-before-delete produces a second source for the same runtime, CloudWatch Logs rejects it as already existing, and the whole update rolls back.

Worth stating because it is not intuitive: renaming these resources can never avoid the collision. The conflict is on the ARN they point at, not on their own names.

Fix

Since no set of literals can describe every account's deployed state, this holds none. The resources go back to being named by the Runtime, which generates them from the construct path — deterministically, identically in every account, with nothing to keep in sync and no table to re-record when it drifts.

Also removes the earlier -c pinnedLogDeliveryStack gate. Gating the safe path behind a flag inverted the default: the failure that teaches an operator the flag exists is a mid-update rollback whose message never mentions it.

The wrong fix, and why it is not this one

My first instinct was to swap the recorded ids for self-chosen stable ones (AgentRuntimeApplicationLogsDeliverySource…), so the library's naming could never leak into the template again. I built it, then measured it: it renames on both accounts. It breaks the account that is already correct in order to fix the other, and would have required a migration step for every operator. Discarded on the evidence.

Effect per account

Measured with cdk diff against live stacks, not inferred:

stack state	churn
already on library naming	zero — resources untouched
held on legacy ids by the table	those six ids converge, once

No operator migration is prescribed, because nothing is being migrated to. A stack that was pinned simply stops being held back from what the library already generates; the account the table was breaking sees no change at all.

Tests

Replaces the two tests that asserted the table's existence:

• no captured logical id, account-unique Name, or overrideLogicalId survives anywhere in the source;
• both log types are still wired, so "no pin" cannot silently mean "no delivery".

Both were confirmed to fail against a reintroduced hard-coded override and against the self-chosen-prefix version above — so they reject the wrong fix as well as the original bug.

Full CDK suite green: 3477 tests, 174 suites. No build mutation.

Note for anyone deploying

One account currently runs main plus an uncommitted local edit disabling the table lookup, which is what let it deploy at all. That edit can be dropped once this merges.

[scottschreckengaust]

Review — Principal AWS Solutions Architect

Verdict: Request changes. The direction is right — the stack-name-keyed pin table was structurally incapable of describing per-account deployed state and deleting it is the correct instinct. But by the linked issue's own empirically-confirmed mechanism, removing the pin outright re-introduces the fatal DeliverySource rename on the one account the pin was protecting (account A), and the PR ships no migration and no evidence of a clean deploy (only cdk diff) to that account. That is a deploy-breaking regression swap, not a fix of the class of bug. See blocking issue #1.

1. Vision alignment

Directionally consistent with bounded blast radius and reviewable outcomes (VISION.md): it removes an unmaintainable hand-recorded table and an opt-in flag that inverted the safe default. No tenet is traded, so no ADR is required. However, the bounded blast radius tenet is what the blocking issue below turns on — the change moves an un-deployable-stack failure from account B to account A rather than eliminating it, and the tenet requires we not widen (or here, relocate) blast radius without a documented, evidenced rationale.

2. Blocking issues

[BLOCKING 1] cdk/src/stacks/agent.ts:406 — removing the pin re-introduces the fatal rename on account A; only cdk diff (not a deploy) backs the claim it is safe.

Issue #703 establishes the state as of 2026-07-28:

• Account A is on the pre-rename library naming (RuntimeCDKSourceAPPLICATIONLOGS…96A02E02). main (with the pin) deploys cleanly to A.
• Account B is on the current library naming (RuntimeApplicationLogsDeliverySource…818497BD). main rolls back on B.

With the pin removed, synth now emits the current library naming — exactly what the new test at agent.test.ts:239 asserts (^Runtime(Application|Usage)LogsDeliverySource[0-9A-F]{8}$), i.e. account B's ids. So:

• B: matches live → zero churn (fixed). ✅
• A: RuntimeCDKSource… → RuntimeApplicationLogsDeliverySource… = a rename of all six delivery resources. By the issue's own confirmed mechanism — a DeliverySource is unique per (runtime ARN, log type) account-wide, the runtime ARN is unchanged by the rename, so CFN's create-before-delete creates a second source for the same ARN and CloudWatch Logs rejects it AlreadyExists → whole-stack rollback — account A's next cdk deploy should roll back, precisely as B's did.

The PR body calls this A outcome "those six ids converge, once" and "No operator migration is prescribed." But it also states the effect was "Measured with cdk diff" — and cdk diff reports the logical-id rename, it does not execute the deploy or surface the AlreadyExists. The only deploy evidence in the PR is for B (the account that runs main + a local edit disabling the table). There is no evidence account A deploys cleanly.

This is internally inconsistent with the PR's own "wrong fix" section, which discards the self-chosen-prefix approach because it "renames on both accounts." A rename of A's sources is fatal regardless of the target name — library-generated or self-chosen — because the collision is on the unchanged runtime ARN, not on the new name (the PR body says this itself). So the reasoning that rejects self-chosen prefixes ("renames A → bad") applies equally to this fix ("renames A → also bad"). The ironic upshot: the flag was removed because "the failure that teaches an operator the flag exists is a mid-update rollback whose message never mentions it" — yet this change hands account A exactly that: a mid-update AlreadyExists rollback with nothing in the diff, comment, or docs to explain it.

Required to clear: either (a) demonstrate an actual clean deploy (not cdk diff) to an account currently on the legacy RuntimeCDKSource… naming, or (b) implement one of issue #703's two account-agnostic shapes with the migration the issue explicitly says is needed — its option 2 (own stable ids) is called out as requiring "a one-time migration for each existing state… likely a retain-and-import, or a documented one-off" — and document the operator step for legacy-naming accounts. Deleting the pin without either leaves A un-deployable.

[BLOCKING 2 — documentation] cdk/src/stacks/agent.ts:408 — comment says "see the note in the design docs," but this PR adds no such note and none exists.

The new comment defers the entire rationale to "the note in the design docs," but the diff touches no docs and grep over docs/design/ for DeliverySource / log-delivery / pin finds only unrelated IAM actions in DEPLOYMENT_ROLES.md. On a repo where doc drift is a blocking concern (review Stage 4), the durable reasoning currently lives only in the PR/issue, which are not in-repo guidance. Either drop the doc reference and inline the rationale, or add the referenced design note (and, if it is a genuine architectural decision about not owning library-generated logical ids, an ADR under docs/decisions/).

3. Non-blocking suggestions / nits

• cdk/test/stacks/agent.test.ts:239 — the test hard-codes the library's current logical-id shape. That makes it a canary: the next @aws-cdk/aws-bedrock-agentcore-alpha rename (the exact event that caused #703) fails this assertion. Good that it fails — but the failure message is a naming mismatch, not a warning that deployed stacks will rename their DeliverySources and roll back. Consider a comment on the assertion pointing back to #703 so the next person who hits it understands the operational hazard, not just the string.
• Source-string assertions (agent.test.ts:551-557) — asserting on fs.readFileSync of the source (not.toContain('overrideLogicalId'), regex over the file text) is brittle by nature (a comment mentioning the word would trip it) but is a reasonable regression guard here; no change required.
• pr/* branch convention — branch is fix/703-no-hardcoded-log-delivery-ids, which is compliant; noted only for completeness.

4. Documentation

See Blocking 2. No docs/guides/ or docs/design/ change accompanies a comment that explicitly references a design-doc note. No Starlight mirror concern (no docs/ sources edited). Issue #703 is filed, carries the approved label, and is assigned to the author — governance (ADR-003) satisfied.

5. Tests & CI

• The full CDK suite (build (agentcore), 3477 tests) passes at the head SHA — the two rewritten tests are green.
• The only failing check, "Secrets, deps, and workflow scan," is pre-existing dependency CVEs (cryptography, fast-uri, undici, ip-address) in agent/uv.lock, yarn.lock, and integrations/jira-forge-app/package-lock.json — none of which this 2-file PR touches. Not introduced here; not a reason to block, but it will keep the branch red until a separate deps bump lands.
• Bootstrap policy coverage: not applicable. The diff changes no CloudFormation resource types — AWS::Logs::DeliverySource / Delivery / DeliveryDestination already exist and remain; only their logical ids and the Name property override are removed. No new actions or ARN patterns, so cdk/src/bootstrap/* and the synth-coverage golden baseline correctly need no update.
• Test coverage gap tied to Blocking 1: the tests assert what synth produces (library ids, both log types wired) but not what the code should guarantee (a legacy-naming account can still deploy). That is the un-testable-in-synth failure — which is exactly why deploy evidence for account A is required rather than another source-string assertion.

6. Review agents run

• code-review skill (/review equivalent, high effort — covers code-reviewer + reuse/simplification/altitude + conventions angles): ran over the 2-file diff; surfaced Blocking 1, Blocking 2, and the canary-message nit.
• comment-analyzer: in scope (the large explanatory comment block was rewritten) — ran; found the dangling "design docs" reference (Blocking 2).
• pr-test-analyzer: in scope (both tests rewritten) — ran; assessed the source-string assertions and the library-shape canary, and flagged the deploy-vs-synth coverage gap.
• silent-failure-hunter: effectively N/A — the diff removes the only error-tolerant path (the if (!res) continue; / if (!pins) return; silent skips inside the deleted pinLogDeliveryLogicalIds) and adds no new catch/fallback. Removal of a silent skip is a net improvement; nothing new to hunt.
• type-design-analyzer: omitted — the diff deletes a type (PinnedLogResource) and introduces none.
• /security-review: omitted — no IAM, Cedar policy, network, secrets, or input-gateway surface in the diff; logical-id/name removal has no IAM footprint (log-delivery IAM in DEPLOYMENT_ROLES.md is unchanged).

7. Human heuristics

• Proportionality — pass. A 2-file, net −123-line deletion of an unmaintainable table is proportionate; the fix reduces complexity rather than adding it.
• Coherence — concern (agent.ts:408). The change belongs in cdk/, but the comment references an out-of-repo "design docs" note that does not exist, so the rationale is not coherently discoverable in-tree.
• Clarity — concern (agent.ts:406). The comment and PR narrative present account A's outcome as a benign one-time convergence; by the issue's confirmed mechanism it is a rollback. The naming clearly communicates intent, but the claim about deploy behavior does not match the evidence provided.
• Appropriateness — concern (agent.test.ts:527-557). Integration behavior (does a legacy-naming stack still deploy?) is verified only against synth output and self-written source-string assertions (AI001/AI005), not against real deploy behavior on the affected account — which is where the actual failure lives.