docs+feat: address review nits N1–N4 on the ECS substrate

Review follow-ups for #596 (fix itself unchanged; B1 dead-export fixed in the
base PR #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.

Author: isadeks

agent/src/pipeline.py

@@ -1275,6 +1275,21 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
 #: shadow it). Any payload key not in this set is ignored, never passed through.
 _RUN_TASK_PARAMS = frozenset(inspect.signature(run_task).parameters)
 
+#: Orchestrator payload keys we KNOW about that ``run_task`` does not (yet)
+#: accept as a parameter. Dropping one of these is expected today (e.g.
+#: ``base_branch`` is consumed via ``hydrated_context``, not a run_task kwarg;
+#: ``github_token_secret_arn`` is resolved before this call), but a key that
+#: shows up here AND is silently dropped is exactly the "wired one side of an
+#: orchestrator→agent field, forgot the other" no-op that ABCA-487 was — so we
+#: WARN when we drop one, making a future contract gap visible instead of silent.
+#: Keys not in this set (genuinely foreign) are dropped quietly as before.
+_KNOWN_ORCHESTRATOR_KEYS = frozenset({
+    "build_command",
+    "merge_branches",
+    "base_branch",
+    "github_token_secret_arn",
+})
+
 
 def run_task_from_payload(payload: dict) -> dict:
     """Invoke :func:`run_task` from a full orchestrator payload dict.
@@ -1301,7 +1316,15 @@ def run_task_from_payload(payload: dict) -> dict:
     for key, value in (payload or {}).items():
         target = _PAYLOAD_KEY_ALIASES.get(key, key)
         if target not in _RUN_TASK_PARAMS:
-            continue  # not a run_task parameter — ignore (e.g. github_token_secret_arn)
+            # Not a run_task parameter — ignore. A KNOWN orchestrator key being
+            # dropped is expected today but worth a breadcrumb: if run_task ever
+            # grows a matching param, this WARN is where a "forgot to wire it
+            # through" no-op surfaces (N4 / ABCA-487 class). Foreign keys are
+            # dropped quietly.
+            if key in _KNOWN_ORCHESTRATOR_KEYS and value is not None:
+                log("WARN", f"run_task_from_payload: dropping known orchestrator key '{key}' "
+                    f"(not a run_task parameter) — consumed elsewhere or not yet wired")
+            continue
         if value is None:
             continue  # let run_task's default apply
         if target in _PAYLOAD_STR_KEYS:

agent/tests/test_run_task_from_payload.py

@@ -142,3 +142,22 @@ def test_every_forwarded_key_is_a_real_run_task_param(self):
             }
         )
         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]

cdk/src/constructs/ecs-agent-cluster.ts

@@ -83,8 +83,9 @@ export interface EcsAgentClusterProps {
    * ``bedrock-agentcore:CreateEvent``) succeed on the ECS substrate. The
    * AgentCore runtime role already gets this via ``agentMemory.grantReadWrite``
    * in agent.ts; without the same grant here, memory writes hit AccessDenied and
-   * silently no-op on ECS (WARN), so learning never persists on an ECS-only
-   * deployment. Omitted in isolated construct tests / memory-less deployments.
+   * no-op on ECS (logged, non-fatal — memory.py treats an AccessDenied as an
+   * infra failure), so learning never persists on an ECS-only deployment.
+   * Omitted in isolated construct tests / memory-less deployments.
    */
   readonly agentMemory?: AgentMemory;
 }
@@ -240,8 +241,8 @@ export class EcsAgentCluster extends Construct {
     // (write_task_episode / write_repo_learnings → bedrock-agentcore:CreateEvent)
     // succeed on ECS. The AgentCore runtime role gets this via
     // agentMemory.grantReadWrite(runtime) in agent.ts; without the same grant
-    // here the writes hit AccessDenied and silently no-op (WARN) on the ECS
-    // substrate, so learning never persists on an ECS-only deployment.
+    // here the writes hit AccessDenied and no-op on the ECS substrate (logged,
+    // non-fatal), so learning never persists on an ECS-only deployment.
     if (props.agentMemory) {
       props.agentMemory.grantReadWrite(taskRole);
     }
@@ -253,7 +254,9 @@ export class EcsAgentCluster extends Construct {
     // 👀→✅ reaction and drive the channel MCP. The AgentCore runtime role +
     // orchestrator/fanout/screenshot roles all have this prefix grant; the ECS
     // task role did NOT, so on ECS the token fetch hit AccessDenied and
-    // reactions/MCP silently no-op'd (ECS-parity gap, live-caught on ABCA-488).
+    // reactions/MCP no-op'd — logged by config.py's token resolver, not silent,
+    // but the channel effect (no 👀→✅, no MCP) is invisible to the user
+    // (ECS-parity gap, live-caught on ABCA-488).
     // GetSecretValue only — the container reads the token; the orchestrator owns
     // refresh/PutSecretValue.
     taskRole.addToPrincipalPolicy(new iam.PolicyStatement({

cdk/src/stacks/agent.ts

@@ -579,8 +579,9 @@ export class AgentStack extends Stack {
     // --- ECS Fargate compute backend (CONTEXT-GATED) ---
     // K12 (2026-06-29): AgentCore's fixed microVM envelope OOM-kills heavy
     // CI-parity builds (ABCA's own ~2800-test `mise run build`). ECS Fargate
-    // gives a tunable 64 GB / 16 vCPU task (see EcsAgentCluster) for repos that
-    // set ``compute_type: 'ecs'``. GATED on the ``compute_type`` deploy context
+    // gives a bigger, tunable task (see EcsAgentCluster for the exact vCPU/memory
+    // sizing + its OOM history — 64 GB was itself OOM-killed, so it runs larger)
+    // for repos that set ``compute_type: 'ecs'``. GATED on the ``compute_type`` deploy context
     // (default 'agentcore') — ECS resources only synthesize when you deploy with
     // ``--context compute_type=ecs``, so the default synth (and the
     // bootstrap-coverage test that synths with default context) stays