refactor(build): lean awsEc2Mac compute provider - #363
Conversation
|
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
PR Summary by QodoRefactor AWS EC2 Mac compute provider into client seam + pure helpers
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const securityGroupId = yield* ensureSecurityGroup(ec2, client, vpcId); | ||
| let goldenAmiId: string | null | undefined = awsConfiguration.amiId; | ||
| if (goldenAmiId === undefined) { | ||
| goldenAmiId = yield* provideComputeServices(getAwsGoldenAmiId()); |
There was a problem hiding this comment.
Suggestion: Concurrent allocations can both read a null cached AMI, independently create and snapshot golden images, and then race when persisting the result. This wastes a second Dedicated Host and leaves the shared cache dependent on whichever write wins. Reserve or lock golden-image creation before provisioning, or recheck the cache atomically before snapshotting. [race condition]
Severity Level: Major ⚠️
- ⚠️ Parallel first builds duplicate Dedicated Host costs.
- ⚠️ Multiple golden AMI snapshots are created unnecessarily.
- ⚠️ Cache contents depend on write completion order.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/providers/compute/awsEc2Mac.ts
**Line:** 268:268
**Comment:**
*Race Condition: Concurrent allocations can both read a null cached AMI, independently create and snapshot golden images, and then race when persisting the result. This wastes a second Dedicated Host and leaves the shared cache dependent on whichever write wins. Reserve or lock golden-image creation before provisioning, or recheck the cache atomically before snapshotting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ): Effect.Effect<void, AwsComputeFailure> => | ||
| Effect.gen(function* () { | ||
| const deadline = Date.now() + 5 * 60 * 1000; | ||
| const deadline = Date.now() + TERMINATE_TIMEOUT_MS; |
There was a problem hiding this comment.
Suggestion: The five-minute deadline causes waitForTerminated to complete successfully even when the instance is still not terminated. Teardown then proceeds to ReleaseHosts, which AWS can reject while the instance is stopping, and the caller does not clear the live-host state because teardown failed. Timeout must be reported as a failure and should not be treated as confirmation that the host is safe to release. [logic error]
Severity Level: Major ⚠️
- ❌ Teardown may attempt release before instance termination.
- ⚠️ AWS release errors leave cloud state uncleared.
- ⚠️ Users must retry cleanup for slow terminations.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/providers/compute/awsEc2Mac.ts
**Line:** 755:755
**Comment:**
*Logic Error: The five-minute deadline causes `waitForTerminated` to complete successfully even when the instance is still not terminated. Teardown then proceeds to `ReleaseHosts`, which AWS can reject while the instance is stopping, and the caller does not clear the live-host state because teardown failed. Timeout must be reported as a failure and should not be treated as confirmation that the host is safe to release.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| try: () => import('@aws-sdk/client-ec2'), | ||
| catch: (cause) => awsFailure('load the EC2 SDK', cause), |
There was a problem hiding this comment.
Suggestion: The dynamic import failure is wrapped in a plain AwsComputeFailure object before reaching requireOptional. requireOptional only checks Error.message or String(cause) for module-resolution text, so the tagged object stringifies as an object and the missing-package case will not produce the documented INSTALL_HINT. Let the original import error reach requireOptional, or preserve its message in the optional-dependency detection path. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Missing AWS SDK produces a non-actionable error.
- ⚠️ AWS allocation setup lacks the documented install hint.
- ⚠️ `cloud doctor` cannot guide dependency installation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/providers/compute/awsEc2MacClient.ts
**Line:** 44:45
**Comment:**
*Api Mismatch: The dynamic import failure is wrapped in a plain `AwsComputeFailure` object before reaching `requireOptional`. `requireOptional` only checks `Error.message` or `String(cause)` for module-resolution text, so the tagged object stringifies as an object and the missing-package case will not produce the documented `INSTALL_HINT`. Let the original import error reach `requireOptional`, or preserve its message in the optional-dependency detection path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review by Qodo
1. awsEc2MacClient missing test file
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| import { Data, Effect } from 'effect'; | ||
| import { errorMessage } from '@core/services/errorMessage.js'; | ||
| import { requireOptional } from '@core/services/optionalDep.js'; | ||
| import type { AllocateRequest, AwsConfig } from '@core/types/remote.js'; |
There was a problem hiding this comment.
1. awsec2macclient missing test file 📘 Rule violation ▣ Testability
src/providers/compute/awsEc2MacClient.ts introduces new provider logic but there is no co-located src/providers/compute/awsEc2MacClient.test.ts. This violates the requirement that each new logic file has a same-directory *.test.ts, which reduces confidence in refactors and regression safety.
Agent Prompt
## Issue description
`src/providers/compute/awsEc2MacClient.ts` is a new logic-bearing module but it does not have a corresponding co-located test file `src/providers/compute/awsEc2MacClient.test.ts`.
## Issue Context
The compliance rule requires a co-located test file for each new logic file. While some of the functions are currently tested via `awsEc2Mac.test.ts`, the rule requires a dedicated sibling `*.test.ts` for the new module.
## Fix Focus Areas
- src/providers/compute/awsEc2MacClient.ts[1-81]
- src/providers/compute/awsEc2Mac.test.ts[1-20]
- src/providers/compute/awsEc2MacClient.test.ts[1-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } else { | ||
| imageId = goldenAmiId; | ||
| } | ||
| reportProgress('Launching the EC2 Mac instance...'); |
There was a problem hiding this comment.
2. Unprefixed reportprogress() messages 📘 Rule violation ◔ Observability
New reportProgress(...) status messages do not start with the required standardized prefixes (e.g., [RUN] ). This makes human-readable effect logging inconsistent and harder to grep/parse.
Agent Prompt
## Issue description
`reportProgress(...)` messages added/moved in the refactor are human-facing status logs but they do not use the required standardized prefixes ("[RUN] ", "[OK] ", "[WARN] ", "[ERROR] ", "[SKIP] ").
## Issue Context
The logging standard requires consistent prefixing for human-readable logs/status reporting.
## Fix Focus Areas
- src/providers/compute/awsEc2Mac.ts[277-290]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/providers/compute/awsEc2MacClient.ts">
<violation number="1" location="src/providers/compute/awsEc2MacClient.ts:45">
P2: Missing AWS SDKs surface a raw module-not-found error instead of the actionable `pnpm add ...` hint because the import rejection is wrapped as `AwsComputeFailure` before `requireOptional` can classify it. Keep the import rejection recognizable to `requireOptional`, then wrap only non-missing loader failures afterward; apply the same ordering to both loaders.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| requireOptional('AWS EC2 Mac builds', INSTALL_HINT, () => | ||
| Effect.tryPromise({ | ||
| try: () => import('@aws-sdk/client-ec2'), | ||
| catch: (cause) => awsFailure('load the EC2 SDK', cause), |
There was a problem hiding this comment.
P2: Missing AWS SDKs surface a raw module-not-found error instead of the actionable pnpm add ... hint because the import rejection is wrapped as AwsComputeFailure before requireOptional can classify it. Keep the import rejection recognizable to requireOptional, then wrap only non-missing loader failures afterward; apply the same ordering to both loaders.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/providers/compute/awsEc2MacClient.ts, line 45:
<comment>Missing AWS SDKs surface a raw module-not-found error instead of the actionable `pnpm add ...` hint because the import rejection is wrapped as `AwsComputeFailure` before `requireOptional` can classify it. Keep the import rejection recognizable to `requireOptional`, then wrap only non-missing loader failures afterward; apply the same ordering to both loaders.</comment>
<file context>
@@ -0,0 +1,81 @@
+ requireOptional('AWS EC2 Mac builds', INSTALL_HINT, () =>
+ Effect.tryPromise({
+ try: () => import('@aws-sdk/client-ec2'),
+ catch: (cause) => awsFailure('load the EC2 SDK', cause),
+ }),
+ );
</file context>
61046ab to
b9fbe2b
Compare
Deslop the EC2 Mac host without changing allocate/status/teardown/doctor semantics: extract shared SDK client/failure plumbing, pure helpers for AMI selection, quota detection, address/host state, and release errors, lift post-host provision into one catch-all path, and cover the helpers with colocated Vitest (no live AWS). Fixes #349
b9fbe2b to
574586e
Compare
1 participant
User description
Summary
Deslop
src/providers/compute/awsEc2Mac.ts(~764) for wave-2 feature #349 without changing allocate / status / teardown / doctor behavior.awsEc2MacClient.ts(failure channel, lazy load, client factory,requireAws)provisionAllocatedHostunder one release-on-failure catchrequireAws(no live AWS)Gate
All green.
Test plan
pnpm typecheckpnpm lint/pnpm lint:stylepnpm docs:checkpnpm test(includessrc/providers/compute/awsEc2Mac.test.ts)pnpm buildFixes #349
CodeAnt-AI Description
Refactor AWS EC2 Mac provisioning while preserving allocation behavior
What Changed
Impact
✅ Fewer billable hosts left after failed provisioning✅ Clearer AWS quota and release errors✅ Safer EC2 Mac status and AMI selection💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by cubic
Refactors the AWS EC2 Mac compute provider into a leaner, testable module with no behavior changes across allocate/status/teardown/doctor. Aligns with Linear #349 by extracting AWS client plumbing and pure helpers to improve maintainability and test coverage.
Refactors
awsEc2MacClient.tswith a typed failure channel, lazy loading for@aws-sdk/client-ec2and@aws-sdk/credential-providers, a client factory,requireAws, andsendAwsRequest.provisionAllocatedHost; exposed helpers for AMI catalog selection (datedMacImages/newestImageId), instance-type→arch mapping, quota detection, public address and host release state, release error details, and Xcode bootstrap checks; sharedDEFAULT_INSTANCE_TYPEand host-counting between doctor and provisioning.Tests
awsEc2Mac.test.tsfor helpers plusawsFailure/requireAws(no live AWS); docs badges updated, test count now 2081.Written for commit 574586e. Summary will update on new commits.