fix: address Copilot PR review comments on PR #8

- Keep gitleaks/osv-scanner enabled in CI (only disable trivy/grype/semgrep)
- Type ComputeStrategy.type and SessionHandle.strategyType as ComputeType
- Trim/filter ECS_SUBNETS to handle whitespace and trailing commas
- Handle undefined exit code in ECS pollSession (container never started)
- Scope iam:PassRole to specific ECS task/execution role ARNs
- Validate all-or-nothing ECS props in TaskOrchestrator constructor
- Remove dead hasEcsBlueprint detection; document env-flag driven approach
- Add comment noting strategy_type as additive event field

Author: MichaelWalker-git

.github/workflows/build.yml

@@ -43,6 +43,8 @@ export class EcsAgentCluster extends Construct {
   public readonly taskDefinition: ecs.FargateTaskDefinition;
   public readonly securityGroup: ec2.SecurityGroup;
   public readonly containerName: string;
+  public readonly taskRoleArn: string;
+  public readonly executionRoleArn: string;
 
   constructor(scope: Construct, id: string, props: EcsAgentClusterProps) {
     super(scope, id);
@@ -129,6 +131,10 @@ export class EcsAgentCluster extends Construct {
     // CloudWatch Logs write
     logGroup.grantWrite(taskRole);
 
+    // Expose role ARNs for scoped iam:PassRole in the orchestrator
+    this.taskRoleArn = taskRole.roleArn;
+    this.executionRoleArn = this.taskDefinition.executionRole!.roleArn;
+
     NagSuppressions.addResourceSuppressions(this.taskDefinition, [
       {
         id: 'AwsSolutions-IAM5',

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

@@ -126,6 +126,13 @@ export interface TaskOrchestratorProps {
    * Container name in the ECS task definition.
    */
   readonly ecsContainerName?: string;
+
+  /**
+   * ARNs of the ECS task role and execution role for scoped iam:PassRole.
+   * Required when ecsClusterArn is provided.
+   */
+  readonly ecsTaskRoleArn?: string;
+  readonly ecsExecutionRoleArn?: string;
 }
 
 /**
@@ -154,6 +161,13 @@ export class TaskOrchestrator extends Construct {
     const handlersDir = path.join(__dirname, '..', 'handlers');
     const maxConcurrent = props.maxConcurrentTasksPerUser ?? 3;
 
+    // Validate ECS props are all-or-nothing
+    const ecsProps = [props.ecsClusterArn, props.ecsTaskDefinitionArn, props.ecsSubnets, props.ecsSecurityGroup, props.ecsContainerName];
+    const ecsPropsProvided = ecsProps.filter(p => p !== undefined);
+    if (ecsPropsProvided.length > 0 && ecsPropsProvided.length < ecsProps.length) {
+      throw new Error('ECS compute strategy requires all of: ecsClusterArn, ecsTaskDefinitionArn, ecsSubnets, ecsSecurityGroup, ecsContainerName');
+    }
+
     this.fn = new lambda.NodejsFunction(this, 'OrchestratorFn', {
       entry: path.join(handlersDir, 'orchestrate-task.ts'),
       handler: 'handler',
@@ -239,9 +253,10 @@ export class TaskOrchestrator extends Construct {
         },
       }));
 
+      const passRoleResources = [props.ecsTaskRoleArn, props.ecsExecutionRoleArn].filter(Boolean) as string[];
       this.fn.addToRolePolicy(new iam.PolicyStatement({
         actions: ['iam:PassRole'],
-        resources: ['*'],
+        resources: passRoleResources.length > 0 ? passRoleResources : ['*'],
         conditions: {
           StringEquals: {
             'iam:PassedToService': 'ecs-tasks.amazonaws.com',
@@ -287,7 +302,7 @@ export class TaskOrchestrator extends Construct {
       },
       {
         id: 'AwsSolutions-IAM5',
-        reason: 'DynamoDB index/* wildcards generated by CDK grantReadWriteData; AgentCore runtime/* required for sub-resource invocation; Secrets Manager wildcards generated by CDK grantRead; AgentCore Memory wildcards generated by CDK grantRead/grantWrite; ECS RunTask/DescribeTasks/StopTask conditioned on cluster ARN; iam:PassRole conditioned on ecs-tasks.amazonaws.com',
+        reason: 'DynamoDB index/* wildcards generated by CDK grantReadWriteData; AgentCore runtime/* required for sub-resource invocation; Secrets Manager wildcards generated by CDK grantRead; AgentCore Memory wildcards generated by CDK grantRead/grantWrite; ECS RunTask/DescribeTasks/StopTask conditioned on cluster ARN; iam:PassRole scoped to ECS task/execution roles and conditioned on ecs-tasks.amazonaws.com',
       },
     ], true);
   }

cdk/src/constructs/task-orchestrator.ts

@@ -129,6 +129,7 @@ const durableHandler: DurableExecutionHandler<OrchestrateTaskEvent, void> = asyn
         session_id: handle.sessionId,
         started_at: new Date().toISOString(),
       });
+      // Note: strategy_type is an additive field (not present in pre-strategy events)
       await emitTaskEvent(taskId, 'session_started', {
         session_id: handle.sessionId,
         strategy_type: handle.strategyType,

cdk/src/handlers/orchestrate-task.ts

@@ -23,7 +23,7 @@ import { EcsComputeStrategy } from './strategies/ecs-strategy';
 
 export interface SessionHandle {
   readonly sessionId: string;
-  readonly strategyType: string;
+  readonly strategyType: ComputeType;
   readonly metadata: Record<string, unknown>;
 }
 
@@ -33,7 +33,7 @@ export type SessionStatus =
   | { readonly status: 'failed'; readonly error: string };
 
 export interface ComputeStrategy {
-  readonly type: string;
+  readonly type: ComputeType;
   startSession(input: {
     taskId: string;
     payload: Record<string, unknown>;

cdk/src/handlers/shared/compute-strategy.ts

@@ -50,7 +50,7 @@ export class EcsComputeStrategy implements ComputeStrategy {
       );
     }
 
-    const subnets = ECS_SUBNETS.split(',');
+    const subnets = ECS_SUBNETS.split(',').map(s => s.trim()).filter(Boolean);
     const { taskId, payload, blueprintConfig } = input;
 
     const containerEnv = [
@@ -136,6 +136,9 @@ export class EcsComputeStrategy implements ComputeStrategy {
       if (exitCode === 0) {
         return { status: 'completed' };
       }
+      if (exitCode === undefined || exitCode === null) {
+        return { status: 'failed', error: `Task stopped: ${stoppedReason}` };
+      }
       return { status: 'failed', error: `Exit code ${exitCode}: ${stoppedReason}` };
     }
 

cdk/src/handlers/shared/strategies/ecs-strategy.ts

@@ -276,10 +276,9 @@ export class AgentStack extends Stack {
     inputGuardrail.createVersion('Initial version');
 
     // --- ECS Fargate compute backend (conditional) ---
-    // Wire ECS infrastructure when any blueprint uses compute.type: 'ecs'
-    const hasEcsBlueprint = blueprints.some(b => b.node.findAll()
-      .some(() => false)); // Currently no blueprints use ECS — infrastructure is opt-in
-    const needsEcs = hasEcsBlueprint || process.env.ABCA_ENABLE_ECS === 'true';
+    // ECS infrastructure is opt-in via the ABCA_ENABLE_ECS env flag.
+    // Repos can then use compute_type: 'ecs' in their blueprint config.
+    const needsEcs = process.env.ABCA_ENABLE_ECS === 'true';
 
     let ecsCluster: EcsAgentCluster | undefined;
     if (needsEcs) {
@@ -314,6 +313,8 @@ export class AgentStack extends Stack {
         ecsSubnets: agentVpc.vpc.selectSubnets({ subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }).subnetIds.join(','),
         ecsSecurityGroup: ecsCluster.securityGroup.securityGroupId,
         ecsContainerName: ecsCluster.containerName,
+        ecsTaskRoleArn: ecsCluster.taskRoleArn,
+        ecsExecutionRoleArn: ecsCluster.executionRoleArn,
       }),
     });
 

cdk/src/stacks/agent.ts

@@ -35,6 +35,8 @@ interface StackOverrides {
   ecsSubnets?: string;
   ecsSecurityGroup?: string;
   ecsContainerName?: string;
+  ecsTaskRoleArn?: string;
+  ecsExecutionRoleArn?: string;
 }
 
 function createStack(overrides?: StackOverrides): { stack: Stack; template: Template } {
@@ -70,6 +72,8 @@ function createStack(overrides?: StackOverrides): { stack: Stack; template: Temp
     ecsSubnets,
     ecsSecurityGroup,
     ecsContainerName,
+    ecsTaskRoleArn,
+    ecsExecutionRoleArn,
     ...rest
   } = overrides ?? {};
 
@@ -87,6 +91,8 @@ function createStack(overrides?: StackOverrides): { stack: Stack; template: Temp
     ...(ecsSubnets && { ecsSubnets }),
     ...(ecsSecurityGroup && { ecsSecurityGroup }),
     ...(ecsContainerName && { ecsContainerName }),
+    ...(ecsTaskRoleArn && { ecsTaskRoleArn }),
+    ...(ecsExecutionRoleArn && { ecsExecutionRoleArn }),
     ...rest,
   });
 
@@ -370,6 +376,8 @@ describe('TaskOrchestrator construct', () => {
       ecsSubnets: 'subnet-aaa,subnet-bbb',
       ecsSecurityGroup: 'sg-12345',
       ecsContainerName: 'AgentContainer',
+      ecsTaskRoleArn: 'arn:aws:iam::123456789012:role/TaskRole',
+      ecsExecutionRoleArn: 'arn:aws:iam::123456789012:role/ExecutionRole',
     };
 
     test('includes ECS env vars when ECS props are provided', () => {
@@ -421,15 +429,18 @@ describe('TaskOrchestrator construct', () => {
       });
     });
 
-    test('grants iam:PassRole conditioned on ecs-tasks.amazonaws.com', () => {
+    test('grants iam:PassRole scoped to task/execution role ARNs', () => {
       const { template } = createStack(ecsOverrides);
       template.hasResourceProperties('AWS::IAM::Policy', {
         PolicyDocument: {
           Statement: Match.arrayWith([
             Match.objectLike({
               Action: 'iam:PassRole',
               Effect: 'Allow',
-              Resource: '*',
+              Resource: Match.arrayWith([
+                'arn:aws:iam::123456789012:role/TaskRole',
+                'arn:aws:iam::123456789012:role/ExecutionRole',
+              ]),
               Condition: {
                 StringEquals: {
                   'iam:PassedToService': 'ecs-tasks.amazonaws.com',
@@ -441,6 +452,12 @@ describe('TaskOrchestrator construct', () => {
       });
     });
 
+    test('throws when only some ECS props are provided', () => {
+      expect(() => createStack({
+        ecsClusterArn: 'arn:aws:ecs:us-east-1:123456789012:cluster/agent-cluster',
+      })).toThrow('ECS compute strategy requires all of');
+    });
+
     test('does not grant ECS permissions when ECS props are omitted', () => {
       const { template } = createStack();
       const policies = template.findResources('AWS::IAM::Policy');

cdk/test/constructs/task-orchestrator.test.ts

@@ -168,6 +168,40 @@ describe('EcsComputeStrategy', () => {
       expect(result).toEqual({ status: 'completed' });
     });
 
+    test('returns failed for STOPPED with undefined exit code (container never started)', async () => {
+      mockSend.mockResolvedValueOnce({
+        tasks: [{
+          lastStatus: 'STOPPED',
+          stoppedReason: 'CannotPullContainerError',
+          containers: [{}],
+        }],
+      });
+
+      const strategy = new EcsComputeStrategy();
+      const result = await strategy.pollSession(makeHandle());
+      expect(result).toEqual({
+        status: 'failed',
+        error: 'Task stopped: CannotPullContainerError',
+      });
+    });
+
+    test('returns failed for STOPPED with no containers', async () => {
+      mockSend.mockResolvedValueOnce({
+        tasks: [{
+          lastStatus: 'STOPPED',
+          stoppedReason: 'EssentialContainerExited',
+          containers: [],
+        }],
+      });
+
+      const strategy = new EcsComputeStrategy();
+      const result = await strategy.pollSession(makeHandle());
+      expect(result).toEqual({
+        status: 'failed',
+        error: 'Task stopped: EssentialContainerExited',
+      });
+    });
+
     test('returns failed for STOPPED with non-zero exit code', async () => {
       mockSend.mockResolvedValueOnce({
         tasks: [{