fix(ecs): address #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 #608 test-seam gaps (finalize deleteEcsPayload, boot-uri parse,
entrypoint re-export, zero-ECS-synth assert) remain tracked, not in scope here.

Author: isadeks

agent/src/pipeline.py

@@ -1328,7 +1328,13 @@ def _on_trace_truncated(max_bytes: int, first_dropped: int) -> None:
         "lint_command",
         "merge_branches",
         "base_branch",
-        "github_token_secret_arn",
+        # NB: ``github_token_secret_arn`` is deliberately NOT listed (N3). It is
+        # ALWAYS in the payload and ALWAYS dropped here (resolved via the
+        # ``GITHUB_TOKEN_SECRET_ARN`` env in build_config, never a run_task
+        # param), so listing it would fire the known-key WARN on 100% of ECS
+        # boots — pure noise that dilutes the channel meant to surface genuine
+        # future "wired one side, forgot the other" gaps. It falls through as a
+        # quiet foreign-key drop instead.
         # 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 does not (yet) set
@@ -1383,14 +1389,25 @@ def run_task_from_payload(payload: dict) -> dict:
         if target in _PAYLOAD_STR_KEYS:
             value = str(value)
         elif target == "max_turns":
-            # Defensive, matching how every other field is handled: a malformed
-            # max_turns must not crash the whole boot — drop it and let run_task's
-            # default apply (with a breadcrumb) rather than raise a ValueError.
+            # Defensive: a malformed max_turns must not crash the whole boot —
+            # drop it and let run_task's default apply (with a breadcrumb) rather
+            # than raise. Unlike the str keys above, this is the one field with a
+            # non-str coercion, so it also guards the surprising int() cases the
+            # orchestrator never emits but a hand-edited payload might: a bool
+            # (``int(True) == 1``) and a non-integral float (``int(3.9) == 3``)
+            # would both silently become a bogus turn count (N4).
+            if isinstance(value, bool) or not isinstance(value, (int, float, str)):
+                log("WARN", f"run_task_from_payload: ignoring non-integer max_turns {value!r}")
+                continue
             try:
-                value = int(value)
+                coerced = int(value)
             except (TypeError, ValueError):
                 log("WARN", f"run_task_from_payload: ignoring non-integer max_turns {value!r}")
                 continue
+            if isinstance(value, float) and coerced != value:
+                log("WARN", f"run_task_from_payload: ignoring non-integral max_turns {value!r}")
+                continue
+            value = coerced
         kwargs[target] = value
 
     kwargs.setdefault("aws_region", os.environ.get("AWS_REGION", ""))

agent/tests/test_run_task_from_payload.py

@@ -102,6 +102,18 @@ def test_ignores_unknown_payload_keys(self):
         assert "github_token_secret_arn" not in seen
         assert "sources" not in seen
 
+    def test_github_token_secret_arn_dropped_quietly(self):
+        # N3: github_token_secret_arn is ALWAYS present and ALWAYS resolved via
+        # the GITHUB_TOKEN_SECRET_ARN env (never a run_task param), so its drop is
+        # 100% expected and must NOT fire the known-key WARN — that channel is for
+        # genuine future contract gaps, not this always-dropped key.
+        logs: list[tuple[str, str]] = []
+        with patch("pipeline.log", side_effect=lambda level, msg, **kw: logs.append((level, msg))):
+            _capture({"github_token_secret_arn": "arn:aws:secretsmanager:...", "repo_url": "r"})
+        assert not [m for level, m in logs if "github_token_secret_arn" in m], (
+            "github_token_secret_arn must drop quietly (N3) — it is always resolved via env"
+        )
+
     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
@@ -143,6 +155,18 @@ def test_every_forwarded_key_is_a_real_run_task_param(self):
         )
         assert set(seen).issubset(accepted)
 
+    def test_max_turns_rejects_surprising_inputs(self):
+        # N4: int() accepts a bool (int(True)==1) and truncates a float
+        # (int(3.9)==3). The orchestrator always emits a real int, but a corrupt
+        # / hand-edited payload must not silently become a bogus turn count —
+        # drop with a breadcrumb and let run_task's default apply.
+        assert "max_turns" not in _capture({"repo_url": "r", "max_turns": True})
+        assert "max_turns" not in _capture({"repo_url": "r", "max_turns": 3.9})
+        # Valid inputs still pass: a real int and an int-valued string / float.
+        assert _capture({"repo_url": "r", "max_turns": 50})["max_turns"] == 50
+        assert _capture({"repo_url": "r", "max_turns": "50"})["max_turns"] == 50
+        assert _capture({"repo_url": "r", "max_turns": 50.0})["max_turns"] == 50
+
     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

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

@@ -56,11 +56,14 @@ export interface EcsAgentClusterProps {
   /**
    * Artifacts bucket for repo-bound artifact workflows (#299 coding/decompose-v1
    * emits its plan JSON here via ``deliver_artifact``). The AgentCore runtime
-   * gets ``ARTIFACTS_BUCKET_NAME`` in its env; the ECS task needs the SAME env +
-   * read/write grant or an artifact workflow fails at delivery with
+   * gets ``ARTIFACTS_BUCKET_NAME`` in its env; the ECS task needs the SAME env
+   * (but NO bucket grant) or an artifact workflow fails at delivery with
    * "ARTIFACTS_BUCKET_NAME is not configured" (live-caught: a :decompose on an
-   * ecs-configured repo). Read/WRITE because the container DELIVERS the artifact
-   * (unlike the read-only payload bucket).
+   * ecs-configured repo). The delivery WRITE goes through the assumed per-task
+   * SessionRole (scoped to ``artifacts/${aws:PrincipalTag/task_id}/*``), so the
+   * task role gets only the env var — parity with the AgentCore runtime role,
+   * which likewise has no direct artifacts grant (see the grant block below for
+   * the rationale).
    *
    * NOTE: this wires only ``ARTIFACTS_BUCKET_NAME`` (artifact delivery). It does
    * NOT set ``TRACE_ARTIFACTS_BUCKET_NAME`` (telemetry.py reads that for the

cdk/src/stacks/agent.ts

@@ -622,8 +622,10 @@ export class AgentStack extends Stack {
         // #502: read-only grant so the container can fetch its payload from S3.
         payloadBucket: ecsPayloadBucket!.bucket,
         // #299 ECS-parity: the same bucket the runtime uses for ARTIFACTS_BUCKET_NAME —
-        // coding/decompose-v1 delivers its plan artifact here (read+write grant in
-        // the construct). Without this, an ecs-repo :decompose fails at delivery.
+        // coding/decompose-v1 delivers its plan artifact here. Wires the
+        // ARTIFACTS_BUCKET_NAME env only; delivery writes go through the per-task
+        // SessionRole (no direct task-role grant — see construct). Without the
+        // env, an ecs-repo :decompose fails at delivery.
         artifactsBucket: traceArtifactsBucket.bucket,
         // Per-session IAM scoping (#209): the ECS task role assumes the same
         // SessionRole as the AgentCore runtime for tenant-data access. The