Fix/review gate idempotency key#618
Closed
isadeks wants to merge 376 commits into
Closed
Conversation
One-command visibility into orchestration state instead of hand-written DynamoDB scans + log tails: scripts/orchestration-debug.sh # list all orchestrations scripts/orchestration-debug.sh <orch_id> # full DAG: per-child status, # deps, task ids, release ctx scripts/orchestration-debug.sh logs [mins] # processor + reconciler logs, # filtered to orchestration events Motivated by the first dev smoke: the idempotency-key bug took log-archaeology to find because release failures logged only a status. This + the response-body logging make the next issue a one-liner to spot.
…data (aws-samples#247) Two bugs found on dev smoke that together prevented dependents releasing: 1. Reconciler OOM: at 256 MB the Lambda crashed during init on every TaskTable-stream event (Runtime.OutOfMemory, 255/256 MB) — it bundles createTaskCore + the Bedrock/S3 SDK stack. Bumped to 512 MB to match the webhook processor running the same code. 2. orchestration_id resolution: createTaskCore persists channel metadata as a nested channel_metadata MAP, never a top-level attribute, but the reconciler's stream parser read img.orchestration_id?.S (top-level) and so skipped every orchestration child. Now reads channel_metadata.M.orchestration_id.S (top-level kept as fallback). Tests: the handler test builder now nests orchestration_id under channel_metadata (the real persisted shape) instead of top-level — the unrealistic top-level shape is what hid bug #2.
…ws-samples#304) Drives seed -> releaseReadyChildren -> REAL createTaskCore -> persisted TaskRecord -> simulated TaskTable stream image -> parseTerminalTaskRecord -> computeReconcilePlan against a stateful in-memory DynamoDB fake. Closes the gap that let the first dev smoke ship 3 bugs the mocked unit tests couldn't catch — verified by reverting each fix: - idempotency key '#' -> the real validator 400s -> test fails - orchestration_id round-trips under nested channel_metadata (asserts the exact shape the reconciler stream-parse must read; top-level is absent) - idempotent replay: releasing the same child twice yields ONE TaskRecord The fake implements the Put/Get/Query/Update + ConditionExpression subset + IdempotencyIndex/ChildTaskIndex GSIs these handlers actually use.
The CDK suite OOM'd the Mac: jest defaulted to ~9 workers on a 10-core machine, each loading the full CDK-synth context with coverage always on. - jest config: maxWorkers 50% + workerIdleMemoryLimit 1536MB (recycles a worker that balloons past 1.5GB — the actual OOM killer; helps CI too). - new 'mise //cdk:testf' task: --coverage=false --runInBand for iterating on one file without spawning the worker fleet. //cdk:test stays the full coverage/CI-parity run.
… (F1/F2) (aws-samples#247) Extends the aws-samples#304 integration test and hardens the reconciler: - F1 fix: the reconciler now RE-READS the orchestration after persisting the terminal child's status, and releases any blocked child whose deps are all persisted-succeeded — instead of trusting the initial snapshot. Closes the lost-update where two predecessors completing simultaneously each saw a stale snapshot and neither released the dependent. - child_task_id added to OrchestrationChildRow (was stamped but untyped). - reconciler unit-test mock made stateful/query-type-aware so it reflects status writes — the re-read path is now actually exercised. - integration test (aws-samples#304): added webhook-replay e2e, diamond timing (wired through the real reconciler), build_passed=false skip, and a concurrent-predecessors case marked test.failing as the executable witness for F2 (double task-create under concurrency — the re-read fix exposes it because releaseChild is create-then-flip, so the conditional flip serializes the row but not the irreversible createTaskCore). - docs/research: a correctness worksheet framing the reconciler as proof obligations + adversarial schedules (S1-S5), and the stacked-PR merge-flow findings (GitHub auto-retarget, bottom-up merges). F2's real fix (flip-then-create / atomic claim, with aws-samples#303's sweep as the liveness backstop) is scoped but not yet implemented.
Scheduled sweep that recovers orchestrations whose terminal child events were lost while the live reconciler was unavailable (deploy/throttle/OOM/ DLQ-parked) — the failure hit live on dev when a child completed during a reconciler OOM window and its dependent never released. For each active orchestration the sweep re-derives gating truth from persisted state and: - recovers LOST TERMINAL events: a 'released' child whose task already reached terminal but whose row never advanced → advance to succeeded/failed (build_passed-aware), - recovers LOST RELEASE events: release any blocked child whose deps are all succeeded, and cascade-skip children with a failed/skipped dep. Idempotent (releaseChild idempotency key + conditional row flips), so it is safe to re-run and to race the live reconciler. This is also the liveness backstop the F2 fix (flip-then-create) will rely on — a child stuck mid-release is recovered here. Construct mirrors StrandedTaskReconciler (10-min rate schedule, 512 MB to match the live reconciler's createTaskCore bundle); wired in agent.ts with the same repo/guardrail/orchestrator/linear-oauth grants. 9 tests; synth + cdk-nag clean.
…amples#247 A4) Pure decision core for where a released child branches from, so it sees its predecessors' code without waiting for a human merge: - 0 predecessors (root) → default branch - 1 predecessor (linear) → stack on its branch (true stacked PR) - N predecessors (diamond) → branch off default + merge all predecessor branches into the child's branch (multi-dep is first-class per design) Dedups predecessor branches before the count check (two preds on the same branch = one stack target, not a diamond) and degrades to root if no predecessor has a usable branch_name (never emits an invalid base). No I/O — the release path (next) feeds it predecessor branch_names and threads {base_branch, merge_branches} to the agent. Design + multi-dep rationale in docs/research/a4-stacked-base-branch-design.md. 9 tests.
…s#247 A4) Released children now branch so they SEE their predecessors' code without waiting for a human merge: - root (0 preds) → branch off main (unchanged) - linear (1 pred) → stack on the predecessor's branch (base = its branch) - diamond (N preds)→ branch off main, merge ALL predecessor branches in Threading (platform): - releaseChild persists the released child's branch_name on its row (child_branch_name) so a dependent's release can stack on / merge it. - releaseReadyChildren takes the full child set + derives each child's base via selectBaseBranch from predecessors' persisted branches; passes base_branch + merge_branches to releaseChild → channel_metadata. - orchestrator reads orchestration_base_branch / orchestration_merge_branches from channel_metadata into the agent payload (base_branch + merge_branches); PR-task resolved_base_branch still wins when set. - reconciler + aws-samples#303 sweep pass the full child set for base derivation. Threading (agent contract): - TaskConfig gains merge_branches; server→pipeline→build_config thread base_branch + merge_branches from the payload. - repo.py: a new_task with base_branch fetches + branches from it (was PR-task only); then merges each predecessor branch. A merge CONFLICT is aborted (keeps the setup tree clean) and noted so the agent integrates origin/<pred> as part of its task — conflict resolution stays agent-driven, never corrupts the deterministic setup phase. base_branch also becomes the PR base + commit-diff range. Diamond multi-dep is first-class (per design doc). Re-merge churn when a predecessor is edited in review is the aws-samples#305 follow-up's scope. Tests: base-branch selection (9), agent models (+2), integration A4 cases (linear/diamond/root, 3) — all green; synth + drift clean.
…amples#247 A5) - orchestration-rollup.ts: render + post an aggregate rollup comment on the PARENT issue when every child reaches terminal (the fan-out plane only comments per-child sub-issue). Pure renderer (complete / partial_failure / cancelled, per-child status + counts) + best-effort postRollup that never fails the reconcile. Wired into the reconciler's completion branch; reconciler construct gains the workspace-registry read grant + env to resolve the per-workspace OAuth token. - orchestration-log-events.ts: central catalogue of stable, greppable log-event names (orch.<phase>.<outcome>) as a TEST CONTRACT — the orchestration plane is event-driven with no sync API, so automated dev/e2e tests assert on these. Rollup emits orch.rollup.posted / orch.rollup.failed with bound ids. - docs: ROADMAP row + LINEAR_SETUP_GUIDE 'Parent/sub-issue orchestration' section (discovery, dependency-ordered execution, stacked PRs, rollup, failure handling) incl. the documented limitations (no whole-epic cancel button yet; child-cancel cascades; diamond re-integration is the aws-samples#305 follow-up). Starlight mirrors synced. Cancel scope (decided): child-cancel already cascades via the reconciler (A3) — A5 adds the rollup + documents that there's no whole-epic cancel entry point yet. 10 rollup tests; reconciler + integration unaffected; synth + drift clean.
…amples#247 A5) - orchestration-rollup.ts: render + post an aggregate rollup comment on the PARENT issue when every child reaches terminal (the fan-out plane only comments per-child sub-issue). Pure renderer (complete / partial_failure / cancelled, per-child status + counts) + best-effort postRollup that never fails the reconcile. Wired into the reconciler's completion branch; reconciler construct gains the workspace-registry read grant + env to resolve the per-workspace OAuth token. - orchestration-log-events.ts: central catalogue of stable, greppable log-event names (orch.<phase>.<outcome>) as a TEST CONTRACT — the orchestration plane is event-driven with no sync API, so automated dev/e2e tests assert on these. Rollup emits orch.rollup.posted / orch.rollup.failed with bound ids. - docs: ROADMAP row + LINEAR_SETUP_GUIDE 'Parent/sub-issue orchestration' section (discovery, dependency-ordered execution, stacked PRs, rollup, failure handling) incl. the documented limitations (no whole-epic cancel button yet; child-cancel cascades; diamond re-integration is the aws-samples#305 follow-up). Starlight mirrors synced. Cancel scope (decided): child-cancel already cascades via the reconciler (A3) — A5 adds the rollup + documents that there's no whole-epic cancel entry point yet. 10 rollup tests; reconciler + integration unaffected; synth + drift clean.
The A5 commit accidentally swept cdk/.jest-cache/ into git via 'git add -A' (the new //cdk:testf task generates it). It's already in.gitignore; remove the 63 tracked cache files so the PR diff is clean.
…ion (aws-samples#247 A4) A4 extracted base_branch + merge_branches in _extract_invocation_params but (a) never put them in the returned params dict and (b) referenced a bare 'base_branch' in _run_task_background — so every agent task crashed on invocation with 'NameError: name base_branch is not defined'. The agent booted, threw immediately, and never cloned/commented/finished, so orchestration children all failed (and the reconciler correctly skipped their dependents + posted a partial-failure rollup — the orchestration plane worked; the agent wiring did not). Fix: add base_branch/merge_branches to the params dict + as parameters of _run_task_background. Why it wasn't caught: A4's agent tests mock at the TaskConfig/run_task level and the CDK integration test stops at createTaskCore — nothing exercised the server.py invocation boundary (the _extract_invocation_params dict → _run_task_background(**kwargs) → run_task seam). Added TestInvocationParamContract: uses inspect.signature to assert every extracted key is a valid _run_task_background parameter and that the dict binds to the signature — structurally catching this class of wiring drift (verified: reverting the fix fails 3 of the new tests).
…#247 A5) The rollup posted TWICE on a completed orchestration (seen live): the 'all children terminal' check passes on more than one TaskTable-stream event — the last child's record typically gets two MODIFYs (status→ COMPLETED, then pr_url/build_passed written), each observing all-terminal. Add claimRollup: a conditional UpdateItem that stamps rollup_posted_at on the parent-meta row only if absent. The reconciler posts the comment only when it wins the claim; a racing/repeat invocation loses the conditional write and skips (logs duplicate_suppressed). Mirrors the release-flip exactly-once pattern. 3 tests (win / lose / error-propagate); verified the duplicate is gone.
…#247 A5) The rollup posted TWICE on a completed orchestration (seen live): the 'all children terminal' check passes on more than one TaskTable-stream event — the last child's record typically gets two MODIFYs (status→ COMPLETED, then pr_url/build_passed written), each observing all-terminal. Add claimRollup: a conditional UpdateItem that stamps rollup_posted_at on the parent-meta row only if absent. The reconciler posts the comment only when it wins the claim; a racing/repeat invocation loses the conditional write and skips (logs duplicate_suppressed). Mirrors the release-flip exactly-once pattern. 3 tests (win / lose / error-propagate); verified the duplicate is gone.
aws-samples#247) In a stacked sub-issue orchestration the deploy screenshot was routed to the wrong Linear issue: extractLinearIdentifier returns the first ABCA-NNN in document order, and an agent's PR body commonly names a predecessor issue ("cherry-picked from ABCA-151... Closes ABCA-152") before the one it closes. Result on the Lisbon epic: ABCA-151 got two screenshots, ABCA-153 got none. Route on the PR head branch name instead — bgagent/{taskId}/abca-NNN-... deterministically encodes the PR's own issue (added extractLinearIdentifierFromBranch). Title/body remain as fallbacks. Also harden findPullRequestForSha to pick the open PR whose head IS the deploy SHA (the commit-pulls API lists every PR stacked above it), and carry the head ref through OpenPr. Adds the stacked-PR regression contract test and branch-extractor unit tests.
# Conflicts: # agent/src/repo.py # cdk/src/constructs/github-screenshot-integration.ts # cdk/src/constructs/screenshot-bucket.ts # cdk/src/handlers/github-webhook-processor.ts # cdk/src/handlers/github-webhook.ts # cdk/src/handlers/shared/agentcore-browser.ts # cdk/src/handlers/shared/github-webhook-verify.ts # cdk/src/handlers/shared/linear-issue-lookup.ts # cdk/test/handlers/github-webhook-processor.test.ts # cdk/test/handlers/shared/agentcore-browser.test.ts # cdk/test/handlers/shared/github-webhook-verify.test.ts # cdk/test/handlers/shared/linear-issue-lookup.test.ts # cli/src/commands/github.ts # docs/guides/DEPLOY_PREVIEW_SCREENSHOTS_GUIDE.md # docs/src/content/docs/using/Deploy-preview-screenshots-guide.md
…ced by full suite Three stale assertions on the aws-samples#247 branch (not caused by the upstream merge; all predate it): - agent.test.ts expected 14 DynamoDB tables; aws-samples#247 adds the OrchestrationTable (15th) — bump the count + comment. - linear-webhook-processor-orchestration: the lib-dynamodb mock omitted QueryCommand/UpdateCommand/BatchWriteCommand (orchestration-store uses them); the seeded-graph test didn't mock the post-seed loadOrchestration Query. Add both. - 'no workspace token' test asserted single-task fallback, but aws-samples#200 changed the handler to DROP the event when the registry token won't resolve (don't burn agent quota on an unrecognized workspace). Update the test to the intended drop behaviour.
The upstream merge brought in aws-samples#248's workflow-driven task model, which replaced config.PR_TASK_TYPES / config.task_type with workflow-id-based detection exposed as TaskConfig.is_pr_workflow. The merge resolution kept our aws-samples#247 repo.py (for the A4 stacked-child base_branch block) but that file still imported PR_TASK_TYPES and branched on config.task_type — a symbol upstream deleted. Every agent task crashed at container runtime with 'ImportError: cannot import name PR_TASK_TYPES from config' (invisible to the test suite, which mocks config — caught only by a live run). Switch the two PR-branch checks to config.is_pr_workflow (matching upstream's repo.py) and drop the dead import; keep the aws-samples#247 A4 'elif config.base_branch' stacked-child block intact.
Live retest (Marrakesh epic) proved the agent ignores the platform-
provisioned branch bgagent/{task_id}/abca-NNN: it runs git checkout -b
feat/... and commits/PRs there instead. This is LLM drift, NOT a aws-samples#296
regression — the prompt text is byte-identical pre/post aws-samples#296; the agent
simply wasn't following the existing 'push to {branch_name}' line.
The rename silently breaks three platform contracts that key off the
provisioned branch: pr_url reporting (agent's commits never land on the
tracked branch -> ensure_pr finds nothing -> pr_url=null), screenshot->
Linear routing, and aws-samples#247 A4 stacking (the stacked child fetches the
predecessor's provisioned branch, which was never pushed -> falls back to
branching off main, so children don't actually stack).
Strengthen the instruction: base.py now explicitly forbids creating a new
branch and explains why (platform tracks work/preview/issue by this exact
branch); new_task.py PR step requires --head {branch_name}. Prompt-side
half of the defense-in-depth fix; platform-side reconcile/verify is
tracked separately (#12/#13/#14).
This reverts commit 222c8fd.
…, not default/agent-v1 aws-samples#296 (workflow-driven tasks) replaced the task_type→workflow mapping with a resolution ladder, but left the repo-aware rung ('Phase 4') unwired. Every task with no explicit workflow_ref — all Slack tasks and all aws-samples#247 orchestration children (neither sets one) — fell through to the platform default default/agent-v1: the freeform, repo-less agent prompt with NO git/PR discipline. The agent then improvised (gh api / gh pr create against an empty local clone), so ensure_pr found no commits and recorded pr_url=null, screenshot→Linear routing lost its branch signal, and aws-samples#247 A4 stacking broke (children couldn't fetch the unpushed predecessor branch). Re-wire the missing rung minimally: resolveWorkflowRef takes hasRepo; an absent ref with a repo present resolves to coding/new-task-v1 (the disciplined coding workflow — edit locally, commit, push, platform opens the PR via ensure_pr), matching pre-aws-samples#296 behaviour. Repo-less tasks still default to default/agent-v1. An explicit workflow_ref always wins. The single create-task-core call site passes Boolean(body.repo), so both Slack and orchestration (which create via createTaskCore with repo set) inherit the fix. Upstream regression — worth an upstream issue too. Updated workflows + create-task-core tests; the old test asserting default/agent-v1 for a repo task encoded the regression.
…A4 stacking (#14) setup_repo only used config.branch_name when is_pr_workflow; for new_task it re-derived its own slug from the title via shell.py slugify. That slug diverges from the platform's gateway.ts slugify: dots become '' vs '-' (guide.html -> agent 'guidehtml' vs platform 'guide-html') and it truncates at 40 vs 50. The platform persists its name as the TaskRecord branch_name AND, for stacked children, as the predecessor's child_branch_name that the reconciler hands to the next child as base_branch. Because the agent pushed a DIFFERENTLY-named branch, the stacked child's 'git fetch origin <base>' 404'd ('couldn't find remote ref') and silently fell back to branching off main — so A4 linear stacking was defeated: children didn't build on predecessors (observed Seville epic: PR#83 quiz had no index.html from the home-page child; every PR based on main, predecessor files duplicated). Use config.branch_name verbatim whenever the platform provided one (all workflows), re-deriving only as a no-branch fallback. Adds test_repo.py covering branch selection (the seam was previously only ever mocked — the bug slipped because nothing exercised setup_repo's branch logic directly).
…tion (aws-samples#247 #10) The parent epic spawned no task, so Linear's GitHub automation (which moves the child sub-issues to In Review on PR-open) never touched it — it sat in Backlog through the whole run with only a rollup comment. Give the parent the SAME lifecycle signal as its children, both state AND emoji reaction: - seed (first time only): 👀 reaction + transition to In Progress - all children succeed: ✅ reaction + transition to In Review (NOT Done — child PRs are open/unmerged) - partial failure / cancelled: ❌ reaction, leave state in place (the rollup comment conveys the outcome) New transitionIssueState resolves the team's workflow states (state IDs are per-team UUIDs, not knowable a priori), picks the target by semantic type + name preference, and issues issueUpdate. Backward-guard ranks by state TYPE (backlog<unstarted<started<completed) then position — never demotes a human-advanced epic (a position-only guard would wrongly demote Done→In Review since Done's position 3 < In Review's 1002). All best-effort; the state/reaction run after the load-bearing rollup comment and never gate it. Tests: transitionIssueState (state-pick by name, backward-guard across types, already-there no-op, no-token, missing-type), postRollup (complete→In Review+✅, partial→no-transition+❌, comment-fail→neither), processor seed (👀+In Progress, replay-skip).
Formatting-only follow-up to 8a311a8 — ruff collapsed the wrapped boolean onto one line during the full build. Committing so CI's 'fail build on mutation' gate (uncommitted lint output) stays green. No logic change.
…BCA-662 real blocker) setup_repo ran `mise trust <repo_dir>` — which trusts ONLY the root mise.toml. A monorepo has per-package config ROOTS (cdk/mise.toml, cli/mise.toml, agent/mise.toml, docs/mise.toml). When the baseline `mise run build` fans out into //cdk:eslint etc. it loads a nested, UNtrusted config → `mise ERROR Config files … are not trusted` → exit 1, and the whole build/lint gate dies in ~seconds BEFORE anything compiles. This was the actual cause of every fork baseline-build failure this session — NOT a test failure, NOT the OOM (that was a separate, real 64GB OOM on an earlier run). The ECS log showed only `mise trust <root>: OK` then a fast exit-1; a clean local clone reproduced the exact `not trusted` error on the nested config. (`mise trust --all` only covers cwd + PARENTS, not children, so it can't fix this.) Fix: after trusting the root, walk the clone and `mise trust` every nested mise.toml (skipping node_modules/.git/cdk.out/etc). _find_mise_configs is pure + unit-tested (root excluded, nested included, vendored dirs pruned).
…ust 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.
…ation guard) Formatting-only follow-up to 3fe99c9 — ruff switched the embedded-double-quote string to single outer quotes. No logic change; committing so CI's mutation gate stays green.
…loudWatch The platform gap behind the whole ABCA-662 investigation: run_cmd used subprocess.run(capture_output=True), which buffers the ENTIRE command output in a Python string and NEVER writes it to container stdout — so awslogs (→ CloudWatch) never saw the raw `mise run build` stream. The only record was whatever the post-hoc summary emitted, so a build failure was diagnosable ONLY if the summary happened to capture the right lines. For a parallel 4-package / 3000-test / ~30min build that meant days of not being able to see WHICH sub-task failed — the failing line was buffered away and shipped nowhere. Fix at the source: add run_cmd(stream=True) — a Popen + two drain threads (one per pipe, deadlock-free, streams kept separate) that write every line to log() (→ CloudWatch verbatim, redacted) AS IT RUNS while still returning a CompletedProcess matching subprocess.run's contract (all 24 callers read .stdout/.stderr unchanged). Wired into the build & lint verify commands only (baseline in repo.py + post-agent in post_hooks.py); the many short git/gh/mise-install commands stay buffered. Now the full build log always exists in CloudWatch + live progress replaces the silent multi-minute gap. On failure the streamed path adds a 'failing lines' pointer (full output already streamed above) instead of re-dumping. 6 tests (real `sh -c`: live stream, separate streams, mid-stream failure surfaced, redaction, check=True raise). Full agent suite 1315 green.
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.
…(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.
…r-test timeout Two orthogonal ECS-parity build-gate defects, both surfaced dogfooding on the fork (ABCA-684/685/686) where the baseline `mise run build` either OOM'd or stalled past its 3600s ceiling. A live matrix proved the build-command concurrency knob (MISE_JOBS 1/2/unbounded) was a red herring — cdk:test always finished in ~64s; the stalls were memory (jest) and a hung agent test. 1) jest maxWorkers is CORE-relative → OOMs the big box. cdk:test's `maxWorkers: 25%` is a % of CPU cores, and each worker holds a full ~700-resource CDK stack synth. On the 16-vCPU ECS build box that's 4 workers, whose combined synths + the resident agent blew past the 120 GB cap → SIGKILL (exit 137; MemoryUtilized pinned at the cap for 8 min). Provisioning a bigger box FOR SPEED made memory WORSE. Fix: the test script now honors `JEST_MAX_WORKERS` (default 25% — CI's 2-4 core / ~16 GB runner and dev machines are unaffected), and the ECS BUILD task def sets JEST_MAX_WORKERS=2 — an ABSOLUTE cap that decouples peak RAM from core count. (The sizing comment already anticipated 'cap jest workers'.) 2) agent pytest can hang on ECS → burns the whole build-verify budget. The 1315 agent tests run in ~7s locally but hung >40 min on ECS (an env-specific test blocking on network/subprocess/Bedrock with no timeout of its own), stalling the baseline until the 3600s guard. Fix: add pytest-timeout with a 120s per-test cap (thread method) so a hung test fails LOUD with a traceback instead of a silent 60-min stall. Full build green; agent suite 1315 passed in 7.4s with the plugin active.
…ually 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.
…(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.
… ~99% of build wall-clock) cdk:test dominates the build (3099 tests / ~156s local; ~99% of wall-clock), and each worker holds a full ~700-resource CDK stack synth (~15-18 GB). Empirical envelope on the 120 GB build box: 2 workers ran clean (ABCA-687), 4 OOM'd (ABCA-685, pinned 122879/122880). 3 is the measured-safe bump — ~50% more parallelism on the dominant phase while staying clearly under the OOM level. Local dev + CI keep the 25% default (JEST_MAX_WORKERS unset). Raise to 4 only on a live MemoryUtilized curve showing headroom.
…ot tens of GB) Corrects an earlier over-cautious estimate. cdk:test at 4 workers peaks at only ~2.2 GB across the whole process tree (sampled on a 16 GB Mac, no swap). The ABCA-685 OOM was NOT cdk:test's worker count — it was TOTAL build concurrency (full-parallel mise: cdk:test + agent:test + cli + docs + synth + the resident agent at once). So 4 jest workers is comfortably safe on the 120 GB box; the real memory driver is cross-package parallelism, not jest's internal fleet. Kept as an explicit env so a bigger box can't silently over-spawn; local/CI keep 25%. Will live-verify the ECS MemoryUtilized curve on the next fork run.
…r (kills the --no-verify OOM) The target repo's `mise run install` installs a prek pre-push hook that re-runs the FULL cdk+cli+agent suite on every git push. In the agent container that suite already ran twice (baseline + post-agent build gate) and CI runs it again, so the pre-push run is pure redundancy — and it runs UNcapped (no JEST_MAX_WORKERS), stacking on the resident agent → OOM. The agent's only escape was `git push --no-verify`, which bypassed ALL hooks (incl. the security scan) and trained a skip-verification habit (the honesty gap: agent reported build_passed while its push had skipped verification). Set SKIP=monorepo-tests-pre-push (the pre-commit/prek standard env) on the build task def — scoped to ONLY the tests hook, so pushes succeed without --no-verify while the pre-push SECURITY scan still runs. Propagates to platform + agent pushes via shell.py::_clean_env (blacklist). Deploys alongside JEST_MAX_WORKERS=4.
…CS test-hang The agent build gate (`mise run build` in the cloned repo) has intermittently wedged on the ECS substrate with the test suite hanging silently for ~53 min, up to the platform's 3600s build-verify ceiling (ABCA-684/686/688 + the warm-cache run). pytest-timeout with method="signal" did NOT fire: SIGALRM only interrupts the MAIN thread during a test's call phase, so a hang in a worker thread (a deadlocked Barrier/join), a fixture, collection, or a C-level socket read is invisible to it. Add the instrument that IS immune to all of those: - pyproject.toml: faulthandler_timeout=300 — per-test all-thread stack dump. - tests/conftest.py: a session-level faulthandler.dump_traceback_later(1200, exit=True) watchdog on a dedicated C thread (immune to the GIL and blocked syscalls) that dumps every thread and HARD-EXITS at 20 min, well inside the 3600s ceiling. The next hang self-reports its exact file:line instead of stalling blind. Also close a real cross-test leak that is a candidate root cause of the hang: aws_session caches the resolved boto3 session in a module global, and tenant_client returns session.client(...) from that cache when scoped — bypassing a downstream @patch("boto3.client"). Only test_aws_session reset the cache; a scoped session left cached by any other test could make a later test (e.g. test_attachments) issue a REAL S3 call that hangs on the ECS network. The autouse _clean_env fixture now calls reset_session_cache() so every test resolves the session under its own patches. Full agent suite (1315) green; mise run quality green (coverage 80.90%).
…ity) The ECS build task role gets MEMORY_ID in its container env (so the agent attempts write_task_episode / write_repo_learnings) but was never granted grantReadWrite on the AgentMemory. The writes use the task role's ambient credentials, so they failed closed with: AccessDeniedException … not authorized to perform: bedrock-agentcore:CreateEvent → memory_written: false on EVERY ECS task; cross-task learning silently no-ops on the ECS-only fork (live-caught on ABCA-691). The AgentCore runtime and the orchestrator both get this grant via agentMemory.grantReadWrite; the ECS task role did not. This was fixed once before (c23d49f) on a side-branch that never merged to linear-vercel, so a redeploy from trunk silently reverted it — this lands it on the branch the platform actually deploys from. - EcsAgentCluster gains an optional `agentMemory` prop and grants the task role read+write when wired (mirrors agent.ts's grant to the runtime). - agent.ts passes agentMemory to the ECS cluster. - Tests: assert the bedrock-agentcore write grant is present when agentMemory is wired, AND a negative proof that no such grant exists when it is omitted (so the positive assertion can't pass vacuously — the exact gap that let this regress silently). Full build green (cdk 3098 tests).
…) + readable pytest Two dogfood findings from the ABCA-691 run, plus a latent classifier bug the log exposed. 1) PREVENT the OOM (K14). `mise run build` fans out its four packages in parallel (agent:quality ‖ cdk:build ‖ cli:build ‖ docs:build), each spawning its own worker fleet; the MEASURED driver of the 32/64/120 GB OOMs is that cross-package storm summing on top of the still-resident coding agent — not any one package. At 120 GB (Fargate's max at 16 vCPU) there is no more RAM, so the documented remedy is to cut peak parallelism: set MISE_JOBS=1 on the build task def so the four packages run SEQUENTIALLY (peak ≈ max-of-one, not sum-of-four) while every package still builds and both gates (baseline + post-agent) stay intact. Within-package parallelism (JEST_MAX_WORKERS=4, pytest) is untouched. Cost is wall-clock, trivial vs BUILD_VERIFY_TIMEOUT_S=3600. 2) CLASSIFY an OOM honestly. A bare SIGKILL (137) was NOT treated as infra unless stderr also carried an OOM string — but the cgroup OOM-killer writes to the KERNEL log, not the build's stderr, so an OOM'd build exits 137 with no such string. On ABCA-691 that 137 was mislabeled INERT (a greedy mise+"not found" heuristic matched an unrelated webhook-test fixture line); a 137 with plain output would have fallen through to a GENUINE build FAILURE → a false gate on healthy code (and a poisoned dependent cascade). is_infra_failure now treats a bare 137 as infra (a healthy build never SIGKILLs itself — a real test failure exits 1/2), checked before the inert/failure paths. Tests cover both the mislabeled-inert and the false-failure fall-through. 3) READABLE test output. pytest's default dot progress + easily-lost summary made an agent burn ~6 turns re-running the suite to confirm it passed under capture. addopts=-ra + console_output_style=count → explicit "N/M passed" and a trailing non-pass summary in any non-TTY log (CI, ECS gate, agent Bash). Display-only; never changes results. Full build green (cdk 3099 + agent quality).
…ted failure, don't diagnose it
The ABCA-662 work re-titled a max_turns failure as "Ran out of turns retrying a
failing step". That over-claims: the stuck-guard's trailing window is only the last
~6 tool calls, which genuinely CANNOT distinguish a hard blocker (bad creds, no
permission — more turns won't help) from a long task that made real progress and
hit a recoverable snag only at the tail (more turns / a retry WOULD help — 662
itself: siblings pushed fine with the same token). Framing the whole run around its
last 6 calls misrepresents the latter.
- error-classifier: drop the separate "retrying a failing step" bucket. A max_turns
failure stays the plain "Exceeded max turns"; the copy points the reader at the
observed detail and still surfaces the environment-blocker path, but asserts NO
cause.
- stuck_guard.recent_failure_summary: emit a NEUTRAL observation ("last tool calls
repeated: `<cmd>` → <err>") instead of "spinning on failing tool calls". The
mid-run steer TO the agent (an advisory nudge, not a user surface) still says
"spinning" — it's coaching the agent, not classifying the outcome.
- tests pinned to the neutral wording + assert the title is NOT re-framed.
# Conflicts: # cdk/src/handlers/orchestrate-task.ts
When `build`/`integ` complete on a PR, aggregate all check-runs + commit statuses and either (a) comment which checks are failing (no review compute spent), (b) auto-update an out-of-date branch so CI re-runs (PAT so build re-fires), (c) comment asking to resolve conflicts, or (d) trigger the ABCA coding/pr-review-v1 agent via the Task API webhook once green. Comments-only, edit-in-place, per-SHA idempotent — does not touch Mergify or the integ-smoke gate. Runs in the trusted base-repo context via workflow_run so secrets/PAT are available for fork PRs; no PR code is checked out or executed. Adds review-gate.yml to .github/zizmor.yml dangerous-triggers and secrets-outside-env ignore lists (intentional workflow_run + repo-level secrets; the gate has no environment/approval boundary).
Operator walkthrough for wiring the review-gate workflow: register an ABCA webhook (bgagent webhook create), set the ABCA_TASK_API_URL / ABCA_WEBHOOK_ID repo vars and ABCA_WEBHOOK_SECRET secret, confirm the workflow is on the default branch, and smoke-test. Covers the decision tree, fork-vs-same-repo update behavior, opting PRs out, and troubleshooting (401/403, not-onboarded, green-but-no-review, dismissed approvals). Mirrors to using/ via sync-starlight (script entry + link route + sidebar slug), and links from USER_GUIDE's webhook bullet. Docs build verified.
A Linear task against the full ABCA monorepo wedged silently for 50+ min: the pre-agent baseline build (mise run build → the agent pytest suite) hung on the ECS substrate, and the issue showed no reaction/comment/state the whole time. Root-caused live on ABCA-707. Three fixes: 1. Kill the hang at its root (agent/tests/conftest.py). The ECS agent task def sets AGENT_SESSION_ROLE_ARN. With it set, aws_session resolves a *scoped* session and tenant_client returns session.client(...), which BYPASSES a @patch("boto3.client") mock. test_attachments then makes a REAL S3 get_object that blocks forever on the ECS network (no egress) in a socket read SIGALRM can't interrupt. Add AGENT_SESSION_ROLE_ARN to the _clean_env scrub list so every test resolves the unscoped path where the mock intercepts. reset_session_cache() alone was insufficient — a cold get_session() re-resolves scoped while the var is still set. + regression test (TestConftestScrubsScopingEnv). 2. Make the hang watchdog actually reap (agent/tests/conftest.py). The prior faulthandler.dump_traceback_later(1200, exit=True) never fired: pytest's faulthandler_timeout re-arms faulthandler's single timer per-test WITHOUT exit=True, cancelling the session-level exit timer. Replace with an independent daemon-thread reaper that dumps all stacks and os._exit(1)s at 600s, so a future hang fails the build fast instead of burning to the 3600s ceiling. 3. Early Linear ACK (agent/src/pipeline.py). Move token resolve + 👀 reaction + Backlog→In Progress + start comment to BEFORE setup_repo() so a large-repo task shows immediate feedback during the multi-minute baseline build instead of looking dead. As a side benefit, a setup-phase failure now has a 👀 for the existing outer crash handler to swap to ❌ — closing the silent-stuck-issue gap. configure_channel_mcp stays after the clone (it needs the repo dir).
aws-samples#614) A follow-up `@bgagent <request>` comment on a completed Linear task that opened no PR was silently dropped. handleStandaloneCommentTrigger is iteration-only: on prNumber === null it checks for a clarify-hold and, if it isn't one, returns with just an info log — the comment vanished. But a task finishes PR-less in several real ways (no change needed, failed-before-commit, or a question/investigation run), and a follow-up on such an issue is almost always NEW work ("then just do X instead"), not iteration. Add maybeStartStandaloneNewWork: when the repo + user are known and the comment carries instruction text, dispatch a fresh coding/new-task-v1 against the same repo using the comment text as the description — reusing the same 👀-ack + threaded reply + fanout terminal ownership as the iteration and clarify-resume paths. Idempotency keyed newwork_<issue>_<comment>. Edge cases: a bare `@bgagent` (no instruction) gets a threaded reply telling the user what to do rather than a vague dispatch or silence; no repo / no user_id falls through to the (now-narrowed) no-op log. Tests: replace the obsolete 'NO PR → no task, no ack' case with the new-work dispatch, bare-mention reply, no-repo, and no-user cases. Full cdk build green (3118 tests). Closes aws-samples#614.
…to-lv-2026-07-15 # Conflicts: # agent/src/pipeline.py # cdk/src/constructs/ecs-agent-cluster.ts # cdk/src/constructs/ecs-payload-bucket.ts # cdk/src/handlers/fanout-task-events.ts # cdk/src/handlers/shared/strategies/ecs-strategy.ts # cdk/src/handlers/shared/workflows.ts # cdk/src/stacks/agent.ts # cdk/test/constructs/ecs-agent-cluster.test.ts # cdk/test/handlers/shared/create-task-core.test.ts # cdk/test/handlers/shared/strategies/ecs-strategy.test.ts # cdk/test/handlers/shared/workflows.test.ts
…rror aws-samples#596 B1) Mirrors the aws-samples#596 review B1 fix onto linear-vercel (this defect rode in via the main→lv merge a9efaa6, which kept lv's version of the grant). coding/decompose-v1 delivers its plan via the assumed SessionRole (scoped to artifacts/${task_id}/*), exactly like the AgentCore runtime whose task role has no direct artifacts grant. The whole-bucket grantReadWrite over-privileged the untrusted-code role and broke cross-task isolation. Task role keeps only the ARTIFACTS_BUCKET_NAME env; corrected the inverted comment + the stale artifacts clause in the cdk-nag IAM5 reason. Test flipped to assert the task role has NO s3:Put*/Delete* action. Full cdk build green.
The Task API validates Idempotency-Key against ^[a-zA-Z0-9_-]{1,128}$, but the
gate built it as `review-$REPO-$PR_NUMBER-$HEAD_SHA` where $REPO is `owner/repo`
— the "/" fails validation (and owner/repo + a 40-char SHA can exceed 128), so
the webhook returned 400 VALIDATION_ERROR and no review was ever triggered on a
green PR. Map disallowed chars to "-" and cap at 128; still per-(repo,PR,SHA)
unique. Verified live: bad key → 400, sanitized key → 201 (task created).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #618 +/- ##
=======================================
Coverage ? 89.29%
=======================================
Files ? 267
Lines ? 71628
Branches ? 7862
=======================================
Hits ? 63958
Misses ? 7670
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Area
cdk— infrastructure, handlers, constructsagent— Python runtime / Docker imagecli—bgagentclientdocs— guides or design sources (docs/guides/,docs/design/)tooling— rootmise.toml, scripts, CI workflowsTip: AGENTS.md lists where to edit and which tests to extend.
Related
Changes
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.