aws-samples / aws-samples/sample-autonomous-cloud-coding-agents
fix(compute): Lambda MicroVMs P2 review follow-ups (#733) — classifier ordering, payload-delete grant, ARN pinning, stale claims, N1 test
- Dominant language
- TypeScript
- Stars
- 143
- Forks
- 46
- Avg merge
- 3d 9h
- Merged PRs (30d)
- 20
Description
Follow-up to #733 (Lambda MicroVMs P2). `baa365fc` closed B1, B2, B6, B7, N2-ii and 11 nits, all mutation-pinned. Five items remain. Backend is experimental (non-suppressible synth warning against production use), so none is on a production path.
Verified by execution at `779849ba`.
## 1. Appended `stateReason` hijacks an earlier classifier pattern
`error-classifier.ts:253` (substrate failure) sits below `:208` (regional availability). B1's fix appends AWS-owned free text into the string feeding that ordered list:
```
"…substrate state completed (Run lifecycle hook returned HTTP status 400…)"
→ "The MicroVM stopped before the agent reported a result" retryable: true ✅
"…substrate state completed (MicroVM host unavailable.)"
→ "Lambda MicroVMs is not available in this Region" retryable: false ❌
"…substrate state completed" (pre-#733 shape)
→ "The MicroVM stopped before the agent reported a result" retryable: true ✅
```
`retryable` flips `true`→`false`, so a transient is reported non-retryable and the user doesn't retry. New in `baa365fc`. Also hits `capacity unavailable in this Availability Zone`, `Lambda MicroVMs not available.`, and via `:166` `The account concurrency limit for MicroVMs was reached.` `error-classifier.test.ts:576` only classifies the reason-less string, so the suite is green.
Two viable fixes.
**(a) Reorder** — move the `:253` substrate block above the `:208` regional entry. Structurally cleaner, but spans two non-adjacent regions.
**(b) One-line lookahead** — replace `error-classifier.ts:208`:
```ts
pattern: /^(?!.*MicroVM substrate terminated).*(?:(?:UnknownEndpoint|Inaccessible host|Could not resolve endpoint).{0,120}lambda|(?:lambda[- ]?microvms?|microvms?).{0,60}(?:not available|not supported|unavailable)|(?:not available|not supported|unavailable).{0,60}(?:lambda[- ]?microvms?))/i,
```
(b) verified at `779849ba`: all 4 hijacks reclassify to `The MicroVM stopped before the agent reported a result` / `retryable: true`; all 4 real Region errors still match (`UnknownEndpoint … lambda`, `Lambda MicroVMs is not available in eu-south-2`, `Could not resolve endpoint for lambda microvms`, `microvms not supported in this region`) → `retryable: false`; baseline and hook-400 unchanged. 229/229 pass across `error-classifier`, `orchestrator`, `lambda-microvm-strategy`.
Note (b) is a stopgap — it hard-codes one message's text. The durable fix is to stop feeding AWS-owned free text to the classifier at all: classify the base message (which the codebase controls and which already implies a live VM was polled) and carry `stateReason` as display text only.
- [ ] Apply (a) or (b)
- [ ] Fix or explicitly accept the `:166` concurrency hijack (`The account concurrency limit for MicroVMs was reached.` — not covered by either fix above)
- [ ] Test pins the ordering — restoring the original order must fail it
- [ ] Consider the durable fix: `stateReason` as display-only, never classifier input
## 2. `deleteMicrovmPayload` has no `s3:DeleteObject` grant
`task-orchestrator.ts:528` has `grantPut` only; the ECS sibling at `:517-518` has `grantPut` + `grantDelete`. Synthesized the real `--context compute_type=lambda-microvm` stack: `PutObject`, `PutObjectLegalHold`, `PutObjectRetention`, `PutObjectTagging`, `PutObjectVersionTagging`, `Abort*` — no `DeleteObject`. Caller is `lambda-microvm-strategy.ts:378` via `orchestrate-task.ts:469`.
Deletes `AccessDenied` and are swallowed as best-effort, so payloads survive to the 1-day TTL. With bucket-wide `grantRead` and `/payload.json` keys, any running MicroVM reads finished tasks' hydrated prompts for ~24 h. Not a regression — the improvement is unrealized while comments say it landed. `orchestrate-task-microvm.test.ts:61-65` module-mocks `@aws-sdk/client-s3`, so it asserts construction, not authorization.
Complementary to #700 (read side, both backends) — schedule together. #700's "write and delete are correctly withheld" is now stale.
- [ ] `grantDelete` added
- [ ] Invert `task-orchestrator.test.ts:816` and `agent.test.ts:1190` — both assert `not.toContain('s3:DeleteObject')`, so CI fails until inverted
- [ ] Correct `task-orchestrator.ts:337-338`, `:522-526`, `lambda-microvm-compute.ts:52` — lifecycle rule is the backstop, not the only reaper
- [ ] Assert the grant at IAM/template level
- [ ] Amend #700's stale premise
## 3. ARN pinning: cross-workspace redirect still accepted
`_reject_foreign_arns` (`server.py:1096-1146`) holds where verified: the anchor can't be disarmed by omission/`None`/`""`/whitespace, parsing resists prefix-account and bare-name tricks, and skipping region is sound (the grant is region-scoped).
The finding's primary case is open. One deployment serves multiple Linear workspaces in the **same account** (`LINEAR_SETUP_GUIDE.md:16`), which an account anchor cannot close:
```
agent_session_role_arn = arn:aws:iam::123456789012:role/…AgentSessionRole-XYZ
github_token_secret_arn = arn:aws:secretsmanager:us-west-2:123456789012:secret:bgagent-linear-oauth-victimworkspace-a1B2c3
→ ACCEPTED
```
`config.py:55-65` fetches it on the unscoped execution role (`aws_session.py:323`), caches the `SecretString` into `os.environ["GITHUB_TOKEN"]`, and `shell.py:148` passes it to every repo subprocess. Fix: pin the value — it's a synth-time constant (`task-orchestrator.ts:436`).
Second gap: `arn_keys`/`account_anchor_key` are absent from `OWNED_PYTHON_PATTERNS` in `check-constants-sync.ts`. Re-declaring both as literals in `server.py` → `Constants sync OK`, exit 0, 216 tests pass; a fifth contract ARN key then leaves `linear_oauth_secret_arn` and `jira_oauth_secret_arn` unpinned. Enforcement is one-way too: `future_kms_key_arn` added to `env_by_key` only was accepted by both guards and installed unpinned with a foreign account.
Needs #645's deferred `NO_INGRESS` reachability probe first — exploitation requires a forged `/run` payload and that barrier has never been negatively tested.
- [ ] Same-account, different-workspace `bgagent-*-oauth-*` ARN rejected
- [ ] Same-account cross-Region secrets and all four current ARN keys still accepted
- [ ] `arn_keys` + `account_anchor_key` in `OWNED_PYTHON_PATTERNS`; literal re-declaration fails the gate
- [ ] Membership enforced both ways — an `*_arn` key in `env_by_key` but not `arn_keys` fails
- [ ] `server.py:1105-1110` says region-scoped, not account-scoped
- [ ] `NO_INGRESS` probe run and recorded
## 4. Retracted claims still asserted in docs
Two claims #733 disproved, corrected where reviewers anchored, still live elsewhere. Docs-only, but both were blocking because acting on them breaks something.
**(a) `boto3.DEFAULT_SESSION` freezes credentials only, not region.** Reproduced clean-room (botocore 1.43.78, all `AWS_*` purged, `logs` not S3): region `us-west-2`→`eu-west-1`, UA `build-app`→`runtime-app`, creds stayed `AKIAbuild`. Fixed at `server.py:1252-1257`, `:1279-1282`, `:1297-1301`. Still wrong at:
- `server.py:1802-1804` — "credentials **and the build region** — frozen in the snapshot" (same file; `baa365fc` has a hunk in this function)
- `agent/README.md:239` — "credential chain **plus the build-time region**"
- `agent/README.md:273` — "(**region included**)"
- `ADR-021:217` + mirror — "(**region included**)"
- `ADR-021:407` + mirror — "**and the build-time region** out of the snapshot"
**(b) The heartbeat is not written unconditionally on every substrate.** `_heartbeat_worker` (`server.py:272`) is the only periodic writer, started only from `_spawn_background` (`:736`), which has two call sites: `:815` and `:2147`. `pipeline.py:930` writes once. ECS bypasses uvicorn (`ecs-strategy.ts:245`), so `ecs => false` is a hard constraint. Fixed at `orchestrator.ts:84-124`. Still wrong at `ADR-021:73` — "unconditionally, with no backend awareness, so the timestamp exists on every substrate" plus "two independently-tuned kill paths", both halves of the framing B3 was filed against. Mirror at `Adr-021-…:77`; `orchestrate-task.test.ts:807-810` repeats the preference rationale. A maintainer reads the ADR before the code comment.
**(c)** `agent/README.md:239` also still says a build-role Logs write "can only fail" — correct at `server.py:1273-1277`: the grant is scoped to `/aws/lambda-microvms/*`. `README:291-293` rejection catalogue predates the fix.
- [ ] All sites corrected; grep for "region included", "build-time region", "unconditionally", "two independently-tuned kill paths", "can only fail" is clean
- [ ] Starlight mirrors regenerated (`mise //docs:sync`) and committed
## 5. N1's dominant case has no test
Classification is correct — `_PayloadFetchError` (`server.py:1394-1413`) is not a `ValueError`, and all 10 cases through the real `/run` route classify right. But deleting the `try/except` at `:1450-1460` leaves the agent suite green (1739 passed) while the original defect reproduces: truncated body → `400 MICROVM_RUN_PAYLOAD_INVALID`. Coverage marks `:1452` and `:1457-1460` never executed. Siblings at `:1461-1464` and `:1520-1525` are pinned. Cause: handler-level S3 tests monkeypatch `_fetch_microvm_payload_from_s3`, so real `json.loads` never sees a bad body.
- [ ] Handler-level test posts an `agent_payload_s3_uri` envelope with truncated bytes, asserts `500` + `MICROVM_RUN_PAYLOAD_UNREADABLE`, and fails when `:1450-1460` is removed
`InvalidRegionError` is a `ValueError` subclass raised at `:1448` from a snapshot-baked region, so a corrupt baked region returns `400` blaming the orchestrator's envelope. Narrow; decide either way.
## Not filed
- **Stale `1.4.0` refs** — false. `version.ts:47` documents why it's 1.6.0; remaining hits are version history and unrelated fixtures.
- **`— ago` in `bgagent list`** — guarded by `t.agent_heartbeat_at &&`; only reachable via a malformed timestamp.
The N2 rebuttal ("dies on the first table write") is wrong — `task_state.py:43-58` returns `None` when `TASK_TABLE_NAME` is unset and every writer early-returns. Harmless since the fix makes it unreachable; noted so the table write isn't mistaken for the safety net and the guard at `server.py:2090` removed.
Contributor guide
Research direction
Start with the named entry points: error-classifier.ts and its tests, task-orchestrator.ts with IAM tests, server.py and ARN-sync checks, and the listed README/ADR mirrors. Run the referenced classifier, orchestration, agent, constants-sync, and handler tests before changing behavior. Done means all five follow-up areas have explicit fixes or decisions, with the listed regression tests and documentation checks passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python, typescript
- Domain
- backend, cloud, documentation, security, testing
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100