Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates BYOC CD execution from ECS tasks to AWS CodeBuild: replaces ECS drivers, task IDs and templates with CodeBuild equivalents, adds CodeBuild run/status/tail/upload/CFN template generation, removes ECS implementations and tests, and updates related CLI interfaces and protobuf to carry a driver identifier. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CFN as CloudFormation
participant CodeBuild
participant CW as CloudWatch
participant S3
User->>CLI: cd setup / deploy
CLI->>CFN: SetUp(ctx, force) -> CreateTemplate(stack)
CFN->>S3: create bucket (optional)
CFN->>CW: create log group
CFN->>CodeBuild: create CodeBuild project
CFN-->>CLI: return ProjectName + outputs
User->>CLI: run CD command
CLI->>CodeBuild: Run(image, env, buildspec)
CodeBuild-->>CLI: BuildID
User->>CLI: poll status
CLI->>CodeBuild: GetBuildStatus(BuildID)
CodeBuild-->>CLI: status (InProgress/Succeeded/Failed)
User->>CLI: tail logs
CLI->>CW: TailBuildID(BuildID)
CW-->>CLI: stream LogEvents
User->>CLI: stop build
CLI->>CodeBuild: Stop(BuildID)
CodeBuild-->>CLI: stopped / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/pkg/clouds/aws/codebuild/cfn/setup_test.go (1)
45-58: This integration test never exercises a successful build.It only proves that
StartBuildreturned an ID and then immediately stops the build. Broken runtime behavior—like an invalid buildspec or wrong working directory inRun—would still pass here. Please wait for one happy-path build to finish successfully before callingStop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup_test.go` around lines 45 - 58, The test currently calls aws.Run and immediately aws.Stop without waiting for the build to complete; update the test around the taskid returned by aws.Run to poll the build status (using the project’s existing describe/status helper such as aws.DescribeBuild or aws.WaitBuild if available) until it reaches a terminal state, assert the terminal state is SUCCESS (fail the test on FAILURE/FAULT/TIMED_OUT/CANCELLED), and only call aws.Stop if the build is still running; use the same taskid variable and the test subtest "Stop" to ensure the happy-path build finishes before stopping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/aws/codebuild/cfn/template.go`:
- Around line 247-249: The role currently attaches the broad
"AdministratorAccess" in ManagedPolicyArns; replace this with a least-privilege
policy tailored to the CD flow: remove
"arn:aws:iam::aws:policy/AdministratorAccess" and instead attach only the
AWS-managed policies and/or a custom inline policy granting the specific S3
(GetObject/PutObject/ListBucket), ECR (GetAuthorizationToken, BatchGetImage,
InitiateLayerUpload, UploadLayerPart, CompleteLayerUpload), CloudWatch Logs
(CreateLogGroup, CreateLogStream, PutLogEvents), CloudFormation
(CreateStack/UpdateStack/DescribeStacks/DescribeChangeSet/DeleteStack if used)
and STS (AssumeRole) actions your build requires; update the role definition
that contains ManagedPolicyArns in template.go to reference those policies (or
an iam:Policy resource linked to the Role) and document which actions were
included. Ensure no use of AdministratorAccess remains.
- Line 272: The template sets ServiceRole using
cloudformation.Ref(_codeBuildServiceRole) which yields the role name but
AWS::CodeBuild::Project.ServiceRole requires the role ARN; change the value to
use the role's ARN via a GetAtt (e.g.,
cloudformation.GetAtt(_codeBuildServiceRole, "Arn") or the library equivalent)
so ServiceRole receives the IAM role ARN instead of the name.
In `@src/pkg/clouds/aws/codebuild/run.go`:
- Around line 26-30: The code collapses argv by using strings.Join(cmd, " ") and
hard-codes cd /app into buildspec; update the Run implementation to shell-quote
each element of cmd (e.g., use strconv.Quote or a shell-escaping helper on each
item) and join those quoted args to preserve argument boundaries, and stop
hard-coding /app: use the explicit workingDir parameter or field (validate it)
and build the buildspec to ensure the directory exists (e.g., mkdir -p
"<workingDir>" && cd "<workingDir>" && <quoted command>) before running the
command; change the BuildspecOverride construction to use the quoted command and
the validated workingDir variable instead of "/app".
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Around line 17-47: The function calling BatchGetBuilds must handle the FAULT
terminal status and surface invalid build IDs: when output.Builds is empty check
output.BuildsNotFound and return a terminal BuildFailure (e.g., Reason "invalid
build ID") instead of continuing to poll, and add a case for
cbtypes.StatusTypeFault in the switch on build.BuildStatus that returns true and
BuildFailure{Reason: "build fault"}; update the switch handling around
build.BuildStatus and the nil/empty response logic so invalid IDs and FAULT are
treated as terminal failures.
---
Nitpick comments:
In `@src/pkg/clouds/aws/codebuild/cfn/setup_test.go`:
- Around line 45-58: The test currently calls aws.Run and immediately aws.Stop
without waiting for the build to complete; update the test around the taskid
returned by aws.Run to poll the build status (using the project’s existing
describe/status helper such as aws.DescribeBuild or aws.WaitBuild if available)
until it reaches a terminal state, assert the terminal state is SUCCESS (fail
the test on FAILURE/FAULT/TIMED_OUT/CANCELLED), and only call aws.Stop if the
build is still running; use the same taskid variable and the test subtest "Stop"
to ensure the happy-path build finishes before stopping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0352564-b181-4ae8-868b-7223bc2f9301
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (63)
Makefilepkgs/defang/cli.nixsrc/cmd/crun/main.gosrc/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_integration_test.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/cd.gosrc/pkg/cli/client/byoc/aws/validation_test.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/waitForCdTaskExit_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidc.gosrc/pkg/clouds/aws/codebuild/cfn/oidc_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidcprovider.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/setup_test.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/cfn/waiter.gosrc/pkg/clouds/aws/codebuild/common.gosrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/clouds/aws/codebuild/tail.gosrc/pkg/clouds/aws/codebuild/upload.gosrc/pkg/clouds/aws/ecs/cfn/outputs.gosrc/pkg/clouds/aws/ecs/cfn/template.gosrc/pkg/clouds/aws/ecs/common.gosrc/pkg/clouds/aws/ecs/common_test.gosrc/pkg/clouds/aws/ecs/fargate.gosrc/pkg/clouds/aws/ecs/fargate_test.gosrc/pkg/clouds/aws/ecs/info.gosrc/pkg/clouds/aws/ecs/run.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/status_test.gosrc/pkg/clouds/aws/ecs/stop.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/ecs/tail_test.gosrc/pkg/clouds/driver.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/crun/common.gosrc/pkg/crun/common_test.gosrc/pkg/crun/destroy.gosrc/pkg/crun/docker/common.gosrc/pkg/crun/docker/info.gosrc/pkg/crun/docker/run.gosrc/pkg/crun/docker/run_test.gosrc/pkg/crun/docker/setup.gosrc/pkg/crun/docker/stop.gosrc/pkg/crun/docker/tail.gosrc/pkg/crun/docker/teardown.gosrc/pkg/crun/factory.gosrc/pkg/crun/info.gosrc/pkg/crun/local/local.gosrc/pkg/crun/local/local_test.gosrc/pkg/crun/logs.gosrc/pkg/crun/run.gosrc/pkg/crun/stop.go
💤 Files with no reviewable changes (36)
- src/pkg/crun/info.go
- src/pkg/clouds/aws/codebuild/cfn/params.go
- src/pkg/crun/destroy.go
- src/pkg/clouds/gcp/cloudrun.go
- src/pkg/cli/client/byoc/aws/cd.go
- src/pkg/crun/docker/teardown.go
- src/pkg/clouds/aws/ecs/cfn/outputs.go
- src/pkg/crun/common.go
- src/cmd/crun/main.go
- src/pkg/crun/docker/run_test.go
- src/pkg/clouds/aws/ecs/stop.go
- src/pkg/crun/docker/info.go
- src/pkg/clouds/aws/ecs/tail.go
- src/pkg/crun/factory.go
- src/pkg/clouds/aws/ecs/common_test.go
- src/pkg/clouds/aws/ecs/status.go
- src/pkg/clouds/aws/ecs/common.go
- src/pkg/clouds/aws/ecs/run.go
- src/pkg/crun/logs.go
- src/pkg/clouds/aws/ecs/status_test.go
- src/pkg/clouds/aws/ecs/fargate_test.go
- src/pkg/crun/common_test.go
- src/pkg/clouds/aws/ecs/fargate.go
- src/pkg/clouds/aws/ecs/tail_test.go
- src/pkg/crun/stop.go
- src/pkg/crun/docker/stop.go
- src/pkg/crun/docker/tail.go
- src/pkg/crun/local/local.go
- src/pkg/clouds/driver.go
- src/pkg/clouds/aws/ecs/info.go
- src/pkg/crun/local/local_test.go
- src/pkg/clouds/aws/ecs/cfn/template.go
- src/pkg/crun/docker/run.go
- src/pkg/crun/docker/setup.go
- src/pkg/crun/run.go
- src/pkg/crun/docker/common.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Around line 168-179: In GetDeploymentStatus, map AWS errors to the friendly
annotated form: when b.driver.LoadConfig(ctx) returns an error, wrap and return
it using AnnotateAwsError before returning; likewise, after calling
awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId), if err is non-nil and does
not match awscodebuild.BuildFailure, wrap that err with AnnotateAwsError and
return it; preserve the existing handling that converts BuildFailure into
client.ErrDeploymentFailed and keep returning done on success.
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 212-227: The fillWithOutputs method on AwsCfn leaves previous
values for conditional outputs (e.g., CIRoleARN) when the stack no longer
exports them; modify fillWithOutputs to reset/clear the target fields (at least
AwsCfn.LogGroupARN, AwsCfn.BucketName, AwsCfn.CIRoleARN, AwsCfn.ProjectName) to
empty strings before iterating dso.Stacks[0].Outputs, then populate them from
outputs as currently implemented so a reused AwsCfn cannot retain stale
conditional values.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 62-63: The template currently double-prefixes OidcProviderIssuer:
stop adding a hardcoded "https://" and use the OidcProviderIssuer parameter
value consistently as the full URL; update the template generator code that
builds the OIDC provider (where it currently prepends "https://") to use the raw
OidcProviderIssuer parameter instead, and ensure the same exact parameter value
is used when constructing IAM condition keys (the `${issuer}:aud` /
`${issuer}:sub` substitutions) so they match the provider host; after making
those changes re-generate the golden template.yaml so the provider ARN and
condition keys no longer contain duplicated "https://".
- Around line 128-150: The Bucket resource in the CloudFormation template is
missing PublicAccessBlockConfiguration; update the template generator to add
PublicAccessBlockConfiguration to the Bucket (resource named "Bucket") with the
four flags BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and
RestrictPublicBuckets all set true, then re-generate the golden file so the
testdata/template.yaml includes this PublicAccessBlockConfiguration for the
AWS::S3::Bucket.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3628f256-18e1-4ee3-bbc1-14f48f79c85f
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
src/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/common.go
💤 Files with no reviewable changes (1)
- src/pkg/clouds/aws/codebuild/cfn/params.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/clouds/aws/codebuild/cfn/template.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/pkg/clouds/aws/codebuild/status.go (1)
17-25:⚠️ Potential issue | 🔴 CriticalHandle invalid build IDs and
FAULTas terminal states.Line 23 currently treats
len(output.Builds)==0as “still running,” and the switch omitsStatusTypeFault. Both can cause indefinite polling for terminal failures.Suggested fix
output, err := client.BatchGetBuilds(ctx, &cb.BatchGetBuildsInput{ Ids: []string{*buildID}, }) if err != nil { return false, err } if len(output.Builds) == 0 { - return false, nil // build doesn't exist yet + if len(output.BuildsNotFound) > 0 { + return true, BuildFailure{Reason: "invalid build ID"} + } + return false, nil // not visible yet } @@ case cbtypes.StatusTypeTimedOut: return true, BuildFailure{Reason: "build timed out"} + case cbtypes.StatusTypeFault: + return true, BuildFailure{Reason: "build fault"} default: return false, nil // still running (IN_PROGRESS) }In AWS CodeBuild BatchGetBuilds, when is an ID returned in buildsNotFound, and is buildStatus=FAULT terminal?Also applies to: 28-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/status.go` around lines 17 - 25, The current BatchGetBuilds handling treats missing builds (len(output.Builds) == 0) as non-terminal and also omits FAULT from terminal statuses; update the client.BatchGetBuilds handling to check output.BuildsNotFound (or when len(output.Builds) == 0) and treat that case as a terminal error (return terminal=true and an appropriate error), and add cb.StatusTypeFault to the switch of terminal statuses so FAULT returns terminal=true; locate the logic around client.BatchGetBuilds, output.Builds / output.BuildsNotFound and the switch on build.Status and apply these changes.src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml (2)
128-150:⚠️ Potential issue | 🟠 MajorS3 bucket is still missing explicit public-access blocking.
Please add
PublicAccessBlockConfigurationwith all four controls enabled onBucket.🔒 Suggested template update
Bucket: DeletionPolicy: Fn::If: - RetainS3Bucket - RetainExceptOnCreate - Delete Properties: + PublicAccessBlockConfiguration: + BlockPublicAcls: true + BlockPublicPolicy: true + IgnorePublicAcls: true + RestrictPublicBuckets: true Tags: - Key: defang:CreatedBy Value: defang🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 128 - 150, The S3 Bucket resource named "Bucket" is missing an explicit PublicAccessBlockConfiguration; under the Bucket resource's Properties (resource symbol: Bucket) add the PublicAccessBlockConfiguration property and set its four controls—BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and RestrictPublicBuckets—to true to ensure public access is blocked by default.
62-63:⚠️ Potential issue | 🟠 MajorOIDC issuer handling is still inconsistent and can break role assumption.
The parameter is labeled as a URL, but the template both prefixes
https://and reuses the same value for IAM condition keys. This can produce invalid provider URLs or mismatched${issuer}:aud/${issuer}:subkeys depending on input format.Also applies to: 108-110, 167-170, 363-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml` around lines 62 - 63, The OidcProviderIssuer parameter is inconsistently treated as both a full URL and a raw issuer string (used with a prefixed "https://" and directly in IAM condition keys like `${OidcProviderIssuer}:aud`), which can produce invalid provider ARNs or condition keys; update the CloudFormation template to normalize the value once and reuse that normalized form: define a single sanitized logical value (e.g., OidcProviderIssuerNormalized) by stripping any leading "https://" if present or by requiring callers to supply the issuer without scheme, then construct the provider URL as "https://{OidcProviderIssuerNormalized}" wherever the full URL is required and use `{OidcProviderIssuerNormalized}` for IAM condition keys (`:aud`, `:sub`) so the same canonical issuer is used consistently across OidcProviderIssuer usages.src/pkg/clouds/aws/codebuild/cfn/template.go (2)
241-241:⚠️ Potential issue | 🔴 CriticalUse the IAM role ARN for
Project.ServiceRole(notRef).On Line 241,
cloudformation.Ref(_codeBuildServiceRole)resolves to role name, whileAWS::CodeBuild::Project.ServiceRoleexpects an ARN.💡 Proposed fix
- ServiceRole: cloudformation.Ref(_codeBuildServiceRole), + ServiceRole: cloudformation.GetAtt(_codeBuildServiceRole, "Arn"),For AWS CloudFormation, what does Ref return for AWS::IAM::Role, and what value type is required by AWS::CodeBuild::Project ServiceRole?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/template.go` at line 241, The ServiceRole for the CodeBuild Project is using cloudformation.Ref(_codeBuildServiceRole) which yields the IAM role name, but AWS::CodeBuild::Project.ServiceRole requires the role's ARN; update the template to pass the role ARN by replacing the Ref with cloudformation.GetAtt(_codeBuildServiceRole, "Arn") (or equivalent GetAtt call) where Project.ServiceRole is set so the property receives the IAM Role ARN instead of the name.
167-169:⚠️ Potential issue | 🟠 MajorBoth runtime roles are still overly broad (
PowerUserAccess).This keeps a large account-level blast radius. Please replace with scoped policies for the exact CD/CI actions required.
Also applies to: 295-297
src/pkg/clouds/aws/codebuild/cfn/setup.go (1)
212-227:⚠️ Potential issue | 🟡 MinorClear cached outputs before rehydrating from stack outputs.
Without reset, conditional outputs can leave stale values in memory (notably
CIRoleARN) after stack updates.🧹 Proposed fix
func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error { if len(dso.Stacks) != 1 { return fmt.Errorf("expected 1 CloudFormation stack, got %d", len(dso.Stacks)) } + a.LogGroupARN = "" + a.BucketName = "" + a.CIRoleARN = "" + a.ProjectName = "" for _, output := range dso.Stacks[0].Outputs { switch *output.OutputKey {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup.go` around lines 212 - 227, The fillWithOutputs method can leave stale values when some outputs are absent; before iterating stack outputs in AwsCfn.fillWithOutputs, reset the target fields (e.g. a.LogGroupARN, a.BucketName, a.CIRoleARN, a.ProjectName) to their zero values (empty strings) so conditional/missing outputs don't preserve previous values, then populate them from dso.Stacks[0].Outputs as currently implemented.src/pkg/cli/client/byoc/aws/byoc.go (1)
169-172:⚠️ Potential issue | 🟡 MinorReapply AWS error annotation in deployment-status path.
Line 171 and Line 178 currently return raw errors, which regresses the friendly AWS diagnostics used elsewhere.
🛠️ Proposed fix
cfg, err := b.driver.LoadConfig(ctx) if err != nil { - return false, err + return false, AnnotateAwsError(err) } done, err := awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId) if err != nil { if buildErr := new(awscodebuild.BuildFailure); errors.As(err, buildErr) { return done, client.ErrDeploymentFailed{Message: buildErr.Error()} } - return done, err + return done, AnnotateAwsError(err) }Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/byoc.go` around lines 169 - 172, The raw errors returned after calling b.driver.LoadConfig(ctx) (and the similar return at the deployment-status path) need to be wrapped with the project’s AWS diagnostics helper instead of returning err directly; replace the plain "return false, err" occurrences (the one after b.driver.LoadConfig(ctx) and the other in the deployment-status path) with a wrapped/annotated error using the same helper used elsewhere (e.g., call the existing annotateAWSError/WrapAWSError function or the b.annotateAWSError(err) helper) so the returned error includes the friendly AWS diagnostic info.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/client/byoc/do/byoc.go`:
- Around line 674-677: The SetUpCD method currently returns early whenever
b.SetupDone is true, making the force parameter a no-op; change the early-return
check in SetUpCD to only return when setup is done and force is false (e.g., if
b.SetupDone && !force { return nil }) so that calling SetUpCD(ctx, true) will
bypass the guard and re-run the setup; ensure any stateful steps inside SetUpCD
(references to b.SetupDone and subsequent setup logic) are safe to re-execute or
reset b.SetupDone appropriately before/after the forced run.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 216-252: The IAM policy CodeBuildIAMPolicy and the
CodeBuildServiceRole grant overly broad privileges (Resource: '*' and
PowerUserAccess); narrow them by removing unneeded iam mutation actions from the
PolicyDocument (only keep actions CodeBuild actually needs), replace Resource:
'*' with specific ARNs (e.g., the exact role, instance profile, S3 buckets, ECR
repos, CloudWatch log groups) and scope iam:PassRole to the specific execution
role ARN, and remove PowerUserAccess from CodeBuildServiceRole in favor of
least-privilege managed policies (e.g., AWSCodeBuildServiceRole or custom
policies limited to S3, ECR, CloudWatch and the specific IAM role). Ensure
CodeBuildIAMPolicy only contains the precise actions referenced by the build
project (attach/detach only if used) and reference the exact resource ARNs for
those actions.
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Line 18: Guard against nil dereferences for buildID and phase context
messages: before using *buildID when constructing the Ids slice, check buildID
!= nil and return or handle the error if nil; likewise when accessing
phase.Contexts[0].Message ensure phase.Contexts is non-empty and
phase.Contexts[0].Message != nil (or use a safe accessor) before dereferencing,
and provide a sensible fallback or error path. Update the code paths around the
Ids: []string{*buildID} usage and the logic that reads phase.Contexts[0].Message
so both validate pointers/lengths and handle failure cases without panicking.
---
Duplicate comments:
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Around line 169-172: The raw errors returned after calling
b.driver.LoadConfig(ctx) (and the similar return at the deployment-status path)
need to be wrapped with the project’s AWS diagnostics helper instead of
returning err directly; replace the plain "return false, err" occurrences (the
one after b.driver.LoadConfig(ctx) and the other in the deployment-status path)
with a wrapped/annotated error using the same helper used elsewhere (e.g., call
the existing annotateAWSError/WrapAWSError function or the
b.annotateAWSError(err) helper) so the returned error includes the friendly AWS
diagnostic info.
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 212-227: The fillWithOutputs method can leave stale values when
some outputs are absent; before iterating stack outputs in
AwsCfn.fillWithOutputs, reset the target fields (e.g. a.LogGroupARN,
a.BucketName, a.CIRoleARN, a.ProjectName) to their zero values (empty strings)
so conditional/missing outputs don't preserve previous values, then populate
them from dso.Stacks[0].Outputs as currently implemented.
In `@src/pkg/clouds/aws/codebuild/cfn/template.go`:
- Line 241: The ServiceRole for the CodeBuild Project is using
cloudformation.Ref(_codeBuildServiceRole) which yields the IAM role name, but
AWS::CodeBuild::Project.ServiceRole requires the role's ARN; update the template
to pass the role ARN by replacing the Ref with
cloudformation.GetAtt(_codeBuildServiceRole, "Arn") (or equivalent GetAtt call)
where Project.ServiceRole is set so the property receives the IAM Role ARN
instead of the name.
In `@src/pkg/clouds/aws/codebuild/cfn/testdata/template.yaml`:
- Around line 128-150: The S3 Bucket resource named "Bucket" is missing an
explicit PublicAccessBlockConfiguration; under the Bucket resource's Properties
(resource symbol: Bucket) add the PublicAccessBlockConfiguration property and
set its four controls—BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, and
RestrictPublicBuckets—to true to ensure public access is blocked by default.
- Around line 62-63: The OidcProviderIssuer parameter is inconsistently treated
as both a full URL and a raw issuer string (used with a prefixed "https://" and
directly in IAM condition keys like `${OidcProviderIssuer}:aud`), which can
produce invalid provider ARNs or condition keys; update the CloudFormation
template to normalize the value once and reuse that normalized form: define a
single sanitized logical value (e.g., OidcProviderIssuerNormalized) by stripping
any leading "https://" if present or by requiring callers to supply the issuer
without scheme, then construct the provider URL as
"https://{OidcProviderIssuerNormalized}" wherever the full URL is required and
use `{OidcProviderIssuerNormalized}` for IAM condition keys (`:aud`, `:sub`) so
the same canonical issuer is used consistently across OidcProviderIssuer usages.
In `@src/pkg/clouds/aws/codebuild/status.go`:
- Around line 17-25: The current BatchGetBuilds handling treats missing builds
(len(output.Builds) == 0) as non-terminal and also omits FAULT from terminal
statuses; update the client.BatchGetBuilds handling to check
output.BuildsNotFound (or when len(output.Builds) == 0) and treat that case as a
terminal error (return terminal=true and an appropriate error), and add
cb.StatusTypeFault to the switch of terminal statuses so FAULT returns
terminal=true; locate the logic around client.BatchGetBuilds, output.Builds /
output.BuildsNotFound and the switch on build.Status and apply these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34ae1620-1176-47b9-8ea0-36d061137d30
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
Makefilepkgs/defang/cli.nixsrc/cmd/cli/command/cd.gosrc/cmd/cli/command/commands.gosrc/cmd/crun/main.gosrc/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_integration_test.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/cd.gosrc/pkg/cli/client/byoc/aws/validation_test.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/install_cd.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/waitForCdTaskExit_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidc.gosrc/pkg/clouds/aws/codebuild/cfn/oidc_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidcprovider.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/setup_test.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/cfn/waiter.gosrc/pkg/clouds/aws/codebuild/common.gosrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/clouds/aws/codebuild/tail.gosrc/pkg/clouds/aws/codebuild/upload.gosrc/pkg/clouds/aws/ecs/cfn/outputs.gosrc/pkg/clouds/aws/ecs/cfn/template.gosrc/pkg/clouds/aws/ecs/cfn/template_test.gosrc/pkg/clouds/aws/ecs/cfn/testdata/template.yamlsrc/pkg/clouds/aws/ecs/common.gosrc/pkg/clouds/aws/ecs/common_test.gosrc/pkg/clouds/aws/ecs/fargate.gosrc/pkg/clouds/aws/ecs/fargate_test.gosrc/pkg/clouds/aws/ecs/info.gosrc/pkg/clouds/aws/ecs/run.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/status_test.gosrc/pkg/clouds/aws/ecs/stop.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/ecs/tail_test.gosrc/pkg/clouds/driver.gosrc/pkg/clouds/gcp/cloudrun.gosrc/pkg/crun/common.gosrc/pkg/crun/common_test.gosrc/pkg/crun/destroy.gosrc/pkg/crun/docker/common.gosrc/pkg/crun/docker/info.gosrc/pkg/crun/docker/run.gosrc/pkg/crun/docker/run_test.gosrc/pkg/crun/docker/setup.gosrc/pkg/crun/docker/stop.gosrc/pkg/crun/docker/tail.gosrc/pkg/crun/docker/teardown.gosrc/pkg/crun/factory.gosrc/pkg/crun/info.gosrc/pkg/crun/local/local.gosrc/pkg/crun/local/local_test.gosrc/pkg/crun/logs.gosrc/pkg/crun/run.gosrc/pkg/crun/stop.go
💤 Files with no reviewable changes (38)
- src/pkg/clouds/aws/ecs/cfn/template_test.go
- src/pkg/crun/docker/run_test.go
- src/pkg/clouds/aws/ecs/fargate_test.go
- src/pkg/crun/docker/tail.go
- src/pkg/crun/local/local_test.go
- src/pkg/clouds/aws/ecs/cfn/template.go
- src/pkg/crun/run.go
- src/pkg/clouds/aws/ecs/common_test.go
- src/pkg/clouds/aws/ecs/fargate.go
- src/pkg/clouds/aws/ecs/run.go
- src/pkg/clouds/gcp/cloudrun.go
- src/pkg/crun/common_test.go
- src/cmd/crun/main.go
- src/pkg/crun/docker/common.go
- src/pkg/clouds/aws/codebuild/cfn/params.go
- src/pkg/crun/docker/stop.go
- src/pkg/crun/common.go
- src/pkg/crun/destroy.go
- src/pkg/crun/docker/teardown.go
- src/pkg/crun/local/local.go
- src/pkg/clouds/aws/ecs/tail_test.go
- src/pkg/clouds/aws/ecs/cfn/testdata/template.yaml
- src/pkg/cli/client/byoc/aws/cd.go
- src/pkg/crun/docker/run.go
- src/pkg/crun/docker/info.go
- src/pkg/crun/docker/setup.go
- src/pkg/clouds/aws/ecs/info.go
- src/pkg/clouds/aws/ecs/stop.go
- src/pkg/crun/factory.go
- src/pkg/crun/logs.go
- src/pkg/clouds/aws/ecs/common.go
- src/pkg/crun/info.go
- src/pkg/clouds/aws/ecs/tail.go
- src/pkg/clouds/aws/ecs/cfn/outputs.go
- src/pkg/clouds/aws/ecs/status_test.go
- src/pkg/clouds/driver.go
- src/pkg/crun/stop.go
- src/pkg/clouds/aws/ecs/status.go
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pkg/cli/client/byoc/aws/validation_test.go
- Makefile
- src/pkg/clouds/aws/codebuild/run.go
- src/pkg/clouds/aws/codebuild/cfn/outputs.go
- src/pkg/clouds/aws/codebuild/upload.go
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/pkg/clouds/aws/codebuild/cfn/setup.go (1)
201-216:⚠️ Potential issue | 🟡 MinorClear output-backed fields before rehydrating stack outputs.
At Line 205 onward, fields are only overwritten when output keys exist. If a conditional output disappears, stale values remain in memory and can affect later calls.
🧹 Proposed fix
func (a *AwsCfn) fillWithOutputs(dso *cloudformation.DescribeStacksOutput) error { if len(dso.Stacks) != 1 { return fmt.Errorf("expected 1 CloudFormation stack, got %d", len(dso.Stacks)) } + a.LogGroupARN = "" + a.BucketName = "" + a.CIRoleARN = "" + a.ProjectName = "" for _, output := range dso.Stacks[0].Outputs { switch *output.OutputKey {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup.go` around lines 201 - 216, In fillWithOutputs, clear the existing output-backed fields before iterating so stale values can't persist: at the start of AwsCfn.fillWithOutputs set a.LogGroupARN, a.BucketName, a.CIRoleARN, and a.ProjectName to empty strings, then iterate dso.Stacks[0].Outputs and assign as currently done (using the existing OutputKey switch on OutputsLogGroupARN, OutputsBucketName, OutputsCIRoleARN, OutputsCodeBuildProjectName) so missing outputs won't leave old data around.src/pkg/cli/client/byoc/aws/byoc.go (1)
169-179:⚠️ Potential issue | 🟡 MinorRe-apply AWS error annotation in deployment-status path.
At Line 171 and Line 178, raw errors are returned. This bypasses the friendly AWS-specific mapping used in the rest of this provider.
🛠 Proposed fix
cfg, err := b.driver.LoadConfig(ctx) if err != nil { - return false, err + return false, AnnotateAwsError(err) } done, err := awscodebuild.GetBuildStatus(ctx, cfg, b.cdBuildId) if err != nil { if buildErr := new(awscodebuild.BuildFailure); errors.As(err, buildErr) { return done, client.ErrDeploymentFailed{Message: buildErr.Error()} } - return done, err + return done, AnnotateAwsError(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/byoc.go` around lines 169 - 179, The code returns raw errors from b.driver.LoadConfig and the non-BuildFailure branch of awscodebuild.GetBuildStatus, bypassing the AWS-specific error mapping used elsewhere; change both returns to pass the error through the provider's AWS error-mapping helper (the same helper used elsewhere in this provider, e.g., awscodebuild.MapAWSError or the equivalent mapping function) before returning so callers get the friendly AWS-mapped errors (update the b.driver.LoadConfig error return and the "return done, err" after GetBuildStatus to return the mapped error instead, and keep the existing errors.As BuildFailure handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/clouds/aws/codebuild/tail.go`:
- Around line 52-55: The code path in the caller of GetBuildStatus (in tail.go)
can return (nil, nil) when done == true and err == nil, which leads to callers
receiving a nil iterator; change the logic in the function that calls
GetBuildStatus (the loop around GetBuildStatus in tail.go) so that when done is
true and err is nil you return an empty iterator (or an initialized zero-value
iterator/sequence) instead of nil, preserving the nil error; specifically update
the branch that checks GetBuildStatus(ctx, cfg, buildID) to return a non-nil,
empty iterator object (or empty slice/chan/wrapper used by your iterator API)
along with nil error so consumers can safely range over it.
---
Duplicate comments:
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Around line 169-179: The code returns raw errors from b.driver.LoadConfig and
the non-BuildFailure branch of awscodebuild.GetBuildStatus, bypassing the
AWS-specific error mapping used elsewhere; change both returns to pass the error
through the provider's AWS error-mapping helper (the same helper used elsewhere
in this provider, e.g., awscodebuild.MapAWSError or the equivalent mapping
function) before returning so callers get the friendly AWS-mapped errors (update
the b.driver.LoadConfig error return and the "return done, err" after
GetBuildStatus to return the mapped error instead, and keep the existing
errors.As BuildFailure handling).
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 201-216: In fillWithOutputs, clear the existing output-backed
fields before iterating so stale values can't persist: at the start of
AwsCfn.fillWithOutputs set a.LogGroupARN, a.BucketName, a.CIRoleARN, and
a.ProjectName to empty strings, then iterate dso.Stacks[0].Outputs and assign as
currently done (using the existing OutputKey switch on OutputsLogGroupARN,
OutputsBucketName, OutputsCIRoleARN, OutputsCodeBuildProjectName) so missing
outputs won't leave old data around.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a51a28d-819c-477d-b94b-a59bf06967ab
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (45)
pkgs/defang/cli.nixsrc/go.modsrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/byoc_integration_test.gosrc/pkg/cli/client/byoc/aws/byoc_test.gosrc/pkg/cli/client/byoc/aws/cd.gosrc/pkg/cli/client/byoc/aws/validation_test.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/tail_test.gosrc/pkg/cli/waitForCdTaskExit_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidc.gosrc/pkg/clouds/aws/codebuild/cfn/oidc_test.gosrc/pkg/clouds/aws/codebuild/cfn/oidcprovider.gosrc/pkg/clouds/aws/codebuild/cfn/outputs.gosrc/pkg/clouds/aws/codebuild/cfn/params.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create.gosrc/pkg/clouds/aws/codebuild/cfn/quick_create_test.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/setup_test.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/template_test.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/cfn/waiter.gosrc/pkg/clouds/aws/codebuild/common.gosrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/clouds/aws/codebuild/tail.gosrc/pkg/clouds/aws/codebuild/upload.gosrc/pkg/clouds/aws/ecs/cfn/outputs.gosrc/pkg/clouds/aws/ecs/cfn/template.gosrc/pkg/clouds/aws/ecs/cfn/template_test.gosrc/pkg/clouds/aws/ecs/cfn/testdata/template.yamlsrc/pkg/clouds/aws/ecs/common.gosrc/pkg/clouds/aws/ecs/common_test.gosrc/pkg/clouds/aws/ecs/fargate.gosrc/pkg/clouds/aws/ecs/fargate_test.gosrc/pkg/clouds/aws/ecs/info.gosrc/pkg/clouds/aws/ecs/run.gosrc/pkg/clouds/aws/ecs/status.gosrc/pkg/clouds/aws/ecs/status_test.gosrc/pkg/clouds/aws/ecs/stop.gosrc/pkg/clouds/aws/ecs/tail.gosrc/pkg/clouds/aws/ecs/tail_test.gosrc/pkg/clouds/driver.gosrc/pkg/clouds/gcp/cloudrun.go
💤 Files with no reviewable changes (19)
- src/pkg/clouds/aws/ecs/fargate_test.go
- src/pkg/clouds/aws/ecs/common_test.go
- src/pkg/clouds/gcp/cloudrun.go
- src/pkg/clouds/aws/ecs/tail.go
- src/pkg/clouds/aws/codebuild/cfn/params.go
- src/pkg/clouds/aws/ecs/cfn/template_test.go
- src/pkg/clouds/aws/ecs/cfn/template.go
- src/pkg/clouds/aws/ecs/stop.go
- src/pkg/clouds/aws/ecs/info.go
- src/pkg/clouds/aws/ecs/status_test.go
- src/pkg/clouds/aws/ecs/cfn/testdata/template.yaml
- src/pkg/clouds/driver.go
- src/pkg/clouds/aws/ecs/tail_test.go
- src/pkg/clouds/aws/ecs/cfn/outputs.go
- src/pkg/cli/client/byoc/aws/cd.go
- src/pkg/clouds/aws/ecs/common.go
- src/pkg/clouds/aws/ecs/run.go
- src/pkg/clouds/aws/ecs/fargate.go
- src/pkg/clouds/aws/ecs/status.go
🚧 Files skipped from review as they are similar to previous changes (8)
- src/pkg/clouds/aws/codebuild/cfn/outputs.go
- src/pkg/cli/client/byoc/aws/byoc_integration_test.go
- src/pkg/clouds/aws/codebuild/common.go
- src/go.mod
- src/pkg/clouds/aws/codebuild/run.go
- src/pkg/clouds/aws/codebuild/upload.go
- pkgs/defang/cli.nix
- src/pkg/cli/client/byoc/aws/byoc_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/clouds/aws/codebuild/cfn/setup.go (1)
104-117:⚠️ Potential issue | 🟠 MajorFilter out
UsePreviousValue=trueparameters beforeCreateStack.The
SetUpmethod builds parameters withUsePreviousValue=truefor updates (lines 147–158). When the stack doesn't exist,upsertStackAndWaitcallscreateStackAndWaitwith the same parameter slice (line 174). CloudFormation'sCreateStackAPI doesn't recognizeUsePreviousValuesince the stack is being created for the first time—this field applies only to updates. Parameters should either provide explicitParameterValueor be omitted to use template defaults. StripUsePreviousValue=trueentries here before passing toCreateStack, consistent with the filtering already done inupdateStackAndWait(lines 76–78).Proposed fix
func (a *AwsCfn) createStackAndWait(ctx context.Context, templateBody string, parameters []cfnTypes.Parameter) error { + parameters = slices.DeleteFunc(parameters, func(p cfnTypes.Parameter) bool { + return p.UsePreviousValue != nil && *p.UsePreviousValue + }) + cfn, err := a.newClient(ctx) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/cfn/setup.go` around lines 104 - 117, The createStackAndWait function is passing parameters that may contain UsePreviousValue=true (built in SetUp and reused in upsertStackAndWait), which CloudFormation's CreateStack does not accept; modify createStackAndWait to filter the incoming parameters slice and remove any cfnTypes.Parameter where UsePreviousValue != nil && *UsePreviousValue == true (produce a new filteredParams slice containing only parameters with an explicit ParameterValue or without UsePreviousValue) and pass that filteredParams to cfn.CreateStack; follow the same filtering approach used in updateStackAndWait to ensure template defaults are used or explicit values are supplied during initial stack creation.
♻️ Duplicate comments (1)
src/pkg/clouds/aws/codebuild/run.go (1)
26-30:⚠️ Potential issue | 🟠 MajorPreserve argv boundaries and avoid hard-coding
/appin the buildspec.Line 27 collapses arguments into one shell string, and Line 30 assumes
/appexists in every image. This can break commands and image portability.🔧 Proposed fix direction
- // Build the command to run; the buildspec executes it directly - command := strings.Join(cmd, " ") - - // CodeBuild overrides the image's WORKDIR; use `cd` to restore it before running the command - buildspec := "version: 0.2\nphases:\n build:\n commands:\n - cd /app && " + command + "\n" + // Preserve argv boundaries by shell-quoting each arg. + quoted := make([]string, len(cmd)) + for i, arg := range cmd { + quoted[i] = "'" + strings.ReplaceAll(arg, "'", `'"'"'`) + "'" + } + command := strings.Join(quoted, " ") + + // Avoid image-specific hardcoded workdir assumptions. + buildspec := "version: 0.2\nphases:\n build:\n commands:\n - " + command + "\n"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/clouds/aws/codebuild/run.go` around lines 26 - 30, The code currently collapses argv by building a single shell string from cmd and hard-codes /app; change buildspec generation so you do NOT join cmd into one shell string (do not use the current command variable) but instead emit the commands section as a YAML array where the first element is a directory restore using the CodeBuild-provided working-dir env (use $CODEBUILD_SRC_DIR or CODEBUILD_SRC_DIR) and subsequent elements are the cmd slice elements (properly escaped), preserving argument boundaries; update the buildspec construction where buildspec is set (reference variables cmd, command, buildspec) to produce a YAML array of commands rather than a single joined shell line and replace hard-coded "/app" with the CodeBuild env variable.
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/aws/byoc.go (1)
579-581: Centralize the CD startup command.
node lib/index.jsis now an AWS-only assumption about the CD image layout.src/pkg/cli/client/byoc/gcp/byoc.gostill handscmd.commandto Cloud Build and relies on the image entrypoint, so the next CD image packaging change can break only one provider. A shared helper/constant for the CD command would keep both runners aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/cli/client/byoc/aws/byoc.go` around lines 579 - 581, The startup command for the CD image ("node lib/index.js") is currently hardcoded in AWS runner (append in byoc.go before b.driver.Run) causing drift with GCP runner (which uses cmd.command and the image ENTRYPOINT); extract this into a shared constant or helper (e.g., CDStartupCommand or GetCDStartupArgs) in the package used by both AWS and GCP runners and replace the inline usage in src/pkg/cli/client/byoc/aws/byoc.go (the args := append([]string{"node", "lib/index.js"}, cmd.command...) call) and the corresponding logic in src/pkg/cli/client/byoc/gcp/byoc.go to both use that shared symbol so future image layout changes stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pkg/cli/client/grpc_logger.go`:
- Around line 36-41: In grpcLogger.logRequest, the code uses
header.Add("X-Request-Id", requestId) which appends values; change it to use
header.Set("X-Request-Id", requestId) so the X-Request-Id header is
single-valued; update the call in the logRequest function (where requestId is
generated via pkg.RandomID()) to call Header.Set instead of Header.Add and keep
the subsequent term.Debug(g.prefix, requestId, reqType, payload) unchanged.
In `@src/pkg/clouds/aws/codebuild/cfn/template.go`:
- Around line 253-262: The CodeBuild Environment block
(codebuild.Project_Environment) enables LOCAL_DOCKER_LAYER_CACHE but is missing
PrivilegedMode; update the Environment struct in template.go (the Environment
field next to Cache and ComputeType/Image entries) to include PrivilegedMode:
ptr.Bool(true) so Docker layer caching runs with privileged mode enabled; use
the existing ptr helper (ptr.Bool) to set the boolean.
---
Outside diff comments:
In `@src/pkg/clouds/aws/codebuild/cfn/setup.go`:
- Around line 104-117: The createStackAndWait function is passing parameters
that may contain UsePreviousValue=true (built in SetUp and reused in
upsertStackAndWait), which CloudFormation's CreateStack does not accept; modify
createStackAndWait to filter the incoming parameters slice and remove any
cfnTypes.Parameter where UsePreviousValue != nil && *UsePreviousValue == true
(produce a new filteredParams slice containing only parameters with an explicit
ParameterValue or without UsePreviousValue) and pass that filteredParams to
cfn.CreateStack; follow the same filtering approach used in updateStackAndWait
to ensure template defaults are used or explicit values are supplied during
initial stack creation.
---
Duplicate comments:
In `@src/pkg/clouds/aws/codebuild/run.go`:
- Around line 26-30: The code currently collapses argv by building a single
shell string from cmd and hard-codes /app; change buildspec generation so you do
NOT join cmd into one shell string (do not use the current command variable) but
instead emit the commands section as a YAML array where the first element is a
directory restore using the CodeBuild-provided working-dir env (use
$CODEBUILD_SRC_DIR or CODEBUILD_SRC_DIR) and subsequent elements are the cmd
slice elements (properly escaped), preserving argument boundaries; update the
buildspec construction where buildspec is set (reference variables cmd, command,
buildspec) to produce a YAML array of commands rather than a single joined shell
line and replace hard-coded "/app" with the CodeBuild env variable.
---
Nitpick comments:
In `@src/pkg/cli/client/byoc/aws/byoc.go`:
- Around line 579-581: The startup command for the CD image ("node
lib/index.js") is currently hardcoded in AWS runner (append in byoc.go before
b.driver.Run) causing drift with GCP runner (which uses cmd.command and the
image ENTRYPOINT); extract this into a shared constant or helper (e.g.,
CDStartupCommand or GetCDStartupArgs) in the package used by both AWS and GCP
runners and replace the inline usage in src/pkg/cli/client/byoc/aws/byoc.go (the
args := append([]string{"node", "lib/index.js"}, cmd.command...) call) and the
corresponding logic in src/pkg/cli/client/byoc/gcp/byoc.go to both use that
shared symbol so future image layout changes stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bfc8a28-2621-4452-915a-c6243d5faa50
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
src/pkg/auth/interceptor.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/caniuse.gosrc/pkg/cli/client/caniuse_test.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/client/grpc_logger.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/clouds/aws/codebuild/cfn/setup.gosrc/pkg/clouds/aws/codebuild/cfn/template.gosrc/pkg/clouds/aws/codebuild/cfn/testdata/template.yamlsrc/pkg/clouds/aws/codebuild/run.gosrc/pkg/clouds/aws/codebuild/status.gosrc/pkg/stacks/selector.gosrc/protos/io/defang/v1/fabric.proto
✅ Files skipped from review due to trivial changes (1)
- src/pkg/stacks/selector.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/clouds/aws/codebuild/status.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Apr 7, 2026
Merged
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
3 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.
Description
This results it much faster start times (~30s less!) and easier bootstrapping since no VPC and Cluster is needed.
Also removing the obsolete
cruncode.Timing benchmark https://gist.github.com/lionello/583c8bda0254230d6092b692fe5917c4:
Linked Issues
Fixes #1342
Checklist
Summary by CodeRabbit
New Features
Chores
Removed / Deprecated