Skip to content

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

Open
isadeks wants to merge 18 commits into
mainfrom
fix/703-no-hardcoded-log-delivery-ids
Open

fix(agent): stop hard-coding log-delivery logical ids#705
isadeks wants to merge 18 commits into
mainfrom
fix/703-no-hardcoded-log-delivery-ids

Conversation

@isadeks

@isadeks isadeks commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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.

What this costs, corrected

A stack still on the pre-rename ids must converge once, and that convergence is the fatal rename above. It is not benign and it does not resolve itself: those six resources have to be deleted before the new ones are created, or the update rolls back exactly as account B's did.

An earlier revision of this description called account A's outcome "those six ids converge, once" and prescribed no operator step. That was wrong, and @scottschreckengaust was right to block on it. There is a one-time step, it is documented, and it has now been run against account A — see the evidence below.

The argument against hand-picked stable ids also has to be corrected. I first rejected them because they rename on both accounts while library naming does not — but library naming renames on A too, so that reasoning does not separate the two. The real distinction is how many stacks need the one-time step:

approach stacks needing the step
library naming (this PR) only those on pre-rename ids
hand-picked stable ids every existing stack
re-recorded pin table whichever account the table misses

Library naming is the only one of the three where a stack already on current naming, and every fresh install, needs nothing at all — and the only one with nothing left to keep in sync afterwards.

Migration

docs/design/OBSERVABILITY.md ("AgentCore log delivery") now carries the mechanism and the operator step: how to tell which side of the rename a stack is on, and the delete-then-deploy sequence for stacks that predate it. Fresh installs and stacks already on the library's naming need nothing.

stack state what it needs
fresh install nothing — nothing to collide with
already on library naming nothing — zero churn
on pre-rename ids the one-time step, once

Live verification on account A

Account A is the account still on all six pre-rename ids. Confirmed live before starting, last updated 2026-08-28:

AWS::Logs::DeliverySource
  RuntimeCDKSourceAPPLICATIONLOGS…96A02E02
  RuntimeCDKSourceUSAGELOGS…544FBB22
+ 2 AWS::Logs::Delivery, 2 AWS::Logs::DeliveryDestination

cdk diff first reproduced the reviewer's prediction exactly — all six rename:

[-] RuntimeCDKSourceAPPLICATIONLOGS…96A02E02   destroy
[+] RuntimeApplicationLogsDeliverySource818497BD
… and the same for the other five

Then the documented step, followed by cdk deploy. Result — UPDATE_COMPLETE, exit 0, 621s:

stack event outcome
6 new ids CREATE_COMPLETE 19:02:37–42Z, no AlreadyExists
6 old ids DELETE_COMPLETE 19:03:39–43Z, already-absent
FAILED / ROLLBACK events none

The DELETE_COMPLETE line is the part worth reading twice: CloudFormation deleted the six old logical ids whose underlying resources the migration had already removed, without erroring. That is what makes delete-then-deploy a safe sequence rather than a race.

Log delivery then verified live, not inferred — events landing in the destination log groups after the new delivery was created at 19:02:42Z, when the old delivery no longer existed:

log type delivered events window
APPLICATION_LOGS 2 19:11:43–19:12:16Z
USAGE_LOGS 1438 19:03:20–19:09:19Z

And re-running cdk diff against the migrated stack: no differences, 0 stacks with differences. Account A is now in the zero-churn state this PR claims for every account already on library naming.

One disclosure: the deploy also carried unrelated asset churn — Lambda code hashes, the agent container image, one guardrail version — because no committed tree reproduces that account's deployed assets byte-for-byte. The six delivery resources above were the only log-delivery change in the diff, and nothing else failed.

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 hand-picked-prefix version above — so they reject the wrong fix as well as the original bug.

The logical-id shape assertion is deliberately a canary on the library's current naming, and now says so: when the library renames these resources again, that test fails first, and its comment explains the deploy hazard for already-deployed stacks rather than reading as a string mismatch.

Full CDK suite green: 4492 tests, 212 suites (plus 903 CLI and 1782 agent). No build mutation. main is merged in as of 4e11c65f, which is what cleared the dependency-scan check — the fast-uri advisory it was failing on is a repo-wide pin that #849 fixed on main, not anything this branch introduced. All required checks are green on that commit.

Review points

point resolution
Blocking 1 — no deploy proof deployed to A; step documented
Blocking 2 — dangling doc ref note added; comment stands alone
Nit — canary needs context comment on the assertion

Note for anyone deploying

Both dev accounts are now on the library's naming: B was already there, and A converged in the verification above. main still carries the pin table, so deploying pristine main to either of them now rolls back. Until this merges they need this branch — which is the same bind B was already in, not a new one, and it disappears on merge.

The stack overrode the logical ids of the six 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 of the same name had
diverged, so the table was correct for one and actively caused the rename on the
other.

That 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 stack update rolls back. Renaming can never avoid that collision, because the
conflict is on the ARN the resources point at rather than on their own names.

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

The wrong instinct here was to swap the recorded ids for self-chosen stable ones. I
tried it: it renames on BOTH accounts, so it breaks the account that is already
correct in order to fix the other. Verified by diff before discarding it.

Also removes the earlier `-c pinnedLogDeliveryStack` gate along with the table.
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.

Effect per account, measured with `cdk diff` against live stacks:

  - a stack already on the library's naming: zero churn, the delivery resources are
    not touched at all. This is the account the table was breaking.
  - a stack the table was holding on legacy ids: those six ids converge once. No
    operator migration is prescribed, because nothing is being migrated TO — the
    stack simply stops being held back from what the library already generates.

Tests replace the two that asserted the table's existence: one that no captured id,
account-unique Name, or override survives anywhere in the source, and one that 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.
@isadeks
isadeks requested review from a team as code owners August 3, 2026 23:11

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.jsonnone 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 typesAWS::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.

…migration

Addresses review on #705.

The code comment deferred its entire rationale to "the note in the design
docs", and no such note existed — so the durable reasoning for a decision
that can roll a stack back lived only in a PR and an issue.

Adds it to OBSERVABILITY.md: why the AgentCore library owns these logical
ids, why renaming any of them collides on the unchanged runtime ARN rather
than on their own names, why a table of recorded ids keyed by stack name
cannot describe per-account state, and the one-time operator step for a
stack that still sits on pre-rename ids — including how to tell whether a
given stack needs it.

That step is the part the previous description got wrong. Convergence on
such a stack is the fatal rename, not a benign one-time id change: the old
delivery resources have to be deleted before the new ones are created, or
the update rolls back. Leaving that undocumented handed those operators the
same unexplained mid-update rollback that removing the context flag was
meant to stop.

Also makes the logical-id assertion say out loud that it is a canary on the
library's current naming, so the next rename reads as an operational
warning about already-deployed stacks rather than a string mismatch.
@isadeks

isadeks commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Both blocking issues are addressed, and on the first one you were right — thanks for holding the line on a deploy rather than a cdk diff.

Blocking 1. I confirmed account A is still on all six pre-rename ids (last updated 2026-08-28), and cdk diff reproduced your prediction exactly: all six rename. So the description's claim that this was a benign one-time convergence was wrong, and there was no operator step where there needed to be one.

Rather than argue it, I documented the one-time step and then ran it against account A and deployed. UPDATE_COMPLETE, exit 0: the six new ids created with no AlreadyExists, the six old ids reached DELETE_COMPLETE without erroring even though the migration had already removed the underlying resources, and no FAILED or ROLLBACK events. Log delivery was then verified live — events landing in both destination log groups after the new delivery was created at 19:02:42Z, when the old delivery no longer existed — and a re-run cdk diff against the migrated stack now reports no differences. Timestamps and counts are in the description.

I also corrected the reasoning you flagged as internally inconsistent. Rejecting hand-picked ids because "they rename on both accounts" does not separate them from this fix, since library naming renames on A too. The distinction that actually holds is how many stacks need the one-time step: only pre-rename stacks here, versus every existing stack for hand-picked ids, versus whichever account the table happens to miss for a re-recorded pin.

Blocking 2. The note now exists — docs/design/OBSERVABILITY.md, "AgentCore log delivery" — carrying the mechanism, why a table keyed by stack name cannot describe per-account state, and the migration with a check for which side of the rename a given stack is on. The code comment no longer defers its rationale to a doc that was not there: it stands on its own and cites the doc only for the operator step. I did not add an ADR, since this decides how to deploy a library-owned resource rather than trading a tenet — happy to add one if you read it as architectural.

Nit. The logical-id assertion now says out loud that it is a canary on the library's current naming, and explains the hazard to already-deployed stacks, so the next rename does not read as a string mismatch.

One thing worth flagging rather than burying: the verification deploy also carried unrelated asset churn — Lambda code hashes, the agent container image, one guardrail version — because no committed tree reproduces that account's deployed assets byte-for-byte. The six delivery resources were the only log-delivery change and nothing else failed, but I would rather you know that than read "clean deploy" as "empty diff".

isadeks and others added 5 commits September 1, 2026 09:20
…the deploy path

The pin-table removal makes the first deploy of an affected stack fail
mid-update (DeliverySource AlreadyExists -> full rollback) with no hint of
the cause, and affected operators cannot be enumerated or notified — the
population includes every default-name stack created while the table was in
effect, whose deployments all work fine until that first post-upgrade deploy.

Add a preflight to `mise //cdk:deploy` that reads the stack's own resource
list and, only when delivery resources still carry the retired pinned ids,
deletes exactly those (by the physical ids CloudFormation reports, deliveries
first) so the deploy recreates them under library naming. Scoped by
construction to the stack's six resources — nothing else in the account is
reachable, unlike the account-wide cleanup loops previously documented, which
this replaces. No-ops for fresh installs and migrated stacks; idempotent on
re-run after interruption; --check-only previews, and
ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 skips.

Uses the AWS CLI (same auth as cdk deploy, no new package deps). Tested by
spawning the real script against a fake `aws` that replays canned responses
and records calls: exact delete targets and order, library-id siblings
survive, check-only exits 2, unknown-state and failed-delete abort, skip
makes no AWS calls. DEPLOYMENT_GUIDE gains the upgrade entry under Known
deployment issues; OBSERVABILITY.md now documents the preflight as the
primary path with scoped manual steps as fallback.

Refs #703

Co-Authored-By: Claude Fable 5 <[email protected]>
The preflight task used `npx tsx`. tsx is in neither `package.json`, so npx fetches
it on first use — observed verbatim: "npm warn exec The following package was not
found and will be installed: [email protected]".

That is a pre-existing idiom in this repo (`bootstrap:generate` does the same) and
fine for an occasional codegen task. It is not fine for a task on the deploy path:
every deploy would take an unpinned download, and a deploy with no outbound network
would fail in the preflight rather than in the deploy.

`node --experimental-strip-types` needs no dependency at all, and is what the
`check:*-sync` script tasks already use — and what this script's own tests already
use to spawn it, so the runner now matches between test and deploy.

Verified: the task runs through `mise //cdk:preflight:log-delivery` against a live
stack, exits 0, reports the already-migrated no-op, and pulls nothing from npm.
ayushtr-aws
ayushtr-aws previously approved these changes Sep 1, 2026

@ayushtr-aws ayushtr-aws left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Principal AWS Solutions Architect

Verdict: Approve (with two doc/UX nits). Both blockers from the prior CHANGES_REQUESTED review are now verifiably resolved. This revision no longer merely deletes the stack-name-keyed pin table — it ships the one-time convergence as an automated, scoped, tested, and documented deploy-path migration, plus the live-deploy evidence on the legacy account that Blocking 1 required. Direction and execution are both sound.

1. Vision alignment

Consistent with bounded blast radius and reviewable outcomes (VISION.md). The change removes an unmaintainable per-account literal table that structurally could not describe two accounts, and replaces the earlier "delete-and-hope" with a preflight whose blast radius is bounded by construction: it deletes only physical ids read off list-stack-resources for the target stack, so nothing else in the account — even a shared one — is reachable. No tenet is traded; no ADR required. The prior review's concern (the fix relocated an un-deployable-stack failure from account B to account A rather than eliminating it) is now answered by an in-repo, converge-on-deploy mechanism rather than an undocumented manual step.

2. Prior blockers — independently re-verified against current HEAD

[BLOCKING 1 — RESOLVED] Removing the pin re-introduced the fatal DeliverySource rename on the legacy account; only cdk diff, no deploy proof, no migration.
The reviewer required either (a) a clean deploy to a legacy-naming account, or (b) an account-agnostic fix with the migration and a documented operator step. This revision delivers both:

  • cdk/scripts/preflight-log-delivery.ts (new) reads the stack's own resources and, only when legacy-pinned delivery ids are present (/CDKSource|CdkLogGroup/ and a AWS::Logs::Delivery* type), deletes exactly those — deliveries first, then source, then destination — so the deploy recreates them under library naming. It is wired into mise //cdk:deploy via depends = [..., ":preflight:log-delivery"] (cdk/mise.toml:101), and a non-zero exit aborts the deploy (fail-closed).
  • The operator step and its rationale are now documented (docs/design/OBSERVABILITY.md "AgentCore log delivery", docs/guides/DEPLOYMENT_GUIDE.md "Known deployment issues").
  • The PR body carries the live account-A verification the reviewer asked for: UPDATE_COMPLETE, exit 0, six old ids DELETE_COMPLETE with no AlreadyExists, and log delivery confirmed landing after the new delivery was created.

I cannot re-execute the account-A deploy from a read-only review, but the requirement was to move the convergence from "undocumented and unproven" to "automated, bounded, tested, documented, and evidenced" — and that is met.

[BLOCKING 2 — RESOLVED] Comment deferred rationale to a design-doc note that did not exist.
cdk/src/stacks/agent.ts now points to docs/design/OBSERVABILITY.md ("AgentCore log delivery"), and that section exists (+59 lines) with the full mechanism, the per-account-state argument, and the migration. Doc drift closed.

Prior nit — canary comment context — RESOLVED. cdk/test/stacks/agent.test.ts now carries a full comment on the ^Runtime(Application|Usage)LogsDeliverySource[0-9A-F]{8}$ assertion explaining it is a canary on the library's current naming and pointing back to the migration note.

3. Blocking issues

None.

4. Non-blocking suggestions / nits

  1. Preflight stack-name resolution diverges from the CDK stack-name context (cdk/scripts/preflight-log-delivery.ts:145-148). The preflight resolves its target from --stack-name / STACK_NAME / default backgroundagent-dev, but the CDK app selects its stack via -c stackName context (cdk/src/main.ts:68), and the deploy task runs a bare npx cdk deploy. So mise //cdk:deploy -- -c stackName=foo deploys foo while the preflight inspects backgroundagent-dev. Bounded in practice — only backgroundagent-dev was ever pinned, so custom stacks are a no-op — but in a two-stacks-in-one-account case the preflight could migrate the default stack's legacy delivery resources while the operator is deploying a different stack. Consider deriving the preflight target from the same -c stackName context, or documenting the coupling explicitly.
  2. STACK_NAME=my-stack mise //cdk:deploy doc example (docs/design/OBSERVABILITY.md). STACK_NAME reaches the preflight but not npx cdk deploy (which needs -c stackName), so this example points the preflight at my-stack while the deploy still targets the default. Since custom stacks were never pinned this is harmless today, but the example reads as if it retargets the whole deploy. Minor wording fix.

5. Documentation

Strong. New OBSERVABILITY.md "AgentCore log delivery" section (mechanism, why-no-pin, migration, manual fallback, knobs); DEPLOYMENT_GUIDE.md "Known deployment issues" entry. Starlight mirrors are in syncdocs/src/content/docs/architecture/Observability.md and .../getting-started/Deployment-guide.md carry byte-identical additions (verified). No hand-edited generated files. Issue #703 is filed, carries the approved label — governance (ADR-003) satisfied.

6. Tests & CI

  • New cdk/test/scripts/preflight-log-delivery.test.ts (286 lines) spawns the real script against a fake aws on PATH and asserts the load-bearing behaviors: exact delete targets and ordering, library-id siblings survive, fresh-install/already-migrated no-ops, --check-only → exit 2 with no deletes, idempotent re-run on ResourceNotFoundException, abort (exit 1) on any other delete failure or indeterminate state, and skip makes zero AWS calls. This exercises argument construction and exit codes rather than a re-implementation — the right shape for a gate with a delete side effect.
  • agent.test.ts replaces the two table-asserting tests with source-scan guards (no captured id / account-unique Name / overrideLogicalId survives) and a synth assertion that both log types are still wired ("no pin" cannot silently mean "no delivery").
  • All CI checks green at HEAD (build (agentcore) incl. full CDK suite; CodeQL; secrets/deps/workflow scan).
  • Bootstrap synth-coverage: not applicable. The diff introduces no new CloudFormation resource typesAWS::Logs::Delivery* already existed; only logical-id/name overrides were removed and a client-side CLI-based migration added. No new IAM actions or ARN patterns, so cdk/src/bootstrap/*, the action map, BOOTSTRAP_VERSION, and the golden baseline correctly need no change. (The preflight uses operator credentials via the AWS CLI, not the CFN execution role.)

7. Review agents run

The pr-review-toolkit sub-agents are not registered as invokable agent types in this environment, so I applied each scope by hand:

  • code-reviewer (manual): ran over all 9 files — surfaced the two nits; confirmed both prior blockers cleared.
  • silent-failure-hunter (manual): in scope — the new script has two nosemgrep-annotated returns (null for fresh-install, success on ResourceNotFoundException). Both are documented goal-state signals that main() branches on explicitly, not swallowed errors; every other failure path throws and aborts the deploy. No silent fallback introduced.
  • type-design-analyzer (manual): in scope — new StackResource/DeliveryType types are minimal and local; the DELIVERY_TYPES-keyed Record<DeliveryType, string[]> gives exhaustive per-type CLI arg construction. No concern.
  • comment-analyzer (manual): in scope — the rewritten agent.ts block and the canary comment now match reality (Blocking 2 was exactly this class). Accurate.
  • pr-test-analyzer (manual): in scope — coverage is strong for both failure directions of the gate; see §6.
  • /security-review (manual): applied — the script runs on the deploy path and performs deletes. execFileSync('aws', args[]) (no shell) avoids injection; delete targets are physical ids scoped to the named stack; idempotent and fail-closed; --check-only and ABCA_SKIP_LOG_DELIVERY_PREFLIGHT=1 escape hatches. No IAM/Cedar/network/secret surface changed. The AWS CLI auto-paginates list-stack-resources, so no truncation gap. Deletes touch delivery configuration only, not log data.

8. Human heuristics

  • Proportionality — pass. A one-time migration guard scoped to six resources is proportionate to a guaranteed mid-deploy rollback with no other warning channel; it removes complexity (the table) and adds a bounded, self-disabling path.
  • Coherence — pass. The rationale now lives coherently in-tree (comment → OBSERVABILITY.md → DEPLOYMENT_GUIDE.md), same terms throughout; mirrors regenerated.
  • Clarity — pass with a nit. Names and comments communicate intent well; the only gap is the preflight-vs-cdk stack-name coupling (nit 1) that a future reader could trip on.
  • Appropriateness — pass. The migration is verified against real aws CLI behavior via a spawned fake that replays canned CloudFormation/Logs responses, plus a real account-A deploy in the PR body — not only self-written mocks. Maintainable by this team.

Base-drift note: this HEAD predates the current origin/main tip and touches an active file (cdk/src/stacks/agent.ts); a rebase/merge before landing is advisable, though the branch has been merging main regularly.

Comment thread cdk/scripts/preflight-log-delivery.ts Outdated
…DK app does

Review nits on #705, both rooted in one divergence: the preflight resolved its target
from `--stack-name`/`STACK_NAME` while the app takes its stack from `stackName` CDK
CONTEXT (`src/main.ts`). On a task that DELETES resources, disagreeing about which
stack is being deployed is not a tidiness problem.

The preflight now reads the same sources the app does — `cdk.json` context included —
and accepts CDK's own `-c stackName=` / `--context stackName=` forms for a direct
invocation, with `--stack-name` > `STACK_NAME` > context > default precedence pinned by
a test.

One gap cannot be closed from inside the script, so it is documented rather than
papered over. Verified empirically with a scratch task: arguments after `--` are
appended to the LAST command of the invoked task's `run`, and a mise `depends` task
receives none of them — so `mise //cdk:deploy -- -c stackName=x` cannot reach the
preflight, while env vars DO propagate. A non-default stack therefore needs the name
twice:

  STACK_NAME=x mise //cdk:deploy -- -c stackName=x

which is also the correction to the second nit — the old doc example passed only
`STACK_NAME`, so it read as retargeting the whole deploy when it retargeted only the
preflight. Setting either one alone is precisely how you migrate one stack while
deploying another. Bounded in practice (only `backgroundagent-dev` was ever pinned, so
a custom-named stack has nothing to find), and the one case where it bites — an account
running a legacy-pinned default stack alongside a custom-named one — is now stated in
OBSERVABILITY.md.

To make a mismatch visible before anything is deleted rather than inferable from what
was, the preflight now prints its target and where the name came from on every run:

  log-delivery preflight: inspecting stack 'backgroundagent-dev' (from default)

Three tests added (context forms, precedence, the printed provenance); the nine
existing ones unchanged and passing.
@isadeks

isadeks commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — both nits addressed at c8c9354b, and they turned out to be the same divergence seen from two angles, so I fixed the cause rather than each symptom.

Nit 1 — preflight vs CDK stack-name resolution. Worth fixing on its own terms: this task deletes resources, so disagreeing with the deploy it gates about which stack is being deployed is not a tidiness issue. resolveStackName now reads the same sources the app does, cdk.json context included, and accepts CDK's own -c stackName= / --context stackName= forms for a direct invocation. Precedence is --stack-name > STACK_NAME > context > default, pinned by a test.

One part cannot be closed from inside the script, so I verified rather than assumed it, with a scratch mise task: arguments after -- are appended to the last command of the invoked task's run, and a depends task receives none of them — while env vars do propagate. So mise //cdk:deploy -- -c stackName=x structurally cannot reach a depends preflight. Restructuring the deploy task to close that would mean folding the preflight into the deploy's own run line, which trades a documented coupling for a riskier change to the deploy path itself. I took the option you offered instead — document it explicitly — and made it self-evident at runtime: the preflight now prints its target and the source of that name on every run, so a mismatch shows up in the deploy log before anything is deleted.

log-delivery preflight: inspecting stack 'backgroundagent-dev'
  (from default)

Nit 2 — the STACK_NAME=my-stack mise //cdk:deploy example. Same root cause, and your reading was right: STACK_NAME reached the preflight but not cdk deploy, so the example looked like it retargeted the whole deploy. Corrected to show both, since neither alone is sufficient:

STACK_NAME=my-stack mise //cdk:deploy -- -c stackName=my-stack

I also wrote down why they are not interchangeable and the one case where getting it wrong matters — an account running a legacy-pinned backgroundagent-dev alongside a second custom-named stack, where deploying the second with only -c would migrate the first. Agreed it is bounded (a custom-named stack was never pinned, so the mismatch is otherwise a no-op), but that is the sentence a future reader needs.

Three tests added for the new resolution paths; the nine existing ones are untouched and passing. mise run build green (CDK 4334 / CLI 791 / agent 1755).

On the base-drift note: agreed, and I will merge main before landing rather than now, since main is currently red on an unrelated browserslist advisory (#845 / #844 has the lockfile fix).

@isadeks
isadeks enabled auto-merge September 1, 2026 20:40

@scottschreckengaust scottschreckengaust left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Principal AWS Solutions Architect (re-review @ 69a4faa2)

Verdict: Request changes. The direction is right and the third revision is a big step up — the preflight is scoped by construction, well tested, and the rationale is now in-tree. But the migration is addressed by stack name, which is the same conceptual error this PR exists to remove: the docs declare custom-named stacks immune when the affected predicate is deployed state (pre-#339 library naming), and the repo's own deploy pipeline — which deploys only custom-named stacks — never runs the preflight at all. Blockers 1–3.

0. Prior reviews, re-verified item by item against 69a4faa2

My own review (2026-08-04, 954fe90e, DISMISSED):

  • BLOCKING 1 (pin removal re-introduces the fatal rename on the legacy account; no migration; only cdk diff evidence) — RESOLVED. Not re-raised. cdk/scripts/preflight-log-delivery.ts + cdk/mise.toml:101 ship the convergence on the deploy path, the operator step is documented, and the PR body carries the live legacy-account deploy (UPDATE_COMPLETE, six old ids DELETE_COMPLETE). I cannot re-execute that deploy; the requirement was automated + bounded + tested + documented + evidenced, and that is met.
  • BLOCKING 2 (comment deferred to a design note that did not exist) — RESOLVED. cdk/src/stacks/agent.ts:640 points at docs/design/OBSERVABILITY.md ("AgentCore log delivery"), which this PR adds.
  • Nit (canary needs operational context) — RESOLVED in cdk/test/stacks/agent.test.ts.

@ayushtr-aws (2026-09-01, 780dd94f, DISMISSED):

  • Nit 1 (preflight target diverges from -c stackName) — PARTLY resolved by c8c9354b: the script now parses -c/--context stackName= and prints the resolved target. The divergence on the actual mise //cdk:deploy -- -c stackName=x path is still live and is now documented rather than closed → Blocking 3.
  • Nit 2 (STACK_NAME=…mise //cdk:deploy example) — RESOLVED: the docs now say to set both.

Blockers below are on code and prose added after both reviews, plus one factual claim neither review checked.

1. Vision alignment

Consistent with bounded blast radius and reviewable outcomes. Holding no literals is the correct shape, and I verified the determinism claim at the source rather than taking it on trust: in [email protected], aws-bedrockagentcore/lib/runtime/observability.jsconfigureLoggingDelivery creates ${logTypeId}DeliverySource, ${id}Dest, ${id}Delivery with names from Names.uniqueResourceName(construct). Logical ids and physical names are therefore construct-path-derived, stack-name-scoped and account-independent — stable across synths and identical in every account, exactly as the comment and the new canary regex claim. No tenet traded, no ADR needed. The tenet pressure is on who the migration reaches (B1/B2) and what it may delete (B3).

2. Blocking issues

[BLOCKING 1] docs/guides/DEPLOYMENT_GUIDE.md:222 and docs/design/OBSERVABILITY.md:215 (+ both mirrors, + cdk/mise.toml:82) — "Custom-named stacks are never affected" is false, and scoping by stack name repeats the bug being fixed.

The legacy ids are not a product of the pin table — they are the previous library's naming:

  • cdk/src/stacks/agent.ts:23 imports aws-cdk-lib/aws-bedrockagentcore. git log -S shows that import replaced @aws-cdk/aws-bedrock-agentcore-alpha in 9a58797c (#339, 2026-06-12); the pin table only landed later, in 4357c353 (#695).
  • The alpha produced the RuntimeCDKSource… / RuntimeCdkLogGroup… shapes recorded in the removed table; aws-cdk-lib 2.260.0 produces RuntimeApplicationLogsDeliverySource<hash> etc. (verified above).

So the affected predicate is "the stack's live AWS::Logs::Delivery* logical ids are the pre-#339 shapes" — a property of deployed state, not of the stack's name or of whether the pin table ever applied. Any stack last deployed before 2026-06-12, whatever its name, will hit the identical AlreadyExists rollback on its next deploy. The docs tell those operators they need no action, and the preflight will not even look at their stack (default target backgroundagent-dev) — i.e. the population that gets no warning and no automation is precisely the one this PR exists to protect. Ironically, "was it pinned? → keyed on stack name" is the same substitution of name for state that #703 is about.

Fix: state the predicate as the preflight's own test — "any stack whose live AWS::Logs::Delivery* logical ids contain CDKSource/CdkLogGroup (i.e. last deployed before the alpha→aws-cdk-lib switch in #339)" — delete the "custom-named stacks are never affected" sentence, and tell every operator to run STACK_NAME=<stack> mise //cdk:preflight:log-delivery -- --check-only once regardless of name. Four files (two sources + two Starlight mirrors) plus the cdk/mise.toml task comment.

[BLOCKING 2] cdk/mise.toml:101 / .github/workflows/deploy.yml:262 — the platform's own deploy pipeline bypasses the preflight, and it deploys exactly the custom-named stacks B1 mis-classifies.

The deploy job runs npx cdk deploy --app cdk/cdk.out --all --require-approval never; the preflight is attached only to the mise task, so the pipeline path is unguarded. And build.yml:148-176 names pipeline stacks pr<N>-<compute>, mg<N>-<compute>, <branch>-<compute> — never backgroundagent-dev. The DEPLOYMENT_GUIDE entry sits immediately below the pipeline description and says "Resolution: None needed if you deploy with mise //cdk:deploy", with nothing about the pipeline; no reader will classify a job that installs mise as "deploying without mise". So the one deploy path with a human approval gate is the one with no automatic remedy and no documented manual step.

Fix (either): add a preflight step to deploy.yml before the deploy (the job already installs mise; confirm the OIDC role carries cloudformation:ListStackResources + logs:DeleteDelivery* — the CFN exec role already does, see §5), or run -- --check-only in the read-only diff job so the need surfaces before approval; and in either case say in the docs that the pipeline path is not auto-migrated. If you wire it into CI, note build.yml:225 writes stackName into cdk/cdk.context.json, which the preflight does not read (B3c).

[BLOCKING 3] cdk/scripts/preflight-log-delivery.ts:88-140 — a destructive step resolves its target independently of the deploy it gates; the divergences are documented rather than closed.

  • (a) stack: mise //cdk:deploy -- -c stackName=x — args after -- reach only cdk deploy, so the preflight silently targets backgroundagent-dev. OBSERVABILITY.md:232 acknowledges this deletes the other stack's delivery resources. That stack's template still declares them, so CloudFormation will not recreate them: its agent logging stays dark until someone deploys it. With B1 the two failures compound — deploying a pre-#339 custom stack this way deletes backgroundagent-dev's log delivery and still rolls back.
  • (b) account/region: aws here inherits the ambient profile/region while cdk deploy --profile prod targets another account. Undocumented, same delete-the-wrong-thing outcome.
  • (c) context source: only cdk.json is read — which has no context block in this repo, so that branch is dead — while cdk.context.json, the file the pipeline actually writes stackName into, is ignored.

Fix: have the deploy task hand its own args to the preflight so the two cannot disagree (mise arg templating, e.g. run = ["node --experimental-strip-types scripts/preflight-log-delivery.ts {{arg(name='cdkargs', var=true)}}", "npx cdk deploy {{arg(name='cdkargs', var=true)}}"]); read cdk.context.json too; print account + region (sts get-caller-identity) on the line that already prints stack + source; and fail closed — refuse to delete when deletions are pending, the target resolved from default, and arguments were supplied.

3. Non-blocking suggestions / nits

  1. cdk/scripts/README.md enumerates every script in a table with its mise entry point; the fourth script is missing. Small, but it is the in-repo index for this directory.
  2. cdk/test/scripts/preflight-log-delivery.test.ts:62 — add an AWS::Logs::ResourcePolicy row whose logical id starts CdkLogGroupLogsDeliveryPolicy to PINNED_IDS. The library really creates that resource at stack scope (observability.js, policyId = "CdkLogGroupLogsDeliveryPolicy"), so LEGACY_ID matches it and only the resource-type filter saves it from deletion. Today nothing locks that filter.
  3. Untested branches: ABCA_LOG_DELIVERY_PREFLIGHT=check (only --check-only is covered), the missing-PhysicalResourceId throw, the DELETE_COMPLETE filter, and the cdk.json context resolution source.
  4. New hard dependency on the aws CLI on the deploy path. Verified locally: with no aws on PATH the script exits 1 with spawnSync aws ENOENT followed by "proceeding would fail mid-update with AlreadyExists and roll back" — an assertion about a cause that does not apply. Separate "could not determine state" from "known collision" in the abort text. Also QUICK_START.mdx explicitly serves "users without AWS CLI access", so the header's "required by the deployment guide" overstates the prerequisite.
  5. String((err as {stderr?: unknown}).stderr ?? err) — an empty-string stderr is not nullish, so a signal-killed CLI yields could not list resources of stack 'x': with no cause. Use || and include the exit status.
  6. listStackResources(): StackResource[] | null needs a nosemgrep to explain that null is a signal, not a swallowed failure. A two-variant result ({kind:'absent'} | {kind:'present', resources}) would put the invariant in the type and retire the suppression — worth it given the repo's stance on suppressions (#730).
  7. Doc/output fidelity: OBSERVABILITY.md:236-239 shows the resolution line wrapped across two lines; the script prints one (inspecting stack 'x' (from default)). Operators pattern-match on that block.
  8. cdk/scripts/*.ts is outside tsconfig.json / tsconfig.dev.json include and outside the eslint globs (eslint.config.mjs:49 — whose comment already claims to cover "scripts files"). Pre-existing for the two bootstrap scripts, but a deploy-path script now sits in that blind spot, covered only by its runtime test. Consider adding scripts/**/*.ts to tsconfig.dev.json and the eslint files list.
  9. Partial-legacy state (legacy source, library-named delivery) makes delete-delivery-source conflict → exit 1 with a raw CLI error. Fail-closed is right; a hint in the message would help.

4. Documentation

Good: the new OBSERVABILITY.md "AgentCore log delivery" section is genuinely explanatory (mechanism, why-no-pin, migration, manual fallback, knobs); the DEPLOYMENT_GUIDE.md known-issue entry exists; the Starlight mirrors are byte-identical for the added section (I compared the two files' ## AgentCore log delivery## Deployment safety slices — identical apart from the intentional site-absolute link rewrite), and no generated file was hand-shaped beyond the sync output. Missing/incorrect: B1 (affected population), B2 (pipeline path), nit 1 (cdk/scripts/README.md). Governance: issue #703 exists, is approved, and the work matches its "no opt-in flag / account-agnostic" constraint; branch fix/703-… is compliant. Note the PR itself carries no labels — if the repo's deploy/label conventions matter for landing this, set them.

5. Tests & CI

  • All checks green at head (build (agentcore), CodeQL ×3, secrets/deps/workflow scan, PR title).
  • The new subprocess suite is the right shape for a gate with a delete side effect: it asserts exact delete targets and ordering, that a library-named sibling of the same type in the same stack survives, fresh-install/already-migrated no-ops, --check-only → exit 2 with zero deletes, idempotent ResourceNotFoundException, abort on any other delete failure or indeterminate state, and zero AWS calls when skipped. Gaps in nits 2–3.
  • Bootstrap synth-coverage: not applicable, and I verified why rather than asserting it. No new CFN resource types (AWS::Logs::Delivery* already existed). cdk/src/bootstrap/policies/observability.ts:55-101 grants logs:CreateDelivery / PutDeliverySource / PutDeliveryDestination and DeleteDelivery / DeleteDeliverySource / DeleteDeliveryDestination on resources: ['*'], so both the recreate and the CFN cleanup-phase deletes on the migration path are permitted, and the removal of the pinned Name overrides cannot hit an ARN-pattern gap. resource-action-map.ts:133-135 needs no change; no BOOTSTRAP_VERSION bump.
  • Test performance: no new stack synth, no bundling re-enabled; the new suite spawns ~12 short subprocesses.
  • What I could not run (per constraints, and because node_modules/agent/.venv are per-tree): jest, tsc, cdk synth, mise run build. Those are static reasoning / CI-attested, not verified by me. What I did execute: the preflight itself under Node 22.23 with a fake aws on an isolated PATH — the fresh-install path returns exit 0 after exactly one cloudformation list-stack-resources --stack-name … --output json, and type stripping plus import.meta.url work even though cdk/package.json declares no "type": "module" (ESM syntax detection). The no-CLI case in nit 4 was likewise executed.
  • Domain question asked of this review — does an existing stack see a logical-id change? Yes, by design, for any stack on pre-#339 naming: all six delivery resources rename, which is fatal in place, which is why the migration deletes first. For a stack already on library naming the diff is empty. The derivation itself is stable across synths and identical across accounts (§1), so nothing here is account- or time-varying — the residual risk is entirely in reaching the affected stacks (B1/B2) and not deleting the wrong one (B3).

6. Review agents run

Nested agent dispatch was unavailable in this batch fan-out, so no pr-review-toolkit agent was invoked. I applied each rubric dimension inline, one at a time — read this as rubric-applied-inline, not agent-dispatched:

  • code-reviewer (inline): all 9 files + the surrounding workflows, tsconfig/eslint scope, cdk/scripts conventions → B2, nits 1, 8.
  • silent-failure-hunter (inline): the two tolerated paths (does not existnull; ResourceNotFoundException → success) are documented goal-state signals that main() branches on, and every other path throws and aborts the deploy. Fail-closed direction is correct; residual findings are nits 4–6 (message accuracy and an empty-stderr blank reason).
  • type-design-analyzer (inline): DELIVERY_TYPES/DeliveryType + Record<DeliveryType, string[]> give exhaustive per-type arg construction — good. One concern: nullable sentinel return (nit 6).
  • comment-analyzer (inline): the rewritten agent.ts:615-640 block checks out against the library source. Two inaccuracies elsewhere: cdk/mise.toml:82 ("only backgroundagent-dev was ever pinned", used to imply immunity → B1) and the script header's AWS-CLI-is-required claim (nit 4).
  • pr-test-analyzer (inline): both failure directions of the gate are covered; gaps in nits 2–3, and no test pins the resource-type filter that prevents deleting the library's stack-level AWS::Logs::ResourcePolicy.
  • security review (inline, by hand — the security-review Skill was not invoked): execFileSync('aws', args[]) with no shell → no injection; delete targets are physical ids read from list-stack-resources for one stack, so nothing else in the account is reachable by name; the residual security-relevant issue is authorization-adjacent rather than injection-adjacent — the target stack/account/region is resolved from a different chain than the deploy it gates (B3). No IAM, Cedar, network, secrets, or input-gateway surface changed; the CFN exec-role grants were checked (§5).

7. Human heuristics

  • Proportionality — pass. A self-disabling one-time guard over six resources is proportionate to a guaranteed, unexplained mid-deploy rollback; the net change still removes more machinery (the table) than it adds.
  • Coherence — concern (docs/guides/DEPLOYMENT_GUIDE.md:222, cdk/mise.toml:101). The rationale is coherent in-tree, but the remedy's addressing model (stack name / "was pinned") does not match the failure's actual key (deployed ids), and it is absent from the pipeline path the same guide documents.
  • Clarity — concern (cdk/scripts/preflight-log-delivery.ts:320-331). Naming and comments are unusually clear, but the abort text asserts a specific cause ("would fail mid-update with AlreadyExists") on failures that have nothing to do with it (missing CLI, denied ListStackResources).
  • Appropriateness — pass with a caveat. Verified against real CLI behavior via a spawned fake plus a real legacy-account deploy in the PR body — the right kind of evidence. The caveat is that the scope claim ("custom-named stacks are never affected") was never tested against git history, and it does not hold (B1).

Comment thread docs/guides/DEPLOYMENT_GUIDE.md Outdated

### Log-delivery rename on upgrade (pin-table removal, #703)

**Affects:** `backgroundagent-dev` stacks (the default name) whose last deploy happened while the stack still pinned the log-delivery logical ids — including stacks *created fresh* from those versions. Custom-named stacks are never affected. A currently-working deployment is **not** evidence of being unaffected: the failure only appears on the first deploy after upgrading past the pin removal.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKING 1] "Custom-named stacks are never affected" is not true, and keying the affected population on the stack name repeats the bug this PR fixes.

The legacy ids are the previous library's naming, not something the pin table invented: cdk/src/stacks/agent.ts:23 imports aws-cdk-lib/aws-bedrockagentcore, and git log -S shows that import replaced @aws-cdk/aws-bedrock-agentcore-alpha in 9a58797c (#339, 2026-06-12) — while the pin table only landed later in 4357c353 (#695). In [email protected], aws-bedrockagentcore/lib/runtime/observability.js (configureLoggingDelivery) names the children ${logTypeId}DeliverySource / ${id}Dest / ${id}DeliveryRuntimeApplicationLogsDeliverySource<hash>; the alpha produced the RuntimeCDKSource… / RuntimeCdkLogGroup… shapes the removed table recorded.

So the predicate for "affected" is deployed state — the live AWS::Logs::Delivery* logical ids being the pre-#339 shapes — exactly what the preflight tests, and independent of the stack's name or of whether it was ever pinned. Any stack last deployed before 2026-06-12 is affected identically, and this sentence tells its operator to do nothing while the preflight's default target (backgroundagent-dev) will not look at their stack.

Suggested wording: affected = "any stack whose live AWS::Logs::Delivery* logical ids contain CDKSource/CdkLogGroup (i.e. last deployed before the alpha→aws-cdk-lib switch in #339), whatever its name", plus "run STACK_NAME=<stack> mise //cdk:preflight:log-delivery -- --check-only once for every stack you own". Same edit needed in docs/design/OBSERVABILITY.md:215, both Starlight mirrors, and the cdk/mise.toml:82 task comment.

Comment thread cdk/mise.toml Outdated
# several GB of working space, and uv/Docker caches accumulate across runs.
depends = [":clean:disk"]
# Then the log-delivery preflight (see its task comment above).
depends = [":clean:disk", ":preflight:log-delivery"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKING 2] The preflight is attached only to this mise task, so the repo's own deploy pipeline never runs it — and the pipeline deploys precisely the custom-named stacks the docs declare immune.

.github/workflows/deploy.yml:262 runs npx cdk deploy --app cdk/cdk.out --all --require-approval never; nothing invokes preflight:log-delivery. And .github/workflows/build.yml:148-176 derives pipeline stack names as pr<N>-<compute>, mg<N>-<compute>, <branch>-<compute> — never backgroundagent-dev. Combined with Blocking 1 (name is not the affected predicate), the one deploy path that has a human approval gate is the one with neither the automatic remedy nor a documented manual step: DEPLOYMENT_GUIDE.md:226 says "None needed if you deploy with mise //cdk:deploy", and a job that installs mise will not read as "deploying without mise".

Either add a preflight step to the deploy job (it already installs mise and assumes the deploy role — check that the OIDC role holds cloudformation:ListStackResources + logs:DeleteDelivery*; the CFN exec role already does), or run -- --check-only in the read-only diff job so the need is visible before approval — and say in the docs that the pipeline path is not auto-migrated. If wiring into CI: build.yml:225 writes stackName into cdk/cdk.context.json, which the script does not read.

Comment thread cdk/scripts/preflight-log-delivery.ts Outdated

// Persisted context — what `cdk deploy` would read when no flag is given.
try {
const cfg = JSON.parse(readFileSync(new URL('../cdk.json', import.meta.url), 'utf8')) as {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKING 3c] The wrong persisted-context file. cdk.json has no context block in this repo, so this branch is dead today — while cdk.context.json, which .github/workflows/build.yml:225 actually writes stackName into (and which the CDK CLI also reads), is ignored. A local tree that has a cdk.context.json (from a pipeline-style flow or a context lookup) will therefore resolve backgroundagent-dev and can delete that stack's delivery resources while the operator deploys another.

Read both files (cdk.context.json first, then cdk.json's context), and — because the deleted resources are only recreated by a deploy of the stack they belong to — print the resolved account and region next to the stack name so a mismatch is visible before anything is deleted.

Comment thread docs/design/OBSERVABILITY.md Outdated

**A non-default stack needs the name given twice, and the two are not interchangeable.** `cdk deploy` selects its stack from `stackName` CDK *context*, while the preflight reads `STACK_NAME` (or `--stack-name`, or `stackName` in `cdk.json` context). Arguments after `--` are appended to the `cdk deploy` command and never reach a mise `depends` task, so `-c stackName=x` alone leaves the preflight on its default target — and `STACK_NAME=x` alone leaves the *deploy* on its default target. Setting only one is how you end up migrating one stack while deploying another.

This is bounded rather than dangerous: only `backgroundagent-dev` was ever pinned, so a custom-named stack has nothing for the preflight to find and the mismatch is a no-op. It matters in one case — an account running both a legacy-pinned `backgroundagent-dev` and a second, custom-named stack — where deploying the second with only `-c` would migrate the first. The preflight prints the stack it is inspecting and where that name came from on every run, so check that line if you are unsure:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKING 3a] This safety argument rests on the claim Blocking 1 disproves. "Only backgroundagent-dev was ever pinned, so a custom-named stack has nothing for the preflight to find" conflates pinned with on legacy ids. Legacy ids came from the pre-#339 alpha library, so a custom-named stack last deployed before 2026-06-12 does have matching resources — meaning the -c-only case is not a no-op there, and the compound outcome is the worst one: the preflight deletes backgroundagent-dev's live delivery resources (which CloudFormation will not recreate, since its template still declares them → agent logging dark until that stack is deployed) and the custom-stack deploy still rolls back with AlreadyExists.

Rather than documenting the divergence, close it: pass the deploy task's own args to the preflight via mise arg templating so the two cannot disagree, e.g.

run = [
  "node --experimental-strip-types scripts/preflight-log-delivery.ts {{arg(name='cdkargs', var=true)}}",
  "npx cdk deploy {{arg(name='cdkargs', var=true)}}",
]

The same gap exists for --profile/region (the CLI here uses the ambient profile while cdk deploy --profile prod targets another account) and is not documented at all. Failing closed when deletions are pending, the target came from default, and arguments were supplied would cover all three.

],
};

const PINNED_IDS = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (worth doing): add a row that locks the resource-type filter, not just the id regex. The library creates a stack-scoped AWS::Logs::ResourcePolicy with construct id CdkLogGroupLogsDeliveryPolicy (aws-cdk-lib/aws-bedrockagentcore/lib/runtime/observability.js), so its logical id matches LEGACY_ID and only DELIVERY_TYPES keeps it out of the delete set — on every deploy of every already-migrated stack. Nothing in the suite pins that today.

{
  LogicalResourceId: 'CdkLogGroupLogsDeliveryPolicy1A2B3C4D',
  PhysicalResourceId: 'backgroundagent-dev-policy',
  ResourceType: 'AWS::Logs::ResourcePolicy',
  ResourceStatus: 'UPDATE_COMPLETE',
},

…and assert it never appears in deleteCalls(r). Also uncovered: ABCA_LOG_DELIVERY_PREFLIGHT=check, the missing-PhysicalResourceId throw, and the DELETE_COMPLETE filter.

…stop it targeting a different stack than the deploy

Addresses the three blocking findings. Each was reproduced before fixing.

**B1 — "custom-named stacks are never affected" was false, and scoping by stack name
repeated the bug being fixed.** Verified from history: #339 (`9a58797c`, 2026-06-13)
switched the stack from `@aws-cdk/aws-bedrock-agentcore-alpha` to
`aws-cdk-lib/aws-bedrockagentcore`; the pin table only landed seven weeks later in #695
(`4357c353`, 2026-08-03). The `RuntimeCDKSource…` ids are therefore the PREVIOUS
LIBRARY's naming, already present before any pin existed — so the affected predicate is
"the stack's live delivery logical ids contain `CDKSource`/`CdkLogGroup`", a property of
deployed state. Those ids embed the stack name, so a custom-named pre-#339 stack reads
`RuntimeCDKSourceAPPLICATIONLOGS<custom>Runtime…` and still matches. It is affected, and
the docs told those operators they were not — the population with no warning and no
automation was precisely the one this work exists to protect. The docs and the mise task
comment now state the predicate, drop the immunity claim, and give every operator a
name-independent check command.

**B2 — the platform's own pipeline bypassed the preflight**, and `build.yml` names
pipeline stacks `pr<N>-<compute>` / `mg<N>-<compute>`, i.e. exactly the custom-named
stacks B1 mis-classified. The read-only `diff` job now runs the preflight with
`--check-only` and writes the verdict into the job summary, so a stack needing migration
is visible BEFORE the approval gate instead of surfacing as a rollback after it.

Deliberately detection, not migration, on that path: both jobs assume
`secrets.AWS_ROLE_TO_ASSUME`, whose permissions are not inspectable from here, and a
fail-closed delete step would break every pipeline deploy if that role lacks
`cloudformation:ListStackResources`. `continue-on-error` for the same reason — this is
information for the approver, not a gate on the diff the approval depends on. The guide
now says plainly that the pipeline reports but does not migrate.

**B3 — a destructive step resolved its target independently of the deploy it gates.**
All three divergences closed rather than documented:

- *stack:* the preflight is now the FIRST COMMAND of the `deploy` task instead of a
  `depends`, so `--` arguments reach it and `cdk deploy` alike. `required=false` on the
  var-arg is load-bearing — without it a bare `mise //cdk:deploy` fails with a usage
  error instead of deploying (found by testing the no-args case).
- *account/region:* `--profile` / `--region` are forwarded to its own AWS calls, and the
  resolution line now names the account and region as well as the stack, because a stack
  name alone is ambiguous across accounts and this step deletes.
- *context source:* `cdk.context.json` is read, not just `cdk.json` — the repo's
  `cdk.json` has no `context` block at all, so that branch was dead, while `build.yml`
  writes `stackName` into `cdk.context.json` for every pipeline stack.

Plus a fail-closed guard: if deletions are pending, arguments were supplied, and the
stack name still fell through to the built-in default, it refuses rather than guessing —
the case that would migrate one stack while deploying another and leave the first one's
agent logging dark.

Non-blocking items also addressed: the missing `cdk/scripts/README.md` row; a fixture
row for the library's stack-scoped `AWS::Logs::ResourcePolicy`, which matches `LEGACY_ID`
and is excluded only by the per-type delete loop (mutation-verified: flattening that loop
now fails two tests); tests for `ABCA_LOG_DELIVERY_PREFLIGHT=check`, the missing-physical-id
abort, and the `DELETE_COMPLETE` filter; the abort text no longer asserts an
`AlreadyExists` collision when the real fault was an undeterminable state (a missing AWS
CLI reported a cause that did not apply); and `errText` replaces `stderr ?? err`, which
kept an empty string and produced a bare-colon message with no cause.

Suite: 9 → 21 tests. Six mutations verified to fail: removing the fail-closed guard,
dropping `--profile`/`--region` forwarding, dropping `cdk.context.json`, dropping the
`DELETE_COMPLETE` filter, flattening the per-type delete loop, and reverting the
resolution line. Full build clean: 4367 CDK, 791 CLI, 1775 agent.
@isadeks

isadeks commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

All three blockers verified before fixing, and all three were right. Addressed in 1f02e2e0.

B1 — the immunity claim was false, and I had endorsed it

Confirmed from history, and the dates are the whole argument: #339 (9a58797c) switched to aws-cdk-lib/aws-bedrockagentcore on 2026-06-13; the pin table landed in #695 (4357c353) on 2026-08-03, seven weeks later. So the RuntimeCDKSource… ids were already on deployed stacks before any pin existed — they are the previous library's naming, exactly as you said.

The consequence I had missed: those logical ids embed the stack name, so a custom-named pre-#339 stack reads RuntimeCDKSourceAPPLICATIONLOGS<custom>Runtime… and still matches LEGACY_ID. It is affected, the preflight would find it, and the docs told that operator they were fine while the default target meant it never looked. The docs and the mise task comment now state the predicate as deployed state and give a name-independent check command.

Worth recording that @ayushtr-aws's inline nit carried the same false premise ("Harmless for custom stacks (only backgroundagent-dev was ever pinned)") — two of us accepted it. It took the git archaeology to break.

B2 — pipeline path, with one deliberate limitation

Confirmed, including your detail that build.yml:158 names pipeline stacks pr<N>-<compute> — the very population B1 mis-classified. I had independently found the bypass and filed it as #843 before this review landed, so it is corroborated twice.

Implemented your second option rather than the first: the read-only diff job runs --check-only and writes the verdict into the job summary, visible before the approval gate. Detection, not migration, and the reason is your own caveat — both jobs assume secrets.AWS_ROLE_TO_ASSUME, whose policy I cannot inspect, and a fail-closed delete step would break every pipeline deploy if that role lacks cloudformation:ListStackResources. continue-on-error for the same reason: this informs the approver, it does not gate the diff the approval depends on. The guide now says plainly that the pipeline reports but does not migrate. If you can confirm the role's permissions, flipping it to auto-migrate is a two-line change.

B3 — all three divergences closed, not documented

  • stack: the preflight is now the first command of the deploy task rather than a depends, so -- args reach both it and cdk deploy. One catch worth flagging: your snippet as written breaks the common case — a bare mise //cdk:deploy fails with a usage error unless the var-arg is required=false. Found by testing the no-args path.
  • account/region: --profile/--region forwarded to its own AWS calls; the resolution line now names account and region too.
  • context source: reads cdk.context.json. You were right that the cdk.json branch was dead — I checked expecting to find a context block and there is none.

Plus the fail-closed guard you asked for: deletions pending + arguments supplied + name still from the default ⇒ refuse.

Nits

Done: README row; ABCA_LOG_DELIVERY_PREFLIGHT=check, missing-physical-id, and DELETE_COMPLETE tests; the abort text no longer asserts AlreadyExists when the state was simply undeterminable; errText replaces stderr ?? err.

Nit 2 taught me something. I added your AWS::Logs::ResourcePolicy row and then mutated the resource-type filter to true — the suite stayed green. The reason is that the per-type delete loop re-applies the restriction independently, so the ResourcePolicy has two layers, not one. Mutating the loop instead (for (const r of legacy) — a plausible simplification) fails two tests. So the risk is real but sits in the loop rather than the filter; both are now pinned and the code says why.

Nit 8 investigated and deliberately deferred. The blind spot is real — eslint cdk/scripts/preflight-log-delivery.ts reports "File ignored because no matching configuration was supplied", and your reading of the self-contradictory comment at eslint.config.mjs:47 is correct. But closing it is not a glob addition: type-aware linting needs the file in a tsconfig project, tsconfig.dev.json gives TS1470 because these scripts are ESM (import.meta.url) while the package resolves as CommonJS, and a dedicated tsconfig then hits TS6059 because rootDir is src. That is a shared-config change with its own review surface, on an already-large PR. I got it working and reverted it rather than ship it half-done — happy to do it as a follow-up.

Suite 9 → 21. Six mutations verified to fail. Full build clean: 4367 CDK, 791 CLI, 1775 agent.

Comment thread cdk/scripts/preflight-log-delivery.ts Fixed
… it from the path

CodeQL alert #43 (`js/incomplete-sanitization`, HIGH) on
`cdk/scripts/preflight-log-delivery.ts`: "This replaces only the first occurrence of
'../'". The line built a log label with `file.replace('../', '')`, and a
single-occurrence string replace on a path is precisely the shape that rule looks for —
the signature of broken traversal sanitization.

No sanitization was intended: `file` is one of two literals in the loop's own array, and
the result only ever reaches a log line. But the pattern is indistinguishable from the
real defect at the rule's level of analysis, and carrying the display name alongside the
path removes the construct entirely — cheaper than justifying a HIGH finding, and one
less thing for a reader to check.

The other two Advanced Security surfaces are clean on this branch: no secret-scanning
alerts, no Dependabot alerts. #43 was the only code-scanning alert on the PR head.

Suites unchanged: 4367 CDK, 791 CLI, 1775 agent; the preflight's own 21 still pass.
The two CI failures on this branch were mine, and neither could reproduce locally.

`build.yml` writes `{"stackName":"pr<N>-<compute>"}` into `cdk/cdk.context.json` before
the test job. Reading that file — added so the preflight resolves the stack the CI
pipeline actually deploys — made every test that expects the DEFAULT stack resolve to the
pipeline's stack instead. Locally the file does not exist, so `mise run build` was green
while CI showed two failures.

The first fix was worse: having the harness delete the file made 73 stack-synth tests
fail. That file is CDK's own lookup cache, shared with every suite jest runs in parallel,
and removing it pulled a cached availability-zone lookup out from under a concurrent
synth. Writing to a shared path from a test is the bug, not the specific write.

So the directory is now injectable — `ABCA_PREFLIGHT_CONTEXT_DIR`, honoured only for
this — and the suite points it at the scratch root it already creates per test. The repo's
file is neither read nor written, so the resolution chain is testable and the suite is
hermetic in both environments.

Verified in all three shapes: with a CI-like `cdk.context.json` planted (21 pass, and the
planted file is byte-identical afterwards), with none (21 pass), and full build clean —
4367 CDK, 791 CLI, 1775 agent. Mutation-verified: dropping `cdk.context.json` from the
chain, and ignoring the override, each fail the suite.

NOT fixed here: `Secrets, deps, and workflow scan` is also red, on 8 HIGH `[email protected]`
advisories in `yarn.lock` and `integrations/jira-forge-app/package-lock.json`. That is
pre-existing on `main` — same `^3.1.5` pin, same lockfile version — so it is inherited
through the merge rather than introduced here, and belongs in its own change.
@isadeks

isadeks commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

CI status on 9ef034c5, for the record — the two red checks had different causes.

build (agentcore) was mine, and only reproducible in CI. build.yml writes {"stackName":"pr<N>-<compute>"} into cdk/cdk.context.json before the test job, and the preflight now reads that file, so two tests expecting the default stack resolved to the pipeline's stack. Locally the file does not exist, which is why mise run build was green.

My first fix was worse: having the harness delete that file broke 73 stack-synth tests. It is CDK's own lookup cache, shared with every suite jest runs in parallel, so removing it pulled a cached availability-zone lookup out from under a concurrent synth. The context directory is now injectable and the suite points it at the scratch root it already creates, so the repo file is neither read nor written. Verified with a CI-like file planted (21 pass, file byte-identical afterwards), with none (21 pass), and full build clean.

Secrets, deps, and workflow scan is not from this PR. 8 HIGH [email protected] advisories in yarn.lock and integrations/jira-forge-app/package-lock.json. main carries the identical ^3.1.5 pin and the identical lockfile version, so this branch inherits it through the merge. Fixed separately in #849 (issue #848) rather than folded in here — once that lands, merging main clears this check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agent): log-delivery pin table is keyed by stack name but holds per-account state, so it breaks deploys it was meant to protect

4 participants