fix(cdk): reclaim resource and byte headroom under both CFN ceilings (#852) by scottschreckengaust · Pull Request #854 · aws-samples/sample-autonomous-cloud-coding-agents

scottschreckengaust · 2026-09-03T17:08:22Z

Two configuration changes that pull the stack back inside CloudFormation's two template ceilings, with no architectural change and no API-contract change.

The widest cell of the deploy gate — compute_type=lambda-microvm with the ADR-019 tool gateway on — could not synthesize on main at all: 504 resources against the hard 500-resource template quota, which CDK enforces by throwing TooManyResourcesInStack. The same stack was also at ~100% of the 1 MB template-body ceiling, which CDK only warns about — so that limit was being ridden over silently.

Area

cdk — infrastructure, handlers, constructs
agent — Python runtime / Docker image
cli — bgagent client
docs — guides or design sources (docs/guides/, docs/design/)
tooling — root mise.toml, scripts, CI workflows

Changes

1. allowTestInvoke: false on all 34 LambdaIntegration call sites — −30 resources app-wide.

The default (true) makes CDK emit a second AWS::Lambda::Permission per method, scoped to method.testMethodArn, so the API Gateway console's "TEST" button works. Nothing in this solution invokes it. Each one is also an extra lambda:InvokeFunction grant, so this narrows the IAM surface as well as the resource count.

Real traffic is unaffected. scopePermissionToMethod stays at its default true, so every route keeps its own narrowly-scoped SourceArn. That distinction matters: the two options are alternatives, not additive — setting scopePermissionToMethod: false saves the same 30 resources but widens every SourceArn to .../{TaskApi}/*/*/*, which is why #852 rejects it.

File	Call sites
`cdk/src/constructs/task-api.ts`	20
`cdk/src/constructs/slack-integration.ts`	5
`cdk/src/constructs/registry-api.ts`	4
`cdk/src/constructs/linear-integration.ts`	2
`cdk/src/constructs/jira-integration.ts`	2
`cdk/src/constructs/github-screenshot-integration.ts`	1

2. suppressTemplateIndentation: true on AgentStack (cdk/src/main.ts) — ~31% fewer bytes, 0 resources.

Roughly a third of the emitted template was pretty-print indentation, every byte of which counted against the 1 MB ceiling. CDK's own @aws-cdk/core:Stack.templateSize warning names this prop as the remedy.

Set as a StackProps value rather than via the @aws-cdk/core:suppressTemplateIndentation context key. Both work — Stack resolves props.suppressTemplateIndentation ?? node.tryGetContext(...) ?? false — but the prop keeps the blast radius to the one stack that is actually near the ceiling. Known limitation: the two nested stacks (AgentRegistryStack, RegistryApi) therefore stay pretty-printed. They are small, and only the context key would reach them.

Measured

Resource counts and bytes are from cdk synth (which emits one AWS::CDK::Metadata that Template.fromStack does not), each run into its own --output directory.

Deploy-gate cell	`main`	this PR
default (`agentcore`)	480 res · 961,200 B (96.1%)	450 res · 640,377 B (64.0%)
`compute_type=ecs`	492 res · 996,129 B (99.6%)	462 res · 662,670 B (66.3%)
`lambda-microvm` + `enableToolGateway` — widest	504 res — `TooManyResourcesInStack`, no template emitted	474 res · 687,955 B (68.8%)

Every cell moves −30 resources exactly, and the widest one goes from "cannot synthesize" to 26 resources of headroom under the ceiling. The enableToolGateway-only cell is still measuring and will be added; it is a flat +7 on whichever substrate it is combined with.

CDK checks bytes against TEMPLATE_BODY_MAXIMUM_SIZE = 1e6, not 1,048,576 — percentages above use 1e6.

Tests

Both gaps that let the broken cell stay green in CI are closed:

cdk/test/stacks/agent.test.ts — new AgentStack CloudFormation resource budget (#852) block. It is the first test to construct compute_type=lambda-microvm and enableToolGateway=true together, which is precisely why the 504-resource cell was never red. Template.fromStack throws if the cell is over the hard ceiling, so reaching the assertions is itself part of the guard; the budget is set to 490 so it fails as a readable assertion ten resources before synth starts throwing.
Same block — asserts no AWS::Lambda::Permission referencing test-invoke-stage is emitted anywhere in the stack, naming offending logical IDs. This is what stops a new route from silently reintroducing the 30 resources.
cdk/test/main.test.ts — new buildApp — CloudFormation template-body budget (#852) block, reading the emitted artifact (not Template.fromStack, which has already parsed indentation away). Asserts the template carries no indentation and stays under 800,000 bytes — the same 80% point at which CDK starts warning, so a regression trips here instead of riding the warning band up to the hard limit.

Not in scope

defaultCorsPreflightOptions (task-api.ts) is left exactly as it was. Dropping it would free a further 34 resources, but unlike the two changes here it alters the API contract, so it is a product decision. It is held in reserve as a contingency of roughly the same size as this whole PR.
API Gateway permission method-scoping still has no test asserting it. CRITICAL: escape the CloudFormation 500-resource ceiling — best-practice strategies (8 resources of headroom on main) #852 records that the suite passes with every SourceArn widened to a wildcard. Adding that guard is a separate change; this PR does not widen any SourceArn.
A @aws-cdk/core:stackResourceLimit context budget. The built-in key already exists and is unset; wiring it is worth doing, but it applies one number to all stacks including nested ones, so it needs its own discussion.

Governance

✅ Cleared under ADR-003. #852 now carries approved + P0 and is assigned; this PR is out of draft. (Superseded note: this section previously read that #852 lacked the approved label and an assignee, which was true when the PR was opened as a draft and is no longer.)

⚠️ Branch name still non-conforming. fix/resource-byte-headroom omits the issue number AGENTS.md requires ((feat|fix|chore|docs)/<issue-number>-short-description). Nothing rejects it today — scripts/hooks/check-branch-name.mjs lands with #679, still open as of 2026-09-03 — but it would be rejected once that merges. GitHub cannot re-point an open PR's head ref, so correcting this means close-and-reopen rather than a rename; not doing that unilaterally to an approved, mergeable PR. Raised in review by @ayushtr-aws and recorded on #852.

⬜ Review nits deferred, not dropped. @ayushtr-aws approved with 7 non-blocking nits; all are transcribed on #852 for follow-up PRs. None affects the deployed template — they concern the guards this PR adds over-claiming their coverage (notably RegistryApi extends NestedStack, so its 4 LambdaIntegration sites are outside the test-invoke-stage assertion).

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

🤖 Generated with Claude Code

…852) The widest cell of the deploy gate — `compute_type=lambda-microvm` with the ADR-019 tool gateway on — synthesized 504 resources against CloudFormation's hard 500-resource template quota, so `main` could not emit a template for it at all (`TooManyResourcesInStack`). The same stack sat at ~100% of the 1 MB template-body ceiling, which CDK only *warns* about, so that limit was being ridden over silently. Two configuration changes, no architectural or API-contract change: - `allowTestInvoke: false` on all 34 `LambdaIntegration` call sites (-30 resources app-wide). The default emits a second `AWS::Lambda::Permission` per method scoped to `method.testMethodArn`, purely so the API Gateway console's "TEST" button works. Nothing here invokes it, and each one is an extra `lambda:InvokeFunction` grant. `scopePermissionToMethod` is left at its default `true`, so every route keeps its own narrow `SourceArn` — the two options are alternatives, not additive. - `suppressTemplateIndentation: true` on `AgentStack` (~31% fewer bytes, 0 resources). About a third of the emitted template was pretty-print indentation, all of it counted against the 1 MB ceiling. Set as a StackProp rather than the equivalent context key to keep the blast radius to the one stack near the ceiling; the two nested stacks stay pretty-printed. Measured with `cdk synth` into isolated output directories: default (agentcore) 480 / 961,200 B -> 450 / 640,377 B compute_type=ecs 492 / 996,129 B -> 462 / 662,670 B lambda-microvm + enableToolGateway 504 / THREW -> 474 / 687,955 B Closes neither ceiling permanently — it buys 26 resources of headroom so the structural work can happen under review instead of under a broken build. Tests close both gaps that let the broken cell stay green: the resource-budget block in agent.test.ts is the first to construct `lambda-microvm` and `enableToolGateway` together, and asserts no `test-invoke-stage` permission is ever emitted; main.test.ts asserts the emitted artifact carries no indentation and stays under CDK's own 80% warning threshold. Refs #852 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…s, cover every compute type Review feedback on #854: - `main.ts` / `main.test.ts` / `agent.test.ts`: cut verbose rationale and the drift-prone figures (31% indentation, 30 permissions, 504 resources). Applied the same trim to the `task-api.ts` block, which carried the same numbers. - `agent.test.ts`: derive the budget as `MAX_RESOURCE_BUDGET - CUSHION` instead of hard-coding 490, and name the `cdk synth` delta `SYNTH_ONLY_RESOURCES`. - `agent.test.ts`: the budget block now runs every deploy-gate cell — all three compute types crossed with the tool gateway — rather than only the widest, so a regression confined to one substrate cannot hide behind the others. - `task-api.ts`: the comment named `task-api.test.ts`; the guard is in `test/stacks/agent.test.ts`. Refs #852 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

scottschreckengaust · 2026-09-03T19:40:41Z

CRITICAL: escape the CloudFormation 500-resource ceiling — best-practice strategies (8 resources of headroom on main) #852 is approved and self-assigned to @scottschreckengaust
naming convention adherence honest miss but renaming doesn't add value

ayushtr-aws

Principal architect review — PR #854

Verdict: Approve with nits. The two configuration changes are correct, strictly narrow IAM, and move the widest deploy-gate cell from "cannot synthesize" to 26 resources of headroom. All affected suites pass locally on the merged branch (main.test.ts, stacks/agent.test.ts, constructs/task-api.test.ts, bootstrap/synth-coverage.test.ts — 187/187). The nits below are about the guards the PR adds over-claiming what they cover; none affects the deployed template.

Vision alignment

Fits. Tenet 5 (least privilege): 30 fewer lambda:InvokeFunction grants, every route keeps its per-method SourceArn. Tenet 6 (bounded cost/capacity): converts a fail-open INFO/WARN signal into a red test. Tenet 10 (honest sample): PR body is explicit that this is Tier 0 headroom, not the #852 fix. No tenet traded away, so no ADR needed.

Governance

#852 now carries approved + P0 and is assigned — cleared under ADR-003. The PR body's Governance section is stale; please refresh it.
Branch fix/resource-byte-headroom omits the issue number required by AGENTS.md. Rename to fix/852-resource-byte-headroom (the #679 hook will reject the current name once it lands).

Blocking issues

None.

Non-blocking (recommend fixing before merge, small)

cdk/test/stacks/agent.test.ts:1621 — nested-stack routes are unguarded. Template.fromStack(AgentStack) returns only the root template; RegistryApi extends NestedStack, so its 4 LambdaIntegration sites are not covered by the test-invoke-stage assertion, while the comment in task-api.ts:849-853 says the test "asserts no test-invoke-stage permission is ever emitted". Add the same offenders filter to test/constructs/registry-api.test.ts (or iterate NestedStack children in the budget block) and soften the task-api comment.
cdk/test/stacks/agent.test.ts:1607 — lambda-microvm cells synth without microvm_base_image_arn/microvm_base_image_version, so AWS::Lambda::MicrovmImage and the TerminateMicrovm grant are omitted. The "widest cell" measured is the bootstrap no-image state (474), not the configured deployment (475). The existing microvm fixture at ~L1054 passes those keys for exactly this reason; add them to the microvm cells.
cdk/test/main.test.ts:174 — byte budget measured only on the default cell. buildApp accepts appProps.context; parametrize with the same six-cell matrix so a byte regression confined to ECS/MicroVM/ToolGateway constructs cannot hide. (Branch numbers: agentcore/off ≈ 669 KB guarded, microvm/on ≈ 717 KB unguarded.)
cdk/test/stacks/agent.test.ts:1583 — header comment accuracy. The unchanged describe at ~L1518 already constructs enableToolGateway=true + compute_type=ecs, so "no test had ever constructed [them] together" is not true (the microvm + gateway combination was new). Also CDK's thrown message is "Number of resources in stack … is greater than allowed maximum of 500"; TooManyResourcesInStack is an internal tag a reader will never see in output.
cdk/test/stacks/agent.test.ts:1591 — SYNTH_ONLY_RESOURCES = 1 is a fudge for AWS::CDK::Metadata, which is absent only because the test App never sets aws:cdk:version-reporting. Setting that context key in the budget cells (and optionally @aws-cdk/core:stackResourceLimit, which CDK's maxResources honors with a per-type breakdown) removes the constant.
cdk/src/main.ts:96 — nested stacks stay pretty-printed. Already acknowledged in the PR body; noting that NestedStack does not inherit the prop and cdk.json has no @aws-cdk/core:suppressTemplateIndentation context, so each nested stack has to rediscover this. Fine to defer to the #852 stack-split work.
cdk/test/main.test.ts:166 — the hand-copied 1_000_000 × 0.8 duplicates CDK's own Stack.templateSize warning; Annotations.fromStack(stack).hasNoWarning('*', Match.stringLikeRegexp('Template size')) would reuse CDK's signal and cover nested stacks for free. Keep the raw read only for the indentation assertion.

Documentation

No docs/guides/docs/design change was required (no contract, env var, or command change), so no Starlight mirror regen needed.
Suggest one bullet under cdk/AGENTS.md "Common mistakes": new LambdaIntegration sites must pass allowTestInvoke: false and new routes count against the resource-budget test. Optional: one operator-facing line that the API Gateway console "TEST" button no longer works by design.
Issue tracking: #852 (P0, approved) is the tracking issue; #851 is the incident record.

Tests & CI

Locally green (see above). CI checks were still queued at review time.
Bootstrap synth-coverage: not applicable — no new CFN resource types; the PR only removes AWS::Lambda::Permission resources. Suite still passes.
Test performance: budget block synths six cells once each in beforeAll (no per-test synth, bundling stays disabled) — ~11 s for the two files. Acceptable.

Review agents run

/code-review (high) — source of nits 1–5 and 7; each spot-verified against the branch.
/security-review — no new vulnerabilities. Verified from aws-cdk-lib 2.261.0 source that allowTestInvoke:false only drops the test-invoke-stage permission (scopePermissionToMethod branch unchanged, per-method SourceArn retained) and that suppressTemplateIndentation only changes JSON.stringify indent.
Omitted: silent-failure-hunter (no error-handling code in diff), type-design-analyzer (no new types), comment-analyzer (folded into code-review nit 4), pr-test-analyzer (folded into code-review nits 1–3).

Human heuristics

Proportionality — pass. Two flags plus two tests for a P0 blocker; no new abstraction.
Coherence — pass. Same option at all 34 call sites; budget constants named consistently.
Clarity — concern: task-api.ts:849-853 and agent.test.ts:1583 claim coverage the tests do not provide (nits 1, 4).
Appropriateness — pass. Verified against real CDK behaviour (synth throws, permission ARNs), not only mocks; tests assert what the template should contain.

scottschreckengaust · 2026-09-03T20:01:57Z

@ayushtr-aws — thank you, genuinely, for the thoroughness here. Verifying allowTestInvoke: false against aws-cdk-lib 2.261.0 source to confirm the scopePermissionToMethod branch and per-method SourceArn are untouched is exactly the check that made this safe to approve, and the seven nits landed on the real weakness of the PR: the guards claim more coverage than they provide.

All of it has been recorded on the tracking issue — #852 (comment) — to be evaluated and remediated in follow-up PRs, grouped as:

Guard accuracy — the nested-stack blind spot (nit 1: RegistryApi extends NestedStack, so Template.fromStack(AgentStack) never sees its 4 LambdaIntegration sites) plus the two inaccurate comments (nit 4), and the cdk/AGENTS.md "Common mistakes" bullet you suggested. Nit 1 is the one I'd rank highest of the seven: a comment in task-api.ts asserting coverage that does not exist is precisely how the next contributor adds an unguarded route. The AGENTS.md bullet is the durable half of that fix.
Budget-cell fidelity — the missing microvm_base_image_arn/microvm_base_image_version context (nit 2, which corrects the widest-cell figure from the bootstrap no-image 474 to the configured 475), the SYNTH_ONLY_RESOURCES fudge (nit 5 — and @aws-cdk/core:stackResourceLimit's per-type breakdown is directly useful to CRITICAL: escape the CloudFormation 500-resource ceiling — best-practice strategies (8 resources of headroom on main) #852's resource accounting), and the six-cell byte-budget parametrization (nits 3 and 7 together).
With the stack split — @aws-cdk/core:suppressTemplateIndentation in cdk.json for nested stacks (nit 6), where your Annotations.fromStack(...).hasNoWarning('*', /Template size/) suggestion is what makes the nested-stack case testable at all rather than a second hand-copied threshold.

Two governance points from your review are not deferrable and are noted separately on #852:

The stale Governance section in the PR body — correcting that now.
The branch name fix/resource-byte-headroom missing the issue number. GitHub cannot re-point an open PR's head ref, so this is a close-and-reopen rather than a rename; leaving that call to @scottschreckengaust rather than churning an approved, mergeable PR unilaterally. Worth noting for anyone reading later: the feat(governance): ADR-003 enforcement hooks — commit-msg, branch-naming, pickup-issue skill (#186) #679 hook that would reject this name is still open, not merged, so it is not yet enforcing.

On the TooManyResourcesInStack point — correct, and worth more than a comment fix: that string is an internal identifier, so anyone grepping deploy output for it finds nothing. The message a human actually sees is Number of resources in stack … is greater than allowed maximum of 500.

…base Rebasing onto main (12c9b63) surfaced three collisions with work that merged after this branch was authored. None were textual conflicts — git merged the affected files cleanly and produced a build that failed. 1. `allowTestInvoke: false` on the DELETE integration. #854 stripped the API Gateway console test-invoke Lambda permissions to reclaim CloudFormation resources under the 500-resource ceiling, and added a guard asserting none are emitted. This route predates that convention, so it re-introduced one and failed the guard 6x (once per compute_type x enableToolGateway variant). The two sibling routes in this same construct already pass the option; now all three agree. 2. Attributed-Lambda count 46 -> 47. The original a19f16d bumped 45 -> 46 for RemoveWorkspaceFn, but main independently reached 46, so `git rebase` dropped the commit as "patch contents already upstream" — identical text, different reason. The textual change survived; the intent did not. With this branch's extra Lambda the correct value is 47. 3. Solution user agent on the new handler's SDK clients. linear-remove-workspace.ts constructed `new DynamoDBClient({})` and `new SecretsManagerClient({})` directly, which drops the solution user agent (#319). Now built through makeDocClient()/makeClient(), matching linear-link.ts and linear-webhook.ts. Also resolved the LINEAR_SETUP_GUIDE.md conflict from #831: kept its new "Vault-managed workspaces" section and dropped the duplicated webhook/ uninstall sentence, which this branch had already relocated into the parent "Removing a workspace" section. Starlight mirror regenerated. Refs #306

@isadeks

…ed resolver (aws-samples#306) (aws-samples#681) * feat(cli): bgagent linear remove-workspace + DELETE route + fail-closed resolver (aws-samples#306) Add a `bgagent linear remove-workspace <slug>` command that deregisters a Linear workspace, replacing the manual DDB + Secrets Manager surgery that removal previously required. CLI (cli/src/commands/linear.ts): new subcommand mirroring add-workspace / update-webhook-secret UX — slug validation + a "type the slug to confirm" prompt (skipped by --yes). Delegates all writes to the backend via a new DELETE call so DDB/Secrets Manager grants stay on the API role, not on every CLI user (same pattern as `link`). Flags: --purge, --keep-mappings, --yes. Backend: new flat handler cdk/src/handlers/linear-remove-workspace.ts behind DELETE /v1/linear/workspaces/{slug} (route + Lambda wired in cdk/src/constructs/linear-integration.ts). Cognito-authenticated, admin-only (caller must match the recorded installed_by_platform_user_id). Default is a SOFT removal: flip the registry row to status=revoked (audit trail preserved) and delete the bgagent-linear-oauth-<slug> secret; --purge deletes the row outright. Secret deletion is idempotent (ResourceNotFoundException swallowed, other SM errors rethrown). Optional project-mapping cleanup keyed on linear_workspace_id. Fail-closed resolver: the OAuth resolver already rejects any non-active registry status (cdk/src/handlers/shared/linear-oauth-resolver.ts) — so a revoked workspace can no longer resolve a token or route webhooks the instant this returns. Added an adversarial test proving a revoked slug is rejected WITHOUT ever reading the secret, plus an active-control case. Docs: rewrote the "Removing a workspace" section of the Linear setup guide (Starlight mirror regenerated) to lead with the command and keep the manual DDB steps as a fallback. Security: no secrets logged (slug/workspace_id/booleans only); reuses existing AWS SDK clients; no new dependencies. SAST clean on new files. Closes aws-samples#306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(aws-samples#306): loud+recoverable partial teardown, phase logging, coverage gaps Review follow-up (self-review agents on PR aws-samples#681): - Secret-delete failure is no longer masked: a non-idempotent SM error (e.g. AccessDenied) after the row is revoked now returns a distinct 500 SECRET_DELETE_FAILED and writes a durable secret_deletion_failed / orphaned_oauth_secret_arn marker on the registry row, so the leaked credential is discoverable (a naive retry 404s at the active-scan and would never re-attempt the delete). ResourceNotFoundException stays idempotent (200). - Top-level catch + mapping-cleanup loop now log the failing phase + workspace id / per-page progress so on-call can locate an orphaned secret or half-cleaned mapping table from the request id. - Tests: SECRET_DELETE_FAILED path + marker write; FilterExpression pins the status='active' fail-closed filter to the handler (not the mock); paginated mapping cleanup across LastEvaluatedKey; missing oauth_secret_arn skip; CLI confirmation-prompt abort/proceed; secret_deleted:false CLI output; api-client query-string mapping (purge / keep_mappings snake_case). Relates to aws-samples#306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(aws-samples#306): clarify remove-workspace Lambda timeout rationale Comment-only: the 30s timeout is higher than the 3s default the link/webhook handlers use, not "higher than the other request handlers" (the webhook processor also uses 30s). Nit from PR aws-samples#681 self-review. Relates to aws-samples#306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(aws-samples#306): paginate registry scan (fix 404 on live workspaces) + purge revoke-first + drop dead mapping cleanup B1 (blocking): the registry lookup used `Limit: 1` on a FILTERED Scan. DynamoDB applies FilterExpression after evaluating `Limit` items, so a single-row limit examined one arbitrary row, filtered it out, and 404'd a live workspace whenever the registry held >1 row (the normal shared-stack state; guaranteed after the first soft-revoke). Drop `Limit` and paginate to completion, matching jira-webhook-processor.ts / shared/linear-issue-lookup.ts; document the small-table assumption. Fix the test double so the registry-scan router honors Limit/ExclusiveStartKey and applies the filter to the examined slice (this is why B1 was invisible), and add a two-active-row regression with the target on the second page (fails before, passes after) plus a follow-LastEvaluatedKey assertion. B2 (blocking): mapping cleanup was a provable no-op reported as success — LinearProjectMappingTable rows carry no workspace id (onboard-project writes none), so the `linear_workspace_id` filter matched zero rows always while the CLI printed "✓ 0 project mapping(s) removed". Take the reviewer's cheapest honest fix: remove the mapping-cleanup path entirely — deleteWorkspaceProjectMappings + its call, the `--keep-mappings` flag, the projectMappingTable.grantReadWriteData grant, the LINEAR_PROJECT_MAPPING_TABLE_NAME env, and the 30s timeout bump (reverted to 10s matching siblings — N3). CLI output + LINEAR_SETUP_GUIDE no longer claim mapping cleanup; mappings are removed by project id. Schema follow-up (record linear_workspace_id at onboard time) to be filed separately. B3 (blocking): on `--purge`, markSecretDeletionFailed early-returned, so a failed DeleteSecret after the row was deleted leaked the OAuth secret with no durable record. Reorder to revoke(Update)→DeleteSecret→delete-row(purge only, after secret confirmed gone), so the marker always lands and fail-closed holds. Drop the `purged` special-case; correct the two backwards comments (:249-251, :283-285). Add a --purge marker regression + an ordering assertion. N2: pin the previously-vacuous construct tests — resolve RemoveWorkspaceFn via its unique DeleteSecret role grant, pin the DELETE method to the {slug} resource, and assert the DeleteSecret grant is bound to that role AND scoped to bgagent-linear-oauth-*. Closes aws-samples#306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * test(aws-samples#306): de-tautologize role-binding assertion + fix pagination/test comments + runbook markers (aws-samples#306) Addresses the 4 non-blocking approval nits from @isadeks: 1. Role-binding assertion (cdk/test/constructs/linear-integration.test.ts): findRemoveWorkspaceFn() now derives the remove-workspace role INDEPENDENTLY from the FUNCTION resource (its registry-only Environment signature + Role Fn::GetAtt), not from the DeleteSecret policy. The secret-prefix test asserts the DeleteSecret grant lands on THAT role, so mis-wiring the grant onto another role now fails the test (proven: moving the grant to linkFn makes `expect(roleRefs).toContain(role)` fail). No longer a tautology. 2. Pagination comment (cdk/src/constructs/linear-integration.ts): the bounded sequence now names the lookup phase (registry lookup → revoke → secret delete → optional row purge) and states the lookup scan is the only paginating phase, bounded by the registry's tens-of-rows scale. 3. Test prose (cdk/test/handlers/linear-remove-workspace.test.ts): :270 comment now says "Page 1: empty (no matching row) + a continuation key" to match `Items: []`, and the :262-265 preamble no longer describes stale routeDdb Limit behavior. Comment-only; test logic unchanged. 4. Runbook markers (docs/guides/LINEAR_SETUP_GUIDE.md + regenerated Starlight mirror): the manual-fallback section now names the durable markers (secret_deletion_failed / secret_deletion_error / orphaned_oauth_secret_arn) and the SECRET_DELETE_FAILED error code, pointing operators to the delete-secret fallback. Closes aws-samples#306 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * fix(aws-samples#306): reconcile DELETE route with post-aws-samples#854 conventions after rebase Rebasing onto main (12c9b63) surfaced three collisions with work that merged after this branch was authored. None were textual conflicts — git merged the affected files cleanly and produced a build that failed. 1. `allowTestInvoke: false` on the DELETE integration. aws-samples#854 stripped the API Gateway console test-invoke Lambda permissions to reclaim CloudFormation resources under the 500-resource ceiling, and added a guard asserting none are emitted. This route predates that convention, so it re-introduced one and failed the guard 6x (once per compute_type x enableToolGateway variant). The two sibling routes in this same construct already pass the option; now all three agree. 2. Attributed-Lambda count 46 -> 47. The original a19f16d bumped 45 -> 46 for RemoveWorkspaceFn, but main independently reached 46, so `git rebase` dropped the commit as "patch contents already upstream" — identical text, different reason. The textual change survived; the intent did not. With this branch's extra Lambda the correct value is 47. 3. Solution user agent on the new handler's SDK clients. linear-remove-workspace.ts constructed `new DynamoDBClient({})` and `new SecretsManagerClient({})` directly, which drops the solution user agent (aws-samples#319). Now built through makeDocClient()/makeClient(), matching linear-link.ts and linear-webhook.ts. Also resolved the LINEAR_SETUP_GUIDE.md conflict from aws-samples#831: kept its new "Vault-managed workspaces" section and dropped the duplicated webhook/ uninstall sentence, which this branch had already relocated into the parent "Removing a workspace" section. Starlight mirror regenerated. Refs aws-samples#306 * fix(aws-samples#306): report vault-managed teardown as incomplete + settle the revoke race (aws-samples#681 B1, N1-N7, N10-N12) Addresses PR aws-samples#681 review feedback on `DELETE /v1/linear/workspaces/{slug}`. B1 (blocking) — the registry lookup could miss an existing workspace. `ScanCommand` was issued with `Limit: 1` plus a `FilterExpression` on `workspace_slug`. DynamoDB applies `Limit` to items *examined*, not items matched, so a filtered scan can legitimately return an empty `Items` array together with a `LastEvaluatedKey` while the target row sits a page deeper. On any table with more than one row the handler therefore 404'd on workspaces that existed. The scan now pages via `ExclusiveStartKey` until the row is found or the keyspace is exhausted, capped at `MAX_SCAN_PAGES` (20) so a pathological table cannot pin the Lambda until timeout — the cap is a 500, not a silent 404, because "we gave up looking" is not "it is not there". `ConsistentRead: true` was added so a removal issued straight after a `linear setup` reads its own write. The paging fix widens, but does not close, a TOCTOU: two concurrent DELETEs could both find the same `active` row and both report success. The revoke `UpdateCommand` now carries `ConditionExpression: '#status = :active'`, so exactly one caller wins; the loser's `ConditionalCheckFailedException` maps to 404 `WORKSPACE_NOT_FOUND` and, critically, does *not* proceed to delete the OAuth secret out from under the winner. N1/N2 — `secret_deleted: boolean` becomes `secret: 'deleted' | 'absent' | 'not_applicable'`. A boolean conflated two very different outcomes: "there was a secret and it is gone now" and "there was never a Secrets Manager secret because this workspace is vault-managed". The latter means teardown is *not* finished — an AgentCore OAuth2 credential provider survives outside CloudFormation, still holding the Linear client secret and a live, self-refreshing grant, and `cdk destroy` will not remove it. The response now echoes `provider_name` for those rows and the CLI prints the exact `aws bedrock-agentcore-control delete-oauth2-credential-provider` follow-up. `not_applicable` is deliberately narrow (`providerName && !oauthSecretArn`): `bgagent linear setup` writes `oauth_secret_arn` unconditionally, so a vault row that also carries an ARN really did have a secret and reports `absent`. N3 — the "removes everything" claims in the CLI prompt and the setup guide were wrong in the vault case. Both now say what is *not* removed, and the pre-confirmation prompt warns before the destructive action rather than only disclosing it afterwards. N7 — the secret delete falls back to the deterministic `bgagent-linear-oauth-<slug>` name when the row records no `oauth_secret_arn`, so a partially-written row does not orphan its secret. `secretsmanager:DeleteSecret` is granted over that name prefix (`linear-integration.ts`), and `SecretId` accepts a name or an ARN, so the by-name call is permitted. The prefix is verified identical in all four of its co-definitions. N11/N12 — `revoked_reason` is now `admin_removed`, not `vault_consent_required`. That distinction is load-bearing: `vault_consent_required` is the one revoked reason the OAuth resolver re-probes instead of refusing, so reusing it here would let a later successful vault probe un-latch a workspace an operator deliberately removed. The vocabulary lives in `LinearRevocationReason` (exported from `shared/linear-oauth-resolver.ts` as a **type only**) and the writer declares its own constant. `import type` is erased before esbuild, so the removal handler takes no runtime dependency on the resolver — a value import would pull SNS alerting, the resolver's DDB/Secrets Manager clients and the token-refresh path into this Lambda's bundle, and would land the handler in the `agent.test.ts` minting-handler census whose entire value is that such an import is a test failure rather than a production 401. N4/N5/N6/N10 — `LINEAR_WORKSPACE_REGISTRY_TABLE_NAME` is read once at module scope and validated in-handler, so a misconfigured deployment 500s with a named cause instead of an opaque SDK error; the response body is typed by a module-local interface applied with `satisfies`; the 404-on-lost-race path logs a WARN naming `oauth_secret_arn`; the 403 wording no longer implies the workspace exists. Not included, per review scope: N8 (409 on duplicate active rows for one slug — currently first-match-wins) and N9 (collapsing the 403 existence oracle to 404). Both change API semantics and belong in their own issues rather than a bugfix PR. N9's wording half is done here. Tests: 24 in the handler suite (by-name delete, `not_applicable` + `provider_name` echo, vault-row-with-ARN => `absent`, lost race => 404 with no secret delete, non-conditional update failure => 500, page cap => 500 after exactly 20 scans, `--purge` delete failure => 500 with the revoke landed, marker-write failure still surfacing `SECRET_DELETE_FAILED`, and a missing-table-name case in its own module registry). Full suites green: cdk 4579/4579 (216 suites), cli 941/941 (63 suites), docs 77 pages, drift-prevention clean, jira-forge-app 11/11. The agent pytest step of `//agent:quality` was NOT run: this diff contains no Python, and that suite writes stray commits when run from a worktree lacking the aws-samples#856 git-config isolation. `//agent:lint` and `//agent:typecheck` were run instead, both clean. Refs aws-samples#306, aws-samples#681. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(aws-samples#306): correct the review-item numbering in 253de07's commit message Empty commit. No code, test, or docs change — this exists only to correct the record, because `253de07f`'s message cannot be amended without force-pushing a shared PR branch. `253de07f` labels its sections with a B1/N-numbering that does NOT correspond to review 5181793802 on PR aws-samples#681. Every change it describes is a change that was actually made, and the descriptions are accurate; only the labels are wrong. Read the labels below, not the ones in that message. Root of the confusion: PR aws-samples#681 has had two "B1"s. July's round 4807516158 had B1 = the `Limit: 1` filtered-Scan bug, fixed back then in `0d006b04`. The current round 5181793802 has B1 = vault-managed teardown reported as complete. `253de07f`'s message narrates the already-landed July fix under the current round's B1 heading, which makes the pagination code read as new work when it predates the commit. Authoritative mapping of review 5181793802 to what `253de07f` changed: B1 vault-managed teardown reported as complete. `secret_deleted: boolean` -> `secret: 'deleted' | 'absent' | 'not_applicable'`, response echoes `provider_name`, CLI prints the `delete-oauth2-credential-provider` follow-up, prompt warns before the destructive action, guide updated. (253de07's message calls this "N1/N2".) N1 `ConsistentRead: true` on the lookup scan + `ConditionExpression: '#status = :active'` on the revoke, so the loser of a race 404s and never reaches `DeleteSecret`. (253de07's message folds this into its "B1" section.) N2 pagination-rationale cross-references corrected: dropped `jira-webhook-processor.ts`, cited `shared/jira-tenant-registry.ts:32-46` as the genuine precedent and as the `ConsistentRead` precedent, kept `linear-issue-lookup.ts:131-136` relabelled a counter-example, and replaced "paginate to completion" with "until a match or key exhaustion". (253de07's message does not label this at all.) N3 the `status != 'active'` claim qualified in five places, and the removal now writes the distinct `revoked_reason: 'admin_removed'`. (253de07's message splits this across its "N3" and "N11/N12".) N4 `DeleteSecretCommand` input asserted exactly: `SecretId` plus `ForceDeleteWithoutRecovery: true`. (253de07's message lists this under "N4/N5/N6/N10" without detail.) N5 all three `JSON.stringify(...).toContain('revoked')` assertions replaced with value-level `[':revoked'] === 'revoked'` and `[':uid'] === ADMIN`. N6 the three untested failure paths covered: revoke rejecting (500, no `DeleteSecret`), `markSecretDeletionFailed` rejecting (still `SECRET_DELETE_FAILED`), `--purge` `DeleteCommand` rejecting (500 with the revoke asserted landed). N7 `MAX_SCAN_PAGES = 20`, returning 500 rather than 404. (253de07's message uses "N7" for the by-name secret-delete fallback, which was not a numbered review item.) N8 NOT in this commit. Filed as aws-samples#883 (409 on duplicate active rows). N9 wording half only. The behavioural half is filed as aws-samples#884. N10 manual-fallback snippet reordered to revoke-first with `--condition-expression '#s = :active'`, plus the note that re-running `remove-workspace` on an already-removed workspace returns 404. N11 module-local `RemoveWorkspaceResponseBody` applied with `satisfies` at the return. (253de07's message lists this under "N4/N5/N6/N10".) N12 `WORKSPACE_REGISTRY_TABLE` is plain `string | undefined`, no non-null assertion, validated once in-handler. (253de07's message lists this under "N4/N5/N6/N10".) Also not a numbered review item, and therefore unlabelled rather than mislabelled: the by-name `bgagent-linear-oauth-<slug>` fallback for the secret delete when the row records no `oauth_secret_arn`. The full mapping with reasoning is in the PR comment: aws-samples#681 (comment) Refs aws-samples#306, aws-samples#681, aws-samples#883, aws-samples#884. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: scottschreckengaust <345885+scottschreckengaust@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Sphia Sadek <isadeks@gmail.com>