Skip to content

Commit 0ee7227

Browse files
isadeksbgagentkrokoko
authored
test(agent): fail hung/slow tests loudly (pytest-timeout + aws_session barrier bound) (aws-samples#598)
* fix(ecs): write task payload to S3, not inline overrides (aws-samples#502) The ECS compute strategy inlined the full orchestrator payload (incl. the large hydrated_context) into the AGENT_PAYLOAD container-override env var. ECS RunTask caps the TOTAL containerOverrides blob at 8192 bytes, so any real task was rejected before the container started: InvalidParameterException: Container Overrides length must be at most 8192 AgentCore is unaffected — it passes the payload in the InvokeAgentRuntime request body, which has no comparable limit. The bug only surfaces with a realistic hydrated payload, which is why the prior ECS smoke test (a small Rust cargo-check, aws-samples#494) didn't catch it. Fix — stash the payload out-of-band and pass only a pointer: - New EcsPayloadBucket construct (mirrors TraceArtifactsBucket): BLOCK_ALL, enforceSSL, S3_MANAGED encryption, 1-day lifecycle TTL (payloads are ephemeral — read once at boot). Dedicated bucket so the ECS task role's S3 read is scoped to payloads only and can't touch attachments/traces. - ecs-strategy: when ECS_PAYLOAD_BUCKET is set, PutObject the payload to <task_id>/payload.json and pass AGENT_PAYLOAD_S3_URI in the override; the boot command fetches+parses it via boto3. Inline AGENT_PAYLOAD remains as a fallback (small payloads / no bucket), so nothing regresses. deleteEcsPayload helper removes the object. - orchestrate-task finalize: best-effort deleteEcsPayload for ECS tasks once terminal (the container has long since read it); lifecycle rule is the crash backstop. - EcsAgentCluster: accept payloadBucket, inject ECS_PAYLOAD_BUCKET env, grant the task role READ ONLY (untrusted repo code must not write/delete payloads; the trusted orchestrator owns write+delete). Session-role-aware. - task-orchestrator: ecsPayloadBucket prop → grantPut + grantDelete to the orchestrator; @aws-sdk/client-s3 added to bundling externals. - agent.ts: updated the commented uncomment-to-enable ECS scaffolding to wire the payload bucket. Tests: new bucket construct (TTL/SSL/block-public/autoDelete); strategy S3-write + URI-pointer + inline fallback + deleteEcsPayload (incl. best-effort swallow + no-op without bucket); cluster read-grant + env var + read-only (no put/delete). Full build green. Closes aws-samples#502 * fix: enable corepack in the agent image so yarn/pnpm resolve Root cause of the exit-127 inert gate: the agent Dockerfile installs node/npm/ mise/uv but NOT yarn, so any repo whose build runs 'yarn install' (incl. ABCA) hit 'yarn: command not found' and the agent hand-built a ~/bin/yarn shim every run. corepack (ships with Node 24) installs the yarn/pnpm shims at image-build time. Systemic — fixes every yarn repo, no per-run shim. Needs redeploy to take effect. (cherry picked from commit 2fd5fca) (cherry picked from commit d5e0e64) * wire ECS/Fargate substrate (context-gated) at 32GB/8vCPU for heavy builds AgentCore's fixed microVM envelope OOM-kills CI-parity builds (live-caught dogfooding ABCA-on-ABCA: the ~2800-test `mise run build` killed the VM at the build-gate step). AgentCore memory isn't tunable, so route repos that set `compute_type: 'ecs'` to a Fargate task instead. - agent.ts: gate EcsAgentCluster + the orchestrator ecsConfig on the `compute_type` deploy context (default 'agentcore'). ECS resources only synthesize under `--context compute_type=ecs`, so default synth (and the bootstrap-coverage test) stays agentcore-only. Mirrors upstream's context-gating of the ECS construct. - ecs-agent-cluster.ts: size the Fargate task at 8 vCPU / 32 GB (was 2/4) — a valid ARM64 combo with headroom for the jest worker fleet + tsc that AgentCore's envelope can't give. Comment documents the live OOM. - test: assert the new 8192/32768 sizing. Verified: default synth has 0 ECS resources; `--context compute_type=ecs` synth emits AWS::ECS::Cluster + TaskDefinition. Full `mise run build` green. (cherry picked from commit 8cb7cfa) (cherry picked from commit 632a36b) * bump ECS Fargate to 64GB/16vCPU + BUILD_VERIFY_TIMEOUT_S=3600 Live-fired the full `mise run build` on the 32GB ECS task (fork dogfood): install OK, build ran ~50 min then OOM-killed at the cap (ECS stoppedReason "OutOfMemoryError: container killed due to memory usage", exit 137; peak working set ~31.6 GB). Root cause: the monorepo build fans out 4 heavy jobs in PARALLEL (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build), each with its own worker fleet (jest, pytest, esbuild Lambda bundling) — 32 GB had no headroom for that concurrent peak, and the ~50-min wall-clock also blew the 1800s build-gate cap. Fix (keep the full build as the gate, give it room): - ecs-agent-cluster.ts: Fargate 8vCPU/32GB → 16vCPU/64GB (valid ARM64 combo; 16 vCPU admits 32–120 GB). 2× memory headroom for the parallel storm + more cores to shorten wall-clock. Comment records the full sizing history. - ecs-agent-cluster.ts: inject BUILD_VERIFY_TIMEOUT_S=3600 into the container env so a slow-but-healthy CI-parity build isn't mis-flagged as a timeout (post_hooks.py reads it; default 1800 stays for AgentCore repos). - tests: assert 16384/65536 + the new env var. Verified: default synth ECS-free; --context compute_type=ecs synth emits Cpu 16384 / Memory 65536 / BUILD_VERIFY_TIMEOUT_S 3600. Full build green. (cherry picked from commit 11fb3a5) (cherry picked from commit cd7b8eb) * fix(ecs): map full payload to run_task so channel/build/cedar fields aren't dropped (ABCA-487) The ECS boot command hand-listed ~18 run_task kwargs and silently dropped the rest. On ECS this meant channel_source/channel_metadata never reached run_task, so resolve_linear_api_token never ran, LINEAR_API_TOKEN was never set, and the Linear/Jira reaction + channel MCP silently no-op'd — @bgagent tasks on ECS posted NO 👀 reaction or anything (live-caught: ABCA-487 on the fork, RUNNING with mcp_servers:[] and no reaction). Also dropped: build_command/lint_command (build-gate used defaults), cedar_policies + approval_* (HITL guardrails), base_branch/merge_branches (orchestration stacking), attachments, trace, user_id. Same "hand-rolled partial payload copy" root as aws-samples#502. Fix — single source of truth, unit-testable: - pipeline.run_task_from_payload(p): maps the WHOLE payload dict to run_task's signature — rename prompt→task_description / model_id→anthropic_model, filter to accepted params (unknown keys ignored, not passed as **kwargs), coerce issue_number/pr_number→str + max_turns→int, aws_region falls back to env. _RUN_TASK_PARAMS computed once at import (patch-safe). - entrypoint exports it; ecs-strategy boot command now calls run_task_from_payload(p) instead of the hand-listed kwargs. - tests: 9 agent tests (rename, channel fields ABCA-487, build/cedar/branch, coercion, unknown-key drop, None-drop, aws_region, signature guard) + ecs-strategy asserts the boot command calls the mapper and no longer hand-lists. Full build green: agent + cdk 2852 + cli 571 + docs. Deployed to dev (--context compute_type=ecs; agent image rebuilt). (cherry picked from commit 777ee0e) (cherry picked from commit 1639c27) * fix(ecs): grant task role GetSecretValue on bgagent-{linear,jira}-oauth-* (ABCA-488) A Linear/Jira-channel task resolves its per-workspace OAuth token from Secrets Manager (bgagent-linear-oauth-<slug>) at startup to fire the 👀→✅ reaction and drive the channel MCP. The AgentCore runtime role + orchestrator/fanout/ screenshot/linear-integration roles all have the bgagent-linear-oauth-* prefix grant, but the ECS task role only had GetSecretValue on the GitHub token. So on ECS the token fetch hit AccessDenied → LINEAR_API_TOKEN unset → linear_reactions skipped AND the Linear MCP stayed needs-auth (agent couldn't post comments either). Live-caught dogfooding on the fork (ABCA-488, ECS task 828dab35). Fourth ECS-parity gap after aws-samples#502 (payload size) + ABCA-487 (dropped channel fields): the AgentCore path had a capability the ECS task role didn't. Fix: grant the ECS task role secretsmanager:GetSecretValue on the bgagent-linear-oauth-* and bgagent-jira-oauth-* prefixes (GetSecretValue only — the container reads; the orchestrator owns refresh/PutSecretValue). Nag reason updated + regression test asserting the prefix grant. Full build green (2853). (cherry picked from commit 5a886bb) (cherry picked from commit 49235d4) * fix(ecs): wire ARTIFACTS_BUCKET_NAME into the Fargate task (ECS-parity, aws-samples#299) Live-caught: a :decompose on an ecs-configured repo ran on ECS (fix confirmed — no more 'ECS_CLUSTER_ARN missing') but then FAILED at delivery with 'ValueError: deliver_artifact: ARTIFACTS_BUCKET_NAME is not configured'. The AgentCore runtime sets ARTIFACTS_BUCKET_NAME; the EcsAgentCluster task def never got the bucket — an ECS-parity gap (same class as aws-samples#502/ABCA-487). A repo-bound artifact workflow (coding/decompose-v1) delivers its plan JSON there. - EcsAgentCluster: new optional artifactsBucket prop → ARTIFACTS_BUCKET_NAME in the container env + grantReadWrite on the task role (read+write: the container DELIVERS the artifact, unlike the read-only payload bucket). IAM5 suppression extended to cover the second scoped S3 grant. - agent.ts: pass traceArtifactsBucket.bucket (same bucket the runtime uses). - 3 construct tests (env injected / read+write grant / omitted when absent). 2872 CDK + 571 CLI + 1184 agent green. Held on branch. (cherry picked from commit 7617d72) (cherry picked from commit e0dfc93) * feat(compute): guard compute_type=ecs against a stack without the ECS substrate A repo onboarded as compute_type=ecs on a stack deployed WITHOUT --context compute_type=ecs fails every task at session start (no ECS_* env vars). Catch the config/deploy mismatch early + make the runtime error actionable: - agent.ts: new ComputeSubstrate CfnOutput — 'ecs' when the gate is on, else 'agentcore' (ecs is additive; the AgentCore runtime is always present, so an agentcore repo works on either). - cli repo onboard: when --compute-type ecs, read ComputeSubstrate and refuse with a redeploy message if it's an explicit non-ecs value. Null (older stacks predating the output) is treated as unknown → proceed (runtime backstop). - ecs-strategy: the missing-env error now names the root cause + remedy (redeploy with the gate, or set the repo to agentcore) instead of a bare env-var list. - Tests: CDK output flips with the gate (agentcore default / ecs gated); CLI onboard refuses/allows/back-commpat; ecs construct unchanged. 2875 CDK + 575 CLI + 1184 agent green. Held on branch. (cherry picked from commit cb7c8c6) (cherry picked from commit 9048db0) * fix(cdk): raise BUILD task memory 64→120 GB (max Fargate) — ABCA-662 baseline OOM ABCA-662's pre-agent baseline build was OOM-killed (exit 137) at 64 GB: dogfooding ABCA-on-ABCA, the full parallel `mise run build` (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build, each with its own worker fleet) peaked past 64 GB. Each build task is memory-ISOLATED (its own Fargate microVM), so limiting concurrency does NOT help a single over-64 GB build — only more per-task RAM or less build parallelism does. 120 GB is the MAX Fargate admits at 16 vCPU (32–120 in 8 GB steps), so this is the clean experiment: if a build still OOMs here, the only remaining lever is cutting the build's peak parallelism (serialize the mise DAG / cap jest --maxWorkers) — there is no more RAM to give. Pairs with the repo.py fix (an OOM-killed baseline is now infra, not 'already broken'), so even if a build does hit the ceiling the verdict stays honest. Tests updated to assert Memory 122880 (both BUILD-def assertions). (cherry picked from commit 25e2bb1) * fix(cdk): make ecs-strategy top-of-file import hermetic vs ambient env The inline-fallback / no-op tests in the top-of-file describe blocks assume the OPTIONAL vars ECS_PAYLOAD_BUCKET and ECS_PLANNING_TASK_DEFINITION_ARN are ABSENT when the module is first imported (it reads them as module-level constants). A dev shell has neither set, so the suite passed locally — but the REAL ECS agent container HAS ECS_PAYLOAD_BUCKET set (aws-samples#502 payload bucket), so on ECS the const was truthy and the 'no bucket → inline fallback' + 'no-op' assertions failed: FAIL test/handlers/shared/strategies/ecs-strategy.test.ts ● startSession › sends RunTaskCommand with correct params ... ● deleteEcsPayload without ECS_PAYLOAD_BUCKET › no-ops ... This was the residual fork baseline-build exit-1: 2 failed / 3093 passed, only ever reproducible inside the ECS microVM. Surfaced by the live-streaming build log (the buffered summary had hidden it). Fix: delete both optional vars before the top-of-file import so it's hermetic regardless of the runner's environment; the aws-samples#502/aws-samples#299 describe blocks already set them via jest.isolateModules. (cherry picked from commit 7a2bde9) (cherry picked from commit 8267c29) * fix(cdk): grant ec2:DescribeAvailabilityZones to ECS agent task role (cdk synth on fresh clone) A CDK-based target repo's build gate runs `cdk synth`. When the stack is wired to a concrete env ({account, region}), CDK does a synth-time availability-zone context lookup (ec2:DescribeAvailabilityZones). A developer box caches the result in the gitignored cdk.context.json so synth is hermetic; the agent clones fresh, so there is no cache and the live lookup fires. The ECS task role lacked the action, so synth failed with: ERROR ...EcsAgentClusterTaskRole... is not authorized to perform: ec2:DescribeAvailabilityZones ... Synthesis finished with errors → a FALSE build-gate failure on code that builds fine everywhere else. Same ECS-parity class as ABCA-488 (GetSecretValue) and F-2 (CreateEvent): a permission the AgentCore path had but the ECS task role didn't. Live-caught on the ABCA fork: baseline `mise run build` passed all 3095 tests then died at `cdk synth -q`, surfaced only by the live-streaming build log. Grant is a read-only describe (Resource:* — EC2 describe actions have no resource-level scoping; IAM5 suppressed, no mutation/data access). +1 regression test pins the statement. (cherry picked from commit 3fc9060) (cherry picked from commit 2bf12f4) * fix(ecs): grant ECS task role AgentCore Memory access (F-2, ABCA-488-class) Live-caught during the fork stress smoke (ABCA-495): the ECS TaskDefTaskRole got AccessDeniedException on bedrock-agentcore:CreateEvent for the AgentMemory resource, so the agent's cross-task learning writes (write_task_episode / write_repo_learnings) silently no-op'd (WARN) on EVERY ECS task — memory never persisted on the fork's ECS-only substrate. Same class as ABCA-488: the AgentCore runtime role gets memory access via agentMemory.grantReadWrite(runtime) in agent.ts, and the orchestrator gets it too, but the ECS task role never did. Fix: EcsAgentCluster gains an optional agentMemory prop; when provided the task role gets agentMemory.grantReadWrite (read actions + CreateEvent), scoped to the memory ARN (synth-verified: no wildcard resource, so the existing IAM5 nag suppression is unaffected). agent.ts passes agentMemory to the ECS cluster alongside the existing memoryId. Omitted in isolated construct tests / memory-less deploys → no grant (asserted). Tests: construct asserts CreateEvent on the task role scoped to MemoryArn when wired, and NO bedrock-agentcore grant when unwired. Full cdk build green (2883). (cherry picked from commit c23d49fbead30ed52860e3975815e3643b22ade0) (cherry picked from commit 44bb71e) * docs(bootstrap): correct the ECS ComputeTypes enable instructions The //cdk:bootstrap task hint and DEPLOYMENT_ROLES.md both said to enable the ECS compute backend with 'mise //cdk:bootstrap -- --context ComputeTypes=agentcore,ecs'. That does NOT work: ComputeTypes is a CloudFormation *template parameter*, and this CDK version's 'cdk bootstrap' has no way to set one — there's no --parameters flag, and --context sets CDK construct context, not a CFN parameter. So the ECS policy (IaCRole-ABCA-Compute-ECS, conditional on ComputeTypes containing 'ecs') was never attached, and a --context compute_type=ecs deploy then failed with AccessDenied on ecs:* — live-caught deploying the substrate to dev. Correct it to the mechanism that actually works: bootstrap normally, then set the parameter directly on the CDKToolkit stack via 'aws cloudformation update-stack ... ParameterKey=ComputeTypes,ParameterValue=agentcore,ecs' (with a describe-stacks verify). Fixed in the mise task description + comment and both DEPLOYMENT_ROLES.md occurrences; Starlight mirror regenerated. Drift gate green. (cherry picked from commit a46683e) * fix(errors): classify a build/verify command TIMEOUT (live-caught ABCA-667, was 'Unexpected error') Replication finding: a live fork coding child (ABCA-667) failed with 'TimeoutExpired: Command [mise run build] timed out after 600 seconds' — the full ~2800-test fork build exceeded the 600s cap. That's the pre-agent verify-build subprocess timeout, which lands raw in error_message and fell through to 'Unexpected error' + generic guidance — exactly the transient-vs-real confusion this rework targets. It's not a crash and not broken code: the build ran too long. Add a TIMEOUT classification for / → 'Build/tests didn't finish in time', user class, retryable, remedy names the real fix (retry / raise BUILD_VERIFY_TIMEOUT_S or move to the larger ECS build box). Ordered before the generic poll-timeout pattern. Test uses the exact ABCA-667 shape. 60 classifier tests green. (cherry picked from commit 6394a59) (cherry picked from commit b9c1a94) * fix(agent): ensure claude-code native binary is placed at image build (Exec format error) claude-code@2.1.191 uses a shim (bin/claude.exe) + platform-optional-dependency install model: its postinstall (install.cjs) is supposed to replace the shim with the platform-native ELF binary. When that postinstall silently falls back (e.g. it hits EACCES overwriting the shim under npm's root de-privileging), the error-shim stays on PATH and every agent run dies at the run_agent step with 'OSError: [Errno 8] Exec format error: \'claude\'' — even though the native arm64 binary is present in the image as an optional dep, just never wired up. Live-caught on ABCA-659's retry: all 3 ECS runs failed this way on a freshly rebuilt image (the prior working image ran the older single-binary 2.1.142). Re-run install.cjs explicitly after the npm install so the native binary is placed at build time, and hard-verify with 'claude --version' so a still-broken shim fails the image build instead of every task at runtime. (cherry picked from commit 7cb4a87) (cherry picked from commit 7de778b) * fix(agent): log stdout on run_cmd failure so build-gate failures are debuggable The post-agent build gate logged only stderr on a non-zero exit. But build/test tooling (jest, tsc, the mise task DAG) writes the ACTUAL failing-task error to STDOUT, not stderr — stderr often carries only the runner's plan echo. So a red `mise run build` surfaced every task STARTING in CloudWatch but never WHICH one failed or why (ABCA-662: a 7s exit-1 with zero captured cause, indistinguishable from a phantom, and the reason Linear showed a build-fail GitHub CI passed). run_cmd now also tails the last 20 lines of stdout on failure (redacted — repo build output is untrusted). 4 tests pin: stdout surfaced, tailed-not-headed, redacted, and NOT dumped on success. (cherry picked from commit 67d84db) (cherry picked from commit bb578fd) * fix(agent): surface the FAILING line from a parallel build DAG, not just a tail The stdout-on-failure logging (67d84db) tailed the last 20 lines — enough for a serial command, but NOT for `mise run build`, which runs 4 packages in PARALLEL and interleaves their output. The failing task's error lands in the MIDDLE while the tail captures whatever finished LAST (e.g. a PASSING package's coverage table) — so the log showed a green coverage table on a red build and named nothing (ABCA-662: three verification runs where the actual failing sub-task was never visible in CloudWatch). run_cmd now scans the WHOLE stdout for failure-signature lines (FAIL/✕/●/error TS/ELIFECYCLE/coverage-threshold/mise 'no task'/'task failed'/traceback/…), filters benign noise ('0 errors', '--no-error'), and logs those FIRST, then a trailing tail for context — deduped, capped at 40 lines. Falls back to the tail when nothing matches (unknown tool). This is the tooling gap that made the whole ABCA-662 build investigation slow: every failure was diagnosable only by luck or local repro. 5 tests incl. the mid-DAG-failure + coverage-threshold cases. (cherry picked from commit 3fe99c9) (cherry picked from commit aaa4f18) * fix(agent): bound the aws_session concurrency test's Barrier + joins (ROOT of the ECS flaky hang) test_concurrent_first_call_builds_once used a bare threading.Barrier(20).wait() + unbounded t.join(). Barrier(N).wait() blocks FOREVER unless all N threads arrive; under ECS container memory pressure a worker thread can be reaped (or thread creation throttled) before reaching the barrier, so every survivor hangs on wait() and the main thread hangs in join() — stalling the whole `mise run build` until the 3600s ceiling. THIS is the ECS-only flaky hang chased across ABCA-684/686/688 (pytest-timeout signal-method is the backstop; this is the root cause). Bounded the barrier wait (timeout=30 → BrokenBarrierError) and the joins (timeout=60), so a missing thread fails the test fast+loud instead of deadlocking. Stress-ran 20x locally: no hang. The test_attachments name pytest-timeout stamped earlier was a red herring — the tracker reports whichever test was last seen, not the barrier-blocked one. (cherry picked from commit 7ff91bf) (cherry picked from commit 85d9881) * fix(build): add pytest per-test timeout so a hung agent test fails loud Agent pytest can hang on the ECS build box (an env-specific test blocking on network/subprocess/Bedrock with no timeout of its own), stalling the pre-agent baseline until the 3600s BUILD_VERIFY_TIMEOUT_S guard — turning one flaky test into an un-diagnosable ~60-min stall (ABCA-684/686). Add pytest-timeout with a 120s per-test cap so the offending test fails LOUDLY with a dumped traceback and the suite moves on. Agent-side only: the jest --maxWorkers cap from the same source commit is Mac-local build-box tuning (core-relative worker count), deliberately NOT brought to main — CI containers have a different core/RAM profile. (cherry-picked agent hunks of 5ea9670 from linear-vercel) (cherry picked from commit bfcd280) * fix(agent): pytest-timeout method thread→signal so a hung test is actually ABORTED The thread method only PRINTS the hung test's stack at the 120s cap — it cannot interrupt a test blocked in a C-level/socket syscall. Proven live on ABCA-688: test_attachments dumped its stack at 120s, then the build kept hanging ~55 min until the platform's 3600s build-verify ceiling finally killed it. pytest runs single-threaded in the main thread here, so SIGALRM (method=signal) interrupts even a syscall-blocked test and fails it in 120s as intended. Full agent suite still 1315 passed (4.5s), plugin active. (cherry picked from commit 803c44b) (cherry picked from commit 1d20679) * fix(ecs): remove dead ECS_PAYLOAD_OBJECT_KEY_PREFIX export (review B1) The `export const ECS_PAYLOAD_OBJECT_KEY_PREFIX = ''` constant was referenced nowhere — ecsPayloadKey() (ecs-strategy.ts) builds `<task_id>/payload.json` independently and already documents that layout in its own docstring. The dead export was a "prefix" whose value was '' with a docstring describing a key layout it didn't produce, and it raised the knip dead-code count, tripping the ratchet inside `mise run build`. Delete it; ecsPayloadKey remains the single source of truth for the key layout. * docs+feat: address review nits N1–N4 on the ECS substrate Review follow-ups for aws-samples#596 (fix itself unchanged; B1 dead-export fixed in the base PR aws-samples#503): - N1: correct the stale "64 GB / 16 vCPU" comment in agent.ts — the task is larger (64 GB was itself OOM-killed per the construct's sizing history). Defer the exact figure to EcsAgentCluster rather than duplicate/drift it. - N2: reword the memory-grant + oauth-grant comments — an AccessDenied on those paths is LOGGED (memory.py treats it as an infra failure; config.py's token resolver logs it), not "silently" no-op'd. Describe the user-visible effect (no persisted learning / no 👀→✅ reaction) rather than pin a log level that drifts. - N3: the ec2:DescribeAvailabilityZones comment already frames it correctly as a FALSE build-gate failure (not a WARN no-op) — no change needed; noted here for completeness. - N4: run_task_from_payload now emits a WARN when it drops a KNOWN orchestrator key (build_command / merge_branches / base_branch / github_token_secret_arn) that run_task doesn't accept — expected today (consumed elsewhere / not yet wired), but makes a future "wired one side, forgot the other" no-op visible instead of silent (the ABCA-487 class). Foreign keys still drop quietly. + 2 tests (warns on a known dropped key; stays quiet on a foreign key). Full cdk build green (2286) + full agent suite green (1194); eslint/ruff clean. * style: ruff-format the N4 payload-key block (fix build-mutation failure) CI's "Fail build on mutation" tripped: I ran `ruff check` (passed) but not `ruff format`, so the `_KNOWN_ORCHESTRATOR_KEYS = frozenset({...})` set literal and the two-line `log(...)` call weren't in ruff's canonical multi-line form. `ruff format` normalizes them; no behavior change (12 payload tests still pass). * fix+docs: WARN on dropped task_started_at (HITL parity) + defensive max_turns + comment nits Follow-up on the aws-samples#596 re-review (code was already approved-quality; these are the new non-blocking finding + 3 nits): - task_started_at HITL parity: AgentCore's server.py exports task_started_at as TASK_STARTED_AT, which hooks._remaining_maxlifetime_s() uses to clip the Cedar HITL approval-gate maxLifetime. The ECS boot path bypasses server.py and never sets it, so run_task_from_payload dropped it SILENTLY — a fail-open AgentCore↔ECS HITL divergence. Added task_started_at to _KNOWN_ORCHESTRATOR_KEYS so the drop now WARNs (surfaces the gap). Fully wiring TASK_STARTED_AT into the ECS containerEnv is a tracked follow-up; this makes the divergence visible today. - max_turns defensive coercion: a malformed max_turns raised ValueError mid-boot while every other field defaults defensively. Now drops it (run_task default applies) with a WARN, matching the surrounding style. - Nit: artifactsBucket JSDoc no longer claims this bucket is the `--trace` upload target (it wires ARTIFACTS_BUCKET_NAME only; the trace uploader reads TRACE_ARTIFACTS_BUCKET_NAME, unset here — noted as a separate ECS-parity gap). - Nit: dropped `2>/dev/null` from the Dockerfile corepack prepare so the prepare-failed diagnostic isn't hidden (the `|| corepack enable` fallback still guarantees resolvable shims — no regression to the exit-127 inert gate). Tests: +3 (task_started_at WARN, malformed-max_turns dropped-not-raised, valid max_turns still coerced). Full agent suite green (1195); cdk tsc + construct tests green; ruff format + eslint clean. * fix(ecs): address aws-samples#596 review — drop over-privileged artifacts grant + nits B1 (least-privilege regression): remove props.artifactsBucket.grantReadWrite from the ECS task role. coding/decompose-v1 delivers its plan via the assumed SessionRole (deliverers.py -> tenant_client), scoped to artifacts/${task_id}/*, exactly like the AgentCore runtime (whose task role likewise has NO direct artifacts grant). The whole-bucket grant over-privileged the untrusted-code role and broke cross-task isolation (a task could read/clobber other tasks' artifacts/<other_id>/, traces/, attachments/). Task role keeps only the ARTIFACTS_BUCKET_NAME env. Correct the inverted 'parity' comment and drop the now-stale artifacts clause from the cdk-nag IAM5 reason. Test: flip 'grants READ+WRITE on artifacts' → asserts the task role has NO S3 Put/Delete action at all (the read-only aws-samples#502 payload GetObject*/List* grant is not flagged). N4: add lint_command to _KNOWN_ORCHESTRATOR_KEYS (sibling of build_command) so a future 'wired build_command, forgot lint_command' contract gap WARNs instead of dropping silently. (N1/N2/N3 comment nits were already addressed on the branch head; B1 dead-const ECS_PAYLOAD_OBJECT_KEY_PREFIX no longer present. B2 governance handled separately.) * fix(ecs): address aws-samples#596 re-review nits — stale parity comments + WARN noise + max_turns coercion Scott's fresh re-review (head 86ab249) is Approve-with-nits: B1 (over-privilege) and B2 (governance) resolved; remaining asks are doc-accuracy + WARN-channel hygiene. - N1: rewrite the artifactsBucket JSDoc (ecs-agent-cluster.ts) that still claimed a read/write task-role grant the B1 fix deliberately removed — now states env-only wiring with delivery through the task_id-scoped SessionRole (parity with the AgentCore runtime role, which has no artifacts grant). The grant block ~30 lines below was already corrected in the B1 commit; this was its residual half. - N2: same inverted-comment fix at the agent.ts wiring site — '(read+write grant in the construct)' → env-only + SessionRole delivery. - N3: drop github_token_secret_arn from _KNOWN_ORCHESTRATOR_KEYS. It is ALWAYS present and ALWAYS resolved via the GITHUB_TOKEN_SECRET_ARN env, so listing it fired the known-key WARN on 100% of ECS boots — pure noise diluting the channel meant for genuine future contract gaps. Now falls through as a quiet foreign-key drop; comment records why it must not be re-added. +test asserting no WARN. - N4: max_turns int() coercion accepted a bool (int(True)==1) and truncated a non-integral float (int(3.9)==3), and the comment falsely claimed it 'matches how every other field is handled' (it is the one non-str coercion). Now rejects bools + non-integral floats with a breadcrumb; valid int / int-string / int-float still pass. +test. Reachable only via a corrupt payload (orchestrator emits a real int), so cosmetic — bundled since cheap. - N5: no-op — the Dockerfile corepack line already has no 2>/dev/null. Gates: cdk 2355 tests + eslint + synth green; agent 1268 tests + ruff + ty green. Non-blocking aws-samples#608 test-seam gaps (finalize deleteEcsPayload, boot-uri parse, entrypoint re-export, zero-ECS-synth assert) remain tracked, not in scope here. --------- Co-authored-by: bgagent <bgagent@noreply.github.com> Co-authored-by: Alain Krok <alkrok@amazon.com>
1 parent 86ccc58 commit 0ee7227

3 files changed

Lines changed: 40 additions & 2 deletions

File tree

agent/pyproject.toml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ dev = [
8989
"pytest",
9090
"pygments==2.20.0",
9191
"pytest-cov==7.1.0",
92+
"pytest-timeout==2.4.0", # per-test wall-clock cap: a single hung test (network/subprocess/Bedrock without its own timeout) must fail LOUDLY with a traceback, not silently burn the whole build-verify budget (ABCA-684/686: one hang stalled the baseline build past its 3600s ceiling)
9293
"vulture==2.16", # dead-code detection (#282): unused functions/classes ruff F can't see
9394
]
9495

@@ -130,6 +131,18 @@ ignore = [
130131
[tool.pytest.ini_options]
131132
testpaths = ["tests"]
132133
pythonpath = ["src"]
134+
# pytest-timeout: hard wall-clock cap PER TEST. A unit test that blocks (an
135+
# unmocked network/subprocess/Bedrock call with no timeout of its own) otherwise
136+
# hangs the whole `mise run build` until the 3600s build-verify ceiling kills it,
137+
# turning one flaky test into an un-diagnosable 60-min stall (ABCA-684/686). With
138+
# this, the offending test fails with a dumped stack in 120s and the suite moves on.
139+
# method="signal" (SIGALRM) actually ABORTS the hung test — pytest runs
140+
# single-threaded in the main thread here, so the alarm interrupts even a blocked
141+
# C-level/socket call. method="thread" only PRINTS the stack and cannot interrupt a
142+
# syscall-blocked test (proven live on ABCA-688: the thread method dumped the stack
143+
# but the build kept hanging ~55 min until the 3600s ceiling). signal is the real fix.
144+
timeout = 120
145+
timeout_method = "signal"
133146

134147
[tool.coverage.run]
135148
branch = true

agent/tests/test_aws_session.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,15 @@ def _slow_build(_arn: str) -> Any:
131131
return MagicMock(name="scoped")
132132

133133
def _worker() -> None:
134-
start.wait()
134+
# Bounded barrier wait. A bare Barrier(20).wait() blocks FOREVER if
135+
# even one of the 20 threads never arrives (e.g. a worker reaped under
136+
# container memory pressure, or thread creation throttled) — every
137+
# survivor then hangs here and the main thread hangs in join() below,
138+
# stalling the whole `mise run build` until the 3600s ceiling. This is
139+
# the ECS-only flaky hang chased across ABCA-684/686/688 (pytest-timeout
140+
# only fixed the SYMPTOM; this Barrier is the ROOT cause). A timeout
141+
# makes the barrier raise BrokenBarrierError so the test fails fast.
142+
start.wait(timeout=30)
135143
session = get_session()
136144
with lock:
137145
results.append(session)
@@ -140,8 +148,11 @@ def _worker() -> None:
140148
threads = [threading.Thread(target=_worker) for _ in range(20)]
141149
for t in threads:
142150
t.start()
151+
# Bounded joins for the same reason — a worker that died before
152+
# appending must not hang the suite; a leftover live thread trips the
153+
# assertion below (results != 20) rather than blocking indefinitely.
143154
for t in threads:
144-
t.join()
155+
t.join(timeout=60)
145156

146157
mk_build.assert_called_once()
147158
assert len(results) == 20

agent/uv.lock

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)