-
Notifications
You must be signed in to change notification settings - Fork 35
feat(ecs): make the ECS Fargate compute backend usable (context-gated wiring + ECS-parity fixes) #596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
isadeks
wants to merge
26
commits into
main
Choose a base branch
from
pr/ecs-substrate-hardening
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
feat(ecs): make the ECS Fargate compute backend usable (context-gated wiring + ECS-parity fixes) #596
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
c3f2b6f
fix(ecs): write task payload to S3, not inline overrides (#502)
331751e
Merge branch 'main' into fix/502-ecs-payload-s3-pointer
krokoko 85e141d
Merge branch 'main' into fix/502-ecs-payload-s3-pointer
isadeks 6e0db96
fix: enable corepack in the agent image so yarn/pnpm resolve
isadeks bef9b86
wire ECS/Fargate substrate (context-gated) at 32GB/8vCPU for heavy bu…
isadeks c6a2f28
bump ECS Fargate to 64GB/16vCPU + BUILD_VERIFY_TIMEOUT_S=3600
isadeks 5da571e
fix(ecs): map full payload to run_task so channel/build/cedar fields …
isadeks bf0a86d
fix(ecs): grant task role GetSecretValue on bgagent-{linear,jira}-oau…
isadeks 70efb8d
fix(ecs): wire ARTIFACTS_BUCKET_NAME into the Fargate task (ECS-parit…
isadeks b9da937
feat(compute): guard compute_type=ecs against a stack without the ECS…
isadeks 26ec150
fix(cdk): raise BUILD task memory 64→120 GB (max Fargate) — ABCA-662 …
isadeks 9add784
fix(cdk): make ecs-strategy top-of-file import hermetic vs ambient env
isadeks 4acccf3
fix(cdk): grant ec2:DescribeAvailabilityZones to ECS agent task role …
isadeks 209c66a
fix(ecs): grant ECS task role AgentCore Memory access (F-2, ABCA-488-…
a5550eb
docs(bootstrap): correct the ECS ComputeTypes enable instructions
isadeks 1ad937d
Merge remote-tracking branch 'upstream/main' into HEAD
isadeks 8e30da4
Merge branch 'main' into fix/502-ecs-payload-s3-pointer
isadeks ce93b62
fix(ecs): remove dead ECS_PAYLOAD_OBJECT_KEY_PREFIX export (review B1)
isadeks 8644f9c
docs+feat: address review nits N1–N4 on the ECS substrate
isadeks 1c5e64a
Merge branch 'main' into fix/502-ecs-payload-s3-pointer
krokoko cc93150
Merge branch 'fix/502-ecs-payload-s3-pointer' into pr/ecs-substrate-h…
isadeks d258e76
style: ruff-format the N4 payload-key block (fix build-mutation failure)
isadeks ca8af9a
fix+docs: WARN on dropped task_started_at (HITL parity) + defensive m…
isadeks faaf395
Merge remote-tracking branch 'upstream/main' into pr/ecs-substrate-ha…
isadeks c2a3f26
Merge branch 'main' into pr/ecs-substrate-hardening
isadeks 68c9735
fix(ecs): address #596 review — drop over-privileged artifacts grant …
isadeks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| """Unit tests for pipeline.run_task_from_payload — the ECS payload→run_task map. | ||
|
|
||
| Regression cover for ABCA-487: the ECS boot command used to hand-list a subset | ||
| of run_task kwargs and silently dropped channel_source/channel_metadata (no | ||
| Linear/Jira reactions on ECS), build_command, cedar_policies, base_branch, etc. | ||
| run_task_from_payload maps the WHOLE payload so nothing is dropped again. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import patch | ||
|
|
||
| from pipeline import _RUN_TASK_PARAMS, run_task_from_payload | ||
|
|
||
|
|
||
| def _capture(payload: dict) -> dict: | ||
| """Run the mapper with run_task replaced by a capturing stub; return kwargs.""" | ||
| seen: dict = {} | ||
|
|
||
| def fake_run_task(**kwargs): | ||
| seen.update(kwargs) | ||
| return {"status": "success"} | ||
|
|
||
| with patch("pipeline.run_task", side_effect=fake_run_task): | ||
| run_task_from_payload(payload) | ||
| return seen | ||
|
|
||
|
|
||
| class TestRunTaskFromPayload: | ||
| def test_renames_prompt_and_model_id(self): | ||
| seen = _capture({"prompt": "do the thing", "model_id": "anthropic.claude-x"}) | ||
| assert seen["task_description"] == "do the thing" | ||
| assert seen["anthropic_model"] == "anthropic.claude-x" | ||
| # The original payload keys must NOT leak through as-is (run_task rejects them). | ||
| assert "prompt" not in seen | ||
| assert "model_id" not in seen | ||
|
|
||
| def test_forwards_channel_fields_ABCA_487(self): | ||
| # THE regression: channel_source/channel_metadata must reach run_task so | ||
| # the Linear/Jira reaction + channel MCP fire on ECS. | ||
| cm = {"linear_issue_id": "iss-1", "linear_oauth_secret_arn": "arn:sm:...:lin"} | ||
| seen = _capture({"channel_source": "linear", "channel_metadata": cm}) | ||
| assert seen["channel_source"] == "linear" | ||
| assert seen["channel_metadata"] == cm | ||
|
|
||
| def test_forwards_cedar_attachments_trace_user_fields(self): | ||
| # HITL guardrails (cedar_policies, approval_*), attachments, trace, and | ||
| # user_id are real run_task params here — they must reach run_task on ECS | ||
| # (the hand-listed boot command used to drop them). | ||
| seen = _capture( | ||
| { | ||
| "cedar_policies": ["p1", "p2"], | ||
| "attachments": [{"filename": "x.png"}], | ||
| "trace": True, | ||
| "user_id": "user-9", | ||
| } | ||
| ) | ||
| assert seen["cedar_policies"] == ["p1", "p2"] | ||
| assert seen["attachments"] == [{"filename": "x.png"}] | ||
| assert seen["trace"] is True | ||
| assert seen["user_id"] == "user-9" | ||
|
|
||
| def test_drops_payload_keys_that_are_not_yet_run_task_params(self): | ||
| # build_command/lint_command (configurable verify, #1) and base_branch/ | ||
| # merge_branches (orchestration stacking, #247) are emitted by the | ||
| # orchestrator but are NOT run_task parameters on this branch. The mapper | ||
| # filters against run_task's REAL signature, so they are dropped rather | ||
| # than smuggled through as an invalid kwarg. When those params land, | ||
| # they forward automatically with no change here — that's the point of | ||
| # keying off inspect.signature instead of a hand-list. | ||
| seen = _capture( | ||
| { | ||
| "repo_url": "org/repo", | ||
| "build_command": "npm ci && npm test", | ||
| "lint_command": "npm run lint", | ||
| "base_branch": "epic-tip", | ||
| "merge_branches": ["a", "b"], | ||
| } | ||
| ) | ||
| assert seen["repo_url"] == "org/repo" | ||
| for not_yet in ("build_command", "lint_command", "base_branch", "merge_branches"): | ||
| assert not_yet not in seen | ||
|
|
||
| def test_coerces_issue_and_pr_number_to_str_and_max_turns_to_int(self): | ||
| seen = _capture({"issue_number": 42, "pr_number": 7, "max_turns": "50"}) | ||
| assert seen["issue_number"] == "42" | ||
| assert seen["pr_number"] == "7" | ||
| assert seen["max_turns"] == 50 | ||
| assert isinstance(seen["max_turns"], int) | ||
|
|
||
| def test_ignores_unknown_payload_keys(self): | ||
| # github_token_secret_arn is on the payload but is NOT a run_task param | ||
| # (it's consumed platform-side); passing it as **kwargs would TypeError. | ||
| seen = _capture( | ||
| { | ||
| "repo_url": "org/repo", | ||
| "github_token_secret_arn": "arn:...", | ||
| "sources": ["x"], | ||
| } | ||
| ) | ||
| assert seen["repo_url"] == "org/repo" | ||
| assert "github_token_secret_arn" not in seen | ||
| assert "sources" not in seen | ||
|
|
||
| def test_drops_none_values_so_run_task_defaults_apply(self): | ||
| seen = _capture({"repo_url": "org/repo", "base_branch": None, "channel_metadata": None}) | ||
| assert "base_branch" not in seen | ||
| assert "channel_metadata" not in seen | ||
|
|
||
| def test_aws_region_falls_back_to_env(self, monkeypatch): | ||
| monkeypatch.setenv("AWS_REGION", "us-east-1") | ||
| seen = _capture({"repo_url": "org/repo"}) | ||
| assert seen["aws_region"] == "us-east-1" | ||
|
|
||
| def test_explicit_aws_region_in_payload_wins(self, monkeypatch): | ||
| monkeypatch.setenv("AWS_REGION", "us-east-1") | ||
| seen = _capture({"repo_url": "org/repo", "aws_region": "eu-west-1"}) | ||
| assert seen["aws_region"] == "eu-west-1" | ||
|
|
||
| def test_every_forwarded_key_is_a_real_run_task_param(self): | ||
| # Guard: whatever the mapper forwards must be accepted by run_task, so a | ||
| # future payload key can never smuggle an invalid kwarg through. Compare | ||
| # against the module's real param set (run_task is patched in _capture). | ||
| accepted = _RUN_TASK_PARAMS | ||
| seen = _capture( | ||
| { | ||
| "prompt": "p", | ||
| "model_id": "m", | ||
| "repo_url": "r", | ||
| "issue_number": 1, | ||
| "channel_source": "linear", | ||
| "channel_metadata": {"a": "b"}, | ||
| "build_command": "b", | ||
| "cedar_policies": ["c"], | ||
| "base_branch": "x", | ||
| "attachments": [{}], | ||
| "trace": False, | ||
| "user_id": "u", | ||
| "pr_number": 3, | ||
| "hydrated_context": {"k": "v"}, | ||
| "resolved_workflow": {"id": "w"}, | ||
| } | ||
| ) | ||
| assert set(seen).issubset(accepted) | ||
|
|
||
| def test_warns_when_dropping_a_known_orchestrator_key(self): | ||
| # N4: a KNOWN orchestrator key that run_task doesn't accept is dropped | ||
| # (expected today) but logged, so a future "wired one side, forgot the | ||
| # other" contract gap (the ABCA-487 class) is visible, not silent. | ||
| logs: list[tuple[str, str]] = [] | ||
| with patch("pipeline.log", side_effect=lambda level, msg, **kw: logs.append((level, msg))): | ||
| _capture({"build_command": "mise run build", "repo_url": "r"}) | ||
| assert [m for level, m in logs if level == "WARN" and "build_command" in m], ( | ||
| "expected a WARN when dropping the known orchestrator key build_command" | ||
| ) | ||
|
|
||
| def test_does_NOT_warn_when_dropping_a_foreign_key(self): | ||
| # A genuinely-foreign key (not a known orchestrator field) is dropped | ||
| # quietly — no log noise for keys we never expected to forward. | ||
| logs: list[tuple[str, str]] = [] | ||
| with patch("pipeline.log", side_effect=lambda level, msg, **kw: logs.append((level, msg))): | ||
| _capture({"some_future_unrelated_key": "v", "repo_url": "r"}) | ||
| assert not [m for level, m in logs if "some_future_unrelated_key" in m] | ||
|
|
||
| def test_warns_when_dropping_task_started_at_HITL_parity(self): | ||
| # task_started_at drives the AgentCore HITL maxLifetime clip via | ||
| # TASK_STARTED_AT; the ECS path doesn't set it yet, so its drop must WARN | ||
| # (surface the AgentCore↔ECS parity gap, not silently fail-open). | ||
| logs: list[tuple[str, str]] = [] | ||
| with patch("pipeline.log", side_effect=lambda level, msg, **kw: logs.append((level, msg))): | ||
| _capture({"task_started_at": "2026-07-14T00:00:00Z", "repo_url": "r"}) | ||
| assert [m for level, m in logs if level == "WARN" and "task_started_at" in m] | ||
|
|
||
| def test_malformed_max_turns_is_dropped_not_raised(self): | ||
| # A non-integer max_turns must not crash the boot — it's dropped (run_task | ||
| # default applies) with a WARN, matching how every other field defaults. | ||
| logs: list[tuple[str, str]] = [] | ||
| with patch("pipeline.log", side_effect=lambda level, msg, **kw: logs.append((level, msg))): | ||
| seen = _capture({"repo_url": "r", "max_turns": "not-a-number"}) | ||
| assert "max_turns" not in seen | ||
| assert [m for level, m in logs if level == "WARN" and "max_turns" in m] | ||
|
|
||
| def test_valid_max_turns_still_coerced_to_int(self): | ||
| seen = _capture({"repo_url": "r", "max_turns": "50"}) | ||
| assert seen["max_turns"] == 50 | ||
| assert isinstance(seen["max_turns"], int) |
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (N1).
_KNOWN_ORCHESTRATOR_KEYSlistsbuild_commandbut not its siblinglint_command. Neither is emitted in the payload today, so no runtime effect — but if configurable-verify lands,build_commandwould drop with the intended WARN breadcrumb whilelint_commanddrops silently, the exact asymmetry this known-key WARN exists to prevent. The test attest_run_task_from_payload.py:240already iterates both as the same class.