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

Open
#817 2 comments 0 reactions 0 assignees View on GitHub
agent-runtime bug P1 security
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.